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

Normalize labeled and unlabeled breaks #39864

Merged
merged 5 commits into from
Feb 25, 2017
Merged

Conversation

cramertj
Copy link
Member

Part of #39849.

@rust-highfive
Copy link
Collaborator

r? @eddyb

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

@cramertj
Copy link
Member Author

r? @nikomatsakis

@rust-highfive rust-highfive assigned nikomatsakis and unassigned eddyb Feb 15, 2017
@cramertj
Copy link
Member Author

Now fixes #37576. I'll add a compile-fail test for unlabeled break in while conditions tomorrow.

Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

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

This looks nice! I left some nits, mostly about comments I'd like to see, but the main thing that I think is missing are more targeted tests:

  • We need a test for the new error (i.e., while break)
  • We need regression tests for the various cases listed on It is not intuitive which loop is broken out from in loop { while break {} } #37576, specifically in this comment
    • we just need to show we do decent things here
  • We need a test using a labeled break applied to the while loop itself (i.e., 'a: while break 'a { }), which I believe your PR now enables.
    • Ideally, we'd try to test the liveness/CFG, but I'm not 100% sure how best to do that and not overly concerned. The most important thing is to show that it doesn't execute the body of the loop ever (i.e., that the MIR control flow is working out OK).
  • Similarly some tests for something like while let None = break { } and 'a: while let None = break 'a { }

Make sense?

let expr_exit = self.add_ast_node(expr.id, &[cond_exit]); // 3

// Create expr_exit without pred (cond_exit)
let expr_exit = {
Copy link
Contributor

Choose a reason for hiding this comment

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

can we not just do self.add_ast_node(expr.id, &[])?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh. Yes.

let data = CFGNodeData::AST(id);
self.graph.add_node(data)
};

self.loop_scopes.push(LoopScope {
Copy link
Contributor

Choose a reason for hiding this comment

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

It's worth adding a comment here indicating that we want to be sure and push LoopScope onto loop_scopes so that they are in scope when evaluating cond

fn lower_label(&mut self, label: Option<(NodeId, Spanned<Ident>)>) -> hir::Label {
match label {
Some((id, label_ident)) => hir::Label {
ident: Some(label_ident),
loop_id: match self.expect_full_def(id) {
Copy link
Contributor

Choose a reason for hiding this comment

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

doesn't have to be done, and also doesn't have to be done in this PR, but as we mentioned on IRC I think i'd prefer to make this an Option type rather than using DUMMY_NODE_ID. No need to reinvent null.

Copy link
Member Author

Choose a reason for hiding this comment

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

My concern with that is that there are a lot of places that handle HIR for which it would just be a bug for it to be None. None/DUMMY_NODE_ID should be caught by the loops pass. I'm really starting to think that the loops pass should just be pulled into lowering, if possible.

@@ -79,6 +80,8 @@ pub struct LoweringContext<'a> {
impl_items: BTreeMap<hir::ImplItemId, hir::ImplItem>,
bodies: FxHashMap<hir::BodyId, hir::Body>,

loop_scopes: Vec<NodeId>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should make this an Option<Vec<NodeId>>? then it can start as None, and with_new_loop_scopes can insert Some(vec![]). This would ensure that all loop scope manipulation happens within the context of a with_new_loop_scopes() call (but other calls, such as with_loop_scope, would want to either ignore None or fail with bug!; not sure which). Do you think this makes sense?

Context is that I always get a bit nervous about the possibility of latent bugs where we are pushing loop scopes and things in places we don't expect and those pushes never get observed. I guess since the loops pass comes later and looks for surprises, this isn't such a big concern here, so maybe it's not worth the effort.

Copy link
Member Author

@cramertj cramertj Feb 16, 2017

Choose a reason for hiding this comment

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

I don't think it needs to be None, since in both of the cases its used, its absence will be obvious:

  • In the unlabeled case, .last() will return None
  • In the labeled case, the label will not be found

Both of these result in DUMMY_NODE_ID.

@@ -1660,10 +1699,13 @@ impl<'a> LoweringContext<'a> {
hir::ExprPath(self.lower_qpath(e.id, qself, path, ParamMode::Optional))
}
ExprKind::Break(opt_ident, ref opt_expr) => {
hir::ExprBreak(self.lower_label(e.id, opt_ident),
hir::ExprBreak(
self.lower_label(opt_ident.map(|ident| (e.id, ident))),
opt_expr.as_ref().map(|x| P(self.lower_expr(x))))
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: funny indentation here

@@ -1804,9 +1846,14 @@ impl<'a> LoweringContext<'a> {
// }
// }

let (body, break_expr, sub_expr) = self.with_loop_scope(e.id, |this| (
Copy link
Contributor

Choose a reason for hiding this comment

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

worth a comment here about the condition being within the while let loop

@@ -1033,8 +1033,7 @@ pub enum LoopSource {

#[derive(Clone, PartialEq, Eq, RustcEncodable, RustcDecodable, Hash, Debug, Copy)]
pub struct Label {
pub span: Span,
pub name: Name,
pub ident: Option<Spanned<Ident>>,
Copy link
Contributor

Choose a reason for hiding this comment

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

worth a comment explaining when this is None

@@ -1033,8 +1033,7 @@ pub enum LoopSource {

#[derive(Clone, PartialEq, Eq, RustcEncodable, RustcDecodable, Hash, Debug, Copy)]
pub struct Label {
pub span: Span,
pub name: Name,
pub ident: Option<Spanned<Ident>>,
pub loop_id: NodeId
Copy link
Contributor

Choose a reason for hiding this comment

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

worth a comment explaining that this is DUMMY_NODE_ID in the case of error (or, better yet, changing the type)

.rev()
.filter(|loop_scope| loop_scope.extent == label)
.next()
.unwrap_or_else(|| span_bug!(span, "no enclosing loop scope found?"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: inconsistent indentation; either align all to 4-space, or align the unwrap_or_else with the other lines

@nikomatsakis
Copy link
Contributor

Also, travis CI is reporting that the error code is a duplicate:

https://travis-ci.org/rust-lang/rust/jobs/202150799#L423

@@ -1699,8 +1728,16 @@ impl<'a> LoweringContext<'a> {
hir::ExprPath(self.lower_qpath(e.id, qself, path, ParamMode::Optional))
}
ExprKind::Break(opt_ident, ref opt_expr) => {
let label_result = if self.is_in_loop_condition && opt_ident.is_none() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Seems worth a comment pointing at the issue where this was discussed.

@nikomatsakis
Copy link
Contributor

Two test failures on travis:

failures:
    [compile-fail] compile-fail/borrowck/borrowck-lend-flow-loop.rs
    [compile-fail] compile-fail/resolve-label.rs

@cramertj cramertj force-pushed the normalize-breaks branch 2 times, most recently from 4116cc2 to c2fc090 Compare February 17, 2017 18:52
@nikomatsakis
Copy link
Contributor

Started a crater run from 48bc082 to c2fc0902af3cba5a7645786900bf6a351c39dd77.

@cramertj cramertj mentioned this pull request Feb 17, 2017
@nikomatsakis
Copy link
Contributor

nikomatsakis commented Feb 18, 2017

Crater run results: https://gist.github.com/nikomatsakis/3bd801c7c66d5448bb330ea03fc5f894

@nikomatsakis
Copy link
Contributor

@cramertj I didn't investigate those yet -- often they are just false errors.

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Feb 18, 2017

OK, all the failures were timeouts, except for errors in clippy that result from the HIR having changed to make the Label non-optional (cc @Manishearth).

@cramertj -- I wonder if we should rename Label to something like Destination? After all, there is always a "label" even if the break had no label...? At minimum it seems to merit a comment. =)

@cramertj
Copy link
Member Author

cramertj commented Feb 18, 2017

@nikomatsakis Destination or Target seem fine to me.

@nikomatsakis
Copy link
Contributor

@cramertj, ok -- if you want to squash/rebase, go ahead. And feel free to change Label to something else, or else just give it a comment like "in the HIR, all breaks/continues are labeled explicitly, even if user didn't write one" or something so we can "justify" the name. =)

@nikomatsakis
Copy link
Contributor

but basically r=me

@cramertj
Copy link
Member Author

Okay. I'll do that right away.

@@ -248,4 +248,6 @@ register_diagnostics! {
E0472, // asm! is unsupported on this target
E0561, // patterns aren't allowed in function pointer types
E0571, // `break` with a value in a non-`loop`-loop
E0583, // `break` with no label in the condition of a `while` loop
E0584, // `continue` with no label in the condition of a `while` loop
Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking that

(a) maybe it makes sense to have just one condition code here ("break or continue with no label in the condition of a while loop")

and

(b) given that we do technically accept while break { } in some situations (only if in another loop, etc), it is probably a good idea to write a more extended explanation and in particular to show an example of how to add a label (i.e., 'a: while break 'a { })

What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

For (a), it seems fine to me either way. I'll merge them if you think that'd be simpler.

For (b), I'm happy to write a more extended error explanation (assuming you mean using register_long_diagnostics!).

@bors
Copy link
Contributor

bors commented Feb 24, 2017

⌛ Testing commit 6f0447b with merge d84b197...

bors added a commit that referenced this pull request Feb 24, 2017
Normalize labeled and unlabeled breaks

Part of #39849.
@bors
Copy link
Contributor

bors commented Feb 24, 2017

💔 Test failed - status-travis

@cramertj
Copy link
Member Author

Error linking-- the failing test is for#25185. Seems spurious. Can someone retry this or add it to a rollup? Is there any better way for me to try to get this through? It got r+'d six days ago.

@alexcrichton
Copy link
Member

@bors: retry

eddyb added a commit to eddyb/rust that referenced this pull request Feb 25, 2017
…tsakis

Normalize labeled and unlabeled breaks

Part of rust-lang#39849.
eddyb added a commit to eddyb/rust that referenced this pull request Feb 25, 2017
…tsakis

Normalize labeled and unlabeled breaks

Part of rust-lang#39849.
bors added a commit that referenced this pull request Feb 25, 2017
@bors bors merged commit 6f0447b into rust-lang:master Feb 25, 2017
@bors
Copy link
Contributor

bors commented Feb 25, 2017

⌛ Testing commit 6f0447b with merge 1572bf1...

@TimNN TimNN mentioned this pull request Mar 3, 2017
frewsxcv added a commit to frewsxcv/rust that referenced this pull request Mar 8, 2017
frewsxcv added a commit to frewsxcv/rust that referenced this pull request Mar 9, 2017
alexcrichton added a commit to alexcrichton/rust that referenced this pull request Mar 10, 2017
alexcrichton added a commit to alexcrichton/rust that referenced this pull request Mar 10, 2017
bors added a commit that referenced this pull request Mar 14, 2017
@cramertj cramertj deleted the normalize-breaks branch September 21, 2017 16:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants