resolve: Cleanup visibility resolution for enum variants and trait items #80762
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? @varkor (rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
cc @camelid |
petrochenkov
commented
Jan 6, 2021
camelid
reviewed
Jan 6, 2021
camelid
added
A-resolve
This comment has been minimized.
This comment has been minimized.
varkor
reviewed
Jan 9, 2021
| mod rank { | ||
| pub use self::Professor::*; | ||
| //~^ ERROR enum is private and its variants cannot be re-exported | ||
| //~^ ERROR glob import doesn't reexport anything |
There was a problem hiding this comment.
Is this a change in behaviour, or is the lint taking precedence over the "enum is private" error?
There was a problem hiding this comment.
This is a change in behavior, now glob import from enum with private variants behaves identically to a glob import from a module with private items.
Previously, in the world without pub(restricted), this required special logic and separate error.
There was a problem hiding this comment.
This definitely seems like better behaviour, but is this something that would need to undergo a lang team FCP, or is this a change that has already been discussed?
There was a problem hiding this comment.
Maybe.
When the error was added in 8f359d5 (as a part of hardening private-in-public errors), the glob case wasn't really discussed.
The glob error was added pretty conservatively to seal any possible sources of leakage, but even then it was inconsistent with behavior of other glob imports.
There was a problem hiding this comment.
Let's cc them just to make sure, though it's likely this isn't large enough to warrant an FCP.
|
@rust-lang/lang: this pull request changes the behaviour of glob imports so that enums with private variants behave identically to modules with private items (see #80762 (comment)). Is this change small enough not to warrant an FCP? If not, could someone kick one off? |
petrochenkov
added
S-waiting-on-team
m-ou-se
added a commit
to m-ou-se/rust
that referenced
this pull request
Jan 16, 2021
…ewjasper resolve: Simplify collection of traits in scope "Traits in scope" for a given location are collected by walking all scopes in type namespace, collecting traits in them and pruning traits that don't have an associated item with the given name and namespace. Previously we tried to prune traits using some kind of hygienic resolution for associated items, but that was complex and likely incorrect, e.g. in rust-lang#80762 correction to visibilites of trait items caused some traits to not be in scope anymore. I previously had some comments and concerns about this in rust-lang#65351. In this PR we are doing some much simpler pruning based on `Symbol` and `Namespace` comparisons, it should be enough to throw away 99.9% of unnecessary traits. It is not necessary for pruning to be precise because for trait aliases, for example, we don't do any pruning at all, and precise hygienic resolution for associated items needs to be done in typeck anyway. The somewhat unexpected effect is that trait imports introduced by macros 2.0 now bring traits into scope due to the removed hygienic check on associated item names. I'm not sure whether it is desirable or not, but I think it's acceptable for now. The old check was certainly incorrect because macros 2.0 did bring trait aliases into scope. If doing this is not desirable, then we should come up with some other way to avoid bringing traits from macros 2.0 into scope, that would accommodate for trait aliases as well. --- The PR also contains a couple of pure refactorings - Scope walk is done by using `visit_scopes` instead of a hand-rolled version. - Code is restructured to accomodate for rustdoc that also wants to query traits in scope, but doesn't want to filter them by associated items at all. r? `@matthewjasper`
m-ou-se
added a commit
to m-ou-se/rust
that referenced
this pull request
Jan 17, 2021
…ewjasper resolve: Simplify collection of traits in scope "Traits in scope" for a given location are collected by walking all scopes in type namespace, collecting traits in them and pruning traits that don't have an associated item with the given name and namespace. Previously we tried to prune traits using some kind of hygienic resolution for associated items, but that was complex and likely incorrect, e.g. in rust-lang#80762 correction to visibilites of trait items caused some traits to not be in scope anymore. I previously had some comments and concerns about this in rust-lang#65351. In this PR we are doing some much simpler pruning based on `Symbol` and `Namespace` comparisons, it should be enough to throw away 99.9% of unnecessary traits. It is not necessary for pruning to be precise because for trait aliases, for example, we don't do any pruning at all, and precise hygienic resolution for associated items needs to be done in typeck anyway. The somewhat unexpected effect is that trait imports introduced by macros 2.0 now bring traits into scope due to the removed hygienic check on associated item names. I'm not sure whether it is desirable or not, but I think it's acceptable for now. The old check was certainly incorrect because macros 2.0 did bring trait aliases into scope. If doing this is not desirable, then we should come up with some other way to avoid bringing traits from macros 2.0 into scope, that would accommodate for trait aliases as well. --- The PR also contains a couple of pure refactorings - Scope walk is done by using `visit_scopes` instead of a hand-rolled version. - Code is restructured to accomodate for rustdoc that also wants to query traits in scope, but doesn't want to filter them by associated items at all. r? ``@matthewjasper``
m-ou-se
added a commit
to m-ou-se/rust
that referenced
this pull request
Jan 17, 2021
…ewjasper resolve: Simplify collection of traits in scope "Traits in scope" for a given location are collected by walking all scopes in type namespace, collecting traits in them and pruning traits that don't have an associated item with the given name and namespace. Previously we tried to prune traits using some kind of hygienic resolution for associated items, but that was complex and likely incorrect, e.g. in rust-lang#80762 correction to visibilites of trait items caused some traits to not be in scope anymore. I previously had some comments and concerns about this in rust-lang#65351. In this PR we are doing some much simpler pruning based on `Symbol` and `Namespace` comparisons, it should be enough to throw away 99.9% of unnecessary traits. It is not necessary for pruning to be precise because for trait aliases, for example, we don't do any pruning at all, and precise hygienic resolution for associated items needs to be done in typeck anyway. The somewhat unexpected effect is that trait imports introduced by macros 2.0 now bring traits into scope due to the removed hygienic check on associated item names. I'm not sure whether it is desirable or not, but I think it's acceptable for now. The old check was certainly incorrect because macros 2.0 did bring trait aliases into scope. If doing this is not desirable, then we should come up with some other way to avoid bringing traits from macros 2.0 into scope, that would accommodate for trait aliases as well. --- The PR also contains a couple of pure refactorings - Scope walk is done by using `visit_scopes` instead of a hand-rolled version. - Code is restructured to accomodate for rustdoc that also wants to query traits in scope, but doesn't want to filter them by associated items at all. r? ```@matthewjasper```
petrochenkov
force-pushed
the
visclean
branch
from
67035af
to
a8fc1db
Compare
This comment has been minimized.
This comment has been minimized.
Special treatment like this was necessary before `pub(restricted)` had been implemented and only two visibilities existed - `pub` and non-`pub`. Now it's no longer necessary and the desired behavior follows from `pub(restricted)`-style visibilities naturally assigned to enum variants and trait items.
petrochenkov
force-pushed
the
visclean
branch
from
a8fc1db
to
8e74842
Compare
|
No response for a month, I'll interpret this as a lack of interest. The current rule is a result of an ad hoc compiler future-proofing rather than a T-lang decision, and this PR removes it in favor of a more general non ad hoc rule, so I think we can just land it. |
petrochenkov
added
S-waiting-on-review
|
I'm inclined to agree. @bors r+ |
|
📌 Commit 8e74842 has been approved by |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
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.
by always delegating it to
fn resolve_visibility.Also remove some special treatment of visibility on enum variants and trait items remaining from pre-
pub(restricted)times.