Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
marcandre opened this issue Jun 14, 2020 · 6 comments
Open

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

marcandre opened this issue Jun 14, 2020 · 6 comments
Assignees

Comments

@marcandre
Copy link
Contributor

marcandre commented Jun 14, 2020

# 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
Copy link
Member

pirj commented Jun 14, 2020

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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 Darhazer self-assigned this Dec 21, 2020
@Darhazer
Copy link
Member

Darhazer commented Jan 1, 2021

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
Copy link
Contributor Author

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
Copy link
Contributor Author

Oops, I'm repeating myself apparently 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants