auto merge of #15233 : jbclements/rust/match-var-hygiene-etc, r=cmr

This PR includes two big things and a bunch of little ones.

1) It enables hygiene for variables bound by 'match' expressions.
2) It fixes a bug discovered indirectly (#15221), wherein fold traversal failed to visit nonterminal nodes.
3) It fixes a small bug in the macro tutorial.

It also adds tests for the first two, and makes a bunch of small comment improvements and cleanup.

2 files changed, 137 insertions, 98 deletions

diff --git a/src/libsyntax/ext/expand.rs b/src/libsyntax/ext/expand.rs
index 321c56d4bbf..b9cedb7a779 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<ast::Expr>, fld: &mut MacroExpander) -> Gc<ast::Expr> {
     match e.node {
         // expr_mac should really be expr_ext or something; it's the
@@ -53,7 +54,6 @@ pub fn expand_expr(e: Gc<ast::Expr>, fld: &mut MacroExpander) -> Gc<ast::Expr> {
                     }
                     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(
@@ -130,8 +130,6 @@ pub fn expand_expr(e: Gc<ast::Expr>, fld: &mut MacroExpander) -> Gc<ast::Expr> {
         // From: `['<ident>:] for <src_pat> in <src_expr> <src_loop_block>`
         // FIXME #6993: change type of opt_ident to Option<Name>
         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;
 
@@ -252,7 +250,7 @@ fn expand_loop_block(loop_block: P<Block>,
             // 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
@@ -281,7 +279,7 @@ macro_rules! with_exts_frame (
 )
 
 // When we enter a module, record it, for the sake of `module!`
-pub fn expand_item(it: Gc<ast::Item>, fld: &mut MacroExpander)
+fn expand_item(it: Gc<ast::Item>, fld: &mut MacroExpander)
                    -> SmallVector<Gc<ast::Item>> {
     let it = expand_item_modifiers(it, fld);
 
@@ -386,13 +384,13 @@ fn expand_item_modifiers(mut it: Gc<ast::Item>, 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<ast::Item>, fld: &mut MacroExpander)
+fn expand_item_mac(it: Gc<ast::Item>, fld: &mut MacroExpander)
                        -> SmallVector<Gc<ast::Item>> {
     let (pth, tts) = match it.node {
         ItemMac(codemap::Spanned {
@@ -498,7 +496,7 @@ pub fn expand_item_mac(it: Gc<ast::Item>, fld: &mut MacroExpander)
 }
 
 // expand a stmt
-pub fn expand_stmt(s: &Stmt, fld: &mut MacroExpander) -> SmallVector<Gc<Stmt>> {
+fn expand_stmt(s: &Stmt, fld: &mut MacroExpander) -> SmallVector<Gc<Stmt>> {
     // why the copying here and not in expand_expr?
     // looks like classic changed-in-only-one-place
     let (pth, tts, semi) = match s.node {
@@ -609,25 +607,21 @@ 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
                     // 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();
+                    // rewrite the pattern using the new names (the old
+                    // ones have already been applied):
                     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):
+                        // 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:
@@ -659,9 +653,47 @@ fn expand_non_macro_stmt(s: &Stmt, fld: &mut MacroExpander)
     }
 }
 
+fn expand_arm(arm: &ast::Arm, fld: &mut MacroExpander) -> ast::Arm {
+    // expand pats... they might contain macro uses:
+    let expanded_pats : Vec<Gc<ast::Pat>> = 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 = expanded_pats.get(0);
+    // code duplicated from 'let', above. Perhaps this can be lifted
+    // into a separate function:
+    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: rewritten_pats,
+        guard: rewritten_guard,
+        body: rewritten_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).
+// array
 #[deriving(Clone)]
 struct NameFinderContext {
     ident_accumulator: Vec<ast::Ident> ,
@@ -701,38 +733,38 @@ 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<ast::Ident> ) -> NameFinderContext {
-    NameFinderContext {
-        ident_accumulator: idents,
-    }
+// find the pat_ident paths in a pattern
+fn pattern_bindings(pat : &ast::Pat) -> Vec<ast::Ident> {
+    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
-pub fn expand_block(blk: &Block, fld: &mut MacroExpander) -> P<Block> {
+fn expand_block(blk: &Block, fld: &mut MacroExpander) -> P<Block> {
     // 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<Block> {
+fn expand_block_elts(b: &Block, fld: &mut MacroExpander) -> P<Block> {
     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)
@@ -747,7 +779,7 @@ pub fn expand_block_elts(b: &Block, fld: &mut MacroExpander) -> P<Block> {
     })
 }
 
-pub fn expand_pat(p: Gc<ast::Pat>, fld: &mut MacroExpander) -> Gc<ast::Pat> {
+fn expand_pat(p: Gc<ast::Pat>, fld: &mut MacroExpander) -> Gc<ast::Pat> {
     let (pth, tts) = match p.node {
         PatMac(ref mac) => {
             match mac.node {
@@ -824,6 +856,9 @@ pub fn expand_pat(p: Gc<ast::Pat>, fld: &mut MacroExpander) -> Gc<ast::Pat> {
     }
 }
 
+// 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,
 }
@@ -840,15 +875,7 @@ 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> {
-    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 +910,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)
     }
@@ -965,35 +996,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<TokenTree> {
-    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<ast::Expr>, m: Mrk) -> Gc<ast::Expr> {
-    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<ast::Pat>, m: Mrk) -> Gc<ast::Pat> {
-    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<ast::Stmt> {
-    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<ast::Item>, m: Mrk) -> SmallVector<Gc<ast::Item>> {
-    new_mark_folder(m).fold_item(expr)
+    Marker{mark:m}.fold_item(expr)
 }
 
 fn original_span(cx: &ExtCtxt) -> Gc<codemap::ExpnInfo> {
@@ -1013,7 +1039,8 @@ fn original_span(cx: &ExtCtxt) -> Gc<codemap::ExpnInfo> {
 
 #[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;
@@ -1043,22 +1070,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<ast::Path> ) -> PathExprFinderContext {
-        PathExprFinderContext {
-            path_accumulator: paths
-        }
+    // find the variable references in a crate
+    fn crate_varrefs(the_crate : &ast::Crate) -> Vec<ast::Path> {
+        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.
 
@@ -1150,6 +1177,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<ast::Ident> {
+        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);
@@ -1246,15 +1281,27 @@ mod test {
             0)
     }
 
-    // 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]
+    // 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(
-            &("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)
+    }
+
+    // interpolated nodes weren't getting labeled.
+    // 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)
     }
@@ -1283,15 +1330,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());
@@ -1315,9 +1355,13 @@ mod test {
                                                            .ctxt,
                                                      invalid_name);
                     if !(varref_name==binding_name) {
+                        let varref_idents : Vec<ast::Ident>
+                            = varref.segments.iter().map(|s|
+                                                         s.identifier)
+                            .collect();
                         println!("uh oh, should match but doesn't:");
-                        println!("varref: {:?}",varref);
-                        println!("binding: {:?}", *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);
@@ -1332,11 +1376,15 @@ mod test {
                             == binding_name);
                     // temp debugging:
                     if fail {
+                        let varref_idents : Vec<ast::Ident>
+                            = 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)
@@ -1344,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);
@@ -1360,10 +1408,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);
@@ -1376,10 +1421,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| {
@@ -1405,10 +1447,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
@@ -1418,12 +1458,10 @@ 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")));
     }
 
+    //
 
 }
diff --git a/src/libsyntax/ext/mtwt.rs b/src/libsyntax/ext/mtwt.rs
index 6c97a8aed1f..48895d34022 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<Vec<SyntaxContext_>>,
     mark_memo: RefCell<HashMap<(SyntaxContext,Mrk),SyntaxContext>>,
@@ -160,7 +161,7 @@ fn with_resolve_table_mut<T>(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 resolve 500+ seconds in resolve for librustc (!)
 fn resolve_internal(id: Ident,
                     table: &SCTable,
                     resolve_table: &mut ResolveTable) -> Name {