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
Further improvement to interface/implementation type mismatch errors #7574
Comments
Comment author: @alainfrisch 1 is available if the .mli has been compiled with -keep-locs. Example: $ cat foo.mli val x: string $ cat foo.ml let x = 42 $ ocamlc -c foo.mli foo.ml File "foo.ml", line 1: Error: The implementation foo.ml does not match the interface foo.cmi: Values do not match: val x : int is not included in val x : string File "foo.ml", line 1, characters 4-5: Actual declaration $ ocamlc -c -keep-locs foo.mli foo.ml File "foo.ml", line 1: Error: The implementation foo.ml does not match the interface foo.cmi: Values do not match: val x : int is not included in val x : string File "foo.mli", line 1, characters 0-13: Expected declaration File "foo.ml", line 1, characters 4-5: Actual declaration Does this correspond to what you have in mind? |
Comment author: @alainfrisch Because some people are concerned that including locations in the .cmi files will force more recompilation with incremental build systems (in particular, adding whitespace to an .mli will force recompiling all code that depend on this interface). I find this argument rather weak. Changes in documentation comments (when they are parsed as attributes) would already force such recompilation. Another argument is that file paths stored in .cmi might not be meaningful (the compilation of the .cmi has occurred in a different environment). In addition to better error messages, -keep-locs makes it easy for external tools to "understand" where a value reference in a compilation unit comes from; we use that for jump-to-definition and global dead code detection, for instance. I'm in favor of making -keep-locs the default but won't spend too much time fighting for it. |
Comment author: @gasche For the record, I seem to remember that I was opposed to have -keep-locs by default, but the reason was that you (Alain) were arguing for it to use locations as a path to value declaration in interface files (for the purpose of warnings/analyses), rather than for the locations themselves. It seemed wrong to me to impose the cost (of recompilation) for something that could be implemented in a different way (by designing a grammar of paths from interfaces to name declarations). If the argument is to have good error locations by default, I find it more convincing and would support having -keep-locs by default. |
Note: since #1219 (which is in 4.06.0), |
I don't know if there has been any discussion on possible improvements to this message–but I do want to point out how I found it confusing and unintuitive. The message looks something like this:
The mlast and cmi files are listed in one order, the implementation and interface types are in the same order, but the interface and implementation file names are in the opposite order. It's difficult to figure out which is which unless you know the orderings of both, and that they're different. It would be a lot more intuitive if all of these were in the same order. My preference would be interface first. So for example it could look like:
|
This issue has been open one year with no activity. Consequently, it is being marked with the "stale" label. What this means is that the issue will be automatically closed in 30 days unless more comments are added or the "stale" label is removed. Comments that provide new information on the issue are especially welcome: is it still reproducible? did it appear in other contexts? how critical is it? etc. |
@yawaramin Could you please report that problem as a separate issue? It does seem worth fixing. The remaining request in this issue is for "diffs" within value inclusion errors. I have a patch lying around that implements this and intend it to fix it up ready for 4.12. |
This issue has been open one year with no activity. Consequently, it is being marked with the "stale" label. What this means is that the issue will be automatically closed in 30 days unless more comments are added or the "stale" label is removed. Comments that provide new information on the issue are especially welcome: is it still reproducible? did it appear in other contexts? how critical is it? etc. |
There is now a PR under review, #10407, that extends the module level error messages . |
This issue has been open one year with no activity. Consequently, it is being marked with the "stale" label. What this means is that the issue will be automatically closed in 30 days unless more comments are added or the "stale" label is removed. Comments that provide new information on the issue are especially welcome: is it still reproducible? did it appear in other contexts? how critical is it? etc. |
Currently, the error message has been extended with Values do not match:
val enum : ?dovc:'a -> 'b -> 'c
is not included in
val enum : ?docv:string -> (string * 'a) list -> 'a t
The type ?dovc:'a -> 'b -> 'c is not compatible with the type
?docv:string -> (string * 'd) list -> 'd t where the typo on |
This issue has been open one year with no activity. Consequently, it is being marked with the "stale" label. What this means is that the issue will be automatically closed in 30 days unless more comments are added or the "stale" label is removed. Comments that provide new information on the issue are especially welcome: is it still reproducible? did it appear in other contexts? how critical is it? etc. |
Original bug ID: 7574
Reporter: @dbuenzli
Status: confirmed (set by @Octachron on 2017-07-11T19:42:39Z)
Resolution: open
Priority: normal
Severity: feature
Category: typing
Child of: #7338
Monitored by: @dbuenzli
Bug description
While #6311 did already bring some improvements. Implementation/Interface mismatch error could still be greatly improved.
diff
function on types. This is useful both for big and small types.Here's a very simple example that may seem obvious out of context but I had to stare it intensely to catch the typo:
Related to #7338
The text was updated successfully, but these errors were encountered: