Skip to content

Commit

Permalink
Make some cops aware of safe navigation operator
Browse files Browse the repository at this point in the history
Fixes #1191, #1192, #1193, #1194, #1196, #1197, #1201, and #1202.

This PR makes `Rails/ActiveSupportAliases`, `Rails/FindBy`, `Rails/FindById`, `Rails/Inquiry`,
`Rails/Pick` `Rails/PluckId`, `Rails/PluckInWhere`, `Rails/WhereEquals`, `Rails/WhereExists`,
and `Rails/WhereNot` cops aware of safe navigation operator.
  • Loading branch information
koic committed Dec 15, 2023
1 parent a3db656 commit 7c672a0
Show file tree
Hide file tree
Showing 22 changed files with 301 additions and 62 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
* [#1204](https://github.com/rubocop/rubocop-rails/pull/1204): Make `Rails/ActiveSupportAliases`, `Rails/FindBy`, `Rails/FindById`, `Rails/Inquiry`, `Rails/Pick` `Rails/PluckId`, `Rails/PluckInWhere`, `Rails/WhereEquals`, `Rails/WhereExists`, and `Rails/WhereNot` cops aware of safe navigation operator. ([@koic][])
2 changes: 1 addition & 1 deletion lib/rubocop/cop/mixin/active_record_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ def polymorphic?(belongs_to)
end

def in_where?(node)
send_node = node.each_ancestor(:send).first
send_node = node.each_ancestor(:send, :csend).first
return false unless send_node

return true if WHERE_METHODS.include?(send_node.method_name)
Expand Down
11 changes: 6 additions & 5 deletions lib/rubocop/cop/rails/active_support_aliases.rb
Original file line number Diff line number Diff line change
Expand Up @@ -27,13 +27,13 @@ class ActiveSupportAliases < Base

ALIASES = {
starts_with?: {
original: :start_with?, matcher: '(send str :starts_with? _)'
original: :start_with?, matcher: '(call str :starts_with? _)'
},
ends_with?: {
original: :end_with?, matcher: '(send str :ends_with? _)'
original: :end_with?, matcher: '(call str :ends_with? _)'
},
append: { original: :<<, matcher: '(send array :append _)' },
prepend: { original: :unshift, matcher: '(send array :prepend _)' }
append: { original: :<<, matcher: '(call array :append _)' },
prepend: { original: :unshift, matcher: '(call array :prepend _)' }
}.freeze

ALIASES.each do |aliased_method, options|
Expand All @@ -47,13 +47,14 @@ def on_send(node)
preferred_method = ALIASES[aliased_method][:original]
message = format(MSG, prefer: preferred_method, current: aliased_method)

add_offense(node, message: message) do |corrector|
add_offense(node.loc.selector.join(node.source_range.end), message: message) do |corrector|
next if append(node)

corrector.replace(node.loc.selector, preferred_method)
end
end
end
alias on_csend on_send
end
end
end
Expand Down
4 changes: 2 additions & 2 deletions lib/rubocop/cop/rails/find_by.rb
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ class FindBy < Base
include RangeHelp
extend AutoCorrector

MSG = 'Use `find_by` instead of `where.%<method>s`.'
MSG = 'Use `find_by` instead of `where%<dot>s%<method>s`.'
RESTRICT_ON_SEND = %i[first take].freeze

def on_send(node)
Expand All @@ -37,7 +37,7 @@ def on_send(node)

range = offense_range(node)

add_offense(range, message: format(MSG, method: node.method_name)) do |corrector|
add_offense(range, message: format(MSG, dot: node.loc.dot.source, method: node.method_name)) do |corrector|
autocorrect(corrector, node)
end
end
Expand Down
32 changes: 9 additions & 23 deletions lib/rubocop/cop/rails/find_by_id.rb
Original file line number Diff line number Diff line change
Expand Up @@ -24,40 +24,39 @@ class FindById < Base
RESTRICT_ON_SEND = %i[take! find_by_id! find_by!].freeze

def_node_matcher :where_take?, <<~PATTERN
(send
$(send _ :where
(call
$(call _ :where
(hash
(pair (sym :id) $_))) :take!)
PATTERN

def_node_matcher :find_by?, <<~PATTERN
{
(send _ :find_by_id! $_)
(send _ :find_by! (hash (pair (sym :id) $_)))
(call _ :find_by_id! $_)
(call _ :find_by! (hash (pair (sym :id) $_)))
}
PATTERN

def on_send(node)
where_take?(node) do |where, id_value|
range = where_take_offense_range(node, where)
bad_method = build_where_take_bad_method(id_value)

register_offense(range, id_value, bad_method)
register_offense(range, id_value)
end

find_by?(node) do |id_value|
range = find_by_offense_range(node)
bad_method = build_find_by_bad_method(node, id_value)

register_offense(range, id_value, bad_method)
register_offense(range, id_value)
end
end
alias on_csend on_send

private

def register_offense(range, id_value, bad_method)
def register_offense(range, id_value)
good_method = build_good_method(id_value)
message = format(MSG, good_method: good_method, bad_method: bad_method)
message = format(MSG, good_method: good_method, bad_method: range.source)

add_offense(range, message: message) do |corrector|
corrector.replace(range, good_method)
Expand All @@ -75,19 +74,6 @@ def find_by_offense_range(node)
def build_good_method(id_value)
"find(#{id_value.source})"
end

def build_where_take_bad_method(id_value)
"where(id: #{id_value.source}).take!"
end

def build_find_by_bad_method(node, id_value)
case node.method_name
when :find_by_id!
"find_by_id!(#{id_value.source})"
when :find_by!
"find_by!(id: #{id_value.source})"
end
end
end
end
end
Expand Down
1 change: 1 addition & 0 deletions lib/rubocop/cop/rails/inquiry.rb
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ def on_send(node)

add_offense(node.loc.selector)
end
alias on_csend on_send
end
end
end
Expand Down
11 changes: 6 additions & 5 deletions lib/rubocop/cop/rails/pick.rb
Original file line number Diff line number Diff line change
Expand Up @@ -28,13 +28,13 @@ class Pick < Base
extend AutoCorrector
extend TargetRailsVersion

MSG = 'Prefer `pick(%<args>s)` over `pluck(%<args>s).first`.'
MSG = 'Prefer `pick(%<args>s)` over `%<current>s`.'
RESTRICT_ON_SEND = %i[first].freeze

minimum_target_rails_version 6.0

def_node_matcher :pick_candidate?, <<~PATTERN
(send (send _ :pluck ...) :first)
(call (call _ :pluck ...) :first)
PATTERN

def on_send(node)
Expand All @@ -44,19 +44,20 @@ def on_send(node)
node_selector = node.loc.selector
range = receiver_selector.join(node_selector)

add_offense(range, message: message(receiver)) do |corrector|
add_offense(range, message: message(receiver, range)) do |corrector|
first_range = receiver.source_range.end.join(node_selector)

corrector.remove(first_range)
corrector.replace(receiver_selector, 'pick')
end
end
end
alias on_csend on_send

private

def message(receiver)
format(MSG, args: receiver.arguments.map(&:source).join(', '))
def message(receiver, current)
format(MSG, args: receiver.arguments.map(&:source).join(', '), current: current.source)
end
end
end
Expand Down
3 changes: 2 additions & 1 deletion lib/rubocop/cop/rails/pluck_id.rb
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ class PluckId < Base
RESTRICT_ON_SEND = %i[pluck].freeze

def_node_matcher :pluck_id_call?, <<~PATTERN
(send _ :pluck {(sym :id) (send nil? :primary_key)})
(call _ :pluck {(sym :id) (send nil? :primary_key)})
PATTERN

def on_send(node)
Expand All @@ -47,6 +47,7 @@ def on_send(node)
corrector.replace(offense_range(node), 'ids')
end
end
alias on_csend on_send

private

Expand Down
3 changes: 2 additions & 1 deletion lib/rubocop/cop/rails/pluck_in_where.rb
Original file line number Diff line number Diff line change
Expand Up @@ -65,13 +65,14 @@ def on_send(node)
corrector.replace(range, replacement)
end
end
alias on_csend on_send

private

def root_receiver(node)
receiver = node.receiver

if receiver&.send_type?
if receiver&.call_type?
root_receiver(receiver)
else
receiver
Expand Down
5 changes: 3 additions & 2 deletions lib/rubocop/cop/rails/where_equals.rb
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,8 @@ class WhereEquals < Base

def_node_matcher :where_method_call?, <<~PATTERN
{
(send _ :where (array $str_type? $_ ?))
(send _ :where $str_type? $_ ?)
(call _ :where (array $str_type? $_ ?))
(call _ :where $str_type? $_ ?)
}
PATTERN

Expand All @@ -55,6 +55,7 @@ def on_send(node)
end
end
end
alias on_csend on_send

EQ_ANONYMOUS_RE = /\A([\w.]+)\s+=\s+\?\z/.freeze # column = ?
IN_ANONYMOUS_RE = /\A([\w.]+)\s+IN\s+\(\?\)\z/i.freeze # column IN (?)
Expand Down
17 changes: 9 additions & 8 deletions lib/rubocop/cop/rails/where_exists.rb
Original file line number Diff line number Diff line change
Expand Up @@ -55,26 +55,27 @@ class WhereExists < Base
RESTRICT_ON_SEND = %i[exists?].freeze

def_node_matcher :where_exists_call?, <<~PATTERN
(send (send _ :where $...) :exists?)
(call (call _ :where $...) :exists?)
PATTERN

def_node_matcher :exists_with_args?, <<~PATTERN
(send _ :exists? $...)
(call _ :exists? $...)
PATTERN

def on_send(node)
find_offenses(node) do |args|
return unless convertable_args?(args)

range = correction_range(node)
good_method = build_good_method(args)
good_method = build_good_method(args, dot_source: node.loc.dot.source)
message = format(MSG, good_method: good_method, bad_method: range.source)

add_offense(range, message: message) do |corrector|
corrector.replace(range, good_method)
end
end
end
alias on_csend on_send

private

Expand Down Expand Up @@ -108,11 +109,11 @@ def correction_range(node)
end
end

def build_good_method(args)
def build_good_method(args, dot_source: '.')
if exists_style?
build_good_method_exists(args)
elsif where_style?
build_good_method_where(args)
build_good_method_where(args, dot_source)
end
end

Expand All @@ -124,11 +125,11 @@ def build_good_method_exists(args)
end
end

def build_good_method_where(args)
def build_good_method_where(args, dot_source)
if args.size > 1
"where(#{args.map(&:source).join(', ')}).exists?"
"where(#{args.map(&:source).join(', ')})#{dot_source}exists?"
else
"where(#{args[0].source}).exists?"
"where(#{args[0].source})#{dot_source}exists?"
end
end
end
Expand Down
13 changes: 7 additions & 6 deletions lib/rubocop/cop/rails/where_not.rb
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,8 @@ class WhereNot < Base

def_node_matcher :where_method_call?, <<~PATTERN
{
(send _ :where (array $str_type? $_ ?))
(send _ :where $str_type? $_ ?)
(call _ :where (array $str_type? $_ ?))
(call _ :where $str_type? $_ ?)
}
PATTERN

Expand All @@ -46,14 +46,15 @@ def on_send(node)
column_and_value = extract_column_and_value(template_node, value_node)
return unless column_and_value

good_method = build_good_method(*column_and_value)
good_method = build_good_method(node.loc.dot.source, *column_and_value)
message = format(MSG, good_method: good_method)

add_offense(range, message: message) do |corrector|
corrector.replace(range, good_method)
end
end
end
alias on_csend on_send

NOT_EQ_ANONYMOUS_RE = /\A([\w.]+)\s+(?:!=|<>)\s+\?\z/.freeze # column != ?, column <> ?
NOT_IN_ANONYMOUS_RE = /\A([\w.]+)\s+NOT\s+IN\s+\(\?\)\z/i.freeze # column NOT IN (?)
Expand Down Expand Up @@ -86,13 +87,13 @@ def extract_column_and_value(template_node, value_node)
[Regexp.last_match(1), value]
end

def build_good_method(column, value)
def build_good_method(dot, column, value)
if column.include?('.')
table, column = column.split('.')

"where.not(#{table}: { #{column}: #{value} })"
"where#{dot}not(#{table}: { #{column}: #{value} })"
else
"where.not(#{column}: #{value})"
"where#{dot}not(#{column}: #{value})"
end
end
end
Expand Down
Loading

0 comments on commit 7c672a0

Please sign in to comment.