Audit uses of apply_mark in built-in macros + Remove default macro transparencies
#63823
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Replace them with equivalents of `Span::{def_site,call_site}` from proc macro API.
The new API is much less error prone and doesn't rely on macros having default transparency.
… contexts Using `ExpnId`s default transparency here instead of the mark's real transparency was actually incorrect.
All transparancies are passed explicitly now. Also remove `#[rustc_macro_transparency]` annotations from built-in macros, they are no longer used. `#[rustc_macro_transparency]` only makes sense for declarative macros now.
rust-highfive
added
the
S-waiting-on-review
eddyb
approved these changes
Aug 23, 2019
| @@ -575,9 +550,15 @@ impl Span { | |||
| /// The returned span belongs to the created expansion and has the new properties, | |||
| /// but its location is inherited from the current span. | |||
| pub fn fresh_expansion(self, expn_data: ExpnData) -> Span { | |||
| self.fresh_expansion_with_transparency(expn_data, Transparency::SemiTransparent) | |||
There was a problem hiding this comment.
Does Transparency::Transparent makes more sense here? This expansion isn't for a macro so it can't be used for hygienic resolution anyway.
There was a problem hiding this comment.
Some of the things using fresh_expansion currently use gensyms too, so it may be better to organize opaque expansions for them.
Transparency::SemiTransparent is used here just to keep the pre-existing behavior for now.
|
@bors r+ |
|
📌 Commit 6548a5f has been approved by |
bors
added
S-waiting-on-bors
bors
added a commit
that referenced
this pull request
Aug 24, 2019
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
|
☀️ Test successful - checks-azure |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Every use of
apply_markin a built-in or procedural macro is supposed to look like thiswhere
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_markdoes, so we had a lot of copy-pasted uses of it looking e.g. like, 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_ruleshygiene.So, to fight this, we stop using
apply_markdirectly in built-in macro implementations, and follow the example of regular proc macros instead and use analogues ofSpan::def_site()andSpan::call_site(), which are much more intuitive and less error-prone.ecx.with_def_site_ctxt(span)takes thespan's location and combines it with a def-site context.ecx.with_call_site_ctxt(span)takes thespan'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_markwhich will grow the mark chain further in this case.After
apply_marks in built-in macros are eliminated, the remainingapply_marks are very few in number, so we can start passing the previously implicitTransparencyargument to them explicitly, thus eliminating the need indefault_transparencyfields 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_ctxtin favor ofwith_def_site_ctxt" rather than "replace#[rustc_macro_transparency = "semitransparent"]with#[rustc_macro_transparency = "opaque"]".r? @matthewjasper