Check structs and enums for use_self

[nickdrozd]

Closes #15555

changelog: [use_self]: Check structs and enums

Summary Notes

• Feature-freeze by blyxyas

Managed by @rustbot—see help for details

[rustbot]

r? @blyxyas

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

[github-actions]

Lintcheck changes for 292d7ef

Lint	Added	Removed	Changed
clippy::use_self	59	0	4

This comment will be updated if you push new changes

[nickdrozd]

Hello. The implementation of this change turned out to be almost suspiciously easy. I tried to be thorough with testing. Happy to add more cases (lifetimes, etc).

I browsed through the new lintcheck added lines. They seem to be fine. There are a few changed lines, but they seem unrelated to this PR? Not sure.

Probably need doc changes here or there too. Will do if changes are okay.

[blyxyas]

Hi Nick!
We're currently in a feature freeze until September 18, so I'll mark this as blocked. Even though it's a very simple case, the reason for the feature freeze isn't that adding features, is hard, is that maintaining them is harder. Sorry for the inconvenience.

@rustbot note Feature-freeze
@rustbot blocked

[nickdrozd]

Addressed @ada4a 's comments. Updated Clippy sites flagged by lint. Eagerly awaiting feature thaw 🐈

[blyxyas]

I'm not sure if we want to actually go forward with this. Like, using Self in an enum seems kinda weird and I'm not sure if all that conventional. We'll discuss it the next meeting, you can join if you want.

They're hosted on our channel on Zulip =^w^=

[nickdrozd]

Recursive enums are not all that common, but when an enum is recursive Self is very nice to have. This pattern is already used in Clippy itself:

#[derive(Clone, Copy)]
enum NormalizedPat<'a> {
    Wild,
    Never,
    Struct(Option<DefId>, &'a [(Symbol, Self)]),  // <--
    Tuple(Option<DefId>, &'a [Self]),             // <--
    Or(&'a [Self]),                               // <--
    Path(Option<DefId>),
    LitStr(Symbol),
    LitBytes(ByteSymbol),
    LitInt(u128),
    LitBool(bool),
    Range(PatRange),
    /// A slice pattern. If the second value is `None`, then this matches an exact size. Otherwise
    /// the first value contains everything before the `..` wildcard pattern, and the second value
    /// contains everything afterwards. Note that either side, or both sides, may contain zero
    /// patterns.
    Slice(&'a [Self], Option<&'a [Self]>),        // <--
    /// A placeholder for a pattern that wasn't well formed in some way.
    Err(ErrorGuaranteed),
}

Use of Self makes it obvious that this enum is recursive, and it also makes it obvious exactly which fields are recursive. It also reduces noisy lifetime repetition.

If Self in enum or struct seems "unconventional", that may just be because it's a language feature that has been poorly publicized. I only became aware of it myself when I had to add lifetimes to a recursive struct, which without Self is an annoying task. (I was additionally annoyed that Clippy had not already alerted me to this even though I have use_self enabled, hence my attempt to add the feature.)

Part of the purpose of a linter like Clippy is to encourage use of language features. "I see you have a recursive struct / enum, did you know that you can use Self instead of explicitly repeating the name?" use_self already does a good job of encouraging use of Self in impl blocks. It could also do the same here.

[nickdrozd]

Hello. I would like to get this merged soon. Sounds like the result of the meeting is to add this new check, but include a config option to disable it.

What should be the name / description of the option?

[rustbot]

This PR was rebased onto a different master 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.

[nickdrozd]

Recent change #15773 illustrates the benefits of Self for recursive objects. Constant was modified to remove a lifetime. Because it uses explicit name repetition, all of its recursive fields had to modified as well. With Self, those fields could have been left alone.