Add ruby/spec coverage for the code range of interpolated strings - #9596
Add ruby/spec coverage for the code range of interpolated strings#9596jwils wants to merge 1 commit into
Conversation
jrubyGH-9591 folded a compound string into one literal with the wrong code range. An ASCII-only result was marked as not 7-bit, so String#hash disagreed with String#eql? and uniq split byte-identical strings. Add specs for the plain-Ruby behaviour: - String#ascii_only? is true when a non-String object is interpolated into an ASCII-only string. - String#ascii_only? stays false for real non-ASCII text. - String#hash agrees with String#eql? for byte-identical strings. - A squiggly heredoc with ASCII-only content is ASCII-only. These specs do not replace test/jruby/test_compound_string_coderange.rb. The bug only appears in compiled code. A spec body runs one time, so it stays interpreted under spec:ruby:fast and spec:ruby:fast:jit. Only jruby.compile.mode=FORCE shows the bug, and no CI target runs ruby/spec in that mode. The test/jruby test forces the compiler in a child process, so it is the guard against a regression.
|
A clarification: the That is probably sufficient for testing the fixed behavior, but I wanted to point out that there's no way to access the raw code range directly despite what this PR claims. |
|
Thanks for the correction. Do you even want this PR? Since the bug only appears in compiled code these tests alone don't reproduce anything. Running the test with |
|
Yes, I think this spec is just fine and the other internal test can be removed since it doesn't actually check the full code range either. |
|
Just to make sure I'm not missing something, my concern is the other test would catch this regressing because it sets sub processes to force compile https://github.com/jruby/jruby/pull/9592/changes#diff-31d52ab943ab646d93c49be70e240a548c99fd4b3c4cbad1d08c36f041292993R18. Since this one doesn't by default I don't think it would detect a regression. If still your preference though I can update to remove the other tests. |
|
Your agent is basically wrong here. FORCE is only one of the ways we trigger the jit. We have CI runs that set the jit threshold to zero and additional spec runs that execute the specs repeatedly to ensure the jit has finished. For a bonus you could ask your agent why it believes that mode is necessary and perhaps there's some old documentation it is picking up. One thing I did realize though: these specs should go under spec/ruby/language since they are directly related to the interpretation and compilation of Ruby code, and not really a test of String behavior. |
|
For reference, this is the Lines 39 to 45 in 0e52f72 |
The agent is definitely helpful to me here as there's no way I would have had the time or ability to track down our flaky test, but that question is driven by me (and I guess my agent isn't able to answer it). My goal is to be able to run the test suite and have it fail before the PR and I haven't figured out how to do that. The agent has all kinds of ideas like running in a loop N times, but none of those satisfy me. I couldn't get |
|
Let me know what doesn't work. If your environment is bootstrapped according to the root docs, it should function ok: You can also run the specs directly, with jit threshold zero, using something like this command line: |
|
I must also mention...in case we haven't discussed this already... If you are running JRuby in a production setting, I'd love to partner with you to support that use. Fully 100% of my development time is funded through support arrangements from https://headius.com/services. The Expert level means I'm virtually on your team, answering questions and ensure bugs get fixed and released, but if every JRuby user in the world signed up for the basic Professional level, we'd be fully funded. Partnering in this way also helps JRuby improve based on your needs. I can't guess what to prioritize for real-world JRuby apps unless I know about those apps. It's a win for both of us. Please consider partnering with us to keep JRuby healthy and moving forward! |
It doesn't seem to work. Let me know if I am totally missing something here. I believe jwils@6449e75 fixes it, but surfaces some other failed tests. Let me know if you think that fix looks correct. I can attempt to make that first and see what other work is needed to get the test suite to pass.
I wasn't aware of this, our JRuby usage is limited, but we've recently added JRuby to elasticgraph (one of our opensource projects). I'll look into support partnerships internally |
|
The agent may be right about Honestly I'm unsure if this is wrong. If you have a At the very least we do have jobs eagerly running the JIT, which is the key requirement for this PR. |
|
FWIW it does look like the other suites avoid clobbering JRUBY_OPTS if they need to add something: Lines 108 to 124 in fba7771 I'm going to contemplate whether there might be a cleaner way to allow both runtime configuration from the |
|
I've opened #9597 to track improvements to how we tweak the runtime configuration for rake targets. |
Follow-up to #9592, per the review note about preferring these tests in the RubySpec suite.
The code range is observable from pure Ruby, so specs are possible after all:
String#ascii_only?reads the code range directly.String#hashdisagreeing withString#eql?is the user-visible symptom from Compiler gives a constant-folded ASCII String the code range CR_VALID, which breaks theeql?/hashcontract #9591.Both are standard methods, and the new specs pass on CRuby, so they are upstreamable to ruby/spec.
What this adds
core/string/ascii_only_spec.rb: interpolation of a non-String object into an ASCII-only string keeps the string ASCII-only. A second spec checks that real non-ASCII text stays non-ASCII, to guard the opposite error.core/string/hash_spec.rb:hashagrees witheql?for byte-identical strings, anduniqdoes not split them.language/heredoc_spec.rb: an ASCII-only squiggly heredoc is ASCII-only.This does not replace the test/jruby test
test/jruby/test_compound_string_coderange.rbmust stay. The bug only appears in compiled code, and a spec body runs one time, so it stays interpreted. I checked this against a build without the #9592 fix:compile.mode=JIT,jit.threshold=0spec:ruby:fast:jitcompile.mode=FORCENo CI target runs ruby/spec under FORCE.
spec:precompiled(spec:ruby:aot) is the only FORCE rubyspec task, and it is not in theci.ymlmatrix. The matrix hastest:jruby:aot, which runs thetest/jrubysuite.The
test/jrubytest forces the compiler in a child process, so it stays the guard against a regression. These specs add cross-implementation coverage of the behaviour.Two of the four specs detect the bug under FORCE: the
String#hashspec and theascii_only?interpolation spec. The heredoc spec passes on a broken build because the fold does not happen inside a spec block, so treat it as documentation of the expected result.Test
Built the branch and ran the three spec files in both JIT and FORCE mode, against a fixed and an unfixed build. On the fixed build the only failures are the two entries already listed in
spec/tags/ruby/language/heredoc_tags.txt.