Rubocop: Lint/SplatKeywordArguments false positives?

[jlfaber]

The new Lint/SplatKeywordArguments cop, added in 0.56.0 (#5871), warns in several cases that are seemingly outside the intent of the new Ruby 2.6.0 warning message it claims to emulate.

The pertinent test code in ruby core is located here:

https://github.com/ruby/ruby/blob/f28d6165f6e39c7723ad88bdf87708ddff5a145f/test/ruby/test_keyword.rb#L506

The only cases in which the new ruby 2.6 warning is expected are when a double-splat arg is passed to a method that has one or more positional args, but no double-splatted keyword arg.

Line 524, which calls a method defined with a single positional arg, and line 527, which calls a method defined with a single positional arg with a default value, are examples where the warning is expected.

Lines 530 and 533 which call methods defined with double-splatted and single-splatted arguments, respectively, are _not_ expected to generate the warning.

It's not entirely clear that rubocop is able to replicate this warning correctly since it would need to examine both the calling arguments (to look for double-splatted hashes) AND the definition of the method being called (to determine whether that hash would be assigned to a positional method argument).

---

Expected behavior

options = { opt1: 1, opt2: 2 }
def method1(kw_args); end

def method2(**kw_args); end

method1(**options) # This will warn in ruby 2.6 and currently warns in rubocop.

method2(**options) # This does not warn in ruby 2.6 and should not warn in rubocop.

Actual behavior

Call to method2 warns.

RuboCop version

$ rubocop -V
0.56.0 (using Parser 2.5.1.0, running on ruby 2.5.1 x86_64-darwin17)

[SpComb]

Here's a gist testing some different combinations of keyword argument splats. Rubocop 0.56 seems to warn about all possible usages of **options, whereas a recent ruby 2.6.0dev only warns about some very specific usages where the method is defined to take a hash as a positional argument instead of keyword arguments.

https://gist.github.com/SpComb/483b2e36ce80c52aa48f9e76cfe4f612

Based on my understanding of the breaking ruby 3 changes / upcoming ruby 2 warnings (per https://bugs.ruby-lang.org/issues/14183), I would consider the following usages in the above gist example to be invalid:

• foo('a', **options)
• foo('a', test: true)
• foo('a', **options, test: false)
• bar('b', options)
• asdf('c', options)

AFAIK the remaining usages should continue to be fine, because they are using explicit positional/keyword arguments, and are not mixing the two together?

[promisedlandt]

Really hurts with double splat + keyword arguments.

````
def example(version:, **content); end

content = { a: 1, b: 2 }

example(version: 1, **content) # works, but Lint/SplatKeywordArguments complains
example(content, version: 1) # ArgumentError: wrong number of arguments (given 1, expected 0)
````

[AlexWayfer]

def foo(arg, options = {})
end

def foo_file(name, **options)
  foo('file', name: name, **options)
end

Now I should use options.merge(name: name) for RuboCop 0.56. :fearful:

[SpComb]

Now I should use options.merge(name: name) for RuboCop 0.56. fearful

No, that specific example is actually covered by the impending ruby 3 changes, and will result in a warning on ruby 2.6:

irb(main):001:0> def foo(arg, options = {})
irb(main):002:1> end
=> :foo
irb(main):003:0> 
irb(main):004:0> def foo_file(name, **options)
irb(main):005:1>   foo('file', name: name, **options)
irb(main):006:1> end
=> :foo_file
irb(main):007:0> 
irb(main):008:0> foo_file('asdf', test: false)
(irb):5: warning: passing splat keyword arguments as a single Hash to `foo'
=> nil

So this is actually an example of a case where the rubocop warning would be legit, and the code needs fixing.

One fix is to use def foo(arg, **options) instead, which will not result in a warning on ruby 2.6. However, that usage will still (incorrectly) result in an rubocop warning...

[AlexWayfer]

No, that specific example is actually covered by the impending ruby 3 changes, and will result in a warning on ruby 2.6:

Yes, I understood it later, sorry.

One fix is to use def foo(arg, **options) instead, which will not result in a warning on ruby 2.6. However, that usage will still result in an incorrect rubocop warning...

Thank you.