Several issues with immutable.is()

[Methuselah96]

From @balagge on Thu, 26 Sep 2019 13:28:45 GMT

NB: see an earlier issue immutable-js#1736 for a specific bug. This is an extension of that ticket.

Since then I discovered some more issues with immutable.is(), which is a core function. All equality checks are based on this, and it is called all the time. Therefore, the impact of these problems appears in all immutable.js operations resulting in

• counter-intuitive or completely unexpected or wrong behavior
• lower performance

Falsy values / Falsy valueOf() cannot equal to anything else, even if they should

see immutable-js#1736 for 0. However, the same problem exists

• for Booleans
• for Strings

is(true, new Boolean(true))    // true (as expected)
is(false, new Boolean(false))  // false (bug)

is("foobar", new String("foobar")) // true (as expected)
is("", new String(""))             // false (bug)

• for any value object that returns a falsy value by its valueOf(). For example, a string-like object:

function myStringLike(s) {this.s = s;}
myStringLike.prototype.valueOf = function(){return this.s;}

is(new myStringLike("foobar"),"foobar"); // true (as expected)
is(new myStringLike(""), "");            // false (bug)

Similar example can be constructed with a custom boolean.

Also, there are 3 more falsy values in javascript, so, unfortunately:

const myUndefined = {valueOf: () => undefined};
const myNull = {valueOf: () => null};
const myNaN = {valueOf: () => NaN};

is(myUndefined, undefined); // false (bug)
is(myNull, null); // false (bug)
is(myNaN, NaN); // false (bug)

Unexpected result with Date objects

Example:

  // 1970-01-01T00:00:00.000Z corresponding to 0ms
is(new Date(Date.UTC(1970,0,1,0,0,0,0)),0)); // false
  // 1970-01-01T00:00:00.001Z corresponding to 1ms
is(new Date(Date.UTC(1970,0,1,0,0,0,1)),1)); // true

Here the intended behavior is questionable, but the current behavior above is definitely inconsistent. Probably in this case both should be false. The problem is caused by the implementation of valueOf() on Date objects as well as the falsy value problem (valueOf() returning 0 is a separate case).

It can be argued that for Date objects valueOf() should not be called at all, since the returned value (milliseconds elapsed since 1970-01-01T00:00:00Z) is not "equal" to the Date object. Even more importantly, Date objects are mutable, which can cause serious issues if included in an immutable collection.

User defined value objects cannot equal primitive values by equals()

Suppose you want to implement complex numbers and you want to ensure that any complex number with zero imaginary part is equal to the corresponding real number. However, you do not want to use valueOf() - because of the above mentioned problem, you cannot use valueOf() consistently. You try equals() and hashCode, and come up with something like this (not actual working code, just idea):

function Complex(re,im) {this.re = re;this.im = im;}

Complex.prototype.equals = function(other) {
  if (this.im === 0) {return this.re === other || this.re === other.re;}
  return this.im === other.im && this.re === other.re;
}

Complex.prototype.hashCode = function() {/*...hash it...*/}

is(new Complex(0,0),0) // false (oops)
is(new Complex(1,0),1) // false (oops)

Unfortunately this will never work as the equals() method is only called if BOTH objects have it:

https://github.com/immutable-js/immutable-js/blob/e65e5af806ea23a32ccf8f56c6fabf39605bac80/src/is.js#L84-L88

Bottom line: you cannot implement Complexes with either valueOf(), nor equals() consistently.

Comparison done twice for all (most) cases (possible performance loss)

If both inputs are objects, then the valueOf property check is done. This will almost always be true as Object.prototype implements valueOf and most user objects will have this in their prototype chain. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/valueOf

The default implementation of valueOf() simply returns the object, so the following lines will essentially double-check equality, which has been checked previously:

https://github.com/immutable-js/immutable-js/blob/e65e5af806ea23a32ccf8f56c6fabf39605bac80/src/is.js#L71-L83

Probably this is a performance burden as immutable.is() is called frequently.

Even boxable primitives (number, boolean, string and symbol) are checked twice for equality each time a comparison is made, as these also exhibit a valueOf() method after boxing:

typeof (4).valueOf === "function" // true
typeof (true).valueOf === "function" // true
typeof ("foobar").valueOf === "function" // true
typeof(Symbol("foobar")).valueOf === "function" // true

Suggestion

the implementation of immutable.is() should be reconsidered. Ideas:

• do not short-circuit falsy values, remove both occurrences of

if (!valueA || !valueB) {
    return false;
}

• this introduces some problems with null / undefined values, so those must be checked explicitly
• call valueOf() upfront for objects, if exists; do it before comparisons; do not repeat same comparison twice
• ? handle Date objects separately, if at all
• ? handle primitive types spearately (do not box them and call valueOf() at all!)
• use equals() both ways, even if only 1 operand has it

These are just ideas. Since this is a core function, caution is advised, of course.

Copied from original issue: immutable-js#1737