[Fix rubocop#3855] Make Lint/NonLocalExitFromIterator cop aware of …

…method definitions

This cop would report an offense if it found a `return` statement inside
a method definition inside a block, even though the `return` is scoped
to the method definition. This change fixes that.

CHANGELOG.md

@@ -13,6 +13,7 @@
 ### Bug fixes
 
 * [#3923](https://github.com/bbatsov/rubocop/issues/3923): Allow asciibetical sorting in `Bundler/OrderedGems`. ([@mikegee][])
+* [#3855](https://github.com/bbatsov/rubocop/issues/3855): Make `Lint/NonLocalExitFromIterator` aware of method definitions. ([@drenmi][])
 
 ## 0.47.1 (2017-01-18)
 

lib/rubocop/cop/lint/non_local_exit_from_iterator.rb

@@ -3,11 +3,16 @@
 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, the block is preceded by a method chain, the block has arguments,
- # and the method which receives the block is not `define_method`
- # or `define_singleton_method`.
+ # This cop checks for non-local exits from iterators without a return
+ # value. It registers an offense under these conditions:
+ #
+ # - No value is returned,
+ # - the block is preceded by a method chain,
+ # - the block has arguments,
+ # - the method which receives the block is not `define_method`
+ # or `define_singleton_method`,
+ # - the return is not contained in an inner scope, e.g. a lambda or a
+ # method definition.
  #
  # @example
  #
@@ -40,37 +45,40 @@ class NonLocalExitFromIterator < Cop
 
  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
 
- # `return` does not exit to outside of lambda block, this is safe.
- break if block_node.lambda?
+ return_node.each_ancestor(:block, :def, :defs) do |node|
+ break if scoped_node?(node)
+
+ send_node, args_node, _body_node = *node
+
  # if a proc is passed to `Module#define_method` or
  # `Object#define_singleton_method`, `return` will not cause a
  # non-local exit error
  break if define_method?(send_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
+ private
 
- def chained_send?(send_node)
- receiver_node, _selector_node = *send_node
- !receiver_node.nil?
+ def scoped_node?(node)
+ node.def_type? || node.defs_type? || node.lambda?
  end
 
- def define_method?(send_node)
- _receiver, selector = *send_node
- %i(define_method define_singleton_method).include? selector
+ def return_value?(return_node)
+ !return_node.children.empty?
  end
+
+ def_node_matcher :chained_send?, '(send !nil ...)'
+ def_node_matcher :define_method?, <<-PATTERN
+ (send _ {:define_method :define_singleton_method} _)
+ PATTERN
  end
  end
  end

manual/cops_lint.md

@@ -1186,11 +1186,16 @@ Enabled by default | Supports autocorrection
 --- | ---
 Enabled | No
 
-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, the block is preceded by a method chain, the block has arguments,
-and the method which receives the block is not `define_method`
-or `define_singleton_method`.
+This cop checks for non-local exits from iterators without a return
+value. It registers an offense under these conditions:
+
+ - No value is returned,
+ - the block is preceded by a method chain,
+ - the block has arguments,
+ - the method which receives the block is not `define_method`
+ or `define_singleton_method`,
+ - the return is not contained in an inner scope, e.g. a lambda or a
+ method definition.
 
 ### Example
 

spec/rubocop/cop/lint/non_local_exit_from_iterator_spec.rb

@@ -174,5 +174,31 @@ def find_first_sold_out_item(items)
 
  it { expect(cop.offenses).to be_empty }
  end
+
+ context 'when the return is within a nested method definition' do
+ context 'with an instance method definition' do
+ let(:source) { <<-END }
+ Foo.configure do |c|
+ def bar
+ return if baz?
+ end
+ end
+ END
+
+ it { expect(cop.offenses).to be_empty }
+ end
+
+ context 'with a class method definition' do
+ let(:source) { <<-END }
+ Foo.configure do |c|
+ def self.bar
+ return if baz?
+ end
+ end
+ END
+
+ it { expect(cop.offenses).to be_empty }
+ end
+ end
  end
 end