`clippy_utils`: make `peel_*_ty_refs` class of functions a bit more consistent #15515

ada4a · 2025-08-19T12:24:41Z

I was looking for the function that peels refs from a type while also returning the number of refs peeled, and it turns out there were actually two of them?? I removed the by far less popular one, and made some other clean-up along the way

changelog: none

rustbot · 2025-08-19T12:24:45Z

r? @Alexendoo

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

clippy_utils/src/ty/mod.rs

ada4a · 2025-08-19T13:47:41Z

Right. I don't really understand why rustfmt is complaining about the line -- it's even marked with #[rustfmt::skip]! Should I remove the attribute and format the line?

EDIT: ah, it's not quite that: what rustfmt actually tries to format is the import line (because of the moved peel_middle_ty_refs import), but then does a max width checks, which for some reason fails on the #[rustfmt::skip]-ed line.. Manually formatting just the import fixed the issue. This is probably a bug in rustfmt?

ada4a · 2025-08-19T13:52:32Z

Also it's a shame that lintcheck / base continues to run even though lintcheck / head already failed -- but there doesn't seem to be a good way to solve this

samueltardieu · 2025-08-22T17:09:24Z

Also it's a shame that lintcheck / base continues to run even though lintcheck / head already failed -- but there doesn't seem to be a good way to solve this

Why? If I remember correctly, the results from lintcheck/base are cached for a given base, so it will be reused if you git push -f after it has finished if you use the same base.

ada4a · 2025-08-22T17:14:29Z

Oh is it? That's nice. I guess the other direction (base fails, so head should as well) is also not a problem, because how would base end up in a broken state in the first place.. Well, good to know:)

samueltardieu

I like clean ups in general, as they reduce the technical debt.

However, when you use longer commit messages, could you use proper sentences and casing in them? It's ok for the short message to be concise, even though most of us still use proper casing, but longer messages should be both useful and easy to read.

clippy_utils/src/ty/mod.rs

samueltardieu · 2025-08-22T17:15:14Z

clippy_utils/src/ty/mod.rs

-        }
+/// Peels off all references on the type. Returns the underlying type and the number of references
+/// removed.
+pub fn peel_middle_ty_refs(mut ty: Ty<'_>) -> (Ty<'_>, usize) {


I think peel_and_count_refs would be a better name, we are in the ty module so I don't think it is useful to repeat "middle" here.

I would agree if this were a method on middle::Ty, but it's a freestanding function which is often imported unqualified, so having some redundancy in the name could be helpful imo, WDYT?

The and_count part I do agree with though.

This is the only part in Clippy where we use "middle_ty" as part of a function name, this is why it looks odd to me. And this is never used in the whole compiler as part of a function name either. In case of an ambiguity, it looks like "hir_ty" is used for rustc_hir::Ty, while "ty" is used for rustc_middle::ty::Ty.

Fair enough. There's also peel_mid_ty_refs_is_mutable right beside it, and I'd like to give it a similarly structured name, but that's a bit hard given that that function does many things at once.

Maybe something like peel_ty_refs_with_count and peel_ty_refs_with_count_is_mutable?

@samueltardieu friendly ping

Or maybe just one function peel_and_count_ty_refs() which is the current peel_mid_ty_refs_is_mutable: it doesn't do much more work, and it is easy to just ignore the mutability result if we don't need it. WDYT?

I still think the "mid" or "middle" part is not useful, including for a standalone function, as there is no risk to use the wrong type thanks to static typing.

Note that you might want to rewrite the function to be non-recursive for the peace of mind, even though for such a simple tail-recursion textbook case I hope that the compiler will always de-recursify it by itself.

Also, a note indicating that if no refs were removed (the count is zero) then Mutability::Mut will always be returned might be useful in the function documentation.

Or maybe just one function peel_and_count_ty_refs() which is the current peel_mid_ty_refs_is_mutable: it doesn't do much more work, and it is easy to just ignore the mutability result if we don't need it. WDYT?

That feels just a bit clunky, but it's probably fine, yeah.

I still think the "mid" or "middle" part is not useful, including for a standalone function, as there is no risk to use the wrong type thanks to static typing.

Yes, I've removed it from the name^^

Note that you might want to rewrite the function to be non-recursive for the peace of mind

Sure

Also, a note indicating that if no refs were removed (the count is zero) then Mutability::Mut will always be returned might be useful in the function documentation.

Maybe return an Option<Mutability> instead? I see that most of the callers explicitly check ref_depth after calling this function, but some don't, and I'm actually unsure whether that's a conscious decision or an oversight. Unwrapping an Option would be a more explicit display of intent.

Okay so I did go with the Option approach, and there were only two real unclear cases, unused_peekable, and a helper function in manual_utils, used by both manual_filter and manual_map. Fortunately, all of those had some tests fail when I tried rejecting None (no refs peeled), so I knew that all those lint depended on None being accepted.

Overall, I'm quite happy with this approach, but do let me know what you think

samueltardieu · 2025-08-22T17:18:13Z

r? samueltardieu @rustbot author

rustbot · 2025-08-22T17:18:33Z

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

samueltardieu

LGTM, just a possibility of shortening the code (and keeping it as clear IMO), tell me what you want to do.

View changes since this review

samueltardieu · 2025-09-07T08:19:35Z

clippy_utils/src/ty/mod.rs

+        match (mutbl, m) {
+            (None, m) => mutbl = Some(*m),
+            (_, Mutability::Not) => mutbl = Some(Mutability::Not),
+            (_, Mutability::Mut) => {},
        }


You can make this more concise with:

Suggested change

match (mutbl, m) {

(None, m) => mutbl = Some(*m),

(_, Mutability::Not) => mutbl = Some(Mutability::Not),

(_, Mutability::Mut) => {},

}

mutbl.replace(mutbl.map_or(*m, |x: Mutability| x.min(*m)));

(and probably get rid of the type by initializing mutbl with None::<Mutability>)

But if you prefer to keep the current form that's fine too, just tell me.

Wow that's terse... But after dabbling with it a bit, I guess that is what my implementation basically boils down to. It kind of looks like Option needs an Entry API, but that would be extremely silly of course.

Oh and I think having the type ascription in the closure helps a bit to not get lost in all the mutbls, so I left it inside.

samueltardieu · 2025-09-07T08:29:28Z

clippy_utils/src/ty/mod.rs

+/// Peels off all references on the type. Returns the underlying type, the number of references
+/// removed, and, if there were any such references, whether the pointer is ultimately mutable or
+/// not.
+pub fn peel_and_count_ty_refs(mut ty: Ty<'_>) -> (Ty<'_>, usize, Option<Mutability>) {


I wonder if a return type of Option<(Ty<'_>, NonZeroUsize, Mutability)> would not be more "logical" here, but that may be an overkill and less readable.

I was thinking of (Ty<'_>, Option<(NonZeroUsize, Mutability)>) myself, but yeah I think it'd only be more annoying to deal with for most of the callers -- which I guess would speak for having two functions after all, each with the return type more fitting to the use case

ada4a · 2025-09-09T20:18:17Z

@rustbot ready

samueltardieu

Looks good to me except for the commits split.

View changes since this review

samueltardieu · 2025-09-12T15:13:40Z

clippy_utils/src/ty/mod.rs

 }

 /// Returns the base type for HIR references and pointers.
 pub fn walk_ptrs_hir_ty<'tcx>(ty: &'tcx hir::Ty<'tcx>) -> &'tcx hir::Ty<'tcx> {


Since you end up renaming then removing this function, could you please cleanup the commit stack so that those operations are replaced by just removing the function? No need to complicate things when looking at the history.

I don't know what tool you're using to manage your repository (I use jj, I was also using git branchless at some point), but you should take all your changes and organize them afterwards into several commits, rather than keep details of the development process in the final submission.

Here, in this case, I think the history would better as just one single squashed commit because everything makes sense at once or, at most, two commits with the cleanup of let prefix = match mutability { … }; as a separate commit but I'm not sure that deserves it as the code around has been modified as well.

I don't know what tool you're using to manage your repository (I use jj, I was also using git branchless at some point), but you should take all your changes and organize them afterwards into several commits, rather than keep details of the development process in the final submission.

Here, in this case, I think the history would better as just one single squashed commit because everything makes sense at once or, at most, two commits

I generally agree, but honestly it's quite keep track of all the confusingly similarly called functions either way... I did squash the commits together, but added a description of all the moves into the commit body.

- give `ty::walk_ptrs_hir_ty` a more standard name `peel_hir_ty_refs_and_ptrs` - move it out of `ty`, since that's for `middle::ty::Ty` - remove `ty::walk_ptrs_ty_depth` for equivalent `peel_middle_ty_refs` - rename the latter into `ty::peel_and_count_ty_refs` - incorporate mutability tracking (from `ty::peel_mid_ty_refs_is_mutable`) into that

rustbot · 2025-09-12T16:14:04Z

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.

ada4a · 2025-09-12T16:15:48Z

Also it looks like in 473fffb, I wanted to move walk_ptrs_hir_ty in out of ty, removed it, but forgot to add in the new place. Should be fixed now

rustbot assigned Alexendoo Aug 19, 2025

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

ada4a commented Aug 19, 2025

View reviewed changes

clippy_utils/src/ty/mod.rs Outdated Show resolved Hide resolved

ada4a force-pushed the peel_ptrs branch from 73c2bf5 to 5c45313 Compare August 19, 2025 13:41

This comment has been minimized.

Sign in to view

ada4a force-pushed the peel_ptrs branch from 5c45313 to cb0298b Compare August 19, 2025 13:44

ada4a force-pushed the peel_ptrs branch 3 times, most recently from 5e9ebc1 to be3f4a8 Compare August 22, 2025 13:33

samueltardieu reviewed Aug 22, 2025

View reviewed changes

rustbot assigned samueltardieu and unassigned Alexendoo Aug 22, 2025

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 Aug 22, 2025

This comment has been minimized.

Sign in to view

ada4a force-pushed the peel_ptrs branch from be3f4a8 to 1ce2042 Compare August 24, 2025 06:21

This comment has been minimized.

Sign in to view

ada4a force-pushed the peel_ptrs branch from 1ce2042 to 82484c8 Compare September 6, 2025 21:40

This comment has been minimized.

Sign in to view

samueltardieu requested changes Sep 7, 2025

View reviewed changes

ada4a force-pushed the peel_ptrs branch from 82484c8 to a7aed64 Compare September 7, 2025 09:18

rustbot removed the S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) label Sep 9, 2025

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

samueltardieu requested changes Sep 12, 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 12, 2025

ada4a added 2 commits September 12, 2025 17:54

other clean-up

68a3ada

ada4a force-pushed the peel_ptrs branch from a7aed64 to 31c7e62 Compare September 12, 2025 16:14

samueltardieu approved these changes Sep 12, 2025

View reviewed changes

samueltardieu added this pull request to the merge queue Sep 12, 2025

Merged via the queue into rust-lang:master with commit 9f02cbc Sep 12, 2025
11 checks passed

ada4a deleted the peel_ptrs branch September 12, 2025 17:04

Uh oh!

clippy_utils: make peel_*_ty_refs class of functions a bit more consistent #15515

clippy_utils: make peel_*_ty_refs class of functions a bit more consistent #15515

Uh oh!

Conversation

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

Uh oh!

Uh oh!

rustbot commented Aug 19, 2025

Uh oh!

Uh oh!

This comment has been minimized.

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

samueltardieu commented Aug 22, 2025

Uh oh!

ada4a commented Aug 22, 2025

Uh oh!

samueltardieu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

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!

samueltardieu commented Aug 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Aug 22, 2025

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

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!

Choose a reason for hiding this comment

Uh oh!

ada4a commented Sep 9, 2025

Uh oh!

samueltardieu left a comment • 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!

rustbot commented Sep 12, 2025

Uh oh!

ada4a commented Sep 12, 2025

Uh oh!

Uh oh!

`clippy_utils`: make `peel_*_ty_refs` class of functions a bit more consistent #15515

`clippy_utils`: make `peel_*_ty_refs` class of functions a bit more consistent #15515

ada4a commented Aug 19, 2025 •

edited

Loading

ada4a commented Aug 19, 2025 •

edited

Loading

ada4a commented Aug 19, 2025 •

edited

Loading

samueltardieu commented Aug 22, 2025 •

edited

Loading

samueltardieu left a comment •

edited by rustbot

Loading

samueltardieu left a comment •

edited

Loading