Skip to content

Commit

Permalink
Make break and continue hygienic
Browse files Browse the repository at this point in the history
Makes labelled loops hygiene by performing renaming of the labels
defined in e.g. `'x: loop { ... }` and then used in break and continue
statements within loop body so that they act hygienically when used with
macros.

Closes #12262.
  • Loading branch information
edwardw committed Feb 23, 2014
1 parent 551da06 commit 386db05
Show file tree
Hide file tree
Showing 18 changed files with 262 additions and 28 deletions.
2 changes: 1 addition & 1 deletion src/librustc/middle/cfg/construct.rs
Original file line number Diff line number Diff line change
Expand Up @@ -490,7 +490,7 @@ impl CFGBuilder {

fn find_scope(&self,
expr: @ast::Expr,
label: Option<ast::Name>) -> LoopScope {
label: Option<ast::Ident>) -> LoopScope {
match label {
None => {
return *self.loop_scopes.last().unwrap();
Expand Down
2 changes: 1 addition & 1 deletion src/librustc/middle/dataflow.rs
Original file line number Diff line number Diff line change
Expand Up @@ -770,7 +770,7 @@ impl<'a, O:DataFlowOperator> PropagationContext<'a, O> {

fn find_scope<'a>(&self,
expr: &ast::Expr,
label: Option<ast::Name>,
label: Option<ast::Ident>,
loop_scopes: &'a mut ~[LoopScope]) -> &'a mut LoopScope {
let index = match label {
None => {
Expand Down
2 changes: 1 addition & 1 deletion src/librustc/middle/liveness.rs
Original file line number Diff line number Diff line change
Expand Up @@ -747,7 +747,7 @@ impl Liveness {
}

pub fn find_loop_scope(&self,
opt_label: Option<Name>,
opt_label: Option<Ident>,
id: NodeId,
sp: Span)
-> NodeId {
Expand Down
9 changes: 5 additions & 4 deletions src/librustc/middle/resolve.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5206,13 +5206,13 @@ impl Resolver {
ExprLoop(_, Some(label)) => {
self.with_label_rib(|this| {
let def_like = DlDef(DefLabel(expr.id));
// plain insert (no renaming)
{
let mut label_ribs = this.label_ribs.borrow_mut();
let rib = label_ribs.get()[label_ribs.get().len() -
1];
let mut bindings = rib.bindings.borrow_mut();
bindings.get().insert(label.name, def_like);
let renamed = mtwt_resolve(label);
bindings.get().insert(renamed, def_like);
}

visit::walk_expr(this, expr, ());
Expand All @@ -5223,11 +5223,12 @@ impl Resolver {

ExprBreak(Some(label)) | ExprAgain(Some(label)) => {
let mut label_ribs = self.label_ribs.borrow_mut();
match self.search_ribs(label_ribs.get(), label, expr.span) {
let renamed = mtwt_resolve(label);
match self.search_ribs(label_ribs.get(), renamed, expr.span) {
None =>
self.resolve_error(expr.span,
format!("use of undeclared label `{}`",
token::get_name(label))),
token::get_ident(label))),
Some(DlDef(def @ DefLabel(_))) => {
// Since this def is a label, it is never read.
self.record_def(expr.id, (def, LastMod(AllPublic)))
Expand Down
8 changes: 4 additions & 4 deletions src/librustc/middle/trans/controlflow.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ use util::ppaux::Repr;
use middle::trans::type_::Type;

use syntax::ast;
use syntax::ast::Name;
use syntax::ast::Ident;
use syntax::ast_util;
use syntax::codemap::Span;
use syntax::parse::token::InternedString;
Expand Down Expand Up @@ -260,7 +260,7 @@ pub fn trans_loop<'a>(bcx:&'a Block<'a>,

pub fn trans_break_cont<'a>(bcx: &'a Block<'a>,
expr_id: ast::NodeId,
opt_label: Option<Name>,
opt_label: Option<Ident>,
exit: uint)
-> &'a Block<'a> {
let _icx = push_ctxt("trans_break_cont");
Expand Down Expand Up @@ -293,14 +293,14 @@ pub fn trans_break_cont<'a>(bcx: &'a Block<'a>,

pub fn trans_break<'a>(bcx: &'a Block<'a>,
expr_id: ast::NodeId,
label_opt: Option<Name>)
label_opt: Option<Ident>)
-> &'a Block<'a> {
return trans_break_cont(bcx, expr_id, label_opt, cleanup::EXIT_BREAK);
}

pub fn trans_cont<'a>(bcx: &'a Block<'a>,
expr_id: ast::NodeId,
label_opt: Option<Name>)
label_opt: Option<Ident>)
-> &'a Block<'a> {
return trans_break_cont(bcx, expr_id, label_opt, cleanup::EXIT_LOOP);
}
Expand Down
13 changes: 9 additions & 4 deletions src/libsyntax/ast.rs
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,13 @@ impl Eq for Ident {
// if it should be non-hygienic (most things are), just compare the
// 'name' fields of the idents. Or, even better, replace the idents
// with Name's.
fail!("not allowed to compare these idents: {:?}, {:?}.
Probably related to issue \\#6993", self, other);
//
// On the other hand, if the comparison does need to be hygienic,
// one example and its non-hygienic counterpart would be:
// syntax::parse::token::mtwt_token_eq
// syntax::ext::tt::macro_parser::token_name_eq
fail!("not allowed to compare these idents: {:?}, {:?}. \
Probably related to issue \\#6993", self, other);
}
}
fn ne(&self, other: &Ident) -> bool {
Expand Down Expand Up @@ -564,8 +569,8 @@ pub enum Expr_ {
ExprPath(Path),

ExprAddrOf(Mutability, @Expr),
ExprBreak(Option<Name>),
ExprAgain(Option<Name>),
ExprBreak(Option<Ident>),
ExprAgain(Option<Ident>),
ExprRet(Option<@Expr>),

/// Gets the log level for the enclosing module
Expand Down
31 changes: 29 additions & 2 deletions src/libsyntax/ext/expand.rs
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,8 @@ pub fn expand_expr(e: @ast::Expr, fld: &mut MacroExpander) -> @ast::Expr {
// Expand any interior macros etc.
// NB: we don't fold pats yet. Curious.
let src_expr = fld.fold_expr(src_expr).clone();
// Rename label before expansion.
let (opt_ident, src_loop_block) = rename_loop_label(opt_ident, src_loop_block, fld);
let src_loop_block = fld.fold_block(src_loop_block);

let span = e.span;
Expand All @@ -165,8 +167,7 @@ pub fn expand_expr(e: @ast::Expr, fld: &mut MacroExpander) -> @ast::Expr {

// `None => break ['<ident>];`
let none_arm = {
// FIXME #6993: this map goes away:
let break_expr = fld.cx.expr(span, ast::ExprBreak(opt_ident.map(|x| x.name)));
let break_expr = fld.cx.expr(span, ast::ExprBreak(opt_ident));
let none_pat = fld.cx.pat_ident(span, none_ident);
fld.cx.arm(span, ~[none_pat], break_expr)
};
Expand Down Expand Up @@ -199,10 +200,36 @@ pub fn expand_expr(e: @ast::Expr, fld: &mut MacroExpander) -> @ast::Expr {
fld.cx.expr_match(span, discrim, ~[arm])
}

ast::ExprLoop(loop_block, opt_ident) => {
let (opt_ident, loop_block) =
rename_loop_label(opt_ident, loop_block, fld);
let loop_block = fld.fold_block(loop_block);
fld.cx.expr(e.span, ast::ExprLoop(loop_block, opt_ident))
}

_ => noop_fold_expr(e, fld)
}
}

// Rename loop label and its all occurrences inside the loop body
fn rename_loop_label(opt_ident: Option<Ident>,
loop_block: P<Block>,
fld: &mut MacroExpander) -> (Option<Ident>, P<Block>) {
match opt_ident {
Some(label) => {
// Generate fresh label and add to the existing pending renames
let new_label = fresh_name(&label);
let rename = (label, new_label);
fld.extsbox.info().pending_renames.push(rename);
let mut pending_renames = ~[rename];
let mut rename_fld = renames_to_fold(&mut pending_renames);
(Some(rename_fld.fold_ident(label)),
rename_fld.fold_block(loop_block))
}
None => (None, loop_block)
}
}

// eval $e with a new exts frame:
macro_rules! with_exts_frame (
($extsboxexpr:expr,$macros_escape:expr,$e:expr) =>
Expand Down
5 changes: 3 additions & 2 deletions src/libsyntax/ext/tt/macro_parser.rs
Original file line number Diff line number Diff line change
Expand Up @@ -218,8 +218,9 @@ pub fn parse_or_else<R: Reader>(sess: @ParseSess,
// perform a token equality check, ignoring syntax context (that is, an unhygienic comparison)
pub fn token_name_eq(t1 : &Token, t2 : &Token) -> bool {
match (t1,t2) {
(&token::IDENT(id1,_),&token::IDENT(id2,_)) =>
id1.name == id2.name,
(&token::IDENT(id1,_),&token::IDENT(id2,_))
| (&token::LIFETIME(id1),&token::LIFETIME(id2)) =>
id1.name == id2.name,
_ => *t1 == *t2
}
}
Expand Down
22 changes: 20 additions & 2 deletions src/libsyntax/fold.rs
Original file line number Diff line number Diff line change
Expand Up @@ -353,6 +353,23 @@ fn fold_arg_<T: Folder>(a: &Arg, fld: &mut T) -> Arg {

// build a new vector of tts by appling the Folder's fold_ident to
// all of the identifiers in the token trees.
//
// This is part of hygiene magic. As far as hygiene is concerned, there
// are three types of let pattern bindings or loop labels:
// - those defined and used in non-macro part of the program
// - those used as part of macro invocation arguments
// - those defined and used inside macro definitions
// Lexically, type 1 and 2 are in one group and type 3 the other. If they
// clash, in order for let and loop label to work hygienically, one group
// or the other needs to be renamed. The problem is that type 2 and 3 are
// parsed together (inside the macro expand function). After being parsed and
// AST being constructed, they can no longer be distinguished from each other.
//
// For that reason, type 2 let bindings and loop labels are actually renamed
// in the form of tokens instead of AST nodes, here. There are wasted effort
// since many token::IDENT are not necessary part of let bindings and most
// token::LIFETIME are certainly not loop labels. But we can't tell in their
// token form. So this is less ideal and hacky but it works.
pub fn fold_tts<T: Folder>(tts: &[TokenTree], fld: &mut T) -> ~[TokenTree] {
tts.map(|tt| {
match *tt {
Expand All @@ -376,6 +393,7 @@ fn maybe_fold_ident<T: Folder>(t: &token::Token, fld: &mut T) -> token::Token {
token::IDENT(id, followed_by_colons) => {
token::IDENT(fld.fold_ident(id), followed_by_colons)
}
token::LIFETIME(id) => token::LIFETIME(fld.fold_ident(id)),
_ => (*t).clone()
}
}
Expand Down Expand Up @@ -802,8 +820,8 @@ pub fn noop_fold_expr<T: Folder>(e: @Expr, folder: &mut T) -> @Expr {
}
ExprPath(ref pth) => ExprPath(folder.fold_path(pth)),
ExprLogLevel => ExprLogLevel,
ExprBreak(opt_ident) => ExprBreak(opt_ident),
ExprAgain(opt_ident) => ExprAgain(opt_ident),
ExprBreak(opt_ident) => ExprBreak(opt_ident.map(|x| folder.fold_ident(x))),
ExprAgain(opt_ident) => ExprAgain(opt_ident.map(|x| folder.fold_ident(x))),
ExprRet(ref e) => {
ExprRet(e.map(|x| folder.fold_expr(x)))
}
Expand Down
6 changes: 3 additions & 3 deletions src/libsyntax/parse/parser.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1822,7 +1822,7 @@ impl Parser {
let ex = if Parser::token_is_lifetime(&self.token) {
let lifetime = self.get_lifetime();
self.bump();
ExprAgain(Some(lifetime.name))
ExprAgain(Some(lifetime))
} else {
ExprAgain(None)
};
Expand Down Expand Up @@ -1885,7 +1885,7 @@ impl Parser {
if Parser::token_is_lifetime(&self.token) {
let lifetime = self.get_lifetime();
self.bump();
ex = ExprBreak(Some(lifetime.name));
ex = ExprBreak(Some(lifetime));
} else {
ex = ExprBreak(None);
}
Expand Down Expand Up @@ -2579,7 +2579,7 @@ impl Parser {
let ex = if Parser::token_is_lifetime(&self.token) {
let lifetime = self.get_lifetime();
self.bump();
ExprAgain(Some(lifetime.name))
ExprAgain(Some(lifetime))
} else {
ExprAgain(None)
};
Expand Down
4 changes: 2 additions & 2 deletions src/libsyntax/parse/token.rs
Original file line number Diff line number Diff line change
Expand Up @@ -704,8 +704,8 @@ pub fn is_reserved_keyword(tok: &Token) -> bool {

pub fn mtwt_token_eq(t1 : &Token, t2 : &Token) -> bool {
match (t1,t2) {
(&IDENT(id1,_),&IDENT(id2,_)) =>
ast_util::mtwt_resolve(id1) == ast_util::mtwt_resolve(id2),
(&IDENT(id1,_),&IDENT(id2,_)) | (&LIFETIME(id1),&LIFETIME(id2)) =>
ast_util::mtwt_resolve(id1) == ast_util::mtwt_resolve(id2),
_ => *t1 == *t2
}
}
Expand Down
4 changes: 2 additions & 2 deletions src/libsyntax/print/pprust.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1471,7 +1471,7 @@ pub fn print_expr(s: &mut State, expr: &ast::Expr) -> io::IoResult<()> {
try!(space(&mut s.s));
for ident in opt_ident.iter() {
try!(word(&mut s.s, "'"));
try!(print_name(s, *ident));
try!(print_ident(s, *ident));
try!(space(&mut s.s));
}
}
Expand All @@ -1480,7 +1480,7 @@ pub fn print_expr(s: &mut State, expr: &ast::Expr) -> io::IoResult<()> {
try!(space(&mut s.s));
for ident in opt_ident.iter() {
try!(word(&mut s.s, "'"));
try!(print_name(s, *ident));
try!(print_ident(s, *ident));
try!(space(&mut s.s))
}
}
Expand Down
19 changes: 19 additions & 0 deletions src/test/compile-fail/hygienic-label-1.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
// Copyright 2012-2014 The Rust Project Developers. See the COPYRIGHT
// file at the top-level directory of this distribution and at
// http://rust-lang.org/COPYRIGHT.
//
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.

#[feature(macro_rules)];

macro_rules! foo {
() => { break 'x; }
}

pub fn main() {
'x: loop { foo!() } //~ ERROR use of undeclared label `x`
}
19 changes: 19 additions & 0 deletions src/test/compile-fail/hygienic-label-2.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
// Copyright 2012-2014 The Rust Project Developers. See the COPYRIGHT
// file at the top-level directory of this distribution and at
// http://rust-lang.org/COPYRIGHT.
//
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.

#[feature(macro_rules)];

macro_rules! foo {
($e: expr) => { 'x: loop { $e } }
}

pub fn main() {
foo!(break 'x); //~ ERROR use of undeclared label `x`
}
21 changes: 21 additions & 0 deletions src/test/compile-fail/hygienic-label-3.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
// Copyright 2012-2014 The Rust Project Developers. See the COPYRIGHT
// file at the top-level directory of this distribution and at
// http://rust-lang.org/COPYRIGHT.
//
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.

#[feature(macro_rules)];

macro_rules! foo {
() => { break 'x; }
}

pub fn main() {
'x: for _ in range(0,1) {
foo!() //~ ERROR use of undeclared label `x`
};
}
19 changes: 19 additions & 0 deletions src/test/compile-fail/hygienic-label-4.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
// Copyright 2012-2014 The Rust Project Developers. See the COPYRIGHT
// file at the top-level directory of this distribution and at
// http://rust-lang.org/COPYRIGHT.
//
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.

#[feature(macro_rules)];

macro_rules! foo {
($e: expr) => { 'x: for _ in range(0,1) { $e } }
}

pub fn main() {
foo!(break 'x); //~ ERROR use of undeclared label `x`
}
Loading

9 comments on commit 386db05

@bors
Copy link
Contributor

@bors bors commented on 386db05 Feb 23, 2014

Choose a reason for hiding this comment

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

saw approval from cmr
at edwardw@386db05

@bors
Copy link
Contributor

@bors bors commented on 386db05 Feb 23, 2014

Choose a reason for hiding this comment

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

merging edwardw/rust/hygienic-break-continue = 386db05 into auto

@bors
Copy link
Contributor

@bors bors commented on 386db05 Feb 23, 2014

Choose a reason for hiding this comment

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

edwardw/rust/hygienic-break-continue = 386db05 merged ok, testing candidate = 71f29014

@bors
Copy link
Contributor

@bors bors commented on 386db05 Feb 23, 2014

Choose a reason for hiding this comment

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

@bors
Copy link
Contributor

@bors bors commented on 386db05 Feb 23, 2014

Choose a reason for hiding this comment

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

saw approval from cmr
at edwardw@386db05

@bors
Copy link
Contributor

@bors bors commented on 386db05 Feb 23, 2014

Choose a reason for hiding this comment

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

merging edwardw/rust/hygienic-break-continue = 386db05 into auto

@bors
Copy link
Contributor

@bors bors commented on 386db05 Feb 23, 2014

Choose a reason for hiding this comment

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

edwardw/rust/hygienic-break-continue = 386db05 merged ok, testing candidate = 329fcd4

@bors
Copy link
Contributor

@bors bors commented on 386db05 Feb 24, 2014

Choose a reason for hiding this comment

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

@bors
Copy link
Contributor

@bors bors commented on 386db05 Feb 24, 2014

Choose a reason for hiding this comment

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

fast-forwarding master to auto = 329fcd4

Please sign in to comment.