From 790b1f65ae8bf36642d1a83d96f628d1d46a214f Mon Sep 17 00:00:00 2001 From: Ryan Rosenblum Date: Mon, 26 Jun 2017 09:23:03 -0400 Subject: [PATCH] [Fix #4518] Register a SafeNavigation offense when method chaining is used --- CHANGELOG.md | 1 + lib/rubocop/cop/style/safe_navigation.rb | 117 +++++++++---- .../rubocop/cop/style/safe_navigation_spec.rb | 162 +++++++++++++++--- 3 files changed, 217 insertions(+), 63 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5d593117af4a..60325123b99f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -34,6 +34,7 @@ * [#4481](https://github.com/bbatsov/rubocop/issues/4481): Prevent `Style/WordArray` and `Style/SymbolArray` from registering offenses where percent arrays don't work. ([@drenmi][]) * [#4447](https://github.com/bbatsov/rubocop/issues/4447): Prevent `Layout/EmptyLineBetweenDefs` from removing too many lines. ([@drenmi][]) * [#3892](https://github.com/bbatsov/rubocop/issues/3892): Make `Style/NumericPredicate` ignore numeric comparison of global variables. ([@drenmi][]) +* [#4518](https://github.com/bbatsov/rubocop/issues/4518): Fix bug where `Style/SafeNavigation` does not register an offense when there are chained method calls. ([@rrosenblum][]) ### Changes diff --git a/lib/rubocop/cop/style/safe_navigation.rb b/lib/rubocop/cop/style/safe_navigation.rb index 860832d8e1c3..12804de413b4 100644 --- a/lib/rubocop/cop/style/safe_navigation.rb +++ b/lib/rubocop/cop/style/safe_navigation.rb @@ -48,33 +48,30 @@ class SafeNavigation < Cop MSG = 'Use safe navigation (`&.`) instead of checking if an object ' \ 'exists before calling the method.'.freeze - NIL_METHODS = nil.methods.freeze + NIL_METHODS = nil.methods.map(&:to_s).freeze minimum_target_ruby_version 2.3 - def_node_matcher :safe_navigation_candidate, <<-PATTERN + # if format: (if checked_variable body nil) + # unless format: (if checked_variable nil body) + def_node_matcher :modifier_if_safe_navigation_candidate?, <<-PATTERN { - (if - {(send (send $_ :nil?) :!) $_} - {(send $_ $_ ...) (block (send $_ $_ ...) ...)} - ...) - (if - (send $_ {:nil? :!}) nil - {(send $_ $_ ...) (block (send $_ $_ ...) ...)} - ...) + (if { + (send $_ {:nil? :!}) + $_ + } nil $_) + + (if { + (send (send $_ :nil?) :!) + $_ + } $_ nil) } PATTERN - def_node_matcher :candidate_that_may_introduce_nil, <<-PATTERN - (and - {(send (send $_ :nil?) :!) $_} - {(send $_ $_ ...) (block (send $_ $_ ...) ...)} - ...) - PATTERN + def_node_matcher :not_nil_check?, '(send (send $_ :nil?) :!)' def on_if(node) - return if node.ternary? - + return if allowed_if_condition?(node) check_node(node) end @@ -84,45 +81,89 @@ def on_and(node) def check_node(node) return if target_ruby_version < 2.3 - return if allowed_if_condition?(node) checked_variable, receiver, method = extract_parts(node) return unless receiver == checked_variable - return if NIL_METHODS.include?(method) - return unless method =~ /\w+[=!?]?/ + return if unsafe_method?(method) add_offense(node) end + def autocorrect(node) + _check, body, = node.node_parts + _checked_variable, matching_receiver, = extract_parts(node) + method_call, = matching_receiver.parent + + lambda do |corrector| + corrector.remove(begin_range(node, body)) + corrector.remove(end_range(node, body)) + corrector.insert_before((method_call || body).loc.dot, '&') + end + end + + private + def allowed_if_condition?(node) - node.if_type? && (node.else? || node.elsif?) + node.else? || node.elsif? || node.ternary? end def extract_parts(node) - if cop_config['ConvertCodeThatCanStartToReturnNil'] - safe_navigation_candidate(node) || - candidate_that_may_introduce_nil(node) - else - safe_navigation_candidate(node) + case node.type + when :if + extract_parts_from_if(node) + when :and + extract_parts_from_and(node) end end - def autocorrect(node) - if node.if_type? - _check, body, = *node.node_parts - else - _check, body = *node + def extract_parts_from_if(node) + checked_variable, receiver = + modifier_if_safe_navigation_candidate?(node) + + matching_receiver = + find_matching_receiver_invocation(receiver, checked_variable) + + if matching_receiver + method = matching_receiver.parent.loc.selector.source end - method_call, = *body if body.block_type? + [checked_variable, matching_receiver, method] + end - lambda do |corrector| - corrector.remove(begin_range(node, body)) - corrector.remove(end_range(node, body)) - corrector.insert_before((method_call || body).loc.dot, '&') + def extract_parts_from_and(node) + checked_variable, rhs = *node + if cop_config['ConvertCodeThatCanStartToReturnNil'] + checked_variable = + not_nil_check?(checked_variable) || checked_variable + end + + matching_receiver = + find_matching_receiver_invocation(rhs, checked_variable) + + if matching_receiver + method = matching_receiver.parent.loc.selector.source end + + [checked_variable, matching_receiver, method] end - private + def find_matching_receiver_invocation(node, checked_variable) + return nil if node.nil? + + if node.block_type? + method_call, = *node + receiver, _method = *method_call + elsif node.send_type? + receiver, _method = *node + end + + return receiver if receiver == checked_variable + + find_matching_receiver_invocation(receiver, checked_variable) + end + + def unsafe_method?(method) + NIL_METHODS.include?(method) || method !~ /\w+[=!?]?/ + end def begin_range(node, method_call) Parser::Source::Range.new(node.loc.expression.source_buffer, diff --git a/spec/rubocop/cop/style/safe_navigation_spec.rb b/spec/rubocop/cop/style/safe_navigation_spec.rb index 687dd8d696aa..8e7bccd697b5 100644 --- a/spec/rubocop/cop/style/safe_navigation_spec.rb +++ b/spec/rubocop/cop/style/safe_navigation_spec.rb @@ -29,6 +29,12 @@ expect(cop.offenses).to be_empty end + it 'allows an object check before a method calls that nil responds to ' do + inspect_source('foo && foo.to_i') + + expect(cop.offenses).to be_empty + end + it 'allows method calls that do not get called using . safe guarded by ' \ 'an object check' do inspect_source('foo + bar if foo') @@ -49,6 +55,15 @@ expect(cop.offenses).to be_empty end + it 'allows a method call as a parameter when the parameter is ' \ + 'safe guarded with an object check' do + inspect_source(<<-RUBY.strip_indent) + foo(bar.baz) if bar + RUBY + + expect(cop.offenses).to be_empty + end + shared_examples 'all variable types' do |variable| context 'modifier if' do it 'registers an offense for a method call on an accessor ' \ @@ -179,6 +194,15 @@ expect(cop.messages).to eq([message]) end + + it 'registers an offense for a chained method call safeguarded ' \ + 'with a negative nil check for the object' do + inspect_source(<<-RUBY.strip_indent) + #{variable}.one.two(baz) { |e| e.qux } if !#{variable}.nil? + RUBY + + expect(cop.messages).to eq([message]) + end end context 'if expression' do @@ -226,7 +250,7 @@ expect(cop.messages).to eq([message]) end - it 'accepts a single method call inside of a check for the object ' \ + it 'allows a single method call inside of a check for the object ' \ 'with an else' do inspect_source(<<-RUBY.strip_indent) if #{variable} @@ -240,7 +264,7 @@ end context 'ternary expression' do - it 'accepts ternary expression' do + it 'allows ternary expression' do source = "!#{variable}.nil? ? #{variable}.bar : something" inspect_source(source) @@ -325,25 +349,46 @@ expect(cop.messages).to eq([message]) end + + context 'method chaining' do + it 'registers an offense for an object check followed by ' \ + 'chained method calls' do + inspect_source(<<-RUBY.strip_indent) + #{variable} && #{variable}.one.two.three(baz) { |e| e.qux } + RUBY + + expect(cop.messages).to eq([message]) + end + + it 'registers an offense for an object check followed by a ' \ + 'chained method calls with blocks' do + inspect_source(<<-RUBY.strip_indent) + #{variable} && #{variable}.one { |a| b}.two(baz) { |e| e.qux } + RUBY + + expect(cop.messages).to eq([message]) + end + end end context 'ConvertCodeThatCanStartToReturnNil false' do - it 'registers an offense for a non-nil object check followed by a ' \ - 'method call' do + let(:cop_config) { { 'ConvertCodeThatCanStartToReturnNil' => false } } + + it 'allows a non-nil object check followed by a method call' do inspect_source("!#{variable}.nil? && #{variable}.bar") expect(cop.offenses).to be_empty end - it 'registers an offense for a non-nil object check followed by a ' \ - 'method call with params' do + it 'allows a non-nil object check followed by a method call ' \ + 'with params' do inspect_source("!#{variable}.nil? && #{variable}.bar(baz)") expect(cop.offenses).to be_empty end - it 'registers an offense for a non-nil object check followed by a ' \ - 'method call with a block' do + it 'allows a non-nil object check followed by a method call ' \ + 'with a block' do source = "!#{variable}.nil? && #{variable}.bar { |e| e.qux }" inspect_source(source) @@ -351,8 +396,8 @@ expect(cop.offenses).to be_empty end - it 'registers an offense for a non-nil object check followed by a ' \ - 'method call with params and a block' do + it 'allows a non-nil object check followed by a method call ' \ + 'with params and a block' do source = "!#{variable}.nil? && #{variable}.bar(baz) { |e| e.qux }" inspect_source(source) @@ -360,45 +405,45 @@ expect(cop.offenses).to be_empty end - it 'registers an offense for an object check followed by a ' \ - 'method call' do + it 'registers an offense for an object check followed by ' \ + 'a method call' do inspect_source("#{variable} && #{variable}.bar") - expect(cop.offenses).to be_empty + expect(cop.messages).to eq([message]) end - it 'registers an offense for an object check followed by a ' \ - 'method call with params' do + it 'registers an offense for an object check followed by ' \ + 'a method call with params' do inspect_source("#{variable} && #{variable}.bar(baz)") - expect(cop.offenses).to be_empty + expect(cop.messages).to eq([message]) end - it 'registers an offense for an object check followed by a ' \ - 'method call with a block' do + it 'registers an offense for an object check followed by ' \ + 'a method call with a block' do inspect_source("#{variable} && #{variable}.bar { |e| e.qux }") - expect(cop.offenses).to be_empty + expect(cop.messages).to eq([message]) end - it 'registers an offense for an object check followed by a ' \ - 'method call with params and a block' do + it 'registers an offense for an object check followed by ' \ + 'a method call with params and a block' do source = "#{variable} && #{variable}.bar(baz) { |e| e.qux }" inspect_source(source) - expect(cop.offenses).to be_empty + expect(cop.messages).to eq([message]) end - it 'registers an offense for a check for the object followed by a ' \ - 'method call in the condition for an if expression' do + it 'registers an offense for a check for the object followed by ' \ + 'a method call in the condition for an if expression' do inspect_source(<<-RUBY.strip_indent) if #{variable} && #{variable}.bar something end RUBY - expect(cop.offenses).to be_empty + expect(cop.messages).to eq([message]) end end @@ -656,6 +701,39 @@ expect(new_source).to eq("#{variable}[1]&.bar") end + + it 'corrects a chained method call safeguarded ' \ + 'with a negative nil check for the object' do + new_source = autocorrect_source(<<-RUBY.strip_indent) + #{variable}.one.two(baz) { |e| e.qux } if !#{variable}.nil? + RUBY + + expect(new_source).to eq(<<-RUBY.strip_indent) + #{variable}&.one.two(baz) { |e| e.qux } + RUBY + end + + it 'corrects a chained method call safeguarded ' \ + 'with a check for the object' do + new_source = autocorrect_source(<<-RUBY.strip_indent) + #{variable}.one.two(baz) { |e| e.qux } if #{variable} + RUBY + + expect(new_source).to eq(<<-RUBY.strip_indent) + #{variable}&.one.two(baz) { |e| e.qux } + RUBY + end + + it 'corrects a chained method call safeguarded ' \ + 'with an unless nil check for the object' do + new_source = autocorrect_source(<<-RUBY.strip_indent) + #{variable}.one.two(baz) { |e| e.qux } unless #{variable}.nil? + RUBY + + expect(new_source).to eq(<<-RUBY.strip_indent) + #{variable}&.one.two(baz) { |e| e.qux } + RUBY + end end context 'if expression' do @@ -928,6 +1006,40 @@ expect(new_source).to eq("#{variable}&.bar && something") end end + + context 'method chaining' do + it 'corrects an object check followed by a chained method call' do + new_source = autocorrect_source(<<-RUBY.strip_indent) + #{variable} && #{variable}.one.two(baz) { |e| e.qux } + RUBY + + expect(new_source).to eq(<<-RUBY.strip_indent) + #{variable}&.one.two(baz) { |e| e.qux } + RUBY + end + + it 'corrects an object check followed by ' \ + 'multiple chained method call' do + new_source = autocorrect_source(<<-RUBY.strip_indent) + #{variable} && #{variable}.one.two.three(baz) { |e| e.qux } + RUBY + + expect(new_source).to eq(<<-RUBY.strip_indent) + #{variable}&.one.two.three(baz) { |e| e.qux } + RUBY + end + + it 'corrects an object check followed by ' \ + 'multiple chained method calls with blocks' do + new_source = autocorrect_source(<<-RUBY.strip_indent) + #{variable} && #{variable}.one { |a| b}.two(baz) { |e| e.qux } + RUBY + + expect(new_source).to eq(<<-RUBY.strip_indent) + #{variable}&.one { |a| b}.two(baz) { |e| e.qux } + RUBY + end + end end end