Show notice about "never used" of Debug for enum

[long-long-float]

Close #123068

If an ADT implements Debug trait 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.

[rustbot]

r? @pnkfelix

rustbot has assigned @pnkfelix.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[pnkfelix]

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...

[estebank]

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?

[pnkfelix]

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

[long-long-float]

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.

[pnkfelix]

yes that is what we're asking for.

[long-long-float]

@pnkfelix
I want to confirm my understanding. Do we need to check encl_def_id is a just Variant (or other kind?) because other item such as Ctor also has Enum parent? If so, I can do it immediately.

[long-long-float]

@rustbot ready

[pnkfelix]

I want to confirm my understanding. Do we need to check encl_def_id is a just Variant (or other kind?) because other item such as Ctor also has Enum parent? If so, I can do it immediately.

Yes, I think this is the sort of (redundant!) double-checking that I was asking for. Its just safe-guarding against future-changes in the representation causing the assumptions in this code to become invalidated.

[pnkfelix]

@rustbot author

(as far as I can tell, the author was waiting for confirmation of what next step to take before they actually took that step. See #124460 (comment) )

[long-long-float]

@pnkfelix Thank you. I added a checking as let Node::Variant(_) = tcx.hir_node_by_def_id(may_variant).

[long-long-float]

@rustbot ready

[pnkfelix]

@bors r+ rollup

[bors]

📌 Commit d630f5d has been approved by pnkfelix

It is now in the queue for this repository.