Enable skipping of derived traversals, with reason

eggyal · eggyal · commit 76e980439833 · 2023-11-05T20:48:18.000Z
diff --git a/compiler/rustc_macros/src/lib.rs b/compiler/rustc_macros/src/lib.rs
@@ -74,14 +74,24 @@ decl_derive!([TyEncodable] => serialize::type_encodable_derive);
 decl_derive!([MetadataDecodable] => serialize::meta_decodable_derive);
 decl_derive!([MetadataEncodable] => serialize::meta_encodable_derive);
 decl_derive!(
-    [TypeFoldable] =>
+    [TypeFoldable, attributes(skip_traversal)] =>
     /// Derives `TypeFoldable` for the annotated `struct` or `enum` (`union` is not supported).
     ///
     /// Folds will produce a value of the same struct or enum variant as the input, with each field
     /// respectively folded (in definition order) using the `TypeFoldable` implementation for its
     /// type if it has one. Fields of non-generic types that do not contain anything that may be of
     /// interest to folders will automatically be left unchanged whether the type implements
-    /// `TypeFoldable` or not.
+    /// `TypeFoldable` or not; the same behaviour can be achieved for fields of generic types by
+    /// applying `#[skip_traversal(because_boring)]` to the field definition (or even to a variant
+    /// definition if it should apply to all fields therein), but the derived implementation will
+    /// only be applicable to concrete types where such annotated fields do not contain anything
+    /// that may be of interest to folders (thus preventing fields from being left unchanged
+    /// erroneously).
+    ///
+    /// 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.
     ///
     /// If the annotated type has a `'tcx` lifetime parameter, then that will be used as the
     /// lifetime for the type context/interner; otherwise the lifetime of the type context/interner
@@ -97,13 +107,23 @@ decl_derive!(
     traversable::traversable_derive::<traversable::Foldable>
 );
 decl_derive!(
-    [TypeVisitable] =>
+    [TypeVisitable, attributes(skip_traversal)] =>
     /// Derives `TypeVisitable` for the annotated `struct` or `enum` (`union` is not supported).
     ///
     /// Each field of the struct or enum variant will be visited (in definition order) using the
     /// `TypeVisitable` implementation for its type if it has one. Fields of non-generic types that
     /// do not contain anything that may be of interest to visitors will automatically be skipped
-    /// whether the type implements `TypeVisitable` or not.
+    /// whether the type implements `TypeVisitable` or not; the same behaviour can be achieved for
+    /// fields of generic types by applying `#[skip_traversal(because_boring)]` to the field
+    /// definition (or even to a variant definition if it should apply to all fields therein), but
+    /// the derived implementation will only be applicable to concrete types where such annotated
+    /// fields do not contain anything that may be of interest to visitors (thus preventing fields
+    /// from being so skipped erroneously).
+    ///
+    /// 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.
     ///
     /// If the annotated type has a `'tcx` lifetime parameter, then that will be used as the
     /// lifetime for the type context/interner; otherwise the lifetime of the type context/interner
diff --git a/compiler/rustc_macros/src/traversable.rs b/compiler/rustc_macros/src/traversable.rs
@@ -1,7 +1,15 @@
 use proc_macro2::{Ident, Span, TokenStream};
 use quote::{quote, ToTokens};
-use std::collections::HashSet;
-use syn::{parse_quote, visit, Field, Generics, Lifetime};
+use std::{
+    collections::{hash_map::Entry, HashMap},
+    marker::PhantomData,
+};
+use syn::{
+    parse::{Error, Parse, ParseStream},
+    parse_quote,
+    spanned::Spanned,
+    visit, Attribute, Field, Generics, Lifetime, LitStr, Token,
+};
 
 #[cfg(test)]
 mod tests;
@@ -16,6 +24,50 @@ fn gen_param(suffix: impl ToString, existing: &Generics) -> Ident {
     Ident::new(&suffix, Span::call_site())
 }
 
+mod kw {
+    syn::custom_keyword!(because_boring); // only applicable on internable fields
+    syn::custom_keyword!(despite_potential_miscompilation_because); // always applicable on internable types and fields
+}
+
+trait SkipReasonKeyword: Default + Parse + ToTokens {
+    const HAS_EXPLANATION: bool;
+}
+impl SkipReasonKeyword for kw::because_boring {
+    const HAS_EXPLANATION: bool = false;
+}
+impl SkipReasonKeyword for kw::despite_potential_miscompilation_because {
+    const HAS_EXPLANATION: bool = true;
+}
+
+struct SkipReason<K>(PhantomData<K>);
+
+impl<K: SkipReasonKeyword> Parse for SkipReason<K> {
+    fn parse(input: ParseStream<'_>) -> Result<Self, Error> {
+        input.parse::<K>()?;
+        if K::HAS_EXPLANATION {
+            input.parse::<Token![=]>()?;
+            let reason = input.parse::<LitStr>()?;
+            if reason.value().trim().is_empty() {
+                return Err(Error::new_spanned(reason, "skip reason must be a non-empty string"));
+            }
+        }
+        Ok(Self(PhantomData))
+    }
+}
+
+#[derive(Clone, Copy)]
+enum WhenToSkip {
+    Forced,
+    Never,
+    Always(Span),
+}
+
+impl WhenToSkip {
+    fn is_skipped(&self) -> bool {
+        !matches!(self, WhenToSkip::Never)
+    }
+}
+
 #[derive(Clone, Copy, PartialEq)]
 enum Type {
     /// Describes a type that is not parameterised by the interner, and therefore cannot
@@ -31,6 +83,36 @@ enum Type {
 }
 use Type::*;
 
+impl WhenToSkip {
+    fn find<const IS_TYPE: bool>(attrs: &[Attribute], ty: Type) -> Result<WhenToSkip, Error> {
+        let mut iter = attrs.iter().filter(|&attr| attr.path().is_ident("skip_traversal"));
+        let Some(attr) = iter.next() else {
+            return Ok(Self::Never);
+        };
+        if let Some(next) = iter.next() {
+            return Err(Error::new_spanned(
+                next,
+                "at most one skip_traversal attribute is supported",
+            ));
+        }
+
+        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, _) | (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)))
+                    .or(Err(Error::new_spanned(attr, "\
+                        Justification must be provided for skipping this potentially interesting field, by specifying EITHER:\n\
+                        `because_boring` if concrete instances never actually contain anything of interest (enforced by the compiler); OR\n\
+                        `despite_potential_miscompilation_because = \"<reason>\"` if this field should always be skipped regardless\
+                    "))),
+                (false, Boring) => Err(Error::new_spanned(attr, "boring fields are always skipped, so this attribute is superfluous")),
+            })
+    }
+}
+
 pub struct Interner<'a>(Option<&'a Lifetime>);
 
 impl<'a> Interner<'a> {
@@ -67,13 +149,13 @@ pub trait Traversable {
     /// Any supertraits that this trait is required to implement.
     fn supertraits(interner: &Interner<'_>) -> TokenStream;
 
-    /// A traversal of this trait upon the `bind` expression.
-    fn traverse(bind: TokenStream) -> TokenStream;
+    /// A (`noop`) traversal of this trait upon the `bind` expression.
+    fn traverse(bind: TokenStream, noop: bool) -> TokenStream;
 
     /// A `match` arm for `variant`, where `f` generates the tokens for each binding.
     fn arm(
         variant: &synstructure::VariantInfo<'_>,
-        f: impl FnMut(&synstructure::BindingInfo<'_>) -> TokenStream,
+        f: impl FnMut(&synstructure::BindingInfo<'_>) -> Result<TokenStream, Error>,
     ) -> TokenStream;
 
     /// The body of an implementation given the `interner`, `traverser` and match expression `body`.
@@ -91,15 +173,19 @@ impl Traversable for Foldable {
     fn supertraits(interner: &Interner<'_>) -> TokenStream {
         Visitable::traversable(interner)
     }
-    fn traverse(bind: TokenStream) -> TokenStream {
-        quote! { ::rustc_middle::ty::noop_traversal_if_boring!(#bind.try_fold_with(folder))? }
+    fn traverse(bind: TokenStream, noop: bool) -> TokenStream {
+        if noop {
+            bind
+        } else {
+            quote! { ::rustc_middle::ty::noop_traversal_if_boring!(#bind.try_fold_with(folder))? }
+        }
     }
     fn arm(
         variant: &synstructure::VariantInfo<'_>,
-        mut f: impl FnMut(&synstructure::BindingInfo<'_>) -> TokenStream,
+        mut f: impl FnMut(&synstructure::BindingInfo<'_>) -> Result<TokenStream, Error>,
     ) -> TokenStream {
         let bindings = variant.bindings();
-        variant.construct(|_, index| f(&bindings[index]))
+        variant.construct(|_, index| f(&bindings[index]).unwrap_or_else(Error::into_compile_error))
     }
     fn impl_body(
         interner: Interner<'_>,
@@ -124,14 +210,23 @@ impl Traversable for Visitable {
     fn supertraits(_: &Interner<'_>) -> TokenStream {
         quote! { ::core::clone::Clone + ::core::fmt::Debug }
     }
-    fn traverse(bind: TokenStream) -> TokenStream {
-        quote! { ::rustc_middle::ty::noop_traversal_if_boring!(#bind.visit_with(visitor))?; }
+    fn traverse(bind: TokenStream, noop: bool) -> TokenStream {
+        if noop {
+            quote! {}
+        } else {
+            quote! { ::rustc_middle::ty::noop_traversal_if_boring!(#bind.visit_with(visitor))?; }
+        }
     }
     fn arm(
         variant: &synstructure::VariantInfo<'_>,
-        f: impl FnMut(&synstructure::BindingInfo<'_>) -> TokenStream,
+        f: impl FnMut(&synstructure::BindingInfo<'_>) -> Result<TokenStream, Error>,
     ) -> TokenStream {
-        variant.bindings().iter().map(f).collect()
+        variant
+            .bindings()
+            .iter()
+            .map(f)
+            .collect::<Result<_, _>>()
+            .unwrap_or_else(Error::into_compile_error)
     }
     fn impl_body(
         interner: Interner<'_>,
@@ -158,12 +253,14 @@ impl Interner<'_> {
         referenced_ty_params: &[&Ident],
         fields: impl IntoIterator<Item = &'a Field>,
     ) -> Type {
+        use visit::Visit;
+
         struct Visitor<'a> {
             interner: &'a Lifetime,
             contains_interner: bool,
         }
 
-        impl visit::Visit<'_> for Visitor<'_> {
+        impl Visit<'_> for Visitor<'_> {
             fn visit_lifetime(&mut self, i: &Lifetime) {
                 if i == self.interner {
                     self.contains_interner = true;
@@ -180,7 +277,7 @@ impl Interner<'_> {
                 Some(interner)
                     if fields.into_iter().any(|field| {
                         let mut visitor = Visitor { interner, contains_interner: false };
-                        visit::visit_type(&mut visitor, &field.ty);
+                        visitor.visit_type(&field.ty);
                         visitor.contains_interner
                     }) =>
                 {
@@ -194,7 +291,11 @@ impl Interner<'_> {
 
 pub fn traversable_derive<T: Traversable>(
     mut structure: synstructure::Structure<'_>,
-) -> TokenStream {
+) -> Result<TokenStream, Error> {
+    use WhenToSkip::*;
+
+    let skip_traversal = quote! { ::rustc_middle::ty::BoringTraversable };
+
     let ast = structure.ast();
 
     let interner = Interner::resolve(&ast.generics);
@@ -205,41 +306,88 @@ pub fn traversable_derive<T: Traversable>(
     structure.add_bounds(synstructure::AddBounds::None);
     structure.bind_with(|_| synstructure::BindStyle::Move);
 
-    if interner.0.is_none() {
+    let not_generic = if interner.0.is_none() {
         structure.add_impl_generic(parse_quote! { 'tcx });
-    }
+        Boring
+    } else {
+        NotGeneric
+    };
 
     // If our derived implementation will be generic over the traversable type, then we must
     // constrain it to only those generic combinations that satisfy the traversable trait's
     // supertraits.
-    if ast.generics.type_params().next().is_some() {
+    let ty = if ast.generics.type_params().next().is_some() {
         let supertraits = T::supertraits(&interner);
         structure.add_where_predicate(parse_quote! { Self: #supertraits });
-    }
+        Generic
+    } else {
+        not_generic
+    };
 
-    // We add predicates to each generic field type, rather than to our generic type parameters.
-    // This results in a "perfect derive", but it can result in trait solver cycles if any type
-    // parameters are involved in recursive type definitions; fortunately that is not the case (yet).
-    let mut predicates = HashSet::new();
-    let arms = structure.each_variant(|variant| {
-        let variant_ty = interner.type_of(&variant.referenced_ty_params(), variant.ast().fields);
-        T::arm(variant, |bind| {
-            if variant_ty == Generic {
+    let when_to_skip = WhenToSkip::find::<true>(&ast.attrs, ty)?;
+    let body = if when_to_skip.is_skipped() {
+        if let Always(_) = when_to_skip {
+            structure.add_where_predicate(parse_quote! { Self: #skip_traversal });
+        }
+        T::traverse(quote! { self }, true)
+    } 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
+        // into wrapping types, but it can result in trait solver cycles if any type parameters are involved
+        // in recursive type definitions; fortunately that is not the case (yet).
+        let mut predicates = HashMap::<_, (_, _)>::new();
+
+        let arms = structure.each_variant(|variant| {
+            let variant_ty = interner.type_of(&variant.referenced_ty_params(), variant.ast().fields);
+            let skipped_variant = match WhenToSkip::find::<false>(variant.ast().attrs, variant_ty) {
+                Ok(skip) => skip,
+                Err(err) => return err.into_compile_error(),
+            };
+            T::arm(variant, |bind| {
                 let ast = bind.ast();
-                let field_ty = interner.type_of(&bind.referenced_ty_params(), [ast]);
-                if field_ty == Generic {
-                    predicates.insert(ast.ty.clone());
-                }
-            }
-            T::traverse(bind.into_token_stream())
-        })
-    });
-    // the order in which `where` predicates appear in rust source is irrelevant
-    #[allow(rustc::potential_query_instability)]
-    for ty in predicates {
-        structure.add_where_predicate(parse_quote! { #ty: #traversable });
-    }
-    let body = quote! { match self { #arms } };
+                let is_skipped = variant_ty != Type::Boring && {
+                    let field_ty = interner.type_of(&bind.referenced_ty_params(), [ast]);
+                    field_ty != Type::Boring && {
+                        let skipped_field = if skipped_variant.is_skipped() {
+                            skipped_variant
+                        } else {
+                            WhenToSkip::find::<false>(&ast.attrs, field_ty)?
+                        };
+
+                        match predicates.entry(ast.ty.clone()) {
+                            Entry::Occupied(existing) => match (&mut existing.into_mut().0, skipped_field) {
+                                (Never, Never) | (Never, Forced) | (Forced, Forced) | (Always(_), Always(_)) => (),
+                                (existing @ Forced, Never) => *existing = Never,
+                                (&mut Always(span), _) | (_, Always(span)) => return Err(Error::new(span, format!("\
+                                    This annotation only makes sense if all fields of type `{0}` are annotated identically.\n\
+                                    In particular, the derived impl will only be applicable when `{0}: BoringTraversable` and therefore all traversals of `{0}` will be no-ops;\n\
+                                    accordingly it makes no sense for other fields of type `{0}` to omit `#[skip_traversal]` or to include `despite_potential_miscompilation_because`.\
+                                ", ast.ty.to_token_stream()))),
+                            },
+                            Entry::Vacant(entry) => { entry.insert((skipped_field, bind.referenced_ty_params().is_empty())); }
+                        }
+
+                        skipped_field.is_skipped()
+                    }
+                };
+
+                Ok(T::traverse(bind.into_token_stream(), is_skipped))
+            })
+        });
+
+        // the order in which `where` predicates appear in rust source is irrelevant
+        #[allow(rustc::potential_query_instability)]
+        for (ty, (when_to_skip, ignore)) in predicates {
+            let bound = match when_to_skip {
+                Always(_) => &skip_traversal,
+                // we only need to add traversable predicate for generic types
+                Never if !ignore => &traversable,
+                _ => continue,
+            };
+            structure.add_where_predicate(parse_quote! { #ty: #bound });
+        }
+        quote! { match self { #arms } }
+    };
 
-    structure.bound_impl(traversable, T::impl_body(interner, traverser, body))
+    Ok(structure.bound_impl(traversable, T::impl_body(interner, traverser, body)))
 }
diff --git a/compiler/rustc_macros/src/traversable/tests.rs b/compiler/rustc_macros/src/traversable/tests.rs
