From b8c5e4650598f43a2e362bcf6f4e919440a997bb Mon Sep 17 00:00:00 2001 From: John Clements Date: Wed, 25 Jun 2014 15:20:01 -0700 Subject: [PATCH 01/18] working on hygiene --- src/libsyntax/ast.rs | 1 + src/libsyntax/ext/expand.rs | 62 ++++++++++++++++++++++++++++++------- 2 files changed, 51 insertions(+), 12 deletions(-) diff --git a/src/libsyntax/ast.rs b/src/libsyntax/ast.rs index d24c2be5a74d..529b460adcd3 100644 --- a/src/libsyntax/ast.rs +++ b/src/libsyntax/ast.rs @@ -401,6 +401,7 @@ pub enum Decl_ { DeclItem(Gc), } +/// represents one arm of a 'match' #[deriving(Clone, PartialEq, Eq, Encodable, Decodable, Hash)] pub struct Arm { pub attrs: Vec, diff --git a/src/libsyntax/ext/expand.rs b/src/libsyntax/ext/expand.rs index 321c56d4bbfd..11f50d685f88 100644 --- a/src/libsyntax/ext/expand.rs +++ b/src/libsyntax/ext/expand.rs @@ -31,6 +31,7 @@ use util::small_vector::SmallVector; use std::gc::{Gc, GC}; + pub fn expand_expr(e: Gc, fld: &mut MacroExpander) -> Gc { match e.node { // expr_mac should really be expr_ext or something; it's the @@ -130,8 +131,6 @@ pub fn expand_expr(e: Gc, fld: &mut MacroExpander) -> Gc { // From: `[':] for in ` // FIXME #6993: change type of opt_ident to Option ast::ExprForLoop(src_pat, src_expr, src_loop_block, opt_ident) => { - // Expand any interior macros etc. - // NB: we don't fold pats yet. Curious. let span = e.span; @@ -281,7 +280,7 @@ macro_rules! with_exts_frame ( ) // When we enter a module, record it, for the sake of `module!` -pub fn expand_item(it: Gc, fld: &mut MacroExpander) +fn expand_item(it: Gc, fld: &mut MacroExpander) -> SmallVector> { let it = expand_item_modifiers(it, fld); @@ -386,13 +385,13 @@ fn expand_item_modifiers(mut it: Gc, fld: &mut MacroExpander) } // does this attribute list contain "macro_escape" ? -pub fn contains_macro_escape(attrs: &[ast::Attribute]) -> bool { +fn contains_macro_escape(attrs: &[ast::Attribute]) -> bool { attr::contains_name(attrs, "macro_escape") } // Support for item-position macro invocations, exactly the same // logic as for expression-position macro invocations. -pub fn expand_item_mac(it: Gc, fld: &mut MacroExpander) +fn expand_item_mac(it: Gc, fld: &mut MacroExpander) -> SmallVector> { let (pth, tts) = match it.node { ItemMac(codemap::Spanned { @@ -498,7 +497,7 @@ pub fn expand_item_mac(it: Gc, fld: &mut MacroExpander) } // expand a stmt -pub fn expand_stmt(s: &Stmt, fld: &mut MacroExpander) -> SmallVector> { +fn expand_stmt(s: &Stmt, fld: &mut MacroExpander) -> SmallVector> { // why the copying here and not in expand_expr? // looks like classic changed-in-only-one-place let (pth, tts, semi) = match s.node { @@ -659,6 +658,42 @@ fn expand_non_macro_stmt(s: &Stmt, fld: &mut MacroExpander) } } +fn expand_arm(arm: &ast::Arm, fld: &mut MacroExpander) -> ast::Arm { + if a.pats.len() == 0 { + fail!("encountered match arm with 0 patterns"); + } + let first_pat = match a.pats.get(0) { + + } + // code duplicated from 'let', above. Perhaps this can be lifted + // into a separate function: + let expanded_pat = fld.fold_pat(pat); + let mut name_finder = new_name_finder(Vec::new()); + name_finder.visit_pat(&*expanded_pat,()); + let mut new_pending_renames = Vec::new(); + for ident in name_finder.ident_accumulator.iter() { + let new_name = fresh_name(ident); + new_pending_renames.push((*ident,new_name)); + } + let rewritten_pat = { + let mut rename_fld = + renames_to_fold(&mut new_pending_renames); + // rewrite the pattern using the new names (the old + // ones have already been applied): + rename_fld.fold_pat(expanded_pat) + }; + + let bound_names + ast::Arm { + attrs: a.attrs.iter().map(|x| self.fold_attribute(*x)).collect(), + pats: a.pats.iter().map(|x| self.fold_pat(*x)).collect(), + guard: a.guard.map(|x| self.fold_expr(x)), + body: self.fold_expr(a.body), + } +} + + + // a visitor that extracts the pat_ident (binding) paths // from a given thingy and puts them in a mutable // array (passed in to the traversal). @@ -711,14 +746,14 @@ fn new_name_finder(idents: Vec ) -> NameFinderContext { } // expand a block. pushes a new exts_frame, then calls expand_block_elts -pub fn expand_block(blk: &Block, fld: &mut MacroExpander) -> P { +fn expand_block(blk: &Block, fld: &mut MacroExpander) -> P { // see note below about treatment of exts table with_exts_frame!(fld.extsbox,false, expand_block_elts(blk, fld)) } // expand the elements of a block. -pub fn expand_block_elts(b: &Block, fld: &mut MacroExpander) -> P { +fn expand_block_elts(b: &Block, fld: &mut MacroExpander) -> P { let new_view_items = b.view_items.iter().map(|x| fld.fold_view_item(x)).collect(); let new_stmts = b.stmts.iter().flat_map(|x| { @@ -747,7 +782,7 @@ pub fn expand_block_elts(b: &Block, fld: &mut MacroExpander) -> P { }) } -pub fn expand_pat(p: Gc, fld: &mut MacroExpander) -> Gc { +fn expand_pat(p: Gc, fld: &mut MacroExpander) -> Gc { let (pth, tts) = match p.node { PatMac(ref mac) => { match mac.node { @@ -842,13 +877,13 @@ impl<'a> Folder for IdentRenamer<'a> { // given a mutable list of renames, return a tree-folder that applies those // renames. -pub fn renames_to_fold<'a>(renames: &'a mut RenameList) -> IdentRenamer<'a> { +fn renames_to_fold<'a>(renames: &'a mut RenameList) -> IdentRenamer<'a> { IdentRenamer { renames: renames, } } -pub fn new_span(cx: &ExtCtxt, sp: Span) -> Span { +fn new_span(cx: &ExtCtxt, sp: Span) -> Span { /* this discards information in the case of macro-defining macros */ Span { lo: sp.lo, @@ -883,6 +918,10 @@ impl<'a, 'b> Folder for MacroExpander<'a, 'b> { expand_block(&*block, self) } + fn fold_arm(&mut self, arm: &ast::Arm) -> ast::Arm { + expand_arm(arm, self) + } + fn new_span(&mut self, span: Span) -> Span { new_span(self.cx, span) } @@ -1248,7 +1287,6 @@ mod test { // FIXME #9384, match variable hygiene. Should expand into // fn z() {match 8 {x_1 => {match 9 {x_2 | x_2 => x_2 + x_1}}}} - #[ignore] #[test] fn issue_9384(){ run_renaming_test( &("macro_rules! bad_macro (($ex:expr) => ({match 9 {x | x => x + $ex}})) From 4b833e24c39112d2e59cfc1287483c2fe61c34e4 Mon Sep 17 00:00:00 2001 From: John Clements Date: Wed, 25 Jun 2014 18:15:07 -0700 Subject: [PATCH 02/18] make fold_attribute part of Folder trait --- src/libsyntax/fold.rs | 48 ++++++++++++++++++++++--------------------- 1 file changed, 25 insertions(+), 23 deletions(-) diff --git a/src/libsyntax/fold.rs b/src/libsyntax/fold.rs index 6d2b0ceed8bf..01742ca131e9 100644 --- a/src/libsyntax/fold.rs +++ b/src/libsyntax/fold.rs @@ -86,7 +86,7 @@ pub trait Folder { kind: sf.node.kind, id: id, ty: self.fold_ty(sf.node.ty), - attrs: sf.node.attrs.iter().map(|e| fold_attribute_(*e, self)).collect() + attrs: sf.node.attrs.iter().map(|e| self.fold_attribute(*e)).collect() }, span: self.new_span(sf.span) } @@ -118,7 +118,7 @@ pub trait Folder { fn fold_arm(&mut self, a: &Arm) -> Arm { Arm { - attrs: a.attrs.iter().map(|x| fold_attribute_(*x, self)).collect(), + attrs: a.attrs.iter().map(|x| self.fold_attribute(*x)).collect(), pats: a.pats.iter().map(|x| self.fold_pat(*x)).collect(), guard: a.guard.map(|x| self.fold_expr(x)), body: self.fold_expr(a.body), @@ -251,7 +251,7 @@ pub trait Folder { } } - let attrs = v.node.attrs.iter().map(|x| fold_attribute_(*x, self)).collect(); + let attrs = v.node.attrs.iter().map(|x| self.fold_attribute(*x)).collect(); let de = match v.node.disr_expr { Some(e) => Some(self.fold_expr(e)), @@ -344,6 +344,21 @@ pub trait Folder { fn fold_lifetime(&mut self, l: &Lifetime) -> Lifetime { noop_fold_lifetime(l, self) } + + //used in noop_fold_item and noop_fold_crate + fn fold_attribute(&mut self, at: Attribute) -> Attribute { + Spanned { + span: self.new_span(at.span), + node: ast::Attribute_ { + id: at.node.id, + style: at.node.style, + value: fold_meta_item_(at.node.value, self), + is_sugared_doc: at.node.is_sugared_doc + } + } + } + + } /* some little folds that probably aren't useful to have in Folder itself*/ @@ -364,19 +379,6 @@ fn fold_meta_item_(mi: Gc, fld: &mut T) -> Gc { span: fld.new_span(mi.span) } } -//used in noop_fold_item and noop_fold_crate -fn fold_attribute_(at: Attribute, fld: &mut T) -> Attribute { - Spanned { - span: fld.new_span(at.span), - node: ast::Attribute_ { - id: at.node.id, - style: at.node.style, - value: fold_meta_item_(at.node.value, fld), - is_sugared_doc: at.node.is_sugared_doc - } - } -} - //used in noop_fold_foreign_item and noop_fold_fn_decl fn fold_arg_(a: &Arg, fld: &mut T) -> Arg { let id = fld.new_id(a.id); // Needs to be first, for ast_map. @@ -526,7 +528,7 @@ fn fold_struct_field(f: &StructField, fld: &mut T) -> StructField { kind: f.node.kind, id: id, ty: fld.fold_ty(f.node.ty), - attrs: f.node.attrs.iter().map(|a| fold_attribute_(*a, fld)).collect(), + attrs: f.node.attrs.iter().map(|a| fld.fold_attribute(*a)).collect(), }, span: fld.new_span(f.span), } @@ -578,7 +580,7 @@ pub fn noop_fold_view_item(vi: &ViewItem, folder: &mut T) }; ViewItem { node: inner_view_item, - attrs: vi.attrs.iter().map(|a| fold_attribute_(*a, folder)).collect(), + attrs: vi.attrs.iter().map(|a| folder.fold_attribute(*a)).collect(), vis: vi.vis, span: folder.new_span(vi.span), } @@ -658,7 +660,7 @@ pub fn noop_fold_type_method(m: &TypeMethod, fld: &mut T) -> TypeMeth TypeMethod { id: id, ident: fld.fold_ident(m.ident), - attrs: m.attrs.iter().map(|a| fold_attribute_(*a, fld)).collect(), + attrs: m.attrs.iter().map(|a| fld.fold_attribute(*a)).collect(), fn_style: m.fn_style, decl: fld.fold_fn_decl(&*m.decl), generics: fold_generics(&m.generics, fld), @@ -681,7 +683,7 @@ pub fn noop_fold_mod(m: &Mod, folder: &mut T) -> Mod { pub fn noop_fold_crate(c: Crate, folder: &mut T) -> Crate { Crate { module: folder.fold_mod(&c.module), - attrs: c.attrs.iter().map(|x| fold_attribute_(*x, folder)).collect(), + attrs: c.attrs.iter().map(|x| folder.fold_attribute(*x)).collect(), config: c.config.iter().map(|x| fold_meta_item_(*x, folder)).collect(), span: folder.new_span(c.span), } @@ -702,7 +704,7 @@ pub fn noop_fold_item(i: &Item, SmallVector::one(box(GC) Item { id: id, ident: folder.fold_ident(ident), - attrs: i.attrs.iter().map(|e| fold_attribute_(*e, folder)).collect(), + attrs: i.attrs.iter().map(|e| folder.fold_attribute(*e)).collect(), node: node, vis: i.vis, span: folder.new_span(i.span) @@ -715,7 +717,7 @@ pub fn noop_fold_foreign_item(ni: &ForeignItem, box(GC) ForeignItem { id: id, ident: folder.fold_ident(ni.ident), - attrs: ni.attrs.iter().map(|x| fold_attribute_(*x, folder)).collect(), + attrs: ni.attrs.iter().map(|x| folder.fold_attribute(*x)).collect(), node: match ni.node { ForeignItemFn(ref fdec, ref generics) => { ForeignItemFn(P(FnDecl { @@ -739,7 +741,7 @@ pub fn noop_fold_method(m: &Method, folder: &mut T) -> Gc { box(GC) Method { id: id, ident: folder.fold_ident(m.ident), - attrs: m.attrs.iter().map(|a| fold_attribute_(*a, folder)).collect(), + attrs: m.attrs.iter().map(|a| folder.fold_attribute(*a)).collect(), generics: fold_generics(&m.generics, folder), explicit_self: folder.fold_explicit_self(&m.explicit_self), fn_style: m.fn_style, From c956f76c3c8bf42aa768c80e16ed16a3d7d370ea Mon Sep 17 00:00:00 2001 From: John Clements Date: Wed, 25 Jun 2014 19:41:16 -0700 Subject: [PATCH 03/18] replaced ignore-pretty with no-pretty-expanded Per @acrichto's suggestion, use the more narrowly focused exclusion. --- src/test/bench/msgsend-ring-mutex-arcs.rs | 2 +- src/test/bench/msgsend-ring-rw-arcs.rs | 2 +- src/test/bench/shootout-meteor.rs | 2 +- src/test/bench/shootout-spectralnorm.rs | 2 +- src/test/run-pass/backtrace.rs | 2 +- src/test/run-pass/deriving-cmp-generic-enum.rs | 2 +- src/test/run-pass/deriving-cmp-generic-struct-enum.rs | 2 +- src/test/run-pass/deriving-cmp-generic-struct.rs | 2 +- src/test/run-pass/deriving-cmp-generic-tuple-struct.rs | 2 +- src/test/run-pass/linear-for-loop.rs | 2 +- src/test/run-pass/task-comm-3.rs | 2 +- src/test/run-pass/unfold-cross-crate.rs | 2 +- src/test/run-pass/utf8.rs | 2 +- 13 files changed, 13 insertions(+), 13 deletions(-) diff --git a/src/test/bench/msgsend-ring-mutex-arcs.rs b/src/test/bench/msgsend-ring-mutex-arcs.rs index 716646da37ee..2b9abfbc350a 100644 --- a/src/test/bench/msgsend-ring-mutex-arcs.rs +++ b/src/test/bench/msgsend-ring-mutex-arcs.rs @@ -15,7 +15,7 @@ // This also serves as a pipes test, because Arcs are implemented with pipes. -// ignore-pretty FIXME #15189 +// no-pretty-expanded FIXME #15189 extern crate time; diff --git a/src/test/bench/msgsend-ring-rw-arcs.rs b/src/test/bench/msgsend-ring-rw-arcs.rs index 2580e6cad212..afed753f455b 100644 --- a/src/test/bench/msgsend-ring-rw-arcs.rs +++ b/src/test/bench/msgsend-ring-rw-arcs.rs @@ -15,7 +15,7 @@ // This also serves as a pipes test, because Arcs are implemented with pipes. -// ignore-pretty FIXME #15189 +// no-pretty-expanded FIXME #15189 extern crate time; diff --git a/src/test/bench/shootout-meteor.rs b/src/test/bench/shootout-meteor.rs index a0ff8e8c1f92..e13c53407e45 100644 --- a/src/test/bench/shootout-meteor.rs +++ b/src/test/bench/shootout-meteor.rs @@ -38,7 +38,7 @@ // ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED // OF THE POSSIBILITY OF SUCH DAMAGE. -// ignore-pretty FIXME #15189 +// no-pretty-expanded FIXME #15189 #![feature(phase)] #[phase(plugin)] extern crate green; diff --git a/src/test/bench/shootout-spectralnorm.rs b/src/test/bench/shootout-spectralnorm.rs index 949cf439df1c..8cec135944fd 100644 --- a/src/test/bench/shootout-spectralnorm.rs +++ b/src/test/bench/shootout-spectralnorm.rs @@ -8,7 +8,7 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. -// ignore-pretty FIXME #15189 +// no-pretty-expanded FIXME #15189 #![feature(phase)] #![allow(non_snake_case_functions)] diff --git a/src/test/run-pass/backtrace.rs b/src/test/run-pass/backtrace.rs index 3b74ec4add31..859fc62647a2 100644 --- a/src/test/run-pass/backtrace.rs +++ b/src/test/run-pass/backtrace.rs @@ -8,7 +8,7 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. -// ignore-pretty FIXME #15189 +// no-pretty-expanded FIXME #15189 // ignore-win32 FIXME #13259 extern crate native; diff --git a/src/test/run-pass/deriving-cmp-generic-enum.rs b/src/test/run-pass/deriving-cmp-generic-enum.rs index 4a9324dd201a..ccd0d967a51e 100644 --- a/src/test/run-pass/deriving-cmp-generic-enum.rs +++ b/src/test/run-pass/deriving-cmp-generic-enum.rs @@ -8,7 +8,7 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. -// ignore-pretty FIXME #15189 +// no-pretty-expanded FIXME #15189 #[deriving(PartialEq, Eq, PartialOrd, Ord)] enum E { diff --git a/src/test/run-pass/deriving-cmp-generic-struct-enum.rs b/src/test/run-pass/deriving-cmp-generic-struct-enum.rs index b21c95d7b50c..2abdf4c7c7e0 100644 --- a/src/test/run-pass/deriving-cmp-generic-struct-enum.rs +++ b/src/test/run-pass/deriving-cmp-generic-struct-enum.rs @@ -8,7 +8,7 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. -// ignore-pretty FIXME #15189 +// no-pretty-expanded FIXME #15189 #![feature(struct_variant)] diff --git a/src/test/run-pass/deriving-cmp-generic-struct.rs b/src/test/run-pass/deriving-cmp-generic-struct.rs index e2b8e1b6b82f..d1610978e2eb 100644 --- a/src/test/run-pass/deriving-cmp-generic-struct.rs +++ b/src/test/run-pass/deriving-cmp-generic-struct.rs @@ -8,7 +8,7 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. -// ignore-pretty FIXME #15189 +// no-pretty-expanded FIXME #15189 #[deriving(PartialEq, Eq, PartialOrd, Ord)] struct S { diff --git a/src/test/run-pass/deriving-cmp-generic-tuple-struct.rs b/src/test/run-pass/deriving-cmp-generic-tuple-struct.rs index c07f124a08d1..25f62e85ba6f 100644 --- a/src/test/run-pass/deriving-cmp-generic-tuple-struct.rs +++ b/src/test/run-pass/deriving-cmp-generic-tuple-struct.rs @@ -8,7 +8,7 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. -// ignore-pretty FIXME #15189 +// no-pretty-expanded FIXME #15189 #[deriving(PartialEq, Eq, PartialOrd, Ord)] struct TS(T,T); diff --git a/src/test/run-pass/linear-for-loop.rs b/src/test/run-pass/linear-for-loop.rs index 640ed3883eb8..e9e2a753469f 100644 --- a/src/test/run-pass/linear-for-loop.rs +++ b/src/test/run-pass/linear-for-loop.rs @@ -8,7 +8,7 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. -// ignore-pretty FIXME #15189 +// no-pretty-expanded FIXME #15189 extern crate debug; diff --git a/src/test/run-pass/task-comm-3.rs b/src/test/run-pass/task-comm-3.rs index 1c14153a1101..afeb9125fe64 100644 --- a/src/test/run-pass/task-comm-3.rs +++ b/src/test/run-pass/task-comm-3.rs @@ -8,7 +8,7 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. -// ignore-pretty FIXME #15189 +// no-pretty-expanded FIXME #15189 extern crate debug; diff --git a/src/test/run-pass/unfold-cross-crate.rs b/src/test/run-pass/unfold-cross-crate.rs index d3e70706867b..2af38047264f 100644 --- a/src/test/run-pass/unfold-cross-crate.rs +++ b/src/test/run-pass/unfold-cross-crate.rs @@ -8,7 +8,7 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. -// ignore-pretty FIXME #15189 +// no-pretty-expanded FIXME #15189 use std::iter::Unfold; diff --git a/src/test/run-pass/utf8.rs b/src/test/run-pass/utf8.rs index 6cf0d518628e..a79fcfa94171 100644 --- a/src/test/run-pass/utf8.rs +++ b/src/test/run-pass/utf8.rs @@ -8,7 +8,7 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. -// ignore-pretty FIXME #15189 +// no-pretty-expanded FIXME #15189 pub fn main() { let yen: char = '¥'; // 0xa5 From 84e8143c4f423011cd337d2a7bfdc54a753eea81 Mon Sep 17 00:00:00 2001 From: John Clements Date: Thu, 26 Jun 2014 13:18:52 -0700 Subject: [PATCH 04/18] get rid of needless wrapper function --- src/libsyntax/ext/expand.rs | 22 ++++++++-------------- 1 file changed, 8 insertions(+), 14 deletions(-) diff --git a/src/libsyntax/ext/expand.rs b/src/libsyntax/ext/expand.rs index 11f50d685f88..073de40382ad 100644 --- a/src/libsyntax/ext/expand.rs +++ b/src/libsyntax/ext/expand.rs @@ -251,7 +251,7 @@ fn expand_loop_block(loop_block: P, // the same context will pick that up in the deferred renaming pass // and be renamed incorrectly. let mut rename_list = vec!(rename); - let mut rename_fld = renames_to_fold(&mut rename_list); + let mut rename_fld = IdentRenamer{renames: &mut rename_list}; let renamed_ident = rename_fld.fold_ident(label); // The rename *must* be added to the enclosed syntax context for @@ -624,7 +624,7 @@ fn expand_non_macro_stmt(s: &Stmt, fld: &mut MacroExpander) } let rewritten_pat = { let mut rename_fld = - renames_to_fold(&mut new_pending_renames); + IdentRenamer{renames: &mut new_pending_renames}; // rewrite the pattern using the new names (the old // ones have already been applied): rename_fld.fold_pat(expanded_pat) @@ -676,8 +676,7 @@ fn expand_arm(arm: &ast::Arm, fld: &mut MacroExpander) -> ast::Arm { new_pending_renames.push((*ident,new_name)); } let rewritten_pat = { - let mut rename_fld = - renames_to_fold(&mut new_pending_renames); + let mut rename_fld = IdentRenamer{renames:&mut new_pending_renames}; // rewrite the pattern using the new names (the old // ones have already been applied): rename_fld.fold_pat(expanded_pat) @@ -757,17 +756,19 @@ fn expand_block_elts(b: &Block, fld: &mut MacroExpander) -> P { let new_view_items = b.view_items.iter().map(|x| fld.fold_view_item(x)).collect(); let new_stmts = b.stmts.iter().flat_map(|x| { + // perform all pending renames let renamed_stmt = { let pending_renames = &mut fld.extsbox.info().pending_renames; - let mut rename_fld = renames_to_fold(pending_renames); + let mut rename_fld = IdentRenamer{renames:pending_renames}; rename_fld.fold_stmt(&**x).expect_one("rename_fold didn't return one value") }; + // expand macros in the statement fld.fold_stmt(&*renamed_stmt).move_iter() }).collect(); let new_expr = b.expr.map(|x| { let expr = { let pending_renames = &mut fld.extsbox.info().pending_renames; - let mut rename_fld = renames_to_fold(pending_renames); + let mut rename_fld = IdentRenamer{renames:pending_renames}; rename_fld.fold_expr(x) }; fld.fold_expr(expr) @@ -859,6 +860,7 @@ fn expand_pat(p: Gc, fld: &mut MacroExpander) -> Gc { } } +// a tree-folder that applies every rename in its (mutable) list pub struct IdentRenamer<'a> { renames: &'a mut RenameList, } @@ -875,14 +877,6 @@ impl<'a> Folder for IdentRenamer<'a> { } } -// given a mutable list of renames, return a tree-folder that applies those -// renames. -fn renames_to_fold<'a>(renames: &'a mut RenameList) -> IdentRenamer<'a> { - IdentRenamer { - renames: renames, - } -} - fn new_span(cx: &ExtCtxt, sp: Span) -> Span { /* this discards information in the case of macro-defining macros */ Span { From a18a63185ca79126842d94505746fccef3ade1b8 Mon Sep 17 00:00:00 2001 From: John Clements Date: Thu, 26 Jun 2014 13:19:34 -0700 Subject: [PATCH 05/18] WIP match hygiene, compiles --- src/libsyntax/ext/expand.rs | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/src/libsyntax/ext/expand.rs b/src/libsyntax/ext/expand.rs index 073de40382ad..c40049a58a7b 100644 --- a/src/libsyntax/ext/expand.rs +++ b/src/libsyntax/ext/expand.rs @@ -659,15 +659,15 @@ fn expand_non_macro_stmt(s: &Stmt, fld: &mut MacroExpander) } fn expand_arm(arm: &ast::Arm, fld: &mut MacroExpander) -> ast::Arm { - if a.pats.len() == 0 { + if arm.pats.len() == 0 { fail!("encountered match arm with 0 patterns"); } - let first_pat = match a.pats.get(0) { - - } + // all of the pats must have the same set of bindings, so use the + // first one to extract them and generate new names: + let first_pat = arm.pats.get(0); // code duplicated from 'let', above. Perhaps this can be lifted // into a separate function: - let expanded_pat = fld.fold_pat(pat); + let expanded_pat = fld.fold_pat(*first_pat); let mut name_finder = new_name_finder(Vec::new()); name_finder.visit_pat(&*expanded_pat,()); let mut new_pending_renames = Vec::new(); @@ -681,13 +681,13 @@ fn expand_arm(arm: &ast::Arm, fld: &mut MacroExpander) -> ast::Arm { // ones have already been applied): rename_fld.fold_pat(expanded_pat) }; - - let bound_names - ast::Arm { - attrs: a.attrs.iter().map(|x| self.fold_attribute(*x)).collect(), - pats: a.pats.iter().map(|x| self.fold_pat(*x)).collect(), - guard: a.guard.map(|x| self.fold_expr(x)), - body: self.fold_expr(a.body), + /* + */ + ast::Arm { + attrs: arm.attrs.iter().map(|x| fld.fold_attribute(*x)).collect(), + pats: arm.pats.iter().map(|x| fld.fold_pat(*x)).collect(), + guard: arm.guard.map(|x| fld.fold_expr(x)), + body: fld.fold_expr(arm.body), } } From 977b380cd2bb45a6556f77b79274460377c6ae94 Mon Sep 17 00:00:00 2001 From: John Clements Date: Thu, 26 Jun 2014 14:39:54 -0700 Subject: [PATCH 06/18] cleanup and shiny new more-functional interface --- src/libsyntax/ext/expand.rs | 91 +++++++++++++++---------------------- 1 file changed, 36 insertions(+), 55 deletions(-) diff --git a/src/libsyntax/ext/expand.rs b/src/libsyntax/ext/expand.rs index c40049a58a7b..69f4fdb9f3fc 100644 --- a/src/libsyntax/ext/expand.rs +++ b/src/libsyntax/ext/expand.rs @@ -614,14 +614,10 @@ fn expand_non_macro_stmt(s: &Stmt, fld: &mut MacroExpander) // oh dear heaven... this is going to include the enum // names, as well... but that should be okay, as long as // the new names are gensyms for the old ones. - let mut name_finder = new_name_finder(Vec::new()); - name_finder.visit_pat(&*expanded_pat,()); // generate fresh names, push them to a new pending list - let mut new_pending_renames = Vec::new(); - for ident in name_finder.ident_accumulator.iter() { - let new_name = fresh_name(ident); - new_pending_renames.push((*ident,new_name)); - } + let idents = pattern_bindings(expanded_pat); + let mut new_pending_renames = + idents.iter().map(|ident| (*ident, fresh_name(ident))).collect(); let rewritten_pat = { let mut rename_fld = IdentRenamer{renames: &mut new_pending_renames}; @@ -668,10 +664,8 @@ fn expand_arm(arm: &ast::Arm, fld: &mut MacroExpander) -> ast::Arm { // code duplicated from 'let', above. Perhaps this can be lifted // into a separate function: let expanded_pat = fld.fold_pat(*first_pat); - let mut name_finder = new_name_finder(Vec::new()); - name_finder.visit_pat(&*expanded_pat,()); let mut new_pending_renames = Vec::new(); - for ident in name_finder.ident_accumulator.iter() { + for ident in pattern_bindings(expanded_pat).iter() { let new_name = fresh_name(ident); new_pending_renames.push((*ident,new_name)); } @@ -681,8 +675,6 @@ fn expand_arm(arm: &ast::Arm, fld: &mut MacroExpander) -> ast::Arm { // ones have already been applied): rename_fld.fold_pat(expanded_pat) }; - /* - */ ast::Arm { attrs: arm.attrs.iter().map(|x| fld.fold_attribute(*x)).collect(), pats: arm.pats.iter().map(|x| fld.fold_pat(*x)).collect(), @@ -695,7 +687,7 @@ fn expand_arm(arm: &ast::Arm, fld: &mut MacroExpander) -> ast::Arm { // a visitor that extracts the pat_ident (binding) paths // from a given thingy and puts them in a mutable -// array (passed in to the traversal). +// array #[deriving(Clone)] struct NameFinderContext { ident_accumulator: Vec , @@ -735,13 +727,11 @@ impl Visitor<()> for NameFinderContext { } -// return a visitor that extracts the pat_ident paths -// from a given thingy and puts them in a mutable -// array (passed in to the traversal) -fn new_name_finder(idents: Vec ) -> NameFinderContext { - NameFinderContext { - ident_accumulator: idents, - } +// find the pat_ident paths in a pattern +fn pattern_bindings(pat : &ast::Pat) -> Vec { + let mut name_finder = NameFinderContext{ident_accumulator:Vec::new()}; + name_finder.visit_pat(pat,()); + name_finder.ident_accumulator } // expand a block. pushes a new exts_frame, then calls expand_block_elts @@ -1046,7 +1036,8 @@ fn original_span(cx: &ExtCtxt) -> Gc { #[cfg(test)] mod test { - use super::{new_name_finder, expand_crate, contains_macro_escape}; + use super::{pattern_bindings, expand_crate, contains_macro_escape}; + use super::{NameFinderContext}; use ast; use ast::{Attribute_, AttrOuter, MetaWord}; use attr; @@ -1076,22 +1067,22 @@ mod test { match *expr { ast::Expr{id:_,span:_,node:ast::ExprPath(ref p)} => { self.path_accumulator.push(p.clone()); - // not calling visit_path, should be fine. + // not calling visit_path, but it should be fine. } _ => visit::walk_expr(self,expr,()) } } } - // return a visitor that extracts the paths - // from a given thingy and puts them in a mutable - // array (passed in to the traversal) - fn new_path_finder(paths: Vec ) -> PathExprFinderContext { - PathExprFinderContext { - path_accumulator: paths - } + // find the variable references in a crate + fn crate_varrefs(the_crate : &ast::Crate) -> Vec { + let mut path_finder = PathExprFinderContext{path_accumulator:Vec::new()}; + visit::walk_crate(&mut path_finder, the_crate, ()); + path_finder.path_accumulator } + + // these following tests are quite fragile, in that they don't test what // *kind* of failure occurs. @@ -1183,6 +1174,14 @@ mod test { expand_crate(&ps,cfg,vec!(),vec!(),crate_ast) } + // find the pat_ident paths in a crate + fn crate_bindings(the_crate : &ast::Crate) -> Vec { + let mut name_finder = NameFinderContext{ident_accumulator:Vec::new()}; + visit::walk_crate(&mut name_finder, the_crate, ()); + name_finder.ident_accumulator + } + + //fn expand_and_resolve(crate_str: @str) -> ast::crate { //let expanded_ast = expand_crate_str(crate_str); // println!("expanded: {:?}\n",expanded_ast); @@ -1315,15 +1314,8 @@ mod test { (ref str,ref conns, bic) => (str.to_owned(), conns.clone(), bic) }; let cr = expand_crate_str(teststr.to_string()); - // find the bindings: - let mut name_finder = new_name_finder(Vec::new()); - visit::walk_crate(&mut name_finder,&cr,()); - let bindings = name_finder.ident_accumulator; - - // find the varrefs: - let mut path_finder = new_path_finder(Vec::new()); - visit::walk_crate(&mut path_finder,&cr,()); - let varrefs = path_finder.path_accumulator; + let bindings = crate_bindings(&cr); + let varrefs = crate_varrefs(&cr); // must be one check clause for each binding: assert_eq!(bindings.len(),bound_connections.len()); @@ -1392,10 +1384,7 @@ foo_module!() ".to_string(); let cr = expand_crate_str(crate_str); // find the xx binding - let mut name_finder = new_name_finder(Vec::new()); - visit::walk_crate(&mut name_finder, &cr, ()); - let bindings = name_finder.ident_accumulator; - + let bindings = crate_bindings(&cr); let cxbinds: Vec<&ast::Ident> = bindings.iter().filter(|b| { let ident = token::get_ident(**b); @@ -1408,10 +1397,7 @@ foo_module!() _ => fail!("expected just one binding for ext_cx") }; let resolved_binding = mtwt::resolve(*cxbind); - // find all the xx varrefs: - let mut path_finder = new_path_finder(Vec::new()); - visit::walk_crate(&mut path_finder, &cr, ()); - let varrefs = path_finder.path_accumulator; + let varrefs = crate_varrefs(&cr); // the xx binding should bind all of the xx varrefs: for (idx,v) in varrefs.iter().filter(|p| { @@ -1437,10 +1423,8 @@ foo_module!() fn pat_idents(){ let pat = string_to_pat( "(a,Foo{x:c @ (b,9),y:Bar(4,d)})".to_string()); - let mut pat_idents = new_name_finder(Vec::new()); - pat_idents.visit_pat(pat, ()); - assert_eq!(pat_idents.ident_accumulator, - strs_to_idents(vec!("a","c","b","d"))); + let idents = pattern_bindings(pat); + assert_eq!(idents, strs_to_idents(vec!("a","c","b","d"))); } // test the list of identifier patterns gathered by the visitor. Note that @@ -1450,11 +1434,8 @@ foo_module!() fn crate_idents(){ let the_crate = string_to_crate("fn main (a : int) -> int {|b| { match 34 {None => 3, Some(i) | i => j, Foo{k:z,l:y} => \"banana\"}} }".to_string()); - let mut idents = new_name_finder(Vec::new()); - //visit::walk_crate(&mut idents, &the_crate, ()); - idents.visit_mod(&the_crate.module, the_crate.span, ast::CRATE_NODE_ID, ()); - assert_eq!(idents.ident_accumulator, - strs_to_idents(vec!("a","b","None","i","i","z","y"))); + let idents = crate_bindings(&the_crate); + assert_eq!(idents, strs_to_idents(vec!("a","b","None","i","i","z","y"))); } From 47eec97cdab94989cc4c5f20a1e1f2310df997aa Mon Sep 17 00:00:00 2001 From: John Clements Date: Thu, 26 Jun 2014 15:37:18 -0700 Subject: [PATCH 07/18] remove unnecessary abstraction --- src/libsyntax/ext/expand.rs | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/src/libsyntax/ext/expand.rs b/src/libsyntax/ext/expand.rs index 69f4fdb9f3fc..832325f897bd 100644 --- a/src/libsyntax/ext/expand.rs +++ b/src/libsyntax/ext/expand.rs @@ -988,35 +988,30 @@ impl Folder for Marker { } } -// just a convenience: -fn new_mark_folder(m: Mrk) -> Marker { - Marker {mark: m} -} - // apply a given mark to the given token trees. Used prior to expansion of a macro. fn mark_tts(tts: &[TokenTree], m: Mrk) -> Vec { - fold_tts(tts, &mut new_mark_folder(m)) + fold_tts(tts, &mut Marker{mark:m}) } // apply a given mark to the given expr. Used following the expansion of a macro. fn mark_expr(expr: Gc, m: Mrk) -> Gc { - new_mark_folder(m).fold_expr(expr) + Marker{mark:m}.fold_expr(expr) } // apply a given mark to the given pattern. Used following the expansion of a macro. fn mark_pat(pat: Gc, m: Mrk) -> Gc { - new_mark_folder(m).fold_pat(pat) + Marker{mark:m}.fold_pat(pat) } // apply a given mark to the given stmt. Used following the expansion of a macro. fn mark_stmt(expr: &ast::Stmt, m: Mrk) -> Gc { - new_mark_folder(m).fold_stmt(expr) + Marker{mark:m}.fold_stmt(expr) .expect_one("marking a stmt didn't return a stmt") } // apply a given mark to the given item. Used following the expansion of a macro. fn mark_item(expr: Gc, m: Mrk) -> SmallVector> { - new_mark_folder(m).fold_item(expr) + Marker{mark:m}.fold_item(expr) } fn original_span(cx: &ExtCtxt) -> Gc { From 7bad96e742b187b671500702bccb37ab1724bae5 Mon Sep 17 00:00:00 2001 From: John Clements Date: Thu, 26 Jun 2014 15:37:45 -0700 Subject: [PATCH 08/18] improve match test case to include guard --- src/libsyntax/ext/expand.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/libsyntax/ext/expand.rs b/src/libsyntax/ext/expand.rs index 832325f897bd..941a4bb68c2d 100644 --- a/src/libsyntax/ext/expand.rs +++ b/src/libsyntax/ext/expand.rs @@ -1274,13 +1274,13 @@ mod test { } // FIXME #9384, match variable hygiene. Should expand into - // fn z() {match 8 {x_1 => {match 9 {x_2 | x_2 => x_2 + x_1}}}} + // fn z() {match 8 {x_1 => {match 9 {x_2 | x_2 if x_2 == x_1 => x_2 + x_1}}}} #[test] fn issue_9384(){ run_renaming_test( - &("macro_rules! bad_macro (($ex:expr) => ({match 9 {x | x => x + $ex}})) - fn z() {match 8 {x => bad_macro!(_x)}}", + &("macro_rules! bad_macro (($ex:expr) => ({match 9 {x | x if x == $ex => x + $ex}})) + fn z() {match 8 {x => bad_macro!(x)}}", // NB: the third "binding" is the repeat of the second one. - vec!(vec!(1),vec!(0),vec!(0)), + vec!(vec!(1,3),vec!(0,2),vec!(0,2)), true), 0) } From 4b46c700f4bf04331b1858e7fe3c363c0451c4cb Mon Sep 17 00:00:00 2001 From: John Clements Date: Thu, 26 Jun 2014 16:49:13 -0700 Subject: [PATCH 09/18] hygiene for match-bound vars now implemented Closes #9384 --- src/libsyntax/ext/expand.rs | 57 +++++++++++++++++++++---------------- 1 file changed, 33 insertions(+), 24 deletions(-) diff --git a/src/libsyntax/ext/expand.rs b/src/libsyntax/ext/expand.rs index 941a4bb68c2d..c2bb2cd73473 100644 --- a/src/libsyntax/ext/expand.rs +++ b/src/libsyntax/ext/expand.rs @@ -608,7 +608,7 @@ fn expand_non_macro_stmt(s: &Stmt, fld: &mut MacroExpander) span: span, source: source, } = **local; - // expand the pat (it might contain exprs... #:(o)> + // expand the pat (it might contain macro uses): let expanded_pat = fld.fold_pat(pat); // find the pat_idents in the pattern: // oh dear heaven... this is going to include the enum @@ -618,11 +618,11 @@ fn expand_non_macro_stmt(s: &Stmt, fld: &mut MacroExpander) let idents = pattern_bindings(expanded_pat); let mut new_pending_renames = idents.iter().map(|ident| (*ident, fresh_name(ident))).collect(); + // rewrite the pattern using the new names (the old + // ones have already been applied): let rewritten_pat = { - let mut rename_fld = - IdentRenamer{renames: &mut new_pending_renames}; - // rewrite the pattern using the new names (the old - // ones have already been applied): + // nested binding to allow borrow to expire: + let mut rename_fld = IdentRenamer{renames: &mut new_pending_renames}; rename_fld.fold_pat(expanded_pat) }; // add them to the existing pending renames: @@ -655,31 +655,38 @@ fn expand_non_macro_stmt(s: &Stmt, fld: &mut MacroExpander) } fn expand_arm(arm: &ast::Arm, fld: &mut MacroExpander) -> ast::Arm { - if arm.pats.len() == 0 { + // expand pats... they might contain macro uses: + let expanded_pats : Vec> = arm.pats.iter().map(|pat| fld.fold_pat(*pat)).collect(); + if expanded_pats.len() == 0 { fail!("encountered match arm with 0 patterns"); } // all of the pats must have the same set of bindings, so use the // first one to extract them and generate new names: - let first_pat = arm.pats.get(0); + let first_pat = expanded_pats.get(0); // code duplicated from 'let', above. Perhaps this can be lifted // into a separate function: - let expanded_pat = fld.fold_pat(*first_pat); - let mut new_pending_renames = Vec::new(); - for ident in pattern_bindings(expanded_pat).iter() { - let new_name = fresh_name(ident); - new_pending_renames.push((*ident,new_name)); - } - let rewritten_pat = { - let mut rename_fld = IdentRenamer{renames:&mut new_pending_renames}; - // rewrite the pattern using the new names (the old - // ones have already been applied): - rename_fld.fold_pat(expanded_pat) - }; + let idents = pattern_bindings(*first_pat); + let mut new_pending_renames = + idents.iter().map(|id| (*id,fresh_name(id))).collect(); + // rewrite all of the patterns using the new names (the old + // ones have already been applied). Note that we depend here + // on the guarantee that after expansion, there can't be any + // Path expressions (a.k.a. varrefs) left in the pattern. If + // this were false, we'd need to apply this renaming only to + // the bindings, and not to the varrefs, using a more targeted + // fold-er. + let mut rename_fld = IdentRenamer{renames:&mut new_pending_renames}; + let rewritten_pats = + expanded_pats.iter().map(|pat| rename_fld.fold_pat(*pat)).collect(); + // apply renaming and then expansion to the guard and the body: + let rewritten_guard = + arm.guard.map(|g| fld.fold_expr(rename_fld.fold_expr(g))); + let rewritten_body = fld.fold_expr(rename_fld.fold_expr(arm.body)); ast::Arm { attrs: arm.attrs.iter().map(|x| fld.fold_attribute(*x)).collect(), - pats: arm.pats.iter().map(|x| fld.fold_pat(*x)).collect(), - guard: arm.guard.map(|x| fld.fold_expr(x)), - body: fld.fold_expr(arm.body), + pats: rewritten_pats, + guard: rewritten_guard, + body: rewritten_body, } } @@ -851,6 +858,8 @@ fn expand_pat(p: Gc, fld: &mut MacroExpander) -> Gc { } // a tree-folder that applies every rename in its (mutable) list +// to every identifier, including both bindings and varrefs +// (and lots of things that will turn out to be neither) pub struct IdentRenamer<'a> { renames: &'a mut RenameList, } @@ -1335,8 +1344,8 @@ mod test { invalid_name); if !(varref_name==binding_name) { println!("uh oh, should match but doesn't:"); - println!("varref: {:?}",varref); - println!("binding: {:?}", *bindings.get(binding_idx)); + println!("varref #{:?}: {:?}",idx, varref); + println!("binding #{:?}: {:?}", binding_idx, *bindings.get(binding_idx)); mtwt::with_sctable(|x| mtwt::display_sctable(x)); } assert_eq!(varref_name,binding_name); From 235ca1801ec8053b7cab46ed6708c382e58df63b Mon Sep 17 00:00:00 2001 From: John Clements Date: Thu, 26 Jun 2014 16:53:33 -0700 Subject: [PATCH 10/18] remove trailing whitespace --- src/libsyntax/ext/expand.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libsyntax/ext/expand.rs b/src/libsyntax/ext/expand.rs index c2bb2cd73473..ab040a5964a5 100644 --- a/src/libsyntax/ext/expand.rs +++ b/src/libsyntax/ext/expand.rs @@ -687,7 +687,7 @@ fn expand_arm(arm: &ast::Arm, fld: &mut MacroExpander) -> ast::Arm { pats: rewritten_pats, guard: rewritten_guard, body: rewritten_body, - } + } } From ee1ee7f463240ebda33d2180dd54be7a5e7f923c Mon Sep 17 00:00:00 2001 From: John Clements Date: Thu, 26 Jun 2014 17:41:43 -0700 Subject: [PATCH 11/18] make tests hygienic... ... and possibly totally pointless. Specifically, fixing these to make their macros hygienic may mean that they no longer test the thing that they were supposed to test. --- src/test/run-pass/issue-8851.rs | 13 +++++++++---- .../run-pass/typeck-macro-interaction-issue-8852.rs | 13 +++++++++---- 2 files changed, 18 insertions(+), 8 deletions(-) diff --git a/src/test/run-pass/issue-8851.rs b/src/test/run-pass/issue-8851.rs index 62970369579a..210371205696 100644 --- a/src/test/run-pass/issue-8851.rs +++ b/src/test/run-pass/issue-8851.rs @@ -10,23 +10,28 @@ #![feature(macro_rules)] +// after fixing #9384 and implementing hygiene for match bindings, +// this now fails because the insertion of the 'y' into the match +// doesn't cause capture. Making this macro hygienic (as I've done) +// could very well make this test case completely pointless.... + enum T { A(int), B(uint) } macro_rules! test( - ($e:expr) => ( + ($id:ident, $e:expr) => ( fn foo(t: T) -> int { match t { - A(y) => $e, - B(y) => $e + A($id) => $e, + B($id) => $e } } ) ) -test!(10 + (y as int)) +test!(y, 10 + (y as int)) pub fn main() { foo(A(20)); diff --git a/src/test/run-pass/typeck-macro-interaction-issue-8852.rs b/src/test/run-pass/typeck-macro-interaction-issue-8852.rs index 50ef1922c8fb..6be79cb62dd7 100644 --- a/src/test/run-pass/typeck-macro-interaction-issue-8852.rs +++ b/src/test/run-pass/typeck-macro-interaction-issue-8852.rs @@ -15,19 +15,24 @@ enum T { B(f64) } +// after fixing #9384 and implementing hygiene for match bindings, +// this now fails because the insertion of the 'y' into the match +// doesn't cause capture. Making this macro hygienic (as I've done) +// could very well make this test case completely pointless.... + macro_rules! test( - ($e:expr) => ( + ($id1:ident, $id2:ident, $e:expr) => ( fn foo(a:T, b:T) -> T { match (a, b) { - (A(x), A(y)) => A($e), - (B(x), B(y)) => B($e), + (A($id1), A($id2)) => A($e), + (B($id1), B($id2)) => B($e), _ => fail!() } } ) ) -test!(x + y) +test!(x,y,x + y) pub fn main() { foo(A(1), A(2)); From e100d26d1d779b4759f3f754b03a35755cb89b84 Mon Sep 17 00:00:00 2001 From: John Clements Date: Fri, 27 Jun 2014 12:57:48 -0700 Subject: [PATCH 12/18] undo helpful attempt to spell-check Yes, that word is spelled \'memoization\' --- src/libsyntax/ext/mtwt.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libsyntax/ext/mtwt.rs b/src/libsyntax/ext/mtwt.rs index 6c97a8aed1f5..6f6eeb3debda 100644 --- a/src/libsyntax/ext/mtwt.rs +++ b/src/libsyntax/ext/mtwt.rs @@ -160,7 +160,7 @@ fn with_resolve_table_mut(op: |&mut ResolveTable| -> T) -> T { } // Resolve a syntax object to a name, per MTWT. -// adding memorization to possibly resolve 500+ seconds in resolve for librustc (!) +// adding memoization to possibly resolve 500+ seconds in resolve for librustc (!) fn resolve_internal(id: Ident, table: &SCTable, resolve_table: &mut ResolveTable) -> Name { From 351a5fd2b40c2be90f94fe2580903e93353b95ee Mon Sep 17 00:00:00 2001 From: John Clements Date: Fri, 27 Jun 2014 13:10:47 -0700 Subject: [PATCH 13/18] added unit and standalone test for 15221, extra debugging output --- src/libsyntax/ext/expand.rs | 31 ++++++++++++++++++++++++++----- src/test/run-pass/issue-15221.rs | 22 ++++++++++++++++++++++ 2 files changed, 48 insertions(+), 5 deletions(-) create mode 100644 src/test/run-pass/issue-15221.rs diff --git a/src/libsyntax/ext/expand.rs b/src/libsyntax/ext/expand.rs index ab040a5964a5..0a8ec5bc40ea 100644 --- a/src/libsyntax/ext/expand.rs +++ b/src/libsyntax/ext/expand.rs @@ -54,7 +54,6 @@ pub fn expand_expr(e: Gc, fld: &mut MacroExpander) -> Gc { } let extname = pth.segments.get(0).identifier; let extnamestr = token::get_ident(extname); - // leaving explicit deref here to highlight unbox op: let marked_after = match fld.extsbox.find(&extname.name) { None => { fld.cx.span_err( @@ -1294,6 +1293,19 @@ mod test { 0) } + // FIXME #15221, somehow pats aren't getting labeled correctly? + // should expand into + // fn main(){let g1_1 = 13; g1_1}} + #[test] fn pat_expand_issue_15221(){ + run_renaming_test( + &("macro_rules! inner ( ($e:pat ) => ($e)) + macro_rules! outer ( ($e:pat ) => (inner!($e))) + fn main() { let outer!(g) = 13; g;}", + vec!(vec!(0)), + true), + 0) + } + // create a really evil test case where a $x appears inside a binding of $x // but *shouldnt* bind because it was inserted by a different macro.... // can't write this test case until we have macro-generating macros. @@ -1343,9 +1355,13 @@ mod test { .ctxt, invalid_name); if !(varref_name==binding_name) { + let varref_idents : Vec + = varref.segments.iter().map(|s| + s.identifier) + .collect(); println!("uh oh, should match but doesn't:"); - println!("varref #{:?}: {:?}",idx, varref); - println!("binding #{:?}: {:?}", binding_idx, *bindings.get(binding_idx)); + println!("varref #{}: {}",idx, varref_idents); + println!("binding #{}: {}", binding_idx, *bindings.get(binding_idx)); mtwt::with_sctable(|x| mtwt::display_sctable(x)); } assert_eq!(varref_name,binding_name); @@ -1360,11 +1376,15 @@ mod test { == binding_name); // temp debugging: if fail { + let varref_idents : Vec + = varref.segments.iter().map(|s| + s.identifier) + .collect(); println!("failure on test {}",test_idx); println!("text of test case: \"{}\"", teststr); println!(""); println!("uh oh, matches but shouldn't:"); - println!("varref: {:?}",varref); + println!("varref: {}",varref_idents); // good lord, you can't make a path with 0 segments, can you? let string = token::get_ident(varref.segments .get(0) @@ -1372,7 +1392,7 @@ mod test { println!("varref's first segment's uint: {}, and string: \"{}\"", varref.segments.get(0).identifier.name, string.get()); - println!("binding: {:?}", *bindings.get(binding_idx)); + println!("binding: {}", *bindings.get(binding_idx)); mtwt::with_sctable(|x| mtwt::display_sctable(x)); } assert!(!fail); @@ -1442,5 +1462,6 @@ foo_module!() assert_eq!(idents, strs_to_idents(vec!("a","b","None","i","i","z","y"))); } + // } diff --git a/src/test/run-pass/issue-15221.rs b/src/test/run-pass/issue-15221.rs new file mode 100644 index 000000000000..e74ba9b85ee0 --- /dev/null +++ b/src/test/run-pass/issue-15221.rs @@ -0,0 +1,22 @@ +// Copyright 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 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +#![feature(macro_rules)] + +macro_rules! inner_bind ( + ( $p:pat, $id:ident) => ({let $p = 13; $id})) + +macro_rules! outer_bind ( + ($p:pat, $id:ident ) => (inner_bind!($p, $id))) + +fn main() { + outer_bind!(g1,g1); +} + From 268f6c56c219e97b444d78dadc387cc549e27aa6 Mon Sep 17 00:00:00 2001 From: John Clements Date: Fri, 27 Jun 2014 14:12:13 -0700 Subject: [PATCH 14/18] removed incomplete comment as written, I don't believe this comment was helpful; I think it's better just to steer the reader toward a general understanding of hygiene. --- src/libsyntax/fold.rs | 17 ----------------- 1 file changed, 17 deletions(-) diff --git a/src/libsyntax/fold.rs b/src/libsyntax/fold.rs index 01742ca131e9..a6e4b4cae1c9 100644 --- a/src/libsyntax/fold.rs +++ b/src/libsyntax/fold.rs @@ -391,23 +391,6 @@ fn fold_arg_(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(tts: &[TokenTree], fld: &mut T) -> Vec { tts.iter().map(|tt| { match *tt { From 764c2fe2d582c36ad00ef67b5553f7777447e415 Mon Sep 17 00:00:00 2001 From: John Clements Date: Fri, 27 Jun 2014 14:41:32 -0700 Subject: [PATCH 15/18] simplified test case --- src/test/run-pass/issue-15221.rs | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/test/run-pass/issue-15221.rs b/src/test/run-pass/issue-15221.rs index e74ba9b85ee0..e5cfccac13a8 100644 --- a/src/test/run-pass/issue-15221.rs +++ b/src/test/run-pass/issue-15221.rs @@ -10,13 +10,14 @@ #![feature(macro_rules)] -macro_rules! inner_bind ( - ( $p:pat, $id:ident) => ({let $p = 13; $id})) +macro_rules! inner ( + ($e:pat ) => ($e)) -macro_rules! outer_bind ( - ($p:pat, $id:ident ) => (inner_bind!($p, $id))) +macro_rules! outer ( + ($e:pat ) => (inner!($e))) fn main() { - outer_bind!(g1,g1); + let outer!(g1) = 13; + g1; } From 2f73b7874ed2ba3be02cbf8c767aaecd6840bbe9 Mon Sep 17 00:00:00 2001 From: John Clements Date: Fri, 27 Jun 2014 16:07:25 -0700 Subject: [PATCH 16/18] looks like a cut-n-paste error in unused code --- src/doc/guide-macros.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/doc/guide-macros.md b/src/doc/guide-macros.md index 45745c7b7bc7..6d3825f8ecf1 100644 --- a/src/doc/guide-macros.md +++ b/src/doc/guide-macros.md @@ -355,6 +355,7 @@ macro_rules! biased_match_rec ( _ => { $err } } ); + // Produce the requested values ( binds $( $bind_res:ident ),* ) => ( ($( $bind_res ),*) ) ) @@ -364,7 +365,7 @@ macro_rules! biased_match ( ( $( ($e:expr) ~ ($p:pat) else $err:stmt ; )* binds $bind_res:ident ) => ( - let ( $( $bind_res ),* ) = biased_match_rec!( + let $bind_res = biased_match_rec!( $( ($e) ~ ($p) else $err ; )* binds $bind_res ); From e3361bcbc259cf25b21e9ae50701d9030ad1f513 Mon Sep 17 00:00:00 2001 From: John Clements Date: Fri, 27 Jun 2014 16:10:23 -0700 Subject: [PATCH 17/18] adjust fold to fold over interpolated items/exprs/etc. Closes #15221 --- src/libsyntax/ext/expand.rs | 4 +- src/libsyntax/fold.rs | 85 +++++++++++++++++++++++++++++-------- 2 files changed, 70 insertions(+), 19 deletions(-) diff --git a/src/libsyntax/ext/expand.rs b/src/libsyntax/ext/expand.rs index 0a8ec5bc40ea..b9cedb7a7797 100644 --- a/src/libsyntax/ext/expand.rs +++ b/src/libsyntax/ext/expand.rs @@ -1281,7 +1281,7 @@ mod test { 0) } - // FIXME #9384, match variable hygiene. Should expand into + // match variable hygiene. Should expand into // fn z() {match 8 {x_1 => {match 9 {x_2 | x_2 if x_2 == x_1 => x_2 + x_1}}}} #[test] fn issue_9384(){ run_renaming_test( @@ -1293,7 +1293,7 @@ mod test { 0) } - // FIXME #15221, somehow pats aren't getting labeled correctly? + // interpolated nodes weren't getting labeled. // should expand into // fn main(){let g1_1 = 13; g1_1}} #[test] fn pat_expand_issue_15221(){ diff --git a/src/libsyntax/fold.rs b/src/libsyntax/fold.rs index a6e4b4cae1c9..c6177ce31f5f 100644 --- a/src/libsyntax/fold.rs +++ b/src/libsyntax/fold.rs @@ -389,36 +389,80 @@ fn fold_arg_(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. -pub fn fold_tts(tts: &[TokenTree], fld: &mut T) -> Vec { - tts.iter().map(|tt| { - match *tt { - TTTok(span, ref tok) => - TTTok(span,maybe_fold_ident(tok,fld)), - TTDelim(ref tts) => TTDelim(Rc::new(fold_tts(tts.as_slice(), fld))), - TTSeq(span, ref pattern, ref sep, is_optional) => +pub fn fold_tt(tt: &TokenTree, fld: &mut T) -> TokenTree { + match *tt { + TTTok(span, ref tok) => + TTTok(span, fold_token(tok,fld)), + TTDelim(ref tts) => TTDelim(Rc::new(fold_tts(tts.as_slice(), fld))), + TTSeq(span, ref pattern, ref sep, is_optional) => TTSeq(span, Rc::new(fold_tts(pattern.as_slice(), fld)), - sep.as_ref().map(|tok|maybe_fold_ident(tok,fld)), + sep.as_ref().map(|tok| fold_token(tok,fld)), is_optional), - TTNonterminal(sp,ref ident) => + TTNonterminal(sp,ref ident) => TTNonterminal(sp,fld.fold_ident(*ident)) - } - }).collect() + } } -// apply ident folder if it's an ident, otherwise leave it alone -fn maybe_fold_ident(t: &token::Token, fld: &mut T) -> token::Token { +pub fn fold_tts(tts: &[TokenTree], fld: &mut T) -> Vec { + tts.iter().map(|tt| fold_tt(tt,fld)).collect() +} + + +// apply ident folder if it's an ident, apply other folds to interpolated nodes +fn fold_token(t: &token::Token, fld: &mut T) -> token::Token { match *t { token::IDENT(id, followed_by_colons) => { token::IDENT(fld.fold_ident(id), followed_by_colons) } token::LIFETIME(id) => token::LIFETIME(fld.fold_ident(id)), + token::INTERPOLATED(ref nt) => token::INTERPOLATED(fold_interpolated(nt,fld)), _ => (*t).clone() } } +// apply folder to elements of interpolated nodes +// +// NB: this can occur only when applying a fold to partially expanded code, where +// parsed pieces have gotten implanted ito *other* macro invocations. This is relevant +// for macro hygiene, but possibly not elsewhere. +// +// One problem here occurs because the types for fold_item, fold_stmt, etc. allow the +// folder to return *multiple* items; this is a problem for the nodes here, because +// they insist on having exactly one piece. One solution would be to mangle the fold +// trait to include one-to-many and one-to-one versions of these entry points, but that +// would probably confuse a lot of people and help very few. Instead, I'm just going +// to put in dynamic checks. I think the performance impact of this will be pretty much +// nonexistent. The danger is that someone will apply a fold to a partially expanded +// node, and will be confused by the fact that their "fold_item" or "fold_stmt" isn't +// getting called on NtItem or NtStmt nodes. Hopefully they'll wind up reading this +// comment, and doing something appropriate. +// +// BTW, design choice: I considered just changing the type of, e.g., NtItem to contain +// multiple items, but decided against it when I looked at parse_item_or_view_item and +// tried to figure out what I would do with multiple items there.... +fn fold_interpolated(nt : &token::Nonterminal, fld: &mut T) -> token::Nonterminal { + match *nt { + token::NtItem(item) => + token::NtItem(fld.fold_item(item) + .expect_one("expected fold to produce exactly one item")), + token::NtBlock(block) => token::NtBlock(fld.fold_block(block)), + token::NtStmt(stmt) => + token::NtStmt(fld.fold_stmt(stmt) + .expect_one("expected fold to produce exactly one statement")), + token::NtPat(pat) => token::NtPat(fld.fold_pat(pat)), + token::NtExpr(expr) => token::NtExpr(fld.fold_expr(expr)), + token::NtTy(ty) => token::NtTy(fld.fold_ty(ty)), + token::NtIdent(ref id, is_mod_name) => + token::NtIdent(box fld.fold_ident(**id),is_mod_name), + token::NtMeta(meta_item) => token::NtMeta(fold_meta_item_(meta_item,fld)), + token::NtPath(ref path) => token::NtPath(box fld.fold_path(*path)), + token::NtTT(tt) => token::NtTT(box (GC) fold_tt(tt,fld)), + // it looks to me like we can leave out the matchers: token::NtMatchers(matchers) + _ => (*nt).clone() + } +} + pub fn noop_fold_fn_decl(decl: &FnDecl, fld: &mut T) -> P { P(FnDecl { inputs: decl.inputs.iter().map(|x| fold_arg_(x, fld)).collect(), // bad copy @@ -672,8 +716,15 @@ pub fn noop_fold_crate(c: Crate, folder: &mut T) -> Crate { } } +// fold one item into possibly many items pub fn noop_fold_item(i: &Item, folder: &mut T) -> SmallVector> { + SmallVector::one(box(GC) noop_fold_item_(i,folder)) +} + + +// fold one item into exactly one item +pub fn noop_fold_item_(i: &Item, folder: &mut T) -> Item { let id = folder.new_id(i.id); // Needs to be first, for ast_map. let node = folder.fold_item_underscore(&i.node); let ident = match node { @@ -684,14 +735,14 @@ pub fn noop_fold_item(i: &Item, _ => i.ident }; - SmallVector::one(box(GC) Item { + Item { id: id, ident: folder.fold_ident(ident), attrs: i.attrs.iter().map(|e| folder.fold_attribute(*e)).collect(), node: node, vis: i.vis, span: folder.new_span(i.span) - }) + } } pub fn noop_fold_foreign_item(ni: &ForeignItem, From 04ced031ad52cfa33f30dbd55f72aff1d95813a3 Mon Sep 17 00:00:00 2001 From: John Clements Date: Fri, 27 Jun 2014 16:11:18 -0700 Subject: [PATCH 18/18] comments only --- src/libsyntax/ext/mtwt.rs | 3 ++- src/libsyntax/parse/token.rs | 1 + 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/src/libsyntax/ext/mtwt.rs b/src/libsyntax/ext/mtwt.rs index 6f6eeb3debda..48895d34022c 100644 --- a/src/libsyntax/ext/mtwt.rs +++ b/src/libsyntax/ext/mtwt.rs @@ -30,6 +30,7 @@ use std::collections::HashMap; // change the semantics--everything here is immutable--but // it should cut down on memory use *a lot*; applying a mark // to a tree containing 50 identifiers would otherwise generate +// 50 new contexts pub struct SCTable { table: RefCell>, mark_memo: RefCell>, @@ -160,7 +161,7 @@ fn with_resolve_table_mut(op: |&mut ResolveTable| -> T) -> T { } // Resolve a syntax object to a name, per MTWT. -// adding memoization to possibly resolve 500+ seconds in resolve for librustc (!) +// adding memoization to resolve 500+ seconds in resolve for librustc (!) fn resolve_internal(id: Ident, table: &SCTable, resolve_table: &mut ResolveTable) -> Name { diff --git a/src/libsyntax/parse/token.rs b/src/libsyntax/parse/token.rs index 9549d5b8389d..a93e8270d986 100644 --- a/src/libsyntax/parse/token.rs +++ b/src/libsyntax/parse/token.rs @@ -114,6 +114,7 @@ pub enum Nonterminal { NtPat( Gc), NtExpr(Gc), NtTy( P), + // see IDENT, above, for meaning of bool in NtIdent: NtIdent(Box, bool), NtMeta(Gc), // stuff inside brackets for attributes NtPath(Box),