Skip to content

fix ICE in layout computation with unnormalizable const #137399

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

Merged
merged 2 commits into from
Feb 22, 2025

Conversation

lukas-code
Copy link
Member

The first commit reverts half of 7a667d2, where I removed a case from layout_of for handling non-generic unevaluated consts in array length, that I incorrectly assumed to be unreachable. This can actually happen with the combination of feature(generic_const_exprs) and feature(trivial_bounds), because GCE makes anon consts inherit their parent's predicates and with an impossible predicate like u8: A it's possible to have an array whose length is an associated const like <u8 as A>::B that is not generic, but also can't be normalized:

#![feature(generic_const_exprs)]
#![feature(trivial_bounds)]

trait A {
    const B: usize;
}

// With GCE + trivial bounds this definition is not a compile error.
// Computing the layout of this type shouldn't ICE.
struct S([u8; <u8 as A>::B])
where
    u8: A;

The first commit also incidentally fixes #137308, which also managed to get an unnormalizable assoc const into an array length:

trait A {
    const B: usize;
}

impl<C: ?Sized> A for u8 { //~ ERROR: the type parameter `C` is not constrained
    const B: usize = 42;
}

// Computing the layout of this type shouldn't ICE, even with the compile error above.
struct S([u8; <u8 as A>::B]);

This happens, because we bail out from codegen_select_candidate with an error if the selected impl has unconstrained params to avoid leaking infer vars out of a query. Instance::try_resolve will then return Ok(None), which for assoc consts roughly means "this const can't be evaluated in a generic context" and is treated as such:

Ok(None) => Err(ErrorHandled::TooGeneric(DUMMY_SP)),
(and this can ICE if the const isn't generic: #135617).

However, here <u8 as A>::B is definitely not "too generic" and also not unresolvable due to an unsatisfiable u8: A bound, so I've included the second commit to change the result of Instance::try_resolve from Ok(None) to Err(ErrorGuaranteed) when resolving an assoc item to an impl with unconstrained generic params. This has the effect that <u8 as A>::B will now be normalized to ConstKind::Error in the example above.

This properly fixes #137308, by no longer treating <u8 as A>::B as unresolvable even though it clearly has a unique impl that it resolves to. It also has the effect of changing the layout error from Unknown ("the type may be valid but has no sensible layout") to ReferencesError ("a non-layout error is reported elsewhere") which seems more appropriate.

r? @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. labels Feb 21, 2025
@compiler-errors
Copy link
Member

r=me when CI is green

@lukas-code
Copy link
Member Author

@bors r=compiler-errors

@bors
Copy link
Collaborator

bors commented Feb 21, 2025

📌 Commit 7fea935 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 Feb 21, 2025
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 21, 2025
…ler-errors

fix ICE in layout computation with unnormalizable const

The first commit reverts half of 7a667d2, where I removed a case from `layout_of` for handling non-generic unevaluated consts in array length, that I incorrectly assumed to be unreachable. This can actually happen with the combination of `feature(generic_const_exprs)` and `feature(trivial_bounds)`, because GCE makes anon consts inherit their parent's predicates and with an impossible predicate like `u8: A` it's possible to have an array whose length is an associated const like `<u8 as A>::B` that is not generic, but also can't be normalized:

```rust
#![feature(generic_const_exprs)]
#![feature(trivial_bounds)]

trait A {
    const B: usize;
}

// With GCE + trivial bounds this definition is not a compile error.
// Computing the layout of this type shouldn't ICE.
struct S([u8; <u8 as A>::B])
where
    u8: A;
```

---

The first commit also incidentally fixes rust-lang#137308, which also managed to get an unnormalizable assoc const into an array length:

```rust
trait A {
    const B: usize;
}

impl<C: ?Sized> A for u8 { //~ ERROR: the type parameter `C` is not constrained
    const B: usize = 42;
}

// Computing the layout of this type shouldn't ICE, even with the compile error above.
struct S([u8; <u8 as A>::B]);
```

This happens, because we bail out from `codegen_select_candidate` with an error if the selected impl has unconstrained params to avoid leaking infer vars out of a query. `Instance::try_resolve` will then return `Ok(None)`, which for assoc consts roughly means "this const can't be evaluated in a generic context" and is treated as such: https://github.com/rust-lang/rust/blob/71e06b9c59d6af50fdc55aed75620493d29baf98/compiler/rustc_middle/src/mir/interpret/queries.rs#L84 (and this can ICE if the const isn't generic: rust-lang#135617).

However, here `<u8 as A>::B` is definitely not "too generic" and also not unresolvable due to an unsatisfiable `u8: A` bound, so I've included the second commit to change the result of `Instance::try_resolve` from `Ok(None)` to `Err(ErrorGuaranteed)` when resolving an assoc item to an impl with unconstrained generic params. This has the effect that `<u8 as A>::B` will now be normalized to `ConstKind::Error` in the example above.

This properly fixes rust-lang#137308, by no longer treating `<u8 as A>::B` as unresolvable even though it clearly has a unique impl that it resolves to. It also has the effect of changing the layout error from `Unknown` ("the type may be valid but has no sensible layout") to `ReferencesError` ("a non-layout error is reported elsewhere") which seems more appropriate.

r? `@compiler-errors`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 21, 2025
…ler-errors

fix ICE in layout computation with unnormalizable const

The first commit reverts half of 7a667d2, where I removed a case from `layout_of` for handling non-generic unevaluated consts in array length, that I incorrectly assumed to be unreachable. This can actually happen with the combination of `feature(generic_const_exprs)` and `feature(trivial_bounds)`, because GCE makes anon consts inherit their parent's predicates and with an impossible predicate like `u8: A` it's possible to have an array whose length is an associated const like `<u8 as A>::B` that is not generic, but also can't be normalized:

```rust
#![feature(generic_const_exprs)]
#![feature(trivial_bounds)]

trait A {
    const B: usize;
}

// With GCE + trivial bounds this definition is not a compile error.
// Computing the layout of this type shouldn't ICE.
struct S([u8; <u8 as A>::B])
where
    u8: A;
```

---

The first commit also incidentally fixes rust-lang#137308, which also managed to get an unnormalizable assoc const into an array length:

```rust
trait A {
    const B: usize;
}

impl<C: ?Sized> A for u8 { //~ ERROR: the type parameter `C` is not constrained
    const B: usize = 42;
}

// Computing the layout of this type shouldn't ICE, even with the compile error above.
struct S([u8; <u8 as A>::B]);
```

This happens, because we bail out from `codegen_select_candidate` with an error if the selected impl has unconstrained params to avoid leaking infer vars out of a query. `Instance::try_resolve` will then return `Ok(None)`, which for assoc consts roughly means "this const can't be evaluated in a generic context" and is treated as such: https://github.com/rust-lang/rust/blob/71e06b9c59d6af50fdc55aed75620493d29baf98/compiler/rustc_middle/src/mir/interpret/queries.rs#L84 (and this can ICE if the const isn't generic: rust-lang#135617).

However, here `<u8 as A>::B` is definitely not "too generic" and also not unresolvable due to an unsatisfiable `u8: A` bound, so I've included the second commit to change the result of `Instance::try_resolve` from `Ok(None)` to `Err(ErrorGuaranteed)` when resolving an assoc item to an impl with unconstrained generic params. This has the effect that `<u8 as A>::B` will now be normalized to `ConstKind::Error` in the example above.

This properly fixes rust-lang#137308, by no longer treating `<u8 as A>::B` as unresolvable even though it clearly has a unique impl that it resolves to. It also has the effect of changing the layout error from `Unknown` ("the type may be valid but has no sensible layout") to `ReferencesError` ("a non-layout error is reported elsewhere") which seems more appropriate.

r? ``@compiler-errors``
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 21, 2025
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#136458 (Do not deduplicate list of associated types provided by dyn principal)
 - rust-lang#136474 ([`compiletest`-related cleanups 3/7] Make the distinction between sources root vs test suite sources root in compiletest less confusing)
 - rust-lang#136592 (Make sure we don't overrun the stack in canonicalizer)
 - rust-lang#136787 (Remove `lifetime_capture_rules_2024` feature)
 - rust-lang#137180 (Give `global_asm` a fake body to store typeck results, represent `sym fn` as a hir expr to fix `sym fn` operands with lifetimes)
 - rust-lang#137257 (Ignore fake borrows for packed field check)
 - rust-lang#137348 (More sophisticated span trimming for suggestions)
 - rust-lang#137399 (fix ICE in layout computation with unnormalizable const)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 22, 2025
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#136458 (Do not deduplicate list of associated types provided by dyn principal)
 - rust-lang#136474 ([`compiletest`-related cleanups 3/7] Make the distinction between sources root vs test suite sources root in compiletest less confusing)
 - rust-lang#136592 (Make sure we don't overrun the stack in canonicalizer)
 - rust-lang#136787 (Remove `lifetime_capture_rules_2024` feature)
 - rust-lang#137207 (Add #[track_caller] to Duration Div impl)
 - rust-lang#137245 (Tweak E0277 when predicate comes indirectly from ?)
 - rust-lang#137257 (Ignore fake borrows for packed field check)
 - rust-lang#137399 (fix ICE in layout computation with unnormalizable const)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 6352044 into rust-lang:master Feb 22, 2025
6 checks passed
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Feb 22, 2025
Rollup merge of rust-lang#137399 - lukas-code:oopsie-woopsie, r=compiler-errors

fix ICE in layout computation with unnormalizable const

The first commit reverts half of 7a667d2, where I removed a case from `layout_of` for handling non-generic unevaluated consts in array length, that I incorrectly assumed to be unreachable. This can actually happen with the combination of `feature(generic_const_exprs)` and `feature(trivial_bounds)`, because GCE makes anon consts inherit their parent's predicates and with an impossible predicate like `u8: A` it's possible to have an array whose length is an associated const like `<u8 as A>::B` that is not generic, but also can't be normalized:

```rust
#![feature(generic_const_exprs)]
#![feature(trivial_bounds)]

trait A {
    const B: usize;
}

// With GCE + trivial bounds this definition is not a compile error.
// Computing the layout of this type shouldn't ICE.
struct S([u8; <u8 as A>::B])
where
    u8: A;
```

---

The first commit also incidentally fixes rust-lang#137308, which also managed to get an unnormalizable assoc const into an array length:

```rust
trait A {
    const B: usize;
}

impl<C: ?Sized> A for u8 { //~ ERROR: the type parameter `C` is not constrained
    const B: usize = 42;
}

// Computing the layout of this type shouldn't ICE, even with the compile error above.
struct S([u8; <u8 as A>::B]);
```

This happens, because we bail out from `codegen_select_candidate` with an error if the selected impl has unconstrained params to avoid leaking infer vars out of a query. `Instance::try_resolve` will then return `Ok(None)`, which for assoc consts roughly means "this const can't be evaluated in a generic context" and is treated as such: https://github.com/rust-lang/rust/blob/71e06b9c59d6af50fdc55aed75620493d29baf98/compiler/rustc_middle/src/mir/interpret/queries.rs#L84 (and this can ICE if the const isn't generic: rust-lang#135617).

However, here `<u8 as A>::B` is definitely not "too generic" and also not unresolvable due to an unsatisfiable `u8: A` bound, so I've included the second commit to change the result of `Instance::try_resolve` from `Ok(None)` to `Err(ErrorGuaranteed)` when resolving an assoc item to an impl with unconstrained generic params. This has the effect that `<u8 as A>::B` will now be normalized to `ConstKind::Error` in the example above.

This properly fixes rust-lang#137308, by no longer treating `<u8 as A>::B` as unresolvable even though it clearly has a unique impl that it resolves to. It also has the effect of changing the layout error from `Unknown` ("the type may be valid but has no sensible layout") to `ReferencesError` ("a non-layout error is reported elsewhere") which seems more appropriate.

r? ```@compiler-errors```
@rustbot rustbot added this to the 1.87.0 milestone Feb 22, 2025
@lukas-code lukas-code deleted the oopsie-woopsie branch February 22, 2025 08:47
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-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: failed to normalize const, but it is not generic: UnevaluatedConst
4 participants