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

Improve signature mismatch error messages #6311

Closed
vicuna opened this Issue Jan 29, 2014 · 17 comments

Comments

Projects
None yet
2 participants
@vicuna
Copy link
Collaborator

commented Jan 29, 2014

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

  1. It would be nice for the error message to indicate which kind of field is missing (type, value, module) in case of signature mismatch.

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

  1. One other thing that would be nice is to output further error messages for each missing field that points to the definition in the signature so that we can quickly jump to the def of what is missing.

Thanks,

Daniel

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 29, 2014

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:
Error: The implementation src/lit.ml
does not match the interface src/lit.cmi:
...
In module Prog:
The field `source' is required but not provided

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 30, 2014

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.

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 30, 2014

Comment author: @alainfrisch

Commit 14428 adds location for module and module type items.

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 30, 2014

Comment author: @dbuenzli

Wow, that was quick. Thanks Alain.

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 24, 2015

Comment author: @dbuenzli

I still get unprecise errors reports here's an example:

File "src/reach.ml", line 1:
Error: The implementation src/reach.ml
does not match the interface src/reach.cmi:
...
At position
module Private :
sig module Store : sig module type S = end end
The value `writer_close' is required but not provided
Command exited with code 2.

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 24, 2015

Comment author: @dbuenzli

(On 4.02.1)

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 31, 2015

Comment author: @alainfrisch

I still get unprecise errors reports here's an example:

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.

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 31, 2015

Comment author: @dbuenzli

To get concrete locations in the .mli file when compiling the .ml file, you need to use the -keep-locs option.

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.

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 31, 2015

Comment author: @dbuenzli

Ok so that's indeeed the problem. So basically now in all my projects I will have to add a true : keep_locs in my ocamlbuild _tags files. That feels completely retarded, it's the wrong default, which hurts both beginner and experienced developers.

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 31, 2015

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.

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 31, 2015

Comment author: @dbuenzli

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

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.

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 don't know the details of all this but I already add true : bin_annot for generating documentation and navigation. Couldn't the compiler look for cmti files for reporting errors if there's no keep-locs in the cmi files ?

Such a scheme could solve the hash problem mentioned above: you would have cmi files for compilers tasks and cmti files for human tasks.

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 31, 2015

Comment author: @alainfrisch

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.

This could lead to weird results. For instance, with:

a.mli: module type S = sig val x : int end
b.mli: module type S = A.S
c.ml: module X : B.S = struct let x = "" end

and the following steps:

  • compile a.mli
  • compile b.mli
  • modify whitespace in a.mli
  • recompile a.mli, but not b.mli
  • compile c.ml

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.

Couldn't the compiler look for cmti files for reporting errors if there's no keep-locs in the cmi files ?

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.

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 31, 2015

Comment author: @dbuenzli

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.

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

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 31, 2015

Comment author: @dbuenzli

Frankly from a user perspective the menagerie of cmi, cmti and cmt files and the various flags to create them and/or add more information to them looks more and more like a pile of mess rather than a coherently designed system.

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 31, 2015

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.

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 31, 2015

Comment author: @lpw25

I still don't see what we actually gain in practice by including only the strict minimum in the signature hash.

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.

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 21, 2015

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.