-a flag rewritten code incorrectly

[laplacesdemon]

I have an interesting problem. Rubocop has rewritten my code incorrectly. The piece of code reduces a list to an object using inject. I'm not a ruby master, in fact I rarely code in ruby and probably there's a better way to achieve what I was doing. But the resulting code after running rubocop -a did a different thing than the original one. See below:

Original code:

operations.map { |o| wrap_error(&o) }.inject(Maybe.new) do |maybe, obj|
  if obj.is_a? StandardError then maybe.add_error(obj) else maybe.add_value(obj) end
  maybe
end

Expected behavior

The code behavior should not change.

Actual behavior

operations.map { |o| wrap_error(&o) }.each_with_object(Maybe.new) do |obj, maybe|
  obj.is_a? StandardError ? maybe.add_error(obj) : maybe.add_value(obj)
end

here, inject became each_with_object which does a different thing.

Steps to reproduce the problem

rubocop -a <filename.rb>

RuboCop version

$ rubocop -V
0.55.0 (using Parser 2.5.1.0, running on ruby 2.3.3 x86_64-darwin17)

[mikegee]

You said The code behavior should not change., but I can't tell which behavior changed by looking at it. It looks like Rubocop's version should behave the same.

Can you be more specific about the how it changed the code's behavior?

[laplacesdemon]

Having a closer look at this, in fact the transformation to each_with_object is correct. I didn't know that api before, it's cool! The problem however is with the ternary operation.

Following line is rewritten:

a = 'test'
if a.is_a? StandardError then true else false end

into

a = 'test'
a.is_a? StandardError ? true : false

Which results in an error: is_a?': class or module required (TypeError). So the problem is (I think) using a question marked method in a ternary operation. The solution is to wrap the predicate in parenthesis:

a = 'test'
(a.is_a? StandardError) ? true : false

[jonas054]

@laplacesdemon Thanks for the clarification!

Some details for the person who fixes this bug: note that this is the output of an --auto-correct run.

file.rb:1:39: C: [Corrected] Style/EachWithObject: Use each_with_object instead of inject.
operations.map { |o| wrap_error(&o) }.inject(Maybe.new) do |maybe, obj|
                                      ^^^^^^
file.rb:1:81: C: Metrics/LineLength: Line is too long. [81/80]
operations.map { |o| wrap_error(&o) }.each_with_object(Maybe.new) do |obj, maybe|
                                                                                ^
file.rb:2:3: C: [Corrected] Style/OneLineConditional: Favor the ternary operator (?:) over if/then/else/end constructs.
  if obj.is_a? StandardError then maybe.add_error(obj) else maybe.add_value(obj) end
  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
file.rb:2:3: C: [Corrected] Style/TernaryParentheses: Omit parentheses for ternary conditions.
  (obj.is_a? StandardError) ? maybe.add_error(obj) : maybe.add_value(obj)
  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

So Style/OneLineConditional does the transformation from if/then/else/end into a ternary, and correctly adds parentheses to preserve the semantics. Then Style/TernaryParentheses messes things up by removing the parentheses.

I think the preferred syntax should be

obj.is_a?(StandardError) ? maybe.add_error(obj) : maybe.add_value(obj)

but I'm not sure which cop should be responsible.

[rrosenblum]

This sounds similar to other bugs that have been reported with Style/TernaryParentheses. See #4460

[laplacesdemon]

Indeed it’s the same issue. Feel free to dupe it.