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

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

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.

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.

Comment author: @alainfrisch

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

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:

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)

)

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

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?

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.