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

Refactor try_find a little #71899

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
Collaborator

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
Contributor

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in 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.