rubocop / rubocop-rspec

Code style checking for RSpec files.
https://docs.rubocop.org/rubocop-rspec
MIT License
809 stars 276 forks source link

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

Open marcandre opened 4 years ago

marcandre commented 4 years ago
# 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.

pirj commented 4 years ago

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?

marcandre commented 4 years ago

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 commented 4 years ago

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

Darhazer commented 3 years ago

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)

marcandre commented 3 years ago

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 commented 3 years ago

Oops, I'm repeating myself apparently 😅