gwtproject / gwt

GWT Open Source Project
http://www.gwtproject.org
1.49k stars 370 forks source link

Emulated indexOf() is incorrect compared to jdk indexOf() #9568

Open Crazysabbath opened 6 years ago

Crazysabbath commented 6 years ago

GWT version: 2.8.2 Browser (with version): Chrome Version 61.0.3163.100 Operating System: Windows 7

Emulated class ArrayList method indexOf() is not working with NaN values correctly (always returning -1 even though element exists within the list).

Steps to reproduce:

Create a simple list, example:

List<Double> doubleList = new ArrayList<Double>();
doubleList.add(Double.NaN);
doubleList.add(Double.NaN);
doubleList.add(Double.NaN);

Try to find an index of NaN value (should return 0 since all values are the same):

Integer index = doubleList.indexOf(Double.NaN);

Instead it returns -1.

Related discusion

tbroyer commented 6 years ago

The bug is in Double#equals() actually: it's implemented as an == on the double value (and has always been btw, despite the recent rewrite in 2.8 to make doubles “unboxed”, and then for j2cl compatibility), which treats NaN as different from itself. See https://docs.oracle.com/javase/8/docs/api/java/lang/Double.html#equals-java.lang.Object-

We fixed a similar bug in Math.min/max recently: https://github.com/gwtproject/gwt/issues/9281

jnehlmeier commented 6 years ago

It is also broken when testing different signed zeros as it should return false but returns true. There was a recent change (https://gwt-review.googlesource.com/c/gwt/+/19960) that fixes handling of +-0.0 in Double#compareTo(). This method already handles NaN correctly, so we could implement Double#equals() as Double#compareTo() == 0 to get consistent behavior and match JRE.

Crazysabbath commented 6 years ago

@tbroyer I can't reproduce this in 2.7.0 version. indexOf() returns 0. So I guess changes from 2.7 to 2.8 broke something?

rluble commented 6 years ago

2.8 started treating boxed and unboxed doubles with the native JavaScript type number instead of having a "wrapping" Double object when boxed. The side effect is that there are subtle semantic difference between == of boxed and unboxed doubles.

Short answer is yes. 2.8 changed the behavior of Double.equals.

gkdn commented 6 years ago

Jens, changing equals is not enough for this, we need to make sure == reflects that as well for boxed doubles. That's why it is not fixed yet; at one point I will re-write so we use Object.is instead since the performance started to catch up.

Crazysabbath commented 6 years ago

If anyone else should come upon this, temporary fix can be (note that it's only for Double type):

    /**
     * Returns the index of the first occurrence of the specified Double element
     * in this list, or -1 if this list does not contain the element.
     *
     * @param compareVal value to compare
     * @param valList list to compare
     * @return
     */
    private int indexOf(Double compareVal, List<Double> valList){
        boolean isNull = (compareVal == null);
        boolean isNan = (isNull ? false : Double.isNaN(compareVal));

        for (int index = 0; index < valList.size(); index++) {
            if (isNull) {
                if (valList.get(index) == null) return index;
            } else if (isNan) {
                if (Double.isNaN(valList.get(index))) return index;
            } else {
                if (compareVal.equals(valList.get(index))) return index;
            }
        }

        return -1;
    }
philiptzou commented 1 year ago

Is this the same issue that comparing two NaN's in Java returns true, but the behavior changed after GWT compiled the code to JavaScript? If we are not going to fix it, can we at least document it somewhere?

Double nan1 = Double.valueOf(Double.NaN);
Double nan2 = Double.valueOf(Double.NaN);

// true in Java but false in JavaScript
assertTrue(nan1.equals(nan2));

// true in both Java and JavaScript
assertTrue(Double.isNaN(nan1) && Double.isNaN(nan2))