Skip to content

Don't mutate the text node while pretty printing - #371

Open
youdie006 wants to merge 1 commit into
ruby:masterfrom
youdie006:pretty-write-text-non-destructive
Open

Don't mutate the text node while pretty printing#371
youdie006 wants to merge 1 commit into
ruby:masterfrom
youdie006:pretty-write-text-non-destructive

Conversation

@youdie006

Copy link
Copy Markdown

Pretty#write_text (lib/rexml/formatters/pretty.rb:88-91) calls the destructive gsub! and squeeze! on what Text#to_s hands back, and Text#to_s returns the node's own string — @string when @raw, otherwise the memoized @normalized:

def to_s
  return @string if @raw
  @normalized ||= Text::normalize( @string, doctype, @entity_filter )
end

So pretty printing permanently rewrites the document it is printing. Through Document#write, the documented pretty-print entry point:

d = REXML::Document.new("<a><b>hello   world</b></a>")
d.write(out, 2)
d.to_s
# before: "<a><b>hello world</b></a>"    <- the source had three spaces
# after:  "<a><b>hello   world</b></a>"

It is not only spaces. Measured the same way:

source to_s after write(out, 2) on master
<a><b>hello world</b></a> <a><b>hello world</b></a>
<a><b>x &amp; y</b></a> <a><b>x &amp; y</b></a>
<a><b>line1\nline2</b></a> <a><b>line1 line2</b></a>

With a @raw text node, Text#value itself changes.

The other four call sites

grep -n "node\.to_s" lib/rexml/formatters/*.rb gives five results. Four of them — transitive.rb:54, default.rb:94, default.rb:99, default.rb:105 — are output << node.to_s, treating the return value as read-only. pretty.rb was the only one writing to it.

The change is to use the non-destructive gsub and squeeze. The rendered output is byte-for-byte the same.

Testing

New test/formatter/test_pretty.rb, following the test/formatter/test_default.rb convention, with four tests: two asserting the source is not modified, and two asserting the whitespace handling that gsub/squeeze perform.

  • The two non-mutation tests fail on master (<"hello world"> expected but was <"hello world">) and pass here.
  • Mutation-checked in three directions, each failing a disjoint test: reverting to gsub!/squeeze! fails the two non-mutation tests; dropping .squeeze fails only test_consecutive_spaces_are_squeezed; dropping .gsub fails only test_whitespace_is_replaced_with_space.

Worth stating plainly: with test/formatter/test_pretty.rb removed, the pre-existing 820-test suite passes under all three of those variants — including the bug itself. There was no coverage of pretty-print whitespace handling at all, which is why the two output assertions are in the patch; without them a behaviour-preserving change here is unobservable.

  • ruby test/run.rb (what rake test shells out to): 824 tests, 2634 assertions, 0 failures, 0 errors.
  • RUBYOPT="--enable-frozen-string-literal" ruby test/run.rb, matching the second CI job: same result. This one matters here, since the patch stops writing into a string that may be frozen.

I did not run the rake warning:error rdoc job — rdoc and bundle are not available in my environment. The patch adds no method and no doc comment, so I do not expect it to affect that job, but I have not executed it. I also only ran Ruby 3.2.3 on Linux, not the full 2.6+/JRuby/macOS/Windows matrix.

Noticed but not touched

Namespace::NAMESPLIT uses ^ rather than \A and has no trailing anchor, so "a:b:c" splits to prefix a, name b; Default#write_element_attributes sorts by name while Pretty and Transitive do not. Neither is verified and neither is in this patch — mentioning them only in case they are of interest.


AI assistance disclosure: this patch was found and written with Claude Code. Every value above is verbatim from running it against master and against this branch.

Pretty#write_text called gsub! and squeeze! on the string returned by
Text#to_s, and Text#to_s returns the node's own string: @string when @raw,
otherwise the memoized @Normalized. So pretty printing rewrote the document
it was printing.

    d = REXML::Document.new("<a><b>hello   world</b></a>")
    d.write(out, 2)
    d.to_s  # => "<a><b>hello world</b></a>"

Use the non-destructive gsub and squeeze. The other four node.to_s call
sites in the formatters only read the value.
Copilot AI lite review requested due to automatic review settings September 9, 2026 00:51

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟢 Approval recommended

The change addresses a clear correctness issue and adds focused regression tests, with only a minor allocation/performance nit noted.

Pull request overview

This PR fixes REXML::Formatters::Pretty#write_text so that pretty-printing no longer mutates the underlying REXML::Text node content, and adds targeted tests to prevent regressions.

Changes:

  • Switch Pretty#write_text from destructive gsub!/squeeze! to non-destructive whitespace normalization to avoid rewriting the document during formatting.
  • Add a new test/formatter/test_pretty.rb suite that asserts both non-mutation and expected whitespace normalization output.
File summaries
File Description
lib/rexml/formatters/pretty.rb Stops pretty-printing from mutating Text#to_s-backed internal strings while preserving output.
test/formatter/test_pretty.rb Adds coverage for non-mutation and Pretty formatter whitespace behavior.
Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 1
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment on lines +89 to +90
# Not gsub!/squeeze!: Text#to_s returns the node's own string.
s = node.to_s().gsub(/\s/, ' ').squeeze(" ")
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