Repository navigation
Expose the location of each parse failure via rfl::Error::details() - #766
clockblocker wants to merge 4 commits into
Conversation
rfl::Error keeps its message, but now also carries the individual failures it summarizes, each with the path of field names and array indices at which it occurred. The readers that prefix nested messages with "Failed to parse field ..." record a path segment at the same place, so programs no longer need to parse what() to find out which field failed. Closes munich-data-engineering#765. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
A note on why
If you'd prefer |
|
|
@clockblocker the PR looks great. Thanks for the initiative. I have minor comments (see above). Also, the build fails on at least some of the compiler settings. Could you take a look? |
Clang 17 does not support floating-point non-type template arguments, so rfl::Minimum<0.0> fails to compile in the JSON-C++20-llvm-17 job. An integer bound tests the same thing. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Thank you for the review. Don't see any "Minor comments" beyond the failing build, though. Ai-authored: Build failure: fixed in 0cc1fcd. The new test used |
| } | ||
| }; | ||
|
|
||
| using DetailsPtr = std::unique_ptr<std::vector<ErrorDetail>, DeleteDetails>; |
There was a problem hiding this comment.
@clockblocker Could you use rfl::Box here? Clearly, the implicit assumption here is that DetailsPtr should not be NULL, so rfl::Box establishes that more clearly.
Moreover, if you use rfl::Box, you won't have to implement the copy operations by hand.
There was a problem hiding this comment.
Sorry for the delay. This turned out to be less straightforward than it seemed.
While the end user sees both path and message as non-nullable, internally most
errors have neither.
Jarvis'es benchmarks got worse after the rfl::Box switch. He explained it like this:
// AI-Authored:
An error built from a message alone carries no details, and details() derives one on the fly
(an empty path plus the message).
So "no details" is the common case, and storing it as nothing keeps plain errors allocation-free and
rfl::Result small.
rfl::Box can't express that: it is never empty, so every error would allocate, and successful reads got 6% slower.
I built and benchmarked the rfl::Box version.
It had issues:
- Every error allocates. A
Boxis never empty, so even a plainrfl::error("..."), which has no details, gets a heap-allocated vector. Copying a plain error became 9.6× / 2.5× slower, with an extra allocation. - Successful reads got 6% slower on both compilers.
- It can't be included as is.
Box.hppincludesResult.hpp, which includesinternal/Error.hpp, so I had to splitBox.hppin two. - A moved-from
Errorbreaks. A moved-fromBoxis empty, so callingdetails()on that error or copying it dereferences a null pointer.
Then I prototyped the std::vector and std::shared_ptr versions. Each had tradeoffs:
- Inline
std::vectoris the simplest code, butErrorgrows by 16 bytes. Successful reads get 3–5% slower, and on GCC failures get up to 41% slower. std::shared_ptrmakes copying an error with details about 9× cheaper, butErrorgrows by 8 bytes, and most failure cases get 10–27% slower, becausedetails() &&has to copy the shared details at every level of nesting.
Finally, I settled on Error::OptionalDetails (a038bc0), a small private type that holds either nothing or a heap-allocated vector, and deep-copies it:
- Its name and
has_value()make it explicit that the details can be empty, so that is no longer an implicit assumption. Error's special members are= defaultagain, exactly as onmain, soErrorhas no hand-written copy operations.- It costs nothing measurable:
Errorkeeps its size, and the timings match the previous version within noise.
Measurements: each cell shows Clang on the first line and GCC on the second. The first column is the previous version; the others show the change relative to it.
| Clang / GCC |
Previous version (unique_ptr) |
OptionalDetails (now in this PR) |
inline std::vector
|
rfl::CopyableBox |
std::shared_ptr |
|---|---|---|---|---|---|
| Successful read, 2,000 objects | 687 µs / 929 µs |
|
+3 % / +5 % |
+6 % / +6 % |
+2 % / +1 % |
| Untagged variant, 3rd alternative matches | 1232 ns / 1386 ns |
+1 % / ±0 % |
+3 % / −1 % |
+2 % / +3 % |
+10 % / +2 % |
| Validator failure | 3832 ns / 5648 ns |
−9 % / ±0 % |
|
−9 % / +21 % |
−12 % / +10 % |
| Failure 2 arrays deep | 666 ns / 896 ns |
−3 % / ±0 % |
−7 % / −7 % |
+16 % / +4 % |
|
| 4 nested failures | 5974 ns / 8294 ns |
±0 % / ±0 % |
|
+2 % / +14 % |
+12 % / +17 % |
| Copy an error with details | 261 ns / 304 ns |
±0 % / +3 % |
−19 % / −16 % |
−2 % / +1 % |
|
| Copy a plain error | 2.1 ns / 26 ns |
±0 % / +7 % |
±0 % / −2 % |
|
−38 % / −6 % |
sizeof(Error) |
32 bytes / 40 bytes |
32 bytes / 40 bytes |
48 bytes / 56 bytes |
32 bytes / 40 bytes |
40 bytes / 48 bytes |
How I measured: Apple Clang 17 with libc++ and GCC 16 with libstdc++, both -O3, on an arm64 Mac, best of 8 interleaved runs. All five versions include 27ebb4d (below).
Two more things came out of this:
-
27ebb4d, "Move errors out of failed reads instead of copying them": the parsers kept the errors they pass upward in
constlocals and returned them witherror(*err)orreturn err;, which copies them. That got expensive once errors carried details. For example, reading astd::variant<Circle, Rectangle, Square>that matches the third alternative went from 1046 ns and 17 allocations onmainto 1676 ns and 31 allocations. With the fix it takes 1232 ns and 21 allocations (Clang). The commit addserror(Error&&)and touches each format'sReader, but only to dropconstfrom a local that is returned. -
rfl::CopyableBoxcan't be copy-assigned today. Its copy assignment operator takesconst Box<T>&, which is the non-copyableBox, so the real copy assignment is implicitly deleted:b = a;with twoCopyableBox<int>doesn't compile onmain. A separate PR for that: Make rfl::CopyableBox copy-assignable #769
| using DetailsPtr = std::unique_ptr<std::vector<ErrorDetail>, DeleteDetails>; | ||
|
|
||
| RFL_COLD static DetailsPtr copy_details(const DetailsPtr& _ptr) { | ||
| return _ptr ? DetailsPtr(new std::vector<ErrorDetail>(*_ptr)) : nullptr; |
There was a problem hiding this comment.
This is what I mean...rfl::Box can handle that more elegantly.
There was a problem hiding this comment.
Ai-authored:
copy_details is gone in a038bc0: the deep copy now lives in Error::OptionalDetails, and Error's copy operations are = default again. The reasons and measurements are in the thread above.
|
@clockblocker you should see the minor comments now |
The parsers kept the errors they propagate in const locals, such as `const auto [set, err] = ...` or `const auto err = ...`, and passed them on with `error(*err)` or `return err;`. Both copy the error: rfl::error only accepted a const Error&, and a const local cannot be moved from. Copying an error used to copy one string. Now that errors carry their details, it also copies every path and message, at every level the error passes through. Reading an untagged variant whose first alternatives fail became about 60% slower than before details existed. Add an rfl::error overload that takes an Error&&, and move the errors instead of copying them. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Error held its details in a unique_ptr with a cold deleter and defined its copy operations by hand to deep-copy them. A null pointer meant "this error is just its message", which a reader had to infer from a comment. The pointer now lives in Error::OptionalDetails, a small private type that holds either nothing or a heap-allocated vector, and deep-copies it. Its name and has_value() make the empty state explicit. Because it is copyable, Error's special members are defaulted again, exactly as they were before details existed. Runtime behaviour does not change: errors that are just their message do not allocate, the member is still a single pointer, and deleting and copying the details stay out of line (RFL_COLD). rfl::Box does not fit here: it is never empty, so every error would allocate, and Box.hpp includes Result.hpp, which includes this header. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Why
I'm using the library in the production project to establish a source of truth for the chain:
Part of the app is a complex form with tricky (yet expressible in Standard Schema) validation rules.
Would like to express the {message, path} in the source-of-truth struct, so that all the clients can expect to send messages like:
and receive errors like:
AI-authored
Context
TypeScript validation libraries such as Zod, Valibot, ArkType and Effect Schema report failures in the Standard Schema shape: a list of issues, each with a
messageand apath. A C++ backend that validates with reflect-cpp can't produce that list today.what()is a single prose string, so a client can't tell which form field to highlight unless it parses the message text.details()maps directly onto Standard Schema issues. EachErrorDetailbecomes one issue, field names become string keys, and indices become numbers. A web client can then show backend validation errors the same way it shows its own.Summary
Closes #765.
rfl::Errorkeepswhat()exactly as it is, and addsdetails(): one entry per failure, each with the path where it occurred.An error that was built from a message alone reports itself as one detail with an empty path. Errors from custom parsers and validators therefore need no changes.
Each reader adds a path segment at the point where it already prefixes the message:
ViewReader / MapReader "Failed to parse field 'x': ..." + segment "x" NamedTupleParser "Field named 'x' not found." + segment "x" ViewReader (NoExtraFields) "Value named 'x' not used." + segment "x" ArrayReader / TupleReader "Failed to parse element/field i" + segment i +VectorReader (message unchanged) + segment i ParserTaggedUnion prefix only; inner paths are kept to_single_error (new) joins messages as before, concatenates detailsUntagged variants stay a single failure at the variant's location, because reflect-cpp can't tell which alternative the input was meant to be. The joined
what()still shows at most 10 errors, butdetails()contains all of them.FieldErrorsandMapReadernow collectErrors instead of strings. The new helpers live inparsing/wrap_error.hpp. Docs are indocs/content/result.mdunder "Locating errors".Evidence
The issue's example, via
rfl::cli::read(tests/cli/test_errors.cpp):The index
1inretriesexists only indetails(). Today'swhat()doesn't say which element failed.tests/json/test_error_details.cppcovers nested structs in vectors withSnakeCaseToCamelCase, maps,std::array, tuples, sets with duplicates,NoExtraFields,FailFast,DefaultIfMissing, tagged unions, untagged variants, and root-level errors. Existing tests that comparewhat()exactly still pass, which shows the messages are unchanged.Local runs (Apple clang 17,
REFLECTCPP_USE_VCPKG=OFF): JSON 273, CLI 72, generic 98, env 41, json_c_arrays_and_inheritance 7, all passing. The new tests plustest_error_messages,test_fail_fastandtest_tagged_unionalso pass withREFLECTCPP_USE_STD_EXPECTED/ C++23. I couldn't build the other formats locally, so CI will be their first run. They share the changed parsing code, and no format-specific code uses the changed internals.Performance. Benchmark: read a
std::vectorof 100k nested structs from JSON, 40 runs, median of interleaved rounds.My first version stored the details as a plain
std::vectormember and cost +8.5%. Bisecting showed the cause:~Error()is inlined into everyResult<T>destructor, and~vector<ErrorDetail>is a lot of code. So the details now sit behind aunique_ptrwhose deleter and copy are out of line, using a newRFL_COLDmacro (noinline+cold). The error-wrapping helpers use it too.sizeof(rfl::Error)grows by 8 bytes (24 → 32 on libc++). The vector index comes fromsize()instead of a per-element counter; maps and sets keep a counter because duplicates don't growsize().Merge Danger
Door: two-way. The API is purely additive and nothing is persisted.
Blast Radius: small. Two things could affect downstream code:
sizeof(rfl::Error)or on its copy being trivially defaulted (unlikely).rfl::parsing::MapReaderdirectly or depends onFieldErrorsbeingstd::vector<std::string>. Both are internal.Not included: the optional "kind" field (missing / extra / validation / wrong type) from the issue. I can add it here or in a follow-up if you want it.
🤖 Generated with Claude Code