Auto merge of #32791 - LeoTestard:feature-gate-clean, r=nikomatsakis

Feature gate clean

This PR does a bit of cleaning in the feature-gate-handling code of libsyntax. It also fixes two bugs (#32782 and #32648). Changes include:

* Change the way the existing features are declared in `feature_gate.rs`. The array of features and the `Features` struct are now defined together by a single macro. `featureck.py` has been updated accordingly. Note: there are now three different arrays for active, removed and accepted features instead of a single one with a `Status` item to tell wether a feature is active, removed, or accepted. This is mainly due to the way I implemented my macro in the first time and I can switch back to a single array if needed. But an advantage of the way it is now is that when an active feature is used, the parser only searches through the list of active features. It goes through the other arrays only if the feature is not found. I like to think that error checking (in this case, checking that an used feature is active) does not slow down compilation of valid code. :) But this is not very important...
* Feature-gate checking pass now use the `Features` structure instead of looking through a string vector. This should speed them up a bit. The construction of the `Features` struct should be faster too since it is build directly when parsing features instead of calling `has_feature` dozens of times.
* The MacroVisitor pass has been removed, it was mostly useless since the `#[cfg]-stripping` phase happens before (fixes #32648). The features that must actually be checked before expansion are now checked at the time they are used. This also allows us to check attributes that are generated by macro expansion and not visible to MacroVisitor, but are also removed by macro expansion and thus not visible to PostExpansionVisitor either. This fixes #32782. Note that in order for `#[derive_*]` to be feature-gated but still accepted when generated by `#[derive(Trait)]`, I had to do a little bit of trickery with spans that I'm not totally confident into. Please review that part carefully. (It's in `libsyntax_ext/deriving/mod.rs`.)::

Note: this is a [breaking change], since programs with feature-gated attributes on macro-generated macro invocations were not rejected before. For example:

```rust
macro_rules! bar (
    () => ()
);

macro_rules! foo (
    () => (
        #[allow_internal_unstable] //~ ERROR allow_internal_unstable side-steps
        bar!();
    );
);
```
foo!();

src/librustc_driver/driver.rs

@@ -528,19 +528,13 @@ pub fn phase_2_configure_and_expand(sess: &Session,
         middle::recursion_limit::update_recursion_limit(sess, &krate);
     });
 
-    time(time_passes, "gated macro checking", || {
+    // these need to be set "early" so that expansion sees `quote` if enabled.
-        sess.track_errors(|| {
+    sess.track_errors(|| {
-            let features =
+        *sess.features.borrow_mut() =
-              syntax::feature_gate::check_crate_macros(sess.codemap(),
+            syntax::feature_gate::get_features(&sess.parse_sess.span_diagnostic,
-                                                       &sess.parse_sess.span_diagnostic,
+                                               &krate);
-                                                       &krate);
-
-            // these need to be set "early" so that expansion sees `quote` if enabled.
-            *sess.features.borrow_mut() = features;
-        })
     })?;
 
-
     krate = time(time_passes, "crate injection", || {
         syntax::std_inject::maybe_inject_crates_ref(krate, sess.opts.alt_std_name.clone())
     });

src/librustc_plugin/load.rs

@@ -51,27 +51,32 @@ pub fn load_plugins(sess: &Session,
                     addl_plugins: Option<Vec<String>>) -> Vec<PluginRegistrar> {
     let mut loader = PluginLoader::new(sess, cstore, crate_name);
 
-    for attr in &krate.attrs {
+    // do not report any error now. since crate attributes are
-        if !attr.check_name("plugin") {
+    // not touched by expansion, every use of plugin without
-            continue;
+    // the feature enabled will result in an error later...
-        }
+    if sess.features.borrow().plugin {
-
+        for attr in &krate.attrs {
-        let plugins = match attr.meta_item_list() {
+            if !attr.check_name("plugin") {
-            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();
+            let plugins = match attr.meta_item_list() {
-            loader.load_plugin(plugin.span, &plugin.name(), args);
+                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);
+            }
         }
     }
 

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<ast::Expr>, fld: &mut MacroExpander) -> P<ast::Expr> {
     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<ast::Expr>, fld: &mut MacroExpander) -> P<ast::Expr> {
         // 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);
@@ -70,10 +83,12 @@ pub fn expand_expr(e: P<ast::Expr>, fld: &mut MacroExpander) -> P<ast::Expr> {
 
         ast::ExprKind::InPlace(placer, value_expr) => {
             // Ensure feature-gate is enabled
-            feature_gate::check_for_placement_in(
+            if !fld.cx.ecfg.features.unwrap().placement_in_syntax {
-                fld.cx.ecfg.features,
+                feature_gate::emit_feature_err(
-                &fld.cx.parse_sess.span_diagnostic,
+                    &fld.cx.parse_sess.span_diagnostic, "placement_in_syntax", expr_span,
-                expr_span);
+                    feature_gate::GateIssue::Language, feature_gate::EXPLAIN_PLACEMENT_IN
+                );
+            }
 
             let placer = fld.fold_expr(placer);
             let value_expr = fld.fold_expr(value_expr);
@@ -370,6 +385,8 @@ pub fn expand_item_mac(it: P<ast::Item>,
         _ => 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) {
@@ -444,18 +461,6 @@ pub fn expand_item_mac(it: P<ast::Item>,
                     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,
@@ -519,6 +524,10 @@ fn expand_stmt(stmt: Stmt, fld: &mut MacroExpander) -> SmallVector<Stmt> {
         _ => 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);
 
@@ -1066,7 +1075,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)
@@ -1075,13 +1084,11 @@ fn expand_impl_item(ii: ast::ImplItem, fld: &mut MacroExpander)
             },
             span: fld.new_span(ii.span)
         }),
-        ast::ImplItemKind::Macro(_) => {
+        ast::ImplItemKind::Macro(mac) => {
-            let (span, mac) = match ii.node {
+            check_attributes(&ii.attrs, fld);
-                ast::ImplItemKind::Macro(mac) => (ii.span, mac),
+
-                _ => unreachable!()
-            };
             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);
@@ -1348,14 +1355,14 @@ impl<'feat> ExpansionConfig<'feat> {
     }
 
     feature_tests! {
-        fn enable_quotes = allow_quote,
+        fn enable_quotes = quote,
-        fn enable_asm = allow_asm,
+        fn enable_asm = asm,
-        fn enable_log_syntax = allow_log_syntax,
+        fn enable_log_syntax = log_syntax,
-        fn enable_concat_idents = allow_concat_idents,
+        fn enable_concat_idents = concat_idents,
-        fn enable_trace_macros = allow_trace_macros,
+        fn enable_trace_macros = trace_macros,
         fn enable_allow_internal_unstable = allow_internal_unstable,
-        fn enable_custom_derive = allow_custom_derive,
+        fn enable_custom_derive = custom_derive,
-        fn enable_pushpop_unsafe = allow_pushpop_unsafe,
+        fn enable_pushpop_unsafe = pushpop_unsafe,
     }
 }
 

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);
                     }

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]
+        #[allow_internal_unstable] //~ ERROR allow_internal_unstable side-steps
-        macro_rules! baz { //~ ERROR allow_internal_unstable side-steps
+        macro_rules! baz {
             () => {}
         }
     }

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() {
+#[allow_internal_unstable] //~ ERROR allow_internal_unstable side-steps
-    // (Infrastructure does not attempt to detect uses in macro definitions.)
+struct S;
-    macro_rules! expando {
-        ($x: ident) => { trace_macros!($x) }
-    }
 
-    expando!(true); //~ ERROR `trace_macros` is not stable
+fn main() {}
-}

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 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, 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

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 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, 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() {}

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
 }

src/test/compile-fail/trace_macros-gate3.rs

@@ -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 <LICENSE-APACHE or
-// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
-// <LICENSE-MIT or http://opensource.org/licenses/MIT>, 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
-}

src/tools/tidy/src/features.rs

@@ -136,18 +136,18 @@ fn collect_lang_features(path: &Path) -> Vec<Feature> {
 
     let mut features = Vec::new();
     for line in contents.lines().map(|l| l.trim()) {
-        if !STATUSES.iter().any(|s| line.contains(s) && line.starts_with("(")) {
+        if !STATUSES.iter().any(|s| line.starts_with(&format!("({}", s))) {
             continue
         }
         let mut parts = line.split(",");
-        let name = parts.next().unwrap().replace("\"", "").replace("(", "");
+        let status = match &parts.next().unwrap().trim().replace("(", "")[..] {
-        let since = parts.next().unwrap().trim().replace("\"", "");
+            "active"   => "unstable",
-        let status = match parts.skip(1).next().unwrap() {
+            "removed"  => "unstable",
-            s if s.contains("Active") => "unstable",
+            "accepted" => "stable",
-            s if s.contains("Removed") => "unstable",
-            s if s.contains("Accepted") => "stable",
             s => panic!("unknown status: {}", s),
         };
+        let name = parts.next().unwrap().trim().to_owned();
+        let since = parts.next().unwrap().trim().replace("\"", "");
 
         features.push(Feature {
             name: name,