Make handling of object indices more in line with the spec (#1392)

In general, treat invalid index values, such as negative numbers, as strings and not integers. This will result in object properties being correctly ordered, and JSON.stringify producing the right output as well.

* Make handling of object indices more in line with the spec
* In more cases, treat out-of-range numeric indices as strings
* For typed arrays, correctly implement index semantics, which are different than for regular objects
* Also fix index handling in JSON.parse

src/org/mozilla/javascript/ScriptRuntime.java

@@ -14,6 +14,7 @@
 import java.text.MessageFormat;
 import java.util.Arrays;
 import java.util.Locale;
+import java.util.Optional;
 import java.util.ResourceBundle;
 import java.util.function.BiConsumer;
 import org.mozilla.javascript.ast.FunctionNode;
@@ -1382,6 +1383,27 @@ public static char toUint16(Object val) {
         return (char) DoubleConversion.doubleToInt32(d);
     }
 
+    /**
+     * If "arg" is a "canonical numeric index," which means any number constructed from a string
+     * that doesn't have extra whitespace or non-standard formatting, return it -- otherwise return
+     * an empty option. Defined in ECMA 7.1.21.
+     */
+    public static Optional<Double> canonicalNumericIndexString(String arg) {
+        if ("-0".equals(arg)) {
+            return Optional.of(Double.NEGATIVE_INFINITY);
+        }
+        double num = toNumber(arg);
+        // According to tests, "NaN" is not a number ;-)
+        if (Double.isNaN(num)) {
+            return Optional.empty();
+        }
+        String numStr = toString(num);
+        if (numStr.equals(arg)) {
+            return Optional.of(num);
+        }
+        return Optional.empty();
+    }
+
     // XXX: this is until setDefaultNamespace will learn how to store NS
     // properly and separates namespace form Scriptable.get etc.
     private static final String DEFAULT_NS_TAG = "__default_namespace__";
@@ -1454,6 +1476,10 @@ static Function getExistingCtor(Context cx, Scriptable scope, String constructor
      * Return -1L if str is not an index, or the index value as lower 32 bits of the result. Note
      * that the result needs to be cast to an int in order to produce the actual index, which may be
      * negative.
+     *
+     * <p>Note that this method on its own does not actually produce an index that is useful for an
+     * actual Object or Array, because it may be larger than Integer.MAX_VALUE. Most callers should
+     * instead call toStringOrIndex, which calls this under the covers.
      */
     public static long indexFromString(String str) {
         // The length of the decimal string representation of
@@ -1540,7 +1566,7 @@ public static long testUint32String(String str) {
     /** If s represents index, then return index value wrapped as Integer and othewise return s. */
     static Object getIndexObject(String s) {
         long indexTest = indexFromString(s);
-        if (indexTest >= 0) {
+        if (indexTest >= 0 && indexTest <= Integer.MAX_VALUE) {
             return Integer.valueOf((int) indexTest);
         }
         return s;
@@ -1564,7 +1590,7 @@ static Object getIndexObject(double d) {
      *
      * @see ScriptRuntime#toStringIdOrIndex(Context, Object)
      */
-    static final class StringIdOrIndex {
+    public static final class StringIdOrIndex {
         final String stringId;
         final int index;
 
@@ -1577,15 +1603,26 @@ static final class StringIdOrIndex {
             this.stringId = null;
             this.index = index;
         }
+
+        public String getStringId() {
+            return stringId;
+        }
+
+        public int getIndex() {
+            return index;
+        }
     }
 
     /**
      * If id is a number or a string presentation of an int32 value, then id the returning
      * StringIdOrIndex has the index set, otherwise the stringId is set.
      */
-    static StringIdOrIndex toStringIdOrIndex(Object id) {
+    public static StringIdOrIndex toStringIdOrIndex(Object id) {
         if (id instanceof Number) {
             double d = ((Number) id).doubleValue();
+            if (d < 0.0) {
+                return new StringIdOrIndex(toString(id));
+            }
             int index = (int) d;
             if (index == d) {
                 return new StringIdOrIndex(index);
@@ -1599,7 +1636,7 @@ static StringIdOrIndex toStringIdOrIndex(Object id) {
             s = toString(id);
         }
         long indexTest = indexFromString(s);
-        if (indexTest >= 0) {
+        if (indexTest >= 0 && indexTest <= Integer.MAX_VALUE) {
             return new StringIdOrIndex((int) indexTest);
         }
         return new StringIdOrIndex(s);
@@ -1723,7 +1760,7 @@ public static Object getObjectIndex(Object obj, double dblIndex, Context cx, Scr
         }
 
         int index = (int) dblIndex;
-        if (index == dblIndex) {
+        if (index == dblIndex && index >= 0) {
             return getObjectIndex(sobj, index, cx);
         }
         String s = toString(dblIndex);
@@ -1827,7 +1864,7 @@ public static Object setObjectIndex(
         }
 
         int index = (int) dblIndex;
-        if (index == dblIndex) {
+        if (index == dblIndex && index >= 0) {
             return setObjectIndex(sobj, index, value, cx);
         }
         String s = toString(dblIndex);

src/org/mozilla/javascript/json/JsonParser.java

@@ -10,6 +10,7 @@
 import java.util.List;
 import org.mozilla.javascript.Context;
 import org.mozilla.javascript.ScriptRuntime;
+import org.mozilla.javascript.ScriptRuntime.StringIdOrIndex;
 import org.mozilla.javascript.Scriptable;
 
 /**
@@ -118,13 +119,12 @@ private Object readObject() throws ParseException {
                     consume(':');
                     value = readValue();
 
-                    long index = ScriptRuntime.indexFromString(id);
-                    if (index < 0) {
-                        object.put(id, object, value);
+                    StringIdOrIndex indexObj = ScriptRuntime.toStringIdOrIndex(id);
+                    if (indexObj.getStringId() == null) {
+                        object.put(indexObj.getIndex(), object, value);
                     } else {
-                        object.put((int) index, object, value);
+                        object.put(indexObj.getStringId(), object, value);
                     }
-
                     needsComma = true;
                     break;
                 default:

src/org/mozilla/javascript/typedarrays/NativeTypedArrayView.java

@@ -11,6 +11,7 @@
 import java.util.Iterator;
 import java.util.List;
 import java.util.ListIterator;
+import java.util.Optional;
 import java.util.RandomAccess;
 import org.mozilla.javascript.Context;
 import org.mozilla.javascript.ExternalArrayData;
@@ -46,26 +47,77 @@ protected NativeTypedArrayView(NativeArrayBuffer ab, int off, int len, int byteL
         length = len;
     }
 
-    // Array properties implementation
+    // Array properties implementation.
+    // Typed array objects are "Integer-indexed exotic objects" in the ECMAScript spec.
+    // Integer properties, and string properties that can be converted to integer indices,
+    // behave differently than in other types of JavaScript objects, in that they are
+    // silently ignored (and always valued as "undefined") when they are out of bounds.
 
     @Override
     public Object get(int index, Scriptable start) {
         return js_get(index);
     }
 
+    @Override
+    public Object get(String name, Scriptable start) {
+        Optional<Double> num = ScriptRuntime.canonicalNumericIndexString(name);
+        if (num.isPresent()) {
+            // Now we had a valid number, so no matter what we try to return an array element
+            int ix = toIndex(num.get());
+            if (ix >= 0) {
+                return js_get(ix);
+            }
+        }
+        return super.get(name, start);
+    }
+
     @Override
     public boolean has(int index, Scriptable start) {
         return !checkIndex(index);
     }
 
+    @Override
+    public boolean has(String name, Scriptable start) {
+        Optional<Double> num = ScriptRuntime.canonicalNumericIndexString(name);
+        if (num.isPresent()) {
+            int ix = toIndex(num.get());
+            if (ix >= 0) {
+                return !checkIndex(ix);
+            }
+        }
+        return super.has(name, start);
+    }
+
     @Override
     public void put(int index, Scriptable start, Object val) {
         js_set(index, val);
     }
 
+    @Override
+    public void put(String name, Scriptable start, Object val) {
+        Optional<Double> num = ScriptRuntime.canonicalNumericIndexString(name);
+        if (num.isPresent()) {
+            int ix = toIndex(num.get());
+            if (ix >= 0) {
+                js_set(ix, val);
+            }
+        } else {
+            super.put(name, start, val);
+        }
+    }
+
     @Override
     public void delete(int index) {}
 
+    @Override
+    public void delete(String name) {
+        Optional<Double> num = ScriptRuntime.canonicalNumericIndexString(name);
+        if (!num.isPresent()) {
+            // No delete for indexed elements, so only delete if "name" is not a number
+            super.delete(name);
+        }
+    }
+
     @Override
     public Object[] getIds() {
         Object[] ret = new Object[length];
@@ -75,6 +127,18 @@ public Object[] getIds() {
         return ret;
     }
 
+    /**
+     * To aid in parsing: Return a positive (or zero) integer if the double is a valid array index,
+     * and -1 if not.
+     */
+    private static int toIndex(double num) {
+        int ix = (int) num;
+        if (ix == num && ix >= 0) {
+            return ix;
+        }
+        return -1;
+    }
+
     // Actual functions
 
     protected boolean checkIndex(int index) {

testsrc/jstests/harmony/typedarrays.js

@@ -531,26 +531,22 @@ function TestTypedArraysWithIllegalIndices() {
   assertEquals(10, a["-1e2"]);
   assertEquals(undefined, a[-1e2]);
 
-  /* Rhino: Seems to think that "-0" is a real index. Not going to change that here.
   a["-0"] = 256;
   var s2 = "     -0";
   a[s2] = 255;
   assertEquals(undefined, a["-0"]);
   assertEquals(255, a[s2]);
   assertEquals(0, a[-0]);
-  */
 
   /* Chromium bug: 424619
    * a[-Infinity] = 50;
    * assertEquals(undefined, a[-Infinity]);
    */
-   /* Rhino: Seems to think that 1.5 is a real index. Not going to change that here.
   a[1.5] = 10;
   assertEquals(undefined, a[1.5]);
   var nan = Math.sqrt(-1);
   a[nan] = 5;
   assertEquals(5, a[nan]);
-  */
 
   var x = 0;
   var y = -0;
@@ -584,26 +580,22 @@ function TestTypedArraysWithIllegalIndicesStrict() {
   assertEquals(10, a["-1e2"]);
   assertEquals(undefined, a[-1e2]);
 
-  /* Rhino: Seems to think that "-0" is a real index. Not going to change that here.
-  a["-0"] = 256;
+   a["-0"] = 256;
   var s2 = "     -0";
   a[s2] = 255;
   assertEquals(undefined, a["-0"]);
   assertEquals(255, a[s2]);
   assertEquals(0, a[-0]);
-  */
 
   /* Chromium bug: 424619
    * a[-Infinity] = 50;
    * assertEquals(undefined, a[-Infinity]);
    */
-   /* Rhino: Seems to think that 1.5 is a real index. Not going to change that here.
   a[1.5] = 10;
   assertEquals(undefined, a[1.5]);
   var nan = Math.sqrt(-1);
   a[nan] = 5;
   assertEquals(5, a[nan]);
-  */
 
   var x = 0;
   var y = -0;

testsrc/org/mozilla/javascript/tests/IndexTest.java

@@ -0,0 +1,59 @@
+package org.mozilla.javascript.tests;
+
+import static org.junit.Assert.*;
+
+import org.junit.Test;
+import org.mozilla.javascript.ScriptRuntime;
+
+public class IndexTest {
+    private void expectInteger(ScriptRuntime.StringIdOrIndex id, int v) {
+        assertEquals(v, id.getIndex());
+        assertNull(id.getStringId());
+    }
+
+    private void expectString(ScriptRuntime.StringIdOrIndex id, String s) {
+        assertEquals(s, id.getStringId());
+    }
+
+    @Test
+    public void testNumericIndices() {
+        // Normal integers
+        expectInteger(ScriptRuntime.toStringIdOrIndex(0), 0);
+        expectInteger(ScriptRuntime.toStringIdOrIndex(1), 1);
+        expectInteger(ScriptRuntime.toStringIdOrIndex(Integer.MAX_VALUE), Integer.MAX_VALUE);
+        // Negative integers
+        expectString(ScriptRuntime.toStringIdOrIndex(-1), "-1");
+        expectString(ScriptRuntime.toStringIdOrIndex(Integer.MIN_VALUE), "-2147483648");
+        // Floating-point -- but rounding is weird so just check nullness
+        ScriptRuntime.StringIdOrIndex id;
+        id = ScriptRuntime.toStringIdOrIndex(1.1f);
+        assertNotNull(id.getStringId());
+    }
+
+    @Test
+    public void testStringIndices() {
+        // Normal integers
+        expectInteger(ScriptRuntime.toStringIdOrIndex("0"), 0);
+        expectInteger(ScriptRuntime.toStringIdOrIndex("1"), 1);
+        expectInteger(
+                ScriptRuntime.toStringIdOrIndex(String.valueOf(Integer.MAX_VALUE)),
+                Integer.MAX_VALUE);
+        // Negative integers
+        expectString(ScriptRuntime.toStringIdOrIndex("-1"), "-1");
+        expectString(
+                ScriptRuntime.toStringIdOrIndex(String.valueOf(Integer.MIN_VALUE)), "-2147483648");
+        // Floating-point
+        expectString(ScriptRuntime.toStringIdOrIndex("3.14"), "3.14");
+        expectString(ScriptRuntime.toStringIdOrIndex("1.1"), "1.1");
+        // Out of range
+        expectString(
+                ScriptRuntime.toStringIdOrIndex(String.valueOf(Long.MAX_VALUE)),
+                "9223372036854775807");
+        // Others
+        expectString(ScriptRuntime.toStringIdOrIndex(Double.NaN), "NaN");
+        // Junk
+        expectString(
+                ScriptRuntime.toStringIdOrIndex("This is not an integer"),
+                "This is not an integer");
+    }
+}