Fix "enum newtype variant" deserialization (#819) by dralley · Pull Request #995 · tafia/quick-xml

dralley · 2026-07-30T18:56:52Z

Enum newtype variants whose inner type is also an enum (e.g. Apply(Vec<MathNode>)) caused infinite re-entry because
newtype_variant_seed passed the raw Deserializer back without consuming the variant's Start event.

Introduce VariantContentDeserializer to consume the Start event first, then dispatch correctly for all inner types: structs use the consumed start as root, enums/sequences delegate to the stream-based Deserializer with proper End-tag cleanup.

This also fixes two related issues:

Newtype variants containing sequences of child elements (e.g. Variant(Vec<i32>) with <Variant><x>1</x><x>2</x></Variant>) previously failed with UnexpectedStart because the unconsumed Start event was treated as the first sequence element.
Sequences inside newtype variants could consume past the enclosing variant's End tag, because the top-level SeqAccess only stops on Eof. The new VariantSeqAccess correctly stops on End events.

Assisted-By: Claude Opus 4.6
closes #819

dralley · 2026-07-30T19:23:36Z

Test results including the new tests but prior to adding the commit with the fix: https://github.com/tafia/quick-xml/actions/runs/30572674406/job/90973123053

dralley · 2026-07-30T19:44:25Z

I looked for existing reports of the two other issues mentioned and didn't find any, #710 is similar but it is about internally-tagged enums rather than externally tagged ones. But maybe I missed something.

codecov-commenter · 2026-07-30T19:50:37Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 67.07317% with 27 lines in your changes missing coverage. Please review.
✅ Project coverage is 55.98%. Comparing base (e00ae5c) to head (8a8c651).
⚠️ Report is 35 commits behind head on master.

Files with missing lines	Patch %	Lines
src/de/var.rs	67.07%	27 Missing ⚠️
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #995      +/-   ##
==========================================
- Coverage   57.31%   55.98%   -1.33%     
==========================================
  Files          46       47       +1     
  Lines       18197    18438     +241     
==========================================
- Hits        10429    10322     -107     
- Misses       7768     8116     +348

Flag	Coverage Δ
unittests	`55.98% <67.07%> (-1.33%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Mingun

Could you rearrange the tests and add missing ones (or explain why you removed them)?

Mingun · 2026-07-31T17:04:50Z

+    /// Newtype variants containing a Vec should be able to deserialize
+    /// child elements. Previously this failed with `UnexpectedStart`
+    /// because the unconsumed variant Start event was fed to the
+    /// sequence as its first element.
+    #[test]
+    fn newtype_variant_with_child_element_seq() {
+        #[derive(Debug, Deserialize, PartialEq)]
+        enum E {
+            Numbers(Vec<i32>),
+        }
+
+        let result: E = from_str("<Numbers><n>1</n><n>2</n><n>3</n></Numbers>").unwrap();
+        assert_eq!(result, E::Numbers(vec![1, 2, 3]));
+    }
+
+    /// When multiple enum instances appear as siblings, the Vec inside
+    /// each newtype variant must stop at its own End tag and not consume
+    /// into the next sibling.
+    #[test]
+    fn newtype_variant_seq_boundary() {
+        #[derive(Debug, Deserialize, PartialEq)]
+        enum E {
+            Group(Vec<i32>),
+        }
+
+        let result: Vec<E> =
+            from_str("<Group><n>1</n><n>2</n></Group><Group><n>3</n></Group>").unwrap();
+        assert_eq!(result, vec![E::Group(vec![1, 2]), E::Group(vec![3])]);
+    }
+
+    /// Truncated XML (missing end tag) inside a newtype variant sequence
+    /// must produce an error, not silently return an empty sequence.
+    #[test]
+    fn newtype_variant_seq_eof_is_error() {
+        #[derive(Debug, Deserialize, PartialEq)]
+        enum E {
+            Numbers(Vec<i32>),
+        }
+
+        let err = from_str::<E>("<Numbers><n>1</n>").unwrap_err();
+        assert!(
+            matches!(err, DeError::InvalidXml(_)),
+            "expected InvalidXml error for truncated input, got: {err:?}"
+        );
+    }


Actually, I do not like that. Where did the name n come from? This fundamentally cannot be serialized back in the same way.

You are correct, my bad

dralley · 2026-08-01T04:33:22Z

Squashed everything into the original commits

New test commit CI run, no fixes: https://github.com/tafia/quick-xml/actions/runs/30657807408/job/91246450438?pr=995

Mingun

It seems that for proper implementation, deserialization will need to be reworked, by separating the places of use of the top-level deserializer and in-XML-tree deserializers. Without that it seems will be impossible to fix that bug without introducing others.

Here at least we need to restore tuple_variant_box and tuple_variant_vec tests, what were remove, in my opinion, without any reasons.

Mingun · 2026-08-02T13:52:23Z

+    fn deserialize_seq<V>(self, visitor: V) -> Result<V::Value, Self::Error>
+    where
+        V: Visitor<'de>,
+    {
+        let mut this = self;
+        let result = visitor.visit_seq(&mut this)?;
+        this.de.read_to_end(this.start.name())?;
+        Ok(result)
+    }


Why not

Suggested change

fn deserialize_seq<V>(self, visitor: V) -> Result<V::Value, Self::Error>

where

V: Visitor<'de>,

{

let mut this = self;

let result = visitor.visit_seq(&mut this)?;

this.de.read_to_end(this.start.name())?;

Ok(result)

}

fn deserialize_seq<V>(mut self, visitor: V) -> Result<V::Value, Self::Error>

where

V: Visitor<'de>,

{

let result = visitor.visit_seq(&mut self)?;

self.de.read_to_end(self.start.name())?;

Ok(result)

}

?

Mingun · 2026-08-02T14:01:45Z

+        match self.de.peek()? {
+            DeEvent::End(_) => Ok(None),
+            DeEvent::Eof => Err(Error::missed_end(self.start.name()).into()),
+            _ => seed.deserialize(&mut *self.de).map(Some),


Probably we should use another deserializer here. Deserializer have some assumptions that it is top-level deserializer (yes, probably in some other places the same reborrow used incorrectly). In particular, it allows deserializing primitives from the <tag>content</tag>, but when we inside XML tree, we should accept only content.

Need to check, what tests reaches this line.

Mingun · 2026-08-02T14:05:35Z

-    // enum tuple variant
-
    #[test]
-    fn tuple_variant_box() {


So, why were tuple recursion tests removed? We should check that this scenario also work.

I misinterpreted your statement about one of the tests being redundant. I will restore them

I see what you mean about the tuple case, but I think it can be fixed. Let me play around with this for a bit.

Mingun · 2026-08-02T14:13:46Z

+  The variant's Start event is now consumed before deserializing the inner type,
+  which also fixes deserialization of newtype variants containing sequences of
+  child elements (e.g. `Variant(Vec<i32>)` with `<Variant><x>1</x><x>2</x></Variant>`),
+  which previously failed with `UnexpectedStart`, and corrects sequence boundary
+  handling so that sequences no longer consume past the enclosing variant's end tag.


I still do not want deserialization of Variant(Vec<i32>) from <Variant><x>1</x><x>2</x></Variant>. I think, it would be serialized as <Variant>1 2</Variant> (using SimpleTypeSerializer), so it should be deserialized using SimpleTypeDeserializer. Acceptance of that form is allowed due to using top-level Deserializer when we inside the XML tree.

I don't know that it can easily work both ways - both support recursion a la #819 (see: MathNode case) while also using SimpleTypeSerializer and rejecting <tag>content</tag>

Anyway, the scope of this has gotten beyond what I'm comfortable working on right now, considering I don't know the serde code anywhere near as well as the rest of the library.

I will pause this for now, and maybe resurrect it later, probably after the next release. The only impetus for working on it was that it was related to 978.

- Update issue978 tests to assert successful deserialization where previously it would encounter recursion limits due to tafia#819 - Add tests for issue tafia#819 covering recursive enums, child-element sequences inside newtype variants, and sequence boundary correctness across sibling variant instances. Assisted-By: Claude Opus 4.6

Enum newtype variants whose inner type is also an enum (e.g. `Apply(Vec<MathNode>)`) caused infinite re-entry because `newtype_variant_seed` passed the raw Deserializer back without consuming the variant's Start event. Introduce VariantContentDeserializer to consume the Start event first, then dispatch correctly for all inner types: structs use the consumed start as root, enums/sequences delegate to the stream-based Deserializer with proper End-tag cleanup. This also fixes two related issues: - Newtype variants containing sequences of child elements (e.g. `Variant(Vec<i32>)` with `<Variant><x>1</x><x>2</x></Variant>`) previously failed with `UnexpectedStart` because the unconsumed Start event was treated as the first sequence element. - Sequences inside newtype variants could consume past the enclosing variant's End tag, because the top-level SeqAccess only stops on Eof. VariantContentDeserializer implements SeqAccess such that it correctly stops on End events. Assisted-By: Claude Opus 4.6 closes tafia#819

Tests in "mod issue978" ensure that recursion depth testing works as expected. There already exist tests in serde-de-enum which check for correctness of the actual deserialization of such values, using enums and newtypes, but no such tests seem to exist for recursive structs. Add recursive_struct tests that assert actual output for Box, Vec, $value Box, and $value Vec field patterns.

The same re-entry bug that affected newtype variants also affected tuple variants. Tuple variants use repeated sibling elements (<V>f1</V><V>f2</V>), so consuming the Start event requires a different approach: a TupleVariantSeqAccess that wraps each repeated element in a VariantContentDeserializer individually. Assisted-By: Claude Opus 4.6

) Replace TooDeeplyNested assertions with success tests using valid recursive XML, and add depth guard tests using a nested_tuple_enum helper.

Variant sequences like `Variant(Vec<i32>)` should not accept child elements like `<Variant><x>1</x><x>2</x></Variant>`. The new InTreeDeserializer wrapper uses `read_string_impl(false)` for primitive types, which rejects Start events with UnexpectedStart. Non-primitive methods (enum, struct, seq) delegate to the underlying Deserializer, so recursive enum sequences like `Vec<MathNode>` from child elements still work correctly.

dralley force-pushed the fix819 branch 2 times, most recently from 8d8afca to bb31e09 Compare July 30, 2026 19:20

dralley force-pushed the fix819 branch 2 times, most recently from 2b4c823 to 9fc33c9 Compare July 30, 2026 19:36

dralley changed the title ~~Fix "enum newtype variant" recursive deserialization (#819)~~ Fix "enum newtype variant" deserialization (#819) Jul 30, 2026

Mingun reviewed Jul 30, 2026

View reviewed changes

Comment thread src/de/var.rs Outdated

dralley marked this pull request as ready for review July 30, 2026 20:42

dralley force-pushed the fix819 branch from 021ae8c to 07b1bea Compare July 30, 2026 21:09

dralley commented Jul 30, 2026

View reviewed changes

Comment thread Changelog.md

dralley requested a review from Mingun July 31, 2026 15:11

Mingun requested changes Jul 31, 2026

View reviewed changes

dralley marked this pull request as draft July 31, 2026 18:22

dralley force-pushed the fix819 branch 2 times, most recently from 40b157f to db77ebf Compare July 31, 2026 19:06

dralley commented Aug 1, 2026

View reviewed changes

Comment thread tests/serde-issues.rs

dralley force-pushed the fix819 branch 3 times, most recently from 7174081 to 89e37d9 Compare August 1, 2026 18:42

dralley marked this pull request as ready for review August 2, 2026 02:23

dralley commented Aug 2, 2026

View reviewed changes

Comment thread tests/serde-de.rs

dralley commented Aug 2, 2026

View reviewed changes

Comment thread tests/serde-de.rs

Mingun requested changes Aug 2, 2026

View reviewed changes

dralley marked this pull request as draft August 2, 2026 23:52

dralley added 3 commits August 2, 2026 20:14

Structure test cases in issue978 more similarly to serde-de-enum

655a8ff

dralley force-pushed the fix819 branch from 32156de to 8a8c651 Compare August 3, 2026 00:14

dralley added 3 commits August 2, 2026 20:39

Update tuple variant tests for recursive enum deserialization (tafia#819

c995a46

) Replace TooDeeplyNested assertions with success tests using valid recursive XML, and add depth guard tests using a nested_tuple_enum helper.

dralley force-pushed the fix819 branch from d21f2d5 to c0878ba Compare August 3, 2026 02:40

dralley closed this Aug 3, 2026

Conversation

dralley commented Jul 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dralley commented Jul 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dralley commented Jul 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Jul 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Mingun left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dralley commented Aug 1, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Mingun left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dralley Aug 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dralley Aug 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dralley commented Jul 30, 2026 •

edited

Loading

dralley commented Jul 30, 2026 •

edited

Loading

dralley commented Jul 30, 2026 •

edited

Loading

codecov-commenter commented Jul 30, 2026 •

edited

Loading

dralley Aug 2, 2026 •

edited

Loading

dralley Aug 3, 2026 •

edited

Loading