From 03ab057f97910769846f06ef904eb3eedc20134e Mon Sep 17 00:00:00 2001 From: Leo Testard Date: Thu, 7 Apr 2016 00:43:03 +0200 Subject: [PATCH] Remove the MacroVisitor pass. This pass was supposed to check use of gated features before `#[cfg]`-stripping but this was not the case since it in fact happens after. Checks that are actually important and must be done before macro expansion are now made where the features are actually used. Close #32648. Also ensure that attributes on macro-generated macro invocations are checked as well. Close #32782 and #32655. --- src/librustc_driver/driver.rs | 16 +-- src/librustc_plugin/load.rs | 39 +++--- src/libsyntax/ext/expand.rs | 43 ++++--- src/libsyntax/feature_gate.rs | 117 ++++-------------- src/libsyntax_ext/deriving/mod.rs | 94 +++++++++----- ...te-allow-internal-unstable-nested-macro.rs | 4 +- ...re-gate-allow-internal-unstable-struct.rs} | 15 +-- src/test/compile-fail/issue-32655.rs | 33 +++++ src/test/compile-fail/issue-32782.rs | 23 ++++ src/test/compile-fail/trace_macros-gate.rs | 2 +- src/test/compile-fail/trace_macros-gate3.rs | 20 --- 11 files changed, 207 insertions(+), 199 deletions(-) rename src/test/compile-fail/{trace_macros-gate2.rs => feature-gate-allow-internal-unstable-struct.rs} (54%) create mode 100644 src/test/compile-fail/issue-32655.rs create mode 100644 src/test/compile-fail/issue-32782.rs delete mode 100644 src/test/compile-fail/trace_macros-gate3.rs diff --git a/src/librustc_driver/driver.rs b/src/librustc_driver/driver.rs index de1a740e0bba4..bda905f3555f5 100644 --- a/src/librustc_driver/driver.rs +++ b/src/librustc_driver/driver.rs @@ -512,19 +512,13 @@ pub fn phase_2_configure_and_expand(sess: &Session, middle::recursion_limit::update_recursion_limit(sess, &krate); }); - time(time_passes, "gated macro checking", || { - sess.track_errors(|| { - let features = - syntax::feature_gate::check_crate_macros(sess.codemap(), - &sess.parse_sess.span_diagnostic, - &krate); - - // these need to be set "early" so that expansion sees `quote` if enabled. - *sess.features.borrow_mut() = features; - }) + // these need to be set "early" so that expansion sees `quote` if enabled. + sess.track_errors(|| { + *sess.features.borrow_mut() = + syntax::feature_gate::get_features(&sess.parse_sess.span_diagnostic, + &krate); })?; - krate = time(time_passes, "crate injection", || { syntax::std_inject::maybe_inject_crates_ref(krate, sess.opts.alt_std_name.clone()) }); diff --git a/src/librustc_plugin/load.rs b/src/librustc_plugin/load.rs index ac40215bbb1d0..036e46c380398 100644 --- a/src/librustc_plugin/load.rs +++ b/src/librustc_plugin/load.rs @@ -51,27 +51,32 @@ pub fn load_plugins(sess: &Session, addl_plugins: Option>) -> Vec { let mut loader = PluginLoader::new(sess, cstore, crate_name); - for attr in &krate.attrs { - if !attr.check_name("plugin") { - continue; - } - - let plugins = match attr.meta_item_list() { - Some(xs) => xs, - None => { - call_malformed_plugin_attribute(sess, attr.span); + // do not report any error now. since crate attributes are + // not touched by expansion, every use of plugin without + // the feature enabled will result in an error later... + if sess.features.borrow().plugin { + for attr in &krate.attrs { + if !attr.check_name("plugin") { continue; } - }; - for plugin in plugins { - if plugin.value_str().is_some() { - call_malformed_plugin_attribute(sess, attr.span); - continue; + let plugins = match attr.meta_item_list() { + Some(xs) => xs, + None => { + call_malformed_plugin_attribute(sess, attr.span); + continue; + } + }; + + for plugin in plugins { + if plugin.value_str().is_some() { + call_malformed_plugin_attribute(sess, attr.span); + continue; + } + + let args = plugin.meta_item_list().map(ToOwned::to_owned).unwrap_or_default(); + loader.load_plugin(plugin.span, &plugin.name(), args); } - - let args = plugin.meta_item_list().map(ToOwned::to_owned).unwrap_or_default(); - loader.load_plugin(plugin.span, &plugin.name(), args); } } diff --git a/src/libsyntax/ext/expand.rs b/src/libsyntax/ext/expand.rs index bc8d5cd770346..16b465ba36eb6 100644 --- a/src/libsyntax/ext/expand.rs +++ b/src/libsyntax/ext/expand.rs @@ -35,6 +35,16 @@ use std_inject; use std::collections::HashSet; use std::env; +// this function is called to detect use of feature-gated or invalid attributes +// on macro invoations since they will not be detected after macro expansion +fn check_attributes(attrs: &[ast::Attribute], fld: &MacroExpander) { + for attr in attrs.iter() { + feature_gate::check_attribute(&attr, &fld.cx.parse_sess.span_diagnostic, + &fld.cx.parse_sess.codemap(), + &fld.cx.ecfg.features.unwrap()); + } +} + pub fn expand_expr(e: P, fld: &mut MacroExpander) -> P { let expr_span = e.span; return e.and_then(|ast::Expr {id, node, span, attrs}| match node { @@ -42,6 +52,9 @@ pub fn expand_expr(e: P, fld: &mut MacroExpander) -> P { // expr_mac should really be expr_ext or something; it's the // entry-point for all syntax extensions. ast::ExprKind::Mac(mac) => { + if let Some(ref attrs) = attrs { + check_attributes(attrs, fld); + } // Assert that we drop any macro attributes on the floor here drop(attrs); @@ -367,6 +380,8 @@ pub fn expand_item_mac(it: P, _ => fld.cx.span_bug(it.span, "invalid item macro invocation") }); + check_attributes(&attrs, fld); + let fm = fresh_mark(); let items = { let expanded = match fld.cx.syntax_env.find(extname) { @@ -441,18 +456,6 @@ pub fn expand_item_mac(it: P, let allow_internal_unstable = attr::contains_name(&attrs, "allow_internal_unstable"); - // ensure any #[allow_internal_unstable]s are - // detected (including nested macro definitions - // etc.) - if allow_internal_unstable && !fld.cx.ecfg.enable_allow_internal_unstable() { - feature_gate::emit_feature_err( - &fld.cx.parse_sess.span_diagnostic, - "allow_internal_unstable", - span, - feature_gate::GateIssue::Language, - feature_gate::EXPLAIN_ALLOW_INTERNAL_UNSTABLE) - } - let export = attr::contains_name(&attrs, "macro_export"); let def = ast::MacroDef { ident: ident, @@ -516,6 +519,10 @@ fn expand_stmt(stmt: Stmt, fld: &mut MacroExpander) -> SmallVector { _ => return expand_non_macro_stmt(stmt, fld) }; + if let Some(ref attrs) = attrs { + check_attributes(attrs, fld); + } + // Assert that we drop any macro attributes on the floor here drop(attrs); @@ -1063,7 +1070,7 @@ fn expand_impl_item(ii: ast::ImplItem, fld: &mut MacroExpander) attrs: ii.attrs, vis: ii.vis, defaultness: ii.defaultness, - node: match ii.node { + node: match ii.node { ast::ImplItemKind::Method(sig, body) => { let (sig, body) = expand_and_rename_method(sig, body, fld); ast::ImplItemKind::Method(sig, body) @@ -1072,13 +1079,11 @@ fn expand_impl_item(ii: ast::ImplItem, fld: &mut MacroExpander) }, span: fld.new_span(ii.span) }), - ast::ImplItemKind::Macro(_) => { - let (span, mac) = match ii.node { - ast::ImplItemKind::Macro(mac) => (ii.span, mac), - _ => unreachable!() - }; + ast::ImplItemKind::Macro(mac) => { + check_attributes(&ii.attrs, fld); + let maybe_new_items = - expand_mac_invoc(mac, span, + expand_mac_invoc(mac, ii.span, |r| r.make_impl_items(), |meths, mark| meths.move_map(|m| mark_impl_item(m, mark)), fld); diff --git a/src/libsyntax/feature_gate.rs b/src/libsyntax/feature_gate.rs index e281e30dbc28f..63fc33e48727d 100644 --- a/src/libsyntax/feature_gate.rs +++ b/src/libsyntax/feature_gate.rs @@ -688,7 +688,7 @@ pub fn check_for_pushpop_syntax(f: Option<&Features>, diag: &Handler, span: Span } struct Context<'a> { - features: Features, + features: &'a Features, span_handler: &'a Handler, cm: &'a CodeMap, plugin_attributes: &'a [(String, AttributeType)], @@ -739,9 +739,7 @@ impl<'a> Context<'a> { with the prefix `rustc_` \ are reserved for internal compiler diagnostics"); } else if name.starts_with("derive_") { - gate_feature!(self, custom_derive, attr.span, - "attributes of the form `#[derive_*]` are reserved \ - for the compiler"); + gate_feature!(self, custom_derive, attr.span, EXPLAIN_DERIVE_UNDERSCORE); } else { // Only run the custom attribute lint during regular // feature gate checking. Macro gating runs @@ -759,6 +757,15 @@ impl<'a> Context<'a> { } } +pub fn check_attribute(attr: &ast::Attribute, handler: &Handler, + cm: &CodeMap, features: &Features) { + let cx = Context { + features: features, span_handler: handler, + cm: cm, plugin_attributes: &[] + }; + cx.check_attribute(attr, true); +} + fn find_lang_feature_issue(feature: &str) -> Option { if let Some(info) = ACTIVE_FEATURES.iter().find(|t| t.0 == feature) { let issue = info.2; @@ -819,64 +826,8 @@ pub const EXPLAIN_ALLOW_INTERNAL_UNSTABLE: &'static str = pub const EXPLAIN_CUSTOM_DERIVE: &'static str = "`#[derive]` for custom traits is not stable enough for use and is subject to change"; -struct MacroVisitor<'a> { - context: &'a Context<'a> -} - -impl<'a, 'v> Visitor<'v> for MacroVisitor<'a> { - fn visit_mac(&mut self, mac: &ast::Mac) { - let path = &mac.node.path; - let name = path.segments.last().unwrap().identifier.name.as_str(); - - // Issue 22234: If you add a new case here, make sure to also - // add code to catch the macro during or after expansion. - // - // We still keep this MacroVisitor (rather than *solely* - // relying on catching cases during or after expansion) to - // catch uses of these macros within conditionally-compiled - // code, e.g. `#[cfg]`-guarded functions. - - if name == "asm" { - gate_feature!(self.context, asm, path.span, EXPLAIN_ASM); - } - - else if name == "log_syntax" { - gate_feature!(self.context, log_syntax, path.span, EXPLAIN_LOG_SYNTAX); - } - - else if name == "trace_macros" { - gate_feature!(self.context, trace_macros, path.span, EXPLAIN_TRACE_MACROS); - } - - else if name == "concat_idents" { - gate_feature!(self.context, concat_idents, path.span, EXPLAIN_CONCAT_IDENTS); - } - } - - fn visit_attribute(&mut self, attr: &'v ast::Attribute) { - self.context.check_attribute(attr, true); - } - - fn visit_expr(&mut self, e: &ast::Expr) { - // Issue 22181: overloaded-`box` and placement-`in` are - // implemented via a desugaring expansion, so their feature - // gates go into MacroVisitor since that works pre-expansion. - // - // Issue 22234: we also check during expansion as well. - // But we keep these checks as a pre-expansion check to catch - // uses in e.g. conditionalized code. - - if let ast::ExprKind::Box(_) = e.node { - gate_feature!(self.context, box_syntax, e.span, EXPLAIN_BOX_SYNTAX); - } - - if let ast::ExprKind::InPlace(..) = e.node { - gate_feature!(self.context, placement_in_syntax, e.span, EXPLAIN_PLACEMENT_IN); - } - - visit::walk_expr(self, e); - } -} +pub const EXPLAIN_DERIVE_UNDERSCORE: &'static str = + "attributes of the form `#[derive_*]` are reserved for the compiler"; struct PostExpansionVisitor<'a> { context: &'a Context<'a>, @@ -1177,13 +1128,7 @@ impl<'a, 'v> Visitor<'v> for PostExpansionVisitor<'a> { } } -fn check_crate_inner(cm: &CodeMap, span_handler: &Handler, - krate: &ast::Crate, - plugin_attributes: &[(String, AttributeType)], - check: F) - -> Features - where F: FnOnce(&mut Context, &ast::Crate) -{ +pub fn get_features(span_handler: &Handler, krate: &ast::Crate) -> Features { let mut features = Features::new(); for attr in &krate.attrs { @@ -1226,32 +1171,24 @@ fn check_crate_inner(cm: &CodeMap, span_handler: &Handler, } } - let mut cx = Context { - features: features, - span_handler: span_handler, - cm: cm, - plugin_attributes: plugin_attributes, - }; - - check(&mut cx, krate); - cx.features -} - -pub fn check_crate_macros(cm: &CodeMap, span_handler: &Handler, krate: &ast::Crate) --> Features { - check_crate_inner(cm, span_handler, krate, &[] as &'static [_], - |ctx, krate| visit::walk_crate(&mut MacroVisitor { context: ctx }, krate)) + features } pub fn check_crate(cm: &CodeMap, span_handler: &Handler, krate: &ast::Crate, plugin_attributes: &[(String, AttributeType)], - unstable: UnstableFeatures) -> Features -{ + unstable: UnstableFeatures) -> Features { maybe_stage_features(span_handler, krate, unstable); - - check_crate_inner(cm, span_handler, krate, plugin_attributes, - |ctx, krate| visit::walk_crate(&mut PostExpansionVisitor { context: ctx }, - krate)) + let features = get_features(span_handler, krate); + { + let ctx = Context { + features: &features, + span_handler: span_handler, + cm: cm, + plugin_attributes: plugin_attributes, + }; + visit::walk_crate(&mut PostExpansionVisitor { context: &ctx }, krate); + } + features } #[derive(Clone, Copy)] diff --git a/src/libsyntax_ext/deriving/mod.rs b/src/libsyntax_ext/deriving/mod.rs index 92a141fb4ec86..4ca3196b9c5ec 100644 --- a/src/libsyntax_ext/deriving/mod.rs +++ b/src/libsyntax_ext/deriving/mod.rs @@ -96,6 +96,36 @@ fn expand_derive(cx: &mut ExtCtxt, let mut found_partial_eq = false; let mut found_eq = false; + // This span is **very** sensitive and crucial to + // getting the stability behavior we want. What we are + // doing is marking the generated `#[derive_*]` with the + // span of the `#[deriving(...)]` attribute (the + // entire attribute, not just the `PartialEq` or `Eq` + // part), but with the current backtrace. The current + // backtrace will contain a topmost entry that IS this + // `#[deriving(...)]` attribute and with the + // "allow-unstable" flag set to true. + // + // Note that we do NOT use the span of the `Eq` + // text itself. You might think this is + // equivalent, because the `Eq` appears within the + // `#[deriving(Eq)]` attribute, and hence we would + // inherit the "allows unstable" from the + // backtrace. But in fact this is not always the + // case. The actual source text that led to + // deriving can be `#[$attr]`, for example, where + // `$attr == deriving(Eq)`. In that case, the + // "#[derive_*]" would be considered to + // originate not from the deriving call but from + // text outside the deriving call, and hence would + // be forbidden from using unstable + // content. + // + // See tests src/run-pass/rfc1445 for + // examples. --nmatsakis + let span = Span { expn_id: cx.backtrace(), .. span }; + assert!(cx.parse_sess.codemap().span_allows_unstable(span)); + for titem in traits.iter().rev() { let tname = match titem.node { MetaItemKind::Word(ref tname) => tname, @@ -121,42 +151,13 @@ fn expand_derive(cx: &mut ExtCtxt, } // #[derive(Foo, Bar)] expands to #[derive_Foo] #[derive_Bar] - item.attrs.push(cx.attribute(titem.span, cx.meta_word(titem.span, + item.attrs.push(cx.attribute(span, cx.meta_word(titem.span, intern_and_get_ident(&format!("derive_{}", tname))))); } // RFC #1445. `#[derive(PartialEq, Eq)]` adds a (trusted) // `#[structural_match]` attribute. if found_partial_eq && found_eq { - // This span is **very** sensitive and crucial to - // getting the stability behavior we want. What we are - // doing is marking `#[structural_match]` with the - // span of the `#[deriving(...)]` attribute (the - // entire attribute, not just the `PartialEq` or `Eq` - // part), but with the current backtrace. The current - // backtrace will contain a topmost entry that IS this - // `#[deriving(...)]` attribute and with the - // "allow-unstable" flag set to true. - // - // Note that we do NOT use the span of the `Eq` - // text itself. You might think this is - // equivalent, because the `Eq` appears within the - // `#[deriving(Eq)]` attribute, and hence we would - // inherit the "allows unstable" from the - // backtrace. But in fact this is not always the - // case. The actual source text that led to - // deriving can be `#[$attr]`, for example, where - // `$attr == deriving(Eq)`. In that case, the - // "#[structural_match]" would be considered to - // originate not from the deriving call but from - // text outside the deriving call, and hence would - // be forbidden from using unstable - // content. - // - // See tests src/run-pass/rfc1445 for - // examples. --nmatsakis - let span = Span { expn_id: cx.backtrace(), .. span }; - assert!(cx.parse_sess.codemap().span_allows_unstable(span)); debug!("inserting structural_match with span {:?}", span); let structural_match = intern_and_get_ident("structural_match"); item.attrs.push(cx.attribute(span, @@ -188,6 +189,39 @@ macro_rules! derive_traits { mitem: &MetaItem, annotatable: &Annotatable, push: &mut FnMut(Annotatable)) { + if !ecx.parse_sess.codemap().span_allows_unstable(sp) + && !ecx.ecfg.features.unwrap().custom_derive { + // FIXME: + // https://github.com/rust-lang/rust/pull/32671#issuecomment-206245303 + // This is just to avoid breakage with syntex. + // Remove that to spawn an error instead. + let cm = ecx.parse_sess.codemap(); + let parent = cm.with_expn_info(ecx.backtrace(), + |info| info.unwrap().call_site.expn_id); + cm.with_expn_info(parent, |info| { + if info.is_some() { + let mut w = ecx.parse_sess.span_diagnostic.struct_span_warn( + sp, feature_gate::EXPLAIN_DERIVE_UNDERSCORE, + ); + if option_env!("CFG_DISABLE_UNSTABLE_FEATURES").is_none() { + w.fileline_help( + sp, &format!("add #![feature(custom_derive)] to \ + the crate attributes to enable") + ); + } + w.emit(); + } else { + feature_gate::emit_feature_err( + &ecx.parse_sess.span_diagnostic, + "custom_derive", sp, feature_gate::GateIssue::Language, + feature_gate::EXPLAIN_DERIVE_UNDERSCORE + ); + + return; + } + }) + } + warn_if_deprecated(ecx, sp, $name); $func(ecx, sp, mitem, annotatable, push); } diff --git a/src/test/compile-fail/feature-gate-allow-internal-unstable-nested-macro.rs b/src/test/compile-fail/feature-gate-allow-internal-unstable-nested-macro.rs index c9251c925cc40..9ebf8a9b74a6c 100644 --- a/src/test/compile-fail/feature-gate-allow-internal-unstable-nested-macro.rs +++ b/src/test/compile-fail/feature-gate-allow-internal-unstable-nested-macro.rs @@ -11,8 +11,8 @@ macro_rules! bar { () => { // more layers don't help: - #[allow_internal_unstable] - macro_rules! baz { //~ ERROR allow_internal_unstable side-steps + #[allow_internal_unstable] //~ ERROR allow_internal_unstable side-steps + macro_rules! baz { () => {} } } diff --git a/src/test/compile-fail/trace_macros-gate2.rs b/src/test/compile-fail/feature-gate-allow-internal-unstable-struct.rs similarity index 54% rename from src/test/compile-fail/trace_macros-gate2.rs rename to src/test/compile-fail/feature-gate-allow-internal-unstable-struct.rs index 71cc45e132d33..b186278ef8b7b 100644 --- a/src/test/compile-fail/trace_macros-gate2.rs +++ b/src/test/compile-fail/feature-gate-allow-internal-unstable-struct.rs @@ -1,4 +1,4 @@ -// Copyright 2015 The Rust Project Developers. See the COPYRIGHT +// Copyright 2016 The Rust Project Developers. See the COPYRIGHT // file at the top-level directory of this distribution and at // http://rust-lang.org/COPYRIGHT. // @@ -8,13 +8,10 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. -// Test that the trace_macros feature gate is on. +// checks that this attribute is caught on non-macro items. +// this needs a different test since this is done after expansion -fn main() { - // (Infrastructure does not attempt to detect uses in macro definitions.) - macro_rules! expando { - ($x: ident) => { trace_macros!($x) } - } +#[allow_internal_unstable] //~ ERROR allow_internal_unstable side-steps +struct S; - expando!(true); //~ ERROR `trace_macros` is not stable -} +fn main() {} diff --git a/src/test/compile-fail/issue-32655.rs b/src/test/compile-fail/issue-32655.rs new file mode 100644 index 0000000000000..edd7fe4a1e588 --- /dev/null +++ b/src/test/compile-fail/issue-32655.rs @@ -0,0 +1,33 @@ +// Copyright 2016 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. + +#![allow(dead_code)] +#![feature(rustc_attrs)] + +macro_rules! foo ( + () => ( + #[derive_Clone] //~ WARN attributes of the form + struct T; + ); +); + +macro_rules! bar ( + ($e:item) => ($e) +); + +foo!(); + +bar!( + #[derive_Clone] //~ WARN attributes of the form + struct S; +); + +#[rustc_error] +fn main() {} //~ ERROR compilation successful diff --git a/src/test/compile-fail/issue-32782.rs b/src/test/compile-fail/issue-32782.rs new file mode 100644 index 0000000000000..696ea0ef5473b --- /dev/null +++ b/src/test/compile-fail/issue-32782.rs @@ -0,0 +1,23 @@ +// Copyright 2016 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. + +macro_rules! bar ( + () => () +); + +macro_rules! foo ( + () => ( + #[allow_internal_unstable] //~ ERROR allow_internal_unstable side-steps + bar!(); + ); +); + +foo!(); +fn main() {} diff --git a/src/test/compile-fail/trace_macros-gate.rs b/src/test/compile-fail/trace_macros-gate.rs index 6473bcece91b6..d627de24d6794 100644 --- a/src/test/compile-fail/trace_macros-gate.rs +++ b/src/test/compile-fail/trace_macros-gate.rs @@ -26,5 +26,5 @@ fn main() { ($x: ident) => { trace_macros!($x) } } - expando!(true); + expando!(true); //~ ERROR `trace_macros` is not stable } diff --git a/src/test/compile-fail/trace_macros-gate3.rs b/src/test/compile-fail/trace_macros-gate3.rs deleted file mode 100644 index 66d03cf9d8046..0000000000000 --- a/src/test/compile-fail/trace_macros-gate3.rs +++ /dev/null @@ -1,20 +0,0 @@ -// Copyright 2015 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. - -// Test that the trace_macros feature gate is on. - -pub fn main() { - println!("arg: {}", trace_macros!()); //~ ERROR `trace_macros` is not stable - println!("arg: {}", trace_macros!(1)); //~ ERROR `trace_macros` is not stable - println!("arg: {}", trace_macros!(ident)); //~ ERROR `trace_macros` is not stable - println!("arg: {}", trace_macros!(for)); //~ ERROR `trace_macros` is not stable - println!("arg: {}", trace_macros!(true,)); //~ ERROR `trace_macros` is not stable - println!("arg: {}", trace_macros!(false 1)); //~ ERROR `trace_macros` is not stable -}