Fix ICE in transmutability error reporting when type aliases are normalized

[zedddie]

Fixes #151462

Transmutability error reporting hit an ICE when type aliases were normalized for diagnostics. For example, when type
JustUnit = () normalizes to (), the check passes unexpectedly even though the original check with JustUnit failed.

Fixed by adding a retry in the Answer::Yes arm that checks with the root obligation's types before panicking. The retry only occurs when the root obligation differs and is a Transmute trait predicate.

Also added a test that reproduces the original ICE.

[rustbot]

r? @jdonszelmann

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

[Kivooeo]

Suggested change

	if obligation.predicate != root_obligation.predicate {
	if let ty::PredicateKind::Clause(ty::ClauseKind::Trait(root_pred)) =
	root_obligation.predicate.kind().skip_binder()
	&& root_pred.def_id() == trait_pred.trait_ref.def_id
	if obligation.predicate != root_obligation.predicate
	&& let ty::PredicateKind::Clause(ty::ClauseKind::Trait(root_pred)) =
	root_obligation.predicate.kind().skip_binder()
	&& root_pred.def_id() == trait_pred.trait_ref.def_id

[Kivooeo]

Btw, I'm curious, there is a comment

rust/compiler/rustc_trait_selection/src/error_reporting/traits/fulfillment_errors.rs

Line 2916 in 4742769

// Should never get a Yes at this point! We already ran it before, and did not get a Yes.

If I understand this correctly, we should never reach this branch, but for some point we do, this brings me to believe that the problem occurs somewhere earlier

[zedddie]

Yes, I've thought about this too, and I tried to use root obligation check earlier to leave the Answer::Yes arm untouched, but it seems to regress diagnostics like bool-mut test(/tests/ui/transmutability/primitives/bool-mut.stderr) from

error[E0277]: `u8` cannot be safely transmuted into `bool`
   ^^^^^^^^^^^^^^^ at least one value of `u8` isn't a bit-valid value of `bool`

to

error[E0277]: `&mut bool` cannot be safely transmuted into `&mut u8`
   ^^^^^^^^^^^^^^^ unsatisfied trait bound
   = help: the nightly-only, unstable trait `TransmuteFrom<u8, ...>` is not implemented for `bool`

which looks incorrect to me. I've thought about this and recheck with root obligation to ensure we are dealing with same type in the Answer::Yes seems reasonable. If it is an incorrect approach I'd like to reimplement this check earlier in error reporting logic, leaving Answer::Yes untouched.

[zedddie]

Actually, It seems to be there is a better place for this check which will not cause diagnostics regression, I will deal with that tommorow

[zedddie]

I am a bit stuck in finding the right place for a check, as it will recheck after transmutability inconsistency, which is basically the same Answer::Yes handling logic but in other place, so I will wait for reviewer to clarify and decide next moves.

[Kivooeo]

that's totally fine! thanks for getting back and explain your reasoning, that's very valuable

but i'm afraid that i would not be able to dive into this because of being busy this week

[jdonszelmann]

@zedddie where is the place you're referring to? could you factor it out into a function?

[zedddie]

In the report_selection_error function before we call get_safe_transmute_error_and_reason. I thought about adding a second check with root obligations after we get inconsistency when trying to get transmute error and reason.

About factoring out to a function, what comes to mind is creating a function which will check is_transmutable and then if got inconsistency, recheck with root obligations, and if not, will proceed with the normalized.

Am I on the right track?

[jdonszelmann]

Yea, that seems reasonable, and then because it follows largely the same code path we can be reasonably sure the same cases do and don't error

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[zedddie]

after some time away from computer I think my function is a bit unreadable, I will refactor it a bit

[zedddie]

@jdonszelmann hi, can you take a look please? does it look good or I should change something?

[jdonszelmann]

@bors r+ rollup

[rust-bors]

📌 Commit a257fb9 has been approved by jdonszelmann

It is now in the queue for this repository.