On dead_code warnings

1

I'm really, really struggling to understand what the use-case is for the way that rust's current "unused fn/field/struct" warnings work.

Basically, these warnings all fall under the umbrella of rust's "dead code" warnings, which warn on all items that are not used by main (in a binary) or any nameable pub items (in a library). This means that a function that is called by other functions is still considered to be unused if all of the functions that call it are unused. (i.e. items can be "transitively unused")

Insofar as I can tell, this behavior has zero use cases. When dead code exists in a crate, there are two possible resolutions:

  • It's not supposed to be dead, in which case you need to call it.
  • It is supposed to be dead, in which case you need to delete it.

I find the current warnings to be a hindrance in both cases.

The current warnings hinder the integration of unused code

Suppose that the correct fix is to add a call to a function somewhere; maybe these functions were recently added and you never wrote their callsite. (for me, this is almost always the correct solution to these warnings). Which function do you need to call?

Unfortunately, the function you need to call is hidden amongst a tangle of warnings on other functions that are already called somewhere, making it difficult to find. This would be vastly easier if these "unused" warnings did not apply transitively.

The current warnings hinder the deletion of unused code

Suppose that the correct fix is to delete the unused functions (which is seldom the case for me). I used to think that the current dead_code warnings would be helpful in this case. But in a recent attempt to minimize a crate for a bug, I found otherwise.

With an IDE

At some point while minimizing a crate for a bug report, I opened up a fairly important module to find 80% of it highlighted in yellow due to dead_code warnings. Awesome, I thought! I can just select all of these things and delete them!

And so I selected the first contiguous region I could select, and deleted it. As I went to go select the next region, my IDE finished redoing its analysis, and all of the rest of the yellow highlights disappeared right before my eyes. The crate could no longer compile due to name resolution errors, so warnings were no longer highlighted. Oops!

With that, my only recourse was to find functions that contain errors, and delete them, one at a time. I did this for about 10 minutes. At some point, I finally stopped and git stashed everything, because I was worried that it might take me upwards of 40 minutes to get back to a state where I can build my code and verify that the bug is still present. I've been burned many times while minimizing code, and that was not a chance I was willing to take.

So what I ended up doing instead was to delete some things, wait for analysis to rerun, and hit Ctrl+Z if errors appear. This kept the crate in a valid state, so that I could always rebuild and run it to check for the bug. (if the compiler only warned on items that were directly unused, I would have been able to do the same thing without having to wait and potentially hit Ctrl+Z all the time)

Without an IDE

It felt absurd to me that I would have to delete things one-by-one when the compiler has already spit out warnings for all of the functions I need to delete.

So at some point I tried putting the cargo build output on one monitor and the code in another, deleting unused functions in each file from the bottom up (so that the line numbers don't change). This felt awkward and was not much faster than my prior strategy of deleting functions that have errors. It still felt like it was going to take forever to get back to a state where I could compile the crate again, so I gave up.

The only way to make this viable would be if there were a tool to automatically delete all of the unused code in a single fell swoop. I recalled that rustfix relies on warnings/errors provided by the compiler to implement its fixes, so I tried looking into its flags and settings to see if there was some way I could get rustfix to "fix" all of the dead_code warnings. Unfortunately, I was unable to find anything.

Any counter-arguments?

So there you have it. I used to think that maybe the current behavior helped some people in some use cases, but after living through that scenario myself, I can only conclude that it is universally unhelpful.

Does anybody disagree with my experience? Who out there finds the current behavior genuinely useful?

1 Like
2

I definitely don't find the current behavior ideal by any means, but it certainly is useful in the sense that I'm glad we have these warnings?

Take one alternative, for example: warning only about top-level unused things.

I personally don't use an IDE which will highlight warnings, so I usually use the second strategy (going through and deleting things unused 1-by-1). I don't think this would be much improved by only warning about top-level things - I would still have to delete things one-by-one, and I'd have to re-run cargo build every time after deleting!

I go through top-down when deleting unused code, and don't use line numbers - just search for the line in question via what code is on it.

With an IDE though - I'm curious: why not just allow the errors to happen, and delete all the erroring code? No valid used code should error after deleting an unused function, so shouldn't it work just as well to delete all functions and types which can't resolve after removing an unused function?

I've kind of done this in my non-IDE approach, sometimes. I'll have one window with cargo watch open, and another with my editor. I will delete one or two unused code things, it'll auto-save and auto-recompile - and then I go and delete the next thing either erroring or being unused.

When doing this strategy, I don't find the current behavior harmful at all.

With all that said, there's one thing I do particularly like about the current behavior, and that's that I can get a scale of how much code is actually dead. I'm compiling my crate, and testing it - and I get a list of warnings. I can see at a glance whether it's 1 function that I missed, or there's a huge section of code which I'm not using.

If the behavior were something like only warning for top-level unused things, then it would be practically impossible to tell how much of a crate is unused until you actually delete things. And unless you really want to go on an unused-code-deletion binge, this isn't very helpful.

Suppose that the correct fix is to add a call to a function somewhere; maybe these functions were recently added and you never wrote their callsite. (for me, this is almost always the correct solution to these warnings). Which function do you need to call?

This is a valid argument! I think we could probably use some way to find this.

What about an alternative: split the current warning into two: one for "top level dead code" and one for "code used by unused code". Then when developing new features it'd be easy to do #![allow(transitive_dead_code)] at the crate root, and only get top-level warnings. This way we could keep some of the advantages of the current system, and get useful behavior for when implementing things.

Thoughts?

1 Like
3

As I wrote, I tried that, but it would have taken dangerously long before the next time I would have been able to double-check that the code I was minimizing still exhibited the issue. After many bad experiences in the past with failed attempts to minimize code, I've come to the conclusion that you should aim to never have more than two minutes of deleting code before the next test and commit.

4

I sometimes add something like #[cfg_attr(debug_assertions, bad_style)] to stop rustc from being picky while I'm editing code.

dead_code has caught some bugs for me, e.g.

let a = foo_a();
let b = foo_b();
let c = foo_b();
// warning foo_c unused

or

fn foo(arg: i32) {
   let arg_flipped = flip(arg);
   use_flipped(arg);
   // warning arg_flipped unused
}

Regarding dead code elimination, I agree it's not as useful as it seems. It'd be awesome if it sorted the warnings starting from "top" functions first, and "leaf" functions last, so that if you start deleting them in this order, nothing will break.

1 Like
5

To be clear, my position is that dead code warnings are extremely useful, but the way they are currently implemented in rust is spammy and hides the useful information amongst a bunch of garbage.

If only the roots of the "dead code forest" were linted, the user experience would be better in all use cases.

6

I'll admit to using #![allow(dead_code)] when making a new project or doing significant changes. But for me this is more of an IDE issue. Dead code warnings seem to take over everything, even obscuring more useful messages. I do switch the warnings back on once I'm done though. I'd be happier if it only warned when there were no other issues or if VSCode at least minimised their visual impact in some way.

Some auto-tools to, for example, comment out all dead code may help (does this exist?). This would help the case where you think the code simply needs to be deleted. But I don't know how awkward that would be in practice.

7

If dead_code were to become cargo fix-able then having everything identified would allow a single cargo fix invocation to clean it all up, instead of having to run it over and over again till it managed to delete everything from the top down.

But, the idea of splitting the warning in two seems pretty good to me, then the recursive dead code lint could be used for cargo fix etc. while the non-recursive one will identify the most likely points for you to look at when attempting to manually fix it.

3 Likes
8

This topic was automatically closed 90 days after the last reply. New replies are no longer allowed.