Ignore derived Clone and Debug implementations during dead code analysis by Fabi...
source link: https://github.com/rust-lang/rust/pull/85200
Go to the source link to view the article. You can view the picture content, updated content and better typesetting reading experience. If the link is broken, please click the button below to view the snapshot at that time.
Conversation
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?
r? @lcnr
(rust-highfive has picked a reviewer for you, use r? to override)
This comment has been hidden.
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).
This comment has been hidden.
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:
Result is returned (e.g. here and here), so I only left a comment there for now.
This comment has been hidden.
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.
The latest upstream changes (presumably #85328) made this pull request unmergeable. Please resolve the merge conflicts.
The latest upstream changes (presumably #85335) made this pull request unmergeable. Please resolve the merge conflicts.
This comment has been hidden.
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.
@@ -37,6 +37,7 @@ use rustc_span::{CharPos, ExpnKind, Pos, SourceFile, Span, Symbol};
/// A simple error message wrapper for `coverage::Error`s.
#[derive(Debug)]
struct Error {
#[allow(dead_code)] // inserted by #85200; maybe remove this whole struct?
message: String,
cc FabianWolff on May 18
Author
Contributor
I don't understand this comment, sorry!
I didn't remove this struct because it is used in various places as the Err component of Result, and I wasn't sure what to replace it with (just ()? An empty Error struct? Option instead of Result?).
FabianWolff on May 18
Author
Contributor
But the functions can fail, it seems; the error value just never escapes this module:
Perhaps we should keep the message field then if it is useful for the bug!() output (or just change it to print e.message instead of e, which would also make the "field never read" warning go away).
richkadel on May 19
Contributor
I don't understand why this attribute is needed. The coverage::Error class is used inside the coverage module, and message is read (AFAICT), and if this somehow needs to be annotated as dead_code when it isn't dead code or unread, seems like a hack. It's so trivial that I would assume whatever change might require this would break a lot of other Rust code.
Regarding the comment, I disagree. I don't think it should be changed or dropped. I should be allowed to have a coverage-specific error type (with a message or any other field). This is specifically typed to ensure the ? try operator applies only to specific coverage function results.
Let me know if I'm missing something.
nikomatsakis on May 19
Contributor
This is an interesting one. The Debug is being used here to convey information...hmm.
compiler/rustc_passes/src/dead.rs
Outdated
im.of_trait.as_ref().and_then(|impls_trait| impls_trait.trait_def_id());
// Get the DefIds of Clone and Debug
let clone_maybe_def_id =
self.tcx.lang_items().require(hir::LangItem::Clone).ok();
compiler/rustc_passes/src/dead.rs
Outdated
.map(|trait_ref| trait_ref.def_id);
// Get the DefIds of Clone and Debug
let clone_maybe_def_id =
self.tcx.lang_items().require(hir::LangItem::Clone).ok();
compiler/rustc_passes/src/dead.rs
Outdated
if trait_maybe_def_id.is_some()
&& (trait_maybe_def_id == clone_maybe_def_id
|| trait_maybe_def_id == debug_maybe_def_id)
&& item.span.in_derive_expansion()
compiler/rustc_passes/src/dead.rs
Outdated
if trait_maybe_def_id.is_some()
&& (trait_maybe_def_id == clone_maybe_def_id
|| trait_maybe_def_id == debug_maybe_def_id)
&& impl_item.span.in_derive_expansion()
lcnr on May 18
Contributor
this feels better to me, it probably doesn't matter too much though.
Would prevent custom derives of Clone to profit from this change 
but maybe that's actually a good thing?
FabianWolff on May 18
Author
Contributor
This was a conscious choice: I thought that it might be too confusing to actually see the field being read in your code and still get a warning that the "field is never read". Of course, if you'd prefer, we can ignore all impls of (say) Clone and Debug, not just automatically derived ones.
lcnr on May 18
Contributor
My edit would not ignore all Clone impls but ignore only those with the attribute #[automatically_derived] which are added by the compiler for the builtin derives.
This change would cause Clone impls emitted by other macros, potentially ones which are better at dealing with implied bounds, to not get ignored by the dead code analysis.
FabianWolff on May 18
Author
Contributor
Ah, I thought you were referring to manual implementations of Clone:
impl Clone for S {
fn clone(&self) -> Self {
...
}
}I actually included your suggested edit in my last commit; the "conscious choice" I was talking about above referred to manual implementations of Clone. Sorry for the confusion!
library/core/tests/fmt/builders.rs
Outdated
struct Foo {
#[allow(dead_code)]
bar: u32,
#[allow(dead_code)]
baz: u32,
}
@@ -8,6 +8,7 @@
struct Point {
x: i32,
y: i32,
//~^ WARNING: field is never read: `y`
@@ -6,6 +6,7 @@
#[derive(Debug)]
struct MyStruct {
a: i32,
//~^ WARNING: field is never read: `a`
src/test/ui/closures/2229_closure_analysis/run_pass/struct-pattern-matching-with-methods.rs
Outdated
@@ -7,8 +7,13 @@
#[derive(Debug, Clone, Copy)]
enum PointType {
TwoD { x: u32, y: u32 },
//~^ WARNING: field is never read: `x`
@@ -9,6 +9,7 @@
struct S {
s: String,
t: String,
//~^ WARNING: field is never read: `t`
nominated for @rust-lang/lang signoff
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?
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.
I'm in favor of the concept, I think extending it to all automatically derived traits is pretty logical.
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:
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.
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?
@@ -37,6 +37,7 @@ use rustc_span::{CharPos, ExpnKind, Pos, SourceFile, Span, Symbol};
/// A simple error message wrapper for `coverage::Error`s.
#[derive(Debug)]
struct Error {
#[allow(dead_code)] // inserted by #85200; maybe remove this whole struct?
message: String,
richkadel on May 19
Contributor
I don't understand why this attribute is needed. The coverage::Error class is used inside the coverage module, and message is read (AFAICT), and if this somehow needs to be annotated as dead_code when it isn't dead code or unread, seems like a hack. It's so trivial that I would assume whatever change might require this would break a lot of other Rust code.
Regarding the comment, I disagree. I don't think it should be changed or dropped. I should be allowed to have a coverage-specific error type (with a message or any other field). This is specifically typed to ensure the ? try operator applies only to specific coverage function results.
Let me know if I'm missing something.
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.
I think PartialEq and PartialOrd are actually legitimate "uses" if and only if the instances are called
Agree.
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.
@@ -269,8 +269,8 @@ pub enum VerifyBound<'tcx> {
#[derive(Copy, Clone, PartialEq, Eq, Hash)]
@@ -37,6 +37,7 @@ use rustc_span::{CharPos, ExpnKind, Pos, SourceFile, Span, Symbol};
/// A simple error message wrapper for `coverage::Error`s.
#[derive(Debug)]
struct Error {
#[allow(dead_code)] // inserted by #85200; maybe remove this whole struct?
message: String,
nikomatsakis on May 19
Contributor
This is an interesting one. The Debug is being used here to convey information...hmm.
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?
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.
I'm inclined to go back to an "opt-in" scheme, with just
CloneandDebug.
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).
This is looking quite good to me. I'm going to go ahead and start an FCP merge.
@rfcbot fcp merge
I am not able to review any PRs in the near future.
The latest upstream changes (presumably #85153) made this pull request unmergeable. Please resolve the merge conflicts.
@nikomatsakis Can you clarify what the final version of the proposal being FCP'd is? It seems like there have been a number of iterations here.
@rfcbot concern taylor-isnt-sure-what-they-are-checking-a-box-for
Could we get explicit confirmation from the proposers here that @rust-lang/lang is only signing off on Debug and Clone here?
Could we get explicit confirmation from the proposers here that @rust-lang/lang is only signing off on
DebugandClonehere?
Yes, this is only about Debug and Clone.
This comment has been hidden.
This comment has been hidden.
This comment has been hidden.
The latest upstream changes (presumably #85904) made this pull request unmergeable. Please resolve the merge conflicts.
rfcbot concern taylor-isnt-sure-what-they-are-checking-a-box-for
Do you feel this concern is resolved? :)
@nikomatsakis I think so-- the plan is to start linting on fields that are unused except in Clone or Debug derives, but not other derives. I'm curious if there's a general principle we plan to apply here, or if we're thinking of this as a one-off "Clone and Debug are especially uninteresting". For example, I could also imagine wanting a lint on a StructOpt field that was never used, as that could indicate that the commandline option was not respected or should be removed. That said, I think I'm fine signing off on this, so..
@rfcbot resolve taylor-isnt-sure-what-they-are-checking-a-box-for
Copy link
rfcbot commented 13 days ago
This is now entering its final comment period, as per the review above.
To me, the difference between Clone and Debug vs {Partial,}Eq, {Partial,}Ord and Hash, is whether there's data actually flowing out of them. Clone is logically the identity function, so it doesn't add anything to the set of meaningful use sites and Debug has no defined output format, so it's only ever useful as a printout to a person. Which is probably why excluding them adds little false-positives.
The only other trait method I can think of that has that kind of "no meaningful return value" semantics and could potentially access all field is Iterator::size_hint (without TrustedLen) – I think it'd be nice to be warned about fields that are used in size_hint but not in next. But that's not a derivable trait. {Partial,}Eq, {Partial,}Ord or Hash on a unit struct would be another case.
Aside: StructOpt doesn't actually seem to prevent dead code detection in any way
I don't like the fact that this is not a visible change in the standard library. I'd like to see an attribute on these derived impls #[trivial_field_reads] (bikesheddable). So, for example, the macro expansion becomes:
#![feature(prelude_import)]
#[prelude_import]
use std::prelude::rust_2018::*;
#[macro_use]
extern crate std;
struct Struct {
field: i32,
}
#[automatically_derived]
#[trivial_field_reads]
#[allow(unused_qualifications)]
impl ::core::clone::Clone for Struct {
#[inline]
fn clone(&self) -> Struct {
match *self {
Struct { field: ref __self_0_0 } =>
Struct{field: ::core::clone::Clone::clone(&(*__self_0_0)),},
}
}
}It could also put the attribute on the function instead of the impl. Then a later PR could e.g. add it to iterator::size_hint if that is actually wanted. And an RFC could stabilize the attribute if that is also wanted.
Copy link
rfcbot commented 3 days ago
The final comment period, with a disposition to merge, as per the review above, is now complete.
As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.
The RFC will be merged soon.
Recommend
About Joyk
Aggregate valuable and interesting links.
Joyk means Joy of geeK