[`use_self`]: Make it aware of lifetimes #12386

Ethiraric · 2024-02-29T13:21:05Z

Have the lint trigger even if Self has generic lifetime parameters.

impl<'a> Foo<'a> {
    type Item = Foo<'a>; // Can be replaced with Self

    fn new() -> Self {
        Foo { // No lifetime, but they are inferred to be that of Self
              // Can be replaced as well
            ...
        }
    }

    // Don't replace `Foo<'b>`, the lifetime is different!
    fn eq<'b>(self, other: Foo<'b>) -> bool {
        ..
    }

Fixes #12381

Please write a short comment explaining your change (or "none" for internal only changes)

changelog: [use_self]: Have the lint trigger even if Self has generic lifetime parameters

rustbot · 2024-02-29T13:21:10Z

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

GuillaumeGomez · 2024-02-29T13:22:59Z

clippy_lints/src/use_self.rs

+            // Ensure the type we encounter and the one from the impl have the same lifetime parameters. It may be that
+            // the lifetime parameters of `ty` are ellided (`impl<'a> Foo<'a> { fn new() -> Self { Foo{..} } }`, in
+            // which case we must still trigger the lint.
+            && (same_lifetimes(ty, impl_ty) || has_no_lifetime(ty))


has_no_lifetime makes less operations so I think it'd be better to have it run first.

Suggested change

&& (same_lifetimes(ty, impl_ty) || has_no_lifetime(ty))

&& (has_no_lifetime(ty) || same_lifetimes(ty, impl_ty))

My point of view was that I kind of rarely use Foo without a lifetime in an impl<'a> Foo<'a> block. I feel it is more common to have methods taking a Foo<'b> as parameter, so I optimized for this scenario.

I don't have a strong opinion on the matter, though. I do not mind changing it.

Me neither so let's wait for a clippy reviewer to see waht they prefer. :)

Sorry for the delay. In general, it's better to optimistically check special cases before the complex general case.
So checking for no lifetimes would be better.

clippy_lints/src/use_self.rs

GuillaumeGomez · 2024-02-29T13:27:49Z

clippy_lints/src/use_self.rs

+                .iter()
+                .zip(args_b.iter())
+                .all(|(arg_a, arg_b)| match (arg_a.unpack(), arg_b.unpack()) {
+                    (GenericArgKind::Lifetime(inner_a), GenericArgKind::Lifetime(inner_b)) => inner_a == inner_b,


I don't think the case where a lifetime is infer is handled here, right?

At this point the lifetime will have been "computed" already so even if '_ is used, it won't appear here. Sorry for the noise!

clippy_lints/src/use_self.rs

Ethiraric · 2024-03-07T19:36:57Z

The 2 comments have been addressed. I've also rebased on top of master.

Have the lint trigger even if `Self` has generic lifetime parameters. ```rs impl<'a> Foo<'a> { type Item = Foo<'a>; // Can be replaced with Self fn new() -> Self { Foo { // No lifetime, but they are inferred to be that of Self // Can be replaced as well ... } } // Don't replace `Foo<'b>`, the lifetime is different! fn eq<'b>(self, other: Foo<'b>) -> bool { .. } ``` Fixes rust-lang#12381

blyxyas

Sorry for the delay, I've been busy these days and this PR got in between the cracks =^w^=

Just this little question, you can either change it to false, or come up with a more complex pattern matching to still lint those cases and achieve enlightenment ✨

clippy_lints/src/use_self.rs

blyxyas

LGTM, thanks! ❤️
@GuillaumeGomez, do you also approve this? ^w^

GuillaumeGomez · 2024-03-14T16:07:41Z

I do! Thanks for your review. ;)

blyxyas · 2024-03-14T16:27:49Z

@bors r+

bors · 2024-03-14T16:27:52Z

📌 Commit f3879b3 has been approved by blyxyas

It is now in the queue for this repository.

bors · 2024-03-14T16:28:59Z

⌛ Testing commit f3879b3 with merge 8a78128...

bors · 2024-03-14T16:37:10Z

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: blyxyas
Pushing 8a78128 to master...

``` error: unnecessary structure name repetition --> aya/src/bpf.rs:198:57 | 198 | pub fn btf(&mut self, btf: Option<&'a Btf>) -> &mut EbpfLoader<'a> { | ^^^^^^^^^^^^^^ help: use the applicable keyword: `Self` | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#use_self note: the lint level is defined here --> aya/src/lib.rs:42:5 | 42 | clippy::use_self, | ^^^^^^^^^^^^^^^^ error: unnecessary structure name repetition --> aya/src/bpf.rs:222:54 | 222 | pub fn allow_unsupported_maps(&mut self) -> &mut EbpfLoader<'a> { | ^^^^^^^^^^^^^^ help: use the applicable keyword: `Self` | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#use_self error: unnecessary structure name repetition --> aya/src/bpf.rs:243:69 | 243 | pub fn map_pin_path<P: AsRef<Path>>(&mut self, path: P) -> &mut EbpfLoader<'a> { | ^^^^^^^^^^^^^^ help: use the applicable keyword: `Self` | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#use_self error: unnecessary structure name repetition --> aya/src/bpf.rs:292:15 | 292 | ) -> &mut EbpfLoader<'a> { | ^^^^^^^^^^^^^^ help: use the applicable keyword: `Self` | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#use_self error: unnecessary structure name repetition --> aya/src/bpf.rs:313:73 | 313 | pub fn set_max_entries(&mut self, name: &'a str, size: u32) -> &mut EbpfLoader<'a> { | ^^^^^^^^^^^^^^ help: use the applicable keyword: `Self` | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#use_self error: unnecessary structure name repetition --> aya/src/bpf.rs:335:56 | 335 | pub fn extension(&mut self, name: &'a str) -> &mut EbpfLoader<'a> { | ^^^^^^^^^^^^^^ help: use the applicable keyword: `Self` | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#use_self error: unnecessary structure name repetition --> aya/src/bpf.rs:353:75 | 353 | pub fn verifier_log_level(&mut self, level: VerifierLogLevel) -> &mut EbpfLoader<'a> { | ^^^^^^^^^^^^^^ help: use the applicable keyword: `Self` | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#use_self ``` See rust-lang/rust-clippy#12386.

rustbot assigned blyxyas Feb 29, 2024

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Feb 29, 2024

GuillaumeGomez reviewed Feb 29, 2024

View reviewed changes

clippy_lints/src/use_self.rs Outdated Show resolved Hide resolved

GuillaumeGomez reviewed Feb 29, 2024

View reviewed changes

clippy_lints/src/use_self.rs Show resolved Hide resolved

Ethiraric force-pushed the fix-12381 branch from 5b68c1d to cd5da22 Compare March 7, 2024 19:35

Ethiraric force-pushed the fix-12381 branch from cd5da22 to 416b5d5 Compare March 7, 2024 19:39

Ethiraric force-pushed the fix-12381 branch from 416b5d5 to f3879b3 Compare March 7, 2024 19:46

blyxyas requested changes Mar 13, 2024

View reviewed changes

clippy_lints/src/use_self.rs Show resolved Hide resolved

blyxyas approved these changes Mar 14, 2024

View reviewed changes

bors merged commit 8a78128 into rust-lang:master Mar 14, 2024
5 checks passed

bors mentioned this pull request Mar 14, 2024

Add missing_transmute_annotations lint #12239

Merged

tamird mentioned this pull request Mar 24, 2024

Appease clippy aya-rs/aya#911

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[`use_self`]: Make it aware of lifetimes #12386

[`use_self`]: Make it aware of lifetimes #12386

Ethiraric commented Feb 29, 2024

rustbot commented Feb 29, 2024

GuillaumeGomez Feb 29, 2024

Ethiraric Feb 29, 2024

GuillaumeGomez Feb 29, 2024

blyxyas Mar 6, 2024

GuillaumeGomez Feb 29, 2024 •

edited

GuillaumeGomez Mar 13, 2024

Ethiraric commented Mar 7, 2024

blyxyas left a comment

blyxyas left a comment

GuillaumeGomez commented Mar 14, 2024

blyxyas commented Mar 14, 2024

bors commented Mar 14, 2024

bors commented Mar 14, 2024

bors commented Mar 14, 2024

	&& (same_lifetimes(ty, impl_ty) \|\| has_no_lifetime(ty))
	&& (has_no_lifetime(ty) \|\| same_lifetimes(ty, impl_ty))

[use_self]: Make it aware of lifetimes #12386

[use_self]: Make it aware of lifetimes #12386

Conversation

Ethiraric commented Feb 29, 2024

rustbot commented Feb 29, 2024

GuillaumeGomez Feb 29, 2024

Choose a reason for hiding this comment

Ethiraric Feb 29, 2024

Choose a reason for hiding this comment

GuillaumeGomez Feb 29, 2024

Choose a reason for hiding this comment

blyxyas Mar 6, 2024

Choose a reason for hiding this comment

GuillaumeGomez Feb 29, 2024 • edited

Choose a reason for hiding this comment

GuillaumeGomez Mar 13, 2024

Choose a reason for hiding this comment

Ethiraric commented Mar 7, 2024

blyxyas left a comment

Choose a reason for hiding this comment

blyxyas left a comment

Choose a reason for hiding this comment

GuillaumeGomez commented Mar 14, 2024

blyxyas commented Mar 14, 2024

bors commented Mar 14, 2024

bors commented Mar 14, 2024

bors commented Mar 14, 2024

[`use_self`]: Make it aware of lifetimes #12386

[`use_self`]: Make it aware of lifetimes #12386

GuillaumeGomez Feb 29, 2024 •

edited