Skip to content

Commit

Permalink
Auto merge of #53270 - petrochenkov:macuse-regr, r=alexcrichton
Browse files Browse the repository at this point in the history
Fix a few regressions from enabling macro modularization

The first commit restores the old behavior for some minor unstable stuff (`rustc_*` and `derive_*` attributes) and adds a new feature gate for arbitrary tokens in non-macro attributes.

The second commit fixes #53205

The third commit fixes #53144.
Same technique is used as for other things blocking expansion progress - if something causes indeterminacy too often, then prohibit it.
In this case referring to crate-local macro-expanded `#[macro_export]` macros via module-relative paths is prohibited, see comments in code for more details.

cc #50911
  • Loading branch information
bors committed Aug 13, 2018
2 parents a78ae85 + dd0a766 commit d5a448b
Show file tree
Hide file tree
Showing 24 changed files with 262 additions and 116 deletions.
1 change: 0 additions & 1 deletion src/librustc_resolve/build_reduced_graph.rs
Original file line number Diff line number Diff line change
Expand Up @@ -789,7 +789,6 @@ impl<'a, 'b, 'cl> BuildReducedGraphVisitor<'a, 'b, 'cl> {
fn visit_invoc(&mut self, id: ast::NodeId) -> &'b InvocationData<'b> {
let mark = id.placeholder_to_mark();
self.resolver.current_module.unresolved_invocations.borrow_mut().insert(mark);
self.resolver.unresolved_invocations_macro_export.insert(mark);
let invocation = self.resolver.invocations[&mark];
invocation.module.set(self.resolver.current_module);
invocation.legacy_scope.set(self.legacy_scope);
Expand Down
16 changes: 12 additions & 4 deletions src/librustc_resolve/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1385,6 +1385,8 @@ pub struct Resolver<'a, 'b: 'a> {
use_injections: Vec<UseError<'a>>,
/// `use` injections for proc macros wrongly imported with #[macro_use]
proc_mac_errors: Vec<macros::ProcMacError>,
/// crate-local macro expanded `macro_export` referred to by a module-relative path
macro_expanded_macro_export_errors: BTreeSet<(Span, Span)>,

gated_errors: FxHashSet<Span>,
disallowed_shadowing: Vec<&'a LegacyBinding<'a>>,
Expand Down Expand Up @@ -1432,9 +1434,6 @@ pub struct Resolver<'a, 'b: 'a> {

/// Only supposed to be used by rustdoc, otherwise should be false.
pub ignore_extern_prelude_feature: bool,

/// Macro invocations in the whole crate that can expand into a `#[macro_export] macro_rules`.
unresolved_invocations_macro_export: FxHashSet<Mark>,
}

/// Nothing really interesting here, it just provides memory for the rest of the crate.
Expand Down Expand Up @@ -1706,6 +1705,7 @@ impl<'a, 'crateloader: 'a> Resolver<'a, 'crateloader> {
proc_mac_errors: Vec::new(),
gated_errors: FxHashSet(),
disallowed_shadowing: Vec::new(),
macro_expanded_macro_export_errors: BTreeSet::new(),

arenas,
dummy_binding: arenas.alloc_name_binding(NameBinding {
Expand Down Expand Up @@ -1737,7 +1737,6 @@ impl<'a, 'crateloader: 'a> Resolver<'a, 'crateloader> {
current_type_ascription: Vec::new(),
injected_crate: None,
ignore_extern_prelude_feature: false,
unresolved_invocations_macro_export: FxHashSet(),
}
}

Expand Down Expand Up @@ -4126,6 +4125,7 @@ impl<'a, 'crateloader: 'a> Resolver<'a, 'crateloader> {
ns: Namespace,
module: Module<'a>,
found_traits: &mut Vec<TraitCandidate>) {
assert!(ns == TypeNS || ns == ValueNS);
let mut traits = module.traits.borrow_mut();
if traits.is_none() {
let mut collected_traits = Vec::new();
Expand Down Expand Up @@ -4371,6 +4371,14 @@ impl<'a, 'crateloader: 'a> Resolver<'a, 'crateloader> {
self.report_proc_macro_import(krate);
let mut reported_spans = FxHashSet();

for &(span_use, span_def) in &self.macro_expanded_macro_export_errors {
let msg = "macro-expanded `macro_export` macros from the current crate \
cannot be referred to by absolute paths";
self.session.struct_span_err(span_use, msg)
.span_note(span_def, "the macro is defined here")
.emit();
}

for &AmbiguityError { span, name, b1, b2, lexical } in &self.ambiguity_errors {
if !reported_spans.insert(span) { continue }
let participle = |binding: &NameBinding| {
Expand Down
40 changes: 30 additions & 10 deletions src/librustc_resolve/macros.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ use syntax::ext::expand::{AstFragment, Invocation, InvocationKind};
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};
use syntax::feature_gate::EXPLAIN_DERIVE_UNDERSCORE;
use syntax::fold::{self, Folder};
use syntax::parse::parser::PathStyle;
use syntax::parse::token::{self, Token};
Expand Down Expand Up @@ -195,9 +196,7 @@ impl<'a, 'crateloader: 'a> base::Resolver for Resolver<'a, 'crateloader> {

self.current_module = invocation.module.get();
self.current_module.unresolved_invocations.borrow_mut().remove(&mark);
self.unresolved_invocations_macro_export.remove(&mark);
self.current_module.unresolved_invocations.borrow_mut().extend(derives);
self.unresolved_invocations_macro_export.extend(derives);
for &derive in derives {
self.invocations.insert(derive, invocation);
}
Expand Down Expand Up @@ -338,19 +337,37 @@ impl<'a, 'crateloader: 'a> base::Resolver for Resolver<'a, 'crateloader> {
match attr_kind {
NonMacroAttrKind::Tool | NonMacroAttrKind::DeriveHelper |
NonMacroAttrKind::Custom if is_attr_invoc => {
let features = self.session.features_untracked();
if attr_kind == NonMacroAttrKind::Tool &&
!self.session.features_untracked().tool_attributes {
!features.tool_attributes {
feature_err(&self.session.parse_sess, "tool_attributes",
invoc.span(), GateIssue::Language,
"tool attributes are unstable").emit();
}
if attr_kind == NonMacroAttrKind::Custom &&
!self.session.features_untracked().custom_attribute {
let msg = format!("The attribute `{}` is currently unknown to the compiler \
and may have meaning added to it in the future", path);
feature_err(&self.session.parse_sess, "custom_attribute", invoc.span(),
GateIssue::Language, &msg).emit();
if attr_kind == NonMacroAttrKind::Custom {
assert!(path.segments.len() == 1);
let name = path.segments[0].ident.name.as_str();
if name.starts_with("rustc_") {
if !features.rustc_attrs {
let msg = "unless otherwise specified, attributes with the prefix \
`rustc_` are reserved for internal compiler diagnostics";
feature_err(&self.session.parse_sess, "rustc_attrs", invoc.span(),
GateIssue::Language, &msg).emit();
}
} else if name.starts_with("derive_") {
if !features.custom_derive {
feature_err(&self.session.parse_sess, "custom_derive", invoc.span(),
GateIssue::Language, EXPLAIN_DERIVE_UNDERSCORE).emit();
}
} else if !features.custom_attribute {
let msg = format!("The attribute `{}` is currently unknown to the \
compiler and may have meaning added to it in the \
future", path);
feature_err(&self.session.parse_sess, "custom_attribute", invoc.span(),
GateIssue::Language, &msg).emit();
}
}

return Ok(Some(Lrc::new(SyntaxExtension::NonMacroAttr {
mark_used: attr_kind == NonMacroAttrKind::Tool,
})));
Expand Down Expand Up @@ -650,7 +667,10 @@ impl<'a, 'cl> Resolver<'a, 'cl> {
}
}
WhereToResolve::BuiltinAttrs => {
if is_builtin_attr_name(ident.name) {
// FIXME: Only built-in attributes are not considered as candidates for
// non-attributes to fight off regressions on stable channel (#53205).
// We need to come up with some more principled approach instead.
if is_attr && is_builtin_attr_name(ident.name) {
let binding = (Def::NonMacroAttr(NonMacroAttrKind::Builtin),
ty::Visibility::Public, ident.span, Mark::root())
.to_name_binding(self.arenas);
Expand Down
20 changes: 17 additions & 3 deletions src/librustc_resolve/resolve_imports.rs
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,14 @@ impl<'a, 'crateloader> Resolver<'a, 'crateloader> {
.try_borrow_mut()
.map_err(|_| Determined)?; // This happens when there is a cycle of imports

if let Some(binding) = resolution.binding {
if !restricted_shadowing && binding.expansion != Mark::root() {
if let NameBindingKind::Def(_, true) = binding.kind {
self.macro_expanded_macro_export_errors.insert((path_span, binding.span));
}
}
}

if record_used {
if let Some(binding) = resolution.binding {
if let Some(shadowed_glob) = resolution.shadowed_glob {
Expand Down Expand Up @@ -211,9 +219,15 @@ impl<'a, 'crateloader> Resolver<'a, 'crateloader> {
// if it cannot be shadowed by some new item/import expanded from a macro.
// This happens either if there are no unexpanded macros, or expanded names cannot
// shadow globs (that happens in macro namespace or with restricted shadowing).
let unexpanded_macros = !module.unresolved_invocations.borrow().is_empty() ||
(ns == MacroNS && ptr::eq(module, self.graph_root) &&
!self.unresolved_invocations_macro_export.is_empty());
//
// Additionally, any macro in any module can plant names in the root module if it creates
// `macro_export` macros, so the root module effectively has unresolved invocations if any
// module has unresolved invocations.
// However, it causes resolution/expansion to stuck too often (#53144), so, to make
// progress, we have to ignore those potential unresolved invocations from other modules
// and prohibit access to macro-expanded `macro_export` macros instead (unless restricted
// shadowing is enabled, see `macro_expanded_macro_export_errors`).
let unexpanded_macros = !module.unresolved_invocations.borrow().is_empty();
if let Some(binding) = resolution.binding {
if !unexpanded_macros || ns == MacroNS || restricted_shadowing {
return check_usable(self, binding);
Expand Down
46 changes: 25 additions & 21 deletions src/libsyntax/feature_gate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ macro_rules! declare_features {
self.macros_in_extern || self.proc_macro_path_invoc ||
self.proc_macro_mod || self.proc_macro_expr ||
self.proc_macro_non_items || self.proc_macro_gen ||
self.stmt_expr_attributes
self.stmt_expr_attributes || self.unrestricted_attribute_tokens
}
}
};
Expand Down Expand Up @@ -504,6 +504,9 @@ declare_features! (
// impl<I:Iterator> Iterator for &mut Iterator
// impl Debug for Foo<'_>
(active, impl_header_lifetime_elision, "1.30.0", Some(15872), Some(Edition::Edition2018)),

// Support for arbitrary delimited token streams in non-macro attributes.
(active, unrestricted_attribute_tokens, "1.30.0", Some(44690), None),
);

declare_features! (
Expand Down Expand Up @@ -721,8 +724,7 @@ pub fn is_builtin_attr_name(name: ast::Name) -> bool {
}

pub fn is_builtin_attr(attr: &ast::Attribute) -> bool {
BUILTIN_ATTRIBUTES.iter().any(|&(builtin_name, _, _)| attr.path == builtin_name) ||
attr.name().as_str().starts_with("rustc_")
BUILTIN_ATTRIBUTES.iter().any(|&(builtin_name, _, _)| attr.path == builtin_name)
}

// Attributes that have a special meaning to rustc or rustdoc
Expand Down Expand Up @@ -1521,25 +1523,27 @@ impl<'a> Visitor<'a> for PostExpansionVisitor<'a> {
}
}

// allow attr_literals in #[repr(align(x))] and #[repr(packed(n))]
let mut allow_attr_literal = false;
if attr.path == "repr" {
if let Some(content) = attr.meta_item_list() {
allow_attr_literal = content.iter().any(
|c| c.check_name("align") || c.check_name("packed"));
}
}

if self.context.features.use_extern_macros() && attr::is_known(attr) {
return
}
match attr.parse_meta(self.context.parse_sess) {
Ok(meta) => {
// allow attr_literals in #[repr(align(x))] and #[repr(packed(n))]
let mut allow_attr_literal = false;
if attr.path == "repr" {
if let Some(content) = meta.meta_item_list() {
allow_attr_literal = content.iter().any(
|c| c.check_name("align") || c.check_name("packed"));
}
}

if !allow_attr_literal {
let meta = panictry!(attr.parse_meta(self.context.parse_sess));
if contains_novel_literal(&meta) {
gate_feature_post!(&self, attr_literals, attr.span,
"non-string literals in attributes, or string \
literals in top-level positions, are experimental");
if !allow_attr_literal && contains_novel_literal(&meta) {
gate_feature_post!(&self, attr_literals, attr.span,
"non-string literals in attributes, or string \
literals in top-level positions, are experimental");
}
}
Err(mut err) => {
err.cancel();
gate_feature_post!(&self, unrestricted_attribute_tokens, attr.span,
"arbitrary tokens in non-macro attributes are unstable");
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ extern crate derive_b;
#[C] //~ ERROR: The attribute `C` is currently unknown to the compiler
#[B(D)]
#[B(E = "foo")]
#[B arbitrary tokens] //~ expected one of `(` or `=`, found `arbitrary`
#[B arbitrary tokens] //~ ERROR arbitrary tokens in non-macro attributes are unstable
struct B;

fn main() {}
32 changes: 14 additions & 18 deletions src/test/compile-fail/gated-attr-literals.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,37 +11,33 @@
// Check that literals in attributes don't parse without the feature gate.

// gate-test-attr_literals
// gate-test-custom_attribute

#![feature(rustc_attrs)]
#![allow(dead_code)]
#![allow(unused_variables)]
#![feature(custom_attribute)]

#[fake_attr] //~ ERROR attribute `fake_attr` is currently unknown
#[fake_attr(100)] //~ ERROR attribute `fake_attr` is currently unknown
#[fake_attr] // OK
#[fake_attr(100)]
//~^ ERROR non-string literals in attributes
#[fake_attr(1, 2, 3)] //~ ERROR attribute `fake_attr` is currently unknown
#[fake_attr(1, 2, 3)]
//~^ ERROR non-string literals in attributes
#[fake_attr("hello")] //~ ERROR attribute `fake_attr` is currently unknown
#[fake_attr("hello")]
//~^ ERROR string literals in top-level positions, are experimental
#[fake_attr(name = "hello")] //~ ERROR attribute `fake_attr` is currently unknown
#[fake_attr(1, "hi", key = 12, true, false)] //~ ERROR attribute `fake_attr` is currently unknown
#[fake_attr(name = "hello")] // OK
#[fake_attr(1, "hi", key = 12, true, false)]
//~^ ERROR non-string literals in attributes, or string literals in top-level positions
#[fake_attr(key = "hello", val = 10)] //~ ERROR attribute `fake_attr` is currently unknown
#[fake_attr(key = "hello", val = 10)]
//~^ ERROR non-string literals in attributes
#[fake_attr(key("hello"), val(10))] //~ ERROR attribute `fake_attr` is currently unknown
#[fake_attr(key("hello"), val(10))]
//~^ ERROR non-string literals in attributes, or string literals in top-level positions
#[fake_attr(enabled = true, disabled = false)] //~ ERROR attribute `fake_attr` is currently unknown
#[fake_attr(enabled = true, disabled = false)]
//~^ ERROR non-string literals in attributes
#[fake_attr(true)] //~ ERROR attribute `fake_attr` is currently unknown
#[fake_attr(true)]
//~^ ERROR non-string literals in attributes
#[fake_attr(pi = 3.14159)] //~ ERROR attribute `fake_attr` is currently unknown
#[fake_attr(pi = 3.14159)]
//~^ ERROR non-string literals in attributes
#[fake_attr(b"hi")] //~ ERROR attribute `fake_attr` is currently unknown
#[fake_attr(b"hi")]
//~^ ERROR string literals in top-level positions, are experimental
#[fake_doc(r"doc")] //~ ERROR attribute `fake_doc` is currently unknown
#[fake_doc(r"doc")]
//~^ ERROR string literals in top-level positions, are experimental
struct Q { }

#[rustc_error]
fn main() { }
2 changes: 1 addition & 1 deletion src/test/compile-fail/macro-attribute.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,5 +8,5 @@
// option. This file may not be copied, modified, or distributed
// except according to those terms.

#[doc = $not_there] //~ error: unexpected token: `$`
#[doc = $not_there] //~ ERROR arbitrary tokens in non-macro attributes are unstable
fn main() { }
2 changes: 1 addition & 1 deletion src/test/parse-fail/attr-bad-meta.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,5 +9,5 @@
// except according to those terms.

// asterisk is bogus
#[path*] //~ ERROR expected one of `(` or `=`
#[path*] //~ ERROR arbitrary tokens in non-macro attributes are unstable
mod m {}
2 changes: 1 addition & 1 deletion src/test/run-pass-fulldeps/proc-macro/derive-b.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
// aux-build:derive-b.rs
// ignore-stage1

#![feature(proc_macro_path_invoc)]
#![feature(proc_macro_path_invoc, unrestricted_attribute_tokens)]

extern crate derive_b;

Expand Down
18 changes: 18 additions & 0 deletions src/test/ui/feature-gate-rustc-attrs-1.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
// 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.

// ignore-tidy-linelength

// Test that `#[rustc_*]` attributes are gated by `rustc_attrs` feature gate.

#[rustc_variance] //~ ERROR the `#[rustc_variance]` attribute is just used for rustc unit tests and will never be stable
#[rustc_error] //~ ERROR the `#[rustc_error]` attribute is just used for rustc unit tests and will never be stable

fn main() {}
19 changes: 19 additions & 0 deletions src/test/ui/feature-gate-rustc-attrs-1.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
error[E0658]: the `#[rustc_variance]` attribute is just used for rustc unit tests and will never be stable (see issue #29642)
--> $DIR/feature-gate-rustc-attrs-1.rs:15:1
|
LL | #[rustc_variance] //~ ERROR the `#[rustc_variance]` attribute is just used for rustc unit tests and will never be stable
| ^^^^^^^^^^^^^^^^^
|
= help: add #![feature(rustc_attrs)] to the crate attributes to enable

error[E0658]: the `#[rustc_error]` attribute is just used for rustc unit tests and will never be stable (see issue #29642)
--> $DIR/feature-gate-rustc-attrs-1.rs:16:1
|
LL | #[rustc_error] //~ ERROR the `#[rustc_error]` attribute is just used for rustc unit tests and will never be stable
| ^^^^^^^^^^^^^^
|
= help: add #![feature(rustc_attrs)] to the crate attributes to enable

error: aborting due to 2 previous errors

For more information about this error, try `rustc --explain E0658`.
2 changes: 0 additions & 2 deletions src/test/ui/feature-gate-rustc-attrs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,6 @@

// Test that `#[rustc_*]` attributes are gated by `rustc_attrs` feature gate.

#[rustc_variance] //~ ERROR the `#[rustc_variance]` attribute is just used for rustc unit tests and will never be stable
#[rustc_error] //~ ERROR the `#[rustc_error]` attribute is just used for rustc unit tests and will never be stable
#[rustc_foo]
//~^ ERROR unless otherwise specified, attributes with the prefix `rustc_` are reserved for internal compiler diagnostics

Expand Down
Loading

0 comments on commit d5a448b

Please sign in to comment.