From 3548b01e34c8e1b021b6ac6a2e3b91cc8d9ce2c8 Mon Sep 17 00:00:00 2001 From: Techassi Date: Wed, 22 May 2024 15:17:32 +0200 Subject: [PATCH] feat(stackable-versioned): Improve action chain generation (#784) * feat(stackable-versioned): Improve action chain generation * Add Neighbor trait * Start to implement more robust version lookup * Finish version lookup * Add deprecation note validation * Update PR links in changelog * Fix PR number in changelog link * Adjust TODO about optional deprecation note * Move neighbor code into own file * Add comment on how to expand generated code * Update Cargo.lock file --- Cargo.lock | 1 + crates/stackable-versioned/CHANGELOG.md | 4 + crates/stackable-versioned/Cargo.toml | 3 + crates/stackable-versioned/src/attrs/field.rs | 22 ++- crates/stackable-versioned/src/gen/field.rs | 187 +++++++++--------- crates/stackable-versioned/src/gen/mod.rs | 1 + .../stackable-versioned/src/gen/neighbors.rs | 71 +++++++ crates/stackable-versioned/src/gen/vstruct.rs | 25 +-- crates/stackable-versioned/tests/basic.rs | 9 +- 9 files changed, 210 insertions(+), 113 deletions(-) create mode 100644 crates/stackable-versioned/src/gen/neighbors.rs diff --git a/Cargo.lock b/Cargo.lock index 0a782129d..42e557581 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2918,6 +2918,7 @@ dependencies = [ "k8s-version", "proc-macro2", "quote", + "rstest", "syn 2.0.65", ] diff --git a/crates/stackable-versioned/CHANGELOG.md b/crates/stackable-versioned/CHANGELOG.md index 87d9afb77..fa261cf80 100644 --- a/crates/stackable-versioned/CHANGELOG.md +++ b/crates/stackable-versioned/CHANGELOG.md @@ -4,6 +4,10 @@ All notable changes to this project will be documented in this file. ## [Unreleased] +- Improve action chain generation ([#784]). + +[#784](ttps://github.com/stackabletech/operator-rs/pull/784) + ## [0.1.0] - 2024-05-08 ### Changed diff --git a/crates/stackable-versioned/Cargo.toml b/crates/stackable-versioned/Cargo.toml index f19d35557..d2ff81b73 100644 --- a/crates/stackable-versioned/Cargo.toml +++ b/crates/stackable-versioned/Cargo.toml @@ -16,3 +16,6 @@ darling.workspace = true proc-macro2.workspace = true syn.workspace = true quote.workspace = true + +[dev-dependencies] +rstest.workspace = true diff --git a/crates/stackable-versioned/src/attrs/field.rs b/crates/stackable-versioned/src/attrs/field.rs index c42ad0783..24f65a11e 100644 --- a/crates/stackable-versioned/src/attrs/field.rs +++ b/crates/stackable-versioned/src/attrs/field.rs @@ -51,7 +51,7 @@ pub(crate) struct RenamedAttributes { #[derive(Clone, Debug, FromMeta)] pub(crate) struct DeprecatedAttributes { pub(crate) since: SpannedValue, - pub(crate) _note: SpannedValue, + pub(crate) note: SpannedValue, } impl FieldAttributes { @@ -64,10 +64,14 @@ impl FieldAttributes { 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()); + // Code quality validation + errors.handle(self.validate_deprecated_options()); + // TODO (@Techassi): Add validation for renames so that renamed fields // match up and form a continous chain (eg. foo -> bar -> baz). @@ -191,6 +195,22 @@ impl FieldAttributes { Ok(()) } + fn validate_deprecated_options(&self) -> Result<(), Error> { + // TODO (@Techassi): Make the field 'note' optional, because in the + // future, the macro will generate parts of the deprecation note + // automatically. The user-provided note will then be appended to the + // auto-generated one. + + if let Some(deprecated) = &self.deprecated { + if deprecated.note.is_empty() { + return Err(Error::custom("deprecation note must not be empty") + .with_span(&deprecated.note.span())); + } + } + + 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/src/gen/field.rs b/crates/stackable-versioned/src/gen/field.rs index 99ac9ecba..e6948310b 100644 --- a/crates/stackable-versioned/src/gen/field.rs +++ b/crates/stackable-versioned/src/gen/field.rs @@ -9,7 +9,7 @@ use syn::{Field, Ident}; use crate::{ attrs::field::FieldAttributes, consts::DEPRECATED_PREFIX, - gen::{version::ContainerVersion, ToTokensExt}, + gen::{neighbors::Neighbors, version::ContainerVersion, ToTokensExt}, }; /// A versioned field, which contains contains common [`Field`] data and a chain @@ -36,96 +36,29 @@ impl ToTokensExt for VersionedField { // The code generation then depends on the relation to other // versions (with actions). - // TODO (@Techassi): Make this more robust by also including - // the container versions in the action chain. I'm not happy - // with the follwoing code at all. It serves as a good first - // implementation to get something out of the door. - match chain.get(&container_version.inner) { - Some(action) => match action { - FieldStatus::Added(field_ident) => { - let field_type = &self.inner.ty; - - Some(quote! { - pub #field_ident: #field_type, - }) - } - FieldStatus::Renamed { from: _, to } => { - let field_type = &self.inner.ty; - - Some(quote! { - pub #to: #field_type, - }) - } - FieldStatus::Deprecated(field_ident) => { - let field_type = &self.inner.ty; - - Some(quote! { - #[deprecated] - pub #field_ident: #field_type, - }) - } - }, - None => { - // Generate field if the container version is not - // included in the action chain. First we check the - // earliest field action version. - if let Some((version, action)) = chain.first_key_value() { - if container_version.inner < *version { - match action { - FieldStatus::Added(_) => return None, - FieldStatus::Renamed { from, to: _ } => { - let field_type = &self.inner.ty; - - return Some(quote! { - pub #from: #field_type, - }); - } - FieldStatus::Deprecated(field_ident) => { - let field_type = &self.inner.ty; - - return Some(quote! { - pub #field_ident: #field_type, - }); - } - } - } - } - - // Check the container version against the latest - // field action version. - if let Some((version, action)) = chain.last_key_value() { - if container_version.inner > *version { - match action { - FieldStatus::Added(field_ident) => { - let field_type = &self.inner.ty; - - return Some(quote! { - pub #field_ident: #field_type, - }); - } - FieldStatus::Renamed { from: _, to } => { - let field_type = &self.inner.ty; - - return Some(quote! { - pub #to: #field_type, - }); - } - FieldStatus::Deprecated(field_ident) => { - let field_type = &self.inner.ty; - - return Some(quote! { - #[deprecated] - pub #field_ident: #field_type, - }); - } - } - } - } - - // TODO (@Techassi): Handle versions which are in between - // versions defined in field actions. - None - } + let field_type = &self.inner.ty; + + match chain + .get(&container_version.inner) + .expect("internal error: chain must contain container version") + { + FieldStatus::Added(field_ident) => Some(quote! { + pub #field_ident: #field_type, + }), + FieldStatus::Renamed { _from: _, to } => Some(quote! { + pub #to: #field_type, + }), + FieldStatus::Deprecated { + ident: field_ident, + note, + } => Some(quote! { + #[deprecated = #note] + pub #field_ident: #field_type, + }), + FieldStatus::NotPresent => None, + FieldStatus::NoChange(field_ident) => Some(quote! { + pub #field_ident: #field_type, + }), } } None => { @@ -144,11 +77,16 @@ impl ToTokensExt for VersionedField { } impl VersionedField { + /// Create a new versioned field by creating a status chain for each version + /// defined in an action in the field attribute. + /// + /// This chain will get extended by the versions defined on the container by + /// calling the [`VersionedField::insert_container_versions`] function. pub(crate) fn new(field: Field, attrs: FieldAttributes) -> Result { - // Constructing the change chain requires going through the actions from + // 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 changes in reverse order, as otherwise it is + // 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 @@ -160,7 +98,13 @@ impl VersionedField { let mut actions = BTreeMap::new(); let ident = field.ident.as_ref().unwrap(); - actions.insert(*deprecated.since, FieldStatus::Deprecated(ident.clone())); + actions.insert( + *deprecated.since, + FieldStatus::Deprecated { + ident: ident.clone(), + note: deprecated.note.to_string(), + }, + ); // When the field is deprecated, any rename which occured beforehand // requires access to the field ident to infer the field ident for @@ -175,7 +119,7 @@ impl VersionedField { actions.insert( *rename.since, FieldStatus::Renamed { - from: from.clone(), + _from: from.clone(), to: ident, }, ); @@ -201,7 +145,7 @@ impl VersionedField { actions.insert( *rename.since, FieldStatus::Renamed { - from: from.clone(), + _from: from.clone(), to: ident, }, ); @@ -241,11 +185,58 @@ impl VersionedField { }) } } + + /// Inserts container versions not yet present in the status chain. + /// + /// When intially creating a new [`VersionedField`], 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 continous chain ensures that when generating code (tokens), each + /// field can lookup the status for a requested version. + pub(crate) fn insert_container_versions(&mut self, versions: &Vec) { + 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(_)) => chain.insert(version.inner, FieldStatus::NotPresent), + (Some(status), None) => { + let ident = match status { + FieldStatus::Added(ident) => ident, + FieldStatus::Renamed { _from: _, to } => to, + FieldStatus::Deprecated { ident, note: _ } => ident, + FieldStatus::NoChange(ident) => ident, + FieldStatus::NotPresent => unreachable!(), + }; + + chain.insert(version.inner, FieldStatus::NoChange(ident.clone())) + } + (Some(status), Some(_)) => { + let ident = match status { + FieldStatus::Added(ident) => ident, + FieldStatus::Renamed { _from: _, to } => to, + FieldStatus::NoChange(ident) => ident, + _ => unreachable!(), + }; + + chain.insert(version.inner, FieldStatus::NoChange(ident.clone())) + } + _ => unreachable!(), + }; + } + } + } } #[derive(Debug)] pub(crate) enum FieldStatus { Added(Ident), - Renamed { from: Ident, to: Ident }, - Deprecated(Ident), + Renamed { _from: Ident, to: Ident }, + Deprecated { ident: Ident, note: String }, + NoChange(Ident), + NotPresent, } diff --git a/crates/stackable-versioned/src/gen/mod.rs b/crates/stackable-versioned/src/gen/mod.rs index f95892882..4c3729873 100644 --- a/crates/stackable-versioned/src/gen/mod.rs +++ b/crates/stackable-versioned/src/gen/mod.rs @@ -9,6 +9,7 @@ use crate::{ }; pub(crate) mod field; +pub(crate) mod neighbors; pub(crate) mod venum; pub(crate) mod version; pub(crate) mod vstruct; diff --git a/crates/stackable-versioned/src/gen/neighbors.rs b/crates/stackable-versioned/src/gen/neighbors.rs new file mode 100644 index 000000000..c37955a39 --- /dev/null +++ b/crates/stackable-versioned/src/gen/neighbors.rs @@ -0,0 +1,71 @@ +use std::{collections::BTreeMap, ops::Bound}; + +pub(crate) trait Neighbors +where + K: Ord + Eq, +{ + fn get_neighbors(&self, key: &K) -> (Option<&V>, Option<&V>); + + fn lo_bound(&self, bound: Bound<&K>) -> Option<(&K, &V)>; + fn up_bound(&self, bound: Bound<&K>) -> Option<(&K, &V)>; +} + +impl Neighbors for BTreeMap +where + K: Ord + Eq, +{ + 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 + // provided by the standard lib. + // See: https://github.com/rust-lang/rust/issues/107540 + match ( + self.lo_bound(Bound::Excluded(key)), + self.up_bound(Bound::Excluded(key)), + ) { + (Some((k, v)), None) => { + if key > k { + (Some(v), None) + } else { + (self.lo_bound(Bound::Excluded(k)).map(|(_, v)| v), None) + } + } + (None, Some((k, v))) => { + if key < k { + (None, Some(v)) + } else { + (None, self.up_bound(Bound::Excluded(k)).map(|(_, v)| v)) + } + } + (Some((_, lo)), Some((_, up))) => (Some(lo), Some(up)), + (None, None) => unreachable!(), + } + } + + fn lo_bound(&self, bound: Bound<&K>) -> Option<(&K, &V)> { + self.range((Bound::Unbounded, bound)).next_back() + } + + fn up_bound(&self, bound: Bound<&K>) -> Option<(&K, &V)> { + self.range((bound, Bound::Unbounded)).next() + } +} + +#[cfg(test)] +mod test { + use super::*; + use rstest::rstest; + + #[rstest] + #[case(0, (None, Some(&"test1")))] + #[case(1, (None, Some(&"test3")))] + #[case(2, (Some(&"test1"), Some(&"test3")))] + #[case(3, (Some(&"test1"), None))] + #[case(4, (Some(&"test3"), None))] + fn test(#[case] key: i32, #[case] expected: (Option<&&str>, Option<&&str>)) { + let map = BTreeMap::from([(1, "test1"), (3, "test3")]); + let neigbors = map.get_neighbors(&key); + + assert_eq!(neigbors, expected); + } +} diff --git a/crates/stackable-versioned/src/gen/vstruct.rs b/crates/stackable-versioned/src/gen/vstruct.rs index 7f829d323..f92587419 100644 --- a/crates/stackable-versioned/src/gen/vstruct.rs +++ b/crates/stackable-versioned/src/gen/vstruct.rs @@ -70,29 +70,30 @@ impl VersionedStruct { data: DataStruct, attributes: ContainerAttributes, ) -> Result { - let mut fields = Vec::new(); + // Convert the raw version attributes into a container version. + let versions = attributes + .versions + .iter() + .map(|v| ContainerVersion { + deprecated: v.deprecated.is_present(), + inner: v.name, + }) + .collect(); // Extract the field attributes for every field from the raw token // stream and also validate that each field action version uses a // version declared by the container attribute. + let mut fields = Vec::new(); + for field in data.fields { let attrs = FieldAttributes::from_field(&field)?; attrs.validate_versions(&attributes, &field)?; - let versioned_field = VersionedField::new(field, attrs)?; + let mut versioned_field = VersionedField::new(field, attrs)?; + versioned_field.insert_container_versions(&versions); fields.push(versioned_field); } - // Convert the raw version attributes into a container version. - let versions = attributes - .versions - .iter() - .map(|v| ContainerVersion { - deprecated: v.deprecated.is_present(), - inner: v.name, - }) - .collect(); - Ok(Self { ident, versions, diff --git a/crates/stackable-versioned/tests/basic.rs b/crates/stackable-versioned/tests/basic.rs index 6cfa11c23..6f952c618 100644 --- a/crates/stackable-versioned/tests/basic.rs +++ b/crates/stackable-versioned/tests/basic.rs @@ -1,18 +1,23 @@ use stackable_versioned::Versioned; +// To expand the generated code (for debugging and testing), it is recommended +// to first change directory via `cd crates/stackable-versioned` and to then +// run `cargo expand --test basic --all-features`. #[derive(Versioned)] #[allow(dead_code)] #[versioned( version(name = "v1alpha1"), version(name = "v1beta1"), - version(name = "v1") + version(name = "v1"), + version(name = "v2"), + version(name = "v3") )] struct Foo { /// My docs #[versioned( added(since = "v1alpha1"), renamed(since = "v1beta1", from = "jjj"), - deprecated(since = "v1", _note = "") + deprecated(since = "v2", note = "not empty") )] deprecated_bar: usize, baz: bool,