Skip to content

Commit

Permalink
Auto merge of #126090 - compiler-errors:supertrait-assoc-ty-unsoundne…
Browse files Browse the repository at this point in the history 
…ss, r=<try>

Fix supertrait associated type unsoundness

This is built on top of #122804 though that's really not related, it's just easier to make this modification with the changes to the object safety code that I did in that PR. The only thing is that PR may make this unsoundness slightly easier to abuse, since there are more positions that allow self-associated-types.

This PR only looks for supertrait associated types *without* normalizing. I'm gonna crater this initially -- preferably we don't need to normalize unless there's a lot of breakage. I assume that most people are writing `Self::Assoc` so they don't really care about the trait ref being normalized, so somewhat tempted to believe that this will just work out and we can extend it later if needed.

r? lcnr
  • Loading branch information
@bors
bors committed Jun 6, 2024
2 parents e1ac0fa + 3c44574 commit 09fa131
Show file tree
Hide file tree
Showing 4 changed files with 316 additions and 113 deletions.
306 changes: 193 additions & 113 deletions compiler/rustc_trait_selection/src/traits/object_safety.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,17 +12,16 @@ use super::elaborate;

use crate::infer::TyCtxtInferExt;
use crate::traits::query::evaluate_obligation::InferCtxtExt;
use crate::traits::{self, Obligation, ObligationCause};
use crate::traits::{util, Obligation, ObligationCause};
use rustc_errors::FatalError;
use rustc_hir as hir;
use rustc_hir::def_id::DefId;
use rustc_middle::query::Providers;
use rustc_middle::ty::{
self, EarlyBinder, ExistentialPredicateStableCmpExt as _, Ty, TyCtxt, TypeSuperVisitable,
TypeVisitable, TypeVisitor,
self, EarlyBinder, ExistentialPredicateStableCmpExt as _, GenericArgs, Ty, TyCtxt,
TypeFoldable, TypeFolder, TypeSuperFoldable, TypeSuperVisitable, TypeVisitable,
TypeVisitableExt, TypeVisitor, Upcast,
};
use rustc_middle::ty::{GenericArg, GenericArgs};
use rustc_middle::ty::{TypeVisitableExt, Upcast};
use rustc_span::symbol::Symbol;
use rustc_span::Span;
use rustc_target::abi::Abi;
Expand Down Expand Up @@ -195,7 +194,13 @@ fn predicates_reference_self(
.predicates
.iter()
.map(|&(predicate, sp)| (predicate.instantiate_supertrait(tcx, &trait_ref), sp))
.filter_map(|predicate| predicate_references_self(tcx, predicate))
.filter_map(|(clause, sp)| {
// Super predicates cannot allow self projections, since they're
// impossible to make into existential bounds without eager resolution
// or something.
// e.g. `trait A: B<Item = Self::Assoc>`.
predicate_references_self(tcx, trait_def_id, clause, sp, AllowSelfProjections::No)
})
.collect()
}

Expand All @@ -204,20 +209,25 @@ fn bounds_reference_self(tcx: TyCtxt<'_>, trait_def_id: DefId) -> SmallVec<[Span
.in_definition_order()
.filter(|item| item.kind == ty::AssocKind::Type)
.flat_map(|item| tcx.explicit_item_bounds(item.def_id).instantiate_identity_iter_copied())
.filter_map(|c| predicate_references_self(tcx, c))
.filter_map(|(clause, sp)| {
// Item bounds *can* have self projections, since they never get
// their self type erased.
predicate_references_self(tcx, trait_def_id, clause, sp, AllowSelfProjections::Yes)
})
.collect()
}

fn predicate_references_self<'tcx>(
tcx: TyCtxt<'tcx>,
(predicate, sp): (ty::Clause<'tcx>, Span),
trait_def_id: DefId,
predicate: ty::Clause<'tcx>,
sp: Span,
allow_self_projections: AllowSelfProjections,
) -> Option<Span> {
let self_ty = tcx.types.self_param;
let has_self_ty = |arg: &GenericArg<'tcx>| arg.walk().any(|arg| arg == self_ty.into());
match predicate.kind().skip_binder() {
ty::ClauseKind::Trait(ref data) => {
// In the case of a trait predicate, we can skip the "self" type.
data.trait_ref.args[1..].iter().any(has_self_ty).then_some(sp)
data.trait_ref.args[1..].iter().any(|&arg| contains_illegal_self_type_reference(tcx, trait_def_id, arg, allow_self_projections)).then_some(sp)
}
ty::ClauseKind::Projection(ref data) => {
// And similarly for projections. This should be redundant with
Expand All @@ -235,9 +245,9 @@ fn predicate_references_self<'tcx>(
//
// This is ALT2 in issue #56288, see that for discussion of the
// possible alternatives.
data.projection_term.args[1..].iter().any(has_self_ty).then_some(sp)
data.projection_term.args[1..].iter().any(|&arg| contains_illegal_self_type_reference(tcx, trait_def_id, arg, allow_self_projections)).then_some(sp)
}
ty::ClauseKind::ConstArgHasType(_ct, ty) => has_self_ty(&ty.into()).then_some(sp),
ty::ClauseKind::ConstArgHasType(_ct, ty) => contains_illegal_self_type_reference(tcx, trait_def_id, ty, allow_self_projections).then_some(sp),

ty::ClauseKind::WellFormed(..)
| ty::ClauseKind::TypeOutlives(..)
Expand Down Expand Up @@ -383,7 +393,12 @@ fn virtual_call_violations_for_method<'tcx>(
let mut errors = Vec::new();

for (i, &input_ty) in sig.skip_binder().inputs().iter().enumerate().skip(1) {
if contains_illegal_self_type_reference(tcx, trait_def_id, sig.rebind(input_ty)) {
if contains_illegal_self_type_reference(
tcx,
trait_def_id,
sig.rebind(input_ty),
AllowSelfProjections::Yes,
) {
let span = if let Some(hir::Node::TraitItem(hir::TraitItem {
kind: hir::TraitItemKind::Fn(sig, _),
..
Expand All @@ -396,7 +411,12 @@ fn virtual_call_violations_for_method<'tcx>(
errors.push(MethodViolationCode::ReferencesSelfInput(span));
}
}
if contains_illegal_self_type_reference(tcx, trait_def_id, sig.output()) {
if contains_illegal_self_type_reference(
tcx,
trait_def_id,
sig.output(),
AllowSelfProjections::Yes,
) {
errors.push(MethodViolationCode::ReferencesSelfOutput);
}
if let Some(code) = contains_illegal_impl_trait_in_trait(tcx, method.def_id, sig.output()) {
Expand Down Expand Up @@ -482,7 +502,7 @@ fn virtual_call_violations_for_method<'tcx>(
return false;
}

contains_illegal_self_type_reference(tcx, trait_def_id, pred)
contains_illegal_self_type_reference(tcx, trait_def_id, pred, AllowSelfProjections::Yes)
}) {
errors.push(MethodViolationCode::WhereClauseReferencesSelf);
}
Expand Down Expand Up @@ -711,120 +731,180 @@ fn receiver_is_dispatchable<'tcx>(
infcx.predicate_must_hold_modulo_regions(&obligation)
}

#[derive(Copy, Clone)]
enum AllowSelfProjections {
Yes,
No,
}

/// This is somewhat subtle. In general, we want to forbid
/// references to `Self` in the argument and return types,
/// since the value of `Self` is erased. However, there is one
/// exception: it is ok to reference `Self` in order to access
/// an associated type of the current trait, since we retain
/// the value of those associated types in the object type
/// itself.
///
/// ```rust,ignore (example)
/// trait SuperTrait {
/// type X;
/// }
///
/// trait Trait : SuperTrait {
/// type Y;
/// fn foo(&self, x: Self) // bad
/// fn foo(&self) -> Self // bad
/// fn foo(&self) -> Option<Self> // bad
/// fn foo(&self) -> Self::Y // OK, desugars to next example
/// fn foo(&self) -> <Self as Trait>::Y // OK
/// fn foo(&self) -> Self::X // OK, desugars to next example
/// fn foo(&self) -> <Self as SuperTrait>::X // OK
/// }
/// ```
///
/// However, it is not as simple as allowing `Self` in a projected
/// type, because there are illegal ways to use `Self` as well:
///
/// ```rust,ignore (example)
/// trait Trait : SuperTrait {
/// ...
/// fn foo(&self) -> <Self as SomeOtherTrait>::X;
/// }
/// ```
///
/// Here we will not have the type of `X` recorded in the
/// object type, and we cannot resolve `Self as SomeOtherTrait`
/// without knowing what `Self` is.
fn contains_illegal_self_type_reference<'tcx, T: TypeVisitable<TyCtxt<'tcx>>>(
tcx: TyCtxt<'tcx>,
trait_def_id: DefId,
value: T,
allow_self_projections: AllowSelfProjections,
) -> bool {
// This is somewhat subtle. In general, we want to forbid
// references to `Self` in the argument and return types,
// since the value of `Self` is erased. However, there is one
// exception: it is ok to reference `Self` in order to access
// an associated type of the current trait, since we retain
// the value of those associated types in the object type
// itself.
//
// ```rust
// trait SuperTrait {
// type X;
// }
//
// trait Trait : SuperTrait {
// type Y;
// fn foo(&self, x: Self) // bad
// fn foo(&self) -> Self // bad
// fn foo(&self) -> Option<Self> // bad
// fn foo(&self) -> Self::Y // OK, desugars to next example
// fn foo(&self) -> <Self as Trait>::Y // OK
// fn foo(&self) -> Self::X // OK, desugars to next example
// fn foo(&self) -> <Self as SuperTrait>::X // OK
// }
// ```
//
// However, it is not as simple as allowing `Self` in a projected
// type, because there are illegal ways to use `Self` as well:
//
// ```rust
// trait Trait : SuperTrait {
// ...
// fn foo(&self) -> <Self as SomeOtherTrait>::X;
// }
// ```
//
// Here we will not have the type of `X` recorded in the
// object type, and we cannot resolve `Self as SomeOtherTrait`
// without knowing what `Self` is.

struct IllegalSelfTypeVisitor<'tcx> {
tcx: TyCtxt<'tcx>,
trait_def_id: DefId,
supertraits: Option<Vec<DefId>>,
}
value
.visit_with(&mut IllegalSelfTypeVisitor {
tcx,
trait_def_id,
supertraits: None,
allow_self_projections,
})
.is_break()
}

impl<'tcx> TypeVisitor<TyCtxt<'tcx>> for IllegalSelfTypeVisitor<'tcx> {
type Result = ControlFlow<()>;
struct IllegalSelfTypeVisitor<'tcx> {
tcx: TyCtxt<'tcx>,
trait_def_id: DefId,
supertraits: Option<Vec<ty::TraitRef<'tcx>>>,
allow_self_projections: AllowSelfProjections,
}

fn visit_ty(&mut self, t: Ty<'tcx>) -> Self::Result {
match t.kind() {
ty::Param(_) => {
if t == self.tcx.types.self_param {
ControlFlow::Break(())
} else {
ControlFlow::Continue(())
}
}
ty::Alias(ty::Projection, ref data)
if self.tcx.is_impl_trait_in_trait(data.def_id) =>
{
// We'll deny these later in their own pass
impl<'tcx> TypeVisitor<TyCtxt<'tcx>> for IllegalSelfTypeVisitor<'tcx> {
type Result = ControlFlow<()>;

fn visit_ty(&mut self, t: Ty<'tcx>) -> Self::Result {
match t.kind() {
ty::Param(_) => {
if t == self.tcx.types.self_param {
ControlFlow::Break(())
} else {
ControlFlow::Continue(())
}
ty::Alias(ty::Projection, ref data) => {
// This is a projected type `<Foo as SomeTrait>::X`.

// Compute supertraits of current trait lazily.
if self.supertraits.is_none() {
let trait_ref =
ty::Binder::dummy(ty::TraitRef::identity(self.tcx, self.trait_def_id));
self.supertraits = Some(
traits::supertraits(self.tcx, trait_ref).map(|t| t.def_id()).collect(),
);
}
}
ty::Alias(ty::Projection, ref data) if self.tcx.is_impl_trait_in_trait(data.def_id) => {
// We'll deny these later in their own pass
ControlFlow::Continue(())
}
ty::Alias(ty::Projection, ref data) => {
match self.allow_self_projections {
AllowSelfProjections::Yes => {
// This is a projected type `<Foo as SomeTrait>::X`.

// Compute supertraits of current trait lazily.
if self.supertraits.is_none() {
self.supertraits = Some(
util::supertraits(
self.tcx,
ty::Binder::dummy(ty::TraitRef::identity(
self.tcx,
self.trait_def_id,
)),
)
.map(|trait_ref| {
self.tcx.erase_regions(
self.tcx.instantiate_bound_regions_with_erased(trait_ref),
)
})
.collect(),
);
}

// Determine whether the trait reference `Foo as
// SomeTrait` is in fact a supertrait of the
// current trait. In that case, this type is
// legal, because the type `X` will be specified
// in the object type. Note that we can just use
// direct equality here because all of these types
// are part of the formal parameter listing, and
// hence there should be no inference variables.
let is_supertrait_of_current_trait = self
.supertraits
.as_ref()
.unwrap()
.contains(&data.trait_ref(self.tcx).def_id);

if is_supertrait_of_current_trait {
ControlFlow::Continue(()) // do not walk contained types, do not report error, do collect $200
} else {
t.super_visit_with(self) // DO walk contained types, POSSIBLY reporting an error
// Determine whether the trait reference `Foo as
// SomeTrait` is in fact a supertrait of the
// current trait. In that case, this type is
// legal, because the type `X` will be specified
// in the object type. Note that we can just use
// direct equality here because all of these types
// are part of the formal parameter listing, and
// hence there should be no inference variables.
let is_supertrait_of_current_trait =
self.supertraits.as_ref().unwrap().contains(
&data.trait_ref(self.tcx).fold_with(
&mut EraseEscapingBoundRegions {
tcx: self.tcx,
binder: ty::INNERMOST,
},
),
);

if is_supertrait_of_current_trait {
ControlFlow::Continue(())
} else {
t.super_visit_with(self)
}
}
AllowSelfProjections::No => t.super_visit_with(self),
}
_ => t.super_visit_with(self), // walk contained types, if any
}
_ => t.super_visit_with(self),
}
}

fn visit_const(&mut self, ct: ty::Const<'tcx>) -> Self::Result {
// Constants can only influence object safety if they are generic and reference `Self`.
// This is only possible for unevaluated constants, so we walk these here.
self.tcx.expand_abstract_consts(ct).super_visit_with(self)
}
fn visit_const(&mut self, ct: ty::Const<'tcx>) -> Self::Result {
// Constants can only influence object safety if they are generic and reference `Self`.
// This is only possible for unevaluated constants, so we walk these here.
self.tcx.expand_abstract_consts(ct).super_visit_with(self)
}
}

value
.visit_with(&mut IllegalSelfTypeVisitor { tcx, trait_def_id, supertraits: None })
.is_break()
struct EraseEscapingBoundRegions<'tcx> {
tcx: TyCtxt<'tcx>,
binder: ty::DebruijnIndex,
}

impl<'tcx> TypeFolder<TyCtxt<'tcx>> for EraseEscapingBoundRegions<'tcx> {
fn interner(&self) -> TyCtxt<'tcx> {
self.tcx
}

fn fold_binder<T>(&mut self, t: ty::Binder<'tcx, T>) -> ty::Binder<'tcx, T>
where
T: TypeFoldable<TyCtxt<'tcx>>,
{
self.binder.shift_in(1);
let result = t.super_fold_with(self);
self.binder.shift_out(1);
result
}

fn fold_region(&mut self, r: ty::Region<'tcx>) -> ty::Region<'tcx> {
if let ty::ReBound(debruijn, _) = *r
&& debruijn < self.binder
{
r
} else {
self.tcx.lifetimes.re_erased
}
}
}

pub fn contains_illegal_impl_trait_in_trait<'tcx>(
Expand Down
Loading

0 comments on commit 09fa131

Please sign in to comment.