Rubocop: Style/DoubleNegation should be allowed for return values

[donv]

Hi!

A typical pattern is to have a boolean query to read object state that is not stored as a boolean inside the object:

class MyClass
def close
@closed_at = Time.now
end

def closed?
!!@closed_at
end
end

The conversion to boolean is to encapsulate the inner state of the object. The double negation is idiomatic for "convert to boolean". I agree with the style guide that it should not be used in control flow, but I think it is good style when using outside of control flow.

Rubocop 0.41.2 lists the above as an offence. It should not since the double negation is not in a control flow.

RuboCop version

0.41.2 (using Parser 2.3.1.2, running on ruby 2.3.1 x86_64-darwin14)

[Drenmi]

I think this discussion needs to be had in the Ruby Style Guide, first, because at this point it succinctly states:

Avoid the use of !!.

and

use nil? instead.

which suggests this is actually the idiomatic way:

def closed?
  @closed_at.nil?
end

So the rationale:

The double negation is idiomatic for "convert to boolean".

is at odds with the current consensus. Of course, the style guide is community driven, and so challenging the consensus is how it evolves. 😀

[dabroz]

An equivalent of !!@closed_at would actually be

!@closed_at.nil?

which (IMO) is quite ugly.

[bbatsov]

which (IMO) is quite ugly.

Ugly? This is as clear as the code could get. Once your familiar with a certain pattern you start forgetting the mental overhead it would introduce for others who are unfamiliar with it.

[dabroz]

That's subjective, I guess? !!foo is a simple and well known idiom (even outside the Ruby world), whether !foo.nil? is much less clear because of the negation and the fact that ! and .nil? are separated.

[Drenmi]

That's subjective, I guess? !!foo is a simple and well known idiom (even outside the Ruby world),

Personally, I think the keyword here is "outside the Ruby world", but that doesn't automatically make it a globally accepted idiom. 😀

Again, I think this discussion should be moved to the Style Guide repo, where people who are concerned with it can also partake. 😉

[jonas054]

Yes. Continue the style discussion in the Style Guide repo. Closing.

[duane]

This seems to be ignoring the fact that !foo.nil? is completely semantically different than !!foo. Double negation allows eliding multiple types into a known type; !false.nil? is true, yet !!false is false.

Not sure why this is in the default rules if it can't give a rationale or suggest a fix.

[juanibiapina]

Just marking this as another representative example of how the style guide is not driven by the community.

[tom-lord]

To echo the concerns raised above:

I agree that !!foo is usually redundant and can be replaced with foo. However, there are some legitimate use cases for doing this - such as when implementing the null object pattern.

And in contrast to the cop overview, the following are _NOT_ equivalent!!:

# bad
!!something

# good
!something.nil?

If something == false, then !!something == false but !something.nil? == true.

Making this substitution blindly will break code.

I am not a fan of this cop in general; I don't consider the above refactoring to be an improvement.

However, I do recognise that using !! as a non-return value is a genuine code smell. I think that the cop should (at least optionally) be restricted to only complaining in such cases, and I think that the "suggested refactoring" should be made clearer about the above nil vs false issue.

[mikegee]

This discussion should continue only on the issue in the style guide repo.

@bbatsov, please consider locking this thread.

[tom-lord]

@mikegee I have moved my above comment over to that thread; however I do still feel there is an issue worth raising/discussing in this repo.

Namely, the fact that the above mentioned refactoring:

# bad
!!something

# good
!something.nil?

Is something prescribed by rubocop specifically, not the ruby-style-guide.

In fact, what the ruby-style-guide says on the matter is perfectly fine:

you don't need this [!!] explicit conversion in the condition of a control expression. [...] If you want to do a nil check, use nil? instead.

The style guide does not say "replace !!object with !object.nil?". This recommendation lies only in rubocop.

If rubocop's rules are supposed to mirror that style guide, then the cop should be updated to only apply within control expressions.

[bpieck]

I agree with tom-lord.

I just want to give to the outside the same behaviour of an object beeing assumed as falsey as it is in ruby. So I wanted to convert null and false into false for a json output, while calling everything else true. I really think for that double negation is really nice and short - while everything that is equivalent is in my opinion unnecessary complex. Since that intention is not in control expressions - I think the recomendation of rubocop is just wrong in my case.

[matiasasis]

!!something is definitely not equals to !something.nil?

When we want to treat equally a false and a nil the above suggestion won't work.

Here is a case:
!{ params: {}}.dig(:params)&.key?(:id).nil? => true

!!{ params: {}}.dig(:params)&.key?(:id) => false

The rubocop friendly approach would be:

!inputs.dig(:params)&.key?(primary_key).blank?

[marcandre]

Here is a case:
!{ params: {}}.dig(:params)&.key?(:id).nil? => true

!!{ params: {}}.dig(:params)&.key?(:id) => false

This doesn't seem like a particularly good usecase. Why not write !!{ params: {}}.dig(:params, :id)?

It still doesn't change the fact that !!foo and !foo.nil? are not the same.

[marcandre]

Reopening this.

First, it is clear that this cop must be marked as not safe.

Second, it seems this cop doesn't reflect the Ruby style guide since it applies it outside of control expressions.

I'll admit it's difficult for me to be 100% objective here, as I'm in the minority that doesn't like using .nil? as a method call, so I'm clearly biased against !foo.nil?

My personal opinion is that this cop's autocorrection is always less readable and less desirable. If !! appears in a if/unless/?:/while/until/when, then it should be entirely removed. Further I disagree that !! in a return statement is an offense at all. A quick check reveals 1526 uses in the top 500 gems; I could make a more specify search for !foo.nil? and if !! appears in other cases than a return.

Let's minimize the reasons to curse at RuboCop.

[marcandre]

This Cop is now marked as having an unsafe autocorrection.

Not apparent in this thread is the new supported style allowed_in_returns, which is turned on by default.