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

A warning to detect more unused values and value declarations #5438

Closed
vicuna opened this issue Dec 21, 2011 · 17 comments

Comments

Projects
None yet
2 participants
@vicuna
Copy link

commented Dec 21, 2011

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:

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

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

@vicuna

This comment has been minimized.

Copy link
Author

commented Dec 21, 2011

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!

@vicuna

This comment has been minimized.

Copy link
Author

commented Dec 21, 2011

Comment author: @gasche

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.

Would the following code
let rec even = function
| 0 -> true
| n -> odd (n - 1)
and odd = function
| 0 -> false
| n -> even (n - 1)
in even n
raise an "unused function 'odd'" warning?

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

@vicuna

This comment has been minimized.

Copy link
Author

commented Dec 21, 2011

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:

module F(X : sig end) = struct end
module X = F(struct let x (* x UNUSED *)= 10 end)

Shadowing of values:

let x (* x UNUSED *) = "abc"
let x = 2
(In our codebase, the shadowing usually comes from an "include F(M)")

I will try the branch asap, and report on our findings.

@vicuna

This comment has been minimized.

Copy link
Author

commented Dec 21, 2011

Comment author: @alainfrisch

Would the following code raise an "unused function 'odd'" warning?

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.

Is it possible/natural/easy to integrate an "redundant open warning"

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.

@vicuna

This comment has been minimized.

Copy link
Author

commented Dec 21, 2011

Comment author: @alainfrisch

For those interested in inspecting the patch:
http://caml.inria.fr/cgi-bin/viewvc.cgi?view=revision&revision=11924

@vicuna

This comment has been minimized.

Copy link
Author

commented Dec 22, 2011

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

@vicuna

This comment has been minimized.

Copy link
Author

commented Dec 22, 2011

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

@vicuna

This comment has been minimized.

Copy link
Author

commented Jan 6, 2012

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:
Index: typing/env.ml

--- typing/env.ml (révision 11994)
+++ typing/env.ml (copie de travail)
@@ -537,7 +537,12 @@
r

let mark_type_path env path =

  • let decl = try find_type path env with Not_found -> assert false in
  • let decl = try find_type path env
  • with Not_found ->
  • let name = Path.name path in
  • Format.eprintf "PATH %s@." name;
  • assert false
  • in
    mark_type_used (Path.name path) decl

let mark_type_constr env = function

The messages

ocamlc.opt -w s -c gPango.ml
PATH
Fatal error: exception Assert_failure("typing/env.ml", 544, 4)

)

@vicuna

This comment has been minimized.

Copy link
Author

commented Jan 6, 2012

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.

@vicuna

This comment has been minimized.

Copy link
Author

commented Jan 6, 2012

Comment author: @alainfrisch

I've committed a fix.

@vicuna

This comment has been minimized.

Copy link
Author

commented Jan 12, 2012

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.

@vicuna

This comment has been minimized.

Copy link
Author

commented Jan 12, 2012

Comment author: @ygrek

the only trouble we had was with pretty-printers or debug code
Please, can you elaborate? Why a pretty-printer will cause a trouble with this patch?

@vicuna

This comment has been minimized.

Copy link
Author

commented Jan 12, 2012

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.

@vicuna

This comment has been minimized.

Copy link
Author

commented Jan 12, 2012

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

@vicuna

This comment has been minimized.

Copy link
Author

commented Jan 12, 2012

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.

@vicuna

This comment has been minimized.

Copy link
Author

commented Jan 17, 2012

Comment author: @alainfrisch

Will be merged soon.

@vicuna

This comment has been minimized.

Copy link
Author

commented Jan 18, 2012

Comment author: @alainfrisch

The branch has been merged to the trunk.

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.