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.

Sign up for GitHub

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

Jump to bottom

Show notice about "never used" of Debug for enum #124460

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
+46 −1
Open

Show notice about "never used" of Debug for enum #124460

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ca67164
Show notice about "never used" for enum
long-long-float Apr 28, 2024
c1974c4
Fix tests
long-long-float Apr 28, 2024
4bd1a5e
Add test for struct variant
long-long-float May 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 10 additions & 0 deletions compiler/rustc_passes/src/dead.rs
View file
Open in desktop
Expand Up @@ -976,6 +976,16 @@ impl<'tcx> DeadVisitor<'tcx> {
};

let encl_def_id = parent_item.unwrap_or(first_item.def_id);
// If parent of encl_def_id is an enum, use the parent ID intead.
let encl_def_id = if let Some(enum_did) = self.tcx.opt_parent(encl_def_id.to_def_id())
&& let Some(enum_local_id) = enum_did.as_local()
&& let Node::Item(item) = self.tcx.hir_node_by_def_id(enum_local_id)
&& let ItemKind::Enum(_, _) = item.kind
Copy link
Member

Choose a reason for hiding this comment

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

This logic is making me nervous.

If I understand correctly, this is putting in a bit of an ad-hoc workaround, specifically into the dead-code lint pass, to hop up two parents instead of just one parent if the second parent is itself identified to be an enum.

i infer that is because it is trying to follow a line of reasoning like "the second parent will be an enum if and only if the first parent is an enum variant and the original item is a data field attached to that enum variant.

  1. It is not immediately obvious to me that such an "if and only if" actually holds.

  2. Even if it does hold, my second worry is that I imagine this same mistake probably presents itself in a bunch of other places (i.e., places where people assumed that the parent of a data field would be the ADT type that holds that field, but in fact one has to go up two parent-hops instead of one in cases like this), and I wonder if we would be better off using an API that let one express that intention directly, rather than putting logic like this everywhere it needs to go...

Copy link
Contributor

Choose a reason for hiding this comment

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

Even if it does hold, my second worry is that I imagine this same mistake probably presents itself in a bunch of other places

I've seen issues around the Enum/Variant/CTor trichotomy, where the DefId of the item is expected for some analysis but you get passed one of the other two, so I'm sure there are more lurking around.

Could you let us know what the contents of parent_item (what it points to, really) and of first_item are?

Copy link
Member

Choose a reason for hiding this comment

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

I talked to @estebank independently.

While I do think better API's might exist, I don't think I want to block this PR on hypothetical API improvements.

What I do want from this PR is for it to change the code slightly to include a few more, potentially redundant, safe-guards where you have the code check, as it does its double hop, that each item kind it encounters is the one you expect (e.g. taken from Enum/Variant/Ctor as appropriate.)

@rustbot author

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for comment and sorry for the late reply.

I'd like to confirm my understanding. We need to check encl_def_id is a just Variant (or other kind?) because other item such as Ctor also has Enum parent.

{
enum_local_id
} else {
encl_def_id
};
let ignored_derived_impls =
if let Some(ign_traits) = self.ignored_derived_traits.get(&encl_def_id) {
let trait_list = ign_traits
Expand Down
12 changes: 10 additions & 2 deletions tests/ui/lint/dead-code/unused-variant.rs
View file
Open in desktop
@@ -1,12 +1,20 @@
#![deny(dead_code)]

#[derive(Clone)]
enum Enum {
enum Enum1 {
Variant1, //~ ERROR: variant `Variant1` is never constructed
Variant2,
}

#[derive(Debug)]
enum Enum2 {
Variant1(i32), //~ ERROR: variant `Variant1` is never constructed
long-long-float marked this conversation as resolved.
Show resolved Hide resolved
Variant2,
}

fn main() {
let e = Enum::Variant2;
let e = Enum1::Variant2;
e.clone();

let _ = Enum2::Variant2;
}
18 changes: 14 additions & 4 deletions tests/ui/lint/dead-code/unused-variant.stderr
View file
Open in desktop
@@ -1,17 +1,27 @@
error: variant `Variant1` is never constructed
--> $DIR/unused-variant.rs:5:5
|
LL | enum Enum {
| ---- variant in this enum
LL | enum Enum1 {
| ----- variant in this enum
LL | Variant1,
| ^^^^^^^^
|
= note: `Enum` has a derived impl for the trait `Clone`, but this is intentionally ignored during dead code analysis
= note: `Enum1` has a derived impl for the trait `Clone`, but this is intentionally ignored during dead code analysis
note: the lint level is defined here
--> $DIR/unused-variant.rs:1:9
|
LL | #![deny(dead_code)]
| ^^^^^^^^^

error: aborting due to 1 previous error
error: variant `Variant1` is never constructed
--> $DIR/unused-variant.rs:11:5
|
LL | enum Enum2 {
| ----- variant in this enum
LL | Variant1(i32),
| ^^^^^^^^
|
= note: `Enum2` has a derived impl for the trait `Debug`, but this is intentionally ignored during dead code analysis

error: aborting due to 2 previous errors