|Anonymous | Login | Signup for a new account||2015-07-30 08:11 CEST|
|Main | My View | View Issues | Change Log | Roadmap|
|View Issue Details|
|ID||Project||Category||View Status||Date Submitted||Last Update|
|0006311||OCaml||OCaml typing||public||2014-01-29 21:27||2015-04-21 18:06|
|Target Version||Fixed in Version||4.02.0+dev|
|Summary||0006311: Improve signature mismatch error messages|
|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
val bla : bla
end = struct
let bla = ()
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
2) 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.
|Tags||No tags attached.|
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
|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.|
|Commit 14428 adds location for module and module type items.|
|Wow, that was quick. Thanks Alain.|
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:
module Private :
sig module Store : sig module type S = <here> end end
The value `writer_close' is required but not provided
Command exited with code 2.
> 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.
> 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.
|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.|
edited on: 2015-03-31 11:07
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.
> 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.
> 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.
> 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).
|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.|
|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.|
> 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.
|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.|
|2014-01-29 21:27||dbuenzli||New Issue|
|2014-01-29 21:31||dbuenzli||Note Added: 0010862|
|2014-01-30 10:15||frisch||Assigned To||=> frisch|
|2014-01-30 10:15||frisch||Status||new => assigned|
|2014-01-30 10:56||frisch||Note Added: 0010863|
|2014-01-30 13:18||frisch||Note Added: 0010864|
|2014-01-30 13:21||frisch||Status||assigned => resolved|
|2014-01-30 13:21||frisch||Fixed in Version||=> 4.02.0+dev|
|2014-01-30 13:21||frisch||Resolution||open => fixed|
|2014-01-30 13:33||dbuenzli||Note Added: 0010866|
|2015-03-25 00:30||dbuenzli||Note Added: 0013537|
|2015-03-25 00:30||dbuenzli||Note Added: 0013538|
|2015-03-25 00:30||dbuenzli||Status||resolved => feedback|
|2015-03-25 00:30||dbuenzli||Resolution||fixed => reopened|
|2015-03-31 09:52||frisch||Note Added: 0013594|
|2015-03-31 10:06||dbuenzli||Note Added: 0013595|
|2015-03-31 10:06||dbuenzli||Status||feedback => assigned|
|2015-03-31 10:45||dbuenzli||Note Added: 0013597|
|2015-03-31 11:07||frisch||Note Added: 0013598|
|2015-03-31 11:07||frisch||Note Edited: 0013598||View Revisions|
|2015-03-31 11:21||dbuenzli||Note Added: 0013599|
|2015-03-31 16:24||frisch||Note Added: 0013601|
|2015-03-31 17:04||dbuenzli||Note Added: 0013603|
|2015-03-31 17:16||dbuenzli||Note Added: 0013604|
|2015-03-31 17:36||frisch||Note Added: 0013607|
|2015-03-31 18:06||lpw25||Note Added: 0013610|
|2015-04-21 18:06||frisch||Note Added: 0013698|
|2015-04-21 18:06||frisch||Status||assigned => resolved|
|2015-04-21 18:06||frisch||Resolution||reopened => fixed|
|Copyright © 2000 - 2011 MantisBT Group|