rustc_marker_type built in attribute created to have certain marker types not trigger dead code warnings from clippy

[asder8215]

View all comments

This PR fixes the issue on PhantomPinned triggering field is never read warning by clippy through a built in attribute called rustc_marker_type.

A few questions I have are:

• If a built in attribute approach is okay to use here, should I extend this attribute to all the other Phantom* types?
• Currently, I have every change I made into 1 commit. Should I separate the changes I made for compiler and the changes I made for clippy into different commits?
• Does a built in attribute like this have uses outside of being internal to the standard library? I'm wondering if people create marker types and wish to not have dead code warnings on them.

Note: This is my first time contributing to the compiler side of Rust repos, so I'm unsure if I'm doing anything wrong with making a built in attribute. I'm just going based off my intuition from go to references in my IDE on what everything is doing.

cc @clarfonthey

[rustbot]

Some changes occurred in compiler/rustc_hir/src/attrs

cc @jdonszelmann, @JonathanBrouwer

Some changes occurred in compiler/rustc_passes/src/check_attr.rs

cc @jdonszelmann, @JonathanBrouwer

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

[rustbot]

r? @davidtwco

rustbot has assigned @davidtwco.
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: compiler
• compiler expanded to 69 candidates
• Random selection from 10 candidates

[asder8215]

View changes since the review

For some reason, it kept telling me that it couldn't find std::marker crate, so I couldn't shortcut the field types to marker::PhantomPinned/marker::PhantomData<T>.

I'm not sure how this test operated prior to this, or if I screwed up something to cause this issue.

[asder8215]

View changes since the review

After hours of exploring how attributes works in the compiler, I figured out that the DefId that I could get from a field type's basic result (if it doesn't Err) could inform me about whether that field type has a specific attribute associated with it. If there's a better way to do this than the way I'm doing it, I'm all ears; I'm not even sure if this is the idiomatic way of getting an attribute from a specific type.

[davidtwco]

I expect that rustc also has lints that check for phantom_pinned, so you might want to make sure to check for this there too and replace all uses of phantom_pinned that are for this purpose with the more generic mechanism you're introducing.

In that case, you might want to move this helper to somewhere in rustc. I'd expect it to take a Ty or DefId and do something along the lines of ty.ty_adt_def().map(|adt| /* find_attr */).unwrap_or(false).

[asder8215]

I expect that rustc also has lints that check for phantom_pinned, so you might want to make sure to check for this there too and replace all uses of phantom_pinned that are for this purpose with the more generic mechanism you're introducing.

I don't see anything from the rustc side that lints for PhantomPinned (though there are ui tests that checks for certain behavior with PhantomPinned). Also, to note, PhantomPinned doesn't have a lang item associated with it, so there's no LangItem::PhantomPinned in language item table. The comment above the library/core/src/marker.rs file suggest that this will be deprecated, so I'm not too sure if this should be added into the table (though that seems like a separate PR to work if necessary):

/// A marker type which does not implement `Unpin`.
///
/// If a type contains a `PhantomPinned`, it will not implement `Unpin` by default.
//
// FIXME(unsafe_pinned): This is *not* a stable guarantee we want to make, at least not yet.
// Note that for backwards compatibility with the new [`UnsafePinned`] wrapper type, placing this
// marker in your struct acts as if you wrapped the entire struct in an `UnsafePinned`. This type
// will likely eventually be deprecated, and all new code should be using `UnsafePinned` instead.
#[stable(feature = "pin", since = "1.33.0")]
#[derive(Debug, Default, Copy, Clone, Eq, PartialEq, Ord, PartialOrd, Hash)]
#[rustc_no_dead_code_warning]
pub struct PhantomPinned;

In that case, you might want to move this helper to somewhere in rustc. I'd expect it to take a Ty or DefId and do something along the lines of ty.ty_adt_def().map(|adt| /* find_attr */).unwrap_or(false).

I'm fine with moving the helper to rustc considering that this attribute is built from the compiler end than clippy. Currently, what I'm thinking about doing is adding a method to the AttributeExt trait in rustc_ast/src/attr/mod.rs, something like is_rustc_no_dead_code_warning(&self, tcx: TyCtxt<'_>, def_id: DefId) -> bool.

I'm not sure if we could use ty.ty_adt_def().map(|adt| /* find_attr */).unwrap_or(false) because field.ty from clippy is a &'hir Ty<'hir, ()> and I believe the ty_adt_def() method is exposed to the Ty<'tcx> impl in rustc_middle.

Are there any other approaches I could take?

[asder8215]

I couldn't find a way to have TyCtxt inside rustc_ast/src/attr/mod.rs since rustc_middle couldn't be imported.

I haven't changed much here except using unwrap_or to write it a bit more ergonomically. Let me know if there's a better way for me to create a helper method on the rustc side and be able to call it here (in a similar fashion to function like is_doc_hidden as seen earlier in this file.

[asder8215]

View changes since the review

I'm not sure if this is correct. I took inspiration from other enum members written like this (like Lang or RustcAsPtr). I do know that it operates with attribute parsing from lint_helpers.rs and I assume that lets us know what symbol name represents this AttributeKind, whether duplicate of the attribute applied to a certain target produces a compiler error, what target is this attribute limited to. I'm unsure what create does specifically. My hunch tells me that it's related to whether the attribute macro takes an argument or not, but unsure; if that's the case, I might just not need any arguments as part of this enum member.

[davidtwco]

Yeah, I don't think this needs any fields - the variant itself indicates that a rustc_marker_type attribute was found, we don't need additional context to tell us any more information. That's different from, say, Lang, as while the presence of the variant tells us we found a #[lang], the Symbol tells us what lang item it was - e.g. #[lang = "phantom_pinned"] has Lang(sym::phantom_pinned). Likewise, you aren't using the Span so we can drop it.

[asder8215]

View changes since the review

Also, let me know if this is the correct spot to put the rustc_attr! for rustc_marker_type. It's in miscellaneous right now.

[rustbot]

Some changes occurred in compiler/rustc_attr_parsing

cc @jdonszelmann, @JonathanBrouwer

[davidtwco]

This is a great start

View changes since this review

[davidtwco]

Instead of rustc_marker_type, I'd rename this rustc_no_dead_code_warning as there's nothing specific to marker types about the behaviour of this attribute - it could be used on anything that we didn't want dead-code warnings for.

[davidtwco]

Yeah, I don't think this needs any fields - the variant itself indicates that a rustc_marker_type attribute was found, we don't need additional context to tell us any more information. That's different from, say, Lang, as while the presence of the variant tells us we found a #[lang], the Symbol tells us what lang item it was - e.g. #[lang = "phantom_pinned"] has Lang(sym::phantom_pinned). Likewise, you aren't using the Span so we can drop it.

[davidtwco]

I expect that rustc also has lints that check for phantom_pinned, so you might want to make sure to check for this there too and replace all uses of phantom_pinned that are for this purpose with the more generic mechanism you're introducing.

In that case, you might want to move this helper to somewhere in rustc. I'd expect it to take a Ty or DefId and do something along the lines of ty.ty_adt_def().map(|adt| /* find_attr */).unwrap_or(false).

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[asder8215]

View changes since the review

Added that the attribute can be applied to enums and type aliases. I wasn't sure how to go about testing it in the rustc's tests/ui; it's not giving me warning when I do something similar in clippy_lints like:

struct S {
    unused_field: i32,
}

impl fmt::Debug for S5 {
    //~ WARN unused field
    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
        f.debug_struct("S")
            .finish()
    }
}

Is this something I would have to test for only on the clippy side of things?

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.