Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions compiler/rustc_trait_selection/src/solve/trait_goals.rs
Original file line number Diff line number Diff line change
Expand Up @@ -599,10 +599,14 @@ impl<'tcx> assembly::GoalKind<'tcx> for TraitPredicate<'tcx> {
// which will ICE for region vars.
let args = ecx.tcx().erase_regions(goal.predicate.trait_ref.args);

let Some(assume) =
rustc_transmute::Assume::from_const(ecx.tcx(), goal.param_env, args.const_at(3))
else {
return Err(NoSolution);
let maybe_assume =
rustc_transmute::Assume::from_const(ecx.tcx(), goal.param_env, args.const_at(3));
let assume = match maybe_assume {
Ok(Some(assume)) => assume,
Ok(None) => {
return ecx.evaluate_added_goals_and_make_canonical_response(Certainty::AMBIGUOUS);
}
Err(_guar) => return Err(NoSolution),
};

let certainty = ecx.is_transmutable(
Expand Down
27 changes: 17 additions & 10 deletions compiler/rustc_trait_selection/src/traits/error_reporting/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,9 @@ pub struct ImplCandidate<'tcx> {
}

enum GetSafeTransmuteErrorAndReason {
Silent,
ErrorGuaranteed(ErrorGuaranteed),
// Unable to compute Safe Transmute result because of ambiguity
Ambiguous,
Error { err_msg: String, safe_transmute_explanation: String },
}

Expand Down Expand Up @@ -756,7 +758,10 @@ impl<'tcx> TypeErrCtxtExt<'tcx> for TypeErrCtxt<'_, 'tcx> {
trait_ref,
span,
) {
GetSafeTransmuteErrorAndReason::Silent => return,
GetSafeTransmuteErrorAndReason::ErrorGuaranteed(_guar) => return,
// 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.
Comment on lines +762 to +763
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

GetSafeTransmuteErrorAndReason::Ambiguous => return,
GetSafeTransmuteErrorAndReason::Error {
err_msg,
safe_transmute_explanation,
Expand Down Expand Up @@ -2884,15 +2889,17 @@ impl<'tcx> InferCtxtPrivExt<'tcx> for TypeErrCtxt<'_, 'tcx> {
src: trait_ref.args.type_at(1),
};
let scope = trait_ref.args.type_at(2);
let Some(assume) = rustc_transmute::Assume::from_const(

let maybe_assume = rustc_transmute::Assume::from_const(
self.infcx.tcx,
obligation.param_env,
trait_ref.args.const_at(3),
) else {
span_bug!(
span,
"Unable to construct rustc_transmute::Assume where it was previously possible"
);
);

let assume = match maybe_assume {
Ok(Some(assume)) => assume,
Ok(None) => return GetSafeTransmuteErrorAndReason::Ambiguous,
Err(guar) => return GetSafeTransmuteErrorAndReason::ErrorGuaranteed(guar),
};

match rustc_transmute::TransmuteTypeEnv::new(self.infcx).is_transmutable(
Expand Down Expand Up @@ -2938,8 +2945,8 @@ impl<'tcx> InferCtxtPrivExt<'tcx> for TypeErrCtxt<'_, 'tcx> {
format!("`{src}` is a shared reference, but `{dst}` is a unique reference")
}
// Already reported by rustc
rustc_transmute::Reason::TypeError => {
return GetSafeTransmuteErrorAndReason::Silent;
rustc_transmute::Reason::ErrorGuaranteed(guar) => {
return GetSafeTransmuteErrorAndReason::ErrorGuaranteed(guar);
}
rustc_transmute::Reason::SrcLayoutUnknown => {
format!("`{src}` has an unknown layout")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -340,12 +340,14 @@ 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 assume = match rustc_transmute::Assume::from_const(
self.infcx.tcx,
obligation.param_env,
predicate.trait_ref.args.const_at(3),
) else {
return Err(Unimplemented);
) {
Ok(Some(assume)) => assume,
Ok(None) => return Ok(vec![]),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Err(_guar) => return Err(Unimplemented),
};

let dst = predicate.trait_ref.args.type_at(0);
Expand Down
4 changes: 2 additions & 2 deletions compiler/rustc_transmute/src/layout/tree.rs
Original file line number Diff line number Diff line change
Expand Up @@ -261,8 +261,8 @@ pub(crate) mod rustc {
use rustc_middle::ty::UintTy::*;
use rustc_target::abi::HasDataLayout;

if let Err(e) = ty.error_reported() {
return Err(Err::TypeError(e));
if let Err(guar) = ty.error_reported() {
return Err(Err::TypeError(guar));
}

let target = tcx.data_layout();
Expand Down
35 changes: 15 additions & 20 deletions compiler/rustc_transmute/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ pub enum Reason {
/// Can't go from shared pointer to unique pointer
DstIsMoreUnique,
/// Encountered a type error
TypeError,
ErrorGuaranteed(ErrorGuaranteed),
/// The layout of src is unknown
SrcLayoutUnknown,
/// The layout of dst is unknown
Expand All @@ -79,6 +79,7 @@ mod rustc {
use rustc_middle::ty::Ty;
use rustc_middle::ty::TyCtxt;
use rustc_middle::ty::ValTree;
use rustc_span::ErrorGuaranteed;

/// The source and destination types of a transmutation.
#[derive(TypeVisitable, Debug, Clone, Copy)]
Expand Down Expand Up @@ -123,23 +124,20 @@ mod rustc {
tcx: TyCtxt<'tcx>,
param_env: ParamEnv<'tcx>,
c: Const<'tcx>,
) -> Option<Self> {
) -> Result<Option<Self>, ErrorGuaranteed> {
use rustc_middle::ty::ScalarInt;
use rustc_middle::ty::TypeVisitableExt;
use rustc_span::symbol::sym;

let c = c.eval(tcx, param_env);

if let Err(err) = c.error_reported() {
return Some(Self {
alignment: true,
lifetimes: true,
safety: true,
validity: true,
});
return Err(err);
}

let adt_def = c.ty().ty_adt_def()?;
let adt_def = match c.ty().ty_adt_def() {
Some(adt_def) => adt_def,
None => return Ok(None),
};

assert_eq!(
tcx.require_lang_item(LangItem::TransmuteOpts, None),
Expand All @@ -151,14 +149,7 @@ mod rustc {
let variant = adt_def.non_enum_variant();
let fields = match c.try_to_valtree() {
Some(ValTree::Branch(branch)) => branch,
_ => {
return Some(Self {
alignment: true,
lifetimes: true,
safety: true,
validity: true,
});
}
_ => return Ok(None),
};

let get_field = |name| {
Expand All @@ -171,15 +162,19 @@ mod rustc {
fields[field_idx].unwrap_leaf() == ScalarInt::TRUE
};

Some(Self {
Ok(Some(Self {
alignment: get_field(sym::alignment),
lifetimes: get_field(sym::lifetimes),
safety: get_field(sym::safety),
validity: get_field(sym::validity),
})
}))
}
}
}

// FIXME(bryangarza): This prevents `cargo build` from building.
// Need to remove this dependency on `ErrorGuaranteed` so the crate can
// build/test independently of rustc again.
#[cfg(feature = "rustc")]
pub use rustc::*;
use rustc_span::ErrorGuaranteed;
Comment on lines 179 to +180
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😮

4 changes: 2 additions & 2 deletions compiler/rustc_transmute/src/maybe_transmutable/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -78,8 +78,8 @@ mod rustc {
let dst = Tree::from_ty(dst, context);

match (src, dst) {
(Err(Err::TypeError(_)), _) | (_, Err(Err::TypeError(_))) => {
Answer::No(Reason::TypeError)
(Err(Err::TypeError(guar)), _) | (_, Err(Err::TypeError(guar))) => {
Answer::No(Reason::ErrorGuaranteed(guar))
}
(Err(Err::UnknownLayout), _) => Answer::No(Reason::SrcLayoutUnknown),
(_, Err(Err::UnknownLayout)) => Answer::No(Reason::DstLayoutUnknown),
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
error[E0308]: mismatched types
--> $DIR/inconsistent-is-transmutable.rs:27:74
|
LL | const FALSE: bool = assert::is_transmutable::<Src, Dst, Context, {}>();
| ^^ expected `Assume`, found `()`

error[E0308]: mismatched types
--> $DIR/inconsistent-is-transmutable.rs:27:29
|
LL | const FALSE: bool = assert::is_transmutable::<Src, Dst, Context, {}>();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected `bool`, found `()`

error: aborting due to 2 previous errors

For more information about this error, try `rustc --explain E0308`.
15 changes: 15 additions & 0 deletions tests/ui/transmutability/inconsistent-is-transmutable.next.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
error[E0308]: mismatched types
--> $DIR/inconsistent-is-transmutable.rs:27:74
|
LL | const FALSE: bool = assert::is_transmutable::<Src, Dst, Context, {}>();
| ^^ expected `Assume`, found `()`

error[E0308]: mismatched types
--> $DIR/inconsistent-is-transmutable.rs:27:29
|
LL | const FALSE: bool = assert::is_transmutable::<Src, Dst, Context, {}>();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected `bool`, found `()`

error: aborting due to 2 previous errors

For more information about this error, try `rustc --explain E0308`.
31 changes: 31 additions & 0 deletions tests/ui/transmutability/inconsistent-is-transmutable.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
// check-fail
// revisions: current next
//[next] compile-flags: -Ztrait-solver=next
// https://github.com/rust-lang/rust/issues/110969
#![feature(adt_const_params, generic_const_exprs, transmutability)]
#![allow(incomplete_features)]

mod assert {
use std::mem::BikeshedIntrinsicFrom;

pub fn is_transmutable<Src, Dst, Context, const ASSUME: std::mem::Assume>()
where
Dst: BikeshedIntrinsicFrom<Src, Context, ASSUME>,
{
}
}

fn main() {
struct Context;
#[repr(C)]
struct Src;
#[repr(C)]
struct Dst;

trait Trait {
// The `{}` should not cause an ICE
const FALSE: bool = assert::is_transmutable::<Src, Dst, Context, {}>();
//~^ ERROR mismatched types
//~^^ ERROR mismatched types
}
}
Original file line number Diff line number Diff line change
@@ -1,29 +1,29 @@
error: expected parameter name, found `,`
--> $DIR/issue-110892.rs:27:9
--> $DIR/issue-110892.rs:29:9
|
LL | ,
| ^ expected parameter name

error: expected parameter name, found `,`
--> $DIR/issue-110892.rs:28:9
--> $DIR/issue-110892.rs:30:9
|
LL | ,
| ^ expected parameter name

error: expected parameter name, found `,`
--> $DIR/issue-110892.rs:29:9
--> $DIR/issue-110892.rs:31:9
|
LL | ,
| ^ expected parameter name

error: expected parameter name, found `,`
--> $DIR/issue-110892.rs:30:9
--> $DIR/issue-110892.rs:32:9
|
LL | ,
| ^ expected parameter name

error[E0308]: mismatched types
--> $DIR/issue-110892.rs:31:10
--> $DIR/issue-110892.rs:33:10
|
LL | const fn from_options(
| ------------ implicitly returns `()` as its body has no tail or `return` expression
Expand Down
42 changes: 42 additions & 0 deletions tests/ui/transmutability/issue-110892.next.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
error: expected parameter name, found `,`
--> $DIR/issue-110892.rs:29:9
|
LL | ,
| ^ expected parameter name

error: expected parameter name, found `,`
--> $DIR/issue-110892.rs:30:9
|
LL | ,
| ^ expected parameter name

error: expected parameter name, found `,`
--> $DIR/issue-110892.rs:31:9
|
LL | ,
| ^ expected parameter name

error: expected parameter name, found `,`
--> $DIR/issue-110892.rs:32:9
|
LL | ,
| ^ expected parameter name

error[E0284]: type annotations needed: cannot satisfy `the constant `{ from_options(ASSUME_ALIGNMENT, ASSUME_LIFETIMES, ASSUME_SAFETY, ASSUME_VALIDITY) }` can be evaluated`
--> $DIR/issue-110892.rs:23:13
|
LL | { from_options(ASSUME_ALIGNMENT, ASSUME_LIFETIMES, ASSUME_SAFETY, ASSUME_VALIDITY) }
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ cannot satisfy `the constant `{ from_options(ASSUME_ALIGNMENT, ASSUME_LIFETIMES, ASSUME_SAFETY, ASSUME_VALIDITY) }` can be evaluated`
|
note: required by a bound in `is_transmutable`
--> $DIR/issue-110892.rs:23:13
|
LL | pub fn is_transmutable<
| --------------- required by a bound in this function
...
LL | { from_options(ASSUME_ALIGNMENT, ASSUME_LIFETIMES, ASSUME_SAFETY, ASSUME_VALIDITY) }
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ required by this bound in `is_transmutable`

error: aborting due to 5 previous errors

For more information about this error, try `rustc --explain E0284`.
6 changes: 4 additions & 2 deletions tests/ui/transmutability/issue-110892.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
// check-fail
// revisions: current next
//[next] compile-flags: -Ztrait-solver=next
#![feature(generic_const_exprs, transmutability)]
#![allow(incomplete_features)]

Expand All @@ -18,7 +20,7 @@ mod assert {
Dst: BikeshedIntrinsicFrom<
Src,
Context,
{ from_options(ASSUME_ALIGNMENT, ASSUME_LIFETIMES, ASSUME_SAFETY, ASSUME_VALIDITY) }
{ from_options(ASSUME_ALIGNMENT, ASSUME_LIFETIMES, ASSUME_SAFETY, ASSUME_VALIDITY) } //[next]~ ERROR type annotations needed
>,
{}

Expand All @@ -28,7 +30,7 @@ mod assert {
, //~ ERROR expected parameter name, found `,`
, //~ ERROR expected parameter name, found `,`
, //~ ERROR expected parameter name, found `,`
) -> Assume {} //~ ERROR mismatched types
) -> Assume {} //[current]~ ERROR mismatched types
}

fn main() {
Expand Down