Skip to content
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

Open
vicuna opened this issue Jun 30, 2017 · 14 comments
Open

Further improvement to interface/implementation type mismatch errors #7574

vicuna opened this issue Jun 30, 2017 · 14 comments

Comments

@vicuna
Copy link

vicuna commented Jun 30, 2017

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.

  1. Allow to jump to the corresponding location in mli file aswell (though I'm not sure the cmi file has the info).
  2. Provide better hints at where the mismatch occurs, i.e. we want some kind of 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:

   Values do not match:
     val enum : ?dovc:'a -> 'b -> 'c
   is not included in
     val enum : ?docv:string -> (string * 'a) list -> 'a t

Related to #7338

@vicuna
Copy link
Author

vicuna commented Jun 30, 2017

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?

@vicuna
Copy link
Author

vicuna commented Jun 30, 2017

Comment author: @dbuenzli

@Frisch yes. So the question is why isn't -keep-locs the default behaviour ?

@vicuna
Copy link
Author

vicuna commented Jun 30, 2017

Comment author: @dbuenzli

I didn't fully reread #6311 where this was suggested at the end so I opened #7575 for my last suggestion.

@vicuna
Copy link
Author

vicuna commented Jun 30, 2017

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.

@vicuna
Copy link
Author

vicuna commented Jun 30, 2017

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.

@yallop
Copy link
Member

yallop commented Apr 11, 2019

Note: since #1219 (which is in 4.06.0), -keep-locs is the default.

@yawaramin
Copy link
Contributor

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 implementation foo.mlast
       does not match the interface foo.cmi:
       Values do not match:
         val foo : ...
       is not included in
         val foo : ...
       File "foo.mli", line 20, characters 1-363:
         Expected declaration
       File "foo.ml", line 11, characters 5-9:
         Actual declaration

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:

The interface foo.cmi
       does not match the implementation foo.mlast:
       Values do not match:
         val foo : ...
       does not include
         val foo : ...
       File "foo.mli", line 20, characters 1-363:
         Expected declaration
       File "foo.ml", line 11, characters 5-9:
         Actual declaration

@github-actions
Copy link

github-actions bot commented May 7, 2020

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.

@github-actions github-actions bot added the Stale label May 7, 2020
@lpw25
Copy link
Contributor

lpw25 commented May 7, 2020

@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.

@github-actions
Copy link

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.

@github-actions github-actions bot added the Stale label May 12, 2021
@Octachron
Copy link
Member

There is now a PR under review, #10407, that extends the module level error messages .

@github-actions github-actions bot removed the Stale label May 14, 2021
@github-actions
Copy link

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.

@github-actions github-actions bot added the Stale label May 16, 2022
@Octachron
Copy link
Member

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 docv is still hard to spot.

@github-actions github-actions bot removed the Stale label May 18, 2022
@github-actions
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants