Fix leading qualifiers in use-import prefixes - #717
Conversation
|
Me, realizing I did not request @tshepang and @kirtchev-adacore to review 🙃 |
|
|
||
| #. :dp:`fls_IPYvldMqduf4` | ||
| Start the :t:`import path prefix` as follows: | ||
| Make the given :t:`use import` the current :t:`use import`, and start the :t:`import path prefix` as follows: |
There was a problem hiding this comment.
This, coupled with "Make the :t:nesting import the current :t:use import.", is not correct, and is confusing.
Syntactically, a "use import" is just use ...; (see the grammar for UseImport). These two rules somewhat imply that the construction of the import path prefix spans across multiple use imports (and I mean multiple items), which is just not true.
There was a problem hiding this comment.
Agree with you. I removed the mutable "current use import" traversal entirely.
The construction now refers only to the nesting imports that contain the target simple or glob import, followed by that target import's own contribution, so it no longer suggests traversal across use items.
| * :dp:`fls_irdKqoYzBM0M` | ||
| If the :t:`use import` is a :t:`nesting import` then start with the | ||
| :t:`[path segment]s` of the :t:`nesting import`'s :t:`common path prefix`. | ||
| If the :t:`nesting import` has a :t:`common path prefix`, prepend the :t:`common path prefix` to the :t:`import path prefix`. If the :t:`common path prefix` contains a :t:`path segment` and the :t:`import path prefix` was not empty, place a :t:`namespace qualifier` ``::`` between them. |
There was a problem hiding this comment.
I see what you are trying to do here, however this is becoming quite confusing. I think we have a higher-level problem with our approach. Perhaps going start-to-end is not the correct approach.
We basically want to identify the prefix that is supposed to resolve to something, and that something depends on the path segments involved, and the use of certain symbols and keywords. For example, in
use aaa::bbb::{self, ccc::*, ddd::{self, eee}};the prefixes of interest are
aaa::bbb::for the subsequentself,cccandddd,aaa::bbb::ccc::for the subsequent*,aaa::bbb::ccc::ddd::for the subsequentselfandeee.
Perhaps the correct approach is to define "import path prefix" in terms of a end-to-start order, relative to some path segment, *, or keyword.
What do you think?
There was a problem hiding this comment.
I'm glad you saw the vision, even if it was kind of confusing.
The definition now starts with a particular simple or glob import and identifies its enclosing nesting imports. Outermost-to-innermost is the order of concatenation; there is no mutable traversal.
For your example, the prefixes are aaa::bbb, aaa::bbb::ccc, and aaa::bbb::ddd (ddd is a sibling of ccc).
This comment has been minimized.
This comment has been minimized.
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
|
||
| #. :dp:`fls_IPYvldMqduf4` | ||
| Start the :t:`import path prefix` as follows: | ||
| Make the given :t:`use import` the current :t:`use import`, and start the :t:`import path prefix` as follows: |
There was a problem hiding this comment.
Agree with you. I removed the mutable "current use import" traversal entirely.
The construction now refers only to the nesting imports that contain the target simple or glob import, followed by that target import's own contribution, so it no longer suggests traversal across use items.
| * :dp:`fls_irdKqoYzBM0M` | ||
| If the :t:`use import` is a :t:`nesting import` then start with the | ||
| :t:`[path segment]s` of the :t:`nesting import`'s :t:`common path prefix`. | ||
| If the :t:`nesting import` has a :t:`common path prefix`, prepend the :t:`common path prefix` to the :t:`import path prefix`. If the :t:`common path prefix` contains a :t:`path segment` and the :t:`import path prefix` was not empty, place a :t:`namespace qualifier` ``::`` between them. |
There was a problem hiding this comment.
I'm glad you saw the vision, even if it was kind of confusing.
The definition now starts with a particular simple or glob import and identifies its enclosing nesting imports. Outermost-to-innermost is the order of concatenation; there is no mutable traversal.
For your example, the prefixes are aaa::bbb, aaa::bbb::ccc, and aaa::bbb::ddd (ddd is a sibling of ccc).
| :t:`[path segment]s` of the :t:`simple import`'s :t:`simple path` | ||
| :t:`path prefix`. | ||
| * :dp:`fls_irdKqoYzBM0M` | ||
| The :t:`nesting import`'s :t:`common path prefix` if it has one, :t:`namespace qualifier` ``::`` if it lacks a :t:`common path prefix` and its :s:`CommonPathPrefix` is present, or an empty sequence otherwise. |
There was a problem hiding this comment.
I could not decipher this paragraphs. The options are
- The nesting import's common path prefix if it has one
- :: if it lacks a common path prefix and its CommonPathPrefix is present
- empty sequence
For 2, what do the two "it"s refer to? If the first "it" refers to the nesting import, then the second "it" cannot be the same nesting import as this would be nonsense.
| If the :t:`use import` is a :t:`nesting import` then start with the | ||
| :t:`[path segment]s` of the :t:`nesting import`'s :t:`common path prefix`. | ||
| * :dp:`fls_2UyFcB6Our1v` | ||
| For a :t:`glob import`, the :t:`glob import`'s :t:`common path prefix` if it has one, :t:`namespace qualifier` ``::`` if it lacks a :t:`common path prefix` and its :s:`CommonPathPrefix` is present, or an empty sequence otherwise. |
There was a problem hiding this comment.
The same issue with the two "it"s is also present here.
| :t:`import path prefix`. Repeat this step with the :t:`nesting import` as | ||
| the current :t:`use import`. | ||
| * :dp:`fls_MOXId37fcNPY` | ||
| For a :t:`simple import`, the :t:`simple import`'s :t:`simple path` after removing its last :t:`path segment` and, if another :t:`path segment` precedes the last :t:`path segment`, the :t:`namespace qualifier` that separates them. |
There was a problem hiding this comment.
I think that "and, if another ..." is redundant. If the path is aaa::bbb, then after you remove bbb, you are left with aaa::, so there is no need to mention that the path also retains the namespace qualifier.
| into :t:`scope`. | ||
|
|
||
| :dp:`fls_BMtRtjJ7gBKT` | ||
| A :t:`glob import`, :t:`nesting import`, or :t:`simple import` expressed with a leading :t:`namespace qualifier` ``::`` shall not be nested, directly or indirectly, within any :t:`nesting import` whose :s:`CommonPathPrefix` is present. |
There was a problem hiding this comment.
I am not sure what "indirect nesting" is. If you have aaa::{::bbb}, then ::bbb is just "nested". I don't understand the distinction between direct and indirect nesting.
| A :t:`glob import`, :t:`nesting import`, or :t:`simple import` expressed with a leading :t:`namespace qualifier` ``::`` shall not be nested, directly or indirectly, within any :t:`nesting import` whose :s:`CommonPathPrefix` is present. | ||
|
|
||
| :dp:`fls_UZHHtqJ0ekju` | ||
| An empty :t:`import path prefix` that selects the :t:`entity` of a :t:`simple import` resolves to the current :t:`module`. |
There was a problem hiding this comment.
How about
A simple import with an empty import path prefix resolves to the current module.
This is consistent with how other similar rules are expressed, plus I am not a fan of "selects". Use imports do not "choose", they simply "import" names.
| :dp:`fls_iNUBX5fJAI1N` | ||
| A :t:`glob import` outside of a :t:`nesting import` without a :t:`common path | ||
| prefix` is rejected, but may still be consumed by :t:`[macro]s`. | ||
| A :t:`glob import` whose :t:`import path prefix` is empty or consists only of :t:`namespace qualifier` ``::`` is rejected, but may still be consumed by :t:`[macro]s`. |
There was a problem hiding this comment.
I know you did not write this, but it might be a good opportunity to fix the language here.
The FLS does not use the verb "to reject" because this implies a particular behavior of a conforming tool (ex: The compiler rejects yada yada with an error message). The more neutral way of expressing this scenario is "static error".
So how about
It is a static error if a glob import whose import path prefix is empty or consists only of namespace qualifier ::.
I am not sure whether it is worth mentioning the "consumption by macros" part, as I suspect more illegal constructs can be triaged by macros, but we never mention that interplay.
Summary
Preserve leading
::in import path prefixes, distinguishing an empty prefix from bare::and qualified paths such as::std.Replace the stateful construction with a declarative definition and reject misplaced root qualifiers, including in empty nested groups.
Reconcile the shadowing rules with explicit global lookup: local names do not prevent
::stdfrom selecting the external crate. The broader nonglobal ambiguity model and trait-parent rules are unchanged.Closes #711
Reference alignment
Rust Reference: use declarations
Global paths and prelude shadowing, for Rust 2021.
Testing
Documentation CI and the clean local build pass. Link checker: 17,062 links, zero errors. All 63 compiler fixtures match their expected outcomes using Rust 1.98.0, edition 2021; eight supplemental probes separately reproduce the shadowing boundary cases and overlap the main suite.