Skip the rest of an element that a visitor did not read fully

[hxperl]

ElementMapAccess consumes the End event of its element only when it is read until next_key_seed() returns None. A Visitor is under no obligation to do that, and when one stops early the End event stays in the stream and is then seen by the deserializer of the parent element, which desynchronises everything after it.

That is reachable from safe user code through impl_deserialize_for_internally_tagged_enum!. The generated visit_map() hands the map to serde's MapAccessDeserializer, and its deserialize_enum() reads exactly one entry — so mapping a variant onto an inner enum leaves the element half-read and </action> unconsumed. That is the shape reported in #928.

The change

A new ElementMapAccess::end() reads the accessor to its end, called from a new deserialize_element() helper that replaces the four bare visitor.visit_map(ElementMapAccess::new(...)) call sites.

It is a no-op when the visitor already reached the end, which is what #[derive(Deserialize)] does for structs — so the common path is unchanged.

Test

tests/serde-issues.rs gains issue928_partially_read_element, which reproduces the reported mapping: the Create variant deliberately maps to the inner Element enum rather than to ElementHolder, which is what pushes serde down the deserialize_enum path that reads only one entry.

Verification — please read this part

I could not complete a cargo test run on this machine, and I am not going to imply otherwise. The environment I was working in ran out of disk repeatedly and the build directory had to be deleted mid-flight more than once, so I have no trustworthy green run to report. CI will need to be the first real confirmation that this compiles and passes.

What I am confident about is the reasoning: the mechanism above is read off the source, the end() call is a no-op on the path derive(Deserialize) takes, and the regression test encodes the exact mapping from the issue rather than an approximation of it.

If CI is red, or if you would rather the drain happened somewhere other than deserialize_element(), say so and I will rework it.

Closes #928

[hxperl]

Follow-up on the verification note in the description: I have since been able to run the suite, so here is the real evidence. I have also pushed the missing Changelog.md entry (0ebc369), which I had overlooked.

Environment: macOS arm64, cargo 1.95.0, against 2eaa844.

Before the fix, the three new tests fail on current master:

---- issue928_partially_read_element::only_element stdout ----
thread '...::only_element' panicked at src/de/map.rs:322:21:
assertion `left == right` failed
  left: QName(root)
 right: QName(action)

---- issue928_partially_read_element::element_then_sibling stdout ----
thread '...::element_then_sibling' panicked at src/de/map.rs:322:21:
assertion `left == right` failed
  left: QName(root)
 right: QName(action)

---- issue928_partially_read_element::sequence stdout ----
thread '...::sequence' panicked at src/de/map.rs:975:21:
assertion `left == right` failed
  left: QName(root)
 right: QName(action)

test result: FAILED. 1 passed; 3 failed

This is not only a debug_assert problem. With the asserts compiled out the desynchronisation produces wrong results instead of a panic. Same three tests, same unmodified master, --release:

• sequence deserialises only the first of the three <action> elements; the Modify and the second Create entries are silently missing from the Vec.
• element_then_sibling fails with Custom("missing field \after`"), even though ` is present in the document.

test result: FAILED. 2 passed; 2 failed

That is the part I would ask you to weigh when judging whether this is worth fixing: release builds lose list items and report spurious missing fields.

After the fix, cargo test --all-features:

test result: ok. 153 passed; 0 failed; 6 ignored   (doc-tests)

25 test binaries, 4313 passed, 0 failed, 40 ignored, exit code 0. cargo test (default features) and cargo test --features serialize are also green, and cargo fmt --check is clean. The same three tests pass under --release as well.

Limits of what I checked: everything above is macOS arm64 only, and I did not run the MSRV or minimal-versions jobs, so CI will need to confirm Linux and the other feature combinations.

A judgement call worth disagreeing with. I chose to skip the unread remainder of the element rather than to error on it. Skipping is what makes the reported document deserialise to the value the reporter expected, and it keeps the derive-generated path byte-identical since those visitors always reach the end. The alternative — returning an error when a visitor stops early — would be stricter and would surface mapping mistakes like the one in #928 instead of quietly accepting them, at the cost of turning today's panic into a hard error for anyone currently relying on the release-mode behaviour. If you would rather have the strict version, I am happy to redo it that way.

Note also that this only restores synchronisation; it does not change the fact that, as you pointed out in the issue, Action::Create is more correctly mapped to ElementHolder. The existing issue928 test still covers that mapping.

[codecov-commenter]

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

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 55.23%. Comparing base (e00ae5c) to head (0ebc369).
⚠️ Report is 76 commits behind head on master.
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1018      +/-   ##
==========================================
- Coverage   57.31%   55.23%   -2.08%     
==========================================
  Files          46       51       +5     
  Lines       18197    18840     +643     
==========================================
- Hits        10429    10407      -22     
- Misses       7768     8433     +665     

Flag	Coverage Δ
unittests	55.23% <100.00%> (-2.08%)	⬇️

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]

ElementMapAccess consumes the End event of its element only when it is read until next_key_seed() returns None. A Visitor is under no obligation to do that

I do not think, that this is true. If you look at the serde-json source code, you will find that it expect that the visitor will consume all the keys.

[hxperl]

You're right, and I tested it rather than argue. Thank you for the pointer.

I reproduced the equivalent shape against serde_json directly — a visit_map that hands the map to MapAccessDeserializer and calls deserialize_enum, so exactly one entry is read and the rest is left:

fn visit_map<A: MapAccess<'de>>(self, map: A) -> Result<Action, A::Error> {
    let e = Element::deserialize(MapAccessDeserializer::new(map))?;
    Ok(Action(e))
}

input:  {"node":{"id":7},"leftover":1}
result: Err(trailing comma at line 1 column 17)

control: {"node":{"id":7}}
result: Ok(Action(Node { id: 7 }))

So serde_json errors on a partially-read map. end_map() expects } immediately and returns TrailingCharacters otherwise. That is the opposite of what this PR does — it drains silently — and my premise in the description ("a Visitor is under no obligation") does not hold up.

Which leaves a design question that is yours, not mine:

1. Match serde_json — error when the visitor stops early, rather than draining. Consistent with the ecosystem, but it turns impl_deserialize_for_internally_tagged_enum panic on deserialize when inner element is itself an enum #928 from a silent corruption into a hard failure, and the user's code in that issue still would not work.
2. Drain, as this PR does — impl_deserialize_for_internally_tagged_enum panic on deserialize when inner element is itself an enum #928's code works, at the cost of diverging from serde_json and accepting input other deserializers reject.
3. Neither — the real problem is impl_deserialize_for_internally_tagged_enum! generating a visitor that reads one entry, and the fix belongs there.

I lean towards 3 being the honest reading of #928, but I have not explored it and it is a bigger change. Happy to take whichever direction you prefer, or to close this if the drain is the wrong shape entirely.

[dralley]

@hxperl Please read the AI assisted contributions policy https://github.com/tafia/quick-xml/blob/master/CONTRIBUTING.md#ai-assisted-contributions-policy

[hxperl]

Closing this — the premise in the description was wrong. serde_json errors on a partially-read map rather than draining it, so draining here diverges from the ecosystem, and the right fix is probably in the macro that generates a one-entry visitor rather than in the deserializer.

Leaving #928 for someone who can give it the design attention it needs. Sorry for the noise.