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

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

Merged
merged 2 commits into from
Nov 2, 2021

Conversation

juntyr
Copy link
Member

@juntyr juntyr commented Oct 29, 2021

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
Copy link
Contributor

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
Copy link
Member Author

juntyr commented Oct 31, 2021

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
Copy link
Member Author

juntyr commented Nov 2, 2021

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

Copy link
Contributor

@torkleyy torkleyy left a comment

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
Fix newtype variant unwrapping inside enum, seq and map
@torkleyy torkleyy mentioned this pull request Jun 6, 2022
2 tasks
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.

2 participants