Skip to content

Fix attribute-metadata parsing and serde rename-rule parity (for rpc-toolkit TS bindings)#1

Open
helix-nine wants to merge 4 commits into
dr-bonez:masterfrom
helix-nine:fix/ts-bindings-metadata
Open

Fix attribute-metadata parsing and serde rename-rule parity (for rpc-toolkit TS bindings)#1
helix-nine wants to merge 4 commits into
dr-bonez:masterfrom
helix-nine:fix/ts-bindings-metadata

Conversation

@helix-nine
Copy link
Copy Markdown

Foundational fixes in visit-rs-derive needed by rpc-toolkit's feature/new-ts (auto-generated TS bindings). These came out of an adversarial review of both repos; this PR addresses the must-fix metadata/rename findings on the visit-rs side.

What was broken

  1. Multi-item attributes collapsed to Unparsed. parse_meta_to_attribute_meta (and all four rename helpers) parsed an attribute list's tokens with syn::parse2::<Meta> — a single Meta. Any comma-separated attribute failed that parse:

    • #[serde(tag = "t", content = "c")] (adjacently-tagged enums) → AttributeMeta::Unparsed, so tag/content were unreadable downstream.
    • #[serde(rename = "x", default)], #[serde(rename_all = "snake_case", deny_unknown_fields)], etc. → rename/rename_all silently dropped, falling back to the Rust identifier.
    • Split-form #[serde(rename(serialize = "s", deserialize = "d"))] was never recognized.
  2. Rename rules diverged from serde. The hand-rolled case converters used one algorithm for both fields and variants and mishandled acronyms / consecutive capitals (IOErrorioerror instead of serde's i_o_error). serde actually uses two distinct algorithms (apply_to_variant vs apply_to_field; e.g. snake_case is identity for fields, PascalCase is identity for variants).

  3. serde skip ignored. Only #[visit(skip)] was honored; #[serde(skip)] / #[serde(skip_serializing)] fields were still emitted. Also, the Sync where-bound was imposed even for #[visit(skip)]-ed fields.

Changes

  • Parse attribute lists with Punctuated::<Meta, Token![,]>::parse_terminated, recursively — multi-item and nested/split-form attributes now survive as structured AttributeMeta::List { items }.
  • Replace the case converters with a faithful port of serde's RenameRule (apply_to_variant + apply_to_field).
  • Resolve rename from both rename = "..." and rename(serialize=..., deserialize=...) (serialize wins, matching the serialized wire shape).
  • Honor #[serde(skip)] / skip_serializing in field_iter, and only bound non-skipped fields with Sync.
  • Remove the duplicate get_rename_attribute in lib.rs.
  • Bump visit-rs 0.1.9 → 0.1.10, visit-rs-derive 0.1.7 → 0.1.8.

Tests

  • Existing test_case_conversions updated: PascalCase-on-variant is identity per serde (was asserting the old non-serde TestVariant); verified against serde_json.
  • New enum_rename::test_acronym_variant_conversions (IOErrori_o_error, i-o-error, …).
  • New tests/metadata.rs: adjacently-tagged tag+content survive as structured metadata (not Unparsed), and a rename combined with another list item is honored.

cargo test --all is green.

Notes / deferred (found in review, not in this PR)

  • Generic/lifetime enums don't compile (the 12 VisitVariants impls splice ImplGenerics inside impl<__visit_rs__V, …>). Orthogonal to TS-binding metadata; happy to do as a follow-up.
  • #[visit(skip)] / serde skip on enum-variant fields isn't filtered in enum_variants.rs (struct fields are). rpc-toolkit handles this consumer-side via field metadata for now.

cc @dr-bonez — opening from a fork since I only have read access here. rpc-toolkit's companion PR (the actual TS-binding work) depends on this branch and will be linked.

- Parse attribute lists as Punctuated<Meta> so multi-item attributes
  (#[serde(tag="t", content="c")], rename + other items, split-form
  rename(serialize=..., deserialize=...)) survive as structured
  AttributeMeta instead of collapsing to Unparsed.
- Port serde's RenameRule with distinct apply_to_variant / apply_to_field
  algorithms (fixes acronym / consecutive-capital divergence and the
  variant-vs-field asymmetry; PascalCase-on-variant is identity, etc.).
- Honor serde skip / skip_serializing (alongside #[visit(skip)]) when
  filtering struct fields, and don't impose a Sync bound for skipped fields.
- Dedupe the duplicate rename helper; add metadata + acronym tests.
…luated)

The derive emitted `#[cfg(feature = "meta")]` and `cfg!(feature = "meta")`
into consumer code, but those cfgs evaluate against the *consumer* crate's
features, not visit-rs's. A downstream crate without its own `meta` feature
got a StructInfoData with the `metadata` field cfg'd out (E0063) and empty
metadata arrays — making attribute metadata unusable cross-crate.

Make the `metadata` fields and the `metadata` module unconditional, and always
emit the metadata arrays from the derive. `meta` stays as a no-op feature for
back-compat.
@helix-nine
Copy link
Copy Markdown
Author

Companion PR on the rpc-toolkit side (the actual auto-generated TS-bindings work, which depends on this branch): Start9Labs/rpc-toolkit#4

That branch points visit-rs at this fork branch with features = ["meta"]; it should switch back to a published crates.io release once this merges and a new visit-rs version is published.

helix-nine added a commit to Start9Labs/rpc-toolkit that referenced this pull request May 29, 2026
Makes the `ts` feature compile and produce serde-accurate TypeScript.

- Drop the runtime-`syn` SerdeTag hack; resolve the serde enum
  representation (external/internal/adjacent/untagged) from visit-rs's
  runtime AttributeMeta, and emit correct TS for every variant kind:
  external `{"V":payload}` / "V" for unit, internal `{tag:"V"}&payload`,
  adjacent `{tag:"V";content:payload}`, untagged bare payload.
- Add impl_ts_enum! so enums can opt in (parallels impl_ts_struct!).
- impl TS for Option<T> (T|null); honor serde skip/skip_serializing
  (omit) and skip_serializing_if/default (optional `?:`) via field metadata.
- Map all integer widths to `number` (serde_json emits JSON numbers, not
  bigint) and map keys to `{[key:string]:V}` (JSON keys are always strings).
- no_ts() children emit a well-formed `{_PARAMS:unknown;_RETURN:unknown}`
  leaf instead of bare `unknown` (which poisoned the whole tree's inference).
- insert_definition no longer panics in release on name clashes.
- Gate the impl_ts_struct! use behind cfg(ts) so --no-default-features builds.
- Add TSVisitor::into_module + handler_bindings() to emit a complete .d.ts
  module, plus examples/generate_ts.rs.
- Depend on the fixed visit-rs branch (features=["meta"]); revert to a
  published release once dr-bonez/visit-rs#1 merges.
- Tests assert exact TS for structs, all four enum taggings, Option, maps,
  named definitions, and a handler tree; verified end-to-end with tsc --strict.
Wrap the generated enum impls in an anonymous `const _: () = { use ... as _; ... }`
block so their method bodies can use trait-method call syntax without the caller
importing Visit/EnumInfo (matching the struct derive's ergonomics).
@helix-nine
Copy link
Copy Markdown
Author

Pushed an import-free improvement to the VisitVariants derive (commit c82dd9a): the generated enum impls are wrapped in an anonymous const _: () = { use visit_rs::{Visit as _, EnumInfo as _, VisitAsync as _}; ... }; (the technique serde's derive uses), so deriving VisitVariants no longer requires the caller to use visit_rs::{Visit, EnumInfo};. New tests/import_free.rs locks it in. The struct VisitFields derive was already import-free (it fully-qualifies its calls).

Note on generic/lifetime support: I scoped this out and it's a larger derive-wide change than a metadata fix — lifetime params don't compile for structs either, and the enum methods need hygienic lifetimes + per-generic-param bounds (incl. Send/Sync for the async stream methods). Left out of this PR; can be a focused follow-up.

The 12 generated Visit* enum impls now compile for enums with generic type
parameters (and lifetime parameters), where the parameters are 'static —
which covers RPC types (owned, deserialized).

- Emit impl-generics via a helper that injects `__visit_rs__V` in the correct
  position (was `impl<__visit_rs__V, #impl_generics>`, which produced
  `impl<__visit_rs__V, <T>>` for generic enums); splice the enum's own
  where-clause to avoid a duplicate `where`.
- Use a hygienic `'__visit_rs__a` method lifetime (was `'a`, which shadowed an
  enum's own `'a`).
- Drop the explicit `+ '__visit_rs__a` from the RPIT of `&self` iterator methods
  (mirrors the struct derive; the explicit bound forced `T: '__a`).
- Add `#ty: Sync` to the `&self` async methods (their futures capture `&self`)
  and the trait's method-level `where V: Send` bounds.
- Add `Self: 'static` to the visit impls — required because `for<'a> Variant<'a,
  Self>: Visit<V>` borrows Self under an HRTB. Non-generic enums satisfy it
  trivially; generic enums require their parameters to be 'static.
- Add generic/bounded enum tests.
@helix-nine
Copy link
Copy Markdown
Author

Pushed generic + lifetime enum support for VisitVariants (commit a6a1120) — the gap discussed earlier. The 12 generated Visit* enum impls now compile for enums with generic type parameters (and lifetime parameters) as long as the parameters are 'static, which covers RPC types (owned/deserialized).

What it took (the derive now mirrors the struct derive's machinery):

  • Emit impl-generics via a helper that injects __visit_rs__V in the correct position — the old impl<__visit_rs__V, #impl_generics> produced impl<__visit_rs__V, <T>> for generic enums. Also splices the enum's own where-clause to avoid a duplicate where.
  • Hygienic '__visit_rs__a method lifetime (the old 'a shadowed an enum's own 'a).
  • Drop the explicit + '__visit_rs__a from the RPIT of &self iterator methods (matches the struct derive; the explicit bound forced T: '__a).
  • #ty: Sync on the &self async methods (their futures capture &self) + the trait's method-level where V: Send bounds.
  • Self: 'static on the visit impls — required because for<'a> Variant<'a, Self>: Visit<V> borrows Self under an HRTB, which is unsatisfiable for a non-'static Self. Non-generic enums satisfy it trivially.

New tests in tests/enum.rs: a generic enum (GenEnum<T>), a bounded generic with a where-clause + rename_all (Bounded<T: Clone> where T: Send), exercising EnumInfo + the field-visiting traits + a runtime visit. Full suite green, 0 warnings.

Caveat (documented): generic params must be 'static, and a lifetime enum only works with the 'static lifetime — the for<'a> Variant<'a, Self>: Visit HRTB borrows Self, so a non-'static Self is fundamentally unsatisfiable there. Non-'static (borrowed) types are out of scope for RPC anyway. (The struct derive has the same effective limitation: its async methods don't compile for a borrowing lifetime struct either.)

@helix-nine helix-nine force-pushed the fix/ts-bindings-metadata branch from a6a1120 to 8fb09da Compare June 2, 2026 20:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant