Checking the type of const parameters #44
Labels
A-param-types
Const parameter types
C-design-docs
Category: This is part of our design documentation
K-impl
Document Kind: regarding implementation
P-optional
Priority: not strictly required
S-active
Comments
lcnr
added
C-design-docs
bors
added a commit
to rust-lang-ci/rust
that referenced
this issue
Jun 4, 2024
Remove the `ty` field from type system `Const`s Part of the work on `adt_const_params`/`generic_const_param_types`/`min_generic_const_exprs`/generally making the compiler nicer. cc rust-lang/project-const-generics#44 Please review commit-by-commit otherwise I wasted a lot of time not just squashing this into a giant mess (and also it'll be SO much nicer because theres a lot of fluff changes mixed in with other more careful changes if looking via File Changes) This is based on top of rust-lang#125967 and rust-lang#125968, and as such should not be merged until those are merged. --- Why do this? - The `ty` field keeps causing ICEs and weird behaviour due to it either being treated as "part of the const" or it being forgotten about leading to ICEs. - As we move forward with `adt_const_params` and a potential `min_generic_const_exprs` it's going to become more complex to actually lower the correct `Ty<'tcx>` - It muddles the idea behind how we check `Const` arguments have the correct type. By having the `ty` field it may seem like we ought to be relating it when we relate two types, or that its generally important information about the `Const`. - Brings the compiler more in line with `a-mir-formality` as that also tracks the type of type system `Const`s via `ConstArgHasType` bounds in the env instead of on the `Const` itself. - A lot of stuff is a lot nicer when you dont have to pass around the type of a const lol. Everywhere we construct `Const` is now significantly nicer 😅 See rust-lang#125671's description for some more information about the `ty` field --- General summary of changes in this PR: - Change how we represent `ty::Expr` from `generic_const_exprs` to not explode the size of `Const` in this PR (This doesn't really matter, I just didn't want to Completely break generic const exprs just yet) - Add `Ty` to `ConstKind::Value` as otherwise there is no way to implement `ConstArgHasType` to ensure that const arguments are correctly typed for the parameter when we stop creating anon consts for all const args. It's also just incredibly difficult/annoying to thread the correct `Ty` around to a bunch of ctfe functions otherwise. - Fully implement `ConstArgHasType` in both the old and new solver. Since it now has no reliance on the `ty` field it serves its originally intended purpose of being able to act as a double check that trait vs impls have correctly typed const parameters. It also will now be able to be responsible for checking types of const arguments to parameters under `min_generic_const_exprs`. - Add `Ty` to `mir::Const::Ty`. I dont have a great understanding of why mir constants are setup like this to be honest. Regardless they need to be able to determine the type of the const and the easiest way to make this happen was to simply store the `Ty` along side the `ty::Const`. Maybe we can do better here in the future but I'd have to spend way more time looking at everywhere we use `mir::Const`. - SMIR represents mir consts and ty consts using the same `Const` type. This... does not work at all anymore so I have split them into two different types in SMIR and it seems to make everything a lot nicer regardless of this PR. - rustdoc has its own `Const` which also has a `ty` field. It was relatively easy to remove this. --- r? `@lcnr` `@compiler-errors`
bors
added a commit
to rust-lang-ci/rust
that referenced
this issue
Jun 4, 2024
…InTheVoid Remove the `ty` field from type system `Const`s Part of the work on `adt_const_params`/`generic_const_param_types`/`min_generic_const_exprs`/generally making the compiler nicer. cc rust-lang/project-const-generics#44 Please review commit-by-commit otherwise I wasted a lot of time not just squashing this into a giant mess (and also it'll be SO much nicer because theres a lot of fluff changes mixed in with other more careful changes if looking via File Changes) This is based on top of rust-lang#125967 and rust-lang#125968, and as such should not be merged until those are merged. --- Why do this? - The `ty` field keeps causing ICEs and weird behaviour due to it either being treated as "part of the const" or it being forgotten about leading to ICEs. - As we move forward with `adt_const_params` and a potential `min_generic_const_exprs` it's going to become more complex to actually lower the correct `Ty<'tcx>` - It muddles the idea behind how we check `Const` arguments have the correct type. By having the `ty` field it may seem like we ought to be relating it when we relate two types, or that its generally important information about the `Const`. - Brings the compiler more in line with `a-mir-formality` as that also tracks the type of type system `Const`s via `ConstArgHasType` bounds in the env instead of on the `Const` itself. - A lot of stuff is a lot nicer when you dont have to pass around the type of a const lol. Everywhere we construct `Const` is now significantly nicer 😅 See rust-lang#125671's description for some more information about the `ty` field --- General summary of changes in this PR: - Change how we represent `ty::Expr` from `generic_const_exprs` to not explode the size of `Const` in this PR (This doesn't really matter, I just didn't want to Completely break generic const exprs just yet) - Add `Ty` to `ConstKind::Value` as otherwise there is no way to implement `ConstArgHasType` to ensure that const arguments are correctly typed for the parameter when we stop creating anon consts for all const args. It's also just incredibly difficult/annoying to thread the correct `Ty` around to a bunch of ctfe functions otherwise. - Fully implement `ConstArgHasType` in both the old and new solver. Since it now has no reliance on the `ty` field it serves its originally intended purpose of being able to act as a double check that trait vs impls have correctly typed const parameters. It also will now be able to be responsible for checking types of const arguments to parameters under `min_generic_const_exprs`. - Add `Ty` to `mir::Const::Ty`. I dont have a great understanding of why mir constants are setup like this to be honest. Regardless they need to be able to determine the type of the const and the easiest way to make this happen was to simply store the `Ty` along side the `ty::Const`. Maybe we can do better here in the future but I'd have to spend way more time looking at everywhere we use `mir::Const`. - SMIR represents mir consts and ty consts using the same `Const` type. This... does not work at all anymore so I have split them into two different types in SMIR and it seems to make everything a lot nicer regardless of this PR. - rustdoc has its own `Const` which also has a `ty` field. It was relatively easy to remove this. --- r? `@lcnr` `@compiler-errors`
bors
added a commit
to rust-lang-ci/rust
that referenced
this issue
Jun 5, 2024
Remove the `ty` field from type system `Const`s Part of the work on `adt_const_params`/`generic_const_param_types`/`min_generic_const_exprs`/generally making the compiler nicer. cc rust-lang/project-const-generics#44 Please review commit-by-commit otherwise I wasted a lot of time not just squashing this into a giant mess (and also it'll be SO much nicer because theres a lot of fluff changes mixed in with other more careful changes if looking via File Changes --- Why do this? - The `ty` field keeps causing ICEs and weird behaviour due to it either being treated as "part of the const" or it being forgotten about leading to ICEs. - As we move forward with `adt_const_params` and a potential `min_generic_const_exprs` it's going to become more complex to actually lower the correct `Ty<'tcx>` - It muddles the idea behind how we check `Const` arguments have the correct type. By having the `ty` field it may seem like we ought to be relating it when we relate two types, or that its generally important information about the `Const`. - Brings the compiler more in line with `a-mir-formality` as that also tracks the type of type system `Const`s via `ConstArgHasType` bounds in the env instead of on the `Const` itself. - A lot of stuff is a lot nicer when you dont have to pass around the type of a const lol. Everywhere we construct `Const` is now significantly nicer 😅 See rust-lang#125671's description for some more information about the `ty` field --- General summary of changes in this PR: - Add `Ty` to `ConstKind::Value` as otherwise there is no way to implement `ConstArgHasType` to ensure that const arguments are correctly typed for the parameter when we stop creating anon consts for all const args. It's also just incredibly difficult/annoying to thread the correct `Ty` around to a bunch of ctfe functions otherwise. - Fully implement `ConstArgHasType` in both the old and new solver. Since it now has no reliance on the `ty` field it serves its originally intended purpose of being able to act as a double check that trait vs impls have correctly typed const parameters. It also will now be able to be responsible for checking types of const arguments to parameters under `min_generic_const_exprs`. - Add `Ty` to `mir::Const::Ty`. I dont have a great understanding of why mir constants are setup like this to be honest. Regardless they need to be able to determine the type of the const and the easiest way to make this happen was to simply store the `Ty` along side the `ty::Const`. Maybe we can do better here in the future but I'd have to spend way more time looking at everywhere we use `mir::Const`. - rustdoc has its own `Const` which also has a `ty` field. It was relatively easy to remove this. --- r? `@lcnr` `@compiler-errors`
bors
added a commit
to rust-lang-ci/rust
that referenced
this issue
Jun 5, 2024
Remove the `ty` field from type system `Const`s Part of the work on `adt_const_params`/`generic_const_param_types`/`min_generic_const_exprs`/generally making the compiler nicer. cc rust-lang/project-const-generics#44 Please review commit-by-commit otherwise I wasted a lot of time not just squashing this into a giant mess (and also it'll be SO much nicer because theres a lot of fluff changes mixed in with other more careful changes if looking via File Changes --- Why do this? - The `ty` field keeps causing ICEs and weird behaviour due to it either being treated as "part of the const" or it being forgotten about leading to ICEs. - As we move forward with `adt_const_params` and a potential `min_generic_const_exprs` it's going to become more complex to actually lower the correct `Ty<'tcx>` - It muddles the idea behind how we check `Const` arguments have the correct type. By having the `ty` field it may seem like we ought to be relating it when we relate two types, or that its generally important information about the `Const`. - Brings the compiler more in line with `a-mir-formality` as that also tracks the type of type system `Const`s via `ConstArgHasType` bounds in the env instead of on the `Const` itself. - A lot of stuff is a lot nicer when you dont have to pass around the type of a const lol. Everywhere we construct `Const` is now significantly nicer 😅 See rust-lang#125671's description for some more information about the `ty` field --- General summary of changes in this PR: - Add `Ty` to `ConstKind::Value` as otherwise there is no way to implement `ConstArgHasType` to ensure that const arguments are correctly typed for the parameter when we stop creating anon consts for all const args. It's also just incredibly difficult/annoying to thread the correct `Ty` around to a bunch of ctfe functions otherwise. - Fully implement `ConstArgHasType` in both the old and new solver. Since it now has no reliance on the `ty` field it serves its originally intended purpose of being able to act as a double check that trait vs impls have correctly typed const parameters. It also will now be able to be responsible for checking types of const arguments to parameters under `min_generic_const_exprs`. - Add `Ty` to `mir::Const::Ty`. I dont have a great understanding of why mir constants are setup like this to be honest. Regardless they need to be able to determine the type of the const and the easiest way to make this happen was to simply store the `Ty` along side the `ty::Const`. Maybe we can do better here in the future but I'd have to spend way more time looking at everywhere we use `mir::Const`. - rustdoc has its own `Const` which also has a `ty` field. It was relatively easy to remove this. --- r? `@lcnr` `@compiler-errors`
bors
added a commit
to rust-lang-ci/rust
that referenced
this issue
Jun 5, 2024
Remove the `ty` field from type system `Const`s Part of the work on `adt_const_params`/`generic_const_param_types`/`min_generic_const_exprs`/generally making the compiler nicer. cc rust-lang/project-const-generics#44 Please review commit-by-commit otherwise I wasted a lot of time not just squashing this into a giant mess (and also it'll be SO much nicer because theres a lot of fluff changes mixed in with other more careful changes if looking via File Changes --- Why do this? - The `ty` field keeps causing ICEs and weird behaviour due to it either being treated as "part of the const" or it being forgotten about leading to ICEs. - As we move forward with `adt_const_params` and a potential `min_generic_const_exprs` it's going to become more complex to actually lower the correct `Ty<'tcx>` - It muddles the idea behind how we check `Const` arguments have the correct type. By having the `ty` field it may seem like we ought to be relating it when we relate two types, or that its generally important information about the `Const`. - Brings the compiler more in line with `a-mir-formality` as that also tracks the type of type system `Const`s via `ConstArgHasType` bounds in the env instead of on the `Const` itself. - A lot of stuff is a lot nicer when you dont have to pass around the type of a const lol. Everywhere we construct `Const` is now significantly nicer 😅 See rust-lang#125671's description for some more information about the `ty` field --- General summary of changes in this PR: - Add `Ty` to `ConstKind::Value` as otherwise there is no way to implement `ConstArgHasType` to ensure that const arguments are correctly typed for the parameter when we stop creating anon consts for all const args. It's also just incredibly difficult/annoying to thread the correct `Ty` around to a bunch of ctfe functions otherwise. - Fully implement `ConstArgHasType` in both the old and new solver. Since it now has no reliance on the `ty` field it serves its originally intended purpose of being able to act as a double check that trait vs impls have correctly typed const parameters. It also will now be able to be responsible for checking types of const arguments to parameters under `min_generic_const_exprs`. - Add `Ty` to `mir::Const::Ty`. I dont have a great understanding of why mir constants are setup like this to be honest. Regardless they need to be able to determine the type of the const and the easiest way to make this happen was to simply store the `Ty` along side the `ty::Const`. Maybe we can do better here in the future but I'd have to spend way more time looking at everywhere we use `mir::Const`. - rustdoc has its own `Const` which also has a `ty` field. It was relatively easy to remove this. --- r? `@lcnr` `@compiler-errors`
bors
added a commit
to rust-lang-ci/rust
that referenced
this issue
Jun 5, 2024
Remove the `ty` field from type system `Const`s Part of the work on `adt_const_params`/`generic_const_param_types`/`min_generic_const_exprs`/generally making the compiler nicer. cc rust-lang/project-const-generics#44 Please review commit-by-commit otherwise I wasted a lot of time not just squashing this into a giant mess (and also it'll be SO much nicer because theres a lot of fluff changes mixed in with other more careful changes if looking via File Changes --- Why do this? - The `ty` field keeps causing ICEs and weird behaviour due to it either being treated as "part of the const" or it being forgotten about leading to ICEs. - As we move forward with `adt_const_params` and a potential `min_generic_const_exprs` it's going to become more complex to actually lower the correct `Ty<'tcx>` - It muddles the idea behind how we check `Const` arguments have the correct type. By having the `ty` field it may seem like we ought to be relating it when we relate two types, or that its generally important information about the `Const`. - Brings the compiler more in line with `a-mir-formality` as that also tracks the type of type system `Const`s via `ConstArgHasType` bounds in the env instead of on the `Const` itself. - A lot of stuff is a lot nicer when you dont have to pass around the type of a const lol. Everywhere we construct `Const` is now significantly nicer 😅 See rust-lang#125671's description for some more information about the `ty` field --- General summary of changes in this PR: - Add `Ty` to `ConstKind::Value` as otherwise there is no way to implement `ConstArgHasType` to ensure that const arguments are correctly typed for the parameter when we stop creating anon consts for all const args. It's also just incredibly difficult/annoying to thread the correct `Ty` around to a bunch of ctfe functions otherwise. - Fully implement `ConstArgHasType` in both the old and new solver. Since it now has no reliance on the `ty` field it serves its originally intended purpose of being able to act as a double check that trait vs impls have correctly typed const parameters. It also will now be able to be responsible for checking types of const arguments to parameters under `min_generic_const_exprs`. - Add `Ty` to `mir::Const::Ty`. I dont have a great understanding of why mir constants are setup like this to be honest. Regardless they need to be able to determine the type of the const and the easiest way to make this happen was to simply store the `Ty` along side the `ty::Const`. Maybe we can do better here in the future but I'd have to spend way more time looking at everywhere we use `mir::Const`. - rustdoc has its own `Const` which also has a `ty` field. It was relatively easy to remove this. --- r? `@lcnr` `@compiler-errors`
bors
added a commit
to rust-lang-ci/rust
that referenced
this issue
Jun 5, 2024
Remove the `ty` field from type system `Const`s Part of the work on `adt_const_params`/`generic_const_param_types`/`min_generic_const_exprs`/generally making the compiler nicer. cc rust-lang/project-const-generics#44 Please review commit-by-commit otherwise I wasted a lot of time not just squashing this into a giant mess (and also it'll be SO much nicer because theres a lot of fluff changes mixed in with other more careful changes if looking via File Changes --- Why do this? - The `ty` field keeps causing ICEs and weird behaviour due to it either being treated as "part of the const" or it being forgotten about leading to ICEs. - As we move forward with `adt_const_params` and a potential `min_generic_const_exprs` it's going to become more complex to actually lower the correct `Ty<'tcx>` - It muddles the idea behind how we check `Const` arguments have the correct type. By having the `ty` field it may seem like we ought to be relating it when we relate two types, or that its generally important information about the `Const`. - Brings the compiler more in line with `a-mir-formality` as that also tracks the type of type system `Const`s via `ConstArgHasType` bounds in the env instead of on the `Const` itself. - A lot of stuff is a lot nicer when you dont have to pass around the type of a const lol. Everywhere we construct `Const` is now significantly nicer 😅 See rust-lang#125671's description for some more information about the `ty` field --- General summary of changes in this PR: - Add `Ty` to `ConstKind::Value` as otherwise there is no way to implement `ConstArgHasType` to ensure that const arguments are correctly typed for the parameter when we stop creating anon consts for all const args. It's also just incredibly difficult/annoying to thread the correct `Ty` around to a bunch of ctfe functions otherwise. - Fully implement `ConstArgHasType` in both the old and new solver. Since it now has no reliance on the `ty` field it serves its originally intended purpose of being able to act as a double check that trait vs impls have correctly typed const parameters. It also will now be able to be responsible for checking types of const arguments to parameters under `min_generic_const_exprs`. - Add `Ty` to `mir::Const::Ty`. I dont have a great understanding of why mir constants are setup like this to be honest. Regardless they need to be able to determine the type of the const and the easiest way to make this happen was to simply store the `Ty` along side the `ty::Const`. Maybe we can do better here in the future but I'd have to spend way more time looking at everywhere we use `mir::Const`. - rustdoc has its own `Const` which also has a `ty` field. It was relatively easy to remove this. --- r? `@lcnr` `@compiler-errors`
bors
added a commit
to rust-lang-ci/rust
that referenced
this issue
Jun 5, 2024
Remove the `ty` field from type system `Const`s Part of the work on `adt_const_params`/`generic_const_param_types`/`min_generic_const_exprs`/generally making the compiler nicer. cc rust-lang/project-const-generics#44 Please review commit-by-commit otherwise I wasted a lot of time not just squashing this into a giant mess (and also it'll be SO much nicer because theres a lot of fluff changes mixed in with other more careful changes if looking via File Changes --- Why do this? - The `ty` field keeps causing ICEs and weird behaviour due to it either being treated as "part of the const" or it being forgotten about leading to ICEs. - As we move forward with `adt_const_params` and a potential `min_generic_const_exprs` it's going to become more complex to actually lower the correct `Ty<'tcx>` - It muddles the idea behind how we check `Const` arguments have the correct type. By having the `ty` field it may seem like we ought to be relating it when we relate two types, or that its generally important information about the `Const`. - Brings the compiler more in line with `a-mir-formality` as that also tracks the type of type system `Const`s via `ConstArgHasType` bounds in the env instead of on the `Const` itself. - A lot of stuff is a lot nicer when you dont have to pass around the type of a const lol. Everywhere we construct `Const` is now significantly nicer 😅 See rust-lang#125671's description for some more information about the `ty` field --- General summary of changes in this PR: - Add `Ty` to `ConstKind::Value` as otherwise there is no way to implement `ConstArgHasType` to ensure that const arguments are correctly typed for the parameter when we stop creating anon consts for all const args. It's also just incredibly difficult/annoying to thread the correct `Ty` around to a bunch of ctfe functions otherwise. - Fully implement `ConstArgHasType` in both the old and new solver. Since it now has no reliance on the `ty` field it serves its originally intended purpose of being able to act as a double check that trait vs impls have correctly typed const parameters. It also will now be able to be responsible for checking types of const arguments to parameters under `min_generic_const_exprs`. - Add `Ty` to `mir::Const::Ty`. I dont have a great understanding of why mir constants are setup like this to be honest. Regardless they need to be able to determine the type of the const and the easiest way to make this happen was to simply store the `Ty` along side the `ty::Const`. Maybe we can do better here in the future but I'd have to spend way more time looking at everywhere we use `mir::Const`. - rustdoc has its own `Const` which also has a `ty` field. It was relatively easy to remove this. --- r? `@lcnr` `@compiler-errors`
bors
added a commit
to rust-lang-ci/rust
that referenced
this issue
Jun 5, 2024
Remove the `ty` field from type system `Const`s Part of the work on `adt_const_params`/`generic_const_param_types`/`min_generic_const_exprs`/generally making the compiler nicer. cc rust-lang/project-const-generics#44 Please review commit-by-commit otherwise I wasted a lot of time not just squashing this into a giant mess (and also it'll be SO much nicer because theres a lot of fluff changes mixed in with other more careful changes if looking via File Changes --- Why do this? - The `ty` field keeps causing ICEs and weird behaviour due to it either being treated as "part of the const" or it being forgotten about leading to ICEs. - As we move forward with `adt_const_params` and a potential `min_generic_const_exprs` it's going to become more complex to actually lower the correct `Ty<'tcx>` - It muddles the idea behind how we check `Const` arguments have the correct type. By having the `ty` field it may seem like we ought to be relating it when we relate two types, or that its generally important information about the `Const`. - Brings the compiler more in line with `a-mir-formality` as that also tracks the type of type system `Const`s via `ConstArgHasType` bounds in the env instead of on the `Const` itself. - A lot of stuff is a lot nicer when you dont have to pass around the type of a const lol. Everywhere we construct `Const` is now significantly nicer 😅 See rust-lang#125671's description for some more information about the `ty` field --- General summary of changes in this PR: - Add `Ty` to `ConstKind::Value` as otherwise there is no way to implement `ConstArgHasType` to ensure that const arguments are correctly typed for the parameter when we stop creating anon consts for all const args. It's also just incredibly difficult/annoying to thread the correct `Ty` around to a bunch of ctfe functions otherwise. - Fully implement `ConstArgHasType` in both the old and new solver. Since it now has no reliance on the `ty` field it serves its originally intended purpose of being able to act as a double check that trait vs impls have correctly typed const parameters. It also will now be able to be responsible for checking types of const arguments to parameters under `min_generic_const_exprs`. - Add `Ty` to `mir::Const::Ty`. I dont have a great understanding of why mir constants are setup like this to be honest. Regardless they need to be able to determine the type of the const and the easiest way to make this happen was to simply store the `Ty` along side the `ty::Const`. Maybe we can do better here in the future but I'd have to spend way more time looking at everywhere we use `mir::Const`. - rustdoc has its own `Const` which also has a `ty` field. It was relatively easy to remove this. --- r? `@lcnr` `@compiler-errors`
bors
added a commit
to rust-lang-ci/rust
that referenced
this issue
Jun 5, 2024
Remove the `ty` field from type system `Const`s Fixes rust-lang#125556 Fixes rust-lang#122908 Part of the work on `adt_const_params`/`generic_const_param_types`/`min_generic_const_exprs`/generally making the compiler nicer. cc rust-lang/project-const-generics#44 Please review commit-by-commit otherwise I wasted a lot of time not just squashing this into a giant mess (and also it'll be SO much nicer because theres a lot of fluff changes mixed in with other more careful changes if looking via File Changes --- Why do this? - The `ty` field keeps causing ICEs and weird behaviour due to it either being treated as "part of the const" or it being forgotten about leading to ICEs. - As we move forward with `adt_const_params` and a potential `min_generic_const_exprs` it's going to become more complex to actually lower the correct `Ty<'tcx>` - It muddles the idea behind how we check `Const` arguments have the correct type. By having the `ty` field it may seem like we ought to be relating it when we relate two types, or that its generally important information about the `Const`. - Brings the compiler more in line with `a-mir-formality` as that also tracks the type of type system `Const`s via `ConstArgHasType` bounds in the env instead of on the `Const` itself. - A lot of stuff is a lot nicer when you dont have to pass around the type of a const lol. Everywhere we construct `Const` is now significantly nicer 😅 See rust-lang#125671's description for some more information about the `ty` field --- General summary of changes in this PR: - Add `Ty` to `ConstKind::Value` as otherwise there is no way to implement `ConstArgHasType` to ensure that const arguments are correctly typed for the parameter when we stop creating anon consts for all const args. It's also just incredibly difficult/annoying to thread the correct `Ty` around to a bunch of ctfe functions otherwise. - Fully implement `ConstArgHasType` in both the old and new solver. Since it now has no reliance on the `ty` field it serves its originally intended purpose of being able to act as a double check that trait vs impls have correctly typed const parameters. It also will now be able to be responsible for checking types of const arguments to parameters under `min_generic_const_exprs`. - Add `Ty` to `mir::Const::Ty`. I dont have a great understanding of why mir constants are setup like this to be honest. Regardless they need to be able to determine the type of the const and the easiest way to make this happen was to simply store the `Ty` along side the `ty::Const`. Maybe we can do better here in the future but I'd have to spend way more time looking at everywhere we use `mir::Const`. - rustdoc has its own `Const` which also has a `ty` field. It was relatively easy to remove this. --- r? `@lcnr` `@compiler-errors`
bors
added a commit
to rust-lang-ci/rust
that referenced
this issue
Jun 6, 2024
Remove the `ty` field from type system `Const`s Fixes rust-lang#125556 Fixes rust-lang#122908 Part of the work on `adt_const_params`/`generic_const_param_types`/`min_generic_const_exprs`/generally making the compiler nicer. cc rust-lang/project-const-generics#44 Please review commit-by-commit otherwise I wasted a lot of time not just squashing this into a giant mess (and also it'll be SO much nicer because theres a lot of fluff changes mixed in with other more careful changes if looking via File Changes --- Why do this? - The `ty` field keeps causing ICEs and weird behaviour due to it either being treated as "part of the const" or it being forgotten about leading to ICEs. - As we move forward with `adt_const_params` and a potential `min_generic_const_exprs` it's going to become more complex to actually lower the correct `Ty<'tcx>` - It muddles the idea behind how we check `Const` arguments have the correct type. By having the `ty` field it may seem like we ought to be relating it when we relate two types, or that its generally important information about the `Const`. - Brings the compiler more in line with `a-mir-formality` as that also tracks the type of type system `Const`s via `ConstArgHasType` bounds in the env instead of on the `Const` itself. - A lot of stuff is a lot nicer when you dont have to pass around the type of a const lol. Everywhere we construct `Const` is now significantly nicer 😅 See rust-lang#125671's description for some more information about the `ty` field --- General summary of changes in this PR: - Add `Ty` to `ConstKind::Value` as otherwise there is no way to implement `ConstArgHasType` to ensure that const arguments are correctly typed for the parameter when we stop creating anon consts for all const args. It's also just incredibly difficult/annoying to thread the correct `Ty` around to a bunch of ctfe functions otherwise. - Fully implement `ConstArgHasType` in both the old and new solver. Since it now has no reliance on the `ty` field it serves its originally intended purpose of being able to act as a double check that trait vs impls have correctly typed const parameters. It also will now be able to be responsible for checking types of const arguments to parameters under `min_generic_const_exprs`. - Add `Ty` to `mir::Const::Ty`. I dont have a great understanding of why mir constants are setup like this to be honest. Regardless they need to be able to determine the type of the const and the easiest way to make this happen was to simply store the `Ty` along side the `ty::Const`. Maybe we can do better here in the future but I'd have to spend way more time looking at everywhere we use `mir::Const`. - rustdoc has its own `Const` which also has a `ty` field. It was relatively easy to remove this. --- r? `@lcnr` `@compiler-errors`
bors
added a commit
to rust-lang-ci/rust
that referenced
this issue
Jun 6, 2024
Remove the `ty` field from type system `Const`s Fixes rust-lang#125556 Fixes rust-lang#122908 Part of the work on `adt_const_params`/`generic_const_param_types`/`min_generic_const_exprs`/generally making the compiler nicer. cc rust-lang/project-const-generics#44 Please review commit-by-commit otherwise I wasted a lot of time not just squashing this into a giant mess (and also it'll be SO much nicer because theres a lot of fluff changes mixed in with other more careful changes if looking via File Changes --- Why do this? - The `ty` field keeps causing ICEs and weird behaviour due to it either being treated as "part of the const" or it being forgotten about leading to ICEs. - As we move forward with `adt_const_params` and a potential `min_generic_const_exprs` it's going to become more complex to actually lower the correct `Ty<'tcx>` - It muddles the idea behind how we check `Const` arguments have the correct type. By having the `ty` field it may seem like we ought to be relating it when we relate two types, or that its generally important information about the `Const`. - Brings the compiler more in line with `a-mir-formality` as that also tracks the type of type system `Const`s via `ConstArgHasType` bounds in the env instead of on the `Const` itself. - A lot of stuff is a lot nicer when you dont have to pass around the type of a const lol. Everywhere we construct `Const` is now significantly nicer 😅 See rust-lang#125671's description for some more information about the `ty` field --- General summary of changes in this PR: - Add `Ty` to `ConstKind::Value` as otherwise there is no way to implement `ConstArgHasType` to ensure that const arguments are correctly typed for the parameter when we stop creating anon consts for all const args. It's also just incredibly difficult/annoying to thread the correct `Ty` around to a bunch of ctfe functions otherwise. - Fully implement `ConstArgHasType` in both the old and new solver. Since it now has no reliance on the `ty` field it serves its originally intended purpose of being able to act as a double check that trait vs impls have correctly typed const parameters. It also will now be able to be responsible for checking types of const arguments to parameters under `min_generic_const_exprs`. - Add `Ty` to `mir::Const::Ty`. I dont have a great understanding of why mir constants are setup like this to be honest. Regardless they need to be able to determine the type of the const and the easiest way to make this happen was to simply store the `Ty` along side the `ty::Const`. Maybe we can do better here in the future but I'd have to spend way more time looking at everywhere we use `mir::Const`. - rustdoc has its own `Const` which also has a `ty` field. It was relatively easy to remove this. --- r? `@lcnr` `@compiler-errors`
github-actions bot
pushed a commit
to rust-lang/miri
that referenced
this issue
Jun 7, 2024
Remove the `ty` field from type system `Const`s Fixes #125556 Fixes #122908 Part of the work on `adt_const_params`/`generic_const_param_types`/`min_generic_const_exprs`/generally making the compiler nicer. cc rust-lang/project-const-generics#44 Please review commit-by-commit otherwise I wasted a lot of time not just squashing this into a giant mess (and also it'll be SO much nicer because theres a lot of fluff changes mixed in with other more careful changes if looking via File Changes --- Why do this? - The `ty` field keeps causing ICEs and weird behaviour due to it either being treated as "part of the const" or it being forgotten about leading to ICEs. - As we move forward with `adt_const_params` and a potential `min_generic_const_exprs` it's going to become more complex to actually lower the correct `Ty<'tcx>` - It muddles the idea behind how we check `Const` arguments have the correct type. By having the `ty` field it may seem like we ought to be relating it when we relate two types, or that its generally important information about the `Const`. - Brings the compiler more in line with `a-mir-formality` as that also tracks the type of type system `Const`s via `ConstArgHasType` bounds in the env instead of on the `Const` itself. - A lot of stuff is a lot nicer when you dont have to pass around the type of a const lol. Everywhere we construct `Const` is now significantly nicer 😅 See #125671's description for some more information about the `ty` field --- General summary of changes in this PR: - Add `Ty` to `ConstKind::Value` as otherwise there is no way to implement `ConstArgHasType` to ensure that const arguments are correctly typed for the parameter when we stop creating anon consts for all const args. It's also just incredibly difficult/annoying to thread the correct `Ty` around to a bunch of ctfe functions otherwise. - Fully implement `ConstArgHasType` in both the old and new solver. Since it now has no reliance on the `ty` field it serves its originally intended purpose of being able to act as a double check that trait vs impls have correctly typed const parameters. It also will now be able to be responsible for checking types of const arguments to parameters under `min_generic_const_exprs`. - Add `Ty` to `mir::Const::Ty`. I dont have a great understanding of why mir constants are setup like this to be honest. Regardless they need to be able to determine the type of the const and the easiest way to make this happen was to simply store the `Ty` along side the `ty::Const`. Maybe we can do better here in the future but I'd have to spend way more time looking at everywhere we use `mir::Const`. - rustdoc has its own `Const` which also has a `ty` field. It was relatively easy to remove this. --- r? `@lcnr` `@compiler-errors`
flip1995
pushed a commit
to flip1995/rust-clippy
that referenced
this issue
Jun 28, 2024
Remove the `ty` field from type system `Const`s Fixes #125556 Fixes #122908 Part of the work on `adt_const_params`/`generic_const_param_types`/`min_generic_const_exprs`/generally making the compiler nicer. cc rust-lang/project-const-generics#44 Please review commit-by-commit otherwise I wasted a lot of time not just squashing this into a giant mess (and also it'll be SO much nicer because theres a lot of fluff changes mixed in with other more careful changes if looking via File Changes --- Why do this? - The `ty` field keeps causing ICEs and weird behaviour due to it either being treated as "part of the const" or it being forgotten about leading to ICEs. - As we move forward with `adt_const_params` and a potential `min_generic_const_exprs` it's going to become more complex to actually lower the correct `Ty<'tcx>` - It muddles the idea behind how we check `Const` arguments have the correct type. By having the `ty` field it may seem like we ought to be relating it when we relate two types, or that its generally important information about the `Const`. - Brings the compiler more in line with `a-mir-formality` as that also tracks the type of type system `Const`s via `ConstArgHasType` bounds in the env instead of on the `Const` itself. - A lot of stuff is a lot nicer when you dont have to pass around the type of a const lol. Everywhere we construct `Const` is now significantly nicer 😅 See #125671's description for some more information about the `ty` field --- General summary of changes in this PR: - Add `Ty` to `ConstKind::Value` as otherwise there is no way to implement `ConstArgHasType` to ensure that const arguments are correctly typed for the parameter when we stop creating anon consts for all const args. It's also just incredibly difficult/annoying to thread the correct `Ty` around to a bunch of ctfe functions otherwise. - Fully implement `ConstArgHasType` in both the old and new solver. Since it now has no reliance on the `ty` field it serves its originally intended purpose of being able to act as a double check that trait vs impls have correctly typed const parameters. It also will now be able to be responsible for checking types of const arguments to parameters under `min_generic_const_exprs`. - Add `Ty` to `mir::Const::Ty`. I dont have a great understanding of why mir constants are setup like this to be honest. Regardless they need to be able to determine the type of the const and the easiest way to make this happen was to simply store the `Ty` along side the `ty::Const`. Maybe we can do better here in the future but I'd have to spend way more time looking at everywhere we use `mir::Const`. - rustdoc has its own `Const` which also has a `ty` field. It was relatively easy to remove this. --- r? `@lcnr` `@compiler-errors`
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
What is this
This is a design document for const generics. Any discussions about its content should be on zulip. The conclusions of these discussions should then be edited back into this issue. Please do not post any comments directly in this issue.
Content
The way we currently check that the type of const parameters is the same for trait definition and impls is quite ad-hoc, resulting in bugs. This was an issue both for methods and for GATs:
While talking about modeling const generics in a-mir-formality @nikomatsakis suggested to instead encode the expected parameter type using a new predicate. Something like this easily prevent these oversights which will still be meaningful if we add generic named constants in the future. It may also make it easier to add generic const parameter types (#28) in the future.
TODO: talk about the fact that we currently check that const arguments have the right type by it simply being the expected type while typechecking the anon const. That requires us to always typeck them which is somewhat surprising.
The text was updated successfully, but these errors were encountered: