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 ControlFlow::{BREAK, CONTINUE} #102697

Closed

Conversation

scottmcm
Copy link
Member

@scottmcm scottmcm commented Oct 5, 2022

I don't think these ever had any libs-api oversight, but they're super-simple so I figured I'd just send this PR to prompt discussion.

I wasn't confident in them when I first added them (#76318 (comment)), but they've been extensively used in the compiler, and #75744 (comment) got posted in their favour. So that makes me think they're worth having. Here's a few examples:

if predicate.flags().intersects(self.flags) {
ControlFlow::Break(FoundFlags)
} else {
ControlFlow::CONTINUE
}

fn visit_ty(&mut self, t: Ty<'tcx>) -> ControlFlow<Self::BreakTy> {
if self.0 == t { ControlFlow::BREAK } else { t.super_visit_with(self) }
}

) -> ControlFlow<Self::BreakVal> {
match prior_status {
Some(NodeStatus::Visited) => ControlFlow::BREAK,
_ => ControlFlow::CONTINUE,
}
}

I think that ControlFlow::CONTINUE is particularly justified because the Continue generic parameter is defaulted to (). And if that constant exists, I think there might as well be BREAK to go with it. It's useful too, for any situation in which you're replacing bool with ControlFlow<()> in chain-of-responsibility style code, or similar.

@rustbot label +needs-fcp +T-libs-api -T-libs

cc the tracking issue #75744, though this is only a partial stabilization so this does not close that.

@rustbot

This comment was marked as resolved.

@rustbot rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Oct 5, 2022
@rust-highfive

This comment was marked as outdated.

@rustbot rustbot added needs-fcp This change is insta-stable, so needs a completed FCP to proceed. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Oct 5, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 5, 2022
@scottmcm scottmcm removed the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Oct 5, 2022
@scottmcm
Copy link
Member Author

scottmcm commented Oct 5, 2022

I flipped a coin between libs-api reviewers and got
r? @m-ou-se

@rust-highfive rust-highfive assigned m-ou-se and unassigned thomcc Oct 5, 2022
@m-ou-se
Copy link
Member

m-ou-se commented Oct 11, 2022

These feel a bit odd to me. I agree Continue(()) is often a useful value, but so is Ok(()) for which we also don't have an OK const.

@m-ou-se m-ou-se added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 11, 2022
@joshtriplett
Copy link
Member

@m-ou-se I had the same reaction. I would lean against adding these.

@afetisov
Copy link

Personally I wouldn't be comfortable directly using them, unless they're part of the prelude. This looks like a feature too niche to just import directly. But if one uses them via the qualified name ControlFlow::BREAK/CONTINUE, then there is little value over the ordinary enum variant constructors.

@scottmcm scottmcm added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). I-libs-api-nominated Nominated for discussion during a libs-api team meeting. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 15, 2023
@scottmcm
Copy link
Member Author

scottmcm commented Jan 15, 2023

Hi libs-api! This has been in limbo for a while, so I'm nominating to get a quorum input on what I should do with it.

Please either:

  1. start fcp on it, or
  2. say that, as the team, you'd like the constants to be removed instead, at which point I'll send a review to remove these.

(Or if there's some other process you'd like me to follow, let me know.)

@joshtriplett
Copy link
Member

@scottmcm We discussed this in today's @rust-lang/libs-api meeting, and the general consensus was that we don't want to stabilize these, and we'd like to see them removed. Same argument as not adding OK for Ok(()).

@rfcbot close

@rfcbot
Copy link

rfcbot commented Jan 17, 2023

Team member @joshtriplett has proposed to close 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-close This PR / issue is in PFCP or FCP with a disposition to close it. labels Jan 17, 2023
@joshtriplett joshtriplett removed needs-fcp This change is insta-stable, so needs a completed FCP to proceed. I-libs-api-nominated Nominated for discussion during a libs-api team meeting. labels Jan 17, 2023
@rfcbot rfcbot added the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label Jan 17, 2023
@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 Jan 17, 2023
@rfcbot
Copy link

rfcbot commented Jan 17, 2023

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

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 18, 2023
Stop using `BREAK` & `CONTINUE` in compiler

Switching them to `Break(())` and `Continue(())` instead.

Entirely search-and-replace, though there's one spot where rustfmt insisted on a reformatting too.

libs-api would like to remove these constants (rust-lang#102697 (comment)), so stop using them in compiler to make the removal PR later smaller.
compiler-errors added a commit to compiler-errors/rust that referenced this pull request Jan 18, 2023
Stop using `BREAK` & `CONTINUE` in compiler

Switching them to `Break(())` and `Continue(())` instead.

Entirely search-and-replace, though there's one spot where rustfmt insisted on a reformatting too.

libs-api would like to remove these constants (rust-lang#102697 (comment)), so stop using them in compiler to make the removal PR later smaller.
@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Jan 27, 2023
@rfcbot
Copy link

rfcbot commented Jan 27, 2023

The final comment period, with a disposition to close, 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.

scottmcm added a commit to scottmcm/rust that referenced this pull request Jan 28, 2023
Libs-API decided to remove these in rust-lang#102697.

Follow-up to rust-lang#107023, which removed them from `compiler/`, but a couple new ones showed up since that was merged.
@scottmcm
Copy link
Member Author

I removed a bunch of uses of these in #107023; PR to remove them completely is up as #107398.

@scottmcm scottmcm closed this Jan 28, 2023
@scottmcm scottmcm deleted the stabilize-control-flow-constants branch January 28, 2023 03:49
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 28, 2023
Remove `ControlFlow::{BREAK, CONTINUE}`

Libs-API decided to remove these in rust-lang#102697.

Follow-up to rust-lang#107023, which removed them from `compiler/`, but a couple new ones showed up since that was merged.

r? libs
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Feb 3, 2023
flip1995 pushed a commit to flip1995/rust that referenced this pull request Feb 10, 2023
Remove `ControlFlow::{BREAK, CONTINUE}`

Libs-API decided to remove these in rust-lang#102697.

Follow-up to rust-lang#107023, which removed them from `compiler/`, but a couple new ones showed up since that was merged.

r? libs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-close This PR / issue is in PFCP or FCP with a disposition to close it. finished-final-comment-period The final comment period is finished for this PR / Issue. S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). 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.

9 participants