dead_code warnings for functions/types used in a unused function

Opened by Alexey at 2024-03-16 09:39:07

Hi everybody. Playground Code:

fn foo() {}
fn bar() {
    foo();
}
fn main() {}

Output:

warning: function is never used: `foo`, #[warn(dead_code)] on by default
 --> <anon>:1:1
  |
1 |   fn foo() {
  |  _^ starting here...
2 | |     
3 | | }
  | |_^ ...ending here

warning: function is never used: `bar`, #[warn(dead_code)] on by default
 --> <anon>:5:1
  |
5 |   fn bar() {
  |  _^ starting here...
6 | |     foo();
7 | | }
  | |_^ ...ending here

I don't like it because it confuses what exactly isn't used.

AFAIR its intentional. Neither of the bar and foo functions are ever used directly or indirectly.

See https://github.com/rust-lang/rust/issues/29064 for similar case.
Simonas Kazlauskas at 2017-02-04 12:12:05
@nagisa, I disagree that it is intentional. Let's think about it. Do I use bar anywhere? No, I don't. It is unused. Do I use foo anywhere? Yes, I do. It is used in bar.
Alexey at 2017-02-04 12:21:09
Note that this is dead_code lint, and both are actually dead code. It is not pleasant to re-compile many times until all dead code are removed and maybe look for mutual usages.
Shotaro Yamada at 2017-02-04 12:44:52
I agree that the dead_code lint being transitive is a good thing.

But I think there is room for improvement: We could list the bar() warning first since that's the one that you probably "forgot to call" in this case, and we could maybe even give the foo() warning a slightly different error message like warning: function is never used: 'foo', because 'bar' is never used
Ixrec at 2017-02-04 16:57:59
How would this interact with unused functions that call themselves/each other?
Robert Grosse at 2017-02-05 04:03:11
@Storyyeller, It is hard to express. I understand why that was done like that.
Alexey at 2017-02-05 07:56:51
I will keep this issue opened for several days to see whether someone suggests something.
Alexey at 2017-02-05 07:57:58
I also don't like dead_code being transitive — or at least, the lint shouldn't report things which are used in other dead code not reported because of allow(unused).

E.g. compiling this code warns that Result and MyErr are unused, which is counter-intuitive given that they are used by some other dead code (f) which was presumably left for a reason. (I can't copy the code because copy got broken in the playground again :( )

Interestingly, the use std::result; line is not reported as unused, so that bit is not transitive (no interaction with the unused_imports lint I guess).
Diggory Hardy at 2017-02-23 13:59:41
I'd like to make the case that this should be changed to be non-transitive.

There are four major reasons this lint would appear in a codebase:
1. Some code was checked in that is "legitimately" unused (planned feature perhaps) where the author forgot to add the suppression before checking it in.
2. The code should be deleted.
3. The code is supposed to be being used somewhere and isn't. (A bug)
4. It is a transient warning appearing in code that is being actively worked on by the person at the keyboard.
Walking through the workflow for resolving these will tell us what is preferable in each case.

In case (1), the person reading the code will need to first identify if this is situation 1,2, or 3 before they know what to do. So they will need to identify the root of the problem. If the warning is transitive they will pick one warning and traverse the warnings upward to find the source. If the warning is non-transitive they will be directed right to source of the problem.

In case (2) they will want to delete all of the dead code. If the warning is transitive they can go to each warning in any order and delete the dead code. However if they are not certain that (1) or (3) apply, they may have to investigate first. If the warnings are non-transitive they will delete the first chunk of code, get another warning, delete that code and so on, until all of it is deleted. In this case, once the user is able to figure out they need to delete it, the transitive warning is faster. This advantage is larger on the command line than in an IDE where new warnings can appear when some code is deleted.

In case (3), the user will need to find the root issue. In this case the non-transitive error is preferred.

Case (4) really tips the scales. Because the warning is just noise that the user will ignore.

The worst fate that can befall a lint check is for it to generate so many warnings (legitimate or not) that the user feels overwhelmed and starts ignoring the warnings or turns off the check. If this happens not only does the dead code problem not get fixed, but other bugs possibly more serious ones, get overlooked also.

Making the error non-transitive might be a little slower in some cases, but it is faster in others. It won't lead to surprise from the user, because almost all linters aren't even capable of detecting transitive non-use. Ultimately being a non-transitive check means emitting fewer higher-quality warnings, which will influence how many users pay attention to them.
Tom Kaitchuck at 2018-10-24 05:52:47
Case (4) really tips the scales. Because the warning is just noise that the user will ignore.

#![allow(unused)] helps, but also hides a bunch of legitimate problems.
Diggory Hardy at 2018-10-24 09:03:42
I wrote on the Users forum explaining why I believe the current design is inferior in all use cases.

tl;dr:
- It cannot help when the solution is to use the code; only when the solution is to delete the code.
- I can find no viable workflow for deleting the code:
  - Without an IDE, it is painfully difficult to match each item in the compiler output to items in the source code.
  - With an IDE, if you delete something that is used by other dead code, all other warnings may suddenly disappear due to the compiler error you introduced.
The only thing that could potentially make it useful would be if dead_code were rustfixable.... but even then, warnings used by rustfix are not warnings we need to see. (so I would still want the other design!)
Michael Lamparski at 2019-10-16 18:48:25
I agree that the dead_code lint being transitive is a good thing.

But I think there is room for improvement: We could list the bar() warning first since that's the one that you probably "forgot to call" in this case, and we could maybe even give the foo() warning a slightly different error message like warning: function is never used: 'foo', because 'bar' is never used

Or at least differentiate a function that is not used anywhere from only used in unused function chain in the warning. This will help user to find out the "root" unused function.
Lu, Wangshan at 2020-10-28 06:43:58
I also find this annoying. How about at least #[allow(indirect_dead_code)?
Enyium at 2024-03-16 09:39:07