Array.prototype.reverse should call [[HasProperty]] on elements before [[Get]]

src/array.js

Index: src/array.js
diff --git a/src/array.js b/src/array.js
index 7baabf83610c3c333eb13b9a5463c652ccaa3839..394ebd080758e8823ac4bd29ec268c1fdd0dd8a5 100644
--- a/src/array.js
+++ b/src/array.js
@@ -567,10 +567,10 @@ function SparseReverse(array, len) {
       high = len - i - 1;
     }
 
-    var current_i = array[low];
-    if (!IS_UNDEFINED(current_i) || low in array) {

[adamk, 2015/07/16 22:08:13]

As discussed offline, I don't think SparseReverse needs to be changed at all,
since it's guaranteed that [[Get]] and [[HasProperty]] are both side-effect free
in here.

[Dan Ehrenberg, 2015/07/16 22:16:49]

Done.

-      var current_j = array[high];
-      if (!IS_UNDEFINED(current_j) || high in array) {
+    if (low in array) {
+      var current_i = array[low];
+      if (high in array) {
+        var current_j = array[high];
         array[low] = current_j;
         array[high] = current_i;
       } else {
@@ -578,8 +578,8 @@ function SparseReverse(array, len) {
         delete array[low];
       }
     } else {
-      var current_j = array[high];
-      if (!IS_UNDEFINED(current_j) || high in array) {
+      if (high in array) {
+        var current_j = array[high];
         array[low] = current_j;
         delete array[high];
       }
@@ -587,14 +587,25 @@ function SparseReverse(array, len) {
   }
 }
 
+function PackedArrayReverse(array, len) {
+  var j = len - 1;
+  for (var i = 0; i < j; i++, j--) {
+    var current_i = array[i];
+    var current_j = array[j];
+    array[i] = current_j;
+    array[j] = current_i;
+  }
+  return array;
+}
+
 
 function InnerArrayReverse(array, len) {
   var j = len - 1;
   for (var i = 0; i < j; i++, j--) {
-    var current_i = array[i];
-    if (!IS_UNDEFINED(current_i) || i in array) {
-      var current_j = array[j];
-      if (!IS_UNDEFINED(current_j) || j in array) {
+    if (i in array) {
+      var current_i = array[i];
+      if (j in array) {
+        var current_j = array[j];
         array[i] = current_j;
         array[j] = current_i;
       } else {
@@ -602,8 +613,8 @@ function InnerArrayReverse(array, len) {
         delete array[i];
       }
     } else {
-      var current_j = array[j];
-      if (!IS_UNDEFINED(current_j) || j in array) {
+      if (j in array) {
+        var current_j = array[j];
         array[i] = current_j;
         delete array[j];
       }
@@ -623,9 +634,11 @@ function ArrayReverse() {
     %NormalizeElements(array);
     SparseReverse(array, len);
     return array;
+  } else if (%_HasFastPackedElements(array)) {

[adamk, 2015/07/16 22:08:13]

This needs an IS_ARRAY check (you can cache this above). Please add a test
covering this. This object violates the contract PackedArrayReverse() is
expecting:

{ length: 2, 0: 'foo', __proto__: { get 1() { delete this[1] } } }

Arrays can't have this problem because of their exotic 'length'.

[Toon Verwaest, 2015/07/16 22:15:32]

Do we have objects with packed elements that are not arrays?

[Dan Ehrenberg, 2015/07/16 22:16:49]

Done.

+    return PackedArrayReverse(array, len);
+  } else {
+    return InnerArrayReverse(array, len);

[adamk, 2015/07/16 22:08:13]

how about "GenericArrayReverse" as the name for this instead?

[Dan Ehrenberg, 2015/07/16 22:16:49]

Done.

   }
-
-  return InnerArrayReverse(array, len);
 }
 
 
@@ -1688,10 +1701,10 @@ utils.Export(function(to) {
   to.InnerArrayMap = InnerArrayMap;
   to.InnerArrayReduce = InnerArrayReduce;
   to.InnerArrayReduceRight = InnerArrayReduceRight;
-  to.InnerArrayReverse = InnerArrayReverse;
   to.InnerArraySome = InnerArraySome;
   to.InnerArraySort = InnerArraySort;
   to.InnerArrayToLocaleString = InnerArrayToLocaleString;
+  to.PackedArrayReverse = PackedArrayReverse;
 });
 
 $arrayConcat = ArrayConcatJS;