[Fix #6969] Fix a false positive in `Style/InverseMethods` #7016

dduugg · 2019-05-03T03:54:57Z

Style/InverseMethods does not contemplate guard clauses or other jumps within block methods. This PR updates the cop to skip further evaluation in such cases.

It was sufficient to consider next nodes in testing this over a large codebase, but it also guards agains break and return nodes, out of an abundance of caution.

The contains_jump? implementation feels suboptimal, and I'd like to know if there's a better approach. I could neither find an existing helper to look for node types within an AST, nor find a valid def_node_matcher that would do the trick.

Resolves #6969

Before submitting the PR make sure the following are checked:

Wrote good commit messages.
Commit message starts with [Fix #issue-number] (if the related issue exists).
Feature branch is up-to-date with master (if not - rebase it).
Squashed related commits together.
Added tests.
Added an entry to the Changelog if the new code introduces user-observable changes. See changelog entry format.
The PR relates to only one subject with a clear title
and description in grammatically correct, complete sentences.
Run bundle exec rake default. It executes all tests and RuboCop for itself, and generates the documentation.

Darhazer · 2019-05-03T09:58:17Z

lib/rubocop/cop/style/inverse_methods.rb

@@ -175,6 +178,15 @@ def camel_case_constant?(node)
        def dot_range(loc)
          range_between(loc.dot.begin_pos, loc.expression.end_pos)
        end
+
+        def contains_jump?(node)
+          return false unless node.is_a?(RuboCop::AST::Node)


one-liner:
node.each_node(:return, :break, :next).any?
each_node starts from the receiver (node) and recursively visits child nodes, with optional filtering by types (which is what you are checking), and yields only nodes, so no need to check for the class.

dduugg · 2019-05-03T15:03:04Z

CHANGELOG.md

@@ -23,7 +24,7 @@
 * [#6992](https://github.com/rubocop-hq/rubocop/pull/6992): Fix unknown default configuration for `Layout/IndentFirstParameter` cop. ([@drenmi][])
 * [#6972](https://github.com/rubocop-hq/rubocop/issues/6972): Fix a false positive for `Style/MixinUsage` when using inside block and `if` condition is after `include`. ([@koic][])
 * [#6738](https://github.com/rubocop-hq/rubocop/issues/6738): Prevent auto-correct conflict of `Style/Next` and `Style/SafeNavigation`. ([@hoshinotsuyoshi][])
-* [#6847](https://github.com/rubocop-hq/rubocop/pull/6847): Fix `Style/BlockDelimiters` to properly check if the node is chaned when `braces_for_chaining` is set. ([@att14][])
+* [#6847](https://github.com/rubocop-hq/rubocop/pull/6847): Fix `Style/BlockDelimiters` to properly check if the node is chained when `braces_for_chaining` is set. ([@att14][])


(unrelated typo fix)

Drenmi

Thanks! Changes look good to me, but if break an return are also covered, I'd like to see this documented with unit tests. 🙂

🚀

dduugg · 2019-05-04T14:59:39Z

@Drenmi thanks for the suggestion. I've withdrawn skipping blocks with return/break. The only default block methods are select/reject (and their ! counterparts), and i've convinced myself that this cop doesn't break when paired with these other jump methods.

rrosenblum · 2019-05-06T18:25:00Z

lib/rubocop/cop/style/inverse_methods.rb

@@ -76,6 +78,7 @@ def on_block(node)
          inverse_block?(node) do |_method_call, method, block|
            return unless inverse_blocks.key?(method)
            return if negated?(node) && negated?(node.parent)
+            return if node.each_node(:next).any?


It was sufficient to consider next nodes in testing this over a large codebase, but it also guards agains break and return nodes, out of an abundance of caution.

I don't see any code in here that would fix this issue for break or return. I think this needs to look for those node types as well.

@rrosenblum it's been reverted after further discussion/consideration. could i trouble you for an example of an issue caused by break or return?

I think I missed something from an earlier conversation. It sounds like you had accounted for this and then reverted it. The comment that I quoted made it seem like we needed to account for break and return. These are likely edge cases because break and return don't make much sense inside of methods that are designed to have a return.

rrosenblum · 2019-05-06T18:28:17Z

lib/rubocop/cop/style/inverse_methods.rb

@@ -28,6 +28,8 @@ module Style
      #   foo == bar
      #   !!('foo' =~ /^\w+$/)
      #   !(foo.class < Numeric) # Checking class hierarchy is allowed
+      #   # Blocks with guard clauses are ignored:
+      #   foo.select { |f| next if f.zero?; f != 1 }


We do not recommend using semicolons within code, and we like to have our code examples follow as many best practices as possible. Please change this example to a multi-line block.

# foo.select do |f| # next if f.zero? # f != 1 # end

rrosenblum · 2019-05-07T12:56:29Z

lib/rubocop/cop/style/inverse_methods.rb

@@ -76,6 +78,7 @@ def on_block(node)
          inverse_block?(node) do |_method_call, method, block|
            return unless inverse_blocks.key?(method)
            return if negated?(node) && negated?(node.parent)
+            return if node.each_node(:next).any?


I think I missed something from an earlier conversation. It sounds like you had accounted for this and then reverted it. The comment that I quoted made it seem like we needed to account for break and return. These are likely edge cases because break and return don't make much sense inside of methods that are designed to have a return.

rrosenblum · 2019-05-07T12:56:54Z

manual/cops_style.md

@@ -2658,6 +2658,8 @@ foo != bar
 foo == bar
 !!('foo' =~ /^\w+$/)
 !(foo.class < Numeric) # Checking class hierarchy is allowed
+# Blocks with guard clauses are ignored:
+foo.select { |f| next if f.zero?; f != 1 }


You'll need to regenerate the documentation to update this.

d'oh, done!

koic · 2019-05-10T02:21:45Z

spec/rubocop/cop/style/inverse_methods_spec.rb

@@ -50,6 +50,20 @@
    expect_no_offenses('!!foo.reject { |e| !e }')
  end

+  it 'allows an inverse method in a block with next' do
+    expect_no_offenses(<<-RUBY.strip_indent)


Could you rebase using latest master branch and change this line to the following code?

-expect_no_offenses(<<-RUBY.strip_indent) +expect_no_offenses(<<~RUBY)

…InverseMethods`

koic · 2019-05-10T03:01:36Z

Thank you!

Darhazer reviewed May 3, 2019

View reviewed changes

dduugg force-pushed the handle-jumps-in-inverse-methods branch 3 times, most recently from e77f497 to e2227c3 Compare May 3, 2019 15:02

dduugg commented May 3, 2019

View reviewed changes

dduugg force-pushed the handle-jumps-in-inverse-methods branch from e2227c3 to 157a6b0 Compare May 4, 2019 00:56

Drenmi approved these changes May 4, 2019

View reviewed changes

dduugg force-pushed the handle-jumps-in-inverse-methods branch from 157a6b0 to 29f7bed Compare May 4, 2019 15:00

rrosenblum suggested changes May 6, 2019

View reviewed changes

dduugg force-pushed the handle-jumps-in-inverse-methods branch 2 times, most recently from 87f7471 to 4593830 Compare May 7, 2019 02:51

rrosenblum reviewed May 7, 2019

View reviewed changes

dduugg force-pushed the handle-jumps-in-inverse-methods branch from 4593830 to 0cacc5c Compare May 7, 2019 13:11

rrosenblum approved these changes May 8, 2019

View reviewed changes

koic reviewed May 10, 2019

View reviewed changes

[Fix rubocop#6969] Fix a false positive with block methods in `Style/…

6406543

…InverseMethods`

dduugg force-pushed the handle-jumps-in-inverse-methods branch from 0cacc5c to 6406543 Compare May 10, 2019 02:51

koic merged commit 4555fe3 into rubocop:master May 10, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Fix #6969] Fix a false positive in `Style/InverseMethods` #7016

[Fix #6969] Fix a false positive in `Style/InverseMethods` #7016

dduugg commented May 3, 2019

Darhazer May 3, 2019

dduugg May 3, 2019

dduugg May 3, 2019

Drenmi left a comment

dduugg commented May 4, 2019

rrosenblum May 6, 2019

dduugg May 6, 2019

rrosenblum May 7, 2019

rrosenblum May 6, 2019

dduugg May 7, 2019

rrosenblum May 7, 2019

rrosenblum May 7, 2019

dduugg May 7, 2019

koic May 10, 2019 •

edited

dduugg May 10, 2019

koic commented May 10, 2019

[Fix #6969] Fix a false positive in Style/InverseMethods #7016

[Fix #6969] Fix a false positive in Style/InverseMethods #7016

Conversation

dduugg commented May 3, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Drenmi left a comment

Choose a reason for hiding this comment

dduugg commented May 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

koic May 10, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

koic commented May 10, 2019

[Fix #6969] Fix a false positive in `Style/InverseMethods` #7016

[Fix #6969] Fix a false positive in `Style/InverseMethods` #7016

koic May 10, 2019 •

edited