Explain no-op traversable impls

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_boring_because = "
+    `Unsafety` impls `Relate`, which is a subtrait of `TypeFoldable`.
+")]
 pub enum Unsafety {
     Unsafe,
     Normal,

compiler/rustc_macros/src/lib.rs

@@ -88,10 +88,21 @@ decl_derive!(
     /// that may be of interest to folders (thus preventing fields from being left unchanged
     /// 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_boring_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 relevant type to implementors of the `BoringTraversable`
+    /// 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
@@ -119,10 +130,21 @@ decl_derive!(
     /// fields do 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_boring_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 relevant type to implementors of the `BoringTraversable`
+    /// 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

@@ -26,6 +26,7 @@ fn gen_param(suffix: impl ToString, existing: &Generics) -> Ident {
 
 mod kw {
     syn::custom_keyword!(because_boring); // only applicable on internable fields
+    syn::custom_keyword!(but_impl_despite_boring_because); // only applicable on non-internable types
     syn::custom_keyword!(despite_potential_miscompilation_because); // always applicable on internable types and fields
 }
 
@@ -35,6 +36,9 @@ trait SkipReasonKeyword: Default + Parse + ToTokens {
 impl SkipReasonKeyword for kw::because_boring {
     const HAS_EXPLANATION: bool = false;
 }
+impl SkipReasonKeyword for kw::but_impl_despite_boring_because {
+    const HAS_EXPLANATION: bool = true;
+}
 impl SkipReasonKeyword for kw::despite_potential_miscompilation_because {
     const HAS_EXPLANATION: bool = true;
 }
@@ -99,7 +103,7 @@ impl WhenToSkip {
         attr.meta
             .require_list()
             .and_then(|list| match (IS_TYPE, ty) {
-                (true, Boring) => Err(Error::new_spanned(attr, "boring types are always skipped, so this attribute is superfluous")),
+                (true, Boring) => list.parse_args::<SkipReason<kw::but_impl_despite_boring_because>>().and_then(|_| Ok(Self::Always(attr.span()))),
                 (true, _) | (false, NotGeneric) => list.parse_args::<SkipReason<kw::despite_potential_miscompilation_because>>().and(Ok(Self::Forced)),
                 (false, Generic) => list.parse_args::<SkipReason<kw::because_boring>>().and_then(|_| Ok(Self::Always(attr.span())))
                     .or_else(|_| list.parse_args::<SkipReason<kw::despite_potential_miscompilation_because>>().and(Ok(Self::Forced)))
@@ -332,6 +336,15 @@ pub fn traversable_derive<T: Traversable>(
             structure.add_where_predicate(parse_quote! { Self: #skip_traversal });
         }
         T::traverse(quote! { self }, true)
+    } else if ty == Type::Boring {
+        return Err(Error::new(
+            Span::call_site(),
+            "\
+            Traversal of boring 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_boring_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

@@ -194,12 +194,40 @@ fn interesting_fields_are_constrained() {
 }
 
 #[test]
-fn skipping_boring_type_is_superfluous() {
+fn boring_type_requires_justification() {
     expect! {
+        {
+            struct NothingInteresting<'a>;
+        } => "Traversal of boring types are no-ops by default"
+
         {
             #[skip_traversal()]
             struct NothingInteresting<'a>;
-        } => "boring types are always skipped, so this attribute is superfluous"
+        } => "unexpected end of input, expected `but_impl_despite_boring_because`"
+
+        {
+            #[skip_traversal(despite_potential_miscompilation_because = ".")]
+            struct NothingInteresting<'a>;
+        } => "expected `but_impl_despite_boring_because`"
+
+        {
+            #[skip_traversal(but_impl_despite_boring_because = ".", despite_potential_miscompilation_because = ".")]
+            struct NothingInteresting<'a>;
+        } => "unexpected token"
+
+        {
+            #[skip_traversal(but_impl_despite_boring_because = ".")]
+            struct NothingInteresting<'a>;
+        } => {
+            impl<'a, I: Interner> TypeFoldable<I> for NothingInteresting<'a>
+            where
+                Self: BoringTraversable // 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/mod.rs

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

compiler/rustc_target/src/spec/abi.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_boring_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