Fully integrate derive helpers into name resolution #64694
Conversation
|
r? @estebank (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
cc @c410-f3r |
|
The current rules ("walk the derive's input AST and mark its helpers as known") works like this: #[derive(Foo)]
struct S {
#[foo_helper] // marked as known
field1: u8,
#[some_attr]
#[foo_helper] // marked as known
field2: u8,
field3: bang_macro_type!(), // `#[foo_helper]` inside the macro is NOT marked as known
}
// Generated by `Foo`
impl Foo for S {
#[derive_helper] // NOT marked as known
fn method(&self) { /* ... */ }
}If we reformulate it in terms of scopes, then we get something like this:
The question is - how close should we mirror these rules? Can the rule be just "The helper is in scope inside the expansion of #[foo_attr] // #[proc_macro_attribute(attributes(foo_helper))] fn foo_attr(...) { ... }
struct S {
// no need to erase this attribute from code generated by `foo_attr`,
// it will be treated as a known helper attribute
#[foo_helper]
field: u8,
}What do you think? |
|
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 |
Centril
added
needs-fcp
| field: [u8; { | ||
| // FIXME No ambiguity, derive helpers are not put into scope for non-attributes | ||
| use empty_helper; | ||
| use empty_helper; //~ ERROR `empty_helper` is ambiguous |
There was a problem hiding this comment.
If this were use crate::empty_helper, is the attribute on struct U still ambiguous?
There was a problem hiding this comment.
The attribute on struct U is still ambiguous because two different empty_helpers still exist in scope at the struct U point, and derive helpers always produce an error in case of ambiguity (shadowing is not allowed).
|
The new behavior seems reasonable to me from reading derive-helper-shadowing.rs. |
|
This all seems reasonable to me! |
|
This sounds good to me. I think the test should include a use of Also, since this is a breaking change, how will it be rolled out? |
I planned to just land this as is for now. |
|
r? @pnkfelix who will look at this from a language team perspective. We did not have time to fully process the PR in this meeting. |
petrochenkov
changed the title
|
Updated with a complete implementation and more tests. The implementation now mirrors existing rules very closely, the helpers are not visible inside code expanded from non-attribute macros. #[derive(Trait)]
struct S {
field: type!(), // code inside `type!()` doesn't see the Trait's helpers
}
// Generated by the derive above
impl Trait for S { ... } // code in the generated impl also doesn't see the Trait's helpers |
|
Ping from Triage: @SergioBenitez @dtolnay -- any updated review for @petrochenkov's last comment? |
|
@petrochenkov I've reviewed your comments here, the implementation, and (perhaps most importantly), the additions you have made to I have two main observations that I want to double-check with you about the test:
My question is: In this latter case, illustrated by the two points marked "// OK, no ambiguity", where the attribute is resolvable: Is the observable behavior the same or potentially different from what we would have observed before this PR? Is it possible that some code exists where a procedural macro successfully expands some code without error both before and after the effects of this PR, but the semantics of the expansion have silently changed? Or is it not possible for this to happen? (My current guess is that any client who was using helper or non-helper attributes in a non-erroneous manner before, and in a manner such that it continues to not error now, must have been using them in a manner that will not be affected by this change.) |
No, this shouldn't be possible.
|
|
@rust-lang/lang I think we can move forward with this |
|
@rfcbot fcp merge |
|
Team member @pnkfelix has proposed to merge this. The next step is review by the rest of the tagged team members: No concerns currently listed. Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for info about what commands tagged team members can give me. |
rfcbot
added
proposed-final-comment-period
petrochenkov
added
S-waiting-on-author
These helpers are resolved before their respective derives through a kind of look ahead into future expansions. Some of these will migrate to proper resolution, others will be deprecated. ``` #[trait_helper] // Deprecate #[derive(Trait)] #[trait_helper] // Migrate to proper resolution ```
Pass them through name resolution instead
petrochenkov
force-pushed
the
reshelp
branch
from
53a90e4
to
8668c1a
Compare
|
Rebased. |
petrochenkov
added
S-waiting-on-review
|
@bors r+ |
|
📌 Commit 8668c1a has been approved by |
bors
added
S-waiting-on-bors
Fully integrate derive helpers into name resolution
```rust
#[derive(Foo)]
#[foo_helper] // already goes through name resolution
struct S {
#[foo_helper] // goes through name resolution after this PR
field: u8
}
```
How name resolution normally works:
- We have an identifier we need to resolve, take its location (in some sense) and look what names are in scope in that location.
How derive helper attributes are "resolved" (before this PR):
- After resolving the derive `Foo` we visit the derive's input (`struct S { ... } `) as a piece of AST and mark attributes textually matching one of the derive's helper attributes (`foo_helper`) as "known", so they never go through name resolution.
This PR changes the rules for derive helpers, so they are not proactively marked as known (which is a big hack ignoring any ambiguities or hygiene), but go through regular name resolution instead.
This change was previously blocked by attributes not being resolved in some positions at all (fixed in #63468).
"Where exactly are derive helpers in scope?" is an interesting question, and I need some feedback from proc macro authors to answer it, see the post below (#64694 (comment)).
|
☀️ Test successful - checks-azure |
resolve: Give derive helpers highest priority during resolution So they just shadow everything else and don't create ambiguity errors. This matches the old pre-rust-lang#64694 behavior most closely. --- The change doesn't apply to this "compatibility" case ```rust #[trait_helper] // The helper attribute is used before it introduced. // Sadly, compiles on stable, supported via hacks. // I plan to make a compatibility warning for this. #[derive(Trait)] struct S; ``` , such attributes still create ambiguities, but rust-lang#64694 didn't change anything for this case. Fixes rust-lang#66508 Fixes rust-lang#66525
resolve: Give derive helpers highest priority during resolution So they just shadow everything else and don't create ambiguity errors. This matches the old pre-rust-lang#64694 behavior most closely. --- The change doesn't apply to this "compatibility" case ```rust #[trait_helper] // The helper attribute is used before it introduced. // Sadly, compiles on stable, supported via hacks. // I plan to make a compatibility warning for this. #[derive(Trait)] struct S; ``` , such attributes still create ambiguities, but rust-lang#64694 didn't change anything for this case. Fixes rust-lang#66508 Fixes rust-lang#66525
How name resolution normally works:
How derive helper attributes are "resolved" (before this PR):
Foowe visit the derive's input (struct S { ... }) as a piece of AST and mark attributes textually matching one of the derive's helper attributes (foo_helper) as "known", so they never go through name resolution.This PR changes the rules for derive helpers, so they are not proactively marked as known (which is a big hack ignoring any ambiguities or hygiene), but go through regular name resolution instead.
This change was previously blocked by attributes not being resolved in some positions at all (fixed in #63468).
"Where exactly are derive helpers in scope?" is an interesting question, and I need some feedback from proc macro authors to answer it, see the post below (#64694 (comment)).