Fix kwargs passing on Ruby 3.0 for sucker_punch #1495
Conversation
lloeki
force-pushed
the
fix/1372-sucker_punch-kwargs-on-ruby3
branch
from
e517a24
to
4048849
Compare
Really? :o I hadn't spotted this, do you remember where you saw it originally? I'd be really curious to follow the discussion. |
Yep indeed, straight from the source :)
Sadly I can't remember where I saw the "might" becoming less evasive and a more definitive "shall", including mention of Ruby 3.2 (the one which will EOL 2.6). |
lloeki
force-pushed
the
fix/1372-sucker_punch-kwargs-on-ruby3
branch
2 times, most recently
from
1a69a0d
to
895b190
Compare
|
Silly me made a mistake on I dropped the complexity down by removing the I'm interested in feedback about which would be preferable (version cap or kwargs syntax behind a conditional). |
The kwargs separation change for Ruby 3.0 is tricky: there are different behaviours on 2.6, 2.7, and 3.0. While ruby2_keywords attempts to smooth that out, it is due to disappear on 3.2, therefore relying on it produces surefire broken code when 3.2 is released and 2.6 EOL'd. Thus the only future-proof and reliable syntax on Ruby >= 3 is to capture the arguments with *args, **kwargs and forward them the same way, without ruby2_keywords. Conversely, the only working syntax on Ruby < 3 is capturing both with *args and forwarding them the same way, using ruby2_keywords on 2.7 to restore the previous behaviour. Therefore either dynamic function definition per version is necessary because of the syntactic difference in argument passing and forwarding. An alternative is to cap the gem's maximum Ruby version.
Ruby 3.2 is scheduled to drop ruby2_kwargs, as it would EOL 2.6. At that point, code depending on *args and respond_to?(:ruby2_keywords) would silently revert behaviour to a non-3.x compatible one. Guarantee kwargs behaviour on 3.x can rely on ruby2_keywords being there. This simplifies the sucker_punch fix without version conditionals, and protects other uses of ruby2_keywords throughout the code base.
lloeki
force-pushed
the
fix/1372-sucker_punch-kwargs-on-ruby3
branch
from
d6788c3
to
30bae5e
Compare
Codecov Report
@@ Coverage Diff @@
## master #1495 +/- ##
=======================================
Coverage 98.23% 98.23%
=======================================
Files 863 863
Lines 41582 41616 +34
=======================================
+ Hits 40849 40883 +34
Misses 733 733
Continue to review full report at Codecov.
|
A maximum version was initially added in #1495 because we expected the `ruby2_keywords` method to be removed (see the PR for the discussion). Now Ruby 3.2.0-preview1 is out and `ruby2_keywords` are still there, and there's even a recent change for it in ruby/ruby#5684 that is documented as "ruby2_keywords needed in 3.2+". So for now let's bump the maximum version to < 3.3 to allow the Ruby 3.2 series to be supported and we can keep an eye on the Ruby 3.2 test releases to see if anything changes. (Otherwise, once Ruby 3.2.0 stable is out, we should probably bump this to 3.4, and so on...)
A maximum version was initially added in #1495 because we expected the `ruby2_keywords` method to be removed (see the PR for the discussion). Now Ruby 3.2.0-preview1 is out and `ruby2_keywords` are still there, and there's even a recent change for it in ruby/ruby#5684 that is documented as "ruby2_keywords needed in 3.2+". So for now let's bump the maximum version to < 3.3 to allow the Ruby 3.2 series to be supported and we can keep an eye on the Ruby 3.2 test releases to see if anything changes. (Otherwise, once Ruby 3.2.0 stable is out, we should probably bump this to 3.4, and so on...)
The kwargs separation change for Ruby 3.0 is tricky: there are different behaviours on 2.6, 2.7, and 3.0. While
ruby2_keywordsattempts to smooth that out, it is due to disappear on 3.2, therefore relying on it produces surefire broken code when 3.2 is released and 2.6 EOL'd.ruby2_keywordsalso suffers from subtle brokenness by virtue of leveraging an internal stateful flag attached to the hash. This is generally OK for very local uses but gets troublesome with complex delegations and even more for instrumentors. Case in point, attempting to useruby2_keywordshere works forperform_inandperform_async, but does not solve the issue in the__run_performdirect call case.Thus the only future-proof and reliable syntax on Ruby >= 3 is to capture the arguments with
*args, **kwargsand forward them the same way, withoutruby2_keywords.Conversely, the only working syntax on Ruby < 3 is capturing both with
*argsand forwarding them the same way, usingruby2_keywordson 2.7 to restore the previous behaviour.Therefore dynamic function definition per version is sadly necessary because of the syntactic difference in argument passing and forwarding.
Fixes #1372