Handle normalization failure in struct_tail_erasing_lifetimes

[gurry]

Fixes #113272

The ICE occurred because the struct being normalized had an error. This PR adds some defensive code to guard against that.

[rustbot]

r? @wesleywiser

rustbot has assigned @wesleywiser.
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

[compiler-errors]

Please provide a detailed explanation for why this actually fails. Specifically, I don't think you have actually explained where this call to struct_tail_with_normalize is being located -- knowing where that happens is very important for making sure that this can't be approached some other way.

[gurry]

Please provide a detailed explanation for why this actually fails. Specifically, I don't think you have actually explained where this call to struct_tail_with_normalize is being located -- knowing where that happens is very important for making sure that this can't be approached some other way.

This is the failing code:

trait Trait {
    type RefTarget;
}

impl Trait for () where Missing: Trait {}

struct Other {
    data: <() as Trait>::RefTarget,
}

fn main() {
    unsafe {
        std::mem::transmute::<Option<()>, Option<&Other>>(None);
    }
}

The ICE'ing code path begins in the typeck of main and eventually hits the call to normalize_erasing_regions as shown in the back trace below:

  22:     0x7f645488cc4d - rustc_middle[9f74ec5cb00a905a]::util::bug::bug_fmt
  23:     0x7f64549cf295 - <rustc_middle[9f74ec5cb00a905a]::ty::normalize_erasing_regions::NormalizeAfterErasingRegionsFolder>::normalize_generic_arg_after_erasing_regions
  24:     0x7f64549cef43 - <rustc_middle[9f74ec5cb00a905a]::ty::Ty as rustc_type_ir[2174c24ce5957ab4]::fold::TypeFoldable<rustc_middle[9f74ec5cb00a905a]::ty::context::TyCtxt>>::fold_with::<rustc_middle[9f74ec5cb00a905a]::ty::normalize_erasing_regions::NormalizeAfterErasingRegionsFolder>
  25:     0x7f6454c26fec - <rustc_middle[9f74ec5cb00a905a]::ty::context::TyCtxt>::struct_tail_erasing_lifetimes
  26:     0x7f64554e6d15 - <rustc_middle[9f74ec5cb00a905a]::ty::layout::SizeSkeleton>::compute
  27:     0x7f645690e0c1 - <rustc_middle[9f74ec5cb00a905a]::ty::layout::SizeSkeleton>::compute::{closure#1}
  28:     0x7f64554e6fd7 - <rustc_middle[9f74ec5cb00a905a]::ty::layout::SizeSkeleton>::compute
  29:     0x7f64559e88f2 - <rustc_hir_typeck[510371791de31c85]::fn_ctxt::FnCtxt>::check_transmute
  30:     0x7f6454d17195 - rustc_hir_typeck[510371791de31c85]::typeck

This happens when we are trying to compute the SizeSkeleton for Option<&Other>.

The underlying reason for the normalization failure of Option<&Other> is that Other is carrying an error because its data field is projecting RefTarget on () which is actually not implemented anywhere.

[compiler-errors]

I am not at my computer, but looking at the backtracr, I think the fix can just be localized to SizeSkeleton then. I don't think we need to touch struct_tail_with_normalize.

Probably just by manually calling struct_tail_with_normalize with a callback that doesn't ICE.

[gurry]

Yeah, that's an option. But doing it in struct_tail_erasing_lifetimes is going to help us in the future because any type that has an error, even from locations other than SizeSkeleton::compute, is likely to ICE in struct_tail_erasing_lifetimes anyway.

[compiler-errors]

That's the problem, though. I consider it a bug if we are ever calling struct_tail_erasing_regions when we have struct definitions with errors. My understanding of that function is that it's only really correct to be used during codegen (or codegen-like operations) -- anywhere else seems suspicious, and you haven't provided any evidence that other callsites deserve this treatment.

If there are enough cases that we want a separate, fallible version of that function, we should be adding that separately. For example, look at the difference between normalize_erasing_regions and try_normalize_erasing_regions.

[gurry]

That's the problem, though. I consider it a bug if we are ever calling struct_tail_erasing_regions when we have struct definitions with errors. My understanding of that function is that it's only really correct to be used during codegen (or codegen-like operations) -- anywhere else seems suspicious, and you haven't provided any evidence that other callsites deserve this treatment.

If there are enough cases that we want a separate, fallible version of that function, we should be adding that separately. For example, look at the difference between normalize_erasing_regions and try_normalize_erasing_regions.

In the past I've been tempted to do just that i.e. create a fallible version of it. Maybe I'll do that now because the need for such a function is likely to grow thanks to #120847. That PR makes it much more likely that types carrying errors will reach typeck and later phases instead of being stopped at wfcheck.

[gurry]

Are you okay with me creating a fallible version of struct_tail_erasing_regions? I'll wait for your answer before I do it.

[compiler-errors]

No, I have just said that there's no evidence that another callsite needs to be treated like this. Please do it in SizeSkeleton by manually calling struct_tail_with_normalize for now.

[gurry]

No, I have just said that there's no evidence that another callsite needs to be treated like this.

I thought when you said that you were discouraging making normalize_erasing_regions itself fallible not creating a separate version of it which is fallible. I was suggesting the latter.

[compiler-errors]

r=me after changing this to a delay span bug

@rustbot author

@rustbot label -S-waiting-on-author +S-waiting-on-review

@bors r+ rollup

📌 Commit 0c71c9d has been approved by compiler-errors

It is now in the queue for this repository.

This appears to have been merged in #124890, but GitHub did not auto-close it. Maybe there was something related to the merge conflict? Anyway, I recommend closing this after confirming it is merged correctly.

oh no :( bors merged an old version!

@gurry can you reopen this PR applying the changes i last asked for?

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

Actually, I did it here: #124909

Actually, I did it here: #124909

Thanks @compiler-errors