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

Stop using Reveal::All before borrowck #101478

Closed
wants to merge 13 commits into from
4 changes: 2 additions & 2 deletions compiler/rustc_const_eval/src/interpret/util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ where
T: TypeVisitable<'tcx>,
{
debug!("ensure_monomorphic_enough: ty={:?}", ty);
if !ty.needs_subst() {
if !(ty.needs_subst() || ty.has_opaque_types()) {
return Ok(());
}

Expand All @@ -31,7 +31,7 @@ where
}

match *ty.kind() {
ty::Param(_) => ControlFlow::Break(FoundParam),
ty::Param(_) | ty::Opaque(..) => ControlFlow::Break(FoundParam),
ty::Closure(def_id, substs)
| ty::Generator(def_id, substs, ..)
| ty::FnDef(def_id, substs) => {
Expand Down
67 changes: 35 additions & 32 deletions compiler/rustc_const_eval/src/transform/validate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

use rustc_data_structures::fx::FxHashSet;
use rustc_index::bit_set::BitSet;
use rustc_infer::infer::DefiningAnchor;
use rustc_infer::infer::TyCtxtInferExt;
use rustc_middle::mir::interpret::Scalar;
use rustc_middle::mir::visit::NonUseContext::VarDebugInfo;
Expand All @@ -12,13 +13,14 @@ use rustc_middle::mir::{
ProjectionElem, RuntimePhase, Rvalue, SourceScope, Statement, StatementKind, Terminator,
TerminatorKind, UnOp, START_BLOCK,
};
use rustc_middle::ty::fold::BottomUpFolder;
use rustc_middle::traits::ObligationCause;
use rustc_middle::ty::subst::Subst;
use rustc_middle::ty::{self, InstanceDef, ParamEnv, Ty, TyCtxt, TypeFoldable, TypeVisitable};
use rustc_middle::ty::{self, InstanceDef, ParamEnv, Ty, TyCtxt, TypeVisitable};
use rustc_mir_dataflow::impls::MaybeStorageLive;
use rustc_mir_dataflow::storage::always_storage_live_locals;
use rustc_mir_dataflow::{Analysis, ResultsCursor};
use rustc_target::abi::{Size, VariantIdx};
use rustc_trait_selection::traits::ObligationCtxt;

#[derive(Copy, Clone, Debug)]
enum EdgeKind {
Expand Down Expand Up @@ -88,25 +90,36 @@ pub fn equal_up_to_regions<'tcx>(
return true;
}

// Normalize lifetimes away on both sides, then compare.
let normalize = |ty: Ty<'tcx>| {
tcx.try_normalize_erasing_regions(param_env, ty).unwrap_or(ty).fold_with(
&mut BottomUpFolder {
tcx,
// FIXME: We erase all late-bound lifetimes, but this is not fully correct.
// If you have a type like `<for<'a> fn(&'a u32) as SomeTrait>::Assoc`,
// this is not necessarily equivalent to `<fn(&'static u32) as SomeTrait>::Assoc`,
// since one may have an `impl SomeTrait for fn(&32)` and
// `impl SomeTrait for fn(&'static u32)` at the same time which
// specify distinct values for Assoc. (See also #56105)
lt_op: |_| tcx.lifetimes.re_erased,
// Leave consts and types unchanged.
ct_op: |ct| ct,
ty_op: |ty| ty,
},
)
};
tcx.infer_ctxt().enter(|infcx| infcx.can_eq(param_env, normalize(src), normalize(dest)).is_ok())
may_subtype_ignoring_regions(tcx, param_env, src, dest)
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need to check both directions? I thought the opaque type equality code did this already: https://doc.rust-lang.org/nightly/nightly-rustc/src/rustc_infer/infer/opaque_types.rs.html#174-177

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because we're actually checking whether either a <: b or a :> b holds. While this is mostly achieved by ignoring regions, for higher ranked stuff that isn't quite the case, e.g.for<'a> fn(&'a ()) is a subtype of fn(&'erased ()) but not the other way around

|| may_subtype_ignoring_regions(tcx, param_env, dest, src)
}

fn may_subtype_ignoring_regions<'tcx>(
tcx: TyCtxt<'tcx>,
param_env: ParamEnv<'tcx>,
src: Ty<'tcx>,
dest: Ty<'tcx>,
) -> bool {
let mut builder =
tcx.infer_ctxt().ignoring_regions().with_opaque_type_inference(DefiningAnchor::Bubble);
builder.enter(|infcx| {
let ocx = ObligationCtxt::new(&infcx);
let cause = ObligationCause::dummy();
let src = ocx.normalize(cause.clone(), param_env, src);
let dest = ocx.normalize(cause.clone(), param_env, dest);
let Ok(infer_ok) = infcx.at(&cause, param_env).eq(src, dest) else {
return false;
};
let () = ocx.register_infer_ok_obligations(infer_ok);
let errors = ocx.select_all_or_error();
// With `Reveal::All`, opaque types get normalized away, with `Reveal::UserFacing`
// we would get unification errors because we're unable to look into opaque types,
// even if they're constrained in our current function.
//
// It seems very unlikely that this hides any bugs.
let _ = infcx.inner.borrow_mut().opaque_type_storage.take_opaque_types();
errors.is_empty()
})
}

struct TypeChecker<'a, 'tcx> {
Expand Down Expand Up @@ -189,16 +202,6 @@ impl<'a, 'tcx> TypeChecker<'a, 'tcx> {
// all normal lifetimes are erased, higher-ranked types with their
// late-bound lifetimes are still around and can lead to type
// differences. So we compare ignoring lifetimes.

// First, try with reveal_all. This might not work in some cases, as the predicates
// can be cleared in reveal_all mode. We try the reveal first anyways as it is used
// by some other passes like inlining as well.
let param_env = self.param_env.with_reveal_all_normalized(self.tcx);
if equal_up_to_regions(self.tcx, param_env, src, dest) {
return true;
}

// If this fails, we can try it without the reveal.
equal_up_to_regions(self.tcx, self.param_env, src, dest)
}
}
Expand Down Expand Up @@ -285,7 +288,7 @@ impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> {
this.fail(
location,
format!(
"Field projection `{:?}.{:?}` specified type `{:?}`, but actual type is {:?}",
"Field projection `{:?}.{:?}` specified type `{:?}`, but actual type is `{:?}`",
parent, f, ty, f_ty
)
)
Expand Down
9 changes: 9 additions & 0 deletions compiler/rustc_infer/src/infer/error_reporting/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3199,4 +3199,13 @@ impl<'tcx> InferCtxt<'_, 'tcx> {
_ => false,
}
}

pub fn concrete_ctfe_failure_error(&self, span: Span) -> ErrorGuaranteed {
self.tcx
.sess
.struct_span_err(span, "unable to use constant with a hidden value in the type system")
.note("this most often happens when trying to look into an opaque type")
.note("the type system cannot access the hidden type of opaque types")
.emit()
}
}
42 changes: 42 additions & 0 deletions compiler/rustc_lint_defs/src/builtin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3365,6 +3365,7 @@ declare_lint_pass! {
DEPRECATED_CFG_ATTR_CRATE_TYPE_NAME,
DUPLICATE_MACRO_ATTRIBUTES,
SUSPICIOUS_AUTO_TRAIT_IMPLS,
HIDDEN_TYPE_OF_OPAQUE_TYPES_IN_TYPE_SYSTEM,
DEPRECATED_WHERE_CLAUSE_LOCATION,
TEST_UNSTABLE_LINT,
FFI_UNWIND_CALLS,
Expand Down Expand Up @@ -3981,6 +3982,47 @@ declare_lint! {
};
}

declare_lint! {
/// The `hidden_type_of_opaque_types_in_type_system` backward compatibility
/// lint detects when the hidden type of an opaque type is required by the type
/// system.
///
///
/// ### Example
///
/// ```rust
/// use std::mem::transmute;
/// fn foo() -> impl Sized {
/// 0u8
/// }
///
/// fn main() {
/// unsafe {
/// transmute::<_, u8>(foo());
/// }
/// }
/// ```
///
/// {{produces}}
///
/// ### Explanation
///
/// When using an opaque type, i.e. `impl Trait`, the
/// concrete underlying type should not be revealed to
/// the type system. The compiler previously revealed this
/// underlying type in some instances causing bugs.
/// It is still unclear whether this lint will be converted to
/// a hard error in the future or whether a different implementation
/// can support these use-cases.
pub HIDDEN_TYPE_OF_OPAQUE_TYPES_IN_TYPE_SYSTEM,
Warn,
"the rules governing auto traits will change in the future",
@future_incompatible = FutureIncompatibleInfo {
reason: FutureIncompatibilityReason::FutureReleaseError,
reference: "issue # <https://github.com/rust-lang/rust/issues/101478>",
};
}

declare_lint! {
/// The `deprecated_where_clause_location` lint detects when a where clause in front of the equals
/// in an associated type.
Expand Down
59 changes: 54 additions & 5 deletions compiler/rustc_middle/src/mir/interpret/queries.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
use super::{ErrorHandled, EvalToConstValueResult, EvalToValTreeResult, GlobalId};

use crate::mir;
use crate::traits::Reveal;
use crate::ty::subst::InternalSubsts;
use crate::ty::visit::TypeVisitable;
use crate::ty::{self, query::TyCtxtAt, query::TyCtxtEnsure, TyCtxt};
use rustc_hir::def_id::DefId;
use rustc_hir::def_id::{DefId, CRATE_DEF_ID};
use rustc_span::{Span, DUMMY_SP};

impl<'tcx> TyCtxt<'tcx> {
Expand All @@ -20,7 +21,9 @@ impl<'tcx> TyCtxt<'tcx> {
let substs = InternalSubsts::identity_for_item(self, def_id);
let instance = ty::Instance::new(def_id, substs);
let cid = GlobalId { instance, promoted: None };
let param_env = self.param_env(def_id).with_reveal_all_normalized(self);
// This function isn't used in the type system, so we're free to use
// `param_env_reveal_all_normalized` here.
let param_env = self.param_env_reveal_all_normalized(def_id);
self.const_eval_global_id(param_env, cid, None)
}
/// Resolves and evaluates a constant.
Expand Down Expand Up @@ -59,6 +62,42 @@ impl<'tcx> TyCtxt<'tcx> {
}
}

/// HACK: We retry evaluation with `Reveal::All` for backwards
/// compatability reasons, see #101478 for more details.
#[cold]
fn const_eval_resolve_for_typeck_backcompat_hack(
self,
param_env: ty::ParamEnv<'tcx>,
ct: ty::Unevaluated<'tcx>,
span: Option<Span>,
) -> EvalToValTreeResult<'tcx> {
let param_env_reveal_all = ty::ParamEnv::new(
self.normalize_opaque_types(param_env.caller_bounds()),
Reveal::All,
param_env.constness(),
);

match self.const_eval_resolve_for_typeck(param_env_reveal_all, ct, span) {
Ok(Some(v)) => {
let local_def_id = ct.def.did.as_local().unwrap_or(CRATE_DEF_ID);
self.struct_span_lint_hir(
rustc_session::lint::builtin::HIDDEN_TYPE_OF_OPAQUE_TYPES_IN_TYPE_SYSTEM,
self.hir().local_def_id_to_hir_id(local_def_id),
self.def_span(ct.def.did),
|err| {
err.build(
"relying on the underlying type of an opaque type in the type system",
)
.note("evaluating this constant relies on the underlying type of an opaque type")
.emit()
},
);
Ok(Some(v))
}
res => res,
}
}

#[instrument(level = "debug", skip(self))]
pub fn const_eval_resolve_for_typeck(
self,
Expand All @@ -76,13 +115,22 @@ impl<'tcx> TyCtxt<'tcx> {
bug!("did not expect inference variables here");
}

match ty::Instance::resolve_opt_const_arg(self, param_env, ct.def, ct.substs) {
let result = match ty::Instance::resolve_opt_const_arg(self, param_env, ct.def, ct.substs) {
Ok(Some(instance)) => {
let cid = GlobalId { instance, promoted: ct.promoted };
self.const_eval_global_id_for_typeck(param_env, cid, span)
}
Ok(None) => Err(ErrorHandled::TooGeneric),
Err(error_reported) => Err(ErrorHandled::Reported(error_reported)),
};

match result {
Err(ErrorHandled::TooGeneric)
if param_env.reveal() != Reveal::All && !ct.needs_subst() =>
{
self.const_eval_resolve_for_typeck_backcompat_hack(param_env, ct, span)
}
result => result,
}
}

Expand Down Expand Up @@ -184,8 +232,9 @@ impl<'tcx> TyCtxtEnsure<'tcx> {
let substs = InternalSubsts::identity_for_item(self.tcx, def_id);
let instance = ty::Instance::new(def_id, substs);
let cid = GlobalId { instance, promoted: None };
let param_env =
self.tcx.param_env(def_id).with_reveal_all_normalized(self.tcx).with_const();
// This function isn't used in the type system, so we're free to use
// `param_env_reveal_all_normalized` here.
let param_env = self.tcx.param_env_reveal_all_normalized(def_id).with_const();
// Const-eval shouldn't depend on lifetimes at all, so we can erase them, which should
// improve caching of queries.
let inputs = self.tcx.erase_regions(param_env.and(cid));
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_middle/src/mir/interpret/value.rs
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ impl<'tcx> ConstValue<'tcx> {
param_env: ParamEnv<'tcx>,
ty: Ty<'tcx>,
) -> Option<u128> {
let size = tcx.layout_of(param_env.with_reveal_all_normalized(tcx).and(ty)).ok()?.size;
let size = tcx.layout_of(param_env.and(ty)).ok()?.size;
self.try_to_bits(size)
}

Expand Down
3 changes: 1 addition & 2 deletions compiler/rustc_middle/src/mir/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2164,8 +2164,7 @@ impl<'tcx> ConstantKind<'tcx> {
Self::Ty(ct) => ct.try_eval_bits(tcx, param_env, ty),
Self::Val(val, t) => {
assert_eq!(*t, ty);
let size =
tcx.layout_of(param_env.with_reveal_all_normalized(tcx).and(ty)).ok()?.size;
let size = tcx.layout_of(param_env.and(ty)).ok()?.size;
val.try_to_bits(size)
}
}
Expand Down
2 changes: 0 additions & 2 deletions compiler/rustc_middle/src/query/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1238,8 +1238,6 @@ rustc_queries! {
}

/// Like `param_env`, but returns the `ParamEnv` in `Reveal::All` mode.
/// Prefer this over `tcx.param_env(def_id).with_reveal_all_normalized(tcx)`,
/// as this method is more efficient.
query param_env_reveal_all_normalized(def_id: DefId) -> ty::ParamEnv<'tcx> {
desc { |tcx| "computing revealed normalized predicates of `{}`", tcx.def_path_str(def_id) }
}
Expand Down
18 changes: 11 additions & 7 deletions compiler/rustc_middle/src/traits/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -71,13 +71,17 @@ pub enum Reveal {
/// Also, `impl Trait` is normalized to the concrete type,
/// which has to be already collected by type-checking.
///
/// NOTE: as `impl Trait`'s concrete type should *never*
/// be observable directly by the user, `Reveal::All`
/// should not be used by checks which may expose
/// type equality or type contents to the user.
/// There are some exceptions, e.g., around auto traits and
/// transmute-checking, which expose some details, but
/// not the whole concrete type of the `impl Trait`.
/// **This should not be used at any point before borrowck**
///
/// The concrete type of an opaque type should *never*
/// be observable directly by the user and doing so can cause
/// cycles if done before borrowck. Therefore, `Reveal::All`
/// should not be used by checks which may expose type equality
/// or type contents to the user.
///
/// There are some places where we do observe some details about
/// the concrete type of opaque types, e.g., around auto traits and
/// transmute-checking, but these shouldn't rely on `Reveal::All`.
All,
}

Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_middle/src/ty/consts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,7 @@ impl<'tcx> Const<'tcx> {
ty: Ty<'tcx>,
) -> Option<u128> {
assert_eq!(self.ty(), ty);
let size = tcx.layout_of(param_env.with_reveal_all_normalized(tcx).and(ty)).ok()?.size;
let size = tcx.layout_of(param_env.and(ty)).ok()?.size;
// if `ty` does not depend on generic parameters, use an empty param_env
self.kind().eval(tcx, param_env).try_to_bits(size)
}
Expand Down
11 changes: 1 addition & 10 deletions compiler/rustc_middle/src/ty/consts/kind.rs
Original file line number Diff line number Diff line change
Expand Up @@ -184,16 +184,7 @@ impl<'tcx> ConstKind<'tcx> {
if let ConstKind::Unevaluated(unevaluated) = self {
use crate::mir::interpret::ErrorHandled;

// HACK(eddyb) this erases lifetimes even though `const_eval_resolve`
// also does later, but we want to do it before checking for
// inference variables.
// Note that we erase regions *before* calling `with_reveal_all_normalized`,
// so that we don't try to invoke this query with
// any region variables.
let param_env_and = tcx
.erase_regions(param_env)
.with_reveal_all_normalized(tcx)
.and(tcx.erase_regions(unevaluated));
let param_env_and = tcx.erase_regions(param_env).and(tcx.erase_regions(unevaluated));

// HACK(eddyb) when the query key would contain inference variables,
// attempt using identity substs and `ParamEnv` instead, that will succeed
Expand Down
1 change: 0 additions & 1 deletion compiler/rustc_middle/src/ty/layout.rs
Original file line number Diff line number Diff line change
Expand Up @@ -232,7 +232,6 @@ fn layout_of<'tcx>(
let (param_env, ty) = query.into_parts();
debug!(?ty);

let param_env = param_env.with_reveal_all_normalized(tcx);
let unnormalized_ty = ty;

// FIXME: We might want to have two different versions of `layout_of`:
Expand Down
Loading