From e79a13d3d258810e314b47adc7f170ac31e813f6 Mon Sep 17 00:00:00 2001 From: Techassi Date: Fri, 28 Jun 2024 10:48:49 +0200 Subject: [PATCH 1/7] Update changelog --- crates/stackable-versioned/CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/crates/stackable-versioned/CHANGELOG.md b/crates/stackable-versioned/CHANGELOG.md index 20aed2ce0..161186d49 100644 --- a/crates/stackable-versioned/CHANGELOG.md +++ b/crates/stackable-versioned/CHANGELOG.md @@ -11,6 +11,7 @@ All notable changes to this project will be documented in this file. ### Changed +- Remove duplicated code and unified struct/enum and field/variant code ([#CHANGEME]). - Change from derive macro to attribute macro to be able to generate code _in place_ instead of _appending_ new code ([#793]). - Improve action chain generation ([#784]). @@ -19,6 +20,7 @@ All notable changes to this project will be documented in this file. [#790]: https://github.com/stackabletech/operator-rs/pull/790 [#793]: https://github.com/stackabletech/operator-rs/pull/793 [#804]: https://github.com/stackabletech/operator-rs/pull/804 +[#CHANGEME]: https://github.com/stackabletech/operator-rs/pull/CHANGEME ## [0.1.0] - 2024-05-08 From 30ec6153cef55cf306e06253de58fa6c9d4a4d28 Mon Sep 17 00:00:00 2001 From: Techassi Date: Fri, 28 Jun 2024 10:52:40 +0200 Subject: [PATCH 2/7] 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 161186d49..a0369bfbd 100644 --- a/crates/stackable-versioned/CHANGELOG.md +++ b/crates/stackable-versioned/CHANGELOG.md @@ -11,7 +11,7 @@ All notable changes to this project will be documented in this file. ### Changed -- Remove duplicated code and unified struct/enum and field/variant code ([#CHANGEME]). +- Remove duplicated code and unified struct/enum and field/variant code ([#820]). - Change from derive macro to attribute macro to be able to generate code _in place_ instead of _appending_ new code ([#793]). - Improve action chain generation ([#784]). @@ -20,7 +20,7 @@ All notable changes to this project will be documented in this file. [#790]: https://github.com/stackabletech/operator-rs/pull/790 [#793]: https://github.com/stackabletech/operator-rs/pull/793 [#804]: https://github.com/stackabletech/operator-rs/pull/804 -[#CHANGEME]: https://github.com/stackabletech/operator-rs/pull/CHANGEME +[#820]: https://github.com/stackabletech/operator-rs/pull/820 ## [0.1.0] - 2024-05-08 From 6fec455d876861f9f39862de715a7a57d9dd0c5f Mon Sep 17 00:00:00 2001 From: Techassi Date: Wed, 31 Jul 2024 14:41:52 +0200 Subject: [PATCH 3/7] refactor: Remove duplicated code for field and variant attrs --- Cargo.lock | 1 + crates/stackable-versioned-macros/Cargo.toml | 1 + .../src/attrs/common/item.rs | 156 +++++++++++++++++- .../src/attrs/field.rs | 135 +-------------- .../src/attrs/variant.rs | 136 +-------------- 5 files changed, 160 insertions(+), 269 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 1063f7b0f..656bfa5d5 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2986,6 +2986,7 @@ dependencies = [ "proc-macro2", "quote", "rstest", + "strum", "syn 2.0.70", ] diff --git a/crates/stackable-versioned-macros/Cargo.toml b/crates/stackable-versioned-macros/Cargo.toml index a2552d870..eef09494e 100644 --- a/crates/stackable-versioned-macros/Cargo.toml +++ b/crates/stackable-versioned-macros/Cargo.toml @@ -16,6 +16,7 @@ convert_case.workspace = true darling.workspace = true itertools.workspace = true proc-macro2.workspace = true +strum.workspace = true syn.workspace = true quote.workspace = true diff --git a/crates/stackable-versioned-macros/src/attrs/common/item.rs b/crates/stackable-versioned-macros/src/attrs/common/item.rs index 15b2885da..49790f2e4 100644 --- a/crates/stackable-versioned-macros/src/attrs/common/item.rs +++ b/crates/stackable-versioned-macros/src/attrs/common/item.rs @@ -1,13 +1,21 @@ use darling::{util::SpannedValue, Error, FromMeta}; use k8s_version::Version; use proc_macro2::Span; -use syn::Path; +use syn::{Ident, Path}; + +use crate::consts::{DEPRECATED_FIELD_PREFIX, DEPRECATED_VARIANT_PREFIX}; + +#[derive(Debug, strum::Display)] +#[strum(serialize_all = "lowercase")] +pub(crate) enum ItemType { + Field, + Variant, +} /// 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. #[derive(Debug, FromMeta)] -#[darling(and_then = ItemAttributes::validate)] pub(crate) struct ItemAttributes { /// This parses the `added` attribute on items (fields or variants). It can /// only be present at most once. @@ -24,8 +32,12 @@ pub(crate) struct ItemAttributes { } impl ItemAttributes { - fn validate(self) -> Result { - // Validate deprecated options + pub(crate) fn validate(&self, item_ident: &Ident, item_type: &ItemType) -> Result<(), Error> { + // NOTE (@Techassi): This associated function is NOT called by darling's + // and_then attribute, but instead by the wrapper, FieldAttributes and + // VariantAttributes. + + let mut errors = Error::accumulator(); // TODO (@Techassi): Make the field 'note' optional, because in the // future, the macro will generate parts of the deprecation note @@ -34,12 +46,142 @@ impl ItemAttributes { if let Some(deprecated) = &self.deprecated { if deprecated.note.is_empty() { - return Err(Error::custom("deprecation note must not be empty") - .with_span(&deprecated.note.span())); + errors.push( + Error::custom("deprecation note must not be empty") + .with_span(&deprecated.note.span()), + ); } } - Ok(self) + // Semantic validation + errors.handle(self.validate_action_combinations(item_ident, item_type)); + errors.handle(self.validate_action_order(item_ident, item_type)); + errors.handle(self.validate_field_name(item_ident, item_type)); + + // TODO (@Techassi): Add hint if a field is added in the first version + // that it might be clever to remove the 'added' attribute. + + errors.finish()?; + + Ok(()) + } + + /// This associated function is called by the top-level validation function + /// and validates that each item uses a valid combination of actions. + /// Invalid combinations are: + /// + /// - `added` and `deprecated` using the same version: A field cannot be + /// marked as added in a particular version and then marked as deprecated + /// immediately after. Fields must be included for at least one version + /// before being marked deprecated. + /// - `added` and `renamed` using the same version: The same reasoning from + /// above applies here as well. Fields must be included for at least one + /// version before being renamed. + /// - `renamed` 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) { + (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) => { + Err(Error::custom(format!( + "{item_type} cannot be marked as `added` and `renamed` in the same version" + )) + .with_span(item_ident)) + } + (_, renamed, Some(deprecated)) + if renamed.iter().any(|r| *r.since == *deprecated.since) => + { + Err(Error::custom( + "field cannot be marked as `deprecated` and `renamed` in the same version", + ) + .with_span(item_ident)) + } + _ => Ok(()), + } + } + + /// This associated function is called by the top-level validation function + /// and validates that actions use a chronologically sound chain of + /// versions. + /// + /// The following rules apply: + /// + /// - `deprecated` must use a greater version than `added`: This function + /// 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 + /// 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); + let deprecated_version = self.deprecated.as_ref().map(|d| *d.since); + + // First, validate that the added version is less than the deprecated + // version. + // NOTE (@Techassi): Is this already covered by the code below? + if let (Some(added_version), Some(deprecated_version)) = (added_version, deprecated_version) + { + if added_version > deprecated_version { + return Err(Error::custom(format!( + "{item_type} was marked as `added` in version `{added_version}` while being marked as `deprecated` in an earlier version `{deprecated_version}`" + )).with_span(item_ident)); + } + } + + // Now, iterate over all renames and ensure that their versions are + // between the added and deprecated version. + if !self.renames.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`", + ) + .with_span(item_ident)); + } + + Ok(()) + } + + /// This associated function is called by the top-level validation function + /// and validates that items use correct names depending on attached + /// actions. + /// + /// The following naming rules apply: + /// + /// - Fields marked as deprecated need to include the 'deprecated_' prefix + /// in their name. The prefix must not be included for fields which are + /// not deprecated. + fn validate_field_name(&self, item_ident: &Ident, item_type: &ItemType) -> Result<(), Error> { + let prefix = match item_type { + ItemType::Field => DEPRECATED_FIELD_PREFIX, + ItemType::Variant => DEPRECATED_VARIANT_PREFIX, + }; + + let starts_with_deprecated = item_ident.to_string().starts_with(prefix); + + if self.deprecated.is_some() && !starts_with_deprecated { + return Err(Error::custom( + format!("{item_type} was marked as `deprecated` and thus must include the `{prefix}` prefix in its name") + ).with_span(item_ident)); + } + + if self.deprecated.is_none() && starts_with_deprecated { + return Err(Error::custom( + format!("{item_type} includes the `{prefix}` prefix in its name but is not marked as `deprecated`") + ).with_span(item_ident)); + } + + Ok(()) } } diff --git a/crates/stackable-versioned-macros/src/attrs/field.rs b/crates/stackable-versioned-macros/src/attrs/field.rs index 94851a8d1..2d5dc78dd 100644 --- a/crates/stackable-versioned-macros/src/attrs/field.rs +++ b/crates/stackable-versioned-macros/src/attrs/field.rs @@ -1,10 +1,7 @@ use darling::{Error, FromField}; use syn::{Field, Ident}; -use crate::{ - attrs::common::{ContainerAttributes, ItemAttributes}, - consts::DEPRECATED_FIELD_PREFIX, -}; +use crate::attrs::common::{ContainerAttributes, ItemAttributes, ItemType}; /// This struct describes all available field attributes, as well as the field /// name to display better diagnostics. @@ -55,138 +52,12 @@ impl FieldAttributes { /// /// Internally, it calls out to other specialized validation functions. fn validate(self) -> Result { - let mut errors = Error::accumulator(); - - // Semantic validation - errors.handle(self.validate_action_combinations()); - errors.handle(self.validate_action_order()); - errors.handle(self.validate_field_name()); - - // TODO (@Techassi): Add hint if a field is added in the first version - // that it might be clever to remove the 'added' attribute. + self.common + .validate(self.ident.as_ref().unwrap(), &ItemType::Field)?; - errors.finish()?; Ok(self) } - /// This associated function is called by the top-level validation function - /// and validates that each field uses a valid combination of actions. - /// Invalid combinations are: - /// - /// - `added` and `deprecated` using the same version: A field cannot be - /// marked as added in a particular version and then marked as deprecated - /// immediately after. Fields must be included for at least one version - /// before being marked deprecated. - /// - `added` and `renamed` using the same version: The same reasoning from - /// above applies here as well. Fields must be included for at least one - /// version before being renamed. - /// - `renamed` and `deprecated` using the same version: Again, the same - /// rules from above apply here as well. - fn validate_action_combinations(&self) -> Result<(), Error> { - match ( - &self.common.added, - &self.common.renames, - &self.common.deprecated, - ) { - (Some(added), _, Some(deprecated)) if *added.since == *deprecated.since => { - Err(Error::custom( - "field cannot be marked as `added` and `deprecated` in the same version", - ) - .with_span(&self.ident)) - } - (Some(added), renamed, _) if renamed.iter().any(|r| *r.since == *added.since) => { - Err(Error::custom( - "field cannot be marked as `added` and `renamed` in the same version", - ) - .with_span(&self.ident)) - } - (_, renamed, Some(deprecated)) - if renamed.iter().any(|r| *r.since == *deprecated.since) => - { - Err(Error::custom( - "field cannot be marked as `deprecated` and `renamed` in the same version", - ) - .with_span(&self.ident)) - } - _ => Ok(()), - } - } - - /// This associated function is called by the top-level validation function - /// and validates that actions use a chronologically sound chain of - /// versions. - /// - /// The following rules apply: - /// - /// - `deprecated` must use a greater version than `added`: This function - /// 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 - /// lesser version than `deprecated`. - fn validate_action_order(&self) -> Result<(), Error> { - let added_version = self.common.added.as_ref().map(|a| *a.since); - let deprecated_version = self.common.deprecated.as_ref().map(|d| *d.since); - - // First, validate that the added version is less than the deprecated - // version. - // NOTE (@Techassi): Is this already covered by the code below? - if let (Some(added_version), Some(deprecated_version)) = (added_version, deprecated_version) - { - if added_version > deprecated_version { - return Err(Error::custom(format!( - "field was marked as `added` in version `{added_version}` while being marked as `deprecated` in an earlier version `{deprecated_version}`" - )).with_span(&self.ident)); - } - } - - // Now, iterate over all renames and ensure that their versions are - // between the added and deprecated version. - if !self.common.renames.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`", - ) - .with_span(&self.ident)); - } - - Ok(()) - } - - /// This associated function is called by the top-level validation function - /// and validates that fields use correct names depending on attached - /// actions. - /// - /// The following naming rules apply: - /// - /// - Fields marked as deprecated need to include the 'deprecated_' prefix - /// in their name. The prefix must not be included for fields which are - /// not deprecated. - fn validate_field_name(&self) -> Result<(), Error> { - let starts_with_deprecated = self - .ident - .as_ref() - .expect("internal error: to be validated fields must have a name") - .to_string() - .starts_with(DEPRECATED_FIELD_PREFIX); - - if self.common.deprecated.is_some() && !starts_with_deprecated { - return Err(Error::custom( - "field was marked as `deprecated` and thus must include the `deprecated_` prefix in its name" - ).with_span(&self.ident)); - } - - if self.common.deprecated.is_none() && starts_with_deprecated { - return Err(Error::custom( - "field includes the `deprecated_` prefix in its name but is not marked as `deprecated`" - ).with_span(&self.ident)); - } - - Ok(()) - } - /// Validates that each field action version is present in the declared /// container versions. pub(crate) fn validate_versions( diff --git a/crates/stackable-versioned-macros/src/attrs/variant.rs b/crates/stackable-versioned-macros/src/attrs/variant.rs index 75f3ebd59..ef00a26a8 100644 --- a/crates/stackable-versioned-macros/src/attrs/variant.rs +++ b/crates/stackable-versioned-macros/src/attrs/variant.rs @@ -2,10 +2,7 @@ use convert_case::{Case, Casing}; use darling::{Error, FromVariant}; use syn::{Ident, Variant}; -use crate::{ - attrs::common::{ContainerAttributes, ItemAttributes}, - consts::DEPRECATED_VARIANT_PREFIX, -}; +use crate::attrs::common::{ContainerAttributes, ItemAttributes, ItemType}; #[derive(Debug, FromVariant)] #[darling( @@ -41,141 +38,20 @@ impl VariantAttributes { fn validate(self) -> Result { let mut errors = Error::accumulator(); - // Semantic validation - errors.handle(self.validate_action_combinations()); - errors.handle(self.validate_action_order()); - errors.handle(self.validate_variant_name()); + errors.handle(self.common.validate(&self.ident, &ItemType::Variant)); - // TODO (@Techassi): Add hint if a item is added in the first version - // that it might be clever to remove the 'added' attribute. - - errors.finish()?; - Ok(self) - } - - /// This associated function is called by the top-level validation function - /// and validates that each variant uses a valid combination of actions. - /// Invalid combinations are: - /// - /// - `added` and `deprecated` using the same version: A variant cannot be - /// marked as added in a particular version and then marked as deprecated - /// immediately after. Variants must be included for at least one version - /// before being marked deprecated. - /// - `added` and `renamed` using the same version: The same reasoning from - /// above applies here as well. Variants must be included for at least one - /// version before being renamed. - /// - `renamed` and `deprecated` using the same version: Again, the same - /// rules from above apply here as well. - fn validate_action_combinations(&self) -> Result<(), Error> { - match ( - &self.common.added, - &self.common.renames, - &self.common.deprecated, - ) { - (Some(added), _, Some(deprecated)) if *added.since == *deprecated.since => { - Err(Error::custom( - "variant cannot be marked as `added` and `deprecated` in the same version", - ) - .with_span(&self.ident)) - } - (Some(added), renamed, _) if renamed.iter().any(|r| *r.since == *added.since) => { - Err(Error::custom( - "variant cannot be marked as `added` and `renamed` in the same version", - ) - .with_span(&self.ident)) - } - (_, renamed, Some(deprecated)) - if renamed.iter().any(|r| *r.since == *deprecated.since) => - { - Err(Error::custom( - "variant cannot be marked as `deprecated` and `renamed` in the same version", - ) - .with_span(&self.ident)) - } - _ => Ok(()), - } - } - - /// This associated function is called by the top-level validation function - /// and validates that actions use a chronologically sound chain of - /// versions. - /// - /// The following rules apply: - /// - /// - `deprecated` must use a greater version than `added`: This function - /// 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 - /// lesser version than `deprecated`. - fn validate_action_order(&self) -> Result<(), Error> { - let added_version = self.common.added.as_ref().map(|a| *a.since); - let deprecated_version = self.common.deprecated.as_ref().map(|d| *d.since); - - // First, validate that the added version is less than the deprecated - // version. - // NOTE (@Techassi): Is this already covered by the code below? - if let (Some(added_version), Some(deprecated_version)) = (added_version, deprecated_version) - { - if added_version > deprecated_version { - return Err(Error::custom(format!( - "variant was marked as `added` in version `{added_version}` while being marked as `deprecated` in an earlier version `{deprecated_version}`" - )).with_span(&self.ident)); - } - } - - // Now, iterate over all renames and ensure that their versions are - // between the added and deprecated version. - if !self.common.renames.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`", - ) - .with_span(&self.ident)); - } - - Ok(()) - } - - /// This associated function is called by the top-level validation function - /// and validates that variants use correct names depending on attached - /// actions. - /// - /// The following naming rules apply: - /// - /// - Variants marked as deprecated need to include the 'deprecated_' prefix - /// in their name. The prefix must not be included for variants which are - /// not deprecated. - fn validate_variant_name(&self) -> Result<(), Error> { + // Validate names of renames if !self .common .renames .iter() .all(|r| r.from.is_case(Case::Pascal)) { - return Err(Error::custom("renamed variants must use PascalCase")); + errors.push(Error::custom("renamed variants must use PascalCase")); } - let starts_with_deprecated = self - .ident - .to_string() - .starts_with(DEPRECATED_VARIANT_PREFIX); - - if self.common.deprecated.is_some() && !starts_with_deprecated { - return Err(Error::custom( - "variant was marked as `deprecated` and thus must include the `Deprecated` prefix in its name" - ).with_span(&self.ident)); - } - - if self.common.deprecated.is_none() && starts_with_deprecated { - return Err(Error::custom( - "variant includes the `Deprecated` prefix in its name but is not marked as `deprecated`" - ).with_span(&self.ident)); - } - - Ok(()) + errors.finish()?; + Ok(self) } pub(crate) fn validate_versions( From 42fb1283051881a48517c630458188abe13103f3 Mon Sep 17 00:00:00 2001 From: Techassi Date: Fri, 16 Aug 2024 15:30:49 +0200 Subject: [PATCH 4/7] refactor: Unify field and variant attributes --- .../src/attrs/common/item.rs | 78 ++++- .../src/attrs/field.rs | 57 +-- .../src/attrs/variant.rs | 57 +-- .../src/{gen => codegen}/chain.rs | 0 .../src/codegen/common/container.rs | 31 ++ .../variant.rs => codegen/common/item.rs} | 257 +++++++------- .../src/codegen/common/mod.rs | 67 ++++ .../src/{gen => codegen}/mod.rs | 2 +- .../src/{gen => codegen}/venum/mod.rs | 12 +- .../src/codegen/venum/variant.rs | 164 +++++++++ .../src/codegen/vstruct/field.rs | 178 ++++++++++ .../src/{gen => codegen}/vstruct/mod.rs | 8 +- .../src/gen/common.rs | 110 ------ .../src/gen/vstruct/field.rs | 327 ------------------ crates/stackable-versioned-macros/src/lib.rs | 4 +- 15 files changed, 668 insertions(+), 684 deletions(-) rename crates/stackable-versioned-macros/src/{gen => codegen}/chain.rs (100%) create mode 100644 crates/stackable-versioned-macros/src/codegen/common/container.rs rename crates/stackable-versioned-macros/src/{gen/venum/variant.rs => codegen/common/item.rs} (52%) create mode 100644 crates/stackable-versioned-macros/src/codegen/common/mod.rs rename crates/stackable-versioned-macros/src/{gen => codegen}/mod.rs (94%) rename crates/stackable-versioned-macros/src/{gen => codegen}/venum/mod.rs (93%) create mode 100644 crates/stackable-versioned-macros/src/codegen/venum/variant.rs create mode 100644 crates/stackable-versioned-macros/src/codegen/vstruct/field.rs rename crates/stackable-versioned-macros/src/{gen => codegen}/vstruct/mod.rs (97%) delete mode 100644 crates/stackable-versioned-macros/src/gen/common.rs delete mode 100644 crates/stackable-versioned-macros/src/gen/vstruct/field.rs diff --git a/crates/stackable-versioned-macros/src/attrs/common/item.rs b/crates/stackable-versioned-macros/src/attrs/common/item.rs index 49790f2e4..b3a83252a 100644 --- a/crates/stackable-versioned-macros/src/attrs/common/item.rs +++ b/crates/stackable-versioned-macros/src/attrs/common/item.rs @@ -1,9 +1,83 @@ use darling::{util::SpannedValue, Error, FromMeta}; use k8s_version::Version; use proc_macro2::Span; -use syn::{Ident, Path}; +use syn::{spanned::Spanned, Ident, Path}; -use crate::consts::{DEPRECATED_FIELD_PREFIX, DEPRECATED_VARIANT_PREFIX}; +use crate::{ + attrs::common::ContainerAttributes, + codegen::common::Attributes, + consts::{DEPRECATED_FIELD_PREFIX, DEPRECATED_VARIANT_PREFIX}, +}; + +pub(crate) trait ValidateVersions +where + I: Spanned, +{ + /// Validates that each field action version is present in the declared + /// container versions. + fn validate_versions( + &self, + container_attrs: &ContainerAttributes, + item: &I, + ) -> Result<(), darling::Error>; +} + +impl ValidateVersions for T +where + T: Attributes, + I: Spanned, +{ + fn validate_versions( + &self, + container_attrs: &ContainerAttributes, + item: &I, + ) -> Result<(), darling::Error> { + // NOTE (@Techassi): Can we maybe optimize this a little? + let mut errors = Error::accumulator(); + + if let Some(added) = &self.common_attrs().added { + if !container_attrs + .versions + .iter() + .any(|v| v.name == *added.since) + { + errors.push(Error::custom( + "variant action `added` uses version which was not declared via #[versioned(version)]") + .with_span(item) + ); + } + } + + for rename in &*self.common_attrs().renames { + if !container_attrs + .versions + .iter() + .any(|v| v.name == *rename.since) + { + errors.push( + Error::custom("variant action `renamed` uses version which was not declared via #[versioned(version)]") + .with_span(item) + ); + } + } + + if let Some(deprecated) = &self.common_attrs().deprecated { + if !container_attrs + .versions + .iter() + .any(|v| v.name == *deprecated.since) + { + errors.push(Error::custom( + "variant action `deprecated` uses version which was not declared via #[versioned(version)]") + .with_span(item) + ); + } + } + + errors.finish()?; + Ok(()) + } +} #[derive(Debug, strum::Display)] #[strum(serialize_all = "lowercase")] diff --git a/crates/stackable-versioned-macros/src/attrs/field.rs b/crates/stackable-versioned-macros/src/attrs/field.rs index 2d5dc78dd..90d6e79eb 100644 --- a/crates/stackable-versioned-macros/src/attrs/field.rs +++ b/crates/stackable-versioned-macros/src/attrs/field.rs @@ -1,7 +1,7 @@ use darling::{Error, FromField}; -use syn::{Field, Ident}; +use syn::Ident; -use crate::attrs::common::{ContainerAttributes, ItemAttributes, ItemType}; +use crate::attrs::common::{ItemAttributes, ItemType}; /// This struct describes all available field attributes, as well as the field /// name to display better diagnostics. @@ -57,57 +57,4 @@ impl FieldAttributes { Ok(self) } - - /// Validates that each field action version is present in the declared - /// container versions. - pub(crate) fn validate_versions( - &self, - container_attrs: &ContainerAttributes, - field: &Field, - ) -> Result<(), Error> { - // NOTE (@Techassi): Can we maybe optimize this a little? - let mut errors = Error::accumulator(); - - if let Some(added) = &self.common.added { - if !container_attrs - .versions - .iter() - .any(|v| v.name == *added.since) - { - errors.push(Error::custom( - "field action `added` uses version which was not declared via #[versioned(version)]") - .with_span(&field.ident) - ); - } - } - - for rename in &self.common.renames { - if !container_attrs - .versions - .iter() - .any(|v| v.name == *rename.since) - { - errors.push( - Error::custom("field action `renamed` uses version which was not declared via #[versioned(version)]") - .with_span(&field.ident) - ); - } - } - - if let Some(deprecated) = &self.common.deprecated { - if !container_attrs - .versions - .iter() - .any(|v| v.name == *deprecated.since) - { - errors.push(Error::custom( - "field action `deprecated` uses version which was not declared via #[versioned(version)]") - .with_span(&field.ident) - ); - } - } - - errors.finish()?; - Ok(()) - } } diff --git a/crates/stackable-versioned-macros/src/attrs/variant.rs b/crates/stackable-versioned-macros/src/attrs/variant.rs index ef00a26a8..d4ff8af3a 100644 --- a/crates/stackable-versioned-macros/src/attrs/variant.rs +++ b/crates/stackable-versioned-macros/src/attrs/variant.rs @@ -1,8 +1,8 @@ use convert_case::{Case, Casing}; use darling::{Error, FromVariant}; -use syn::{Ident, Variant}; +use syn::Ident; -use crate::attrs::common::{ContainerAttributes, ItemAttributes, ItemType}; +use crate::attrs::common::{ItemAttributes, ItemType}; #[derive(Debug, FromVariant)] #[darling( @@ -53,57 +53,4 @@ impl VariantAttributes { errors.finish()?; Ok(self) } - - pub(crate) fn validate_versions( - &self, - container_attrs: &ContainerAttributes, - variant: &Variant, - ) -> Result<(), Error> { - // NOTE (@Techassi): Can we maybe optimize this a little? - // TODO (@Techassi): Unify this with the field impl, e.g. by introducing - // a T: Spanned bound for the second function parameter. - let mut errors = Error::accumulator(); - - if let Some(added) = &self.common.added { - if !container_attrs - .versions - .iter() - .any(|v| v.name == *added.since) - { - errors.push(Error::custom( - "variant action `added` uses version which was not declared via #[versioned(version)]") - .with_span(&variant.ident) - ); - } - } - - for rename in &*self.common.renames { - if !container_attrs - .versions - .iter() - .any(|v| v.name == *rename.since) - { - errors.push( - Error::custom("variant action `renamed` uses version which was not declared via #[versioned(version)]") - .with_span(&variant.ident) - ); - } - } - - if let Some(deprecated) = &self.common.deprecated { - if !container_attrs - .versions - .iter() - .any(|v| v.name == *deprecated.since) - { - errors.push(Error::custom( - "variant action `deprecated` uses version which was not declared via #[versioned(version)]") - .with_span(&variant.ident) - ); - } - } - - errors.finish()?; - Ok(()) - } } diff --git a/crates/stackable-versioned-macros/src/gen/chain.rs b/crates/stackable-versioned-macros/src/codegen/chain.rs similarity index 100% rename from crates/stackable-versioned-macros/src/gen/chain.rs rename to crates/stackable-versioned-macros/src/codegen/chain.rs diff --git a/crates/stackable-versioned-macros/src/codegen/common/container.rs b/crates/stackable-versioned-macros/src/codegen/common/container.rs new file mode 100644 index 000000000..4c50a87bc --- /dev/null +++ b/crates/stackable-versioned-macros/src/codegen/common/container.rs @@ -0,0 +1,31 @@ +use std::ops::Deref; + +use proc_macro2::TokenStream; +use syn::Ident; + +use crate::{attrs::common::ContainerAttributes, codegen::common::ContainerVersion}; + +pub(crate) trait Container +where + Self: Sized + Deref>, +{ + fn new(ident: Ident, data: D, attributes: ContainerAttributes) -> syn::Result; + + /// This generates the complete code for a single versioned container. + /// + /// Internally, it will create a module for each declared version which + /// contains the container with the appropriate items (fields or variants) + /// Additionally, it generates `From` implementations, which enable + /// conversion from an older to a newer version. + fn generate_tokens(&self) -> TokenStream; +} + +#[derive(Debug)] +pub(crate) struct VersionedContainer { + pub(crate) versions: Vec, + pub(crate) items: Vec, + pub(crate) ident: Ident, + + pub(crate) from_ident: Ident, + pub(crate) skip_from: bool, +} diff --git a/crates/stackable-versioned-macros/src/gen/venum/variant.rs b/crates/stackable-versioned-macros/src/codegen/common/item.rs similarity index 52% rename from crates/stackable-versioned-macros/src/gen/venum/variant.rs rename to crates/stackable-versioned-macros/src/codegen/common/item.rs index d06c29651..68cb9959a 100644 --- a/crates/stackable-versioned-macros/src/gen/venum/variant.rs +++ b/crates/stackable-versioned-macros/src/codegen/common/item.rs @@ -1,46 +1,99 @@ -use std::{collections::BTreeMap, ops::Deref}; +use std::{collections::BTreeMap, marker::PhantomData, ops::Deref}; -use darling::FromVariant; -use proc_macro2::TokenStream; -use quote::{format_ident, quote}; -use syn::{Ident, Variant}; +use quote::format_ident; +use syn::{spanned::Spanned, Ident, Path}; use crate::{ - attrs::{common::ContainerAttributes, variant::VariantAttributes}, - gen::{ - chain::{BTreeMapExt, Neighbors}, - common::{remove_deprecated_variant_prefix, ContainerVersion, ItemStatus, VersionChain}, + attrs::common::{ContainerAttributes, ItemAttributes, ValidateVersions}, + codegen::{ + chain::Neighbors, + common::{ContainerVersion, VersionChain}, }, }; +/// This trait describes versioned container items, fields ans variants in a +/// common way. +/// +/// Shared functionality is implemented in a single place. Code which cannot be +/// shared is implemented on the wrapping type, like [`VersionedField`][1]. +/// +/// [1]: crate::codegen::vstruct::field::VersionedField +pub(crate) trait Item: Sized +where + A: for<'i> TryFrom<&'i I> + Attributes, + I: Named + Spanned, +{ + /// Creates a new versioned item (struct field or enum variant) by consuming + /// the parsed [Field](syn::Field) or [Variant](syn::Variant) and validating + /// the versions of field actions against versions attached on the container. + fn new(item: I, container_attrs: &ContainerAttributes) -> syn::Result; + + /// Inserts container versions not yet present in the status chain. + /// + /// When initially creating a new versioned item, the code doesn't have + /// access to the versions defined on the container. This function inserts + /// all non-present container versions and decides which status and ident + /// is the right fit based on the status neighbors. + /// + /// This continuous chain ensures that when generating code (tokens), each + /// field can lookup the status (and ident) for a requested version. + fn insert_container_versions(&mut self, versions: &[ContainerVersion]); + + /// Returns the ident of the item based on the provided container version. + fn get_ident(&self, version: &ContainerVersion) -> Option<&Ident>; +} + +pub(crate) trait Named { + fn cleaned_ident(&self) -> Ident; + fn ident(&self) -> &Ident; +} + +pub(crate) trait Attributes { + fn common_attrs_owned(self) -> ItemAttributes; + fn common_attrs(&self) -> &ItemAttributes; +} + #[derive(Debug)] -pub(crate) struct VersionedVariant { - chain: Option, - inner: Variant, +pub(crate) struct VersionedItem +where + A: for<'i> TryFrom<&'i I> + Attributes, + I: Named + Spanned, +{ + pub(crate) chain: Option, + pub(crate) inner: I, + _marker: PhantomData, } -// TODO (@Techassi): Figure out a way to be able to only write the following code -// once for both a versioned field and variant, because the are practically -// identical. +impl Item for VersionedItem +where + syn::Error: for<'i> From<>::Error>, + A: for<'i> TryFrom<&'i I> + Attributes + ValidateVersions, + I: Named + Spanned, +{ + fn new(item: I, container_attrs: &ContainerAttributes) -> syn::Result { + let attrs = A::try_from(&item)?; + attrs.validate_versions(container_attrs, &item)?; -impl VersionedVariant { - pub(crate) fn new( - variant: Variant, - container_attrs: &ContainerAttributes, - ) -> syn::Result { - // NOTE (@Techassi): This is straight up copied from the VersionedField - // impl. As mentioned above, unify this. + let item_attrs = attrs.common_attrs_owned(); - let variant_attrs = VariantAttributes::from_variant(&variant)?; - variant_attrs.validate_versions(container_attrs, &variant)?; + // Constructing the action chain requires going through the actions + // starting at the end, because the base container always represents the + // latest (most up-to-date) version of that struct. That's why the + // following code needs to go through the actions in reverse order, as + // otherwise it is impossible to extract the item ident for each version. - if let Some(deprecated) = variant_attrs.common.deprecated { - let deprecated_ident = &variant.ident; + // 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 + // deprecated item is guaranteed to include the 'deprecated_' or + // 'DEPRECATED_' prefix. The ident can thus be used as is. + if let Some(deprecated) = item_attrs.deprecated { + let deprecated_ident = item.ident(); - // When the variant is deprecated, any rename which occurred beforehand - // requires access to the variant ident to infer the variant ident for + // When the item is deprecated, any rename which occurred beforehand + // requires access to the item ident to infer the item ident for // the latest rename. - let mut ident = remove_deprecated_variant_prefix(deprecated_ident); + let mut ident = item.cleaned_ident(); let mut actions = BTreeMap::new(); actions.insert( @@ -52,7 +105,7 @@ impl VersionedVariant { }, ); - for rename in variant_attrs.common.renames.iter().rev() { + for rename in item_attrs.renames.iter().rev() { let from = format_ident!("{from}", from = *rename.from); actions.insert( *rename.since, @@ -66,7 +119,7 @@ impl VersionedVariant { // After the last iteration above (if any) we use the ident for the // added action if there is any. - if let Some(added) = variant_attrs.common.added { + if let Some(added) = item_attrs.added { actions.insert( *added.since, ItemStatus::Added { @@ -77,14 +130,15 @@ impl VersionedVariant { } Ok(Self { + _marker: PhantomData, chain: Some(actions), - inner: variant, + inner: item, }) - } else if !variant_attrs.common.renames.is_empty() { + } else if !item_attrs.renames.is_empty() { let mut actions = BTreeMap::new(); - let mut ident = variant.ident.clone(); + let mut ident = item.ident().clone(); - for rename in variant_attrs.common.renames.iter().rev() { + for rename in item_attrs.renames.iter().rev() { let from = format_ident!("{from}", from = *rename.from); actions.insert( *rename.since, @@ -98,7 +152,7 @@ impl VersionedVariant { // After the last iteration above (if any) we use the ident for the // added action if there is any. - if let Some(added) = variant_attrs.common.added { + if let Some(added) = item_attrs.added { actions.insert( *added.since, ItemStatus::Added { @@ -109,44 +163,38 @@ impl VersionedVariant { } Ok(Self { + _marker: PhantomData, chain: Some(actions), - inner: variant, + inner: item, }) } else { - if let Some(added) = variant_attrs.common.added { + if let Some(added) = item_attrs.added { let mut actions = BTreeMap::new(); actions.insert( *added.since, ItemStatus::Added { default_fn: added.default_fn.deref().clone(), - ident: variant.ident.clone(), + ident: item.ident().clone(), }, ); return Ok(Self { + _marker: PhantomData, chain: Some(actions), - inner: variant, + inner: item, }); } Ok(Self { + _marker: PhantomData, chain: None, - inner: variant, + inner: item, }) } } - /// Inserts container versions not yet present in the status chain. - /// - /// When initially creating a new versioned item, the code doesn't have - /// access to the versions defined on the container. This function inserts - /// all non-present container versions and decides which status and ident - /// is the right fit based on the status neighbors. - /// - /// This continuous chain ensures that when generating code (tokens), each - /// variant can lookup the status (and ident) for a requested version. - pub(crate) fn insert_container_versions(&mut self, versions: &[ContainerVersion]) { + fn insert_container_versions(&mut self, versions: &[ContainerVersion]) { if let Some(chain) = &mut self.chain { for version in versions { if chain.contains_key(&version.inner) { @@ -195,87 +243,44 @@ impl VersionedVariant { } } - pub(crate) fn generate_for_container( - &self, - container_version: &ContainerVersion, - ) -> Option { + fn get_ident(&self, version: &ContainerVersion) -> Option<&Ident> { match &self.chain { - Some(chain) => match chain - .get(&container_version.inner) + Some(chain) => chain + .get(&version.inner) .expect("internal error: chain must contain container version") - { - ItemStatus::Added { ident, .. } => Some(quote! { - #ident, - }), - ItemStatus::Renamed { to, .. } => Some(quote! { - #to, - }), - ItemStatus::Deprecated { ident, .. } => Some(quote! { - #[deprecated] - #ident, - }), - ItemStatus::NoChange(ident) => Some(quote! { - #ident, - }), - ItemStatus::NotPresent => None, - }, - None => { - // If there is no chain of variant actions, the variant is not - // versioned and code generation is straight forward. - // Unversioned variants are always included in versioned enums. - let variant_ident = &self.inner.ident; - - Some(quote! { - #variant_ident, - }) - } + .get_ident(), + None => Some(self.inner.ident()), } } +} - pub(crate) fn generate_for_from_impl( - &self, - module_name: &Ident, - next_module_name: &Ident, - version: &ContainerVersion, - next_version: &ContainerVersion, - enum_ident: &Ident, - ) -> TokenStream { - match &self.chain { - Some(chain) => match ( - chain.get_expect(&version.inner), - chain.get_expect(&next_version.inner), - ) { - (_, ItemStatus::Added { .. }) => quote! {}, - (old, next) => { - let old_variant_ident = old - .get_ident() - .expect("internal error: old variant must have a name"); - let next_variant_ident = next - .get_ident() - .expect("internal error: next variant must have a name"); - - quote! { - #module_name::#enum_ident::#old_variant_ident => #next_module_name::#enum_ident::#next_variant_ident, - } - } - }, - None => { - let variant_ident = &self.inner.ident; - - quote! { - #module_name::#enum_ident::#variant_ident => #next_module_name::#enum_ident::#variant_ident, - } - } - } - } +#[derive(Debug)] +pub(crate) enum ItemStatus { + Added { + ident: Ident, + default_fn: Path, + }, + Renamed { + from: Ident, + to: Ident, + }, + Deprecated { + previous_ident: Ident, + ident: Ident, + note: String, + }, + NoChange(Ident), + NotPresent, +} - pub(crate) fn get_ident(&self, version: &ContainerVersion) -> Option<&syn::Ident> { - match &self.chain { - Some(chain) => chain - .get(&version.inner) - .expect("internal error: chain must contain container version") - .get_ident(), - None => Some(&self.inner.ident), +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::NoChange(ident) => Some(ident), + ItemStatus::NotPresent => None, } } } diff --git a/crates/stackable-versioned-macros/src/codegen/common/mod.rs b/crates/stackable-versioned-macros/src/codegen/common/mod.rs new file mode 100644 index 000000000..2067856ac --- /dev/null +++ b/crates/stackable-versioned-macros/src/codegen/common/mod.rs @@ -0,0 +1,67 @@ +use std::collections::BTreeMap; + +use k8s_version::Version; +use proc_macro2::Span; +use quote::format_ident; +use syn::Ident; + +use crate::{ + attrs::common::ContainerAttributes, + consts::{DEPRECATED_FIELD_PREFIX, DEPRECATED_VARIANT_PREFIX}, +}; + +mod container; +mod item; + +pub(crate) use container::*; +pub(crate) use item::*; + +pub(crate) type VersionChain = BTreeMap; + +#[derive(Debug, Clone)] +pub(crate) struct ContainerVersion { + /// Indicates that the container version is deprecated. + pub(crate) deprecated: bool, + + /// Indicates that the generation of `From for NEW` should be skipped. + pub(crate) skip_from: bool, + + /// A validated Kubernetes API version. + pub(crate) inner: Version, + + /// The ident of the container. + pub(crate) ident: Ident, +} + +impl From<&ContainerAttributes> for Vec { + fn from(attributes: &ContainerAttributes) -> Self { + attributes + .versions + .iter() + .map(|v| ContainerVersion { + skip_from: v.skip.as_ref().map_or(false, |s| s.from.is_present()), + ident: Ident::new(&v.name.to_string(), Span::call_site()), + deprecated: v.deprecated.is_present(), + inner: v.name, + }) + .collect() + } +} + +/// Returns the container ident used in [`From`] implementations. +pub(crate) fn format_container_from_ident(ident: &Ident) -> Ident { + format_ident!("__sv_{ident}", ident = ident.to_string().to_lowercase()) +} + +/// Removes the deprecated prefix from field ident. +pub(crate) fn remove_deprecated_field_prefix(ident: &Ident) -> Ident { + remove_ident_prefix(ident, DEPRECATED_FIELD_PREFIX) +} + +pub(crate) fn remove_deprecated_variant_prefix(ident: &Ident) -> Ident { + remove_ident_prefix(ident, DEPRECATED_VARIANT_PREFIX) +} + +pub(crate) fn remove_ident_prefix(ident: &Ident, prefix: &str) -> Ident { + format_ident!("{}", ident.to_string().trim_start_matches(prefix)) +} diff --git a/crates/stackable-versioned-macros/src/gen/mod.rs b/crates/stackable-versioned-macros/src/codegen/mod.rs similarity index 94% rename from crates/stackable-versioned-macros/src/gen/mod.rs rename to crates/stackable-versioned-macros/src/codegen/mod.rs index ea069d445..da964e988 100644 --- a/crates/stackable-versioned-macros/src/gen/mod.rs +++ b/crates/stackable-versioned-macros/src/codegen/mod.rs @@ -3,7 +3,7 @@ use syn::{spanned::Spanned, Data, DeriveInput, Error, Result}; use crate::{ attrs::common::ContainerAttributes, - gen::{common::Container, venum::VersionedEnum, vstruct::VersionedStruct}, + codegen::{common::Container, venum::VersionedEnum, vstruct::VersionedStruct}, }; pub(crate) mod chain; diff --git a/crates/stackable-versioned-macros/src/gen/venum/mod.rs b/crates/stackable-versioned-macros/src/codegen/venum/mod.rs similarity index 93% rename from crates/stackable-versioned-macros/src/gen/venum/mod.rs rename to crates/stackable-versioned-macros/src/codegen/venum/mod.rs index 3d417ecef..711a13148 100644 --- a/crates/stackable-versioned-macros/src/gen/venum/mod.rs +++ b/crates/stackable-versioned-macros/src/codegen/venum/mod.rs @@ -7,14 +7,20 @@ use syn::{DataEnum, Error, Ident}; use crate::{ attrs::common::ContainerAttributes, - gen::{ - common::{format_container_from_ident, Container, ContainerVersion, VersionedContainer}, + codegen::{ + common::{ + format_container_from_ident, Container, ContainerVersion, Item, VersionedContainer, + }, venum::variant::VersionedVariant, }, }; -mod variant; +pub(crate) mod variant; +/// Stores individual versions of a single enum. Each version tracks variant +/// actions, which describe if the variant was added, renamed or deprecated in +/// that version. Variants which are not versioned, are included in every +/// version of the struct. #[derive(Debug)] pub(crate) struct VersionedEnum(VersionedContainer); diff --git a/crates/stackable-versioned-macros/src/codegen/venum/variant.rs b/crates/stackable-versioned-macros/src/codegen/venum/variant.rs new file mode 100644 index 000000000..04979f37e --- /dev/null +++ b/crates/stackable-versioned-macros/src/codegen/venum/variant.rs @@ -0,0 +1,164 @@ +use std::ops::{Deref, DerefMut}; + +use darling::FromVariant; +use proc_macro2::TokenStream; +use quote::quote; +use syn::{Ident, Variant}; + +use crate::{ + attrs::{ + common::{ContainerAttributes, ItemAttributes}, + variant::VariantAttributes, + }, + codegen::{ + chain::BTreeMapExt, + common::{ + remove_deprecated_variant_prefix, Attributes, ContainerVersion, Item, ItemStatus, + Named, VersionedItem, + }, + }, +}; + +#[derive(Debug)] +pub(crate) struct VersionedVariant(VersionedItem); + +impl Deref for VersionedVariant { + type Target = VersionedItem; + + fn deref(&self) -> &Self::Target { + &self.0 + } +} + +impl DerefMut for VersionedVariant { + fn deref_mut(&mut self) -> &mut Self::Target { + &mut self.0 + } +} + +impl TryFrom<&Variant> for VariantAttributes { + type Error = darling::Error; + + fn try_from(variant: &Variant) -> Result { + Self::from_variant(variant) + } +} + +impl Attributes for VariantAttributes { + fn common_attrs_owned(self) -> ItemAttributes { + self.common + } + + fn common_attrs(&self) -> &ItemAttributes { + &self.common + } +} + +impl Named for Variant { + fn cleaned_ident(&self) -> Ident { + let ident = self.ident(); + remove_deprecated_variant_prefix(ident) + } + + fn ident(&self) -> &Ident { + &self.ident + } +} + +// TODO (@Techassi): Figure out a way to be able to only write the following code +// once for both a versioned field and variant, because the are practically +// identical. + +impl VersionedVariant { + pub(crate) fn new( + variant: Variant, + container_attributes: &ContainerAttributes, + ) -> syn::Result { + let item = VersionedItem::<_, VariantAttributes>::new(variant, container_attributes)?; + Ok(Self(item)) + } + + pub(crate) fn generate_for_container( + &self, + container_version: &ContainerVersion, + ) -> Option { + match &self.chain { + Some(chain) => match chain + .get(&container_version.inner) + .expect("internal error: chain must contain container version") + { + ItemStatus::Added { ident, .. } => Some(quote! { + #ident, + }), + ItemStatus::Renamed { to, .. } => Some(quote! { + #to, + }), + ItemStatus::Deprecated { ident, .. } => Some(quote! { + #[deprecated] + #ident, + }), + ItemStatus::NoChange(ident) => Some(quote! { + #ident, + }), + ItemStatus::NotPresent => None, + }, + None => { + // If there is no chain of variant actions, the variant is not + // versioned and code generation is straight forward. + // Unversioned variants are always included in versioned enums. + let variant_ident = &self.inner.ident; + + Some(quote! { + #variant_ident, + }) + } + } + } + + pub(crate) fn generate_for_from_impl( + &self, + module_name: &Ident, + next_module_name: &Ident, + version: &ContainerVersion, + next_version: &ContainerVersion, + enum_ident: &Ident, + ) -> TokenStream { + match &self.chain { + Some(chain) => match ( + chain.get_expect(&version.inner), + chain.get_expect(&next_version.inner), + ) { + (_, ItemStatus::Added { .. }) => quote! {}, + (old, next) => { + let old_variant_ident = old + .get_ident() + .expect("internal error: old variant must have a name"); + let next_variant_ident = next + .get_ident() + .expect("internal error: next variant must have a name"); + + quote! { + #module_name::#enum_ident::#old_variant_ident => #next_module_name::#enum_ident::#next_variant_ident, + } + } + }, + None => { + let variant_ident = &self.inner.ident; + + quote! { + #module_name::#enum_ident::#variant_ident => #next_module_name::#enum_ident::#variant_ident, + } + } + } + } + + pub(crate) fn get_ident(&self, version: &ContainerVersion) -> Option<&syn::Ident> { + match &self.chain { + Some(chain) => chain + .get(&version.inner) + .expect("internal error: chain must contain container version") + .get_ident(), + None => Some(&self.inner.ident), + } + } +} diff --git a/crates/stackable-versioned-macros/src/codegen/vstruct/field.rs b/crates/stackable-versioned-macros/src/codegen/vstruct/field.rs new file mode 100644 index 000000000..fb6dd86b4 --- /dev/null +++ b/crates/stackable-versioned-macros/src/codegen/vstruct/field.rs @@ -0,0 +1,178 @@ +use std::ops::{Deref, DerefMut}; + +use darling::FromField; +use proc_macro2::TokenStream; +use quote::quote; +use syn::{Field, Ident}; + +use crate::{ + attrs::{ + common::{ContainerAttributes, ItemAttributes}, + field::FieldAttributes, + }, + codegen::common::{ + remove_deprecated_field_prefix, Attributes, ContainerVersion, Item, ItemStatus, Named, + VersionedItem, + }, +}; + +/// A versioned field, which contains contains common [`Field`] data and a chain +/// of actions. +/// +/// The chain of action maps versions to an action and the appropriate field +/// name. Additionally, the [`Field`] data can be used to forward attributes, +/// generate documentation, etc. +#[derive(Debug)] +pub(crate) struct VersionedField(VersionedItem); + +impl Deref for VersionedField { + type Target = VersionedItem; + + fn deref(&self) -> &Self::Target { + &self.0 + } +} + +impl DerefMut for VersionedField { + fn deref_mut(&mut self) -> &mut Self::Target { + &mut self.0 + } +} + +impl TryFrom<&Field> for FieldAttributes { + type Error = darling::Error; + + fn try_from(field: &Field) -> Result { + Self::from_field(field) + } +} + +impl Attributes for FieldAttributes { + fn common_attrs_owned(self) -> ItemAttributes { + self.common + } + + fn common_attrs(&self) -> &ItemAttributes { + &self.common + } +} + +impl Named for Field { + fn cleaned_ident(&self) -> Ident { + let ident = self.ident(); + remove_deprecated_field_prefix(ident) + } + + fn ident(&self) -> &Ident { + self.ident + .as_ref() + .expect("internal error: field must have an ident") + } +} + +impl VersionedField { + pub(crate) fn new( + field: Field, + container_attributes: &ContainerAttributes, + ) -> syn::Result { + let item = VersionedItem::<_, FieldAttributes>::new(field, container_attributes)?; + Ok(Self(item)) + } + + pub(crate) fn generate_for_container( + &self, + container_version: &ContainerVersion, + ) -> Option { + match &self.chain { + Some(chain) => { + // Check if the provided container version is present in the map + // of actions. If it is, some action occurred in exactly that + // version and thus code is generated for that field based on + // the type of action. + // If not, the provided version has no action attached to it. + // The code generation then depends on the relation to other + // versions (with actions). + + let field_type = &self.inner.ty; + + match chain + .get(&container_version.inner) + .expect("internal error: chain must contain container version") + { + ItemStatus::Added { ident, .. } => Some(quote! { + pub #ident: #field_type, + }), + ItemStatus::Renamed { to, .. } => Some(quote! { + pub #to: #field_type, + }), + ItemStatus::Deprecated { + ident: field_ident, + note, + .. + } => Some(quote! { + #[deprecated = #note] + pub #field_ident: #field_type, + }), + ItemStatus::NotPresent => None, + ItemStatus::NoChange(field_ident) => Some(quote! { + pub #field_ident: #field_type, + }), + } + } + None => { + // If there is no chain of field actions, the field is not + // versioned and code generation is straight forward. + // Unversioned fields are always included in versioned structs. + let field_ident = &self.inner.ident; + let field_type = &self.inner.ty; + + Some(quote! { + pub #field_ident: #field_type, + }) + } + } + } + + pub(crate) fn generate_for_from_impl( + &self, + version: &ContainerVersion, + next_version: &ContainerVersion, + from_ident: &Ident, + ) -> TokenStream { + match &self.chain { + Some(chain) => { + match ( + chain + .get(&version.inner) + .expect("internal error: chain must contain container version"), + chain + .get(&next_version.inner) + .expect("internal error: chain must contain container version"), + ) { + (_, ItemStatus::Added { ident, default_fn }) => quote! { + #ident: #default_fn(), + }, + (old, next) => { + let old_field_ident = old + .get_ident() + .expect("internal error: old field must have a name"); + + let next_field_ident = next + .get_ident() + .expect("internal error: new field must have a name"); + + quote! { + #next_field_ident: #from_ident.#old_field_ident, + } + } + } + } + None => { + let field_ident = &self.inner.ident; + quote! { + #field_ident: #from_ident.#field_ident, + } + } + } + } +} diff --git a/crates/stackable-versioned-macros/src/gen/vstruct/mod.rs b/crates/stackable-versioned-macros/src/codegen/vstruct/mod.rs similarity index 97% rename from crates/stackable-versioned-macros/src/gen/vstruct/mod.rs rename to crates/stackable-versioned-macros/src/codegen/vstruct/mod.rs index 6bbf6b98e..b0fefaa5d 100644 --- a/crates/stackable-versioned-macros/src/gen/vstruct/mod.rs +++ b/crates/stackable-versioned-macros/src/codegen/vstruct/mod.rs @@ -7,13 +7,15 @@ use syn::{DataStruct, Error, Ident}; use crate::{ attrs::common::ContainerAttributes, - gen::{ - common::{format_container_from_ident, Container, ContainerVersion, VersionedContainer}, + codegen::{ + common::{ + format_container_from_ident, Container, ContainerVersion, Item, VersionedContainer, + }, vstruct::field::VersionedField, }, }; -mod field; +pub(crate) mod field; /// Stores individual versions of a single struct. Each version tracks field /// actions, which describe if the field was added, renamed or deprecated in diff --git a/crates/stackable-versioned-macros/src/gen/common.rs b/crates/stackable-versioned-macros/src/gen/common.rs deleted file mode 100644 index 298b85c11..000000000 --- a/crates/stackable-versioned-macros/src/gen/common.rs +++ /dev/null @@ -1,110 +0,0 @@ -use std::{collections::BTreeMap, ops::Deref}; - -use k8s_version::Version; -use proc_macro2::{Span, TokenStream}; -use quote::format_ident; -use syn::{Ident, Path}; - -use crate::{ - attrs::common::ContainerAttributes, - consts::{DEPRECATED_FIELD_PREFIX, DEPRECATED_VARIANT_PREFIX}, -}; - -pub(crate) type VersionChain = BTreeMap; - -#[derive(Debug, Clone)] -pub(crate) struct ContainerVersion { - pub(crate) deprecated: bool, - pub(crate) skip_from: bool, - pub(crate) inner: Version, - pub(crate) ident: Ident, -} - -impl From<&ContainerAttributes> for Vec { - fn from(attributes: &ContainerAttributes) -> Self { - attributes - .versions - .iter() - .map(|v| ContainerVersion { - skip_from: v.skip.as_ref().map_or(false, |s| s.from.is_present()), - ident: Ident::new(&v.name.to_string(), Span::call_site()), - deprecated: v.deprecated.is_present(), - inner: v.name, - }) - .collect() - } -} - -pub(crate) trait Container -where - Self: Sized + Deref>, -{ - fn new(ident: Ident, data: D, attributes: ContainerAttributes) -> syn::Result; - - /// This generates the complete code for a single versioned container. - /// - /// Internally, it will create a module for each declared version which - /// contains the container with the appropriate items (fields or variants) - /// Additionally, it generates `From` implementations, which enable - /// conversion from an older to a newer version. - fn generate_tokens(&self) -> TokenStream; -} - -#[derive(Debug)] -pub(crate) struct VersionedContainer { - pub(crate) versions: Vec, - pub(crate) items: Vec, - pub(crate) ident: Ident, - - pub(crate) from_ident: Ident, - pub(crate) skip_from: bool, -} - -#[derive(Debug)] -pub(crate) enum ItemStatus { - Added { - ident: Ident, - default_fn: Path, - }, - Renamed { - from: Ident, - to: Ident, - }, - Deprecated { - previous_ident: Ident, - ident: Ident, - note: String, - }, - NoChange(Ident), - NotPresent, -} - -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::NoChange(ident) => Some(ident), - ItemStatus::NotPresent => None, - } - } -} - -/// Returns the container ident used in [`From`] implementations. -pub(crate) fn format_container_from_ident(ident: &Ident) -> Ident { - format_ident!("__sv_{ident}", ident = ident.to_string().to_lowercase()) -} - -/// Removes the deprecated prefix from field ident. -pub(crate) fn remove_deprecated_field_prefix(ident: &Ident) -> Ident { - remove_ident_prefix(ident, DEPRECATED_FIELD_PREFIX) -} - -pub(crate) fn remove_deprecated_variant_prefix(ident: &Ident) -> Ident { - remove_ident_prefix(ident, DEPRECATED_VARIANT_PREFIX) -} - -pub(crate) fn remove_ident_prefix(ident: &Ident, prefix: &str) -> Ident { - format_ident!("{}", ident.to_string().trim_start_matches(prefix)) -} diff --git a/crates/stackable-versioned-macros/src/gen/vstruct/field.rs b/crates/stackable-versioned-macros/src/gen/vstruct/field.rs deleted file mode 100644 index c3b77a7e7..000000000 --- a/crates/stackable-versioned-macros/src/gen/vstruct/field.rs +++ /dev/null @@ -1,327 +0,0 @@ -use std::{collections::BTreeMap, ops::Deref}; - -use darling::FromField; -use proc_macro2::TokenStream; -use quote::{format_ident, quote}; -use syn::{Field, Ident}; - -use crate::{ - attrs::{common::ContainerAttributes, field::FieldAttributes}, - gen::{ - chain::Neighbors, - common::{remove_deprecated_field_prefix, ContainerVersion, ItemStatus, VersionChain}, - }, -}; - -/// A versioned field, which contains contains common [`Field`] data and a chain -/// of actions. -/// -/// The chain of action maps versions to an action and the appropriate field -/// name. Additionally, the [`Field`] data can be used to forward attributes, -/// generate documentation, etc. -#[derive(Debug)] -pub(crate) struct VersionedField { - pub(crate) chain: Option, - pub(crate) inner: Field, -} - -// TODO (@Techassi): Figure out a way to be able to only write the following code -// once for both a versioned field and variant, because the are practically -// identical. - -impl VersionedField { - /// Create a new versioned field (of a versioned struct) by consuming the - /// parsed [Field] and validating the versions of field actions against - /// versions attached on the container. - pub(crate) fn new(field: Field, container_attrs: &ContainerAttributes) -> syn::Result { - let field_attrs = FieldAttributes::from_field(&field)?; - field_attrs.validate_versions(container_attrs, &field)?; - - // Constructing the action chain requires going through the actions from - // the end, because the base struct always represents the latest (most - // up-to-date) version of that struct. That's why the following code - // needs to go through the actions in reverse order, as otherwise it is - // impossible to extract the field ident for each version. - - // Deprecating a field is always the last state a field can end up in. For - // fields which are not deprecated, the last change is either the latest - // rename or addition, which is handled below. - // The ident of the deprecated field is guaranteed to include the - // 'deprecated_' prefix. The ident can thus be used as is. - if let Some(deprecated) = field_attrs.common.deprecated { - let deprecated_ident = field - .ident - .as_ref() - .expect("internal error: field must have an ident"); - - // When the field is deprecated, any rename which occurred beforehand - // requires access to the field ident to infer the field ident for - // the latest rename. - let mut ident = remove_deprecated_field_prefix(deprecated_ident); - let mut actions = BTreeMap::new(); - - actions.insert( - *deprecated.since, - ItemStatus::Deprecated { - previous_ident: ident.clone(), - ident: deprecated_ident.clone(), - note: deprecated.note.to_string(), - }, - ); - - for rename in field_attrs.common.renames.iter().rev() { - let from = format_ident!("{from}", from = *rename.from); - actions.insert( - *rename.since, - ItemStatus::Renamed { - from: from.clone(), - to: ident, - }, - ); - ident = from; - } - - // After the last iteration above (if any) we use the ident for the - // added action if there is any. - if let Some(added) = field_attrs.common.added { - actions.insert( - *added.since, - ItemStatus::Added { - default_fn: added.default_fn.deref().clone(), - ident, - }, - ); - } - - Ok(Self { - chain: Some(actions), - inner: field, - }) - } else if !field_attrs.common.renames.is_empty() { - let mut actions = BTreeMap::new(); - let mut ident = field - .ident - .clone() - .expect("internal error: field must have an ident"); - - for rename in field_attrs.common.renames.iter().rev() { - let from = format_ident!("{from}", from = *rename.from); - actions.insert( - *rename.since, - ItemStatus::Renamed { - from: from.clone(), - to: ident, - }, - ); - ident = from; - } - - // After the last iteration above (if any) we use the ident for the - // added action if there is any. - if let Some(added) = field_attrs.common.added { - actions.insert( - *added.since, - ItemStatus::Added { - default_fn: added.default_fn.deref().clone(), - ident, - }, - ); - } - - Ok(Self { - chain: Some(actions), - inner: field, - }) - } else { - if let Some(added) = field_attrs.common.added { - let mut actions = BTreeMap::new(); - - actions.insert( - *added.since, - ItemStatus::Added { - default_fn: added.default_fn.deref().clone(), - ident: field - .ident - .clone() - .expect("internal error: field must have a name"), - }, - ); - - return Ok(Self { - chain: Some(actions), - inner: field, - }); - } - - Ok(Self { - chain: None, - inner: field, - }) - } - } - - /// Inserts container versions not yet present in the status chain. - /// - /// When initially creating a new versioned item, the code doesn't have - /// access to the versions defined on the container. This function inserts - /// all non-present container versions and decides which status and ident - /// is the right fit based on the status neighbors. - /// - /// This continuous chain ensures that when generating code (tokens), each - /// field can lookup the status (and ident) for a requested version. - pub(crate) fn insert_container_versions(&mut self, versions: &[ContainerVersion]) { - if let Some(chain) = &mut self.chain { - for version in versions { - if chain.contains_key(&version.inner) { - continue; - } - - match chain.get_neighbors(&version.inner) { - (None, Some(status)) => match status { - ItemStatus::Added { .. } => { - chain.insert(version.inner, ItemStatus::NotPresent) - } - ItemStatus::Renamed { from, .. } => { - chain.insert(version.inner, ItemStatus::NoChange(from.clone())) - } - ItemStatus::Deprecated { previous_ident, .. } => chain - .insert(version.inner, ItemStatus::NoChange(previous_ident.clone())), - ItemStatus::NoChange(ident) => { - chain.insert(version.inner, ItemStatus::NoChange(ident.clone())) - } - ItemStatus::NotPresent => unreachable!(), - }, - (Some(status), None) => { - let ident = match status { - ItemStatus::Added { ident, .. } => ident, - ItemStatus::Renamed { to, .. } => to, - ItemStatus::Deprecated { ident, .. } => ident, - ItemStatus::NoChange(ident) => ident, - ItemStatus::NotPresent => unreachable!(), - }; - - chain.insert(version.inner, ItemStatus::NoChange(ident.clone())) - } - (Some(status), Some(_)) => { - let ident = match status { - ItemStatus::Added { ident, .. } => ident, - ItemStatus::Renamed { to, .. } => to, - ItemStatus::NoChange(ident) => ident, - _ => unreachable!(), - }; - - chain.insert(version.inner, ItemStatus::NoChange(ident.clone())) - } - _ => unreachable!(), - }; - } - } - } - - pub(crate) fn get_ident(&self, version: &ContainerVersion) -> Option<&Ident> { - match &self.chain { - Some(chain) => chain - .get(&version.inner) - .expect("internal error: chain must contain container version") - .get_ident(), - None => self.inner.ident.as_ref(), - } - } - - pub(crate) fn generate_for_container( - &self, - container_version: &ContainerVersion, - ) -> Option { - match &self.chain { - Some(chain) => { - // Check if the provided container version is present in the map - // of actions. If it is, some action occurred in exactly that - // version and thus code is generated for that field based on - // the type of action. - // If not, the provided version has no action attached to it. - // The code generation then depends on the relation to other - // versions (with actions). - - let field_type = &self.inner.ty; - - match chain - .get(&container_version.inner) - .expect("internal error: chain must contain container version") - { - ItemStatus::Added { ident, .. } => Some(quote! { - pub #ident: #field_type, - }), - ItemStatus::Renamed { to, .. } => Some(quote! { - pub #to: #field_type, - }), - ItemStatus::Deprecated { - ident: field_ident, - note, - .. - } => Some(quote! { - #[deprecated = #note] - pub #field_ident: #field_type, - }), - ItemStatus::NotPresent => None, - ItemStatus::NoChange(field_ident) => Some(quote! { - pub #field_ident: #field_type, - }), - } - } - None => { - // If there is no chain of field actions, the field is not - // versioned and code generation is straight forward. - // Unversioned fields are always included in versioned structs. - let field_ident = &self.inner.ident; - let field_type = &self.inner.ty; - - Some(quote! { - pub #field_ident: #field_type, - }) - } - } - } - - pub(crate) fn generate_for_from_impl( - &self, - version: &ContainerVersion, - next_version: &ContainerVersion, - from_ident: &Ident, - ) -> TokenStream { - match &self.chain { - Some(chain) => { - match ( - chain - .get(&version.inner) - .expect("internal error: chain must contain container version"), - chain - .get(&next_version.inner) - .expect("internal error: chain must contain container version"), - ) { - (_, ItemStatus::Added { ident, default_fn }) => quote! { - #ident: #default_fn(), - }, - (old, next) => { - let old_field_ident = old - .get_ident() - .expect("internal error: old field must have a name"); - - let next_field_ident = next - .get_ident() - .expect("internal error: new field must have a name"); - - quote! { - #next_field_ident: #from_ident.#old_field_ident, - } - } - } - } - None => { - let field_ident = &self.inner.ident; - quote! { - #field_ident: #from_ident.#field_ident, - } - } - } - } -} diff --git a/crates/stackable-versioned-macros/src/lib.rs b/crates/stackable-versioned-macros/src/lib.rs index 44a8b7e51..479eab5e2 100644 --- a/crates/stackable-versioned-macros/src/lib.rs +++ b/crates/stackable-versioned-macros/src/lib.rs @@ -5,8 +5,8 @@ use syn::{DeriveInput, Error}; use crate::attrs::common::ContainerAttributes; mod attrs; +mod codegen; mod consts; -mod gen; /// This macro enables generating versioned structs. /// @@ -223,7 +223,7 @@ pub fn versioned(attrs: TokenStream, input: TokenStream) -> TokenStream { // Module(ItemMod). let input = syn::parse_macro_input!(input as DeriveInput); - gen::expand(attrs, input) + codegen::expand(attrs, input) .unwrap_or_else(Error::into_compile_error) .into() } From 709e6f62c62c30b9df06229ec7eae5f2d277154c Mon Sep 17 00:00:00 2001 From: Techassi Date: Fri, 16 Aug 2024 16:27:58 +0200 Subject: [PATCH 5/7] docs: Update field and variant attribute docs --- .../src/attrs/common/item.rs | 19 +++++++++++++ .../src/attrs/field.rs | 27 +++++++------------ .../src/attrs/variant.rs | 21 +++++++++------ 3 files changed, 41 insertions(+), 26 deletions(-) diff --git a/crates/stackable-versioned-macros/src/attrs/common/item.rs b/crates/stackable-versioned-macros/src/attrs/common/item.rs index b3a83252a..899976f4f 100644 --- a/crates/stackable-versioned-macros/src/attrs/common/item.rs +++ b/crates/stackable-versioned-macros/src/attrs/common/item.rs @@ -9,6 +9,12 @@ use crate::{ consts::{DEPRECATED_FIELD_PREFIX, DEPRECATED_VARIANT_PREFIX}, }; +/// This trait helps to unify attribute validation for both field and variant +/// attributes. +/// +/// This trait is implemented using a blanket implementation on types +/// `T: Attributes`. The [`Attributes`] trait allows access to the common +/// attributes shared across field and variant attributes. pub(crate) trait ValidateVersions where I: Spanned, @@ -79,6 +85,10 @@ where } } +// NOTE (@Techassi): It might be possible (but is it required) to move this +// functionality into a shared trait, which knows what type of item 'Self' is. + +/// This enum is used to run different validation based on the type of item. #[derive(Debug, strum::Display)] #[strum(serialize_all = "lowercase")] pub(crate) enum ItemType { @@ -89,6 +99,15 @@ pub(crate) enum ItemType { /// 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. +/// +/// ### 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 +/// [`Vec`]. +/// - An item can only be deprecated once. A field not marked as 'deprecated' +/// will be included up until the latest version. #[derive(Debug, FromMeta)] pub(crate) struct ItemAttributes { /// This parses the `added` attribute on items (fields or variants). It can diff --git a/crates/stackable-versioned-macros/src/attrs/field.rs b/crates/stackable-versioned-macros/src/attrs/field.rs index 90d6e79eb..0f1e017b4 100644 --- a/crates/stackable-versioned-macros/src/attrs/field.rs +++ b/crates/stackable-versioned-macros/src/attrs/field.rs @@ -9,17 +9,13 @@ use crate::attrs::common::{ItemAttributes, ItemType}; /// Data stored in this struct is validated using darling's `and_then` attribute. /// During darlings validation, it is not possible to validate that action /// versions match up with declared versions on the container. This validation -/// can be done using the associated [`FieldAttributes::validate_versions`] +/// can be done using the associated [`ValidateVersions::validate_versions`][1] /// function. /// -/// ### Field Rules +/// Rules shared across fields and variants can be found [here][2]. /// -/// - A field can only ever be added once at most. A field not marked as 'added' -/// is part of the struct in every version until renamed or deprecated. -/// - A field can be renamed many times. That's why renames are stored in a -/// [`Vec`]. -/// - A field can only be deprecated once. A field not marked as 'deprecated' -/// will be included up until the latest version. +/// [1]: crate::attrs::common::ValidateVersions::validate_versions +/// [2]: crate::attrs::common::ItemAttributes #[derive(Debug, FromField)] #[darling( attributes(versioned), @@ -37,14 +33,6 @@ pub(crate) struct FieldAttributes { } impl FieldAttributes { - // NOTE (@Techassi): Ideally, these validations should be moved to the - // ItemAttributes impl, because common validation like action combinations - // and action order can be validated without taking the type of attribute - // into account (field vs variant). However, we would loose access to the - // field / variant ident and as such, cannot display the error directly on - // the affected field / variant. This is a significant decrease in DX. - // See https://github.com/TedDriggs/darling/discussions/294 - /// This associated function is called by darling (see and_then attribute) /// after it successfully parsed the attribute. This allows custom /// validation of the attribute which extends the validation already in @@ -52,8 +40,11 @@ impl FieldAttributes { /// /// Internally, it calls out to other specialized validation functions. fn validate(self) -> Result { - self.common - .validate(self.ident.as_ref().unwrap(), &ItemType::Field)?; + let ident = self + .ident + .as_ref() + .expect("internal error: field must have an ident"); + self.common.validate(ident, &ItemType::Field)?; Ok(self) } diff --git a/crates/stackable-versioned-macros/src/attrs/variant.rs b/crates/stackable-versioned-macros/src/attrs/variant.rs index d4ff8af3a..182d9f478 100644 --- a/crates/stackable-versioned-macros/src/attrs/variant.rs +++ b/crates/stackable-versioned-macros/src/attrs/variant.rs @@ -4,6 +4,19 @@ use syn::Ident; use crate::attrs::common::{ItemAttributes, ItemType}; +/// This struct describes all available variant attributes, as well as the +/// variant name to display better diagnostics. +/// +/// Data stored in this struct is validated using darling's `and_then` attribute. +/// During darlings validation, it is not possible to validate that action +/// versions match up with declared versions on the container. This validation +/// can be done using the associated [`FieldAttributes::validate_versions`][1] +/// function. +/// +/// Rules shared across fields and variants can be found [here][2]. +/// +/// [1]: crate::attrs::common::ValidateVersions::validate_versions +/// [2]: crate::attrs::common::ItemAttributes #[derive(Debug, FromVariant)] #[darling( attributes(versioned), @@ -21,14 +34,6 @@ pub(crate) struct VariantAttributes { } impl VariantAttributes { - // NOTE (@Techassi): Ideally, these validations should be moved to the - // ItemAttributes impl, because common validation like action combinations - // and action order can be validated without taking the type of attribute - // into account (field vs variant). However, we would loose access to the - // field / variant ident and as such, cannot display the error directly on - // the affected field / variant. This is a significant decrease in DX. - // See https://github.com/TedDriggs/darling/discussions/294 - /// This associated function is called by darling (see and_then attribute) /// after it successfully parsed the attribute. This allows custom /// validation of the attribute which extends the validation already in From 6afcd0e928909876ba06f836dbdd1a28e5be8629 Mon Sep 17 00:00:00 2001 From: Techassi Date: Mon, 19 Aug 2024 11:02:01 +0200 Subject: [PATCH 6/7] docs: Add doc and developer comments --- .../src/codegen/chain.rs | 9 +++++ .../src/codegen/common/container.rs | 12 ++++++ .../src/codegen/common/item.rs | 29 ++++++++++++++ .../src/codegen/common/mod.rs | 10 ++++- .../src/codegen/venum/mod.rs | 6 +-- .../src/codegen/venum/variant.rs | 40 +++++++++---------- .../src/codegen/vstruct/field.rs | 17 ++++++-- 7 files changed, 95 insertions(+), 28 deletions(-) diff --git a/crates/stackable-versioned-macros/src/codegen/chain.rs b/crates/stackable-versioned-macros/src/codegen/chain.rs index 47675bcb6..94a7ec955 100644 --- a/crates/stackable-versioned-macros/src/codegen/chain.rs +++ b/crates/stackable-versioned-macros/src/codegen/chain.rs @@ -14,6 +14,15 @@ impl Neighbors for BTreeMap where K: Ord + Eq, { + /// Returns the values of keys which are neighbors of `key`. + /// + /// Imagine 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 diff --git a/crates/stackable-versioned-macros/src/codegen/common/container.rs b/crates/stackable-versioned-macros/src/codegen/common/container.rs index 4c50a87bc..0e211897a 100644 --- a/crates/stackable-versioned-macros/src/codegen/common/container.rs +++ b/crates/stackable-versioned-macros/src/codegen/common/container.rs @@ -5,10 +5,22 @@ use syn::Ident; use crate::{attrs::common::ContainerAttributes, codegen::common::ContainerVersion}; +/// This trait helps to unify versioned containers, like structs and enums. +/// +/// This trait is implemented by wrapper structs, which wrap the generic +/// [`VersionedContainer`] struct. The generic type parameter `D` describes the +/// kind of data, like [`DataStruct`](syn::DataStruct) in case of a struct and +/// [`DataEnum`](syn::DataEnum) in case of an enum. +/// The type parameter `I` describes the type of the versioned items, like +/// [`VersionedField`][1] and [`VersionedVariant`][2]. +/// +/// [1]: crate::codegen::vstruct::field::VersionedField +/// [2]: crate::codegen::venum::variant::VersionedVariant pub(crate) trait Container where Self: Sized + Deref>, { + /// Creates a new versioned container. fn new(ident: Ident, data: D, attributes: ContainerAttributes) -> syn::Result; /// This generates the complete code for a single versioned container. diff --git a/crates/stackable-versioned-macros/src/codegen/common/item.rs b/crates/stackable-versioned-macros/src/codegen/common/item.rs index 68cb9959a..ad2d5a088 100644 --- a/crates/stackable-versioned-macros/src/codegen/common/item.rs +++ b/crates/stackable-versioned-macros/src/codegen/common/item.rs @@ -43,16 +43,40 @@ where fn get_ident(&self, version: &ContainerVersion) -> Option<&Ident>; } +/// This trait enables access to the ident of named items, like fields and +/// variants. +/// +/// It additionally provides a function to retrieve the cleaned ident, which +/// removes the deprecation prefixes. pub(crate) trait Named { fn cleaned_ident(&self) -> Ident; fn ident(&self) -> &Ident; } +/// This trait enables access to the common attributes across field and variant +/// attributes. pub(crate) trait Attributes { fn common_attrs_owned(self) -> ItemAttributes; fn common_attrs(&self) -> &ItemAttributes; } +/// This struct combines common common code for versioned fields and variants. +/// +/// Most of the initial creation of a versioned field and variant are identical. +/// Currently, the following steps are unified: +/// +/// - Initial creation of the action chain based on item attributes. +/// - Insertion of container versions into the chain. +/// +/// The generic type parameter `I` describes the type of the versioned item, +/// usually [`Field`](syn::Field) or [`Variant`](syn::Variant). The parameter +/// `A` indicates the type of item attributes, usually [`FieldAttributes`][1] or +/// [`VariantAttributes`][2] depending on the used item type. As this type is +/// only needed during creation of [`Self`](VersionedItem), we must use a +/// [`PhantomData`] marker. +/// +/// [1]: crate::attrs::field::FieldAttributes +/// [2]: crate::attrs::variant::VariantAttributes #[derive(Debug)] pub(crate) struct VersionedItem where @@ -71,6 +95,11 @@ where I: Named + Spanned, { fn new(item: I, container_attrs: &ContainerAttributes) -> syn::Result { + // We use the TryFrom trait here, because the type parameter `A` can use + // it as a trait bound. Internally this then calls either `from_field` + // for field attributes or `from_variant` for variant attributes. Sadly + // darling doesn't provide a "generic" trait which abstracts over the + // different `from_` functions. let attrs = A::try_from(&item)?; attrs.validate_versions(container_attrs, &item)?; diff --git a/crates/stackable-versioned-macros/src/codegen/common/mod.rs b/crates/stackable-versioned-macros/src/codegen/common/mod.rs index 2067856ac..93668e768 100644 --- a/crates/stackable-versioned-macros/src/codegen/common/mod.rs +++ b/crates/stackable-versioned-macros/src/codegen/common/mod.rs @@ -16,6 +16,7 @@ mod item; pub(crate) use container::*; pub(crate) use item::*; +/// Type alias to make the type of the version chain easier to handle. pub(crate) type VersionChain = BTreeMap; #[derive(Debug, Clone)] @@ -53,15 +54,22 @@ pub(crate) fn format_container_from_ident(ident: &Ident) -> Ident { format_ident!("__sv_{ident}", ident = ident.to_string().to_lowercase()) } -/// Removes the deprecated prefix from field ident. +/// Removes the deprecated prefix from a field ident. +/// +/// See [`DEPRECATED_FIELD_PREFIX`]. pub(crate) fn remove_deprecated_field_prefix(ident: &Ident) -> Ident { remove_ident_prefix(ident, DEPRECATED_FIELD_PREFIX) } +/// Removes the deprecated prefix from a variant ident. +/// +/// See [`DEPRECATED_VARIANT_PREFIX`]. pub(crate) fn remove_deprecated_variant_prefix(ident: &Ident) -> Ident { remove_ident_prefix(ident, DEPRECATED_VARIANT_PREFIX) } +/// Removes the provided prefix from an ident and returns the newly created +/// ident. pub(crate) fn remove_ident_prefix(ident: &Ident, prefix: &str) -> Ident { format_ident!("{}", ident.to_string().trim_start_matches(prefix)) } diff --git a/crates/stackable-versioned-macros/src/codegen/venum/mod.rs b/crates/stackable-versioned-macros/src/codegen/venum/mod.rs index 711a13148..46db0af71 100644 --- a/crates/stackable-versioned-macros/src/codegen/venum/mod.rs +++ b/crates/stackable-versioned-macros/src/codegen/venum/mod.rs @@ -20,7 +20,7 @@ pub(crate) mod variant; /// Stores individual versions of a single enum. Each version tracks variant /// actions, which describe if the variant was added, renamed or deprecated in /// that version. Variants which are not versioned, are included in every -/// version of the struct. +/// version of the enum. #[derive(Debug)] pub(crate) struct VersionedEnum(VersionedContainer); @@ -61,7 +61,7 @@ impl Container for VersionedEnum { if !items.iter().map(|f| f.get_ident(version)).all_unique() { return Err(Error::new( ident.span(), - format!("struct contains renamed fields which collide with other fields in version {version}", version = version.inner), + format!("Enum contains renamed variants which collide with other variants in version {version}", version = version.inner), )); } } @@ -115,7 +115,7 @@ impl VersionedEnum { .deprecated .then_some(quote! {#[deprecated = #deprecated_note]}); - // Generate tokens for the module and the contained struct + // Generate tokens for the module and the contained enum token_stream.extend(quote! { #[automatically_derived] #deprecated_attr diff --git a/crates/stackable-versioned-macros/src/codegen/venum/variant.rs b/crates/stackable-versioned-macros/src/codegen/venum/variant.rs index 04979f37e..1a559ca99 100644 --- a/crates/stackable-versioned-macros/src/codegen/venum/variant.rs +++ b/crates/stackable-versioned-macros/src/codegen/venum/variant.rs @@ -19,6 +19,12 @@ use crate::{ }, }; +/// A versioned variant, which contains contains common [`Variant`] data and a +/// chain of actions. +/// +/// The chain of action maps versions to an action and the appropriate variant +/// name. Additionally, the [`Variant`] data can be used to forward attributes, +/// generate documentation, etc. #[derive(Debug)] pub(crate) struct VersionedVariant(VersionedItem); @@ -56,8 +62,7 @@ impl Attributes for VariantAttributes { impl Named for Variant { fn cleaned_ident(&self) -> Ident { - let ident = self.ident(); - remove_deprecated_variant_prefix(ident) + remove_deprecated_variant_prefix(self.ident()) } fn ident(&self) -> &Ident { @@ -65,11 +70,11 @@ impl Named for Variant { } } -// TODO (@Techassi): Figure out a way to be able to only write the following code -// once for both a versioned field and variant, because the are practically -// identical. - impl VersionedVariant { + /// Creates a new versioned variant. + /// + /// Internally this calls [`VersionedItem::new`] to handle most of the + /// common creation code. pub(crate) fn new( variant: Variant, container_attributes: &ContainerAttributes, @@ -78,15 +83,19 @@ impl VersionedVariant { Ok(Self(item)) } + /// Generates tokens to be used in a container definition. pub(crate) fn generate_for_container( &self, container_version: &ContainerVersion, ) -> Option { match &self.chain { - Some(chain) => match chain - .get(&container_version.inner) - .expect("internal error: chain must contain container version") - { + // NOTE (@Techassi): https://rust-lang.github.io/rust-clippy/master/index.html#/expect_fun_call + Some(chain) => match chain.get(&container_version.inner).unwrap_or_else(|| { + panic!( + "internal error: chain must contain container version {}", + container_version.inner + ) + }) { ItemStatus::Added { ident, .. } => Some(quote! { #ident, }), @@ -115,6 +124,7 @@ impl VersionedVariant { } } + /// Generates tokens to be used in a [`From`] implementation. pub(crate) fn generate_for_from_impl( &self, module_name: &Ident, @@ -151,14 +161,4 @@ impl VersionedVariant { } } } - - pub(crate) fn get_ident(&self, version: &ContainerVersion) -> Option<&syn::Ident> { - match &self.chain { - Some(chain) => chain - .get(&version.inner) - .expect("internal error: chain must contain container version") - .get_ident(), - None => Some(&self.inner.ident), - } - } } diff --git a/crates/stackable-versioned-macros/src/codegen/vstruct/field.rs b/crates/stackable-versioned-macros/src/codegen/vstruct/field.rs index fb6dd86b4..b84932d7f 100644 --- a/crates/stackable-versioned-macros/src/codegen/vstruct/field.rs +++ b/crates/stackable-versioned-macros/src/codegen/vstruct/field.rs @@ -71,6 +71,10 @@ impl Named for Field { } impl VersionedField { + /// Creates a new versioned field. + /// + /// Internally this calls [`VersionedItem::new`] to handle most of the + /// common creation code. pub(crate) fn new( field: Field, container_attributes: &ContainerAttributes, @@ -79,6 +83,7 @@ impl VersionedField { Ok(Self(item)) } + /// Generates tokens to be used in a container definition. pub(crate) fn generate_for_container( &self, container_version: &ContainerVersion, @@ -95,10 +100,13 @@ impl VersionedField { let field_type = &self.inner.ty; - match chain - .get(&container_version.inner) - .expect("internal error: chain must contain container version") - { + // NOTE (@Techassi): https://rust-lang.github.io/rust-clippy/master/index.html#/expect_fun_call + match chain.get(&container_version.inner).unwrap_or_else(|| { + panic!( + "internal error: chain must contain container version {}", + container_version.inner + ) + }) { ItemStatus::Added { ident, .. } => Some(quote! { pub #ident: #field_type, }), @@ -133,6 +141,7 @@ impl VersionedField { } } + /// Generates tokens to be used in a [`From`] implementation. pub(crate) fn generate_for_from_impl( &self, version: &ContainerVersion, From 005a59bf7d0b0655437db0c5a66de040a19370c2 Mon Sep 17 00:00:00 2001 From: Techassi Date: Mon, 19 Aug 2024 16:10:27 +0200 Subject: [PATCH 7/7] chore: Apply suggestions Co-authored-by: Nick <10092581+NickLarsenNZ@users.noreply.github.com> --- .../src/attrs/common/item.rs | 1 + .../src/attrs/variant.rs | 2 +- .../src/codegen/chain.rs | 2 +- .../src/codegen/common/item.rs | 13 +++++++------ .../src/codegen/vstruct/field.rs | 15 ++++++++------- 5 files changed, 18 insertions(+), 15 deletions(-) diff --git a/crates/stackable-versioned-macros/src/attrs/common/item.rs b/crates/stackable-versioned-macros/src/attrs/common/item.rs index 899976f4f..430c6eb3b 100644 --- a/crates/stackable-versioned-macros/src/attrs/common/item.rs +++ b/crates/stackable-versioned-macros/src/attrs/common/item.rs @@ -39,6 +39,7 @@ where item: &I, ) -> Result<(), darling::Error> { // NOTE (@Techassi): Can we maybe optimize this a little? + let mut errors = Error::accumulator(); if let Some(added) = &self.common_attrs().added { diff --git a/crates/stackable-versioned-macros/src/attrs/variant.rs b/crates/stackable-versioned-macros/src/attrs/variant.rs index 182d9f478..557e082e4 100644 --- a/crates/stackable-versioned-macros/src/attrs/variant.rs +++ b/crates/stackable-versioned-macros/src/attrs/variant.rs @@ -10,7 +10,7 @@ use crate::attrs::common::{ItemAttributes, ItemType}; /// Data stored in this struct is validated using darling's `and_then` attribute. /// During darlings validation, it is not possible to validate that action /// versions match up with declared versions on the container. This validation -/// can be done using the associated [`FieldAttributes::validate_versions`][1] +/// can be done using the associated [`ValidateVersions::validate_versions`][1] /// function. /// /// Rules shared across fields and variants can be found [here][2]. diff --git a/crates/stackable-versioned-macros/src/codegen/chain.rs b/crates/stackable-versioned-macros/src/codegen/chain.rs index 94a7ec955..097214b69 100644 --- a/crates/stackable-versioned-macros/src/codegen/chain.rs +++ b/crates/stackable-versioned-macros/src/codegen/chain.rs @@ -16,7 +16,7 @@ where { /// Returns the values of keys which are neighbors of `key`. /// - /// Imagine a map which contains the following keys: 1, 3, 5. Calling this + /// 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))` diff --git a/crates/stackable-versioned-macros/src/codegen/common/item.rs b/crates/stackable-versioned-macros/src/codegen/common/item.rs index ad2d5a088..347e22c7c 100644 --- a/crates/stackable-versioned-macros/src/codegen/common/item.rs +++ b/crates/stackable-versioned-macros/src/codegen/common/item.rs @@ -11,7 +11,7 @@ use crate::{ }, }; -/// This trait describes versioned container items, fields ans variants in a +/// This trait describes versioned container items, fields and variants in a /// common way. /// /// Shared functionality is implemented in a single place. Code which cannot be @@ -60,7 +60,7 @@ pub(crate) trait Attributes { fn common_attrs(&self) -> &ItemAttributes; } -/// This struct combines common common code for versioned fields and variants. +/// This struct combines common code for versioned fields and variants. /// /// Most of the initial creation of a versioned field and variant are identical. /// Currently, the following steps are unified: @@ -106,10 +106,11 @@ where let item_attrs = attrs.common_attrs_owned(); // Constructing the action chain requires going through the actions - // starting at the end, because the base container always represents the - // latest (most up-to-date) version of that struct. That's why the - // following code needs to go through the actions in reverse order, as - // otherwise it is impossible to extract the item ident for each version. + // starting at the end, because the container definition always + // represents the latest (most up-to-date) version of that struct. + // That's why the following code needs to go through the actions in + // reverse order, as otherwise it is impossible to extract the item + // ident for each version. // 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 diff --git a/crates/stackable-versioned-macros/src/codegen/vstruct/field.rs b/crates/stackable-versioned-macros/src/codegen/vstruct/field.rs index b84932d7f..17bb3eafc 100644 --- a/crates/stackable-versioned-macros/src/codegen/vstruct/field.rs +++ b/crates/stackable-versioned-macros/src/codegen/vstruct/field.rs @@ -16,12 +16,14 @@ use crate::{ }, }; -/// A versioned field, which contains contains common [`Field`] data and a chain -/// of actions. +/// A versioned field, which contains common [`Field`] data and a chain of +/// actions. /// -/// The chain of action maps versions to an action and the appropriate field -/// name. Additionally, the [`Field`] data can be used to forward attributes, -/// generate documentation, etc. +/// The chain of actions maps versions to an action and the appropriate field +/// name. +/// +/// Additionally, the [`Field`] data can be used to forward attributes, generate +/// documentation, etc. #[derive(Debug)] pub(crate) struct VersionedField(VersionedItem); @@ -129,8 +131,7 @@ impl VersionedField { } None => { // If there is no chain of field actions, the field is not - // versioned and code generation is straight forward. - // Unversioned fields are always included in versioned structs. + // versioned and therefore included in all versions. let field_ident = &self.inner.ident; let field_type = &self.inner.ty;