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

Wrong value for _.isEqual(0, new Number(Number.MIN_VALUE)) #2815

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

dubzzz
Copy link

@dubzzz dubzzz commented Oct 30, 2019

The current implementation of underscore is returning an invalid answer for:

_.isEqual(0, new Number(Number.MIN_VALUE))
// expected to be false but was true before the fix

Similar to a bug found in Jest jestjs/jest#7941


This bug has been discovered with the following test - property based testing test:

import * as _ from "underscore";
import * as fc from "fast-check";

describe("_.toEqual", () => {
  it("should be symmetric", () => {
    fc.assert(
      fc.property(
        fc.anything({ withBoxedValues: true }),
        fc.anything({ withBoxedValues: true }),
        (a, b) => _.isEqual(a, b) === _.isEqual(b, a)
      ),
      { numRuns: 1000000 }
    );
  });
});

Please note that such kind of tests are currently used within Jest to avoid future issues.

Copy link
Collaborator

@jgonggrijp jgonggrijp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dubzzz You are assuming that

_.isEqual(0, new Number(Number.MIN_VALUE)) === true

is wrong, but the code seems to suggest that this was intended (“zero is equal to very close to zero”). Given the division that converts both operands to Infinity, it might actually be

_.isEqual(new Number(Number.MIN_VALUE), 0) === false

that is wrong instead.

Although the original author of EGAL (see here, section 3B, “Equality of Symbols and Numbers”), the algorithm that the comment in the source refers to, actually seems to agree with you that zero is not equal to very close to zero.

In both cases, however, you are right to point out that the comparison is asymmetric. I agree that it shouldn’t.

This seems like a unique opportunity for Underscore to make a choice. Currently, it is fuzzy in one direction but exact in the other direction. It should probably behave the same in both directions: fuzzy or exact?

@jashkenas What do you think? I would vote for exact, partly because it is less code and partly because this smells like a special case for zero (although one of my suggestions below fixes that in a way that preserves fuzziness and also reduces the amount of code). If choosing fuzzy, the comment probably shouldn't refer to EGAL, because EGAL seems to be exact.

test/objects.js Show resolved Hide resolved
@@ -1223,7 +1223,7 @@
// Object(NaN) is equivalent to NaN.
if (+a !== +a) return +b !== +b;
// An `egal` comparison is performed for other numeric values.
return +a === 0 ? 1 / +a === 1 / b : +a === +b;
return +a === 0 && +b === 0 ? 1 / +a === 1 / b : +a === +b;
Copy link
Collaborator

@jgonggrijp jgonggrijp Apr 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This just collapses the entire expression to +a === +b, since to enter the first branch, both +a and +b must already have been found to exactly equal 0. You can preserve the original fuzziness and still make the comparison symmetric by using a disjunction instead of a conjunction:

Suggested change
return +a === 0 && +b === 0 ? 1 / +a === 1 / b : +a === +b;
return +a === 0 || +b === 0 ? 1 / +a === 1 / +b : +a === +b;

It may actually be worth considering to also allow fuzziness when both values are close to zero but neither is exactly zero. This seems more consistent as it doesn't treat zero as a special case:

Suggested change
return +a === 0 && +b === 0 ? 1 / +a === 1 / b : +a === +b;
return +a === +b || 1 / +a === 1 / +b;

However, maybe there is something to say for making the comparison symmetrically exact instead of symmetrically fuzzy:

Suggested change
return +a === 0 && +b === 0 ? 1 / +a === 1 / b : +a === +b;
return +a === +b;

Copy link
Author

@dubzzz dubzzz Apr 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually this is not fully equivalent. The initial check distinguishes 0 from -0 which was already the case before my change. It is not the case of the suggested change:

---        return +a === 0 && +b === 0 ? 1 / +a === 1 / b : +a === +b;
+++        return +a === +b;

As in JavaScript -0 === 0 🤔
Alternatively we can use: Object.is(+a, +b)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. So actually we have four symmetric options, from most to least fuzzy:

(1) +0 === -0 and values that approach zero from the same side are equivalent.

+a === +b || 1 / +a === 1 / +b

(2) +0 !== -0 and values that approach zero from the same side are equivalent.

+a === 0 || +b === 0 ? 1 / +a === 1 / +b : +a === +b

(3) +0 === -0, otherwise exact comparison.

+a === +b

(4) +0 !== -0, entirely like Object.is but compatible with IE.

+a === 0 && +b === 0 ? 1 / +a === 1 / +b : +a === +b

@jashkenas
Copy link
Owner

I’d be happy to defer to whichever version you think is best for this, because I don't have a particularly strong opinion about how this should behave, in the context of JavaScript floating point ubiquity.

But if we do decide to make a change here, it would be a breaking change.

Co-Authored-By: Julian Gonggrijp <[email protected]>
@dubzzz
Copy link
Author

dubzzz commented Apr 10, 2020

@jashkenas IMO as the original source code was considering 0 to be different from -0, I'd say the change should consider 0 to be different from Number.MIN_VALUE.

What's your opinion?

If it can help for the decision both Jasmine and Jest opted to distinguish 0 from Number.MN_VALUE, see the fixes jestjs/jest#7948 and jasmine/jasmine#1764

@jgonggrijp
Copy link
Collaborator

jgonggrijp commented Apr 10, 2020

I made a new comment above that numbers four options from most to least fuzzy. The current code behaves like option (2) or (3) depending on the values of the operands and the order in which they are fed into _.isEqual. So any change that makes this consistent is a breaking change, but I think you can still share this under "bug fixes" if you choose option (2) or (3).

If choosing option (1) or (4), maybe it should also be considered a semantic change. Option (4) is what @dubzzz is currently proposing.

Edit to add: option (4) is probably the most EGAL, so I'm not ruling out that this is what was originally intended, but this is not how the code on the master branch actually behaves.

@jashkenas
Copy link
Owner

Thanks for laying out the tl;dr of the four options. It sounds like it might be time to have "the big semver conversation" if we go with option 4...

@jgonggrijp
Copy link
Collaborator

I think I agree with @dubzzz that option 4 is the most correct, but it is not a strong opinion.

As for semver, we could work with parallel 1.x and 2.x branches for a while so we don't have to rush the 2.0 release. One of those branches could be master.

I'm going to compose an email to you because there's something I'd like to bring to the table for a potential 2.0.

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

Successfully merging this pull request may close these issues.

3 participants