CTFE: avoid emitting a hard error on generic normalization failures

[lcnr]

Fixes #149081.

The fix is quite unsatisfying and should not be necessary, cc #149283. That change is significantly more involved. This temporary fix introduces some unnecessary complexity and may hide other type system bugs.

cc @rust-lang/types I think we should try to fix issue #149283 in the near future and then remove this hack again.

I originally intended a more targeted fix. I wanted to skip evaluating constants in MIR opts if their body was polymorphic and the current generic arguments still reference generic parameters. Notes from looking into this:

• we only fetch the MIR in the eval_to_allocation_raw query
• figuring out which MIR to use is hard
  • dealing with trivial consts is annoying
  • need to resolve instances for associated consts
  • implementing this by hand is hard
• inlining handles this issue by bailing on literally all normalization failures, even the ones that imply an unsoundness
  • try_normalize_after_erasing_regions generally does two things
    • deal with ambiguity after inlining
    • deal with error tainting issues (please don't, we should stop doing that)
• CTFE could be changed to always silently ignore normalization failures if we're in a generic body
  • hides actual bugs <- went with this option

r? types

[rustbot]

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri

Some changes occurred to the CTFE machinery

cc @RalfJung, @oli-obk, @lcnr

[rust-log-analyzer]

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

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

warning ../../../../../package.json: License should be a valid SPDX license expression
warning No license field
[1/4] Resolving packages...
[2/4] Fetching packages...
error Error: https://registry.yarnpkg.com/ms/-/ms-2.1.3.tgz: Request failed "500 Internal Server Error"
    at ResponseError.ExtendableBuiltin (/node/lib/node_modules/yarn/lib/cli.js:696:66)
    at new ResponseError (/node/lib/node_modules/yarn/lib/cli.js:802:124)
    at Request.<anonymous> (/node/lib/node_modules/yarn/lib/cli.js:66750:16)
    at Request.emit (node:events:518:28)
    at module.exports.Request.onRequestResponse (/node/lib/node_modules/yarn/lib/cli.js:142287:10)
    at ClientRequest.emit (node:events:518:28)
    at HTTPParser.parserOnIncomingClient (node:_http_client:698:27)
    at HTTPParser.parserOnHeadersComplete (node:_http_common:119:17)
    at TLSSocket.socketOnData (node:_http_client:540:22)
    at TLSSocket.emit (node:events:518:28)
info Visit https://yarnpkg.com/en/docs/cli/install for documentation about this command.
yarn install did not exit successfully

thread 'main' (59568) panicked at src/tools/rustdoc-gui-test/src/main.rs:63:10:
unable to install browser-ui-test: Custom { kind: Other, error: "yarn install returned exit code exit status: 1" }
stack backtrace:
   0: __rustc::rust_begin_unwind
             at /rustc/3b4dd9bf1410f8da6329baa36ce5e37673cbbd1f/library/std/src/panicking.rs:698:5
   1: core::panicking::panic_fmt
             at /rustc/3b4dd9bf1410f8da6329baa36ce5e37673cbbd1f/library/core/src/panicking.rs:80:14

[RalfJung]

😢
But I can live with this as a temporary work-around.

[lcnr]

@rust-lang/types would like to beta backport this as early as possible ideally. Can I get one of you to quickly look at this PR?

[lqd]

@lcnr Are you confident we can fix #149283 soon-ish? (I'm concerned about potential breakage issues we have to deal with, which if I remember correctly were encountered in previous efforts)

Either way, this PR also makes sense to me as a temporary fix. So r=me should you want it.

[BoxyUwU]

I originally intended a more targeted fix. I wanted to skip evaluating constants in MIR opts if their body was polymorphic and the current generic arguments still reference generic parameters

In the long term I think this is the right fix 🤔

Before this PR were we getting similar kinds of ICEs due to opaque types? I was looking at the doc comments for NormalizationFailure which link to tests/ui/layout/normalization-failure.rs which is also about layout computation failure due for generic bodies.

I'm not sure I feel confident in being able to judge whether there are any weird side effects from this PR or not. I'd certainly feel better to revert #147793 and then land this "normally" or if it really will be the near future that we fix #149283 then land that and then re-land #147793

[lcnr]

Good catch @BoxyUwU, there are apparently other "not quite a bug" reasons for normalization failures in well-formed programs. This PR causes us to also weaken CTFE errors due to param-env shadowing from a hard-error to simply returning TooGeneric :x

Now, not erroring in these cases also feels desirable to me. However, this is something I did not consider when writing this PR. More thoughts on this later

[lcnr]

@lcnr Are you confident we can fix #149283 soon-ish? (I'm concerned about potential breakage issues we have to deal with, which if I remember correctly were encountered in previous efforts)

soon-ish? no

it depends on whether somebody ends up working on this, it'd probably take a few months to get something working here (with mentoring) for a somewhat experienced contributor.

[oli-obk]

Short term let's land this. While I expect it to be possible to construct some edge case where we hide a bug and start erroring again when we fix #149283, I expect the fallout from that to be very small if nonexistent.

@bors r+

[bors]

📌 Commit 02d84c8 has been approved by oli-obk

It is now in the queue for this repository.

[oli-obk]

@bors r=lqd,oli-obk

[bors]

💡 This pull request was already approved, no need to approve it again.

• There's another pull request that is currently being tested, blocking this pull request: Update cargo submodule #149555

[bors]

📌 Commit 02d84c8 has been approved by lqd,oli-obk

It is now in the queue for this repository.

[apiraino]

Beta backport accepted as per compiler team on Zulip. A backport PR will be authored by the release team at the end of the current development cycle. Backport labels handled by them.

Note: before moving on with the backport @wesleywiser will investigate if we can revert the culprit PR first (comment). So this backport is approved but in stand-by. We will comment later on.

@rustbot label +beta-accepted