rubocop-rspec icon indicating copy to clipboard operation
rubocop-rspec copied to clipboard

Numeric values should be compared with `eq`, not `equal` / `be`

Open marcandre opened this issue 4 years ago • 6 comments

# bad
expect(foo).to be(0)
expect(foo).to equal(0)
# good
expect(foo).to eq(0)
expect(foo).to eql(0)

Error message are overly complex, e.g.:

expected #<Integer: 1> => 0
      got #<Integer: 3595> => 1797

Notice the object IDs in there...

It never really makes sense to compare numbers with equal?. It also could lead to issues, for example with 1<<42 which could work on 64 bit platforms and not on 32 bit platforms.

marcandre avatar Jun 14 '20 04:06 marcandre

Do you think there are other types like that one? Like symbols, or maybe strings?

1<<42 ... not on 32 bit platforms

I'm not sure, won't it be a BigNumber or something on those platforms?

(1 << 200)
=> 1606938044258990275541962092341162602522202993782792835301376

worked quite fine on my computer.

Error message are overly complex, e.g.:

IDK, maybe some would like to make sure that this is the same object. On some platforms, numbers between -127 and 128 are always same objects, while this doesn't apply to other numbers. That's what I remember from my Java days, pretty sure it applies to JRuby as well.

So, do you think this check will fit some existing cop, or should we come up with a new one?

pirj avatar Jun 14 '20 07:06 pirj

Yeah, for BigNum it won't work:

     Failure/Error: it { expect(1 << 80).to be(1 << 80) }
     
       expected #<Integer:70246648039640> => 1208925819614629174706176
            got #<Integer:70246648039700> => 1208925819614629174706176

       Compared using equal?, which compares object identity,
       but expected and actual are not the same object. Use
       `expect(actual).to eq(expected)` if you don't care about
       object identity in this example.

Fixnum/Bignum boundary is platform dependent, so 1 << 40 will be Bignum on 32 bit platforms, Fixnum on 64 bit platforms.

So, do you think this check will fit some existing cop, or should we come up with a new one?

I don't know enough the existing cops to have an informed opinion.

marcandre avatar Jun 14 '20 08:06 marcandre

BTW, same rule should applies to floats. On 64-bit platforms, most floats have same object id, none have the same id on 32 bit platforms iirc

marcandre avatar Jun 14 '20 08:06 marcandre

I started working on this and found out that it completely contradicts RSpec/BeEql cop. Should it be added as an alternative style to BeEql? Should we change the default behavior? Or should we add a preferred style in the rspec-style-guide and change the default of BeEql / retire it (though that would require releasing a major version)?

@pirj @bquorning @marcandre (cc: @backus as the author of the original cop)

Darhazer avatar Jan 01 '21 15:01 Darhazer

Best might be to invert or refactor BeEql 😅

For floats and integers, BeEql is flat out wrong. Detailed answer is platform dependent:

# On 64 bit platform, some values don't fit in `VALUE`:
flt = 2.315841784746324e+77
flt.equal?(flt + 0.0) # => false

On 32 bit platforms, this is true for all floats, although I don't know how many 32 bit platform there are compiling Ruby nowadays.

For Integers, same kind of issue for big enough integers

int = 1 << 62
int.equal?(int + 0) # => false

On 32 bit platforms, this is true from 1 << 30 onwards.

In short: some Cop should actively discourage from using equal and instead use eql or eq for floats and integers.

For nil, true and false, testing with equal? is not "more precise" as the cop says, but equally precise.

Personally, for nil, true and false I don't really care what is used as long as it is precise (i.e. not be_truthy).

marcandre avatar Jan 01 '21 20:01 marcandre

Oops, I'm repeating myself apparently 😅

marcandre avatar Jan 01 '21 20:01 marcandre