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

Closed
vicuna opened this issue Sep 21, 2018 · 11 comments
Closed

Spurious unused value warning with destructive substitution #7852

vicuna opened this issue Sep 21, 2018 · 11 comments
Assignees

Comments

@vicuna
Copy link

vicuna 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
Copy link
Author

vicuna commented Sep 21, 2018

Comment author: @trefis

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

@vicuna
Copy link
Author

vicuna 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
Copy link
Author

vicuna 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
Copy link
Contributor

hhugo commented Mar 15, 2019

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

@trefis
Copy link
Contributor

trefis 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
Copy link
Contributor

trefis 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
Copy link
Member

gasche 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

@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

@trefis Has this been fixed now?

@trefis
Copy link
Contributor

trefis commented May 7, 2020

The issue has been fixed by reverting #1737, and a couple of weeks ago the "proper fix" I was talking about got merged (#8934).
I think I only need to resubmit #1737 for inclusion before we can finally close this issue.

I'll try to do that in the not too distant future.

@trefis
Copy link
Contributor

trefis commented May 7, 2020

Actually, looks like #7859 is already tracking #1737 reinclusion, so let's close this.

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