fix(use_self): don't early-return if the outer type has no lifetimes #15611

ada4a · 2025-09-04T17:46:45Z

Unfortunately breaks another case, which has only been working thanks to the now fixed bug -- see last commit.

changelog: [use_self]: don't early-return if the outer type has no lifetimes

rustbot · 2025-09-04T17:46:50Z

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

ada4a · 2025-09-04T17:46:56Z

r? clippy

github-actions · 2025-09-04T17:57:15Z

Lintcheck changes for e7c91ab

Lint	Added	Removed	Changed
`clippy::use_self`	0	3	0

This comment will be updated if you push new changes

ada4a · 2025-09-04T18:20:39Z

Although now that I think of it, maybe checking only the top-level type was the point of has_no_lifetime? Because now that we descend in it exactly like we do in same_lifetimes, I think it becomes kind of redundant?

@Ethiraric, I see that you were the one who added this function back in #12386 -- do you maybe remember the motivation behind it?

Looking at lintcheck, this PR does add FNs in real-life code, which is of course unfortunate, though it's all elided-lifetimes-related, which that PR explicitly left out as future work -- I guess it's FNs are preferable to positives that only happen to be true positives due to a bug...

samueltardieu

"misc" is not a very descriptive commit name

View changes since this review

clippy_lints/src/use_self.rs

ada4a · 2025-09-04T18:34:53Z

"misc" is not a very descriptive commit name

That's the solution I came up with as a way to avoid having multiple misc-type commits, which you previously also complained about -- I assumend that, if you don't think they each deserve their own commit message, then I should probably squash them all together. But if that's not the right approach either, then I honestly don't know what is. I didn't want to squash them into the main commit either, because the two commits touch related code.

Please do tell me how you would like to see those misc changes committed (maybe as a separate PR with multiple commits?), and I'll happily follow that.

Ethiraric · 2025-09-04T18:40:59Z

I unfortunately don't recall the exact reason why. I was discovering the codebase at that time. I've had a look at #12386 and #12381 (the associated issue) but can't figure out more context out of it.

Although now that I think of it, maybe checking only the top-level type was the point of has_no_lifetime? Because now that we descend in it exactly like we do in same_lifetimes, I think it becomes kind of redundant?

This looks like an oversight from me, more than an informed decision.

ada4a · 2025-09-04T18:46:22Z

Thank you for the quick response @Ethiraric:)

This looks like an oversight from me, more than an informed decision.

In that case I think I might as well remove that method^^

Ethiraric · 2025-09-04T21:50:06Z

Whether an oversight or not, as long as the tests pass and a rationale for removing it is established, it's all good. I saw the example in issue 13277 and the contents of my (former) function don't seem at all to be able to handle it :)

flip1995 · 2025-09-04T22:40:53Z

I'm on vacation right now (this assignment reminded me to actually tell triagebot that). So re-assigning to Samuel, as he already took a look. r? samueltardieu

samueltardieu · 2025-09-05T15:57:27Z

"misc" is not a very descriptive commit name

That's the solution I came up with as a way to avoid having multiple misc-type commits, which you previously also complained about -- I assumend that, if you don't think they each deserve their own commit message, then I should probably squash them all together. But if that's not the right approach either, then I honestly don't know what is. I didn't want to squash them into the main commit either, because the two commits touch related code.

Please do tell me how you would like to see those misc changes committed (maybe as a separate PR with multiple commits?), and I'll happily follow that.

I feel you.

With "misc", you cannot tell whether the commit changes things or not. If the commit does some refactoring and cleanups, "Minor cleanup" would be appropriate, and if some code got refactored into a utility function for example you can say that in the longer description.

If I see a commit titled "Minor cleanup", I would expect any behavior difference between the state before the commit and the state after the commit to be an unintended change, thus a bug. This facilitates a commit-by-commit review. Also, when chasing regression between releases, we might need to look at commits that touch some file. Bisecting is not always easy to do, as some versions don't compile (this should never happen in my opinion, every commit should pass the tests, but that's another story). Being able to skip "Minor cleanup", or "Refactoring" commits in the first pass makes things easier.

So thank you for separating iso-functionality changes into separate commits (I like that!), but we have to find a middle ground between commit spam in PRs and non-descriptive messages.

Note that this depends on the project: on Linux, QEMU, or jj, I would make a lot of smaller commits as you did, because this is how their review process works. In Rust or Clippy, people tend to not like PRs with lots of commits inside, although very small commits are very often accepted in isolation. I understand things would be easier if we had PRs stack as independent cleanup PRs could be depended on by a more concrete PR, and each cleanup PR could be reviewed and accepted very quickly. Unfortunately, GitHub PR model makes this difficult…

ada4a · 2025-09-05T16:11:34Z

Thank you for the empathy:)

I agree that "misc" was too short to be helpful -- "minor cleanup" plus an optional description of larger refactorings does sound like a better middle-ground.

[some commits in the PR not compiling] should never happen in my opinion, every commit should pass the tests, but that's another story

Very much agree -- I sometimes go as far as uiblessing after each commit to show what caused each output change (e.g. removing #[allow(unused)] moves all the lint firing locations, and it's nice to isolate that noise away).

In Rust or Clippy, people tend to not like PRs with lots of commits inside

That is somewhat surprising to me, because Rust's syntax can be quite diff-unfriendly sometimes.. I guess using word-diffs and programs like difftastic could help a bit.

Again, thank you for taking the time to write this out -- it helps a lot to know what the person on the other side has on their mind when looking at my PR:)

samueltardieu

In addition to the "misc" commit name, some small nits.

View changes since this review

samueltardieu · 2025-09-05T16:13:24Z

clippy_utils/src/ty/mod.rs

+                (GenericArgKind::Const(inner_a), GenericArgKind::Const(inner_b)) => inner_a == inner_b,
+                (GenericArgKind::Type(type_a), GenericArgKind::Type(type_b)) => same_type_and_consts(type_a, type_b),
+                _ => true,
+            })


I know this predates your patch, but shouldn't this function name contain _modulo_regions, and document that lifetime mismatches don't cause the function to return false?

I'd be curious to know whether this makes sense to ignore the lifetimes in all use cases of this function though, but at least this would make the intention clearer.

but shouldn't this function name contain _modulo_regions, and document that lifetime mismatches don't cause the function to return false

As #15610 just pointed out, a lot of things in Clippy tend to ignore lifetimes it seems..

I guess lifetimes being ignored is implied by contrast ("only takes into account the type and const generics, therefore ignores lifetimes"), but a more explicit name could not hurt. I think same_types_modulo_regions is a good option.

I added a few "doctests" for some extra clarity (I often wish more util functions included these, to help understand what the function does at a glance) -- let me know what you think

You mean "examples", not "doctests", right?

~~Well I avoided calling them "doctests"~~ because the main thing about doctests is that they're, well, tests, that you can run^^ I really wish there were actual doctests on these things, but that would require somehow getting HIR/AST nodes to pass into the functions, which I think would require to run the entire compiler pipeline for each test? That would be a bit wasteful of course

EDIT: Ah, sorry, misread your comment. Yes, that's why put "doctests" in quotes -- they aren't actual doctests after all. The rest of my comment still holds though^^

samueltardieu · 2025-09-05T16:14:20Z

tests/ui/use_self.rs

@@ -531,7 +531,7 @@ mod issue7206 {
    impl<'a> S2<S<'a>> {
        fn new_again() -> Self {
            S2::new()
-            //~^ use_self
+            // FIXME: this^ should lint


Just to make sure:

If this linted before, maybe add Broken by PR #15611 and open an issue when this PR is merged (or, better, fix it if it can be done easily)

If this doesn't lint even before this PR, can you open an issue and reference it in the comment?

It's a bit complicated:

S2::new() actually contains an erased¹ lifetime (the '_ in S2::<S<'_>>::new()), and when #12386 first added the handling of lifetimes, it explicitly left out erased lifetimes as future work.

The only reason this has worked before this PR was that the !has_lifetime check false-positively fired on S2<S<'a>>, because it only looked for generic lifetime params on S2, forgetting to descend into S -- and so the lint fired before having the chance to check same_lifetimes. And now that I first made has_lifetime descend into inner types, and then removed it, same_lifetimes stops the lint from firing, because it doesn't handle the erased lifetime.

So if I were to open an issue, it would pretty much amount to "C-improvement: use_self doesn't handle erased lifetimes" -- which I guess is fair enough. Fixing that issue would be non-trivial (that is actually what I tried to do for a while before giving up and opening this PR, with the breakage included), and would need to wait for the end of the feature freeze anyway.

Footnotes

"erased" might be a synonym for "elided", but I'm not sure, so I'll stick to the former ↩

samueltardieu · 2025-09-05T17:09:24Z

clippy_utils/src/ty/mod.rs

+///
+/// `True` for:
+/// - `Option<u32>` and `Option<u32>`
+/// - `[u8; N]` and `[u8; M]`, if `N=M`


Suggested change

/// - `[u8; N]` and `[u8; M]`, if `N=M`

/// - `[u8; N]` and `[u8; M]`, if `same_type_modulo_regions(N, M)` holds

Hm, not really -- N and M would be Consts, not Tys. I could add an example like

Option<T> and Option<U>, if same_type_modulo_regions(T, U) holds

to illustrate that case though

samueltardieu · 2025-09-05T17:09:55Z

clippy_utils/src/ty/mod.rs

+/// Checks whether `a` and `b` are same types having same `Const` generic args, but ignores
+/// lifetimes.
+///
+/// `True` for:


It would be clearer to indicate that those are examples, not the only cases handled:

Suggested change

/// `True` for:

/// For example, the function would return `true` for

also the constant is true

also the constant is true

I capitalized it because it was kind of the first word in the sentence, but yeah you're right.

samueltardieu · 2025-09-05T17:10:36Z

clippy_utils/src/ty/mod.rs

+/// - `[u8; N]` and `[u8; M]`, if `N=M`
+/// - `&'a str` and `&'b str`
+///
+/// `False` for:


Suggested change

/// `False` for:

/// and `false` for:

samueltardieu · 2025-09-05T17:11:43Z

clippy_utils/src/ty/mod.rs

+                (GenericArgKind::Const(inner_a), GenericArgKind::Const(inner_b)) => inner_a == inner_b,
+                (GenericArgKind::Type(type_a), GenericArgKind::Type(type_b)) => same_type_and_consts(type_a, type_b),
+                _ => true,
+            })


You mean "examples", not "doctests", right?

Notably, rename `same_type_and_consts` to `same_type_modulo_regions` to be more explicit about the function ignoring lifetimes

This unfortunately breaks another test case -- it had only worked because when we checked `!has_lifetime` on `S2<S<'a>>`, we only looked for generic lifetime params on `S2`, forgetting to descend into `S`. One thing that makes this a bit less sad is that this particular test case doesn't come from a reported issue, but rather was added as a miscellaneous check during a kind of unrelated PR.

It's now redundant to `same_lifetimes`: if `ty` and `impl_ty` are the same type, and they both have no lifetimes, `same_lifetimes` will return `true` already

rustbot · 2025-09-06T10:47:09Z

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.

rustbot assigned Alexendoo Sep 4, 2025

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Sep 4, 2025

rustbot assigned flip1995 and unassigned Alexendoo Sep 4, 2025

samueltardieu reviewed Sep 4, 2025

View reviewed changes

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

ada4a changed the title ~~fix(use_self): descend into type's children when looking for lifetimes~~ fix(use_self): don't early-return if the outer type has no lifetimes Sep 4, 2025

rustbot assigned samueltardieu and unassigned flip1995 Sep 4, 2025

samueltardieu reviewed Sep 5, 2025

View reviewed changes

ada4a force-pushed the use_self-13277 branch from 98e7329 to facb442 Compare September 5, 2025 17:00

samueltardieu requested changes Sep 5, 2025

View reviewed changes

rustbot added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Sep 5, 2025

ada4a added 3 commits September 6, 2025 12:43

clean-up a bit

4ae4f67

Notably, rename `same_type_and_consts` to `same_type_modulo_regions` to be more explicit about the function ignoring lifetimes

remove has_lifetime

e7c91ab

It's now redundant to `same_lifetimes`: if `ty` and `impl_ty` are the same type, and they both have no lifetimes, `same_lifetimes` will return `true` already

ada4a force-pushed the use_self-13277 branch from facb442 to e7c91ab Compare September 6, 2025 10:47

	/// - `[u8; N]` and `[u8; M]`, if `N=M`
	/// - `[u8; N]` and `[u8; M]`, if `same_type_modulo_regions(N, M)` holds

	/// `True` for:
	/// For example, the function would return `true` for

fix(use_self): don't early-return if the outer type has no lifetimes #15611

Are you sure you want to change the base?

fix(use_self): don't early-return if the outer type has no lifetimes #15611

Uh oh!

Conversation

ada4a commented Sep 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Sep 4, 2025

Uh oh!

ada4a commented Sep 4, 2025

Uh oh!

github-actions bot commented Sep 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ada4a commented Sep 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

samueltardieu left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ada4a commented Sep 4, 2025

Uh oh!

Ethiraric commented Sep 4, 2025

Uh oh!

ada4a commented Sep 4, 2025

Uh oh!

Ethiraric commented Sep 4, 2025

Uh oh!

flip1995 commented Sep 4, 2025

Uh oh!

samueltardieu commented Sep 5, 2025

Uh oh!

ada4a commented Sep 5, 2025

Uh oh!

samueltardieu left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ada4a Sep 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ada4a Sep 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ada4a Sep 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rustbot commented Sep 6, 2025

Uh oh!

Uh oh!

ada4a commented Sep 4, 2025 •

edited

Loading

github-actions bot commented Sep 4, 2025 •

edited

Loading

ada4a commented Sep 4, 2025 •

edited

Loading

samueltardieu left a comment •

edited by rustbot

Loading

samueltardieu left a comment •

edited by rustbot

Loading

ada4a Sep 5, 2025 •

edited

Loading

ada4a Sep 5, 2025 •

edited

Loading

ada4a Sep 5, 2025 •

edited

Loading