Add supertrait item shadowing for type-level path resolution#152225
Conversation
|
HIR ty lowering was modified cc @fmease |
rustbot
added
S-waiting-on-review
|
r? @madsmtm rustbot has assigned @madsmtm. Use Why was this reviewer chosen?The reviewer was selected based on:
|
| println!("{}", size_of::<T::T>()); | ||
| } | ||
|
|
||
| fn generic2<T: C<T = i16>>() { |
There was a problem hiding this comment.
What about T: B<T = i16> and T: A<T = i8> - do those resolve to B::T and A::T respectively?
Also, please use a different name for the generic and the associated type, it's a bit confusing as-is.
There was a problem hiding this comment.
I've cleaned up the example and added tests showing how B<T = i16> and A<T = i8> are resolved.
|
r? types |
rustbot
added
the
T-types
traviscross
added
the
I-lang-radar
Wait, I'm not sure if that should've unassigned me or not? In any case, if you also need a compiler reviewer, I'm the wrong one for the job, so I'm gonna unassign myself. |
Amanieu
force-pushed
the
type-supertrait-shadowing
branch
from
1cde4c7 to
5a9ef14
Compare
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
In general, I think this needs some more tests. I've already commented about adding the testing attr, but ideally we should go through the existing tests/ui/methods/supertrait-shadowing tests (which in general are pretty thorough) and write the equivalent test for associated types and consts. These cover cases like multiple ancestors, unsatisfied trait bounds, scoping, and (in a PR I'm about to make) stability.
I also would like to see test that show that <U as A>::T works just fine, even if you have U: B.
| use std::mem::size_of; | ||
|
|
||
| trait A { | ||
| type T; |
There was a problem hiding this comment.
Please use a different name for the associated type to name lexically clash with the param.
| println!("{}", size_of::<U::T>()); | ||
| } | ||
|
|
||
| fn generic3<U: B<T = i16>>() { |
There was a problem hiding this comment.
What is missing here is a test of U: B<T = i8> and U: B, just to show the former is not callable (because of the blanket impl), and the latter still uses <U as B>::T. (Similar for C below)
| trait C: B {} | ||
| impl<T> C for T {} | ||
|
|
||
| fn main() { |
There was a problem hiding this comment.
We have rustc_dump_predicates, which we could use to verify the predicates are getting lowered as we expect. It would be good to use that in this (or another) test.
There was a problem hiding this comment.
So, this test probably doesn't make sense here, right? This is not about method resolution.
There was a problem hiding this comment.
Finally getting back around to this! Some questions about your preferences for the test organization.
Would you prefer moving all supertrait-shadowing tests into their own directory (tests/ui/supertrait-shadowing) or would you prefer adding a separate directory which just contains tests for type-level shadowing?
In the former case, would you like type-level shadowing tests to be in separate files or in the same file as the corresponding method-level test?
There was a problem hiding this comment.
No strong preference. I think a separate directory is good. Putting them in the same file also seems reasonable.
| /// supertrait of another candidate trait. | ||
| /// | ||
| /// This implements RFC #3624. | ||
| fn collapse_candidates_to_subtrait_pick( |
There was a problem hiding this comment.
I think docs here and for collapse_candidates_to_subtrait_pick in method::probe should link to each other to help ensure they are kept up to date.
It's kind of unfortunate we can't have some shared code here.
| continue; | ||
| } | ||
|
|
||
| // This pick is not a supertrait of the `child_pick`. |
There was a problem hiding this comment.
I have a commit with some cleanup that I'm about to push - but child_pick doesn't exist. Should be child_trait. Here and below.
rustbot
added
S-waiting-on-author
|
What's the current state of this PR? AIUI, this is the remaining blocker for stabilizing supertrait item shadowing. |
|
I think this is waiting on @Amanieu to address the review. |
This comment has been minimized.
This comment has been minimized.
|
I've addressed the review feedback. I couldn't adapt the following tests because the corresponding functionality doesn't exist in type-level name resolution, and always results in an ambiguity error:
|
Amanieu
force-pushed
the
type-supertrait-shadowing
branch
from
5a9ef14 to
9407a97
Compare
This comment has been minimized.
This comment has been minimized.
@Amanieu hmm for Generally, it's also interesting to me (and I must have missed this when I was going through) that Given the above, I'm wondering if may we want to limit that resolution even for the method case right now. Thoughts? |
|
For //@ run-pass
// Make sure we don't prefer a subtrait that we would've otherwise eliminated
// in `consider_probe` during method probing.
use core::mem::size_of;
#![allow(dead_code)]
struct W<T>(T);
trait Upstream {
fn hello(&self) -> &'static str {
"upstream"
}
type Assoc = i8;
}
impl<T> Upstream for T {}
trait Downstream: Upstream {
fn hello(&self) -> &'static str {
"downstream"
}
type Assoc = i16;
}
impl<T> Downstream for W<T> where T: Foo {}
trait Foo {}
fn main() {
let x = W(1i32);
assert_eq!(x.hello(), "upstream");
// This doesn't work: accessing type-level associated items requires
// specifying the trait name explicitly with <W<i32> as Upstream> or
// <W<i32> as Downstream>. But then that overrides supertrait shadowing
// anyways...
assert_eq!(size_of::<W<i32>::Assoc>(), 1);
// This works but always resolves to the supertrait because that's what's
// in the bounds of check1.
check1::<W<i32>>();
// This fails because W<i32> doesn't implement Downstream
check2::<W<i32>>();
}
fn check1<T: Upstream>() {
assert_eq!(size_of::<T::Assoc>(), 1);
}
fn check2<T: Downstream>() {
assert_eq!(size_of::<T::Assoc>(), 2);
}Similar issue with |
That seems difficult to do in the current code structure, which I'm not super familiar with. Additionally, this is really an edge case: you would need to be intentionally shadowing a supertrait item while also having different trait bounds on the subtrait. |
Which feature are you talking about specifically? I'm not sure which of my comments you're replying to. |
|
@bors r+ |
This comment has been minimized.
This comment has been minimized.
Amanieu
force-pushed
the
type-supertrait-shadowing
branch
from
1ab6862 to
af3eaee
Compare
|
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. |
|
A job failed! Check out the build log: (web) (plain enhanced) (plain) Click to see the possible cause of the failure (guessed by this bot) |
This comment has been minimized.
This comment has been minimized.
Add supertrait item shadowing for type-level path resolution try-job: x86_64-gnu-llvm-22-3
|
💔 Test for a559af7 failed: CI. Failed job:
|
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
This is a spurious issue, not the fault of this PR, see #158500 |
|
Spurious issue fixed by github.com//pull/158502 |
rust-bors
Bot
added
S-waiting-on-bors
… r=jackh726 Add supertrait item shadowing for type-level path resolution This makes type-level name resolution (used primarily for associated types) also prefer subtrait items to supertrait items in case of ambiguity. This addresses the concern from rust-lang#148605 about the inconsistency with name resolution in expressions and method calls.
…uwer Rollup of 10 pull requests Successful merges: - #158502 (Revert "LLVM 23: Adapt codegen test to moved assume") - #152225 (Add supertrait item shadowing for type-level path resolution) - #158194 (Adds RmetaLinkCache a per-link cache that uses path as the key of dec…) - #158466 (rustdoc: show impl Trait<Box<Local>> for Foreign, etc on Local's docs) - #158501 (miri subtree update) - #153097 (Expand `OptionFlatten`'s iterator methods) - #158163 (Fix too-short variance slice in `variances_of` cycle recovery) - #158233 (Allow the unstable attribute on foreign type) - #158470 (Upgrade `jsonsocck` and `jsondoclint` to edition 2024.) - #158488 (Upgrade `rustdoc-json-types` to 2024 edition.)
…uwer Rollup of 10 pull requests Successful merges: - #158502 (Revert "LLVM 23: Adapt codegen test to moved assume") - #152225 (Add supertrait item shadowing for type-level path resolution) - #158194 (Adds RmetaLinkCache a per-link cache that uses path as the key of dec…) - #158466 (rustdoc: show impl Trait<Box<Local>> for Foreign, etc on Local's docs) - #158501 (miri subtree update) - #153097 (Expand `OptionFlatten`'s iterator methods) - #158163 (Fix too-short variance slice in `variances_of` cycle recovery) - #158233 (Allow the unstable attribute on foreign type) - #158470 (Upgrade `jsonsocck` and `jsondoclint` to edition 2024.) - #158488 (Upgrade `rustdoc-json-types` to 2024 edition.)
…uwer Rollup of 10 pull requests Successful merges: - #158502 (Revert "LLVM 23: Adapt codegen test to moved assume") - #152225 (Add supertrait item shadowing for type-level path resolution) - #158194 (Adds RmetaLinkCache a per-link cache that uses path as the key of dec…) - #158466 (rustdoc: show impl Trait<Box<Local>> for Foreign, etc on Local's docs) - #158501 (miri subtree update) - #153097 (Expand `OptionFlatten`'s iterator methods) - #158163 (Fix too-short variance slice in `variances_of` cycle recovery) - #158233 (Allow the unstable attribute on foreign type) - #158470 (Upgrade `jsonsocck` and `jsondoclint` to edition 2024.) - #158488 (Upgrade `rustdoc-json-types` to 2024 edition.)
…uwer Rollup of 10 pull requests Successful merges: - #158502 (Revert "LLVM 23: Adapt codegen test to moved assume") - #152225 (Add supertrait item shadowing for type-level path resolution) - #158194 (Adds RmetaLinkCache a per-link cache that uses path as the key of dec…) - #158466 (rustdoc: show impl Trait<Box<Local>> for Foreign, etc on Local's docs) - #158501 (miri subtree update) - #153097 (Expand `OptionFlatten`'s iterator methods) - #158163 (Fix too-short variance slice in `variances_of` cycle recovery) - #158233 (Allow the unstable attribute on foreign type) - #158470 (Upgrade `jsonsocck` and `jsondoclint` to edition 2024.) - #158488 (Upgrade `rustdoc-json-types` to 2024 edition.)
… r=jackh726 Add supertrait item shadowing for type-level path resolution This makes type-level name resolution (used primarily for associated types) also prefer subtrait items to supertrait items in case of ambiguity. This addresses the concern from rust-lang#148605 about the inconsistency with name resolution in expressions and method calls.
Rollup of 15 pull requests Successful merges: - #158497 (stdarch subtree update) - #152225 (Add supertrait item shadowing for type-level path resolution) - #158194 (Adds RmetaLinkCache a per-link cache that uses path as the key of dec…) - #158466 (rustdoc: show impl Trait<Box<Local>> for Foreign, etc on Local's docs) - #158501 (miri subtree update) - #153097 (Expand `OptionFlatten`'s iterator methods) - #157614 (Move tests drop) - #157996 (perf: drop the full-crate AST walk in check_unused) - #158163 (Fix too-short variance slice in `variances_of` cycle recovery) - #158233 (Allow the unstable attribute on foreign type) - #158433 (Fix inconsistent safety requirement in VecDeque::nonoverlapping_ranges) - #158464 (Reorganize `tests/ui/issues` [15/N]) - #158470 (Upgrade `jsonsocck` and `jsondoclint` to edition 2024.) - #158485 (Reorganize `tests/ui/issues` [16/N]) - #158488 (Upgrade `rustdoc-json-types` to 2024 edition.)
Rollup merge of #152225 - Amanieu:type-supertrait-shadowing, r=jackh726 Add supertrait item shadowing for type-level path resolution This makes type-level name resolution (used primarily for associated types) also prefer subtrait items to supertrait items in case of ambiguity. This addresses the concern from #148605 about the inconsistency with name resolution in expressions and method calls.
Rollup of 15 pull requests Successful merges: - rust-lang/rust#158497 (stdarch subtree update) - rust-lang/rust#152225 (Add supertrait item shadowing for type-level path resolution) - rust-lang/rust#158194 (Adds RmetaLinkCache a per-link cache that uses path as the key of dec…) - rust-lang/rust#158466 (rustdoc: show impl Trait<Box<Local>> for Foreign, etc on Local's docs) - rust-lang/rust#158501 (miri subtree update) - rust-lang/rust#153097 (Expand `OptionFlatten`'s iterator methods) - rust-lang/rust#157614 (Move tests drop) - rust-lang/rust#157996 (perf: drop the full-crate AST walk in check_unused) - rust-lang/rust#158163 (Fix too-short variance slice in `variances_of` cycle recovery) - rust-lang/rust#158233 (Allow the unstable attribute on foreign type) - rust-lang/rust#158433 (Fix inconsistent safety requirement in VecDeque::nonoverlapping_ranges) - rust-lang/rust#158464 (Reorganize `tests/ui/issues` [15/N]) - rust-lang/rust#158470 (Upgrade `jsonsocck` and `jsondoclint` to edition 2024.) - rust-lang/rust#158485 (Reorganize `tests/ui/issues` [16/N]) - rust-lang/rust#158488 (Upgrade `rustdoc-json-types` to 2024 edition.)
9 participants
View all comments
This makes type-level name resolution (used primarily for associated types) also prefer subtrait items to supertrait items in case of ambiguity.
This addresses the concern from #148605 about the inconsistency with name resolution in expressions and method calls.