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.

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

Add find_map to the Iterator trait #1801

Closed
Cobrand opened this issue Nov 26, 2016 · 15 comments
Closed

Add find_map to the Iterator trait #1801

Cobrand opened this issue Nov 26, 2016 · 15 comments
Labels
T-libs-api Relevant to the library API team, which will review and decide on the RFC.

Comments

@Cobrand
Copy link

Cobrand commented Nov 26, 2016

This is a very simple proposition to add a find_map trait method to the Iterator trait.
It would look like this :

fn find_map<B, P>(&mut self, predicate: P) -> Option<B> where P: FnMut(&Self::Item) -> Option<B>;

Currently to reproduce similar behavior, one would have to use a combination of filter_map and next. It is obvious for the Rust connoisseur that is aware that an Iterator is lazy and will not do anything unless consumed, but it much less obvious for the standard programmer that doesn't think this way yet.

Let's take an example :

#[derive(Debug,Copy,Clone)]
enum Flavor {
    Chocolate,
    Strawberry,
    Cherry
}

#[derive(Debug,Copy,Clone)]
enum Dessert {
    Banana,
    Pudding,
    Cake(Flavor)
}

fn main() {
    let desserts_of_the_week = vec![
        Dessert::Banana,
        Dessert::Cake(Flavor::Chocolate),
        Dessert::Pudding,
        Dessert::Pudding,
        Dessert::Pudding,
    ];
}

If I want to have the "flavor of the cake of the week", as an Option<Flavor>, a non-newbie Rust user will use this :

let cake_of_the_week : Option<Flavor> = desserts_of_the_week.iter().filter_map(|dessert|{
        match dessert {
            &Dessert::Cake(ref flavor) => Some(flavor.clone()),
            _ => None
        }
    }).next();

However a new user will be tempted to do something like this :

let cake_of_the_week : Option<Flavor> = desserts_of_the_week.iter().find(|dessert|{
        match dessert {
            &&Dessert::Cake(_) => true,
            _ => false
        }
    }).map(|dessert|{
        match dessert {
            &Dessert::Cake(ref flavor) => flavor.clone(),
            _ => unreachable!()
        }
    });

That is why I propose to add a find_map method to the Iterator trait, which would

  • Reduce a little bit your code and increase readability (in the case of filter_map(...).next()
  • Avoid new users to code in a non-idiomatic way

It would then look like this :

let cake_of_the_week : Option<Flavor> = desserts_of_the_week.iter().find_map(|dessert|{
        match dessert {
            &Dessert::Cake(ref flavor) => Some(flavor.clone()),
            _ => None
        }
    });

There aren't that much advantages, but I still do think it's worth the while to add such a method.

Maybe this has been discussed before, in that case I'd appreciate a link to some discussion related to this topic, because I could not find anything.

Thoughts ?

@nrc nrc added the T-libs-api Relevant to the library API team, which will review and decide on the RFC. label Nov 27, 2016
@bluss
Copy link
Member

bluss commented Nov 29, 2016

The closure should get the iterator element by value. In that shape find_map is a good generalization of find, all, any, position etc, so it is a candidate for being the basis method of all of those (similar to this draft plan, and similar to search_while here (internal method)).

@brendanzab
Copy link
Member

@bluss Perhaps find_map could be trialed on your itertools crate in the meantime?

@bluss
Copy link
Member

bluss commented Nov 29, 2016

Sure. I'm also happy since it's a better alternative to the “search_while” alternative in the draft RFC.

@bluss
Copy link
Member

bluss commented Nov 29, 2016

cc @Veedrac

@Veedrac
Copy link

Veedrac commented Nov 29, 2016

@bluss Lots of iterator methods are already technically equivalent in this sense; all can emulate find_map just as much as the other way around. find_map doesn't strike me as a better basis method; we probably want either simpler (all) or safer (fold_while).

In terms of adding find_map on its own merits, I don't have a strong opinion. It feels better suited to itertools, though.

@bluss
Copy link
Member

bluss commented Nov 29, 2016

Concretely I don't agree.

To implement find_map with all, you have to do some tricky closure capture logic that is not neat. The reverse is trivial.

The same goes for fold_while and all, except the closure capture logic is even worse.

@Veedrac
Copy link

Veedrac commented Nov 29, 2016

Doesn't seem that tricky to me.

let mut ret = None;
iter.all(|x| { ret = predicate(x); !ret.is_some() });
ret

Anyway, in theory people will never have to build on top of an abstraction they don't like - that will be done once in the standard library. From then on the only time they will be required to use all directly is when they are overloading it, in which case the simpler to implement the better, or when it turns out to be the nicest way to express the logic they want.

Using fold_while as the primitive makes every it harder to create a new iterator with fast internal iteration, and only benefits the standard library.

@bluss
Copy link
Member

bluss commented Nov 29, 2016

Yes I agree with that, but using all to make a fold_while or fold is not as simple as that.

@Veedrac
Copy link

Veedrac commented Nov 29, 2016

Oh, yeah I see what you mean for the ones with accumulators.

@bluss
Copy link
Member

bluss commented Dec 2, 2016

@Veedrac Do you want to collaborate on an RFC? I'd prefer either the fold_while, rfold_while (2 methods) or the find_map, rfind_map, fold, rfold (4 method) approaches, with the methods inside the iterator traits.

@Veedrac
Copy link

Veedrac commented Dec 3, 2016

@bluss I'd be happy to help. The draft for fold_while looks pretty solid, and nicer than a 4-method approach. I still think Result is more ergonomic, so I'd push for calling that fold_ok to fix the semantic mismatch.

@bluss
Copy link
Member

bluss commented Dec 3, 2016

Discussion about that side track can continue on the internals forum Pre-RFC: fold_ok is composable internal iteration.

I'm sorry for the disruption @Cobrand!

@scottmcm
Copy link
Member

scottmcm commented Feb 4, 2018

The side track now exists as try_fold as of rust-lang/rust#45594, in case that can unblock this.

@matklad
Copy link
Member

matklad commented Mar 16, 2018

+1

I want this so often that I've even written a PR (rust-lang/rust#49098), and, in the process of writing it up, stumbled upon this issue :)

bors added a commit to rust-lang/rust that referenced this issue Apr 3, 2018
Add Iterator::find_map

I'd like to propose to add `find_map` method to the `Iterator`: an occasionally useful utility, which relates to `filter_map` in the same way that `find` relates to `filter`.

`find_map` takes an `Option`-returning function, applies it to the elements of the iterator, and returns the first non-`None` result. In other words, `find_map(f) == filter_map(f).next()`.

Why do we want to add a function to the `Iterator`, which can be trivially expressed as a combination of existing ones? Observe that `find(f) == filter(f).next()`, so, by the same logic, `find` itself is unnecessary!

The more positive argument is that desugaring of  `find[_map]` in terms of `filter[_map]().next()` is not super obvious, because the `filter` operation reads as if it is applies to the whole collection, although in reality we are interested only in the first element. That is, the jump from "I need a **single** result" to "let's use a function which maps **many** values to **many** values" is a non-trivial speed-bump, and causes friction when reading and writing code.

Does the need for `find_map` arise in practice? Yes!

* Anecdotally, I've more than once searched the docs for the function with `[T] -> (T -> Option<U>) -> Option<U>` signature.
* The direct cause for this PR was [this](https://github.com/rust-lang/cargo/pull/5187/files/1291c50e86ed4b31db0c76de03a47a5d0074bbd7#r174934173) discussion in Cargo, which boils down to "there's some pattern that we try to express here, but current approaches looks non-pretty" (and the pattern is `filter_map`
* There are several `filter_map().next` combos in Cargo: [[1]](https://github.com/rust-lang/cargo/blob/545a4a2c930916cc9c3dc1716fb7a33299e4062b/src/cargo/ops/cargo_new.rs#L585), [[2]](https://github.com/rust-lang/cargo/blob/545a4a2c930916cc9c3dc1716fb7a33299e4062b/src/cargo/core/resolver/mod.rs#L1130), [[3]](https://github.com/rust-lang/cargo/blob/545a4a2c930916cc9c3dc1716fb7a33299e4062b/src/cargo/ops/cargo_rustc/mod.rs#L1086).
* I've also needed similar functionality in `Kotlin` several times. There, it is expressed as `mapNotNull {}.firstOrNull`, as can be seen [here](https://github.com/intellij-rust/intellij-rust/blob/ee8bdb4e073fd07142fc6e1853ca288c57495e69/src/main/kotlin/org/rust/cargo/project/model/impl/CargoProjectImpl.kt#L154), [here](https://github.com/intellij-rust/intellij-rust/blob/ee8bdb4e073fd07142fc6e1853ca288c57495e69/src/main/kotlin/org/rust/lang/core/resolve/ImplLookup.kt#L444) [here](https://github.com/intellij-rust/intellij-rust/blob/ee8bdb4e073fd07142fc6e1853ca288c57495e69/src/main/kotlin/org/rust/ide/inspections/RsLint.kt#L38) and [here](https://github.com/intellij-rust/intellij-rust/blob/ee8bdb4e073fd07142fc6e1853ca288c57495e69/src/main/kotlin/org/rust/cargo/toolchain/RustToolchain.kt#L74) (and maybe in some other cases as well)

Note that it is definitely not among the most popular functions (it definitely is less popular than `find`), but, for example it (in case of Cargo) seems to be more popular than `rposition` (1 occurrence), `step_by` (zero occurrences) and `nth` (three occurrences as `nth(0)` which probably should be replaced with `next`).

Do we necessary need this function in `std`? Could we move it to itertools? That is possible, but observe that `filter`, `filter_map`, `find` and `find_map` together really form a complete table:

|||
|-------|---------|
| filter| find|
|filter_map|find_map|

It would be somewhat unsatisfying to have one quarter of this table live elsewhere :) Also, if `Itertools` adds an `find_map` method, it would be more difficult to move it to std due to name collision.

Hm, at this point I've searched for `filter_map` the umpteenth time, and, strangely, this time I do find this RFC: rust-lang/rfcs#1801. I guess this could be an implementation though? :)

To sum up:

Pro:
  - complete the symmetry with existing method
  - codify a somewhat common non-obvious pattern

Contra:
  - niche use case
  - we can, and do, live without it
@Centril
Copy link
Contributor

Centril commented Apr 26, 2018

The PR was merged so I'm closing this.

@Centril Centril closed this as completed Apr 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-libs-api Relevant to the library API team, which will review and decide on the RFC.
Projects
None yet
Development

No branches or pull requests

8 participants