-Znext-solver Allow method calls on chains of assoc types of not-yet defined opaque types - #161414
-Znext-solver Allow method calls on chains of assoc types of not-yet defined opaque types#161414ShoyuVanilla wants to merge 8 commits into
-Znext-solver Allow method calls on chains of assoc types of not-yet defined opaque types#161414Conversation
|
|
||
| pub type PredefinedOpaques<'tcx> = &'tcx ty::List<(ty::OpaqueTypeKey<'tcx>, Ty<'tcx>)>; | ||
| pub type HiddenTypesOfOpaques<'tcx> = | ||
| &'tcx ty::List<(Ty<'tcx>, Option<ty::OpaqueHiddenTyBound<'tcx>>)>; |
There was a problem hiding this comment.
This is the flattend version of { (?x, [p1, p2]), (?y, []) } into [(?x, Some(p1)), (?x, Some(p2)), (?y, None) ], to make it slice-internable.
I'm not sure whether we should use a single representation 🤔
And I'm not sure whether we really need the empty bounds, as we still have <?x: MetaSized> bounds for impl ?Sized (would we have some real empty one someday?)
This comment has been minimized.
This comment has been minimized.
67742fd to
a99368e
Compare
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
`-Znext-solver` Allow method calls on (recursive) assoc types of not-yet defined opaque types
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (9b6316b): comparison URL. Overall result: ❌ regressions - please read:Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf. Next, please: If you can, justify the regressions found in this try perf run in writing along with @bors rollup=never rustc-perf Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary -1.1%, secondary 1.8%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary 6.5%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary -0.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 456.256s -> 457.797s (0.34%) |
| self.cx | ||
| } | ||
|
|
||
| fn fold_ty(&mut self, ty: I::Ty) -> I::Ty { |
There was a problem hiding this comment.
this needs to fold_binder to because we need to shift the index of the anon-bound inwards. Otherwise, use could use https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/ty/enum.BoundVarIndexKind.html#variant.Canonical and change this to BoundVarIndexKind::UniqueOuterScope
There was a problem hiding this comment.
The former sounds more preferable, as the later might not work well with the canonicalization 😄
| opaque_types: usize, | ||
| duplicate_entries: usize, | ||
| hidden_types_of_opaques: FxIndexMap<Ty<'tcx>, usize>, |
There was a problem hiding this comment.
that seems quite expensive 🤔
I guess this and more importantly, canonicalization of opaque types now has to also canonicalize hidden_types_of_opaques
There was a problem hiding this comment.
Do you mean the canonicalizations in the rustc_infer? (as the canonicaliztions in the solver handles them)
There was a problem hiding this comment.
For most cases, hidden_types_of_opaques doesn't grow too large by now, so it might make sense to just flatten this in the storage and track the whole len() for it
|
I feel like one big change is to change the way we handle opaque types in inputs and external constraints in the response outside of this PR. The fact that we intern and then canonicalize and then reintern seems clearly bad 🤣 You have a FIXME in the code somewhere for that already :> |
Yeah, I really should refactor and optimize them. Currently I'm trying to make things correct wrt the above my comment ( |
| assert!(delegate.clone_opaque_types_lookup_table().is_empty()); | ||
| } | ||
|
|
||
| for chunk in input.hidden_types_of_opaques_in_body.as_slice().chunk_by(|a, b| a.0 == b.0) { |
There was a problem hiding this comment.
why do we flatten the bounds outside the method_autoderef query and gather them by hidden type here again?
There was a problem hiding this comment.
It's kinda mess how I pass hidden_types_of_opaques in this PR yet, basically in two ways: flattened vec and the nested one keyed by hidden type.
I gathered them by the hidden type here just to prevent pushing to many undo logs. I'll fix the hidden_types_of_opaques eventually and I hope it would be look much nicer by then 😅
| let self_ty_is_opaque = |ty: Ty<'_>| { | ||
| if let &ty::Infer(ty::TyVar(vid)) = ty.kind() { | ||
| infcx.has_opaques_with_sub_unified_hidden_type(vid) | ||
| infcx.has_hidden_types_of_opaques_modulo_sub_unification(vid) |
There was a problem hiding this comment.
This might be obvious but I'm struggling to understand why we can detect the projection term via hidden infer var. 😢
Issue 248 is about <hidden as Trait>::Projection: ProjectionItemBound. The self_ty in method lookup is an fresh infer var from normalizing <hidden as Trait>::Projection, I presume? So it's not the hidden infer var. However, item bounds we gather from the projection item are keyed by hidden 🤔
There was a problem hiding this comment.
The
self_tyin method lookup is an fresh infer var from normalizing<hidden as Trait>::Projection, I presume?
Yes, it is. So has_hidden_types_of_opaques_modulo_sub_unification checks both cases for ?fresh_infer_var_for_opaque and ?fresh_infer_var_for_possibly_multiple_projection_on_opaque), the key for those bounds.
It is necessary to check the later as we have the cases with <<{opaque} as TraitA>::Assoc> as TraitB>::Assoc like the following, which is compiled with the old solver (and this PR) but not with the next-solver:
trait Foo {
fn foo(&self) {}
}
trait Bar {
type Assoc: Foo;
fn bar(&self) -> Self::Assoc {
loop {}
}
}
trait Baz {
type Assoc: Bar;
fn baz(&self) -> Self::Assoc {
loop {}
}
}
impl Foo for () {}
impl Bar for () {
type Assoc = ();
}
impl Baz for () {
type Assoc = ();
}
fn heck() -> impl Baz {
heck().baz().bar().foo()
}
fn main() {}There was a problem hiding this comment.
So
has_hidden_types_of_opaques_modulo_sub_unificationchecks both cases for ?fresh_infer_var_for_opaque and ?fresh_infer_var_for_possibly_multiple_projection_on_opaque), the key for those bounds.
In the method body, has_hidden_types_of_opaques_modulo_sub_unification only checks the keys of hidden_types_of_opaques so you mean those keys also contains fresh infer vars from normalizing projections 🤔
But I don't find where we add bounds for these fresh infer vars? They're all for hidden infers of opaques?
There was a problem hiding this comment.
There was a problem hiding this comment.
Finally got it. Thank you for the explanation!
We could use better naming for these things now that they handle more than hidden types of opaques?
Ofc we should fix correctness issues first :>
There was a problem hiding this comment.
Yeah, I really should fix those namings 😄 I hope most of the correctness things(except folding binders and some canonicalization things) might be fixed by now so I'll try the perf and the naming/comments sides soon
a99368e to
dba8782
Compare
| // FIXME: Explain this hack. Why this is needed and why should be done here | ||
| // FIXME: Maybe we need probing for whole this call as the following lines | ||
| // directly add `hidden_types_of_opaques` to the context without probing or | ||
| // instantiating the response. |
There was a problem hiding this comment.
Hmm, thinking twice, this feel might be okay to me, as the num_entries are taken from the delegate before the canonicalization of the goal and compared to the evaluation result for both function-like evaluation of normalized-to and the outer projection goal
This comment has been minimized.
This comment has been minimized.
|
Hmm, the CI failure looks unfortunate fn features_ok1() -> impl Iterator<Item = ()> {
None.into_iter()
}
fn features_ok2<'a>() -> impl Iterator<Item = &'a ()> {
None.into_iter()
}
// This emits an error :<
fn features_err() -> impl Iterator<Item = &'static ()> {
None.into_iter()
}
So we have two different So, basically I'm fixing such infinite progress with no fruitful result at all over multiple times 😅 I think I would be able to fix this one as well. Edit) Fixed by checking whether the instantiation of the response actually changes the |
-Znext-solver Allow method calls on (recursive) assoc types of not-yet defined opaque types-Znext-solver Allow method calls on chains of assoc types of not-yet defined opaque types
|
I'd like to do a perf run before trying some optimizations @bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
`-Znext-solver` Allow method calls on chains of assoc types of not-yet defined opaque types
99f86af to
81691be
Compare
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (3d55cff): comparison URL. Overall result: ❌✅ regressions and improvements - please read:Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf. Next, please: If you can, justify the regressions found in this try perf run in writing along with @bors rollup=never rustc-perf Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary -0.4%, secondary 1.8%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary 5.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis perf run didn't have relevant results for this metric. Bootstrap: 475.013s -> 478.006s (0.63%) |
This comment has been minimized.
This comment has been minimized.
…yet defined opaque types
81691be to
cf9f807
Compare
|
I'd like to do a perf run before trying some optimizations, again 😅 @bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
`-Znext-solver` Allow method calls on chains of assoc types of not-yet defined opaque types
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (3f30747): comparison URL. Overall result: ❌✅ regressions and improvements - please read:Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf. Next, please: If you can, justify the regressions found in this try perf run in writing along with @bors rollup=never rustc-perf Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (secondary -2.8%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary -2.5%, secondary 4.4%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis perf run didn't have relevant results for this metric. Bootstrap: 478.423s -> 479.616s (0.25%) |
View all comments
Fixes rust-lang/trait-system-refactor-initiative#248
Still lacks of tons of comments and some tests (such as recursive projections or assoc item of supertraits) 😅
And it have some temporary
FIXMEcomments as I'm leaving for a vacation until next Monday and will fix them as soon as I canr? lcnr