[Fix #8859] Add new `Lint/UnmodifiedReduceAccumulator` cop #8916

dvandersluis · 2020-10-21T20:24:33Z

Adds a cop to check for reduce or inject blocks that do not modify the accumulator. Closes #8859.

I tried to do this in the safest way, and avoid false positives as much as possible. The cop takes a look at all the return values from the block and makes some judgement on them (in order):

If the block returns the accumulator indexed (ie. reduce { |acc, el| acc[el] += 1 }), it'll always register an offense because that will likely cause exceptions to be thrown.
If the block returns the accumulator, or an expression containing it, in any branch, it is considered valid and no offenses will be registered.
If the block returns an expression containing the element and another value (or just other values), it is considered valid because we cannot determine for sure what that value is.
Otherwise, returns containing the element are registered as offenses.

I think I covered a lot of edge cases (running this on rubocop itself helped there), but please let me know if I've missed something, I'd like to try to avoid having a new cop immediately have a bunch of bug reports 😅

PS: I'm not 100% sold on the name and went back and forth for a while. Any suggests are appreciated.

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.

config/default.yml

dvandersluis · 2020-10-21T20:25:47Z

lib/rubocop/cop/lint/reduce_accumulator.rb

+        def_node_search :expression_values, <<~PATTERN
+          `{
+            (%RuboCop::AST::Node::VARIABLES $_)
+            (%RuboCop::AST::Node::EQUALS_ASSIGNMENTS $_ ...)
+            (send (%RuboCop::AST::Node::VARIABLES $_) :<< ...)
+            (send nil? $_)
+            (dstr (begin {(send nil? $_) (%RuboCop::AST::Node::VARIABLES $_)}))
+            (%RuboCop::AST::Node::SHORTHAND_ASSIGNMENTS (%RuboCop::AST::Node::EQUALS_ASSIGNMENTS $_) ...)
+          }
+        PATTERN


@marcandre I feel like you probably have a simpler way to do this 😁

lib/rubocop/cop/lint/reduce_accumulator.rb

bbatsov · 2020-10-22T11:59:40Z

I like the cop, but I find the name confusing. Perhaps something like UnusedReduceAccumulator? I'm totally open to better suggestions myself. :D

marcandre

I started a review, but I think this cop is too ambitious and it's in general not possible to do what it wants to do.
I fear that the risk of false positive is too high. Unless I'm mistaken, this would be a false positive, and it would be very difficult to distinguish:

enum.inject do |acc, elem|
  x = [*acc, elem]
  x << 42 if foo
  x
end

lib/rubocop/cop/lint/reduce_accumulator.rb

dvandersluis · 2020-10-22T14:06:59Z

@marcandre thanks for the feedback, your example actually won't trip the cop (I can add it as a test if you'd like, I have a whole section of does not register an offense if the return value cannot be determined tests already here).

I missed it in the description, but the cop only flags return values where the element is the only variable, for this exact reason. I didn't want the cop to make any assumptions here so if it's not clearly just returning the element, it does not register.

dvandersluis · 2020-10-22T14:11:24Z

I like the cop, but I find the name confusing. Perhaps something like UnusedReduceAccumulator? I'm totally open to better suggestions myself. :D

@bbatsov I agree it's not the best name. UnmodifiedReduceAccumulator?

dvandersluis · 2020-10-22T15:20:55Z

@bbatsov @marcandre I updated for your feedback so far. I've also renamed the cop to UnmodifiedReduceAccumulator for now, still happy to rename further if a better name comes up. I've also added more examples to the documentation.

bbatsov · 2020-10-23T07:43:01Z

I like the new name and I'm fine with merging the cop in its current state, unless @marcandre has some objections.

marcandre

Sorry I didn't have the time to reply earlier, and I do have objections.

It checks for two scenarios, both of which can yield false positives for perfectly valid code (in the review)

I think this should either be reverted (at least until it is revised, but again I think it is not possible to do so), or this cop should remain disabled by default.

marcandre · 2020-10-25T22:54:30Z

lib/rubocop/cop/lint/reduce_accumulator.rb

+      #   # ignored as the return value cannot be determined
+      #   enum.reduce do |acc, el|
+      #     x + y
+      #   end


Bad example, as it could be determined that this is an error. x = foo(acc, el); bar(x) would be a better example

marcandre · 2020-10-25T22:55:12Z

lib/rubocop/cop/lint/reduce_accumulator.rb

+      #   %w(a b c).reduce({}) do |acc, letter|
+      #     acc[letter] = true
+      #   end


Issue is in the word may.
There are very valid uses:

def chain(*values) values.inject({value: nil}) do |acc, elem| node = {value: elem, next: acc} acc[:prev] = node end end

I concede that that is a valid use that will be registered by the cop, but I'd argue that this is going to be much less prevalent than mistakenly forgetting to return the accumulator when setting a hash key.

I see a few options:

We can remove this behaviour (I think this isn't ideal, because I do think this is still a good check)

We can put it behind a conf variable that is not enabled by default

We can accept the false positives (if this was my code I'd just # rubocop:disable it, but maybe that's suboptimal).

What do we think? /cc @bbatsov

forgetting to return the accumulator when setting a hash key.

BTW this is a double error, as inject probably shouldn't be used in the first place in that case (see Style/EachWithObject)

Definitely true, but in this case Style/EachWithObject doesn't register (because the accumulator is not being returned). We definitely have other places where a piece of code is chain corrected by multiple cops so I don't see this as being an issue (except for it not being autocorrectable here I guess 😓 ).

marcandre · 2020-10-25T22:57:03Z

lib/rubocop/cop/lint/reduce_accumulator.rb

+      #   (1..4).reduce(0) do |acc, el|
+      #     el * 2
+      #   end


Bad example, acc is not used at all. There are already cops that deal with this, and Ruby too.

Even if acc was used, this could still lead to false negatives:

def nest(*values) values.inject([]) do |prev, elem| elem = [elem] unless elem.is_a?(Array) elem << prev elem end end

Bad example, acc is not used at all. There are already cops that deal with this, and Ruby too.

Do you mean Lint/UnusedBlockArgument here? While that's true it doesn't really convey the same thing that this cop does. (I don't see any other offenses for the example, so I think it's valid).

I think you meant false positives (false negatives are inherent here on purpose), but you're right that there is a class of false positives here that I did not consider. I'm looking at this now, thanks!

I don't see any other offenses for the example, so I think it's valid

specs only check for the current cop, you can't rely on that.

I think you meant false positives

Yes, sorry 😅

@marcandre no I meant I ran all of rubocop on the example code haha, it raised Lint/UnusedBlockArgument and this cop. 😁

lib/rubocop/cop/lint/reduce_accumulator.rb

marcandre · 2020-10-25T23:02:29Z

lib/rubocop/cop/lint/reduce_accumulator.rb

+            (%RuboCop::AST::Node::VARIABLES $_)
+            (%RuboCop::AST::Node::EQUALS_ASSIGNMENTS $_ ...)
+            (send (%RuboCop::AST::Node::VARIABLES $_) :<< ...)
+            (send nil? $_)


This mistakes the name of a method with the name of a variable

This is actually needed, because I'm looking for identifiers in general, not specifically variables. This is valid code and should not be flagged since we can't determine what method is from static analysis:

(1..4).reduce(0) do |acc, el| method + el end

I'm confused as to why there's nil? then. For example self.method + el, or el.method?

(BTW, please do not "resolve conversation" when there's no agreement)

You're right there shouldn't be!

(BTW, please do not "resolve conversation" when there's no agreement)

Sorry about that!

You're right there shouldn't be!

Ok, if you remove nil?, then el and self.el are treated the same way, even though they are not related?

Good call, I've fixed that now too. (btw I've been taking all your notes and applying them locally for the purpose of a new fix PR)

marcandre · 2020-10-25T23:02:35Z

lib/rubocop/cop/lint/reduce_accumulator.rb

+            (%RuboCop::AST::Node::EQUALS_ASSIGNMENTS $_ ...)
+            (send (%RuboCop::AST::Node::VARIABLES $_) :<< ...)
+            (send nil? $_)
+            (dstr (begin {(send nil? $_) (%RuboCop::AST::Node::VARIABLES $_)}))


Same here (first part of {})

dvandersluis · 2020-10-26T21:14:00Z

@marcandre I opened #8949 to continue this.

dvandersluis commented Oct 21, 2020

View reviewed changes

config/default.yml Outdated Show resolved Hide resolved

dvandersluis commented Oct 21, 2020

View reviewed changes

bbatsov reviewed Oct 22, 2020

View reviewed changes

lib/rubocop/cop/lint/reduce_accumulator.rb Outdated Show resolved Hide resolved

marcandre requested changes Oct 22, 2020

View reviewed changes

dvandersluis force-pushed the lint/reduce-return-accum branch from 681673e to 0294d53 Compare October 22, 2020 15:17

dvandersluis changed the title ~~[Fix #8859] Add new Lint/ReduceAccumulator cop~~ [Fix #8859] Add new Lint/UnmodifiedReduceAccumulator cop Oct 22, 2020

[Fix rubocop#8859] Add new Lint/UnmodifiedReduceAccumulator cop.

d4509a0

dvandersluis force-pushed the lint/reduce-return-accum branch from 0294d53 to d4509a0 Compare October 22, 2020 15:19

bbatsov approved these changes Oct 23, 2020

View reviewed changes

bbatsov merged commit 69a0284 into rubocop:master Oct 25, 2020

marcandre reviewed Oct 25, 2020

View reviewed changes

dvandersluis mentioned this pull request Oct 26, 2020

Fix issues with Lint/UnmodifiedReduceAccumulator #8949

Merged

8 tasks

dvandersluis deleted the lint/reduce-return-accum branch January 18, 2021 20:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Fix #8859] Add new `Lint/UnmodifiedReduceAccumulator` cop #8916

[Fix #8859] Add new `Lint/UnmodifiedReduceAccumulator` cop #8916

dvandersluis commented Oct 21, 2020 •

edited

dvandersluis Oct 21, 2020

bbatsov commented Oct 22, 2020

marcandre left a comment

dvandersluis commented Oct 22, 2020

dvandersluis commented Oct 22, 2020

dvandersluis commented Oct 22, 2020

bbatsov commented Oct 23, 2020

marcandre left a comment

marcandre Oct 25, 2020

marcandre Oct 25, 2020

dvandersluis Oct 26, 2020

marcandre Oct 26, 2020

dvandersluis Oct 26, 2020 •

edited

marcandre Oct 25, 2020

dvandersluis Oct 26, 2020

marcandre Oct 26, 2020

dvandersluis Oct 26, 2020 •

edited

marcandre Oct 25, 2020

dvandersluis Oct 26, 2020

marcandre Oct 26, 2020

marcandre Oct 26, 2020

dvandersluis Oct 26, 2020

dvandersluis Oct 26, 2020

marcandre Oct 26, 2020

dvandersluis Oct 26, 2020

marcandre Oct 25, 2020

dvandersluis commented Oct 26, 2020

[Fix #8859] Add new Lint/UnmodifiedReduceAccumulator cop #8916

[Fix #8859] Add new Lint/UnmodifiedReduceAccumulator cop #8916

Conversation

dvandersluis commented Oct 21, 2020 • edited

Choose a reason for hiding this comment

bbatsov commented Oct 22, 2020

marcandre left a comment

Choose a reason for hiding this comment

dvandersluis commented Oct 22, 2020

dvandersluis commented Oct 22, 2020

dvandersluis commented Oct 22, 2020

bbatsov commented Oct 23, 2020

marcandre left a 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

dvandersluis Oct 26, 2020 • edited

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

dvandersluis Oct 26, 2020 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dvandersluis commented Oct 26, 2020

[Fix #8859] Add new `Lint/UnmodifiedReduceAccumulator` cop #8916

[Fix #8859] Add new `Lint/UnmodifiedReduceAccumulator` cop #8916

dvandersluis commented Oct 21, 2020 •

edited

dvandersluis Oct 26, 2020 •

edited

dvandersluis Oct 26, 2020 •

edited