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

Make untagged enums work #253

Closed
wants to merge 2 commits into from
Closed

Make untagged enums work #253

wants to merge 2 commits into from

Conversation

joonazan
Copy link

@joonazan joonazan commented Jul 6, 2020

When deserializing an untagged enum, serde uses deserialize_any. There is ambiguity, as we cannot know if a struct or an enum is expected. That ambiguity is resolved by serde if we just give it a JSON-like data structure.

When we encounter an lone identifier, it becomes a string. An identifier with parens after it becomes a singleton map.

Solves #217

Newtype pain

Id(Val) would be {"Id": "Val"} in JSON, but RON produces a one-element tuple instead. The only way to identify that case that I could think of is checking for commas, which unfortunately makes deserializing somewhat quadratic. It could be linear if this was done as a preprocessing pass.

Is there a way to first read a tuple but then change it if it only has one element?

Failing test case

One of the tests (

fn test_complex() {
) now fails because it doesn't expect a map. I think it should expect a map, but I don't understand that test's purpose very well, so maybe someone else can comment on this?

@kvark kvark requested a review from torkleyy July 7, 2020 03:50
Copy link
Collaborator

@kvark kvark left a comment

Choose a reason for hiding this comment

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

Thank you for this addition!
I'd love to have @torkleyy reviewing this. If not, I'll try to take another pass.
Having a hard time wrapping my head around serde stuff when I'm out of context for a while

src/de/mod.rs Outdated Show resolved Hide resolved
@halvnykterist
Copy link

I came upon this issue and tried using the PR branch in my code. It seems to work as intended, but there are two issues.

  • Line/Column information seems to be missing on the errrors.
  • In the case of errors in self-referential enums, a stack overflow (!!) occurs. I've got a minimal example below:
use serde::Deserialize;
use ron;

#[derive(Deserialize)]
#[serde(untagged)]
enum Number {
    Number(i32),
    SelfReferencing(Box<Number>),
}

fn main() {
    let _: Number = ron::de::from_str("This causes a stack overflow").unwrap();
}

@kvark
Copy link
Collaborator

kvark commented Dec 3, 2020

@torkleyy could you have a look, please?

@joonazan
Copy link
Author

joonazan commented Dec 4, 2020

@halvnykterist This PR should not be merged. Instead, a new version should be written that implements this in a cleaner way.

Maybe there is some way to use serde that I couldn't find. My first post describes the problems I faced.

If not, the best solution would be to feed the input characters into a state machine as they are read in. The current implementation checks the same characters multiple times.

The stack overflow is not ideal but does make sense. After all, there could be an infinite number of nested SelfReferencing I don't think it is related to this patch.

@github-actions
Copy link

Issue has had no activity in the last 60 days and is going to be closed in 7 days if no further activity occurs

@github-actions github-actions bot added the stale label Nov 18, 2021
@github-actions github-actions bot closed this Nov 25, 2021
juntyr added a commit to juntyr/ron that referenced this pull request Jan 5, 2022
juntyr added a commit to juntyr/ron that referenced this pull request Jan 5, 2022
juntyr added a commit to juntyr/ron that referenced this pull request Sep 18, 2022
@juntyr juntyr mentioned this pull request Sep 18, 2022
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants