New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve signature mismatch error messages #6311
Comments
Comment author: @dbuenzli Just to add, in fact when your module is large the signature is eluded in the error message, so here 2) would be really nice: File "src/lit.ml", line 1: |
Comment author: @alainfrisch Commit 14427 on trunk adds the requested information (kind + location of missing items) to the error message. The location is currently not available for module and module type items. |
Comment author: @alainfrisch Commit 14428 adds location for module and module type items. |
Comment author: @dbuenzli Wow, that was quick. Thanks Alain. |
Comment author: @dbuenzli I still get unprecise errors reports here's an example: File "src/reach.ml", line 1: |
Comment author: @dbuenzli (On 4.02.1) |
Comment author: @alainfrisch
Can you upload a reproduction case? To get concrete locations in the .mli file when compiling the .ml file, you need to use the -keep-locs option. |
Comment author: @dbuenzli
Ok I don't do that, is that something new with 4.02.1 ? It seems a rather bad idea that by default the toolchain reports bad error messages. |
Comment author: @dbuenzli Ok so that's indeeed the problem. So basically now in all my projects I will have to add a |
Comment author: @alainfrisch I don't remember exactly when -keep-locs was introduced, but before it was, locations were always dropped from .cmi files, and the idea was to remain conservative. Some people are concerned that .cmi files change even when only whitespace/comment is touched in the .mli file, which could trigger a lot of recompilation (and also some interface mismatch since the signature digest is based on the entire file content). FWIW, -keep-locs makes it straightforward to implement jump-to-definition (since .cmt files contain location of the definitions for referenced values) and dead code detectors (as https://github.com/alainfrisch/dead_code_analyzer ). I also believe it should be the default, but I'm not the one to be convinced. |
Comment author: @dbuenzli
As I said in the other issue I don't think that location/comments should be part of the hash used to determine if a module's interface changed. We are hashing the wrong thing here.
I don't know the details of all this but I already add Such a scheme could solve the hash problem mentioned above: you would have cmi files for compilers tasks and cmti files for human tasks. |
Comment author: @alainfrisch
This could lead to weird results. For instance, with: a.mli: module type S = sig val x : int end and the following steps:
then the error message will show the location (in a.mli) of the value declaration as stored in b.cmi, which is now incoherent with the source code even though a.cmi is up to date. Build systems will of course also recompile b.cmi since they record a dependency on a.cmi, not just on its "signature hash" but it is better if the toolchain reports a proper interface mismatch in that case.
That would be very complex and add a lot of code just to support the invariant that the signature hash stored in .cmi files should not depend on attributes and concrete locations, which doesn't bring much in practice. |
Comment author: @dbuenzli
Well it seems to me that is solves a lot of problems in practice. It makes a clear distinction between what the compiler needs for guaranteeing interface compatibility and solves the discussions about which information should be retained an where with a definitive criterion (i.e. cmi files never have locations and/or comments). |
Comment author: @dbuenzli Frankly from a user perspective the menagerie of |
Comment author: @alainfrisch I tend to agree, but I'd rather merge .cmi and .cmti files (i.e. store everything in the .cmi file), and always keep locations. It would be important to evaluate how bigger .cmi files impact compilation time, though. I still don't see what we actually gain in practice by including only the strict minimum in the signature hash. |
Comment author: @lpw25
You clearly gain the ability to have fewer re-compilations in certain circumstances, and for some use cases this is probably important. So it is probably important to have an option to include the minimum information in .cmi files. But I agree that it is not clear that the minimum information is the right default. |
Comment author: @alainfrisch The discussion has drifted away from the ticket description. If people want to propose -keep-locs as the new default, please do it in a separate ticket. |
Original bug ID: 6311
Reporter: @dbuenzli
Assigned to: @alainfrisch
Status: closed (set by @xavierleroy on 2016-12-07T10:48:53Z)
Resolution: fixed
Priority: normal
Severity: tweak
Version: 4.01.0
Fixed in version: 4.02.0+dev
Category: typing
Monitored by: @dbuenzli
Bug description
Here's an example, of course it's obvious here since the sig is small but sometimes one forgets that there are two fields with the same name of different kind and you wonder for a little while about the error message as you actually contemplate the other field in your implementation.
module M : sig
type bla
val bla : bla
end = struct
let bla = ()
end
File "test.ml", line 5, characters 6-31:
Error: Signature mismatch:
Modules do not match:
sig val bla : unit end
is not included in
sig type bla val bla : bla end
The field `bla' is required but not provided
Thanks,
Daniel
The text was updated successfully, but these errors were encountered: