Safe Transmute: Pass ErrorGuaranteed up to error reporting by bryangarza · Pull Request #110669 · rust-lang/rust

bryangarza · 2023-04-22T03:16:56Z

This patch updates the Answer enum used by Safe Transmute to have a variant for errors, and includes the ErrorGuaranteed type so that we know that an error was already emitted. Before this change, we would return Answer::Yes for failures, which gives the same end-result, but is confusing because Answer::Yes is supposed to mean that safe transmutation is possible. Now, it's clear when we have an error, and we can decide what to do with it during error reporting. Also, we now know for sure that an error was emitted for LayoutError (previously, we just assumed without confirming).

This patch updates the `Answer` enum used by Safe Transmute to have a variant for errors, and includes the `ErrorGuaranteed` type so that we know that an error was already emitted. Before this change, we would return `Answer::Yes` for failures, which gives the same end-result, but is confusing because `Answer::Yes` is supposed to mean that safe transmutation is possible. Now, it's clear when we have an error, and we can decide what to do with it during error reporting. Also, we now know for sure that an error was emitted for `LayoutError` (previously, we just assumed without confirming).

rustbot · 2023-04-22T03:17:01Z

r? @jackh726

(rustbot has picked a reviewer for you, use r? to override)

rustbot · 2023-04-22T03:17:04Z

Some changes occurred to the core trait solver

cc @rust-lang/initiative-trait-system-refactor

compiler-errors · 2023-04-22T03:19:26Z

compiler/rustc_transmute/src/layout/tree.rs


    impl<'tcx> From<LayoutError<'tcx>> for Err {
        fn from(err: LayoutError<'tcx>) -> Self {
+            let guar = rustc_middle::ty::tls::expect_compilation_to_fail();


A layout error does not imply that compilation is going to fail. This can happen when the type is sufficiently polymorphic, such as &T where T: ?Sized.

This would ICE, for example, when we relax this requirement:

rust/compiler/rustc_trait_selection/src/traits/select/candidate_assembly.rs

Lines 778 to 780 in 80a2ec4

if obligation.predicate.has_non_region_param() {

return;

}

If I'm understanding correctly, you're saying that a layout error may or may not cause compilation to fail? If so, I can change it to Unknown(Option<ErrorGuaranteed>), and use if let Some(guar) = with(|tcx| tcx.sess.is_compilation_going_to_fail()) to decide what to return in this From impl

No, I don't really think that relying on global state such as that for trait solver behavior is a good idea.

Gotcha, I can undo the changes in this section then

compiler-errors · 2023-04-22T03:33:51Z

compiler/rustc_transmute/src/maybe_transmutable/mod.rs

-                    (Err(Err::TypeError(_)), _) => Err(Answer::Yes),
-                    (_, Err(Err::TypeError(_))) => Err(Answer::Yes),
-                    (Err(Err::Unknown), _) => Err(Answer::Yes),
-                    (_, Err(Err::Unknown)) => Err(Answer::Yes),


Why not just return No here? If this is just to not annoy users with errors, we should be doing diagnostic deduplication on the error reporting side.

Yeah, I added deduplication here

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

Lines 743 to 754 in 0d74752

// Recompute the safe transmute reason and use that for the error reporting

match self.get_safe_transmute_error_and_reason(

obligation.clone(),

trait_ref,

span,

) {

SafeTransmuteError::ShouldReport { err_msg, explanation } => {

(err_msg, Some(explanation))

}

// An error is guaranteed to already have been reported at this point, no need to continue

SafeTransmuteError::ErrorGuaranteed(_) => return,

}

Is that what you mean?

The reason I made a separate Answer variant instead of reusing Answer::No is cause there is a semantic difference between safe transmute not being possible (Answer::No) and not knowing if safe transmute is possible because of an error that occurred when trying to get an answer (Answer::Err).

Well in the case of a layout error, it seems like you should be modeling this case with an additional variant that signifies "unknown".

bryangarza · 2023-04-25T02:06:33Z

@rustbot author

bors · 2023-04-25T08:08:47Z

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

JohnCSimon · 2023-06-17T14:32:01Z

@bryangarza

ping from triage - can you post your status on this PR? There hasn't been an update in a few months. Thanks!

FYI: when a PR is ready for review, send a message containing
@rustbot ready to switch to S-waiting-on-review so the PR is in the reviewer's backlog.

bryangarza · 2023-07-19T21:24:25Z

I'm gonna close since I integrated some of these changes into other PRs that I merged. No longer needed.

rustbot assigned jackh726 Apr 22, 2023

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. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver) labels Apr 22, 2023

compiler-errors reviewed Apr 22, 2023

View reviewed changes

compiler-errors self-assigned this Apr 22, 2023

jackh726 removed their assignment Apr 23, 2023

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 Apr 25, 2023

bryangarza closed this Jul 19, 2023

bryangarza deleted the safe-transmute-small-refactor-on-answer-type branch July 19, 2023 21:24

	// Recompute the safe transmute reason and use that for the error reporting
	match self.get_safe_transmute_error_and_reason(
	obligation.clone(),
	trait_ref,
	span,
	) {
	SafeTransmuteError::ShouldReport { err_msg, explanation } => {
	(err_msg, Some(explanation))
	}
	// An error is guaranteed to already have been reported at this point, no need to continue
	SafeTransmuteError::ErrorGuaranteed(_) => return,
	}

Uh oh!

Conversation

bryangarza commented Apr 22, 2023

Uh oh!

rustbot commented Apr 22, 2023

Uh oh!

rustbot commented Apr 22, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bryangarza commented Apr 25, 2023

Uh oh!

bors commented Apr 25, 2023

Uh oh!

JohnCSimon commented Jun 17, 2023

Uh oh!

bryangarza commented Jul 19, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants