serde: serialize dyn Error Values using collect_str
#89
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
lgtm. As for the CI (codegen) failure, I implemented a way to automatically fix that (rust-lang/futures-rs#2562), but forgot to port it to this repository. I will open a PR later. |
|
@taiki-e great, happy to rebase this once CI is sorted out! |
Currently, `valuable-serde` handles `Value::Error` variants incorrectly. When `valuable-serde` encounters a `Value::Error`, it returns the error immediately as a serialization error: https://github.com/tokio-rs/valuable/blob/1fc2ad50fcae7fc0da81dc40a385c235636ba525/valuable-serde/src/lib.rs#L267 This is _not_ correct. If a `serde` serializer returns an error, this means something has gone wrong while serializing a value. Returning an error makes the serialization fail. However, this is *not* what `valuable`'s `Value::Error` means. `Value::Error` represents that we are _recording a value which happens to be an `Error`_, not that something went wrong _while_ recording the value. This means that `valuable-serde` will currently return an `Error` (indicating that serialization failed) any time it attempts to record an `Error` value, and serialization will fail. This commit changes `valuable-serde` to record the error using its `Display` implementation, using `serde`'s `collect_str` method. Now, `Error`s will be serialized by recording their messages as a string, rather than causing the serialization to fail. Using `collect_str` allows the serializer to write the display representation to its own internal buffer, rather than having to format the error to a temporary `String` beforehand. Fixes #88
taiki-e
approved these changes
Mar 8, 2022
hawkw
added a commit
that referenced
this pull request
Mar 8, 2022
Depends on #89 This PR changes `valuable-serde`'s recording of `dyn Error` values to record the error as a `serde` struct with `message` and `source` fields. This way, we can serialize errors with source chains more nicely. When the backtrace support for `std::error::Error` is stable, we could also record backtraces as a field. We could even consider adding a build script to detect the nightly compiler and conditionally enable a `cfg` for backtrace support, but that seems better left to a follow-up.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently,
valuable-serdehandlesValue::Errorvariants incorrectly.When
valuable-serdeencounters aValue::Error, it returns the errorimmediately as a serialization error:
valuable/valuable-serde/src/lib.rs
Line 267 in 1fc2ad5
This is not correct. If a
serdeserializer returns an error, thismeans something has gone wrong while serializing a value. Returning an
error makes the serialization fail. However, this is not what
valuable'sValue::Errormeans.Value::Errorrepresents that we arerecording a value which happens to be an
Error, not that somethingwent wrong while recording the value. This means that
valuable-serdewill currently return an
Error(indicating that serialization failed)any time it attempts to record an
Errorvalue, and serialization willfail.
This commit changes
valuable-serdeto record the error using itsDisplayimplementation, usingserde'scollect_strmethod. Now,Errors will be serialized by recording their messages as a string,rather than causing the serialization to fail.
Using
collect_strallows the serializer to write the displayrepresentation to its own internal buffer, rather than having to format
the error to a temporary
Stringbeforehand.Fixes #88