Skip to content

Commit 09fa131

Browse files
borsbors
committed
Auto merge of #126090 - compiler-errors:supertrait-assoc-ty-unsoundness, 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
2 parents e1ac0fa + 3c44574 commit 09fa131

File tree

4 files changed

+316
-113
lines changed

4 files changed

+316
-113
lines changed

‎compiler/rustc_trait_selection/src/traits/object_safety.rs

+193-113
Original file line numberDiff line numberDiff line change
@@ -12,17 +12,16 @@ use super::elaborate;
1212

1313
use crate::infer::TyCtxtInferExt;
1414
use crate::traits::query::evaluate_obligation::InferCtxtExt;
15-
use crate::traits::{self, Obligation, ObligationCause};
15+
use crate::traits::{util, Obligation, ObligationCause};
1616
use rustc_errors::FatalError;
1717
use rustc_hir as hir;
1818
use rustc_hir::def_id::DefId;
1919
use rustc_middle::query::Providers;
2020
use rustc_middle::ty::{
21-
self, EarlyBinder, ExistentialPredicateStableCmpExt as _, Ty, TyCtxt, TypeSuperVisitable,
22-
TypeVisitable, TypeVisitor,
21+
self, EarlyBinder, ExistentialPredicateStableCmpExt as _, GenericArgs, Ty, TyCtxt,
22+
TypeFoldable, TypeFolder, TypeSuperFoldable, TypeSuperVisitable, TypeVisitable,
23+
TypeVisitableExt, TypeVisitor, Upcast,
2324
};
24-
use rustc_middle::ty::{GenericArg, GenericArgs};
25-
use rustc_middle::ty::{TypeVisitableExt, Upcast};
2625
use rustc_span::symbol::Symbol;
2726
use rustc_span::Span;
2827
use rustc_target::abi::Abi;
@@ -195,7 +194,13 @@ fn predicates_reference_self(
195194
.predicates
196195
.iter()
197196
.map(|&(predicate, sp)| (predicate.instantiate_supertrait(tcx, &trait_ref), sp))
198-
.filter_map(|predicate| predicate_references_self(tcx, predicate))
197+
.filter_map(|(clause, sp)| {
198+
// Super predicates cannot allow self projections, since they're
199+
// impossible to make into existential bounds without eager resolution
200+
// or something.
201+
// e.g. `trait A: B<Item = Self::Assoc>`.
202+
predicate_references_self(tcx, trait_def_id, clause, sp, AllowSelfProjections::No)
203+
})
199204
.collect()
200205
}
201206

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

211220
fn predicate_references_self<'tcx>(
212221
tcx: TyCtxt<'tcx>,
213-
(predicate, sp): (ty::Clause<'tcx>, Span),
222+
trait_def_id: DefId,
223+
predicate: ty::Clause<'tcx>,
224+
sp: Span,
225+
allow_self_projections: AllowSelfProjections,
214226
) -> Option<Span> {
215-
let self_ty = tcx.types.self_param;
216-
let has_self_ty = |arg: &GenericArg<'tcx>| arg.walk().any(|arg| arg == self_ty.into());
217227
match predicate.kind().skip_binder() {
218228
ty::ClauseKind::Trait(ref data) => {
219229
// In the case of a trait predicate, we can skip the "self" type.
220-
data.trait_ref.args[1..].iter().any(has_self_ty).then_some(sp)
230+
data.trait_ref.args[1..].iter().any(|&arg| contains_illegal_self_type_reference(tcx, trait_def_id, arg, allow_self_projections)).then_some(sp)
221231
}
222232
ty::ClauseKind::Projection(ref data) => {
223233
// And similarly for projections. This should be redundant with
@@ -235,9 +245,9 @@ fn predicate_references_self<'tcx>(
235245
//
236246
// This is ALT2 in issue #56288, see that for discussion of the
237247
// possible alternatives.
238-
data.projection_term.args[1..].iter().any(has_self_ty).then_some(sp)
248+
data.projection_term.args[1..].iter().any(|&arg| contains_illegal_self_type_reference(tcx, trait_def_id, arg, allow_self_projections)).then_some(sp)
239249
}
240-
ty::ClauseKind::ConstArgHasType(_ct, ty) => has_self_ty(&ty.into()).then_some(sp),
250+
ty::ClauseKind::ConstArgHasType(_ct, ty) => contains_illegal_self_type_reference(tcx, trait_def_id, ty, allow_self_projections).then_some(sp),
241251

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

385395
for (i, &input_ty) in sig.skip_binder().inputs().iter().enumerate().skip(1) {
386-
if contains_illegal_self_type_reference(tcx, trait_def_id, sig.rebind(input_ty)) {
396+
if contains_illegal_self_type_reference(
397+
tcx,
398+
trait_def_id,
399+
sig.rebind(input_ty),
400+
AllowSelfProjections::Yes,
401+
) {
387402
let span = if let Some(hir::Node::TraitItem(hir::TraitItem {
388403
kind: hir::TraitItemKind::Fn(sig, _),
389404
..
@@ -396,7 +411,12 @@ fn virtual_call_violations_for_method<'tcx>(
396411
errors.push(MethodViolationCode::ReferencesSelfInput(span));
397412
}
398413
}
399-
if contains_illegal_self_type_reference(tcx, trait_def_id, sig.output()) {
414+
if contains_illegal_self_type_reference(
415+
tcx,
416+
trait_def_id,
417+
sig.output(),
418+
AllowSelfProjections::Yes,
419+
) {
400420
errors.push(MethodViolationCode::ReferencesSelfOutput);
401421
}
402422
if let Some(code) = contains_illegal_impl_trait_in_trait(tcx, method.def_id, sig.output()) {
@@ -482,7 +502,7 @@ fn virtual_call_violations_for_method<'tcx>(
482502
return false;
483503
}
484504

485-
contains_illegal_self_type_reference(tcx, trait_def_id, pred)
505+
contains_illegal_self_type_reference(tcx, trait_def_id, pred, AllowSelfProjections::Yes)
486506
}) {
487507
errors.push(MethodViolationCode::WhereClauseReferencesSelf);
488508
}
@@ -711,120 +731,180 @@ fn receiver_is_dispatchable<'tcx>(
711731
infcx.predicate_must_hold_modulo_regions(&obligation)
712732
}
713733

734+
#[derive(Copy, Clone)]
735+
enum AllowSelfProjections {
736+
Yes,
737+
No,
738+
}
739+
740+
/// This is somewhat subtle. In general, we want to forbid
741+
/// references to `Self` in the argument and return types,
742+
/// since the value of `Self` is erased. However, there is one
743+
/// exception: it is ok to reference `Self` in order to access
744+
/// an associated type of the current trait, since we retain
745+
/// the value of those associated types in the object type
746+
/// itself.
747+
///
748+
/// ```rust,ignore (example)
749+
/// trait SuperTrait {
750+
/// type X;
751+
/// }
752+
///
753+
/// trait Trait : SuperTrait {
754+
/// type Y;
755+
/// fn foo(&self, x: Self) // bad
756+
/// fn foo(&self) -> Self // bad
757+
/// fn foo(&self) -> Option<Self> // bad
758+
/// fn foo(&self) -> Self::Y // OK, desugars to next example
759+
/// fn foo(&self) -> <Self as Trait>::Y // OK
760+
/// fn foo(&self) -> Self::X // OK, desugars to next example
761+
/// fn foo(&self) -> <Self as SuperTrait>::X // OK
762+
/// }
763+
/// ```
764+
///
765+
/// However, it is not as simple as allowing `Self` in a projected
766+
/// type, because there are illegal ways to use `Self` as well:
767+
///
768+
/// ```rust,ignore (example)
769+
/// trait Trait : SuperTrait {
770+
/// ...
771+
/// fn foo(&self) -> <Self as SomeOtherTrait>::X;
772+
/// }
773+
/// ```
774+
///
775+
/// Here we will not have the type of `X` recorded in the
776+
/// object type, and we cannot resolve `Self as SomeOtherTrait`
777+
/// without knowing what `Self` is.
714778
fn contains_illegal_self_type_reference<'tcx, T: TypeVisitable<TyCtxt<'tcx>>>(
715779
tcx: TyCtxt<'tcx>,
716780
trait_def_id: DefId,
717781
value: T,
782+
allow_self_projections: AllowSelfProjections,
718783
) -> bool {
719-
// This is somewhat subtle. In general, we want to forbid
720-
// references to `Self` in the argument and return types,
721-
// since the value of `Self` is erased. However, there is one
722-
// exception: it is ok to reference `Self` in order to access
723-
// an associated type of the current trait, since we retain
724-
// the value of those associated types in the object type
725-
// itself.
726-
//
727-
// ```rust
728-
// trait SuperTrait {
729-
// type X;
730-
// }
731-
//
732-
// trait Trait : SuperTrait {
733-
// type Y;
734-
// fn foo(&self, x: Self) // bad
735-
// fn foo(&self) -> Self // bad
736-
// fn foo(&self) -> Option<Self> // bad
737-
// fn foo(&self) -> Self::Y // OK, desugars to next example
738-
// fn foo(&self) -> <Self as Trait>::Y // OK
739-
// fn foo(&self) -> Self::X // OK, desugars to next example
740-
// fn foo(&self) -> <Self as SuperTrait>::X // OK
741-
// }
742-
// ```
743-
//
744-
// However, it is not as simple as allowing `Self` in a projected
745-
// type, because there are illegal ways to use `Self` as well:
746-
//
747-
// ```rust
748-
// trait Trait : SuperTrait {
749-
// ...
750-
// fn foo(&self) -> <Self as SomeOtherTrait>::X;
751-
// }
752-
// ```
753-
//
754-
// Here we will not have the type of `X` recorded in the
755-
// object type, and we cannot resolve `Self as SomeOtherTrait`
756-
// without knowing what `Self` is.
757-
758-
struct IllegalSelfTypeVisitor<'tcx> {
759-
tcx: TyCtxt<'tcx>,
760-
trait_def_id: DefId,
761-
supertraits: Option<Vec<DefId>>,
762-
}
784+
value
785+
.visit_with(&mut IllegalSelfTypeVisitor {
786+
tcx,
787+
trait_def_id,
788+
supertraits: None,
789+
allow_self_projections,
790+
})
791+
.is_break()
792+
}
763793

764-
impl<'tcx> TypeVisitor<TyCtxt<'tcx>> for IllegalSelfTypeVisitor<'tcx> {
765-
type Result = ControlFlow<()>;
794+
struct IllegalSelfTypeVisitor<'tcx> {
795+
tcx: TyCtxt<'tcx>,
796+
trait_def_id: DefId,
797+
supertraits: Option<Vec<ty::TraitRef<'tcx>>>,
798+
allow_self_projections: AllowSelfProjections,
799+
}
766800

767-
fn visit_ty(&mut self, t: Ty<'tcx>) -> Self::Result {
768-
match t.kind() {
769-
ty::Param(_) => {
770-
if t == self.tcx.types.self_param {
771-
ControlFlow::Break(())
772-
} else {
773-
ControlFlow::Continue(())
774-
}
775-
}
776-
ty::Alias(ty::Projection, ref data)
777-
if self.tcx.is_impl_trait_in_trait(data.def_id) =>
778-
{
779-
// We'll deny these later in their own pass
801+
impl<'tcx> TypeVisitor<TyCtxt<'tcx>> for IllegalSelfTypeVisitor<'tcx> {
802+
type Result = ControlFlow<()>;
803+
804+
fn visit_ty(&mut self, t: Ty<'tcx>) -> Self::Result {
805+
match t.kind() {
806+
ty::Param(_) => {
807+
if t == self.tcx.types.self_param {
808+
ControlFlow::Break(())
809+
} else {
780810
ControlFlow::Continue(())
781811
}
782-
ty::Alias(ty::Projection, ref data) => {
783-
// This is a projected type `<Foo as SomeTrait>::X`.
784-
785-
// Compute supertraits of current trait lazily.
786-
if self.supertraits.is_none() {
787-
let trait_ref =
788-
ty::Binder::dummy(ty::TraitRef::identity(self.tcx, self.trait_def_id));
789-
self.supertraits = Some(
790-
traits::supertraits(self.tcx, trait_ref).map(|t| t.def_id()).collect(),
791-
);
792-
}
812+
}
813+
ty::Alias(ty::Projection, ref data) if self.tcx.is_impl_trait_in_trait(data.def_id) => {
814+
// We'll deny these later in their own pass
815+
ControlFlow::Continue(())
816+
}
817+
ty::Alias(ty::Projection, ref data) => {
818+
match self.allow_self_projections {
819+
AllowSelfProjections::Yes => {
820+
// This is a projected type `<Foo as SomeTrait>::X`.
821+
822+
// Compute supertraits of current trait lazily.
823+
if self.supertraits.is_none() {
824+
self.supertraits = Some(
825+
util::supertraits(
826+
self.tcx,
827+
ty::Binder::dummy(ty::TraitRef::identity(
828+
self.tcx,
829+
self.trait_def_id,
830+
)),
831+
)
832+
.map(|trait_ref| {
833+
self.tcx.erase_regions(
834+
self.tcx.instantiate_bound_regions_with_erased(trait_ref),
835+
)
836+
})
837+
.collect(),
838+
);
839+
}
793840

794-
// Determine whether the trait reference `Foo as
795-
// SomeTrait` is in fact a supertrait of the
796-
// current trait. In that case, this type is
797-
// legal, because the type `X` will be specified
798-
// in the object type. Note that we can just use
799-
// direct equality here because all of these types
800-
// are part of the formal parameter listing, and
801-
// hence there should be no inference variables.
802-
let is_supertrait_of_current_trait = self
803-
.supertraits
804-
.as_ref()
805-
.unwrap()
806-
.contains(&data.trait_ref(self.tcx).def_id);
807-
808-
if is_supertrait_of_current_trait {
809-
ControlFlow::Continue(()) // do not walk contained types, do not report error, do collect $200
810-
} else {
811-
t.super_visit_with(self) // DO walk contained types, POSSIBLY reporting an error
841+
// Determine whether the trait reference `Foo as
842+
// SomeTrait` is in fact a supertrait of the
843+
// current trait. In that case, this type is
844+
// legal, because the type `X` will be specified
845+
// in the object type. Note that we can just use
846+
// direct equality here because all of these types
847+
// are part of the formal parameter listing, and
848+
// hence there should be no inference variables.
849+
let is_supertrait_of_current_trait =
850+
self.supertraits.as_ref().unwrap().contains(
851+
&data.trait_ref(self.tcx).fold_with(
852+
&mut EraseEscapingBoundRegions {
853+
tcx: self.tcx,
854+
binder: ty::INNERMOST,
855+
},
856+
),
857+
);
858+
859+
if is_supertrait_of_current_trait {
860+
ControlFlow::Continue(())
861+
} else {
862+
t.super_visit_with(self)
863+
}
812864
}
865+
AllowSelfProjections::No => t.super_visit_with(self),
813866
}
814-
_ => t.super_visit_with(self), // walk contained types, if any
815867
}
868+
_ => t.super_visit_with(self),
816869
}
870+
}
817871

818-
fn visit_const(&mut self, ct: ty::Const<'tcx>) -> Self::Result {
819-
// Constants can only influence object safety if they are generic and reference `Self`.
820-
// This is only possible for unevaluated constants, so we walk these here.
821-
self.tcx.expand_abstract_consts(ct).super_visit_with(self)
822-
}
872+
fn visit_const(&mut self, ct: ty::Const<'tcx>) -> Self::Result {
873+
// Constants can only influence object safety if they are generic and reference `Self`.
874+
// This is only possible for unevaluated constants, so we walk these here.
875+
self.tcx.expand_abstract_consts(ct).super_visit_with(self)
823876
}
877+
}
824878

825-
value
826-
.visit_with(&mut IllegalSelfTypeVisitor { tcx, trait_def_id, supertraits: None })
827-
.is_break()
879+
struct EraseEscapingBoundRegions<'tcx> {
880+
tcx: TyCtxt<'tcx>,
881+
binder: ty::DebruijnIndex,
882+
}
883+
884+
impl<'tcx> TypeFolder<TyCtxt<'tcx>> for EraseEscapingBoundRegions<'tcx> {
885+
fn interner(&self) -> TyCtxt<'tcx> {
886+
self.tcx
887+
}
888+
889+
fn fold_binder<T>(&mut self, t: ty::Binder<'tcx, T>) -> ty::Binder<'tcx, T>
890+
where
891+
T: TypeFoldable<TyCtxt<'tcx>>,
892+
{
893+
self.binder.shift_in(1);
894+
let result = t.super_fold_with(self);
895+
self.binder.shift_out(1);
896+
result
897+
}
898+
899+
fn fold_region(&mut self, r: ty::Region<'tcx>) -> ty::Region<'tcx> {
900+
if let ty::ReBound(debruijn, _) = *r
901+
&& debruijn < self.binder
902+
{
903+
r
904+
} else {
905+
self.tcx.lifetimes.re_erased
906+
}
907+
}
828908
}
829909

830910
pub fn contains_illegal_impl_trait_in_trait<'tcx>(

0 commit comments

Comments
 (0)