Add a clippy attribute #[clippy::no_dead_field_warning] to prevent certain types from propagating dead field warnings

[asder8215]

Summary

Note this is my first time contributing to the clippy repo.

This PR introduces a new clippy attribute to prevent certain types from emitting dead field warnings. This is useful in the event that you have a certain marker type, like PhantomData or PhantomPinned, that you don't want dead field warnings to be propagated when that type/struct is a field member of another struct. While PhantomData is handled already in missing_fields_in_debug.rs and pub_underscore_fields.rs, PhantomPinned does not have the same treatment and it can't be treated the same either because it doesn't have a LangItem symbol. This clippy attribute has its own general use case if other people have marker (or general) types that they don't want dead field warnings to be emitted.

There were discussion occurring in the issue that brought up how PhantomPinned emits dead field warnings and we came to an agreement that instead of introducing a LangItem for each Phantom* member to prevent dead field warning (especially since PhantomPinned will be deprecated soon at least from the comments I'm reading), it would better to use an attribute that could be shared among these Phantom* marker types.

In my original PR, I initially created rustc built in attribute to use within the clippy dead field warning scripts, but on review and feedback, we thought it would be better to create a clippy attribute instead for this. I did create a clippy attribute in the original PR (though this PR renames it from no_dead_code_warning to no_dead_field_warning), but I was redirected to making the PR here instead. I plan to remove the field.ty.basic_res().is_lang_item(cx, LangItem::PhantomData) lines in a future PR if this gets approved.

Please let me know if I did anything incorrect with making a clippy built in attribute or if there's anything else that I need to do.

changelog: [no_dead_field_warning] introduces a new clippy attribute #[clippy::no_dead_field_warning] to prevent certain types from propagating dead field warnings.

[rustbot]

r? @Jarcho

rustbot has assigned @Jarcho.
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

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: 7 candidates
• 7 candidates expanded to 7 candidates
• Random selection from Jarcho, llogiq, samueltardieu

[samueltardieu]

You can also special-case PhantomPinned and add a diagnostic item as a compiler PR. Do you have any evidence that it would be useful to designate other types in real-world uses?

(lang items are used for items that are generated when the compiler expands code, but diagnostic items can be used more liberally to identify items in the standard libraries and following them if they move around)

[asder8215]

You can also special-case PhantomPinned and add a diagnostic item as a compiler PR.

I didn't know about this; I'm unfamiliar with working on the compiler of the Rust repo (just learnt how to make a rustc built in attr while making my original PR) since I mostly make PRs to the std lib. I could definitely try that out in the original PR, and I suppose I would be checking the diagnostic item in missing_fields_in_debug.rs and pub_underscore_fields.rs?

But regarding this point:

Do you have any evidence that it would be useful to designate other types in real-world uses?

I have no evidence or crates I can refer to at the top of my head, but I'd also like to imagine that some people might want to make their own marker (or general) types that doesn't emit dead code warnings as fields of other structs, no?

Actually, now that I think about it. One use case could be in the Tokio filesystem library. This was a PR I was working on in getting statx io-uring support for tokio::fs. I've noticed a couple places where they have #[allow(dead_code)] above the path field, as the field will be read by the kernel during the operation (this pattern is also exhibited for the open operation). If this pattern occurs in multiple areas within tokio, I think it would be neater to use a type alias or new type wrapper for the CString and mark it with this clippy attribute as not needing to emit dead field warnings instead of applying #[allow(dead_code)] everywhere.

[Jarcho]

The Phantom* types are unique in that they tell the compiler something just by existing. No other types have this property. With auto traits and negative impls being unstable and may not end up stabilized no type outside std can practically define them. Since there aren't many of these types (currently two) and they aren't going to be added very frequently I'm not clear why this is an attribute and not a just a function in clippy_utils with a hardcoded list of types.

Actually, now that I think about it. One use case could be in the Tokio filesystem library. This was a PR I was working on in getting statx io-uring support for tokio::fs. I've noticed a couple places where they have #[allow(dead_code)] above the path field, as the field will be read by the kernel during the operation (this pattern is also exhibited for the open operation). If this pattern occurs in multiple areas within tokio, I think it would be neater to use a type alias or new type wrapper for the CString and mark it with this clippy attribute as not needing to emit dead field warnings instead of applying #[allow(dead_code)] everywhere.

I'm going to very strongly disagree with that being a good idea. This just makes the type a way to add an allow attribute while actively hiding it which just makes it harder to find where they actually occur.

[asder8215]

The Phantom* types are unique in that they tell the compiler something just by existing. No other types have this property.

I get that; the Phantom* types are what allow types to have extra generic parameters as markers and allow type checking at compile time. I've seen how it's used in BTreeMap's NodeRef to mark whether that node is Leaf node, Internal, or LeafOrInternal and have specific methods introduced to certain states of that node.

I think it's less so about what the type tells the compiler, but rather it's the very idea that a type could mean something just by existing (and not seen as dead code) that this attribute could be useful (be it for testing purposes or the field is read somewhere else). However, looking back on it, with the #[allow(dead_code)] in the tokio PR (which, I took this pattern from the previous author who made progress on the PR who got this from a separate file that did this), I see that #[repr(C)] might be more appropriate to use there to denote that this could be crossing an FFI boundary (and it also removes dead code warnings on fields as well).

Since there aren't many of these types (currently two) and they aren't going to be added very frequently I'm not clear why this is an attribute and not a just a function in clippy_utils with a hardcoded list of types.

There is no LangItem::PhantomPinned, so we can't do !field.ty.basic_res().is_lang_item(cx, LangItem::PhantomPinned) as it's done for PhantomData in pub_underscore_fields.rs and missing_fields_in_debug.rs. From the issue I pointed out earlier, we discussed about whether to give a lang item symbol to PhantomPinned or not, and we thought it wouldn't be a good idea because there are also other Phantom* types (PhantomCovariant, PhantomInvariant, and 4 others, which are all wrappers around PhantomData) that do not receive a lang item symbol either, and that would introduce unnecessary additions to the lang item symbol table, especially since reading source code comments on PhantomPinned says that it will be deprecated in favor for UnsafePinned. Instead of introducing another symbol, we thought an attribute would be a better idea.

I introduced it as a rustc built in attribute earlier (as #[rustc_marker_type]), but then because this rustc attribute is only used in clippy lint scripts, I was redirected to making a clippy attribute instead.

I know Samuel pointed out diagnostic items, which seems like an alternate viable solution to prevent dead code warning to PhantomPinned; I still have to look into this. I'm also open to suggestions on how to go about this.

I'm going to very strongly disagree with that being a good idea. This just makes the type a way to add an allow attribute while actively hiding it which just makes it harder to find where they actually occur.

I'm unsure if that's a strong argument to make because I do not see why documenting that the type is being read from another area (be it kernel or some FFI boundary) wouldn't solve this issue. I don't see why I necessarily need to see the #[allow(dead_code)] label when good documentation on the field/type (which seems necessary since it's confusing to see dead code being allowed here without reason) will tell me that this field is being read elsewhere outside of this Rust crate/code. However, with regards to FFI/kernel reading, that's the point of #[repr(C)], and I can agree that this attribute may not have strong grounds to hold on.