-
Notifications
You must be signed in to change notification settings - Fork 562
Add section on expansion-time (early) name resolution #2055
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
base: master
Are you sure you want to change the base?
Conversation
|
Thanks for posting. Lot of good here. The main thing I'd suggest, by way of helping to sharpen this up, would be to try to write a concise example after each claim, in as many cases as that might make sense, that demonstrates that the claim is true (and would not pass if the claim were false). Aside from the intrinsic benefit of having such examples, I think this might help to focus the text on the language-level effects. (It's not surprising, given the good research you've been doing, that some bits of this currently have some "description of the implementation" flavor.) |
yaahc
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a fair number of comments are also contained inline in the documents
|
|
||
| r[names.resolution.early.imports.shadowing] | ||
|
|
||
| The following is a list of situations where shadowing of use declarations is permitted: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| The following is a list of situations where shadowing of use declarations is permitted: | |
| The following is a list of situations where shadowing of use declarations is not permitted: |
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the original text is correct.
Use glob shadowing: https://doc.rust-lang.org/nightly/reference/items/use-declarations.html#r-items.use.glob.shadowing
macro textual scope shadowing: https://doc.rust-lang.org/nightly/reference/macros-by-example.html#r-macro.decl.scope.textual.shadow
In my mind the "not permitted" set is the list of ambiguity errors below
src/names/name-resolution.md
Outdated
| * .visitation-order | ||
| * derive helpers | ||
| * not visited when resolving derive macros in the parent scope (starting scope) | ||
| * derive helpers compat |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same concern as above about this being deprecated and removed next year.
This comment has been minimized.
This comment has been minimized.
|
@ehuss and I are looking at this together on the lang-docs office hours call, and we just wanted to express our appreciation to @petrochenkov for having been so responsive with @yaahc on working out the details here. This is a chapter that we've long wanted to exist, and we're thrilled and appreciative that @yaahc is digging in to shape this up. |
5397d08 to
1bd3afe
Compare
80fd707 to
570bedd
Compare
| * derive helpers used before their associated derive may not shadow other attributes or other derive helpers that are otherwise in scope after their derive | ||
| * TODO example? This ones harder to do concisely afaik | ||
|
|
||
| Helper attributes may not be used before the macro that introduces them. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- What happens if two macros introduce the same helper, will the second one not
be able to see the attribute of the first anymore, assuming their order is
"firstmacro" "helper" "secondmacro"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will the second one not
be able to see the attribute of the first anymore
It will, helpers are not removed.
Helpers don't have "identity" really, technically the compiler tracks it, but for the compiler it's important that it's someone's helper and that's enough, and proc macros detect helpers by string comparison, so they'll happily use someone else's helpers with the same name as their own.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice to add a test for this to rust-lang/rust, if there's none currently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was setting up a test for this and I think I found a mistake I made in interpreting the logic in resolve_ident_in_scope_set. Specifically I said
} else if innermost_res == derive_helper_compat
|| res == derive_helper_compat && innermost_res != derive_helper
{
Some(AmbiguityKind::DeriveHelper)means
- if a derive helper which is used before it's derive has the same name as an outer item
- or if a derive helper has the same name as an outer derive helper which is used before it's derive
The first bullet is true afaict, being in this codepath implies theres a res and innermost_res, so having the innermost be a derive_helper_compat means that we have a second candidate we found later in the same sub-namespace.
The second bullet is incorrect. It seems like I interpreted the != as an ==. This is just saying that the second resolution is a compat resolution, and the first resolution is not a helper, which means it could only be another helper compat due to visitation order. Given the lack of identity on helpers, I'm not sure how to trigger this second half of the condition.
Here's the test I setup:
#[empty_helper]
#[derive(Empty)]
#[empty_helper]
#[derive(Empty2)]
struct S;(edited next two paragraphs)
Both derives introduce the same #[empty_helper] attribute.
Assuming the language I drafted in this PR which this comment is attached to is correct, the first #[empty_helper] should produce a warning and the second one should produce an error, since Empty2's helper attribute is shadowing the correctly in scope attribute from Empty. Instead, the only error this produces is the future incompat warning on the first helper for using a helper before it's derive which is converted into an error by deny(future_incompat).
I'm going to try digging into this a bit deeper to see if I can pinpoint exactly when this subbranch gets triggered or whether I'm correct in thinking this is inert.
https://github.com/rust-lang/rust/pull/79078/files#diff-c75db064f0f7310431b21d0ecb61c9438c6e48f9ab2810a25251f504d21c6d9dR865 introduced this logic
Edit: looking at visit_scopes it looks like we're never "iterating through derive_helper_compat" scopes so I think it will only ever call the closure from resolve_ident_in_scope_set once for the derivehelpercompat scope and only ever produce one derive_helper_compat candidate, so the only candidates that could be visited earlier are the proper derivehelper candidates.
I went ahead and added a panic in the second case and I'm checking if that causes our testsuite to fall over anywhere but initial indications (running it in tests/ui/proc-macro) are coming up clean, implying it is in fact inert.
7e17934 to
4744e19
Compare
9ada73e to
f982337
Compare
src/names/namespaces.md
Outdated
|
|
||
| r[names.namespaces.sub-namespaces.use-shadow] | ||
| It is still an error for a [`use` import] to shadow another macro, regardless of their sub-namespaces. | ||
| * TODO revisit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Revisiting this. I think I've decided against wanting to move the entire sub-namespaces section into the name resolution chapter. I was consider if it made sense to argue that they aren't a true namespace since the mechanism through which they're implemented is different and that they're actually a property of the name resolution algorithm that creates "apparent sub-namespaces" or something along those line. I ended up convincing myself that this is a distinction without a difference and its just simpler pedagogically to call them sub-namespaces.
I do think I want to give the use-shadowing subsection the same treatment as the other ambiguity errors, remove the section here and add an admonition linking to the relevant section in name-resolution.
| ``` | ||
| r[names.resolution.early.imports.ambiguity.pathvstextualmacro] | ||
| Path-based scope bindings for macros may not shadow textual scope bindings to macros. For bindings from [use declarations], this applies regardless of their [sub-namespace]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
implication here that I need to investigate: that it is possible to have path-based bindings to macros that would shadow textual bindings if imported but not if otherwise defined in the same scope.
| Macros are resolved by iterating through the available scopes until a candidate | ||
| is found. Macros are split into two sub-namespaces, one for bang macros, and | ||
| the other for attributes and derives. Resolution candidates from the incorrect | ||
| sub-namespace are ignored. The available scopes are visited in the following order. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not at all happy with this, in particular the first sentence seems like a gross oversimplification. I wrote this as is because I was trying to find some way to introduce the scope visitation order logic.
src/names/name-resolution.md
Outdated
|
|
||
| r[names.resolution.intro] | ||
|
|
||
| _Name resolution_ is the process of tying paths and other identifiers to the declarations of those entities. Names are segregated into different [namespaces], allowing entities in different namespaces to share the same name without conflict. Each name is valid within a [scope], or a region of source text where that name may be referenced. Access to certain names may be restricted based on their [visibility]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What other identifiers?
Single-segment paths are also paths.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lifetimes / type parameters come to mind as an example which I wouldn't consider paths that need their names resolved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lifetimes are not paths, true.
I actually haven't seen this mentioned in the added text, but that's because named lifetimes are resolved in later resolution, which is not documented yet.
Type parameter use is a single-segment path, you don't know that it's a type parameter until you resolve that path.
I recalled one more example though, names of trait impl items are resolved to trait items, and they are not paths.
trait Trait { fn f(); }
impl Trait for () { fn f() {} } // `f` is resolved to its trait origin (during late resolution)
src/names/name-resolution.md
Outdated
| ## Ambiguities | ||
|
|
||
| r[names.resolution.expansion.imports.ambiguity.intro] | ||
| Some situations are an error when there is an ambiguity as to which name a `use` declaration refers. This happens when there are two name candidates that do not resolve to the same entity where neither import is [permitted] to shadow the other. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| Some situations are an error when there is an ambiguity as to which name a `use` declaration refers. This happens when there are two name candidates that do not resolve to the same entity where neither import is [permitted] to shadow the other. | |
| Some situations are an error when there is an ambiguity as to which name a `use` declaration or a macro path refers. This happens when there are two name candidates that do not resolve to the same entity where neither import is [permitted] to shadow the other. |
I think all the cases below are relevant for both.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you are correct. I went ahead and added the extra cases and improved the examples for globvsouter to better demonstrate the variety of cases. I also reworded it to clarify that the ambiguities come from the definitions and are found when resolving names for macro invocations or use declarations. This let me also capture that modules can be a source of ambiguity.
6689d3b to
8a6f849
Compare
Co-authored-by: Vadim Petrochenkov <[email protected]>
currently mostly a skeleton of a draft so we can collaboratively massage it into shape more easily before filling in with proper reference verbiage.
hoping to take a significant chunk out of #568