-
-
Notifications
You must be signed in to change notification settings - Fork 14.5k
mGCA: Add associated const type check #152146
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 and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
This comment has been minimized.
This comment has been minimized.
|
|
||
| for projection in data.projection_bounds() { | ||
| let pred_binder = projection | ||
| .with_self_ty(tcx, tcx.types.trait_object_dummy_self) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| .with_self_ty(tcx, tcx.types.trait_object_dummy_self) | |
| .with_self_ty(tcx, t) |
I expect using trait_object_dummy_self might lead to code on the error-path crashing so we should use the "actual" self type. It also might just lead to weird diagnostics, such as Infer(FreshTy(0)) appearing in user facing text which we don't want.
Though eventually we'll also support assocaited constants such as const ASSOC: <Self as Other>::Assoc at which point Self will need to be a real type that implements Trait so that we can figure out the type of Assoc.
What's going on here is that we have the type dyn Trait<T, ASSOC = 10> (for example), and we want to construct [dyn Trait<T, ASSOC = 10>, T] as a list of generic arguments to <T as Trait<U>>::ASSOC for when we look at the type of ASSOC.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I really missed we have the type in scope and can use it -_-, then it will be perfect here
|
|
||
| trait Trait { #[type_const] const CT: bool; } | ||
|
|
||
| // FIXME: this should yield a type mismatch (`bool` v `i32`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will get fixed once you add wf checks for T: Trait<ASSOC = N> where clauses.
You can do this by adding fairly similar logic to what you've already done, but in rustc_hir_analysis/src/check/wfcheck.rs in the check_where_clause function
You should be able to similarly iterate over all of the predicates and filter to only those that are associated const bindings
|
very nice :) |
93d0247 to
b2f6570
Compare
This comment has been minimized.
This comment has been minimized.
b2f6570 to
c290611
Compare
This comment has been minimized.
This comment has been minimized.
| .filter_map(|(clause, sp)| clause.as_projection_clause().map(|proj| (proj, sp))) | ||
| .filter_map(|(proj, sp)| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| .filter_map(|(clause, sp)| clause.as_projection_clause().map(|proj| (proj, sp))) | |
| .filter_map(|(proj, sp)| { | |
| .filter_map(|(clause, sp)| { | |
| let proj = clause.as_projection_clause().map(|proj| (proj, sp)))?; |
does this work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, and i simplified it a bit further now
| } | ||
|
|
||
| for projection in data.projection_bounds() { | ||
| if !t.has_escaping_bound_vars() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you move this out of the for loop? I think it'd read nicer as we don't care about anything to do with wf of the trait object if it has bound vars, whereas currently it sort of reads like this check is dependent on information about each project (which is not true)
| }) | ||
| .transpose(); | ||
| pred_binder.map(|pred_binder| { | ||
| let pred: ty::Predicate<'tcx> = pred_binder.upcast(tcx); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Obligation::new accepts arbitrary T that implements Upcast you might be able to just do Obligation::new(... pred_binder) but I don't know for sure :)
|
Looks correct to me :) Just waiting on you to sort out the tests now I think |
c290611 to
f5d85b4
Compare
| .copied() | ||
| .zip(predicates.spans.iter().copied()) | ||
| .filter_map(|(clause, sp)| { | ||
| let proj = clause.as_projection_clause()?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in previous variant I didn't really use sp, just passed to the next filter_map, so can be simplified to this
| @@ -0,0 +1,11 @@ | |||
| //! Check associated const binding with escaping bound vars doesn't cause ICE | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
was not sure on adding this, but i guess this works as regression test for the check logic :"
|
I think this looks good to me, you can undraft and I'll approve this after #152324 has landed (though you'll have to rebase after that to handle the merge conflicts) |
This comment has been minimized.
This comment has been minimized.
f5d85b4 to
f670e0a
Compare
|
changes to the core type system cc @lcnr |
This comment has been minimized.
This comment has been minimized.
f670e0a to
ec03e39
Compare
|
@bors r+ rollup thanks for working on this :3 |
…s-wfck, r=BoxyUwU mGCA: Add associated const type check rust-lang#151642 r? BoxyUwU I didn't bless tests just yet as it only fixes the dyn arm
…s-wfck, r=BoxyUwU mGCA: Add associated const type check rust-lang#151642 r? BoxyUwU I didn't bless tests just yet as it only fixes the dyn arm
…uwer Rollup of 13 pull requests Successful merges: - #149937 (try enabling `linker-messages` by default again) - #151733 (Use function shims to make sure EII works on apple targets) - #152120 (Don't ICE on layout error in vtable computation) - #152419 (Move more query system code) - #152431 (Restrict the set of things that const stability can be applied to) - #152436 (Reenable a GCI+mGCA+GCPT test case) - #151142 (Support ADT types in type info reflection) - #152021 (Bump tvOS, visionOS and watchOS Aarch64 targets to tier 2) - #152146 (mGCA: Add associated const type check) - #152372 (style: remove unneeded trailing commas) - #152383 (BikeshedGuaranteedNoDrop trait: add comments indicating that it can be observed on stable) - #152397 (Update books) - #152441 (Fix typos and grammar in top-level and src/doc documentation)
…s-wfck, r=BoxyUwU mGCA: Add associated const type check rust-lang#151642 r? BoxyUwU I didn't bless tests just yet as it only fixes the dyn arm
#151642
r? BoxyUwU
I didn't bless tests just yet as it only fixes the dyn arm