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
A warning to detect more unused values and value declarations #5438
Comments
Comment author: @mmottl Any feature that helps developers find bugs more effectively is a good thing. I'd definitely like to see this one in a future release of OCaml. Thanks, Alain! |
Comment author: @gasche
Would the following code Is it possible/natural/easy to integrate an "redundant open warning" as proposed by Jun Furuse in #5357? The difficulty, I suppose, would be to know that "let open M in M.foo" is a redundant open, despite some identifier resolving to a value in M. |
Comment author: @yakobowski We (= the Frama-C team) are also very interested in this functionality. Given the way we (ab)use functors, the two patterns patterns below will certainly find some glitches: Unused fields in functors inputs:
Shadowing of values:
I will try the branch asap, and report on our findings. |
Comment author: @alainfrisch
No, of course, in your example "odd" is reachable from "even" which is used outside the recursive definition, so it is not reported as unused.
The patch proposed by Jun is orthogonal (we have something very similar at LexiFi as well, and I confirm it is very useful!). Note that we need to track all components, not just values, in order to detect useless opens. |
Comment author: @alainfrisch For those interested in inspecting the patch: |
Comment author: @alainfrisch See also another patch to detect unused open statements (cf #5357): http://caml.inria.fr/cgi-bin/viewvc.cgi?view=revision&revision=11937 |
Comment author: @alainfrisch And another one for detecting unused type declarations (no clever treatment of recursive definitions yet): http://caml.inria.fr/cgi-bin/viewvc.cgi?view=revision&revision=11940 |
Comment author: @yakobowski A first batch of feedback: this branch is great! We have already found quite a bit of cruft, and fixed at least one (minor) bug. That being said, we observe a reproductible crash at typing/env.ml:540 (rev. 11976). The crash occurs for example when compiling gPango.ml in lablgtk 2.14. I have tried to print the faulty/not found path, but it prints as the empty string. (my diff:
The messages
) |
Comment author: @alainfrisch Boris, thanks a lot for the feedback. The assertion is caused by the use of a dummy identifier (unbound_class) in Typeclass. The trivial fix (ignoring this case in mark_type_path) creates false positives. I'll see what to do. |
Comment author: @alainfrisch I've committed a fix. |
Comment author: @yakobowski Thanks, the fix worked great. We have finished cleaning up all of Frama-C, with great success. In particular, we found a subtle name-capture bug in the gui, that we had attributed to the Mac implementation of Gtk :-) There was only one warning I think is incorrect; it can be reproduced using bug.ml{,i}. To summarize our experience: the only trouble we had was with pretty-printers or debug code, but I think we can live with 'let _pretty' or 'let _ = ignore foo' for now. (A whitelist mechanism would however be great: perhaps special comments?) All in all, those warnings are extraordinarily useful, especially in a big code base that has changed a lot. We strongly support the inclusion of this branch in the compiler. |
Comment author: @ygrek
|
Comment author: @yakobowski The issue was essentially with "pretty-debuguers". Some of our types are not exported at all, so their pretty-printers might very well not be referenced anywhere. We use those pretty-printers only when we want to test a broken invariant that is related to the hidden type. |
Comment author: @alainfrisch Boris: thanks again for your feedback and support! I've committed a fix (rev 12016) for the bug you reported. I don't think we should introduce a new syntax to disable the warning on specific identifiers. What's wrong with "let () = ignore ..."? |
Comment author: @alainfrisch I've committed an extra warning (36) to detect unused constructors. A constructor is considered to be used when used in positive position (to create a value), not in patterns. |
Comment author: @alainfrisch Will be merged soon. |
Comment author: @alainfrisch The branch has been merged to the trunk. |
Original bug ID: 5438
Reporter: @alainfrisch
Assigned to: @alainfrisch
Status: closed (set by @xavierleroy on 2015-12-11T18:07:17Z)
Resolution: fixed
Priority: normal
Severity: minor
Fixed in version: 3.13.0+dev
Category: ~DO NOT USE (was: OCaml general)
Has duplicate: #4033 #4076
Related to: #5357
Parent of: #4960
Monitored by: meyer tgazagna @protz mehdi dario @ygrek @hcarty
Bug description
The current warnings 26, 27 detect some unused variables using a
syntactic traversal of the parsetree.
I propose a different implementation strategy, which avoids several
limitations of this approach and detects a lot more unused value declarations.
This is implemented in the following branch:
http://caml.inria.fr/cgi-bin/viewvc.cgi/ocaml/branches/unused_declarations
The idea is to track the use of value declarations during type
checking. Here, value declaration is taken in a broad sense; it
includes local let-bound and lambda-bound variables, for-loop indices, self
pattern for classes, but also toplevel let-definitions and value
declarations in signatures.
Detecting unused variables during type-checking has two main advantages:
We benefit from the typed resolution, thus avoiding to interpret
wrongly a reference to an identifier shadowed by an open
statement (or an object field, etc).
We benefit from a basic data-flow analysis. This allows us to detect
toplevel declaration which are not exported; or unused value components
in signatures.
Moreover, recursive definitions are treated carefully in order to
address #4960. In a recursive definition of multiple values, if a
subset of definitions are not used (except for internal references
within those definitions), they are reported as unused.
We have been using a similar warning at LexiFi for a few years (we
also detect other kinds of unused declarations: types, exceptions,
etc). It is very important for us to be able to detect dead code
(left behind during refactoring; introduced for debugging; etc)
in order to keep a clean code base. When the warning was first
introduced, it allowed to capture several subtle bugs.
Here are some examples (I indicate the warning location in comments),
detected by the new warning but not the older ones:
(* 1. Unused value components, considering the expected signature *)
module X : sig val x : int end =
struct
let x, y (* y UNUSED *)= 10, 20
end
module F(X : sig end) = struct end
module X = F(struct let x (* x UNUSED *)= 10 end)
module Foo : sig
val x : int
end =
struct
module type S = sig val x : int val y : int (* y UNUSED *) end
module F(X : S) = X
include F(struct let x = 2 let y = 3 end)
end
(* 2. Unused variables, with tricky scope *)
let f x (* x UNUSED ) =
let module X = struct let x = 10 let y ( y UNUSED *) = 20 end in
let open X in
x
let f x =
let x (* x UNUSED *) = x + 1 in
let o = object
val x = 10
method foo = x
end
in
o # foo
let x (* x UNUSED *) = "abc"
let x = 2
(* 3. Unused (subset of) a recursive definition *)
let () =
let rec f (* f UNUSED *)x = f x
in ()
let rec f x = g x
and g x = f x
and h (* h UNUSED ) x = f x + i x
and i ( i UNUSED *) x = h x
in
f 3
File attachments
The text was updated successfully, but these errors were encountered: