From 50d9965c258276eadd8a87de20d232451aabbf83 Mon Sep 17 00:00:00 2001 From: John Clements Date: Thu, 10 Jul 2014 12:09:56 -0700 Subject: [PATCH 1/4] rename one of the two confusing MacroExpanders There were two things named MacroExpander, which was confusing. I renamed one of them TTMacroExpander. [breaking change] --- src/libsyntax/ext/base.rs | 7 ++++--- src/libsyntax/ext/tt/macro_rules.rs | 4 ++-- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/src/libsyntax/ext/base.rs b/src/libsyntax/ext/base.rs index 9a5c7e86d21..7ff680497bf 100644 --- a/src/libsyntax/ext/base.rs +++ b/src/libsyntax/ext/base.rs @@ -48,7 +48,8 @@ pub struct BasicMacroExpander { pub span: Option } -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; -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, Option), + NormalTT(Box, Option), /// A function-like syntax extension that has an extra ident before /// the block. 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>, } -impl MacroExpander for MacroRulesMacroExpander { +impl TTMacroExpander for MacroRulesMacroExpander { fn expand(&self, cx: &mut ExtCtxt, sp: Span, From f1ad425199b0d89dab275a8c8f6f29a73d316f70 Mon Sep 17 00:00:00 2001 From: John Clements Date: Thu, 10 Jul 2014 15:41:11 -0700 Subject: [PATCH 2/4] use side table to store exported macros Per discussion with @sfackler, refactored the expander to change the way that exported macros are collected. Specifically, a crate now contains a side table of spans that exported macros go into. This has two benefits. First, the encoder doesn't need to scan through the expanded crate in order to discover exported macros. Second, the expander can drop all expanded macros from the crate, with the pleasant result that a fully expanded crate contains no macro invocations (which include macro definitions). --- src/librustc/driver/session.rs | 3 ++- src/librustc/metadata/encoder.rs | 44 ++++++++++++-------------------- src/libsyntax/ast.rs | 2 ++ src/libsyntax/ext/base.rs | 7 ++++- src/libsyntax/ext/expand.rs | 9 ++++--- src/libsyntax/fold.rs | 1 + src/libsyntax/parse/parser.rs | 3 ++- 7 files changed, 34 insertions(+), 35 deletions(-) diff --git a/src/librustc/driver/session.rs b/src/librustc/driver/session.rs index 50f61f8f06a..946aa62f91c 100644 --- a/src/librustc/driver/session.rs +++ b/src/librustc/driver/session.rs @@ -28,7 +28,8 @@ use syntax::{ast, codemap}; use std::os; use std::cell::{Cell, RefCell}; - +// Represents the data associated with a compilation +// session for a single crate. pub struct Session { pub targ_cfg: config::Config, pub opts: config::Options, diff --git a/src/librustc/metadata/encoder.rs b/src/librustc/metadata/encoder.rs index fbf0288418a..7c4eda6dd71 100644 --- a/src/librustc/metadata/encoder.rs +++ b/src/librustc/metadata/encoder.rs @@ -1584,37 +1584,25 @@ fn encode_plugin_registrar_fn(ecx: &EncodeContext, ebml_w: &mut Encoder) { } } -struct MacroDefVisitor<'a, 'b, 'c> { - ecx: &'a EncodeContext<'b>, - ebml_w: &'a mut Encoder<'c> +/// Given a span, write the text of that span into the output stream +/// as an exported macro +fn encode_macro_def(ecx: &EncodeContext, + ebml_w: &mut Encoder, + span: &syntax::codemap::Span) { + let def = ecx.tcx.sess.codemap().span_to_snippet(*span) + .expect("Unable to find source for macro"); + ebml_w.start_tag(tag_macro_def); + ebml_w.wr_str(def.as_slice()); + ebml_w.end_tag(); } -impl<'a, 'b, 'c> Visitor<()> for MacroDefVisitor<'a, 'b, 'c> { - fn visit_item(&mut self, item: &Item, _: ()) { - match item.node { - ItemMac(..) => { - let def = self.ecx.tcx.sess.codemap().span_to_snippet(item.span) - .expect("Unable to find source for macro"); - self.ebml_w.start_tag(tag_macro_def); - self.ebml_w.wr_str(def.as_slice()); - self.ebml_w.end_tag(); - } - _ => {} - } - visit::walk_item(self, item, ()); - } -} - -fn encode_macro_defs<'a>(ecx: &'a EncodeContext, - krate: &Crate, - ebml_w: &'a mut Encoder) { +/// Serialize the text of the exported macros +fn encode_macro_defs(ecx: &EncodeContext, + krate: &Crate, + ebml_w: &mut Encoder) { ebml_w.start_tag(tag_exported_macros); - { - let mut visitor = MacroDefVisitor { - ecx: ecx, - ebml_w: ebml_w, - }; - visit::walk_crate(&mut visitor, krate, ()); + for span in krate.exported_macros.iter() { + encode_macro_def(ecx, ebml_w, span); } ebml_w.end_tag(); } diff --git a/src/libsyntax/ast.rs b/src/libsyntax/ast.rs index ebfc45d22ce..4a48d3f7028 100644 --- a/src/libsyntax/ast.rs +++ b/src/libsyntax/ast.rs @@ -249,6 +249,7 @@ pub struct Crate { pub attrs: Vec, pub config: CrateConfig, pub span: Span, + pub exported_macros: Vec } pub type MetaItem = Spanned; @@ -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; diff --git a/src/libsyntax/ext/base.rs b/src/libsyntax/ext/base.rs index 7ff680497bf..dcb69ae8f7e 100644 --- a/src/libsyntax/ext/base.rs +++ b/src/libsyntax/ext/base.rs @@ -410,6 +410,7 @@ pub struct ExtCtxt<'a> { pub mod_path: Vec , pub trace_mac: bool, + pub exported_macros: Vec } impl<'a> ExtCtxt<'a> { @@ -421,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(), } } @@ -539,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 b7d72ae4deb..e8a78e85d89 100644 --- a/src/libsyntax/ext/expand.rs +++ b/src/libsyntax/ext/expand.rs @@ -518,10 +518,9 @@ fn expand_item_mac(it: Gc, 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() { @@ -1039,6 +1038,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, user_exts: Vec, c: Crate) -> Crate { @@ -1066,7 +1066,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; } diff --git a/src/libsyntax/fold.rs b/src/libsyntax/fold.rs index bcdf920e5dd..f7cb1ae1934 100644 --- a/src/libsyntax/fold.rs +++ b/src/libsyntax/fold.rs @@ -713,6 +713,7 @@ pub fn noop_fold_crate(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(), } } 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(), } } From 53642eed801d157613de0998cdcf0a3da8c36cb3 Mon Sep 17 00:00:00 2001 From: John Clements Date: Wed, 9 Jul 2014 14:48:12 -0700 Subject: [PATCH 3/4] make walk/visit_mac opt-in only macros can expand into arbitrary items, exprs, etc. This means that using a default walker or folder on an AST before macro expansion is complete will miss things (the things that the macros expand into). As a partial fence against this, this commit moves the default traversal of macros into a separate procedure, and makes the default trait implementation signal an error. This means that Folders and Visitors can traverse macros if they want to, but they need to explicitly add an impl that calls the walk_mac or fold_mac procedure This should prevent problems down the road. --- src/librustc/front/config.rs | 5 ++++ src/librustc/plugin/load.rs | 5 ++++ src/libsyntax/ast_map.rs | 9 +++++++ src/libsyntax/ext/expand.rs | 7 +++++- src/libsyntax/fold.rs | 46 +++++++++++++++++++++++++++--------- src/libsyntax/visit.rs | 13 ++++++++-- 6 files changed, 71 insertions(+), 14 deletions(-) diff --git a/src/librustc/front/config.rs b/src/librustc/front/config.rs index 9bff6620aaa..0c39cf350a6 100644 --- a/src/librustc/front/config.rs +++ b/src/librustc/front/config.rs @@ -14,6 +14,8 @@ use syntax::codemap; use std::gc::{Gc, GC}; +/// A folder that strips out items that do not belong in the current +/// configuration. struct Context<'a> { in_cfg: |attrs: &[ast::Attribute]|: 'a -> bool, } @@ -41,6 +43,9 @@ impl<'a> fold::Folder for Context<'a> { fn fold_expr(&mut self, expr: Gc) -> Gc { fold_expr(self, expr) } + fn fold_mac(&mut self, mac: &ast::Mac) -> ast::Mac { + fold::fold_mac(mac, self) + } } pub fn strip_items(krate: ast::Crate, diff --git a/src/librustc/plugin/load.rs b/src/librustc/plugin/load.rs index 79d0690653f..499cffa42aa 100644 --- a/src/librustc/plugin/load.rs +++ b/src/librustc/plugin/load.rs @@ -72,6 +72,7 @@ pub fn load_plugins(sess: &Session, krate: &ast::Crate) -> Plugins { loader.plugins } +// note that macros aren't expanded yet, and therefore macros can't add plugins. impl<'a> Visitor<()> for PluginLoader<'a> { fn visit_view_item(&mut self, vi: &ast::ViewItem, _: ()) { match vi.node { @@ -109,6 +110,10 @@ impl<'a> Visitor<()> for PluginLoader<'a> { _ => (), } } + fn visit_mac(&mut self, _: &ast::Mac, _:()) { + // bummer... can't see plugins inside macros. + // do nothing. + } } impl<'a> PluginLoader<'a> { 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), } +/// 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(krate: Crate, fold_ops: F) -> (Crate, Map) { diff --git a/src/libsyntax/ext/expand.rs b/src/libsyntax/ext/expand.rs index e8a78e85d89..084faca02c5 100644 --- a/src/libsyntax/ext/expand.rs +++ b/src/libsyntax/ext/expand.rs @@ -753,7 +753,6 @@ impl Visitor<()> for PatIdentFinder { _ => visit::walk_pat(self, pattern, ()) } } - } /// find the PatIdent paths in a pattern @@ -902,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 @@ -931,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 diff --git a/src/libsyntax/fold.rs b/src/libsyntax/fold.rs index f7cb1ae1934..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| -> Gc, @@ -361,6 +367,20 @@ pub trait Folder { } + +pub fn fold_mac(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 @@ -986,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. @@ -1001,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/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 { 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) From c253b3675ab03b6c64021e8acee3988cea81f3f9 Mon Sep 17 00:00:00 2001 From: John Clements Date: Wed, 9 Jul 2014 16:41:13 -0700 Subject: [PATCH 4/4] add Macro Exterminator the Macro Exterminator ensures that there are no macro invocations in an AST. This should help make later passes confident that there aren't hidden items, methods, expressions, etc. --- src/librustc/driver/driver.rs | 7 ++++++- src/libsyntax/ext/expand.rs | 19 +++++++++++++++++++ 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/src/librustc/driver/driver.rs b/src/librustc/driver/driver.rs index 6a016edcd28..ad83e2cfe17 100644 --- a/src/librustc/driver/driver.rs +++ b/src/librustc/driver/driver.rs @@ -259,6 +259,8 @@ pub fn phase_2_configure_and_expand(sess: &Session, } ); + // JBC: make CFG processing part of expansion to avoid this problem: + // strip again, in case expansion added anything with a #[cfg]. krate = time(time_passes, "configuration 2", krate, |krate| front::config::strip_unconfigured_items(krate)); @@ -279,6 +281,9 @@ pub fn phase_2_configure_and_expand(sess: &Session, krate.encode(&mut json).unwrap(); } + time(time_passes, "checking that all macro invocations are gone", &krate, |krate| + syntax::ext::expand::check_for_macros(&sess.parse_sess, krate)); + Some((krate, map)) } @@ -291,6 +296,7 @@ pub struct CrateAnalysis { pub name: String, } + /// Run the resolution, typechecking, region checking and other /// miscellaneous analysis passes on the crate. Return various /// structures carrying the results of the analysis. @@ -298,7 +304,6 @@ pub fn phase_3_run_analysis_passes(sess: Session, krate: &ast::Crate, ast_map: syntax::ast_map::Map, name: String) -> CrateAnalysis { - let time_passes = sess.time_passes(); time(time_passes, "external crate/lib resolution", (), |_| diff --git a/src/libsyntax/ext/expand.rs b/src/libsyntax/ext/expand.rs index 084faca02c5..c82364fb17e 100644 --- a/src/libsyntax/ext/expand.rs +++ b/src/libsyntax/ext/expand.rs @@ -1151,6 +1151,25 @@ fn original_span(cx: &ExtCtxt) -> Gc { 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};