Skip to content

Commit 3548b01

Browse files
TechassiTechassi
authored
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
1 parent 990de9c commit 3548b01

File tree

9 files changed

+210
-113
lines changed

9 files changed

+210
-113
lines changed

‎Cargo.lock

+1
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎crates/stackable-versioned/CHANGELOG.md

+4
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,10 @@ All notable changes to this project will be documented in this file.
44

55
## [Unreleased]
66

7+
- Improve action chain generation ([#784]).
8+
9+
[#784](ttps://github.com/stackabletech/operator-rs/pull/784)
10+
711
## [0.1.0] - 2024-05-08
812

913
### Changed

‎crates/stackable-versioned/Cargo.toml

+3
Original file line numberDiff line numberDiff line change
@@ -16,3 +16,6 @@ darling.workspace = true
1616
proc-macro2.workspace = true
1717
syn.workspace = true
1818
quote.workspace = true
19+
20+
[dev-dependencies]
21+
rstest.workspace = true

‎crates/stackable-versioned/src/attrs/field.rs

+21-1
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,7 @@ pub(crate) struct RenamedAttributes {
5151
#[derive(Clone, Debug, FromMeta)]
5252
pub(crate) struct DeprecatedAttributes {
5353
pub(crate) since: SpannedValue<Version>,
54-
pub(crate) _note: SpannedValue<String>,
54+
pub(crate) note: SpannedValue<String>,
5555
}
5656

5757
impl FieldAttributes {
@@ -64,10 +64,14 @@ impl FieldAttributes {
6464
fn validate(self) -> Result<Self, Error> {
6565
let mut errors = Error::accumulator();
6666

67+
// Semantic validation
6768
errors.handle(self.validate_action_combinations());
6869
errors.handle(self.validate_action_order());
6970
errors.handle(self.validate_field_name());
7071

72+
// Code quality validation
73+
errors.handle(self.validate_deprecated_options());
74+
7175
// TODO (@Techassi): Add validation for renames so that renamed fields
7276
// match up and form a continous chain (eg. foo -> bar -> baz).
7377

@@ -191,6 +195,22 @@ impl FieldAttributes {
191195
Ok(())
192196
}
193197

198+
fn validate_deprecated_options(&self) -> Result<(), Error> {
199+
// TODO (@Techassi): Make the field 'note' optional, because in the
200+
// future, the macro will generate parts of the deprecation note
201+
// automatically. The user-provided note will then be appended to the
202+
// auto-generated one.
203+
204+
if let Some(deprecated) = &self.deprecated {
205+
if deprecated.note.is_empty() {
206+
return Err(Error::custom("deprecation note must not be empty")
207+
.with_span(&deprecated.note.span()));
208+
}
209+
}
210+
211+
Ok(())
212+
}
213+
194214
/// Validates that each field action version is present in the declared
195215
/// container versions.
196216
pub(crate) fn validate_versions(

‎crates/stackable-versioned/src/gen/field.rs

+89-98
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ use syn::{Field, Ident};
99
use crate::{
1010
attrs::field::FieldAttributes,
1111
consts::DEPRECATED_PREFIX,
12-
gen::{version::ContainerVersion, ToTokensExt},
12+
gen::{neighbors::Neighbors, version::ContainerVersion, ToTokensExt},
1313
};
1414

1515
/// A versioned field, which contains contains common [`Field`] data and a chain
@@ -36,96 +36,29 @@ impl ToTokensExt for VersionedField {
3636
// The code generation then depends on the relation to other
3737
// versions (with actions).
3838

39-
// TODO (@Techassi): Make this more robust by also including
40-
// the container versions in the action chain. I'm not happy
41-
// with the follwoing code at all. It serves as a good first
42-
// implementation to get something out of the door.
43-
match chain.get(&container_version.inner) {
44-
Some(action) => match action {
45-
FieldStatus::Added(field_ident) => {
46-
let field_type = &self.inner.ty;
47-
48-
Some(quote! {
49-
pub #field_ident: #field_type,
50-
})
51-
}
52-
FieldStatus::Renamed { from: _, to } => {
53-
let field_type = &self.inner.ty;
54-
55-
Some(quote! {
56-
pub #to: #field_type,
57-
})
58-
}
59-
FieldStatus::Deprecated(field_ident) => {
60-
let field_type = &self.inner.ty;
61-
62-
Some(quote! {
63-
#[deprecated]
64-
pub #field_ident: #field_type,
65-
})
66-
}
67-
},
68-
None => {
69-
// Generate field if the container version is not
70-
// included in the action chain. First we check the
71-
// earliest field action version.
72-
if let Some((version, action)) = chain.first_key_value() {
73-
if container_version.inner < *version {
74-
match action {
75-
FieldStatus::Added(_) => return None,
76-
FieldStatus::Renamed { from, to: _ } => {
77-
let field_type = &self.inner.ty;
78-
79-
return Some(quote! {
80-
pub #from: #field_type,
81-
});
82-
}
83-
FieldStatus::Deprecated(field_ident) => {
84-
let field_type = &self.inner.ty;
85-
86-
return Some(quote! {
87-
pub #field_ident: #field_type,
88-
});
89-
}
90-
}
91-
}
92-
}
93-
94-
// Check the container version against the latest
95-
// field action version.
96-
if let Some((version, action)) = chain.last_key_value() {
97-
if container_version.inner > *version {
98-
match action {
99-
FieldStatus::Added(field_ident) => {
100-
let field_type = &self.inner.ty;
101-
102-
return Some(quote! {
103-
pub #field_ident: #field_type,
104-
});
105-
}
106-
FieldStatus::Renamed { from: _, to } => {
107-
let field_type = &self.inner.ty;
108-
109-
return Some(quote! {
110-
pub #to: #field_type,
111-
});
112-
}
113-
FieldStatus::Deprecated(field_ident) => {
114-
let field_type = &self.inner.ty;
115-
116-
return Some(quote! {
117-
#[deprecated]
118-
pub #field_ident: #field_type,
119-
});
120-
}
121-
}
122-
}
123-
}
124-
125-
// TODO (@Techassi): Handle versions which are in between
126-
// versions defined in field actions.
127-
None
128-
}
39+
let field_type = &self.inner.ty;
40+
41+
match chain
42+
.get(&container_version.inner)
43+
.expect("internal error: chain must contain container version")
44+
{
45+
FieldStatus::Added(field_ident) => Some(quote! {
46+
pub #field_ident: #field_type,
47+
}),
48+
FieldStatus::Renamed { _from: _, to } => Some(quote! {
49+
pub #to: #field_type,
50+
}),
51+
FieldStatus::Deprecated {
52+
ident: field_ident,
53+
note,
54+
} => Some(quote! {
55+
#[deprecated = #note]
56+
pub #field_ident: #field_type,
57+
}),
58+
FieldStatus::NotPresent => None,
59+
FieldStatus::NoChange(field_ident) => Some(quote! {
60+
pub #field_ident: #field_type,
61+
}),
12962
}
13063
}
13164
None => {
@@ -144,11 +77,16 @@ impl ToTokensExt for VersionedField {
14477
}
14578

14679
impl VersionedField {
80+
/// Create a new versioned field by creating a status chain for each version
81+
/// defined in an action in the field attribute.
82+
///
83+
/// This chain will get extended by the versions defined on the container by
84+
/// calling the [`VersionedField::insert_container_versions`] function.
14785
pub(crate) fn new(field: Field, attrs: FieldAttributes) -> Result<Self, Error> {
148-
// Constructing the change chain requires going through the actions from
86+
// Constructing the action chain requires going through the actions from
14987
// the end, because the base struct always represents the latest (most
15088
// up-to-date) version of that struct. That's why the following code
151-
// needs to go through the changes in reverse order, as otherwise it is
89+
// needs to go through the actions in reverse order, as otherwise it is
15290
// impossible to extract the field ident for each version.
15391

15492
// Deprecating a field is always the last state a field can end up in. For
@@ -160,7 +98,13 @@ impl VersionedField {
16098
let mut actions = BTreeMap::new();
16199

162100
let ident = field.ident.as_ref().unwrap();
163-
actions.insert(*deprecated.since, FieldStatus::Deprecated(ident.clone()));
101+
actions.insert(
102+
*deprecated.since,
103+
FieldStatus::Deprecated {
104+
ident: ident.clone(),
105+
note: deprecated.note.to_string(),
106+
},
107+
);
164108

165109
// When the field is deprecated, any rename which occured beforehand
166110
// requires access to the field ident to infer the field ident for
@@ -175,7 +119,7 @@ impl VersionedField {
175119
actions.insert(
176120
*rename.since,
177121
FieldStatus::Renamed {
178-
from: from.clone(),
122+
_from: from.clone(),
179123
to: ident,
180124
},
181125
);
@@ -201,7 +145,7 @@ impl VersionedField {
201145
actions.insert(
202146
*rename.since,
203147
FieldStatus::Renamed {
204-
from: from.clone(),
148+
_from: from.clone(),
205149
to: ident,
206150
},
207151
);
@@ -241,11 +185,58 @@ impl VersionedField {
241185
})
242186
}
243187
}
188+
189+
/// Inserts container versions not yet present in the status chain.
190+
///
191+
/// When intially creating a new [`VersionedField`], the code doesn't have
192+
/// access to the versions defined on the container. This function inserts
193+
/// all non-present container versions and decides which status and ident
194+
/// is the right fit based on the status neighbors.
195+
///
196+
/// This continous chain ensures that when generating code (tokens), each
197+
/// field can lookup the status for a requested version.
198+
pub(crate) fn insert_container_versions(&mut self, versions: &Vec<ContainerVersion>) {
199+
if let Some(chain) = &mut self.chain {
200+
for version in versions {
201+
if chain.contains_key(&version.inner) {
202+
continue;
203+
}
204+
205+
match chain.get_neighbors(&version.inner) {
206+
(None, Some(_)) => chain.insert(version.inner, FieldStatus::NotPresent),
207+
(Some(status), None) => {
208+
let ident = match status {
209+
FieldStatus::Added(ident) => ident,
210+
FieldStatus::Renamed { _from: _, to } => to,
211+
FieldStatus::Deprecated { ident, note: _ } => ident,
212+
FieldStatus::NoChange(ident) => ident,
213+
FieldStatus::NotPresent => unreachable!(),
214+
};
215+
216+
chain.insert(version.inner, FieldStatus::NoChange(ident.clone()))
217+
}
218+
(Some(status), Some(_)) => {
219+
let ident = match status {
220+
FieldStatus::Added(ident) => ident,
221+
FieldStatus::Renamed { _from: _, to } => to,
222+
FieldStatus::NoChange(ident) => ident,
223+
_ => unreachable!(),
224+
};
225+
226+
chain.insert(version.inner, FieldStatus::NoChange(ident.clone()))
227+
}
228+
_ => unreachable!(),
229+
};
230+
}
231+
}
232+
}
244233
}
245234

246235
#[derive(Debug)]
247236
pub(crate) enum FieldStatus {
248237
Added(Ident),
249-
Renamed { from: Ident, to: Ident },
250-
Deprecated(Ident),
238+
Renamed { _from: Ident, to: Ident },
239+
Deprecated { ident: Ident, note: String },
240+
NoChange(Ident),
241+
NotPresent,
251242
}

‎crates/stackable-versioned/src/gen/mod.rs

+1
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ use crate::{
99
};
1010

1111
pub(crate) mod field;
12+
pub(crate) mod neighbors;
1213
pub(crate) mod venum;
1314
pub(crate) mod version;
1415
pub(crate) mod vstruct;

‎crates/stackable-versioned/src/gen/neighbors.rs

+71
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,71 @@
1+
use std::{collections::BTreeMap, ops::Bound};
2+
3+
pub(crate) trait Neighbors<K, V>
4+
where
5+
K: Ord + Eq,
6+
{
7+
fn get_neighbors(&self, key: &K) -> (Option<&V>, Option<&V>);
8+
9+
fn lo_bound(&self, bound: Bound<&K>) -> Option<(&K, &V)>;
10+
fn up_bound(&self, bound: Bound<&K>) -> Option<(&K, &V)>;
11+
}
12+
13+
impl<K, V> Neighbors<K, V> for BTreeMap<K, V>
14+
where
15+
K: Ord + Eq,
16+
{
17+
fn get_neighbors(&self, key: &K) -> (Option<&V>, Option<&V>) {
18+
// NOTE (@Techassi): These functions might get added to the standard
19+
// library at some point. If that's the case, we can use the ones
20+
// provided by the standard lib.
21+
// See: https://github.com/rust-lang/rust/issues/107540
22+
match (
23+
self.lo_bound(Bound::Excluded(key)),
24+
self.up_bound(Bound::Excluded(key)),
25+
) {
26+
(Some((k, v)), None) => {
27+
if key > k {
28+
(Some(v), None)
29+
} else {
30+
(self.lo_bound(Bound::Excluded(k)).map(|(_, v)| v), None)
31+
}
32+
}
33+
(None, Some((k, v))) => {
34+
if key < k {
35+
(None, Some(v))
36+
} else {
37+
(None, self.up_bound(Bound::Excluded(k)).map(|(_, v)| v))
38+
}
39+
}
40+
(Some((_, lo)), Some((_, up))) => (Some(lo), Some(up)),
41+
(None, None) => unreachable!(),
42+
}
43+
}
44+
45+
fn lo_bound(&self, bound: Bound<&K>) -> Option<(&K, &V)> {
46+
self.range((Bound::Unbounded, bound)).next_back()
47+
}
48+
49+
fn up_bound(&self, bound: Bound<&K>) -> Option<(&K, &V)> {
50+
self.range((bound, Bound::Unbounded)).next()
51+
}
52+
}
53+
54+
#[cfg(test)]
55+
mod test {
56+
use super::*;
57+
use rstest::rstest;
58+
59+
#[rstest]
60+
#[case(0, (None, Some(&"test1")))]
61+
#[case(1, (None, Some(&"test3")))]
62+
#[case(2, (Some(&"test1"), Some(&"test3")))]
63+
#[case(3, (Some(&"test1"), None))]
64+
#[case(4, (Some(&"test3"), None))]
65+
fn test(#[case] key: i32, #[case] expected: (Option<&&str>, Option<&&str>)) {
66+
let map = BTreeMap::from([(1, "test1"), (3, "test3")]);
67+
let neigbors = map.get_neighbors(&key);
68+
69+
assert_eq!(neigbors, expected);
70+
}
71+
}

0 commit comments

Comments
 (0)