-
Notifications
You must be signed in to change notification settings - Fork 123
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[v0.9] Breaking: Treat Some like any newtype variant #413
Conversation
Codecov ReportBase: 88.84% // Head: 88.89% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #413 +/- ##
==========================================
+ Coverage 88.84% 88.89% +0.04%
==========================================
Files 55 55
Lines 6437 6464 +27
==========================================
+ Hits 5719 5746 +27
Misses 718 718
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
62b7571
to
9b94603
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the change makes sense, but I'm wondering if it will break anything in practice. In any case this change needs to be in a breaking version release. Maybe we should merge it when we are about to release other breaking changes.
Good point! I agree that we can keep around this change until we’re ready for 0.9 as it’s quite small and won’t be difficult to rebase then |
#465 shows that this might actually be a bad idea, as |
#465 now includes and supersedes this PR |
* Early prototyping with a typed arbitrary fuzzer (ser only so far) and reading in the corpus * Also fuzz the ser::PrettyConfig (identation-excluded) * Start implementing the arbitrary typed data deserialising fuzzing * Fix None inside stack of implicit Some-s * Detect problematic Some inside deserialize_any with unwrap_variant_newtypes * Fix clippy::useless_conversion lint * Another alternative: allow newtype variant unwrapping in deserialize_any, also for Some + Fix check_struct_type lookahead * Fix PartialOrd impls for Map and Float * Implement arbitrary tuple struct (static field names slice FIXME) fuzzing deserialisation * Fully fix Float comparison with total_ord * Fix clippy lints * Finished arbitrary struct and enum deserialisation fuzzing * Create CI workflow for benchmarking * Fix corpus download and unzip * Give benchmark the comparison branch name * Restrict the benchmark to unique cases (ty, value, ron) * Add test for the Serialize identifier validation * Add tests for further fuzzer-found bugs * Add the extensive CHANGELOG entry * Add the test and changelog entry from the subsumed #413 * Add an early return + more tests for the expensive newtype or tuple check for unwrapping newtype variants
How should
Some
be treated in the presence of theunwrap_variant_newtypes
extension? With that extension,NewtypeVariant(Some(a: 42, b: 24))
already worked, butSome(a: 42, b: 24)
did not. In my opinionSome
is just a newtype variant and should parse consistently here. @torkleyy what are your thoughts?CHANGELOG.md