Skip to content

Fix redundant_closure_for_method_calls suggesting an ambiguous method path - #17750

Open
Itz-Agasta wants to merge 1 commit into
rust-lang:masterfrom
Itz-Agasta:fix-17730-ambiguous-method-path
Open

Itz-Agasta wants to merge 1 commit into
rust-lang:masterfrom
Itz-Agasta:fix-17730-ambiguous-method-path

Conversation

@Itz-Agasta

Copy link
Copy Markdown

copied is defined by an inherent impl on both Option<&T> & Option<&mut T>, so the suggested Option::copied path has nothing to pick between them and fails with E0034. It reduces to:

fn main() {
    let x: Option<Option<&i32>> = None;
    let _ = x.map(|o| o.copied());
}

The cause is in get_path_to_ty, where the Adt arm dropped the impl's generic arguments while the Ref/Slice/Tuple/Dynamic arms next to it instantiate theirs, so impl<T: Copy> Option<&T> printed as plain Option....I checked it, It isn't option specific: any type with two inherent impls differing in the shape of a generic argument hits it, i mean Option::cloned, Result::copied and Result::cloned all do.

fixes #17730

changelog: Fix [redundant_closure_for_method_calls] suggesting a method path that does not resolve

@rustbot rustbot added S-waiting-on-community-reviews Status: This is awaiting for positive reviews from the community before a maintainer is assigned. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Sep 17, 2026
@rustbot

rustbot commented Sep 17, 2026

Copy link
Copy Markdown
Collaborator

Thanks for the pull request, and welcome!

You should hear from one of our reviewers after this PR gets at least 2 reviews from the community.

Please see the contribution instructions for more information.

@Itz-Agasta

Itz-Agasta commented Sep 17, 2026

Copy link
Copy Markdown
Author

btw i tried the turbofish route first (Option::<&i32>::copied) . But the generic args need to be printable from the call site, and rustc prints them with crate-root paths. so types defined locally come out as W::scope::Inner::get which doesn't resolve.

doing it right means writing a scope-aware type printer ig, which basically duplicates rustc internals. Too much churn for this patch in my opinion :( now idk maybe that will be a better way to handel as this pr is somewhat patch type.

@CommanderStorm CommanderStorm left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Community review: Nice work. LGTM.

I have two minor refactorig requests that I think applying would make this cleaner for the next person.. 👉🏻👈🏻
If you do decide to include them, please make sure to squash things into one commit ^^

View changes since this review

Comment thread clippy_utils/src/lib.rs Outdated
Comment thread tests/ui/eta.fixed Outdated
path

`get_path_to_ty` discarded the generic arguments of an inherent impl's
self type, so a method defined by several impls differing only in those
arguments was suggested as a bare `Adt::method` path. Applying the
suggestion fails with E0034 because a path carries no receiver to
disambiguate.

Detect that case and make no suggestion. Names defined by a single
inherent impl are unaffected.
@Itz-Agasta
Itz-Agasta force-pushed the fix-17730-ambiguous-method-path branch from 4cc5fa7 to 5e397df Compare September 17, 2026 13:44
@github-actions

Copy link
Copy Markdown

Lintcheck changes for 5e397df

Lint Added Removed Changed
clippy::redundant_closure_for_method_calls 0 1 0

This comment will be updated if you push new changes

@Itz-Agasta

Itz-Agasta commented Sep 17, 2026

Copy link
Copy Markdown
Author

thanks @CommanderStorm both applied and squashed, pls have a look

btw yk something is bothering me a bit with the design of "don't suggest when ambiguous"....like lets say user has this

fn f(x: Option<Option<&i32>>) -> Option<Option<i32>> {
    x.map(|o| o.copied())
}

they won't get any complaint, but the closure is really redundant imo. i personally feel clippy should suggest the cleaner version:

x.map(Option::<&i32>::copied)

i tried generating the turbofish automatically but its kinda hard.... the generic args need to be nameable from the call site, and Ty::to_string() gives crate-root-relative paths. so types from other modules can come out wrong, and closure types don't have valid syntax at all
i did try building that version and it was getting into "scope-aware type printer" territory, which felt like huge work needs its own PR. and since the suggestion is MachineApplicable, getting it even slightly wrong means cargo clippy --fix can silently break people's code

so im kinda confused.... what y think?

ps: hey frank, i was in this year's gsoc with osm, working on the geocoder nominatim. saw u there as a mentor, nice to see u here too :)

@CommanderStorm

Copy link
Copy Markdown
Contributor

something is bothering me a bit with the design of "don't suggest when ambiguous"....like lets say user has this

So the general preference is that bad suggestions are less good than false positives than false negatives.
Therefore, having an false negative is not ideal, but much better 😉

We don't need to do this this way though as you noted.

tried generating the turbofish automatically but its kinda hard.... the generic args need to be nameable from the call site, and Ty::to_string() gives crate-root-relative paths. so types from other modules can come out wrong, and closure types don't have valid syntax at all.
I did try building that version and it was getting into "scope-aware type printer" territory, which felt like huge work needs its own PR. and since the suggestion is MachineApplicable, getting it even slightly wrong means cargo clippy --fix can silently break people's code

I would need to look into this. I am like 60% sure that that should already exist somewhere.
Getting the nameable path (as you noted you can fuze of an module via pub(super/...)) seems like something where we should have utilities for.

But the good thing is that those are also not required. (assuming this is actually not done yet)

We don't need to be MachineApplicable for all cases, we can also degrade to Applicability::HasPlaceholders which we can use here for the time beeing.
So no false negative, but also no bad MachineApplicable suggestion 😆

docs:

The suggestion contains placeholders like (...) or { /* fields */ }.
The suggestion cannot be applied automatically because it will not result in valid Rust code. The user will need to fill in the placeholders.

@CommanderStorm

CommanderStorm commented Sep 17, 2026

Copy link
Copy Markdown
Contributor

i was in this year's gsoc with osm

Thank you for your work! ❤️
When I talked with Sarah in Paris she was very enthusiastic recomended me to actually read your article. Really cool stuff and very well written!
I am patiently waiting to use that on my unis' small little roomfinder deployment of nominatim :)

(For the others, here is the blog post: https://nominatim.org/2026/09/01/gsoc-2026-nominatim-categories.html )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-community-reviews Status: This is awaiting for positive reviews from the community before a maintainer is assigned. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties

Projects

None yet

Development

Successfully merging this pull request may close these issues.

redundant_closure_for_method_calls suggestion causes multiple applicable items in scope

3 participants