Troubleshooting a Rubocop Bug
How a bug in Rubocop and compounding human factors led to an uncommon error.

I recently ran across a weird bug involving Rubocop and an obscure Ruby error that you don’t often come across.

The application I work at my new job on doesn’t enforce lint rules in our CI pipeline, so there are lots of infractions that have been added over time. At our most recent retrospective, this was a top item to start tackling before it really gets out of hand. As a preliminary step before requiring a lint check to pass in CI, our team lead volunteered to run the company’s default Rubocop rules over the entire repository (bundle exec rubocop -a) and put up a PR for the team to review. Having previously contributed to Rubocop, I requested to be involved in the review of this code. Most of the autocorrected code was of no consequence, but we did notice that it broke unit tests with an obscure error.

The Code

Prior to Rubocop’s changes, our application utilized a fairly non-standard case statement syntax that looked something like the following:

def example
  return case
      when condition.foo? then :foo
      when condition.bar? then :baz

      # Lots more conditions...

      else :bat
    end
end

This method is isolated and anonymized here, but was originally located somewhere deep within a class, so there were lots of other linter violations that were present in the file (this will be important later).

There are a few important things to note about a case statement written this way:

1. It has an empty case
2. The return value of the method is the result of the case statement
3. The conditions are inlined using when..then syntax

Empty case

Normally, when you write a case statement, you evaluate some expression in the first line and then check the result below using when:

def example
  case condition
    when :foo then :is_foo
    when :bar then :is_bar

    # etc.

    else :is_something_else
  end
end

In fact, it’s so standardized (across languages, even!) that I didn’t even know it would work without having an expression following case. For this reason, there’s a Rubocop linter rule (known as a cop) that raises style warnings when you do this: Style/EmptyCaseCondition. If it detects and empty case, it’s supposed to suggest using alternate if..elsif..else syntax instead:

# example.rb
def example
  case
    when condition.foo? then :is_foo
    when condition.bar? then :is_bar
    else :is_something_else
  end
end

# $ rubocop -a example.rb --only Style/EmptyCaseCondition

# example.rb (autocorrected)
def example
  if condition.foo? then :is_foo
    elsif condition.bar? then :is_bar
    else :is_something_else
  end
end

# $ ruby example.rb
# No output, no errors!

In the example I’ve isolated only the cop that we’re interested in, so there are other issues that would be raised normally by using the standard rubocop -a example.rb. We’re not interested with those here, since the Style/EmptyCaseCondition cop is our entry point to this bug.

Returning Result of case

You might notice that the above code doesn’t use the return keyword. That’s because if you do return this autocorrected case, the code breaks!

# example.rb
def example
  return case
      when condition.foo? then :foo
      when condition.bar? then :baz
      else :bat
    end
end

# $ ruby example.rb
# No output, no errors!

# $ rubocop -a example.rb --only Style/EmptyCaseCondition

# example.rb (autocorrected)
def example
  return if condition.foo? then :foo
      elsif condition.bar? then :baz
      else :bat
    end
end

# $ ruby example.rb
# Multiple syntax errors (unexpected keyword then)!

Uh-oh! But what’s gone wrong?

Inline Case Conditions (and All About then)

Let’s take a step back and talk about how we inline things in Ruby. For starters, this is an inline when..then:

case condition.foo?
  when true then :foo
end

It’s more common form is functionally equivalent written like so:

case condition.foo?
  when true 
    :foo
end

We can see here from these two examples that there’s basically two common ways to delimit the condition versus the value – a newline or the then keyword. You can also use a semicolon, but… c’mon… it’s Ruby. You almost never see that.

if statements also behave this way:

if condition.foo?
  :foo
end

is the same as

if condition.foo? then :foo end

Putting It All Together

Let’s take a look at our failing autocorrected code one more time:

def example
  return if condition.foo? then :foo
      elsif condition.bar? then :baz
      else :bat
    end
end

The important piece here is that we’re trying to return an if statement that has been inlined using then. One thing to note about return is that it can only return a single value. Therefore, it must evaluate any expression that it’s returning.

return doesn’t evaluate if statements as multiline expressions, but instead as guard clauses. You may have seen or used the following syntax:

def early_return(condition)
  return if condition

  do_stuff
end

That means that Ruby interprets return if condition.foo? as a syntactically complete expression, and everything that follows is in error.

We can prove this by forcing the broken code to work by evaluating it using parens:

def example
  return (if condition.foo? then :foo
         elsif condition.bar? then :baz
         else :bat
         end)
end

An Especially Dangerous Example

What if our initial code had been written as follows?

def example
  return case
      when condition.foo?
        :foo
      end
end

If you revise with all cops (as of Rubocop 1.23.0), you’ll get the following code:

def example
  :foo if condition.foo?
end

This is quite clean compared to the contrived example above… But I can see where something like that might happen if you had a large case statement with lots of options that gradually over time dwindled in size. Coding a solution like the autocorrected version should be quite obvious if you need this method from the start, but writing something like the original example over time is not impossible given sufficiently large tech debt.

If you run only the Style/EmptyCaseCondition cop on the original example, you’ll get this:

def example
  return if condition.foo?
        :foo
      end
end

Luckily, on running this code, it’ll throw another syntax error. But this time, the first and second lines of the method body are fine, if poorly formatted; it’ll just complain about the extra end. If you’re not paying close attention, you can delete the end and leave the rest:

def example
  return if condition.foo?
        :foo
end

This code is valid, and even worse, it does the exact opposite of the original method!

To be clear, the failure mode here is quite specific – use a specific cop or subset of cops that won’t automatically reduce the case statement to a one-line if statement. As you can see, it’s within the realm of possibility to do some really dangerous stuff.

Compounding Factors to Fixing Our Issue

At the beginning I mentioned that we ran into some obscure Ruby errors you don’t usually see. So what happened, considering it looks like the issue we had was completely caused by a Rubocop bug?

Firstly, long story short, unlike our examples above we were autocorrecting using a full suite of linter rules, not just the Style/EmptyCaseCondition cop we’ve already identified as the root cause. We also ran an older version of Rubocop (in 0.9x.0 range) that partially autocorrected issues once the syntax error was introduced. Essentially, we had layered errors that we needed to peel back before we could identify the root issue.

Secondly, one engineer did identify this code block as an issue and attempted to correct it, but inadvertently introduced a second bug similar to the following:

def example
  if condition.foo return :foo
  end
end

If you try running this code, it returns a void value error. Remember that to inline an if statement this way you need to use then, otherwise it’ll evaluate the entire line as the condition (i.e. the condition here is condition.foo return :foo, not condition.foo). Somehow an early return here returns nothing at all (not even nil!), which can’t happen in Ruby.

An obvious fix to either of these given what has been discussed is:

def example
  if condition.foo then return :foo
  end
end

# or

def example
  if condition.foo 
    return :foo
  end
end

Compounding this problem, there was also a bug ticket in Ruby related to a similar issue that led us down a rabbit hole to ensure that it wasn’t the cause.

Wrap Up

Long story short, we ended up manually fixing the contrived case statement that we were working with and solved the issue without much hassle once the root cause was identified. After that, we disabled Style/EmptyCaseCondition for our repository and others until we can be sure no one else has used this code style elsewhere.

I also opened a bug ticket in Rubocop to get this issue resolved at the source.

That’s all for now. Thanks for reading!