[destructuring] Get non-coercible message contents in runtime

For desrtucturing assignments from null/undefined, we throw an error
that references the destructuring object literal's property name, e.g.
for
  var { x } = null;
we report that we cannot destructure 'x' from null.

Rather than calculating this property during bytecode generation (and
including it in the bytecode as an argument to the type error
constructor), we can calculate it at exception throwing time, by
re-parsing the source in a similar way to the existing call site
rendering.

This slightly decreases bytecode size and slightly decreases the amount
of work the bytecode compiler needs to do. In the future, it could also
allow us to give more detailed error messages, as we now have access to
the entire AST and are on the slow path anyway.

Bug: v8:6499
Change-Id: Icdbd4667db548b4e5e62ef97797a3771b5c1bf72
Reviewed-on: https://chromium-review.googlesource.com/c/1396080Reviewed-by: Toon Verwaest <verwaest@chromium.org>
Reviewed-by: Sathya Gunasekaran <gsathya@chromium.org>
Commit-Queue: Leszek Swirski <leszeks@chromium.org>
Cr-Commit-Position: refs/heads/master@{#58706}

src/debug/debug-evaluate.cc

@@ -279,6 +279,7 @@ bool IntrinsicHasNoSideEffect(Runtime::FunctionId id) {
   V(ThrowInvalidStringLength)                 \
   V(ThrowIteratorError)                       \
   V(ThrowIteratorResultNotAnObject)           \
+  V(ThrowPatternAssignmentNonCoercible)       \
   V(ThrowReferenceError)                      \
   V(ThrowSymbolIteratorInvalid)               \
   /* Strings */                               \

src/interpreter/bytecode-generator.cc

@@ -3455,32 +3455,8 @@ void BytecodeGenerator::BuildDestructuringObjectAssignment(
 
   {
     builder()->Bind(&is_null_or_undefined);
-
-    // TODO(leszeks): Don't do this calculation here, but instead do it in a
-    // runtime method.
-    auto source_position = pattern->position();
-    const AstRawString* property_name = ast_string_constants()->empty_string();
-    MessageTemplate msg = MessageTemplate::kNonCoercible;
-    for (ObjectLiteralProperty* pattern_property : *pattern->properties()) {
-      Expression* key = pattern_property->key();
-      if (key->IsPropertyName()) {
-        property_name = key->AsLiteral()->AsRawPropertyName();
-        msg = MessageTemplate::kNonCoercibleWithProperty;
-        source_position = key->position();
-        break;
-      }
-    }
-
-    RegisterList new_type_error_args = register_allocator()->NewRegisterList(2);
-    // throw %NewTypeError(msg, property_name, source_position)
-    builder()->SetExpressionPosition(source_position);
-    builder()
-        ->LoadLiteral(Smi::FromEnum(msg))
-        .StoreAccumulatorInRegister(new_type_error_args[0])
-        .LoadLiteral(property_name)
-        .StoreAccumulatorInRegister(new_type_error_args[1])
-        .CallRuntime(Runtime::kNewTypeError, new_type_error_args)
-        .Throw();
+    builder()->SetExpressionPosition(pattern);
+    builder()->CallRuntime(Runtime::kThrowPatternAssignmentNonCoercible);
   }
 
   // Store the assignment value in a register.

src/runtime/runtime-internal.cc

@@ -6,6 +6,7 @@
 
 #include "src/api.h"
 #include "src/arguments-inl.h"
+#include "src/ast/ast-traversal-visitor.h"
 #include "src/ast/prettyprinter.h"
 #include "src/bootstrapper.h"
 #include "src/builtins/builtins.h"
@@ -426,6 +427,94 @@ RUNTIME_FUNCTION(Runtime_ThrowConstructedNonConstructable) {
   THROW_NEW_ERROR_RETURN_FAILURE(isolate, NewTypeError(id, callsite));
 }
 
+namespace {
+
+// Helper visitor for ThrowPatternAssignmentNonCoercible which finds an
+// object literal (representing a destructuring assignment) at a given source
+// position.
+class PatternFinder final : public AstTraversalVisitor<PatternFinder> {
+ public:
+  PatternFinder(Isolate* isolate, Expression* root, int position)
+      : AstTraversalVisitor(isolate, root),
+        position_(position),
+        object_literal_(nullptr) {}
+
+  ObjectLiteral* object_literal() const { return object_literal_; }
+
+ private:
+  // This is required so that the overriden Visit* methods can be
+  // called by the base class (template).
+  friend class AstTraversalVisitor<PatternFinder>;
+
+  void VisitObjectLiteral(ObjectLiteral* lit) {
+    // TODO(leszeks): This could be smarter in only traversing object literals
+    // that are known to be a destructuring pattern. We could then also
+    // potentially find the corresponding assignment value and report that too.
+    if (lit->position() == position_) {
+      object_literal_ = lit;
+      return;
+    }
+    AstTraversalVisitor::VisitObjectLiteral(lit);
+  }
+
+  int position_;
+  ObjectLiteral* object_literal_;
+};
+
+}  // namespace
+
+RUNTIME_FUNCTION(Runtime_ThrowPatternAssignmentNonCoercible) {
+  HandleScope scope(isolate);
+  DCHECK_EQ(0, args.length());
+
+  // Find the object literal representing the destructuring assignment, so that
+  // we can try to attribute the error to a property name on it rather than to
+  // the literal itself.
+  MaybeHandle<String> maybe_property_name;
+  MessageLocation location;
+  if (ComputeLocation(isolate, &location)) {
+    ParseInfo info(isolate, location.shared());
+    if (parsing::ParseAny(&info, location.shared(), isolate)) {
+      info.ast_value_factory()->Internalize(isolate);
+
+      PatternFinder finder(isolate, info.literal(), location.start_pos());
+      finder.Run();
+      if (finder.object_literal()) {
+        for (ObjectLiteralProperty* pattern_property :
+             *finder.object_literal()->properties()) {
+          Expression* key = pattern_property->key();
+          if (key->IsPropertyName()) {
+            int pos = key->position();
+            maybe_property_name =
+                key->AsLiteral()->AsRawPropertyName()->string();
+            // Change the message location to point at the property name.
+            location = MessageLocation(location.script(), pos, pos + 1,
+                                       location.shared());
+            break;
+          }
+        }
+      }
+    } else {
+      isolate->clear_pending_exception();
+    }
+  }
+
+  // Create a "non-coercible" type error with a property name if one is
+  // available, otherwise create a generic one.
+  Handle<Object> error;
+  Handle<String> property_name;
+  if (maybe_property_name.ToHandle(&property_name)) {
+    error = isolate->factory()->NewTypeError(
+        MessageTemplate::kNonCoercibleWithProperty, property_name);
+  } else {
+    error = isolate->factory()->NewTypeError(MessageTemplate::kNonCoercible);
+  }
+
+  // Explicitly pass the calculated location, as we may have updated it to match
+  // the property name.
+  return isolate->Throw(*error, &location);
+}
+
 RUNTIME_FUNCTION(Runtime_ThrowConstructorReturnedNonObject) {
   HandleScope scope(isolate);
   DCHECK_EQ(0, args.length());

src/runtime/runtime.h

@@ -241,6 +241,7 @@ namespace internal {
   F(ThrowIteratorError, 1, 1)                        \
   F(ThrowIteratorResultNotAnObject, 1, 1)            \
   F(ThrowNotConstructor, 1, 1)                       \
+  F(ThrowPatternAssignmentNonCoercible, 0, 1)        \
   F(ThrowRangeError, -1 /* >= 1 */, 1)               \
   F(ThrowReferenceError, 1, 1)                       \
   F(ThrowStackOverflow, 0, 1)                        \

test/cctest/interpreter/bytecode_expectations/DestructuringAssignment.golden

@@ -376,23 +376,18 @@ snippet: "
   var x, a = {x:1};
   ({x} = a);
 "
-frame size: 5
+frame size: 3
 parameter count: 1
-bytecode array length: 35
+bytecode array length: 26
 bytecodes: [
   /*   30 E> */ B(StackCheck),
   /*   45 S> */ B(CreateObjectLiteral), U8(0), U8(0), U8(41),
                 B(Star), R(1),
   /*   52 S> */ B(JumpIfNull), U8(4),
-                B(JumpIfNotUndefined), U8(16),
-                B(LdaSmi), I8(81),
+                B(JumpIfNotUndefined), U8(7),
+  /*   53 E> */ B(CallRuntime), U16(Runtime::kThrowPatternAssignmentNonCoercible), R(0), U8(0),
                 B(Star), R(2),
-                B(LdaConstant), U8(1),
-                B(Star), R(3),
-  /*   54 E> */ B(CallRuntime), U16(Runtime::kNewTypeError), R(2), U8(2),
-                B(Throw),
-                B(Star), R(4),
-  /*   54 S> */ B(LdaNamedProperty), R(4), U8(1), U8(1),
+  /*   54 S> */ B(LdaNamedProperty), R(2), U8(1), U8(1),
                 B(Star), R(0),
                 B(LdaUndefined),
   /*   63 S> */ B(Return),
@@ -409,9 +404,9 @@ snippet: "
   var x={}, a = {y:1};
   ({y:x.foo} = a);
 "
-frame size: 5
+frame size: 3
 parameter count: 1
-bytecode array length: 40
+bytecode array length: 31
 bytecodes: [
   /*   30 E> */ B(StackCheck),
   /*   40 S> */ B(CreateEmptyObjectLiteral),
@@ -419,15 +414,10 @@ bytecodes: [
   /*   48 S> */ B(CreateObjectLiteral), U8(0), U8(0), U8(41),
                 B(Star), R(1),
   /*   55 S> */ B(JumpIfNull), U8(4),
-                B(JumpIfNotUndefined), U8(16),
-                B(LdaSmi), I8(81),
-                B(Star), R(2),
-                B(LdaConstant), U8(1),
-                B(Star), R(3),
-  /*   57 E> */ B(CallRuntime), U16(Runtime::kNewTypeError), R(2), U8(2),
-                B(Throw),
-  /*   61 S> */ B(Star), R(4),
-                B(LdaNamedProperty), R(4), U8(1), U8(1),
+                B(JumpIfNotUndefined), U8(7),
+  /*   56 E> */ B(CallRuntime), U16(Runtime::kThrowPatternAssignmentNonCoercible), R(0), U8(0),
+  /*   61 S> */ B(Star), R(2),
+                B(LdaNamedProperty), R(2), U8(1), U8(1),
                 B(StaNamedProperty), R(0), U8(2), U8(3),
                 B(LdaUndefined),
   /*   72 S> */ B(Return),
@@ -445,29 +435,24 @@ snippet: "
   var x, a = {y:1, w:2, v:3};
   ({x=0,...y} = a);
 "
-frame size: 6
+frame size: 4
 parameter count: 1
-bytecode array length: 50
+bytecode array length: 41
 bytecodes: [
   /*   30 E> */ B(StackCheck),
   /*   45 S> */ B(CreateObjectLiteral), U8(0), U8(0), U8(41),
                 B(Star), R(1),
   /*   62 S> */ B(JumpIfNull), U8(4),
-                B(JumpIfNotUndefined), U8(16),
-                B(LdaSmi), I8(81),
+                B(JumpIfNotUndefined), U8(7),
+  /*   63 E> */ B(CallRuntime), U16(Runtime::kThrowPatternAssignmentNonCoercible), R(0), U8(0),
                 B(Star), R(2),
-                B(LdaConstant), U8(1),
-                B(Star), R(3),
-  /*   64 E> */ B(CallRuntime), U16(Runtime::kNewTypeError), R(2), U8(2),
-                B(Throw),
-                B(Star), R(4),
   /*   64 S> */ B(LdaConstant), U8(1),
-                B(Star), R(5),
-                B(LdaNamedProperty), R(4), U8(1), U8(1),
+                B(Star), R(3),
+                B(LdaNamedProperty), R(2), U8(1), U8(1),
                 B(JumpIfNotUndefined), U8(3),
                 B(LdaZero),
                 B(Star), R(0),
-  /*   71 S> */ B(CallRuntime), U16(Runtime::kCopyDataPropertiesWithExcludedProperties), R(4), U8(2),
+  /*   71 S> */ B(CallRuntime), U16(Runtime::kCopyDataPropertiesWithExcludedProperties), R(2), U8(2),
                 B(StaGlobal), U8(2), U8(3),
                 B(LdaUndefined),
   /*   80 S> */ B(Return),

test/cctest/interpreter/bytecode_expectations/ForOfLoop.golden

@@ -458,9 +458,9 @@ snippet: "
   }
   f([{ x: 0, y: 3 }, { x: 1, y: 9 }, { x: -12, y: 17 }]);
 "
-frame size: 20
+frame size: 19
 parameter count: 2
-bytecode array length: 280
+bytecode array length: 271
 bytecodes: [
   /*   10 E> */ B(StackCheck),
                 B(LdaZero),
@@ -483,7 +483,7 @@ bytecodes: [
                 B(JumpIfFalse), U8(7),
                 B(CallRuntime), U16(Runtime::kThrowIteratorResultNotAnObject), R(8), U8(1),
                 B(LdaNamedProperty), R(8), U8(2), U8(8),
-                B(JumpIfToBooleanTrue), U8(63),
+                B(JumpIfToBooleanTrue), U8(54),
                 B(LdaNamedProperty), R(8), U8(3), U8(10),
                 B(Star), R(10),
                 B(LdaSmi), I8(2),
@@ -492,24 +492,19 @@ bytecodes: [
   /*   20 E> */ B(StackCheck),
   /*   36 S> */ B(Ldar), R(5),
                 B(JumpIfNull), U8(4),
-                B(JumpIfNotUndefined), U8(16),
-                B(LdaSmi), I8(81),
+                B(JumpIfNotUndefined), U8(7),
+  /*   29 E> */ B(CallRuntime), U16(Runtime::kThrowPatternAssignmentNonCoercible), R(0), U8(0),
                 B(Star), R(17),
-                B(LdaConstant), U8(4),
-                B(Star), R(18),
-  /*   31 E> */ B(CallRuntime), U16(Runtime::kNewTypeError), R(17), U8(2),
-                B(Throw),
-                B(Star), R(19),
-  /*   31 S> */ B(LdaNamedProperty), R(19), U8(4), U8(12),
+  /*   31 S> */ B(LdaNamedProperty), R(17), U8(4), U8(12),
                 B(Star), R(1),
-  /*   34 S> */ B(LdaNamedProperty), R(19), U8(5), U8(14),
+  /*   34 S> */ B(LdaNamedProperty), R(17), U8(5), U8(14),
                 B(Star), R(2),
   /*   56 S> */ B(Ldar), R(2),
   /*   58 E> */ B(Add), R(1), U8(16),
                 B(Star), R(0),
                 B(LdaZero),
                 B(Star), R(9),
-                B(JumpLoop), U8(82), I8(0),
+                B(JumpLoop), U8(73), I8(0),
                 B(Jump), U8(33),
                 B(Star), R(17),
   /*   56 E> */ B(CreateCatchContext), R(17), U8(6),
@@ -594,9 +589,9 @@ constant pool: [
   ONE_BYTE_INTERNALIZED_STRING_TYPE [""],
 ]
 handlers: [
-  [7, 156, 164],
-  [10, 123, 125],
-  [224, 234, 236],
+  [7, 147, 155],
+  [10, 114, 116],
+  [215, 225, 227],
 ]
 
 ---

test/cctest/interpreter/bytecode_expectations/StandardForLoop.golden

@@ -218,24 +218,19 @@ snippet: "
   }
   f();
 "
-frame size: 6
+frame size: 4
 parameter count: 1
-bytecode array length: 62
+bytecode array length: 53
 bytecodes: [
   /*   10 E> */ B(StackCheck),
   /*   37 S> */ B(CreateObjectLiteral), U8(0), U8(0), U8(41),
                 B(JumpIfNull), U8(4),
-                B(JumpIfNotUndefined), U8(16),
-                B(LdaSmi), I8(81),
+                B(JumpIfNotUndefined), U8(7),
+  /*   26 E> */ B(CallRuntime), U16(Runtime::kThrowPatternAssignmentNonCoercible), R(0), U8(0),
                 B(Star), R(3),
-                B(LdaConstant), U8(1),
-                B(Star), R(4),
-  /*   28 E> */ B(CallRuntime), U16(Runtime::kNewTypeError), R(3), U8(2),
-                B(Throw),
-                B(Star), R(5),
-  /*   28 S> */ B(LdaNamedProperty), R(5), U8(1), U8(1),
+  /*   28 S> */ B(LdaNamedProperty), R(3), U8(1), U8(1),
                 B(Star), R(1),
-  /*   31 S> */ B(LdaNamedProperty), R(5), U8(2), U8(3),
+  /*   31 S> */ B(LdaNamedProperty), R(3), U8(2), U8(3),
                 B(Star), R(2),
   /*   55 S> */ B(LdaZero),
   /*   55 E> */ B(TestGreaterThan), R(2), U8(5),

test/message/fail/destructuring-undefined-string-property.out

@@ -2,4 +2,4 @@
 var { x } = undefined;
       ^
 TypeError: Cannot destructure property `x` of 'undefined' or 'null'.
-    at *%(basename)s:5:7
+    at *%(basename)s:5:5