mGCA: Add associated const type check

[zedddie]

#151642

r? BoxyUwU

I didn't bless tests just yet as it only fixes the dyn arm

[BoxyUwU]

Suggested change

	.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.

[zedddie]

Oh I really missed we have the type in scope and can use it -_-, then it will be perfect here

[BoxyUwU]

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

[BoxyUwU]

very nice :)

[BoxyUwU]

Suggested change

	.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?

[zedddie]

yes, and i simplified it a bit further now

[BoxyUwU]

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)

[BoxyUwU]

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 :)

[BoxyUwU]

Looks correct to me :) Just waiting on you to sort out the tests now I think

[zedddie]

in previous variant I didn't really use sp, just passed to the next filter_map, so can be simplified to this

[zedddie]

was not sure on adding this, but i guess this works as regression test for the check logic :"

[BoxyUwU]

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)

[rustbot]

changes to the core type system

cc @lcnr

[BoxyUwU]

@bors r+ rollup

thanks for working on this :3

[rust-bors]

📌 Commit ec03e39 has been approved by BoxyUwU

It is now in the queue for this repository.

[zedddie]

#151642

r? BoxyUwU

I didn't bless tests just yet as it only fixes the dyn arm

[BoxyUwU]

Suggested change

	.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.

[zedddie]

Oh I really missed we have the type in scope and can use it -_-, then it will be perfect here

[BoxyUwU]

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

[BoxyUwU]

very nice :)

[BoxyUwU]

Suggested change

	.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?

[zedddie]

yes, and i simplified it a bit further now

[BoxyUwU]

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)

[BoxyUwU]

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 :)

[BoxyUwU]

Looks correct to me :) Just waiting on you to sort out the tests now I think

[zedddie]

in previous variant I didn't really use sp, just passed to the next filter_map, so can be simplified to this

[zedddie]

was not sure on adding this, but i guess this works as regression test for the check logic :"

[BoxyUwU]

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)

[rustbot]

changes to the core type system

cc @lcnr

[BoxyUwU]

@bors r+ rollup

thanks for working on this :3

[rust-bors]

📌 Commit ec03e39 has been approved by BoxyUwU

It is now in the queue for this repository.

[zedddie]

#151642

r? BoxyUwU

I didn't bless tests just yet as it only fixes the dyn arm

[BoxyUwU]

Suggested change

	.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.

[zedddie]

Oh I really missed we have the type in scope and can use it -_-, then it will be perfect here

[BoxyUwU]

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

[BoxyUwU]

very nice :)

[BoxyUwU]

Suggested change

	.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?

[zedddie]

yes, and i simplified it a bit further now

[BoxyUwU]

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)

[BoxyUwU]

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 :)

[BoxyUwU]

Looks correct to me :) Just waiting on you to sort out the tests now I think

[zedddie]

in previous variant I didn't really use sp, just passed to the next filter_map, so can be simplified to this

[zedddie]

was not sure on adding this, but i guess this works as regression test for the check logic :"

[BoxyUwU]

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)

[rustbot]

changes to the core type system

cc @lcnr

[BoxyUwU]

@bors r+ rollup

thanks for working on this :3

[rust-bors]

📌 Commit ec03e39 has been approved by BoxyUwU

It is now in the queue for this repository.

[zedddie]

#151642

r? BoxyUwU

I didn't bless tests just yet as it only fixes the dyn arm

[BoxyUwU]

Suggested change

	.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.

[zedddie]

Oh I really missed we have the type in scope and can use it -_-, then it will be perfect here

[BoxyUwU]

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

[BoxyUwU]

very nice :)

[BoxyUwU]

Suggested change

	.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?

[zedddie]

yes, and i simplified it a bit further now

[BoxyUwU]

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)

[BoxyUwU]

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 :)

[BoxyUwU]

Looks correct to me :) Just waiting on you to sort out the tests now I think

[zedddie]

in previous variant I didn't really use sp, just passed to the next filter_map, so can be simplified to this

[zedddie]

was not sure on adding this, but i guess this works as regression test for the check logic :"

[BoxyUwU]

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)

[rustbot]

changes to the core type system

cc @lcnr

[BoxyUwU]

@bors r+ rollup

thanks for working on this :3

[rust-bors]

📌 Commit ec03e39 has been approved by BoxyUwU

It is now in the queue for this repository.

[zedddie]

#151642

r? BoxyUwU

I didn't bless tests just yet as it only fixes the dyn arm

[BoxyUwU]

Suggested change

	.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.

[zedddie]

Oh I really missed we have the type in scope and can use it -_-, then it will be perfect here

[BoxyUwU]

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

[BoxyUwU]

very nice :)

[BoxyUwU]

Suggested change

	.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?

[zedddie]

yes, and i simplified it a bit further now

[BoxyUwU]

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)

[BoxyUwU]

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 :)

[BoxyUwU]

Looks correct to me :) Just waiting on you to sort out the tests now I think

[zedddie]

in previous variant I didn't really use sp, just passed to the next filter_map, so can be simplified to this

[zedddie]

was not sure on adding this, but i guess this works as regression test for the check logic :"

[BoxyUwU]

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)

[rustbot]

changes to the core type system

cc @lcnr

[BoxyUwU]

@bors r+ rollup

thanks for working on this :3

[rust-bors]

📌 Commit ec03e39 has been approved by BoxyUwU

It is now in the queue for this repository.

[zedddie]

#151642

r? BoxyUwU

I didn't bless tests just yet as it only fixes the dyn arm

[BoxyUwU]

Suggested change

	.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.

[zedddie]

Oh I really missed we have the type in scope and can use it -_-, then it will be perfect here

[BoxyUwU]

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

[BoxyUwU]

very nice :)

[BoxyUwU]

Suggested change

	.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?

[zedddie]

yes, and i simplified it a bit further now

[BoxyUwU]

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)

[BoxyUwU]

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 :)

[BoxyUwU]

Looks correct to me :) Just waiting on you to sort out the tests now I think

[zedddie]

in previous variant I didn't really use sp, just passed to the next filter_map, so can be simplified to this

[zedddie]

was not sure on adding this, but i guess this works as regression test for the check logic :"

[BoxyUwU]

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)

[rustbot]

changes to the core type system

cc @lcnr

[BoxyUwU]

@bors r+ rollup

thanks for working on this :3

[rust-bors]

📌 Commit ec03e39 has been approved by BoxyUwU

It is now in the queue for this repository.

[zedddie]

#151642

r? BoxyUwU

I didn't bless tests just yet as it only fixes the dyn arm

[BoxyUwU]

Suggested change

	.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.

[zedddie]

Oh I really missed we have the type in scope and can use it -_-, then it will be perfect here

[BoxyUwU]

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

[BoxyUwU]

very nice :)

[BoxyUwU]

Suggested change

	.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?

[zedddie]

yes, and i simplified it a bit further now

[BoxyUwU]

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)

[BoxyUwU]

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 :)

[BoxyUwU]

Looks correct to me :) Just waiting on you to sort out the tests now I think

[zedddie]

in previous variant I didn't really use sp, just passed to the next filter_map, so can be simplified to this

[zedddie]

was not sure on adding this, but i guess this works as regression test for the check logic :"

[BoxyUwU]

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)

[rustbot]

changes to the core type system

cc @lcnr

[BoxyUwU]

@bors r+ rollup

thanks for working on this :3

[rust-bors]

📌 Commit ec03e39 has been approved by BoxyUwU

It is now in the queue for this repository.

[zedddie]

#151642

r? BoxyUwU

I didn't bless tests just yet as it only fixes the dyn arm

[BoxyUwU]

Suggested change

	.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.

[zedddie]

Oh I really missed we have the type in scope and can use it -_-, then it will be perfect here

[BoxyUwU]

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

[BoxyUwU]

very nice :)

[BoxyUwU]

Suggested change

	.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?

[zedddie]

yes, and i simplified it a bit further now

[BoxyUwU]

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)

[BoxyUwU]

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 :)

[BoxyUwU]

Looks correct to me :) Just waiting on you to sort out the tests now I think

[zedddie]

in previous variant I didn't really use sp, just passed to the next filter_map, so can be simplified to this

[zedddie]

was not sure on adding this, but i guess this works as regression test for the check logic :"

[BoxyUwU]

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)

[rustbot]

changes to the core type system

cc @lcnr

[BoxyUwU]

@bors r+ rollup

thanks for working on this :3

[rust-bors]

📌 Commit ec03e39 has been approved by BoxyUwU

It is now in the queue for this repository.