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.
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:
- It has an empty case
- The return value of the method is the result of the
- The conditions are inlined using
Normally, when you write a case statement, you evaluate some expression in the first line and then check the result below using
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
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
Let’s take a step back and talk about how we inline things in Ruby. For starters, this is an inline
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.
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!