feat(stackable-versioned): Add conditional allow attr generation (#876)

* feat(stackable-versioned): Add conditional allow attr generation * feat: Add allow attr to enum, carry over deprecated attr
stackabletech · Sep 26, 2024 · c3c6291 · c3c6291
1 parent 7ba94b5
commit c3c6291
Show file tree

Hide file tree

Showing 10 changed files with 188 additions and 44 deletions.
diff --git a/crates/stackable-versioned-macros/src/codegen/chain.rs b/crates/stackable-versioned-macros/src/codegen/chain.rs
@@ -4,8 +4,23 @@ pub(crate) trait Neighbors<K, V>
 where
     K: Ord + Eq,
 {
+    /// Returns the values of keys which are neighbors of `key`.
+    ///
+    /// Given a map which contains the following keys: 1, 3, 5. Calling this
+    /// function with these keys, results in the following return values:
+    ///
+    /// - Key **0**: `(None, Some(1))`
+    /// - Key **2**: `(Some(1), Some(3))`
+    /// - Key **4**: `(Some(3), Some(5))`
+    /// - Key **6**: `(Some(5), None)`
     fn get_neighbors(&self, key: &K) -> (Option<&V>, Option<&V>);
 
+    /// Returns whether the function `f` returns true if applied to the value
+    /// identified by `key`.
+    fn value_is<F>(&self, key: &K, f: F) -> bool
+    where
+        F: Fn(&V) -> bool;
+
     fn lo_bound(&self, bound: Bound<&K>) -> Option<(&K, &V)>;
     fn up_bound(&self, bound: Bound<&K>) -> Option<(&K, &V)>;
 }
@@ -14,15 +29,6 @@ impl<K, V> Neighbors<K, V> for BTreeMap<K, V>
 where
     K: Ord + Eq,
 {
-    /// Returns the values of keys which are neighbors of `key`.
-    ///
-    /// Given a map which contains the following keys: 1, 3, 5. Calling this
-    /// function with these keys, results in the following return values:
-    ///
-    /// - Key **0**: `(None, Some(1))`
-    /// - Key **2**: `(Some(1), Some(3))`
-    /// - Key **4**: `(Some(3), Some(5))`
-    /// - Key **6**: `(Some(5), None)`
     fn get_neighbors(&self, key: &K) -> (Option<&V>, Option<&V>) {
         // NOTE (@Techassi): These functions might get added to the standard
         // library at some point. If that's the case, we can use the ones
@@ -51,6 +57,13 @@ where
         }
     }
 
+    fn value_is<F>(&self, key: &K, f: F) -> bool
+    where
+        F: Fn(&V) -> bool,
+    {
+        self.get(key).map_or(false, f)
+    }
+
     fn lo_bound(&self, bound: Bound<&K>) -> Option<(&K, &V)> {
         self.range((Bound::Unbounded, bound)).next_back()
     }

diff --git a/crates/stackable-versioned-macros/src/codegen/common/item.rs b/crates/stackable-versioned-macros/src/codegen/common/item.rs
@@ -303,58 +303,79 @@ where
                         } => chain.insert(
                             version.inner,
                             ItemStatus::NoChange {
+                                previously_deprecated: false,
                                 ident: from_ident.clone(),
                                 ty: from_type.clone(),
                             },
                         ),
                         ItemStatus::Deprecation { previous_ident, .. } => chain.insert(
                             version.inner,
                             ItemStatus::NoChange {
+                                previously_deprecated: false,
                                 ident: previous_ident.clone(),
                                 ty: self.inner.ty(),
                             },
                         ),
-                        ItemStatus::NoChange { ident, ty } => chain.insert(
+                        ItemStatus::NoChange {
+                            previously_deprecated,
+                            ident,
+                            ty,
+                        } => chain.insert(
                             version.inner,
                             ItemStatus::NoChange {
+                                previously_deprecated: *previously_deprecated,
                                 ident: ident.clone(),
                                 ty: ty.clone(),
                             },
                         ),
                         ItemStatus::NotPresent => unreachable!(),
                     },
                     (Some(status), None) => {
-                        let (ident, ty) = match status {
-                            ItemStatus::Addition { ident, ty, .. } => (ident, ty),
+                        let (ident, ty, previously_deprecated) = match status {
+                            ItemStatus::Addition { ident, ty, .. } => (ident, ty, false),
                             ItemStatus::Change {
                                 to_ident, to_type, ..
-                            } => (to_ident, to_type),
-                            ItemStatus::Deprecation { ident, .. } => (ident, &self.inner.ty()),
-                            ItemStatus::NoChange { ident, ty } => (ident, ty),
+                            } => (to_ident, to_type, false),
+                            ItemStatus::Deprecation { ident, .. } => {
+                                (ident, &self.inner.ty(), true)
+                            }
+                            ItemStatus::NoChange {
+                                previously_deprecated,
+                                ident,
+                                ty,
+                                ..
+                            } => (ident, ty, *previously_deprecated),
                             ItemStatus::NotPresent => unreachable!(),
                         };
 
                         chain.insert(
                             version.inner,
                             ItemStatus::NoChange {
+                                previously_deprecated,
                                 ident: ident.clone(),
                                 ty: ty.clone(),
                             },
                         )
                     }
                     (Some(status), Some(_)) => {
-                        let (ident, ty) = match status {
-                            ItemStatus::Addition { ident, ty, .. } => (ident, ty),
+                        let (ident, ty, previously_deprecated) = match status {
+                            ItemStatus::Addition { ident, ty, .. } => (ident, ty, false),
                             ItemStatus::Change {
                                 to_ident, to_type, ..
-                            } => (to_ident, to_type),
-                            ItemStatus::NoChange { ident, ty, .. } => (ident, ty),
+                            } => (to_ident, to_type, false),
+                            ItemStatus::NoChange {
+                                previously_deprecated,
+                                ident,
+                                ty,
+                                ..
+                            } => (ident, ty, *previously_deprecated),
                             _ => unreachable!(),
                         };
 
                         chain.insert(
                             version.inner,
                             ItemStatus::NoChange {
+                                previously_deprecated,
                                 ident: ident.clone(),
                                 ty: ty.clone(),
                             },
@@ -398,6 +419,7 @@ pub(crate) enum ItemStatus {
         ident: Ident,
     },
     NoChange {
+        previously_deprecated: bool,
         ident: Ident,
         ty: Type,
     },

diff --git a/crates/stackable-versioned-macros/src/codegen/venum/mod.rs b/crates/stackable-versioned-macros/src/codegen/venum/mod.rs
@@ -8,7 +8,10 @@ use syn::{DataEnum, Error};
 use crate::{
     attrs::common::ContainerAttributes,
     codegen::{
-        common::{Container, ContainerInput, ContainerVersion, Item, VersionedContainer},
+        chain::Neighbors,
+        common::{
+            Container, ContainerInput, ContainerVersion, Item, ItemStatus, VersionedContainer,
+        },
         venum::variant::VersionedVariant,
     },
 };
@@ -193,11 +196,18 @@ impl VersionedEnum {
                 ))
             }
 
-            // TODO (@Techassi): Be a little bit more clever about when to include
-            // the #[allow(deprecated)] attribute.
+            // Include allow(deprecated) only when this or the next version is
+            // deprecated. Also include it, when a variant in this or the next
+            // version is deprecated.
+            let allow_attribute = (version.deprecated
+                || next_version.deprecated
+                || self.is_any_variant_deprecated(version)
+                || self.is_any_variant_deprecated(next_version))
+            .then_some(quote! { #[allow(deprecated)] });
+
             return quote! {
                 #[automatically_derived]
-                #[allow(deprecated)]
+                #allow_attribute
                 impl From<#module_name::#enum_ident> for #next_module_name::#enum_ident {
                     fn from(#from_ident: #module_name::#enum_ident) -> Self {
                         match #from_ident {
@@ -210,4 +220,28 @@ impl VersionedEnum {
 
         quote! {}
     }
+
+    /// Returns whether any field is deprecated in the provided
+    /// [`ContainerVersion`].
+    fn is_any_variant_deprecated(&self, version: &ContainerVersion) -> bool {
+        // First, iterate over all fields. Any will return true if any of the
+        // function invocations return true. If a field doesn't have a chain,
+        // we can safely default to false (unversioned fields cannot be
+        // deprecated). Then we retrieve the status of the field and ensure it
+        // is deprecated.
+        self.items.iter().any(|f| {
+            f.chain.as_ref().map_or(false, |c| {
+                c.value_is(&version.inner, |a| {
+                    matches!(
+                        a,
+                        ItemStatus::Deprecation { .. }
+                            | ItemStatus::NoChange {
+                                previously_deprecated: true,
+                                ..
+                            }
+                    )
+                })
+            })
+        })
+    }
 }
diff --git a/crates/stackable-versioned-macros/src/codegen/venum/variant.rs b/crates/stackable-versioned-macros/src/codegen/venum/variant.rs
@@ -142,10 +142,21 @@ impl VersionedVariant {
                         #ident,
                     })
                 }
-                ItemStatus::NoChange { ident, .. } => Some(quote! {
-                    #(#original_attributes)*
-                    #ident,
-                }),
+                ItemStatus::NoChange {
+                    previously_deprecated,
+                    ident,
+                    ..
+                } => {
+                    // TODO (@Techassi): Also carry along the deprecation
+                    // note.
+                    let deprecated_attr = previously_deprecated.then(|| quote! {#[deprecated]});
+
+                    Some(quote! {
+                        #(#original_attributes)*
+                        #deprecated_attr
+                        #ident,
+                    })
+                }
                 ItemStatus::NotPresent => None,
             },
             None => {

diff --git a/crates/stackable-versioned-macros/src/codegen/vstruct/field.rs b/crates/stackable-versioned-macros/src/codegen/vstruct/field.rs
@@ -157,10 +157,22 @@ impl VersionedField {
                         })
                     }
                     ItemStatus::NotPresent => None,
-                    ItemStatus::NoChange { ident, ty } => Some(quote! {
-                        #(#original_attributes)*
-                        pub #ident: #ty,
-                    }),
+                    ItemStatus::NoChange {
+                        previously_deprecated,
+                        ident,
+                        ty,
+                        ..
+                    } => {
+                        // TODO (@Techassi): Also carry along the deprecation
+                        // note.
+                        let deprecated_attr = previously_deprecated.then(|| quote! {#[deprecated]});
+
+                        Some(quote! {
+                            #(#original_attributes)*
+                            #deprecated_attr
+                            pub #ident: #ty,
+                        })
+                    }
                 }
             }
             None => {

diff --git a/crates/stackable-versioned-macros/src/codegen/vstruct/mod.rs b/crates/stackable-versioned-macros/src/codegen/vstruct/mod.rs
@@ -8,8 +8,10 @@ use syn::{parse_quote, DataStruct, Error, Ident};
 use crate::{
     attrs::common::ContainerAttributes,
     codegen::{
+        chain::Neighbors,
         common::{
-            Container, ContainerInput, ContainerVersion, Item, VersionExt, VersionedContainer,
+            Container, ContainerInput, ContainerVersion, Item, ItemStatus, VersionExt,
+            VersionedContainer,
         },
         vstruct::field::VersionedField,
     },
@@ -241,11 +243,18 @@ impl VersionedStruct {
 
             let fields = self.generate_from_fields(version, next_version, from_ident);
 
-            // TODO (@Techassi): Be a little bit more clever about when to include
-            // the #[allow(deprecated)] attribute.
+            // Include allow(deprecated) only when this or the next version is
+            // deprecated. Also include it, when a field in this or the next
+            // version is deprecated.
+            let allow_attribute = (version.deprecated
+                || next_version.deprecated
+                || self.is_any_field_deprecated(version)
+                || self.is_any_field_deprecated(next_version))
+            .then_some(quote! { #[allow(deprecated)] });
+
             return Some(quote! {
                 #[automatically_derived]
-                #[allow(deprecated)]
+                #allow_attribute
                 impl From<#module_name::#struct_ident> for #next_module_name::#struct_ident {
                     fn from(#from_ident: #module_name::#struct_ident) -> Self {
                         Self {
@@ -275,6 +284,30 @@ impl VersionedStruct {
 
         token_stream
     }
+
+    /// Returns whether any field is deprecated in the provided
+    /// [`ContainerVersion`].
+    fn is_any_field_deprecated(&self, version: &ContainerVersion) -> bool {
+        // First, iterate over all fields. Any will return true if any of the
+        // function invocations return true. If a field doesn't have a chain,
+        // we can safely default to false (unversioned fields cannot be
+        // deprecated). Then we retrieve the status of the field and ensure it
+        // is deprecated.
+        self.items.iter().any(|f| {
+            f.chain.as_ref().map_or(false, |c| {
+                c.value_is(&version.inner, |a| {
+                    matches!(
+                        a,
+                        ItemStatus::Deprecation { .. }
+                            | ItemStatus::NoChange {
+                                previously_deprecated: true,
+                                ..
+                            }
+                    )
+                })
+            })
+        })
+    }
 }
 
 // Kubernetes specific code generation

diff --git a/crates/stackable-versioned-macros/src/lib.rs b/crates/stackable-versioned-macros/src/lib.rs
@@ -456,7 +456,7 @@ pub struct FooSpec {
 }
 
 # fn main() {
-let merged_crd = Foo::merged_crd("v1").unwrap();
+let merged_crd = Foo::merged_crd(Foo::V1).unwrap();
 println!("{}", serde_yaml::to_string(&merged_crd).unwrap());
 # }
 ```

diff --git a/crates/stackable-versioned-macros/tests/default/pass/deprecate_enum.rs b/crates/stackable-versioned-macros/tests/default/pass/deprecate_enum.rs
@@ -0,0 +1,16 @@
+use stackable_versioned_macros::versioned;
+
+fn main() {
+    #[versioned(
+        version(name = "v1alpha1"),
+        version(name = "v1beta1"),
+        version(name = "v1"),
+        version(name = "v2"),
+        version(name = "v3")
+    )]
+    enum Foo {
+        #[versioned(deprecated(since = "v1"))]
+        DeprecatedBar,
+        Baz,
+    }
+}
diff --git a/...ed-macros/tests/default/pass/deprecate.rs → ...os/tests/default/pass/deprecate_struct.rs b/...ed-macros/tests/default/pass/deprecate.rs → ...os/tests/default/pass/deprecate_struct.rs
@@ -4,10 +4,12 @@ fn main() {
     #[versioned(
         version(name = "v1alpha1"),
         version(name = "v1beta1"),
-        version(name = "v1")
+        version(name = "v1"),
+        version(name = "v2"),
+        version(name = "v3")
     )]
     struct Foo {
-        #[versioned(deprecated(since = "v1beta1", note = "gone"))]
+        #[versioned(deprecated(since = "v1", note = "gone"))]
         deprecated_bar: usize,
         baz: bool,
     }

diff --git a/crates/stackable-versioned-macros/tests/trybuild.rs b/crates/stackable-versioned-macros/tests/trybuild.rs
@@ -17,13 +17,14 @@
 #[allow(dead_code)]
 mod default {
     // mod pass {
-    //     mod attributes_enum;
-    //     mod attributes_struct;
-    //     mod basic;
+    //     // mod attributes_enum;
+    //     // mod attributes_struct;
+    //     // mod basic;
 
-    //     mod deprecate;
-    //     mod rename;
-    //     mod skip_from_version;
+    //     // mod deprecate_enum;
+    //     // mod deprecate_struct;
+    //     // mod rename;
+    //     // mod skip_from_version;
     // }
 
     // mod fail {