Safe Transmute: Fix ICE (Inconsistent is_transmutable result) #113867

bryangarza · 2023-07-19T20:52:06Z

This patch updates the code that constructs the Assume type to return an error instead of silently falling back to a default Assume. This fixes an ICE where error reporting would get a different is_transmutable result that is inconsistent with the original one computed during trait confirmation.

Fixes #110969

rustbot · 2023-07-19T20:52:11Z

r? @wesleywiser

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

rustbot · 2023-07-19T20:52:14Z

Some changes occurred to the core trait solver

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

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

compiler/rustc_transmute/src/lib.rs

compiler/rustc_trait_selection/src/solve/trait_goals.rs

lcnr · 2023-07-20T09:01:24Z

r? @lcnr

This patch updates the code that constructs the `Assume` type to return an error instead of silently falling back to a default `Assume`. This fixes an ICE where error reporting would get a different `is_transmutable` result that is inconsistent with the original one computed during trait confirmation. Fixes rust-lang#110969

bryangarza · 2023-07-21T00:08:32Z

@rustbot ready

lcnr · 2023-07-21T08:08:52Z

compiler/rustc_trait_selection/src/traits/select/confirmation.rs

@@ -340,7 +340,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
        let predicate =
            self.tcx().erase_regions(self.tcx().erase_late_bound_regions(obligation.predicate));

-        let Some(assume) = rustc_transmute::Assume::from_const(
+        let Some(Ok(assume)) = rustc_transmute::Assume::from_const(


for None you have to return ambiguity, not Err(Unimplemented) 🤔

Is there an existing way to represent ambiguity in the current solver? Just return an empty list of candidates? Was going through the code in the select directory, but it's not clear to me

lcnr · 2023-07-21T08:08:59Z

compiler/rustc_trait_selection/src/solve/trait_goals.rs

+        let assume = match maybe_assume {
+            Some(Ok(assume)) => assume,
+            Some(Err(_guar)) => return Err(NoSolution),
+            None => return Err(NoSolution),


Since NoSolution was just a struct, I created a new enum QueryError so I could return QueryError::NoSolution or QueryError::Ambiguous. But the change might be too invasive. Let me know what you think! I made the change in a separate commit.

please keep using NoSolution. Ambiguity in the new solver is represented as Ok with Certainty::Maybe(MaybeCause::Ambiguity)/Certainty::AMBIGUOUS.

use ecx.evaluate_added_goals_and_make_query_response(Certainty::AMBIGUOUS) instead

Thanks, that makes a lot more sense! evaluate_added_goals_and_make_query_response doesn't exist, did you mean evaluate_added_goals_and_make_canonical_response?

rustbot · 2023-07-21T21:58:03Z

Some changes occurred in engine.rs, potentially modifying the public API of ObligationCtxt.

cc @lcnr, @compiler-errors

lcnr · 2023-07-25T07:05:42Z

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

+                                // Unable to compute whether Safe Transmute is possible (for example, due to an unevaluated const).
+                                // The same thing occurred during trait selection/confirmation, so there is no error to report here.


this feels somewhat dangerous to me 🤷 i guess it's fine because fulfillment error reporting uses delay_span_bug so if it's wrong we just ICE

yeah it's not ideal :/ I could emit a warning or something maybe 🤔

ah, maybe add tcx.sess.delay_span_bug("expected another error here") or sth to get a better ICE if it does go wrong. THis should simplify debugging if it blows up

lcnr · 2023-07-25T07:07:27Z

compiler/rustc_transmute/src/lib.rs

    /// Encountered a type error
-    TypeError,
+    TypeError(ErrorGuaranteed),


nit: rename the variant to ErrorGuaranteed 🤔

lcnr · 2023-07-25T07:08:16Z

compiler/rustc_transmute/src/lib.rs

@@ -123,20 +124,14 @@ mod rustc {
            tcx: TyCtxt<'tcx>,
            param_env: ParamEnv<'tcx>,
            c: Const<'tcx>,
-        ) -> Option<Self> {
+        ) -> Option<Result<Self, ErrorGuaranteed>> {


Suggested change

) -> Option<Result<Self, ErrorGuaranteed>> {

) -> Result<Option<Self>, ErrorGuaranteed> {

general convention: errors are "more important/immediate/whatever" than ambiguity

thanks, I didn't know this convention but it makes a lot of sense!

lcnr · 2023-07-25T07:09:18Z

compiler/rustc_transmute/src/lib.rs

 pub use rustc::*;
+use rustc_span::ErrorGuaranteed;


this import should live in the rustc module, shouldn't it? I guess why is the Reason enum outside of rustc 🤔 i would assume that we mostly need this when doing "rustc stuff"?

Yeah, we are trying to make it so that rustc_transmute can build independently of rustc, so pulling in ErrorGuaranteed is not ideal. I'm going to add a FIXME for this. It's getting more and more coupled as I make fixes/improvements 😮

This patch updates a couple spots in the trait selection (in both solvers) to return ambiguity instead of an error.

lcnr · 2023-07-27T08:59:00Z

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

+                                // Unable to compute whether Safe Transmute is possible (for example, due to an unevaluated const).
+                                // The same thing occurred during trait selection/confirmation, so there is no error to report here.


ah, maybe add tcx.sess.delay_span_bug("expected another error here") or sth to get a better ICE if it does go wrong. THis should simplify debugging if it blows up

lcnr · 2023-07-27T09:00:19Z

compiler/rustc_trait_selection/src/traits/select/confirmation.rs

-            return Err(Unimplemented);
+        ) {
+            Ok(Some(assume)) => assume,
+            Ok(None) => return Ok(vec![]),


that's wrong. We would consider transmute to always be possible if the assume is ambig '^^ it's annoying because confirmation cannot handle ambiguity. We have to instead check that we can build the Assume in assembly and then we can ICE here

bors · 2023-09-13T16:46:35Z

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

Dylan-DPC · 2023-11-04T15:12:55Z

@bryangarza any updates on this?

JohnCSimon · 2024-02-11T20:54:30Z

@bryangarza

Ping from triage: I'm closing this due to inactivity, Please reopen when you are ready to continue with this.
Note: if you are going to continue please open the PR BEFORE you push to it, else you won't be able to reopen - this is a quirk of github.
Thanks for your contribution.

@rustbot label: +S-inactive

rustbot assigned wesleywiser Jul 19, 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 Jul 19, 2023

bryangarza force-pushed the inconsistent-is-transmutable_issue-110969 branch 2 times, most recently from 1416a53 to 8b56a0c Compare July 19, 2023 20:58

compiler-errors assigned compiler-errors and unassigned wesleywiser Jul 19, 2023

lcnr reviewed Jul 20, 2023

View reviewed changes

compiler/rustc_trait_selection/src/traits/error_reporting/mod.rs Outdated Show resolved Hide resolved

compiler/rustc_transmute/src/lib.rs Show resolved Hide resolved

compiler/rustc_trait_selection/src/solve/trait_goals.rs Outdated Show resolved Hide resolved

rustbot assigned lcnr and unassigned compiler-errors Jul 20, 2023

bryangarza force-pushed the inconsistent-is-transmutable_issue-110969 branch from 8b56a0c to f45177e Compare July 20, 2023 21:01

bryangarza force-pushed the inconsistent-is-transmutable_issue-110969 branch from f45177e to 98ddc0f Compare July 20, 2023 21:24

lcnr reviewed Jul 21, 2023

View reviewed changes

bryangarza force-pushed the inconsistent-is-transmutable_issue-110969 branch from d19279c to 77c542c Compare July 24, 2023 23:57

lcnr reviewed Jul 25, 2023

View reviewed changes

bryangarza force-pushed the inconsistent-is-transmutable_issue-110969 branch from 77c542c to 9702a5d Compare July 26, 2023 20:33

This comment has been minimized.

Sign in to view

Safe Transmute: Return ambiguity instead of an error, when necessary

641a43d

This patch updates a couple spots in the trait selection (in both solvers) to return ambiguity instead of an error.

bryangarza force-pushed the inconsistent-is-transmutable_issue-110969 branch from 9702a5d to 641a43d Compare July 26, 2023 20:59

lcnr reviewed Jul 27, 2023

View reviewed changes

lcnr 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 Aug 11, 2023

JohnCSimon closed this Feb 11, 2024

rustbot added the S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. label Feb 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Safe Transmute: Fix ICE (Inconsistent is_transmutable result) #113867

Safe Transmute: Fix ICE (Inconsistent is_transmutable result) #113867

bryangarza commented Jul 19, 2023 •

edited

Loading

rustbot commented Jul 19, 2023

rustbot commented Jul 19, 2023

lcnr commented Jul 20, 2023

bryangarza commented Jul 21, 2023

lcnr Jul 21, 2023

bryangarza Jul 24, 2023

lcnr Jul 21, 2023

bryangarza Jul 21, 2023

lcnr Jul 24, 2023

bryangarza Jul 24, 2023

rustbot commented Jul 21, 2023

lcnr Jul 25, 2023

bryangarza Jul 26, 2023

lcnr Jul 27, 2023

lcnr Jul 25, 2023

lcnr Jul 25, 2023

bryangarza Jul 26, 2023

lcnr Jul 25, 2023

bryangarza Jul 26, 2023

This comment has been minimized.

lcnr Jul 27, 2023

lcnr Jul 27, 2023

bors commented Sep 13, 2023

Dylan-DPC commented Nov 4, 2023

JohnCSimon commented Feb 11, 2024

		// Unable to compute whether Safe Transmute is possible (for example, due to an unevaluated const).
		// The same thing occurred during trait selection/confirmation, so there is no error to report here.

	) -> Option<Result<Self, ErrorGuaranteed>> {
	) -> Result<Option<Self>, ErrorGuaranteed> {

Safe Transmute: Fix ICE (Inconsistent is_transmutable result) #113867

Safe Transmute: Fix ICE (Inconsistent is_transmutable result) #113867

Conversation

bryangarza commented Jul 19, 2023 • edited Loading

rustbot commented Jul 19, 2023

rustbot commented Jul 19, 2023

lcnr commented Jul 20, 2023

bryangarza commented Jul 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rustbot commented Jul 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Sep 13, 2023

Dylan-DPC commented Nov 4, 2023

JohnCSimon commented Feb 11, 2024

bryangarza commented Jul 19, 2023 •

edited

Loading