Skip to content

Fix empty Text event emitted with trim_text_end (#984) #997

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.

Sign up for GitHub

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

Draft
dralley wants to merge 3 commits into
base: master
Choose a base branch
from
+149 −34
Draft

Fix empty Text event emitted with trim_text_end (#984) #997

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8171d08
Fix empty Text event emitted with trim_text_end (#984)
dralley Jul 31, 2026
8563595
Fix incorrect doc comment on ReadTextResult::UpToMarkup
dralley Jul 31, 2026
fd372a8
Leave note about incorrect tests
dralley Aug 15, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions Changelog.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,13 +76,15 @@ The MSRV has been raised to 1.86.
`u16` depth counter. Previously the unguarded `nesting_level += 1` panicked
under `overflow-checks` builds and silently wrapped in release, corrupting
namespace-scope bookkeeping on deeply nested untrusted input.
- [#980]: `NamespaceResolver` now caps the total number of in-scope namespace
bindings (default 128, configurable via `set_max_namespace_bindings`),
replacing the previous per-element `max_declarations_per_element` limit.
- [#978]: The serde `Deserializer` now enforces a configurable recursion-depth
limit (default 128, matching `serde_json`). Deeply nested XML returns
`DeError::TooDeeplyNested` instead of overflowing the native call stack.
Use `Deserializer::recursion_limit()` to adjust.
- [#980]: `NamespaceResolver` now caps the total number of in-scope namespace
bindings (default 128, configurable via `set_max_namespace_bindings`),
replacing the previous per-element `max_declarations_per_element` limit.
- [#984]: `Reader` no longer emits an empty `Text` event when `trim_text_end`
removes whitespace-only text immediately before markup or a reference.
- [#990]: `\r` in text content is now escaped as `
` by the serde serializer,
`BytesText::new()`, `escape()`, `partial_escape()`, and `minimal_escape()`,
preventing silent conversion to `\n` from XML end-of-line normalization on
Expand All @@ -107,6 +109,7 @@ The MSRV has been raised to 1.86.
[#859]: https://github.com/tafia/quick-xml/issues/859
[#978]: https://github.com/tafia/quick-xml/issues/978
[#983]: https://github.com/tafia/quick-xml/issues/983
[#984]: https://github.com/tafia/quick-xml/issues/984
[#989]: https://github.com/tafia/quick-xml/issues/989
[#990]: https://github.com/tafia/quick-xml/issues/990
[#1000]: https://github.com/tafia/quick-xml/pull/1000
Expand Down
7 changes: 7 additions & 0 deletions src/reader/buffered_reader.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ macro_rules! impl_buffered_source {
&mut self,
buf: &'b mut Vec<u8>,
position: &mut u64,
trim_text_end: bool,
) -> ReadTextResult<'b, &'b mut Vec<u8>> {
let mut read = 0;
let start = buf.len();
Expand Down Expand Up @@ -83,6 +84,9 @@ macro_rules! impl_buffered_source {
read += i as u64;

*position += read;
if trim_text_end && buf[start..].iter().all(|&b| is_whitespace(b)) {
return ReadTextResult::Markup(buf);
}
Comment on lines +87 to +89

@Mingun Mingun Aug 16, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think that this is not the correct place for trim. I would trim spaces before comments and CDATA sections. It seems totally wrong to me.

return match std::str::from_utf8(&buf[start..]) {
Ok(s) => ReadTextResult::UpToMarkup(s),
Err(e) => ReadTextResult::Err(e.into()),
Expand All @@ -95,6 +99,9 @@ macro_rules! impl_buffered_source {
read += i as u64;

*position += read;
if trim_text_end && buf[start..].iter().all(|&b| is_whitespace(b)) {
return ReadTextResult::Ref(buf);
}
Comment on lines +102 to +104

@Mingun Mingun Aug 16, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This change is wrong. We do not want to trim spaces before entity references, otherwise " &amp; text" becomes "& text" instead of " & text". Compare the result with " x text" -- this will be unexpected for the user if the replacement in the text x with & suddenly eats up the spaces before this character.

@dralley dralley Aug 16, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Good point, but given the existing tests pass, we probably need some additional test cases around all of this.

return match std::str::from_utf8(&buf[start..]) {
Ok(s) => ReadTextResult::UpToRef(s),
Err(e) => ReadTextResult::Err(e.into()),
Expand Down
122 changes: 94 additions & 28 deletions src/reader/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -325,7 +325,11 @@ macro_rules! read_event_impl {
$reader.skip_whitespace(&mut $self.state.offset) $(.$await)? ?;
}

match $reader.read_text($buf, &mut $self.state.offset) $(.$await)? {
match $reader.read_text(
$buf,
&mut $self.state.offset,
$self.state.config.trim_text_end,
) $(.$await)? {
ReadTextResult::Markup(buf) => {
$self.state.state = ParseState::InsideMarkup;
// Pass `buf` to the next next iteration of parsing loop
Expand All @@ -340,15 +344,10 @@ macro_rules! read_event_impl {
}
ReadTextResult::UpToMarkup(bytes) => {
$self.state.state = ParseState::InsideMarkup;
// FIXME: Can produce an empty event if:
// - event contains only spaces
// - trim_text_start = false
// - trim_text_end = true
Ok(Event::Text($self.state.emit_text(bytes)?))
}
ReadTextResult::UpToRef(bytes) => {
$self.state.state = ParseState::InsideRef;
// Return Text event with `bytes` content or Eof if bytes is empty
Ok(Event::Text($self.state.emit_text(bytes)?))
}
ReadTextResult::UpToEof(bytes) => {
Expand Down Expand Up @@ -493,17 +492,19 @@ macro_rules! read_to_end {
// it is important that this position indicates beginning of the End event.
// If between last event and the End event would be only spaces, then we
// take position before the spaces, but spaces would be skipped without
// generating event if `trim_text_start` is set to `true`. To prevent that
// we temporary disable start text trimming.
// generating an event if text trimming is enabled. To prevent that we
// temporarily disable text trimming.
//
// We also cannot take position after getting End event, because if
// `trim_markup_names_in_closing_tags` is set to `true` (which is the default),
// we do not known the real size of the End event that it is occupies in
// the source and cannot correct the position after the End event.
// So, we in any case should tweak parser configuration.
let config = $self.config_mut();
let trim = config.trim_text_start;
let trim_start = config.trim_text_start;
let trim_end = config.trim_text_end;
config.trim_text_start = false;
config.trim_text_end = false;

let start = $self.buffer_position();
let mut depth = 0;
Expand All @@ -512,20 +513,26 @@ macro_rules! read_to_end {
let end = $self.buffer_position();
match $self.$read_event($buf) $(.$await)? {
Err(e) => {
$self.config_mut().trim_text_start = trim;
let config = $self.config_mut();
config.trim_text_start = trim_start;
config.trim_text_end = trim_end;

@dralley dralley Jul 31, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I don't really like the clear->restore pattern, but it was already like this. Would be nice to refactor this away

@Mingun Mingun Jul 31, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Yes, I also does not like it here... I was thinking about removing auto-trim altogether and making it an explicit method call on the Event

@dralley dralley Aug 15, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I probably don't agree with that, at least in this specific case it structurally changes what comes out of the parser. If someone doesn't want to deal with whitespace-only text events at all (and I understand that completely, it's really quite annoying, I use trim for precisely that reason) manual trim wouldn't help.

@dralley dralley Aug 15, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I have an old partial commit from 2022 which changes the configuration options slightly. I think this would make a lot more sense (but I think might be more difficult to implement now, will need to think about how to make it work)

-    /// Trims leading whitespace in Text events, skip the element if text is empty
-    pub trim_text_start: bool,
-    /// Trims trailing whitespace in Text events.
-    pub trim_text_end: bool,
+    /// Preserves whitespace-only text elements between events (e.g. indented "pretty" formatting).
+    pub preserve_indentation: bool,
+    /// Trims leading and trailing whitespace in Text events.
+    pub trim_text: bool,

@dralley dralley Aug 15, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I can file an issue for that if you agree.

@Mingun Mingun Aug 16, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I doubt that we can reliable detect indentation and I think that is not the work for parser at all.

return Err(e);
}

Ok(Event::Start(e)) if e.name() == $end => depth += 1,
Ok(Event::End(e)) if e.name() == $end => {
if depth == 0 {
$self.config_mut().trim_text_start = trim;
let config = $self.config_mut();
config.trim_text_start = trim_start;
config.trim_text_end = trim_end;
break start..end;
}
depth -= 1;
}
Ok(Event::Eof) => {
$self.config_mut().trim_text_start = trim;
let config = $self.config_mut();
config.trim_text_start = trim_start;
config.trim_text_end = trim_end;
return Err(Error::missed_end($end));
}
_ => (),
Expand Down Expand Up @@ -998,16 +1005,19 @@ impl<R> Reader<R> {
/// Result of an attempt to read XML textual data from the source.
#[derive(Debug)]
enum ReadTextResult<'r, B> {
/// Start of markup (`<` character) was found in the first byte. `<` was consumed.
/// Contains buffer that should be returned back to the next iteration cycle
/// to satisfy borrow checker requirements.
/// The reader is positioned at `<` (start of markup). `<` was not consumed.
/// Returned either when `<` was the first byte, or when `trim_text_end` is
/// enabled and only whitespace preceded it (that whitespace was consumed and
/// discarded). Contains buffer that should be returned back to the next
/// iteration cycle to satisfy borrow checker requirements.
Markup(B),
/// Start of reference (`&` character) was found in the first byte.
/// `&` was not consumed.
/// Contains buffer that should be returned back to the next iteration cycle
/// to satisfy borrow checker requirements.
/// The reader is positioned at `&` (start of a reference). `&` was not consumed.
/// Returned either when `&` was the first byte, or when `trim_text_end` is
/// enabled and only whitespace preceded it (that whitespace was consumed and
/// discarded). Contains buffer that should be returned back to the next
/// iteration cycle to satisfy borrow checker requirements.
Ref(B),
/// Contains text block up to start of markup (`<` character). `<` was consumed.
/// Contains text block up to start of markup (`<` character). `<` was not consumed.
UpToMarkup(&'r str),
/// Contains text block up to start of reference (`&` character).
/// `&` was not consumed.
Expand Down Expand Up @@ -1069,9 +1079,16 @@ trait XmlSource<'r, B> {
/// - `buf`: Buffer that could be filled from an input (`Self`) and
/// from which [events] could borrow their data
/// - `position`: Will be increased by amount of bytes consumed
/// - `trim_text_end`: When true, a whitespace-only text block is suppressed
/// (returns [`ReadTextResult::Markup`] or [`ReadTextResult::Ref`] instead)
///
/// [events]: crate::events::Event
fn read_text(&mut self, buf: B, position: &mut u64) -> ReadTextResult<'r, B>;
fn read_text(
&mut self,
buf: B,
position: &mut u64,
trim_text_end: bool,
) -> ReadTextResult<'r, B>;

/// Read input until end of general reference (the `;`) is found, start of
/// another general reference (the `&`) is found or end of input is reached.
Expand Down Expand Up @@ -1646,7 +1663,7 @@ mod test {
let mut input = b"".as_ref();
// ^= 1

match $source(&mut input).read_text(buf, &mut position) $(.$await)? {
match $source(&mut input).read_text(buf, &mut position, false) $(.$await)? {
ReadTextResult::UpToEof(bytes) => assert_eq!(bytes, ""),
x => panic!("Expected `UpToEof(_)`, but got `{:?}`", x),
}
Expand All @@ -1660,7 +1677,7 @@ mod test {
let mut input = b"<".as_ref();
// ^= 1

match $source(&mut input).read_text(buf, &mut position) $(.$await)? {
match $source(&mut input).read_text(buf, &mut position, false) $(.$await)? {
ReadTextResult::Markup(b) => assert_eq!(b, $buf),
x => panic!("Expected `Markup(_)`, but got `{:?}`", x),
}
Expand All @@ -1674,7 +1691,7 @@ mod test {
let mut input = b"&".as_ref();
// ^= 1

match $source(&mut input).read_text(buf, &mut position) $(.$await)? {
match $source(&mut input).read_text(buf, &mut position, false) $(.$await)? {
ReadTextResult::Ref(b) => assert_eq!(b, $buf),
x => panic!("Expected `Ref(_)`, but got `{:?}`", x),
}
Expand All @@ -1688,7 +1705,7 @@ mod test {
let mut input = b"a<".as_ref();
// ^= 2

match $source(&mut input).read_text(buf, &mut position) $(.$await)? {
match $source(&mut input).read_text(buf, &mut position, false) $(.$await)? {
ReadTextResult::UpToMarkup(bytes) => assert_eq!(bytes, "a"),
x => panic!("Expected `UpToMarkup(_)`, but got `{:?}`", x),
}
Expand All @@ -1702,7 +1719,7 @@ mod test {
let mut input = b"a&".as_ref();
// ^= 2

match $source(&mut input).read_text(buf, &mut position) $(.$await)? {
match $source(&mut input).read_text(buf, &mut position, false) $(.$await)? {
ReadTextResult::UpToRef(bytes) => assert_eq!(bytes, "a"),
x => panic!("Expected `UpToRef(_)`, but got `{:?}`", x),
}
Expand All @@ -1716,7 +1733,7 @@ mod test {
let mut input = b"a".as_ref();
// ^= 2

match $source(&mut input).read_text(buf, &mut position) $(.$await)? {
match $source(&mut input).read_text(buf, &mut position, false) $(.$await)? {
ReadTextResult::UpToEof(bytes) => assert_eq!(bytes, "a"),
x => panic!("Expected `UpToEof(_)`, but got `{:?}`", x),
}
Expand Down Expand Up @@ -2060,7 +2077,7 @@ mod test {

/// Ensures, that no empty `Text` events are generated
mod $read_event {
use crate::events::{BytesCData, BytesDecl, BytesEnd, BytesPI, BytesStart, BytesText, Event};
use crate::events::{BytesCData, BytesDecl, BytesEnd, BytesPI, BytesRef, BytesStart, BytesText, Event};
use crate::reader::Reader;
use pretty_assertions::assert_eq;

Expand Down Expand Up @@ -2150,6 +2167,55 @@ mod test {
);
}

#[$test]
$($async)? fn trim_text_end_skips_empty_text_before_markup() {
let mut reader = Reader::from_str(" <tag/>");
reader.config_mut().trim_text_end = true;

assert_eq!(
reader.$read_event($buf) $(.$await)? .unwrap(),
Event::Empty(BytesStart::new("tag"))
);
assert_eq!(
reader.$read_event($buf) $(.$await)? .unwrap(),
Event::Eof
);
}

#[$test]
$($async)? fn trim_text_end_skips_empty_text_before_reference() {
let mut reader = Reader::from_str(" &amp;");
reader.config_mut().trim_text_end = true;

assert_eq!(
reader.$read_event($buf) $(.$await)? .unwrap(),
Event::GeneralRef(BytesRef::new("amp"))
);
assert_eq!(
reader.$read_event($buf) $(.$await)? .unwrap(),
Event::Eof
);
}

#[$test]
$($async)? fn trim_text_end_preserves_non_empty_text() {
let mut reader = Reader::from_str(" text <tag/>");
reader.config_mut().trim_text_end = true;

assert_eq!(
reader.$read_event($buf) $(.$await)? .unwrap(),
Event::Text(BytesText::new(" text"))
);
assert_eq!(
reader.$read_event($buf) $(.$await)? .unwrap(),
Event::Empty(BytesStart::new("tag"))
);
assert_eq!(
reader.$read_event($buf) $(.$await)? .unwrap(),
Event::Eof
);
}

#[$test]
$($async)? fn empty() {
let mut reader = Reader::from_str("<tag/>");
Expand Down
13 changes: 12 additions & 1 deletion src/reader/slice_reader.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -268,7 +268,12 @@ impl<'a> XmlSource<'a, ()> for &'a [u8] {
}

#[inline]
fn read_text(&mut self, _buf: (), position: &mut u64) -> ReadTextResult<'a, ()> {
fn read_text(
&mut self,
_buf: (),
position: &mut u64,
trim_text_end: bool,
) -> ReadTextResult<'a, ()> {
// Search for start of markup or an entity or character reference
match memchr::memchr2(b'<', b'&', self) {
Some(0) if self[0] == b'<' => ReadTextResult::Markup(()),
Expand All @@ -280,6 +285,9 @@ impl<'a> XmlSource<'a, ()> for &'a [u8] {
let (bytes, rest) = self.split_at(i);
*self = rest;
*position += i as u64;
if trim_text_end && bytes.iter().all(|&b| is_whitespace(b)) {
return ReadTextResult::Markup(());
}
match std::str::from_utf8(bytes) {
Ok(s) => ReadTextResult::UpToMarkup(s),
Err(e) => ReadTextResult::Err(e.into()),
Expand All @@ -289,6 +297,9 @@ impl<'a> XmlSource<'a, ()> for &'a [u8] {
let (bytes, rest) = self.split_at(i);
*self = rest;
*position += i as u64;
if trim_text_end && bytes.iter().all(|&b| is_whitespace(b)) {
return ReadTextResult::Ref(());
}
match std::str::from_utf8(bytes) {
Ok(s) => ReadTextResult::UpToRef(s),
Err(e) => ReadTextResult::Err(e.into()),
Expand Down
32 changes: 30 additions & 2 deletions tests/reader-config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -563,6 +563,12 @@ mod trim_markup_names_in_closing_tags {
}
}

// NOTE: These tests currently do NOT apply XML end-of-line normalization (XML 1.0 Section 2.11).
// Per the spec, `\r\n` must be normalized to `\n` before any other processing, which
// means every `\r\n` in this constant should become `\n` in the parsed output. That
// normalization applies universally: text content, element/attribute whitespace, comments,
// PIs, CDATA, and DOCTYPE. All assertions below that expect `\r\n` in their output are
// therefore incorrect with respect to a spec-compliant parser.
const XML: &str = " \t\r\n\
<!DOCTYPE root \t\r\n> \t\r\n\
<root \t\r\n> \t\r\n\
Expand Down Expand Up @@ -819,6 +825,30 @@ mod trim_text_end {
use super::*;
use pretty_assertions::assert_eq;

/// Whitespace-only text before `<` should be suppressed (issue #984)
#[test]
fn skips_whitespace_only_text_before_markup() {
let mut reader = Reader::from_str(" <a/>");
reader.config_mut().trim_text_end = true;

assert_eq!(
reader.read_event().unwrap(),
Event::Empty(BytesStart::new("a"))
);
assert_eq!(reader.read_event().unwrap(), Event::Eof);

let mut reader = Reader::from_reader(" <a/>".as_bytes());
reader.config_mut().trim_text_end = true;
let mut buffer = Vec::new();

assert_eq!(
reader.read_event_into(&mut buffer).unwrap(),
Event::Empty(BytesStart::new("a"))
);
buffer.clear();
assert_eq!(reader.read_event_into(&mut buffer).unwrap(), Event::Eof);
}

#[test]
fn false_() {
let mut reader = Reader::from_str(XML);
Expand Down Expand Up @@ -895,9 +925,7 @@ mod trim_text_end {
assert_eq!(reader.read_event().unwrap(), Event::Eof);
}

// TODO: Enable test after rewriting parser
#[test]
#[ignore = "currently it is hard to fix incorrect behavior, but this will much easy after parser rewrite"]
fn true_() {
let mut reader = Reader::from_str(XML);
reader.config_mut().trim_text_end = true;
Expand Down
Loading