diff --git a/changelog/new_add_new_lintruby2keywords_cop.md b/changelog/new_add_new_lintruby2keywords_cop.md new file mode 100644 index 00000000000..4ceaf6ae2c2 --- /dev/null +++ b/changelog/new_add_new_lintruby2keywords_cop.md @@ -0,0 +1 @@ +* [#10202](https://github.com/rubocop/rubocop/issues/10202): Add new `Lint/UselessRuby2Keywords` cop. ([@dvandersluis][]) diff --git a/config/default.yml b/config/default.yml index d3de8ba3a7c..3b2d09403ac 100644 --- a/config/default.yml +++ b/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: '<>' + Lint/UselessSetterCall: Description: 'Checks for useless setter call to a local variable.' Enabled: true diff --git a/lib/rubocop.rb b/lib/rubocop.rb index dcd734cd258..54363c1a8f2 100644 --- a/lib/rubocop.rb +++ b/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' diff --git a/lib/rubocop/cop/lint/useless_ruby2_keywords.rb b/lib/rubocop/cop/lint/useless_ruby2_keywords.rb new file mode 100644 index 00000000000..3b6485d3a02 --- /dev/null +++ b/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 `%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 diff --git a/spec/rubocop/cop/lint/useless_ruby2_keywords_spec.rb b/spec/rubocop/cop/lint/useless_ruby2_keywords_spec.rb new file mode 100644 index 00000000000..536564b532e --- /dev/null +++ b/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