Skip to content

Fix newtype variant unwrapping inside enum, seq and map#331

Merged
torkleyy merged 2 commits into
ron-rs:masterfrom
juntyr:master
Nov 2, 2021
Merged

Fix newtype variant unwrapping inside enum, seq and map#331
torkleyy merged 2 commits into
ron-rs:masterfrom
juntyr:master

Conversation

@juntyr

@juntyr juntyr commented Oct 29, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

This is a follow-up to #319 which added the unwrap_variant_newtypes extension.

Unfortunately, it seems like I missed a few interactions with parsing enums, sequences and maps nested inside a newtype variant (i.e. not inside a containing non-newtype struct). I have fixed all three and added extra test cases to cover their intended functionality, i.e. NewtypeVariant( [ Struct(...) ] ), NewtypeVariant( { "key": Struct(...) } ), and NewtypeVariant( StructVariant(...) ) all parse again.

Discussion

Option<T> is another fun case. Here I think carrying through the unwrapping actually is quite neat, but I want to get your input if this should be the intended behaviour:

  • NewtypeVariant( None )
  • NewtypeVariant( Some(a: 4, bool: false) ) for Option<Struct> and struct Struct { a: u32, b: bool }
  • NewtypeVariant( a: 4, bool: false ) is the implicit_some extension is switched on as well

I am also a bit unsure about how to handle deserialize_any. I think I may need your help with that one.

TODO

  • I've included my change in CHANGELOG.md

@torkleyy

torkleyy commented Oct 31, 2021

Copy link
Copy Markdown
Member

Hey, thanks for the follow-up! I'm not sure when I will be able to review this.

I am also a bit unsure about how to handle deserialize_any. I think I may need your help with that one.

I don't think there's anything special to do for deserialize_any; if there's no type information given to us we don't need to implicitly generate anything. That does mean that self-described and statically typed data will not deserialize into the same thing, but that is also the case with other self-describing serde formats (and already the case with RON as well).

@juntyr

juntyr commented Oct 31, 2021

Copy link
Copy Markdown
Member Author

I guess it might be easiest then to just turn off the newtype variant unwrapping if we enter deserialize_any? So then the semantics would be that unless type information is given, the unwrapping cannot be performed. Ideally there wouldn’t be a special case, of course, but some types (e.g. wrapped unit and struct) can only be parsed if they are described in the RON, which is exactly what the extension avoids.

@juntyr

juntyr commented Nov 2, 2021

Copy link
Copy Markdown
Member Author

@torkleyy I've disabled newtype variant unwrapping inside deserialize_any as discussed and added the fix to the changelog

torkleyy
torkleyy approved these changes Nov 2, 2021
View reviewed changes

@torkleyy torkleyy left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks, looks good to me!

@torkleyy torkleyy merged commit 37c0b22 into ron-rs:master Nov 2, 2021
torkleyy added a commit to torkleyy/ron that referenced this pull request Jun 6, 2022
@torkleyy
Merge pull request ron-rs#331 from MomoLangenstein/master
11cdae6 
Fix newtype variant unwrapping inside enum, seq and map
@torkleyy torkleyy mentioned this pull request Jun 6, 2022
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@torkleyy torkleyy torkleyy approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@juntyr @torkleyy