TransmuteFrom: Gracefully handle unnormalized types and normalization errors by jswrenn · Pull Request #131112 · rust-lang/rust

jswrenn · 2024-10-01T19:31:35Z

~~Refactor to share code between TransmuteFrom's trait selection and error reporting code paths. Additionally normalizes the source and destination types, and gracefully handles normalization errors.~~

Fixes #130413

r? @compiler-errors

rustbot · 2024-10-01T19:31:43Z

r? @BoxyUwU

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

So I think this PR is overengineered 😅 I left some inline comments, but realized that they're kinda ignoring the fact that this isn't the right general fix, I think.

Factoring out the confirmation code may be useful, but right now it's tied together with unnecessary/incorrect normalization calls which mask the real issue here:

The correct fix is just to add checks in get_safe_transmute_error_and_reason for the invariants that the transmutability code currently relies on. Specifically, we should be checking the same things as we do in assemble_candidates_for_transmutability -- i.e.

        if obligation.predicate.has_non_region_param()

since right now, the transmutability code doesn't handle type/const parameters, and

        if obligation.has_non_region_infer()

which implies ambiguity.

compiler-errors · 2024-10-01T19:35:14Z

compiler/rustc_trait_selection/src/transmute.rs

+        use rustc_infer::infer::TyCtxtInferExt;
+
+        use crate::traits::ObligationCtxt;
+        let infcx = tcx.infer_ctxt().with_next_trait_solver(true).build();


Making a new inference context is definitely not the right approach.

compiler/rustc_trait_selection/src/transmute.rs

compiler-errors · 2024-10-01T19:55:46Z

Also unrelated, I believe the transmutability code should be resilient to NormalizationFailure layout errors. This PR doesn't, afaict, fix this ICE:

#![feature(transmutability)]

trait A {
    type AssocA;
}

trait B {
    type AssocB: std::mem::TransmuteFrom<()>;
}

impl<T> B for T
where
    for<'a> &'a i32: A,
{
    type AssocB = <&'static i32 as A>::AssocA;
}

… errors Fixes rust-lang#130413

compiler-errors · 2024-10-01T20:59:28Z

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

    ) -> GetSafeTransmuteErrorAndReason {
        use rustc_transmute::Answer;

+        if obligation.predicate.has_non_region_param() || obligation.has_non_region_infer() {


Could use a comment

jswrenn · 2024-10-01T21:00:03Z

tests/ui/transmutability/assoc-bound.rs

+where
+    T: A,
+{
+    type AssocB = T::AssocA; //~ERROR: the trait bound `<T as A>::AssocA: TransmuteFrom<(), Assume { alignment: false, lifetimes: false, safety: false, validity: false }>` is not satisfied [E0277]


I'm personally fine to punt on making this error message prettier — my top priority is making TransmuteFrom work — but happy to try to improve this further if you'd like to block the PR on it.

No, it's fine. But yeah, it could be customized to say something like "cannot transmute types with ty/ct params in it".

But happy to do that later.

jswrenn · 2024-10-01T21:00:07Z

@compiler-errors, you're right, that was a lot more simple: https://github.com/rust-lang/rust/pull/131112/files

compiler-errors · 2024-10-01T21:16:40Z

Also please run ./x.py test crashes, you need to remove the relevant crashes tests.

compiler-errors · 2024-10-03T13:11:24Z

@rustbot author

compiler-errors · 2024-10-03T19:21:33Z

I went ahead and removed the crashes and added that comment I asked for, since this is blocking (well, mostly conflicting with) #130950.

@bors r+ rollup

bors · 2024-10-03T19:21:35Z

📌 Commit bc5f952 has been approved by compiler-errors

It is now in the queue for this repository.

…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#131024 (Don't give method suggestions when method probe fails due to bad implementation of `Deref`) - rust-lang#131112 (TransmuteFrom: Gracefully handle unnormalized types and normalization errors) - rust-lang#131176 (.gitignore files for nix) - rust-lang#131183 (Refactoring to `OpaqueTyOrigin`) - rust-lang#131187 (Avoid ICE in coverage builds with bad `#[coverage(..)]` attributes) - rust-lang#131192 (Handle `rustc_query_impl` cases of `rustc::potential_query_instability` lint) - rust-lang#131197 (Avoid emptiness check in `PeekMut::pop`) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#131112 - jswrenn:fix-130413, r=compiler-errors TransmuteFrom: Gracefully handle unnormalized types and normalization errors ~~Refactor to share code between `TransmuteFrom`'s trait selection and error reporting code paths. Additionally normalizes the source and destination types, and gracefully handles normalization errors.~~ Fixes rust-lang#130413 r? `@compiler-errors`

rustbot assigned BoxyUwU Oct 1, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 1, 2024

compiler-errors suggested changes Oct 1, 2024

View reviewed changes

compiler-errors assigned compiler-errors and unassigned BoxyUwU Oct 1, 2024

This comment has been minimized.

Sign in to view

TransmuteFrom: Gracefully handle unnormalized types and normalization…

5b1a2b8

… errors Fixes rust-lang#130413

jswrenn force-pushed the fix-130413 branch from eaee913 to 5b1a2b8 Compare October 1, 2024 20:55

compiler-errors reviewed Oct 1, 2024

View reviewed changes

jswrenn commented Oct 1, 2024

View reviewed changes

jswrenn changed the title ~~TransmuteFrom: normalize types, unify confirmation and error reporting~~ TransmuteFrom: Gracefully handle unnormalized types and normalization errors Oct 1, 2024

This comment has been minimized.

Sign in to view

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 3, 2024

Remove crashes, add comment

bc5f952

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 3, 2024

matthiaskrgr mentioned this pull request Oct 3, 2024

Rollup of 7 pull requests #131215

Merged

bors merged commit 33b4947 into rust-lang:master Oct 4, 2024

rustbot added this to the 1.83.0 milestone Oct 4, 2024

This was referenced Oct 17, 2024

ICE: Inconsistent rustc_transmute::is_transmutable(...) result, got Yes #126377

Closed

ICE: upvar_tys called before capture types are inferred in rustc_type_ir/src/ty_kind/closure.rs #125881

Closed

Uh oh!

Conversation

jswrenn commented Oct 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Oct 1, 2024

Uh oh!

compiler-errors left a comment

Choose a reason for hiding this comment

Uh oh!

compiler-errors Oct 1, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

compiler-errors commented Oct 1, 2024

Uh oh!

This comment has been minimized.

compiler-errors Oct 1, 2024

Choose a reason for hiding this comment

Uh oh!

jswrenn Oct 1, 2024

Choose a reason for hiding this comment

Uh oh!

compiler-errors Oct 1, 2024

Choose a reason for hiding this comment

Uh oh!

compiler-errors Oct 1, 2024

Choose a reason for hiding this comment

Uh oh!

jswrenn commented Oct 1, 2024

Uh oh!

compiler-errors commented Oct 1, 2024

Uh oh!

This comment has been minimized.

compiler-errors commented Oct 3, 2024

Uh oh!

compiler-errors commented Oct 3, 2024

Uh oh!

bors commented Oct 3, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

jswrenn commented Oct 1, 2024 •

edited

Loading