From 50199ea52f33b29f7ffade693eac76887df1945e Mon Sep 17 00:00:00 2001 From: clockblocker Date: Tue, 3 Nov 2026 10:50:53 +0100 Subject: [PATCH 1/4] Expose the location of each parse failure via rfl::Error::details() 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 #765. Co-Authored-By: Claude Opus 5.5 --- docs/content/result.md | 49 +++++ include/rfl/Result.hpp | 1 + include/rfl/internal/Error.hpp | 85 ++++++++- include/rfl/internal/cold.hpp | 15 ++ include/rfl/parsing/ArrayReader.hpp | 7 +- include/rfl/parsing/FieldErrors.hpp | 10 +- include/rfl/parsing/MapParser.hpp | 4 +- include/rfl/parsing/MapReader.hpp | 8 +- include/rfl/parsing/NamedTupleParser.hpp | 8 +- include/rfl/parsing/ParserTaggedUnion.hpp | 8 +- include/rfl/parsing/Parser_default.hpp | 2 +- include/rfl/parsing/TupleReader.hpp | 14 +- include/rfl/parsing/VectorReader.hpp | 23 ++- include/rfl/parsing/ViewReader.hpp | 25 ++- include/rfl/parsing/ViewReaderWithDefault.hpp | 15 +- ...ReaderWithDefaultAndStrippedFieldNames.hpp | 6 +- .../ViewReaderWithStrippedFieldNames.hpp | 6 +- .../rfl/parsing/to_single_error_message.hpp | 27 +++ include/rfl/parsing/wrap_error.hpp | 87 +++++++++ tests/cli/test_errors.cpp | 29 +++ tests/json/test_error_details.cpp | 176 ++++++++++++++++++ 21 files changed, 547 insertions(+), 58 deletions(-) create mode 100644 include/rfl/internal/cold.hpp create mode 100644 include/rfl/parsing/wrap_error.hpp create mode 100644 tests/json/test_error_details.cpp diff --git a/docs/content/result.md b/docs/content/result.md index f8b11969d..fb59c2280 100644 --- a/docs/content/result.md +++ b/docs/content/result.md @@ -133,6 +133,55 @@ if (my_result) { } ``` +## Locating errors + +`.what()` returns a message meant to be read by humans. When an error summarizes several failures, for example because more than one field is invalid, the message lists all of them: + +``` +Found 2 errors: +1) Failed to parse field 'inputFile': Size validation failed: Value expected to be greater than or equal to 5, but got 4. +2) Field named 'outputFile' not found. +``` + +If your program needs to know where each failure occurred, for instance to show each message next to the right input in a form, use `.details()` instead. It returns a `std::vector` with one entry per failure: + +```cpp +struct ErrorDetail { + using PathSegment = std::variant; + + // Field names and array indices, starting from the root. + std::vector path; + + // The message, without the "Failed to parse field '...': " prefixes. + std::string message; +}; +``` + +Field names are spelled as they appear in the input, that is, after processors such as `rfl::SnakeCaseToCamelCase` or `rfl::SnakeCaseToKebabCase` (which `rfl::cli::read` uses) have been applied. This means the paths line up with the JSON schema generated by `rfl::json::to_schema`. + +```cpp +struct FileSpec { + rfl::Validator>> input_file; + std::string output_file; +}; + +struct Request { + std::vector files; +}; + +const auto result = rfl::json::read( + R"({"files":[{"inputFile":"a.txt","outputFile":"a.out"},{"inputFile":"b"}]})"); + +for (const auto& detail : result.error().details()) { + // First iteration: path = {"files", 1, "inputFile"}, + // message = "Size validation failed: ..." + // Second iteration: path = {"files", 1, "outputFile"}, + // message = "Field named 'outputFile' not found." +} +``` + +An error that does not belong to any particular field, such as a syntax error in the document, is reported as a single detail with an empty path. Untagged variants are also reported as a single detail at the location of the variant, because reflect-cpp cannot know which alternative the input was meant to be. For `rfl::TaggedUnion`, the tag determines the alternative, so the paths extend into the fields of that alternative. + ## `rfl::error` `rfl::error("error message")` is a simple utility function for `rfl::Unexpected(Error("error message"))`. diff --git a/include/rfl/Result.hpp b/include/rfl/Result.hpp index c74c91cc6..0a92a6bc4 100644 --- a/include/rfl/Result.hpp +++ b/include/rfl/Result.hpp @@ -12,6 +12,7 @@ namespace rfl { using Error = rfl::internal::Error; +using ErrorDetail = rfl::internal::ErrorDetail; /// To be returned when there is nothing to return, but there might be an error. struct Nothing {}; diff --git a/include/rfl/internal/Error.hpp b/include/rfl/internal/Error.hpp index 20231c33f..f8b96d969 100644 --- a/include/rfl/internal/Error.hpp +++ b/include/rfl/internal/Error.hpp @@ -1,10 +1,36 @@ #ifndef RFL_INTERNAL_ERROR_HPP_ #define RFL_INTERNAL_ERROR_HPP_ +#include +#include #include +#include +#include +#include + +#include "cold.hpp" namespace rfl::internal { +/// Describes a single failure contained in an Error: where it occurred and +/// what went wrong. +struct ErrorDetail { + /// A field name or an array index. + using PathSegment = std::variant; + + /// The location of the failure, starting from the root of the value being + /// read. Field names are spelled as they appear in the input, that is, after + /// processors such as rfl::SnakeCaseToCamelCase have been applied. An empty + /// path refers to the root value itself. + std::vector path; + + /// The message describing the failure, without the "Failed to parse field + /// '...': " prefixes that Error::what() contains for every level of nesting. + std::string message; + + bool operator==(const ErrorDetail&) const = default; +}; + /// Defines the error class to be returned when something went wrong. /// This is used throughout reflect-cpp for error handling. class Error { @@ -20,12 +46,22 @@ class Error { /// @param _what The error message describing what went wrong (will be moved) Error(std::string&& _what) : what_(std::move(_what)) {} + /// Constructs an error with a message and the individual failures it + /// summarizes. + /// @param _what The error message describing what went wrong + /// @param _details The individual failures, see details() + Error(std::string _what, std::vector _details) + : what_(std::move(_what)), + details_(_details.empty() + ? nullptr + : new std::vector(std::move(_details))) {} + /// Destructor. ~Error() = default; /// Copy constructor. /// @param e The error to copy from - Error(const Error& e) = default; + Error(const Error& e) : what_(e.what_), details_(copy_details(e.details_)) {} /// Move constructor. /// @param e The error to move from @@ -34,7 +70,13 @@ class Error { /// Copy assignment operator. /// @param _other The error to copy from /// @return Reference to this error - Error& operator=(const Error& _other) = default; + Error& operator=(const Error& _other) { + if (this != &_other) { + what_ = _other.what_; + details_ = copy_details(_other.details_); + } + return *this; + } /// Move assignment operator. /// @param _other The error to move from @@ -47,9 +89,48 @@ class Error { /// from state std::string what() && { return std::move(what_); } + /// Returns the individual failures summarized by what(), each with the + /// location at which it occurred. An error that was constructed from a + /// message only is reported as a single failure at the root. + std::vector details() const& { + if (!details_) { + return {ErrorDetail{.path = {}, .message = what_}}; + } + return *details_; + } + + /// Moves the individual failures out of the Error object and leaves it in a + /// moved from state. + std::vector details() && { + if (!details_) { + return {ErrorDetail{.path = {}, .message = std::move(what_)}}; + } + return std::move(*details_); + } + + private: + /// Errors are created and destroyed in many places, mostly as part of + /// rfl::Result. Deleting and copying the details out of line keeps that code + /// small, which matters for the performance of successful reads. + struct DeleteDetails { + RFL_COLD void operator()(std::vector* _ptr) const { + delete _ptr; + } + }; + + using DetailsPtr = std::unique_ptr, DeleteDetails>; + + RFL_COLD static DetailsPtr copy_details(const DetailsPtr& _ptr) { + return _ptr ? DetailsPtr(new std::vector(*_ptr)) : nullptr; + } + private: /// Documents what went wrong std::string what_; + + /// The individual failures summarized by what_. Null, unless what_ was + /// assembled from failures at nested locations. + DetailsPtr details_; }; } // namespace rfl::internal diff --git a/include/rfl/internal/cold.hpp b/include/rfl/internal/cold.hpp new file mode 100644 index 000000000..b714a7147 --- /dev/null +++ b/include/rfl/internal/cold.hpp @@ -0,0 +1,15 @@ +#ifndef RFL_INTERNAL_COLD_HPP_ +#define RFL_INTERNAL_COLD_HPP_ + +/// Marks a function that is rarely called, such as one that only runs when +/// something went wrong. Such functions are kept out of the code of their +/// callers, so that they do not slow down the common case. +#if defined(_MSC_VER) +#define RFL_COLD __declspec(noinline) +#elif defined(__GNUC__) || defined(__clang__) +#define RFL_COLD __attribute__((noinline, cold)) +#else +#define RFL_COLD +#endif + +#endif diff --git a/include/rfl/parsing/ArrayReader.hpp b/include/rfl/parsing/ArrayReader.hpp index e4f094b24..eb600ee5a 100644 --- a/include/rfl/parsing/ArrayReader.hpp +++ b/include/rfl/parsing/ArrayReader.hpp @@ -3,11 +3,13 @@ #include #include +#include #include #include "../Result.hpp" #include "../internal/is_array.hpp" #include "Parser_base.hpp" +#include "wrap_error.hpp" namespace rfl::parsing { @@ -62,8 +64,9 @@ class ArrayReader { if (res) { move_to(&((*array_)[num_set_]), &(*res)); } else { - return Error("Failed to parse element " + std::to_string(num_set_) + - ": " + res.error().what()); + return wrap_error( + "Failed to parse element " + std::to_string(num_set_) + ": ", + num_set_, std::move(res).error()); } ++num_set_; return std::optional(); diff --git a/include/rfl/parsing/FieldErrors.hpp b/include/rfl/parsing/FieldErrors.hpp index 37bcde887..1bf7101e3 100644 --- a/include/rfl/parsing/FieldErrors.hpp +++ b/include/rfl/parsing/FieldErrors.hpp @@ -3,7 +3,6 @@ #include #include -#include #include #include #include @@ -19,8 +18,8 @@ class FirstFieldError { void reserve(std::size_t) {} - void emplace_back(std::string message) { - if (!error_) error_.emplace(std::move(message)); + void emplace_back(Error _err) { + if (!error_) error_.emplace(std::move(_err)); } std::optional error() && { return std::move(error_); } @@ -30,9 +29,8 @@ class FirstFieldError { }; template -using FieldErrors = - std::conditional_t, FirstFieldError, - std::vector>; +using FieldErrors = std::conditional_t, + FirstFieldError, std::vector>; template bool should_stop_on_field_error(const FieldErrors& errors) { diff --git a/include/rfl/parsing/MapParser.hpp b/include/rfl/parsing/MapParser.hpp index eda6ddd00..c03574b13 100644 --- a/include/rfl/parsing/MapParser.hpp +++ b/include/rfl/parsing/MapParser.hpp @@ -96,7 +96,7 @@ struct MapParser { } return map; } else { - std::vector errors; + std::vector errors; const auto map_reader = MapReader(&_r, &map, &errors); if constexpr (schemaful::IsSchemafulReader) { @@ -111,7 +111,7 @@ struct MapParser { } } if (errors.size() != 0) { - return error(to_single_error_message(errors)); + return error(to_single_error(std::move(errors))); } return map; } diff --git a/include/rfl/parsing/MapReader.hpp b/include/rfl/parsing/MapReader.hpp index fe0cd9a1e..fe1e8ffdc 100644 --- a/include/rfl/parsing/MapReader.hpp +++ b/include/rfl/parsing/MapReader.hpp @@ -11,6 +11,7 @@ #include "../always_false.hpp" #include "../internal/has_reflection_type_v.hpp" #include "Parser_base.hpp" +#include "wrap_error.hpp" namespace rfl::parsing { @@ -30,7 +31,7 @@ class MapReader { * @param _map The map to write to. * @param _errors The vector to collect errors in. */ - MapReader(const R* _r, MapType* _map, std::vector* _errors) + MapReader(const R* _r, MapType* _map, std::vector* _errors) : r_(_r), map_(_map), errors_(_errors) {} ~MapReader() = default; @@ -47,8 +48,7 @@ class MapReader { if (res) { map_->emplace(std::move(*res)); } else { - errors_->push_back("Failed to parse field '" + std::string(_name) + - "': " + res.error().what()); + errors_->emplace_back(wrap_field_error(_name, std::move(res).error())); } } @@ -122,7 +122,7 @@ class MapReader { MapType* map_; /// Collects any errors we may have come across. - std::vector* errors_; + std::vector* errors_; }; } // namespace rfl::parsing diff --git a/include/rfl/parsing/NamedTupleParser.hpp b/include/rfl/parsing/NamedTupleParser.hpp index f5243352b..66ce4d397 100644 --- a/include/rfl/parsing/NamedTupleParser.hpp +++ b/include/rfl/parsing/NamedTupleParser.hpp @@ -36,6 +36,7 @@ #include "is_required.hpp" #include "schema/Type.hpp" #include "to_single_error_message.hpp" +#include "wrap_error.hpp" namespace rfl { @@ -348,7 +349,8 @@ struct NamedTupleParser { std::stringstream stream; stream << "Field named '" << std::string(current_name) << "' not found."; - _errors->emplace_back(stream.str()); + _errors->emplace_back( + make_nested_error(std::string(current_name), stream.str())); } else if constexpr (!internal::has_default_val_v) { if constexpr (!std::is_const_v) { @@ -410,7 +412,7 @@ struct NamedTupleParser { return std::make_pair(set, std::move(errors).error()); } else { if (errors.size() != 0) [[unlikely]] { - return std::make_pair(set, to_single_error_message(errors)); + return std::make_pair(set, to_single_error(std::move(errors))); } return std::make_pair(set, std::optional()); } @@ -443,7 +445,7 @@ struct NamedTupleParser { return std::move(errors).error(); } else { if (errors.size() != 0) [[unlikely]] { - return to_single_error_message(errors); + return to_single_error(std::move(errors)); } return std::nullopt; } diff --git a/include/rfl/parsing/ParserTaggedUnion.hpp b/include/rfl/parsing/ParserTaggedUnion.hpp index 2cd2dc974..cafbf1ce0 100644 --- a/include/rfl/parsing/ParserTaggedUnion.hpp +++ b/include/rfl/parsing/ParserTaggedUnion.hpp @@ -19,6 +19,7 @@ #include "schemaful/IsSchemafulReader.hpp" #include "schemaful/IsSchemafulWriter.hpp" #include "tagged_union_wrapper_no_ptr.hpp" +#include "wrap_error.hpp" namespace rfl::parsing { @@ -182,13 +183,12 @@ struct ParserTaggedUnion, std::move(_val)); }; - const auto embellish_error = [&](auto&& _e) { + const auto embellish_error = [&](Error _e) { std::stringstream stream; stream << "Could not parse tagged union with " "discriminator " - << _discriminator.str() << " '" << _disc_value - << "': " << _e.what(); - return Error(stream.str()); + << _discriminator.str() << " '" << _disc_value << "': "; + return wrap_error(stream.str(), std::move(_e)); }; using T = tagged_union_wrapper_no_ptr_t::read_view(_r, _var, &view); if (err) [[unlikely]] { call_destructors_where_necessary(set, &view); - return error(err->what()); + return error(*err); } auto res = Result(std::move(*ptr)); call_destructors_where_necessary(set, &view); diff --git a/include/rfl/parsing/TupleReader.hpp b/include/rfl/parsing/TupleReader.hpp index 15ecf5f7e..bc48a8700 100644 --- a/include/rfl/parsing/TupleReader.hpp +++ b/include/rfl/parsing/TupleReader.hpp @@ -2,6 +2,7 @@ #define RFL_PARSING_TUPLEREADER_HPP_ #include +#include #include #include "../Result.hpp" @@ -10,6 +11,7 @@ #include "AreReaderAndWriter.hpp" #include "Parser_base.hpp" #include "is_required.hpp" +#include "wrap_error.hpp" namespace rfl::parsing { @@ -74,8 +76,9 @@ class TupleReader { if constexpr (_all_required || is_required()) { - *_err = Error("Field " + std::to_string(_i) + - " was required, but missing."); + *_err = make_nested_error( + static_cast(_i), + "Field " + std::to_string(_i) + " was required, but missing."); return; } else { ::new (&(rfl::get<_i>(*tuple_))) CurrentType(); @@ -99,10 +102,9 @@ class TupleReader { move_to(&(rfl::get<_i>(*tuple_)), &(*res)); ++num_set_; } else { - std::stringstream stream; - stream << "Failed to parse field " << _i << ": " - << res.error().what(); - *_err = Error(stream.str()); + *_err = + wrap_error("Failed to parse field " + std::to_string(_i) + ": ", + _i, std::move(res).error()); } return; } diff --git a/include/rfl/parsing/VectorReader.hpp b/include/rfl/parsing/VectorReader.hpp index 903e2e4a1..2e22fef88 100644 --- a/include/rfl/parsing/VectorReader.hpp +++ b/include/rfl/parsing/VectorReader.hpp @@ -13,6 +13,7 @@ #include "is_inplace_vector.hpp" #include "is_map_like.hpp" #include "is_set_like.hpp" +#include "wrap_error.hpp" namespace rfl::parsing { @@ -41,7 +42,8 @@ class VectorReader { * @param _r The reader to use. * @param _vec The vector to read into. */ - VectorReader(const R* _r, VecType* _vec) : r_(_r), vec_(_vec) {} + VectorReader(const R* _r, VecType* _vec) + : num_inserted_(0), r_(_r), vec_(_vec) {} ~VectorReader() = default; @@ -67,10 +69,13 @@ class VectorReader { RFL_ALWAYS_INLINE std::optional read(const InputVarType& _var) const { auto res = Parser::read(*r_, _var); if (!res) { - return std::move(res).error(); + // The message stays as it is, but the index of the element becomes + // part of the path. + return wrap_error("", index(), std::move(res).error()); } if constexpr (is_map_like_v || is_set_like_v) { vec_->insert(std::move(*res)); + ++num_inserted_; } else if constexpr (is_inplace_vector_v) { if (!vec_->try_emplace_back(std::move(*res))) { return Error("Too many elements, capacity is " + @@ -83,6 +88,20 @@ class VectorReader { } private: + /// Returns the index of the element that is currently being read. + size_t index() const { + if constexpr (is_map_like_v || is_set_like_v) { + // Inserting a duplicate does not change the size of a map or set. + return num_inserted_; + } else { + return vec_->size(); + } + } + + private: + /// The number of elements inserted into a map or set so far. + mutable size_t num_inserted_; + /// The underlying reader. const R* r_; diff --git a/include/rfl/parsing/ViewReader.hpp b/include/rfl/parsing/ViewReader.hpp index 0ee615e92..84ee4b826 100644 --- a/include/rfl/parsing/ViewReader.hpp +++ b/include/rfl/parsing/ViewReader.hpp @@ -3,6 +3,7 @@ #include #include +#include #include #include #include @@ -14,6 +15,7 @@ #include "FieldErrors.hpp" #include "Parser_base.hpp" #include "schemaful/IsSchemafulReader.hpp" +#include "wrap_error.hpp" namespace rfl::parsing { @@ -91,6 +93,15 @@ class ViewReader { return _current_name == FieldType::name(); } + static ErrorDetail::PathSegment to_path_segment(const int _current_index) { + return static_cast(_current_index); + } + + static ErrorDetail::PathSegment to_path_segment( + const std::string_view& _current_name) { + return std::string(_current_name); + } + template static RFL_ALWAYS_INLINE void assign_if_field_matches( const R& _r, const auto _current_name_or_index, const auto& _var, @@ -107,13 +118,10 @@ class ViewReader { auto res = Parser::read(_r, _var); if (!res) [[unlikely]] { constexpr auto name = FieldType::name(); - std::stringstream stream; - stream << "Failed to parse field '" << name - << "': " << res.error().what(); // Reserve lazily to avoid error-vector reallocations while keeping // successful reads allocation-free. _errors->reserve(size_); - _errors->emplace_back(stream.str()); + _errors->emplace_back(wrap_field_error(name, std::move(res).error())); return; } if constexpr (std::is_pointer_v) { @@ -142,10 +150,8 @@ class ViewReader { } auto res = Parser::read(_r, _var); if (!res) [[unlikely]] { - std::stringstream stream; - stream << "Failed to parse field '" << _current_name - << "': " << res.error().what(); - _errors->emplace_back(stream.str()); + _errors->emplace_back( + wrap_field_error(_current_name, std::move(res).error())); return; } extra_fields->emplace(std::string(_current_name), std::move(*res)); @@ -180,7 +186,8 @@ class ViewReader { if (!already_assigned) { std::stringstream stream; stream << "Value named '" << _current_name_or_index << "' not used."; - _errors->emplace_back(stream.str()); + _errors->emplace_back(make_nested_error( + to_path_segment(_current_name_or_index), stream.str())); } } } diff --git a/include/rfl/parsing/ViewReaderWithDefault.hpp b/include/rfl/parsing/ViewReaderWithDefault.hpp index 2cad93817..5faf08f21 100644 --- a/include/rfl/parsing/ViewReaderWithDefault.hpp +++ b/include/rfl/parsing/ViewReaderWithDefault.hpp @@ -13,6 +13,7 @@ #include "../internal/no_extra_fields_v.hpp" #include "FieldErrors.hpp" #include "Parser_base.hpp" +#include "wrap_error.hpp" namespace rfl::parsing { @@ -83,10 +84,7 @@ class ViewReaderWithDefault { *_already_assigned = true; auto res = Parser::read(_r, _var); if (!res) { - std::stringstream stream; - stream << "Failed to parse field '" << std::string(name) - << "': " << res.error().what(); - _errors->emplace_back(stream.str()); + _errors->emplace_back(wrap_field_error(name, std::move(res).error())); return; } if constexpr (std::is_pointer_v) { @@ -109,10 +107,8 @@ class ViewReaderWithDefault { std::remove_pointer_t>; auto res = Parser::read(_r, _var); if (!res) { - std::stringstream stream; - stream << "Failed to parse field '" << _current_name - << "': " << res.error().what(); - _errors->emplace_back(stream.str()); + _errors->emplace_back( + wrap_field_error(_current_name, std::move(res).error())); return; } extra_fields->emplace(std::string(_current_name), std::move(*res)); @@ -141,7 +137,8 @@ class ViewReaderWithDefault { stream << "Value named '" << std::string(_current_name) << "' not used. Remove the rfl::NoExtraFields processor or add " "rfl::ExtraFields to avoid this error message."; - _errors->emplace_back(stream.str()); + _errors->emplace_back( + make_nested_error(std::string(_current_name), stream.str())); } } } diff --git a/include/rfl/parsing/ViewReaderWithDefaultAndStrippedFieldNames.hpp b/include/rfl/parsing/ViewReaderWithDefaultAndStrippedFieldNames.hpp index 086ed839d..264380265 100644 --- a/include/rfl/parsing/ViewReaderWithDefaultAndStrippedFieldNames.hpp +++ b/include/rfl/parsing/ViewReaderWithDefaultAndStrippedFieldNames.hpp @@ -12,6 +12,7 @@ #include "../internal/is_array.hpp" #include "FieldErrors.hpp" #include "Parser_base.hpp" +#include "wrap_error.hpp" namespace rfl::parsing { @@ -86,10 +87,7 @@ class ViewReaderWithDefaultAndStrippedFieldNames { if (_i == i) { auto res = Parser::read(_r, _var); if (!res) { - std::stringstream stream; - stream << "Failed to parse field '" << std::string(name) - << "': " << res.error().what(); - _errors->emplace_back(stream.str()); + _errors->emplace_back(wrap_field_error(name, std::move(res).error())); return; } if constexpr (std::is_pointer_v) { diff --git a/include/rfl/parsing/ViewReaderWithStrippedFieldNames.hpp b/include/rfl/parsing/ViewReaderWithStrippedFieldNames.hpp index 498d4cc6d..2b356f807 100644 --- a/include/rfl/parsing/ViewReaderWithStrippedFieldNames.hpp +++ b/include/rfl/parsing/ViewReaderWithStrippedFieldNames.hpp @@ -12,6 +12,7 @@ #include "../internal/is_array.hpp" #include "FieldErrors.hpp" #include "Parser_base.hpp" +#include "wrap_error.hpp" namespace rfl::parsing { @@ -86,10 +87,7 @@ class ViewReaderWithStrippedFieldNames { std::get(*_found) = true; auto res = Parser::read(_r, _var); if (!res) { - std::stringstream stream; - stream << "Failed to parse field '" << std::string(name) - << "': " << res.error().what(); - _errors->emplace_back(stream.str()); + _errors->emplace_back(wrap_field_error(name, std::move(res).error())); return; } if constexpr (std::is_pointer_v) { diff --git a/include/rfl/parsing/to_single_error_message.hpp b/include/rfl/parsing/to_single_error_message.hpp index 88163cf36..bb3d0326e 100644 --- a/include/rfl/parsing/to_single_error_message.hpp +++ b/include/rfl/parsing/to_single_error_message.hpp @@ -4,9 +4,11 @@ #include #include #include +#include #include #include "../Result.hpp" +#include "../internal/cold.hpp" #include "../internal/strings/strings.hpp" namespace rfl::parsing { @@ -45,6 +47,31 @@ inline std::string to_single_error_message( } } +/** + * @brief Combines a set of errors to a single error. + * + * @param _errors The vector of errors. + * @return An error whose message is the combined error message, as produced by + * to_single_error_message, and whose details are the details of all errors. + */ +RFL_COLD inline Error to_single_error(std::vector _errors) { + if (_errors.size() == 1) { + return std::move(_errors[0]); + } + std::vector messages; + messages.reserve(_errors.size()); + std::vector details; + details.reserve(_errors.size()); + for (auto& err : _errors) { + messages.emplace_back(err.what()); + for (auto& detail : std::move(err).details()) { + details.emplace_back(std::move(detail)); + } + } + return Error(to_single_error_message(std::move(messages)), + std::move(details)); +} + } // namespace rfl::parsing #endif diff --git a/include/rfl/parsing/wrap_error.hpp b/include/rfl/parsing/wrap_error.hpp new file mode 100644 index 000000000..0a5fb625e --- /dev/null +++ b/include/rfl/parsing/wrap_error.hpp @@ -0,0 +1,87 @@ +#ifndef RFL_PARSING_WRAPERROR_HPP_ +#define RFL_PARSING_WRAPERROR_HPP_ + +#include +#include +#include +#include + +#include "../Result.hpp" +#include "../internal/cold.hpp" + +namespace rfl::parsing { + +/** + * @brief Wraps an error that occurred while reading a nested value, such as a + * field or an array element. + * + * @param _prefix The text to put in front of the error message. + * @param _segment The location of the nested value relative to its parent, + * which is prepended to the path of every detail. + * @param _err The error that occurred while reading the nested value. + * @return The error as seen from the parent. + */ +RFL_COLD inline Error wrap_error(const std::string_view _prefix, + const ErrorDetail::PathSegment& _segment, + Error&& _err) { + auto what = std::string(_prefix) + _err.what(); + auto details = std::move(_err).details(); + for (auto& detail : details) { + detail.path.insert(detail.path.begin(), _segment); + } + return Error(std::move(what), std::move(details)); +} + +/** + * @brief Wraps an error that occurred while reading a field. + * + * @param _name The name of the field. + * @param _err The error that occurred while reading the field. + * @return The error as seen from the object containing the field. + */ +RFL_COLD inline Error wrap_field_error(const std::string_view _name, + Error&& _err) { + auto name = std::string(_name); + return wrap_error("Failed to parse field '" + name + "': ", name, + std::move(_err)); +} + +/** + * @brief Adds context to an error without changing the location at which it + * occurred. + * + * @param _prefix The text to put in front of the error message. It is also put + * in front of the messages of all details at the root, because these describe + * the same failure as the error message. + * @param _err The error to add context to. + * @return The error with the added context. + */ +RFL_COLD inline Error wrap_error(const std::string_view _prefix, Error&& _err) { + auto what = std::string(_prefix) + _err.what(); + auto details = std::move(_err).details(); + for (auto& detail : details) { + if (detail.path.empty()) { + detail.message = std::string(_prefix) + detail.message; + } + } + return Error(std::move(what), std::move(details)); +} + +/** + * @brief Creates an error for a nested value that could not be read at all, + * for instance because it is missing. + * + * @param _segment The location of the nested value relative to its parent. + * @param _msg The error message. + * @return The error as seen from the parent. + */ +RFL_COLD inline Error make_nested_error( + const ErrorDetail::PathSegment& _segment, std::string _msg) { + auto details = std::vector( + {ErrorDetail{.path = {_segment}, .message = _msg}}); + return Error(std::move(_msg), std::move(details)); +} + +} // namespace rfl::parsing + +#endif diff --git a/tests/cli/test_errors.cpp b/tests/cli/test_errors.cpp index 0570191c0..da6befe4c 100644 --- a/tests/cli/test_errors.cpp +++ b/tests/cli/test_errors.cpp @@ -2,6 +2,7 @@ #include #include #include +#include namespace test_errors { @@ -28,4 +29,32 @@ TEST(cli, test_error_duplicate_key) { EXPECT_FALSE(result); } +struct Files { + rfl::Validator>> input_file; + std::string output_file; +}; + +struct Job { + Files files; + std::vector retries; +}; + +TEST(cli, test_error_details) { + const char* args[] = {"program", "--files.input-file=abcd", + "--retries=1,two"}; + const auto result = rfl::cli::read(3, const_cast(args)); + ASSERT_FALSE(result); + + using Path = std::vector; + const auto details = result.error().details(); + ASSERT_EQ(details.size(), 3u) << result.error().what(); + EXPECT_EQ(details[0].path, (Path{"files", "input-file"})); + EXPECT_EQ(details[0].message, + "Size validation failed: Value expected to be greater than or " + "equal to 5, but got 4."); + EXPECT_EQ(details[1].path, (Path{"files", "output-file"})); + EXPECT_EQ(details[1].message, "Field named 'output-file' not found."); + EXPECT_EQ(details[2].path, (Path{"retries", size_t{1}})); +} + } // namespace test_errors diff --git a/tests/json/test_error_details.cpp b/tests/json/test_error_details.cpp new file mode 100644 index 000000000..c785b342e --- /dev/null +++ b/tests/json/test_error_details.cpp @@ -0,0 +1,176 @@ +#include + +#include +#include +#include +#include +#include +#include +#include +#include +#include + +namespace test_error_details { + +using Path = std::vector; + +struct FileSpec { + rfl::Validator>> input_file; + std::string output_file; +}; + +struct Request { + std::string name; + std::vector files; +}; + +TEST(json, test_error_details_of_top_level_fields) { + const auto result = + rfl::json::read(R"({"input_file":"abcd","unknown":1})"); + ASSERT_FALSE(result); + + EXPECT_EQ(result.error().what(), R"(Found 2 errors: +1) Failed to parse field 'input_file': Size validation failed: Value expected to be greater than or equal to 5, but got 4. +2) Field named 'output_file' not found.)"); + + const auto expected = std::vector( + {{.path = {"input_file"}, + .message = "Size validation failed: Value expected to be greater " + "than or equal to 5, but got 4."}, + {.path = {"output_file"}, + .message = "Field named 'output_file' not found."}}); + EXPECT_EQ(result.error().details(), expected); +} + +TEST(json, test_error_details_of_nested_fields) { + const auto result = rfl::json::read( + R"({"name":"job","files":[ + {"inputFile":"first.txt","outputFile":"first.out"}, + {"inputFile":"second.txt","outputFile":"second.out"}, + {"inputFile":"abc","outputFile":1}]})"); + ASSERT_FALSE(result); + + const auto details = result.error().details(); + ASSERT_EQ(details.size(), 2); + EXPECT_EQ(details[0].path, (Path{"files", size_t{2}, "inputFile"})); + EXPECT_EQ(details[0].message, + "Size validation failed: Value expected to be greater than or " + "equal to 5, but got 3."); + EXPECT_EQ(details[1].path, (Path{"files", size_t{2}, "outputFile"})); + EXPECT_EQ(details[1].message, "Could not cast to string."); +} + +TEST(json, test_error_details_of_root_error) { + const auto result = rfl::json::read(R"({"name":"job";})"); + ASSERT_FALSE(result); + + const auto details = result.error().details(); + ASSERT_EQ(details.size(), 1); + EXPECT_TRUE(details[0].path.empty()); + EXPECT_EQ(details[0].message, result.error().what()); +} + +TEST(json, test_error_details_of_containers) { + struct Containers { + std::map map; + std::array array; + std::tuple tuple; + std::set set; + }; + + const auto result = rfl::json::read( + R"({"map":{"a":1,"b":"two"},"array":[1,"two"],"tuple":[1,2], + "set":[1,1,"three"]})"); + ASSERT_FALSE(result); + + const auto details = result.error().details(); + ASSERT_EQ(details.size(), 4); + EXPECT_EQ(details[0].path, (Path{"map", "b"})); + EXPECT_EQ(details[0].message, "Could not cast to int."); + EXPECT_EQ(details[1].path, (Path{"array", size_t{1}})); + EXPECT_EQ(details[1].message, "Could not cast to int."); + EXPECT_EQ(details[2].path, (Path{"tuple", size_t{1}})); + EXPECT_EQ(details[2].message, "Could not cast to string."); + // The duplicate does not end up in the set, but it still counts as an + // element of the input. + EXPECT_EQ(details[3].path, (Path{"set", size_t{2}})); +} + +TEST(json, test_error_details_of_extra_fields) { + const auto result = rfl::json::read( + R"({"input_file":"input.txt","output_file":"output.txt","extra":1})"); + ASSERT_FALSE(result); + + const auto details = result.error().details(); + ASSERT_EQ(details.size(), 1); + EXPECT_EQ(details[0].path, (Path{"extra"})); + EXPECT_EQ(details[0].message, "Value named 'extra' not used."); +} + +TEST(json, test_error_details_with_fail_fast) { + const auto result = rfl::json::read( + R"({"name":"job","files":[{"input_file":"abc"}]})"); + ASSERT_FALSE(result); + + const auto details = result.error().details(); + ASSERT_EQ(details.size(), 1); + EXPECT_EQ(details[0].path, (Path{"files", size_t{0}, "input_file"})); +} + +TEST(json, test_error_details_with_default_if_missing) { + struct WithDefaults { + int count = 0; + std::string label; + }; + + const auto result = rfl::json::read( + R"({"count":"many"})"); + ASSERT_FALSE(result); + + const auto details = result.error().details(); + ASSERT_EQ(details.size(), 1); + EXPECT_EQ(details[0].path, (Path{"count"})); + EXPECT_EQ(details[0].message, "Could not cast to int."); +} + +struct Circle { + rfl::Validator> radius; +}; + +struct Square { + double width; +}; + +struct Drawing { + rfl::TaggedUnion<"shape", Circle, Square> frame; + std::vector> shapes; + std::variant id; +}; + +TEST(json, test_error_details_of_tagged_unions_and_variants) { + const auto result = rfl::json::read( + R"({"frame":{"shape":"Square"}, + "shapes":[{"shape":"Square","width":1.0}, + {"shape":"Circle","radius":-1.0}], + "id":1.5})"); + ASSERT_FALSE(result); + + const auto details = result.error().details(); + ASSERT_EQ(details.size(), 3) << result.error().what(); + + // The tag selects the alternative, so failures inside it keep their + // location. + EXPECT_EQ(details[0].path, (Path{"frame", "width"})); + EXPECT_EQ(details[0].message, "Field named 'width' not found."); + EXPECT_EQ(details[1].path, (Path{"shapes", size_t{1}, "radius"})); + EXPECT_EQ(details[1].message, + "Value expected to be greater than or equal to 0, but got -1."); + + // For variants without a tag, the failures of the individual alternatives + // are reported as a single failure of the variant. + EXPECT_EQ(details[2].path, (Path{"id"})); + EXPECT_TRUE(details[2].message.starts_with("Could not parse the variant.")) + << details[2].message; +} + +} // namespace test_error_details From 0cc1fcdf5969c6b172ff50dccebfff9e68b1b81f Mon Sep 17 00:00:00 2001 From: clockblocker Date: Thu, 5 Nov 2026 11:15:17 +0100 Subject: [PATCH 2/4] Use an integer validator in the error details test 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 --- tests/json/test_error_details.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/json/test_error_details.cpp b/tests/json/test_error_details.cpp index c785b342e..ac5c16809 100644 --- a/tests/json/test_error_details.cpp +++ b/tests/json/test_error_details.cpp @@ -134,7 +134,7 @@ TEST(json, test_error_details_with_default_if_missing) { } struct Circle { - rfl::Validator> radius; + rfl::Validator> radius; }; struct Square { @@ -151,7 +151,7 @@ TEST(json, test_error_details_of_tagged_unions_and_variants) { const auto result = rfl::json::read( R"({"frame":{"shape":"Square"}, "shapes":[{"shape":"Square","width":1.0}, - {"shape":"Circle","radius":-1.0}], + {"shape":"Circle","radius":-1}], "id":1.5})"); ASSERT_FALSE(result); From 27ebb4d750213f65cc49ddefdba3ab933a46c394 Mon Sep 17 00:00:00 2001 From: clockblocker Date: Fri, 6 Nov 2026 08:19:40 +0100 Subject: [PATCH 3/4] Move errors out of failed reads instead of copying them 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 --- include/rfl/Result.hpp | 5 +++++ include/rfl/avro/Reader.hpp | 4 ++-- include/rfl/boost_serialization/Reader.hpp | 2 +- include/rfl/bson/Reader.hpp | 2 +- include/rfl/capnproto/Reader.hpp | 4 ++-- include/rfl/cbor/Reader.hpp | 4 ++-- include/rfl/cereal/Reader.hpp | 2 +- include/rfl/cli/Reader.hpp | 2 +- include/rfl/env/Reader.hpp | 2 +- include/rfl/flexbuf/Reader.hpp | 4 ++-- include/rfl/generic/Reader.hpp | 4 ++-- include/rfl/json/Reader.hpp | 4 ++-- include/rfl/msgpack/Reader.hpp | 6 +++--- include/rfl/parsing/FieldVariantParser.hpp | 2 +- include/rfl/parsing/MapParser.hpp | 12 ++++++------ include/rfl/parsing/NamedTupleParser.hpp | 16 ++++++++-------- include/rfl/parsing/ParserArray.hpp | 4 ++-- include/rfl/parsing/Parser_default.hpp | 8 ++++---- include/rfl/parsing/TupleParser.hpp | 4 ++-- include/rfl/parsing/VectorParser.hpp | 4 ++-- include/rfl/parsing/read_array_elements.hpp | 4 ++-- include/rfl/toml/Reader.hpp | 4 ++-- include/rfl/ubjson/Reader.hpp | 4 ++-- include/rfl/xml/Reader.hpp | 2 +- include/rfl/yaml/Reader.hpp | 4 ++-- include/rfl/yas/Reader.hpp | 2 +- 26 files changed, 60 insertions(+), 55 deletions(-) diff --git a/include/rfl/Result.hpp b/include/rfl/Result.hpp index 0a92a6bc4..08d01fc1f 100644 --- a/include/rfl/Result.hpp +++ b/include/rfl/Result.hpp @@ -51,6 +51,11 @@ inline Unexpected error(const Error& _err) { return Unexpected(_err); } +/// Shorthand for unexpected error. +inline Unexpected error(Error&& _err) { + return Unexpected(std::move(_err)); +} + } // namespace rfl #ifdef REFLECTCPP_USE_STD_EXPECTED diff --git a/include/rfl/avro/Reader.hpp b/include/rfl/avro/Reader.hpp index a5b002c11..902550517 100644 --- a/include/rfl/avro/Reader.hpp +++ b/include/rfl/avro/Reader.hpp @@ -222,7 +222,7 @@ struct Reader { "): " + avro_strerror()); } if constexpr (requires { _array_reader.reserve(size); }) { - if (const auto err = _array_reader.reserve(size)) { + if (auto err = _array_reader.reserve(size)) { return err; } } @@ -233,7 +233,7 @@ struct Reader { return Error(std::string(__FUNCTION__) + " error(" + std::to_string(res) + "): " + avro_strerror()); } - const auto err = _array_reader.read(InputVarType{&element}); + auto err = _array_reader.read(InputVarType{&element}); if (err) { return err; } diff --git a/include/rfl/boost_serialization/Reader.hpp b/include/rfl/boost_serialization/Reader.hpp index 004a1439d..d07ac5d48 100644 --- a/include/rfl/boost_serialization/Reader.hpp +++ b/include/rfl/boost_serialization/Reader.hpp @@ -161,7 +161,7 @@ struct Reader { const auto size = _map.size; for (size_t i = 0; i < size; ++i) { const auto var = InputVarType{_map.ar}; - if (const auto err = _reader.read(var, var)) { + if (auto err = _reader.read(var, var)) { return err; } } diff --git a/include/rfl/bson/Reader.hpp b/include/rfl/bson/Reader.hpp index 3ff11849f..0d7bf53f2 100644 --- a/include/rfl/bson/Reader.hpp +++ b/include/rfl/bson/Reader.hpp @@ -188,7 +188,7 @@ struct Reader { if (bson_init_static(&b, doc.data, doc.data_len)) { if (bson_iter_init(&iter, &b)) { while (bson_iter_next(&iter)) { - const auto err = _array_reader.read(to_input_var(&iter)); + auto err = _array_reader.read(to_input_var(&iter)); if (err) { return err; } diff --git a/include/rfl/capnproto/Reader.hpp b/include/rfl/capnproto/Reader.hpp index 92f14b674..31bbfdb50 100644 --- a/include/rfl/capnproto/Reader.hpp +++ b/include/rfl/capnproto/Reader.hpp @@ -151,12 +151,12 @@ class RFL_API Reader { std::optional read_array(const ArrayReader& _array_reader, const InputArrayType& _arr) const noexcept { if constexpr (requires { _array_reader.reserve(_arr.val_.size()); }) { - if (const auto err = _array_reader.reserve(_arr.val_.size())) { + if (auto err = _array_reader.reserve(_arr.val_.size())) { return err; } } for (auto element : _arr.val_) { - const auto err = _array_reader.read(InputVarType{std::move(element)}); + auto err = _array_reader.read(InputVarType{std::move(element)}); if (err) { return err; } diff --git a/include/rfl/cbor/Reader.hpp b/include/rfl/cbor/Reader.hpp index 05f49414f..62fcfa4fa 100644 --- a/include/rfl/cbor/Reader.hpp +++ b/include/rfl/cbor/Reader.hpp @@ -207,12 +207,12 @@ class Reader { std::optional read_array(const ArrayReader& _array_reader, const InputArrayType& _arr) const noexcept { if constexpr (requires { _array_reader.reserve(_arr.val_->size()); }) { - if (const auto err = _array_reader.reserve(_arr.val_->size())) { + if (auto err = _array_reader.reserve(_arr.val_->size())) { return err; } } for (auto& val : _arr.val_->array_range()) { - const auto err = _array_reader.read(InputVarType{&val}); + auto err = _array_reader.read(InputVarType{&val}); if (err) { return err; } diff --git a/include/rfl/cereal/Reader.hpp b/include/rfl/cereal/Reader.hpp index 0ad601dbb..2e2042b72 100644 --- a/include/rfl/cereal/Reader.hpp +++ b/include/rfl/cereal/Reader.hpp @@ -159,7 +159,7 @@ struct Reader { (*_map.archive_)(::cereal::make_size_tag(size)); for (size_t i = 0; i < size; ++i) { const auto var = InputVarType{_map.archive_}; - if (const auto err = _reader.read(var, var)) { + if (auto err = _reader.read(var, var)) { return err; } } diff --git a/include/rfl/cli/Reader.hpp b/include/rfl/cli/Reader.hpp index 1d46e5d6e..58c16d37b 100644 --- a/include/rfl/cli/Reader.hpp +++ b/include/rfl/cli/Reader.hpp @@ -201,7 +201,7 @@ struct Reader { const InputArrayType& _arr ) const noexcept { for (const auto& val : _arr.values) { - const auto err = _array_reader.read( + auto err = _array_reader.read( InputVarType{nullptr, "", val}); if (err) { return err; diff --git a/include/rfl/env/Reader.hpp b/include/rfl/env/Reader.hpp index ae4d28a18..68545f790 100644 --- a/include/rfl/env/Reader.hpp +++ b/include/rfl/env/Reader.hpp @@ -205,7 +205,7 @@ struct Reader { if (is_empty(var)) { break; } - const auto err = _array_reader.read(var); + auto err = _array_reader.read(var); if (err) { return err; } diff --git a/include/rfl/flexbuf/Reader.hpp b/include/rfl/flexbuf/Reader.hpp index 07428b396..8ef601d5e 100644 --- a/include/rfl/flexbuf/Reader.hpp +++ b/include/rfl/flexbuf/Reader.hpp @@ -104,12 +104,12 @@ struct Reader { const InputArrayType& _arr) const noexcept { const auto size = _arr.size(); if constexpr (requires { _array_reader.reserve(size); }) { - if (const auto err = _array_reader.reserve(size)) { + if (auto err = _array_reader.reserve(size)) { return err; } } for (size_t i = 0; i < size; ++i) { - const auto err = _array_reader.read(InputVarType(_arr[i])); + auto err = _array_reader.read(InputVarType(_arr[i])); if (err) { return err; } diff --git a/include/rfl/generic/Reader.hpp b/include/rfl/generic/Reader.hpp index c56cdd9e7..db9631800 100644 --- a/include/rfl/generic/Reader.hpp +++ b/include/rfl/generic/Reader.hpp @@ -87,12 +87,12 @@ struct Reader { std::optional read_array(const ArrayReader& _array_reader, const InputArrayType& _arr) const noexcept { if constexpr (requires { _array_reader.reserve(_arr.size()); }) { - if (const auto err = _array_reader.reserve(_arr.size())) { + if (auto err = _array_reader.reserve(_arr.size())) { return err; } } for (const auto& v : _arr) { - const auto err = _array_reader.read(InputVarType(v)); + auto err = _array_reader.read(InputVarType(v)); if (err) { return err; } diff --git a/include/rfl/json/Reader.hpp b/include/rfl/json/Reader.hpp index 855834f1d..05b2469b4 100644 --- a/include/rfl/json/Reader.hpp +++ b/include/rfl/json/Reader.hpp @@ -102,7 +102,7 @@ struct Reader { const InputArrayType& _arr) const noexcept { const auto size = yyjson_arr_size(_arr.val_); if constexpr (requires { _array_reader.reserve(size); }) { - if (const auto err = _array_reader.reserve(size)) { + if (auto err = _array_reader.reserve(size)) { return err; } } @@ -110,7 +110,7 @@ struct Reader { yyjson_arr_iter iter; yyjson_arr_iter_init(_arr.val_, &iter); while ((val = yyjson_arr_iter_next(&iter))) { - const auto err = _array_reader.read(InputVarType(val)); + auto err = _array_reader.read(InputVarType(val)); if (err) { return err; } diff --git a/include/rfl/msgpack/Reader.hpp b/include/rfl/msgpack/Reader.hpp index 5ca3afd6b..5b84b2332 100644 --- a/include/rfl/msgpack/Reader.hpp +++ b/include/rfl/msgpack/Reader.hpp @@ -171,12 +171,12 @@ struct Reader { std::optional read_array(const ArrayReader& _array_reader, const InputArrayType& _arr) const noexcept { if constexpr (requires { _array_reader.reserve(_arr.size); }) { - if (const auto err = _array_reader.reserve(_arr.size)) { + if (auto err = _array_reader.reserve(_arr.size)) { return err; } } for (uint32_t i = 0; i < _arr.size; ++i) { - const auto err = _array_reader.read(_arr.ptr[i]); + auto err = _array_reader.read(_arr.ptr[i]); if (err) { return err; } @@ -189,7 +189,7 @@ struct Reader { std::optional read_typed_map( const MapReader& _reader, const InputObjectType& _map) const noexcept { for (uint32_t i = 0; i < _map.size; ++i) { - if (const auto err = _reader.read(_map.ptr[i].key, _map.ptr[i].val)) { + if (auto err = _reader.read(_map.ptr[i].key, _map.ptr[i].val)) { return err; } } diff --git a/include/rfl/parsing/FieldVariantParser.hpp b/include/rfl/parsing/FieldVariantParser.hpp index ea4556954..7e586b749 100644 --- a/include/rfl/parsing/FieldVariantParser.hpp +++ b/include/rfl/parsing/FieldVariantParser.hpp @@ -50,7 +50,7 @@ struct FieldVariantParser { &_r, &field_variant); auto err = _r.read_object(reader, _obj); if (err) { - return error(*err); + return error(std::move(*err)); } if (!field_variant) { return error( diff --git a/include/rfl/parsing/MapParser.hpp b/include/rfl/parsing/MapParser.hpp index c03574b13..ed4cb7454 100644 --- a/include/rfl/parsing/MapParser.hpp +++ b/include/rfl/parsing/MapParser.hpp @@ -91,8 +91,8 @@ struct MapParser { R, std::remove_cvref_t>) { const auto reader = TypedMapReader{&_r, &map}; - if (const auto err = _r.read_typed_map(reader, _obj_or_map)) { - return error(*err); + if (auto err = _r.read_typed_map(reader, _obj_or_map)) { + return error(std::move(*err)); } return map; } else { @@ -100,14 +100,14 @@ struct MapParser { const auto map_reader = MapReader(&_r, &map, &errors); if constexpr (schemaful::IsSchemafulReader) { - const auto err = _r.read_map(map_reader, _obj_or_map); + auto err = _r.read_map(map_reader, _obj_or_map); if (err) { - return error(*err); + return error(std::move(*err)); } } else { - const auto err = _r.read_object(map_reader, _obj_or_map); + auto err = _r.read_object(map_reader, _obj_or_map); if (err) { - return error(*err); + return error(std::move(*err)); } } if (errors.size() != 0) { diff --git a/include/rfl/parsing/NamedTupleParser.hpp b/include/rfl/parsing/NamedTupleParser.hpp index 66ce4d397..c4e2fd312 100644 --- a/include/rfl/parsing/NamedTupleParser.hpp +++ b/include/rfl/parsing/NamedTupleParser.hpp @@ -129,11 +129,11 @@ struct NamedTupleParser { auto ptr = internal::ptr_cast*>(&buf); auto view = rfl::to_view(*ptr); using ViewType = std::remove_cvref_t; - const auto [set, err] = + auto [set, err] = Parser::read_view(_r, _var, &view); if (err) [[unlikely]] { call_destructors_where_necessary(set, &view); - return error(*err); + return error(std::move(*err)); } auto res = Result>(std::move(*ptr)); call_destructors_where_necessary(set, &view); @@ -394,14 +394,14 @@ struct NamedTupleParser { FieldErrors errors; const auto reader = ViewReaderType(&_r, _view, &found, &set, &errors); if constexpr (_no_field_names) { - const auto err = _r.read_array(reader, _obj_or_arr); + auto err = _r.read_array(reader, _obj_or_arr); if (err) [[unlikely]] { - return std::make_pair(set, err); + return std::make_pair(set, std::move(err)); } } else { - const auto err = _r.read_object(reader, _obj_or_arr); + auto err = _r.read_object(reader, _obj_or_arr); if (err) [[unlikely]] { - return std::make_pair(set, err); + return std::make_pair(set, std::move(err)); } } if (!should_stop_on_field_error(errors)) { @@ -424,12 +424,12 @@ struct NamedTupleParser { FieldErrors errors; const auto reader = ViewReaderWithDefaultType(&_r, _view, &errors); if constexpr (_no_field_names) { - const auto err = _r.read_array(reader, _obj_or_arr); + auto err = _r.read_array(reader, _obj_or_arr); if (err) [[unlikely]] { return err; } } else { - const auto err = _r.read_object(reader, _obj_or_arr); + auto err = _r.read_object(reader, _obj_or_arr); if (err) [[unlikely]] { return err; } diff --git a/include/rfl/parsing/ParserArray.hpp b/include/rfl/parsing/ParserArray.hpp index 8a9a66de4..c34e38cf3 100644 --- a/include/rfl/parsing/ParserArray.hpp +++ b/include/rfl/parsing/ParserArray.hpp @@ -66,12 +66,12 @@ struct ParserArray { }(); if (err) { call_destructors_on_array_where_necessary(array_reader.num_set(), ptr); - return error(*err); + return error(std::move(*err)); } err = array_reader.check_size(); if (err) { call_destructors_on_array_where_necessary(array_reader.num_set(), ptr); - return error(*err); + return error(std::move(*err)); } auto result = Result>(std::move(*ptr)); call_destructors_on_array_where_necessary(array_reader.num_set(), ptr); diff --git a/include/rfl/parsing/Parser_default.hpp b/include/rfl/parsing/Parser_default.hpp index a5f5abb57..14c33a1d1 100644 --- a/include/rfl/parsing/Parser_default.hpp +++ b/include/rfl/parsing/Parser_default.hpp @@ -694,11 +694,11 @@ struct Parser { auto raw_view = to_view(*ptr); auto view = ProcessorsType::template process(std::move(raw_view)); using ViewType = std::remove_cvref_t; - const auto [set, err] = + auto [set, err] = Parser::read_view(_r, _var, &view); if (err) [[unlikely]] { call_destructors_where_necessary(set, &view); - return error(*err); + return error(std::move(*err)); } auto res = Result(std::move(*ptr)); call_destructors_where_necessary(set, &view); @@ -717,11 +717,11 @@ struct Parser { // return the error. auto view = ProcessorsType::template process(to_view(t)); using ViewType = decltype(view); - const auto err = + auto err = Parser::read_view_with_default( _r, _var, &view); if (err) [[unlikely]] { - return error(*err); + return error(std::move(*err)); } return t; } catch (std::exception& e) { diff --git a/include/rfl/parsing/TupleParser.hpp b/include/rfl/parsing/TupleParser.hpp index b9dd097c1..0e3655dff 100644 --- a/include/rfl/parsing/TupleParser.hpp +++ b/include/rfl/parsing/TupleParser.hpp @@ -62,13 +62,13 @@ struct TupleParser { if (err) { call_destructors_on_tuple_where_necessary(tuple_reader.num_set(), ptr); - return error(*err); + return error(std::move(*err)); } err = tuple_reader.handle_missing_fields(); if (err) { call_destructors_on_tuple_where_necessary(tuple_reader.num_set(), ptr); - return error(*err); + return error(std::move(*err)); } auto res = Result(std::move(*ptr)); call_destructors_on_tuple_where_necessary(tuple_reader.num_set(), ptr); diff --git a/include/rfl/parsing/VectorParser.hpp b/include/rfl/parsing/VectorParser.hpp index 36ac892f7..2be9ee2e7 100644 --- a/include/rfl/parsing/VectorParser.hpp +++ b/include/rfl/parsing/VectorParser.hpp @@ -68,9 +68,9 @@ struct VectorParser { VecType vec; auto vector_reader = VectorReader(&_r, &vec); - const auto err = _r.read_array(vector_reader, _arr); + auto err = _r.read_array(vector_reader, _arr); if (err) { - return error(*err); + return error(std::move(*err)); } return vec; }; diff --git a/include/rfl/parsing/read_array_elements.hpp b/include/rfl/parsing/read_array_elements.hpp index b8025e962..031b3808f 100644 --- a/include/rfl/parsing/read_array_elements.hpp +++ b/include/rfl/parsing/read_array_elements.hpp @@ -16,12 +16,12 @@ std::optional read_array_elements(const ArrayReader& _array_reader, const InputVarType& _var, const std::size_t _size) { if constexpr (requires { _array_reader.reserve(_size); }) { - if (const auto err = _array_reader.reserve(_size)) { + if (auto err = _array_reader.reserve(_size)) { return err; } } for (std::size_t i = 0; i < _size; ++i) { - if (const auto err = _array_reader.read(_var)) { + if (auto err = _array_reader.read(_var)) { return err; } } diff --git a/include/rfl/toml/Reader.hpp b/include/rfl/toml/Reader.hpp index ac87aaf1e..d54d139f8 100644 --- a/include/rfl/toml/Reader.hpp +++ b/include/rfl/toml/Reader.hpp @@ -121,12 +121,12 @@ struct Reader { std::optional read_array(const ArrayReader& _array_reader, const InputArrayType& _arr) const noexcept { if constexpr (requires { _array_reader.reserve(_arr->size()); }) { - if (const auto err = _array_reader.reserve(_arr->size())) { + if (auto err = _array_reader.reserve(_arr->size())) { return err; } } for (auto& node : *_arr) { - const auto err = _array_reader.read(&node); + auto err = _array_reader.read(&node); if (err) { return err; } diff --git a/include/rfl/ubjson/Reader.hpp b/include/rfl/ubjson/Reader.hpp index 3c0f5fa1b..49472f178 100644 --- a/include/rfl/ubjson/Reader.hpp +++ b/include/rfl/ubjson/Reader.hpp @@ -149,12 +149,12 @@ class Reader { std::optional read_array(const ArrayReader& _array_reader, const InputArrayType& _arr) const noexcept { if constexpr (requires { _array_reader.reserve(_arr.val_->size()); }) { - if (const auto err = _array_reader.reserve(_arr.val_->size())) { + if (auto err = _array_reader.reserve(_arr.val_->size())) { return err; } } for (auto& val : _arr.val_->array_range()) { - const auto err = _array_reader.read(InputVarType{&val}); + auto err = _array_reader.read(InputVarType{&val}); if (err) { return err; } diff --git a/include/rfl/xml/Reader.hpp b/include/rfl/xml/Reader.hpp index fc0725a32..6ef06975b 100644 --- a/include/rfl/xml/Reader.hpp +++ b/include/rfl/xml/Reader.hpp @@ -171,7 +171,7 @@ struct Reader { const InputArrayType& _arr) const noexcept { const auto name = _arr.node_.name(); for (auto node = _arr.node_; node; node = node.next_sibling(name)) { - const auto err = _array_reader.read(InputVarType(node)); + auto err = _array_reader.read(InputVarType(node)); if (err) { return err; } diff --git a/include/rfl/yaml/Reader.hpp b/include/rfl/yaml/Reader.hpp index 48560a007..c8625c04a 100644 --- a/include/rfl/yaml/Reader.hpp +++ b/include/rfl/yaml/Reader.hpp @@ -160,12 +160,12 @@ struct Reader { std::optional read_array(const ArrayReader& _array_reader, const InputArrayType& _arr) const noexcept { if constexpr (requires { _array_reader.reserve(_arr.node_.size()); }) { - if (const auto err = _array_reader.reserve(_arr.node_.size())) { + if (auto err = _array_reader.reserve(_arr.node_.size())) { return err; } } for (size_t i = 0; i < _arr.node_.size(); ++i) { - const auto err = _array_reader.read(_arr.node_[i]); + auto err = _array_reader.read(_arr.node_[i]); if (err) { return err; } diff --git a/include/rfl/yas/Reader.hpp b/include/rfl/yas/Reader.hpp index 3820b932e..06749c67a 100644 --- a/include/rfl/yas/Reader.hpp +++ b/include/rfl/yas/Reader.hpp @@ -150,7 +150,7 @@ struct Reader { (*_map.ar) & size; for (size_t i = 0; i < size; ++i) { const auto var = InputVarType{_map.ar}; - if (const auto err = _reader.read(var, var)) { + if (auto err = _reader.read(var, var)) { return err; } } From a038bc02cfe5bec2bdf7f4d1f9766e87ef769c89 Mon Sep 17 00:00:00 2001 From: clockblocker Date: Fri, 6 Nov 2026 09:37:45 +0100 Subject: [PATCH 4/4] Move the error details pointer into an explicitly optional type 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 --- include/rfl/internal/Error.hpp | 79 +++++++++++++++++++++++----------- 1 file changed, 55 insertions(+), 24 deletions(-) diff --git a/include/rfl/internal/Error.hpp b/include/rfl/internal/Error.hpp index f8b96d969..f8797c1f0 100644 --- a/include/rfl/internal/Error.hpp +++ b/include/rfl/internal/Error.hpp @@ -52,16 +52,15 @@ class Error { /// @param _details The individual failures, see details() Error(std::string _what, std::vector _details) : what_(std::move(_what)), - details_(_details.empty() - ? nullptr - : new std::vector(std::move(_details))) {} + details_(_details.empty() ? OptionalDetails() + : OptionalDetails(std::move(_details))) {} /// Destructor. ~Error() = default; /// Copy constructor. /// @param e The error to copy from - Error(const Error& e) : what_(e.what_), details_(copy_details(e.details_)) {} + Error(const Error& e) = default; /// Move constructor. /// @param e The error to move from @@ -70,13 +69,7 @@ class Error { /// Copy assignment operator. /// @param _other The error to copy from /// @return Reference to this error - Error& operator=(const Error& _other) { - if (this != &_other) { - what_ = _other.what_; - details_ = copy_details(_other.details_); - } - return *this; - } + Error& operator=(const Error& _other) = default; /// Move assignment operator. /// @param _other The error to move from @@ -93,7 +86,7 @@ class Error { /// location at which it occurred. An error that was constructed from a /// message only is reported as a single failure at the root. std::vector details() const& { - if (!details_) { + if (!details_.has_value()) { return {ErrorDetail{.path = {}, .message = what_}}; } return *details_; @@ -102,35 +95,73 @@ class Error { /// Moves the individual failures out of the Error object and leaves it in a /// moved from state. std::vector details() && { - if (!details_) { + if (!details_.has_value()) { return {ErrorDetail{.path = {}, .message = std::move(what_)}}; } return std::move(*details_); } private: + /// Either nothing or a heap-allocated vector of details, deep-copied along + /// with the Error. Holding nothing is the common case: it means that the + /// Error is just its message, and it does not allocate. + /// /// Errors are created and destroyed in many places, mostly as part of /// rfl::Result. Deleting and copying the details out of line keeps that code /// small, which matters for the performance of successful reads. - struct DeleteDetails { - RFL_COLD void operator()(std::vector* _ptr) const { - delete _ptr; + class OptionalDetails { + public: + /// Holds nothing. + OptionalDetails() = default; + + /// Holds a heap-allocated vector containing _details. + explicit OptionalDetails(std::vector&& _details) + : ptr_(new std::vector(std::move(_details))) {} + + /// Holds a copy of the details _other holds, if any. + OptionalDetails(const OptionalDetails& _other) : ptr_(copy(_other.ptr_)) {} + + OptionalDetails(OptionalDetails&&) noexcept = default; + + /// Holds a copy of the details _other holds, if any. + OptionalDetails& operator=(const OptionalDetails& _other) { + ptr_ = copy(_other.ptr_); + return *this; } - }; - using DetailsPtr = std::unique_ptr, DeleteDetails>; + OptionalDetails& operator=(OptionalDetails&&) noexcept = default; - RFL_COLD static DetailsPtr copy_details(const DetailsPtr& _ptr) { - return _ptr ? DetailsPtr(new std::vector(*_ptr)) : nullptr; - } + bool has_value() const { return ptr_ != nullptr; } + + /// Must only be called if has_value() is true. + const std::vector& operator*() const { return *ptr_; } + + /// Must only be called if has_value() is true. + std::vector& operator*() { return *ptr_; } + + private: + struct Delete { + RFL_COLD void operator()(std::vector* _ptr) const { + delete _ptr; + } + }; + + using Ptr = std::unique_ptr, Delete>; + + RFL_COLD static Ptr copy(const Ptr& _ptr) { + return _ptr ? Ptr(new std::vector(*_ptr)) : nullptr; + } + + Ptr ptr_; + }; private: /// Documents what went wrong std::string what_; - /// The individual failures summarized by what_. Null, unless what_ was - /// assembled from failures at nested locations. - DetailsPtr details_; + /// The individual failures summarized by what_. Holds nothing, unless what_ + /// was assembled from failures at nested locations. + OptionalDetails details_; }; } // namespace rfl::internal