about summary refs log tree commit diff
path: root/src/libcore
diff options
context:
space:
mode:
authorbors <bors@rust-lang.org>2018-04-03 06:28:41 +0000
committerbors <bors@rust-lang.org>2018-04-03 06:28:41 +0000
commit577d29c10aed2d6e024fb2a532898be904abd115 (patch)
tree6913ab8e9f0d2595bc07ab682d8d54dd3237344b /src/libcore
parent5ee891cfeabc0872624104611cc0a359f46447cc (diff)
parent591dd5d992ef5c9eb6a37cf9870c5f094d6363e4 (diff)
downloadrust-577d29c10aed2d6e024fb2a532898be904abd115.tar.gz
rust-577d29c10aed2d6e024fb2a532898be904abd115.zip
Auto merge of #49098 - matklad:find_map, r=KodrAus
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: https://github.com/rust-lang/rfcs/issues/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
Diffstat (limited to 'src/libcore')
-rw-r--r--src/libcore/iter/iterator.rs32
-rw-r--r--src/libcore/tests/iter.rs27
-rw-r--r--src/libcore/tests/lib.rs1
3 files changed, 60 insertions, 0 deletions
diff --git a/src/libcore/iter/iterator.rs b/src/libcore/iter/iterator.rs
index 31f77f92435..f039d1730eb 100644
--- a/src/libcore/iter/iterator.rs
+++ b/src/libcore/iter/iterator.rs
@@ -1745,6 +1745,38 @@ pub trait Iterator {
         }).break_value()
     }
 
+    /// Applies function to the elements of iterator and returns
+    /// the first non-none result.
+    ///
+    /// `iter.find_map(f)` is equivalent to `iter.filter_map(f).next()`.
+    ///
+    ///
+    /// # Examples
+    ///
+    /// ```
+    /// #![feature(iterator_find_map)]
+    /// let a = ["lol", "NaN", "2", "5"];
+    ///
+    /// let mut first_number = a.iter().find_map(|s| s.parse().ok());
+    ///
+    /// assert_eq!(first_number, Some(2));
+    /// ```
+    #[inline]
+    #[unstable(feature = "iterator_find_map",
+               reason = "unstable new API",
+               issue = "49602")]
+    fn find_map<B, F>(&mut self, mut f: F) -> Option<B> where
+        Self: Sized,
+        F: FnMut(Self::Item) -> Option<B>,
+    {
+        self.try_for_each(move |x| {
+            match f(x) {
+                Some(x) => LoopState::Break(x),
+                None => LoopState::Continue(()),
+            }
+        }).break_value()
+    }
+
     /// Searches for an element in an iterator, returning its index.
     ///
     /// `position()` takes a closure that returns `true` or `false`. It applies
diff --git a/src/libcore/tests/iter.rs b/src/libcore/tests/iter.rs
index a962efadd64..2abac0cf1d5 100644
--- a/src/libcore/tests/iter.rs
+++ b/src/libcore/tests/iter.rs
@@ -1147,6 +1147,33 @@ fn test_find() {
 }
 
 #[test]
+fn test_find_map() {
+    let xs: &[isize] = &[];
+    assert_eq!(xs.iter().find_map(half_if_even), None);
+    let xs: &[isize] = &[3, 5];
+    assert_eq!(xs.iter().find_map(half_if_even), None);
+    let xs: &[isize] = &[4, 5];
+    assert_eq!(xs.iter().find_map(half_if_even), Some(2));
+    let xs: &[isize] = &[3, 6];
+    assert_eq!(xs.iter().find_map(half_if_even), Some(3));
+
+    let xs: &[isize] = &[1, 2, 3, 4, 5, 6, 7];
+    let mut iter = xs.iter();
+    assert_eq!(iter.find_map(half_if_even), Some(1));
+    assert_eq!(iter.find_map(half_if_even), Some(2));
+    assert_eq!(iter.find_map(half_if_even), Some(3));
+    assert_eq!(iter.next(), Some(&7));
+
+    fn half_if_even(x: &isize) -> Option<isize> {
+        if x % 2 == 0 {
+            Some(x / 2)
+        } else {
+            None
+        }
+    }
+}
+
+#[test]
 fn test_position() {
     let v = &[1, 3, 9, 27, 103, 14, 11];
     assert_eq!(v.iter().position(|x| *x & 1 == 0).unwrap(), 5);
diff --git a/src/libcore/tests/lib.rs b/src/libcore/tests/lib.rs
index 1a68f04532d..6f6105553d4 100644
--- a/src/libcore/tests/lib.rs
+++ b/src/libcore/tests/lib.rs
@@ -48,6 +48,7 @@
 #![feature(atomic_nand)]
 #![feature(reverse_bits)]
 #![feature(inclusive_range_fields)]
+#![feature(iterator_find_map)]
 
 extern crate core;
 extern crate test;