salsify / goldiloader

Just the right amount of Rails eager loading
MIT License
1.61k stars 53 forks source link

gemspec claims compatibility with Ruby 3, but uses Ruby 2 argument forwarding #157

Closed kyrofa closed 2 months ago

kyrofa commented 2 months ago

I got some inexplicable "ArgumentError: wrong number of arguments (given 1, expected 0)" errors against Rails edge today. I traced it back to some Ruby 2-era argument forwarding in goldiloader. Turns out there's a bit of that in the ActiveRecord patches that needs to be updated to be compatible with Ruby 3.

To make the issue clear, take this simple example:

class Foo
  def initialize(*args, **kwargs)
    puts "ARGS: #{args}, KWARGS: #{kwargs}"
  end
end

class Bar < Foo
  def initialize(*args)
    super
  end
end

puts 'Foo:'
Foo.new('positional', keyword: 'argument')

puts 'Bar:'
Bar.new('positional', keyword: 'argument')

This runs fine on ruby 2.5:

$ rvm-exec 2.5.8 ruby example.rb
Foo:
ARGS: ["positional"], KWARGS: {:keyword=>"argument"}
Bar:
ARGS: ["positional"], KWARGS: {:keyword=>"argument"}

Also fine on 2.7:

$ rvm-exec 2.7.5 ruby example.rb
Foo:
ARGS: ["positional"], KWARGS: {:keyword=>"argument"}
Bar:
ARGS: ["positional"], KWARGS: {:keyword=>"argument"}

However, it does NOT work on 3.0:

$ rvm-exec 3.0.4 ruby example.rb
Foo:
ARGS: ["positional"], KWARGS: {:keyword=>"argument"}
Bar:
ARGS: ["positional", {:keyword=>"argument"}], KWARGS: {}

As you can see, that ends up passing kwargs as a positional arg. In my case, since upstream find_target only accepts kwargs, it explodes.

In Ruby 3 we can use a combination of ... and explicit *args, **kwargs.