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

Audit uses of `apply_mark` in built-in macros + Remove default macro transparencies #63823

Merged
merged 5 commits into from Aug 24, 2019

Conversation

@petrochenkov
Copy link
Contributor

commented Aug 22, 2019

Every use of apply_mark in a built-in or procedural macro is supposed to look like this

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

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_marks in built-in macros are eliminated, the remaining apply_marks 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

Audit uses of `apply_mark` in built-in macros
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.
incremental: Do not rely on default transparency when decoding syntax…
… contexts

Using `ExpnId`s default transparency here instead of the mark's real transparency was actually incorrect.
Remove default macro transparencies
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.
@eddyb
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)

This comment has been minimized.

Copy link
@matthewjasper

matthewjasper Aug 24, 2019

Contributor

Does Transparency::Transparent makes more sense here? This expansion isn't for a macro so it can't be used for hygienic resolution anyway.

This comment has been minimized.

Copy link
@petrochenkov

petrochenkov Aug 24, 2019

Author Contributor

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.

@matthewjasper

This comment has been minimized.

Copy link
Contributor

commented Aug 24, 2019

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

commented Aug 24, 2019

📌 Commit 6548a5f has been approved by matthewjasper

@bors

This comment has been minimized.

Copy link
Contributor

commented Aug 24, 2019

⌛️ Testing commit 6548a5f with merge 5ade61a...

bors added a commit that referenced this pull request Aug 24, 2019
Auto merge of #63823 - petrochenkov:noapply2, r=matthewjasper
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
@bors

This comment has been minimized.

Copy link
Contributor

commented Aug 24, 2019

☀️ Test successful - checks-azure
Approved by: matthewjasper
Pushing 5ade61a to master...

@bors bors added the merged-by-bors label Aug 24, 2019

@bors bors merged commit 6548a5f into rust-lang:master Aug 24, 2019

5 checks passed

homu Test successful
Details
pr Build #20190822.32 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-6.0) Linux x86_64-gnu-llvm-6.0 succeeded
Details
pr (LinuxTools) LinuxTools succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.