Suggest replacing filter-for-some-and-unwrap with .flatten

[NoraCodes]

What it does

Suggests replacing code that filters with Option::is_some and then maps Option::unwrap with .flatten() which does the same thing due to Option implementing IntoIterator.

Categories (optional)

• Kind: clippy::style

Advantage

It is one function call fewer and makes it clearer what is happening - one level of indirection is being removed. In addition, many codebases want to eliminate all possible panic sources, of which Option::unwrap is one, although in this case it is statically known not to panic.

Drawbacks

None.

Example

fn odds_out(x: i32) -> Option<i32> { if x % 2 == 0 { Some(x) } else { None } }
let evens: Vec<_> = myints.iter().map(odds_out).filter(Option::is_some).map(Option::unwrap).collect();

Could be written as:

fn odds_out(x: i32) -> Option<i32> { if x % 2 == 0 { Some(x) } else { None } }
let evens: Vec<_> = myints.iter().map(odds_out).flatten().collect();

[bbqbaron]

Heyo, could I take a crack at this? New Rust user, but I certainly understand the intent here so hopefully should be able to suss it out.

Shouldn't the recommendation itself be linted to use filter_map?

[bbqbaron]

Shouldn't the recommendation itself be linted to use filter_map?

My (newbie) understanding is that .filter_map would require some closure arg |x| x to return the option x, whereas .flatten just converts the options to to iters, which automatically removes any empty ones. May be a matter of taste, but .flatten seems more direct.

I meant replacing the map(..).flatten(..) chain with filter_map. In fact we already have a pedantic lint, clippy::map_flatten, which triggers on the recommendation.

Playground example

[bbqbaron]

I meant replacing the map(..).flatten(..) chain with filter_map. In fact we already have a pedantic lint, clippy::map_flatten, which triggers on the recommendation.

Playground example

Ah, that makes sense. I wonder if maybe this example is a red herring, since you might chain filter into map given any iterable of options, even one you didn't construct. Ie, the map(odds_out) could be seen as just a contrived part of the setup, not the "target code" being checked?

I leave it to maintainers' judgment, since they know better than I what code people tend to write often enough to be worth checking, and how exhaustive we want our list of cases to be.

[flip1995]

So on the one hand @mikerite is right, that map(..).flatten(..) gets linted by Clippy, so the suggested code in the above example would again get linted. But the filter(is_some).map(unwrap) construct doesn't have to follow a .map(..) call in the iterator chain. So suggesting to use flatten(..) in this case is valid, IMO.

But rather than implementing this as a new lint, I just would enhance the filter_map lint with that special case.

[mlegner]

@flip1995 So you suggest to check for both .map(..).filter(is_some).map(unwrap) and .filter(is_some).map(unwrap) and suggest .filter_map(..) and .flatten(), respectively? Currently, only chains of two methods are considered in the lint. So simply checking for .filter(is_some).map(unwrap) would fit the current structure better but would potentially require the programmer to change their code twice.

@bbqbaron Are you still working on this?

[bbqbaron]

@mlegner I've been following the convo. I got a hacky implementation working when I first picked it up, just to get the project running, and then was waiting for this desired behavior question to shake out.

[flip1995]

So you suggest to check for both .map(..).filter(is_some).map(unwrap) and .filter(is_some).map(unwrap) and suggest .filter_map(..) and .flatten(), respectively?

Nope, I'm suggesting only linting .filter(is_some).map(unwrap) and let the other lint handle the map() case, if applicable. Linting applied suggestions is totally fine for Clippy.