Style/GuardClause weird parsing problem

[madwort]

I've been experiencing weird behaviour on our codebase using the latest git version of rubocop, where --auto-gen-config does not detect cop offenses but subsequent runs of rubocop do. Here is the smallest reproducing piece of code I can create that triggers the bug with Style/GuardClause - there are several features in this code snippet that should be removed but if I remove any one of them I no longer trigger the bug (eg. the line >80 chars, the attribute in the string in the multi-line fail).

def test_func(my_var)
  if my_var != 'a'
    if my_var != 'b'
      if my_var != 'c'
        if my_var != 'd'
          fail "ERROR #{my_var}" \
                "ERROR #{my_var.part}"
        else
          puts 'ok wtf'
        end
      end
    end
  end
  fail 'we only see the issue if this line is too long (more than 80 chars)' if my_var == 'blah'
end

And here is the weird behaviour:

$ bundle exec rubocop test.rb --auto-gen-config ; bundle exec rubocop test.rb
warning: parser/current is loading parser/ruby22, which recognizes
warning: 2.2.4-compliant syntax, but you are running 2.2.2.
warning: please see https://github.com/whitequark/parser#compatibility-with-ruby-mri.
Inspecting 1 file
C

Offenses:

test.rb:1:1: C: Method has too many lines. [13/10]
def test_func(my_var)
^^^
test.rb:5:9: C: Avoid more than 3 levels of block nesting.
        if my_var != 'd'
        ^^^^^^^^^^^^^^^^
test.rb:14:81: C: Line is too long. [96/80]
  fail 'we only see the issue if this line is too long (more than 80 chars)' if my_var == 'blah'
                                                                                ^^^^^^^^^^^^^^^^

1 file inspected, 3 offenses detected
Created .rubocop_todo.yml.
Run `rubocop --config .rubocop_todo.yml`, or
add inherit_from: .rubocop_todo.yml in a .rubocop.yml file.
warning: parser/current is loading parser/ruby22, which recognizes
warning: 2.2.4-compliant syntax, but you are running 2.2.2.
warning: please see https://github.com/whitequark/parser#compatibility-with-ruby-mri.
Inspecting 1 file
C

Offenses:

test.rb:5:9: C: Use a guard clause instead of wrapping the code inside a conditional expression.
        if my_var != 'd'
        ^^

1 file inspected, 1 offense detected

[madwort]

compare to this:

def test_func(my_var)
  if my_var != 'a'
    if my_var != 'b'
      if my_var != 'c'
        if my_var != 'd'
          fail "ERROR #{my_var}" \
                'ERROR'
        else
          puts 'ok wtf'
        end
      end
    end
  end
  fail 'we only see the issue if this line is too long (more than 80 chars)' if my_var == 'blah'
end

which produces expected result:

$ bundle exec rubocop test.rb --auto-gen-config ; bundle exec rubocop test.rb
warning: parser/current is loading parser/ruby22, which recognizes
warning: 2.2.4-compliant syntax, but you are running 2.2.2.
warning: please see https://github.com/whitequark/parser#compatibility-with-ruby-mri.
Inspecting 1 file
C

Offenses:

test.rb:1:1: C: Method has too many lines. [13/10]
def test_func(my_var)
^^^
test.rb:5:9: C: Avoid more than 3 levels of block nesting.
        if my_var != 'd'
        ^^^^^^^^^^^^^^^^
test.rb:5:9: C: Use a guard clause instead of wrapping the code inside a conditional expression.
        if my_var != 'd'
        ^^
test.rb:14:81: C: Line is too long. [96/80]
  fail 'we only see the issue if this line is too long (more than 80 chars)' if my_var == 'blah'
                                                                                ^^^^^^^^^^^^^^^^

1 file inspected, 4 offenses detected
Created .rubocop_todo.yml.
Run `rubocop --config .rubocop_todo.yml`, or
add inherit_from: .rubocop_todo.yml in a .rubocop.yml file.
warning: parser/current is loading parser/ruby22, which recognizes
warning: 2.2.4-compliant syntax, but you are running 2.2.2.
warning: please see https://github.com/whitequark/parser#compatibility-with-ruby-mri.
Inspecting 1 file
.

1 file inspected, no offenses detected

[jonas054]

The reason for the weird behavior is the interaction between Style/GuardClause and Metrics/LineLength. When LineLength: Max is 80 (default), then GuardClause doesn't report anything, because it doesn't think that the code would fit on one line if it was changed to a guard clause. In this case, however, I think it's making a mistake. It would fit.

When LineLength: Max is 96, as it is in .rubocop_todo.yml, then we get the report.

[madwort]

Ah, yes, that makes sense, thanks!

[alexdowad]

In this case, however, I think it's making a mistake. It would fit.

The reason for this is that GuardClause looks at the character length of the body. In this case, the body is split over 2 lines using \, which means that the character length is rather long.

[alexdowad]

Regarding GuardClause, we could:

• Not register an offense if the fail line is split over 2 or more lines
• Look at the column position of the end of the fail line, rather than its total character length

What makes more sense?

[madwort]

Second one please - this is still a useful cop in the exit scope case even if the guard doesn't fit on one line.

On 30 Dec 2015, at 15:55, Alex Dowad notifications@github.com wrote:

Regarding GuardClause, we could:

Not register an offense if the fail line is split over 2 or more lines
Look at the column position of the end of the fail line, rather than its total character length
What makes more sense?

—
Reply to this email directly or view it on GitHub.

[jonas054]

@alexdowad What GuardClause suggests in this case is to change

        if my_var != 'd'
          fail "ERROR #{my_var}" \
                'ERROR'
        else
          puts 'ok wtf'
        end

to

        fail "ERROR #{my_var}" 'ERROR' if my_var != 'd'
        puts 'ok wtf'

or something, which I think is overstepping its authority. What it should want (and ideally auto-correct to) is IMO

        if my_var != 'd'
          fail "ERROR #{my_var}" \
                'ERROR'
        end
        puts 'ok wtf'

and then let Style/IfUnlessModifier deal with the rest.

[madwort]

in case it's useful @alexdowad here's a better test case:

if my_var 
  fail '12345678901234567890123456789012345678901234567890' \
    '12345678901234567890123456789012345678901234567890'
else
  puts 'ok'
end
puts '123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890'

demo of the unexpected behaviour:

$ bundle exec rubocop test_guard.rb  --auto-gen-config ; bundle exec rubocop test_guard.rb
warning: parser/current is loading parser/ruby22, which recognizes
warning: 2.2.4-compliant syntax, but you are running 2.2.2.
warning: please see https://github.com/whitequark/parser#compatibility-with-ruby-mri.
Inspecting 1 file
C

Offenses:

test_guard.rb:7:81: C: Line is too long. [157/80]
puts '123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890'
                                                                                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

1 file inspected, 1 offense detected
Created .rubocop_todo.yml.
Run `rubocop --config .rubocop_todo.yml`, or
add inherit_from: .rubocop_todo.yml in a .rubocop.yml file.
warning: parser/current is loading parser/ruby22, which recognizes
warning: 2.2.4-compliant syntax, but you are running 2.2.2.
warning: please see https://github.com/whitequark/parser#compatibility-with-ruby-mri.
Inspecting 1 file
C

Offenses:

test_guard.rb:1:1: C: Use a guard clause instead of wrapping the code inside a conditional expression.
if my_var
^^

1 file inspected, 1 offense detected

Expected behaviour: fail both cops when doing the first (--auto-gen) pass.

[madwort]

of course, if everyone stuck to line length limits this wouldn't be a problem...

[alexdowad]

I'm thinking that GuardClause can simply ignore if nodes which have a condition spread over multiple lines. For non-trailing ifs which have a return, next, etc. in the body, they will also be ignored if the control flow statement is spread over multiple lines.

[jonas054]

I think that's fine as a solution for the problem reported here. I see some other problems with the current behavior of GuardClause, but I can open a separate issue for those.

[bbatsov]

Sure. File those issues!

Btw, my initial idea was to have GuardClase operate only upon trivial conditions (e.g. just one term), but for some reason I never got to actually changing this behaviour. It seems to me that guard clauses are a great option for simple conditions, but they hamper the readability otherwise.