Skip to content

Add spec for ruby2_keywords hash passed directly as positional argument#1380

Open
jessecai33 wants to merge 2 commits into
ruby:masterfrom
jessecai33:ruby2-keywords-positional-arg
Open

Add spec for ruby2_keywords hash passed directly as positional argument#1380
jessecai33 wants to merge 2 commits into
ruby:masterfrom
jessecai33:ruby2-keywords-positional-arg

Conversation

@jessecai33

Copy link
Copy Markdown

Adds a spec asserting that a Hash carrying the ruby2_keywords flag is not copied or unmarked when passed directly as an ordinary positional argument (i.e. not through a splat). It complements the existing "makes a copy and unmark the Hash when calling a method taking (arg)/(**kw)/(*args)" specs, which all cover the splat path — the direct positional path was previously unspecified.

Motivation

JRuby 9.4.15.0 shipped a regression (fixed in jruby/jruby#9517) where JIT-compiled code replaced a flagged hash with an unflagged copy on exactly this path. Because the behavior wasn't covered by ruby/spec, the regression reached a release and silently corrupted Rails ActiveJob argument serialization (ActiveJob::Arguments.serialize checks Hash.ruby2_keywords_hash? on a hash received as a positional parameter). The JRuby maintainers asked for the regression spec to be ported here.

The spec calls the target method repeatedly so implementations that compile methods lazily also exercise their compiled path — the JRuby regression only manifested in JIT-compiled code.

Verification

🤖 Generated with Claude Code

A Hash carrying the ruby2_keywords flag must not be copied or unmarked
when passed as an ordinary positional argument (not through a splat).
Complements the existing "makes a copy and unmark" specs, which cover
the splat cases.

Guards against the regression fixed in jruby/jruby#9517, where
JIT-compiled code replaced such a hash with an unflagged copy,
corrupting Rails ActiveJob argument serialization.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

@eregon eregon left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks it looks good, could you address those two things?

Comment thread core/module/ruby2_keywords_spec.rb Outdated
Comment on lines +137 to +141
obj.singleton_class.class_exec do
def single(arg)
arg
end
end

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
obj.singleton_class.class_exec do
def single(arg)
arg
end
end
def obj.single(arg)
arg
end

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Applied in 132e4e1.

Comment thread core/module/ruby2_keywords_spec.rb Outdated
Comment on lines +147 to +149
# Call repeatedly so implementations that compile methods lazily also
# exercise their compiled path (regression: jruby/jruby#9517)
100.times do

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

While I understand this makes it easier to catch the bug if it's in the JIT, one could argue many specs could do this and the test suite would take a while even if run in interpreter.
Since MSpec supports -R <repeats> I think that's better, it's already possible to run each example N times and that way to ensure it's JIT-compiled (possibly lowering the JIT threshold to compile faster).
That's notably what ZJIT does and it has proven to work well.

So could you remove the 100.times do?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Removed in 132e4e1. Verified the spec still catches the JRuby 9.4.15.0 regression without the loop: it fails there under -Xjit.threshold=0 / -X+C, and also on default settings with mspec -R 100. A plain single run on 9.4.15.0 passes since the interpreter path was not affected — relying on -R for JIT coverage as you describe seems right.

Review feedback: define obj.single with def obj.single instead of
singleton_class.class_exec, and rely on mspec -R rather than an
in-spec loop to exercise JIT-compiled paths.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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