Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Fix #10698] Enhance Style/HashExcept to support array inclusion checks #10699

Merged
merged 2 commits into from Jun 20, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions 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][])
@@ -0,0 +1 @@
* [#10699](https://github.com/rubocop/rubocop/pull/10699): Add new global `ActiveSupportExtensionsEnabled` option. ([@nobuyo][])
3 changes: 3 additions & 0 deletions config/default.yml
Expand Up @@ -153,6 +153,8 @@ AllCops:
rubocop-sequel: [sequel]
rubocop-rake: [rake]
rubocop-graphql: [graphql]
# Enable/Disable checking the methods extended by Active Support.
ActiveSupportExtensionsEnabled: false

#################### Bundler ###############################

Expand Down Expand Up @@ -3724,6 +3726,7 @@ Style/HashExcept:
that can be replaced with `Hash#except` method.
Enabled: pending
VersionAdded: '1.7'
VersionChanged: '<<next>>'

Style/HashLikeCase:
Description: >-
Expand Down
11 changes: 11 additions & 0 deletions docs/modules/ROOT/pages/configuration.adoc
Expand Up @@ -784,3 +784,14 @@ The following is an example of specifying `Rails` department.
Rails:
DocumentationBaseURL: https://docs.rubocop.org/rubocop-rails
----

== Enable checking Active Support extensions

Some cops for checking specified methods (e.g. `Style/HashExcept`) supports Active Support extensions.
This is off by default, but can be enabled by the `ActiveSupportExtensionsEnabled` option.

[source,yaml]
----
AllCops:
ActiveSupportExtensionsEnabled: true
----
4 changes: 4 additions & 0 deletions lib/rubocop/config.rb
Expand Up @@ -146,6 +146,10 @@ def enabled_new_cops?
for_all_cops['NewCops'] == 'enable'
end

def active_support_extensions_enabled?
for_all_cops['ActiveSupportExtensionsEnabled']
end

def file_to_include?(file)
relative_file_path = path_relative_to_config(file)

Expand Down
4 changes: 4 additions & 0 deletions lib/rubocop/cop/base.rb
Expand Up @@ -220,6 +220,10 @@ def target_rails_version
@config.target_rails_version
end

def active_support_extensions_enabled?
@config.active_support_extensions_enabled?
end

def relevant_file?(file)
file == RuboCop::AST::ProcessedSource::STRING_SOURCE_NAME ||
(file_name_matches_any?(file, 'Include', true) &&
Expand Down
96 changes: 88 additions & 8 deletions lib/rubocop/cop/style/hash_except.rb
Expand Up @@ -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)
Expand All @@ -33,15 +36,36 @@ class HashExcept < Base
MSG = 'Use `%<prefer>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)
Expand All @@ -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)
Expand All @@ -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 }
Expand Down