Skip to content

Refactor try_find a little #71899

New issue

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

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

Already on GitHub? # to your account

Merged
merged 1 commit into from
Jun 20, 2020
Merged

Refactor try_find a little #71899

merged 1 commit into from
Jun 20, 2020

Conversation

cuviper
Copy link
Member

@cuviper cuviper commented May 4, 2020

This works like find_map, but mapping to a Try type. It stops when Ok is Some(value), with an additional short-circuit on Try::Error. This is similar to the unstable try_find, but has the advantage of being able to directly return the user's R: Try type directly, rather than converting to Result.
(removed -- try_find_map was declined in review)

This PR also refactors try_find a little to match style. The E type parameter was unnecessary, so it's now removed. The folding closure now has reduced parametricity on just T = Self::Item, rather
than the whole Self iterator type. There's otherwise no functional change in this.

@rust-highfive
Copy link
Contributor

r? @kennytm

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 4, 2020
@leonardo-m
Copy link

Instead of Iterator::try_find_map I'd like more useful methods like Iterator::sorted, Iterator::unfold, and few other handy things.
I also propose to put to votes further additions to Iterator in the discussion groups instead, so more broadly useful things could be addd.

@cuviper
Copy link
Member Author

cuviper commented May 5, 2020

@leonardo-m - the usual playground for such stuff is the itertools crate, which does actually have sorted and unfold. But try_find_map can't be implemented externally while Try is unstable.

@leonardo-m
Copy link

Why is try_find_map necessary? Why can't try_find_map wait Try to be stable so you can implment it in the playground itertools?

@cuviper
Copy link
Member Author

cuviper commented May 5, 2020

It's on par with try_find, at least, and I think the API is even nicer since it doesn't force Result.

@joelpalmer joelpalmer added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 14, 2020
@bors

This comment has been minimized.

@kennytm
Copy link
Member

kennytm commented May 21, 2020

This is non-trivial enough that I'm going to reassign it to an actual T-libs member 🙂.

r? @sfackler

@rust-highfive rust-highfive assigned sfackler and unassigned kennytm May 21, 2020
@joelpalmer joelpalmer added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 28, 2020
@crlf0710 crlf0710 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 5, 2020
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 16, 2020
@Dylan-DPC-zz
Copy link

r? @dtolnay

@rust-highfive rust-highfive assigned dtolnay and unassigned sfackler Jun 16, 2020
Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I would prefer not to add the new method. The other try iterator methods (try_fold and try_for_each) expose a tangible new capability over the non-try (fold, for_each) because the non-try do not have an ability to early terminate. But Iterator::find_map is already entirely about early termination. In find_map the closure is supposed to return Some if you want to early terminate and None to keep iterating, and find_map's return value gives the thing you early terminated with if any, whether that's an error or an object if you want. The proposed try_find_map is the same but with a "transposed" interpretation of the closure:

iter.try_find_map(|s| parse_even(s))

// equivalent to:
iter.find_map(|s| parse_even(s).transpose()).transpose()

All in all I don't think the benefit is there more than e.g. try_map, try_filter, try_filter_map, try_skip_while, try_scan, try_any, try_partition etc, and I'm not on board with more in this direction just for small convenience. I think the fallible-iterator crate is a suitable place for this kind of thing.

I would be happy to take the try_find fixes though: the doc fix, removed type parameter, and reduced parametricity.

@dtolnay dtolnay added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 19, 2020
The `E` type parameter was unnecessary, so it's now removed. The folding
closure now has reduced parametricity on just `T = Self::Item`, rather
than the whole `Self` iterator type. There's otherwise no functional
change in this.
@cuviper cuviper changed the title Add Iterator::try_find_map Refactor try_find a little Jun 19, 2020
@cuviper cuviper added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 19, 2020
@cuviper
Copy link
Member Author

cuviper commented Jun 19, 2020

OK, thanks for the detailed consideration. I've dropped the try_find_map commit, so now this has just the try_find adjustments.

@dtolnay
Copy link
Member

dtolnay commented Jun 19, 2020

@bors r+

@bors
Copy link
Collaborator

bors commented Jun 19, 2020

📌 Commit db0d70e has been approved by dtolnay

@bors 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 Jun 19, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 20, 2020
…arth

Rollup of 16 pull requests

Successful merges:

 - rust-lang#71420 (Specialization is unsound)
 - rust-lang#71899 (Refactor `try_find` a little)
 - rust-lang#72689 (add str to common types)
 - rust-lang#72791 (update coerce docs and unify relevant tests)
 - rust-lang#72934 (forbid mutable references in all constant contexts except for const-fns)
 - rust-lang#73027 (Make `need_type_info_err` more conservative)
 - rust-lang#73347 (Diagnose use of incompatible sanitizers)
 - rust-lang#73359 (shim.rs: avoid creating `Call` terminators calling `Self`)
 - rust-lang#73399 (Clean up E0668 explanation)
 - rust-lang#73436 (Clean up E0670 explanation)
 - rust-lang#73440 (Add src/librustdoc as an alias for src/tools/rustdoc)
 - rust-lang#73442 (pretty/mir: const value enums with no variants)
 - rust-lang#73452 (Unify region variables when projecting associated types)
 - rust-lang#73458 (Use alloc::Layout in DroplessArena API)
 - rust-lang#73484 (Update the doc for std::prelude to the correct behavior)
 - rust-lang#73506 (Bump Rustfmt and RLS)

Failed merges:

r? @ghost
@bors bors merged commit 5c9cd82 into rust-lang:master Jun 20, 2020
@cuviper cuviper deleted the try_find_map branch August 9, 2020 23:37
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.