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

"Unused stuff" warnings broken in presence of multiple values with the same name #6446

Closed
vicuna opened this issue Jun 2, 2014 · 7 comments

Comments

Projects
None yet
2 participants
@vicuna
Copy link

commented Jun 2, 2014

Original bug ID: 6446
Reporter: @alainfrisch
Assigned to: @alainfrisch
Status: closed (set by @xavierleroy on 2015-12-11T18:27:49Z)
Resolution: fixed
Priority: normal
Severity: minor
Target version: 4.02.0+beta1 / +rc1
Fixed in version: 4.02.0+dev
Category: typing
Monitored by: @yakobowski

Bug description

Commit 14896 introduces an intermediate coercion when a structure defines several values with the same name. This interacts badly with how the "unused stuff" warnings are implemented, because this coercion marks components as being used (they are "required" by the coercion).

Example:

module X : sig
val y : int
end = struct
let x = 1
let y = 2
let y = y
end

@vicuna

This comment has been minimized.

Copy link
Author

commented Jun 2, 2014

Comment author: @alainfrisch

A pseudo-fix could be to check that location are different between source and target declarations before marking the source as being used. E.g. for Includemod.value_descriptions:

let value_descriptions env cxt subst id vd1 vd2 =
Cmt_format.record_value_dependency vd1 vd2;
if vd1.val_loc <> vd2.val_loc then
Env.mark_value_used (Ident.name id) vd1;
...

but this breaks the warnings for mli files (where we rely on the fact that the signature is matched against itself in compile.ml).

@vicuna

This comment has been minimized.

Copy link
Author

commented Jun 2, 2014

Comment author: @mshinwell

(Commit 14896 is the fix for PR 6435.)

@vicuna

This comment has been minimized.

Copy link
Author

commented Jun 2, 2014

Comment author: @alainfrisch

Fixed by commit 14936 on 4.02.

@vicuna

This comment has been minimized.

Copy link
Author

commented Jun 2, 2014

Comment author: @garrigue

I'm afraid I don't understand the discussion here.
Introducing any coercion is should mark omitted components as being unused, not used.
Why is there any need to distinguish between implicit and explicit coercions?
Or is your tracking going down to the lambda code?

@vicuna

This comment has been minimized.

Copy link
Author

commented Jun 2, 2014

Comment author: @alainfrisch

The warnings are implemented by collecting references to values. When a signature S1 is matched against S2, this results in a value declaration X1 matched against the expected declaration X2, and the effect of it is to mark X1 as being used (it is required to make the inclusion check succeed). Consider for example:

module type S = sig val x : int end
module F(X : S) = struct ... end
module A = struct let x = 1 let y = 2 end
module B = F(A)

The signature inferred for A contains two value declarations (with location derived from the implementation). The inclusion check induced by the functor application checks that this signature is a subtype of S, and this results in marking "let x = 1" in A, but not "let y = 2" (which is not checked against any other declaration).

Basically, we assume that inclusion check corresponds to some kind of "static data flow".

Now, if we add an inclusion check between the signature inferred for A and itself, this will result in the declaration for "let y = 2" (val y: int) be matched against itself, and it will thus be marked as being used.

Does it make sense?

@vicuna

This comment has been minimized.

Copy link
Author

commented Jun 2, 2014

Comment author: @garrigue

I see. This makes sense, but this is only an approximation.
If you have nested coercions, only the outermost one should do that. In practice, the signature of the compilation unit, or of a functor parameter (since we cannot know how it is used inside). But this may be harder to implement.

@vicuna

This comment has been minimized.

Copy link
Author

commented Jun 2, 2014

Comment author: @alainfrisch

The reasoning is that if a value is required to make an inclusion check succeed, it cannot be considered as unused. So even with nested coercions, this seems ok to me. But clearly, we don't do a full fledged data flow analysis, only a local approximation of it.

@vicuna vicuna closed this Dec 11, 2015

@vicuna vicuna added the typing label Mar 14, 2019

@vicuna vicuna added this to the 4.02.0 milestone Mar 14, 2019

@vicuna vicuna added the bug label Mar 20, 2019

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.