resolve: Future proof resolutions for potentially built-in attributes #53913
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
|
r? @estebank (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
This needs a check-only crater run before proceeding. |
bors
added a commit
that referenced
this pull request
Sep 3, 2018
resolve: Future proof resolutions for potentially built-in attributes Based on #53778 This is not full "pass all attributes through name resolution", but a more conservative solution. If built-in attribute is ambiguous with any other macro in scope, then an error is reported. TODO: Explain what complications arise with the full solution.
petrochenkov
added
S-waiting-on-crater
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
💔 Test failed - status-travis |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
bors
added
S-waiting-on-author
petrochenkov
force-pushed
the
biattr4
branch
from
bc49117
to
ebb7a7b
Compare
|
@bors try |
bors
added a commit
that referenced
this pull request
Sep 3, 2018
resolve: Future proof resolutions for potentially built-in attributes Based on #53778 This is not full "pass all attributes through name resolution", but a more conservative solution. If built-in attribute is ambiguous with any other macro in scope, then an error is reported. TODO: Explain what complications arise with the full solution. cc #50911 (comment) Closes #53531
|
☀️ Test successful - status-travis |
|
@craterbot run start=master#2687112ea6a8701cbf36e6dd4d77d64694cf93d8 end=try#5fe695a3844714c70e8f7731a553f7947987a68a mode=check-only |
|
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
craterbot
added
S-waiting-on-crater
|
🚧 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
Why passing all attributes through name resolution / expansion is not so simple.It's actually easy to pass all attributes through name resolution / expansion, it's much harder to fight the consequences. If some attribute may be a macro and goes through the process of determining whether it's a macro or not, then... it's target becomes potentially macro-expanded (e.g. coming from an expansion). #[inline]
fn f() {}
For most items it doesn't pose any problems. Actually, most of structs/enums are already macro-expanded due to Now imagine if built-in attributes on macros start going through resolution/expansion. So, even if attributes on non-macros/modules/imports go through usual macro expansion, attributes on macros/modules/imports still need immediate built-in attribute resolution and, as a consequence, stricter ambiguity checking - exactly what this PR does. Other uncovered issues with the full solution (I also didn't want to deal with all them in the limited time until beta):
|
|
This sounds like a solid approach to me and the tests look amazingly comprehensive, thanks so much for taking this on @petrochenkov! I think I'm a bit out of my depth reviewing the internals of this PR, but with a "green enough" crater run I'd be comfortable r+'ing |
|
🎉 Experiment
|
craterbot
added
S-waiting-on-review
This was referenced Sep 7, 2018
Invocation/expansion ID (aka `Mark`) is not really necessary for resolving a macro path. What is really necessary is its parent module, parent expansion and parent legacy scope. This is required for validation resolutions of built-in attributes, which don't get their own `Mark`s
petrochenkov
force-pushed
the
biattr4
branch
from
ebb7a7b
to
de153d6
Compare
|
@alexcrichton |
petrochenkov
added
S-waiting-on-review
|
@bors: r+ |
petrochenkov
added
S-waiting-on-bors
|
📌 Commit de153d6 has been approved by |
bors
added a commit
that referenced
this pull request
Sep 11, 2018
resolve: Future proof resolutions for potentially built-in attributes This is not full "pass all attributes through name resolution", but a more conservative solution. If built-in attribute is ambiguous with any other macro in scope, then an error is reported. What complications arise with the full solution - #53913 (comment). cc #50911 (comment) cc #52269 Closes #53531
|
☀️ Test successful - status-appveyor, status-travis |
bors
added a commit
that referenced
this pull request
Sep 14, 2018
resolve: Introduce two sub-namespaces in macro namespace
Two sub-namespaces are introduced in the macro namespace - one for bang macros and one for attribute-like macros (attributes, derives).
"Sub-namespace" means this is not a newly introduced full namespace, the single macro namespace is still in place.
I.e. you still can't define/import two macros with the same name in a single module, `use` imports still import only one name in macro namespace (from any sub-namespace) and not possibly two.
However, when we are searching for a name used in a `!` macro call context (`my_macro!()`) we skip attribute names in scope, and when we are searching for a name used in attribute context (`#[my_macro]`/`#[derive(my_macro)]`) we are skipping bang macro names in scope.
In other words, bang macros cannot shadow attribute macros and vice versa.
For a non-macro analogy, we could e.g. skip non-traits when searching for `MyTrait` in `impl MyTrait for Type { ... }`.
However we do not do it in non-macro namespaces because we don't have practical issues with e.g. non-traits shadowing traits with the same name, but with macros we do, especially after macro modularization.
For `#[test]` and `#[bench]` we have a hack in the compiler right now preventing their shadowing by `macro_rules! test` and similar things. This hack was introduced after making `#[test]`/`#[bench]` built-in macros instead of built-in attributes (#53410), something that needed to be done from the start since they are "active" attributes transforming their inputs.
Now they are passed through normal name resolution and can be shadowed, but that's a breaking change, so we have a special hack basically applying this PR for `#[test]` and `#[bench]` only.
Soon all potentially built-in attributes will be passed through normal name resolution (#53913) and that uncovers even more cases where the strict "macro namespace is a single namespace" rule needs to be broken.
For example, with strict rules, built-in macro `cfg!(...)` would shadow built-in attribute `#[cfg]` (they are different things), standard library macro `thread_local!(...)` would shadow built-in attribute `#[thread_local]` - both of these cases are covered by special hacks in #53913 as well.
Crater run uncovered more cases of attributes being shadowed by user-defined macros (`warn`, `doc`, `main`, even `deprecated`), we cannot add exceptions in the compiler for all of them.
Regressions with user-defined attributes like #53583 and #53898 also appeared after enabling macro modularization.
People are also usually confused (#53205 (comment), #53583 (comment)) when they see conflicts between attributes and non-attribute macros for the first time.
So my proposed solution is to solve this issue by introducing two sub-namespaces and thus skipping resolutions of the wrong kind and preventing more error-causing cases of shadowing.
Fixes #53583
This was referenced Jun 25, 2019
Closed
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.
This is not full "pass all attributes through name resolution", but a more conservative solution.
If built-in attribute is ambiguous with any other macro in scope, then an error is reported.
What complications arise with the full solution - #53913 (comment).
cc #50911 (comment)
cc #52269
Closes #53531