Repository navigation
Conversation
There was a problem hiding this comment.
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
- 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.
- The status parameter is always called S. It now gets added next to the user's own generics, so a user struct
Foo<S>generatesimpl<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.
I already discovered this as well. It is even a TODO/FIXME in code. So yes, that will be tackled. See #1302 |
Co-authored-by: Sebastian Bernauer <sebastian.bernauer@stackable.tech>
Co-authored-by: Sebastian Bernauer <sebastian.bernauer@stackable.de>
Co-authored-by: Sebastian Bernauer <sebastian.bernauer@stackable.de>
Came out of #1294.
This re-adds support for generics in tracking impls similar to how we support it for enums in #1294.