Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ignore derived Clone and Debug implementations during dead code analysis #85200

Merged
merged 1 commit into from
Sep 11, 2021

Conversation

FabianWolff
Copy link
Contributor

This pull request fixes #84647. Derived implementations of Clone and Debug always trivially read all fields, so "field is never read" dead code warnings are never triggered. Arguably, though, a user most likely will only be interested in whether their code ever reads those fields, which is the behavior I have implemented here.

Note that implementations of Clone and Debug are only ignored if they are #[derive(...)]d; a custom impl Clone/Debug for ... will still be analyzed normally (i.e. if a custom Clone implementation uses all fields of the struct, this will continue to suppress dead code warnings about unused fields); this seemed like the least intrusive change to me (although it would be easy to change — just drop the && [impl_]item.span.in_derive_expansion() in the if conditions).

The only thing that I am slightly unsure about is that in #84647, @matklad said

Doesn't seem easy to fix though :(

However, it was pretty straightforward to fix, so did I perhaps overlook something obvious? @matklad, could you weigh in on this?

@rust-highfive
Copy link
Collaborator

r? @lcnr

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 11, 2021
@rust-log-analyzer

This comment has been minimized.

@FabianWolff
Copy link
Contributor Author

As an argument in favor of this pull request, it has already found a number of unused fields in the compiler code, breaking the bootstrap process. I have now sprinkled a few #[allow(dead_code)] (with comments) throughout the code to make it build again; of course, some of those fields actually should be dropped, but I lack the knowledge about how and where all of those structs are used to make this decision (perhaps instead of dropping the field, it should be used somewhere).

I would have preferred #[warn(dead_code)] instead of #[allow(dead_code)], but this does not work with -D warnings (it always emits errors, even with #[warn(...)], see #75668).

@rust-log-analyzer

This comment has been minimized.

@cjgillot
Copy link
Contributor

For compiler code, it would be better just to drop the fields. For tests, it may be preferrable to add a dummy let just to read the field.

@FabianWolff
Copy link
Contributor Author

For compiler code, it would be better just to drop the fields.

I've dropped the unused fields from the compiler code now, except for the Error struct in compiler/rustc_mir/src/transform/coverage/mod.rs, where dropping the field amounts to dropping the whole struct:

/// A simple error message wrapper for `coverage::Error`s.
#[derive(Debug)]
struct Error {
message: String,
}
impl Error {
pub fn from_string<T>(message: String) -> Result<T, Error> {
Err(Self { message })
}
}

But the struct is used in various places where a Result is returned (e.g. here and here), so I only left a comment there for now.

@rust-log-analyzer

This comment has been minimized.

@matklad
Copy link
Member

matklad commented May 13, 2021

However, it was pretty straightforward to fix, so did I perhaps overlook something obvious? @matklad, could you weigh in on this?

Looks like I was just mistaken, the impl looks ok to me.

One thing I worry is that, judging by the impact on rustc itself, this would be a noticable user-visible change (ie, big projects are guaranteed to get a couple instances of this at least). So it seems like some design process is needed here, culminating in an FCP.

I don't know whats the specific appropriate process here is, so let me tag a random t-compiled lead for this: @pnkfelix.

@bors
Copy link
Contributor

bors commented May 15, 2021

☔ The latest upstream changes (presumably #85328) made this pull request unmergeable. Please resolve the merge conflicts.

@bors
Copy link
Contributor

bors commented May 15, 2021

☔ The latest upstream changes (presumably #85335) made this pull request unmergeable. Please resolve the merge conflicts.

@rust-log-analyzer

This comment has been minimized.

Copy link
Contributor

@lcnr lcnr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this change makes sense. It does have some incorrect warnings if a struct adds a field solely for the sideeffects of that structs Clone impl, but that seems like a fine tradeoff to me.

It might also make sense to extend this to all impls with the syn::automatically_derived attribute. But the pattern of using fields for their behavior in derived impls is probably more prevalent for traits like Hash.

compiler/rustc_mir/src/transform/coverage/mod.rs Outdated Show resolved Hide resolved
compiler/rustc_passes/src/dead.rs Outdated Show resolved Hide resolved
compiler/rustc_passes/src/dead.rs Outdated Show resolved Hide resolved
compiler/rustc_passes/src/dead.rs Outdated Show resolved Hide resolved
compiler/rustc_passes/src/dead.rs Outdated Show resolved Hide resolved
library/core/tests/fmt/builders.rs Outdated Show resolved Hide resolved
@lcnr lcnr added I-nominated T-lang Relevant to the language team, which will review and decide on the PR/issue. labels May 18, 2021
@lcnr
Copy link
Contributor

lcnr commented May 18, 2021

nominated for @rust-lang/lang signoff

@scottmcm
Copy link
Member

This seems good to me -- the compiler examples of how much this has been hiding is quite strong justification 👍

If a field is really only for a Clone side effect, it getting a _name to hide the warning seems totally fine.

One thing I was pondering: What it is about Clone and Debug specifically that makes it worth excluding them, but not others? Why wouldn't it make sense to also exclude a derived PartialEq, for example?

@matklad
Copy link
Member

matklad commented May 18, 2021

One thing I was pondering: What it is about Clone and Debug specifically that makes it worth excluding them, but not others?

Answer, as usual, history -- it was the two traits that preventing the useful unused warning from firing in the case which prompted me to crate the issue.

I think, if we are to do this, we should do this for all build-in derives at least.

@nikomatsakis
Copy link
Contributor

I'm in favor of the concept, I think extending it to all automatically derived traits is pretty logical.

@FabianWolff
Copy link
Contributor Author

Thanks for your comments everyone, and thanks for your review and suggestions @lcnr!

I have implemented them now, along with the suggestion to ignore all automatically derived impls, not just those of Clone and Debug. Predictably, this caused more warnings in the compiler and library code, and as it turned out, many of the fields now (correctly, when ignoring automatically derived impls) marked as "never read" are actually necessary. One example is ordering versions: The fields major, minor, and patch are never explicitly read, but versions are compared, so the fields do have an effect:

if sess.assume_incomplete_release {
rustc_version > min_version
} else {
rustc_version >= min_version
}

I have prefixed such field names with underscores for now to silence the warning. I think that's acceptable, because the underscore makes explicit that the field is needed only for its "side-effects" (e.g. on ordering).

Also keep in mind that I have already removed most of the actually unnecessary fields in an earlier commit, so the above doesn't mean that this change causes unreasonably many false positives.

Copy link
Contributor

@richkadel richkadel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't make sense (to me) to qualify the coverage::Error struct or field. The struct and the message field are used. If Rust thinks it isn't used, then something else is wrong with how this is being compiled.

If you must annotate it to workaround another problem, maybe you can add a FIXME comment with a bug ID, so this annotation can eventually be removed?

compiler/rustc_mir/src/transform/coverage/mod.rs Outdated Show resolved Hide resolved
@joshtriplett
Copy link
Member

I do think it's appropriate to exclude Debug and Clone and similar. However, I think PartialEq and PartialOrd are actually legitimate "uses" if and only if the instances are called. It's valid to have a struct with fields that are only used in equality/ordering comparisons, and nowhere else; I think those shouldn't need an underscore.

@nikomatsakis
Copy link
Contributor

@joshtriplett

I think PartialEq and PartialOrd are actually legitimate "uses" if and only if the instances are called

Agree.

Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hadn't actually looked at the diff before, but I agree that we need to be considering PartialEq, Hash, and friends as using the fields. I think a good goal for the PR should be that "no false warnings" and right now there are a lot, but almost all seem to be linked to PartialEq / Eq / Hash.

The case of the Error struct with the message field is interesting and a bit more complex.

compiler/rustc_infer/src/infer/region_constraints/mod.rs Outdated Show resolved Hide resolved
compiler/rustc_mir/src/transform/coverage/mod.rs Outdated Show resolved Hide resolved
@FabianWolff
Copy link
Contributor Author

So should I go back to just Clone and Debug for now? Or should I keep the warning for all automatically derived traits except Eq, PartialEq, Ord, PartialOrd, and Hash?

@nikomatsakis
Copy link
Contributor

I'm inclined to go back to an "opt-in" scheme, with just Clone and Debug. I'm trying to put my finger on what makes those traits different. I'd like to articulate the logic behind the heuristic. It seems to be just more true in practice that people rarely add fields "just for the purpose of cloning them or having the appear in debug output", although you can certainly imagine the latter (the former is a stretch, in my opinion-- though of course not impossible).

That said, this is only a lint. I think that both of those kinds of cases are ones where it'd be reasonable to put a comment explaining what the field is doing there

I think the difference with (e.g.) PartialEq is that you could be using the thing a a key in a BTreeMap, and therefore making use of those fields in a very intentional way.

@FabianWolff
Copy link
Contributor Author

I'm inclined to go back to an "opt-in" scheme, with just Clone and Debug.

Thanks for the clarification! I have now gone back to just Clone and Debug again. I've also rebased and squashed my previous commits, so I'd say this is ready for re-review (@lcnr).

@nikomatsakis
Copy link
Contributor

This is looking quite good to me. I'm going to go ahead and start an FCP merge.

@rfcbot fcp merge

erickt added a commit to erickt/argh that referenced this pull request Dec 8, 2021
rust-lang/rust#85200 changed rust to emit more
unused warnings if fields in a struct are ultimately never read. This
adds a test to make sure that we don't experience these warnings.
erickt added a commit to google/argh that referenced this pull request Dec 8, 2021
rust-lang/rust#85200 changed rust to emit more
unused warnings if fields in a struct are ultimately never read. This
adds a test to make sure that we don't experience these warnings.
@shepmaster
Copy link
Member

I'd like to see an attribute on these derived impls

it'd be nice to have the code use a #[rustc_foo] attribute

Is there another issue I can follow to track the stabilization of this attribute? One of my crates uses derives to create structures and those structures effectively disable the dead code lint. I believe this attribute might help me in my case.

nsunderland1 added a commit to nsunderland1/rust that referenced this pull request Feb 12, 2022
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 12, 2022
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 12, 2022
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 12, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 12, 2022
…askrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#91908 (Add 2 tests)
 - rust-lang#93595 (fix ICE when parsing lifetime as function argument)
 - rust-lang#93757 (Add some known GAT bugs as tests)
 - rust-lang#93759 (Pretty print ItemKind::Use in rustfmt style)
 - rust-lang#93897 (linkchecker: fix panic on directory symlinks)
 - rust-lang#93898 (tidy: Extend error code check)
 - rust-lang#93928 (Add missing release notes for rust-lang#85200)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
aswild added a commit to aswild/ripgrep that referenced this pull request Feb 21, 2022
This code hasn't changed, but the warnings are new in Rust 1.57 because
of "ignore derived Clone and Debug implementations during dead code
analysis"[1]. Rust now doesn't count its generated derive code as a use
of these fields, revealing that they're not actually needed. Sure
enough, removing them compiles successfully and passes tests.

[1] rust-lang/rust#85200
naturallymitchell pushed a commit to naturallymitchell/fuchsia-storage that referenced this pull request Jun 10, 2022
Upstream rustc has expanded the unused field lint to to not count
usages performed by derived Clone and Debug implementations.
rust-lang/rust#85200

This CL marks such fields with an #[allow(unused)] tag.

Bug: 84550
Change-Id: Ie5a84dd7c941b47419eb2a011a8daed22fc16695
Reviewed-on: https://fuchsia-review.googlesource.com/c/fuchsia/+/581062
Fuchsia-Auto-Submit: Adrian Danis <[email protected]>
Commit-Queue: Auto-Submit <[email protected]>
Reviewed-by: Tyler Mandry <[email protected]>
SifraiTeam pushed a commit to grandinetech/grandine that referenced this pull request Sep 6, 2024
The warnings in slasher::status seem to be the result of 2 changes:
- rust-lang/rust#85200
- rust-lang/rust#118297
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

derive(Clone, Debug) should not prevent "field is never read" from firing