Mantis Bug Tracker
Main | My View | View Issues | Change Log | Roadmap
  

View Issue Details [ Jump to Notes ] [ Issue History ] [ Print ]
IDProjectCategoryView StatusDate SubmittedLast Update
0005438OCamlOCaml generalpublic2011-12-21 17:222012-07-11 23:06
Reporterfrisch 
Assigned Tofrisch 
PrioritynormalSeverityminorReproducibilityhave not tried
StatusresolvedResolutionfixed 
PlatformOSOS Version
Product Version 
Target VersionFixed in Version3.13.0+dev 
Summary0005438: A warning to detect more unused values and value declarations
DescriptionThe 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 0004960. 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
TagsNo tags attached.
Attached Files? file icon bug.ml [^] (122 bytes) 2012-01-12 08:57
? file icon bug.mli [^] (118 bytes) 2012-01-12 08:58

- Relationships
related to 0005357resolvedfrisch Warning for redundant open M and let open M in 
parent of 0004960closedfrisch Warning for unused variables fails with recursive functions 
has duplicate 0004033closedfrisch Using "unused values" warnings also for vlaues at the top-level of a module/compilation unit 
has duplicate 0004076closedfrisch Amélioration du warning des variables non utilisées 

-  Notes
(0006468)
mottl (reporter)
2011-12-21 17:28

 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!
(0006473)
gasche (developer)
2011-12-21 21:09
edited on: 2011-12-21 21:09

 > 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 0005357? 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.
http://caml.inria.fr/mantis/view.php?id=5357 [^]
(0006475)
Boris Yakobowski (reporter)
2011-12-21 22:29

 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.
(0006478)
frisch (developer)
2011-12-21 23:20

 > 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.
(0006479)
frisch (developer)
2011-12-21 23:39

 For those interested in inspecting the patch:
http://caml.inria.fr/cgi-bin/viewvc.cgi?view=revision&revision=11924 [^]
(0006502)
frisch (developer)
2011-12-22 12:42

 See also another patch to detect unused open statements (cf 0005357):

http://caml.inria.fr/cgi-bin/viewvc.cgi?view=revision&revision=11937 [^]
(0006506)
frisch (developer)
2011-12-22 16:54

 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 [^]
(0006607)
Boris Yakobowski (reporter)
2012-01-06 13:23

 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)

)
(0006608)
frisch (developer)
2012-01-06 14:53

 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.
(0006613)
frisch (developer)
2012-01-06 15:47

 I've committed a fix.
(0006655)
Boris Yakobowski (reporter)
2012-01-12 09:10

 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.
(0006656)
ygrek (reporter)
2012-01-12 09:20

 > 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?
(0006657)
Boris Yakobowski (reporter)
2012-01-12 09:43

 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.
(0006659)
frisch (developer)
2012-01-12 10:20

 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 ..."?
(0006663)
frisch (developer)
2012-01-12 13:07

 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.
(0006699)
frisch (developer)
2012-01-17 17:06

 Will be merged soon.
(0006717)
frisch (developer)
2012-01-18 10:35

 The branch has been merged to the trunk.

- Issue History
Date Modified Username Field Change
2011-12-21 17:22 frisch New Issue
2011-12-21 17:22 frisch Relationship added related to 0004960
2011-12-21 17:23 frisch Relationship added has duplicate 0004033
2011-12-21 17:24 frisch Relationship added has duplicate 0004076
2011-12-21 17:25 frisch Relationship replaced parent of 0004960
2011-12-21 17:28 mottl Note Added: 0006468
2011-12-21 18:55 frisch Summary A warning to detecting more unused values and value declarations => A warning to detect more unused values and value declarations
2011-12-21 21:09 gasche Note Added: 0006473
2011-12-21 21:09 gasche Note Edited: 0006473 View Revisions
2011-12-21 21:09 gasche Note Edited: 0006473 View Revisions
2011-12-21 22:29 Boris Yakobowski Note Added: 0006475
2011-12-21 23:20 frisch Note Added: 0006478
2011-12-21 23:39 frisch Note Added: 0006479
2011-12-22 12:42 frisch Note Added: 0006502
2011-12-22 16:54 frisch Note Added: 0006506
2012-01-06 13:23 Boris Yakobowski Note Added: 0006607
2012-01-06 14:53 frisch Note Added: 0006608
2012-01-06 15:47 frisch Note Added: 0006613
2012-01-12 08:57 Boris Yakobowski File Added: bug.ml
2012-01-12 08:58 Boris Yakobowski File Added: bug.mli
2012-01-12 09:10 Boris Yakobowski Note Added: 0006655
2012-01-12 09:20 ygrek Note Added: 0006656
2012-01-12 09:43 Boris Yakobowski Note Added: 0006657
2012-01-12 10:20 frisch Note Added: 0006659
2012-01-12 13:07 frisch Note Added: 0006663
2012-01-17 17:06 frisch Assigned To => frisch
2012-01-17 17:06 frisch Status new => assigned
2012-01-17 17:06 frisch Note Added: 0006699
2012-01-17 23:27 lefessan Relationship added related to 0005357
2012-01-18 10:35 frisch Note Added: 0006717
2012-01-18 10:35 frisch Status assigned => resolved
2012-01-18 10:35 frisch Fixed in Version => 3.13.0+dev
2012-01-18 10:35 frisch Resolution open => fixed

Copyright © 2000 - 2011 MantisBT Group
Powered by Mantis Bugtracker