Skip to content
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

Do not equate Const's ty in super_combine_const #125671

Merged
merged 1 commit into from
May 30, 2024

Conversation

BoxyUwU
Copy link
Member

@BoxyUwU BoxyUwU commented May 28, 2024

Fixes #114456

In #125451 we started relating the Const's tys outside of a probe so it was no longer simply an assertion to catch bugs.

This was done so that when we do provide a wrongly typed const argument to an item if we wind up relating it with some other instantiation we'll have a TypeError we can bubble up and taint the resulting mir allowing const eval to skip evaluation.

In this PR I instead change ConstArgHasType to correctly handle checking the types of const inference variables. Previously if we had something like impl<const N: u32> Trait for [(); N], when using the impl we would instantiate it with infer vars and then check that ?x: u32 is of type u32 and succeed. Then later we would infer ?x to some Const of type usize.

We now stall on ?x in ConstArgHasType until it has a concrete value that we can determine the type of. This allows us to fail using the erroneous implementation of Trait which allows us to taint the mir.

Long term we intend to remove the ty field on Const so we would have no way of accessing the ty of a const inference variable anyway and would have to do this. I did not fully update ConstArgHasType to avoid using the ty field as it's not entirely possible right now- we would need to lookup ConstArgHasType candidates in the env.


As for why I think we should do this, relating the types of const's is not necessary for soundness of the type system. Originally this check started off as a plain == in super_relate_consts and gradually has been growing in complexity as we support more complicated types. It was never actually required to ensure that const arguments are correctly typed for their parameters however.

The way we currently check that a const argument has the correct type is a little convoluted and confusing (and will hopefully be less weird as time goes on). Every const argument has an anon const with its return type set to type of the const parameter it is an argument to. When type checking the anon const regular type checking rules require that the expression is the same type as the return type. This effectively ensure that no matter what every const argument always has the correct type.

An extra bit of complexity is that during hir_ty_lowering we do not represent everything as a ConstKind::Unevaluated corresponding to the anon const. For generic parameters i.e. [(); N] we simply represent them as ConstKind::Param as we do not want ConstKind::Unevaluated with generic substs on stable under min const generics. The anon const still gets type checked resulting in errors about type mismatches.

Eventually we intend to not create anon consts for all const arguments (for example for ConstKind::Param) and instead check that the argument type is correct via ConstArgHasType obligations (these effectively also act as a check that the anon consts have the correctly set return type).

What this all means is that the the only time we should ever have mismatched types when relating two Consts is if we have messed up our logic for ensuring that const arguments are of the correct type. Having this not be an assert is:

  • Confusing as it may incorrectly lead people to believe this is an important check that is actually required
  • Opens the possibility for bugs or behaviour reliant on this (unnecessary) check existing

This PR makes two tests go from pass->ICE (generic_const_exprs/ice-125520-layout-mismatch-mulwithoverflow.rs and tests/crashes/121858.rs). This is caused by the fact that we evaluate anon consts even if their where clauses do not hold and is a pre-existing issue and only affects generic_const_exprs. I am comfortable exposing the brokenness of generic_const_exprs more with this PR

This PR makes a test go from ICE->pass (const-generics/issues/issue-105821.rs). I have no idea why this PR affects that but I believe that ICE is an unrelated issue to do with the fact that under generic_const_exprs/adt_const_params we do not handle lifetimes in const parameter types correctly. This PR is likely just masking this bug.

Note: this PR doesn't re-introduce the assertion that the two consts' tys are equal. I'm not really sure how I feel about this but tbh it has caused more ICEs than its found lately so 🤷‍♀️

r? @oli-obk @compiler-errors

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative labels May 28, 2024
@rust-log-analyzer

This comment has been minimized.

@BoxyUwU BoxyUwU marked this pull request as ready for review May 29, 2024 00:20
@rustbot
Copy link
Collaborator

rustbot commented May 29, 2024

Some changes occurred to the core trait solver

cc @rust-lang/initiative-trait-system-refactor

changes to the core type system

cc @compiler-errors, @lcnr

@rust-log-analyzer

This comment has been minimized.

trait ConstChunksExactTrait<T> {
fn const_chunks_exact<const N: usize>(&self) -> ConstChunksExact<'_, T, { N }>;

trait Trait {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had to rewrite this test as this PR masked the ICE due to the fact that const N: usize now introduces a bound that is ambiguous for an inference variable.

@@ -1,5 +1,7 @@
//@ known-bug: #114456
//@ check-pass
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was fixed by removing the eq call as previously we did not normalize any aliases in the ty field before equating them.

@rust-log-analyzer

This comment has been minimized.

@BoxyUwU
Copy link
Member Author

BoxyUwU commented May 29, 2024

That test literally passes locally, ????? Sigh

@BoxyUwU
Copy link
Member Author

BoxyUwU commented May 29, 2024

Hell the previous CI failure was because it was in tests/crashes/xxxxx and was passing

@matthiaskrgr
Copy link
Member

crashes tests "fail" ci as soon as the test no longer crashes, to that we notice when ICEs get fixed by accident. 🙂

A crash test must be linked to a corresponding ticket in the bugtracker. If a crash is fixed, we can move it to ui tests and make sure the corresponding issue is closed upon merge.

@BoxyUwU
Copy link
Member Author

BoxyUwU commented May 29, 2024

Yeah what happend was CI failed because there was a passing test in crashes so I moved it to a ui test (and verified locally it passed) then pushed and CI failed because the ui test ICEd so I moved it back to crashes and now it works lol

Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r=me after nit, since it slightly simplifies the code.

compiler/rustc_trait_selection/src/traits/fulfill.rs Outdated Show resolved Hide resolved
compiler/rustc_trait_selection/src/traits/select/mod.rs Outdated Show resolved Hide resolved
@compiler-errors
Copy link
Member

ty!

@bors r+

@bors
Copy link
Contributor

bors commented May 29, 2024

📌 Commit d5bd4e2 has been approved by compiler-errors

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 29, 2024
fmease added a commit to fmease/rust that referenced this pull request May 30, 2024
…iler-errors

Do not equate `Const`'s ty in `super_combine_const`

Fixes rust-lang#114456

In rust-lang#125451 we started relating the `Const`'s tys outside of a probe so it was no longer simply an assertion to catch bugs.

This was done so that when we _do_ provide a wrongly typed const argument to an item if we wind up relating it with some other instantiation we'll have a `TypeError` we can bubble up and taint the resulting mir allowing const eval to skip evaluation.

In this PR I instead change `ConstArgHasType` to correctly handle checking the types of const inference variables. Previously if we had something like `impl<const N: u32> Trait for [(); N]`, when using the impl we would instantiate it with infer vars and then check that `?x: u32` is of type `u32` and succeed. Then later we would infer `?x` to some `Const` of type `usize`.

We now stall on `?x` in `ConstArgHasType` until it has a concrete value that we can determine the type of. This allows us to fail using the erroneous implementation of `Trait` which allows us to taint the mir.

Long term we intend to remove the `ty` field on `Const` so we would have no way of accessing the `ty` of a const inference variable anyway and would have to do this. I did not fully update `ConstArgHasType` to avoid using the `ty` field as it's not entirely possible right now- we would need to lookup `ConstArgHasType` candidates in the env.

---

As for _why_ I think we should do this, relating the types of const's is not necessary for soundness of the type system. Originally this check started off as a plain `==` in `super_relate_consts` and gradually has been growing in complexity as we support more complicated types. It was never actually required to ensure that const arguments are correctly typed for their parameters however.

The way we currently check that a const argument has the correct type is a little convoluted and confusing (and will hopefully be less weird as time goes on). Every const argument has an anon const with its return type set to type of the const parameter it is an argument to. When type checking the anon const regular type checking rules require that the expression is the same type as the return type. This effectively ensure that no matter what every const argument _always_ has the correct type.

An extra bit of complexity is that during `hir_ty_lowering` we do not represent everything as a `ConstKind::Unevaluated` corresponding to the anon const. For generic parameters i.e. `[(); N]` we simply represent them as `ConstKind::Param` as we do not want `ConstKind::Unevaluated` with generic substs on stable under min const generics. The anon const still gets type checked resulting in errors about type mismatches.

Eventually we intend to not create anon consts for all const arguments (for example for `ConstKind::Param`) and instead check that the argument type is correct via `ConstArgHasType` obligations (these effectively also act as a check that the anon consts have the correctly set return type).

What this all means is that the the only time we should ever have mismatched types when relating two `Const`s is if we have messed up our logic for ensuring that const arguments are of the correct type. Having this not be an assert is:
- Confusing as it may incorrectly lead people to believe this is an important check that is actually required
- Opens the possibility for bugs or behaviour reliant on this (unnecessary) check existing

---

This PR makes two tests go from pass->ICE (`generic_const_exprs/ice-125520-layout-mismatch-mulwithoverflow.rs` and `tests/crashes/121858.rs`). This is caused by the fact that we evaluate anon consts even if their where clauses do not hold and is a pre-existing issue and only affects `generic_const_exprs`. I am comfortable exposing the brokenness of `generic_const_exprs` more with this PR

This PR makes a test go from ICE->pass (`const-generics/issues/issue-105821.rs`). I have no idea why this PR affects that but I believe that ICE is an unrelated issue to do with the fact that under `generic_const_exprs`/`adt_const_params` we do not handle lifetimes in const parameter types correctly. This PR is likely just masking this bug.

Note: this PR doesn't re-introduce the assertion that the two consts' tys are equal. I'm not really sure how I feel about this but tbh it has caused more ICEs than its found lately so 🤷‍♀️

r? `@oli-obk` `@compiler-errors`
bors added a commit to rust-lang-ci/rust that referenced this pull request May 30, 2024
Rollup of 6 pull requests

Successful merges:

 - rust-lang#124636 (Make `std::env::{set_var, remove_var}` unsafe in edition 2024)
 - rust-lang#125342 (Document platform-specifics for `Read` and `Write` of `File`)
 - rust-lang#125671 (Do not equate `Const`'s ty in `super_combine_const`)
 - rust-lang#125711 (Make `body_owned_by` return the `Body` instead of just the `BodyId`)
 - rust-lang#125745 (Bump the stage0 compiler to beta.7 (2024-05-26))
 - rust-lang#125753 (compiletest: Unify `cmd2procres` with `run_command_to_procres`)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request May 30, 2024
Rollup of 6 pull requests

Successful merges:

 - rust-lang#124636 (Make `std::env::{set_var, remove_var}` unsafe in edition 2024)
 - rust-lang#125342 (Document platform-specifics for `Read` and `Write` of `File`)
 - rust-lang#125671 (Do not equate `Const`'s ty in `super_combine_const`)
 - rust-lang#125711 (Make `body_owned_by` return the `Body` instead of just the `BodyId`)
 - rust-lang#125745 (Bump the stage0 compiler to beta.7 (2024-05-26))
 - rust-lang#125753 (compiletest: Unify `cmd2procres` with `run_command_to_procres`)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors
Copy link
Contributor

bors commented May 30, 2024

⌛ Testing commit d5bd4e2 with merge 32a3ed2...

@bors
Copy link
Contributor

bors commented May 30, 2024

☀️ Test successful - checks-actions
Approved by: compiler-errors
Pushing 32a3ed2 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 30, 2024
@bors bors merged commit 32a3ed2 into rust-lang:master May 30, 2024
7 checks passed
@rustbot rustbot added this to the 1.80.0 milestone May 30, 2024
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (32a3ed2): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results (primary 2.1%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
2.1% [2.1%, 2.1%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 2.1% [2.1%, 2.1%] 1

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 667.284s -> 667.227s (-0.01%)
Artifact size: 318.83 MiB -> 318.88 MiB (0.02%)

bors added a commit to rust-lang-ci/rust that referenced this pull request 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 pull request 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 pull request 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 pull request 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 pull request 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 pull request 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 pull request 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 pull request 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 pull request 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 pull request 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 pull request 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 pull request 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`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ICE: lazy_type_alias: cannot relate consts of different types
8 participants