Properly handle holes following spreads in array literals

ajklein · Commit bot · commit e4273007b613 · 2016-09-08T18:50:41.000Z
Before this change, the spread desugaring would naively call `%AppendElement($R, the_hole)` and in some cases $R would have a non-holey elements kind, putting the array into the bad state of exposing holes to author code. This patch avoids calling %AppendElement with a hole, instead simply incrementing $R.length when it sees a hole in the literal (this is safe because $R is known to be an Array). The existing logic for elements transitions takes care of giving the array a holey ElementsKind. BUG=chromium:644215 Review-Url: https://codereview.chromium.org/2321533003 Cr-Commit-Position: refs/heads/master@{#39294}
diff --git a/src/ast/ast-value-factory.h b/src/ast/ast-value-factory.h
@@ -296,6 +296,7 @@ class AstValue : public ZoneObject {
   F(eval, "eval")                               \
   F(function, "function")                       \
   F(get_space, "get ")                          \
+  F(length, "length")                           \
   F(let, "let")                                 \
   F(native, "native")                           \
   F(new_target, ".new.target")                  \
diff --git a/src/parsing/parser.cc b/src/parsing/parser.cc
@@ -4979,15 +4979,32 @@ Expression* Parser::RewriteSpreads(ArrayLiteral* lit) {
     if (spread == nullptr) {
       // If the element is not a spread, we're adding a single:
       // %AppendElement($R, value)
-      ZoneList<Expression*>* append_element_args = NewExpressionList(2);
-      append_element_args->Add(factory()->NewVariableProxy(result), zone());
-      append_element_args->Add(value, zone());
-      do_block->statements()->Add(
-          factory()->NewExpressionStatement(
-              factory()->NewCallRuntime(Runtime::kAppendElement,
-                                        append_element_args, kNoSourcePosition),
-              kNoSourcePosition),
-          zone());
+      // or, in case of a hole,
+      // ++($R.length)
+      if (!value->IsLiteral() ||
+          !value->AsLiteral()->raw_value()->IsTheHole()) {
+        ZoneList<Expression*>* append_element_args = NewExpressionList(2);
+        append_element_args->Add(factory()->NewVariableProxy(result), zone());
+        append_element_args->Add(value, zone());
+        do_block->statements()->Add(
+            factory()->NewExpressionStatement(
+                factory()->NewCallRuntime(Runtime::kAppendElement,
+                                          append_element_args,
+                                          kNoSourcePosition),
+                kNoSourcePosition),
+            zone());
+      } else {
+        Property* length_property = factory()->NewProperty(
+            factory()->NewVariableProxy(result),
+            factory()->NewStringLiteral(ast_value_factory()->length_string(),
+                                        kNoSourcePosition),
+            kNoSourcePosition);
+        CountOperation* count_op = factory()->NewCountOperation(
+            Token::INC, true /* prefix */, length_property, kNoSourcePosition);
+        do_block->statements()->Add(
+            factory()->NewExpressionStatement(count_op, kNoSourcePosition),
+            zone());
+      }
     } else {
       // If it's a spread, we're adding a for/of loop iterating through it.
       Variable* each = NewTemporary(ast_value_factory()->dot_for_string());
diff --git a/src/runtime/runtime-object.cc b/src/runtime/runtime-object.cc
@@ -418,6 +418,7 @@ RUNTIME_FUNCTION(Runtime_AppendElement) {
 
   CONVERT_ARG_HANDLE_CHECKED(JSArray, array, 0);
   CONVERT_ARG_HANDLE_CHECKED(Object, value, 1);
+  CHECK(!value->IsTheHole(isolate));
 
   uint32_t index;
   CHECK(array->length()->ToArrayIndex(&index));
diff --git a/test/mjsunit/regress/regress-crbug-644215.js b/test/mjsunit/regress/regress-crbug-644215.js
@@ -0,0 +1,13 @@
+// Copyright 2016 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// Flags: --allow-natives-syntax
+
+var arr = [...[],,];
+assertTrue(%HasFastHoleyElements(arr));
+assertEquals(1, arr.length);
+assertFalse(arr.hasOwnProperty(0));
+assertEquals(undefined, arr[0]);
+// Should not crash.
+assertThrows(() => arr[0][0], TypeError);
