Skip to content
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

Fuzz serde enum representations #502

Merged
merged 60 commits into from
Oct 7, 2023
Merged

Fuzz serde enum representations #502

merged 60 commits into from
Oct 7, 2023

Conversation

juntyr
Copy link
Member

@juntyr juntyr commented Sep 11, 2023

Fixes #500, since all (known) roundtrip issues are now documented and tested. Once serde allows us to fix them, they can be removed from the newly expanded restrictions section.

  • externally tagged enums
  • internally tagged enums
  • internally tagged structs
  • adjacently tagged enums
  • untagged enums
  • other variant in internally tagged enums
  • other variant in adjacently tagged enums
  • serialize flattened struct
  • serialize flattened struct variant
  • deserialize flattened struct
  • deserialize flattened struct variant
    • externally tagged
    • untagged
    • adjacently tagged
    • internally tagged
  • document one-tuple/one-array limitation inside unwrapped newtype variants
  • document that serde only supports unit, bool, integer, float, char, string, and byte keys inside flattened structs, and only string and byte keys and u8 and u64 when the flattened struct is inside an untagged or internally tagged enum document that we only support string keys inside flattened maps (even though serde technically permits more, this support is not roundtrip-safe as soon as flattened maps are involved)
  • document that serde does not support flattened structs containing conflicting keys, e.g. an inner key matches a later outer key or two flattened maps in the same struct share a key
  • document that #[enable(implicit_some)] is not supported with Options inside #[serde(flatten)]
  • document that PrettyConfig::struct_names(true) is not supported inside flattened structs / struct variants
  • Add tests for any bug fixes
  • Add tests for all known bugs
    • structs or struct variants that contain a #[serde(flatten)]ed field:
      • are only serialised as maps and deserialised from maps
      • must not contain duplicate fields / keys, e.g. where an inner-struct field matches an outer-struct or inner-struct field
      • must not contain more than one (within the super-struct of all flattened structs) #[serde(flatten)]ed map field, which collects all unknown fields
      • if they contain a #[serde(flatten)]ed map, they must not contain:
        • a struct that is not flattened itself but contains some flattened fields and is flattened into the outer struct (variant)
        • an untagged struct variant that contains some flattened fields
        • a flattened externally tagged newtype, tuple, or struct variant, flattened internally tagged unit, newtype, or struct variant, or any flattened adjacently tagged variant
        • a flattened tagged struct
    • ron only supports string keys inside maps flattened into structs
    • internally (or adjacently) tagged or untagged enum variants or #[serde(flatten)]ed fields must not contain:
      • i128 or u128 values
      • struct names, e.g. by enabling the PrettyConfig::struct_names setting
      • newtypes
      • zero-length arrays / tuples / tuple structs / structs / tuple variants / struct variants
        • Options with #[enable(implicit_some)] must not contain any of these or a unit, unit struct, or an untagged unit variant
      • externally tagged tuple variants with just one field (that are not newtype variants)
      • tuples or arrays with just one element are not supported inside newtype variants with #[enable(unwrap_variant_newtypes)]
      • a ron::value::RawValue
    • untagged tuple / struct variants with no fields are not supported
    • untagged tuple variants with just one field (that are not newtype variants) are not supported when the #![enable(unwrap_variant_newtypes)] extension is enabled
    • internally tagged newtype variants and #[serde(flatten)]ed fields must not contain:
      • a unit or a unit struct inside an untagged newtype variant
      • an untagged unit variant
    • serializing a ron::value::RawValue using a PrettyConfig may add leading and trailing whitespace and comments, which the ron::value::RawValue absorbs upon deserialization
  • I've included my change in CHANGELOG.md
  • Check all // BUG: comments in the fuzzer again to ensure that nothing was missed / is unnecessary or out of place

@codecov-commenter
Copy link

codecov-commenter commented Sep 11, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (2f3e5a8) 100.00% compared to head (2493a59) 100.00%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@             Coverage Diff             @@
##            master      #502     +/-   ##
===========================================
  Coverage   100.00%   100.00%             
===========================================
  Files           80        82      +2     
  Lines        11178     14244   +3066     
===========================================
+ Hits         11178     14244   +3066     
Files Coverage Δ
src/de/id.rs 100.00% <100.00%> (ø)
src/de/mod.rs 100.00% <100.00%> (ø)
src/de/tag.rs 100.00% <100.00%> (ø)
src/parse.rs 100.00% <100.00%> (ø)
src/value/raw.rs 100.00% <100.00%> (ø)
tests/115_minimal_flattening.rs 100.00% <100.00%> (ø)
tests/401_raw_identifier.rs 100.00% <100.00%> (ø)
tests/407_raw_value.rs 100.00% <100.00%> (ø)
tests/449_tagged_enum.rs 100.00% <100.00%> (ø)
tests/502_known_bugs.rs 100.00% <100.00%> (ø)
... and 1 more

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@juntyr juntyr mentioned this pull request Sep 17, 2023
1 task
@juntyr juntyr marked this pull request as ready for review September 22, 2023 17:16
@juntyr juntyr marked this pull request as draft September 22, 2023 17:16
@juntyr
Copy link
Member Author

juntyr commented Sep 22, 2023

@torkleyy Holy dang, serde attributes are a mess. This nightmare has really only unearthed two or three bugs in ron (around tagged enum tag identifier deserialising), many cases where we cannot roundtrip because serde's Content loses information (see serde-rs/serde#1183), but MANY nightmares around serde attributes just plain not roundtripping themselves. I will try to add some tests and update our restrictions README in the PR, but I won't spend any time prettifying our fuzzer at this point. It will hopefully get nicer over time as the above restrictions are lifted.

Hopefully I can get this PR merged early next week and then finally, with the confidence that we now know about the limitations of serde attributes in ron, release v0.9.

... and I need to fix those tests ... later. And as always, the fuzzer finds another bug just as I think I'm done with this ;(

src/parse.rs Outdated Show resolved Hide resolved
@juntyr juntyr marked this pull request as ready for review October 6, 2023 22:07
@juntyr
Copy link
Member Author

juntyr commented Oct 6, 2023

@torkleyy I think this is finally ready* (I want to merge and rebase on #512 first). Gosh this PR is huge ... and soooo cursed. And probably once the fuzzer changes lands I get many new edge case reports soon. But it is a small step phew

Oh and the benchmark failure should be fixed by this PR too

Copy link
Member Author

@juntyr juntyr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@juntyr juntyr merged commit 5f12b4a into ron-rs:master Oct 7, 2023
9 checks passed
@juntyr juntyr deleted the fuzz-untagged branch October 7, 2023 06:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Untagged enum roundtrip issues
2 participants