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.

Sign up for GitHub

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

Jump to bottom

Fix supertrait associated type unsoundness #126090

Merged
merged 2 commits into from
Jul 26, 2024
Merged

Fix supertrait associated type unsoundness #126090

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8edf9b8
Item bounds can reference self projections and still be object safe
compiler-errors Mar 21, 2024
172cf9b
Fix unsoundness when associated types dont actually come from supertr…
compiler-errors Jun 6, 2024
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
308 changes: 194 additions & 114 deletions compiler/rustc_trait_selection/src/traits/object_safety.rs
View file
Open in desktop
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).iter_identity_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,121 +731,181 @@ 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),
)
Comment on lines +833 to +835
Copy link
Member Author

Choose a reason for hiding this comment

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

This sucks lol. but I don't really care about regions here

})
.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);

// only walk contained types if it's not a super trait
if is_supertrait_of_current_trait {
ControlFlow::Continue(())
} else {
t.super_visit_with(self) // 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,
},
),
);

// only walk contained types if it's not a super trait
if is_supertrait_of_current_trait {
ControlFlow::Continue(())
} else {
t.super_visit_with(self) // POSSIBLY reporting an error
}
}
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 cx(&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
Loading