Don't mutate the text node while pretty printing - #371
Open
youdie006 wants to merge 1 commit into
Open
Conversation
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.
There was a problem hiding this comment.
🟢 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_textfrom destructivegsub!/squeeze!to non-destructive whitespace normalization to avoid rewriting the document during formatting. - Add a new
test/formatter/test_pretty.rbsuite 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(" ") |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pretty#write_text(lib/rexml/formatters/pretty.rb:88-91) calls the destructivegsub!andsqueeze!on whatText#to_shands back, andText#to_sreturns the node's own string —@stringwhen@raw, otherwise the memoized@normalized:So pretty printing permanently rewrites the document it is printing. Through
Document#write, the documented pretty-print entry point:It is not only spaces. Measured the same way:
to_safterwrite(out, 2)on master<a><b>hello world</b></a><a><b>hello world</b></a><a><b>x & y</b></a><a><b>x & y</b></a><a><b>line1\nline2</b></a><a><b>line1 line2</b></a>With a
@rawtext node,Text#valueitself changes.The other four call sites
grep -n "node\.to_s" lib/rexml/formatters/*.rbgives five results. Four of them —transitive.rb:54,default.rb:94,default.rb:99,default.rb:105— areoutput << node.to_s, treating the return value as read-only.pretty.rbwas the only one writing to it.The change is to use the non-destructive
gsubandsqueeze. The rendered output is byte-for-byte the same.Testing
New
test/formatter/test_pretty.rb, following thetest/formatter/test_default.rbconvention, with four tests: two asserting the source is not modified, and two asserting the whitespace handling thatgsub/squeezeperform.master(<"hello world"> expected but was <"hello world">) and pass here.gsub!/squeeze!fails the two non-mutation tests; dropping.squeezefails onlytest_consecutive_spaces_are_squeezed; dropping.gsubfails onlytest_whitespace_is_replaced_with_space.Worth stating plainly: with
test/formatter/test_pretty.rbremoved, 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(whatrake testshells 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 rdocjob —rdocandbundleare 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::NAMESPLITuses^rather than\Aand has no trailing anchor, so"a:b:c"splits to prefixa, nameb;Default#write_element_attributessorts by name whilePrettyandTransitivedo 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
masterand against this branch.