Skip to content

Commit

Permalink
Auto merge of #63823 - petrochenkov:noapply2, r=matthewjasper
Browse files Browse the repository at this point in the history
Audit uses of `apply_mark` in built-in macros + Remove default macro transparencies

Every use of `apply_mark` in a built-in or procedural macro is supposed to look like this
```rust
location.with_ctxt(SyntaxContext::root().apply_mark(ecx.current_expansion.id))
```
where `SyntaxContext::root()` means that the built-in/procedural macro is defined directly, rather than expanded from some other macro.

However, few people understood what `apply_mark` does, so we had a lot of copy-pasted uses of it looking e.g. like
```rust
span = span.apply_mark(ecx.current_expansion.id);
```
, which doesn't really make sense for procedural macros, but at the same time is not too harmful, if the macros use the traditional `macro_rules` hygiene.

So, to fight this, we stop using `apply_mark` directly in built-in macro implementations, and follow the example of regular proc macros instead and use analogues of `Span::def_site()` and `Span::call_site()`, which are much more intuitive and less error-prone.
- `ecx.with_def_site_ctxt(span)` takes the `span`'s location and combines it with a def-site context.
- `ecx.with_call_site_ctxt(span)` takes the `span`'s location and combines it with a call-site context.

Even if called multiple times (which sometimes happens due to some historical messiness of the built-in macro code) these functions will produce the same result, unlike `apply_mark` which will grow  the mark chain further in this case.

---

After `apply_mark`s in built-in macros are eliminated, the remaining `apply_mark`s are very few in number, so we can start passing the previously implicit `Transparency` argument to them explicitly, thus eliminating the need in `default_transparency` fields in hygiene structures and `#[rustc_macro_transparency]` annotations on built-in macros.

So, the task of making built-in macros opaque can now be formulated as "eliminate `with_legacy_ctxt` in favor of `with_def_site_ctxt`" rather than "replace `#[rustc_macro_transparency = "semitransparent"]` with `#[rustc_macro_transparency = "opaque"]`".

r? @matthewjasper
  • Loading branch information
bors committed Aug 24, 2019
2 parents 4784645 + 6548a5f commit 5ade61a
Show file tree
Hide file tree
Showing 28 changed files with 152 additions and 179 deletions.
8 changes: 0 additions & 8 deletions src/libcore/macros.rs
Original file line number Diff line number Diff line change
Expand Up @@ -734,7 +734,6 @@ pub(crate) mod builtin {
#[allow_internal_unstable(fmt_internals)]
#[rustc_builtin_macro]
#[macro_export]
#[rustc_macro_transparency = "opaque"]
macro_rules! format_args {
($fmt:expr) => ({ /* compiler built-in */ });
($fmt:expr, $($args:tt)*) => ({ /* compiler built-in */ })
Expand All @@ -747,7 +746,6 @@ pub(crate) mod builtin {
#[allow_internal_unstable(fmt_internals)]
#[rustc_builtin_macro]
#[macro_export]
#[rustc_macro_transparency = "opaque"]
macro_rules! format_args_nl {
($fmt:expr) => ({ /* compiler built-in */ });
($fmt:expr, $($args:tt)*) => ({ /* compiler built-in */ })
Expand Down Expand Up @@ -1235,42 +1233,36 @@ pub(crate) mod builtin {
#[stable(feature = "rust1", since = "1.0.0")]
#[allow_internal_unstable(test, rustc_attrs)]
#[rustc_builtin_macro]
#[rustc_macro_transparency = "semitransparent"]
pub macro test($item:item) { /* compiler built-in */ }

/// Attribute macro applied to a function to turn it into a benchmark test.
#[unstable(feature = "test", issue = "50297",
reason = "`bench` is a part of custom test frameworks which are unstable")]
#[allow_internal_unstable(test, rustc_attrs)]
#[rustc_builtin_macro]
#[rustc_macro_transparency = "semitransparent"]
pub macro bench($item:item) { /* compiler built-in */ }

/// An implementation detail of the `#[test]` and `#[bench]` macros.
#[unstable(feature = "custom_test_frameworks", issue = "50297",
reason = "custom test frameworks are an unstable feature")]
#[allow_internal_unstable(test, rustc_attrs)]
#[rustc_builtin_macro]
#[rustc_macro_transparency = "semitransparent"]
pub macro test_case($item:item) { /* compiler built-in */ }

/// Attribute macro applied to a static to register it as a global allocator.
#[stable(feature = "global_allocator", since = "1.28.0")]
#[allow_internal_unstable(rustc_attrs)]
#[rustc_builtin_macro]
#[rustc_macro_transparency = "semitransparent"]
pub macro global_allocator($item:item) { /* compiler built-in */ }

/// Unstable implementation detail of the `rustc` compiler, do not use.
#[rustc_builtin_macro]
#[cfg_attr(boostrap_stdarch_ignore_this, rustc_macro_transparency = "semitransparent")]
#[stable(feature = "rust1", since = "1.0.0")]
#[allow_internal_unstable(core_intrinsics, libstd_sys_internals)]
pub macro RustcDecodable($item:item) { /* compiler built-in */ }

/// Unstable implementation detail of the `rustc` compiler, do not use.
#[rustc_builtin_macro]
#[cfg_attr(boostrap_stdarch_ignore_this, rustc_macro_transparency = "semitransparent")]
#[stable(feature = "rust1", since = "1.0.0")]
#[allow_internal_unstable(core_intrinsics)]
pub macro RustcEncodable($item:item) { /* compiler built-in */ }
Expand Down
1 change: 0 additions & 1 deletion src/librustc/ich/impls_syntax.rs
Original file line number Diff line number Diff line change
Expand Up @@ -402,7 +402,6 @@ impl_stable_hash_for!(struct ::syntax_pos::hygiene::ExpnData {
parent -> _,
call_site,
def_site,
default_transparency,
allow_internal_unstable,
allow_internal_unsafe,
local_inner_macros,
Expand Down
20 changes: 10 additions & 10 deletions src/librustc/ty/query/on_disk_cache.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ use std::mem;
use syntax::ast::NodeId;
use syntax::source_map::{SourceMap, StableSourceFileId};
use syntax_pos::{BytePos, Span, DUMMY_SP, SourceFile};
use syntax_pos::hygiene::{ExpnId, SyntaxContext, ExpnData};
use syntax_pos::hygiene::{ExpnId, SyntaxContext};

const TAG_FILE_FOOTER: u128 = 0xC0FFEE_C0FFEE_C0FFEE_C0FFEE_C0FFEE;

Expand Down Expand Up @@ -593,8 +593,8 @@ impl<'a, 'tcx> SpecializedDecoder<Span> for CacheDecoder<'a, 'tcx> {
// don't seem to be used after HIR lowering, so everything should be fine
// as long as incremental compilation does not kick in before that.
let location = || Span::with_root_ctxt(lo, hi);
let recover_from_expn_data = |this: &Self, expn_data, pos| {
let span = location().fresh_expansion(expn_data);
let recover_from_expn_data = |this: &Self, expn_data, transparency, pos| {
let span = location().fresh_expansion_with_transparency(expn_data, transparency);
this.synthetic_syntax_contexts.borrow_mut().insert(pos, span.ctxt());
span
};
Expand All @@ -603,9 +603,9 @@ impl<'a, 'tcx> SpecializedDecoder<Span> for CacheDecoder<'a, 'tcx> {
location()
}
TAG_EXPN_DATA_INLINE => {
let expn_data = Decodable::decode(self)?;
let (expn_data, transparency) = Decodable::decode(self)?;
recover_from_expn_data(
self, expn_data, AbsoluteBytePos::new(self.opaque.position())
self, expn_data, transparency, AbsoluteBytePos::new(self.opaque.position())
)
}
TAG_EXPN_DATA_SHORTHAND => {
Expand All @@ -614,9 +614,9 @@ impl<'a, 'tcx> SpecializedDecoder<Span> for CacheDecoder<'a, 'tcx> {
if let Some(ctxt) = cached_ctxt {
Span::new(lo, hi, ctxt)
} else {
let expn_data =
self.with_position(pos.to_usize(), |this| ExpnData::decode(this))?;
recover_from_expn_data(self, expn_data, pos)
let (expn_data, transparency) =
self.with_position(pos.to_usize(), |this| Decodable::decode(this))?;
recover_from_expn_data(self, expn_data, transparency, pos)
}
}
_ => {
Expand Down Expand Up @@ -819,15 +819,15 @@ where
if span_data.ctxt == SyntaxContext::root() {
TAG_NO_EXPN_DATA.encode(self)
} else {
let (expn_id, expn_data) = span_data.ctxt.outer_expn_with_data();
let (expn_id, transparency, expn_data) = span_data.ctxt.outer_mark_with_data();
if let Some(pos) = self.expn_data_shorthands.get(&expn_id).cloned() {
TAG_EXPN_DATA_SHORTHAND.encode(self)?;
pos.encode(self)
} else {
TAG_EXPN_DATA_INLINE.encode(self)?;
let pos = AbsoluteBytePos::new(self.position());
self.expn_data_shorthands.insert(expn_id, pos);
expn_data.encode(self)
(expn_data, transparency).encode(self)
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/librustc_resolve/build_reduced_graph.rs
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ impl<'a> Resolver<'a> {
}
}

fn get_macro_by_def_id(&mut self, def_id: DefId) -> Option<Lrc<SyntaxExtension>> {
crate fn get_macro_by_def_id(&mut self, def_id: DefId) -> Option<Lrc<SyntaxExtension>> {
if let Some(ext) = self.macro_map.get(&def_id) {
return Some(ext.clone());
}
Expand Down
12 changes: 8 additions & 4 deletions src/librustc_resolve/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1647,10 +1647,14 @@ impl<'a> Resolver<'a> {
if module.expansion != parent.expansion &&
module.expansion.is_descendant_of(parent.expansion) {
// The macro is a proc macro derive
if module.expansion.looks_like_proc_macro_derive() {
if parent.expansion.outer_expn_is_descendant_of(span.ctxt()) {
*poisoned = Some(node_id);
return module.parent;
if let Some(&def_id) = self.macro_defs.get(&module.expansion) {
if let Some(ext) = self.get_macro_by_def_id(def_id) {
if !ext.is_builtin && ext.macro_kind() == MacroKind::Derive {
if parent.expansion.outer_expn_is_descendant_of(span.ctxt()) {
*poisoned = Some(node_id);
return module.parent;
}
}
}
}
}
Expand Down
50 changes: 23 additions & 27 deletions src/libsyntax/ext/base.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ use crate::attr::{HasAttrs, Stability, Deprecation};
use crate::source_map::SourceMap;
use crate::edition::Edition;
use crate::ext::expand::{self, AstFragment, Invocation};
use crate::ext::hygiene::{ExpnId, SyntaxContext, Transparency};
use crate::ext::hygiene::{ExpnId, Transparency};
use crate::mut_visit::{self, MutVisitor};
use crate::parse::{self, parser, DirectoryOwnership};
use crate::parse::token;
Expand Down Expand Up @@ -549,8 +549,6 @@ pub struct SyntaxExtension {
pub kind: SyntaxExtensionKind,
/// Span of the macro definition.
pub span: Span,
/// Hygienic properties of spans produced by this macro by default.
pub default_transparency: Transparency,
/// Whitelist of unstable features that are treated as stable inside this macro.
pub allow_internal_unstable: Option<Lrc<[Symbol]>>,
/// Suppresses the `unsafe_code` lint for code produced by this macro.
Expand All @@ -572,22 +570,6 @@ pub struct SyntaxExtension {
pub is_derive_copy: bool,
}

impl SyntaxExtensionKind {
/// When a syntax extension is constructed,
/// its transparency can often be inferred from its kind.
fn default_transparency(&self) -> Transparency {
match self {
SyntaxExtensionKind::Bang(..) |
SyntaxExtensionKind::Attr(..) |
SyntaxExtensionKind::Derive(..) |
SyntaxExtensionKind::NonMacroAttr { .. } => Transparency::Opaque,
SyntaxExtensionKind::LegacyBang(..) |
SyntaxExtensionKind::LegacyAttr(..) |
SyntaxExtensionKind::LegacyDerive(..) => Transparency::SemiTransparent,
}
}
}

impl SyntaxExtension {
/// Returns which kind of macro calls this syntax extension.
pub fn macro_kind(&self) -> MacroKind {
Expand All @@ -606,7 +588,6 @@ impl SyntaxExtension {
pub fn default(kind: SyntaxExtensionKind, edition: Edition) -> SyntaxExtension {
SyntaxExtension {
span: DUMMY_SP,
default_transparency: kind.default_transparency(),
allow_internal_unstable: None,
allow_internal_unsafe: false,
local_inner_macros: false,
Expand Down Expand Up @@ -646,7 +627,6 @@ impl SyntaxExtension {
parent,
call_site,
def_site: self.span,
default_transparency: self.default_transparency,
allow_internal_unstable: self.allow_internal_unstable.clone(),
allow_internal_unsafe: self.allow_internal_unsafe,
local_inner_macros: self.local_inner_macros,
Expand Down Expand Up @@ -760,23 +740,39 @@ impl<'a> ExtCtxt<'a> {
pub fn call_site(&self) -> Span {
self.current_expansion.id.expn_data().call_site
}
pub fn backtrace(&self) -> SyntaxContext {
SyntaxContext::root().apply_mark(self.current_expansion.id)

/// Equivalent of `Span::def_site` from the proc macro API,
/// except that the location is taken from the span passed as an argument.
pub fn with_def_site_ctxt(&self, span: Span) -> Span {
span.with_ctxt_from_mark(self.current_expansion.id, Transparency::Opaque)
}

/// Equivalent of `Span::call_site` from the proc macro API,
/// except that the location is taken from the span passed as an argument.
pub fn with_call_site_ctxt(&self, span: Span) -> Span {
span.with_ctxt_from_mark(self.current_expansion.id, Transparency::Transparent)
}

/// Span with a context reproducing `macro_rules` hygiene (hygienic locals, unhygienic items).
/// FIXME: This should be eventually replaced either with `with_def_site_ctxt` (preferably),
/// or with `with_call_site_ctxt` (where necessary).
pub fn with_legacy_ctxt(&self, span: Span) -> Span {
span.with_ctxt_from_mark(self.current_expansion.id, Transparency::SemiTransparent)
}

/// Returns span for the macro which originally caused the current expansion to happen.
///
/// Stops backtracing at include! boundary.
pub fn expansion_cause(&self) -> Option<Span> {
let mut ctxt = self.backtrace();
let mut expn_id = self.current_expansion.id;
let mut last_macro = None;
loop {
let expn_data = ctxt.outer_expn_data();
let expn_data = expn_id.expn_data();
// Stop going up the backtrace once include! is encountered
if expn_data.is_root() || expn_data.kind.descr() == sym::include {
break;
}
ctxt = expn_data.call_site.ctxt();
expn_id = expn_data.call_site.ctxt().outer_expn();
last_macro = Some(expn_data.call_site);
}
last_macro
Expand Down Expand Up @@ -865,7 +861,7 @@ impl<'a> ExtCtxt<'a> {
ast::Ident::from_str(st)
}
pub fn std_path(&self, components: &[Symbol]) -> Vec<ast::Ident> {
let def_site = DUMMY_SP.apply_mark(self.current_expansion.id);
let def_site = self.with_def_site_ctxt(DUMMY_SP);
iter::once(Ident::new(kw::DollarCrate, def_site))
.chain(components.iter().map(|&s| Ident::with_dummy_span(s)))
.collect()
Expand Down
15 changes: 0 additions & 15 deletions src/libsyntax/ext/expand.rs
Original file line number Diff line number Diff line change
Expand Up @@ -565,7 +565,6 @@ impl<'a, 'b> MacroExpander<'a, 'b> {
return fragment_kind.dummy(span);
}
let meta = ast::MetaItem { node: ast::MetaItemKind::Word, span, path };
let span = span.with_ctxt(self.cx.backtrace());
let items = expander.expand(self.cx, span, &meta, item);
fragment_kind.expect_from_annotatables(items)
}
Expand Down Expand Up @@ -1389,17 +1388,3 @@ impl<'feat> ExpansionConfig<'feat> {
self.features.map_or(false, |features| features.custom_inner_attributes)
}
}

// A Marker adds the given mark to the syntax context.
#[derive(Debug)]
pub struct Marker(pub ExpnId);

impl MutVisitor for Marker {
fn visit_span(&mut self, span: &mut Span) {
*span = span.apply_mark(self.0)
}

fn visit_mac(&mut self, mac: &mut ast::Mac) {
noop_visit_mac(mac, self)
}
}
11 changes: 2 additions & 9 deletions src/libsyntax/ext/proc_macro_server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ use crate::tokenstream::{self, DelimSpan, IsJoint::*, TokenStream, TreeAndJoint}
use errors::{Diagnostic, DiagnosticBuilder};
use rustc_data_structures::sync::Lrc;
use syntax_pos::{BytePos, FileName, MultiSpan, Pos, SourceFile, Span};
use syntax_pos::hygiene::{SyntaxContext, Transparency};
use syntax_pos::symbol::{kw, sym, Symbol};

use proc_macro::{Delimiter, Level, LineColumn, Spacing};
Expand Down Expand Up @@ -363,16 +362,10 @@ impl<'a> Rustc<'a> {
pub fn new(cx: &'a ExtCtxt<'_>) -> Self {
// No way to determine def location for a proc macro right now, so use call location.
let location = cx.current_expansion.id.expn_data().call_site;
let to_span = |transparency| {
location.with_ctxt(
SyntaxContext::root()
.apply_mark_with_transparency(cx.current_expansion.id, transparency),
)
};
Rustc {
sess: cx.parse_sess,
def_site: to_span(Transparency::Opaque),
call_site: to_span(Transparency::Transparent),
def_site: cx.with_def_site_ctxt(location),
call_site: cx.with_call_site_ctxt(location),
}
}

Expand Down
20 changes: 12 additions & 8 deletions src/libsyntax/ext/tt/macro_rules.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ use crate::{ast, attr, attr::TransparencyError};

use errors::{DiagnosticBuilder, FatalError};
use log::debug;
use syntax_pos::hygiene::Transparency;
use syntax_pos::Span;

use rustc_data_structures::fx::FxHashMap;
Expand Down Expand Up @@ -128,6 +129,7 @@ impl<'a> ParserAnyMacro<'a> {
struct MacroRulesMacroExpander {
name: ast::Ident,
span: Span,
transparency: Transparency,
lhses: Vec<quoted::TokenTree>,
rhses: Vec<quoted::TokenTree>,
valid: bool,
Expand All @@ -143,7 +145,9 @@ impl TTMacroExpander for MacroRulesMacroExpander {
if !self.valid {
return DummyResult::any(sp);
}
generic_extension(cx, sp, self.span, self.name, input, &self.lhses, &self.rhses)
generic_extension(
cx, sp, self.span, self.name, self.transparency, input, &self.lhses, &self.rhses
)
}
}

Expand All @@ -158,6 +162,7 @@ fn generic_extension<'cx>(
sp: Span,
def_span: Span,
name: ast::Ident,
transparency: Transparency,
arg: TokenStream,
lhses: &[quoted::TokenTree],
rhses: &[quoted::TokenTree],
Expand Down Expand Up @@ -187,7 +192,7 @@ fn generic_extension<'cx>(

let rhs_spans = rhs.iter().map(|t| t.span()).collect::<Vec<_>>();
// rhs has holes ( `$id` and `$(...)` that need filled)
let mut tts = transcribe(cx, &named_matches, rhs);
let mut tts = transcribe(cx, &named_matches, rhs, transparency);

// Replace all the tokens for the corresponding positions in the macro, to maintain
// proper positions in error reporting, while maintaining the macro_backtrace.
Expand Down Expand Up @@ -415,11 +420,7 @@ pub fn compile(
// that is not lint-checked and trigger the "failed to process buffered lint here" bug.
valid &= macro_check::check_meta_variables(sess, ast::CRATE_NODE_ID, def.span, &lhses, &rhses);

let expander: Box<_> =
Box::new(MacroRulesMacroExpander { name: def.ident, span: def.span, lhses, rhses, valid });

let (default_transparency, transparency_error) =
attr::find_transparency(&def.attrs, body.legacy);
let (transparency, transparency_error) = attr::find_transparency(&def.attrs, body.legacy);
match transparency_error {
Some(TransparencyError::UnknownTransparency(value, span)) =>
sess.span_diagnostic.span_err(
Expand All @@ -432,6 +433,10 @@ pub fn compile(
None => {}
}

let expander: Box<_> = Box::new(MacroRulesMacroExpander {
name: def.ident, span: def.span, transparency, lhses, rhses, valid
});

let allow_internal_unstable =
attr::find_by_name(&def.attrs, sym::allow_internal_unstable).map(|attr| {
attr.meta_item_list()
Expand Down Expand Up @@ -473,7 +478,6 @@ pub fn compile(
SyntaxExtension {
kind: SyntaxExtensionKind::LegacyBang(expander),
span: def.span,
default_transparency,
allow_internal_unstable,
allow_internal_unsafe: attr::contains_name(&def.attrs, sym::allow_internal_unsafe),
local_inner_macros,
Expand Down
Loading

0 comments on commit 5ade61a

Please sign in to comment.