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

Stabilize ops::ControlFlow (just the type) #85608

Merged
merged 2 commits into from
Jun 15, 2021

Conversation

scottmcm
Copy link
Member

Tracking issue: #75744 (which also tracks items not closed by this PR).

With the new ? desugar implemented, it's no longer possible to mix Result and ControlFlow. (At the time of making this PR, godbolt was still on the 2021-05-01 nightly, where you can see that the mixing example compiled.) That resolves the only blocker I know of, so I'd like to propose that ControlFlow be considered for stabilization.

Its basic existence was part of rust-lang/rfcs#3058, where it got a bunch of positive comments (examples 1 2 3 4). Its use in the compiler has been well received (#78182 (comment)), and there are ecosystem updates interested in using it (rust-itertools/itertools#469 (comment), jonhoo/rust-imap#194).

As this will need an FCP, picking a libs member manually:
r? @m-ou-se

Stabilized APIs

#[derive(Debug, Clone, Copy, PartialEq)]
pub enum ControlFlow<B, C = ()> {
    /// Exit the operation without running subsequent phases.
    Break(B),
    /// Move on to the next phase of the operation as normal.
    Continue(C),
}

As well as using ? on a ControlFlow<B, _> in a function returning ControlFlow<B, _>. (Note, in particular, that there's no From::from-conversion on the Break value, the way there is for Errs.)

Existing APIs not stabilized here

All the associated methods and constants: break_value, is_continue, map_break, CONTINUE, etc.

Some of the existing methods in nightly seem reasonable, some seem like they should be removed, and some need more discussion to decide. But none of them are essential, so as in the RFC, they're all omitted from this PR.

They can be considered separately later, as further usage demonstrates which are important.

@scottmcm scottmcm added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. needs-fcp This change is insta-stable, so needs a completed FCP to proceed. labels May 23, 2021
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 23, 2021
@jonas-schievink jonas-schievink added the relnotes Marks issues that should be documented in the release notes of the next release. label May 23, 2021
@rust-log-analyzer

This comment has been minimized.

@scottmcm scottmcm force-pushed the stabilize-control-flow-enum-basics branch from 0b58335 to 65a0a8b Compare May 23, 2021 20:20
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.
@m-ou-se
Copy link
Member

m-ou-se commented Jun 2, 2021

@rfcbot merge

@rfcbot
Copy link

rfcbot commented Jun 2, 2021

Team member @m-ou-se has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Jun 2, 2021
@rfcbot rfcbot added the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label Jun 4, 2021
@rfcbot
Copy link

rfcbot commented Jun 4, 2021

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot removed the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Jun 4, 2021
@jhpratt
Copy link
Member

jhpratt commented Jun 11, 2021

To be clear, this will permit using this enum for actual control flow purposes? Or is this only stabilizing the enum itself and not its associated behavior? I think it's the former given the diff, but I'm not positive.

@scottmcm
Copy link
Member Author

scottmcm commented Jun 11, 2021

I'm not sure what "for actual control flow purposes" means. This would stabilize use of ? on ControlFlow, including using ControlFlow as a return type from a try_fold closure (since trait implementations cannot be private).

@jhpratt
Copy link
Member

jhpratt commented Jun 11, 2021

That's exactly what I meant. Just wanted to be positive this wasn't just stabilizing an enum for no other purpose (which would be unlike you!)

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Jun 14, 2021
@rfcbot
Copy link

rfcbot commented Jun 14, 2021

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

The RFC will be merged soon.

@rfcbot rfcbot added the to-announce Announce this issue on triage meeting label Jun 14, 2021
@m-ou-se
Copy link
Member

m-ou-se commented Jun 14, 2021

@bors r+

@bors
Copy link
Contributor

bors commented Jun 14, 2021

📌 Commit 590d452 has been approved by m-ou-se

@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 14, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 15, 2021
Rollup of 10 pull requests

Successful merges:

 - rust-lang#80269 (Explain non-dropped sender recv in docs)
 - rust-lang#82179 (Add functions `Duration::try_from_secs_{f32, f64}`)
 - rust-lang#85608 (Stabilize `ops::ControlFlow` (just the type))
 - rust-lang#85792 (Refactor windows sockets impl methods)
 - rust-lang#86220 (Improve maybe_uninit_extra docs)
 - rust-lang#86277 (Remove must_use from ALLOWED_ATTRIBUTES)
 - rust-lang#86285 (:arrow_up: rust-analyzer)
 - rust-lang#86294 (Stabilize {std, core}::prelude::rust_*.)
 - rust-lang#86306 (Add mailmap entries for myself)
 - rust-lang#86314 (Remove trailing triple backticks in `mut_keyword` docs)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 5936ecc into rust-lang:master Jun 15, 2021
@rustbot rustbot added this to the 1.55.0 milestone Jun 15, 2021
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Jun 17, 2021
@scottmcm scottmcm deleted the stabilize-control-flow-enum-basics branch June 29, 2021 00:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. needs-fcp This change is insta-stable, so needs a completed FCP to proceed. relnotes Marks issues that should be documented in the release notes of the next release. 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.

10 participants