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

Rename and expose LoopState as ControlFlow #76204

Merged
merged 2 commits into from
Sep 3, 2020

Conversation

NoraCodes
Copy link
Contributor

Basic PR for #75744. Addresses everything there except for documentation; lots of examples are probably a good idea.

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @dtolnay (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 1, 2020
@NoraCodes
Copy link
Contributor Author

r? @scottmcm

@rust-highfive rust-highfive assigned scottmcm and unassigned dtolnay Sep 1, 2020
@jyn514 jyn514 added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Sep 1, 2020
@scottmcm
Copy link
Member

scottmcm commented Sep 1, 2020

Ping @rust-lang/libs to check that this is a plausible addition (as unstable, so no full FCP needed):

@scottmcm
Copy link
Member

scottmcm commented Sep 1, 2020

@NoraCodes Looks like tidy is complaining:

 tidy error: /checkout/library/core/src/iter/mod.rs: too many trailing newlines (2)
some tidy checks failed

Can you fix that, rebase to upstream/master (to avoid the merge conflict that kept CI from running initially), and force-push the new change, please?

Copy link
Contributor

@ecstatic-morse ecstatic-morse left a comment

Choose a reason for hiding this comment

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

Thanks @NoraCodes! I left some minor notes/suggestions. You're free to ignore them until someone from the libs team weighs in.


/// Used to make try_fold closures more like normal loops
#[unstable(feature="control_flow_enum", reason="new API", issue="75744")]
#[derive(Debug, Clone, Copy, PartialEq)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#[derive(Debug, Clone, Copy, PartialEq)]
#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]

Hash and especially Eq seem handy, although they are not needed to replace LoopState and are less essential for ControlFlow than for Option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that seems sensible! I don't know when you'd want to hash this but I suppose there's no reason to prevent it.

/// Used to make try_fold closures more like normal loops
#[unstable(feature="control_flow_enum", reason="new API", issue="75744")]
#[derive(Debug, Clone, Copy, PartialEq)]
pub enum ControlFlow<C, B> {
Copy link
Contributor

Choose a reason for hiding this comment

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

I have found that C = () is very common when using this enum, common enough that it might deserve a defaulted type parameter. We could add one backwards compatibly but only if C follows B in the parameter list. Otherwise we would have to default B at the same time and anyone that wishes to override it would have to override C as well.

Copy link
Member

Choose a reason for hiding this comment

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

Good point! It's even usually that way in the uses this PR touches, like
image
I suppose we could even do ControlFlow<B = (), C = ()> the same way break; is break ();.

(Though I suppose the reorder might make the diff messier, so could also be a separate PR.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it would be confusing to do this without changing the order of the variants as well, and we may as well do that too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On consideration, I think a separate PR would be best.

}
}

impl<R: Try> ControlFlow<R::Ok, R> {
Copy link
Contributor

@ecstatic-morse ecstatic-morse Sep 1, 2020

Choose a reason for hiding this comment

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

Shouldn't the second parameter be R::Error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, definitely.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I see - we can't do this because it's an unconstrained type parameter. Is there a good way to solve that?

Copy link
Member

@scottmcm scottmcm Sep 2, 2020

Choose a reason for hiding this comment

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

No, the break part here is intentionally the full impl Try type (Result, etc). They're definitely strange APIs, though -- break_value is something that could plausibly stabilize, but these ones probably aren't.

Maybe leave this particular impl block over in iter as non-pub so they can still be used there, but don't show up in the rustdoc? (And won't need #[unstable] since they'll be unusable outside iter.)

I've been contemplating making a pass through the implementations to use feature(try_blocks) instead of explicit Try::foo calls where possible; this PR going in would be a good impetus to go do that -- and hopefully delete these methods while I'm at it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I see. I misunderstood how these were being used. You're right, it's a very strange API.

@scottmcm scottmcm added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 2, 2020
@scottmcm
Copy link
Member

scottmcm commented Sep 2, 2020

@dtolnay
Copy link
Member

dtolnay commented Sep 2, 2020

Looks plausible, so @bors r=scottmcm

I agree with #75744 (comment) that "a Break/Continue enum is the correct abstraction for the Try trait instead of Result". As I understand it, while this type is structurally identical to Result, its API is less expansive and better geared toward readable code in uses of try_fold, in a way that use Result::{self as ControlFlow, Ok as Continue, Err as Break}; wouldn't quite accomplish.

@bors
Copy link
Contributor

bors commented Sep 2, 2020

📌 Commit 96eb5e1 has been approved by scottmcm

@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-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Sep 2, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 3, 2020
Rollup of 12 pull requests

Successful merges:

 - rust-lang#75150 (Add a note for Ipv4Addr::to_ipv6_compatible)
 - rust-lang#76120 (Add `[T; N]::as_[mut_]slice`)
 - rust-lang#76142 (Make all methods of `std::net::Ipv4Addr` const)
 - rust-lang#76164 (Link to slice pattern in array docs)
 - rust-lang#76167 (Replace MinGW library hack with heuristic controlling link mode)
 - rust-lang#76204 (Rename and expose LoopState as ControlFlow)
 - rust-lang#76238 (Move to intra-doc links for library/core/src/iter/traits/iterator.rs)
 - rust-lang#76242 (Read: adjust a FIXME reference)
 - rust-lang#76243 (Fix typos in vec try_reserve(_exact) docs)
 - rust-lang#76245 (inliner: Avoid query cycles when optimizing generators)
 - rust-lang#76255 (Update books)
 - rust-lang#76261 (Use intra-doc links in `core::marker`)

Failed merges:

r? @ghost
@bors bors merged commit d059f26 into rust-lang:master Sep 3, 2020
JohnTitor added a commit to JohnTitor/rust that referenced this pull request May 25, 2021
Demote `ControlFlow::{from|into}_try` to `pub(crate)`

They have mediocre names and non-obvious semantics, so personally I don't think they're worth trying to stabilize, and thus might as well just be internal (they're used for convenience in iterator adapters), not something shown in the rustdocs.

I don't think anyone actually wanted to use them outside `core` -- they just got made public-but-unstable along with the whole type in rust-lang#76204 that promoted `LoopState` from an internal type to the exposed `ControlFlow` type.

cc rust-lang#75744, the tracking issue they mention.
JohnTitor added a commit to JohnTitor/rust that referenced this pull request May 26, 2021
Demote `ControlFlow::{from|into}_try` to `pub(crate)`

They have mediocre names and non-obvious semantics, so personally I don't think they're worth trying to stabilize, and thus might as well just be internal (they're used for convenience in iterator adapters), not something shown in the rustdocs.

I don't think anyone actually wanted to use them outside `core` -- they just got made public-but-unstable along with the whole type in rust-lang#76204 that promoted `LoopState` from an internal type to the exposed `ControlFlow` type.

cc rust-lang#75744, the tracking issue they mention.
cc rust-lang#85608, the PR where I'm proposing stabilizing the type.
JohnTitor added a commit to JohnTitor/rust that referenced this pull request May 26, 2021
Demote `ControlFlow::{from|into}_try` to `pub(crate)`

They have mediocre names and non-obvious semantics, so personally I don't think they're worth trying to stabilize, and thus might as well just be internal (they're used for convenience in iterator adapters), not something shown in the rustdocs.

I don't think anyone actually wanted to use them outside `core` -- they just got made public-but-unstable along with the whole type in rust-lang#76204 that promoted `LoopState` from an internal type to the exposed `ControlFlow` type.

cc rust-lang#75744, the tracking issue they mention.
cc rust-lang#85608, the PR where I'm proposing stabilizing the type.
@cuviper cuviper added this to the 1.48.0 milestone Nov 16, 2023
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.

8 participants