check for object safety violations in constants

Author: lcnr

compiler/rustc_trait_selection/src/traits/const_evaluatable.rs

@@ -85,8 +85,10 @@ pub fn is_const_evaluatable<'cx, 'tcx>(
                         } else if leaf.has_param_types_or_consts() {
                             failure_kind = cmp::min(failure_kind, FailureKind::MentionsParam);
                         }
+
+                        false
                     }
-                    Node::Binop(_, _, _) | Node::UnaryOp(_, _) | Node::FunctionCall(_, _) => (),
+                    Node::Binop(_, _, _) | Node::UnaryOp(_, _) | Node::FunctionCall(_, _) => false,
                 });
 
                 match failure_kind {
@@ -194,12 +196,12 @@ pub fn is_const_evaluatable<'cx, 'tcx>(
 ///
 /// This is only able to represent a subset of `MIR`,
 /// and should not leak any information about desugarings.
-#[derive(Clone, Copy)]
+#[derive(Debug, Clone, Copy)]
 pub struct AbstractConst<'tcx> {
     // FIXME: Consider adding something like `IndexSlice`
     // and use this here.
-    inner: &'tcx [Node<'tcx>],
-    substs: SubstsRef<'tcx>,
+    pub inner: &'tcx [Node<'tcx>],
+    pub substs: SubstsRef<'tcx>,
 }
 
 impl AbstractConst<'tcx> {
@@ -212,6 +214,17 @@ impl AbstractConst<'tcx> {
         Ok(inner.map(|inner| AbstractConst { inner, substs }))
     }
 
+    pub fn from_const(
+        tcx: TyCtxt<'tcx>,
+        ct: &ty::Const<'tcx>,
+    ) -> Result<Option<AbstractConst<'tcx>>, ErrorReported> {
+        match ct.val {
+            ty::ConstKind::Unevaluated(def, substs, None) => AbstractConst::new(tcx, def, substs),
+            ty::ConstKind::Error(_) => Err(ErrorReported),
+            _ => Ok(None),
+        }
+    }
+
     #[inline]
     pub fn subtree(self, node: NodeId) -> AbstractConst<'tcx> {
         AbstractConst { inner: &self.inner[..=node.index()], substs: self.substs }
@@ -550,31 +563,32 @@ pub(super) fn try_unify_abstract_consts<'tcx>(
     // on `ErrorReported`.
 }
 
-fn walk_abstract_const<'tcx, F>(tcx: TyCtxt<'tcx>, ct: AbstractConst<'tcx>, mut f: F)
+// FIXME: Use `std::ops::ControlFlow` instead of `bool` here.
+pub fn walk_abstract_const<'tcx, F>(tcx: TyCtxt<'tcx>, ct: AbstractConst<'tcx>, mut f: F) -> bool
 where
-    F: FnMut(Node<'tcx>),
+    F: FnMut(Node<'tcx>) -> bool,
 {
-    recurse(tcx, ct, &mut f);
-    fn recurse<'tcx>(tcx: TyCtxt<'tcx>, ct: AbstractConst<'tcx>, f: &mut dyn FnMut(Node<'tcx>)) {
+    fn recurse<'tcx>(
+        tcx: TyCtxt<'tcx>,
+        ct: AbstractConst<'tcx>,
+        f: &mut dyn FnMut(Node<'tcx>) -> bool,
+    ) -> bool {
         let root = ct.root();
-        f(root);
-        match root {
-            Node::Leaf(_) => (),
-            Node::Binop(_, l, r) => {
-                recurse(tcx, ct.subtree(l), f);
-                recurse(tcx, ct.subtree(r), f);
-            }
-            Node::UnaryOp(_, v) => {
-                recurse(tcx, ct.subtree(v), f);
-            }
-            Node::FunctionCall(func, args) => {
-                recurse(tcx, ct.subtree(func), f);
-                for &arg in args {
-                    recurse(tcx, ct.subtree(arg), f);
+        f(root)
+            || match root {
+                Node::Leaf(_) => false,
+                Node::Binop(_, l, r) => {
+                    recurse(tcx, ct.subtree(l), f) || recurse(tcx, ct.subtree(r), f)
+                }
+                Node::UnaryOp(_, v) => recurse(tcx, ct.subtree(v), f),
+                Node::FunctionCall(func, args) => {
+                    recurse(tcx, ct.subtree(func), f)
+                        || args.iter().any(|&arg| recurse(tcx, ct.subtree(arg), f))
                 }
             }
-        }
     }
+
+    recurse(tcx, ct, &mut f)
 }
 
 /// Tries to unify two abstract constants using structural equality.

compiler/rustc_trait_selection/src/traits/object_safety.rs

@@ -11,6 +11,7 @@
 use super::elaborate_predicates;
 
 use crate::infer::TyCtxtInferExt;
+use crate::traits::const_evaluatable::{self, AbstractConst};
 use crate::traits::query::evaluate_obligation::InferCtxtExt;
 use crate::traits::{self, Obligation, ObligationCause};
 use rustc_errors::FatalError;
@@ -249,7 +250,7 @@ fn predicates_reference_self(
     predicates
         .predicates
         .iter()
-        .map(|(predicate, sp)| (predicate.subst_supertrait(tcx, &trait_ref), *sp))
+        .map(|&(predicate, sp)| (predicate.subst_supertrait(tcx, &trait_ref), sp))
         .filter_map(|predicate| predicate_references_self(tcx, predicate))
         .collect()
 }
@@ -260,7 +261,7 @@ 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))
-        .map(|(predicate, sp)| (predicate.subst_supertrait(tcx, &trait_ref), *sp))
+        .map(|&(predicate, sp)| (predicate.subst_supertrait(tcx, &trait_ref), sp))
         .filter_map(|predicate| predicate_references_self(tcx, predicate))
         .collect()
 }
@@ -415,7 +416,7 @@ fn virtual_call_violation_for_method<'tcx>(
         ));
     }
 
-    for (i, input_ty) in sig.skip_binder().inputs()[1..].iter().enumerate() {
+    for (i, &input_ty) in sig.skip_binder().inputs()[1..].iter().enumerate() {
         if contains_illegal_self_type_reference(tcx, trait_def_id, input_ty) {
             return Some(MethodViolationCode::ReferencesSelfInput(i));
         }
@@ -438,10 +439,7 @@ fn virtual_call_violation_for_method<'tcx>(
         // so outlives predicates will always hold.
         .cloned()
         .filter(|(p, _)| p.to_opt_type_outlives().is_none())
-        .collect::<Vec<_>>()
-        // Do a shallow visit so that `contains_illegal_self_type_reference`
-        // may apply it's custom visiting.
-        .visit_tys_shallow(|t| contains_illegal_self_type_reference(tcx, trait_def_id, t))
+        .any(|pred| contains_illegal_self_type_reference(tcx, trait_def_id, pred))
     {
         return Some(MethodViolationCode::WhereClauseReferencesSelf);
     }
@@ -715,10 +713,10 @@ fn receiver_is_dispatchable<'tcx>(
     })
 }
 
-fn contains_illegal_self_type_reference<'tcx>(
+fn contains_illegal_self_type_reference<'tcx, T: TypeFoldable<'tcx>>(
     tcx: TyCtxt<'tcx>,
     trait_def_id: DefId,
-    ty: Ty<'tcx>,
+    value: T,
 ) -> bool {
     // This is somewhat subtle. In general, we want to forbid
     // references to `Self` in the argument and return types,
@@ -761,15 +759,14 @@ fn contains_illegal_self_type_reference<'tcx>(
 
     struct IllegalSelfTypeVisitor<'tcx> {
         tcx: TyCtxt<'tcx>,
-        self_ty: Ty<'tcx>,
         trait_def_id: DefId,
         supertraits: Option<Vec<ty::PolyTraitRef<'tcx>>>,
     }
 
     impl<'tcx> TypeVisitor<'tcx> for IllegalSelfTypeVisitor<'tcx> {
         fn visit_ty(&mut self, t: Ty<'tcx>) -> bool {
             match t.kind() {
-                ty::Param(_) => t == self.self_ty,
+                ty::Param(_) => t == self.tcx.types.self_param,
                 ty::Projection(ref data) => {
                     // This is a projected type `<Foo as SomeTrait>::X`.
 
@@ -802,22 +799,62 @@ fn contains_illegal_self_type_reference<'tcx>(
             }
         }
 
-        fn visit_const(&mut self, _c: &ty::Const<'tcx>) -> bool {
-            // FIXME(#72219) Look into the unevaluated constants for object safety violations.
-            // Do not walk substitutions of unevaluated consts, as they contain `Self`, even
-            // though the const expression doesn't necessary use it. Currently type variables
-            // inside array length expressions are forbidden, so they can't break the above
-            // rules.
-            false
+        fn visit_const(&mut self, ct: &ty::Const<'tcx>) -> bool {
+            // First check if the type of this constant references `Self`.
+            if self.visit_ty(ct.ty) {
+                return true;
+            }
+
+            // Constants can only influence object safety if they reference `Self`.
+            // This is only possible for unevaluated constants, so we walk these here.
+            //
+            // If `AbstractConst::new` returned an error we already failed compilation
+            // so we don't have to emit an additional error here.
+            //
+            // We currently recurse into abstract consts here but do not recurse in
+            // `is_const_evaluatable`. This means that the object safety check is more
+            // liberal than the const eval check.
+            //
+            // This shouldn't really matter though as we can't really use any
+            // constants which are not considered const evaluatable.
+            use rustc_middle::mir::abstract_const::Node;
+            if let Ok(Some(ct)) = AbstractConst::from_const(self.tcx, ct) {
+                const_evaluatable::walk_abstract_const(self.tcx, ct, |node| match node {
+                    Node::Leaf(leaf) => {
+                        let leaf = leaf.subst(self.tcx, ct.substs);
+                        self.visit_const(leaf)
+                    }
+                    Node::Binop(..) | Node::UnaryOp(..) | Node::FunctionCall(_, _) => false,
+                })
+            } else {
+                false
+            }
+        }
+
+        fn visit_predicate(&mut self, pred: ty::Predicate<'tcx>) -> bool {
+            if let ty::PredicateAtom::ConstEvaluatable(def, substs) = pred.skip_binders() {
+                // FIXME(const_evaluatable_checked): We should probably deduplicate the logic for
+                // `AbstractConst`s here, it might make sense to change `ConstEvaluatable` to
+                // take a `ty::Const` instead.
+                use rustc_middle::mir::abstract_const::Node;
+                if let Ok(Some(ct)) = AbstractConst::new(self.tcx, def, substs) {
+                    const_evaluatable::walk_abstract_const(self.tcx, ct, |node| match node {
+                        Node::Leaf(leaf) => {
+                            let leaf = leaf.subst(self.tcx, ct.substs);
+                            self.visit_const(leaf)
+                        }
+                        Node::Binop(..) | Node::UnaryOp(..) | Node::FunctionCall(_, _) => false,
+                    })
+                } else {
+                    false
+                }
+            } else {
+                pred.super_visit_with(self)
+            }
         }
     }
 
-    ty.visit_with(&mut IllegalSelfTypeVisitor {
-        tcx,
-        self_ty: tcx.types.self_param,
-        trait_def_id,
-        supertraits: None,
-    })
+    value.visit_with(&mut IllegalSelfTypeVisitor { tcx, trait_def_id, supertraits: None })
 }
 
 pub fn provide(providers: &mut ty::query::Providers) {

src/test/ui/const-generics/const_evaluatable_checked/object-safety-err-ret.rs

@@ -0,0 +1,21 @@
+#![feature(const_generics, const_evaluatable_checked)]
+#![allow(incomplete_features)]
+
+
+const fn bar<T: ?Sized>() -> usize { 7 }
+
+trait Foo {
+    fn test(&self) -> [u8; bar::<Self>()];
+}
+
+impl Foo for () {
+    fn test(&self) -> [u8; bar::<Self>()] {
+        [0; bar::<Self>()]
+    }
+}
+
+fn use_dyn(v: &dyn Foo) { //~ERROR the trait `Foo` cannot be made into an object
+    v.test();
+}
+
+fn main() {}

src/test/ui/const-generics/const_evaluatable_checked/object-safety-err-ret.stderr

@@ -0,0 +1,18 @@
+error[E0038]: the trait `Foo` cannot be made into an object
+  --> $DIR/object-safety-err-ret.rs:17:15
+   |
+LL | fn use_dyn(v: &dyn Foo) {
+   |               ^^^^^^^^ `Foo` cannot be made into an object
+   |
+   = help: consider moving `test` to another trait
+note: for a trait to be "object safe" it needs to allow building a vtable to allow the call to be resolvable dynamically; for more information visit <https://doc.rust-lang.org/reference/items/traits.html#object-safety>
+  --> $DIR/object-safety-err-ret.rs:8:23
+   |
+LL | trait Foo {
+   |       --- this trait cannot be made into an object...
+LL |     fn test(&self) -> [u8; bar::<Self>()];
+   |                       ^^^^^^^^^^^^^^^^^^^ ...because method `test` references the `Self` type in its return type
+
+error: aborting due to previous error
+
+For more information about this error, try `rustc --explain E0038`.

src/test/ui/const-generics/const_evaluatable_checked/object-safety-err-where-bounds.rs

@@ -0,0 +1,22 @@
+#![feature(const_generics, const_evaluatable_checked)]
+#![allow(incomplete_features)]
+#![deny(where_clauses_object_safety)]
+
+
+const fn bar<T: ?Sized>() -> usize { 7 }
+
+trait Foo {
+    fn test(&self) where [u8; bar::<Self>()]: Sized;
+    //~^ ERROR the trait `Foo` cannot be made into an object
+    //~| WARN this was previously accepted by the compiler but is being phased out
+}
+
+impl Foo for () {
+    fn test(&self) where [u8; bar::<Self>()]: Sized {}
+}
+
+fn use_dyn(v: &dyn Foo) {
+    v.test();
+}
+
+fn main() {}

src/test/ui/const-generics/const_evaluatable_checked/object-safety-err-where-bounds.stderr

@@ -0,0 +1,24 @@
+error: the trait `Foo` cannot be made into an object
+  --> $DIR/object-safety-err-where-bounds.rs:9:8
+   |
+LL |     fn test(&self) where [u8; bar::<Self>()]: Sized;
+   |        ^^^^
+   |
+note: the lint level is defined here
+  --> $DIR/object-safety-err-where-bounds.rs:3:9
+   |
+LL | #![deny(where_clauses_object_safety)]
+   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
+   = note: for more information, see issue #51443 <https://github.com/rust-lang/rust/issues/51443>
+note: for a trait to be "object safe" it needs to allow building a vtable to allow the call to be resolvable dynamically; for more information visit <https://doc.rust-lang.org/reference/items/traits.html#object-safety>
+  --> $DIR/object-safety-err-where-bounds.rs:9:8
+   |
+LL | trait Foo {
+   |       --- this trait cannot be made into an object...
+LL |     fn test(&self) where [u8; bar::<Self>()]: Sized;
+   |        ^^^^ ...because method `test` references the `Self` type in its `where` clause
+   = help: consider moving `test` to another trait
+
+error: aborting due to previous error
+

src/test/ui/const-generics/const_evaluatable_checked/object-safety-ok-infer-err.rs

@@ -0,0 +1,21 @@
+#![feature(const_generics, const_evaluatable_checked)]
+#![allow(incomplete_features)]
+
+trait Foo<const N: usize> {
+    fn test(&self) -> [u8; N + 1];
+}
+
+impl<const N: usize> Foo<N> for () {
+    fn test(&self) -> [u8; N + 1] {
+        [0; N + 1]
+    }
+}
+
+fn use_dyn<const N: usize>(v: &dyn Foo<N>) where [u8; N + 1]: Sized {
+    assert_eq!(v.test(), [0; N + 1]);
+}
+
+fn main() {
+    use_dyn(&());
+    //~^ ERROR type annotations needed
+}

src/test/ui/const-generics/const_evaluatable_checked/object-safety-ok-infer-err.stderr

@@ -0,0 +1,12 @@
+error[E0284]: type annotations needed: cannot satisfy `the constant `use_dyn::<{_: usize}>::{constant#0}` can be evaluated`
+  --> $DIR/object-safety-ok-infer-err.rs:19:5
+   |
+LL | fn use_dyn<const N: usize>(v: &dyn Foo<N>) where [u8; N + 1]: Sized {
+   |                                                       ----- required by this bound in `use_dyn`
+...
+LL |     use_dyn(&());
+   |     ^^^^^^^ cannot satisfy `the constant `use_dyn::<{_: usize}>::{constant#0}` can be evaluated`
+
+error: aborting due to previous error
+
+For more information about this error, try `rustc --explain E0284`.

src/test/ui/const-generics/const_evaluatable_checked/object-safety-ok.rs

@@ -0,0 +1,21 @@
+// run-pass
+#![feature(const_generics, const_evaluatable_checked)]
+#![allow(incomplete_features)]
+
+trait Foo<const N: usize> {
+    fn test(&self) -> [u8; N + 1];
+}
+
+impl<const N: usize> Foo<N> for () {
+    fn test(&self) -> [u8; N + 1] {
+        [0; N + 1]
+    }
+}
+
+fn use_dyn<const N: usize>(v: &dyn Foo<N>) where [u8; N + 1]: Sized {
+    assert_eq!(v.test(), [0; N + 1]);
+}
+
+fn main() {
+    use_dyn::<3>(&());
+}