feat(serde): add virtual schema core types and tests#105
Open
16bit-ykiko wants to merge 16 commits intomainfrom
Open
feat(serde): add virtual schema core types and tests#10516bit-ykiko wants to merge 16 commits intomainfrom
16bit-ykiko wants to merge 16 commits intomainfrom
Conversation
Introduce compile-time virtual schema infrastructure for the serde framework: - field_info.h: type_kind enum, type_info inheritance hierarchy, field_info struct - field_slot.h: type_list, field_slot compile-time descriptors - virtual_schema.h: schema building with full attribute resolution (skip, flatten, rename, alias, rename_all, default_value, literal, deny_unknown, as, with, enum_string, tagged, skip_if) Tests split into 6 files covering kind_of, type_info, schema attrs, behavior attrs, rename_all policies, and field slots (31 test cases). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…schema_opaque trait - Replace type_info_instance struct specializations with a single constexpr type_info_of<T, Config>() function using if-constexpr dispatch - Add schema_opaque<T> trait to opt out types from recursive schema decomposition - Format with clang-format Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a compile-time serde schema system: fine-grained type_kind/type_info graph, per-field field_info/field_slot metadata, virtual_schema generation for reflectable types (rename/alias/flatten/skip/offsets/flags), FlatBuffers .fbs codegen, and many unit tests with CMake wiring. Changes
Sequence Diagram(s)sequenceDiagram
participant ReflectableType as Reflectable Type
participant VirtualSchema as virtual_schema<T,Config>
participant TypeInfoGraph as type_info graph (static nodes)
participant FbsEmitter as schema::codegen::fbs::emitter
ReflectableType->>VirtualSchema: reflect fields & attributes
VirtualSchema->>TypeInfoGraph: create type_info nodes and field_info array
VirtualSchema->>VirtualSchema: compute slots, offsets, flags
TypeInfoGraph->>FbsEmitter: provide root type_info pointer
FbsEmitter->>FbsEmitter: emit enums, map-entry tables, structs/tables
FbsEmitter-->>User: return .fbs schema text
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
include/eventide/serde/schema/virtual_schema.h (1)
718-723: Clarify thatstruct_type_info::fieldsis intentionally empty.The
struct_type_inforeturned bytype_info_of<T, Config>()for reflectable classes has an emptyfieldsspan. The comment notes this, but it may surprise users who expect the type descriptor to include field information. Consider adding a brief explanation of why this is the case (avoiding circular dependency withvirtual_schema) or providing a helper to retrieve the populated fields.📝 Suggested documentation improvement
} else if constexpr(refl::reflectable_class<V>) { constexpr static struct_type_info info = { {type_kind::structure, refl::type_name<T>()}, - {}, // fields: obtain via virtual_schema<T, Config>::fields + {}, // fields intentionally empty here to avoid circular instantiation. + // Use virtual_schema<T, Config>::fields for field descriptors. }; return &info;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/eventide/serde/schema/virtual_schema.h` around lines 718 - 723, The returned struct_type_info (in type_info_of<T, Config>() for refl::reflectable_class<V>) has an intentionally empty fields span which can confuse users; update the comment where struct_type_info info is constructed to explicitly state that fields is empty to avoid a circular dependency with virtual_schema<T, Config>::fields and explain how callers should obtain the populated field descriptors (e.g., by calling virtual_schema<T, Config>::fields or a new helper function); reference struct_type_info, type_info_of<T, Config>(), and virtual_schema<T, Config>::fields in the comment so readers can find the alternative for retrieving field information.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@include/eventide/serde/schema/virtual_schema.h`:
- Around line 45-56: The push method in struct name_buf writes data[len++]
without bounds checking which can overflow the 64-byte array; update
name_buf::push to verify len < data.size() before writing (use
constexpr-friendly handling), and on violation either assert or throw a clear
exception (e.g., std::out_of_range or std::length_error) or return a failure
indicator; ensure any added check is usable in constexpr contexts (prefer a
static_assert for compile-time guarantees where possible, or a conditional
throw/assert for runtime) and keep name_buf::view() unchanged.
---
Nitpick comments:
In `@include/eventide/serde/schema/virtual_schema.h`:
- Around line 718-723: The returned struct_type_info (in type_info_of<T,
Config>() for refl::reflectable_class<V>) has an intentionally empty fields span
which can confuse users; update the comment where struct_type_info info is
constructed to explicitly state that fields is empty to avoid a circular
dependency with virtual_schema<T, Config>::fields and explain how callers should
obtain the populated field descriptors (e.g., by calling virtual_schema<T,
Config>::fields or a new helper function); reference struct_type_info,
type_info_of<T, Config>(), and virtual_schema<T, Config>::fields in the comment
so readers can find the alternative for retrieving field information.
🪄 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: afdda4d7-6cd3-4886-af55-4127b0f8f418
📒 Files selected for processing (10)
include/eventide/serde/schema/field_info.hinclude/eventide/serde/schema/field_slot.hinclude/eventide/serde/schema/virtual_schema.htests/CMakeLists.txttests/unit/serde/schema/behavior_attrs_tests.cpptests/unit/serde/schema/kind_of_tests.cpptests/unit/serde/schema/rename_all_tests.cpptests/unit/serde/schema/schema_attrs_tests.cpptests/unit/serde/schema/slots_tests.cpptests/unit/serde/schema/type_info_tests.cpp
…_node - Add enum_type_info with member_names/values and underlying_kind - Add struct_info_node with declaration/definition separation to break self-referential constexpr cycles without function pointers - struct_type_info now holds direct std::span<const field_info> - Add is_trivially_copyable to virtual_schema - Add runtime FBS schema generator in serde/schema/codegen/fbs.h - Fix MSVC alias_storage if constexpr false branch evaluation bug - Migrate flatbuffers_schema_tests to use codegen::fbs::render Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@include/eventide/serde/schema/codegen/fbs.h`:
- Around line 84-90: The struct validation wrongly relies on is_scalar() which
counts offset types (string/bytes) as scalars; update the check in the loop that
iterates si->fields to explicitly allow only FlatBuffers inline types (numeric
types, boolean, char) plus enumerations and nested structs: inspect ft->kind
(the type_kind enum) rather than is_scalar(), reject kinds corresponding to
string/bytes/offset-based types, and keep the existing nested-struct check via
is_fbs_struct(ft); modify the condition around ft->is_scalar() / ft->kind to
explicitly accept only the true inline kinds and enumeration before continuing,
otherwise return false.
- Around line 44-47: The emit function must validate the root type before
casting and emitting: check that root->kind == type_kind::structure (or
equivalent) before calling emit_object(root) and before appending the
FlatBuffers "root_type" line; if the check fails, return or emit an appropriate
error/exception/log without calling emit_object or adding root_type. Update emit
(and any callers if needed) to perform this explicit validation using the
type_info->kind enum and only cast to struct_type_info inside emit_object after
the guard.
- Around line 96-120: The wire_fbs_name function currently falls back to
"string" for unknown kinds and allows nested vectors; change it to explicitly
handle type_kind::bytes (map to "byte" or "ubyte" as appropriate via
scalar_fbs_name or equivalent) and make the array/set branch (where
array_type_info and its element are inspected) validate that the element is not
itself an array/set (reject nested vectors by failing/throwing/returning an
error instead of recursing), and remove the permissive default that coerces
unknown kinds to "string"; use unwrap(type_info* ti), normalize_identifier, and
scalar_fbs_name helpers to locate the places to update.
In `@include/eventide/serde/schema/field_info.h`:
- Around line 127-130: The enum_type_info metadata is lossy because
member_values is typed std::span<const std::int64_t>, which cannot represent
enumerators > INT64_MAX even when underlying_kind == type_kind::uint64; change
the representation to preserve full uint64 values (e.g., replace member_values
with a type that can hold both signed and unsigned enumerator domains such as
std::variant<std::span<const std::int64_t>, std::span<const std::uint64_t>> or
two fields like member_signed_values/member_unsigned_values) and update all code
that constructs or reads enum_type_info (places that set/iterate member_values)
to select the unsigned variant/field when underlying_kind == type_kind::uint64
so enum reflection/codegen can round-trip full uint64 enumerators.
- Around line 303-311: The code currently treats any input_range<V> with
format_kind<V> == range_format::disabled as array; change the constexpr chain in
the input_range<V> branch to explicitly check for range_format::disabled (i.e.
if constexpr(fmt == range_format::disabled)) and return the corresponding
disabled sentinel in type_kind instead of falling through to type_kind::array;
update the branch order to test disabled first, then map map->type_kind::map,
set->type_kind::set, else->type_kind::array so intentionally suppressed ranges
are preserved.
In `@tests/unit/serde/flatbuffers/flatbuffers_schema_tests.cpp`:
- Around line 78-82: The tests hardcode type identifiers ("point2d", "color",
"colored_payload") which makes them brittle; change the EXPECT_NE checks to
compute the expected identifiers the same way as other tests by calling
normalize_identifier(refl::type_name<T>()) for each type (e.g., use
normalize_identifier(refl::type_name<point2d>()) and similarly for color and
colored_payload) and then search the generated schema string for those computed
identifiers (the TEST_CASE(render_simple_struct) and the nearby assertions
around lines 84-89 that check the rendered schema).
🪄 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: b16ceea5-64d9-4f61-8446-c20d259ce257
📒 Files selected for processing (4)
include/eventide/serde/schema/codegen/fbs.hinclude/eventide/serde/schema/field_info.hinclude/eventide/serde/schema/virtual_schema.htests/unit/serde/flatbuffers/flatbuffers_schema_tests.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- include/eventide/serde/schema/virtual_schema.h
- Nest if constexpr instead of else-if to prevent MSVC from evaluating discarded branch conditions that reference typename field_t::attrs - Use forward-declared struct_info_node with inline static const definition after virtual_schema to avoid MSVC C2737 error Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
16bit-ykiko
force-pushed
the
feat/virtual-schema
branch
from
e9cbb22 to
3c63466
Compare
…r MSVC
MSVC evaluates both branches of if constexpr in consteval functions,
causing errors when accessing typename T::attrs on non-annotated types.
Replace all if constexpr(has_attrs<T>) { T::attrs } patterns with
unwrap_annotated<T>::attrs which uses template specialization (MSVC-safe)
and returns std::tuple<> for non-annotated types.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
include/eventide/serde/schema/virtual_schema.h (2)
620-726: Consider usingVconsistently fortype_namecalls.The function uses both
refl::type_name<T>()andrefl::type_name<V>()inconsistently (e.g., lines 625, 630, 668 useT; lines 653, 677 useV). SinceV = std::remove_cvref_t<T>, usingVconsistently would avoid any potential differences iftype_namepreserves reference/cv qualifiers.♻️ Suggestion for consistency
if constexpr(schema_opaque<V>) { - constexpr static type_info info = {type_kind::unknown, refl::type_name<T>()}; + constexpr static type_info info = {type_kind::unknown, refl::type_name<V>()}; return &info; } else if constexpr(is_optional_v<V>) { using inner_t = typename V::value_type; constexpr static optional_type_info info = { - {type_kind::optional, refl::type_name<T>()}, + {type_kind::optional, refl::type_name<V>()},Apply similar changes throughout for consistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/eventide/serde/schema/virtual_schema.h` around lines 620 - 726, The code inconsistently passes T and V to refl::type_name inside type_info_of<T, Config>() which can produce different names when T has cv/ref qualifiers; update all refl::type_name<T>() calls to refl::type_name<V>() within type_info_of (including in optional_type_info, pointer/unique_ptr/shared_ptr blocks, map/array/set/tuple/variant/enum and the fallback) so the removed-cvref alias V is used uniformly; locate these in the type_info_of function and replace the occurrences of refl::type_name<T>() with refl::type_name<V>().
746-748: Naming mismatch:is_trivially_copyablechecks for trivial + standard layout, notstd::is_trivially_copyable.The member
is_trivially_copyableusesstd::is_trivial_v<V> && std::is_standard_layout_v<V>, which is stricter thanstd::is_trivially_copyable_v<V>. The comment mentions "POD-like," which matches the implementation but not the name. This could mislead users expecting the standard trait semantics.♻️ Consider renaming for clarity
- /// Whether the struct is trivially copyable (POD-like, candidate for inline/fixed-size layout). - constexpr static bool is_trivially_copyable = - std::is_trivial_v<V> && std::is_standard_layout_v<V>; + /// Whether the struct is trivial and standard-layout (POD-like, candidate for inline/fixed-size layout). + constexpr static bool is_pod_like = + std::is_trivial_v<V> && std::is_standard_layout_v<V>;Alternatively, if the intent is just trivially copyable:
- constexpr static bool is_trivially_copyable = - std::is_trivial_v<V> && std::is_standard_layout_v<V>; + constexpr static bool is_trivially_copyable = std::is_trivially_copyable_v<V>;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/eventide/serde/schema/virtual_schema.h` around lines 746 - 748, The static member is_trivially_copyable on the virtual_schema template is misnamed: it currently computes (std::is_trivial_v<V> && std::is_standard_layout_v<V>) which is stricter than std::is_trivially_copyable_v<V>; rename the member to something that reflects the actual predicate (e.g., is_pod_like or is_trivial_standard_layout) or alternatively change the expression to std::is_trivially_copyable_v<V> if the intent was the standard trait; update the comment to match the chosen semantics and keep references to the symbol is_trivially_copyable (or the new name) and the used type traits (std::is_trivial_v, std::is_standard_layout_v, or std::is_trivially_copyable_v) so reviewers can find and verify the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@include/eventide/serde/schema/virtual_schema.h`:
- Around line 594-605: The lambda in enum_values_as_i64 (values) unconditionally
casts enum underlying values to int64_t which can overflow for unsigned 64-bit
enums; update enum_values_as_i64 to detect the underlying type
(std::underlying_type_t<E>) and, if it is unsigned 64-bit, store values as
std::uint64_t (or use a conditional std::variant/span) instead of casting to
std::int64_t, and update any consumers (e.g., enum_type_info) to accept the
wider/alternative storage; locate refl::reflection<E>::member_values and
enum_values_as_i64::values to implement the conditional storage and adjust
enum_type_info accordingly.
---
Nitpick comments:
In `@include/eventide/serde/schema/virtual_schema.h`:
- Around line 620-726: The code inconsistently passes T and V to refl::type_name
inside type_info_of<T, Config>() which can produce different names when T has
cv/ref qualifiers; update all refl::type_name<T>() calls to refl::type_name<V>()
within type_info_of (including in optional_type_info,
pointer/unique_ptr/shared_ptr blocks, map/array/set/tuple/variant/enum and the
fallback) so the removed-cvref alias V is used uniformly; locate these in the
type_info_of function and replace the occurrences of refl::type_name<T>() with
refl::type_name<V>().
- Around line 746-748: The static member is_trivially_copyable on the
virtual_schema template is misnamed: it currently computes (std::is_trivial_v<V>
&& std::is_standard_layout_v<V>) which is stricter than
std::is_trivially_copyable_v<V>; rename the member to something that reflects
the actual predicate (e.g., is_pod_like or is_trivial_standard_layout) or
alternatively change the expression to std::is_trivially_copyable_v<V> if the
intent was the standard trait; update the comment to match the chosen semantics
and keep references to the symbol is_trivially_copyable (or the new name) and
the used type traits (std::is_trivial_v, std::is_standard_layout_v, or
std::is_trivially_copyable_v) so reviewers can find and verify 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: 5c28715c-13a1-4036-9977-70bbe9258041
📒 Files selected for processing (1)
include/eventide/serde/schema/virtual_schema.h
Move struct_type_info definition after field_info so that std::span<const field_info> sees a complete type when clang-cl eagerly evaluates range concepts. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Remove decorative separator comments (=== and ---), redundant doc comments, and inline comments that restate the code. Simplify kind_of() if-constexpr chain to use consistent else-if style. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace void_t SFINAE with C++20 requires, remove unnecessary remove_cvref_t aliases, use cv-dispatch guard instead of using V alias. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@include/eventide/serde/schema/virtual_schema.h`:
- Around line 583-610: The range branch currently treats any non-map/set formats
as type_kind::array, which incorrectly maps range_format::disabled to array;
update the conditional in the else-if chain that checks constexpr auto fmt =
format_kind<T> to explicitly handle range_format::disabled (e.g., add else if
constexpr(fmt == range_format::disabled) { /* fallthrough to outer branch */ })
so that disabled ranges do not return an array type and instead fall through to
the final branch that yields type_kind::unknown; reference symbols:
format_kind<T>, range_format::disabled, type_kind::array, and type_info_of.
🪄 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: 56cce368-b6bb-4856-b150-4633c3d026d5
📒 Files selected for processing (2)
include/eventide/serde/schema/field_info.hinclude/eventide/serde/schema/virtual_schema.h
Add runtime JSON Schema (draft 2020-12) generation by walking the type_info tree. Supports structs ($ref/$defs for deduplication), enums (string member names), optional/pointer (excluded from required), arrays, sets (uniqueItems), maps (additionalProperties), tuples (prefixItems), variants (oneOf with tag_mode dispatch), and deny_unknown_fields (additionalProperties: false). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Remove yyjson DOM dependency from json_schema.h. The emitter now builds JSON output via direct string append, eliminating the content/dom.h include. Schema module has zero external dependencies. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ching Rewrite JSON Schema and FlatBuffers schema tests to use EXPECT_EQ with complete output strings instead of substring find checks. JSON Schema (93 tests, 2000 lines): scalars with min/max, enums, containers, tuples, structs with $ref/$defs dedup, optional/pointer required exclusion, deny_unknown, skip/flatten/rename/default attrs, variant all 4 tag modes, deep nesting and combination cases. FlatBuffers (113 tests, 2023 lines): scalar mappings, struct vs table, enums with underlying types, array/set/map fields, entry tables, nested dependency ordering, optional unwrap, normalize_identifier edge cases, re-render statelessness, cross-type isolation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
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
1 participant
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.
Summary
virtual_schema<T, Config>that pre-computes field layout, attribute resolution (skip, flatten, rename, alias, rename_all, default, tagged, etc.), and type descriptors for reflectable structsfield_info/field_slot/type_infohierarchy for schema-driven ser/deserschema_opaque<T>trait to opt out custom-hooked types from recursive decompositiontype_info_of<T>()from struct specializations to a single constexpr functionTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests
Chores