Scan the code range when folding a compound string - #9592
Conversation
Sorry, something went wrong.
headius
left a comment
There was a problem hiding this comment.
The fix makes sense. Normally I'd also prefer that the tests go into the RubySpec suite (spec/ruby), but I don't believe there's a good way to observe the code range from pure-Ruby.
We can go with this as-is and leave the test/jruby test in place.
Sorry, something went wrong.
|
I approved this PR, but it probably should be applied to JRuby 10.0 instead of master (10.1) as that is the current LTS and this is clearly an observable behavior difference from standard Ruby. @jwils Could you rebase this fix atop the jruby-10.0 branch? |
Sorry, something went wrong.
BuildCompoundStringInstr#simplifyInstr folds a compound string into a single literal. It assigned CR_VALID without a scan of the bytes, so a result that held only ASCII was marked as not 7-bit. String#hash mixes the encoding into the digest when a string is not 7-bit. Two byte-identical strings then compared equal but hashed differently, so uniq, Hash and Set split them. The interpreter appends each piece with RubyString#cat and keeps the code range correct, so this only appeared once a method was compiled. Keep the code range of the operand that is folded, and use CR_7BIT for the empty string. Fixes jruby#9591
|
Thanks for the quick review. Rebased. |
Sorry, something went wrong.
headius
left a comment
There was a problem hiding this comment.
Approved again. Might revisit the tests later to see if they make sense for rubyspec.
Sorry, something went wrong.
Fixes #9591.
BuildCompoundStringInstr#simplifyInstr folds a compound string into a single
literal. Two places assigned CR_VALID without a scan of the bytes. A string
that holds only ASCII must be CR_7BIT.
String#hash mixes the encoding into the digest when a string is not 7-bit.
Two byte-identical strings with the same encoding therefore compared equal but
hashed differently, and Array#uniq, Hash and Set treated them as two
values.
The interpreter is not affected. interpret appends each piece with
RubyString#cat, which keeps the code range correct. The two modes disagreed,
so one process could hold both a correct and an incorrect copy of the same
text once the JIT threshold was crossed part way through a run.
The change
branch was already correct, because it reuses the operand and its code range.
asOperand in the same file already uses CR_UNKNOWN, which is safe, because
a later scan computes the true value.
Which expressions were affected
Code range of the result, with -Xcompile.mode=FORCE:
The dedent of a squiggly heredoc makes the parser produce a compound string,
which is why only the indented form was affected. An interpolated String was
not affected, because the parser merges those pieces earlier and rescans.
Tests
test/jruby/test_compound_string_coderange.rb is new. The bug only appears
once a method is compiled, so each case runs in a child process with
jruby.compile.mode=FORCE. Without this change 5 of the 6 tests fail. The
sixth checks that real non-ASCII text still gets CR_VALID, and it passes
either way.
I also ran spec/ruby/core/string, spec/ruby/core/symbol,
spec/ruby/core/encoding, spec/ruby/language/string_spec.rb and
spec/ruby/language/heredoc_spec.rb with -Xcompile.mode=FORCE. The result is
the same before and after: 4862 examples, 50 failures, 8 errors. Those failures
are already present on master.