From 8a5f755f8b032225c4957fda0c2f91dd4e08ca2e Mon Sep 17 00:00:00 2001 From: nobuyo Date: Fri, 17 Jun 2022 23:23:02 +0900 Subject: [PATCH] [Fix #10698] Enhance Style/HashExcept to support array inclusion checks --- ...ncance_stylehashexcept_to_support_array.md | 1 + config/default.yml | 1 + lib/rubocop/cop/style/hash_except.rb | 96 ++++- spec/rubocop/cop/style/hash_except_spec.rb | 360 ++++++++++++++++++ 4 files changed, 450 insertions(+), 8 deletions(-) create mode 100644 changelog/fix_encance_stylehashexcept_to_support_array.md diff --git a/changelog/fix_encance_stylehashexcept_to_support_array.md b/changelog/fix_encance_stylehashexcept_to_support_array.md new file mode 100644 index 00000000000..82cc1e5896f --- /dev/null +++ b/changelog/fix_encance_stylehashexcept_to_support_array.md @@ -0,0 +1 @@ +* [#10698](https://github.com/rubocop/rubocop/issues/10698): Enhance `Style/HashExcept` to support array inclusion checks. ([@nobuyo][]) diff --git a/config/default.yml b/config/default.yml index 54404cbc74c..2ded17c5f7e 100644 --- a/config/default.yml +++ b/config/default.yml @@ -3726,6 +3726,7 @@ Style/HashExcept: that can be replaced with `Hash#except` method. Enabled: pending VersionAdded: '1.7' + VersionChanged: '<>' Style/HashLikeCase: Description: >- diff --git a/lib/rubocop/cop/style/hash_except.rb b/lib/rubocop/cop/style/hash_except.rb index 6e764bf5913..6b5a489035e 100644 --- a/lib/rubocop/cop/style/hash_except.rb +++ b/lib/rubocop/cop/style/hash_except.rb @@ -19,6 +19,9 @@ module Style # {foo: 1, bar: 2, baz: 3}.reject {|k, v| k == :bar } # {foo: 1, bar: 2, baz: 3}.select {|k, v| k != :bar } # {foo: 1, bar: 2, baz: 3}.filter {|k, v| k != :bar } + # {foo: 1, bar: 2, baz: 3}.reject {|k, v| %i[foo bar].include?(k) } + # {foo: 1, bar: 2, baz: 3}.select {|k, v| !%i[foo bar].include?(k) } + # {foo: 1, bar: 2, baz: 3}.filter {|k, v| !%i[foo bar].include?(k) } # # # good # {foo: 1, bar: 2, baz: 3}.except(:bar) @@ -33,15 +36,36 @@ class HashExcept < Base MSG = 'Use `%s` instead.' RESTRICT_ON_SEND = %i[reject select filter].freeze - # @!method bad_method?(node) - def_node_matcher :bad_method?, <<~PATTERN + # @!method bad_method_with_poro?(node) + def_node_matcher :bad_method_with_poro?, <<~PATTERN (block (send _ _) (args (arg _) (arg _)) - (send - _ {:== :!= :eql?} _)) + { + (send + _ {:== :!= :eql? :include?} _) + (send + (send + _ {:== :!= :eql? :include?} _) :!) + }) + PATTERN + + # @!method bad_method_with_active_support?(node) + def_node_matcher :bad_method_with_active_support?, <<~PATTERN + (block + (send _ _) + (args + (arg _) + (arg _)) + { + (send + _ {:== :!= :eql? :in? :include? :exclude?} _) + (send + (send + _ {:== :!= :eql? :in? :include? :exclude?} _) :!) + }) PATTERN def on_send(node) @@ -52,7 +76,7 @@ def on_send(node) return if except_key.nil? || !safe_to_register_offense?(block, except_key) range = offense_range(node) - preferred_method = "except(#{except_key.source})" + preferred_method = "except(#{except_key_source(except_key)})" add_offense(range, message: format(MSG, prefer: preferred_method)) do |corrector| corrector.replace(range, preferred_method) @@ -61,28 +85,84 @@ def on_send(node) private + def bad_method?(block) + if active_support_extensions_enabled? + bad_method_with_active_support?(block) + else + bad_method_with_poro?(block) + end + end + def semantically_except_method?(send, block) body = block.body + negated = body.method?('!') + body = body.receiver if negated + case send.method_name when :reject - body.method?('==') || body.method?('eql?') + body.method?('==') || body.method?('eql?') || included?(negated, body) when :select, :filter - body.method?('!=') + body.method?('!=') || not_included?(negated, body) else false end end + def included?(negated, body) + body.method?('include?') || body.method?('in?') || (negated && body.method?('exclude?')) + end + + def not_included?(negated, body) + body.method?('exclude?') || (negated && (body.method?('include?') || body.method?('in?'))) + end + def safe_to_register_offense?(block, except_key) + extracted = extract_body_if_nagated(block.body) + if extracted.method?('in?') || extracted.method?('include?') || \ + extracted.method?('exclude?') + return true + end return true if block.body.method?('eql?') except_key.sym_type? || except_key.str_type? end + def extract_body_if_nagated(body) + return body unless body.method?('!') + + body.receiver + end + + def except_key_source(key) + if key.array_type? + key = if key.percent_literal? + key.each_value.map { |v| decorate_source(v) } + else + key.each_value.map(&:source) + end + return key.join(', ') + end + + key.source + end + + def decorate_source(value) + return ":\"#{value.source}\"" if value.dsym_type? + return "\"#{value.source}\"" if value.dstr_type? + return ":#{value.source}" if value.sym_type? + + "'#{value.source}'" + end + def except_key(node) key_argument = node.argument_list.first.source - lhs, _method_name, rhs = *node.body + body = extract_body_if_nagated(node.body) + lhs, _method_name, rhs = *body + + return lhs if body.method?('include?') + return lhs if body.method?('exclude?') + return rhs if body.method?('in?') return if [lhs, rhs].map(&:source).none?(key_argument) [lhs, rhs].find { |operand| operand.source != key_argument } diff --git a/spec/rubocop/cop/style/hash_except_spec.rb b/spec/rubocop/cop/style/hash_except_spec.rb index 14623cd993b..f22ad234569 100644 --- a/spec/rubocop/cop/style/hash_except_spec.rb +++ b/spec/rubocop/cop/style/hash_except_spec.rb @@ -79,6 +79,79 @@ RUBY end + context 'using `in?`' do + it 'does not register offenses when using `reject` and calling `key.in?` method with symbol array' do + expect_no_offenses(<<~RUBY) + {foo: 1, bar: 2, baz: 3}.reject { |k, v| k.in?(%i[foo bar]) } + RUBY + end + end + + context 'using `include?`' do + it 'registers and corrects an offense when using `reject` and calling `include?` method with symbol array' do + expect_offense(<<~RUBY) + {foo: 1, bar: 2, baz: 3}.reject { |k, v| !%i[foo bar].include?(k) } + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `except(:foo, :bar)` instead. + RUBY + + expect_correction(<<~RUBY) + {foo: 1, bar: 2, baz: 3}.except(:foo, :bar) + RUBY + end + + it 'registers and corrects an offense when using `select` and calling `!include?` method with symbol array' do + expect_offense(<<~RUBY) + {foo: 1, bar: 2, baz: 3}.select { |k, v| !%i[foo bar].include?(k) } + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `except(:foo, :bar)` instead. + RUBY + + expect_correction(<<~RUBY) + {foo: 1, bar: 2, baz: 3}.except(:foo, :bar) + RUBY + end + + it 'registers and corrects an offense when using `filter` and calling `!include?` method with symbol array' do + expect_offense(<<~RUBY) + {foo: 1, bar: 2, baz: 3}.filter { |k, v| ![:foo, :bar].include?(k) } + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `except(:foo, :bar)` instead. + RUBY + + expect_correction(<<~RUBY) + {foo: 1, bar: 2, baz: 3}.except(:foo, :bar) + RUBY + end + + it 'registers and corrects an offense when using `reject` and calling `include?` method with dynamic symbol array' do + expect_offense(<<~RUBY) + {foo: 1, bar: 2, baz: 3}.reject { |k, v| %I[\#{foo} bar].include?(k) } + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `except(:"\#{foo}", :bar)` instead. + RUBY + + expect_correction(<<~RUBY) + {foo: 1, bar: 2, baz: 3}.except(:"\#{foo}", :bar) + RUBY + end + + it 'registers and corrects an offense when using `reject` and calling `include?` method with dynamic string array' do + expect_offense(<<~RUBY) + {foo: 1, bar: 2, baz: 3}.reject { |k, v| %W[\#{foo} bar].include?(k) } + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `except("\#{foo}", 'bar')` instead. + RUBY + + expect_correction(<<~RUBY) + {foo: 1, bar: 2, baz: 3}.except("\#{foo}", 'bar') + RUBY + end + end + + context 'using `exclude?`' do + it 'does not register offenses when using `reject` and calling `!exclude?` method with symbol array' do + expect_no_offenses(<<~RUBY) + {foo: 1, bar: 2, baz: 3}.reject { |k, v| !%i[foo bar].exclude?(k) } + RUBY + end + end + it 'does not register an offense when using `reject` and other than comparison by string and symbol using `==`' do expect_no_offenses(<<~RUBY) hash.reject { |k, v| k == 0.0 } @@ -102,6 +175,287 @@ {foo: 1, bar: 2, baz: 3}.reject { |k, v| v.eql? :bar } RUBY end + + context 'when `AllCops/ActiveSupportExtensionsEnabled: true`' do + let(:config) do + RuboCop::Config.new('AllCops' => { + 'TargetRubyVersion' => '3.0', + 'ActiveSupportExtensionsEnabled' => true + }) + end + + it 'registers and corrects an offense when using `reject` and comparing with `lvar == :sym`' do + expect_offense(<<~RUBY) + {foo: 1, bar: 2, baz: 3}.reject { |k, v| k == :bar } + ^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `except(:bar)` instead. + RUBY + + expect_correction(<<~RUBY) + {foo: 1, bar: 2, baz: 3}.except(:bar) + RUBY + end + + it 'registers and corrects an offense when using `reject` and comparing with `:sym == lvar`' do + expect_offense(<<~RUBY) + {foo: 1, bar: 2, baz: 3}.reject { |k, v| :bar == k } + ^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `except(:bar)` instead. + RUBY + + expect_correction(<<~RUBY) + {foo: 1, bar: 2, baz: 3}.except(:bar) + RUBY + end + + it 'registers and corrects an offense when using `select` and comparing with `lvar != :sym`' do + expect_offense(<<~RUBY) + {foo: 1, bar: 2, baz: 3}.select { |k, v| k != :bar } + ^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `except(:bar)` instead. + RUBY + + expect_correction(<<~RUBY) + {foo: 1, bar: 2, baz: 3}.except(:bar) + RUBY + end + + it 'registers and corrects an offense when using `select` and comparing with `:sym != lvar`' do + expect_offense(<<~RUBY) + {foo: 1, bar: 2, baz: 3}.select { |k, v| :bar != k } + ^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `except(:bar)` instead. + RUBY + + expect_correction(<<~RUBY) + {foo: 1, bar: 2, baz: 3}.except(:bar) + RUBY + end + + it "registers and corrects an offense when using `reject` and comparing with `lvar == 'str'`" do + expect_offense(<<~RUBY) + hash.reject { |k, v| k == 'str' } + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `except('str')` instead. + RUBY + + expect_correction(<<~RUBY) + hash.except('str') + RUBY + end + + it 'registers and corrects an offense when using `reject` and other than comparison by string and symbol using `eql?`' do + expect_offense(<<~RUBY) + hash.reject { |k, v| k.eql?(0.0) } + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `except(0.0)` instead. + RUBY + + expect_correction(<<~RUBY) + hash.except(0.0) + RUBY + end + + it 'registers and corrects an offense when using `filter` and comparing with `lvar != :sym`' do + expect_offense(<<~RUBY) + {foo: 1, bar: 2, baz: 3}.filter { |k, v| k != :bar } + ^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `except(:bar)` instead. + RUBY + + expect_correction(<<~RUBY) + {foo: 1, bar: 2, baz: 3}.except(:bar) + RUBY + end + + context 'using `in?`' do + it 'registers and corrects an offense when using `reject` and calling `key.in?` method with symbol array' do + expect_offense(<<~RUBY) + {foo: 1, bar: 2, baz: 3}.reject { |k, v| k.in?(%i[foo bar]) } + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `except(:foo, :bar)` instead. + RUBY + + expect_correction(<<~RUBY) + {foo: 1, bar: 2, baz: 3}.except(:foo, :bar) + RUBY + end + + it 'registers and corrects an offense when using `select` and calling `!key.in?` method with symbol array' do + expect_offense(<<~RUBY) + {foo: 1, bar: 2, baz: 3}.select { |k, v| !k.in?(%i[foo bar]) } + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `except(:foo, :bar)` instead. + RUBY + + expect_correction(<<~RUBY) + {foo: 1, bar: 2, baz: 3}.except(:foo, :bar) + RUBY + end + + it 'registers and corrects an offense when using `filter` and calling `!key.in?` method with symbol array' do + expect_offense(<<~RUBY) + {foo: 1, bar: 2, baz: 3}.filter { |k, v| !k.in?(%i[foo bar]) } + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `except(:foo, :bar)` instead. + RUBY + + expect_correction(<<~RUBY) + {foo: 1, bar: 2, baz: 3}.except(:foo, :bar) + RUBY + end + + it 'registers and corrects an offense when using `reject` and calling `key.in?` method with dynamic symbol array' do + expect_offense(<<~RUBY) + {foo: 1, bar: 2, baz: 3}.reject { |k, v| k.in?(%I[\#{foo} bar]) } + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `except(:"\#{foo}", :bar)` instead. + RUBY + + expect_correction(<<~RUBY) + {foo: 1, bar: 2, baz: 3}.except(:"\#{foo}", :bar) + RUBY + end + + it 'registers and corrects an offense when using `reject` and calling `key.in?` method with dynamic string array' do + expect_offense(<<~RUBY) + {foo: 1, bar: 2, baz: 3}.reject { |k, v| k.in?(%W[\#{foo} bar]) } + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `except("\#{foo}", 'bar')` instead. + RUBY + + expect_correction(<<~RUBY) + {foo: 1, bar: 2, baz: 3}.except("\#{foo}", 'bar') + RUBY + end + end + + context 'using `include?`' do + it 'registers and corrects an offense when using `reject` and calling `include?` method with symbol array' do + expect_offense(<<~RUBY) + {foo: 1, bar: 2, baz: 3}.reject { |k, v| !%i[foo bar].include?(k) } + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `except(:foo, :bar)` instead. + RUBY + + expect_correction(<<~RUBY) + {foo: 1, bar: 2, baz: 3}.except(:foo, :bar) + RUBY + end + + it 'registers and corrects an offense when using `select` and calling `!include?` method with symbol array' do + expect_offense(<<~RUBY) + {foo: 1, bar: 2, baz: 3}.select { |k, v| !%i[foo bar].include?(k) } + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `except(:foo, :bar)` instead. + RUBY + + expect_correction(<<~RUBY) + {foo: 1, bar: 2, baz: 3}.except(:foo, :bar) + RUBY + end + + it 'registers and corrects an offense when using `filter` and calling `!include?` method with symbol array' do + expect_offense(<<~RUBY) + {foo: 1, bar: 2, baz: 3}.filter { |k, v| ![:foo, :bar].include?(k) } + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `except(:foo, :bar)` instead. + RUBY + + expect_correction(<<~RUBY) + {foo: 1, bar: 2, baz: 3}.except(:foo, :bar) + RUBY + end + + it 'registers and corrects an offense when using `reject` and calling `include?` method with dynamic symbol array' do + expect_offense(<<~RUBY) + {foo: 1, bar: 2, baz: 3}.reject { |k, v| %I[\#{foo} bar].include?(k) } + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `except(:"\#{foo}", :bar)` instead. + RUBY + + expect_correction(<<~RUBY) + {foo: 1, bar: 2, baz: 3}.except(:"\#{foo}", :bar) + RUBY + end + + it 'registers and corrects an offense when using `reject` and calling `include?` method with dynamic string array' do + expect_offense(<<~RUBY) + {foo: 1, bar: 2, baz: 3}.reject { |k, v| %W[\#{foo} bar].include?(k) } + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `except("\#{foo}", 'bar')` instead. + RUBY + + expect_correction(<<~RUBY) + {foo: 1, bar: 2, baz: 3}.except("\#{foo}", 'bar') + RUBY + end + end + + context 'using `exclude?`' do + it 'registers and corrects an offense when using `reject` and calling `!exclude?` method with symbol array' do + expect_offense(<<~RUBY) + {foo: 1, bar: 2, baz: 3}.reject { |k, v| !%i[foo bar].exclude?(k) } + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `except(:foo, :bar)` instead. + RUBY + + expect_correction(<<~RUBY) + {foo: 1, bar: 2, baz: 3}.except(:foo, :bar) + RUBY + end + + it 'registers and corrects an offense when using `select` and calling `exclude?` method with symbol array' do + expect_offense(<<~RUBY) + {foo: 1, bar: 2, baz: 3}.select { |k, v| %i[foo bar].exclude?(k) } + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `except(:foo, :bar)` instead. + RUBY + + expect_correction(<<~RUBY) + {foo: 1, bar: 2, baz: 3}.except(:foo, :bar) + RUBY + end + + it 'registers and corrects an offense when using `filter` and calling `exclude?` method with symbol array' do + expect_offense(<<~RUBY) + {foo: 1, bar: 2, baz: 3}.filter { |k, v| [:foo, :bar].exclude?(k) } + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `except(:foo, :bar)` instead. + RUBY + + expect_correction(<<~RUBY) + {foo: 1, bar: 2, baz: 3}.except(:foo, :bar) + RUBY + end + + it 'registers and corrects an offense when using `reject` and calling `!exclude?` method with dynamic symbol array' do + expect_offense(<<~RUBY) + {foo: 1, bar: 2, baz: 3}.reject { |k, v| !%I[\#{foo} bar].exclude?(k) } + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `except(:"\#{foo}", :bar)` instead. + RUBY + + expect_correction(<<~RUBY) + {foo: 1, bar: 2, baz: 3}.except(:"\#{foo}", :bar) + RUBY + end + + it 'registers and corrects an offense when using `reject` and calling `!exclude?` method with dynamic string array' do + expect_offense(<<~RUBY) + {foo: 1, bar: 2, baz: 3}.reject { |k, v| !%W[\#{foo} bar].exclude?(k) } + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `except("\#{foo}", 'bar')` instead. + RUBY + + expect_correction(<<~RUBY) + {foo: 1, bar: 2, baz: 3}.except("\#{foo}", 'bar') + RUBY + end + end + + it 'does not register an offense when using `reject` and other than comparison by string and symbol using `==`' do + expect_no_offenses(<<~RUBY) + hash.reject { |k, v| k == 0.0 } + RUBY + end + + it 'does not register an offense when using `delete_if` and comparing with `lvar == :sym`' do + expect_no_offenses(<<~RUBY) + {foo: 1, bar: 2, baz: 3}.delete_if { |k, v| k == :bar } + RUBY + end + + it 'does not register an offense when using `keep_if` and comparing with `lvar != :sym`' do + expect_no_offenses(<<~RUBY) + {foo: 1, bar: 2, baz: 3}.keep_if { |k, v| k != :bar } + RUBY + end + + it 'does not register an offense when comparing with hash value' do + expect_no_offenses(<<~RUBY) + {foo: 1, bar: 2, baz: 3}.reject { |k, v| v.eql? :bar } + RUBY + end + end end context 'Ruby 2.7 or lower', :ruby27 do @@ -160,6 +514,12 @@ RUBY end + it 'does not register an offense when using `reject` and `include?`' do + expect_no_offenses(<<~RUBY) + {foo: 1, bar: 2, baz: 3}.reject { |k, v| [:bar].include?(k) } + RUBY + end + it 'does not register an offense when not using block`' do expect_no_offenses(<<~RUBY) {foo: 1, bar: 2, baz: 3}.reject