Skip to content
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

Safe Transmute: Pass ErrorGuaranteed up to error reporting #110669

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
9 changes: 9 additions & 0 deletions compiler/rustc_middle/src/ty/context/tls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ use crate::dep_graph::TaskDepsRef;
use crate::ty::query;
use rustc_data_structures::sync::{self, Lock};
use rustc_errors::Diagnostic;
use rustc_errors::ErrorGuaranteed;
#[cfg(not(parallel_compiler))]
use std::cell::Cell;
use std::mem;
Expand Down Expand Up @@ -153,3 +154,11 @@ where
{
with_context_opt(|opt_context| f(opt_context.map(|context| context.tcx)))
}

pub fn expect_compilation_to_fail() -> ErrorGuaranteed {
if let Some(guar) = with(|tcx| tcx.sess.is_compilation_going_to_fail()) {
guar
} else {
bug!("expect tcx.sess.is_compilation_going_to_fail() to return `Some`");
}
}
10 changes: 1 addition & 9 deletions compiler/rustc_middle/src/ty/visit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -59,15 +59,7 @@ pub trait TypeVisitableExt<'tcx>: TypeVisitable<TyCtxt<'tcx>> {
self.has_type_flags(TypeFlags::HAS_ERROR)
}
fn error_reported(&self) -> Result<(), ErrorGuaranteed> {
if self.references_error() {
if let Some(reported) = ty::tls::with(|tcx| tcx.sess.is_compilation_going_to_fail()) {
Err(reported)
} else {
bug!("expect tcx.sess.is_compilation_going_to_fail return `Some`");
}
} else {
Ok(())
}
if self.references_error() { Err(ty::tls::expect_compilation_to_fail()) } else { Ok(()) }
}
fn has_non_region_param(&self) -> bool {
self.has_type_flags(TypeFlags::NEEDS_SUBST - TypeFlags::HAS_RE_PARAM)
Expand Down
4 changes: 3 additions & 1 deletion compiler/rustc_trait_selection/src/solve/eval_ctxt.rs
Original file line number Diff line number Diff line change
Expand Up @@ -672,7 +672,9 @@ impl<'tcx> EvalCtxt<'_, 'tcx> {
rustc_transmute::Answer::No(_)
| rustc_transmute::Answer::IfTransmutable { .. }
| rustc_transmute::Answer::IfAll(_)
| rustc_transmute::Answer::IfAny(_) => Err(NoSolution),
| rustc_transmute::Answer::IfAny(_)
// FIXME(bryangarza): Pass the `ErrorGuaranteed` along instead of losing it?
| rustc_transmute::Answer::Err(_) => Err(NoSolution),
}
}
}
26 changes: 19 additions & 7 deletions compiler/rustc_trait_selection/src/traits/error_reporting/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -741,11 +741,17 @@ impl<'tcx> TypeErrCtxtExt<'tcx> for TypeErrCtxt<'_, 'tcx> {
== 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(
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,
}
} else {
(err_msg, None)
};
Expand Down Expand Up @@ -1631,7 +1637,7 @@ trait InferCtxtPrivExt<'tcx> {
obligation: Obligation<'tcx, ty::Predicate<'tcx>>,
trait_ref: ty::Binder<'tcx, ty::TraitRef<'tcx>>,
span: Span,
) -> (String, Option<String>);
) -> SafeTransmuteError;
}

impl<'tcx> InferCtxtPrivExt<'tcx> for TypeErrCtxt<'_, 'tcx> {
Expand Down Expand Up @@ -2922,7 +2928,7 @@ impl<'tcx> InferCtxtPrivExt<'tcx> for TypeErrCtxt<'_, 'tcx> {
obligation: Obligation<'tcx, ty::Predicate<'tcx>>,
trait_ref: ty::Binder<'tcx, ty::TraitRef<'tcx>>,
span: Span,
) -> (String, Option<String>) {
) -> SafeTransmuteError {
// Erase regions because layout code doesn't particularly care about regions.
let trait_ref = self.tcx.erase_regions(self.tcx.erase_late_bound_regions(trait_ref));

Expand All @@ -2944,10 +2950,10 @@ impl<'tcx> InferCtxtPrivExt<'tcx> for TypeErrCtxt<'_, 'tcx> {
rustc_transmute::Answer::No(reason) => {
let dst = trait_ref.substs.type_at(0);
let src = trait_ref.substs.type_at(1);
let custom_err_msg = format!(
let err_msg = format!(
"`{src}` cannot be safely transmuted into `{dst}` in the defining scope of `{scope}`"
);
let reason_msg = match reason {
let explanation = match reason {
rustc_transmute::Reason::SrcIsUnspecified => {
format!("`{src}` does not have a well-specified layout")
}
Expand All @@ -2968,13 +2974,14 @@ impl<'tcx> InferCtxtPrivExt<'tcx> for TypeErrCtxt<'_, 'tcx> {
format!("The size of `{src}` is smaller than the size of `{dst}`")
}
};
(custom_err_msg, Some(reason_msg))
SafeTransmuteError::ShouldReport { err_msg, explanation }
}
// 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",
),
rustc_transmute::Answer::Err(guar) => SafeTransmuteError::ErrorGuaranteed(guar),
_ => span_bug!(span, "Unsupported rustc_transmute::Reason variant"),
}
}
Expand Down Expand Up @@ -3103,3 +3110,8 @@ pub enum DefIdOrName {
DefId(DefId),
Name(&'static str),
}

enum SafeTransmuteError {
ShouldReport { err_msg: String, explanation: String },
ErrorGuaranteed(ErrorGuaranteed),
}
6 changes: 3 additions & 3 deletions compiler/rustc_transmute/src/layout/tree.rs
Original file line number Diff line number Diff line change
Expand Up @@ -187,15 +187,15 @@ pub(crate) mod rustc {
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,
Unknown(ErrorGuaranteed),
TypeError(ErrorGuaranteed),
}

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

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

if obligation.predicate.has_non_region_param() {
return;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gotcha, I can undo the changes in this section then

match err {
LayoutError::Unknown(..) => Self::Unknown,
LayoutError::Unknown(..) => Self::Unknown(guar),
err => unimplemented!("{:?}", err),
}
}
Expand Down
4 changes: 4 additions & 0 deletions compiler/rustc_transmute/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,9 @@ where

/// `Src` is transmutable into `Dst` if any of the enclosed requirements are met.
IfAny(Vec<Answer<R>>),

/// Encountered an error during safe transmute computation
Err(ErrorGuaranteed),
}

/// Answers: Why wasn't the source type transmutable into the destination type?
Expand Down Expand Up @@ -162,3 +165,4 @@ mod rustc {

#[cfg(feature = "rustc")]
pub use rustc::*;
use rustc_span::ErrorGuaranteed;
10 changes: 4 additions & 6 deletions compiler/rustc_transmute/src/maybe_transmutable/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -75,12 +75,10 @@ mod rustc {
let dst = Tree::from_ty(dst, context);

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),
Copy link
Member

Choose a reason for hiding this comment

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

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.

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, I added deduplication here

// 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).

Copy link
Member

Choose a reason for hiding this comment

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

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

(Err(Err::TypeError(guar)), _) => Err(Answer::Err(guar)),
(_, Err(Err::TypeError(guar))) => Err(Answer::Err(guar)),
(Err(Err::Unknown(guar)), _) => Err(Answer::Err(guar)),
(_, Err(Err::Unknown(guar))) => Err(Answer::Err(guar)),
(Err(Err::Unspecified), _) => Err(Answer::No(Reason::SrcIsUnspecified)),
(_, Err(Err::Unspecified)) => Err(Answer::No(Reason::DstIsUnspecified)),
(Ok(src), Ok(dst)) => Ok((src, dst)),
Expand Down