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

Derived "apply" implementation for enums is broken #35

Open
Diggsey opened this issue Jul 4, 2021 · 0 comments · May be fixed by #36
Open

Derived "apply" implementation for enums is broken #35

Diggsey opened this issue Jul 4, 2021 · 0 comments · May be fixed by #36

Comments

@Diggsey
Copy link

Diggsey commented Jul 4, 2021

When diffing an enum like:

#[derive(SerdeDiff, Serialize, Deserialize, Debug, PartialEq, Clone)]
enum Value {
    Str(String),
    Int(i32),
}

Let's say we have the diff Value::Str("A") -> Value::Str("B").

The diff implementation will produce the following sequence of commands:

[{"Enter":{"EnumVariant":"Str"}},{"Enter":{"FieldIndex":0}},{"Value":"B"},"Exit"]

However, the apply implementation does not consume the final Exit command, causing the rest of the command stream to get out of sync.

This is because of this code:

fn apply<'de, A>(
&mut self,
seq: &mut A,
ctx: &mut serde_diff::ApplyContext,
) -> Result<bool, <A as serde_diff::_serde::de::SeqAccess<'de>>::Error>
where
A: serde_diff::_serde::de::SeqAccess<'de>, {
let mut __changed__ = false;
match (self, ctx.next_path_element(seq)?) {
(this, Some(serde_diff::DiffPathElementValue::FullEnumVariant)) => {
ctx.read_value(seq, this)?;
__changed__ = true;
}
#(#apply_match_arms)*
_ => ctx.skip_value(seq)?,
}
Ok(__changed__)
}

Specifically it's because next_path_element is only called once, instead of being called until it finds an Exit command. It makes sense to only call it once because an enum can only have one variant, but since the variant still counts as a path segment, an additional Exit should be consumed.

@Diggsey Diggsey linked a pull request Jul 4, 2021 that will close this issue
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 a pull request may close this issue.

1 participant