fix(serde): reject structs with missing required fields#95
Conversation
…alization Previously, if a non-optional struct field was absent from JSON, it was silently default-constructed. This broke variant backtracking: when deserializing variant<A, B>, if A's required fields were missing, A would still "succeed", and B would never be tried. Add compile-time required_field_mask<T>() that computes which field indices must be present (non-skip, non-flatten, non-std::optional). During deserialization, track seen fields via bitmask and reject the struct if any required field is missing. Also fix json_rpc_incoming::result to be std::optional<RawValue> since the result field is only present in response messages. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
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 schema-level Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
Two fixes: 1. stateful_worker: move CompileParams copy after strand lock so concurrent Compile requests don't overwrite doc fields while et::queue work is reading them. DocumentUpdate only marks dirty instead of modifying doc.text to avoid data race with thread pool. 2. master_server: remove whole-document didChange workaround now that eventide correctly rejects variant candidates with missing required fields (clice-io/kotatsu#95). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/serde/utils/reflectable.h`:
- Around line 298-315: is_field_optional() is incorrectly treating flattened
fields as optional because schema::is_field_excluded<T,I>() lumps flatten with
skip; change the logic so flatten does not make a field optional: replace the
single check of schema::is_field_excluded<T,I>() with a distinction between skip
and flatten (e.g., use or add schema::is_field_skipped<T,I>() vs
schema::is_field_flattened<T,I>() or equivalent helpers) and only return true
for actual skips; keep the existing std::optional detection for real optional
types (the branch using refl::field_type and serde::annotated_type stays the
same), and ensure the seen_fields bookkeeping (the code that updates seen_fields
on lookup_field hits) is updated to mark nested flattened inner fields as seen
when their keys are matched so flattened required members no longer bypass the
missing-field check.
- Around line 317-325: The file uses std::uint64_t in the consteval function
required_field_mask() but does not include <cstdint>; add a direct include of
<cstdint> at the top of the header so std::uint64_t is defined without relying
on transitive includes—update the include list in
include/eventide/serde/serde/utils/reflectable.h to `#include` <cstdint> so
required_field_mask() and any other uses of fixed-width integer types compile
reliably.
In `@src/ipc/json_codec.cpp`:
- Around line 85-92: The code incorrectly treats RawValue::empty() as “field
absent” — change the presence checks to rely solely on the optional flags rather
than RawValue::empty(): use envelope->result.has_value() and
envelope->error.has_value() to detect presence, and when result is present
return IncomingResponse{*envelope->id, std::move(envelope->result->data)} even
if the RawValue is the empty/null sentinel; likewise when error is present
return IncomingErrorResponse{*envelope->id, std::move(*envelope->error)}; keep
the mutual-presence and neither-present branches (e.g., InvalidRequest) intact
but remove the empty() checks and any logic that treats empty() as absence.
🪄 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: 05fd5a7d-c5ae-471e-ae5c-e2f2cea61eb2
📒 Files selected for processing (2)
include/eventide/serde/serde/utils/reflectable.hsrc/ipc/json_codec.cpp
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/serde/attrs/schema.h`:
- Around line 116-118: The predicate is_schema_attr_v omits the field-level
attribute optional_field, causing it to be treated as non-schema attribute;
update the is_schema_attr_v predicate to include optional_field in its
type-list/trait checks alongside the other schema attributes so that
optional_field is recognized as part of the closed set (locate the
is_schema_attr_v definition and add optional_field to the same union/trait list
that contains the other schema attribute types).
🪄 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: da8e15d0-655e-4d36-a03d-e97ed36a4bb4
📒 Files selected for processing (4)
include/eventide/serde/serde/attrs/schema.hinclude/eventide/serde/serde/utils/reflectable.hsrc/ipc/json_codec.cpptests/unit/serde/json/simdjson_variant_detail_tests.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/ipc/json_codec.cpp
d7c5c0e to
d7b9176
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
include/eventide/serde/serde/attrs/schema.h (1)
116-118:⚠️ Potential issue | 🟡 Minor
optional_fieldis still missing fromis_schema_attr_v.
schema::optional_field(Line 118) should be part of the closed schema-attribute set inis_schema_attr_v(Line 243-247), otherwise it can be treated as non-schema in validation paths.Suggested fix
template <typename T> constexpr bool is_schema_attr_v = std::is_same_v<T, schema::skip> || std::is_same_v<T, schema::flatten> || + std::is_same_v<T, schema::optional_field> || is_rename_attr<T>::value || is_alias_attr<T>::value || is_literal_attr<T>::value || is_specialization_of<schema::rename_all, T> || std::is_same_v<T, schema::deny_unknown_fields> || is_tagged_attr<T>::value;Also applies to: 243-247
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/eventide/serde/serde/attrs/schema.h` around lines 116 - 118, The schema attribute set is missing schema::optional_field so it can be misclassified; update the compile-time trait is_schema_attr_v to include optional_field alongside the other schema attributes (i.e., add optional_field to the closed set tested by is_schema_attr_v) so that optional_field is recognized as a valid schema attribute during validation and type trait checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@include/eventide/serde/serde/attrs/schema.h`:
- Around line 116-118: The schema attribute set is missing
schema::optional_field so it can be misclassified; update the compile-time trait
is_schema_attr_v to include optional_field alongside the other schema attributes
(i.e., add optional_field to the closed set tested by is_schema_attr_v) so that
optional_field is recognized as a valid schema attribute during validation and
type trait checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b2923a48-c0d2-443b-801d-885d587493cb
📒 Files selected for processing (4)
include/eventide/serde/serde/attrs/schema.hinclude/eventide/serde/serde/utils/reflectable.hsrc/ipc/json_codec.cpptests/unit/serde/json/simdjson_variant_detail_tests.cpp
🚧 Files skipped from review as they are similar to previous changes (3)
- src/ipc/json_codec.cpp
- tests/unit/serde/json/simdjson_variant_detail_tests.cpp
- include/eventide/serde/serde/utils/reflectable.h
d7b9176 to
8fd13db
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
include/eventide/serde/serde/utils/reflectable.h (1)
323-332:⚠️ Potential issue | 🟡 MinorAdd
<cstdint>include forstd::uint64_t.The code uses
std::uint64_t(lines 325, 327, 329) but the file does not directly include<cstdint>. Relying on transitive includes is fragile.🔧 Proposed fix at the top of the file
`#include` <array> `#include` <cstddef> +#include <cstdint> `#include` <expected>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/eventide/serde/serde/utils/reflectable.h` around lines 323 - 332, Add a direct include of <cstdint> to the header so std::uint64_t is defined; update the top of the file that contains the template function required_field_mask() to include <cstdint> (instead of relying on transitive includes) so uses of std::uint64_t inside required_field_mask() and related helper code are well-defined.
🤖 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/serde/attrs/schema.h`:
- Around line 116-119: Add the new field-level attribute struct default_value to
the closed-set predicate is_schema_attr_v so it is recognized as a valid schema
attribute; locate the is_schema_attr_v trait/predicate (the compile-time check
at lines referencing is_schema_attr_v) and include default_value among the
listed allowed types alongside the existing field- and struct-level attribute
types to keep the predicate consistent with the documented closed set.
---
Duplicate comments:
In `@include/eventide/serde/serde/utils/reflectable.h`:
- Around line 323-332: Add a direct include of <cstdint> to the header so
std::uint64_t is defined; update the top of the file that contains the template
function required_field_mask() to include <cstdint> (instead of relying on
transitive includes) so uses of std::uint64_t inside required_field_mask() and
related helper code are well-defined.
🪄 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: 1af5c138-557d-443b-a54b-d3cc67008b29
📒 Files selected for processing (6)
include/eventide/serde/serde/annotation.hinclude/eventide/serde/serde/attrs/schema.hinclude/eventide/serde/serde/utils/reflectable.hsrc/ipc/json_codec.cpptests/unit/serde/json/simdjson_variant_detail_tests.cpptests/unit/serde/standard_case_suite.h
🚧 Files skipped from review as they are similar to previous changes (1)
- src/ipc/json_codec.cpp
0de68c1 to
b3a1a48
Compare
…alization Previously, if a non-optional struct field was absent from the input, it was silently default-constructed. This broke variant backtracking: when deserializing variant<A, B>, if A's required fields were missing, A would still "succeed" and B would never be tried. Semantics aligned with Rust's serde: - Default strict: missing non-optional field → error - std::optional<T> fields are automatically allowed to be absent (like Rust's Option<T>) - schema::default_value attribute (like Rust's #[serde(default)]) allows a field to be absent, keeping its default-constructed value - defaulted<T> convenience alias for annotation<T, schema::default_value> Implementation: - Add required_field_mask<T>() — compile-time bitmask of required field indices (non-excluded, non-optional, no default_value attr) - Track seen_fields in deserialize_reflectable and reject if any required field is missing - Strip cv from refl::field_type before specialization checks - Add forwarding operator= to inherit_use_type annotation for move-only types (e.g. unique_ptr) Also: - Fix json_rpc_incoming::result to use defaulted<RawValue> - Update test structs to use defaulted<T> on nullable fields - Add 8 required field check tests (strict, optional, defaulted) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
b3a1a48 to
acbb38e
Compare
Two fixes: 1. stateful_worker: move CompileParams copy after strand lock so concurrent Compile requests don't overwrite doc fields while et::queue work is reading them. DocumentUpdate only marks dirty instead of modifying doc.text to avoid data race with thread pool. 2. master_server: remove whole-document didChange workaround now that eventide correctly rejects variant candidates with missing required fields (clice-io/kotatsu#95). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
`skip_if_default<T>` omits fields during serialization when they equal the default value, but did not declare them as optional during deserialization. After #95 added required field checking, this caused LSP types using `optional_bool` (= `skip_if_default<bool>`) to reject valid messages when clients omit proposed/newer capability fields. Fix: add `schema::default_value` to `skip_if_default`'s annotation list so the field is recognized as optional during deserialization. This keeps serialization and deserialization semantics orthogonal — `skip_if` alone remains serialization-only, `default_value` alone is deserialization-only, and `skip_if_default` explicitly opts into both. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…389) ## Summary Fix two data races in the stateful worker that caused spurious "redefinition" errors during rapid edits, and remove a didChange workaround that is no longer needed after clice-io/kotatsu#95. ### stateful_worker.cpp **Compile handler**: move `params` → `doc` field copy **after** `strand.lock()`. Previously the copy happened before the lock, so a concurrent Compile request waiting on the strand could overwrite `doc.text` while `et::queue` was reading it on the thread pool: ``` T1: Compile A → doc.text = text_A → lock → et::queue reads doc.text T2: Compile B → doc.text = text_B → waits for strand (overwrites!) T3: et::queue sees text_B instead of text_A → PCH/text mismatch ``` **DocumentUpdate handler**: only mark `dirty`, stop modifying `doc.text`/`doc.version`. The event loop notification can fire while `et::queue` work is running on the thread pool — writing `doc.text` from one thread while reading it from another is a data race. ### master_server.cpp Remove the `{0,0}-{0,0}` range workaround for whole-document `didChange`. eventide's variant deserialization now correctly rejects `TextDocumentContentChangePartial` when the `range` field is absent (clice-io/kotatsu#95), so `TextDocumentContentChangeWholeDocument` is matched as intended. ### protocol.h Remove `text` field from `DocumentUpdateParams` — the worker no longer needs it since DocumentUpdate only sets the dirty flag. ### Integration tests (+312 lines) Extend test_staleness.py from 5 to 14 tests covering document lifecycle: - `didChange` body edit → recompilation with updated diagnostics - `didChange` preamble edit → PCH rebuild + clean recompilation - `didClose` + reopen → compiles fresh from disk - `didClose` → hover returns None - `didSave` header → dependent file recompiles - `didSave` module → CompileGraph dependents invalidated ## Test plan - [x] 422 unit tests pass (426 on CI with extra test suites) - [x] 14 integration tests pass locally - [x] Depends on clice-io/kotatsu#95 (merged) 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Smaller document-update notifications sent to background workers (only path and version). * **Bug Fixes** * Reduced races and unnecessary work between update and compile flows. * Prevented notifications from overwriting in-memory document text, improving state consistency. * Safer concurrent handling to avoid mid-request eviction of active documents. * **Tests** * Added integration tests for staleness, dependency propagation, and LSP lifecycle. * Updated unit tests to match revised update behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary
required_field_mask<T>()and runtimeseen_fieldsbitmask todeserialize_reflectable— if any non-optional, non-excluded field is absent from JSON, deserialization fails withtype_mismatchvariant<A, B>backtracking: if A has a required field missing, A is rejected and B is triedjson_rpc_incoming::resulttostd::optional<RawValue>since theresultfield only appears in JSON-RPC response messagesMotivation
LSP defines
TextDocumentContentChangeEventasvariant<Partial, Whole>.Partialhas a requiredrangefield. When the client sends a whole-document change ({"text": "..."}, norange), the deserializer should rejectPartial(missingrange) and fall back toWhole. Previously,rangewas silently default-constructed to{0,0}-{0,0}, soPartialalways "succeeded" andWholewas never tried.Implementation
is_field_optional<T, I>()— consteval, returns true if field I is excluded (skip/flatten) or its underlying type isstd::optional<T>. Strips cv fromrefl::field_typewhich carriesconstfrom the reflection machinery.required_field_mask<T>()— consteval, computesuint64_tbitmask of required field indices (supports up to 64 fields).deserialize_reflectable, trackseen_fieldsbitmask. After the key-scanning loop, verify(seen_fields & required) == required.Test plan
didChangewith whole-document changes that previously hit the variant bug🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests