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

Simplify array length mismatch error reporting (to not try to turn consts into target usizes) #133367

Merged
merged 1 commit into from
Nov 27, 2024

Conversation

compiler-errors
Copy link
Member

@compiler-errors compiler-errors commented Nov 23, 2024

This changes TypeError::FixedArrayLen to use ExpectedFound<ty::Const<'tcx>> (instead of ExpectedFound<u64>), and renames it to TypeError::ArrayLen. This allows us to avoid a try_to_target_usize call in the type relation, which ICEs when we have a scalar of the wrong bit length (i.e. u8).

This also makes structurally_relate_tys to always use this type error kind any time we have a const mismatch resulting from relating the array-len part of [T; N].

This has the effect of changing the error message we issue for array length mismatches involving non-valtree consts. I actually quite like the change, though, since before:

LL | fn test<const N: usize, const M: usize>() -> [u8; M] {
   |                                              ------- expected `[u8; M]` because of return type
LL |     [0; N]
   |     ^^^^^^ expected `M`, found `N`
   |
   = note: expected array `[u8; M]`
              found array `[u8; N]`

and after, which I think is far less verbose:

LL | fn test<const N: usize, const M: usize>() -> [u8; M] {
   |                                              ------- expected `[u8; M]` because of return type
LL |     [0; N]
   |     ^^^^^^ expected an array with a size of M, found one with a size of N

The only questions I have are:

  1. Should we do something about backticks here? Right now we don't backtick either fully evaluated consts like 2, or rigid consts like Foo::BAR.... but maybe we should? It seems kinda verbose to do for numbers -- maybe we could intercept those specifically.
  2. I guess we may still run the risk of leaking unevaluated consts into error reporting like 2 + 1...?

r? @BoxyUwU

Fixes #126359
Fixes #131101

@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. labels Nov 23, 2024
@bors
Copy link
Contributor

bors commented Nov 23, 2024

☔ The latest upstream changes (presumably #133377) made this pull request unmergeable. Please resolve the merge conflicts.

@rustbot rustbot added T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 24, 2024
@rustbot

This comment was marked as outdated.

@rustbot rustbot removed has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 24, 2024
@BoxyUwU
Copy link
Member

BoxyUwU commented Nov 25, 2024

@bors r+ rollup

I think putting backticks around non-literal array lengths would make sense. It probably also makes sense to normalize somewhere in diagnostics to ensure we don't talk about arrays with size 1 + 2 🤔 Can be done in a follow up PR if you want

@bors
Copy link
Contributor

bors commented Nov 25, 2024

📌 Commit 28970a2 has been approved by BoxyUwU

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 Nov 25, 2024
jhpratt added a commit to jhpratt/rust that referenced this pull request Nov 26, 2024
…, r=BoxyUwU

Simplify array length mismatch error reporting (to not try to turn consts into target usizes)

This changes `TypeError::FixedArrayLen` to use `ExpectedFound<ty::Const<'tcx>>` (instead of `ExpectedFound<u64>`), and renames it to `TypeError::ArrayLen`. This allows us to avoid a `try_to_target_usize` call in the type relation, which ICEs when we have a scalar of the wrong bit length (i.e. u8).

This also makes `structurally_relate_tys` to always use this type error kind any time we have a const mismatch resulting from relating the array-len part of `[T; N]`.

This has the effect of changing the error message we issue for array length mismatches involving non-valtree consts. I actually quite like the change, though, since before:

```
LL | fn test<const N: usize, const M: usize>() -> [u8; M] {
   |                                              ------- expected `[u8; M]` because of return type
LL |     [0; N]
   |     ^^^^^^ expected `M`, found `N`
   |
   = note: expected array `[u8; M]`
              found array `[u8; N]`
```

and after, which I think is far less verbose:

```
LL | fn test<const N: usize, const M: usize>() -> [u8; M] {
   |                                              ------- expected `[u8; M]` because of return type
LL |     [0; N]
   |     ^^^^^^ expected an array with a size of M, found one with a size of N
```

The only questions I have are:
1. Should we do something about backticks here? Right now we don't backtick either fully evaluated consts like `2`, or rigid consts like `Foo::BAR`.... but maybe we should? It seems kinda verbose to do for numbers -- maybe we could intercept those specifically.
2. I guess we may still run the risk of leaking unevaluated consts into error reporting like `2 + 1`...?

r? `@BoxyUwU`

Fixes rust-lang#126359
Fixes rust-lang#131101
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 26, 2024
Rollup of 28 pull requests

Successful merges:

 - rust-lang#132605 (CI: increase timeout from 4h to 6h)
 - rust-lang#133042 (btree: add `{Entry,VacantEntry}::insert_entry`)
 - rust-lang#133070 (Lexer tweaks)
 - rust-lang#133136 (Support ranges in `<[T]>::get_many_mut()`)
 - rust-lang#133140 (Inline ExprPrecedence::order into Expr::precedence)
 - rust-lang#133248 (CI: split x86_64-msvc-ext job)
 - rust-lang#133282 (Shorten the `MaybeUninit` `Debug` implementation)
 - rust-lang#133304 (Revert diagnostics hack to fix ICE 132920)
 - rust-lang#133326 (Remove the `DefinitelyInitializedPlaces` analysis.)
 - rust-lang#133362 (No need to re-sort existential preds in relate impl)
 - rust-lang#133367 (Simplify array length mismatch error reporting (to not try to turn consts into target usizes))
 - rust-lang#133394 (Bail on more errors in dyn ty lowering)
 - rust-lang#133410 (target check_consistency: ensure target feature string makes some basic sense)
 - rust-lang#133411 (the emscripten OS no longer exists on non-wasm targets)
 - rust-lang#133419 (Added a doc test for std::path::strip_prefix)
 - rust-lang#133430 (Tweak parameter mismatch explanation to not say `{unknown}`)
 - rust-lang#133435 (miri: disable test_downgrade_observe test on macOS)
 - rust-lang#133443 (Remove dead code stemming from the old effects desugaring (II))
 - rust-lang#133449 (std: expose `const_io_error!` as `const_error!`)
 - rust-lang#133450 (remove "onur-ozkan" from  users_on_vacation)
 - rust-lang#133454 (Update test expectations to accept LLVM 'initializes' attribute)
 - rust-lang#133458 (Fix `Result` and `Option` not getting a jump to def link generated)
 - rust-lang#133462 (Use ReadCache for archive reading in bootstrap)
 - rust-lang#133464 (std::thread: avoid leading whitespace in some panic messages)
 - rust-lang#133467 (tests: Add recursive associated type bound regression tests)
 - rust-lang#133470 (Cleanup: delete `//@ pretty-expanded` directive)
 - rust-lang#133473 (tests: Add regression test for recursive enum with Cow and Clone)
 - rust-lang#133481 (Disable `avr-rjmp-offset` on Windows for now)

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

Successful merges:

 - rust-lang#132605 (CI: increase timeout from 4h to 6h)
 - rust-lang#133042 (btree: add `{Entry,VacantEntry}::insert_entry`)
 - rust-lang#133070 (Lexer tweaks)
 - rust-lang#133136 (Support ranges in `<[T]>::get_many_mut()`)
 - rust-lang#133140 (Inline ExprPrecedence::order into Expr::precedence)
 - rust-lang#133248 (CI: split x86_64-msvc-ext job)
 - rust-lang#133282 (Shorten the `MaybeUninit` `Debug` implementation)
 - rust-lang#133304 (Revert diagnostics hack to fix ICE 132920)
 - rust-lang#133326 (Remove the `DefinitelyInitializedPlaces` analysis.)
 - rust-lang#133362 (No need to re-sort existential preds in relate impl)
 - rust-lang#133367 (Simplify array length mismatch error reporting (to not try to turn consts into target usizes))
 - rust-lang#133394 (Bail on more errors in dyn ty lowering)
 - rust-lang#133410 (target check_consistency: ensure target feature string makes some basic sense)
 - rust-lang#133411 (the emscripten OS no longer exists on non-wasm targets)
 - rust-lang#133419 (Added a doc test for std::path::strip_prefix)
 - rust-lang#133430 (Tweak parameter mismatch explanation to not say `{unknown}`)
 - rust-lang#133435 (miri: disable test_downgrade_observe test on macOS)
 - rust-lang#133443 (Remove dead code stemming from the old effects desugaring (II))
 - rust-lang#133449 (std: expose `const_io_error!` as `const_error!`)
 - rust-lang#133450 (remove "onur-ozkan" from  users_on_vacation)
 - rust-lang#133454 (Update test expectations to accept LLVM 'initializes' attribute)
 - rust-lang#133458 (Fix `Result` and `Option` not getting a jump to def link generated)
 - rust-lang#133462 (Use ReadCache for archive reading in bootstrap)
 - rust-lang#133464 (std::thread: avoid leading whitespace in some panic messages)
 - rust-lang#133467 (tests: Add recursive associated type bound regression tests)
 - rust-lang#133470 (Cleanup: delete `//@ pretty-expanded` directive)
 - rust-lang#133473 (tests: Add regression test for recursive enum with Cow and Clone)
 - rust-lang#133481 (Disable `avr-rjmp-offset` on Windows for now)

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

Successful merges:

 - rust-lang#132605 (CI: increase timeout from 4h to 6h)
 - rust-lang#133042 (btree: add `{Entry,VacantEntry}::insert_entry`)
 - rust-lang#133070 (Lexer tweaks)
 - rust-lang#133136 (Support ranges in `<[T]>::get_many_mut()`)
 - rust-lang#133140 (Inline ExprPrecedence::order into Expr::precedence)
 - rust-lang#133248 (CI: split x86_64-msvc-ext job)
 - rust-lang#133282 (Shorten the `MaybeUninit` `Debug` implementation)
 - rust-lang#133304 (Revert diagnostics hack to fix ICE 132920)
 - rust-lang#133326 (Remove the `DefinitelyInitializedPlaces` analysis.)
 - rust-lang#133362 (No need to re-sort existential preds in relate impl)
 - rust-lang#133367 (Simplify array length mismatch error reporting (to not try to turn consts into target usizes))
 - rust-lang#133394 (Bail on more errors in dyn ty lowering)
 - rust-lang#133410 (target check_consistency: ensure target feature string makes some basic sense)
 - rust-lang#133411 (the emscripten OS no longer exists on non-wasm targets)
 - rust-lang#133419 (Added a doc test for std::path::strip_prefix)
 - rust-lang#133430 (Tweak parameter mismatch explanation to not say `{unknown}`)
 - rust-lang#133435 (miri: disable test_downgrade_observe test on macOS)
 - rust-lang#133443 (Remove dead code stemming from the old effects desugaring (II))
 - rust-lang#133449 (std: expose `const_io_error!` as `const_error!`)
 - rust-lang#133450 (remove "onur-ozkan" from  users_on_vacation)
 - rust-lang#133454 (Update test expectations to accept LLVM 'initializes' attribute)
 - rust-lang#133458 (Fix `Result` and `Option` not getting a jump to def link generated)
 - rust-lang#133462 (Use ReadCache for archive reading in bootstrap)
 - rust-lang#133464 (std::thread: avoid leading whitespace in some panic messages)
 - rust-lang#133467 (tests: Add recursive associated type bound regression tests)
 - rust-lang#133470 (Cleanup: delete `//@ pretty-expanded` directive)
 - rust-lang#133473 (tests: Add regression test for recursive enum with Cow and Clone)
 - rust-lang#133481 (Disable `avr-rjmp-offset` on Windows for now)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 26, 2024
…mpiler-errors

Rollup of 12 pull requests

Successful merges:

 - rust-lang#133042 (btree: add `{Entry,VacantEntry}::insert_entry`)
 - rust-lang#133070 (Lexer tweaks)
 - rust-lang#133136 (Support ranges in `<[T]>::get_many_mut()`)
 - rust-lang#133140 (Inline ExprPrecedence::order into Expr::precedence)
 - rust-lang#133155 (Yet more `rustc_mir_dataflow` cleanups)
 - rust-lang#133282 (Shorten the `MaybeUninit` `Debug` implementation)
 - rust-lang#133326 (Remove the `DefinitelyInitializedPlaces` analysis.)
 - rust-lang#133362 (No need to re-sort existential preds in relate impl)
 - rust-lang#133367 (Simplify array length mismatch error reporting (to not try to turn consts into target usizes))
 - rust-lang#133394 (Bail on more errors in dyn ty lowering)
 - rust-lang#133410 (target check_consistency: ensure target feature string makes some basic sense)
 - rust-lang#133435 (miri: disable test_downgrade_observe test on macOS)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit cf09718 into rust-lang:master Nov 27, 2024
6 checks passed
@rustbot rustbot added this to the 1.85.0 milestone Nov 27, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Nov 27, 2024
Rollup merge of rust-lang#133367 - compiler-errors:array-len-mismatch, r=BoxyUwU

Simplify array length mismatch error reporting (to not try to turn consts into target usizes)

This changes `TypeError::FixedArrayLen` to use `ExpectedFound<ty::Const<'tcx>>` (instead of `ExpectedFound<u64>`), and renames it to `TypeError::ArrayLen`. This allows us to avoid a `try_to_target_usize` call in the type relation, which ICEs when we have a scalar of the wrong bit length (i.e. u8).

This also makes `structurally_relate_tys` to always use this type error kind any time we have a const mismatch resulting from relating the array-len part of `[T; N]`.

This has the effect of changing the error message we issue for array length mismatches involving non-valtree consts. I actually quite like the change, though, since before:

```
LL | fn test<const N: usize, const M: usize>() -> [u8; M] {
   |                                              ------- expected `[u8; M]` because of return type
LL |     [0; N]
   |     ^^^^^^ expected `M`, found `N`
   |
   = note: expected array `[u8; M]`
              found array `[u8; N]`
```

and after, which I think is far less verbose:

```
LL | fn test<const N: usize, const M: usize>() -> [u8; M] {
   |                                              ------- expected `[u8; M]` because of return type
LL |     [0; N]
   |     ^^^^^^ expected an array with a size of M, found one with a size of N
```

The only questions I have are:
1. Should we do something about backticks here? Right now we don't backtick either fully evaluated consts like `2`, or rigid consts like `Foo::BAR`.... but maybe we should? It seems kinda verbose to do for numbers -- maybe we could intercept those specifically.
2. I guess we may still run the risk of leaking unevaluated consts into error reporting like `2 + 1`...?

r? ``@BoxyUwU``

Fixes rust-lang#126359
Fixes rust-lang#131101
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ICE: expected int of size 8, but got size 1 ICE: expected int of size 8, but got size 1
4 participants