Skip to content

feat(versioned): Re-add support for generics in tracking impls - #1304

Open
Techassi wants to merge 8 commits into
mainfrom
feat/versioned-re-added-generics-support-in-tracking-impls
Open

Techassi wants to merge 8 commits into
mainfrom
feat/versioned-re-added-generics-support-in-tracking-impls

Conversation

@Techassi

Copy link
Copy Markdown
Member

Came out of #1294.

This re-adds support for generics in tracking impls similar to how we support it for enums in #1294.

@Techassi Techassi self-assigned this Oct 19, 2026
@Techassi Techassi moved this to Development: In Progress in Stackable Engineering Oct 19, 2026

@sbernauer sbernauer left a comment •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed the code and it looked good to me 👍

However, I let claude analyze the PR and it found a few things that are valid.
I made suggestions for the nits (easy for you to pick) and fixed the two easy ones in d14737b (test change) and ff52596 (code change). Feel free to pick the commits.

The existing bug sounds bad, but I leave it up to you if you want to fix it, and if so in which PR.

Existing bug, not caused by this PR: when a parent struct and a struct nested in it both add a field in the same version, the conversion webhook panics during upgrade. The nested struct's conversion takes all tracked changes for that version, including the parent's, and the parent's entries hit unreachable!() (struct/conversion.rs:276, run before the parent's own read-back at 195–203). This deserves its own issue. A second possible bug, where hint(vec)/hint(map) elements lose their tracked values, comes from the review subagent and I haven't checked it.

For the record, if you are interested in Claude's response, but no need to read it.

Details

PR #1304 is fine to merge once two things are fixed: the test doesn't exercise generics together with tracking, and the hard-coded status parameter S clashes with user generics. The rest are nits. I reviewed the code by reading it and didn't build or run anything; CI on the PR is green.

Fix before merging

  1. The test misses the new code path. Baz has no versioned field, so no tracking code is generated for it. The snapshot only shows the plain pass-through impl. Adding something like #[versioned(added(since = "v1"))] inner_3: String to Baz would cover it. Use v1, not v1beta1, so it doesn't run into the existing bug below.
  2. The status parameter is always called S. It now gets added next to the user's own generics, so a user struct Foo<S> generates impl<S, S> and fails to compile. The enum version merged in #1294 has the same problem. Renaming it to something users won't pick (e.g. __S) in both places fixes it.

Nits

  • Changelog: "Re-add" sounds like a regression fix, though as far as I can tell no release ever had generics here.
  • Duplicate helper: get_tracking_generics is now copied in struct and enum. It's pub on the enum and private on the struct.
  • Doc comment (copied into both): "the macro the re-clone" and "it's value" are typos, and SAFETY: normally only marks justifications for unsafe code.
  • Test input leftovers: there's an unused usize_to_u16 helper and an unrelated bar/bah field.

Known limitation: if a field whose type is the generic parameter gets tracked, it needs serde bounds that aren't added automatically. Users can add them to the struct's where clause, so documenting this is enough. The OnceCell caching is the same pattern already merged in #1294 and is fine as is.

Existing bug, not caused by this PR: when a parent struct and a struct nested in it both add a field in the same version, the conversion webhook panics during upgrade. The nested struct's conversion takes all tracked changes for that version, including the parent's, and the parent's entries hit unreachable!() (struct/conversion.rs:276, run before the parent's own read-back at 195–203). This deserves its own issue. A second possible bug, where hint(vec)/hint(map) elements lose their tracked values, comes from the review subagent and I haven't checked it.

Comment thread crates/stackable-versioned-macros/src/codegen/container/enum.rs Outdated
Comment thread crates/stackable-versioned-macros/src/codegen/container/struct/mod.rs Outdated
Comment thread crates/stackable-versioned-macros/src/codegen/container/struct/mod.rs Outdated
Comment thread crates/stackable-versioned-macros/src/codegen/container/enum.rs Outdated
@Techassi

Techassi commented Oct 19, 2026 •

Copy link
Copy Markdown
Member Author

The existing bug sounds bad, but I leave it up to you if you want to fix it, and if so in which PR.

I already discovered this as well. It is even a TODO/FIXME in code. So yes, that will be tackled. See #1302

Techassi and others added 2 commits October 19, 2026 17:15
Co-authored-by: Sebastian Bernauer <sebastian.bernauer@stackable.tech>
Co-authored-by: Sebastian Bernauer <sebastian.bernauer@stackable.de>
@Techassi
Techassi requested a review from sbernauer October 19, 2026 15:17
Techassi and others added 2 commits October 19, 2026 17:27
@Techassi

Techassi commented Oct 19, 2026 •

Copy link
Copy Markdown
Member Author

I picked both of your commits (with a slight adjustment (see 800bd35) in the first one (d14737b)) as they both made sense. Thx!

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Development: In Progress

Development

Successfully merging this pull request may close these issues.

2 participants