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.

Sign up for GitHub

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

Normalize labeled and unlabeled breaks #39864

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5205e2f
Normalize labeled and unlabeled breaks
cramertj Feb 15, 2017
4d65622
Properly implement labeled breaks in while conditions
cramertj Feb 16, 2017
56e519d
Add tests for control flow in while condition
cramertj Feb 17, 2017
a611bbc
Rename hir::Label to hir::Destination
cramertj Feb 18, 2017
6f0447b
Change break or continue with no label to error nmbr 590
cramertj Feb 23, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 23 additions & 13 deletions src/librustc/cfg/construct.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,15 +220,24 @@ impl<'a, 'tcx> CFGBuilder<'a, 'tcx> {
// Note that `break` and `continue` statements
// may cause additional edges.

// Is the condition considered part of the loop?
let loopback = self.add_dummy_node(&[pred]); // 1
let cond_exit = self.expr(&cond, loopback); // 2
let expr_exit = self.add_ast_node(expr.id, &[cond_exit]); // 3

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

// The LoopScope needs to be on the loop_scopes stack while evaluating the
// condition and the body of the loop (both can break out of the loop)
self.loop_scopes.push(LoopScope {
loop_id: expr.id,
continue_index: loopback,
break_index: expr_exit
});

let cond_exit = self.expr(&cond, loopback); // 2

// Add pred (cond_exit) to expr_exit
self.add_contained_edge(cond_exit, expr_exit);

let body_exit = self.block(&body, cond_exit); // 4
self.add_contained_edge(body_exit, loopback); // 5
self.loop_scopes.pop();
Expand Down Expand Up @@ -294,17 +303,17 @@ impl<'a, 'tcx> CFGBuilder<'a, 'tcx> {
self.add_unreachable_node()
}

hir::ExprBreak(label, ref opt_expr) => {
hir::ExprBreak(destination, ref opt_expr) => {
let v = self.opt_expr(opt_expr, pred);
let loop_scope = self.find_scope(expr, label);
let loop_scope = self.find_scope(expr, destination);
let b = self.add_ast_node(expr.id, &[v]);
self.add_exiting_edge(expr, b,
loop_scope, loop_scope.break_index);
self.add_unreachable_node()
}

hir::ExprAgain(label) => {
let loop_scope = self.find_scope(expr, label);
hir::ExprAgain(destination) => {
let loop_scope = self.find_scope(expr, destination);
let a = self.add_ast_node(expr.id, &[pred]);
self.add_exiting_edge(expr, a,
loop_scope, loop_scope.continue_index);
Expand Down Expand Up @@ -579,17 +588,18 @@ impl<'a, 'tcx> CFGBuilder<'a, 'tcx> {

fn find_scope(&self,
expr: &hir::Expr,
label: Option<hir::Label>) -> LoopScope {
match label {
None => *self.loop_scopes.last().unwrap(),
Some(label) => {
destination: hir::Destination) -> LoopScope {

match destination.loop_id.into() {
Ok(loop_id) => {
for l in &self.loop_scopes {
if l.loop_id == label.loop_id {
if l.loop_id == loop_id {
return *l;
}
}
span_bug!(expr.span, "no loop scope for id {}", label.loop_id);
span_bug!(expr.span, "no loop scope for id {}", loop_id);
}
Err(err) => span_bug!(expr.span, "loop scope error: {}", err)
}
}
}
24 changes: 14 additions & 10 deletions src/librustc/hir/intravisit.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1006,18 +1006,22 @@ pub fn walk_expr<'v, V: Visitor<'v>>(visitor: &mut V, expression: &'v Expr) {
ExprPath(ref qpath) => {
visitor.visit_qpath(qpath, expression.id, expression.span);
}
ExprBreak(None, ref opt_expr) => {
ExprBreak(label, ref opt_expr) => {
label.ident.map(|ident| {
if let Ok(loop_id) = label.loop_id.into() {
visitor.visit_def_mention(Def::Label(loop_id));
}
visitor.visit_name(ident.span, ident.node.name);
});
walk_list!(visitor, visit_expr, opt_expr);
}
ExprBreak(Some(label), ref opt_expr) => {
visitor.visit_def_mention(Def::Label(label.loop_id));
visitor.visit_name(label.span, label.name);
walk_list!(visitor, visit_expr, opt_expr);
}
ExprAgain(None) => {}
ExprAgain(Some(label)) => {
visitor.visit_def_mention(Def::Label(label.loop_id));
visitor.visit_name(label.span, label.name);
ExprAgain(label) => {
label.ident.map(|ident| {
if let Ok(loop_id) = label.loop_id.into() {
visitor.visit_def_mention(Def::Label(loop_id));
}
visitor.visit_name(ident.span, ident.node.name);
});
}
ExprRet(ref optional_expression) => {
walk_list!(visitor, visit_expr, optional_expression);
Expand Down
172 changes: 134 additions & 38 deletions src/librustc/hir/lowering.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ use util::nodemap::{DefIdMap, NodeMap, FxHashMap};

use std::collections::BTreeMap;
use std::iter;
use std::mem;

use syntax::attr;
use syntax::ast::*;
Expand Down Expand Up @@ -79,6 +80,9 @@ 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 •
edited
Loading

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.

is_in_loop_condition: bool,

type_def_lifetime_params: DefIdMap<usize>,
}

Expand Down Expand Up @@ -112,6 +116,8 @@ pub fn lower_crate(sess: &Session,
trait_items: BTreeMap::new(),
impl_items: BTreeMap::new(),
bodies: FxHashMap(),
loop_scopes: Vec::new(),
is_in_loop_condition: false,
type_def_lifetime_params: DefIdMap(),
}.lower_crate(krate)
}
Expand Down Expand Up @@ -244,6 +250,55 @@ impl<'a> LoweringContext<'a> {
span
}

fn with_loop_scope<T, F>(&mut self, loop_id: NodeId, f: F) -> T
where F: FnOnce(&mut LoweringContext) -> T
{
// We're no longer in the base loop's condition; we're in another loop.
let was_in_loop_condition = self.is_in_loop_condition;
self.is_in_loop_condition = false;

let len = self.loop_scopes.len();
self.loop_scopes.push(loop_id);

let result = f(self);
assert_eq!(len + 1, self.loop_scopes.len(),
"Loop scopes should be added and removed in stack order");

self.loop_scopes.pop().unwrap();

self.is_in_loop_condition = was_in_loop_condition;

result
}

fn with_loop_condition_scope<T, F>(&mut self, f: F) -> T
where F: FnOnce(&mut LoweringContext) -> T
{
let was_in_loop_condition = self.is_in_loop_condition;
self.is_in_loop_condition = true;

let result = f(self);

self.is_in_loop_condition = was_in_loop_condition;

result
}

fn with_new_loop_scopes<T, F>(&mut self, f: F) -> T
where F: FnOnce(&mut LoweringContext) -> T
{
let was_in_loop_condition = self.is_in_loop_condition;
self.is_in_loop_condition = false;

let loop_scopes = mem::replace(&mut self.loop_scopes, Vec::new());
let result = f(self);
mem::replace(&mut self.loop_scopes, loop_scopes);

self.is_in_loop_condition = was_in_loop_condition;

result
}

fn with_parent_def<T, F>(&mut self, parent_id: NodeId, f: F) -> T
where F: FnOnce(&mut LoweringContext) -> T
{
Expand Down Expand Up @@ -271,17 +326,24 @@ impl<'a> LoweringContext<'a> {
o_id.map(|sp_ident| respan(sp_ident.span, sp_ident.node.name))
}

fn lower_label(&mut self, id: NodeId, label: Option<Spanned<Ident>>) -> Option<hir::Label> {
label.map(|sp_ident| {
hir::Label {
span: sp_ident.span,
name: sp_ident.node.name,
loop_id: match self.expect_full_def(id) {
Def::Label(loop_id) => loop_id,
_ => DUMMY_NODE_ID
fn lower_destination(&mut self, destination: Option<(NodeId, Spanned<Ident>)>)
-> hir::Destination
{
match destination {
Some((id, label_ident)) => hir::Destination {
ident: Some(label_ident),
loop_id: if let Def::Label(loop_id) = self.expect_full_def(id) {
hir::LoopIdResult::Ok(loop_id)
} else {
hir::LoopIdResult::Err(hir::LoopIdError::UnresolvedLabel)
}
},
None => hir::Destination {
ident: None,
loop_id: self.loop_scopes.last().map(|innermost_loop_id| Ok(*innermost_loop_id))
.unwrap_or(Err(hir::LoopIdError::OutsideLoopScope)).into()
}
})
}
}

fn lower_attrs(&mut self, attrs: &Vec<Attribute>) -> hir::HirVec<Attribute> {
Expand Down Expand Up @@ -992,15 +1054,17 @@ impl<'a> LoweringContext<'a> {
self.record_body(value, None))
}
ItemKind::Fn(ref decl, unsafety, constness, abi, ref generics, ref body) => {
let body = self.lower_block(body);
let body = self.expr_block(body, ThinVec::new());
let body_id = self.record_body(body, Some(decl));
hir::ItemFn(self.lower_fn_decl(decl),
self.lower_unsafety(unsafety),
self.lower_constness(constness),
abi,
self.lower_generics(generics),
body_id)
self.with_new_loop_scopes(|this| {
let body = this.lower_block(body);
let body = this.expr_block(body, ThinVec::new());
let body_id = this.record_body(body, Some(decl));
hir::ItemFn(this.lower_fn_decl(decl),
this.lower_unsafety(unsafety),
this.lower_constness(constness),
abi,
this.lower_generics(generics),
body_id)
})
}
ItemKind::Mod(ref m) => hir::ItemMod(self.lower_mod(m)),
ItemKind::ForeignMod(ref nm) => hir::ItemForeignMod(self.lower_foreign_mod(nm)),
Expand Down Expand Up @@ -1562,26 +1626,32 @@ impl<'a> LoweringContext<'a> {
hir::ExprIf(P(self.lower_expr(cond)), self.lower_block(blk), else_opt)
}
ExprKind::While(ref cond, ref body, opt_ident) => {
hir::ExprWhile(P(self.lower_expr(cond)), self.lower_block(body),
self.lower_opt_sp_ident(opt_ident))
self.with_loop_scope(e.id, |this|
hir::ExprWhile(
this.with_loop_condition_scope(|this| P(this.lower_expr(cond))),
this.lower_block(body),
this.lower_opt_sp_ident(opt_ident)))
}
ExprKind::Loop(ref body, opt_ident) => {
hir::ExprLoop(self.lower_block(body),
self.lower_opt_sp_ident(opt_ident),
hir::LoopSource::Loop)
self.with_loop_scope(e.id, |this|
hir::ExprLoop(this.lower_block(body),
this.lower_opt_sp_ident(opt_ident),
hir::LoopSource::Loop))
}
ExprKind::Match(ref expr, ref arms) => {
hir::ExprMatch(P(self.lower_expr(expr)),
arms.iter().map(|x| self.lower_arm(x)).collect(),
hir::MatchSource::Normal)
}
ExprKind::Closure(capture_clause, ref decl, ref body, fn_decl_span) => {
self.with_parent_def(e.id, |this| {
let expr = this.lower_expr(body);
hir::ExprClosure(this.lower_capture_clause(capture_clause),
this.lower_fn_decl(decl),
this.record_body(expr, Some(decl)),
fn_decl_span)
self.with_new_loop_scopes(|this| {
this.with_parent_def(e.id, |this| {
let expr = this.lower_expr(body);
hir::ExprClosure(this.lower_capture_clause(capture_clause),
this.lower_fn_decl(decl),
this.record_body(expr, Some(decl)),
fn_decl_span)
})
})
}
ExprKind::Block(ref blk) => hir::ExprBlock(self.lower_block(blk)),
Expand Down Expand Up @@ -1660,10 +1730,29 @@ 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),
opt_expr.as_ref().map(|x| P(self.lower_expr(x))))
let label_result = if self.is_in_loop_condition && opt_ident.is_none() {
hir::Destination {
ident: opt_ident,
loop_id: Err(hir::LoopIdError::UnlabeledCfInWhileCondition).into(),
}
} else {
self.lower_destination(opt_ident.map(|ident| (e.id, ident)))
};
hir::ExprBreak(
label_result,
opt_expr.as_ref().map(|x| P(self.lower_expr(x))))
}
ExprKind::Continue(opt_ident) => hir::ExprAgain(self.lower_label(e.id, opt_ident)),
ExprKind::Continue(opt_ident) =>
hir::ExprAgain(
if self.is_in_loop_condition && opt_ident.is_none() {
hir::Destination {
ident: opt_ident,
loop_id: Err(
hir::LoopIdError::UnlabeledCfInWhileCondition).into(),
}
} else {
self.lower_destination(opt_ident.map( |ident| (e.id, ident)))
}),
ExprKind::Ret(ref e) => hir::ExprRet(e.as_ref().map(|x| P(self.lower_expr(x)))),
ExprKind::InlineAsm(ref asm) => {
let hir_asm = hir::InlineAsm {
Expand Down Expand Up @@ -1804,9 +1893,16 @@ impl<'a> LoweringContext<'a> {
// }
// }

// Note that the block AND the condition are evaluated in the loop scope.
// This is done to allow `break` from inside the condition of the loop.
let (body, break_expr, sub_expr) = self.with_loop_scope(e.id, |this| (
this.lower_block(body),
this.expr_break(e.span, ThinVec::new()),
this.with_loop_condition_scope(|this| P(this.lower_expr(sub_expr))),
));

// `<pat> => <body>`
let pat_arm = {
let body = self.lower_block(body);
let body_expr = P(self.expr_block(body, ThinVec::new()));
let pat = self.lower_pat(pat);
self.arm(hir_vec![pat], body_expr)
Expand All @@ -1815,13 +1911,11 @@ impl<'a> LoweringContext<'a> {
// `_ => break`
let break_arm = {
let pat_under = self.pat_wild(e.span);
let break_expr = self.expr_break(e.span, ThinVec::new());
self.arm(hir_vec![pat_under], break_expr)
};

// `match <sub_expr> { ... }`
let arms = hir_vec![pat_arm, break_arm];
let sub_expr = P(self.lower_expr(sub_expr));
let match_expr = self.expr(e.span,
hir::ExprMatch(sub_expr,
arms,
Expand Down Expand Up @@ -1863,7 +1957,7 @@ impl<'a> LoweringContext<'a> {

// `::std::option::Option::Some(<pat>) => <body>`
let pat_arm = {
let body_block = self.lower_block(body);
let body_block = self.with_loop_scope(e.id, |this| this.lower_block(body));
let body_expr = P(self.expr_block(body_block, ThinVec::new()));
let pat = self.lower_pat(pat);
let some_pat = self.pat_some(e.span, pat);
Expand All @@ -1873,7 +1967,8 @@ impl<'a> LoweringContext<'a> {

// `::std::option::Option::None => break`
let break_arm = {
let break_expr = self.expr_break(e.span, ThinVec::new());
let break_expr = self.with_loop_scope(e.id, |this|
this.expr_break(e.span, ThinVec::new()));
let pat = self.pat_none(e.span);
self.arm(hir_vec![pat], break_expr)
};
Expand Down Expand Up @@ -2151,7 +2246,8 @@ impl<'a> LoweringContext<'a> {
}

fn expr_break(&mut self, span: Span, attrs: ThinVec<Attribute>) -> P<hir::Expr> {
P(self.expr(span, hir::ExprBreak(None, None), attrs))
let expr_break = hir::ExprBreak(self.lower_destination(None), None);
P(self.expr(span, expr_break, attrs))
}

fn expr_call(&mut self, span: Span, e: P<hir::Expr>, args: hir::HirVec<hir::Expr>)
Expand Down
Loading