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.46988% with 27 lines in your changes missing coverage. Please review.
✅ Project coverage is 55.98%. Comparing base (e00ae5c) to head (89e37d9).
⚠️ Report is 34 commits behind head on master.

Files with missing lines	Patch %	Lines
src/de/var.rs	67.46%	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    18439     +242     
==========================================
- Hits        10429    10323     -106     
- Misses       7768     8116     +348

Flag	Coverage Δ
unittests	`55.98% <67.46%> (-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-31T16:27:59Z

+    fn tuple_variant() {
+        #[derive(Debug, Deserialize, PartialEq)]
        enum E {
-            Wrap(Vec<E>, String),
-            #[allow(dead_code)]
-            Leaf,
+            Wrap(f64, String),
        }

-        // FIXME: #819 — same re-entry issue as newtype variants
-        let xml = "<Wrap></Wrap>";
+        // Tuple variants use repeated elements
+        let xml = "<Wrap>4.2</Wrap><Wrap>answer</Wrap>";
        let mut de = XmlDeserializer::from_str(xml);
        de.recursion_limit(3);
-        assert!(matches!(
-            E::deserialize(&mut de),
-            Err(DeError::TooDeeplyNested(_))
-        ));
+        assert_eq!(
+            E::deserialize(&mut de).unwrap(),
+            E::Wrap(4.2, "answer".into())
+        );
    }


Why you remove recursion from the type? Now it does not test what we need and is redundant -- there is already such a test here:

quick-xml/tests/serde-de-enum.rs

Lines 84 to 88 in 4e85c3d

#[test]

fn tuple_struct() {

let data: Node = from_str("<Tuple>42</Tuple><Tuple>answer</Tuple>").unwrap();

assert_eq!(data, Node::Tuple(42.0, "answer".into()));

}

I think the commit is just messy because of what was moved around - some of which obviously should have been deleted (the duplicates)

Sorry. Cleaning this up

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

Mingun · 2026-07-31T17:20:20Z

+/// A [`SeqAccess`] that iterates over child elements within a variant element.
+///
+/// Unlike the top-level [`SeqAccess`] impl on [`Deserializer`] (which only stops
+/// on [`DeEvent::Eof`]), this stops on [`DeEvent::End`] as well, since the
+/// sequence is bounded by the enclosing variant element's end tag.
+struct VariantSeqAccess<'de, 'd, R, E>


Separate struct is not necessary, you may implement trait for &mut VariantContentDeserializer<'de, 'd, R, E>. Less code -- more clear, I think. What do you think?

Also, "as well" assumes, that it stops (without errors) at End and Eof, which is not true.

Agreed, it's cleaner that way.

Mingun · 2026-07-31T18:09:34Z

+                // SAFETY: the other events are filtered in `variant_seed()`
+                _ => unreachable!("Only `Start` events are possible here"),
+            }
        }


It seems it is even unnecessary to check is_text here, this also worked:

match self.de.next()? { DeEvent::Start(e) => seed.deserialize(VariantContentDeserializer { start: e, de: self.de, }), DeEvent::Text(e) => seed.deserialize(SimpleTypeDeserializer::from_text_content(e)), // SAFETY: the other events are filtered in `variant_seed()` _ => unreachable!("Only `Start` or `Text` events are possible here"), }

Perhaps, that check is redundant for tuple_variant too and is_text could be removed.

It does look like it can be removed

- 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

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

dralley · 2026-08-01T05:13:45Z

    #[test]
    fn newtype_variant_box() {
-        #[derive(Debug, Deserialize)]
+        #[derive(Debug, Deserialize, PartialEq)]


So, just double checking - you also want newtype_variant_vec and newtype_variant_box moved to serde-de-enum.rs, yes?

Many of the other tests under mod issue978 (not these specifically, the existing untouched ones that aren't part of this PR) feel like they belong elsewhere also - they seem very generalized.

So, just double checking - you also want newtype_variant_vec and newtype_variant_box moved to serde-de-enum.rs, yes?

Yes, that's natural place for those tests.

issues.rs and serde-issues.rs are intended for tests which would test literal scenarios from our issues to ensure, that what was fixed will never broken again. Sometimes they can be shortened by throwing out code that is clearly unnecessary for reproduction, and of course they are adapted to the current API. But the main idea is to still save the code that was provided in the issue. The rest should be placed in regular tests.

Well, both of these tests rely on nested_enum_value and are specifically related to deep nesting. They fit in well here, especially if you look at the surrounding tests. So to me it feels like they should either stay, or most of what is here should be moved elsewhere and not just these two.

It's just less obvious where the others would go - probably not in the enum tests, maybe mod nested_struct in serde-de.rs. It would mean the nesting tests get split apart and distributed.

I'm slightly in favor of just leaving everything here. Yes it's the same enum layout (roughly) but it's testing a different thing.

Well, then let's leave it as is.

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 added 2 commits July 31, 2026 17:03

dralley commented Aug 1, 2026

View reviewed changes

dralley force-pushed the fix819 branch 2 times, most recently from cdf69e1 to 7174081 Compare August 1, 2026 18:08

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

89e37d9

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

	#[test]
	fn tuple_struct() {
	let data: Node = from_str("<Tuple>42</Tuple><Tuple>answer</Tuple>").unwrap();
	assert_eq!(data, Node::Tuple(42.0, "answer".into()));
	}

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

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!

dralley commented Aug 1, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dralley Aug 1, 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!

dralley Aug 1, 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!

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 1, 2026 •

edited

Loading

dralley Aug 1, 2026 •

edited

Loading