Add Iterator::find_map #49098

matklad · 2018-03-16T23:12:32Z

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 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], [2], [3].
I've also needed similar functionality in Kotlin several times. There, it is expressed as mapNotNull {}.firstOrNull, as can be seen here, here here and here (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

rust-highfive · 2018-03-16T23:12:36Z

r? @KodrAus

(rust_highfive has picked a reviewer for you, use r? to override)

Centril · 2018-03-16T23:26:33Z

I think I'm for this PR, but a small note re:

Also, if Itertools adds an find_map method, it would be more difficult to move it to std due to name collision.

I think given the latest .flatten() debacle, we kinda informally "decided" that Iterator extension proposals should first be proposed to libstd, and if libstd doesn't want it, then it should be proposed for Itertools inclusion.

leonardo-m · 2018-03-17T11:15:34Z

Instead of adding iterators randomly, I suggest to take a look at the most commonly used iterators from the itertools crate, and add those.

Centril · 2018-03-17T11:24:35Z

Instead of adding iterators randomly,

There's nothing random about this addition - I think it is right to fill consistency holes per @matklad's table above.

That said, we should add some operations from Itertools - any suggestions?

leonardo-m · 2018-03-17T12:05:54Z

By "randomly" I meant: "not needed in lot of code by lot of people". I agree it's hard to measure this need exactly, but even measuring it approximately is OK. I have written a ton of iterator-based Rust/Python/D code and I think there are things (much) more useful than a find_map.

Regarding the "most useful itertools things", I can give you a list, but probably every programmer has a slightly different list. In my opinion the intersection of such lists is far from empty. I think this topic is better discussed in a forum thread than here.

Centril · 2018-03-17T12:36:23Z

Regarding the "most useful itertools things", I can give you a list,

Please do =) (and also discount things that require allocation, because those can't be in libcore)

matklad · 2018-03-17T13:14:26Z

@leonardo-m I totally agree that “how often is this used in practice” should be one of the primarily axis along which to measure additions to std. That’s why I’ve tried to measure how often could find_map be used in practice, by giving examples of real-life code :)

I have written a ton of iterator-based Rust/Python/D code and I think there are things (much) more useful than a find_map.

Keep in mind that what kinds of iterators you need most highly depends on the kind of code you are writing. For me, find_map is the second most important missing method at the moment (the first one is join of course, producing a comma separated list of things in Rust is ridiculously not straight forward at the moment)

shepmaster · 2018-03-23T15:56:53Z

Ping from triage, @KodrAus ! Will you have time to review this soon?

KodrAus · 2018-03-25T23:33:33Z

This particular find_map addition looks reasonable to me, since it rounds out some existing set of methods as @matklad described.

cc @rust-lang/libs any thoughts on find_map? If we're generally happy with landing this then maybe we can throw a test case or two in there.

matklad · 2018-03-27T12:38:01Z

Pushed some tests!

KodrAus

Thanks @matklad! I think this has been sitting around long enough. Once we have the unstable issue number sorted I think this will be ready to go.

KodrAus · 2018-04-02T21:21:53Z

src/libcore/iter/iterator.rs

+    #[inline]
+    #[unstable(feature = "iterator_find_map",
+               reason = "unstable new API",
+               issue = "0")]


I've created a tracking issue so we can update the value here. The issue is #49602

Filled in the issue number!

KodrAus · 2018-04-02T22:08:16Z

@bors r+

bors · 2018-04-02T22:08:17Z

📌 Commit 591dd5d has been approved by KodrAus

bors · 2018-04-03T06:28:48Z

⌛ Testing commit 591dd5d with merge 577d29c...

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

bors · 2018-04-03T09:11:30Z

☀️ Test successful - status-appveyor, status-travis
Approved by: KodrAus
Pushing 577d29c to master...

meven · 2018-10-23T14:36:03Z

src/libcore/iter/iterator.rs

+    /// #![feature(iterator_find_map)]
+    /// let a = ["lol", "NaN", "2", "5"];
+    ///
+    /// let mut first_number = a.iter().find_map(|s| s.parse().ok());


The mut is not needed here, I believe.

@meven seems plausible — submit a PR to fix it, please?

PR opened at #55389

…lates to rust-lang#49098

Remove unnecessary mut in iterator.find_map documentation example, R… Relates to rust-lang#49098 Removes a mut that could induce newcomers to put a mut in their code that the compiler would comply about. https://github.com/rust-lang/rust/pull/49098/files#r227422388

rust-highfive assigned KodrAus Mar 16, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 16, 2018

This was referenced Mar 16, 2018

Faster resolver: clean code and the backtrack_stack rust-lang/cargo#5187

Merged

Add find_map to the Iterator trait rust-lang/rfcs#1801

Closed

matklad force-pushed the find_map branch from f20a612 to 0580b38 Compare March 27, 2018 12:35

KodrAus mentioned this pull request Apr 2, 2018

Tracking Issue for Iterator::find_map #49602

Closed

KodrAus approved these changes Apr 2, 2018

View reviewed changes

Add Iterator::find_map

591dd5d

matklad force-pushed the find_map branch from 0580b38 to 591dd5d Compare April 2, 2018 21:47

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 2, 2018

bors merged commit 591dd5d into rust-lang:master Apr 3, 2018

matklad deleted the find_map branch April 3, 2018 17:13

SimonSapin mentioned this pull request Aug 20, 2018

Tracking issue for RFC 2351, "Add is_sorted to the standard library" #53485

Closed

10 tasks

alexcrichton mentioned this pull request Sep 24, 2018

[1.30 beta] Multiple applicable items in scope #54474

Closed

meven reviewed Oct 23, 2018

View reviewed changes

meven mentioned this pull request Oct 26, 2018

Remove unnecessary mut in iterator.find_map documentation example, R… #55389

Merged

meven added a commit to meven/rust that referenced this pull request Oct 26, 2018

Remove unnecessary mut in iterator.find_map documentation example, Re…

c674802

…lates to rust-lang#49098

This was referenced May 27, 2024

Add Iterator::as_rev rust-lang/libs-team#385

Closed

feat(FilterMapOk): implement DoubleEndedIterator rust-itertools/itertools#950

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Iterator::find_map #49098

Add Iterator::find_map #49098

matklad commented Mar 16, 2018 •

edited

Loading

rust-highfive commented Mar 16, 2018

Centril commented Mar 16, 2018

leonardo-m commented Mar 17, 2018 •

edited

Loading

Centril commented Mar 17, 2018

leonardo-m commented Mar 17, 2018 •

edited

Loading

Centril commented Mar 17, 2018

matklad commented Mar 17, 2018

shepmaster commented Mar 23, 2018

KodrAus commented Mar 25, 2018

matklad commented Mar 27, 2018

KodrAus left a comment

KodrAus Apr 2, 2018

matklad Apr 2, 2018

KodrAus commented Apr 2, 2018

bors commented Apr 2, 2018

bors commented Apr 3, 2018

bors commented Apr 3, 2018

meven Oct 23, 2018

shepmaster Oct 23, 2018

meven Oct 26, 2018

Add Iterator::find_map #49098

Add Iterator::find_map #49098

Conversation

matklad commented Mar 16, 2018 • edited Loading

rust-highfive commented Mar 16, 2018

Centril commented Mar 16, 2018

leonardo-m commented Mar 17, 2018 • edited Loading

Centril commented Mar 17, 2018

leonardo-m commented Mar 17, 2018 • edited Loading

Centril commented Mar 17, 2018

matklad commented Mar 17, 2018

shepmaster commented Mar 23, 2018

KodrAus commented Mar 25, 2018

matklad commented Mar 27, 2018

KodrAus left a comment

Choose a reason for hiding this comment

KodrAus Apr 2, 2018

Choose a reason for hiding this comment

matklad Apr 2, 2018

Choose a reason for hiding this comment

KodrAus commented Apr 2, 2018

bors commented Apr 2, 2018

bors commented Apr 3, 2018

bors commented Apr 3, 2018

meven Oct 23, 2018

Choose a reason for hiding this comment

shepmaster Oct 23, 2018

Choose a reason for hiding this comment

meven Oct 26, 2018

Choose a reason for hiding this comment

matklad commented Mar 16, 2018 •

edited

Loading

leonardo-m commented Mar 17, 2018 •

edited

Loading

leonardo-m commented Mar 17, 2018 •

edited

Loading