oracle / truffleruby

A high performance implementation of the Ruby programming language, built on GraalVM.
https://www.graalvm.org/ruby/
Other
3.02k stars 185 forks source link

Empty splatted keyword arguments are not ignored #2541

Closed postmodern closed 2 years ago

postmodern commented 2 years ago

TruffleRuby attempts to support 2.7.4, but I noticed a difference in behavior with the splatting of keyword arguments when calling super. It appears that splatting {} as keyword arguments to a super method, TruffleRuby treats the empty Hash as an actual argument, instead of omitting it.

Examples

class Base
end

module Mixin
  def initialize(var: 1, **kwargs)
    @var = var

    puts "kwargs: #{kwargs.inspect}"
    super(**kwargs)
  end
end

class Subclass < Base
  include Mixin
end

p Subclass.new(var: 2)

ruby 2.7.4

$ ruby test.rb 
kwargs: {}
#<Subclass:0x000056066ad74198 @var=2>

truffleruby 21.3.0

$ ruby test.rb 
kwargs: {}
test.rb:9:in `initialize': wrong number of arguments (given 1, expected 0) (ArgumentError)
    from test.rb:9:in `initialize'
    from test.rb:17:in `<main>'

Version Information

eregon commented 2 years ago

This is an expected difference, such code is anyway broken on 2.6 and on 2.7 (https://eregon.me/blog/2021/02/13/correct-delegation-in-ruby-2-27-3.html, https://github.com/oracle/truffleruby/issues/2260). Delegating keywords via **kwargs is only correct on Ruby 3+.

eregon commented 2 years ago

@postmodern Could you fix the issue in the source instead? As I said above the same happens on CRuby 2.6:

kwargs: {}
Traceback (most recent call last):
    3: from -:19:in `<main>'
    2: from -:19:in `new'
    1: from -:11:in `initialize'
-:11:in `initialize': wrong number of arguments (given 1, expected 0) (ArgumentError)

It only works on 2.7 by luck, no Ruby code should rely on that, unless it's marked as requiring Ruby 3.0+.

postmodern commented 2 years ago

The above example code also fails on truffleruby 21.3.0, which claims to support ruby 2.7.4. CRuby 2.7.x however does support this behavior. Once truffleruby gains ruby 3.0 support I can ignore this bug and move forward with truffleruby support.

eregon commented 2 years ago

@postmodern It breaks on CRuby 2.7 if e.g., the super initialize would use delegation using ruby2_keywords:

class Base
  ruby2_keywords def initialize(*args)
    # delegate(*args)
    p args
  end
end

module Mixin
  def initialize(var: 1, **kwargs)
    @var = var

    puts "kwargs: #{kwargs.inspect}"
    super(**kwargs)
  end
end

class Subclass < Base
  include Mixin
end

p Subclass.new(var: 2)

=>

kwargs: {}
[{}]
#<Subclass:0x0000000001e06db0 @var=2>

In Ruby < 3, you should only pass keywords to a method if you know if the method accepts keywords. If not you might pass an extra empty Hash and break things like here.

Concretely in this case, the easiest fix is kwargs.empty? ? super() : super(**kwargs). (or not take **kwargs)

postmodern commented 2 years ago

@eregon that is a different example than mine, which does not use ruby2_keywords modifier. My example works on Ruby 2.7 but does not work on TruffleRuby.

eregon commented 2 years ago

I just wanted to mention (with my extensive knowledge of kwargs semantics) that this is IMHO a bug in the user code because it'll only work on 2.7 in some specific cases, and it would always break in 2.6. It's made a bit less clear by the gem not supporting 2.6, otherwise it would be clearly a bug of the gem since it wouldn't work on CRuby 2.6.

So this difference is expected in TruffleRuby targetting 2.7. In fact this difference helped uncover several bugs in other projects which e.g. did delegation incorrectly in 2.7 (linked issues and more).

Anyway, this will get resolved when TruffleRuby implements the new Ruby 3 keyword arguments semantics, which are much better defined than the semantics in 2.7.

eregon commented 2 years ago

FYI, TruffleRuby now fully implements Ruby 3 keyword arguments (since b8086afe6ecc64a78e1f80f42d4ae9c05cf56605)