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

Author: bors

compiler/rustc_trait_selection/src/traits/object_safety.rs

@@ -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;
@@ -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()
 }
 
@@ -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
@@ -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(..)
@@ -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, _),
                 ..
@@ -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()) {
@@ -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);
     }
@@ -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>(