Merge pull request rubocop#1728 from ypresto/return-from-iteration-block

Add NonLocalExitFromIterator lint and fix other cops complained by it

CHANGELOG.md

@@ -23,6 +23,7 @@
 * New cop `Performance/ReverseEach` to convert `reverse.each` to `reverse_each`. ([@rrosenblum][])
 * [#1281](https://github.com/bbatsov/rubocop/issues/1281): `IfUnlessModifier` cop does auto-correction. ([@lumeet][])
 * New cop `Performance/Detect` to detect usage of `select.first`, `select.last`, `find_all.first`, and `find_all.last` and convert them to use `detect` instead. ([@palkan][], [@rrosenblum][])
+* [#1728](https://github.com/bbatsov/rubocop/pull/1728): New cop `NonLocalExitFromIterator` checks for misused `return` in block. ([@ypresto][])
 
 ### Bugs fixed
 
@@ -38,6 +39,7 @@
 * [#1579](https://github.com/bbatsov/rubocop/issues/1579): Fix handling of similar-looking blocks in `BlockAlignment`. ([@lumeet][])
 * [#1676](https://github.com/bbatsov/rubocop/pull/1676): Fix auto-correct in `Lambda` when a new multi-line lambda is used as an argument. ([@lumeet][])
 * [#1656](https://github.com/bbatsov/rubocop/issues/1656): Fix bug that would include hidden directories implicitly. ([@jonas054][])
+* [#1728](https://github.com/bbatsov/rubocop/pull/1728): Fix bug in `LiteralInInterpolation` and `AssignmentInCondition`. ([@ypresto][])
 
 ### Changes
 
@@ -1310,3 +1312,4 @@
 [@zvkemp]: https://github.com/zvkemp
 [@vassilevsky]: https://github.com/vassilevsky
 [@gerry3]: https://github.com/gerry3
+[@ypresto]: https://github.com/ypresto

config/enabled.yml

@@ -891,6 +891,10 @@ Lint/Loop:
   StyleGuide: 'https://github.com/bbatsov/ruby-style-guide#loop-with-break'
   Enabled: true
 
+Lint/NonLocalExitFromIterator:
+  Description: 'Do not use return in iterator to cause non-local exit.'
+  Enabled: true
+
 Lint/ParenthesesAsGroupedExpression:
   Description: >-
                  Checks for method calls with a space before the opening

lib/rubocop.rb

@@ -87,6 +87,7 @@
 require 'rubocop/cop/lint/literal_in_condition'
 require 'rubocop/cop/lint/literal_in_interpolation'
 require 'rubocop/cop/lint/loop'
+require 'rubocop/cop/lint/non_local_exit_from_iterator'
 require 'rubocop/cop/lint/parentheses_as_grouped_expression'
 require 'rubocop/cop/lint/require_parentheses'
 require 'rubocop/cop/lint/rescue_exception'

lib/rubocop/cop/lint/assignment_in_condition.rb

@@ -9,6 +9,7 @@ class AssignmentInCondition < Cop
         include SafeAssignment
 
         MSG = 'Assignment in condition - you probably meant to use `==`.'
+        ASGN_TYPES = [:begin, *EQUALS_ASGN_NODES, :send]
 
         def on_if(node)
           check(node)
@@ -29,22 +30,33 @@ def check(node)
 
           # assignments inside blocks are not what we're looking for
           return if condition.type == :block
-
-          condition.each_node(:begin, *EQUALS_ASGN_NODES, :send) do |asgn_node|
+          traverse_node(condition, ASGN_TYPES) do |asgn_node|
             if asgn_node.type == :send
               _receiver, method_name, *_args = *asgn_node
-              return if method_name != :[]=
+              next :skip_children if method_name != :[]=
             end
 
             # skip safe assignment nodes if safe assignment is allowed
-            return if safe_assignment_allowed? && safe_assignment?(asgn_node)
+            if safe_assignment_allowed? && safe_assignment?(asgn_node)
+              next :skip_children
+            end
 
             # assignment nodes from shorthand ops like ||= don't have operator
             if asgn_node.type != :begin && asgn_node.loc.operator
               add_offense(asgn_node, :operator)
             end
           end
         end
+
+        # each_node/visit_descendants_with_types with :skip_children
+        def traverse_node(node, types, &block)
+          result = yield node if types.include?(node.type)
+          # return to skip all descendant nodes
+          return if result == :skip_children
+          node.children.each do |child|
+            traverse_node(child, types, &block) if child.is_a?(Astrolabe::Node)
+          end
+        end
       end
     end
   end

lib/rubocop/cop/lint/literal_in_interpolation.rb

@@ -19,7 +19,7 @@ def on_dstr(node)
             final_node = begin_node.children.last
             next unless final_node
             # handle strings like __FILE__
-            return if special_string?(final_node)
+            next if special_string?(final_node)
             next unless LITERALS.include?(final_node.type)
 
             add_offense(final_node, :expression)

lib/rubocop/cop/lint/non_local_exit_from_iterator.rb

@@ -0,0 +1,61 @@
+# encoding: utf-8
+
+module RuboCop
+  module Cop
+    module Lint
+      # This cop checks for non-local exit from iterator, without return value.
+      # It warns only when satisfies all of these: `return` doesn't have return
+      # value, block followed by method chain, and block have arguments.
+      #
+      # @example
+      #
+      #   class ItemApi
+      #     rescue_from ValidationError do |e| # non-iteration block with arg
+      #       return message: 'validation error' unless e.errors # allowd
+      #       error_array = e.errors.map do |error| # block with method chain
+      #         return if error.suppress? # warned
+      #         return "#{error.param}: invalid" unless error.message # allowed
+      #         "#{error.param}: #{error.message}"
+      #       end
+      #       message: 'validation error', errors: error_array
+      #     end
+      #
+      #     def update_items
+      #       transaction do # block without arguments
+      #         return unless update_necessary? # allowed
+      #         find_each do |item| # block without method chain
+      #           return if item.stock == 0 # false-negative...
+      #           item.update!(foobar: true)
+      #         end
+      #       end
+      #     end
+      #   end
+      #
+      class NonLocalExitFromIterator < Cop
+        MSG = 'Non-local exit from iterator, without return value. ' \
+          '`next`, `break`, `Array#find`, `Array#any?`, etc. is preferred.'
+
+        def on_return(return_node)
+          return if return_value?(return_node)
+          return_node.each_ancestor(:block) do |block_node|
+            send_node, args_node, _body_node = *block_node
+            next if args_node.children.empty?
+            if chained_send?(send_node)
+              add_offense(return_node, :keyword)
+              break
+            end
+          end
+        end
+
+        def return_value?(return_node)
+          !return_node.children.empty?
+        end
+
+        def chained_send?(send_node)
+          receiver_node, _selector_node = *send_node
+          !receiver_node.nil?
+        end
+      end
+    end
+  end
+end

lib/rubocop/cop/style/unneeded_capital_w.rb

@@ -17,13 +17,11 @@ def on_array(node)
         private
 
         def on_percent_literal(node)
-          node.children.each do |string|
-            if string.type == :dstr ||
-               string.loc.expression.source =~ StringHelp::ESCAPED_CHAR_REGEXP
-              return
-            end
+          requires_interpolation = node.children.any? do |string|
+            string.type == :dstr ||
+            string.loc.expression.source =~ StringHelp::ESCAPED_CHAR_REGEXP
           end
-          add_offense(node, :expression)
+          add_offense(node, :expression) unless requires_interpolation
         end
 
         def autocorrect(node)

spec/rubocop/cop/lint/assignment_in_condition_spec.rb

@@ -78,6 +78,14 @@
     expect(cop.offenses).to be_empty
   end
 
+  it 'registers an offense for assignment after == in condition' do
+    inspect_source(cop,
+                   ['if test == 10 || foobar = 1',
+                    'end'
+                   ])
+    expect(cop.offenses.size).to eq(1)
+  end
+
   it 'accepts = in a block that is called in a condition' do
     inspect_source(cop,
                    'return 1 if any_errors? { o = inspect(file) }')
@@ -90,6 +98,12 @@
     expect(cop.offenses).to be_empty
   end
 
+  it 'registers an offense for assignment after ||= in condition' do
+    inspect_source(cop,
+                   'raise StandardError unless (foo ||= bar) || a = b')
+    expect(cop.offenses.size).to eq(1)
+  end
+
   context 'safe assignment is allowed' do
     it 'accepts = in condition surrounded with braces' do
       inspect_source(cop,

spec/rubocop/cop/lint/literal_in_interpolation_spec.rb

@@ -28,4 +28,10 @@
     inspect_source(cop, '"this is #{__FILE__} silly"')
     expect(cop.offenses).to be_empty
   end
+
+  it 'registers an offense for interpolation after __FILE__' do
+    inspect_source(cop,
+                   '"this is the #{__FILE__} #{1}"')
+    expect(cop.offenses.size).to eq(1)
+  end
 end

spec/rubocop/cop/lint/non_local_exit_from_iterator_spec.rb

@@ -0,0 +1,132 @@
+# encoding: utf-8
+
+require 'spec_helper'
+
+describe RuboCop::Cop::Lint::NonLocalExitFromIterator do
+  subject(:cop) { described_class.new }
+
+  context 'inspection' do
+    before do
+      inspect_source(cop, source)
+    end
+
+    let(:message) do
+      'Non-local exit from iterator, without return value. ' \
+        '`next`, `break`, `Array#find`, `Array#any?`, etc. is preferred.'
+    end
+
+    shared_examples_for 'offense detector' do
+      it 'registers an offense' do
+        expect(cop.offenses.size).to eq(1)
+        expect(cop.offenses.first.message).to eq(message)
+        expect(cop.offenses.first.severity.name).to eq(:warning)
+        expect(cop.highlights).to eq(['return'])
+      end
+    end
+
+    context 'when block is followed by method chain' do
+      context 'and has single argument' do
+        let(:source) { <<-END }
+          items.each do |item|
+            return if item.stock == 0
+            item.update!(foobar: true)
+          end
+        END
+
+        it_behaves_like('offense detector')
+        it { expect(cop.offenses.first.line).to eq(2) }
+      end
+
+      context 'and has multiple arguments' do
+        let(:source) { <<-END }
+          items.each_with_index do |item, i|
+            return if item.stock == 0
+            item.update!(foobar: true)
+          end
+        END
+
+        it_behaves_like('offense detector')
+        it { expect(cop.offenses.first.line).to eq(2) }
+      end
+
+      context 'and has no argument' do
+        let(:source) { <<-END }
+          item.with_lock do
+            return if item.stock == 0
+            item.update!(foobar: true)
+          end
+        END
+
+        it { expect(cop.offenses).to be_empty }
+      end
+    end
+
+    context 'when block is not followed by method chain' do
+      let(:source) { <<-END }
+        transaction do
+          return unless update_necessary?
+          find_each do |item|
+            return if item.stock == 0 # false-negative...
+            item.update!(foobar: true)
+          end
+        end
+      END
+
+      it { expect(cop.offenses).to be_empty }
+    end
+
+    context 'when block is lambda' do
+      let(:source) { <<-END }
+        items.each(lambda do |item|
+          return if item.stock == 0
+          item.update!(foobar: true)
+        end)
+        items.each -> (item) {
+          return if item.stock == 0
+          item.update!(foobar: true)
+        }
+      END
+
+      it { expect(cop.offenses).to be_empty }
+    end
+
+    context 'when block in middle of nest is followed by method chain' do
+      let(:source) { <<-END }
+        transaction do
+          return unless update_necessary?
+          items.each do |item|
+            return if item.nil?
+            item.with_lock do
+              return if item.stock == 0
+              item.very_complicated_update_operation!
+            end
+          end
+        end
+      END
+
+      it 'registers offenses' do
+        expect(cop.offenses.size).to eq(2)
+        expect(cop.offenses[0].message).to eq(message)
+        expect(cop.offenses[0].severity.name).to eq(:warning)
+        expect(cop.offenses[0].line).to eq(4)
+        expect(cop.offenses[1].message).to eq(message)
+        expect(cop.offenses[1].severity.name).to eq(:warning)
+        expect(cop.offenses[1].line).to eq(6)
+        expect(cop.highlights).to eq(%w(return return))
+      end
+    end
+
+    context 'when return with value' do
+      let(:source) { <<-END }
+        def find_first_sold_out_item(items)
+          items.each do |item|
+            return item if item.stock == 0
+            item.foobar!
+          end
+        end
+      END
+
+      it { expect(cop.offenses).to be_empty }
+    end
+  end
+end