resolve: Simplify collection of traits in scope #80765
Merged
Conversation
This file contains hidden or 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
This comment has been minimized.
This comment has been minimized.
rust-highfive
added
the
S-waiting-on-review
petrochenkov
commented
Jan 6, 2021
jyn514
approved these changes
Jan 7, 2021
jyn514
added
A-resolve
|
It's probably better to land #80782 first. |
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jan 10, 2021
resolve: Scope visiting doesn't need an `Ident` Resolution scope visitor (`fn visit_scopes`) currently takes an `Ident` parameter, but it doesn't need a full identifier, or even its span, it only needs the `SyntaxContext` part. The `SyntaxContext` part is necessary because scope visitor has to jump to macro definition sites, so it has to be directed by macro expansion information somehow. I think it's clearer to pass only the necessary part. Yes, usually visiting happens as a part of an identifier resolution, but in cases like collecting traits in scope (rust-lang#80765) or collecting typo suggestions that's not the case. r? `@matthewjasper`
This comment has been minimized.
This comment has been minimized.
petrochenkov
added
S-waiting-on-author
petrochenkov
force-pushed
the
traitsinscope
branch
from
f7652b0 to
b7071b2
Compare
|
Updated. |
petrochenkov
added
S-waiting-on-review
|
@bors r+ |
|
📌 Commit b7071b2 has been approved by |
bors
added
S-waiting-on-bors
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``
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jan 17, 2021
Rollup of 13 pull requests Successful merges: - rust-lang#79298 (correctly deal with late-bound lifetimes in anon consts) - rust-lang#80031 (resolve: Reject ambiguity built-in attr vs different built-in attr) - rust-lang#80201 (Add benchmark and fast path for BufReader::read_exact) - rust-lang#80635 (Improve diagnostics when closure doesn't meet trait bound) - rust-lang#80765 (resolve: Simplify collection of traits in scope) - rust-lang#80932 (Allow downloading LLVM on Windows and MacOS) - rust-lang#80983 (Remove is_dllimport_foreign_item definition from cg_ssa) - rust-lang#81064 (Support non-stage0 check) - rust-lang#81080 (Force vec![] to expression position only) - rust-lang#81082 (BTreeMap: clean up a few more comments) - rust-lang#81084 (Use Option::map instead of open-coding it) - rust-lang#81095 (Use Option::unwrap_or instead of open-coding it) - rust-lang#81107 (Add NonZeroUn::is_power_of_two) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
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.
"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 #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 #65351.
In this PR we are doing some much simpler pruning based on
SymbolandNamespacecomparisons, 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
visit_scopesinstead of a hand-rolled version.r? @matthewjasper