simplify handling of null values for Map (mozilla#1022)

NativeMap's Map.entries() emulation can return NativeMap.NULL_VALUE instead of actual null instance

src/org/mozilla/javascript/Hashtable.java

@@ -1,30 +1,27 @@
-package org.mozilla.javascript;
-
-import java.io.Serializable;
 /* -*- Mode: java; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 4 -*-
  *
  * This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
+package org.mozilla.javascript;
+
+import java.io.Serializable;
 import java.util.HashMap;
 import java.util.Iterator;
 import java.util.NoSuchElementException;
 
 /**
- * This generic hash table class is used by Set and Map. It uses
- * a standard HashMap for storing keys and values so that we can handle
- * lots of hash collisions if necessary, and a doubly-linked list to support the iterator
- * capability.
- * <p>
- * This second one is important because JavaScript handling of
- * the iterator is completely different from the way that Java does it. In Java
- * an attempt to modify a collection on a HashMap or LinkedHashMap while iterating
- * through it (except by using the "remove" method on the Iterator object itself) results in a
- * ConcurrentModificationException. JavaScript Maps and Sets explicitly allow
- * the collection to be modified, or even cleared completely, while iterators
- * exist, and even lets an iterator keep on iterating on a collection that was
- * empty when it was created..
+ * This generic hash table class is used by Set and Map. It uses a standard HashMap for storing keys
+ * and values so that we can handle lots of hash collisions if necessary, and a doubly-linked list
+ * to support the iterator capability.
+ *
+ * <p>This second one is important because JavaScript handling of the iterator is completely
+ * different from the way that Java does it. In Java an attempt to modify a collection on a HashMap
+ * or LinkedHashMap while iterating through it (except by using the "remove" method on the Iterator
+ * object itself) results in a ConcurrentModificationException. JavaScript Maps and Sets explicitly
+ * allow the collection to be modified, or even cleared completely, while iterators exist, and even
+ * lets an iterator keep on iterating on a collection that was empty when it was created..
  */
 public class Hashtable implements Serializable, Iterable<Hashtable.Entry> {
 
@@ -34,28 +31,26 @@ public class Hashtable implements Serializable, Iterable<Hashtable.Entry> {
     private Entry last = null;
 
     /**
-     * One entry in the hash table. Override equals and hashcode because this is
-     * another area in which JavaScript and Java differ. This entry also becomes a
-     * node in the linked list.
+     * One entry in the hash table. Override equals and hashcode because this is another area in
+     * which JavaScript and Java differ. This entry also becomes a node in the linked list.
      */
-
-      public static final class Entry implements Serializable {
-      private static final long serialVersionUID = 4086572107122965503L;
-      protected Object key;
-      protected Object value;
-      protected boolean deleted;
-      protected Entry next;
-      protected Entry prev;
-      private final int hashCode;
+    public static final class Entry implements Serializable {
+        private static final long serialVersionUID = 4086572107122965503L;
+        protected Object key;
+        protected Object value;
+        protected boolean deleted;
+        protected Entry next;
+        protected Entry prev;
+        private final int hashCode;
 
         Entry() {
             hashCode = 0;
         }
 
         Entry(Object k, Object value) {
-            if ((k instanceof Number) && ( ! ( k instanceof Double))) {
+            if ((k instanceof Number) && (!(k instanceof Double))) {
                 // Hash comparison won't work if we don't do this
-                this.key = Double.valueOf(((Number)k).doubleValue());
+                this.key = Double.valueOf(((Number) k).doubleValue());
             } else if (k instanceof ConsString) {
                 this.key = k.toString();
             } else {
@@ -81,15 +76,11 @@ public Object value() {
             return value;
         }
 
-        /**
-         * Zero out key and value and return old value.
-         */
-        Object clear() {
-            final Object ret = value;
+        /** Zero out key and value and return old value. */
+        void clear() {
             key = Undefined.instance;
             value = Undefined.instance;
             deleted = true;
-            return ret;
         }
 
         @Override
@@ -103,7 +94,7 @@ public boolean equals(Object o) {
                 return false;
             }
             try {
-                return ScriptRuntime.sameZero(key, ((Entry)o).key);
+                return ScriptRuntime.sameZero(key, ((Entry) o).key);
             } catch (ClassCastException cce) {
                 return false;
             }
@@ -138,6 +129,10 @@ public void put(Object key, Object value) {
         }
     }
 
+    /**
+     * @deprecated use getEntry(Object key) instead because this returns null if the entry was not
+     *     found or the value of the entry is null
+     */
     public Object get(Object key) {
         final Entry e = new Entry(key, null);
         final Entry v = map.get(e);
@@ -147,11 +142,20 @@ public Object get(Object key) {
         return v.value;
     }
 
+    public Entry getEntry(Object key) {
+        final Entry e = new Entry(key, null);
+        return map.get(e);
+    }
+
     public boolean has(Object key) {
         final Entry e = new Entry(key, null);
         return map.containsKey(e);
     }
 
+    /**
+     * @deprecated use deleteEntry(Object key) instead because this returns null if the entry was
+     *     not found or the value of the entry is null
+     */
     public Object delete(Object key) {
         final Entry e = new Entry(key, null);
         final Entry v = map.remove(e);
@@ -187,12 +191,58 @@ public Object delete(Object key) {
             if (v.next != null) {
                 v.next.prev = prev;
             } else {
-                assert(v == last);
+                assert (v == last);
+                last = prev;
+            }
+        }
+        // Still clear the node in case it is in the chain of some iterator
+        final Object ret = v.value;
+        v.clear();
+        return ret;
+    }
+
+    public boolean deleteEntry(Object key) {
+        final Entry e = new Entry(key, null);
+        final Entry v = map.remove(e);
+        if (v == null) {
+            return false;
+        }
+
+        // To keep existing iterators moving forward as specified in EC262,
+        // we will remove the "prev" pointers from the list but leave the "next"
+        // pointers intact. Once we do that, then the only things pointing to
+        // the deleted nodes are existing iterators. Once those are gone, then
+        // these objects will be GCed.
+        // This way, new iterators will not "see" the deleted elements, and
+        // existing iterators will continue from wherever they left off to
+        // continue iterating in insertion order.
+        if (v == first) {
+            if (v == last) {
+                // Removing the only element. Leave it as a dummy or existing iterators
+                // will never stop.
+                v.clear();
+                v.prev = null;
+            } else {
+                first = v.next;
+                first.prev = null;
+                if (first.next != null) {
+                    first.next.prev = first;
+                }
+            }
+        } else {
+            final Entry prev = v.prev;
+            prev.next = v.next;
+            v.prev = null;
+            if (v.next != null) {
+                v.next.prev = prev;
+            } else {
+                assert (v == last);
                 last = prev;
             }
         }
         // Still clear the node in case it is in the chain of some iterator
-        return v.clear();
+        v.clear();
+        return true;
     }
 
     public void clear() {
@@ -220,9 +270,7 @@ public Iterator<Entry> iterator() {
 
     // The iterator for this class works directly on the linked list so that it implements
     // the specified iteration behavior, which is very different from Java.
-    private final static class Iter
-        implements Iterator<Entry>
-    {
+    private static final class Iter implements Iterator<Entry> {
         private Entry pos;
 
         Iter(Entry start) {

src/org/mozilla/javascript/NativeMap.java

@@ -13,8 +13,6 @@ public class NativeMap extends IdScriptableObject {
     private static final Object MAP_TAG = "Map";
     static final String ITERATOR_TAG = "Map Iterator";
 
-    private static final Object NULL_VALUE = new Object();
-
     private final Hashtable entries = new Hashtable();
 
     private boolean instanceOfMap = false;
@@ -89,32 +87,25 @@ public Object execIdCall(
     }
 
     private Object js_set(Object k, Object v) {
-        // Map.get() does not distinguish between "not found" and a null value. So,
-        // replace true null here with a marker so that we can re-convert in "get".
-        final Object value = (v == null ? NULL_VALUE : v);
         // Special handling of "negative zero" from the spec.
         Object key = k;
         if ((key instanceof Number) && ((Number) key).doubleValue() == ScriptRuntime.negativeZero) {
             key = ScriptRuntime.zeroObj;
         }
-        entries.put(key, value);
+        entries.put(key, v);
         return this;
     }
 
     private Object js_delete(Object arg) {
-        final Object e = entries.delete(arg);
-        return Boolean.valueOf(e != null);
+        return Boolean.valueOf(entries.deleteEntry(arg));
     }
 
     private Object js_get(Object arg) {
-        final Object val = entries.get(arg);
-        if (val == null) {
+        final Hashtable.Entry entry = entries.getEntry(arg);
+        if (entry == null) {
             return Undefined.instance;
         }
-        if (val == NULL_VALUE) {
-            return null;
-        }
-        return val;
+        return entry.value;
     }
 
     private Object js_has(Object arg) {
@@ -155,12 +146,7 @@ private Object js_forEach(Context cx, Scriptable scope, Object arg1, Object arg2
             }
 
             final Hashtable.Entry e = i.next();
-            Object val = e.value;
-            if (val == NULL_VALUE) {
-                val = null;
-            }
-
-            f.call(cx, scope, thisObj, new Object[] {val, e.key, this});
+            f.call(cx, scope, thisObj, new Object[] {e.value, e.key, this});
         }
         return Undefined.instance;
     }

src/org/mozilla/javascript/NativeObject.java

@@ -13,7 +13,6 @@
 import java.util.Collection;
 import java.util.Iterator;
 import java.util.Map;
-import java.util.Map.Entry;
 import java.util.NoSuchElementException;
 import java.util.Set;
 import org.mozilla.javascript.ScriptRuntime.StringIdOrIndex;

src/org/mozilla/javascript/NativeSet.java

@@ -95,8 +95,7 @@ private Object js_add(Object k) {
     }
 
     private Object js_delete(Object arg) {
-        final Object ov = entries.delete(arg);
-        return Boolean.valueOf(ov != null);
+        return Boolean.valueOf(entries.deleteEntry(arg));
     }
 
     private Object js_has(Object arg) {

testsrc/jstests/es6/map.js

@@ -34,6 +34,60 @@ function logElement(value, key, m) {
   assertEquals("c) map[key1] = '' map[key2] = 'undefined' map[key3] = 'undefined' map[key4] = 'null' ", res);
 })();
 
+(function TestForOfNoKey() {
+  res = "d) ";
+  var myMap = new Map([['', 'value1'], [, 17], [undefined, 18], [null, 19]]);
+  for (var entry of myMap) {
+    res += "map[" + entry[0] + "] = '" + entry[1] + "' "; 
+  }
+
+  assertEquals("d) map[] = 'value1' map[undefined] = '18' map[null] = '19' ", res);
+})();
+
+(function TestForOfNoValue() {
+  res = "e) ";
+  var myMap = new Map([['key1', ''], ['key2',], ['key3', undefined], ['key4', null]]);
+  for (var entry of myMap) {
+    res += "map[" + entry[0] + "] = '" + entry[1] + "' "; 
+  }
+
+  assertEquals("e) map[key1] = '' map[key2] = 'undefined' map[key3] = 'undefined' map[key4] = 'null' ", res);
+})();
+
+(function TestEntriesNoKey() {
+  res = "f) ";
+  var myMap = new Map([['', 'value1'], [, 17], [undefined, 18], [null, 19]]);
+
+  var myIter = myMap.entries();
+  var entry = myIter.next();
+  res += "map[0] = '" + entry.value + "' "; 
+  entry = myIter.next();
+  res += "map[1] = '" + entry.value + "' "; 
+  var entry = myIter.next();
+  res += "map[2] = '" + entry.value + "' "; 
+  var entry = myIter.next();
+  res += "map[3] = '" + entry.value + "' "; 
+
+  assertEquals("f) map[0] = ',value1' map[1] = ',18' map[2] = ',19' map[3] = 'undefined' ", res);
+})();
+
+(function TestEntriesNoValue() {
+  res = "g) ";
+  var myMap = new Map([['key1', ''], ['key2',], ['key3', undefined], ['key4', null]]);
+
+  var myIter = myMap.entries();
+  var entry = myIter.next();
+  res += "map[0] = '" + entry.value + "' "; 
+  entry = myIter.next();
+  res += "map[1] = '" + entry.value + "' "; 
+  var entry = myIter.next();
+  res += "map[2] = '" + entry.value + "' "; 
+  var entry = myIter.next();
+  res += "map[3] = '" + entry.value + "' "; 
+
+  assertEquals("g) map[0] = 'key1,' map[1] = 'key2,' map[2] = 'key3,' map[3] = 'key4,' ", res);
+})();
+
 (function TestGetConcatenatedStrings() {
   var myMap = new Map([['key1', 'value1'], ['key2', 17]]);
   for(let i = 1; i <= 1; i++) {