GTS macro proposal#78
Open
asmith987 wants to merge 5 commits into
Open
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 8 minutes and 9 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (68)
📝 WalkthroughWalkthroughThis pull request introduces a new Changes
Sequence DiagramsequenceDiagram
participant User as User Code
participant Parser as Attribute Parser
participant Validator as Validator
participant CodeGen as Code Generator
participant Serde as Serde Generator
participant Compiler as Rust Compiler
User->>Parser: Invoke #[derive(GtsSchema)]
Parser->>Parser: Parse #[gts(...)] attributes
Parser->>Parser: Parse #[gts(...)] field attributes
Parser->>Validator: Validate parsed attributes
Validator->>Validator: Check struct shape
Validator->>Validator: Validate schema-id format
Validator->>Validator: Verify version alignment
Validator->>Validator: Check segment counts
Validator->>Validator: Validate identity fields
alt Validation fails
Validator->>Compiler: Emit compile error
Compiler->>User: Error tokens
else Validation succeeds
Validator->>CodeGen: Pass validated attributes
CodeGen->>CodeGen: Generate GtsSchema impl
CodeGen->>CodeGen: Generate schema constants
CodeGen->>CodeGen: Generate allOf composition
CodeGen->>CodeGen: Generate constructors
CodeGen->>Serde: Pass parsed structure
Serde->>Serde: Generate Serde impls
Serde->>Serde: Generate GtsSerialize/GtsDeserialize
Serde->>Serde: Emit trait guards
CodeGen->>User: Expanded token stream
Serde->>User: Expanded token stream
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes The diff introduces substantial logic density across multiple new codegen modules (gts_codegen.rs at 792 lines, gts_serde.rs at 650 lines), heterogeneous changes spanning attribute parsing, validation, code generation, and 40+ test fixtures with mixed complexities. While organized into cohesive modules, each codegen/serde section involves intricate proc-macro token manipulation and serde implementation detail requiring careful reasoning per module. The extensive test suite partially offsets this complexity through coherent patterns, but validation logic and serde bounds interactions demand focused review. Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
docs/001-macro-proposal.md (2)
126-174: Clear before/after migration example with helpful summary table.The side-by-side comparison makes the transition immediately understandable. The table at lines 167-174 provides excellent quick-reference guidance for each change.
One minor observation: The "After" example (line 149) explicitly includes
JsonSchemain the derives, but according to the ADR (lines 235-236 in the ADR file),JsonSchemais auto-added if not present. Consider adding a note clarifying thatJsonSchemacan be omitted and will be added automatically.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/001-macro-proposal.md` around lines 126 - 174, Add a brief clarifying sentence to the "After" example explaining that JsonSchema in the derive list is optional because the macro will auto-add JsonSchema when omitted per the ADR; reference the example struct BaseEventV1 and the derive attributes (Serialize, Deserialize, JsonSchema, GtsSchema) and mention the ADR lines about auto-adding JsonSchema so readers know they can omit JsonSchema and rely on the macro to inject it.
380-418: Well-planned modular architecture for extensibility.The proposed module structure (lines 380-390) is a significant improvement over the current ~1,843-line single file. The two concrete examples (lines 392-417) demonstrate how the architecture accommodates future spec features:
- Adding a new field-level attribute requires changes to only 4 focused modules
- Schema traits (§9.7) can be added via the same parse-validate-generate pipeline
However, the code blocks at lines 400 and 418 are missing language specifiers. While these are pseudo-structure examples rather than actual code, adding language hints would improve rendering.
Minor: Add language specifiers to code blocks
-``` +```text gts-macros/src/ lib.rs Entry points (current + updated macro) gts_schema_derive.rs #[derive(GtsSchema)] orchestrationand
-``` +```json "x-gts-traits-schema": { "properties": { "topicRef": { "x-gts-ref": "gts.x.core.events.topic.v1~" },As per coding guidelines, this addresses the markdownlint warnings for missing language specifiers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/001-macro-proposal.md` around lines 380 - 418, Add missing Markdown language specifiers to the two fenced code blocks showing the directory layout and the JSON trait example: change the first fence to use "text" (for the gts-macros/src/ listing) and the second fence to use "json" (for the "x-gts-traits-schema"/"x-gts-traits" example). Locate the blocks that contain the "gts-macros/src/" directory listing and the JSON snippet starting with "x-gts-traits-schema" and update their opening triple-backtick fences to ```text and ```json respectively so markdownlint warnings are resolved and syntax rendering improves.docs/001-macro-alignment-implementation-plan.md (1)
60-82: Comprehensive compile-fail test coverage.The 15 compile-fail tests cover all error cases for the new macro. Each test has a clear validation purpose and spec reference. However, the code block at line 78 is missing a language specifier.
Minor: Add language specifier
-| `unknown_gts_attr` | `#[gts(nonexistent)]` | Fail-fast on typos | +| `unknown_gts_attr` | `#[gts(nonexistent)]` (markdown table, no code block) | Fail-fast on typos |Actually, reviewing the context, this appears to be within a markdown table, not a code block. The static analysis warning at line 78 may be a false positive or referring to a different issue. No change needed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/001-macro-alignment-implementation-plan.md` around lines 60 - 82, The reviewer flagged a missing code-block language specifier around the table entry near `nested_direct_serialize_cfg_attr`; inspect the markdown row containing `nested_direct_serialize` / `nested_direct_serialize_cfg_attr` to confirm whether it is truly a fenced code block or just table text; if it's just table content, leave it unchanged and mark the static-analysis warning as a false positive (or add a comment in the PR), but if there is an accidental fenced code block, add the appropriate language specifier (e.g., ```rust) to that fence to silence the warning.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@docs/001-macro-alignment-implementation-plan.md`:
- Around line 60-82: The reviewer flagged a missing code-block language
specifier around the table entry near `nested_direct_serialize_cfg_attr`;
inspect the markdown row containing `nested_direct_serialize` /
`nested_direct_serialize_cfg_attr` to confirm whether it is truly a fenced code
block or just table text; if it's just table content, leave it unchanged and
mark the static-analysis warning as a false positive (or add a comment in the
PR), but if there is an accidental fenced code block, add the appropriate
language specifier (e.g., ```rust) to that fence to silence the warning.
In `@docs/001-macro-proposal.md`:
- Around line 126-174: Add a brief clarifying sentence to the "After" example
explaining that JsonSchema in the derive list is optional because the macro will
auto-add JsonSchema when omitted per the ADR; reference the example struct
BaseEventV1 and the derive attributes (Serialize, Deserialize, JsonSchema,
GtsSchema) and mention the ADR lines about auto-adding JsonSchema so readers
know they can omit JsonSchema and rely on the macro to inject it.
- Around line 380-418: Add missing Markdown language specifiers to the two
fenced code blocks showing the directory layout and the JSON trait example:
change the first fence to use "text" (for the gts-macros/src/ listing) and the
second fence to use "json" (for the "x-gts-traits-schema"/"x-gts-traits"
example). Locate the blocks that contain the "gts-macros/src/" directory listing
and the JSON snippet starting with "x-gts-traits-schema" and update their
opening triple-backtick fences to ```text and ```json respectively so
markdownlint warnings are resolved and syntax rendering improves.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f4d4cf41-1e27-4748-96ae-3af4dd6c9af5
📒 Files selected for processing (4)
.gts-specdocs/001-macro-alignment-adr.mddocs/001-macro-alignment-implementation-plan.mddocs/001-macro-proposal.md
Artifizer
reviewed
Apr 15, 2026
Artifizer
reviewed
Apr 15, 2026
Artifizer
requested changes
Apr 15, 2026
aviator5
reviewed
Apr 24, 2026
|
|
||
| The `#[struct_to_gts_schema]` macro has been the primary integration point between Rust structs and the Global Type System. It delivers compile-time validation, JSON Schema generation, and a runtime API from a single annotation. As the GTS specification has matured and usage has grown, several areas have emerged where the macro's assumptions can be brought into closer alignment with the spec. | ||
|
|
||
| This proposal evolves the macro from `#[struct_to_gts_schema]` to `#[derive(GtsSchema)]` with `#[gts(...)]` attributes. The goals are: |
Contributor
There was a problem hiding this comment.
- On one hand, I understand that
#[derive(GtsSchema)]looks more Rust-idiomatic. On the other hand, it is not clear to me why we now need two separate annotations:#[derive(GtsSchema)]and#[gts(...)].
Does#[derive(GtsSchema)]make sense without#[gts(...)], and vice versa? If they are expected to always be used together, maybe it's enough to have just#[gts(...)]? - From my perspective, we should have two independent macros instead:
gts_schemaandgts_instance.
This PR only improves the schema-related macros; instance-related functionality is out of scope here.
I’m fine with that approach and will follow up with a separate PR forgts_instanceonce this one is merged (discussed with @Artifizer).
Author
There was a problem hiding this comment.
@aviator5 Regarding the points you mentioned:
[gts(...)]is just a helper attribute declared by the derive macro. It provides configuration details for the derive macro to read. Similar toclapwith#[derive(Parser)]+#[clap(...)]. We could revert back to using a attribute macro without derive and only use#[gts(...)], but then the struct is no longer the source of truth since the attribute macro is free to manipulate it as it pleases. The original#[struct_to_gts_schema]did this.- When you say two independent macros, do you mean two derive macros or two procedural attribute macros? If you mean two attribute macros, wouldn't that be a little brittle since the first macro would run on source and the second would run on whatever was emitted by the first attribute macro?
asmith987
force-pushed
the
gts-macro-proposal
branch
from
7620bc1 to
2d5acfb
Compare
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
gts-macros/src/lib.rs (2)
198-221:⚠️ Potential issue | 🟠 MajorParse
#[serde(rename = ...)]structurally instead of slicing the token string.The current implementation fails silently when
renameis not the final attribute. For example,#[serde(rename = "type", alias = "kind")]extractstype", alias = "kindinstead oftype. The token string slicing assumes the closing quote is always at the end of the remainder, which breaks once additional comma-separated attributes follow.Proposed fix
pub(crate) fn get_serde_rename(field: &syn::Field) -> Option<String> { for attr in &field.attrs { - // Parse the serde attribute using a simpler approach - if attr.path().is_ident("serde") - && let Ok(meta) = attr.meta.require_list() - { - let tokens = meta.tokens.to_string(); - - // Look for rename = "value" pattern in the token string - if let Some(rename_start) = tokens.find("rename") { - let rename_part = &tokens[rename_start..]; - if let Some(eq_pos) = rename_part.find('=') { - let value_part = &rename_part[eq_pos + 1..].trim(); - // Extract the string value between quotes - if value_part.starts_with('"') && value_part.ends_with('"') { - let rename_value = &value_part[1..value_part.len() - 1]; - return Some(rename_value.to_owned()); - } - } - } + if !attr.path().is_ident("serde") { + continue; + } + let Ok(meta) = attr.meta.require_list() else { + continue; + }; + let Ok(items) = meta.parse_args_with( + syn::punctuated::Punctuated::<syn::Meta, syn::Token![,]>::parse_terminated, + ) else { + continue; + }; + + for item in items { + if let syn::Meta::NameValue(name_value) = item + && name_value.path.is_ident("rename") + && let syn::Expr::Lit(syn::ExprLit { + lit: syn::Lit::Str(lit_str), + .. + }) = name_value.value + { + return Some(lit_str.value()); + } } } None }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gts-macros/src/lib.rs` around lines 198 - 221, get_serde_rename currently slices the token string and fails when rename isn't the last serde key; instead parse the attribute structurally: for each attr where attr.path().is_ident("serde") use attr.parse_meta() (or attr.meta.require_list()) to get a Meta::List and iterate its nested entries, match each NestedMeta::Meta(Meta::NameValue) whose path is_ident("rename"), then extract the value by matching the NameValue.lit as Lit::Str and returning its value; update get_serde_rename to return None if no such NameValue Lit::Str is found.
603-620:⚠️ Potential issue | 🔴 CriticalFix the
whereclause concatenation to prevent invalid Rust syntax.The function concatenates an existing
whereclause with a new predicate usingquote! {#existing#generic_ident:#bounds_tokens, }, which lacks a comma separator between predicates. For input likewhere P: Clone, this expands towhere P: Clone T: ..., causing any#[derive(GtsSchema)]struct with an existingwhereclause to fail during macro expansion.Replace the quote-based concatenation with proper manipulation of
syn::WhereClause::predicatesto ensure predicates are correctly separated:Proposed fix
pub(crate) fn build_where_clause( generics: &syn::Generics, where_clause: Option<&syn::WhereClause>, bounds: &str, ) -> proc_macro2::TokenStream { - if let Some(generic_param) = generics.type_params().next() { - let generic_ident = &generic_param.ident; - let bounds_tokens: proc_macro2::TokenStream = - bounds.parse().expect("Failed to parse bounds"); - if let Some(existing) = where_clause { - quote! { `#existing` `#generic_ident`: `#bounds_tokens`, } - } else { - quote! { where `#generic_ident`: `#bounds_tokens` } - } - } else { - quote! { `#where_clause` } - } + let Some(generic_param) = generics.type_params().next() else { + return quote! { `#where_clause` }; + }; + + let generic_ident = &generic_param.ident; + let bounds_tokens: proc_macro2::TokenStream = + bounds.parse().expect("Failed to parse bounds"); + + let mut clause = where_clause + .cloned() + .unwrap_or_else(|| syn::parse_quote!(where)); + clause + .predicates + .push(syn::parse_quote!(`#generic_ident`: `#bounds_tokens`)); + + quote! { `#clause` } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gts-macros/src/lib.rs` around lines 603 - 620, The concatenation in build_where_clause produces invalid Rust by inserting predicates without proper separators; instead, parse the new predicate from the generic identifier and bounds (use bounds.parse::<syn::WherePredicate>()) and if where_clause is Some, clone it, push the parsed predicate into its .predicates (syn::punctuated::Punctuated will preserve commas), then return the modified where clause as a TokenStream; if where_clause is None, build a new syn::WhereClause with the single parsed predicate and return that. Ensure you reference build_where_clause, generics.type_params().next(), generic_ident, bounds.parse(), and syn::WhereClause::predicates when locating and implementing the change.
🧹 Nitpick comments (5)
gts-macros/src/gts_field_attrs.rs (2)
25-65: Minor: duplicate error message and docstring don't match all rejection paths.The doc comment on Line 27 says "duplicate
#[gts(...)]blocks", but the code at Lines 56–61 also rejects duplicates within a single block (e.g.#[gts(type_field, instance_id)]or#[gts(skip, skip)]). The error text "only one #[gts(...)] attribute per field is allowed" is then a bit misleading in the intra-block case — the user wrote one block.Consider tightening both:
♻️ Proposed wording tweak
- /// Parse all `#[gts(...)]` attributes on a single field. - /// - /// Returns an error for unknown attributes or duplicate `#[gts(...)]` blocks. + /// Parse all `#[gts(...)]` attributes on a single field. + /// + /// Returns an error for unknown directives or when more than one + /// `#[gts(...)]` directive is present on the same field (whether across + /// multiple `#[gts(...)]` blocks or within a single block). pub fn from_field(field: &syn::Field) -> syn::Result<Self> { @@ - if result.is_some() { - return Err(syn::Error::new( - ident.span(), - "GtsSchema: only one #[gts(...)] attribute per field is allowed", - )); - } + if result.is_some() { + return Err(syn::Error::new( + ident.span(), + "GtsSchema: only one #[gts(...)] directive (type_field, instance_id, or skip) per field is allowed", + )); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gts-macros/src/gts_field_attrs.rs` around lines 25 - 65, The docstring and duplicate-error text in from_field are inconsistent with the actual duplicate-detection location inside attr.parse_nested_meta: update the doc comment on from_field to say it rejects duplicate attributes both across multiple #[gts(...)] blocks and within a single block, and change the syn::Error message produced when result.is_some() (inside attr.parse_nested_meta) to something like "GtsSchema: duplicate gts attribute for this field; only one of type_field, instance_id, or skip is allowed" so it clearly covers intra-block duplicates; refer to the from_field function, the attr.parse_nested_meta closure, the result variable, and the GtsFieldAttr variants when making the text changes.
36-65: Consider explicitly rejecting values on field directives with a dedicated error message.Field-level directives (
type_field,instance_id,skip) do not accept values, but when someone mistakenly writes#[gts(type_field = "x")], the resulting error comes fromsyn'sparse_nested_metaparser rather than this module. This produces a generic error without the "GtsSchema:" prefix, which is less helpful than the module's custom error messages. No test case currently covers this edge case.Either confirm the generic error is acceptable, or explicitly consume and reject the value with a tailored error (similar to how struct-level parsing handles
=ingts_attrs.rs).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gts-macros/src/gts_field_attrs.rs` around lines 36 - 65, The parser for field-level directives (inside attr.parse_nested_meta in gts_field_attrs.rs) currently lets syn produce a generic error when a user supplies a value (e.g., #[gts(type_field = "x")]); update the nested-meta handling to explicitly reject value tokens: inside the closure used by attr.parse_nested_meta, after extracting ident and before returning Ok(()), check for any attached value (e.g., meta.value or by using meta.parse_nested_meta to detect `=`/Lit` patterns) and, if present, return a syn::Error with a GtsSchema-prefixed message (similar wording to struct-level checks in gts_attrs.rs) indicating that type_field/instance_id/skip do not accept values; ensure you reference GtsFieldAttr and the existing result handling so behavior and duplicate-attribute checks remain unchanged.gts-macros/tests/v2_compile_fail/type_field_wrong_type.rs (1)
11-15: Fixture asserts an unintended failure ordering — consider hardening.Both
event_type: Stringandpub id: Stringare present, and neither is aGtsSchemaId/GtsInstanceId. The test relies onvalidate_field_gts_attrsreaching theis_type_gts_schema_idcheck before the "root struct must declare exactly one identity field" check (which would otherwise fire iftype_fieldis silently dropped or refactored). Two independent failure paths can lead to brittle.stderrsnapshots if the validation order is reordered later.Consider adding a sibling fixture (or comment in the snapshot) that pins this to the type-mismatch error specifically — e.g., make
id: GtsInstanceIdso a regression that drops theis_type_gts_schema_idcheck still fails distinctly rather than masking as a "missing identity field" error.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gts-macros/tests/v2_compile_fail/type_field_wrong_type.rs` around lines 11 - 15, The test fixture EventV1 relies on validation ordering and can produce brittle stderr snapshots; update the fixture so the type-mismatch failure is unambiguous by making the identity field a GtsInstanceId/GtsSchemaId (e.g., change id to a GtsInstanceId) while keeping #[gts(type_field)] on event_type, so validate_field_gts_attrs and its is_type_gts_schema_id check will produce the type-mismatch error rather than triggering the "root struct must declare exactly one identity field" path; reference EventV1, the #[gts(type_field)] attribute, validate_field_gts_attrs, and is_type_gts_schema_id when making the change.gts-macros/src/gts_attrs.rs (2)
44-90: Duplicate keys are silently overwritten.
#[gts(dir_path = "a", dir_path = "b")]currently parses without error and silently keeps the last value. This is a minor footgun and inconsistent with the strictness shown elsewhere (unknown keys are rejected). Consider erroring on duplicates so misconfigurations surface at compile time instead of producing a confusing schema artifact.♻️ Proposed fix
match key_str.as_str() { "dir_path" => { input.parse::<Token![=]>()?; let value: LitStr = input.parse()?; + if dir_path.is_some() { + return Err(syn::Error::new_spanned( + key, + "GtsSchema: duplicate attribute 'dir_path' in #[gts(...)]", + )); + } dir_path = Some(value.value()); } "schema_id" => { input.parse::<Token![=]>()?; let value: LitStr = input.parse()?; + if schema_id.is_some() { + return Err(syn::Error::new_spanned( + key, + "GtsSchema: duplicate attribute 'schema_id' in #[gts(...)]", + )); + } schema_id = Some(value.value()); } "description" => { input.parse::<Token![=]>()?; let value: LitStr = input.parse()?; + if description.is_some() { + return Err(syn::Error::new_spanned( + key, + "GtsSchema: duplicate attribute 'description' in #[gts(...)]", + )); + } description = Some(value.value()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gts-macros/src/gts_attrs.rs` around lines 44 - 90, The parser currently allows duplicate keys (e.g., dir_path, schema_id, description, extends) to silently overwrite previous values; update the parsing loop in gts_attrs.rs (the while !input.is_empty() block that parses key_str and sets dir_path, schema_id, description, extends) to check whether the target Option is already Some before assigning and, if so, return a syn::Error::new_spanned(key, "... duplicate attribute ...") describing the duplicate; for extends treat the explicit "None" vs an ident the same way but still error if extends was already provided. Ensure each match arm for "dir_path", "schema_id", "description", and "extends" performs this duplicate-check and returns an error instead of overwriting.
87-89: Comma between attributes is parsed as optional.The loop only consumes a comma if present;
#[gts(dir_path = "a" schema_id = "b")]would parse successfully. Consider requiring a comma (when!input.is_empty()after a value) so syntax stays consistent with typical attribute grammars and trybuild diagnostics for missing commas remain obvious.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gts-macros/src/gts_attrs.rs` around lines 87 - 89, The attribute parser currently treats the comma separator as optional (the if input.peek(Token![,]) { input.parse::<Token![,]>()? } block), allowing malformed attributes like #[gts(dir_path = "a" schema_id = "b")]; change this so that after parsing a value you check if !input.is_empty() and if so require a comma: if input.peek(Token![,]) { input.parse::<Token![,]>()? } else { return Err(syn::Error::new(input.span(), "expected `,` between attributes")); } — update the loop that consumes separators (the block using input.peek and input.parse::<Token![,]>()) to enforce the comma and return a syn::Error when missing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/002-struct-to-gts-schema-migration.md`:
- Line 307: Update the table row for "JsonSchema derive" to indicate it is NOT
auto-added by #[derive(GtsSchema)]; explicitly state that callers must still add
#[derive(JsonSchema)] themselves (the new test fixtures do this), and adjust the
description to clarify that #[derive(GtsSchema)] does not provide JsonSchema
required for schemars::schema_for! so leaving "Auto-added" would cause compile
failures.
In `@gts-macros/README.md`:
- Around line 58-64: The README example shows deriving Serialize and Deserialize
on the generic-root struct BaseEventV1<P: GtsSchema>, which conflicts with the
generated generic-root serde bridge; remove the explicit #[derive(Serialize,
Deserialize)] from BaseEventV1 (and the other occurrences noted around lines
~232-235) so the docs match the implementation that relies on the generated
serde bridge via GtsSchema/GtsSchema-derived code instead of user-written serde
derives; keep Debug/Clone/JsonSchema/GtsSchema derives and update the “Root
structs must derive…” guidance to not instruct adding Serialize/Deserialize for
generic-root types.
In `@gts-macros/src/gts_attrs.rs`:
- Around line 64-75: When validating collisions between multi-segment schema IDs
and the parsed extends value, improve the error emitted to explicitly mention
that extends = "None" is treated as the sentinel meaning "no extends" (the
sentinel is detected in the parsing branch where ident == "None" and the parsed
variable is set via extends = None), and add a diagnostic hint explaining that
if the user intended to reference a parent type they should use the parent's
actual type name (and that qualified paths like crate::events::Parent are not
accepted because parsing currently only accepts a syn::Ident). Update the error
message that currently reads like "schema_id has {count} segments but no
'extends' is specified" to include a note that "extends = None was parsed as the
reserved sentinel" and suggest using a real parent identifier or change the
attribute format to support paths.
In `@gts-macros/src/gts_codegen.rs`:
- Around line 113-128: The auto-populator currently always emits <#generic_ident
as ::gts::GtsSchema>::SCHEMA_ID which causes BaseEventV1::<()>::new(...) to
stamp an empty id; change the logic in the type_field_ident handling so when
info.generic_param_name is present but equals the sentinel for the unit case
(the empty inner schema id / ""), emit the root schema id expression (e.g.
::gts::gts::GtsSchema::SCHEMA_ID wrapped in ::gts::gts::GtsSchemaId::new)
instead of referencing the generic_ident; keep the existing branch that emits
<#generic_ident as ::gts::GtsSchema>::SCHEMA_ID for all other generic_param_name
values and still use Self::gts_schema_id().clone() for the non-generic branch so
the populator generation (the variable populator used in inits.push) produces
the correct code for the unit sentinel.
- Around line 288-300: The has_serde_skip function incorrectly uses substring
matching on meta.tokens; replace that with proper parsing of the serde attribute
list: call attr.meta.require_list(), iterate meta.nested and for each
NestedMeta::Meta(Meta::Path(path)) check path.is_ident("skip") ||
path.is_ident("skip_serializing") || path.is_ident("skip_deserializing") and
return true only for those exact idents; leave has_serde_skip signature and use
syn::NestedMeta / syn::Meta types so skip_serializing_if (and other name-value
or path entries) are not treated as matches.
In `@gts-macros/src/gts_serde.rs`:
- Around line 208-225: The generated visit_map currently treats every field as
required by unconditionally calling .ok_or_else(||
serde::de::Error::missing_field(`#field_names`)) on each `#field_idents`; update the
logic inside the visit_map (the block that initializes #(let mut `#field_idents`:
Option<_> = None;)* and then resolves them) to detect when a field is Option<T>
or has serde(default) and in those cases do not call missing_field but instead
leave the Option as None or call T::default() respectively; specifically, adjust
the resolution of `#field_idents` (the code that currently does .ok_or_else(...)?
for each `#field_names`) to conditionally use .unwrap_or(None) /
.unwrap_or_default() semantics for optional/defaulted fields, or alternatively
add a compile-time validation in the macro that rejects fields annotated with
Option<T> or #[serde(default)] to avoid diverging from standard serde behavior.
- Around line 439-495: The Deserialize impl and UnitStructVisitor in
gen_base_unit_struct_serde currently drop the struct generics (the code emits
"impl<'de> serde::Deserialize<'de> for `#struct_name` `#where_clause`" and uses
"type Value = `#struct_name`" / "Ok(`#struct_name`)"), causing a mismatch with the
Serialize impl which correctly uses `#impl_generics` / `#ty_generics` /
`#where_clause`; fix by threading the same impl_generics, ty_generics and
where_clause into the Deserialize emission (use impl `#impl_generics`
serde::Deserialize<'de> for `#struct_name` `#ty_generics` `#where_clause`, set
Visitor::Value = `#struct_name` `#ty_generics`, and return Ok(`#struct_name`
`#ty_generics`) in visit_map/visit_unit). Apply the same pattern to
gen_nested_unit_struct_serde so it also emits generic-aware impls (use the same
`#impl_generics/`#ty_generics/#where_clause there).
- Around line 98-103: The code appends a new where-predicate by interpolating an
existing syn::WhereClause (`where_clause` / `existing`) with `#existing
`#gp_ident`: ::gts::GtsSerialize,` which can produce invalid syntax when the
original clause has no trailing comma; update the logic in `ser_where` and the
other places in `build_where_clause_for_gts` to reconstruct the predicates list
instead of concatenating raw `#existing` tokens: walk or take
`existing.predicates`, extend it with a new `PredicateType` for `#gp_ident:
::gts::GtsSerialize`, and then emit the where clause using a punctuated pattern
like `where #(`#predicates`),*` (matching the approach used in
`build_visitor_where_clause`) so the resulting clause always has proper
punctuation.
In `@gts-macros/src/gts_validation.rs`:
- Around line 23-24: The validation only checks the number of generic type
parameters but not that there are corresponding struct fields using those
generic parameters; update validate_generics (and the other validation block
around gen_base_assertion) to iterate the struct's fields, count/collect which
fields' types are exactly the generic type idents declared on the item, and
ensure each generic parameter has at least one matching field (and that the
total distinct matched generic params equals the number of generic params). Use
the item/type parameter idents and field type paths to perform the match
(referencing validate_generics, validate_field_gts_attrs, and gen_base_assertion
to locate the checks) and return a clear error when a generic parameter has no
corresponding field. Ensure duplicate fields mapping to the same generic param
still counts as satisfying that param but validate all params are covered.
In `@gts-macros/tests/v2_compile_fail/extends_parent_no_generic.rs`:
- Around line 22-31: The test fails for the wrong reason: LeafTypeV1 is missing
#[derive(serde::Serialize)] and the generated const block unconditionally
instantiates a generic GtsSchema assoc (causing E0107) so the parent-generic
assertion never runs; fix by either (A) add #[derive(serde::Serialize)] to
LeafTypeV1 and change the const assertion logic that checks
PARENT_GENERIC_FIELD/is_none() to first verify the parent arity (i.e., only
evaluate <P as GtsSchema>::SCHEMA_ID or related consts when P is declared
generic) or (B) if you intend to accept arity mismatch failures, update the test
comment to state arity errors are an expected failure mode instead of asserting
the parent-generic-field behavior. Ensure references to LeafTypeV1, the const
block that reads PARENT_GENERIC_FIELD, and the <P as GtsSchema>::SCHEMA_ID
instantiation are the locations you change.
---
Outside diff comments:
In `@gts-macros/src/lib.rs`:
- Around line 198-221: get_serde_rename currently slices the token string and
fails when rename isn't the last serde key; instead parse the attribute
structurally: for each attr where attr.path().is_ident("serde") use
attr.parse_meta() (or attr.meta.require_list()) to get a Meta::List and iterate
its nested entries, match each NestedMeta::Meta(Meta::NameValue) whose path
is_ident("rename"), then extract the value by matching the NameValue.lit as
Lit::Str and returning its value; update get_serde_rename to return None if no
such NameValue Lit::Str is found.
- Around line 603-620: The concatenation in build_where_clause produces invalid
Rust by inserting predicates without proper separators; instead, parse the new
predicate from the generic identifier and bounds (use
bounds.parse::<syn::WherePredicate>()) and if where_clause is Some, clone it,
push the parsed predicate into its .predicates (syn::punctuated::Punctuated will
preserve commas), then return the modified where clause as a TokenStream; if
where_clause is None, build a new syn::WhereClause with the single parsed
predicate and return that. Ensure you reference build_where_clause,
generics.type_params().next(), generic_ident, bounds.parse(), and
syn::WhereClause::predicates when locating and implementing the change.
---
Nitpick comments:
In `@gts-macros/src/gts_attrs.rs`:
- Around line 44-90: The parser currently allows duplicate keys (e.g., dir_path,
schema_id, description, extends) to silently overwrite previous values; update
the parsing loop in gts_attrs.rs (the while !input.is_empty() block that parses
key_str and sets dir_path, schema_id, description, extends) to check whether the
target Option is already Some before assigning and, if so, return a
syn::Error::new_spanned(key, "... duplicate attribute ...") describing the
duplicate; for extends treat the explicit "None" vs an ident the same way but
still error if extends was already provided. Ensure each match arm for
"dir_path", "schema_id", "description", and "extends" performs this
duplicate-check and returns an error instead of overwriting.
- Around line 87-89: The attribute parser currently treats the comma separator
as optional (the if input.peek(Token![,]) { input.parse::<Token![,]>()? }
block), allowing malformed attributes like #[gts(dir_path = "a" schema_id =
"b")]; change this so that after parsing a value you check if !input.is_empty()
and if so require a comma: if input.peek(Token![,]) {
input.parse::<Token![,]>()? } else { return Err(syn::Error::new(input.span(),
"expected `,` between attributes")); } — update the loop that consumes
separators (the block using input.peek and input.parse::<Token![,]>()) to
enforce the comma and return a syn::Error when missing.
In `@gts-macros/src/gts_field_attrs.rs`:
- Around line 25-65: The docstring and duplicate-error text in from_field are
inconsistent with the actual duplicate-detection location inside
attr.parse_nested_meta: update the doc comment on from_field to say it rejects
duplicate attributes both across multiple #[gts(...)] blocks and within a single
block, and change the syn::Error message produced when result.is_some() (inside
attr.parse_nested_meta) to something like "GtsSchema: duplicate gts attribute
for this field; only one of type_field, instance_id, or skip is allowed" so it
clearly covers intra-block duplicates; refer to the from_field function, the
attr.parse_nested_meta closure, the result variable, and the GtsFieldAttr
variants when making the text changes.
- Around line 36-65: The parser for field-level directives (inside
attr.parse_nested_meta in gts_field_attrs.rs) currently lets syn produce a
generic error when a user supplies a value (e.g., #[gts(type_field = "x")]);
update the nested-meta handling to explicitly reject value tokens: inside the
closure used by attr.parse_nested_meta, after extracting ident and before
returning Ok(()), check for any attached value (e.g., meta.value or by using
meta.parse_nested_meta to detect `=`/Lit` patterns) and, if present, return a
syn::Error with a GtsSchema-prefixed message (similar wording to struct-level
checks in gts_attrs.rs) indicating that type_field/instance_id/skip do not
accept values; ensure you reference GtsFieldAttr and the existing result
handling so behavior and duplicate-attribute checks remain unchanged.
In `@gts-macros/tests/v2_compile_fail/type_field_wrong_type.rs`:
- Around line 11-15: The test fixture EventV1 relies on validation ordering and
can produce brittle stderr snapshots; update the fixture so the type-mismatch
failure is unambiguous by making the identity field a GtsInstanceId/GtsSchemaId
(e.g., change id to a GtsInstanceId) while keeping #[gts(type_field)] on
event_type, so validate_field_gts_attrs and its is_type_gts_schema_id check will
produce the type-mismatch error rather than triggering the "root struct must
declare exactly one identity field" path; reference EventV1, the
#[gts(type_field)] attribute, validate_field_gts_attrs, and
is_type_gts_schema_id when making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d7c4c10a-6e30-486b-a5cc-b42cc54534d3
📒 Files selected for processing (68)
docs/001-gts-schema-derive-macro-adr.mddocs/002-macro-migration-implementation-plan.mddocs/002-struct-to-gts-schema-migration.mdgts-macros/README.mdgts-macros/src/gts_attrs.rsgts-macros/src/gts_codegen.rsgts-macros/src/gts_field_attrs.rsgts-macros/src/gts_schema_derive.rsgts-macros/src/gts_serde.rsgts-macros/src/gts_validation.rsgts-macros/src/lib.rsgts-macros/tests/compile_fail/base_parent_mismatch.stderrgts-macros/tests/compile_fail/non_gts_generic.stderrgts-macros/tests/v2_compile_fail/both_type_and_instance.rsgts-macros/tests/v2_compile_fail/both_type_and_instance.stderrgts-macros/tests/v2_compile_fail/duplicate_instance_id.rsgts-macros/tests/v2_compile_fail/duplicate_instance_id.stderrgts-macros/tests/v2_compile_fail/duplicate_type_field.rsgts-macros/tests/v2_compile_fail/duplicate_type_field.stderrgts-macros/tests/v2_compile_fail/enum_not_supported.rsgts-macros/tests/v2_compile_fail/enum_not_supported.stderrgts-macros/tests/v2_compile_fail/extends_none_multi_segment.rsgts-macros/tests/v2_compile_fail/extends_none_multi_segment.stderrgts-macros/tests/v2_compile_fail/extends_parent_mismatch.rsgts-macros/tests/v2_compile_fail/extends_parent_mismatch.stderrgts-macros/tests/v2_compile_fail/extends_parent_no_generic.rsgts-macros/tests/v2_compile_fail/extends_parent_no_generic.stderrgts-macros/tests/v2_compile_fail/extends_single_segment.rsgts-macros/tests/v2_compile_fail/extends_single_segment.stderrgts-macros/tests/v2_compile_fail/identity_on_derived_struct.rsgts-macros/tests/v2_compile_fail/identity_on_derived_struct.stderrgts-macros/tests/v2_compile_fail/instance_id_wrong_type.rsgts-macros/tests/v2_compile_fail/instance_id_wrong_type.stderrgts-macros/tests/v2_compile_fail/invalid_gts_id.rsgts-macros/tests/v2_compile_fail/invalid_gts_id.stderrgts-macros/tests/v2_compile_fail/missing_description.rsgts-macros/tests/v2_compile_fail/missing_description.stderrgts-macros/tests/v2_compile_fail/missing_dir_path.rsgts-macros/tests/v2_compile_fail/missing_dir_path.stderrgts-macros/tests/v2_compile_fail/missing_identity_annotation.rsgts-macros/tests/v2_compile_fail/missing_identity_annotation.stderrgts-macros/tests/v2_compile_fail/missing_schema_id.rsgts-macros/tests/v2_compile_fail/missing_schema_id.stderrgts-macros/tests/v2_compile_fail/multiple_generics.rsgts-macros/tests/v2_compile_fail/multiple_generics.stderrgts-macros/tests/v2_compile_fail/nested_direct_serialize.rsgts-macros/tests/v2_compile_fail/nested_direct_serialize.stderrgts-macros/tests/v2_compile_fail/nested_direct_serialize_cfg_attr.rsgts-macros/tests/v2_compile_fail/nested_direct_serialize_cfg_attr.stderrgts-macros/tests/v2_compile_fail/root_multi_segment.rsgts-macros/tests/v2_compile_fail/root_multi_segment.stderrgts-macros/tests/v2_compile_fail/tuple_struct.rsgts-macros/tests/v2_compile_fail/tuple_struct.stderrgts-macros/tests/v2_compile_fail/type_field_wrong_type.rsgts-macros/tests/v2_compile_fail/type_field_wrong_type.stderrgts-macros/tests/v2_compile_fail/unknown_field_attr.rsgts-macros/tests/v2_compile_fail/unknown_field_attr.stderrgts-macros/tests/v2_compile_fail/unknown_gts_attr.rsgts-macros/tests/v2_compile_fail/unknown_gts_attr.stderrgts-macros/tests/v2_compile_fail/version_mismatch.rsgts-macros/tests/v2_compile_fail/version_mismatch.stderrgts-macros/tests/v2_compile_fail_tests.rsgts-macros/tests/v2_inheritance_tests.rsgts-macros/tests/v2_inheritance_tests_mixed.rsgts-macros/tests/v2_integration_tests.rsgts-macros/tests/v2_parity_tests.rsgts-macros/tests/v2_serde_rename_tests.rsgts-macros/tests/v2_serialization_tests.rs
✅ Files skipped from review due to trivial changes (31)
- gts-macros/tests/v2_compile_fail/enum_not_supported.stderr
- gts-macros/tests/v2_compile_fail/instance_id_wrong_type.stderr
- gts-macros/tests/v2_compile_fail/invalid_gts_id.stderr
- gts-macros/tests/v2_compile_fail/tuple_struct.stderr
- gts-macros/tests/v2_compile_fail/duplicate_type_field.stderr
- gts-macros/tests/v2_compile_fail/duplicate_instance_id.stderr
- gts-macros/tests/v2_compile_fail/missing_description.stderr
- gts-macros/tests/v2_compile_fail/both_type_and_instance.stderr
- gts-macros/tests/v2_compile_fail/extends_none_multi_segment.stderr
- gts-macros/tests/v2_compile_fail/extends_parent_no_generic.stderr
- gts-macros/tests/compile_fail/base_parent_mismatch.stderr
- gts-macros/tests/v2_compile_fail/missing_identity_annotation.stderr
- gts-macros/tests/v2_compile_fail/missing_dir_path.stderr
- gts-macros/tests/v2_compile_fail/type_field_wrong_type.stderr
- gts-macros/tests/v2_compile_fail/unknown_field_attr.stderr
- gts-macros/tests/v2_compile_fail/multiple_generics.stderr
- gts-macros/tests/v2_compile_fail/missing_schema_id.stderr
- gts-macros/tests/v2_compile_fail/extends_single_segment.stderr
- gts-macros/tests/v2_compile_fail/unknown_gts_attr.stderr
- gts-macros/tests/v2_compile_fail/identity_on_derived_struct.stderr
- gts-macros/tests/v2_compile_fail/version_mismatch.stderr
- gts-macros/tests/v2_compile_fail/root_multi_segment.stderr
- gts-macros/tests/v2_compile_fail/invalid_gts_id.rs
- docs/002-macro-migration-implementation-plan.md
- gts-macros/tests/v2_compile_fail/nested_direct_serialize.stderr
- gts-macros/tests/v2_compile_fail/missing_schema_id.rs
- gts-macros/tests/v2_compile_fail/extends_parent_mismatch.stderr
- gts-macros/tests/compile_fail/non_gts_generic.stderr
- gts-macros/tests/v2_compile_fail/nested_direct_serialize_cfg_attr.stderr
- gts-macros/tests/v2_compile_fail/tuple_struct.rs
- gts-macros/tests/v2_compile_fail/unknown_gts_attr.rs
…ct/field attributes Replaces the prior plan for a future attribute-macro rewrite with a shipped derive macro. `#[derive(GtsSchema)]` + `#[gts(...)]` helper attributes live alongside the existing `#[struct_to_gts_schema]` attribute macro; coexistence is the steady state, deprecation and removal are out of scope. Highlights: - Struct-level `#[gts(dir_path, schema_id, description, extends)]` and field-level `#[gts(type_field | instance_id | skip)]` attribute grammar. - Identity-field rule enforced at compile time: every root struct declares exactly one of `type_field` / `instance_id`; derived structs (`extends = Parent`) declare neither. `extends = None` is an equivalent explicit root marker. - Generated `pub fn new(...)` constructor on every named-field struct. `type_field` auto-populated from `GtsSchemaId::new(<P as GtsSchema>::SCHEMA_ID)` (generic root) or `Self::gts_schema_id().clone()` (non-generic root); `instance_id` passed by the caller. - Unconditional block on direct `Serialize` / `Deserialize` on derived structs via the `GtsNoDirectSerialize` / `GtsNoDirectDeserialize` marker traits. No opt-out attribute. - Spec-correct `"x-gts-ref": "/$id"` on identity fields; generic `"x-gts-ref": "gts.*"` retained for other `GtsSchemaId` fields. - `description` now emitted into runtime schemas via `gts_schema_with_refs()`. - Per-field serde rename in the nested deserializer (replaces the prior `rename_all = "snake_case"` blanket). Docs: - New ADR (`docs/001-gts-schema-derive-macro-adr.md`). - Migration guide (`docs/002-struct-to-gts-schema-migration.md`) describing the old/new diff, schema-output parity, compile-fail fixture mapping, and the coexistence stance. - Implementation plan (`docs/002-macro-migration-implementation-plan.md`). - Old `001-macro-proposal.md` / `001-macro-alignment-*.md` superseded and removed. - `gts-macros/README.md` rewritten around the derive macro, with the old macro presented as legacy. Tests (all suites green, ~104 tests + 24 trybuild fixtures): - `v2_compile_fail/` — compile-time rejection of invalid configurations (missing identity, wrong field types, schema-ID format, inheritance mismatches, direct-serde on nested, etc.). - `v2_integration_tests` — runtime API + schema output for base structs, incl. generated-constructor behavior. - `v2_inheritance_tests` — multi-level inheritance chains. - `v2_serialization_tests` — `Serialize` / `Deserialize` round-trips through the `GtsSerialize` bridge. - `v2_serde_rename_tests` — per-field `#[serde(rename)]` handling. - `v2_parity_tests` — identical output vs `#[struct_to_gts_schema]` on equivalent structs (with the `x-gts-ref: "/$id"` improvement noted above). - `v2_inheritance_tests_mixed` — interop between old and new macros. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Andre Smith <andre.smith@acronis.com>
asmith987
force-pushed
the
gts-macro-proposal
branch
from
2d5acfb to
034c776
Compare
Author
|
@Artifizer I added the implementation of the macro as part of this PR. The implementation considers all of the comments that was made. The README is here. This PR does not include any CLI modifications, but that would be a follow up. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR contains a proposal to evolve
#[struct_to_gts_schema] into a derive-based#[derive(GtsSchema)]` macro. The goal is to bring the macro into closer alignment with the GTS spec v0.8.0, particularly around identity field requirements.The full proposal is at
docs/001-macro-proposal.md. Some key changes:x-gts-ref: "/$idvsgts.*- the spec distinguishes self-reference from cross-reference. The current macro generatesgts.*for allGtsSchemaIdfields. The new macro would use field annotations such as#[gts(type_field)]or#[gts(instance_id)]to enable this functionalityproperties = "field1,field2"parameterextends = Parentreplacesbase = True / base = Parent- Clearer naming and ifextendsis omitted, the struct is considered root by defaultAll schema output would be structurally identical.
The working implementation is at https://github.com/asmith987/gts-rust/tree/gts-macro-implementation
Relevant docs in the PR:
Summary by CodeRabbit
Release Notes
New Features
#[derive(GtsSchema)]macro with#[gts(...)]attributes for schema configuration and validation.extendswith compile-time validation.Documentation