Allow `filter_map_identity` when the closure is typed #12562

m-rph · 2024-03-25T22:33:22Z

This extends the filter_map_identity lint to support typed closures.

For untyped closures, we know that the program compiles, and therefore we can safely suggest using flatten.

For typed closures, they may participate in type resolution. In this case we use Applicability::MaybeIncorrect.

Details:
https://rust-lang.zulipchat.com/#narrow/stream/257328-clippy/topic/Should.20.60filter_map_identity.60.20lint.20when.20closures.20are.20typed.3F

changelog: filter_map_identity will now suggest using flatten for typed closures.

r? @y21 && @Centri3

This extends the `filter_map_identity` lint to support typed closures. For untyped closures, we know that the program compiles, and therefore we can safely suggest using flatten. For typed closures, they may participate in type resolution. In this case we use `Applicability::MaybeIncorrect`. Details: https://rust-lang.zulipchat.com/#narrow/stream/257328-clippy/topic/Should.20.60filter_map_identity.60.20lint.20when.20closures.20are.20typed.3F

y21 · 2024-03-26T08:51:16Z

Is there a reason that we'd only want to do this for filter_map_identity, and not for all callers of is_expr_untyped_identity_function?

But also, are we sure that type annotations can always be "reasonably" specified elsewhere when the closure is removed? I'm having a hard time coming up with a practical example where you can't. I think there's usually always somewhere where the type could be specified to make type inference happy. E.g.

fn accepts_iter(_: impl Iterator) {}

accepts_iter(iter::empty().map(|v: i32| v));

Removing the .map() as map_identity would suggest (if we made this change there as well) would break type inference, but the user can just write iter::empty::<i32>().

#9122 is a more practical FP where these "untyped" functions were introduced, but even here one can write Ok::<_, std::io::Error>(())

m-rph · 2024-03-26T09:02:55Z

Is there a reason that we'd only want to do this for filter_map_identity, and not for all callers of is_expr_untyped_identity_function?

I was thinking we could introduce this change gradually, starting from less error prone cases such as this one, and slowly moving into more common ones while accounting for issues that can occur such as the one you've presented.

are we sure that type annotations can always be "reasonably" specified elsewhere when the closure is removed?

I have a hard time coming up with situations where this is exceptionally hard to be honest.

Even in the tests that I have added where I'd have expected the identity function to actually help with inference, rustc flatout refused to compile it, though that is certainly different to the map case.

For map specifically, we could perhaps suggest adding the type as in iter::empty::<i32>() ?

y21

looks good to me with the nit fixed, @Centri3 does this make sense to you as well?

y21 · 2024-03-26T17:46:12Z

clippy_lints/src/methods/filter_map_identity.rs

+        return Some(Applicability::MachineApplicable);
+    }
+    if is_expr_identity_function(cx, expr) {
+        return Some(Applicability::MaybeIncorrect);


This should be Unspecified (or HasPlaceholders).

MaybeIncorrect suggestions should still compile.

I've interpreted "valid Rust code" as it parsing correctly, everything else can fail I'm pretty sure

I remember @xFrednet commenting about it here

But oh well, the fact that we have a different understanding is all the more reason that the documentation/naming for Applicability could be better

rustfix somewhere has an issue to rename these variants to make the difference clearer. I think it's mostly stuck on finding better names:

rust-lang/cargo#13028

y21 · 2024-03-26T17:55:41Z

For map specifically, we could perhaps suggest adding the type as in iter::empty::<i32>() ?

I'm not sure if special casing fn calls as the .map() receiver (which is only one out of many possible ways where the inference variable could have possibly come from) is worth the complexity that that adds, especially when the inferred type is not as trivial. E.g.

<std::iter::Empty<Option<_>> as Default>::default().flat_map(|v: Option<i32>| v)
//                       ^ i32 needs to go here

…tity`

Centri3

I don't really think we should pass this as is without some kind of backwards heuristic to type_certainty. I have no problems with the code itself, but if possible we should extend type_certainty to either look "inwards" (as it does currently), or "outwards".

I've seen FPs in the past caused by this, MREs will probably look contrived but this definitely can (and will) come up often in practice.

Let's at least run lintcheck on a lot of popular crates before we do merge as is, it'd be nice if it can select a lot of random ones in the top 1000

m-rph · 2024-03-26T22:35:25Z

fwiw, running cargo lintcheck doesn't introduce any additional lints going from master to this PR. I will let it run with the --recursive option tonight.

I think we should first extend cargo lintcheck with some more crates before we take this any further.

y21 · 2024-03-26T22:43:13Z

you can run cargo r --bin popular-crates lintcheck_crates.toml -n <number_of_crates> in the lintcheck dir locally to populate the toml file with the top n crates (but yes I agree we should definitely add more crates to the default lintcheck_crates.toml, because those 25 crates that it by default has aren't very useful on their own, not sure if we want to block this PR on that tho since it's not really a blocker?)

Centri3 · 2024-03-26T23:44:54Z

I didn't know about that, though it still would be nice if it could choose them random (otherwise we're checking the same crates everytime)

m-rph · 2024-03-28T23:01:32Z

What I did:

Downloaded top 200 crates via cargo r --bin popular-crates lintcheck_crates.toml -n 200
Ran cargo lintcheck
Switched to PR: git switch 12501
Ran cargo lintcheck again

Result:

Stats:

((empty))

Thus no errors were introduced from this.

Should I increase the number to say top 500?

Centri3 · 2024-03-28T23:32:57Z

Top 200 is probably enough for now.

m-rph · 2024-03-28T23:39:28Z

Is the new applicability okay?

y21 · 2024-03-29T12:20:54Z

Yeah, looks fine to me. I'd say its ready to go, but I'll wait a bit to see if others have something to say

xFrednet · 2024-03-29T12:39:48Z

The applicability also looks good to me(I haven't looked at anything else)

y21 · 2024-03-31T11:41:03Z

Lets move this forward, thanks! LGTM

@bors r+

bors · 2024-03-31T11:41:05Z

📌 Commit 60937bf has been approved by y21

It is now in the queue for this repository.

bors · 2024-03-31T11:42:12Z

⌛ Testing commit 60937bf with merge 95338e7...

Allow `filter_map_identity` when the closure is typed This extends the `filter_map_identity` lint to support typed closures. For untyped closures, we know that the program compiles, and therefore we can safely suggest using flatten. For typed closures, they may participate in type resolution. In this case we use `Applicability::MaybeIncorrect`. Details: https://rust-lang.zulipchat.com/#narrow/stream/257328-clippy/topic/Should.20.60filter_map_identity.60.20lint.20when.20closures.20are.20typed.3F changelog: `filter_map_identity` will now suggest using flatten for typed closures. r? `@y21` && `@Centri3`

bors · 2024-03-31T11:50:50Z

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: y21
Pushing 95338e7 to master...

bors · 2024-03-31T11:51:47Z

👀 Test was successful, but fast-forwarding failed: 422 1 review requesting changes and 1 approving review by reviewers with write access.

xFrednet · 2024-03-31T11:59:28Z

@bors retry

Allow `filter_map_identity` when the closure is typed This extends the `filter_map_identity` lint to support typed closures. For untyped closures, we know that the program compiles, and therefore we can safely suggest using flatten. For typed closures, they may participate in type resolution. In this case we use `Applicability::MaybeIncorrect`. Details: https://rust-lang.zulipchat.com/#narrow/stream/257328-clippy/topic/Should.20.60filter_map_identity.60.20lint.20when.20closures.20are.20typed.3F changelog: `filter_map_identity` will now suggest using flatten for typed closures. r? `@y21` && `@Centri3`

bors · 2024-03-31T12:00:35Z

⌛ Testing commit 60937bf with merge ecca4e3...

bors · 2024-03-31T12:08:56Z

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: y21
Pushing ecca4e3 to master...

bors · 2024-03-31T12:09:54Z

👀 Test was successful, but fast-forwarding failed: 422 1 review requesting changes and 1 approving review by reviewers with write access.

Let's see if this blocks bors?

xFrednet · 2024-03-31T12:14:56Z

Third time's the charm?

@bors retry

bors · 2024-03-31T12:16:03Z

⌛ Testing commit 60937bf with merge 797d50d...

bors · 2024-03-31T12:24:50Z

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: y21
Pushing 797d50d to master...

rustbot assigned y21 Mar 25, 2024

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

m-rph force-pushed the 12501 branch from a682048 to 9655231 Compare March 25, 2024 22:42

y21 approved these changes Mar 26, 2024

View reviewed changes

:adjust applicability for typed identity closures in `filter_map_iden…

60937bf

…tity`

Centri3 previously requested changes Mar 26, 2024

View reviewed changes

m-rph requested review from y21, Centri3 and xFrednet March 29, 2024 11:01

y21 approved these changes Mar 31, 2024

View reviewed changes

bors merged commit 797d50d into rust-lang:master Mar 31, 2024
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow `filter_map_identity` when the closure is typed #12562

Allow `filter_map_identity` when the closure is typed #12562

m-rph commented Mar 25, 2024 •

edited

y21 commented Mar 26, 2024 •

edited

m-rph commented Mar 26, 2024 •

edited

y21 left a comment

y21 Mar 26, 2024

Centri3 Mar 26, 2024

y21 Mar 26, 2024

xFrednet Mar 27, 2024

y21 commented Mar 26, 2024

Centri3 left a comment •

edited

m-rph commented Mar 26, 2024 •

edited

y21 commented Mar 26, 2024 •

edited

Centri3 commented Mar 26, 2024

m-rph commented Mar 28, 2024 •

edited

Centri3 commented Mar 28, 2024

m-rph commented Mar 28, 2024

y21 commented Mar 29, 2024

xFrednet commented Mar 29, 2024

y21 commented Mar 31, 2024

bors commented Mar 31, 2024

bors commented Mar 31, 2024

bors commented Mar 31, 2024

bors commented Mar 31, 2024

xFrednet commented Mar 31, 2024

bors commented Mar 31, 2024

bors commented Mar 31, 2024

bors commented Mar 31, 2024

xFrednet commented Mar 31, 2024

bors commented Mar 31, 2024

bors commented Mar 31, 2024

Allow filter_map_identity when the closure is typed #12562

Allow filter_map_identity when the closure is typed #12562

Conversation

m-rph commented Mar 25, 2024 • edited

y21 commented Mar 26, 2024 • edited

m-rph commented Mar 26, 2024 • edited

y21 left a comment

Choose a reason for hiding this comment

y21 Mar 26, 2024

Choose a reason for hiding this comment

Centri3 Mar 26, 2024

Choose a reason for hiding this comment

y21 Mar 26, 2024

Choose a reason for hiding this comment

xFrednet Mar 27, 2024

Choose a reason for hiding this comment

y21 commented Mar 26, 2024

Centri3 left a comment • edited

Choose a reason for hiding this comment

m-rph commented Mar 26, 2024 • edited

y21 commented Mar 26, 2024 • edited

Centri3 commented Mar 26, 2024

m-rph commented Mar 28, 2024 • edited

Centri3 commented Mar 28, 2024

m-rph commented Mar 28, 2024

y21 commented Mar 29, 2024

xFrednet commented Mar 29, 2024

y21 commented Mar 31, 2024

bors commented Mar 31, 2024

bors commented Mar 31, 2024

bors commented Mar 31, 2024

bors commented Mar 31, 2024

xFrednet commented Mar 31, 2024

bors commented Mar 31, 2024

bors commented Mar 31, 2024

bors commented Mar 31, 2024

xFrednet commented Mar 31, 2024

bors commented Mar 31, 2024

bors commented Mar 31, 2024

Allow `filter_map_identity` when the closure is typed #12562

Allow `filter_map_identity` when the closure is typed #12562

m-rph commented Mar 25, 2024 •

edited

y21 commented Mar 26, 2024 •

edited

m-rph commented Mar 26, 2024 •

edited

Centri3 left a comment •

edited

m-rph commented Mar 26, 2024 •

edited

y21 commented Mar 26, 2024 •

edited

m-rph commented Mar 28, 2024 •

edited