Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Temporarily prohibit proc macro attributes placed after derives #54277

Merged
merged 1 commit into from
Sep 17, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 15 additions & 6 deletions src/librustc_resolve/macros.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ use syntax::attr;
use syntax::errors::DiagnosticBuilder;
use syntax::ext::base::{self, Determinacy, MultiModifier, MultiDecorator};
use syntax::ext::base::{MacroKind, SyntaxExtension, Resolver as SyntaxResolver};
use syntax::ext::expand::{AstFragment, Invocation, InvocationKind};
use syntax::ext::expand::{AstFragment, Invocation, InvocationKind, TogetherWith};
use syntax::ext::hygiene::{self, Mark};
use syntax::ext::tt::macro_rules;
use syntax::feature_gate::{self, feature_err, emit_feature_err, is_builtin_attr_name, GateIssue};
Expand Down Expand Up @@ -341,21 +341,30 @@ impl<'a, 'crateloader: 'a> base::Resolver for Resolver<'a, 'crateloader> {

fn resolve_macro_invocation(&mut self, invoc: &Invocation, invoc_id: Mark, force: bool)
-> Result<Option<Lrc<SyntaxExtension>>, Determinacy> {
let (path, kind, derives_in_scope) = match invoc.kind {
let (path, kind, derives_in_scope, together_with) = match invoc.kind {
InvocationKind::Attr { attr: None, .. } =>
return Ok(None),
InvocationKind::Attr { attr: Some(ref attr), ref traits, .. } =>
(&attr.path, MacroKind::Attr, traits.clone()),
InvocationKind::Attr { attr: Some(ref attr), ref traits, together_with, .. } =>
(&attr.path, MacroKind::Attr, traits.clone(), together_with),
InvocationKind::Bang { ref mac, .. } =>
(&mac.node.path, MacroKind::Bang, Vec::new()),
(&mac.node.path, MacroKind::Bang, Vec::new(), TogetherWith::None),
InvocationKind::Derive { ref path, .. } =>
(path, MacroKind::Derive, Vec::new()),
(path, MacroKind::Derive, Vec::new(), TogetherWith::None),
};

let parent_scope = self.invoc_parent_scope(invoc_id, derives_in_scope);
let (def, ext) = self.resolve_macro_to_def(path, kind, &parent_scope, force)?;

if let Def::Macro(def_id, _) = def {
match together_with {
TogetherWith::Derive =>
self.session.span_err(invoc.span(),
"macro attributes must be placed before `#[derive]`"),
TogetherWith::TestBench if !self.session.features_untracked().plugin =>
self.session.span_err(invoc.span(),
"macro attributes cannot be used together with `#[test]` or `#[bench]`"),
_ => {}
}
self.macro_defs.insert(invoc.expansion_data.mark, def_id);
let normal_module_def_id =
self.macro_def_scope(invoc.expansion_data.mark).normal_ancestor_id;
Expand Down
98 changes: 59 additions & 39 deletions src/libsyntax/ext/expand.rs
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,14 @@ pub struct Invocation {
pub expansion_data: ExpansionData,
}

// Needed for feature-gating attributes used after derives or together with test/bench
#[derive(Clone, Copy, PartialEq)]
pub enum TogetherWith {
None,
Derive,
TestBench,
}

pub enum InvocationKind {
Bang {
mac: ast::Mac,
Expand All @@ -226,6 +234,7 @@ pub enum InvocationKind {
attr: Option<ast::Attribute>,
traits: Vec<Path>,
item: Annotatable,
together_with: TogetherWith,
},
Derive {
path: Path,
Expand Down Expand Up @@ -353,7 +362,7 @@ impl<'a, 'b> MacroExpander<'a, 'b> {
let dummy = invoc.fragment_kind.dummy(invoc.span()).unwrap();
let fragment = self.expand_invoc(invoc, &*ext).unwrap_or(dummy);
self.collect_invocations(fragment, &[])
} else if let InvocationKind::Attr { attr: None, traits, item } = invoc.kind {
} else if let InvocationKind::Attr { attr: None, traits, item, .. } = invoc.kind {
if !item.derive_allowed() {
let attr = attr::find_by_name(item.attrs(), "derive")
.expect("`derive` attribute should exist");
Expand Down Expand Up @@ -1069,14 +1078,23 @@ impl<'a, 'b> InvocationCollector<'a, 'b> {
attr: Option<ast::Attribute>,
traits: Vec<Path>,
item: Annotatable,
kind: AstFragmentKind)
kind: AstFragmentKind,
together_with: TogetherWith)
-> AstFragment {
self.collect(kind, InvocationKind::Attr { attr, traits, item })
self.collect(kind, InvocationKind::Attr { attr, traits, item, together_with })
}

fn find_attr_invoc(&self, attrs: &mut Vec<ast::Attribute>) -> Option<ast::Attribute> {
fn find_attr_invoc(&self, attrs: &mut Vec<ast::Attribute>, together_with: &mut TogetherWith)
-> Option<ast::Attribute> {
let attr = attrs.iter()
.position(|a| !attr::is_known(a) && !is_builtin_attr(a))
.position(|a| {
if a.path == "derive" {
*together_with = TogetherWith::Derive
} else if a.path == "rustc_test_marker2" {
*together_with = TogetherWith::TestBench
}
!attr::is_known(a) && !is_builtin_attr(a)
})
.map(|i| attrs.remove(i));
if let Some(attr) = &attr {
if !self.cx.ecfg.enable_custom_inner_attributes() &&
Expand All @@ -1086,14 +1104,19 @@ impl<'a, 'b> InvocationCollector<'a, 'b> {
"non-builtin inner attributes are unstable");
}
}
if together_with == &TogetherWith::None &&
attrs.iter().any(|a| a.path == "rustc_test_marker2") {
*together_with = TogetherWith::TestBench;
}
attr
}

/// If `item` is an attr invocation, remove and return the macro attribute and derive traits.
fn classify_item<T>(&mut self, mut item: T) -> (Option<ast::Attribute>, Vec<Path>, T)
fn classify_item<T>(&mut self, mut item: T)
-> (Option<ast::Attribute>, Vec<Path>, T, TogetherWith)
where T: HasAttrs,
{
let (mut attr, mut traits) = (None, Vec::new());
let (mut attr, mut traits, mut together_with) = (None, Vec::new(), TogetherWith::None);

item = item.map_attrs(|mut attrs| {
if let Some(legacy_attr_invoc) = self.cx.resolver.find_legacy_attr_invoc(&mut attrs,
Expand All @@ -1102,19 +1125,20 @@ impl<'a, 'b> InvocationCollector<'a, 'b> {
return attrs;
}

attr = self.find_attr_invoc(&mut attrs);
attr = self.find_attr_invoc(&mut attrs, &mut together_with);
traits = collect_derives(&mut self.cx, &mut attrs);
attrs
});

(attr, traits, item)
(attr, traits, item, together_with)
}

/// Alternative of `classify_item()` that ignores `#[derive]` so invocations fallthrough
/// to the unused-attributes lint (making it an error on statements and expressions
/// is a breaking change)
fn classify_nonitem<T: HasAttrs>(&mut self, mut item: T) -> (Option<ast::Attribute>, T) {
let mut attr = None;
fn classify_nonitem<T: HasAttrs>(&mut self, mut item: T)
-> (Option<ast::Attribute>, T, TogetherWith) {
let (mut attr, mut together_with) = (None, TogetherWith::None);

item = item.map_attrs(|mut attrs| {
if let Some(legacy_attr_invoc) = self.cx.resolver.find_legacy_attr_invoc(&mut attrs,
Expand All @@ -1123,11 +1147,11 @@ impl<'a, 'b> InvocationCollector<'a, 'b> {
return attrs;
}

attr = self.find_attr_invoc(&mut attrs);
attr = self.find_attr_invoc(&mut attrs, &mut together_with);
attrs
});

(attr, item)
(attr, item, together_with)
}

fn configure<T: HasAttrs>(&mut self, node: T) -> Option<T> {
Expand Down Expand Up @@ -1166,7 +1190,7 @@ impl<'a, 'b> Folder for InvocationCollector<'a, 'b> {
expr.node = self.cfg.configure_expr_kind(expr.node);

// ignore derives so they remain unused
let (attr, expr) = self.classify_nonitem(expr);
let (attr, expr, together_with) = self.classify_nonitem(expr);

if attr.is_some() {
// collect the invoc regardless of whether or not attributes are permitted here
Expand All @@ -1175,7 +1199,7 @@ impl<'a, 'b> Folder for InvocationCollector<'a, 'b> {

// AstFragmentKind::Expr requires the macro to emit an expression
return self.collect_attr(attr, vec![], Annotatable::Expr(P(expr)),
AstFragmentKind::Expr).make_expr();
AstFragmentKind::Expr, together_with).make_expr();
}

if let ast::ExprKind::Mac(mac) = expr.node {
Expand All @@ -1191,14 +1215,13 @@ impl<'a, 'b> Folder for InvocationCollector<'a, 'b> {
expr.node = self.cfg.configure_expr_kind(expr.node);

// ignore derives so they remain unused
let (attr, expr) = self.classify_nonitem(expr);
let (attr, expr, together_with) = self.classify_nonitem(expr);

if attr.is_some() {
attr.as_ref().map(|a| self.cfg.maybe_emit_expr_attr_err(a));

return self.collect_attr(attr, vec![], Annotatable::Expr(P(expr)),
AstFragmentKind::OptExpr)
.make_opt_expr();
AstFragmentKind::OptExpr, together_with).make_opt_expr();
}

if let ast::ExprKind::Mac(mac) = expr.node {
Expand Down Expand Up @@ -1230,19 +1253,18 @@ impl<'a, 'b> Folder for InvocationCollector<'a, 'b> {

// we'll expand attributes on expressions separately
if !stmt.is_expr() {
let (attr, derives, stmt_) = if stmt.is_item() {
let (attr, derives, stmt_, together_with) = if stmt.is_item() {
self.classify_item(stmt)
} else {
// ignore derives on non-item statements so it falls through
// to the unused-attributes lint
let (attr, stmt) = self.classify_nonitem(stmt);
(attr, vec![], stmt)
let (attr, stmt, together_with) = self.classify_nonitem(stmt);
(attr, vec![], stmt, together_with)
};

if attr.is_some() || !derives.is_empty() {
return self.collect_attr(attr, derives,
Annotatable::Stmt(P(stmt_)), AstFragmentKind::Stmts)
.make_stmts();
return self.collect_attr(attr, derives, Annotatable::Stmt(P(stmt_)),
AstFragmentKind::Stmts, together_with).make_stmts();
}

stmt = stmt_;
Expand Down Expand Up @@ -1284,10 +1306,10 @@ impl<'a, 'b> Folder for InvocationCollector<'a, 'b> {
fn fold_item(&mut self, item: P<ast::Item>) -> OneVector<P<ast::Item>> {
let item = configure!(self, item);

let (attr, traits, item) = self.classify_item(item);
let (attr, traits, item, together_with) = self.classify_item(item);
if attr.is_some() || !traits.is_empty() {
let item = Annotatable::Item(item);
return self.collect_attr(attr, traits, item, AstFragmentKind::Items).make_items();
return self.collect_attr(attr, traits, Annotatable::Item(item),
AstFragmentKind::Items, together_with).make_items();
}

match item.node {
Expand Down Expand Up @@ -1359,11 +1381,10 @@ impl<'a, 'b> Folder for InvocationCollector<'a, 'b> {
fn fold_trait_item(&mut self, item: ast::TraitItem) -> OneVector<ast::TraitItem> {
let item = configure!(self, item);

let (attr, traits, item) = self.classify_item(item);
let (attr, traits, item, together_with) = self.classify_item(item);
if attr.is_some() || !traits.is_empty() {
let item = Annotatable::TraitItem(P(item));
return self.collect_attr(attr, traits, item, AstFragmentKind::TraitItems)
.make_trait_items()
return self.collect_attr(attr, traits, Annotatable::TraitItem(P(item)),
AstFragmentKind::TraitItems, together_with).make_trait_items()
}

match item.node {
Expand All @@ -1379,11 +1400,10 @@ impl<'a, 'b> Folder for InvocationCollector<'a, 'b> {
fn fold_impl_item(&mut self, item: ast::ImplItem) -> OneVector<ast::ImplItem> {
let item = configure!(self, item);

let (attr, traits, item) = self.classify_item(item);
let (attr, traits, item, together_with) = self.classify_item(item);
if attr.is_some() || !traits.is_empty() {
let item = Annotatable::ImplItem(P(item));
return self.collect_attr(attr, traits, item, AstFragmentKind::ImplItems)
.make_impl_items();
return self.collect_attr(attr, traits, Annotatable::ImplItem(P(item)),
AstFragmentKind::ImplItems, together_with).make_impl_items();
}

match item.node {
Expand Down Expand Up @@ -1414,12 +1434,12 @@ impl<'a, 'b> Folder for InvocationCollector<'a, 'b> {

fn fold_foreign_item(&mut self,
foreign_item: ast::ForeignItem) -> OneVector<ast::ForeignItem> {
let (attr, traits, foreign_item) = self.classify_item(foreign_item);
let (attr, traits, foreign_item, together_with) = self.classify_item(foreign_item);

if attr.is_some() || !traits.is_empty() {
let item = Annotatable::ForeignItem(P(foreign_item));
return self.collect_attr(attr, traits, item, AstFragmentKind::ForeignItems)
.make_foreign_items();
return self.collect_attr(attr, traits, Annotatable::ForeignItem(P(foreign_item)),
AstFragmentKind::ForeignItems, together_with)
.make_foreign_items();
}

if let ast::ForeignItemKind::Macro(mac) = foreign_item.node {
Expand Down
4 changes: 4 additions & 0 deletions src/libsyntax/feature_gate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -970,6 +970,10 @@ pub const BUILTIN_ATTRIBUTES: &'static [(&'static str, AttributeType, AttributeG
"the `#[rustc_test_marker]` attribute \
is used internally to track tests",
cfg_fn!(rustc_attrs))),
("rustc_test_marker2", Normal, Gated(Stability::Unstable,
"rustc_attrs",
"temporarily used by rustc to report some errors",
cfg_fn!(rustc_attrs))),
("rustc_transparent_macro", Whitelisted, Gated(Stability::Unstable,
"rustc_attrs",
"used internally for testing macro hygiene",
Expand Down
8 changes: 7 additions & 1 deletion src/libsyntax/ptr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@

use std::fmt::{self, Display, Debug};
use std::iter::FromIterator;
use std::ops::Deref;
use std::ops::{Deref, DerefMut};
use std::{mem, ptr, slice, vec};

use serialize::{Encodable, Decodable, Encoder, Decoder};
Expand Down Expand Up @@ -103,6 +103,12 @@ impl<T: ?Sized> Deref for P<T> {
}
}

impl<T: ?Sized> DerefMut for P<T> {
fn deref_mut(&mut self) -> &mut T {
&mut self.ptr
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was intentionally not implemented, but I'm considering ways in which we can make ASTs fully immutable (and interned, I suppose).


impl<T: 'static + Clone> Clone for P<T> {
fn clone(&self) -> P<T> {
P((**self).clone())
Expand Down
8 changes: 7 additions & 1 deletion src/libsyntax_ext/test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ pub fn expand_test_or_bench(
// If we're not in test configuration, remove the annotated item
if !cx.ecfg.should_test { return vec![]; }

let item =
let mut item =
if let Annotatable::Item(i) = item { i }
else {
cx.parse_sess.span_diagnostic.span_fatal(item.span(),
Expand Down Expand Up @@ -192,6 +192,12 @@ pub fn expand_test_or_bench(

debug!("Synthetic test item:\n{}\n", pprust::item_to_string(&test_const));

// Temporarily add another marker to the original item for error reporting
let marker2 = cx.attribute(
attr_sp, cx.meta_word(attr_sp, Symbol::intern("rustc_test_marker2"))
);
item.attrs.push(marker2);

vec![
// Access to libtest under a gensymed name
Annotatable::Item(test_extern),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,12 @@
#[macro_use]
extern crate derive_b;

#[derive(B)]
#[B] //~ ERROR `B` is a derive mode
#[C]
#[B(D)]
#[B(E = "foo")]
#[B(arbitrary tokens)]
#[derive(B)]
struct B;

fn main() {}
32 changes: 32 additions & 0 deletions src/test/ui-fulldeps/attribute-order-restricted.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
// aux-build:attr_proc_macro.rs
// compile-flags:--test

#![feature(test)]

extern crate test;
extern crate attr_proc_macro;
use attr_proc_macro::*;

#[attr_proc_macro] // OK
#[derive(Clone)]
struct Before;

#[derive(Clone)]
#[attr_proc_macro] //~ ERROR macro attributes must be placed before `#[derive]`
struct After;

#[attr_proc_macro] //~ ERROR macro attributes cannot be used together with `#[test]` or `#[bench]`
#[test]
fn test_before() {}

#[test]
#[attr_proc_macro] //~ ERROR macro attributes cannot be used together with `#[test]` or `#[bench]`
fn test_after() {}

#[attr_proc_macro] //~ ERROR macro attributes cannot be used together with `#[test]` or `#[bench]`
#[bench]
fn bench_before(b: &mut test::Bencher) {}

#[bench]
#[attr_proc_macro] //~ ERROR macro attributes cannot be used together with `#[test]` or `#[bench]`
fn bench_after(b: &mut test::Bencher) {}
Loading