Merge pull request #10205 from dvandersluis/issue/10202

[Fix #10202] Add new `Lint/Ruby2Keywords` cop

changelog/new_add_new_lintruby2keywords_cop.md

@@ -0,0 +1 @@
+* [#10202](https://github.com/rubocop/rubocop/issues/10202): Add new `Lint/UselessRuby2Keywords` cop. ([@dvandersluis][])

config/default.yml

@@ -2308,6 +2308,11 @@ Lint/UselessMethodDefinition:
   Safe: false
   AllowComments: true
 
+Lint/UselessRuby2Keywords:
+  Description: 'Finds unnecessary uses of `ruby2_keywords`.'
+  Enabled: pending
+  VersionAdded: '<<next>>'
+
 Lint/UselessSetterCall:
   Description: 'Checks for useless setter call to a local variable.'
   Enabled: true

lib/rubocop.rb

@@ -382,6 +382,7 @@
 require_relative 'rubocop/cop/lint/useless_assignment'
 require_relative 'rubocop/cop/lint/useless_else_without_rescue'
 require_relative 'rubocop/cop/lint/useless_method_definition'
+require_relative 'rubocop/cop/lint/useless_ruby2_keywords'
 require_relative 'rubocop/cop/lint/useless_setter_call'
 require_relative 'rubocop/cop/lint/useless_times'
 require_relative 'rubocop/cop/lint/void'

lib/rubocop/cop/lint/useless_ruby2_keywords.rb

@@ -0,0 +1,117 @@
+# frozen_string_literal: true
+
+module RuboCop
+  module Cop
+    module Lint
+      # This cop looks for `ruby2_keywords` calls for methods that do not need it.
+      #
+      # `ruby2_keywords` should only be called on methods that accept an argument splat
+      # (`*args`) but do not explicit keyword arguments (`k:` or `k: true`) or
+      # a keyword splat (`**kwargs`).
+      #
+      # @example
+      #   # good (splat argument without keyword arguments)
+      #   ruby2_keywords def foo(*args); end
+      #
+      #   # bad (no arguments)
+      #   ruby2_keywords def foo; end
+      #
+      #   # good
+      #   def foo; end
+      #
+      #   # bad (positional argument)
+      #   ruby2_keywords def foo(arg); end
+      #
+      #   # good
+      #   def foo(arg); end
+      #
+      #   # bad (double splatted argument)
+      #   ruby2_keywords def foo(**args); end
+      #
+      #   # good
+      #   def foo(**args); end
+      #
+      #   # bad (keyword arguments)
+      #   ruby2_keywords def foo(i:, j:); end
+      #
+      #   # good
+      #   def foo(i:, j:); end
+      #
+      #   # bad (splat argument with keyword arguments)
+      #   ruby2_keywords def foo(*args, i:, j:); end
+      #
+      #   # good
+      #   def foo(*args, i:, j:); end
+      #
+      #   # bad (splat argument with double splat)
+      #   ruby2_keywords def foo(*args, **kwargs); end
+      #
+      #   # good
+      #   def foo(*args, **kwargs); end
+      #
+      #   # bad (ruby2_keywords given a symbol)
+      #   def foo; end
+      #   ruby2_keywords :foo
+      #
+      #   # good
+      #   def foo; end
+      #
+      #   # bad (ruby2_keywords with dynamic method)
+      #   define_method(:foo) { |arg| }
+      #   ruby2_keywords :foo
+      #
+      #   # good
+      #   define_method(:foo) { |arg| }
+      #
+      class UselessRuby2Keywords < Base
+        MSG = '`ruby2_keywords` is unnecessary for method `%<method_name>s`.'
+        RESTRICT_ON_SEND = %i[ruby2_keywords].freeze
+
+        # Looks for statically or dynamically defined methods with a given name
+        # @!method method_definition(node, method_name)
+        def_node_matcher :method_definition, <<~PATTERN
+          {
+            (def %1 ...)
+            ({block numblock} (send _ :define_method (sym %1)) ...)
+          }
+        PATTERN
+
+        def on_send(node)
+          if node.first_argument.def_type?
+            inspect_def(node, node.first_argument)
+          elsif node.first_argument.sym_type?
+            inspect_sym(node, node.first_argument)
+          end
+        end
+
+        private
+
+        def inspect_def(node, def_node)
+          return if allowed_arguments(def_node.arguments)
+
+          add_offense(node.loc.selector, message: format(MSG, method_name: def_node.method_name))
+        end
+
+        def inspect_sym(node, sym_node)
+          return unless node.parent
+
+          method_name = sym_node.value
+          definition = node.parent.each_child_node.detect { |n| method_definition(n, method_name) }
+
+          return unless definition
+          return if allowed_arguments(definition.arguments)
+
+          add_offense(node, message: format(MSG, method_name: method_name))
+        end
+
+        # `ruby2_keywords` is only allowed if there's a `restarg` and no keyword arguments
+        def allowed_arguments(arguments)
+          return false if arguments.empty?
+
+          arguments.each_child_node(:restarg).any? &&
+            arguments.each_child_node(:kwarg, :kwoptarg, :kwrestarg).none?
+        end
+      end
+    end
+  end
+end

spec/rubocop/cop/lint/useless_ruby2_keywords_spec.rb

@@ -0,0 +1,167 @@
+# frozen_string_literal: true
+
+RSpec.describe RuboCop::Cop::Lint::UselessRuby2Keywords, :config do
+  context 'when `ruby2_keywords` is given a `def` node' do
+    it 'registers an offense for a method without arguments' do
+      expect_offense(<<~RUBY)
+        ruby2_keywords def foo
+        ^^^^^^^^^^^^^^ `ruby2_keywords` is unnecessary for method `foo`.
+        end
+      RUBY
+    end
+
+    it 'registers an offense for a method with only positional args' do
+      expect_offense(<<~RUBY)
+        ruby2_keywords def foo(arg)
+        ^^^^^^^^^^^^^^ `ruby2_keywords` is unnecessary for method `foo`.
+        end
+      RUBY
+    end
+
+    it 'registers an offense for a method with only `kwrestarg`' do
+      expect_offense(<<~RUBY)
+        ruby2_keywords def foo(**kwargs)
+        ^^^^^^^^^^^^^^ `ruby2_keywords` is unnecessary for method `foo`.
+        end
+      RUBY
+    end
+
+    it 'registers an offense for a method with only keyword args' do
+      expect_offense(<<~RUBY)
+        ruby2_keywords def foo(i:, j:)
+        ^^^^^^^^^^^^^^ `ruby2_keywords` is unnecessary for method `foo`.
+        end
+      RUBY
+    end
+
+    it 'registers an offense for a method with a `restarg` and keyword args' do
+      expect_offense(<<~RUBY)
+        ruby2_keywords def foo(*args, i:, j:)
+        ^^^^^^^^^^^^^^ `ruby2_keywords` is unnecessary for method `foo`.
+        end
+      RUBY
+    end
+
+    it 'registers an offense for a method with a `restarg` and `kwoptarg`' do
+      expect_offense(<<~RUBY)
+        ruby2_keywords def foo(*args, i: 1)
+        ^^^^^^^^^^^^^^ `ruby2_keywords` is unnecessary for method `foo`.
+        end
+      RUBY
+    end
+
+    it 'registers an offense for a method with a `restarg` and `kwrestarg`' do
+      expect_offense(<<~RUBY)
+        ruby2_keywords def foo(*args, **kwargs)
+        ^^^^^^^^^^^^^^ `ruby2_keywords` is unnecessary for method `foo`.
+        end
+      RUBY
+    end
+
+    it 'does not register an offense for a method with a `restarg` and no `kwrestarg`' do
+      expect_no_offenses(<<~RUBY)
+        ruby2_keywords def foo(*args)
+        end
+      RUBY
+    end
+
+    it 'does not register an offense for a method with a `restarg` other positional args' do
+      expect_no_offenses(<<~RUBY)
+        ruby2_keywords def foo(arg1, arg2, *rest)
+        end
+      RUBY
+    end
+
+    it 'does not register an offense for a method with a `restarg` other optional args' do
+      expect_no_offenses(<<~RUBY)
+        ruby2_keywords def foo(arg1 = 5, *rest)
+        end
+      RUBY
+    end
+
+    it 'does not register an offense for a method with a `restarg` and `blockarg`' do
+      expect_no_offenses(<<~RUBY)
+        ruby2_keywords def foo(*rest, &block)
+        end
+      RUBY
+    end
+  end
+
+  context 'when `ruby2_keywords` is given a symbol' do
+    it 'registers an offense for an unnecessary `ruby2_keywords`' do
+      expect_offense(<<~RUBY)
+        def foo(**kwargs)
+        end
+        ruby2_keywords :foo
+        ^^^^^^^^^^^^^^^^^^^ `ruby2_keywords` is unnecessary for method `foo`.
+      RUBY
+    end
+
+    it 'does not register an offense for an allowed def' do
+      expect_no_offenses(<<~RUBY)
+        def foo(*args)
+        end
+        ruby2_keywords :foo
+      RUBY
+    end
+
+    it 'does not register an offense when there is no `def`' do
+      expect_no_offenses(<<~RUBY)
+        ruby2_keywords :foo
+      RUBY
+    end
+
+    it 'does not register an offense when the `def` is at a different depth' do
+      expect_no_offenses(<<~RUBY)
+        class C
+          class D
+            def foo(**kwargs)
+            end
+          end
+
+          ruby2_keywords :foo
+        end
+      RUBY
+    end
+  end
+
+  context 'with a dynamically defined method' do
+    it 'registers an offense for an unnecessary `ruby2_keywords`' do
+      expect_offense(<<~RUBY)
+        define_method(:foo) { |**kwargs| }
+        ruby2_keywords :foo
+        ^^^^^^^^^^^^^^^^^^^ `ruby2_keywords` is unnecessary for method `foo`.
+      RUBY
+    end
+
+    it 'does not register an offense for an allowed `ruby2_keywords`' do
+      expect_no_offenses(<<~RUBY)
+        define_method(:foo) { |*args| }
+        ruby2_keywords :foo
+      RUBY
+    end
+
+    it 'registers an offense when the method has a `shadowarg`' do
+      expect_offense(<<~RUBY)
+        define_method(:foo) { |x; y| }
+        ruby2_keywords :foo
+        ^^^^^^^^^^^^^^^^^^^ `ruby2_keywords` is unnecessary for method `foo`.
+      RUBY
+    end
+
+    it 'does not register an offense when the method has a `restarg` and a `shadowarg`' do
+      expect_no_offenses(<<~RUBY)
+        define_method(:foo) { |*args; y| }
+        ruby2_keywords :foo
+      RUBY
+    end
+
+    it 'registers an offense for a numblock', :ruby27 do
+      expect_offense(<<~RUBY)
+        define_method(:foo) { _1 }
+        ruby2_keywords :foo
+        ^^^^^^^^^^^^^^^^^^^ `ruby2_keywords` is unnecessary for method `foo`.
+      RUBY
+    end
+  end
+end