From cbd3e7be18e3dd43e29b15ac378bf762136e4589 Mon Sep 17 00:00:00 2001 From: Techassi Date: Wed, 21 Aug 2024 14:33:30 +0200 Subject: [PATCH 1/9] chore: Update changelog --- crates/stackable-versioned/CHANGELOG.md | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/crates/stackable-versioned/CHANGELOG.md b/crates/stackable-versioned/CHANGELOG.md index 764e01445..041003c9a 100644 --- a/crates/stackable-versioned/CHANGELOG.md +++ b/crates/stackable-versioned/CHANGELOG.md @@ -4,6 +4,13 @@ All notable changes to this project will be documented in this file. ## [Unreleased] +### Added + +- Add `changed()` action and support for the `ty` parameter in the `renamed()` + action ([#xxx]). + +[#xxx]: https://github.com/stackabletech/operator-rs/pull/xxx + ### Fixed - Report variant rename validation error at the correct span and trim underscores From cb395c67eb72cc254fa2e0863a944d1e75b19d48 Mon Sep 17 00:00:00 2001 From: Techassi Date: Wed, 21 Aug 2024 16:26:51 +0200 Subject: [PATCH 2/9] chore: Update PR link in changelog --- crates/stackable-versioned/CHANGELOG.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/stackable-versioned/CHANGELOG.md b/crates/stackable-versioned/CHANGELOG.md index 041003c9a..49ef8d3cf 100644 --- a/crates/stackable-versioned/CHANGELOG.md +++ b/crates/stackable-versioned/CHANGELOG.md @@ -7,9 +7,9 @@ All notable changes to this project will be documented in this file. ### Added - Add `changed()` action and support for the `ty` parameter in the `renamed()` - action ([#xxx]). + action ([#844]). -[#xxx]: https://github.com/stackabletech/operator-rs/pull/xxx +[#844]: https://github.com/stackabletech/operator-rs/pull/844 ### Fixed From b3f2cf0c38f937b6249dde9aa43ac2a8a9e8497a Mon Sep 17 00:00:00 2001 From: Techassi Date: Fri, 30 Aug 2024 13:50:52 +0200 Subject: [PATCH 3/9] refactor: Rename renamed action to changed --- .../src/attrs/common/item.rs | 59 ++++++------ .../src/attrs/field.rs | 2 +- .../src/attrs/variant.rs | 14 +-- .../src/codegen/chain.rs | 2 +- .../src/codegen/common/item.rs | 96 ++++++++++--------- .../src/codegen/venum/variant.rs | 10 +- .../src/codegen/vstruct/field.rs | 10 +- .../tests/good/attributes_enum.rs | 4 +- .../tests/good/attributes_struct.rs | 4 +- .../tests/good/basic.rs | 5 +- .../tests/good/rename.rs | 2 +- 11 files changed, 113 insertions(+), 95 deletions(-) diff --git a/crates/stackable-versioned-macros/src/attrs/common/item.rs b/crates/stackable-versioned-macros/src/attrs/common/item.rs index 744549a0e..4c2e39b32 100644 --- a/crates/stackable-versioned-macros/src/attrs/common/item.rs +++ b/crates/stackable-versioned-macros/src/attrs/common/item.rs @@ -1,7 +1,7 @@ use darling::{util::SpannedValue, Error, FromMeta}; use k8s_version::Version; use proc_macro2::Span; -use syn::{spanned::Spanned, Attribute, Ident, Path}; +use syn::{spanned::Spanned, Attribute, Ident, Path, Type}; use crate::{ attrs::common::ContainerAttributes, @@ -55,14 +55,14 @@ where } } - for rename in &*self.common_attributes().renames { + for change in &*self.common_attributes().changes { if !container_attrs .versions .iter() - .any(|v| v.name == *rename.since) + .any(|v| v.name == *change.since) { errors.push( - Error::custom("variant action `renamed` uses version which was not declared via #[versioned(version)]") + Error::custom("variant action `changed` uses version which was not declared via #[versioned(version)]") .with_span(item) ); } @@ -104,8 +104,8 @@ pub(crate) enum ItemType { /// ### Shared Item Rules /// /// - An item can only ever be added once at most. An item not marked as 'added' -/// is part of the container in every version until renamed or deprecated. -/// - An item can be renamed many times. That's why renames are stored in a +/// is part of the container in every version until changed or deprecated. +/// - An item can be changed many times. That's why changes are stored in a /// [`Vec`]. /// - An item can only be deprecated once. A field or variant not marked as /// 'deprecated' will be included up until the latest version. @@ -115,10 +115,10 @@ pub(crate) struct ItemAttributes { /// only be present at most once. pub(crate) added: Option, - /// This parses the `renamed` attribute on items (fields or variants). It + /// This parses the `changed` attribute on items (fields or variants). It /// can be present 0..n times. - #[darling(multiple, rename = "renamed")] - pub(crate) renames: Vec, + #[darling(multiple, rename = "changed")] + pub(crate) changes: Vec, /// This parses the `deprecated` attribute on items (fields or variants). It /// can only be present at most once. @@ -175,34 +175,34 @@ impl ItemAttributes { /// cannot be marked as added in a particular version and then marked as /// deprecated immediately after. Fields and variants must be included for /// at least one version before being marked deprecated. - /// - `added` and `renamed` using the same version: The same reasoning from + /// - `added` and `changed` using the same version: The same reasoning from /// above applies here as well. Fields and variants must be included for - /// at least one version before being renamed. - /// - `renamed` and `deprecated` using the same version: Again, the same + /// at least one version before being changed. + /// - `changed` and `deprecated` using the same version: Again, the same /// rules from above apply here as well. fn validate_action_combinations( &self, item_ident: &Ident, item_type: &ItemType, ) -> Result<(), Error> { - match (&self.added, &self.renames, &self.deprecated) { + match (&self.added, &self.changes, &self.deprecated) { (Some(added), _, Some(deprecated)) if *added.since == *deprecated.since => { Err(Error::custom(format!( "{item_type} cannot be marked as `added` and `deprecated` in the same version" )) .with_span(item_ident)) } - (Some(added), renamed, _) if renamed.iter().any(|r| *r.since == *added.since) => { + (Some(added), changed, _) if changed.iter().any(|r| *r.since == *added.since) => { Err(Error::custom(format!( - "{item_type} cannot be marked as `added` and `renamed` in the same version" + "{item_type} cannot be marked as `added` and `changed` in the same version" )) .with_span(item_ident)) } - (_, renamed, Some(deprecated)) - if renamed.iter().any(|r| *r.since == *deprecated.since) => + (_, changed, Some(deprecated)) + if changed.iter().any(|r| *r.since == *deprecated.since) => { Err(Error::custom( - format!("{item_type} cannot be marked as `deprecated` and `renamed` in the same version"), + format!("{item_type} cannot be marked as `deprecated` and `changed` in the same version"), ) .with_span(item_ident)) } @@ -220,7 +220,7 @@ impl ItemAttributes { /// ensures that these versions are chronologically sound, that means, /// that the version of the deprecated action must be greater than the /// version of the added action. - /// - All `renamed` actions must use a greater version than `added` but a + /// - All `changed` actions must use a greater version than `added` but a /// lesser version than `deprecated`. fn validate_action_order(&self, item_ident: &Ident, item_type: &ItemType) -> Result<(), Error> { let added_version = self.added.as_ref().map(|a| *a.since); @@ -238,14 +238,14 @@ impl ItemAttributes { } } - // Now, iterate over all renames and ensure that their versions are + // Now, iterate over all changes and ensure that their versions are // between the added and deprecated version. - if !self.renames.iter().all(|r| { + if !self.changes.iter().all(|r| { added_version.map_or(true, |a| a < *r.since) && deprecated_version.map_or(true, |d| d > *r.since) }) { return Err(Error::custom( - "all renames must use versions higher than `added` and lower than `deprecated`", + "all changes must use versions higher than `added` and lower than `deprecated`", ) .with_span(item_ident)); } @@ -320,19 +320,24 @@ pub(crate) struct AddedAttributes { fn default_default_fn() -> SpannedValue { SpannedValue::new( - syn::parse_str("std::default::Default::default").expect("internal error: path must parse"), + syn::parse_str("::std::default::Default::default") + .expect("internal error: path must parse"), Span::call_site(), ) } -/// For the renamed() action +/// For the changed() action /// /// Example usage: -/// - `renamed(since = "...", from = "...")` +/// - `changed(since = "...", from_name = "...")` +/// - `changed(since = "...", from_name = "..." from_type="...")` #[derive(Clone, Debug, FromMeta)] -pub(crate) struct RenamedAttributes { +pub(crate) struct ChangedAttributes { pub(crate) since: SpannedValue, - pub(crate) from: SpannedValue, + pub(crate) from_name: Option>, + + #[allow(dead_code)] + pub(crate) from_type: Option>, } /// For the deprecated() action diff --git a/crates/stackable-versioned-macros/src/attrs/field.rs b/crates/stackable-versioned-macros/src/attrs/field.rs index e2816d927..d5c9aa9ff 100644 --- a/crates/stackable-versioned-macros/src/attrs/field.rs +++ b/crates/stackable-versioned-macros/src/attrs/field.rs @@ -50,8 +50,8 @@ impl FieldAttributes { .ident .as_ref() .expect("internal error: field must have an ident"); - self.common.validate(ident, &ItemType::Field, &self.attrs)?; + self.common.validate(ident, &ItemType::Field, &self.attrs)?; Ok(self) } } diff --git a/crates/stackable-versioned-macros/src/attrs/variant.rs b/crates/stackable-versioned-macros/src/attrs/variant.rs index 7f1e69657..bcdde256a 100644 --- a/crates/stackable-versioned-macros/src/attrs/variant.rs +++ b/crates/stackable-versioned-macros/src/attrs/variant.rs @@ -55,12 +55,14 @@ impl VariantAttributes { ); // Validate names of renames - for rename in &self.common.renames { - if !rename.from.is_case(Case::Pascal) { - errors.push( - Error::custom("renamed variant must use PascalCase") - .with_span(&rename.from.span()), - ) + for change in &self.common.changes { + if let Some(from_name) = &change.from_name { + if !from_name.is_case(Case::Pascal) { + errors.push( + Error::custom("renamed variant must use PascalCase") + .with_span(&from_name.span()), + ) + } } } diff --git a/crates/stackable-versioned-macros/src/codegen/chain.rs b/crates/stackable-versioned-macros/src/codegen/chain.rs index 097214b69..7c6913dc5 100644 --- a/crates/stackable-versioned-macros/src/codegen/chain.rs +++ b/crates/stackable-versioned-macros/src/codegen/chain.rs @@ -91,7 +91,7 @@ mod test { #[case(2, (Some(&"test1"), Some(&"test3")))] #[case(3, (Some(&"test1"), None))] #[case(4, (Some(&"test3"), None))] - fn test(#[case] key: i32, #[case] expected: (Option<&&str>, Option<&&str>)) { + fn neighbors(#[case] key: i32, #[case] expected: (Option<&&str>, Option<&&str>)) { let map = BTreeMap::from([(1, "test1"), (3, "test3")]); let neigbors = map.get_neighbors(&key); diff --git a/crates/stackable-versioned-macros/src/codegen/common/item.rs b/crates/stackable-versioned-macros/src/codegen/common/item.rs index 6baf1ff34..7fcf58a40 100644 --- a/crates/stackable-versioned-macros/src/codegen/common/item.rs +++ b/crates/stackable-versioned-macros/src/codegen/common/item.rs @@ -89,9 +89,9 @@ where A: for<'i> TryFrom<&'i I> + Attributes, I: Named + Spanned, { + pub(crate) original_attributes: Vec, pub(crate) chain: Option, pub(crate) inner: I, - pub(crate) original_attributes: Vec, _marker: PhantomData, } @@ -125,34 +125,39 @@ where // Deprecating an item is always the last state an item can end up in. // For items which are not deprecated, the last change is either the - // latest rename or addition, which is handled below. The ident of the + // latest change or addition, which is handled below. The ident of the // deprecated item is guaranteed to include the 'deprecated_' or // 'DEPRECATED_' prefix. The ident can thus be used as is. if let Some(deprecated) = common_attributes.deprecated { let deprecated_ident = item.ident(); - // When the item is deprecated, any rename which occurred beforehand + // When the item is deprecated, any change which occurred beforehand // requires access to the item ident to infer the item ident for - // the latest rename. + // the latest change. let mut ident = item.cleaned_ident(); let mut actions = BTreeMap::new(); actions.insert( *deprecated.since, - ItemStatus::Deprecated { + ItemStatus::Deprecation { previous_ident: ident.clone(), ident: deprecated_ident.clone(), note: deprecated.note.to_string(), }, ); - for rename in common_attributes.renames.iter().rev() { - let from = format_ident!("{from}", from = *rename.from); + for change in common_attributes.changes.iter().rev() { + let from = if let Some(from) = change.from_name.as_deref() { + format_ident!("{from}") + } else { + ident.clone() + }; + actions.insert( - *rename.since, - ItemStatus::Renamed { - from: from.clone(), - to: ident, + *change.since, + ItemStatus::Change { + from_ident: from.clone(), + to_ident: ident, }, ); ident = from; @@ -163,7 +168,7 @@ where if let Some(added) = common_attributes.added { actions.insert( *added.since, - ItemStatus::Added { + ItemStatus::Addition { default_fn: added.default_fn.deref().clone(), ident, }, @@ -173,20 +178,25 @@ where Ok(Self { _marker: PhantomData, chain: Some(actions), - inner: item, original_attributes, + inner: item, }) - } else if !common_attributes.renames.is_empty() { + } else if !common_attributes.changes.is_empty() { let mut actions = BTreeMap::new(); let mut ident = item.ident().clone(); - for rename in common_attributes.renames.iter().rev() { - let from = format_ident!("{from}", from = *rename.from); + for change in common_attributes.changes.iter().rev() { + let from = if let Some(from) = change.from_name.as_deref() { + format_ident!("{from}") + } else { + ident.clone() + }; + actions.insert( - *rename.since, - ItemStatus::Renamed { - from: from.clone(), - to: ident, + *change.since, + ItemStatus::Change { + from_ident: from.clone(), + to_ident: ident, }, ); ident = from; @@ -197,7 +207,7 @@ where if let Some(added) = common_attributes.added { actions.insert( *added.since, - ItemStatus::Added { + ItemStatus::Addition { default_fn: added.default_fn.deref().clone(), ident, }, @@ -207,8 +217,8 @@ where Ok(Self { _marker: PhantomData, chain: Some(actions), - inner: item, original_attributes, + inner: item, }) } else { if let Some(added) = common_attributes.added { @@ -216,7 +226,7 @@ where actions.insert( *added.since, - ItemStatus::Added { + ItemStatus::Addition { default_fn: added.default_fn.deref().clone(), ident: item.ident().clone(), }, @@ -225,16 +235,16 @@ where return Ok(Self { _marker: PhantomData, chain: Some(actions), - inner: item, original_attributes, + inner: item, }); } Ok(Self { _marker: PhantomData, + original_attributes, chain: None, inner: item, - original_attributes, }) } } @@ -248,13 +258,13 @@ where match chain.get_neighbors(&version.inner) { (None, Some(status)) => match status { - ItemStatus::Added { .. } => { + ItemStatus::Addition { .. } => { chain.insert(version.inner, ItemStatus::NotPresent) } - ItemStatus::Renamed { from, .. } => { - chain.insert(version.inner, ItemStatus::NoChange(from.clone())) + ItemStatus::Change { from_ident, .. } => { + chain.insert(version.inner, ItemStatus::NoChange(from_ident.clone())) } - ItemStatus::Deprecated { previous_ident, .. } => chain + ItemStatus::Deprecation { previous_ident, .. } => chain .insert(version.inner, ItemStatus::NoChange(previous_ident.clone())), ItemStatus::NoChange(ident) => { chain.insert(version.inner, ItemStatus::NoChange(ident.clone())) @@ -263,9 +273,9 @@ where }, (Some(status), None) => { let ident = match status { - ItemStatus::Added { ident, .. } => ident, - ItemStatus::Renamed { to, .. } => to, - ItemStatus::Deprecated { ident, .. } => ident, + ItemStatus::Addition { ident, .. } => ident, + ItemStatus::Change { to_ident, .. } => to_ident, + ItemStatus::Deprecation { ident, .. } => ident, ItemStatus::NoChange(ident) => ident, ItemStatus::NotPresent => unreachable!(), }; @@ -274,8 +284,8 @@ where } (Some(status), Some(_)) => { let ident = match status { - ItemStatus::Added { ident, .. } => ident, - ItemStatus::Renamed { to, .. } => to, + ItemStatus::Addition { ident, .. } => ident, + ItemStatus::Change { to_ident, .. } => to_ident, ItemStatus::NoChange(ident) => ident, _ => unreachable!(), }; @@ -299,17 +309,17 @@ where } } -#[derive(Debug)] +#[derive(Debug, PartialEq)] pub(crate) enum ItemStatus { - Added { + Addition { ident: Ident, default_fn: Path, }, - Renamed { - from: Ident, - to: Ident, + Change { + from_ident: Ident, + to_ident: Ident, }, - Deprecated { + Deprecation { previous_ident: Ident, ident: Ident, note: String, @@ -321,9 +331,9 @@ pub(crate) enum ItemStatus { impl ItemStatus { pub(crate) fn get_ident(&self) -> Option<&Ident> { match &self { - ItemStatus::Added { ident, .. } => Some(ident), - ItemStatus::Renamed { to, .. } => Some(to), - ItemStatus::Deprecated { ident, .. } => Some(ident), + ItemStatus::Addition { ident, .. } => Some(ident), + ItemStatus::Change { to_ident, .. } => Some(to_ident), + ItemStatus::Deprecation { ident, .. } => Some(ident), ItemStatus::NoChange(ident) => Some(ident), ItemStatus::NotPresent => None, } diff --git a/crates/stackable-versioned-macros/src/codegen/venum/variant.rs b/crates/stackable-versioned-macros/src/codegen/venum/variant.rs index e2b7eb6cf..2e9a76e57 100644 --- a/crates/stackable-versioned-macros/src/codegen/venum/variant.rs +++ b/crates/stackable-versioned-macros/src/codegen/venum/variant.rs @@ -102,15 +102,15 @@ impl VersionedVariant { container_version.inner ) }) { - ItemStatus::Added { ident, .. } => Some(quote! { + ItemStatus::Addition { ident, .. } => Some(quote! { #(#original_attributes)* #ident, }), - ItemStatus::Renamed { to, .. } => Some(quote! { + ItemStatus::Change { to_ident, .. } => Some(quote! { #(#original_attributes)* - #to, + #to_ident, }), - ItemStatus::Deprecated { ident, .. } => Some(quote! { + ItemStatus::Deprecation { ident, .. } => Some(quote! { #(#original_attributes)* #[deprecated] #ident, @@ -149,7 +149,7 @@ impl VersionedVariant { chain.get_expect(&version.inner), chain.get_expect(&next_version.inner), ) { - (_, ItemStatus::Added { .. }) => quote! {}, + (_, ItemStatus::Addition { .. }) => quote! {}, (old, next) => { let old_variant_ident = old .get_ident() diff --git a/crates/stackable-versioned-macros/src/codegen/vstruct/field.rs b/crates/stackable-versioned-macros/src/codegen/vstruct/field.rs index 579cb13e4..752497069 100644 --- a/crates/stackable-versioned-macros/src/codegen/vstruct/field.rs +++ b/crates/stackable-versioned-macros/src/codegen/vstruct/field.rs @@ -115,15 +115,15 @@ impl VersionedField { container_version.inner ) }) { - ItemStatus::Added { ident, .. } => Some(quote! { + ItemStatus::Addition { ident, .. } => Some(quote! { #(#original_attributes)* pub #ident: #field_type, }), - ItemStatus::Renamed { to, .. } => Some(quote! { + ItemStatus::Change { to_ident, .. } => Some(quote! { #(#original_attributes)* - pub #to: #field_type, + pub #to_ident: #field_type, }), - ItemStatus::Deprecated { + ItemStatus::Deprecation { ident: field_ident, note, .. @@ -170,7 +170,7 @@ impl VersionedField { .get(&next_version.inner) .expect("internal error: chain must contain container version"), ) { - (_, ItemStatus::Added { ident, default_fn }) => quote! { + (_, ItemStatus::Addition { ident, default_fn }) => quote! { #ident: #default_fn(), }, (old, next) => { diff --git a/crates/stackable-versioned-macros/tests/good/attributes_enum.rs b/crates/stackable-versioned-macros/tests/good/attributes_enum.rs index 578f1cc5e..ae73085dd 100644 --- a/crates/stackable-versioned-macros/tests/good/attributes_enum.rs +++ b/crates/stackable-versioned-macros/tests/good/attributes_enum.rs @@ -36,8 +36,8 @@ fn main() { Baz, /// This is will keep changing over time. - #[versioned(renamed(since = "v1beta1", from = "Qoox"))] - #[versioned(renamed(since = "v1", from = "Qaax"))] + #[versioned(changed(since = "v1beta1", from_name = "Qoox"))] + #[versioned(changed(since = "v1", from_name = "Qaax"))] Quux, } } diff --git a/crates/stackable-versioned-macros/tests/good/attributes_struct.rs b/crates/stackable-versioned-macros/tests/good/attributes_struct.rs index 7747a960f..7790d424e 100644 --- a/crates/stackable-versioned-macros/tests/good/attributes_struct.rs +++ b/crates/stackable-versioned-macros/tests/good/attributes_struct.rs @@ -31,8 +31,8 @@ fn main() { baz: String, /// This is will keep changing over time. - #[versioned(renamed(since = "v1beta1", from = "qoox"))] - #[versioned(renamed(since = "v1", from = "qaax"))] + #[versioned(changed(since = "v1beta1", from_name = "qoox"))] + #[versioned(changed(since = "v1", from_name = "qaax"))] quux: String, } diff --git a/crates/stackable-versioned-macros/tests/good/basic.rs b/crates/stackable-versioned-macros/tests/good/basic.rs index 7ba40df21..9bf3d38ce 100644 --- a/crates/stackable-versioned-macros/tests/good/basic.rs +++ b/crates/stackable-versioned-macros/tests/good/basic.rs @@ -11,10 +11,11 @@ use stackable_versioned_macros::versioned; version(name = "v2"), version(name = "v3") )] -struct Foo { +pub(crate) struct Foo { #[versioned( added(since = "v1alpha1"), - renamed(since = "v1beta1", from = "jjj"), + changed(since = "v1beta1", from_name = "jjj", from_type = "u8"), + changed(since = "v1", from_type = "u16"), deprecated(since = "v2", note = "not empty") )] /// Test diff --git a/crates/stackable-versioned-macros/tests/good/rename.rs b/crates/stackable-versioned-macros/tests/good/rename.rs index 0728f4804..dac9b7689 100644 --- a/crates/stackable-versioned-macros/tests/good/rename.rs +++ b/crates/stackable-versioned-macros/tests/good/rename.rs @@ -7,7 +7,7 @@ fn main() { version(name = "v1") )] struct Foo { - #[versioned(renamed(since = "v1beta1", from = "bat"))] + #[versioned(changed(since = "v1beta1", from_name = "bat"))] bar: usize, baz: bool, } From 0bfa8da197d78924d8ae6fbac8fad56f3a426930 Mon Sep 17 00:00:00 2001 From: Techassi Date: Fri, 30 Aug 2024 14:18:31 +0200 Subject: [PATCH 4/9] fix: Update doc test --- crates/stackable-versioned-macros/src/lib.rs | 2 +- crates/stackable-versioned/src/lib.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/stackable-versioned-macros/src/lib.rs b/crates/stackable-versioned-macros/src/lib.rs index 479eab5e2..d01ffb2ef 100644 --- a/crates/stackable-versioned-macros/src/lib.rs +++ b/crates/stackable-versioned-macros/src/lib.rs @@ -27,7 +27,7 @@ mod consts; /// /// My docs /// #[versioned( /// added(since = "v1beta1"), -/// renamed(since = "v1", from = "gau"), +/// changed(since = "v1", from_name = "gau"), /// deprecated(since = "v2", note = "not empty") /// )] /// deprecated_bar: usize, diff --git a/crates/stackable-versioned/src/lib.rs b/crates/stackable-versioned/src/lib.rs index 22925d547..4f3de2f34 100644 --- a/crates/stackable-versioned/src/lib.rs +++ b/crates/stackable-versioned/src/lib.rs @@ -19,7 +19,7 @@ //! /// My docs //! #[versioned( //! added(since = "v1beta1"), -//! renamed(since = "v1", from = "gau"), +//! changed(since = "v1", from_name = "gau"), //! deprecated(since = "v2", note = "not empty") //! )] //! deprecated_bar: usize, From 3937f577673ba0021eb35aa0be350246b0d6e818 Mon Sep 17 00:00:00 2001 From: Techassi Date: Fri, 30 Aug 2024 16:46:08 +0200 Subject: [PATCH 5/9] feat: Basic support for type changes The current implementation is very brittle and less than ideal. It currently only produces correct code in the struct definitions, but not in the From impls. For that reason, test files currently skip the From impl generation. I have a bunch of thoughts on how to improve the situation, but I would like to tackle these changes in follow-up commits. --- .../src/attrs/common/item.rs | 5 ++ .../src/codegen/common/item.rs | 64 +++++++++++++++---- .../src/codegen/venum/variant.rs | 19 ++++-- .../src/codegen/vstruct/field.rs | 27 ++++++-- .../tests/good/basic.rs | 3 +- 5 files changed, 95 insertions(+), 23 deletions(-) diff --git a/crates/stackable-versioned-macros/src/attrs/common/item.rs b/crates/stackable-versioned-macros/src/attrs/common/item.rs index 4c2e39b32..b3dba755b 100644 --- a/crates/stackable-versioned-macros/src/attrs/common/item.rs +++ b/crates/stackable-versioned-macros/src/attrs/common/item.rs @@ -97,6 +97,11 @@ pub(crate) enum ItemType { Variant, } +// TODO (@Techassi): Shower thought: Track actions as a Vec of an ActionAttribute +// enum and implement Ord on it. This would allow us to order the items by type +// of action (added < changed < deprecated) as well as sort changed action to +// each other by specified version (which already implements Ord) + /// These attributes are meant to be used in super structs, which add /// [`Field`](syn::Field) or [`Variant`](syn::Variant) specific attributes via /// darling's flatten feature. This struct only provides shared attributes. diff --git a/crates/stackable-versioned-macros/src/codegen/common/item.rs b/crates/stackable-versioned-macros/src/codegen/common/item.rs index 7fcf58a40..8d3024569 100644 --- a/crates/stackable-versioned-macros/src/codegen/common/item.rs +++ b/crates/stackable-versioned-macros/src/codegen/common/item.rs @@ -1,7 +1,7 @@ use std::{collections::BTreeMap, marker::PhantomData, ops::Deref}; use quote::format_ident; -use syn::{spanned::Spanned, Attribute, Ident, Path}; +use syn::{spanned::Spanned, Attribute, Ident, Path, Type}; use crate::{ attrs::common::{ContainerAttributes, ItemAttributes, ValidateVersions}, @@ -21,7 +21,7 @@ use crate::{ pub(crate) trait Item: Sized where A: for<'i> TryFrom<&'i I> + Attributes, - I: Named + Spanned, + I: InnerItem, { /// Creates a new versioned item (struct field or enum variant) by consuming /// the parsed [Field](syn::Field) or [Variant](syn::Variant) and validating @@ -43,6 +43,10 @@ where fn get_ident(&self, version: &ContainerVersion) -> Option<&Ident>; } +pub(crate) trait InnerItem: Named + Spanned { + fn ty(&self) -> Type; +} + /// This trait enables access to the ident of named items, like fields and /// variants. /// @@ -87,7 +91,7 @@ pub(crate) trait Attributes { pub(crate) struct VersionedItem where A: for<'i> TryFrom<&'i I> + Attributes, - I: Named + Spanned, + I: InnerItem, { pub(crate) original_attributes: Vec, pub(crate) chain: Option, @@ -99,7 +103,7 @@ impl Item for VersionedItem where syn::Error: for<'i> From<>::Error>, A: for<'i> TryFrom<&'i I> + Attributes + ValidateVersions, - I: Named + Spanned, + I: InnerItem, { fn new(item: I, container_attrs: &ContainerAttributes) -> syn::Result { // We use the TryFrom trait here, because the type parameter `A` can use @@ -135,6 +139,8 @@ where // requires access to the item ident to infer the item ident for // the latest change. let mut ident = item.cleaned_ident(); + let mut ty = item.ty(); + let mut actions = BTreeMap::new(); actions.insert( @@ -147,20 +153,33 @@ where ); for change in common_attributes.changes.iter().rev() { - let from = if let Some(from) = change.from_name.as_deref() { + dbg!(&ty, &change.since); + let from_ident = if let Some(from) = change.from_name.as_deref() { format_ident!("{from}") } else { ident.clone() }; + // TODO (@Techassi): This is an awful lot of cloning, can we get + // rid of it? + let from_ty = change + .from_type + .as_ref() + .map(|sv| sv.deref().clone()) + .unwrap_or(ty.clone()); + actions.insert( *change.since, ItemStatus::Change { - from_ident: from.clone(), + from_ident: from_ident.clone(), to_ident: ident, + from_type: from_ty.clone(), + to_type: ty, }, ); - ident = from; + + ident = from_ident; + ty = from_ty; } // After the last iteration above (if any) we use the ident for the @@ -171,6 +190,7 @@ where ItemStatus::Addition { default_fn: added.default_fn.deref().clone(), ident, + ty, }, ); } @@ -182,24 +202,39 @@ where inner: item, }) } else if !common_attributes.changes.is_empty() { - let mut actions = BTreeMap::new(); let mut ident = item.ident().clone(); + let mut ty = item.ty(); + + let mut actions = BTreeMap::new(); for change in common_attributes.changes.iter().rev() { - let from = if let Some(from) = change.from_name.as_deref() { + dbg!(&ty, &change.since); + let from_ident = if let Some(from) = change.from_name.as_deref() { format_ident!("{from}") } else { ident.clone() }; + // TODO (@Techassi): This is an awful lot of cloning, can we get + // rid of it? + let from_ty = change + .from_type + .as_ref() + .map(|sv| sv.deref().clone()) + .unwrap_or(ty.clone()); + actions.insert( *change.since, ItemStatus::Change { - from_ident: from.clone(), + from_ident: from_ident.clone(), to_ident: ident, + from_type: from_ty.clone(), + to_type: ty, }, ); - ident = from; + + ident = from_ident; + ty = from_ty; } // After the last iteration above (if any) we use the ident for the @@ -210,6 +245,7 @@ where ItemStatus::Addition { default_fn: added.default_fn.deref().clone(), ident, + ty, }, ); } @@ -229,6 +265,7 @@ where ItemStatus::Addition { default_fn: added.default_fn.deref().clone(), ident: item.ident().clone(), + ty: item.ty(), }, ); @@ -314,10 +351,15 @@ pub(crate) enum ItemStatus { Addition { ident: Ident, default_fn: Path, + // NOTE (@Techassi): We need to carry idents and type information in + // nearly every status. Ideally, we would store this in separate maps. + ty: Type, }, Change { from_ident: Ident, to_ident: Ident, + from_type: Type, + to_type: Type, }, Deprecation { previous_ident: Ident, diff --git a/crates/stackable-versioned-macros/src/codegen/venum/variant.rs b/crates/stackable-versioned-macros/src/codegen/venum/variant.rs index 2e9a76e57..ebefb2152 100644 --- a/crates/stackable-versioned-macros/src/codegen/venum/variant.rs +++ b/crates/stackable-versioned-macros/src/codegen/venum/variant.rs @@ -1,9 +1,9 @@ use std::ops::{Deref, DerefMut}; use darling::FromVariant; -use proc_macro2::TokenStream; +use proc_macro2::{Span, TokenStream}; use quote::quote; -use syn::{Ident, Variant}; +use syn::{token::Not, Ident, Type, TypeNever, Variant}; use crate::{ attrs::{ @@ -13,8 +13,8 @@ use crate::{ codegen::{ chain::BTreeMapExt, common::{ - remove_deprecated_variant_prefix, Attributes, ContainerVersion, Item, ItemStatus, - Named, VersionedItem, + remove_deprecated_variant_prefix, Attributes, ContainerVersion, InnerItem, Item, + ItemStatus, Named, VersionedItem, }, }, }; @@ -64,6 +64,17 @@ impl Attributes for VariantAttributes { } } +impl InnerItem for Variant { + fn ty(&self) -> syn::Type { + // FIXME (@Techassi): As we currently don't support enum variants with + // data, we just return the Never type as the code generation code for + // enum variants won't use this type information. + Type::Never(TypeNever { + bang_token: Not([Span::call_site()]), + }) + } +} + impl Named for Variant { fn cleaned_ident(&self) -> Ident { remove_deprecated_variant_prefix(self.ident()) diff --git a/crates/stackable-versioned-macros/src/codegen/vstruct/field.rs b/crates/stackable-versioned-macros/src/codegen/vstruct/field.rs index 752497069..9d0604001 100644 --- a/crates/stackable-versioned-macros/src/codegen/vstruct/field.rs +++ b/crates/stackable-versioned-macros/src/codegen/vstruct/field.rs @@ -11,8 +11,8 @@ use crate::{ field::FieldAttributes, }, codegen::common::{ - remove_deprecated_field_prefix, Attributes, ContainerVersion, Item, ItemStatus, Named, - VersionedItem, + remove_deprecated_field_prefix, Attributes, ContainerVersion, InnerItem, Item, ItemStatus, + Named, VersionedItem, }, }; @@ -63,6 +63,12 @@ impl Attributes for FieldAttributes { } } +impl InnerItem for Field { + fn ty(&self) -> syn::Type { + self.ty.clone() + } +} + impl Named for Field { fn cleaned_ident(&self) -> Ident { let ident = self.ident(); @@ -115,13 +121,15 @@ impl VersionedField { container_version.inner ) }) { - ItemStatus::Addition { ident, .. } => Some(quote! { + ItemStatus::Addition { ident, ty, .. } => Some(quote! { #(#original_attributes)* - pub #ident: #field_type, + pub #ident: #ty, }), - ItemStatus::Change { to_ident, .. } => Some(quote! { + ItemStatus::Change { + to_ident, to_type, .. + } => Some(quote! { #(#original_attributes)* - pub #to_ident: #field_type, + pub #to_ident: #to_type, }), ItemStatus::Deprecation { ident: field_ident, @@ -170,7 +178,12 @@ impl VersionedField { .get(&next_version.inner) .expect("internal error: chain must contain container version"), ) { - (_, ItemStatus::Addition { ident, default_fn }) => quote! { + ( + _, + ItemStatus::Addition { + ident, default_fn, .. + }, + ) => quote! { #ident: #default_fn(), }, (old, next) => { diff --git a/crates/stackable-versioned-macros/tests/good/basic.rs b/crates/stackable-versioned-macros/tests/good/basic.rs index 9bf3d38ce..c3d5f0d21 100644 --- a/crates/stackable-versioned-macros/tests/good/basic.rs +++ b/crates/stackable-versioned-macros/tests/good/basic.rs @@ -9,7 +9,8 @@ use stackable_versioned_macros::versioned; version(name = "v1beta1"), version(name = "v1"), version(name = "v2"), - version(name = "v3") + version(name = "v3"), + options(skip(from)) )] pub(crate) struct Foo { #[versioned( From 118ed09875c647fc4371c38377c0708942f1a0b3 Mon Sep 17 00:00:00 2001 From: Techassi Date: Mon, 2 Sep 2024 11:51:57 +0200 Subject: [PATCH 6/9] feat: Make note of deprecated action optional --- .../src/attrs/common/item.rs | 17 ++---------- .../src/codegen/common/item.rs | 4 +-- .../src/codegen/venum/variant.rs | 26 +++++++++++++++---- .../src/codegen/vstruct/field.rs | 26 +++++++++++++++---- 4 files changed, 46 insertions(+), 27 deletions(-) diff --git a/crates/stackable-versioned-macros/src/attrs/common/item.rs b/crates/stackable-versioned-macros/src/attrs/common/item.rs index b3dba755b..c3a9e4849 100644 --- a/crates/stackable-versioned-macros/src/attrs/common/item.rs +++ b/crates/stackable-versioned-macros/src/attrs/common/item.rs @@ -143,20 +143,6 @@ impl ItemAttributes { let mut errors = Error::accumulator(); - // TODO (@Techassi): Make the field or variant 'note' optional, because - // in the future, the macro will generate parts of the deprecation note - // automatically. The user-provided note will then be appended to the - // auto-generated one. - - if let Some(deprecated) = &self.deprecated { - if deprecated.note.is_empty() { - errors.push( - Error::custom("deprecation note must not be empty") - .with_span(&deprecated.note.span()), - ); - } - } - // Semantic validation errors.handle(self.validate_action_combinations(item_ident, item_type)); errors.handle(self.validate_action_order(item_ident, item_type)); @@ -348,9 +334,10 @@ pub(crate) struct ChangedAttributes { /// For the deprecated() action /// /// Example usage: +/// - `deprecated(since = "...")` /// - `deprecated(since = "...", note = "...")` #[derive(Clone, Debug, FromMeta)] pub(crate) struct DeprecatedAttributes { pub(crate) since: SpannedValue, - pub(crate) note: SpannedValue, + pub(crate) note: Option>, } diff --git a/crates/stackable-versioned-macros/src/codegen/common/item.rs b/crates/stackable-versioned-macros/src/codegen/common/item.rs index 8d3024569..7c3ce49ea 100644 --- a/crates/stackable-versioned-macros/src/codegen/common/item.rs +++ b/crates/stackable-versioned-macros/src/codegen/common/item.rs @@ -148,7 +148,7 @@ where ItemStatus::Deprecation { previous_ident: ident.clone(), ident: deprecated_ident.clone(), - note: deprecated.note.to_string(), + note: deprecated.note.as_deref().cloned(), }, ); @@ -363,8 +363,8 @@ pub(crate) enum ItemStatus { }, Deprecation { previous_ident: Ident, + note: Option, ident: Ident, - note: String, }, NoChange(Ident), NotPresent, diff --git a/crates/stackable-versioned-macros/src/codegen/venum/variant.rs b/crates/stackable-versioned-macros/src/codegen/venum/variant.rs index ebefb2152..3224b08f6 100644 --- a/crates/stackable-versioned-macros/src/codegen/venum/variant.rs +++ b/crates/stackable-versioned-macros/src/codegen/venum/variant.rs @@ -121,11 +121,27 @@ impl VersionedVariant { #(#original_attributes)* #to_ident, }), - ItemStatus::Deprecation { ident, .. } => Some(quote! { - #(#original_attributes)* - #[deprecated] - #ident, - }), + ItemStatus::Deprecation { ident, note, .. } => { + // FIXME (@Techassi): Emitting the deprecated attribute + // should cary over even when the item status is + // 'NoChange'. + // TODO (@Techassi): Make the generation of deprecated + // items customizable. When a container is used as a K8s + // CRD, the item must continue to exist, even when + // deprecated. For other versioning use-cases, that + // might not be the case. + let deprecated_attr = if let Some(note) = note { + quote! {#[deprecated = #note]} + } else { + quote! {#[deprecated]} + }; + + Some(quote! { + #(#original_attributes)* + #deprecated_attr + #ident, + }) + } ItemStatus::NoChange(ident) => Some(quote! { #(#original_attributes)* #ident, diff --git a/crates/stackable-versioned-macros/src/codegen/vstruct/field.rs b/crates/stackable-versioned-macros/src/codegen/vstruct/field.rs index 9d0604001..6ac2cb35a 100644 --- a/crates/stackable-versioned-macros/src/codegen/vstruct/field.rs +++ b/crates/stackable-versioned-macros/src/codegen/vstruct/field.rs @@ -135,11 +135,27 @@ impl VersionedField { ident: field_ident, note, .. - } => Some(quote! { - #(#original_attributes)* - #[deprecated = #note] - pub #field_ident: #field_type, - }), + } => { + // FIXME (@Techassi): Emitting the deprecated attribute + // should cary over even when the item status is + // 'NoChange'. + // TODO (@Techassi): Make the generation of deprecated + // items customizable. When a container is used as a K8s + // CRD, the item must continue to exist, even when + // deprecated. For other versioning use-cases, that + // might not be the case. + let deprecated_attr = if let Some(note) = note { + quote! {#[deprecated = #note]} + } else { + quote! {#[deprecated]} + }; + + Some(quote! { + #(#original_attributes)* + #deprecated_attr + pub #field_ident: #field_type, + }) + } ItemStatus::NotPresent => None, ItemStatus::NoChange(field_ident) => Some(quote! { #(#original_attributes)* From f1495272d6269c14043536852fb1b76d3bfce8d7 Mon Sep 17 00:00:00 2001 From: Techassi Date: Tue, 3 Sep 2024 14:33:14 +0200 Subject: [PATCH 7/9] feat: Generate correct From impls on type changes --- .../src/codegen/vstruct/field.rs | 19 +++++++++++++++++++ .../tests/good/basic.rs | 3 +-- 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/crates/stackable-versioned-macros/src/codegen/vstruct/field.rs b/crates/stackable-versioned-macros/src/codegen/vstruct/field.rs index 6ac2cb35a..fa6c48530 100644 --- a/crates/stackable-versioned-macros/src/codegen/vstruct/field.rs +++ b/crates/stackable-versioned-macros/src/codegen/vstruct/field.rs @@ -202,6 +202,25 @@ impl VersionedField { ) => quote! { #ident: #default_fn(), }, + ( + _, + ItemStatus::Change { + from_ident: old_field_ident, + to_ident, + from_type, + to_type, + }, + ) => { + if from_type == to_type { + quote! { + #to_ident: #from_ident.#old_field_ident, + } + } else { + quote! { + #to_ident: #from_ident.#old_field_ident.into(), + } + } + } (old, next) => { let old_field_ident = old .get_ident() diff --git a/crates/stackable-versioned-macros/tests/good/basic.rs b/crates/stackable-versioned-macros/tests/good/basic.rs index c3d5f0d21..9bf3d38ce 100644 --- a/crates/stackable-versioned-macros/tests/good/basic.rs +++ b/crates/stackable-versioned-macros/tests/good/basic.rs @@ -9,8 +9,7 @@ use stackable_versioned_macros::versioned; version(name = "v1beta1"), version(name = "v1"), version(name = "v2"), - version(name = "v3"), - options(skip(from)) + version(name = "v3") )] pub(crate) struct Foo { #[versioned( From 5089e4db8b57aa4609e4a4d91a97d73fee3bfde0 Mon Sep 17 00:00:00 2001 From: Techassi Date: Tue, 3 Sep 2024 14:40:17 +0200 Subject: [PATCH 8/9] chore: Update changelog --- crates/stackable-versioned/CHANGELOG.md | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/crates/stackable-versioned/CHANGELOG.md b/crates/stackable-versioned/CHANGELOG.md index af2e02d77..85d7b2360 100644 --- a/crates/stackable-versioned/CHANGELOG.md +++ b/crates/stackable-versioned/CHANGELOG.md @@ -6,12 +6,16 @@ All notable changes to this project will be documented in this file. ### Added -- Add `changed()` action and support for the `ty` parameter in the `renamed()` - action ([#844]). +- Add new `from_type` parameter to `changed()` action ([#844]). - Pass through container and item attributes (including doc-comments). Add attribute for version specific docs ([#847]). - Forward container visibility to generated modules ([#850]). +### Changed + +- BREAKING: Rename `renamed()` action to `changed()` and renamed `from` + parameter to `from_name` ([#844]). + ### Fixed - Report variant rename validation error at the correct span and trim underscores From 6774ab54c118a3ec7fb71a4345842cc25ea2d6dc Mon Sep 17 00:00:00 2001 From: Techassi Date: Tue, 3 Sep 2024 15:45:24 +0200 Subject: [PATCH 9/9] chore: Apply suggestions Co-authored-by: Nick <10092581+NickLarsenNZ@users.noreply.github.com> --- crates/stackable-versioned-macros/src/attrs/common/item.rs | 1 - crates/stackable-versioned-macros/src/codegen/common/item.rs | 1 - 2 files changed, 2 deletions(-) diff --git a/crates/stackable-versioned-macros/src/attrs/common/item.rs b/crates/stackable-versioned-macros/src/attrs/common/item.rs index c3a9e4849..c6ee146eb 100644 --- a/crates/stackable-versioned-macros/src/attrs/common/item.rs +++ b/crates/stackable-versioned-macros/src/attrs/common/item.rs @@ -327,7 +327,6 @@ pub(crate) struct ChangedAttributes { pub(crate) since: SpannedValue, pub(crate) from_name: Option>, - #[allow(dead_code)] pub(crate) from_type: Option>, } diff --git a/crates/stackable-versioned-macros/src/codegen/common/item.rs b/crates/stackable-versioned-macros/src/codegen/common/item.rs index 7c3ce49ea..aa40d5abb 100644 --- a/crates/stackable-versioned-macros/src/codegen/common/item.rs +++ b/crates/stackable-versioned-macros/src/codegen/common/item.rs @@ -153,7 +153,6 @@ where ); for change in common_attributes.changes.iter().rev() { - dbg!(&ty, &change.since); let from_ident = if let Some(from) = change.from_name.as_deref() { format_ident!("{from}") } else {