Add dead-code warning pass

[ktt3ja]

PR for issue #1749 mainly to get some feedback and suggestion. This adds a pass that warns if a function, struct, enum, or static item is never used. For the following code,

pubstatic pub_static:int = 0;static priv_static:int = 0;static used_static:int = 0;pubfnpub_fn(){used_fn();}fnpriv_fn(){let unused_struct = PrivStruct;}fnused_fn(){}pubstructPubStruct();structPrivStruct();structUsedStruct1{x:int}structUsedStruct2(int);structUsedStruct3();pubenumpub_enum{ foo1, bar1 }enumpriv_enum{ foo2, bar2 }enumused_enum{ foo3, bar3 }fnfoo(){bar();let unused_enum = foo2;}fnbar(){foo();}fnmain(){let used_struct1 = UsedStruct1{x:1};let used_struct2 = UsedStruct2(1);let used_struct3 = UsedStruct3;let t = used_static;let e = foo3;}

it would add the following warnings:

/home/ktt3ja/test.rs:2:0:2:28 warning: code is never used: `priv_static`,#[warn(dead_code)] on by default
/home/ktt3ja/test.rs:2static priv_static:int = 0;
^~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/ktt3ja/test.rs:6:0:6:48 warning: code is never used: `priv_fn`,#[warn(dead_code)] on by default
/home/ktt3ja/test.rs:6fnpriv_fn(){let unused_struct = PrivStruct;}
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/ktt3ja/test.rs:10:0:10:20 warning: code is never used: `PrivStruct`,#[warn(dead_code)] on by default
/home/ktt3ja/test.rs:10structPrivStruct();
^~~~~~~~~~~~~~~~~~~~
/home/ktt3ja/test.rs:16:0:16:29 warning: code is never used: `priv_enum`,#[warn(dead_code)] on by default
/home/ktt3ja/test.rs:16enumpriv_enum{ foo2, bar2 }
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/ktt3ja/test.rs:19:0:22:1 warning: code is never used: `foo`,#[warn(dead_code)] on by default
/home/ktt3ja/test.rs:19fn foo(){
/home/ktt3ja/test.rs:20bar();
/home/ktt3ja/test.rs:21let unused_enum = foo2;
/home/ktt3ja/test.rs:22}
/home/ktt3ja/test.rs:24:0:26:1 warning: code is never used: `bar`,#[warn(dead_code)] on by default
/home/ktt3ja/test.rs:24fnbar(){
/home/ktt3ja/test.rs:25foo();
/home/ktt3ja/test.rs:26}

Furthermore, I would like to solicit some test cases since I haven't tested extensively and I'm still unclear about some of the things in here. For example, I'm not sure how reexports would affect this and just assumed that LiveContext (which is a copy of reachable::ReachableContext) does enough work to handle it. Also, the test case above doesn't include any impl or methods, etc.

[catamorphism]

@ktt3ja Great! I'll take a look at this in the morning.

[huonw]

What's the criterion for exploring? (could we have a comment about what it is checking?)

[alexcrichton]

This seems like it copies a fair bit of code from middle::reachable (include the XXX(pcwalton) comment), which seems unfortunate. I'd much rather see this functionality refactored into a common location, or possibly have one of these compiler passes propagate information to the other pass.

[huonw]

Some test-case ideas:

• all the types of entry points are handled correctly, i.e. files with #[main] fn something_thats_not_main() {}, and #[start] fn start(_: int, _: **u8) -> int { 0 }.

• using/not using methods, functions, enums, structs (etc) doesn't warn/warns as appropriate.

• groups of items which are only used amongst themselves warn, i.e. everything other than main should be warned about:

  structFoo;implFoo{fnfoo(&self){bar()}}fnbar(){fnbaz(){}Foo.foo();baz();}fnmain(){}

• A group of items that call each other where only a few are directly used (e.g. the above with fn main() { bar() } should warn about nothing)

• Unused inner items are warned about (e.g. baz above)

• Similar things with libraries, along with:

  • pub items that aren't called anywhere aren't warned about (i.e. exported but not used)

  • otherwise private things that are reexported with pub use aren't warned about, e.g.

    #[crate_type="lib"];pubuse foo::Bar;mod foo {pubstructBar;// not externally visible without the `pub use`}

[ktt3ja]

@alexcrichton: I wanted to see how the final code would take shape before thinking of refactoring. If it turns out that the propagation logic does not need to change at all, then I think it would be as simple as modifying reachable::ReachableContext to take in a visitor and then pass in my own visitor. (that would require making reachable::ReachableContext pub, though, which I'm not sure would be fine)

[alexcrichton]

Certainly! That sounds like a good strategy to me (thanks!)

[ktt3ja]

@alexcrichton: It looks like I have to change ReachableContext a bit more to make dead-code warning pass works. In particular, I need to replace the lines

for method in methods.iter(){self.worklist.push(method.id);}

in reachable with

for method in methods.iter(){match method.vis{
ast::public => {self.worklist.push(method.id);}
_ => (),}}

However, I'm not sure if it's going to break something, so I would like you to give comment on that (I tried to run make check to be sure, but my own dead-code warning pass prevent it from finishing; it gave error on this and this. EDIT: I ran make check and there was no error; should I be sufficiently confident that it works?). Anyway, from what I understand about reachable.rs, for the following code

pubstructFoo;implFoo{fnbaz();}

the method baz would be reachable. I'm not sure if baz is supposed to be reachable, but I know that it should dead.

[alexcrichton]

I'm fairly confident that it's possible to do that without breaking things, but I would rather not have anything look at privacy after the privacy pass. That should be possible by checking if the method is within the exported_items array I believe.

[huonw]

I was thinking test6 would be

#[crate_type="lib"];pubuse foo::Bar;mod foo {pubstructBar;}

and so it shouldn't warn. (Although, the one you have there should warn, I would think.)

BTW, you should add these test cases to src/test/compile-fail, lint-dead-code-1.rs etc, you can see lint-unused-mut-variables.rs for an example (or any of the files there really). Note testsuite describes some how they work.

[ktt3ja]

Ah, I'm sorry. I deleted the comment since I was going to fix something first before notifying you. Here's the comment that huonw responded to (that I modified a bit at the end):

@huonw: With the latest commit, here's my current progress: the code from original post produces the same error message. Here are test2.rs and its warning:

fndead_fn(){}fnused_fn(){}#[main]fnthis_is_main(){used_fn();}// this is not mainfnmain(){dead_fn();}

/home/ktt3ja/dead-code-test/test2.rs:1:0:2:1 warning: code is never used: `dead_fn`,#[warn(dead_code)] on by default
/home/ktt3ja/dead-code-test/test2.rs:1fn dead_fn(){
/home/ktt3ja/dead-code-test/test2.rs:2}
/home/ktt3ja/dead-code-test/test2.rs:13:0:1:0 warning: code is never used: `main`,#[warn(dead_code)] on by default

test3.rs:

fndead_fn(){}#[main]fndead_fn2(){}fnused_fn(){}#[start]fnstart(_:int, _:**u8) -> int{used_fn();0}// this is not mainfnmain(){dead_fn();dead_fn2();}

/home/ktt3ja/dead-code-test/test3.rs:1:0:2:1 warning: code is never used: `dead_fn`,#[warn(dead_code)] on by default
/home/ktt3ja/dead-code-test/test3.rs:1fn dead_fn(){
/home/ktt3ja/dead-code-test/test3.rs:2}
/home/ktt3ja/dead-code-test/test3.rs:5:0:6:1 warning: code is never used: `dead_fn2`,#[warn(dead_code)] on by default
/home/ktt3ja/dead-code-test/test3.rs:5fn dead_fn2(){
/home/ktt3ja/dead-code-test/test3.rs:6} /home/ktt3ja/dead-code-test/test3.rs:18:0:1:0 warning: code is never used: `main`,#[warn(dead_code)] on by default

test4.rs

structFoo;implFoo{fnfoo(&self){bar()}}fnbar(){fnbaz(){}Foo.foo();baz();}fnmain(){}structFoo2;implFoo2{fnfoo2(&self){bar2()}}fnbar2(){fnbaz2(){}Foo2.foo2();baz2();}pubfnpub_fn(){let foo2_struct = Foo2;
foo2_struct.foo2();}

/home/ktt3ja/dead-code-test/test4.rs:1:0:1:11 warning: code is never used: `Foo`,#[warn(dead_code)] on by default
/home/ktt3ja/dead-code-test/test4.rs:1structFoo;
^~~~~~~~~~~
/home/ktt3ja/dead-code-test/test4.rs:2:11:2:34 warning: code is never used: `foo`,#[warn(dead_code)] on by default
/home/ktt3ja/dead-code-test/test4.rs:2implFoo{fnfoo(&self){bar()}}
^~~~~~~~~~~~~~~~~~~~~~~
/home/ktt3ja/dead-code-test/test4.rs:3:0:8:1 warning: code is never used: `bar`,#[warn(dead_code)] on by default
/home/ktt3ja/dead-code-test/test4.rs:3fn bar(){
/home/ktt3ja/dead-code-test/test4.rs:4fnbaz(){}
/home/ktt3ja/dead-code-test/test4.rs:5 /home/ktt3ja/dead-code-test/test4.rs:6Foo.foo();
/home/ktt3ja/dead-code-test/test4.rs:7baz();
/home/ktt3ja/dead-code-test/test4.rs:8}
/home/ktt3ja/dead-code-test/test4.rs:4:3:4:14 warning: code is never used: `baz`,#[warn(dead_code)] on by default
/home/ktt3ja/dead-code-test/test4.rs:4fnbaz(){}
^~~~~~~~~~~

test5.rs

structFoo;implFoo{pubfnfoo1(){}fnfoo2(){}}fnmain(){}

/home/ktt3ja/dead-code-test/test5.rs:1:0:1:11 warning: code is never used: `Foo`,#[warn(dead_code)] on by default
/home/ktt3ja/dead-code-test/test5.rs:1structFoo;
^~~~~~~~~~~
/home/ktt3ja/dead-code-test/test5.rs:5:1:5:13 warning: code is never used: `foo2`,#[warn(dead_code)] on by default
/home/ktt3ja/dead-code-test/test5.rs:5fnfoo2(){}
^~~~~~~~~~~~

and test6.rs

#[crate_type="lib"];mod foo {pubstructBar;// not externally visible without the `pub use`}

produces no dead-code warning.

I think that covers almost everything. Perhaps the foo1 method in test5 and the Bar struct in test6 should be warned. The problem is that they come from exported_items, which get seeded into the worklist initially. I can handle the foo1 case by checking that the item is a method, but I'm not sure how to handle the Bar case.

[huonw]

I'm not 100% sure these are meant to be public, although all their methods are pub. (In any case, they shouldn't be warning, since they are used in the BitvVariant enum, which is used in the public Bitv struct.)

[ktt3ja]

There was actually no warning on the SmallBitv and BigBitv struct; it was just that the negate method of BigBitv was never used. I have reverted the change on the visibility of those two structs, and modified Bitv's negate method to call BigBitv's negate. I think that's what was intended.

[ktt3ja]

r? @alexcrichton and @huonw

A few things that need mentioning:

• I have modified reachable more than I had hoped. It now may be too cluttered with the dead_code_pass boolean.
• As indicated in MarkTraitMethodVisitor, I mark all implemented methods of a trait as live. I figured that due to dynamic dispatch, we can never be sure such methods are alive or not, and so this is a desirable behavior.
• When I was removing dead codes, I would search around to see if they were used or not. I removed them in most of the cases. In some cases, I added, e.g., #[cfg(windows)]. The codes that are not currently in used but may be useful if they remain visible are commented out (see libstd/num/f32.rs; in this respect, I'm not sure if it was right to remove instead of comment out the static items from, e.g., libextra/flate.rs). I put #[allow(dead_code)]; in places I'm unsure about (libextra/btree.rs) or places I guess to be undergoing a lot of changes (libstd/rt/*).
• The dead-code pass that I used to check for dead codes is slightly different from the one that you see now. It used to warn differently depending on whether the crate is built as library or as bin (it essentially uses the check inside ReachableContext::propagate_node). However, since it didn't pass the test on try buildbot, I changed it so that the warning is the same always. The old behavior is probably too confusing anyway, and it may be better to leverage it to some kind of flag instead.

[huonw]

It seems ok to me, and the tests look good; but I don't have enough familiarity with these parts of rustc to r+.

[ktt3ja]

Forgot to mention: it's still missing a test with extern {} block, but I don't know how to put one in there that does not cause linking error.

[huonw]

Something like

extern{fnabort() -> !;}

which imports a symbol from libc should be ok, or, if it's not, using a compiler-internal rust-intrinsic:

extern"rust-intrinsic"{fntransmute<A,B>(x:A) -> B;}

[alexcrichton]

Two more high-level comments:

• I agree that reachability and deadness are getting a little special cased, so it looks like the dead code pass may just want to be its own standalone pass (how much does it still share with reachability?)
• There is a much larger use of allow(dead_code) than I would expect throughout the codebase. I think this may be a sign that this should be an allow-by-default lint. It appears there are a large number of cases in which dead code isn't really a bad thing. On the other hand, periodically running the dead code lint looks like it can be amazingly useful! You certainly found a lot of code that has long since been dead.

[huonw]

There is a much larger use of allow(dead_code) than I would expect throughout the codebase. I think this may be a sign that this should be an allow-by-default lint. It appears there are a large number of cases in which dead code isn't really a bad thing. On the other hand, periodically running the dead code lint looks like it can be amazingly useful! You certainly found a lot of code that has long since been dead.

I think this is mostly just because making the decisions about whether something is supposed to be removed or not is hard for someone who's just starting out with the Rust code base (i.e. @ktt3ja), and it gets fairly tiresome. (This is me voting for warn by default.)

(That is, people who are familiar with the various areas can go through and progressively remove the dead code, and adjust the code that's not supposed to be dead.)

[ktt3ja]

huonw is right about why there are so many allow(dead_code). For the most part, I just estimated and prayed that nothing break.

I agree that reachability and deadness are getting a little special cased, so it looks like the dead code pass may just want to be its own standalone pass (how much does it still share with reachability?)

The dead-code pass is already a standalone pass. The differences between them are:

1. When seeding the worklist at the beginning, dead-code pass also seeds the entry point and implemented trait methods
2. Dead-code pass propagates more and is more lenient when propagating, as seen by the changes in ReachableContext::propagate_inner
3. Dead-code pass's MarkSymbolVisitor also visit type, as such there's a need to handle the ast::DefPrimTy case when looking up for def_id; also, when seeing an enum variant, dead-code pass inserts the enum's DefId to the live symbols set, whereas reachable pass inserts the variant's DefId to the reachable symbols set.

[ktt3ja]

@alexcrichton: hm, it looks like github removed all the comments you made on my old commits and my responses to them. Anyway, I just updated my commits to reduce the amount of code duplication. Unfortunately, it involves littering dead_code_pass boolean throughout middle::reachable and does not look pretty. I also included the changes to reachable from PR #10777.

[ktt3ja]

alexcrichton's comments on an older commit (that got deleted by github):

After reading this, I feel that trying to use a middle::reachable backend for both middle::reachable and middle::dead is not the right way to do this. These two passes are loosely based around propagation of reachability from an initial worklist, but they are distinct in their own separate ways:

• Reachability stops at all non-generic boundaries. The purpose of this pass is find linker reachable code in the sense that downstream crates will attempt to link to particular symbols or items.
• Dead code stops at no boundaries. The purpose of this pass is to find all reachable code.

These two passes to me are fundamentally different, and the code for propagation should not all get mixed together. There may be a need for a 'propagation' abstraction, but it honestly looks like the dead code pass should just look like:

while !worklist.empty()
visit(worklist.pop());

The reachability pass has so many different special cases for dealing with monmorphization boundaries, that I think it's just the wrong direction to attempt to use all of that code.

[ktt3ja]

@alexcrichton: r?

I just updated my commit to separate out middle::dead and middle::reachable. There are some duplications between them again, but I don't think that's avoidable. However, now middle::dead seeds the worklist initially with reachable symbols rather than exported symbols, so any future changes to middle::reachable should reach middle::dead, and thus there's no danger of it reporting any reachable thing as dead.

[alexcrichton]

Awesome work! Just ping me when my comments have been addressed, and I think this is good to go!

[ktt3ja]

I'm going to repost one of your comments and respond to it here in case it gets deleted the next time I force push:

I'm not sure that you want to only recurse into modules. I can definitely imagine trait implementations inside of functions to be reachable (if rare). I'm not entirely certain why trait methods are all not dead (although I can guess as to why it's a tricky situation).

I'm guessing though that you know why all trait methods are seeded, so do you know how trait implementations inside functions work into that reasoning?

It just didn't occur to me that you could implement trait inside function, that's why I only traversed module.

I mark all trait methods as live because it seems to be the easiest course of action. I don't have sufficient understanding of static/dynamic dispatch (and how they are implemented), nor do I a solid enough model in my head to tackle this problem in a more detailed-oriented manner. Perhaps a stricter analysis would be something like this:

1. If the trait is exported, then it follows that all the trait methods are live.
2. If the trait is not exported, we would go see which trait methods are used throughout. Then we would go through all implementations and mark the implementation of all such trait methods as live. The other trait methods are considered dead.

However, I think a justification to the current behavior is that traits define an interface, and defining an interface is ultimately a matter of design. I think usually someone would remove a required method from a trait definition if they are convinced that it wouldn't be useful, and conversely they would want to keep it if they feel that the method is fitting and will be used eventually. It would be awkward to warn dead code on a trait method implementation, then for someone to remove it they would have to remove the method from the trait definition and all the other trait implementations.

[alexcrichton]

I think that assuming all trait implementations are "live code" is certainly ok for now. I'd just recommend recursing into inner functions as well.

[ktt3ja]

@alexcrichton: r+?

My previous changes (re-)introduced some problems that made bors fail to build. I have fixed them by:

• Do not warn methods inside trait definition.
• Seed the worklist with exported items to make warning more consistent. (otherwise, some items that are live when crate is built as library are then warned as dead when it's built as binary, and bors errors out)
• Do not warn dead codes in doc.

Here's the diff between my last commit and this one:

diff --git a/src/etc/extract-tests.py b/src/etc/extract-tests.py
index 5904e10..ab2556f 100644
--- a/src/etc/extract-tests.py+++ b/src/etc/extract-tests.py@@ -64,6 +64,7 @@ while cur < len(lines):
#[ allow(dead_assignment) ];\n
#[ allow(unused_mut) ];\n
#[ allow(attribute_usage) ];\n
+#[ allow(dead_code) ];\n
#[ feature(macro_rules, globs, struct_variant, managed_boxes) ];\n
""" + block
if xfail:
diff --git a/src/librustc/driver/driver.rs b/src/librustc/driver/driver.rs
index f07887c..e08726a 100644
--- a/src/librustc/driver/driver.rs+++ b/src/librustc/driver/driver.rs@@ -312,7 +312,7 @@ pub fn phase_3_run_analysis_passes(sess: Session,
time(time_passes, "death checking", (), |_|
middle::dead::check_crate(ty_cx, method_map,
- reachable_map, crate));+ &exported_items, reachable_map, crate));
time(time_passes, "lint checking", (), |_|
lint::check_crate(ty_cx, &exported_items, crate));
diff --git a/src/librustc/middle/dead.rs b/src/librustc/middle/dead.rs
index 88b0a35..332b706 100644
--- a/src/librustc/middle/dead.rs+++ b/src/librustc/middle/dead.rs@@ -196,31 +196,46 @@ impl Visitor<()> for TraitMethodSeeder {
}
fn create_and_seed_worklist(tcx: ty::ctxt,
+ exported_items: &privacy::ExportedItems,
reachable_symbols: &HashSet<ast::NodeId>,
crate: &ast::Crate) -> ~[ast::NodeId] {
let mut worklist = ~[];
+ // Preferably, we would only need to seed the worklist with reachable+ // symbols. However, since the set of reachable symbols differs+ // depending on whether a crate is built as bin or lib, and we want+ // the warning to be consistent, we also seed the worklist with+ // exported symbols.+ for &id in exported_items.iter() {+ worklist.push(id);+ }
for &id in reachable_symbols.iter() {
worklist.push(id);
}
++ // Seed entry point
match *tcx.sess.entry_fn {
Some((id, _)) => worklist.push(id),
None => ()
}
- let mut trait_method_visitor = TraitMethodSeeder {++ // Seed implemeneted trait methods+ let mut trait_method_seeder = TraitMethodSeeder {
worklist: worklist
};
- visit::walk_crate(&mut trait_method_visitor, crate, ());+ visit::walk_crate(&mut trait_method_seeder, crate, ());- return trait_method_visitor.worklist;+ return trait_method_seeder.worklist;
}
fn find_live(tcx: ty::ctxt,
method_map: typeck::method_map,
+ exported_items: &privacy::ExportedItems,
reachable_symbols: &HashSet<ast::NodeId>,
crate: &ast::Crate)
-> ~HashSet<ast::NodeId> {
- let worklist = create_and_seed_worklist(tcx, reachable_symbols, crate);+ let worklist = create_and_seed_worklist(tcx, exported_items,+ reachable_symbols, crate);
let mut symbol_visitor = MarkSymbolVisitor::new(tcx, method_map, worklist);
symbol_visitor.mark_live_symbols();
symbol_visitor.live_symbols
@@ -315,13 +330,19 @@ impl Visitor<()> for DeadVisitor {
}
visit::walk_block(self, block, ());
}
++ fn visit_trait_method(&mut self, _ :&ast::trait_method, _: ()) {+ // Don't warn on trait method+ }
}
pub fn check_crate(tcx: ty::ctxt,
method_map: typeck::method_map,
exported_items: &privacy::ExportedItems,
+ reachable_symbols: &HashSet<ast::NodeId>,
crate: &ast::Crate) {
- let live_symbols = find_live(tcx, method_map, exported_items, crate);+ let live_symbols = find_live(tcx, method_map, exported_items,+ reachable_symbols, crate);
let mut visitor = DeadVisitor { tcx: tcx, live_symbols: live_symbols };
visit::walk_crate(&mut visitor, crate, ());
}

[ktt3ja]

This not only doesn't warn about trait methods, but this also doesn't recurse into trait methods at all. This means there could be lots of dead code in a trait method is ignored. I think that this should skip checking the trait method itself and then continue with the recursion. Either that, or you could set a temporary flag to get unset in visit_fn, but regardless I think that this shouldn't halt recursion entirely.

@alexcrichton: sorry, I keep missing these small details. :( Now I have it walk the body of a trait method. Is it okay now?

[huonw]

\o/