Skip to content

Commit

Permalink
Rollup merge of #109800 - bryangarza:safe-transmute-improved-errors, …
Browse files Browse the repository at this point in the history 
…r=compiler-errors

Improve safe transmute error reporting

This patch updates the error reporting when Safe Transmute is not possible between 2 types by including the reason.

Also, fix some small bugs that occur when computing the `Answer` for transmutability.
  • Loading branch information
@matthiaskrgr
matthiaskrgr authored Apr 14, 2023
2 parents ec08676 + 36febe1 commit c6223e1
Show file tree
Hide file tree
Showing 29 changed files with 495 additions and 593 deletions.
128 changes: 104 additions & 24 deletions compiler/rustc_trait_selection/src/traits/error_reporting/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -673,6 +673,7 @@ impl<'tcx> TypeErrCtxtExt<'tcx> for TypeErrCtxt<'_, 'tcx> {
return;
}
let trait_ref = trait_predicate.to_poly_trait_ref();

let (post_message, pre_message, type_def) = self
.get_parent_trait_ref(obligation.cause.code())
.map(|(t, s)| {
Expand Down Expand Up @@ -712,33 +713,45 @@ impl<'tcx> TypeErrCtxtExt<'tcx> for TypeErrCtxt<'_, 'tcx> {
(message, note, append_const_msg)
};

let mut err = struct_span_err!(
self.tcx.sess,
span,
E0277,
"{}",
message
.and_then(|cannot_do_this| {
match (predicate_is_const, append_const_msg) {
// do nothing if predicate is not const
(false, _) => Some(cannot_do_this),
// suggested using default post message
(true, Some(None)) => {
Some(format!("{cannot_do_this} in const contexts"))
}
// overridden post message
(true, Some(Some(post_message))) => {
Some(format!("{cannot_do_this}{post_message}"))
}
// fallback to generic message
(true, None) => None,
let err_msg = message
.and_then(|cannot_do_this| {
match (predicate_is_const, append_const_msg) {
// do nothing if predicate is not const
(false, _) => Some(cannot_do_this),
// suggested using default post message
(true, Some(None)) => {
Some(format!("{cannot_do_this} in const contexts"))
}
// overridden post message
(true, Some(Some(post_message))) => {
Some(format!("{cannot_do_this}{post_message}"))
}
})
.unwrap_or_else(|| format!(
// fallback to generic message
(true, None) => None,
}
})
.unwrap_or_else(|| {
format!(
"the trait bound `{}` is not satisfied{}",
trait_predicate, post_message,
))
);
)
});

let (err_msg, safe_transmute_explanation) = if Some(trait_ref.def_id())
== self.tcx.lang_items().transmute_trait()
{
// Recompute the safe transmute reason and use that for the error reporting
self.get_safe_transmute_error_and_reason(
trait_predicate,
obligation.clone(),
trait_ref,
span,
)
} else {
(err_msg, None)
};

let mut err = struct_span_err!(self.tcx.sess, span, E0277, "{}", err_msg);

if is_try_conversion && let Some(ret_span) = self.return_type_span(&obligation) {
err.span_label(
Expand Down Expand Up @@ -828,6 +841,8 @@ impl<'tcx> TypeErrCtxtExt<'tcx> for TypeErrCtxt<'_, 'tcx> {
// at the type param with a label to suggest constraining it.
err.help(&explanation);
}
} else if let Some(custom_explanation) = safe_transmute_explanation {
err.span_label(span, custom_explanation);
} else {
err.span_label(span, explanation);
}
Expand Down Expand Up @@ -1611,6 +1626,14 @@ trait InferCtxtPrivExt<'tcx> {
obligated_types: &mut Vec<Ty<'tcx>>,
cause_code: &ObligationCauseCode<'tcx>,
) -> bool;

fn get_safe_transmute_error_and_reason(
&self,
trait_predicate: ty::Binder<'tcx, ty::TraitPredicate<'tcx>>,
obligation: Obligation<'tcx, ty::Predicate<'tcx>>,
trait_ref: ty::Binder<'tcx, ty::TraitRef<'tcx>>,
span: Span,
) -> (String, Option<String>);
}

impl<'tcx> InferCtxtPrivExt<'tcx> for TypeErrCtxt<'_, 'tcx> {
Expand Down Expand Up @@ -2895,6 +2918,63 @@ impl<'tcx> InferCtxtPrivExt<'tcx> for TypeErrCtxt<'_, 'tcx> {
}
false
}

fn get_safe_transmute_error_and_reason(
&self,
trait_predicate: ty::Binder<'tcx, ty::TraitPredicate<'tcx>>,
obligation: Obligation<'tcx, ty::Predicate<'tcx>>,
trait_ref: ty::Binder<'tcx, ty::TraitRef<'tcx>>,
span: Span,
) -> (String, Option<String>) {
let src_and_dst = trait_predicate.map_bound(|p| rustc_transmute::Types {
dst: p.trait_ref.substs.type_at(0),
src: p.trait_ref.substs.type_at(1),
});
let scope = trait_ref.skip_binder().substs.type_at(2);
let Some(assume) =
rustc_transmute::Assume::from_const(self.infcx.tcx, obligation.param_env, trait_ref.skip_binder().substs.const_at(3)) else {
span_bug!(span, "Unable to construct rustc_transmute::Assume where it was previously possible");
};
match rustc_transmute::TransmuteTypeEnv::new(self.infcx).is_transmutable(
obligation.cause,
src_and_dst,
scope,
assume,
) {
rustc_transmute::Answer::No(reason) => {
let dst = trait_ref.skip_binder().substs.type_at(0);
let src = trait_ref.skip_binder().substs.type_at(1);
let custom_err_msg = format!("`{src}` cannot be safely transmuted into `{dst}` in the defining scope of `{scope}`").to_string();
let reason_msg = match reason {
rustc_transmute::Reason::SrcIsUnspecified => {
format!("`{src}` does not have a well-specified layout").to_string()
}
rustc_transmute::Reason::DstIsUnspecified => {
format!("`{dst}` does not have a well-specified layout").to_string()
}
rustc_transmute::Reason::DstIsBitIncompatible => {
format!("At least one value of `{src}` isn't a bit-valid value of `{dst}`")
.to_string()
}
rustc_transmute::Reason::DstIsPrivate => format!(
"`{dst}` is or contains a type or field that is not visible in that scope"
)
.to_string(),
// FIXME(bryangarza): Include the number of bytes of src and dst
rustc_transmute::Reason::DstIsTooBig => {
format!("The size of `{src}` is smaller than the size of `{dst}`")
}
};
(custom_err_msg, Some(reason_msg))
}
// Should never get a Yes at this point! We already ran it before, and did not get a Yes.
rustc_transmute::Answer::Yes => span_bug!(
span,
"Inconsistent rustc_transmute::is_transmutable(...) result, got Yes",
),
_ => span_bug!(span, "Unsupported rustc_transmute::Reason variant"),
}
}
}

/// Crude way of getting back an `Expr` from a `Span`.
Expand Down
28 changes: 16 additions & 12 deletions compiler/rustc_transmute/src/layout/tree.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -167,31 +167,31 @@ where
}
}

#[derive(Debug, Copy, Clone)]
pub(crate) enum Err {
/// The layout of the type is unspecified.
Unspecified,
/// This error will be surfaced elsewhere by rustc, so don't surface it.
Unknown,
}

#[cfg(feature = "rustc")]
pub(crate) mod rustc {
use super::{Err, Tree};
use super::Tree;
use crate::layout::rustc::{Def, Ref};

use rustc_middle::ty;
use rustc_middle::ty::layout::LayoutError;
use rustc_middle::ty::util::Discr;
use rustc_middle::ty::AdtDef;
use rustc_middle::ty::ParamEnv;
use rustc_middle::ty::SubstsRef;
use rustc_middle::ty::Ty;
use rustc_middle::ty::TyCtxt;
use rustc_middle::ty::VariantDef;
use rustc_middle::ty::{self, Ty, TyCtxt, TypeVisitableExt};
use rustc_span::ErrorGuaranteed;
use rustc_target::abi::Align;
use std::alloc;

#[derive(Debug, Copy, Clone)]
pub(crate) enum Err {
/// The layout of the type is unspecified.
Unspecified,
/// This error will be surfaced elsewhere by rustc, so don't surface it.
Unknown,
TypeError(ErrorGuaranteed),
}

impl<'tcx> From<LayoutError<'tcx>> for Err {
fn from(err: LayoutError<'tcx>) -> Self {
match err {
Expand Down Expand Up @@ -261,6 +261,10 @@ 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));
}

let target = tcx.data_layout();

match ty.kind() {
Expand Down
27 changes: 14 additions & 13 deletions compiler/rustc_transmute/src/maybe_transmutable/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ where
#[cfg(feature = "rustc")]
mod rustc {
use super::*;
use crate::layout::tree::Err;
use crate::layout::tree::rustc::Err;

use rustc_middle::ty::Ty;
use rustc_middle::ty::TyCtxt;
Expand All @@ -71,19 +71,20 @@ mod rustc {
// representations. If these conversions fail, conclude that the transmutation is
// unacceptable; the layouts of both the source and destination types must be
// well-defined.
let src = Tree::from_ty(src, context).map_err(|err| match err {
// Answer `Yes` here, because "Unknown Type" will already be reported by
// rustc. No need to spam the user with more errors.
Err::Unknown => Answer::Yes,
Err::Unspecified => Answer::No(Reason::SrcIsUnspecified),
})?;
let src = Tree::from_ty(src, context);
let dst = Tree::from_ty(dst, context);

let dst = Tree::from_ty(dst, context).map_err(|err| match err {
Err::Unknown => Answer::Yes,
Err::Unspecified => Answer::No(Reason::DstIsUnspecified),
})?;

Ok((src, dst))
match (src, dst) {
// Answer `Yes` here, because 'unknown layout' and type errors will already
// be reported by rustc. No need to spam the user with more errors.
(Err(Err::TypeError(_)), _) => Err(Answer::Yes),
(_, Err(Err::TypeError(_))) => Err(Answer::Yes),
(Err(Err::Unknown), _) => Err(Answer::Yes),
(_, Err(Err::Unknown)) => Err(Answer::Yes),
(Err(Err::Unspecified), _) => Err(Answer::No(Reason::SrcIsUnspecified)),
(_, Err(Err::Unspecified)) => Err(Answer::No(Reason::DstIsUnspecified)),
(Ok(src), Ok(dst)) => Ok((src, dst)),
}
});

match query_or_answer {
Expand Down
6 changes: 3 additions & 3 deletions compiler/rustc_transmute/src/maybe_transmutable/tests.rs
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use super::query_context::test::{Def, UltraMinimal};
use crate::maybe_transmutable::MaybeTransmutableQuery;
use crate::{layout, Answer, Reason, Set};
use crate::{layout, Answer, Reason};
use itertools::Itertools;

mod bool {
Expand Down Expand Up @@ -48,9 +48,9 @@ mod bool {

let into_set = |alts: Vec<_>| {
#[cfg(feature = "rustc")]
let mut set = Set::default();
let mut set = crate::Set::default();
#[cfg(not(feature = "rustc"))]
let mut set = Set::new();
let mut set = std::collections::HashSet::new();
set.extend(alts);
set
};
Expand Down
4 changes: 0 additions & 4 deletions library/core/src/mem/transmutability.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,6 @@
/// notwithstanding whatever safety checks you have asked the compiler to [`Assume`] are satisfied.
#[unstable(feature = "transmutability", issue = "99571")]
#[lang = "transmute_trait"]
#[rustc_on_unimplemented(
message = "`{Src}` cannot be safely transmuted into `{Self}` in the defining scope of `{Context}`.",
label = "`{Src}` cannot be safely transmuted into `{Self}` in the defining scope of `{Context}`."
)]
pub unsafe trait BikeshedIntrinsicFrom<Src, Context, const ASSUME: Assume = { Assume::NOTHING }>
where
Src: ?Sized,
Expand Down
30 changes: 12 additions & 18 deletions tests/ui/transmutability/arrays/should_require_well_defined_layout.stderr
View file
Original file line number Diff line number Diff line change
@@ -1,10 +1,9 @@
error[E0277]: `[String; 0]` cannot be safely transmuted into `()` in the defining scope of `assert::Context`.
error[E0277]: `[String; 0]` cannot be safely transmuted into `()` in the defining scope of `assert::Context`
--> $DIR/should_require_well_defined_layout.rs:26:52
|
LL | assert::is_maybe_transmutable::<repr_rust, ()>();
| ^^ `[String; 0]` cannot be safely transmuted into `()` in the defining scope of `assert::Context`.
| ^^ `[String; 0]` does not have a well-specified layout
|
= help: the trait `BikeshedIntrinsicFrom<[String; 0], assert::Context, Assume { alignment: true, lifetimes: true, safety: true, validity: true }>` is not implemented for `()`
note: required by a bound in `is_maybe_transmutable`
--> $DIR/should_require_well_defined_layout.rs:13:14
|
Expand All @@ -20,13 +19,12 @@ LL | | .and(Assume::VALIDITY)
LL | | }>
| |__________^ required by this bound in `is_maybe_transmutable`

error[E0277]: `u128` cannot be safely transmuted into `[String; 0]` in the defining scope of `assert::Context`.
error[E0277]: `u128` cannot be safely transmuted into `[String; 0]` in the defining scope of `assert::Context`
--> $DIR/should_require_well_defined_layout.rs:27:47
|
LL | assert::is_maybe_transmutable::<u128, repr_rust>();
| ^^^^^^^^^ `u128` cannot be safely transmuted into `[String; 0]` in the defining scope of `assert::Context`.
| ^^^^^^^^^ `[String; 0]` does not have a well-specified layout
|
= help: the trait `BikeshedIntrinsicFrom<u128, assert::Context, Assume { alignment: true, lifetimes: true, safety: true, validity: true }>` is not implemented for `[String; 0]`
note: required by a bound in `is_maybe_transmutable`
--> $DIR/should_require_well_defined_layout.rs:13:14
|
Expand All @@ -42,13 +40,12 @@ LL | | .and(Assume::VALIDITY)
LL | | }>
| |__________^ required by this bound in `is_maybe_transmutable`

error[E0277]: `[String; 1]` cannot be safely transmuted into `()` in the defining scope of `assert::Context`.
error[E0277]: `[String; 1]` cannot be safely transmuted into `()` in the defining scope of `assert::Context`
--> $DIR/should_require_well_defined_layout.rs:32:52
|
LL | assert::is_maybe_transmutable::<repr_rust, ()>();
| ^^ `[String; 1]` cannot be safely transmuted into `()` in the defining scope of `assert::Context`.
| ^^ `[String; 1]` does not have a well-specified layout
|
= help: the trait `BikeshedIntrinsicFrom<[String; 1], assert::Context, Assume { alignment: true, lifetimes: true, safety: true, validity: true }>` is not implemented for `()`
note: required by a bound in `is_maybe_transmutable`
--> $DIR/should_require_well_defined_layout.rs:13:14
|
Expand All @@ -64,13 +61,12 @@ LL | | .and(Assume::VALIDITY)
LL | | }>
| |__________^ required by this bound in `is_maybe_transmutable`

error[E0277]: `u128` cannot be safely transmuted into `[String; 1]` in the defining scope of `assert::Context`.
error[E0277]: `u128` cannot be safely transmuted into `[String; 1]` in the defining scope of `assert::Context`
--> $DIR/should_require_well_defined_layout.rs:33:47
|
LL | assert::is_maybe_transmutable::<u128, repr_rust>();
| ^^^^^^^^^ `u128` cannot be safely transmuted into `[String; 1]` in the defining scope of `assert::Context`.
| ^^^^^^^^^ `[String; 1]` does not have a well-specified layout
|
= help: the trait `BikeshedIntrinsicFrom<u128, assert::Context, Assume { alignment: true, lifetimes: true, safety: true, validity: true }>` is not implemented for `[String; 1]`
note: required by a bound in `is_maybe_transmutable`
--> $DIR/should_require_well_defined_layout.rs:13:14
|
Expand All @@ -86,13 +82,12 @@ LL | | .and(Assume::VALIDITY)
LL | | }>
| |__________^ required by this bound in `is_maybe_transmutable`

error[E0277]: `[String; 2]` cannot be safely transmuted into `()` in the defining scope of `assert::Context`.
error[E0277]: `[String; 2]` cannot be safely transmuted into `()` in the defining scope of `assert::Context`
--> $DIR/should_require_well_defined_layout.rs:38:52
|
LL | assert::is_maybe_transmutable::<repr_rust, ()>();
| ^^ `[String; 2]` cannot be safely transmuted into `()` in the defining scope of `assert::Context`.
| ^^ `[String; 2]` does not have a well-specified layout
|
= help: the trait `BikeshedIntrinsicFrom<[String; 2], assert::Context, Assume { alignment: true, lifetimes: true, safety: true, validity: true }>` is not implemented for `()`
note: required by a bound in `is_maybe_transmutable`
--> $DIR/should_require_well_defined_layout.rs:13:14
|
Expand All @@ -108,13 +103,12 @@ LL | | .and(Assume::VALIDITY)
LL | | }>
| |__________^ required by this bound in `is_maybe_transmutable`

error[E0277]: `u128` cannot be safely transmuted into `[String; 2]` in the defining scope of `assert::Context`.
error[E0277]: `u128` cannot be safely transmuted into `[String; 2]` in the defining scope of `assert::Context`
--> $DIR/should_require_well_defined_layout.rs:39:47
|
LL | assert::is_maybe_transmutable::<u128, repr_rust>();
| ^^^^^^^^^ `u128` cannot be safely transmuted into `[String; 2]` in the defining scope of `assert::Context`.
| ^^^^^^^^^ `[String; 2]` does not have a well-specified layout
|
= help: the trait `BikeshedIntrinsicFrom<u128, assert::Context, Assume { alignment: true, lifetimes: true, safety: true, validity: true }>` is not implemented for `[String; 2]`
note: required by a bound in `is_maybe_transmutable`
--> $DIR/should_require_well_defined_layout.rs:13:14
|
Expand Down
Loading

0 comments on commit c6223e1

Please sign in to comment.