Show notice about "never used" of Debug for enum #124460
Conversation
rustbot
added
S-waiting-on-review
| 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 |
There was a problem hiding this comment.
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.
-
It is not immediately obvious to me that such an "if and only if" actually holds.
-
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...
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
rustbot
added
S-waiting-on-author
Close #123068
If an ADT implements
Debugtrait and it is not used, the compiler says a note that indicates intentionally ignored during dead code analysis as this note.However this node is not shown for variants that have fields in enum. This PR fixes to show the note.