Do not ICE when trying to get layout of an unexpected type

[gurry]

Fixes #126942

See issue for why the ICE occurs.

In this PR we return an error from layout query instead of raising a bug when we encounter a type that we cannot compute the layout of.

Edit

Put the fix in try_normalize_erasing_regions instead of layout_of_uncached because we were triggering a debug_assertion in the former.

Also fixes #127804

[rustbot]

r? @TaKO8Ki

rustbot has assigned @TaKO8Ki.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[gurry]

@rustbot author

[compiler-errors]

Why didn't you put this check into check_static_inhabited? Seems like a more specific place to put it, according to the ICE message in the issue you linked.

[gurry]

I felt that it's cleaner to put it in layout because if I put it in check_static_inhabited I'll have to make an extra call totry_normalize_erasing_regions which layout already does.

[compiler-errors]

Well it looks like the test still ICEs anyways 🤔. I would encourage you to try to make this fix more specific if possible, to avoid changing existing assertions which were added as a sanity check.

[gurry]

Yeah, the test is ICEing in try_normalize_erasing_regions and even if I make it more specific I'll still have to normalize the type and therefore it might still ICE.

It works fine on my MacBook and I don't have a Linux machine. But I guess the test's ICE is more due to x64 rather than the OS. So I'm going to test it on a Windows machine (which I do have) and see if I can fix it.

[gurry]

I'll also try to see if I can make it more specific nonetheless.

[compiler-errors]

This is because of a debug assertion, which you need to enable in your config.toml.

This has nothing to do with operating systems or CPU architectures--it's a type system bug after all.

[gurry]

Ah, thanks for that. Saved me a lot of time.

[gurry]

I tried to put the fix in check_static_inhabited, but I would have had to call try_normalize_erasing_regions from there before calling layout_of and it is ICEing due to the debug_assertion. So I had make the change in try_normalize_erasing_regions itself to remove the debug_assertion and return an error instead.

If you have any suggestions to make it more specific, please let me know.

[gurry]

@rustbot review

[rust-log-analyzer]

The job x86_64-gnu-llvm-17 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

 - LegacyKeyValueFormat: "ENV key=value" should be used instead of legacy "ENV key value" format (line 46)
 - LegacyKeyValueFormat: "ENV key=value" should be used instead of legacy "ENV key value" format (line 49)
 - LegacyKeyValueFormat: "ENV key=value" should be used instead of legacy "ENV key value" format (line 61)
##[endgroup]
Setting extra environment values for docker:  --env ENABLE_GCC_CODEGEN=1 --env GCC_EXEC_PREFIX=/usr/lib/gcc/
[CI_JOB_NAME=x86_64-gnu-llvm-17]
---
sccache: Starting the server...
##[group]Configure the build
configure: processing command line
configure: 
configure: build.configure-args := ['--build=x86_64-unknown-linux-gnu', '--llvm-root=/usr/lib/llvm-17', '--enable-llvm-link-shared', '--set', 'rust.thin-lto-import-instr-limit=10', '--set', 'change-id=99999999', '--enable-verbose-configure', '--enable-sccache', '--disable-manage-submodules', '--enable-locked-deps', '--enable-cargo-native-static', '--set', 'rust.codegen-units-std=1', '--set', 'dist.compression-profile=balanced', '--dist-compression-formats=xz', '--set', 'rust.lld=false', '--disable-dist-src', '--release-channel=nightly', '--enable-debug-assertions', '--enable-overflow-checks', '--enable-llvm-assertions', '--set', 'rust.verify-llvm-ir', '--set', 'rust.codegen-backends=llvm,cranelift,gcc', '--set', 'llvm.static-libstdcpp', '--enable-new-symbol-mangling']
configure: target.x86_64-unknown-linux-gnu.llvm-config := /usr/lib/llvm-17/bin/llvm-config
configure: llvm.link-shared     := True
configure: rust.thin-lto-import-instr-limit := 10
configure: change-id            := 99999999
---
##[group]Testing stage2 compiletest suite=crashes mode=crashes (x86_64-unknown-linux-gnu)

running 224 tests
........................................................................................  88/224
......2024-07-17T04:57:16.940333Z ERROR compiletest::runtest: fatal error, panic: "crashtest no longer crashes/triggers ICE, horray! Please give it a meaningful name, add a doc-comment to the start of the test explaining why it exists and move it to tests/ui or wherever you see fit. Adding 'Fixes #<issueNr>' to your PR description ensures that the corresponding ticket is auto-closed upon merge."
[crashes] tests/crashes/118590.rs ... F
[crashes] tests/crashes/118590.rs ... F
...............................2024-07-17T04:57:17.199199Z ERROR compiletest::runtest: fatal error, panic: "crashtest no longer crashes/triggers ICE, horray! Please give it a meaningful name, add a doc-comment to the start of the test explaining why it exists and move it to tests/ui or wherever you see fit. Adding 'Fixes #<issueNr>' to your PR description ensures that the corresponding ticket is auto-closed upon merge."
[crashes] tests/crashes/123141.rs ... F
................................................. 176/224
................................................. 176/224
........2024-07-17T04:57:17.676914Z ERROR compiletest::runtest: fatal error, panic: "crashtest no longer crashes/triggers ICE, horray! Please give it a meaningful name, add a doc-comment to the start of the test explaining why it exists and move it to tests/ui or wherever you see fit. Adding 'Fixes #<issueNr>' to your PR description ensures that the corresponding ticket is auto-closed upon merge."
[crashes] tests/crashes/107362.rs ... F
.......................................

failures:
failures:

---- [crashes] tests/crashes/118590.rs stdout ----

error: crashtest no longer crashes/triggers ICE, horray! Please give it a meaningful name, add a doc-comment to the start of the test explaining why it exists and move it to tests/ui or wherever you see fit. Adding 'Fixes #<issueNr>' to your PR description ensures that the corresponding ticket is auto-closed upon merge.
thread '[crashes] tests/crashes/118590.rs' panicked at src/tools/compiletest/src/runtest.rs:377:18:
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

---- [crashes] tests/crashes/123141.rs stdout ----


error: crashtest no longer crashes/triggers ICE, horray! Please give it a meaningful name, add a doc-comment to the start of the test explaining why it exists and move it to tests/ui or wherever you see fit. Adding 'Fixes #<issueNr>' to your PR description ensures that the corresponding ticket is auto-closed upon merge.
thread '[crashes] tests/crashes/123141.rs' panicked at src/tools/compiletest/src/runtest.rs:377:18:

---- [crashes] tests/crashes/107362.rs stdout ----


error: crashtest no longer crashes/triggers ICE, horray! Please give it a meaningful name, add a doc-comment to the start of the test explaining why it exists and move it to tests/ui or wherever you see fit. Adding 'Fixes #<issueNr>' to your PR description ensures that the corresponding ticket is auto-closed upon merge.
thread '[crashes] tests/crashes/107362.rs' panicked at src/tools/compiletest/src/runtest.rs:377:18:


failures:
    [crashes] tests/crashes/118590.rs

[gurry]

The job x86_64-gnu-llvm-17 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

 - LegacyKeyValueFormat: "ENV key=value" should be used instead of legacy "ENV key value" format (line 46)
 - LegacyKeyValueFormat: "ENV key=value" should be used instead of legacy "ENV key value" format (line 49)
 - LegacyKeyValueFormat: "ENV key=value" should be used instead of legacy "ENV key value" format (line 61)
##[endgroup]
Setting extra environment values for docker:  --env ENABLE_GCC_CODEGEN=1 --env GCC_EXEC_PREFIX=/usr/lib/gcc/
[CI_JOB_NAME=x86_64-gnu-llvm-17]
---
sccache: Starting the server...
##[group]Configure the build
configure: processing command line
configure: 
configure: build.configure-args := ['--build=x86_64-unknown-linux-gnu', '--llvm-root=/usr/lib/llvm-17', '--enable-llvm-link-shared', '--set', 'rust.thin-lto-import-instr-limit=10', '--set', 'change-id=99999999', '--enable-verbose-configure', '--enable-sccache', '--disable-manage-submodules', '--enable-locked-deps', '--enable-cargo-native-static', '--set', 'rust.codegen-units-std=1', '--set', 'dist.compression-profile=balanced', '--dist-compression-formats=xz', '--set', 'rust.lld=false', '--disable-dist-src', '--release-channel=nightly', '--enable-debug-assertions', '--enable-overflow-checks', '--enable-llvm-assertions', '--set', 'rust.verify-llvm-ir', '--set', 'rust.codegen-backends=llvm,cranelift,gcc', '--set', 'llvm.static-libstdcpp', '--enable-new-symbol-mangling']
configure: target.x86_64-unknown-linux-gnu.llvm-config := /usr/lib/llvm-17/bin/llvm-config
configure: llvm.link-shared     := True
configure: rust.thin-lto-import-instr-limit := 10
configure: change-id            := 99999999
---
##[group]Testing stage2 compiletest suite=crashes mode=crashes (x86_64-unknown-linux-gnu)

running 224 tests
........................................................................................  88/224
......2024-07-17T04:57:16.940333Z ERROR compiletest::runtest: fatal error, panic: "crashtest no longer crashes/triggers ICE, horray! Please give it a meaningful name, add a doc-comment to the start of the test explaining why it exists and move it to tests/ui or wherever you see fit. Adding 'Fixes #<issueNr>' to your PR description ensures that the corresponding ticket is auto-closed upon merge."
[crashes] tests/crashes/118590.rs ... F
[crashes] tests/crashes/118590.rs ... F
...............................2024-07-17T04:57:17.199199Z ERROR compiletest::runtest: fatal error, panic: "crashtest no longer crashes/triggers ICE, horray! Please give it a meaningful name, add a doc-comment to the start of the test explaining why it exists and move it to tests/ui or wherever you see fit. Adding 'Fixes #<issueNr>' to your PR description ensures that the corresponding ticket is auto-closed upon merge."
[crashes] tests/crashes/123141.rs ... F
................................................. 176/224
................................................. 176/224
........2024-07-17T04:57:17.676914Z ERROR compiletest::runtest: fatal error, panic: "crashtest no longer crashes/triggers ICE, horray! Please give it a meaningful name, add a doc-comment to the start of the test explaining why it exists and move it to tests/ui or wherever you see fit. Adding 'Fixes #<issueNr>' to your PR description ensures that the corresponding ticket is auto-closed upon merge."
[crashes] tests/crashes/107362.rs ... F
.......................................

failures:
failures:

---- [crashes] tests/crashes/118590.rs stdout ----

error: crashtest no longer crashes/triggers ICE, horray! Please give it a meaningful name, add a doc-comment to the start of the test explaining why it exists and move it to tests/ui or wherever you see fit. Adding 'Fixes #<issueNr>' to your PR description ensures that the corresponding ticket is auto-closed upon merge.
thread '[crashes] tests/crashes/118590.rs' panicked at src/tools/compiletest/src/runtest.rs:377:18:
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

---- [crashes] tests/crashes/123141.rs stdout ----


error: crashtest no longer crashes/triggers ICE, horray! Please give it a meaningful name, add a doc-comment to the start of the test explaining why it exists and move it to tests/ui or wherever you see fit. Adding 'Fixes #<issueNr>' to your PR description ensures that the corresponding ticket is auto-closed upon merge.
thread '[crashes] tests/crashes/123141.rs' panicked at src/tools/compiletest/src/runtest.rs:377:18:

---- [crashes] tests/crashes/107362.rs stdout ----


error: crashtest no longer crashes/triggers ICE, horray! Please give it a meaningful name, add a doc-comment to the start of the test explaining why it exists and move it to tests/ui or wherever you see fit. Adding 'Fixes #<issueNr>' to your PR description ensures that the corresponding ticket is auto-closed upon merge.
thread '[crashes] tests/crashes/107362.rs' panicked at src/tools/compiletest/src/runtest.rs:377:18:


failures:
    [crashes] tests/crashes/118590.rs

Woah! Didn't realise this one stone was gonna hit so many birds.

[compiler-errors]

This is not the right fix. This debug assertion exists for a good reason -- because we shouldn't be returning totally unconstrained inference variables from the normalize_after_erasing_regions query.

[compiler-errors]

In general, when fixing an ICE, I would appreciate if you looker deeper than just the surface. Suppressing a debug assertion from triggering by just removing the debug assertion is not typically the right answer.

[compiler-errors]

Firstly, it would be useful to understand why #126942 began to ICE in the first place. Specifically, it seems like @oli-obk caused this in #121154. This is a classic case of the types of ICEs introduced with this error refactoring work, so maybe he has some solutions in mind that are less invasive than just removing debug assertions.

Perhaps one solution to this would be to project associated types to ty::Error if we detect an impl with unconstrained arguments. That would require querifying the check_impl_wf function, or at least the enforce_impl_params_are_constrained part. Both of those seem like possibly better solution if they're not costly.

[compiler-errors]

Alternatively, it does seem a bit weird to me that we're checking whether static layouts are inhabited this early in compilation (check_mod_wf). I wonder if we can move this check around to somewhere else to avoid calling it when the impls aren't even well-formed.

[gurry]

Awesome. These are all good suggestions. I'll investigate further.

Thanks :)

[gurry]

This is not the right fix. This debug assertion exists for a good reason -- because we shouldn't be returning totally unconstrained inference variables from the normalize_after_erasing_regions query.

After this change we're not returning inference variables. We're returning an error. So it's not all bad. But still it's not the best solution.

[oli-obk]

Unfortunately, as with most wfcheck logic, we can't invoke it from anywhere else but top-level queries, as it invokes so many other type queries, that we run into cycles left and right. I did assume enforce_impl_params_are_constrained on its own could be small enough to allow us to call it from other queries, but it turns out it does some stuff for backwards compatibility that makes that hard: https://rust-lang.zulipchat.com/#narrow/stream/144729-t-types/topic/unconstrained.20lifetimes.20in.20impls

[gurry]

Closing in favour of #127973