Mantis Bug Tracker

View Issue Details Jump to Notes ] Issue History ] Print ]
IDProjectCategoryView StatusDate SubmittedLast Update
0006311OCamlOCaml typingpublic2014-01-29 21:272015-03-31 18:06
Reporterdbuenzli 
Assigned Tofrisch 
PrioritynormalSeveritytweakReproducibilityalways
StatusassignedResolutionreopened 
PlatformOSOS Version
Product Version4.01.0 
Target VersionFixed in Version4.02.0+dev 
Summary0006311: Improve signature mismatch error messages
Description1) 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

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.

Thanks,

Daniel


TagsNo tags attached.
Attached Files

- Relationships

-  Notes
(0010862)
dbuenzli (reporter)
2014-01-29 21:31

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
(0010863)
frisch (developer)
2014-01-30 10:56

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.
(0010864)
frisch (developer)
2014-01-30 13:18

Commit 14428 adds location for module and module type items.
(0010866)
dbuenzli (reporter)
2014-01-30 13:33

Wow, that was quick. Thanks Alain.
(0013537)
dbuenzli (reporter)
2015-03-25 00:30

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 = <here> end end
       The value `writer_close' is required but not provided
Command exited with code 2.
(0013538)
dbuenzli (reporter)
2015-03-25 00:30

(On 4.02.1)
(0013594)
frisch (developer)
2015-03-31 09:52

> 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.
(0013595)
dbuenzli (reporter)
2015-03-31 10:06

> 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.
(0013597)
dbuenzli (reporter)
2015-03-31 10:45

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.
(0013598)
frisch (developer)
2015-03-31 11:07
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.

(0013599)
dbuenzli (reporter)
2015-03-31 11:21

> 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.
(0013601)
frisch (developer)
2015-03-31 16:24

> 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.
(0013603)
dbuenzli (reporter)
2015-03-31 17:04

> 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).
(0013604)
dbuenzli (reporter)
2015-03-31 17:16

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.
(0013607)
frisch (developer)
2015-03-31 17:36

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.
(0013610)
lpw25 (developer)
2015-03-31 18:06

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

- Issue History
Date Modified Username Field Change
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


Copyright © 2000 - 2011 MantisBT Group
Powered by Mantis Bugtracker