-
Notifications
You must be signed in to change notification settings - Fork 560
Expand name resolution stub #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
src/items/use-declarations.md
Outdated
|
|
||
| * except where shadowing is allowed | ||
| r[names.resolution.early.imports.errors.ambiguity.globvsglob] | ||
| * it is an error to name an item through ambiguous use declarations |
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.
documented already
src/items/use-declarations.md
Outdated
| * it is an error to name an item through ambiguous use declarations | ||
| * two globs imports which both have an item matching that name where the items are different | ||
| * this is still an error even if there is a third non glob binding resolution to an item with the same name | ||
| * it is not an error to have two glob imports which include items which would be ambiguous so long as you do not name one of those items through the ambiguous glob imports |
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.
documented
| For example, the [`cfg` attribute] and the [`cfg` macro] are two different entities with the same name in the macro namespace, but they can still be used in their respective context. | ||
|
|
||
| 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. |
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 feel conflicted about this piece of info being in this section. On the one hand sub-namespaces are quite similar to shadowing, though they work via an independent mechanism. On the other, I'm fairly certain this is exclusively referring to the TextualVsPathBased ambiguity error with the added bit of information of pointing out that this is unaffected by sub-namespaces. I need to verify this but I suspect this is because of how imports bring in names from all namespaces so it probably always counts as a sub-namespace match preventing the subnamespace mismatch early exit from ever bailing before we report the ambiguity error.
Assuming this explanation is correct, we can maybe find a place within the use-declarations or mbe chapters to mention how imports are unaffected by subnamespaces. I'm currently wondering if it would make sense to move the subnamespace stuff to live in one of the macros chapters and link to it from here.
src/items/use-declarations.md
Outdated
| * [macro.decl.scope.path.ambiguity] | ||
| r[names.resolution.early.imports.errors.ambiguity.globvsouter] | ||
| * it is an error to shadow an outer name binding with a glob import | ||
| * This seems to only apply to early resolution (duh, I documented this as part of an early resolution codepath) |
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.
Most of the new text here need to be moved to name resolution, IMO.
I'd use src/items/use-declarations.md for things that happen at import definition time.
But most of ambiguity errors here do not happen at import definition time, but rather when someone tries to resolve a name from some unlucky point of view.
Only some situations create inherently ambiguous import definitions, like glob-vs-glob conflicts, and even then they are not reported at definition time.
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 don't mind moving this but the reason I put them alongside their items was because that seemed most consistent with how we documented other parts of name resolution, specifically how the names that are introduced by each item are documented next to their items and then theres an aggregated list linking to all those sections in the namespaces chapter.
My plan was to put ambiguity and shadowing sections alongside their items and aggregate links to all those sections in name-resolution.md.
You can see where I discussed this plan with eric starting here: #t-lang-docs/reference > name resolution @ 💬
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've gone ahead and moved the ambiguity sections to the name-res chapter.
|
|
||
| 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
| * .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
22aaa92 to
80fd707
Compare
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.
TODO: come back and add a section mentioning the macro-related ambiguity errors and linking to the name-res chapter
| pub fn qux() {} | ||
| ``` | ||
|
|
||
| r[names.resolution.early.imports.errors.ambiguity.derivehelper] |
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.
TODO: go back and add a section mentioning the proc macro-related ambiguity errors to the proc macros chapter and link to the name-res chapter
| // m!(); // Error: m is not in scope. | ||
| ``` | ||
|
|
||
| * textual scope name bindings for macros may shadow path-based scope bindings |
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.
should this get a rule?
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.
Seems so, yes.
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