ron-rs
Improve error information for serde errors
Followup to #393
Adds explicit variants in ron::Error for serde errors from serde::de::Error (serde::ser::Error only has the Error::custom method).
- [ ] I've included my change in
CHANGELOG.md
This is the first proof-of-concept. Notably, I've also ported serde's Unexpected enum, which I'm not quite sure about. It does give us slightly nicer error messages and would allow users to match on the unexpected value (though I'm not quite sure if anyone would need that). However, it also adds some very serde-specific terms to our codebase, which don't map quite so well to ron. We could decide to just remove that part and stringify the unexpected values as serde does, or we could limit the enum to entries that make sense for us (we'd still adopt serde's decision to only store [u|i]64).
Codecov Report
Merging #394 (021eb4c) into master (f7e8417) will decrease coverage by
3.09%. The diff coverage is76.69%.
@@ Coverage Diff @@
## master #394 +/- ##
==========================================
- Coverage 89.65% 86.55% -3.10%
==========================================
Files 48 49 +1
Lines 5228 5803 +575
==========================================
+ Hits 4687 5023 +336
- Misses 541 780 +239
| Impacted Files | Coverage Δ | |
|---|---|---|
| src/error.rs | 30.46% <43.35%> (+22.57%) |
:arrow_up: |
| tests/203_error_positions.rs | 86.02% <86.44%> (-13.98%) |
:arrow_down: |
| src/de/mod.rs | 74.46% <93.22%> (-15.40%) |
:arrow_down: |
| src/parse.rs | 93.76% <100.00%> (ø) |
|
| tests/359_deserialize_seed.rs | 100.00% <100.00%> (ø) |
|
| tests/393_serde_errors.rs | 100.00% <100.00%> (ø) |
|
| tests/roundtrip.rs | 100.00% <100.00%> (ø) |
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
What I haven't touched yet is the area of adding more contextual information to those new errors. Something we might be able to do is to decorate the deserialize_enum and deserialize_struct-like methods to check for the new errors and insert struct/enum name information.
We could probably also try to find the name of the current field, though in my opinion, that would be overkill and is already handled by crates like serde_path_to_error.
I've tried to do the struct/enum/variant name extraction for even better error messages. Just extracting struct/enum names is easy, and we can store them as &'static str. For variant names, however, which are important for struct variants which to the ron-writing user look just like structs, I had to add a small hack. The Deserializer now remembers what the last deserialized identifier was. Since enums in ron use identifiers for the variants, I can check for that last one and assume it was the variant id. This does provide better errors (for externally tagged enums, internally tagged ones cannot get any context since they are based on deserialize_any, adjacently tagged and untagged enums get ok-ish errors), but means that the struct/enum/variant name is stored as a String.
I will have to give your comments a closer look later this week, I'm busy right now, sorry. But feel free to proceed without me. I think workarounds are fine if they don't overly complicate the codebase and provide useful information, but I'd be cautious of exposing those details via an API because there's a higher chance we'll change them in the future. In general I think we shouldn't expose the full error enum as we do know.
@torkleyy I've opted to simplify the error enum a bit by merging serde's invalid_type and invalid_value into the same InvalidValueForType type in ron. Furthermore, this error now contains the stringified version of found field instead of replicating serde's de::Unexpected type (however, I still implement some custom formatting for the conversion to get slightly nicer error messages). This now seems like a good compromise to me where we get some extra info from serde's useful errors like the missing field or unknown variant ones without having to also include its expected vs unexpected system.
Since we're doing a API-breaking release, I think we should use the opportunity to bump MSRV and get rid of the CI problem at the same time :)
Since we're doing a API-breaking release, I think we should use the opportunity to bump MSRV and get rid of the CI problem at the same time :)
@torkleyy I've opened #396 to bump the MSRV to 1.56. Once that PR is merged I'll rebase this one off it.
@torkleyy This PR is now ready to be merged if you're happy with the changes :)