Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

filter_map_identity is arguably overzealous #9377

Open
scottmcm opened this issue Aug 25, 2022 · 1 comment
Open

filter_map_identity is arguably overzealous #9377

scottmcm opened this issue Aug 25, 2022 · 1 comment
Labels
I-false-positive Issue: The lint was triggered on code it shouldn't have

Comments

@scottmcm
Copy link
Member

scottmcm commented Aug 25, 2022 •
edited

Description

lint: https://rust-lang.github.io/rust-clippy/master/index.html#filter_map_identity

This was originally added as being like flat_map_identity (#6685), but they're not quite analogous. .flatten() is unambiguously better than .flat_map(identity), but the same isn't true for .filter_map(identity).

The problem is that (as I recently mentioned in rust-lang/rust#99230 (comment)), FlatMap (including Flatten) is a fundamentally harder problem than FilterMap, because it needs to deal in iterators that might have more than one item. And thus, for example,

let flatten_it = a.iter().copied().flatten();
let filter_map_it = a.iter().copied().filter_map(|x| x);
dbg!([size_of_val(&flatten_it), size_of_val(&filter_map_it)]); // 32, 16
dbg!([flatten_it.size_hint(), filter_map_it.size_hint()]); // (0, None), (0, Some(3))

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=f1ccc42f95540d435e6d7caa58d29ed8

So perhaps this shouldn't be warn-by-default when it would also be plausible to have a performance-category lint to say to do exactly the opposite.

Version

(Not really version-specific; inherent to the definition of the lint.)

Additional Labels

@rustbot label +I-false-positive
@rustbot rustbot added the I-false-positive Issue: The lint was triggered on code it shouldn't have label Aug 25, 2022
@scottmcm scottmcm mentioned this issue Aug 25, 2022
Closed
@cuviper cuviper mentioned this issue Feb 16, 2024
Merged
cuviper added a commit to cuviper/rust that referenced this issue Feb 16, 2024
@cuviper
Specialize flattening iterators with only one inner item
974bc45 
For iterators like `Once` and `option::IntoIter` that only ever have a
single item at most, the front and back iterator states in `FlatMap` and
`Flatten` are a waste, as they're always consumed already. We can use
specialization for these types to simplify the iterator methods.

It's a somewhat common pattern to use `flatten()` for options and
results, even recommended by [multiple][1] [clippy][2] [lints][3]. The
implementation is more efficient with `filter_map`, as mentioned in
[clippy#9377], but this new specialization should close some of that
gap for existing code that flattens.

[1]: https://rust-lang.github.io/rust-clippy/master/#filter_map_identity
[2]: https://rust-lang.github.io/rust-clippy/master/#option_filter_map
[3]: https://rust-lang.github.io/rust-clippy/master/#result_filter_map
[clippy#9377]: rust-lang/rust-clippy#9377
bors added a commit to rust-lang-ci/rust that referenced this issue Feb 16, 2024
@bors
Auto merge of rust-lang#121204 - cuviper:flatten-one-shot, r=<try>
4b67ea2 
Specialize flattening iterators with only one inner item

For iterators like `Once` and `option::IntoIter` that only ever have a
single item at most, the front and back iterator states in `FlatMap` and
`Flatten` are a waste, as they're always consumed already. We can use
specialization for these types to simplify the iterator methods.

It's a somewhat common pattern to use `flatten()` for options and
results, even recommended by [multiple][1] [clippy][2] [lints][3]. The
implementation is more efficient with `filter_map`, as mentioned in
[clippy#9377], but this new specialization should close some of that
gap for existing code that flattens.

[1]: https://rust-lang.github.io/rust-clippy/master/#filter_map_identity
[2]: https://rust-lang.github.io/rust-clippy/master/#option_filter_map
[3]: https://rust-lang.github.io/rust-clippy/master/#result_filter_map
[clippy#9377]: rust-lang/rust-clippy#9377
bors added a commit to rust-lang-ci/rust that referenced this issue Feb 17, 2024
@bors
Auto merge of rust-lang#121204 - cuviper:flatten-one-shot, r=the8472
6672c16 
Specialize flattening iterators with only one inner item

For iterators like `Once` and `option::IntoIter` that only ever have a
single item at most, the front and back iterator states in `FlatMap` and
`Flatten` are a waste, as they're always consumed already. We can use
specialization for these types to simplify the iterator methods.

It's a somewhat common pattern to use `flatten()` for options and
results, even recommended by [multiple][1] [clippy][2] [lints][3]. The
implementation is more efficient with `filter_map`, as mentioned in
[clippy#9377], but this new specialization should close some of that
gap for existing code that flattens.

[1]: https://rust-lang.github.io/rust-clippy/master/#filter_map_identity
[2]: https://rust-lang.github.io/rust-clippy/master/#option_filter_map
[3]: https://rust-lang.github.io/rust-clippy/master/#result_filter_map
[clippy#9377]: rust-lang/rust-clippy#9377
github-actions bot pushed a commit to rust-lang/miri that referenced this issue Feb 18, 2024
@bors
Auto merge of #121204 - cuviper:flatten-one-shot, r=the8472
bf4d531 
Specialize flattening iterators with only one inner item

For iterators like `Once` and `option::IntoIter` that only ever have a
single item at most, the front and back iterator states in `FlatMap` and
`Flatten` are a waste, as they're always consumed already. We can use
specialization for these types to simplify the iterator methods.

It's a somewhat common pattern to use `flatten()` for options and
results, even recommended by [multiple][1] [clippy][2] [lints][3]. The
implementation is more efficient with `filter_map`, as mentioned in
[clippy#9377], but this new specialization should close some of that
gap for existing code that flattens.

[1]: https://rust-lang.github.io/rust-clippy/master/#filter_map_identity
[2]: https://rust-lang.github.io/rust-clippy/master/#option_filter_map
[3]: https://rust-lang.github.io/rust-clippy/master/#result_filter_map
[clippy#9377]: rust-lang/rust-clippy#9377
@cuviper
Copy link
Member

cuviper commented Feb 18, 2024

FWIW, nightly flatten now fares a little better in that example. The size_of difference is still there, but it uses a specialized implementation, including a size hint as good as filter_map's.

@antonilol antonilol mentioned this issue Apr 9, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
I-false-positive Issue: The lint was triggered on code it shouldn't have
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@cuviper @scottmcm @rustbot