Implement version of normalize_erasing_regions that allows for normalization failure

[b-naber]

Fixes #59324
Fixes #67684
Fixes #69398
Fixes #71113
Fixes #82079
Fixes #85103
Fixes #88856
Fixes #91231
Fixes #91234

Previously we called normalize_erasing_regions inside layout_of. normalize_erasing_regions assumes that the normalization succeeds. Since some layout_of calls happen before typecheck has finished, we introduce a new variant that allows for returning an error.

[rust-highfive]

Some changes occured to the CTFE / Miri engine

cc @rust-lang/miri

[rust-highfive]

r? @matthewjasper

(rust-highfive has picked a reviewer for you, use r? to override)

[oli-obk]

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

[bors]

⌛ Trying commit 11f105cd6e9635ae233a61121bdf330f1b040582 with merge 7cd727ea460dd27fc8f52e430e9cd2301eec1439...

[RalfJung]

Do we even care about the size of this type? We care about InterpErrorInfo but that is a Box so it is independent of the size of InterpError.

So I think I would favor removing this assertion, it might just lead someone to unnecessarily introduce another unnecessary Box indirection.

[jackh726]

This is great :) does this fix the test in #82039?

[camelid]

Does this fix #91231 as well, since it's very similar to #91234 (which this is marked as fixing)?

[jackh726]

Probably just make these two "normalizing {}", so the error outputs are nicer

[jackh726]

So, this is never actually emitted? Makes me wonder if we actually need to keep track of what we couldn't normalize.

[b-naber]

yeah I noticed that too and was debating whether we need the type, but think that it makes sense to output the type for which normalization fails here. There are a couple other issues which triggered this ICE (but not through layout_of and which have yet to be fixed (although I think the new function would make that fairly straightforward)), and we might need this type information there.

I also added another test for issue 85103 which emits this error.

[jackh726]

I think this deserves a comment on why failing to fully normalize is valid here. I would potentially imagine that by the time we need to know layout, we should have all the information to normalize (though I'm not sure, this might not be true)

[b-naber]

I think it might make sense to have two different versions of layout_of. Currently most of the calls of that function happen after typecheck and normalization is always guaranteed to succeed, but there are some calls in typecheck and some lints that use it. Does it makes sense to have one version which requires normalization to succeed and another in which it can fail?

[jackh726]

This is a good question! In #82039, IIRC we were basically coming to the conclusion that we shouldn't be trying to get the layout of things until after typechecking. So, I think splitting out layout_of into two version is a decent intermediate step to "audit", in some way, incorrect behavior.

I think this would be better as a followup. Can you add a FIXME or file an issue?

[camelid]

Does this fix #85103? (cc #85485)

[bors]

💔 Test failed - checks-actions

[camelid]

You'll need to update rustdoc:

error[E0004]: non-exhaustive patterns: `Err(NormalizationFailure(_, _))` not covered
681
    --> src/librustdoc/html/render/print_item.rs:1705:11
682
     |
683
1705 |     match tcx.layout_of(param_env.and(ty)) {
684
     |           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ pattern `Err(NormalizationFailure(_, _))` not covered
685
     |
686
    ::: /checkout/library/core/src/result.rs:512:5
687
     |
688
512  |     Err(#[stable(feature = "rust1", since = "1.0.0")] E),
689
     |     --- not covered
690
     |
691
     = help: ensure that all possible cases are being handled, possibly by adding wildcards or more match arms
692
     = note: the matched value is of type `Result<TyAndLayout<&TyS>, rustc_middle::ty::layout::LayoutError>`
693

[b-naber]

@jackh726 The test in #82039 is not fixed by this, since I only used try_normalize_erasing_regions in layout_of. There are a couple of other calls of normalize_erasing_regions which are used before normalization is guaranteed to succeed. I can change those to use this new function.

@camelid The two issues you mentioned are fixed by this PR, added tests.

[b-naber]

Can anybody request another another timer run once CI passes, please?

[oli-obk]

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

[bors]

⌛ Trying commit ce0e6f8c86311589ecc6df277b37bbe236e0e8ca with merge 3393f73e33f01001d1dbe2362ae0d7311dba2447...

[b-naber]

@bors r=jackh276

[bors]

📌 Commit 6952470 has been approved by jackh276

[bors]

⌛ Testing commit 6952470 with merge f04a2f4...

[bors]

☀️ Test successful - checks-actions
Approved by: jackh276
Pushing f04a2f4 to master...

[jyn514]

Ooh, I wonder if this could be used for #82692 :)

[rust-timer]

Finished benchmarking commit (f04a2f4): comparison url.

Summary: This change led to small relevant regressions 😿 in compiler performance.

• Small regression in instruction counts (up to 0.9% on incr-full builds of ctfe-stress-4)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression

[rylev]

@b-naber @jackh726 as you can see this PR yielded some performance regressions (albeit not huge ones and mostly in stress tests. Looking at the self profiling info, nothing jumped out at me. A run of cachegrind may be in order, but I unfortunately don't have time to do that right now. Any thoughts on potential causes of the regressions?

[b-naber]

@rylev performance regression might be due to the fact that we don't cache normalize_erasing_regions queries in functions that now use th try version. We might be able to improve this by implementing normalize_erasing_regions in terms of try_normalize_erasing_regions. I can implement this tomorrow.