Skip to content

Conversation

@ada4a
Copy link
Contributor

@ada4a ada4a commented Nov 5, 2025

Fixes #16031

changelog: [manual_map]: lint exprs with adjustments, but with reduced Applicability
changelog: [manual_filter]: lint exprs with adjustments, but with reduced Applicability

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Nov 5, 2025
@rustbot
Copy link
Collaborator

rustbot commented Nov 5, 2025

r? @y21

rustbot has assigned @y21.
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
Copy link

github-actions bot commented Nov 5, 2025

Lintcheck changes for 2af95a4

Lint Added Removed Changed
clippy::manual_filter 1 0 0
clippy::manual_map 14 0 0

This comment will be updated if you push new changes

@y21
Copy link
Member

y21 commented Nov 8, 2025

I agree with the idea here, but what do you think about adding a note to the diagnostic if it relies on coercions, like "note: you may need to add an explicit as cast if this match is coerced to another type" (or something like that)?

This would make it clearer that the user might need to make some small manual changes on top of the suggestion, since the applicability isn't directly visible.

@ada4a
Copy link
Contributor Author

ada4a commented Nov 8, 2025

Right -- I just wasn't sure how to smuggle that note from manual_utils to manual_map (and manual_filter, probably?). But in the meantime, I did find a reasonably good solution I think.

There's just one more thing: most of the time (maybe? That's what the original issue used as an example at least), as casts aren't even needed, just some refs/derefs -- would you maybe know of a way to incorporate that into the note?

One solution would be to just list every (?) possible coercion:

note: you may need to add explicit as casts/references/dereferences if this match is coerced to another type

Another solution could be to make expr_requires_coercion actually return the list of coercions, and adjust the note based on that. Though maybe at that point we might just perform those adjustments ourselves?..

WDYT?

@y21
Copy link
Member

y21 commented Nov 16, 2025

One solution would be to just list every (?) possible coercion:

note: you may need to add explicit as casts/references/dereferences if this match is coerced to another type

Sounds fine to me to also mention dereferences, but do you have an example where only adding a reference is necessary? The 3 false negatives in #16031 seem like they wouldn't need any extra code changes at all AFAICT. I can come up with cases where adding dereferences or casts/return type annotations on the closure are necessary, but not references.

Other than that, this note seems like a good middle ground to me.

@ada4a ada4a changed the title manual_map: lint exprs with adjustments, but with reduced Applicability manual_{map,filter}: lint exprs with adjustments, but with reduced Applicability Nov 17, 2025
@ada4a ada4a force-pushed the manual_map branch 2 times, most recently from ca2ab86 to 01ec211 Compare November 17, 2025 07:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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.

False negatives on manual_map (regressions since 1.86)

3 participants