Fix Lint/ElseLayout when using multi-branch statements

[bkuhlmann]

Expected behavior

With the latest release of Rubocop, I'm noticing a behavioral change that I think is a bug or at least an oddity. This issue is related to this original issue. If I understand the intent of the original issue, I think it was intended for single if and else statement -- which makes sense -- but I think this breaks when used in an if..elsif..else statement?

Actual behavior

I'm seeing Lint/ElseLayout issues with single line else in a if statement that didn't get flagged before the change in the latest release. Below is the full output:

Debug Trace

For /Users/bkuhlmann/Engineering/OSS/git-lint: configuration from /Users/bkuhlmann/Engineering/OSS/git-lint/.rubocop.yml
configuration from /Users/bkuhlmann/Engineering/OSS/git-lint/.rubocop-https---raw-githubusercontent-com-bkuhlmann-code-quality-main-configurations-rubocop-ruby-yml
Default configuration from /Users/bkuhlmann/.cache/frum/versions/3.0.2/lib/ruby/gems/3.0.0/gems/rubocop-1.22.0/config/default.yml
configuration from /Users/bkuhlmann/Engineering/OSS/git-lint/.rubocop-https---raw-githubusercontent-com-bkuhlmann-code-quality-main-configurations-rubocop-rake-yml
configuration from /Users/bkuhlmann/.cache/frum/versions/3.0.2/lib/ruby/gems/3.0.0/gems/rubocop-rake-0.6.0/config/default.yml
configuration from /Users/bkuhlmann/.cache/frum/versions/3.0.2/lib/ruby/gems/3.0.0/gems/rubocop-rake-0.6.0/config/default.yml
configuration from /Users/bkuhlmann/Engineering/OSS/git-lint/.rubocop-https---raw-githubusercontent-com-bkuhlmann-code-quality-main-configurations-rubocop-performance-yml
configuration from /Users/bkuhlmann/.cache/frum/versions/3.0.2/lib/ruby/gems/3.0.0/gems/rubocop-performance-1.11.5/config/default.yml
configuration from /Users/bkuhlmann/.cache/frum/versions/3.0.2/lib/ruby/gems/3.0.0/gems/rubocop-performance-1.11.5/config/default.yml
configuration from /Users/bkuhlmann/Engineering/OSS/git-lint/.rubocop-https---raw-githubusercontent-com-bkuhlmann-code-quality-main-configurations-rubocop-rspec-yml
configuration from /Users/bkuhlmann/.cache/frum/versions/3.0.2/lib/ruby/gems/3.0.0/gems/rubocop-rspec-2.5.0/config/default.yml
configuration from /Users/bkuhlmann/.cache/frum/versions/3.0.2/lib/ruby/gems/3.0.0/gems/rubocop-rspec-2.5.0/config/default.yml
Running parallel inspection
For /Users/bkuhlmann/Engineering/Misc: configuration from /Users/bkuhlmann/.config/rubocop/config.yml
configuration from /Users/bkuhlmann/.config/rubocop/.rubocop-https---raw-githubusercontent-com-bkuhlmann-code-quality-main-configurations-rubocop-ruby-yml
configuration from /Users/bkuhlmann/.config/rubocop/.rubocop-https---raw-githubusercontent-com-bkuhlmann-code-quality-main-configurations-rubocop-rake-yml
configuration from /Users/bkuhlmann/.config/rubocop/.rubocop-https---raw-githubusercontent-com-bkuhlmann-code-quality-main-configurations-rubocop-performance-yml
configuration from /Users/bkuhlmann/.config/rubocop/.rubocop-https---raw-githubusercontent-com-bkuhlmann-code-quality-main-configurations-rubocop-rspec-yml
Loading cache from /Users/bkuhlmann/.cache/rubocop_cache/7cd34d310cda33d35ab370b487ae14bb76d5a73e/132c4e53bc39cc09f7d3f2461c2f48ebce8751d3/ac0019aa6070aba42163cf1bca85c5ed3bf19db3
Inspecting 1 file
Scanning /Users/bkuhlmann/Engineering/Misc/snippet.rb
For /Users/bkuhlmann/Engineering/Misc: configuration from /Users/bkuhlmann/.config/rubocop/config.yml
configuration from /Users/bkuhlmann/.config/rubocop/.rubocop-https---raw-githubusercontent-com-bkuhlmann-code-quality-main-configurations-rubocop-ruby-yml
configuration from /Users/bkuhlmann/.config/rubocop/.rubocop-https---raw-githubusercontent-com-bkuhlmann-code-quality-main-configurations-rubocop-rake-yml
configuration from /Users/bkuhlmann/.config/rubocop/.rubocop-https---raw-githubusercontent-com-bkuhlmann-code-quality-main-configurations-rubocop-performance-yml
configuration from /Users/bkuhlmann/.config/rubocop/.rubocop-https---raw-githubusercontent-com-bkuhlmann-code-quality-main-configurations-rubocop-rspec-yml
Loading cache from /Users/bkuhlmann/.cache/rubocop_cache/7cd34d310cda33d35ab370b487ae14bb76d5a73e/132c4e53bc39cc09f7d3f2461c2f48ebce8751d3/ac0019aa6070aba42163cf1bca85c5ed3bf19db3
W

Offenses:

/Users/bkuhlmann/Engineering/Misc/snippet.rb:21:1: C: [Correctable] Style/CaseLikeIf: Convert if-elsif to case-when. (https://rubystyle.guide#case-vs-if-else)
if number == 1 then puts "One" ...
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
/Users/bkuhlmann/Engineering/Misc/snippet.rb:23:6: W: [Correctable] Lint/ElseLayout: Odd else layout detected. Did you mean to use elsif?
else "Unknown."
     ^^^^^^^^^^

1 file inspected, 2 offenses detected, 2 offenses auto-correctable
Finished in 0.22139099997002631 seconds

You can ignore the Style/CaseLikeIf issue as that's legit because I'm using a case statement for comparison purposes below.

Steps to reproduce the problem

Throw the following in snippet.rb and run Rubocop against it:

number = 1

case number
  when 1 then puts "One"
  when 2 then puts "Two"
  else "Unknown"
end

if number == 1 then puts "One"
elsif number == 2 then puts "Two"
else "Unknown"
end

The above code is silly but notice how the else in the case and if statements used to be allowed. However, with the latest version, you now have to write the if statement like this to fix the Lint/ElseLayout issue:

if number == 1 then puts "One"
elsif number == 2 then puts "Two"
else
  "Unknown"
end

This seems wrong to me because it's not consisent with case statements but, more importantly, if you have simple statements above the else all on one line, why can't else be a one liner as well in order to maintain symmetry?

RuboCop version

rubocop -V

1.22.0 (using Parser 3.0.2.0, rubocop-ast 1.12.0, running on ruby 3.0.2 arm64-darwin20.5.0)
  - rubocop-performance 1.11.5
  - rubocop-rake 0.6.0
  - rubocop-rspec 2.5.0

[dvandersluis]

Thank you, this is a really well reasoned issue.

You are absolutely right, the inline style is going to be affected by this change, and it may in fact be the missing reason that else was explicitly ignored as I mentioned in the PR. That being said we now have a conundrum because we can't both flag this and not flag this at the same time, and it speaks to my confusion about this being a Lint cop vs a Layout cop.

As a Lint cop, it is consistent for else foo on a single line to be flagged, because it's plausible that it was in fact supposed to be an elsif instead. Also as a lint cop, the comparison to case is less meaningful, because elsif doesn't exist for case statements, so there's no "ambiguity" (from the reader's perspective, not the parser's) about what is intended. However, for layout, your original code is also consistent.

I have a few thoughts that I need to think further on:

1. Occam's razor would suggest that Lint/ElseLayout is not a useful cop for codebases that predominately use a compact if style as you have demonstrated, because it implicitly removes the ambiguity by making it convention, so maybe just outright disabling the cop in your case is the easiest solution.
2. We could perhaps add a special case for else when the previous branches are all on a single line, or if all the other branches use then. This probably has some brittleness to it that would lead to more issues.
3. Similarly, maybe there could be a special case when the else "body" is a literal, but even that is probably a factor of simplification for this issue, given that the other branches call puts and your else probably would as well in reality.

[bkuhlmann]

🙇 Daniel. Ha, yeah, it is definitely confusing that layout is in the Lint/ElseLayout name. 🙃 A couple notes in reply (same sequence):

1. Yeah, I think I'm going to disable as a workaround. I can't seem to find much harm in doing that so seems fine.
2. Yeah, I'd like that a lot. So use compact style (i.e. multiple one-line then's) or switch to indented style (making up words here) when no thens are used. ...or even supported a mixed layout (more on that below).
3. So this one is a bit odd to me. I'm not sure if I see or understand the distinction between literal and command. In my mind it's either compact, indented, or possibly a mixed layout.

There is one other situation that might be worth pointing out. Take this mixed layout scenerio, for example (again, ignore the silly logic, just focus on the layout):

if number == 1 then puts "One"
elsif number == 2
  puts "Pellentque morbi-trist sentus et netus et malesuada fames ac turpis egestas."
else "Unknown"
end

Notice that the second logic branch needs to be indented because it's too long (assumption being that the line limit cop would flag this otherwise so you switch to indented style for this branch). Granted, and depending on the situation, if you had more complex logic you might extract a private method to be called for the second branch logic rather than make the whole condition itself bloated but sometimes there is value in have a mixed layout too, I think.

I think the above mixed layout scenerio is fine? Maybe? ...or would there need to be a configuration for indented, compact, mixed?

[dvandersluis]

Lint cops almost never have configurable styles, because lints aren't generally meant to be configurable obviously. The configuration comes from whether you agree with the lint (keep the cop enabled) or disagree with the lint (disable it), although in general lints should be pretty universal. So I don't think we should be adding configuration to this cop to handle multiple styles (which as you have documented, has the potential to get into complex edge cases).

Maybe the best solution would be to revert Lint/ElseLayout to the previous version, because it is a more concrete issue when the else has multiple lines but one of them are on the same line as the keyword (this "should not" be an intentional style, but of course there's probably a codebase out there that does this on purpose...). Then we can perhaps introduce a new Layout cop to check for consistent if...else styling, or a Style cop to require or forbid then with if (which I'm surprised doesn't exist yet) for style consistency. I haven't landed on anything yet.

@bbatsov any thoughts?

[cetinajero]

Hi everyone, here with the same oddity

if number == 1 then puts "One"
elsif number == 2
  puts "Pellentque morbi-trist sentus et netus et malesuada fames ac turpis egestas."
else "Unknown"
end

I'm also getting Rubocop warnings regarding snippets like those, which... IMHO, seem perfectly fine (i.e. else being a one liner as the better layout approach).

Looking forward to see how this develops.

Cheers!

[dvandersluis]

@cetinajero I get that this isn't real code, but there are three different styles in that if... 😄

[cetinajero]

That's true @dvandersluis!

As you mentioned, "... a new layout cop to check for consistent if...else styling..." sounds pretty good to enforce the preferred style!

[bkuhlmann]

ℹ️ Closing this issue as I've verified this is fixed in Rubocop 1.24.0 by running Rubocop 124.0 against 30+ projects which use the one-line else style. I'm not exactly sure what changed but glad to see this is working again. Thanks all!

[bkuhlmann]

Apologies, everyone. I was able to prove this still occurs.

I believe the reason I didn't detect this sooner was due to running Rubocop against some projects which hadn't cleared their local Rubocop cache which initially gave me a false positive.

[forthrin]

+1. Was just about to post the very same. What's the status on this one? Using 1.62.1.

def cop
  _ =
    case foo
    when true then 'bar'
    else 'baz'
    end

  _ =
    if foo then 'bar'
    # elsif ...
    # elsif ...
    else 'baz' # Lint/ElseLayout
    end
end