From 41da99dff417eadf8f296a93529d9810f79e1d1b Mon Sep 17 00:00:00 2001 From: Corey Richardson Date: Wed, 24 Dec 2014 00:44:13 -0500 Subject: [PATCH] Feature gate macro arguments Uses the same approach as https://github.com/rust-lang/rust/pull/17286 (and subsequent changes making it more correct), where the visitor will skip any pieces of the AST that are from "foreign code", where the spans don't line up, indicating that that piece of code is due to a macro expansion. If this breaks your code, read the error message to determine which feature gate you should add to your crate, and bask in the knowledge that your code won't mysteriously break should you try to use the 1.0 release. Closes #18102 [breaking-change] --- src/librand/lib.rs | 3 +- src/librustc/lint/builtin.rs | 28 +--- src/librustc_driver/driver.rs | 40 +++--- src/libsyntax/codemap.rs | 32 +++++ src/libsyntax/feature_gate.rs | 123 +++++++++++++----- .../feature-gated-feature-in-macro-arg.rs | 24 ++++ 6 files changed, 172 insertions(+), 78 deletions(-) create mode 100644 src/test/compile-fail/feature-gated-feature-in-macro-arg.rs diff --git a/src/librand/lib.rs b/src/librand/lib.rs index 273b991bc22f2..59f86db73c903 100644 --- a/src/librand/lib.rs +++ b/src/librand/lib.rs @@ -271,7 +271,8 @@ pub trait Rng { /// let choices = [1i, 2, 4, 8, 16, 32]; /// let mut rng = thread_rng(); /// println!("{}", rng.choose(&choices)); - /// assert_eq!(rng.choose(choices[..0]), None); + /// # // replace with slicing syntax when it's stable! + /// assert_eq!(rng.choose(choices.slice_to(0)), None); /// ``` fn choose<'a, T>(&mut self, values: &'a [T]) -> Option<&'a T> { if values.is_empty() { diff --git a/src/librustc/lint/builtin.rs b/src/librustc/lint/builtin.rs index 35c29f646e4a0..f6f79f5d0005e 100644 --- a/src/librustc/lint/builtin.rs +++ b/src/librustc/lint/builtin.rs @@ -1681,33 +1681,7 @@ impl Stability { } fn is_internal(&self, cx: &Context, span: Span) -> bool { - // first, check if the given expression was generated by a macro or not - // we need to go back the expn_info tree to check only the arguments - // of the initial macro call, not the nested ones. - let mut expnid = span.expn_id; - let mut is_internal = false; - while cx.tcx.sess.codemap().with_expn_info(expnid, |expninfo| { - match expninfo { - Some(ref info) => { - // save the parent expn_id for next loop iteration - expnid = info.call_site.expn_id; - if info.callee.span.is_none() { - // it's a compiler built-in, we *really* don't want to mess with it - // so we skip it, unless it was called by a regular macro, in which case - // we will handle the caller macro next turn - is_internal = true; - true // continue looping - } else { - // was this expression from the current macro arguments ? - is_internal = !( span.lo > info.call_site.lo && - span.hi < info.call_site.hi ); - true // continue looping - } - }, - _ => false // stop looping - } - }) { /* empty while loop body */ } - return is_internal; + cx.tcx.sess.codemap().span_is_internal(span) } } diff --git a/src/librustc_driver/driver.rs b/src/librustc_driver/driver.rs index 9084e745bfb8e..91902b906735b 100644 --- a/src/librustc_driver/driver.rs +++ b/src/librustc_driver/driver.rs @@ -178,21 +178,6 @@ pub fn phase_2_configure_and_expand(sess: &Session, *sess.crate_metadata.borrow_mut() = collect_crate_metadata(sess, krate.attrs[]); - time(time_passes, "gated feature checking", (), |_| { - let (features, unknown_features) = - syntax::feature_gate::check_crate(&sess.parse_sess.span_diagnostic, &krate); - - for uf in unknown_features.iter() { - sess.add_lint(lint::builtin::UNKNOWN_FEATURES, - ast::CRATE_NODE_ID, - *uf, - "unknown feature".to_string()); - } - - sess.abort_if_errors(); - *sess.features.borrow_mut() = features; - }); - time(time_passes, "recursion limit", (), |_| { middle::recursion_limit::update_recursion_limit(sess, &krate); }); @@ -205,6 +190,23 @@ pub fn phase_2_configure_and_expand(sess: &Session, // // baz! should not use this definition unless foo is enabled. + time(time_passes, "gated macro checking", (), |_| { + let (features, unknown_features) = + syntax::feature_gate::check_crate_macros(sess.codemap(), + &sess.parse_sess.span_diagnostic, + &krate); + for uf in unknown_features.iter() { + sess.add_lint(lint::builtin::UNKNOWN_FEATURES, + ast::CRATE_NODE_ID, + *uf, + "unknown feature".to_string()); + } + + // these need to be set "early" so that expansion sees `quote` if enabled. + *sess.features.borrow_mut() = features; + sess.abort_if_errors(); + }); + krate = time(time_passes, "configuration 1", krate, |krate| syntax::config::strip_unconfigured_items(sess.diagnostic(), krate)); @@ -289,6 +291,14 @@ pub fn phase_2_configure_and_expand(sess: &Session, } ); + // Needs to go *after* expansion to be able to check the results of macro expansion. + time(time_passes, "complete gated feature checking", (), |_| { + syntax::feature_gate::check_crate(sess.codemap(), + &sess.parse_sess.span_diagnostic, + &krate); + sess.abort_if_errors(); + }); + // JBC: make CFG processing part of expansion to avoid this problem: // strip again, in case expansion added anything with a #[cfg]. diff --git a/src/libsyntax/codemap.rs b/src/libsyntax/codemap.rs index 6b9af29c60457..e61afb8b193af 100644 --- a/src/libsyntax/codemap.rs +++ b/src/libsyntax/codemap.rs @@ -563,6 +563,38 @@ impl CodeMap { ExpnId(i) => f(Some(&(*self.expansions.borrow())[i as uint])) } } + + /// Check if a span is "internal" to a macro. This means that it is entirely generated by a + /// macro expansion and contains no code that was passed in as an argument. + pub fn span_is_internal(&self, span: Span) -> bool { + // first, check if the given expression was generated by a macro or not + // we need to go back the expn_info tree to check only the arguments + // of the initial macro call, not the nested ones. + let mut is_internal = false; + let mut expnid = span.expn_id; + while self.with_expn_info(expnid, |expninfo| { + match expninfo { + Some(ref info) => { + // save the parent expn_id for next loop iteration + expnid = info.call_site.expn_id; + if info.callee.span.is_none() { + // it's a compiler built-in, we *really* don't want to mess with it + // so we skip it, unless it was called by a regular macro, in which case + // we will handle the caller macro next turn + is_internal = true; + true // continue looping + } else { + // was this expression from the current macro arguments ? + is_internal = !( span.lo > info.call_site.lo && + span.hi < info.call_site.hi ); + true // continue looping + } + }, + _ => false // stop looping + } + }) { /* empty while loop body */ } + return is_internal; + } } #[cfg(test)] diff --git a/src/libsyntax/feature_gate.rs b/src/libsyntax/feature_gate.rs index 4607520655ea1..b2c2d7eb626d1 100644 --- a/src/libsyntax/feature_gate.rs +++ b/src/libsyntax/feature_gate.rs @@ -24,7 +24,7 @@ use ast::NodeId; use ast; use attr; use attr::AttrMetaMethods; -use codemap::Span; +use codemap::{CodeMap, Span}; use diagnostic::SpanHandler; use visit; use visit::Visitor; @@ -127,6 +127,7 @@ impl Features { struct Context<'a> { features: Vec<&'static str>, span_handler: &'a SpanHandler, + cm: &'a CodeMap, } impl<'a> Context<'a> { @@ -144,7 +145,71 @@ impl<'a> Context<'a> { } } -impl<'a, 'v> Visitor<'v> for Context<'a> { +struct MacroVisitor<'a> { + context: &'a Context<'a> +} + +impl<'a, 'v> Visitor<'v> for MacroVisitor<'a> { + fn visit_view_item(&mut self, i: &ast::ViewItem) { + match i.node { + ast::ViewItemExternCrate(..) => { + for attr in i.attrs.iter() { + if attr.name().get() == "phase"{ + self.context.gate_feature("phase", attr.span, + "compile time crate loading is \ + experimental and possibly buggy"); + } + } + }, + _ => { } + } + visit::walk_view_item(self, i) + } + + fn visit_mac(&mut self, macro: &ast::Mac) { + let ast::MacInvocTT(ref path, _, _) = macro.node; + let id = path.segments.last().unwrap().identifier; + + if id == token::str_to_ident("macro_rules") { + self.context.gate_feature("macro_rules", path.span, "macro definitions are \ + not stable enough for use and are subject to change"); + } + + else if id == token::str_to_ident("asm") { + self.context.gate_feature("asm", path.span, "inline assembly is not \ + stable enough for use and is subject to change"); + } + + else if id == token::str_to_ident("log_syntax") { + self.context.gate_feature("log_syntax", path.span, "`log_syntax!` is not \ + stable enough for use and is subject to change"); + } + + else if id == token::str_to_ident("trace_macros") { + self.context.gate_feature("trace_macros", path.span, "`trace_macros` is not \ + stable enough for use and is subject to change"); + } + + else if id == token::str_to_ident("concat_idents") { + self.context.gate_feature("concat_idents", path.span, "`concat_idents` is not \ + stable enough for use and is subject to change"); + } + } +} + +struct PostExpansionVisitor<'a> { + context: &'a Context<'a> +} + +impl<'a> PostExpansionVisitor<'a> { + fn gate_feature(&self, feature: &str, span: Span, explain: &str) { + if !self.context.cm.span_is_internal(span) { + self.context.gate_feature(feature, span, explain) + } + } +} + +impl<'a, 'v> Visitor<'v> for PostExpansionVisitor<'a> { fn visit_name(&mut self, sp: Span, name: ast::Name) { if !token::get_name(name).get().is_ascii() { self.gate_feature("non_ascii_idents", sp, @@ -217,7 +282,7 @@ impl<'a, 'v> Visitor<'v> for Context<'a> { } ast::ItemImpl(_, _, _, _, ref items) => { - if attr::contains_name(i.attrs.as_slice(), + if attr::contains_name(i.attrs[], "unsafe_destructor") { self.gate_feature("unsafe_destructor", i.span, @@ -256,36 +321,6 @@ impl<'a, 'v> Visitor<'v> for Context<'a> { } } - fn visit_mac(&mut self, macro: &ast::Mac) { - let ast::MacInvocTT(ref path, _, _) = macro.node; - let id = path.segments.last().unwrap().identifier; - - if id == token::str_to_ident("macro_rules") { - self.gate_feature("macro_rules", path.span, "macro definitions are \ - not stable enough for use and are subject to change"); - } - - else if id == token::str_to_ident("asm") { - self.gate_feature("asm", path.span, "inline assembly is not \ - stable enough for use and is subject to change"); - } - - else if id == token::str_to_ident("log_syntax") { - self.gate_feature("log_syntax", path.span, "`log_syntax!` is not \ - stable enough for use and is subject to change"); - } - - else if id == token::str_to_ident("trace_macros") { - self.gate_feature("trace_macros", path.span, "`trace_macros` is not \ - stable enough for use and is subject to change"); - } - - else if id == token::str_to_ident("concat_idents") { - self.gate_feature("concat_idents", path.span, "`concat_idents` is not \ - stable enough for use and is subject to change"); - } - } - fn visit_foreign_item(&mut self, i: &ast::ForeignItem) { if attr::contains_name(i.attrs[], "linkage") { self.gate_feature("linkage", i.span, @@ -371,10 +406,15 @@ impl<'a, 'v> Visitor<'v> for Context<'a> { } } -pub fn check_crate(span_handler: &SpanHandler, krate: &ast::Crate) -> (Features, Vec) { +fn check_crate_inner(cm: &CodeMap, span_handler: &SpanHandler, krate: &ast::Crate, + check: F) + -> (Features, Vec) + where F: FnOnce(&mut Context, &ast::Crate) +{ let mut cx = Context { features: Vec::new(), span_handler: span_handler, + cm: cm, }; let mut unknown_features = Vec::new(); @@ -419,7 +459,7 @@ pub fn check_crate(span_handler: &SpanHandler, krate: &ast::Crate) -> (Features, } } - visit::walk_crate(&mut cx, krate); + check(&mut cx, krate); (Features { default_type_params: cx.has_feature("default_type_params"), @@ -432,3 +472,16 @@ pub fn check_crate(span_handler: &SpanHandler, krate: &ast::Crate) -> (Features, }, unknown_features) } + +pub fn check_crate_macros(cm: &CodeMap, span_handler: &SpanHandler, krate: &ast::Crate) +-> (Features, Vec) { + check_crate_inner(cm, span_handler, krate, + |ctx, krate| visit::walk_crate(&mut MacroVisitor { context: ctx }, krate)) +} + +pub fn check_crate(cm: &CodeMap, span_handler: &SpanHandler, krate: &ast::Crate) +-> (Features, Vec) { + check_crate_inner(cm, span_handler, krate, + |ctx, krate| visit::walk_crate(&mut PostExpansionVisitor { context: ctx }, + krate)) +} diff --git a/src/test/compile-fail/feature-gated-feature-in-macro-arg.rs b/src/test/compile-fail/feature-gated-feature-in-macro-arg.rs new file mode 100644 index 0000000000000..cd49c7c016eeb --- /dev/null +++ b/src/test/compile-fail/feature-gated-feature-in-macro-arg.rs @@ -0,0 +1,24 @@ +// Copyright 2014 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. + +// tests that input to a macro is checked for use of gated features. If this +// test succeeds due to the acceptance of a feature, pick a new feature to +// test. Not ideal, but oh well :( + +fn main() { + let a = &[1i32, 2, 3]; + println!("{}", { + extern "rust-intrinsic" { //~ ERROR intrinsics are subject to change + fn atomic_fence(); + } + atomic_fence(); + 42 + }); +}