Require explanation of trivial traversable derives

Author: eggyal

compiler/rustc_hir/src/hir.rs

@@ -3257,6 +3257,9 @@ impl<'hir> Item<'hir> {
 
 #[derive(Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash, Debug)]
 #[derive(Encodable, Decodable, HashStable_Generic, TypeFoldable, TypeVisitable)]
+#[skip_traversal(
+    but_impl_despite_trivial_because = "`Unsafety` impls `Relate`, which is a subtrait of `TypeFoldable`."
+)]
 pub enum Unsafety {
     Unsafe,
     Normal,

compiler/rustc_macros/src/lib.rs

@@ -90,10 +90,21 @@ decl_derive!(
     /// only be applicable if `T` does not contain anything that may be of interest to folders
     /// (thus preventing fields from being so-skipped erroneously).
     ///
+    /// By default, `TypeFoldable` cannot be derived on types that contain nothing that may be of
+    /// interest to folders as such an implementation is wholly superfluous and probably in error.
+    /// However, on occasion it may nevertheless be necessary to implement `TypeFoldable` for such
+    /// types even though any such fold will always be a noop (e.g. so that instances can be used
+    /// in a generic context that is constrained to implementors of the trait); in such situations
+    /// one can add a `#[skip_traversal(but_impl_despite_trivial_because = "<reason>"]` attribute.
+    ///
     /// In some rare situations, it may be desirable to skip folding of an item or field (or
-    /// variant) that might otherwise be of interest to folders: **this is dangerous and could lead
-    /// to miscompilation if user expectations are not met!**  Nevertheless, such can be achieved
-    /// via a `#[skip_traversal(despite_potential_miscompilation_because = "<reason>"]` attribute.
+    /// variant) that might otherwise be of interest to folders. This can be achieved via a
+    /// `#[skip_traversal(despite_potential_miscompilation_because = "<reason>"]` attribute.
+    /// Whereas the preceding usages of the `#[skip_traversal]` attribute are guaranteed to be
+    /// sound by constraining the interner to implementors of the `TriviallyTraverses<T>` trait,
+    /// use of `despite_potential_miscompilation_because` does not add such constraint or provide
+    /// any such guarantee.  **It is therefore dangerous and could lead to miscompilation if user
+    /// expectations are not met!**
     ///
     /// The derived implementation will use `TyCtxt<'tcx>` as the interner iff the annotated type
     /// has a `'tcx` lifetime parameter; otherwise it will be generic over all interners. It
@@ -124,10 +135,21 @@ decl_derive!(
     /// only be applicable if `T` does not contain anything that may be of interest to visitors
     /// (thus preventing fields from being so-skipped erroneously).
     ///
+    /// By default, `TypeVisitable` cannot be derived on types that contain nothing that may be of
+    /// interest to visitors as such an implementation is wholly superfluous and probably in error.
+    /// However, on occasion it may nevertheless be necessary to implement `TypeVisitable` for such
+    /// types even though any such visit will always be a noop (e.g. so that instances can be used
+    /// in a generic context that is constrained to implementors of the trait); in such situations
+    /// one can add a `#[skip_traversal(but_impl_despite_trivial_because = "<reason>"]` attribute.
+    ///
     /// In some rare situations, it may be desirable to skip visiting of an item or field (or
-    /// variant) that might otherwise be of interest to visitors: **this is dangerous and could lead
-    /// to miscompilation if user expectations are not met!**  Nevertheless, such can be achieved
-    /// via a `#[skip_traversal(despite_potential_miscompilation_because = "<reason>"]` attribute.
+    /// variant) that might otherwise be of interest to visitors. This can be achieved via a
+    /// `#[skip_traversal(despite_potential_miscompilation_because = "<reason>"]` attribute.
+    /// Whereas the preceding usages of the `#[skip_traversal]` attribute are guaranteed to be
+    /// sound by constraining the interner to implementors of the `TriviallyTraverses<T>` trait,
+    /// use of `despite_potential_miscompilation_because` does not add such constraint or provide
+    /// any such guarantee.  **It is therefore dangerous and could lead to miscompilation if user
+    /// expectations are not met!**
     ///
     /// The derived implementation will use `TyCtxt<'tcx>` as the interner iff the annotated type
     /// has a `'tcx` lifetime parameter; otherwise it will be generic over all interners. It

compiler/rustc_macros/src/traversable.rs

@@ -90,6 +90,15 @@ impl WhenToSkip {
             if attr.path().is_ident("skip_traversal") {
                 found = Some(attr);
                 attr.parse_nested_meta(|meta| {
+                    if IS_TYPE
+                        && ty == Trivial
+                        && meta.path.is_ident("but_impl_despite_trivial_because")
+                    {
+                        parse_reason(&meta)?;
+                        *self |= Self::Always(meta.error("").span());
+                        return Ok(());
+                    }
+
                     if meta.path.is_ident("despite_potential_miscompilation_because") {
                         parse_reason(&meta)?;
                         *self |= Self::Forced;
@@ -111,9 +120,7 @@ impl WhenToSkip {
                 attr,
                 if IS_TYPE {
                     match ty {
-                        Trivial => {
-                            "trivially traversable types are always skipped, so this attribute is superfluous"
-                        }
+                        Trivial => return Ok(()), // triggers error in caller
                         _ => {
                             "\
                             Justification must be provided for skipping this potentially interesting type, by specifying\n\
@@ -358,6 +365,15 @@ pub fn traversable_derive<T: Traversable>(
             structure.add_where_predicate(skip_traversal(&<Token![Self]>::default()));
         }
         T::traverse(quote! { self }, true, &interner)
+    } else if ty == Trivial {
+        return Err(Error::new(
+            Span::call_site(),
+            "\
+            Traversal of trivial types are no-ops by default, so explicitly deriving the traversable traits for them is rarely necessary.\n\
+            If the need has arisen to due the appearance of this type in an anonymous tuple, consider replacing that tuple with a named struct;\n\
+            otherwise add `#[skip_traversal(but_impl_despite_trivial_because = \"<reason for implementation>\")]` to this type.\
+        ",
+        ));
     } else {
         // We add predicates to each generic field type, rather than to our generic type parameters.
         // This results in a "perfect derive" that avoids having to propagate `#[skip_traversal]` annotations

compiler/rustc_macros/src/traversable/tests.rs

@@ -203,12 +203,41 @@ fn interesting_fields_are_constrained() {
 }
 
 #[test]
-fn skipping_trivial_type_is_superfluous() {
+fn skipping_trivial_type_requires_justification() {
     expect! {
+        {
+            struct NothingInteresting<'a>;
+        } => "Traversal of trivial types are no-ops by default"
+
         {
             #[skip_traversal()]
             struct NothingInteresting<'a>;
-        } => "trivially traversable types are always skipped, so this attribute is superfluous"
+        } => "Traversal of trivial types are no-ops by default"
+
+        {
+            #[skip_traversal(but_impl_despite_trivial_because = ".", despite_potential_miscompilation_because = ".")]
+            struct NothingInteresting<'a>;
+        } => {
+            impl<'a, I: Interner> TypeFoldable<I> for NothingInteresting<'a> {
+                fn try_fold_with<T: FallibleTypeFolder<I>>(self, folder: &mut T) -> Result<Self, T::Error> {
+                    Ok(self) // no attempt to fold
+                }
+            }
+        }
+
+        {
+            #[skip_traversal(but_impl_despite_trivial_because = ".")]
+            struct NothingInteresting<'a>;
+        } => {
+            impl<'a, I: Interner> TypeFoldable<I> for NothingInteresting<'a>
+            where
+                I: TriviallyTraverses<Self> // impl only applies when type actually contains nothing interesting
+            {
+                fn try_fold_with<T: FallibleTypeFolder<I>>(self, folder: &mut T) -> Result<Self, T::Error> {
+                    Ok(self) // no attempt to fold
+                }
+            }
+        }
     }
 }
 

compiler/rustc_middle/src/ty/closure.rs

@@ -78,6 +78,9 @@ pub type MinCaptureList<'tcx> = Vec<CapturedPlace<'tcx>>;
 /// `tcx.closure_env_ty()`.
 #[derive(Clone, Copy, PartialOrd, Ord, PartialEq, Eq, Hash, Debug, TyEncodable, TyDecodable)]
 #[derive(HashStable, TypeFoldable, TypeVisitable)]
+#[skip_traversal(
+    but_impl_despite_trivial_because = "traversed generically in `rustc_type_ir::PredicateKind<TyCtxt>`"
+)]
 pub enum ClosureKind {
     // Warning: Ordering is significant here! The ordering is chosen
     // because the trait Fn is a subtrait of FnMut and so in turn, and

compiler/rustc_middle/src/ty/mod.rs

@@ -251,6 +251,9 @@ pub enum ImplSubject<'tcx> {
 
 #[derive(Copy, Clone, PartialEq, Eq, Hash, TyEncodable, TyDecodable, HashStable, Debug)]
 #[derive(TypeFoldable, TypeVisitable)]
+#[skip_traversal(but_impl_despite_trivial_because = "
+    `ImplPolarity` impls `Relate`, which is a subtrait of `TypeFoldable`.
+")]
 pub enum ImplPolarity {
     /// `impl Trait for Type`
     Positive,
@@ -306,6 +309,9 @@ pub enum Visibility<Id = LocalDefId> {
 
 #[derive(Copy, Clone, Debug, PartialEq, Eq, Hash, HashStable, TyEncodable, TyDecodable)]
 #[derive(TypeFoldable, TypeVisitable)]
+#[skip_traversal(but_impl_despite_trivial_because = "
+    `BoundConstness` impls `Relate`, which is a subtrait of `TypeFoldable`.
+")]
 pub enum BoundConstness {
     /// `T: Trait`
     NotConst,

compiler/rustc_span/src/def_id.rs

@@ -239,6 +239,9 @@ impl<D: Decoder> Decodable<D> for DefIndex {
 ///
 /// You can create a `DefId` from a `LocalDefId` using `local_def_id.to_def_id()`.
 #[derive(Clone, PartialEq, Eq, Copy, TypeFoldable, TypeVisitable)]
+#[skip_traversal(
+    but_impl_despite_trivial_because = "traversed generically in `rustc_type_ir::PredicateKind<TyCtxt>`"
+)]
 // Don't derive order on 64-bit big-endian, so we can be consistent regardless of field order.
 #[cfg_attr(not(all(target_pointer_width = "64", target_endian = "big")), derive(PartialOrd, Ord))]
 // On below-64 bit systems we can simply use the derived `Hash` impl

compiler/rustc_target/src/spec/abi/mod.rs

@@ -9,6 +9,9 @@ mod tests;
 
 #[derive(PartialEq, Eq, PartialOrd, Ord, Hash, Clone, Copy, Debug)]
 #[derive(HashStable_Generic, Encodable, Decodable, TypeFoldable, TypeVisitable)]
+#[skip_traversal(
+    but_impl_despite_trivial_because = "`Abi` impls `Relate`, which is a subtrait of `TypeFoldable`."
+)]
 pub enum Abi {
     // Some of the ABIs come first because every time we add a new ABI, we have to re-bless all the
     // hashing tests. These are used in many places, so giving them stable values reduces test