auto merge of #15601 : jbclements/rust/disable-default-macro-behavior, r=alexcrichton

Our AST definition can include macro invocations, which can expand into all kinds of things. Macro invocations are expanded away during expansion time, and the rest of the compiler doesn't have to deal with them. However, we have no way of enforcing this.

This patch adds two protective mechanisms.

First, it adds a (quick) explicit check that ensures there are no macro invocations remaining in the AST after expansion. Second, it updates the visit and fold mechanisms so that by default, they will not traverse macro invocations. It's easy enough to add this, if desired (it's documented in the source, and examples appear, e.g. in the IdentFinder.

Along the way, I also consulted with @sfackler to refactor the macro export mechanism so that it stores macro text spans in a side table, rather than leaving them in the AST.

8 files changed, 102 insertions, 25 deletions

diff --git a/src/libsyntax/ast.rs b/src/libsyntax/ast.rs
index 778f77ac7a8..dd830401443 100644
--- a/src/libsyntax/ast.rs
+++ b/src/libsyntax/ast.rs
@@ -249,6 +249,7 @@ pub struct Crate {
     pub attrs: Vec<Attribute>,
     pub config: CrateConfig,
     pub span: Span,
+    pub exported_macros: Vec<Span>
 }
 
 pub type MetaItem = Spanned<MetaItem_>;
@@ -1245,6 +1246,7 @@ mod test {
                 hi: BytePos(20),
                 expn_info: None,
             },
+            exported_macros: Vec::new(),
         };
         // doesn't matter which encoder we use....
         let _f = &e as &serialize::Encodable<json::Encoder, io::IoError>;
diff --git a/src/libsyntax/ast_map.rs b/src/libsyntax/ast_map.rs
index 25c8e81bdbc..de2ecd9a264 100644
--- a/src/libsyntax/ast_map.rs
+++ b/src/libsyntax/ast_map.rs
@@ -112,6 +112,7 @@ pub enum Node {
     NodeLifetime(Gc<Lifetime>),
 }
 
+/// Represents an entry and its parent Node ID
 /// The odd layout is to bring down the total size.
 #[deriving(Clone)]
 enum MapEntry {
@@ -184,6 +185,8 @@ impl MapEntry {
     }
 }
 
+/// Represents a mapping from Node IDs to AST elements and their parent
+/// Node IDs
 pub struct Map {
     /// NodeIds are sequential integers from 0, so we can be
     /// super-compact by storing them in a vector. Not everything with
@@ -430,6 +433,8 @@ pub trait FoldOps {
     }
 }
 
+/// A Folder that walks over an AST and constructs a Node ID Map. Its
+/// fold_ops argument has the opportunity to replace Node IDs and spans.
 pub struct Ctx<'a, F> {
     map: &'a Map,
     /// The node in which we are currently mapping (an item or a method).
@@ -584,6 +589,10 @@ impl<'a, F: FoldOps> Folder for Ctx<'a, F> {
         self.insert(lifetime.id, EntryLifetime(self.parent, box(GC) lifetime));
         lifetime
     }
+
+    fn fold_mac(&mut self, mac: &Mac) -> Mac {
+        fold::fold_mac(mac, self)
+    }
 }
 
 pub fn map_crate<F: FoldOps>(krate: Crate, fold_ops: F) -> (Crate, Map) {
diff --git a/src/libsyntax/ext/base.rs b/src/libsyntax/ext/base.rs
index 9a5c7e86d21..dcb69ae8f7e 100644
--- a/src/libsyntax/ext/base.rs
+++ b/src/libsyntax/ext/base.rs
@@ -48,7 +48,8 @@ pub struct BasicMacroExpander {
     pub span: Option<Span>
 }
 
-pub trait MacroExpander {
+/// Represents a thing that maps token trees to Macro Results
+pub trait TTMacroExpander {
     fn expand(&self,
               ecx: &mut ExtCtxt,
               span: Span,
@@ -60,7 +61,7 @@ pub type MacroExpanderFn =
     fn(ecx: &mut ExtCtxt, span: codemap::Span, token_tree: &[ast::TokenTree])
        -> Box<MacResult>;
 
-impl MacroExpander for BasicMacroExpander {
+impl TTMacroExpander for BasicMacroExpander {
     fn expand(&self,
               ecx: &mut ExtCtxt,
               span: Span,
@@ -259,7 +260,7 @@ pub enum SyntaxExtension {
     /// A normal, function-like syntax extension.
     ///
     /// `bytes!` is a `NormalTT`.
-    NormalTT(Box<MacroExpander + 'static>, Option<Span>),
+    NormalTT(Box<TTMacroExpander + 'static>, Option<Span>),
 
     /// A function-like syntax extension that has an extra ident before
     /// the block.
@@ -409,6 +410,7 @@ pub struct ExtCtxt<'a> {
 
     pub mod_path: Vec<ast::Ident> ,
     pub trace_mac: bool,
+    pub exported_macros: Vec<codemap::Span>
 }
 
 impl<'a> ExtCtxt<'a> {
@@ -420,7 +422,8 @@ impl<'a> ExtCtxt<'a> {
             backtrace: None,
             mod_path: Vec::new(),
             ecfg: ecfg,
-            trace_mac: false
+            trace_mac: false,
+            exported_macros: Vec::new(),
         }
     }
 
@@ -538,6 +541,9 @@ impl<'a> ExtCtxt<'a> {
     pub fn name_of(&self, st: &str) -> ast::Name {
         token::intern(st)
     }
+    pub fn push_exported_macro(&mut self, span: codemap::Span) {
+        self.exported_macros.push(span);
+    }
 }
 
 /// Extract a string literal from the macro expanded version of `expr`,
diff --git a/src/libsyntax/ext/expand.rs b/src/libsyntax/ext/expand.rs
index b5f7005c2a3..6e44bfa6747 100644
--- a/src/libsyntax/ext/expand.rs
+++ b/src/libsyntax/ext/expand.rs
@@ -518,10 +518,9 @@ fn expand_item_mac(it: Gc<ast::Item>, fld: &mut MacroExpander)
             // create issue to recommend refactoring here?
             fld.extsbox.insert(intern(name.as_slice()), ext);
             if attr::contains_name(it.attrs.as_slice(), "macro_export") {
-                SmallVector::one(it)
-            } else {
-                SmallVector::zero()
+                fld.cx.push_exported_macro(it.span);
             }
+            SmallVector::zero()
         }
         None => {
             match expanded.make_items() {
@@ -754,7 +753,6 @@ impl Visitor<()> for PatIdentFinder {
             _ => visit::walk_pat(self, pattern, ())
         }
     }
-
 }
 
 /// find the PatIdent paths in a pattern
@@ -903,6 +901,9 @@ impl<'a> Folder for IdentRenamer<'a> {
             ctxt: mtwt::apply_renames(self.renames, id.ctxt),
         }
     }
+    fn fold_mac(&mut self, macro: &ast::Mac) -> ast::Mac {
+        fold::fold_mac(macro, self)
+    }
 }
 
 /// A tree-folder that applies every rename in its list to
@@ -932,6 +933,9 @@ impl<'a> Folder for PatIdentRenamer<'a> {
             _ => noop_fold_pat(pat, self)
         }
     }
+    fn fold_mac(&mut self, macro: &ast::Mac) -> ast::Mac {
+        fold::fold_mac(macro, self)
+    }
 }
 
 // expand a method
@@ -1039,6 +1043,7 @@ pub struct ExportedMacros {
 
 pub fn expand_crate(parse_sess: &parse::ParseSess,
                     cfg: ExpansionConfig,
+                    // these are the macros being imported to this crate:
                     macros: Vec<ExportedMacros>,
                     user_exts: Vec<NamedSyntaxExtension>,
                     c: Crate) -> Crate {
@@ -1066,7 +1071,8 @@ pub fn expand_crate(parse_sess: &parse::ParseSess,
         expander.extsbox.insert(name, extension);
     }
 
-    let ret = expander.fold_crate(c);
+    let mut ret = expander.fold_crate(c);
+    ret.exported_macros = expander.cx.exported_macros.clone();
     parse_sess.span_diagnostic.handler().abort_if_errors();
     return ret;
 }
@@ -1145,6 +1151,25 @@ fn original_span(cx: &ExtCtxt) -> Gc<codemap::ExpnInfo> {
     return einfo;
 }
 
+/// Check that there are no macro invocations left in the AST:
+pub fn check_for_macros(sess: &parse::ParseSess, krate: &ast::Crate) {
+    visit::walk_crate(&mut MacroExterminator{sess:sess}, krate, ());
+}
+
+/// A visitor that ensures that no macro invocations remain in an AST.
+struct MacroExterminator<'a>{
+    sess: &'a parse::ParseSess
+}
+
+impl<'a> visit::Visitor<()> for MacroExterminator<'a> {
+    fn visit_mac(&mut self, macro: &ast::Mac, _:()) {
+        self.sess.span_diagnostic.span_bug(macro.span,
+                                           "macro exterminator: expected AST \
+                                           with no macro invocations");
+    }
+}
+
+
 #[cfg(test)]
 mod test {
     use super::{pattern_bindings, expand_crate, contains_macro_escape};
diff --git a/src/libsyntax/ext/tt/macro_rules.rs b/src/libsyntax/ext/tt/macro_rules.rs
index 249e9305150..923b3e78731 100644
--- a/src/libsyntax/ext/tt/macro_rules.rs
+++ b/src/libsyntax/ext/tt/macro_rules.rs
@@ -13,7 +13,7 @@ use ast::{TTDelim};
 use ast;
 use codemap::{Span, Spanned, DUMMY_SP};
 use ext::base::{ExtCtxt, MacResult, MacroDef};
-use ext::base::{NormalTT, MacroExpander};
+use ext::base::{NormalTT, TTMacroExpander};
 use ext::base;
 use ext::tt::macro_parser::{Success, Error, Failure};
 use ext::tt::macro_parser::{NamedMatch, MatchedSeq, MatchedNonterminal};
@@ -95,7 +95,7 @@ struct MacroRulesMacroExpander {
     rhses: Vec<Rc<NamedMatch>>,
 }
 
-impl MacroExpander for MacroRulesMacroExpander {
+impl TTMacroExpander for MacroRulesMacroExpander {
     fn expand(&self,
               cx: &mut ExtCtxt,
               sp: Span,
diff --git a/src/libsyntax/fold.rs b/src/libsyntax/fold.rs
index bcdf920e5dd..3e3b57be6e4 100644
--- a/src/libsyntax/fold.rs
+++ b/src/libsyntax/fold.rs
@@ -8,6 +8,16 @@
 // option. This file may not be copied, modified, or distributed
 // except according to those terms.
 
+//! A Folder represents an AST->AST fold; it accepts an AST piece,
+//! and returns a piece of the same type. So, for instance, macro
+//! expansion is a Folder that walks over an AST and produces another
+//! AST.
+//!
+//! Note: using a Folder (other than the MacroExpander Folder) on
+//! an AST before macro expansion is probably a bad idea. For instance,
+//! a folder renaming item names in a module will miss all of those
+//! that are created by the expansion of a macro.
+
 use ast::*;
 use ast;
 use ast_util;
@@ -299,17 +309,13 @@ pub trait Folder {
         }
     }
 
-    fn fold_mac(&mut self, macro: &Mac) -> Mac {
-        Spanned {
-            node: match macro.node {
-                MacInvocTT(ref p, ref tts, ctxt) => {
-                    MacInvocTT(self.fold_path(p),
-                               fold_tts(tts.as_slice(), self),
-                               ctxt)
-                }
-            },
-            span: self.new_span(macro.span)
-        }
+    fn fold_mac(&mut self, _macro: &Mac) -> Mac {
+        fail!("fold_mac disabled by default");
+        // NB: see note about macros above.
+        // if you really want a folder that
+        // works on macros, use this
+        // definition in your trait impl:
+        // fold::fold_mac(_macro, self)
     }
 
     fn map_exprs(&self, f: |Gc<Expr>| -> Gc<Expr>,
@@ -361,6 +367,20 @@ pub trait Folder {
 
 }
 
+
+pub fn fold_mac<T: Folder>(macro: &Mac, fld: &mut T) -> Mac {
+    Spanned {
+        node: match macro.node {
+            MacInvocTT(ref p, ref tts, ctxt) => {
+                MacInvocTT(fld.fold_path(p),
+                           fold_tts(tts.as_slice(), fld),
+                           ctxt)
+            }
+        },
+        span: fld.new_span(macro.span)
+    }
+}
+
 /* some little folds that probably aren't useful to have in Folder itself*/
 
 //used in noop_fold_item and noop_fold_crate and noop_fold_crate_directive
@@ -713,6 +733,7 @@ pub fn noop_fold_crate<T: Folder>(c: Crate, folder: &mut T) -> Crate {
         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),
+        exported_macros: c.exported_macros.iter().map(|sp| folder.new_span(*sp)).collect(),
     }
 }
 
@@ -985,6 +1006,7 @@ mod test {
     use util::parser_testing::{string_to_crate, matches_codepattern};
     use parse::token;
     use print::pprust;
+    use fold;
     use super::*;
 
     // this version doesn't care about getting comments or docstrings in.
@@ -1000,6 +1022,9 @@ mod test {
         fn fold_ident(&mut self, _: ast::Ident) -> ast::Ident {
             token::str_to_ident("zz")
         }
+        fn fold_mac(&mut self, macro: &ast::Mac) -> ast::Mac {
+            fold::fold_mac(macro, self)
+        }
     }
 
     // maybe add to expand.rs...
diff --git a/src/libsyntax/parse/parser.rs b/src/libsyntax/parse/parser.rs
index 743eeed9da5..84db2bc5a22 100644
--- a/src/libsyntax/parse/parser.rs
+++ b/src/libsyntax/parse/parser.rs
@@ -5386,7 +5386,8 @@ impl<'a> Parser<'a> {
             module: m,
             attrs: inner,
             config: self.cfg.clone(),
-            span: mk_sp(lo, self.span.lo)
+            span: mk_sp(lo, self.span.lo),
+            exported_macros: Vec::new(),
         }
     }
 
diff --git a/src/libsyntax/visit.rs b/src/libsyntax/visit.rs
index 9298b58c426..7caaf2f6cc1 100644
--- a/src/libsyntax/visit.rs
+++ b/src/libsyntax/visit.rs
@@ -20,6 +20,10 @@
 //! execute before AST node B, then A is visited first.  The borrow checker in
 //! particular relies on this property.
 //!
+//! Note: walking an AST before macro expansion is probably a bad idea. For
+//! instance, a walker looking for item names in a module will miss all of
+//! those that are created by the expansion of a macro.
+
 use abi::Abi;
 use ast::*;
 use ast;
@@ -124,8 +128,13 @@ pub trait Visitor<E: Clone> {
     fn visit_explicit_self(&mut self, es: &ExplicitSelf, e: E) {
         walk_explicit_self(self, es, e)
     }
-    fn visit_mac(&mut self, macro: &Mac, e: E) {
-        walk_mac(self, macro, e)
+    fn visit_mac(&mut self, _macro: &Mac, _e: E) {
+        fail!("visit_mac disabled by default");
+        // NB: see note about macros above.
+        // if you really want a visitor that
+        // works on macros, use this
+        // definition in your trait impl:
+        // visit::walk_mac(self, _macro, _e)
     }
     fn visit_path(&mut self, path: &Path, _id: ast::NodeId, e: E) {
         walk_path(self, path, e)