From 6eadcd92f5ad34d33e761fe65eb7a74f17389637 Mon Sep 17 00:00:00 2001 From: nobuyo Date: Wed, 4 May 2022 14:39:12 +0900 Subject: [PATCH] Add new `InternalAffairs/MethodNameEndWith` cop This cop checks potentially usage of method identifier predicates instead of `method_name.to_s.end_with?`. ```ruby node.method_name.to_s.end_with?('=') node.assignment_method? node.method_name.to_s.end_with?('?') node.predicate_method? node.method_name.to_s.end_with?('!') node.bang_method? ``` Use `call` instead of `{send csend}` in node matcher expression Co-authored-by: Koichi ITO Tweak conditional method call for more DRY Co-authored-by: Koichi ITO --- .../cop/gemspec/duplicated_assignment.rb | 7 +- lib/rubocop/cop/internal_affairs.rb | 1 + .../internal_affairs/method_name_end_with.rb | 80 ++++++++++++++++ .../cop/lint/return_in_void_context.rb | 22 +---- lib/rubocop/cop/naming/predicate_name.rb | 4 +- lib/rubocop/cop/style/collection_compact.rb | 6 +- .../cop/style/redundant_self_assignment.rb | 3 +- lib/rubocop/cop/style/trivial_accessors.rb | 13 ++- .../method_name_end_with_spec.rb | 96 +++++++++++++++++++ 9 files changed, 196 insertions(+), 36 deletions(-) create mode 100644 lib/rubocop/cop/internal_affairs/method_name_end_with.rb create mode 100644 spec/rubocop/cop/internal_affairs/method_name_end_with_spec.rb diff --git a/lib/rubocop/cop/gemspec/duplicated_assignment.rb b/lib/rubocop/cop/gemspec/duplicated_assignment.rb index 7463606dee5..e31ab891f29 100644 --- a/lib/rubocop/cop/gemspec/duplicated_assignment.rb +++ b/lib/rubocop/cop/gemspec/duplicated_assignment.rb @@ -44,7 +44,7 @@ class DuplicatedAssignment < Base # @!method assignment_method_declarations(node) def_node_search :assignment_method_declarations, <<~PATTERN (send - (lvar #match_block_variable_name?) #assignment_method? ...) + (lvar #match_block_variable_name?) _ ...) PATTERN def on_new_investigation @@ -65,12 +65,9 @@ def match_block_variable_name?(receiver_name) end end - def assignment_method?(method_name) - method_name.to_s.end_with?('=') - end - def duplicated_assignment_method_nodes assignment_method_declarations(processed_source.ast) + .select(&:assignment_method?) .group_by(&:method_name) .values .select { |nodes| nodes.size > 1 } diff --git a/lib/rubocop/cop/internal_affairs.rb b/lib/rubocop/cop/internal_affairs.rb index abf8855263f..e1eaa1665b1 100644 --- a/lib/rubocop/cop/internal_affairs.rb +++ b/lib/rubocop/cop/internal_affairs.rb @@ -4,6 +4,7 @@ require_relative 'internal_affairs/example_description' require_relative 'internal_affairs/inherit_deprecated_cop_class' require_relative 'internal_affairs/location_line_equality_comparison' +require_relative 'internal_affairs/method_name_end_with' require_relative 'internal_affairs/method_name_equal' require_relative 'internal_affairs/node_destructuring' require_relative 'internal_affairs/node_matcher_directive' diff --git a/lib/rubocop/cop/internal_affairs/method_name_end_with.rb b/lib/rubocop/cop/internal_affairs/method_name_end_with.rb new file mode 100644 index 00000000000..8fed95b84cd --- /dev/null +++ b/lib/rubocop/cop/internal_affairs/method_name_end_with.rb @@ -0,0 +1,80 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module InternalAffairs + # This cop checks potentially usage of method identifier predicates + # defined in rubocop-ast instead of `method_name.end_with?`. + # + # @example + # # bad + # node.method_name.to_s.end_with?('=') + # + # # good + # node.assignment_method? + # + # # bad + # node.method_name.to_s.end_with?('?') + # + # # good + # node.predicate_method? + # + # # bad + # node.method_name.to_s.end_with?('!') + # + # # good + # node.bang_method? + # + class MethodNameEndWith < Base + include RangeHelp + extend AutoCorrector + + MSG = 'Use `%s` instead of `%s`.' + SUGGEST_METHOD_FOR_SUFFIX = { + '=' => 'assignment_method?', + '!' => 'bang_method?', + '?' => 'predicate_method?' + }.freeze + + # @!method method_name_end_with?(node) + def_node_matcher :method_name_end_with?, <<~PATTERN + { + (call + (call + $(... :method_name) :to_s) :end_with? + $(str {"=" "?" "!"})) + (call + $(... :method_name) :end_with? + $(str {"=" "?" "!"})) + } + PATTERN + + def on_send(node) + method_name_end_with?(node) do |method_name_node, end_with_arg| + range = range(method_name_node, node) + message = format( + MSG, + method_name: SUGGEST_METHOD_FOR_SUFFIX[end_with_arg.value], + method_suffix: range.source + ) + + add_offense(node, message: message) + end + end + alias on_csend on_send + + private + + def range(method_name_node, node) + range = if method_name_node.call_type? + method_name_node.loc.selector + else + method_name_node.source_range + end + + range_between(range.begin_pos, node.source_range.end_pos) + end + end + end + end +end diff --git a/lib/rubocop/cop/lint/return_in_void_context.rb b/lib/rubocop/cop/lint/return_in_void_context.rb index 1c32cb6da98..a88f4d81f4f 100644 --- a/lib/rubocop/cop/lint/return_in_void_context.rb +++ b/lib/rubocop/cop/lint/return_in_void_context.rb @@ -40,12 +40,12 @@ def on_return(return_node) context_node = non_void_context(return_node) return unless context_node&.def_type? + return unless context_node&.void_context? - method_name = method_name(context_node) - - return unless method_name && void_context_method?(method_name) - - add_offense(return_node.loc.keyword, message: format(message, method: method_name)) + add_offense( + return_node.loc.keyword, + message: format(message, method: context_node.method_name) + ) end private @@ -53,18 +53,6 @@ def on_return(return_node) def non_void_context(return_node) return_node.each_ancestor(:block, :def, :defs).first end - - def method_name(context_node) - context_node.children.first - end - - def void_context_method?(method_name) - method_name == :initialize || setter_method?(method_name) - end - - def setter_method?(method_name) - method_name.to_s.end_with?('=') && !AST::Node::COMPARISON_OPERATORS.include?(method_name) - end end end end diff --git a/lib/rubocop/cop/naming/predicate_name.rb b/lib/rubocop/cop/naming/predicate_name.rb index 5d9e49b8a88..aac396633e4 100644 --- a/lib/rubocop/cop/naming/predicate_name.rb +++ b/lib/rubocop/cop/naming/predicate_name.rb @@ -70,7 +70,7 @@ def allowed_method_name?(method_name, prefix) !(method_name.start_with?(prefix) && # cheap check to avoid allocating Regexp method_name.match?(/^#{prefix}[^0-9]/)) || method_name == expected_name(method_name, prefix) || - method_name.end_with?('=') || + method_name.end_with?('=') || # rubocop:todo InternalAffairs/MethodNameEndWith allowed_method?(method_name) end @@ -80,7 +80,7 @@ def expected_name(method_name, prefix) else method_name.dup end - new_name << '?' unless method_name.end_with?('?') + new_name << '?' unless method_name.end_with?('?') # rubocop:todo InternalAffairs/MethodNameEndWith new_name end diff --git a/lib/rubocop/cop/style/collection_compact.rb b/lib/rubocop/cop/style/collection_compact.rb index cb76edc0419..5ca0f81328d 100644 --- a/lib/rubocop/cop/style/collection_compact.rb +++ b/lib/rubocop/cop/style/collection_compact.rb @@ -70,7 +70,7 @@ class CollectionCompact < Base def on_send(node) return unless (range = offense_range(node)) - good = good_method_name(node.method_name) + good = good_method_name(node) message = format(MSG, good: good, bad: range.source) add_offense(range, message: message) { |corrector| corrector.replace(range, good) } @@ -94,8 +94,8 @@ def offense_range(node) end end - def good_method_name(method_name) - if method_name.to_s.end_with?('!') + def good_method_name(node) + if node.bang_method? 'compact!' else 'compact' diff --git a/lib/rubocop/cop/style/redundant_self_assignment.rb b/lib/rubocop/cop/style/redundant_self_assignment.rb index 3fa1624ac07..5e3ab746ae8 100644 --- a/lib/rubocop/cop/style/redundant_self_assignment.rb +++ b/lib/rubocop/cop/style/redundant_self_assignment.rb @@ -67,8 +67,7 @@ def on_lvasgn(node) alias on_gvasgn on_lvasgn def on_send(node) - # TODO: Remove `Symbol#to_s` after supporting only Ruby >= 2.7. - return unless node.method_name.to_s.end_with?('=') + return unless node.assignment_method? return unless redundant_assignment?(node) message = format(MSG, method_name: node.first_argument.method_name) diff --git a/lib/rubocop/cop/style/trivial_accessors.rb b/lib/rubocop/cop/style/trivial_accessors.rb index c8839e9ca57..cdd83380406 100644 --- a/lib/rubocop/cop/style/trivial_accessors.rb +++ b/lib/rubocop/cop/style/trivial_accessors.rb @@ -167,8 +167,8 @@ def allowed_method_names allowed_methods.map(&:to_sym) + [:initialize] end - def dsl_writer?(method_name) - !method_name.to_s.end_with?('=') + def dsl_writer?(node) + !node.assignment_method? end def trivial_reader?(node) @@ -180,8 +180,7 @@ def looks_like_trivial_reader?(node) end def trivial_writer?(node) - looks_like_trivial_writer?(node) && - !allowed_method_name?(node) && !allowed_writer?(node.method_name) + looks_like_trivial_writer?(node) && !allowed_method_name?(node) && !allowed_writer?(node) end # @!method looks_like_trivial_writer?(node) @@ -195,8 +194,8 @@ def allowed_method_name?(node) (exact_name_match? && !names_match?(node)) end - def allowed_writer?(method_name) - allow_dsl_writers? && dsl_writer?(method_name) + def allowed_writer?(node) + allow_dsl_writers? && dsl_writer?(node) end def allowed_reader?(node) @@ -210,7 +209,7 @@ def names_match?(node) end def trivial_accessor_kind(node) - if trivial_writer?(node) && !dsl_writer?(node.method_name) + if trivial_writer?(node) && !dsl_writer?(node) 'writer' elsif trivial_reader?(node) 'reader' diff --git a/spec/rubocop/cop/internal_affairs/method_name_end_with_spec.rb b/spec/rubocop/cop/internal_affairs/method_name_end_with_spec.rb new file mode 100644 index 00000000000..1725d9bec31 --- /dev/null +++ b/spec/rubocop/cop/internal_affairs/method_name_end_with_spec.rb @@ -0,0 +1,96 @@ +# frozen_string_literal: true + +RSpec.describe RuboCop::Cop::InternalAffairs::MethodNameEndWith, :config do + it 'registers an offense if there is potentially usage of `assignment_method?`' do + expect_offense(<<~RUBY) + node.method_name.to_s.end_with?('=') + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `assignment_method?` instead of `method_name.to_s.end_with?('=')`. + RUBY + end + + it 'registers an offense if `method_name` is a variable and there is potentially usage of `assignment_method?`' do + expect_offense(<<~RUBY) + def assignment_method?(method_name) + method_name.to_s.end_with?('=') + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `assignment_method?` instead of `method_name.to_s.end_with?('=')`. + end + RUBY + end + + it 'registers offense if there is potentially usage of `predicate_method?`' do + expect_offense(<<~RUBY) + node.method_name.to_s.end_with?('?') + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `predicate_method?` instead of `method_name.to_s.end_with?('?')`. + RUBY + end + + it 'registers offense if there is potentially usage of `bang_method?`' do + expect_offense(<<~RUBY) + node.method_name.to_s.end_with?('!') + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `bang_method?` instead of `method_name.to_s.end_with?('!')`. + RUBY + end + + it 'registers offense if there is potentially usage of `bang_method?` with safe navigation operator' do + expect_offense(<<~RUBY) + node.method_name&.to_s&.end_with?('!') + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `bang_method?` instead of `method_name&.to_s&.end_with?('!')`. + RUBY + end + + it 'does not register offense if argument for end_with? is some other string' do + expect_no_offenses(<<~RUBY) + node.method_name.to_s.end_with?('_foo') + RUBY + end + + context 'Ruby >= 2.7', :ruby27 do + it 'registers an offense if method_name is symbol' do + expect_offense(<<~RUBY) + node.method_name.end_with?('=') + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `assignment_method?` instead of `method_name.end_with?('=')`. + RUBY + end + + it 'registers an offense if method_name is symbol with safe navigation operator' do + expect_offense(<<~RUBY) + node&.method_name&.end_with?('=') + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `assignment_method?` instead of `method_name&.end_with?('=')`. + RUBY + end + + it 'registers offense if argument for Symbol#end_with? is \'?\'' do + expect_offense(<<~RUBY) + node.method_name.end_with?('?') + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `predicate_method?` instead of `method_name.end_with?('?')`. + RUBY + end + + it 'registers offense if argument for Symbol#end_with? is \'?\' with safe navigation operator' do + expect_offense(<<~RUBY) + node.method_name&.end_with?('?') + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `predicate_method?` instead of `method_name&.end_with?('?')`. + RUBY + end + + it 'registers offense if argument for Symbol#end_with? is \'!\'' do + expect_offense(<<~RUBY) + node.method_name.end_with?('!') + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `bang_method?` instead of `method_name.end_with?('!')`. + RUBY + end + + it 'registers offense if argument for Symbol#end_with? is \'!\' with safe navigation operator' do + expect_offense(<<~RUBY) + node.method_name&.end_with?('!') + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `bang_method?` instead of `method_name&.end_with?('!')`. + RUBY + end + + it 'does not register offense if argument for Symbol#end_with? is some other string' do + expect_no_offenses(<<~RUBY) + node.method_name.end_with?('_foo') + RUBY + end + end +end