Skip to content

Fix indentation when the indent character is not ASCII - #1017

Open
youdie006 wants to merge 1 commit into
tafia:masterfrom
youdie006:fix-multi-byte-indent-char
Open

youdie006 wants to merge 1 commit into
tafia:masterfrom
youdie006:fix-multi-byte-indent-char

Conversation

@youdie006

Copy link
Copy Markdown

The bug

Indentation (src/writer.rs:670-737) keeps current_indent_len as a byte index into its indents
cache -- current() slices with it -- but grow and shrink moved it by indent_size, which is a count of
characters. The two agree only while the indent character is one byte long.

Writer::new_with_indent(inner, indent_char: u8, indent_size: usize) is public and takes a u8, and
char::from(u8) maps every byte above 0x7F to a character that is two bytes in UTF-8. So the mismatch is
reachable from safe code with a plain byte argument:

// 0xA0 -> U+00A0 NO-BREAK SPACE, two bytes in UTF-8
let mut w = Writer::new_with_indent(Vec::new(), 0xA0, 1);
w.write_event(Event::Start(BytesStart::new("a")))?;
w.write_event(Event::Start(BytesStart::new("b")))?;
thread 'main' panicked at src/writer.rs:710:22:
end byte index 1 is not a char boundary; it is inside '\u{a0}' (bytes 0..2 of string)

With an even indent_size it does not panic, it just indents half as far as asked:

indent_size = 2, expected "<a>\n\u{a0}\u{a0}<b>t</b>\n</a>"
                 actual   "<a>\n\u{a0}<b>t</b>\n</a>"

Separately, Serializer::indent (src/se/mod.rs:819) accepts a char and then throws most of it away:

self.ser.indent = Indent::Owned(Indentation::new(indent_char as u8, indent_size));

'\u{2003}' as u8 is 0x03. Serializing with indent('\u{2003}', 2) produces

"<Outer>\n\u{3}\u{3}<inner>\n\u{3}\u{3}\u{3}\u{3}<t>x</t>\n\u{3}\u{3}</inner>\n</Outer>"

U+0003 is not a legal XML character (XML 1.0 Char excludes C0 controls other than tab, LF and CR), so the
serializer returns Ok with a document that a conformant parser must reject. Any indent character above
U+007F -- an em space, an ideographic space -- goes the same way, and char is the only type the caller can
pass.

The fix

  • Add Indentation::with_char(char, usize); Indentation::new(u8, _) becomes a thin wrapper over it, so
    Writer::new_with_indent's public signature is unchanged.
  • Add level_len() = indent_size * indent_char.len_utf8(), and use it in grow and shrink so
    current_indent_len stays a byte length.
  • In additional(), convert the added level the same way. The value passed in is
    AttributeIndent::WriteConfigured(i.indent_size) (src/writer.rs:533), and the variant's own doc comment
    at src/writer.rs:444 says "Write specified count of indent characters" -- so the added amount is a
    character count and needs the same conversion, applied to the added level only, not to the running total.
  • Point Serializer::indent at with_char so the char it already accepts survives.

ASCII is untouched: len_utf8() is 1, and every arithmetic site reduces to what it was.

I kept new_with_indent's u8 rather than widening it to char, since that would be a breaking change for
every caller. Happy to switch it if you would rather fix the signature at the same time.

Verification

New module multi_byte_indent_char in tests/writer-indentation.rs: nested elements, attribute indent at
depth 0 and at depth 1, an odd indent_size (the case that panicked), an ASCII case pinning that nothing
moved, and the serializer case.

  • cargo test --all-features passes; so do --no-default-features, --features serialize,
    --features serialize,encoding and --features serialize,escape-html, and
    cargo test --all-features --benches --tests.
  • cargo fmt -- --check clean, cargo +1.86.0 check (the MSRV the workflow pins) clean.
  • Reverting the whole patch fails 5 of the new tests and no existing ones.
  • Two one-line mutations fail disjoint sets, so neither half of the change is dead weight and the
    boundary is pinned from both sides:
    • restoring only Indentation::new(indent_char as u8, ..) in Serializer::indent fails only
      serializer_indent_char_is_not_truncated;
    • over-correcting additional() to (current_indent_len + additional_indent) * len_utf8() fails only
      in_attributes_nested.

That second one is why in_attributes_nested exists. The depth-0 attribute test cannot see the difference --
current_indent_len is 0 there, so both forms give the same answer. It only shows up one level in.

AI disclosure

Written with AI assistance (Claude Code, claude-opus-5). Per the AI policy: I have reviewed the change, I can
explain it without going back to the agent, and I will answer review questions in my own words rather than
pasting model output.

Indentation tracks current_indent_len as a byte index into its indents
cache, but grew and shrank it by indent_size, a count of characters.
Writer::new_with_indent takes a u8 and char::from turns any byte above
0x7F into a two byte character, so the index landed inside a character
and the slice in current() panicked, or one level of indent came out
half as wide as asked for.

Serializer::indent takes a char and passed it on as `indent_char as u8`,
which truncated every non-ASCII character to a byte and emitted control
characters instead. Route it through Indentation::with_char so the
character survives.
@codecov-commenter

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' 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.10%. Comparing base (e00ae5c) to head (491ea01).
⚠️ 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    #1017      +/-   ##
==========================================
- Coverage   57.31%   55.10%   -2.22%     
==========================================
  Files          46       51       +5     
  Lines       18197    18822     +625     
==========================================
- Hits        10429    10371      -58     
- Misses       7768     8451     +683     
Flag Coverage Δ
unittests 55.10% <100.00%> (-2.22%) ⬇️

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.

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 this pull request may close these issues.

2 participants