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

Spurious unused value warning with destructive substitution #7852

Open
vicuna opened this issue Sep 21, 2018 · 7 comments

Comments

Projects
None yet
5 participants
@vicuna
Copy link

commented Sep 21, 2018

Original bug ID: 7852
Reporter: @trefis
Assigned to: @trefis
Status: assigned (set by @trefis on 2018-09-21T16:46:49Z)
Resolution: open
Priority: normal
Severity: minor
Version: 4.08.0+dev/beta1/beta2
Category: typing
Monitored by: @nojb @hhugo @Drup

Bug description

This regression was introduced by #1737

I'll try to fix it without having to revert the GPR.
But if I fail, we can always revert it, it's not that important.

Steps to reproduce

$ cat test.mli
module M : sig
  type t
  val foo : t -> int
  val bar : t -> int
end

module N : sig
  type outer
  type t
  val foo : t -> outer
  val bar : t -> outer
end with type outer := int
$ ./ocamlc.opt -w +32 -c ./test.mli
File "./test.mli", line 10, characters 2-22:
Warning 32: unused value foo.
File "./test.mli", line 11, characters 2-22:
Warning 32: unused value bar.
$ $HOME/.opam/4.07.0/bin/ocamlc -w +32 -c ./test.mli
$
@vicuna

This comment has been minimized.

Copy link
Author

commented Sep 21, 2018

Comment author: @trefis

Btw, this would also happen when destructively substituting a module.

@vicuna

This comment has been minimized.

Copy link
Author

commented Jan 10, 2019

Comment author: @trefis

This was reverted in #2092

FTR, what was happening is basically this:

Usage of items in OCaml is tracked by their name (i.e. the string, not the ident) and a Location.
When first typing the "sig .. end" part of "sig .. end with ..." (that is done in Typemod), we enter the names and locations of foo and bar in some table in Env.
When we process the constraint (the "with ..." part) (also done in Typemod), we assign a new location to these items.

Once we're done with the typing (we're now in Compile_common.typecheck_intf), we call "ignore (Includemod.signatures info.env sg sg)". Which has the effect of marking every item in the signature as used (which makes sense for mlis).

The issue is that we're doing that by going over the signature, taking the name + location of every item, and then using that to look in the table in Env. But the location we have is the new one, not the initial one which was used to add the items to the table. So we never mark the items that were initially added.
And so we get the spurious warnings.

@vicuna

This comment has been minimized.

Copy link
Author

commented Jan 14, 2019

Comment author: @gasche

Thanks for the summary, I get it now. (Another motivation to have a proper "declaration path" notion instead of source locations.)

@hhugo

This comment has been minimized.

Copy link
Contributor

commented Mar 15, 2019

#1737 was reverted in #2092. Can be closed.

@stedolan stedolan closed this Mar 15, 2019

@trefis

This comment has been minimized.

Copy link
Contributor

commented Mar 15, 2019

Actually, I’d like to keep this open until “a proper fix” is implemented.

@trefis trefis reopened this Mar 15, 2019

@vicuna vicuna added the bug label Mar 20, 2019

@trefis

This comment has been minimized.

Copy link
Contributor

commented Mar 27, 2019

Another motivation to have a proper "declaration path" notion instead of source locations.

I wrote a prototype for that based on 4.07 last week (because at the time it's still wasn't easy to test 4.08 / trunk on actual code) which I plan to rebase onto trunk (and submit a PR) next week.

@gasche

This comment has been minimized.

Copy link
Member

commented Mar 27, 2019

Thanks! I vaguely thought of this again when seeing the error type of includemod.ml while reviewing #8546:

type pos =
    Module of Ident.t | Modtype of Ident.t | Arg of Ident.t | Body of Ident.t
type error = pos list * Env.t * symptom
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.