False positive of UnmodifiedReduceAccumulator for self

[AlexWayfer]

I'm working on R18n gem and I've got a new offense from RuboCop.

---

Expected behavior

No offenses.

Actual behavior

The offense:

r18n-core/lib/r18n-core/translation.rb:171:41: W: Lint/UnmodifiedReduceAccumulator: Do not return an element of the accumulator in reduce.
      keys.reduce(self) { |result, key| result[key] }
                                        ^^^^^^^^^^^

Steps to reproduce the problem

# Custom class with `#[]` method
def dig(*keys)
  keys.reduce(self) { |result, key| result[key] }
end

RuboCop version

$ [bundle exec] rubocop -V
1.3.1 (using Parser 2.7.2.0, rubocop-ast 1.1.1, running on ruby 2.7.2 x86_64-linux)

[marcandre]

Interesting false positive; there are others too, which is why the cop is "Safe: false".

I don't know if it's worth addressing it.

FWIW, I think your implementation is not completely correct:

1. nil should interrupt the digging
2. you should call digon your result, not [] (unless you are absolutely sure that the intermediary results are all of your own class)

# Custom class with `#[]` method
def dig(key, *other_keys)
  result = self[key]
  result = result&.dig(*other_keys) unless other_keys.empty?
  result
end

[AlexWayfer]

1. nil should interrupt the digging

No: for untranslated string (nonexistent keys) there are instances of Untranslated class.

2. you should call digon your result, not [] (unless you are absolutely sure that the intermediary results are all of your own class)

Yes, [] always returns our own classes.

Your code seems more complex.

[marcandre]

1. nil should interrupt the digging

No: for untranslated string (nonexistent keys) there are instances of Untranslated class.

You meant "Yes, but they are never nil" 😆

2. you should call digon your result, not [] (unless you are absolutely sure that the intermediary results are all of your own class)

Yes, [] always returns our own classes.

👍

@dvandersluis how do you feel about detecting this use case?

[dvandersluis]

So the idea as @marcandre mentioned was that we can't cover all cases, and it was decided in #8949 to flag index returns. However, I can add an exception if the method argument is self I guess?

[marcandre]

So the idea as @marcandre mentioned was that we can't cover all cases, and it was decided in #8949 to flag index returns. However, I can add an exception if the method argument is self I guess?

I don't think the argument matters, only the form of the block. If the block is { |result, key| result.send <anything>, key }, then it's fine:

%i[a b c].inject({a: {b: {c: 42}}}) { |result, key| result[key] } # => 42

That corresponds to the non-block form too, @AlexWayfer you may want to simplify it:

def dig(*keys)
  keys.reduce(self, :[])
end

[dvandersluis]

Oh yeah, good call @marcandre I'll fix that up.

[AlexWayfer]

That corresponds to the non-block form too, @AlexWayfer you may want to simplify it:

def dig(*keys)
  keys.reduce(self, :[])
end

Thank you, didn't know about this.