Resolve associated item bindings by namespace by fmease · Pull Request #118668 · rust-lang/rust (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation17 Commits1 Checks0 Files changed
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
[ Show hidden characters]({{ revealButtonHref }})
This is the 3rd commit split off from #118360 with tests reblessed (they no longer contain duplicated diags which were caused by 4c0addc) & slightly adapted (removed supertraits from a UI test, cc #118040).
- Resolve all assoc item bindings (type, const, fn (feature
return_type_notation)) by namespace instead of trying to resolve a type first (in the non-RTN case) and falling back to consts afterwards. This is consistent with RTN. E.g., forTr<K = {…}>we now always try to look up assoc consts (this extends to supertrait bounds). This gets rid of assoc tys shadowing assoc consts in assoc item bindings which is undesirable & inconsistent (types and consts live in different namespaces after all)- Consolidate the resolution of assoc {ty, const} bindings and RTN (dedup, better diags for RTN)
- Fix assoc consts being labeled as assoc types in several diagnostics
- Make a bunch of diagnostics translatable
Fixes #112560 (error → pass).
As discussed
r? @compiler-errors
Addendum: What I call “associated item bindings” are commonly referred to as “type bindings” for historical reasons. Nowadays, “type bindings” include assoc type bindings, assoc const bindings and RTN (return type notation) which is why I prefer not to use this outdated term.
Status: Awaiting review from the assignee but also interested parties.
Relevant to the compiler team, which will review and decide on the PR/issue.
labels
| None |
|---|
| }; |
| // FIXME(associated_const_equality): This has quite a few false positives and negatives. |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(preexisting)
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯
I just had one question about hygeine
r=me though if it's something unfixable at the moment
| let assoc_item = tcx |
|---|
| .associated_items(candidate.def_id()) |
| .filter_by_name_unhygienic(assoc_ident.name) |
| .find(|i |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh, how does this differ from find_by_name_and_kind?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
find_by_name_and_kind is semantically identical to the code above but this version is slightly faster in theory since we only normalize the LHS above instead of normalizing both sides (which is what hygienic_eq does). assoc_ident has already been normalized above in adjust_ident_and_get_scope, so no need to do it again (adjust_ident_and_get_scope also returns the def_scope which is used later in is_accessible_from).
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pls note this
| LL | fn foo1<F: Foo<Z=3>>() {} |
|---|
| | ^ associated type `Z` not found |
| LL | fn foo1<F: Foo<Z = 3>>() {} |
| | ^ help: there is an associated constant with a similar name: `N` |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is awesome
| | ^^^^^^^^^ |
|---|
| | ^^^ unexpected constant |
| | |
| note: the associated constant is defined here |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:D
| --> $DIR/consts.rs:3:29 |
|---|
| | |
| LL | pub fn accept(_: impl Trait<K: Copy>) {} |
| | ^ |
| | ^------ bounds are not allowed on associated constants |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✨
This comment has been minimized.
If a const is expected, resolve a const. If a type is expected, resolve a type. Don't try to resolve a type first falling back to consts.
📌 Commit 55559d9 has been approved by compiler-errors
It is now in the queue for this repository.
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
and removed S-waiting-on-author
Status: This is awaiting some action (such as code changes or more information) from the author.
labels
Finished benchmarking commit (5ea6256): comparison URL.
Overall result: no relevant changes - no action needed
@rustbot label: -perf-regression
Instruction count
This benchmark run did not return any relevant results for this metric.
Max RSS (memory usage)
Results
This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
| mean | range | count | |
|---|---|---|---|
| Regressions ❌ (primary) | 1.2% | [0.5%, 1.9%] | 2 |
| Regressions ❌ (secondary) | 2.3% | [2.3%, 2.3%] | 1 |
| Improvements ✅ (primary) | -0.5% | [-0.5%, -0.5%] | 2 |
| Improvements ✅ (secondary) | -2.3% | [-4.1%, -1.1%] | 8 |
| All ❌✅ (primary) | 0.4% | [-0.5%, 1.9%] | 4 |
Cycles
Results
This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
| mean | range | count | |
|---|---|---|---|
| Regressions ❌ (primary) | 1.1% | [0.6%, 1.7%] | 2 |
| Regressions ❌ (secondary) | - | - | 0 |
| Improvements ✅ (primary) | - | - | 0 |
| Improvements ✅ (secondary) | - | - | 0 |
| All ❌✅ (primary) | 1.1% | [0.6%, 1.7%] | 2 |
Binary size
This benchmark run did not return any relevant results for this metric.
Bootstrap: 674.492s -> 674.729s (0.04%)
Artifact size: 314.05 MiB -> 314.07 MiB (0.01%)
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request
…-bindings, r=compiler-errors
AstConv: Refactor lowering of associated item bindings a bit
Split off from rust-lang#119385 as discussed, namely the first two commits (modulo one FIXME getting turned into a NOTE).
The second commit removes astconv::ConvertedBinding{,Kind} in favor of hir::TypeBinding{,Kind}. The former was a — in my opinion — super useless intermediary. As you can tell from the diff, its removal shaves off some code. Furthermore, yeeting it makes it easier to implement the changes in rust-lang#119385.
Nothing in this PR should have any semantic effect.
r? @compiler-errors
Addendum as in rust-lang#118668: What I call “associated item bindings” are commonly referred to as “type bindings” for historical reasons. Nowadays, “type bindings” include assoc type bindings, assoc const bindings and RTN (return type notation) which is why I prefer not to use this outdated term.
fmease added a commit to fmease/rust that referenced this pull request
…-bindings, r=compiler-errors
AstConv: Refactor lowering of associated item bindings a bit
Split off from rust-lang#119385 as discussed, namely the first two commits (modulo one FIXME getting turned into a NOTE).
The second commit removes astconv::ConvertedBinding{,Kind} in favor of hir::TypeBinding{,Kind}. The former was a — in my opinion — super useless intermediary. As you can tell from the diff, its removal shaves off some code. Furthermore, yeeting it will make it easier to implement the type resolution fixes in rust-lang#119385.
Nothing in this PR should have any semantic effect.
r? @compiler-errors
Addendum as in rust-lang#118668: What I call “associated item bindings” are commonly referred to as “type bindings” for historical reasons. Nowadays, “type bindings” include assoc type bindings, assoc const bindings and RTN (return type notation) which is why I prefer not to use this outdated term.
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request
Rollup merge of rust-lang#121221 - fmease:refactor-astconv-assoc-item-bindings, r=compiler-errors
AstConv: Refactor lowering of associated item bindings a bit
Split off from rust-lang#119385 as discussed, namely the first two commits (modulo one FIXME getting turned into a NOTE).
The second commit removes astconv::ConvertedBinding{,Kind} in favor of hir::TypeBinding{,Kind}. The former was a — in my opinion — super useless intermediary. As you can tell from the diff, its removal shaves off some code. Furthermore, yeeting it will make it easier to implement the type resolution fixes in rust-lang#119385.
Nothing in this PR should have any semantic effect.
r? @compiler-errors
Addendum as in rust-lang#118668: What I call “associated item bindings” are commonly referred to as “type bindings” for historical reasons. Nowadays, “type bindings” include assoc type bindings, assoc const bindings and RTN (return type notation) which is why I prefer not to use this outdated term.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request
…li-obk,cjgillot
Fix type resolution of associated const equality bounds (take 2)
Instead of trying to re-resolve the type of assoc const bindings inside the type_of query impl in an incomplete manner, transfer the already (correctly) resolved type from add_predicates_for_ast_type_binding to type_of/anon_type_of through query feeding.
Together with rust-lang#118668 (merged) and rust-lang#121258, this supersedes rust-lang#118360. Fixes rust-lang#118040.
r? @ghost
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request
…li-obk,cjgillot
Fix type resolution of associated const equality bounds (take 2)
Instead of trying to re-resolve the type of assoc const bindings inside the type_of query impl in an incomplete manner, transfer the already (correctly) resolved type from add_predicates_for_ast_type_binding to type_of/anon_type_of through query feeding.
Together with rust-lang#118668 (merged) and rust-lang#121258, this supersedes rust-lang#118360. Fixes rust-lang#118040.
r? @ghost
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request
Rollup merge of rust-lang#119385 - fmease:assoc-const-eq-fixes-2, r=oli-obk,cjgillot
Fix type resolution of associated const equality bounds (take 2)
Instead of trying to re-resolve the type of assoc const bindings inside the type_of query impl in an incomplete manner, transfer the already (correctly) resolved type from add_predicates_for_ast_type_binding to type_of/anon_type_of through query feeding.
Together with rust-lang#118668 (merged) and rust-lang#121258, this supersedes rust-lang#118360. Fixes rust-lang#118040.
r? @ghost
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request
…y-generic-tys, r=compiler-errors
Reject overly generic assoc const binding types
Split off from rust-lang#119385 to make rust-lang#119385 easier to review.
In the instantiated type of assoc const bindings
- reject early-bound generic params
- Provide a rich error message instead of ICE'ing (rust-lang#108271).
- This is a temporary and semi-artificial restriction until the arrival of generic const generics.
- It's quite possible that rustc could already perfectly support this subset of generic const generics if we just removed some checks (some
.no_bound_vars().expect(…)) but even if that was the case, I'd rather gate it behind a new feature flag. Reporting an error instead of ICE'ing is a good first step towards an eventual feature gate error.
- reject escaping late-bound generic params
- They lead to ICEs before & I'm pretty sure that they remain incorrect even in a world with generic const generics
Together with rust-lang#118668 & rust-lang#119385, this supersedes rust-lang#118360. Fixes rust-lang#108271.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request
…y-generic-tys, r=compiler-errors
Reject overly generic assoc const binding types
Split off from rust-lang#119385 to make rust-lang#119385 easier to review.
In the instantiated type of assoc const bindings
- reject early-bound generic params
- Provide a rich error message instead of ICE'ing (rust-lang#108271).
- This is a temporary and semi-artificial restriction until the arrival of generic const generics.
- It's quite possible that rustc could already perfectly support this subset of generic const generics if we just removed some checks (some
.no_bound_vars().expect(…)) but even if that was the case, I'd rather gate it behind a new feature flag. Reporting an error instead of ICE'ing is a good first step towards an eventual feature gate error.
- reject escaping late-bound generic params
- They lead to ICEs before & I'm pretty sure that they remain incorrect even in a world with generic const generics
Together with rust-lang#118668 & rust-lang#119385, this supersedes rust-lang#118360. Fixes rust-lang#108271.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request
…y-generic-tys, r=compiler-errors
Reject overly generic assoc const binding types
Split off from rust-lang#119385 to make rust-lang#119385 easier to review.
In the instantiated type of assoc const bindings
- reject early-bound generic params
- Provide a rich error message instead of ICE'ing (rust-lang#108271).
- This is a temporary and semi-artificial restriction until the arrival of generic const generics.
- It's quite possible that rustc could already perfectly support this subset of generic const generics if we just removed some checks (some
.no_bound_vars().expect(…)) but even if that was the case, I'd rather gate it behind a new feature flag. Reporting an error instead of ICE'ing is a good first step towards an eventual feature gate error.
- reject escaping late-bound generic params
- They lead to ICEs before & I'm pretty sure that they remain incorrect even in a world with generic const generics
Together with rust-lang#118668 & rust-lang#119385, this supersedes rust-lang#118360. Fixes rust-lang#108271.
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request
Rollup merge of rust-lang#121258 - fmease:assoc-const-eq-reject-overly-generic-tys, r=compiler-errors
Reject overly generic assoc const binding types
Split off from rust-lang#119385 to make rust-lang#119385 easier to review.
In the instantiated type of assoc const bindings
- reject early-bound generic params
- Provide a rich error message instead of ICE'ing (rust-lang#108271).
- This is a temporary and semi-artificial restriction until the arrival of generic const generics.
- It's quite possible that rustc could already perfectly support this subset of generic const generics if we just removed some checks (some
.no_bound_vars().expect(…)) but even if that was the case, I'd rather gate it behind a new feature flag. Reporting an error instead of ICE'ing is a good first step towards an eventual feature gate error.
- reject escaping late-bound generic params
- They lead to ICEs before & I'm pretty sure that they remain incorrect even in a world with generic const generics
Together with rust-lang#118668 & rust-lang#119385, this supersedes rust-lang#118360. Fixes rust-lang#108271.
Labels
This PR was explicitly merged by bors.
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Relevant to the compiler team, which will review and decide on the PR/issue.