[destructuring] Elide coercible check for simple keys

Simple object destructuring, such as `let {a,b} = o`, is less efficient
than the equivalent assignments `let a = o.a; let b = o.b`. This is
because it does a nil check of `o` before the assignments. However, this
nil check is not strictly necessary for simple (i.e. non-computed) names,
as there will be an equivalent nil check on the first access to o in
`o.a`. For computed names the computation is unfortunately obervable.

So, we can elide the nil check when the first property (if any) of the
destructuring target is a non-computed name. This messes a bit with our
error messages, so we re-use the CallPrinter to also find destructuring
assignment based errors, and fiddle with the error message there. As
a side-effect, we also get out the object name in the AST, so we can
output a slightly nicer error message.

Change-Id: Iafa858e27ed771a146cd3ba57903cc73bb46951d
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1773254Reviewed-by: Leszek Swirski <leszeks@chromium.org>
Reviewed-by: Toon Verwaest <verwaest@chromium.org>
Commit-Queue: Leszek Swirski <leszeks@chromium.org>
Cr-Commit-Position: refs/heads/master@{#63453}

src/ast/ast.h

@@ -1405,7 +1405,6 @@ class ObjectLiteral final : public AggregateLiteral {
   void set_has_null_protoype(bool has_null_prototype) {
     bit_field_ = HasNullPrototypeField::update(bit_field_, has_null_prototype);
   }
-
   uint32_t boilerplate_properties_;
   Handle<ObjectBoilerplateDescription> boilerplate_description_;
   ZoneList<Property*> properties_;

src/ast/prettyprinter.cc

@@ -27,6 +27,8 @@ CallPrinter::CallPrinter(Isolate* isolate, bool is_user_js)
   is_call_error_ = false;
   is_iterator_error_ = false;
   is_async_iterator_error_ = false;
+  destructuring_prop_ = nullptr;
+  destructuring_assignment_ = nullptr;
   is_user_js_ = is_user_js;
   function_kind_ = kNormalFunction;
   InitializeAstVisitor(isolate);
@@ -299,6 +301,26 @@ void CallPrinter::VisitVariableProxy(VariableProxy* node) {
 
 
 void CallPrinter::VisitAssignment(Assignment* node) {
+  bool was_found = false;
+  if (node->target()->IsObjectLiteral()) {
+    ObjectLiteral* target = node->target()->AsObjectLiteral();
+    if (target->position() == position_) {
+      was_found = !found_;
+      found_ = true;
+      destructuring_assignment_ = node;
+    } else {
+      for (ObjectLiteralProperty* prop : *target->properties()) {
+        if (prop->value()->position() == position_) {
+          was_found = !found_;
+          found_ = true;
+          destructuring_prop_ = prop;
+          destructuring_assignment_ = node;
+          break;
+        }
+      }
+    }
+  }
+  if (!was_found) {
     Find(node->target());
     if (node->target()->IsArrayLiteral()) {
       // Special case the visit for destructuring array assignment.
@@ -306,10 +328,8 @@ void CallPrinter::VisitAssignment(Assignment* node) {
       if (node->value()->position() == position_) {
         is_iterator_error_ = true;
         was_found = !found_;
-      if (was_found) {
         found_ = true;
       }
-    }
       Find(node->value(), true);
       if (was_found) {
         done_ = true;
@@ -318,6 +338,14 @@ void CallPrinter::VisitAssignment(Assignment* node) {
     } else {
       Find(node->value());
     }
+  } else {
+    Find(node->value(), true);
+  }
+
+  if (was_found) {
+    done_ = true;
+    found_ = false;
+  }
 }
 
 void CallPrinter::VisitCompoundAssignment(CompoundAssignment* node) {

src/ast/prettyprinter.h

@@ -31,6 +31,12 @@ class CallPrinter final : public AstVisitor<CallPrinter> {
     kCallAndAsyncIterator
   };
   ErrorHint GetErrorHint() const;
+  ObjectLiteralProperty* destructuring_prop() const {
+    return destructuring_prop_;
+  }
+  Assignment* destructuring_assignment() const {
+    return destructuring_assignment_;
+  }
 
 // Individual nodes
 #define DECLARE_VISIT(type) void Visit##type(type* node);
@@ -54,6 +60,8 @@ class CallPrinter final : public AstVisitor<CallPrinter> {
   bool is_iterator_error_;
   bool is_async_iterator_error_;
   bool is_call_error_;
+  ObjectLiteralProperty* destructuring_prop_;
+  Assignment* destructuring_assignment_;
   FunctionKind function_kind_;
   DEFINE_AST_VISITOR_SUBCLASS_MEMBERS();
 

src/common/message-template.h

@@ -118,9 +118,9 @@ namespace internal {
   T(NoAccess, "no access")                                                     \
   T(NonCallableInInstanceOfCheck,                                              \
     "Right-hand side of 'instanceof' is not callable")                         \
-  T(NonCoercible, "Cannot destructure 'undefined' or 'null'.")                 \
+  T(NonCoercible, "Cannot destructure '%' as it is %.")                        \
   T(NonCoercibleWithProperty,                                                  \
-    "Cannot destructure property `%` of 'undefined' or 'null'.")               \
+    "Cannot destructure property '%' of '%' as it is %.")                      \
   T(NonExtensibleProto, "% is not extensible")                                 \
   T(NonObjectInInstanceOfCheck,                                                \
     "Right-hand side of 'instanceof' is not an object")                        \

src/execution/messages.cc

@@ -7,8 +7,11 @@
 #include <memory>
 
 #include "src/api/api-inl.h"
+#include "src/ast/ast.h"
+#include "src/ast/prettyprinter.h"
 #include "src/base/v8-fallthrough.h"
 #include "src/execution/execution.h"
+#include "src/execution/frames-inl.h"
 #include "src/execution/frames.h"
 #include "src/execution/isolate-inl.h"
 #include "src/logging/counters.h"
@@ -18,6 +21,9 @@
 #include "src/objects/keys.h"
 #include "src/objects/stack-frame-info-inl.h"
 #include "src/objects/struct-inl.h"
+#include "src/parsing/parse-info.h"
+#include "src/parsing/parsing.h"
+#include "src/roots/roots.h"
 #include "src/strings/string-builder-inl.h"
 #include "src/wasm/wasm-code-manager.h"
 #include "src/wasm/wasm-objects.h"
@@ -1166,5 +1172,232 @@ MaybeHandle<Object> ErrorUtils::MakeGenericError(
                                no_caller, StackTraceCollection::kDetailed);
 }
 
+namespace {
+
+bool ComputeLocation(Isolate* isolate, MessageLocation* target) {
+  JavaScriptFrameIterator it(isolate);
+  if (!it.done()) {
+    // Compute the location from the function and the relocation info of the
+    // baseline code. For optimized code this will use the deoptimization
+    // information to get canonical location information.
+    std::vector<FrameSummary> frames;
+    it.frame()->Summarize(&frames);
+    auto& summary = frames.back().AsJavaScript();
+    Handle<SharedFunctionInfo> shared(summary.function()->shared(), isolate);
+    Handle<Object> script(shared->script(), isolate);
+    SharedFunctionInfo::EnsureSourcePositionsAvailable(isolate, shared);
+    int pos = summary.abstract_code()->SourcePosition(summary.code_offset());
+    if (script->IsScript() &&
+        !(Handle<Script>::cast(script)->source().IsUndefined(isolate))) {
+      Handle<Script> casted_script = Handle<Script>::cast(script);
+      *target = MessageLocation(casted_script, pos, pos + 1, shared);
+      return true;
+    }
+  }
+  return false;
+}
+
+Handle<String> BuildDefaultCallSite(Isolate* isolate, Handle<Object> object) {
+  IncrementalStringBuilder builder(isolate);
+
+  builder.AppendString(Object::TypeOf(isolate, object));
+  if (object->IsString()) {
+    builder.AppendCString(" \"");
+    builder.AppendString(Handle<String>::cast(object));
+    builder.AppendCString("\"");
+  } else if (object->IsNull(isolate)) {
+    builder.AppendCString(" ");
+    builder.AppendString(isolate->factory()->null_string());
+  } else if (object->IsTrue(isolate)) {
+    builder.AppendCString(" ");
+    builder.AppendString(isolate->factory()->true_string());
+  } else if (object->IsFalse(isolate)) {
+    builder.AppendCString(" ");
+    builder.AppendString(isolate->factory()->false_string());
+  } else if (object->IsNumber()) {
+    builder.AppendCString(" ");
+    builder.AppendString(isolate->factory()->NumberToString(object));
+  }
+
+  return builder.Finish().ToHandleChecked();
+}
+
+Handle<String> RenderCallSite(Isolate* isolate, Handle<Object> object,
+                              MessageLocation* location,
+                              CallPrinter::ErrorHint* hint) {
+  if (ComputeLocation(isolate, location)) {
+    ParseInfo info(isolate, location->shared());
+    if (parsing::ParseAny(&info, location->shared(), isolate)) {
+      info.ast_value_factory()->Internalize(isolate);
+      CallPrinter printer(isolate, location->shared()->IsUserJavaScript());
+      Handle<String> str = printer.Print(info.literal(), location->start_pos());
+      *hint = printer.GetErrorHint();
+      if (str->length() > 0) return str;
+    } else {
+      isolate->clear_pending_exception();
+    }
+  }
+  return BuildDefaultCallSite(isolate, object);
+}
+
+MessageTemplate UpdateErrorTemplate(CallPrinter::ErrorHint hint,
+                                    MessageTemplate default_id) {
+  switch (hint) {
+    case CallPrinter::ErrorHint::kNormalIterator:
+      return MessageTemplate::kNotIterable;
+
+    case CallPrinter::ErrorHint::kCallAndNormalIterator:
+      return MessageTemplate::kNotCallableOrIterable;
+
+    case CallPrinter::ErrorHint::kAsyncIterator:
+      return MessageTemplate::kNotAsyncIterable;
+
+    case CallPrinter::ErrorHint::kCallAndAsyncIterator:
+      return MessageTemplate::kNotCallableOrAsyncIterable;
+
+    case CallPrinter::ErrorHint::kNone:
+      return default_id;
+  }
+  return default_id;
+}
+
+}  // namespace
+
+Handle<Object> ErrorUtils::NewIteratorError(Isolate* isolate,
+                                            Handle<Object> source) {
+  MessageLocation location;
+  CallPrinter::ErrorHint hint = CallPrinter::kNone;
+  Handle<String> callsite = RenderCallSite(isolate, source, &location, &hint);
+  MessageTemplate id = MessageTemplate::kNotIterableNoSymbolLoad;
+
+  if (hint == CallPrinter::kNone) {
+    Handle<Symbol> iterator_symbol = isolate->factory()->iterator_symbol();
+    return isolate->factory()->NewTypeError(id, callsite, iterator_symbol);
+  }
+
+  id = UpdateErrorTemplate(hint, id);
+  return isolate->factory()->NewTypeError(id, callsite);
+}
+
+Handle<Object> ErrorUtils::NewCalledNonCallableError(Isolate* isolate,
+                                                     Handle<Object> source) {
+  MessageLocation location;
+  CallPrinter::ErrorHint hint = CallPrinter::kNone;
+  Handle<String> callsite = RenderCallSite(isolate, source, &location, &hint);
+  MessageTemplate id = MessageTemplate::kCalledNonCallable;
+  id = UpdateErrorTemplate(hint, id);
+  return isolate->factory()->NewTypeError(id, callsite);
+}
+
+Handle<Object> ErrorUtils::NewConstructedNonConstructable(
+    Isolate* isolate, Handle<Object> source) {
+  MessageLocation location;
+  CallPrinter::ErrorHint hint = CallPrinter::kNone;
+  Handle<String> callsite = RenderCallSite(isolate, source, &location, &hint);
+  MessageTemplate id = MessageTemplate::kNotConstructor;
+  return isolate->factory()->NewTypeError(id, callsite);
+}
+
+Object ErrorUtils::ThrowLoadFromNullOrUndefined(Isolate* isolate,
+                                                Handle<Object> object) {
+  return ThrowLoadFromNullOrUndefined(isolate, object, MaybeHandle<Object>());
+}
+Object ErrorUtils::ThrowLoadFromNullOrUndefined(Isolate* isolate,
+                                                Handle<Object> object,
+                                                MaybeHandle<Object> key) {
+  DCHECK(object->IsNullOrUndefined());
+
+  MaybeHandle<String> maybe_property_name;
+
+  // Try to extract the property name from the given key, if any.
+  Handle<Object> key_handle;
+  if (key.ToHandle(&key_handle)) {
+    if (key_handle->IsString()) {
+      maybe_property_name = Handle<String>::cast(key_handle);
+    }
+  }
+
+  Handle<String> callsite;
+
+  // Inline the RenderCallSite logic here so that we can additonally access the
+  // destructuring property.
+  bool location_computed = false;
+  bool is_destructuring = false;
+  MessageLocation location;
+  if (ComputeLocation(isolate, &location)) {
+    location_computed = true;
+
+    ParseInfo info(isolate, location.shared());
+    if (parsing::ParseAny(&info, location.shared(), isolate)) {
+      info.ast_value_factory()->Internalize(isolate);
+      CallPrinter printer(isolate, location.shared()->IsUserJavaScript());
+      Handle<String> str = printer.Print(info.literal(), location.start_pos());
+
+      int pos = -1;
+      is_destructuring = printer.destructuring_assignment() != nullptr;
+
+      if (is_destructuring) {
+        // If we don't have one yet, try to extract the property name from the
+        // destructuring property in the AST.
+        ObjectLiteralProperty* destructuring_prop =
+            printer.destructuring_prop();
+        if (maybe_property_name.is_null() && destructuring_prop != nullptr &&
+            destructuring_prop->key()->IsPropertyName()) {
+          maybe_property_name = destructuring_prop->key()
+                                    ->AsLiteral()
+                                    ->AsRawPropertyName()
+                                    ->string();
+          // Change the message location to point at the property name.
+          pos = destructuring_prop->key()->position();
+        }
+        if (maybe_property_name.is_null()) {
+          // Change the message location to point at the destructured value.
+          pos = printer.destructuring_assignment()->value()->position();
+        }
+
+        // If we updated the pos to a valid pos, rewrite the location.
+        if (pos != -1) {
+          location = MessageLocation(location.script(), pos, pos + 1,
+                                     location.shared());
+        }
+      }
+
+      if (str->length() > 0) callsite = str;
+    } else {
+      isolate->clear_pending_exception();
+    }
+  }
+
+  if (callsite.is_null()) {
+    callsite = BuildDefaultCallSite(isolate, object);
+  }
+
+  Handle<Object> error;
+  Handle<String> property_name;
+  if (is_destructuring) {
+    if (maybe_property_name.ToHandle(&property_name)) {
+      error = isolate->factory()->NewTypeError(
+          MessageTemplate::kNonCoercibleWithProperty, property_name, callsite,
+          object);
+    } else {
+      error = isolate->factory()->NewTypeError(MessageTemplate::kNonCoercible,
+                                               callsite, object);
+    }
+  } else {
+    Handle<Object> key_handle;
+    if (!key.ToHandle(&key_handle)) {
+      key_handle = ReadOnlyRoots(isolate).undefined_value_handle();
+    }
+    if (*key_handle == ReadOnlyRoots(isolate).iterator_symbol()) {
+      error = NewIteratorError(isolate, object);
+    } else {
+      error = isolate->factory()->NewTypeError(
+          MessageTemplate::kNonObjectPropertyLoad, key_handle, object);
+    }
+  }
+
+  return isolate->Throw(*error, location_computed ? &location : nullptr);
+}
+
 }  // namespace internal
 }  // namespace v8

src/execution/messages.h

@@ -283,6 +283,18 @@ class ErrorUtils : public AllStatic {
   static MaybeHandle<Object> FormatStackTrace(Isolate* isolate,
                                               Handle<JSObject> error,
                                               Handle<Object> stack_trace);
+
+  static Handle<Object> NewIteratorError(Isolate* isolate,
+                                         Handle<Object> source);
+  static Handle<Object> NewCalledNonCallableError(Isolate* isolate,
+                                                  Handle<Object> source);
+  static Handle<Object> NewConstructedNonConstructable(Isolate* isolate,
+                                                       Handle<Object> source);
+  static Object ThrowLoadFromNullOrUndefined(Isolate* isolate,
+                                             Handle<Object> object);
+  static Object ThrowLoadFromNullOrUndefined(Isolate* isolate,
+                                             Handle<Object> object,
+                                             MaybeHandle<Object> key);
 };
 
 class MessageFormatter {

src/ic/ic.cc

@@ -14,6 +14,7 @@
 #include "src/execution/execution.h"
 #include "src/execution/frames-inl.h"
 #include "src/execution/isolate-inl.h"
+#include "src/execution/runtime-profiler.h"
 #include "src/handles/handles-inl.h"
 #include "src/ic/call-optimization.h"
 #include "src/ic/handler-configuration-inl.h"
@@ -28,14 +29,13 @@
 #include "src/objects/heap-number-inl.h"
 #include "src/objects/js-array-inl.h"
 #include "src/objects/module-inl.h"
-#include "src/objects/struct-inl.h"
-#include "src/utils/ostreams.h"
-#include "src/execution/runtime-profiler.h"
 #include "src/objects/prototype.h"
+#include "src/objects/struct-inl.h"
 #include "src/runtime/runtime-utils.h"
 #include "src/runtime/runtime.h"
 #include "src/tracing/trace-event.h"
 #include "src/tracing/tracing-category-observer.h"
+#include "src/utils/ostreams.h"
 
 namespace v8 {
 namespace internal {
@@ -391,11 +391,17 @@ MaybeHandle<Object> LoadIC::Load(Handle<Object> object, Handle<Name> name) {
     }
 
     if (*name == ReadOnlyRoots(isolate()).iterator_symbol()) {
-      return Runtime::ThrowIteratorError(isolate(), object);
+      return isolate()->Throw<Object>(
+          ErrorUtils::NewIteratorError(isolate(), object));
+    }
+
+    if (IsAnyHas()) {
+      return TypeError(MessageTemplate::kInvalidInOperatorUse, object, name);
+    } else {
+      DCHECK(object->IsNullOrUndefined(isolate()));
+      ErrorUtils::ThrowLoadFromNullOrUndefined(isolate(), object, name);
+      return MaybeHandle<Object>();
     }
-    return TypeError(IsAnyHas() ? MessageTemplate::kInvalidInOperatorUse
-                                : MessageTemplate::kNonObjectPropertyLoad,
-                     object, name);
   }
 
   if (MigrateDeprecated(isolate(), object)) use_ic = false;

src/interpreter/bytecode-generator.cc

@@ -3713,33 +3713,44 @@ void BytecodeGenerator::BuildDestructuringObjectAssignment(
     LookupHoistingMode lookup_hoisting_mode) {
   RegisterAllocationScope scope(this);
 
+  // Store the assignment value in a register.
+  Register value;
+  RegisterList rest_runtime_callargs;
+  if (pattern->has_rest_property()) {
+    rest_runtime_callargs =
+        register_allocator()->NewRegisterList(pattern->properties()->length());
+    value = rest_runtime_callargs[0];
+  } else {
+    value = register_allocator()->NewRegister();
+  }
+  builder()->StoreAccumulatorInRegister(value);
+
   // if (value === null || value === undefined)
   //   throw new TypeError(kNonCoercible);
   //
-  // TODO(leszeks): Eliminate check if value is known to be non-null (e.g.
-  // an object literal).
+  // Since the first property access on null/undefined will also trigger a
+  // TypeError, we can elide this check. The exception is when there are no
+  // properties and no rest property (this is an empty literal), or when the
+  // first property is a computed name and accessing it can have side effects.
+  //
+  // TODO(leszeks): Also eliminate this check if the value is known to be
+  // non-null (e.g. an object literal).
+  if (pattern->properties()->is_empty() ||
+      (pattern->properties()->at(0)->is_computed_name() &&
+       pattern->properties()->at(0)->kind() != ObjectLiteralProperty::SPREAD)) {
     BytecodeLabel is_null_or_undefined, not_null_or_undefined;
     builder()
-      ->JumpIfNull(&is_null_or_undefined)
-      .JumpIfNotUndefined(&not_null_or_undefined);
+        ->JumpIfUndefinedOrNull(&is_null_or_undefined)
+        .Jump(&not_null_or_undefined);
 
     {
       builder()->Bind(&is_null_or_undefined);
       builder()->SetExpressionPosition(pattern);
-    builder()->CallRuntime(Runtime::kThrowPatternAssignmentNonCoercible);
+      builder()->CallRuntime(Runtime::kThrowPatternAssignmentNonCoercible,
+                             value);
     }
-
-  // Store the assignment value in a register.
-  Register value;
-  RegisterList rest_runtime_callargs;
-  if (pattern->has_rest_property()) {
-    rest_runtime_callargs =
-        register_allocator()->NewRegisterList(pattern->properties()->length());
-    value = rest_runtime_callargs[0];
-  } else {
-    value = register_allocator()->NewRegister();
+    builder()->Bind(&not_null_or_undefined);
   }
-  builder()->Bind(&not_null_or_undefined).StoreAccumulatorInRegister(value);
 
   int i = 0;
   for (ObjectLiteralProperty* pattern_property : *pattern->properties()) {

src/runtime/runtime-internal.cc

@@ -13,6 +13,8 @@
 #include "src/execution/arguments-inl.h"
 #include "src/execution/frames-inl.h"
 #include "src/execution/isolate-inl.h"
+#include "src/execution/messages.h"
+#include "src/handles/maybe-handles.h"
 #include "src/init/bootstrapper.h"
 #include "src/logging/counters.h"
 #include "src/numbers/conversions.h"
@@ -374,228 +376,35 @@ RUNTIME_FUNCTION(Runtime_AllocateSeqTwoByteString) {
   return *result;
 }
 
-namespace {
-
-bool ComputeLocation(Isolate* isolate, MessageLocation* target) {
-  JavaScriptFrameIterator it(isolate);
-  if (!it.done()) {
-    // Compute the location from the function and the relocation info of the
-    // baseline code. For optimized code this will use the deoptimization
-    // information to get canonical location information.
-    std::vector<FrameSummary> frames;
-    it.frame()->Summarize(&frames);
-    auto& summary = frames.back().AsJavaScript();
-    Handle<SharedFunctionInfo> shared(summary.function()->shared(), isolate);
-    Handle<Object> script(shared->script(), isolate);
-    SharedFunctionInfo::EnsureSourcePositionsAvailable(isolate, shared);
-    int pos = summary.abstract_code()->SourcePosition(summary.code_offset());
-    if (script->IsScript() &&
-        !(Handle<Script>::cast(script)->source().IsUndefined(isolate))) {
-      Handle<Script> casted_script = Handle<Script>::cast(script);
-      *target = MessageLocation(casted_script, pos, pos + 1, shared);
-      return true;
-    }
-  }
-  return false;
-}
-
-Handle<String> BuildDefaultCallSite(Isolate* isolate, Handle<Object> object) {
-  IncrementalStringBuilder builder(isolate);
-
-  builder.AppendString(Object::TypeOf(isolate, object));
-  if (object->IsString()) {
-    builder.AppendCString(" \"");
-    builder.AppendString(Handle<String>::cast(object));
-    builder.AppendCString("\"");
-  } else if (object->IsNull(isolate)) {
-    builder.AppendCString(" ");
-    builder.AppendString(isolate->factory()->null_string());
-  } else if (object->IsTrue(isolate)) {
-    builder.AppendCString(" ");
-    builder.AppendString(isolate->factory()->true_string());
-  } else if (object->IsFalse(isolate)) {
-    builder.AppendCString(" ");
-    builder.AppendString(isolate->factory()->false_string());
-  } else if (object->IsNumber()) {
-    builder.AppendCString(" ");
-    builder.AppendString(isolate->factory()->NumberToString(object));
-  }
-
-  return builder.Finish().ToHandleChecked();
-}
-
-Handle<String> RenderCallSite(Isolate* isolate, Handle<Object> object,
-                              CallPrinter::ErrorHint* hint) {
-  MessageLocation location;
-  if (ComputeLocation(isolate, &location)) {
-    ParseInfo info(isolate, location.shared());
-    if (parsing::ParseAny(&info, location.shared(), isolate)) {
-      info.ast_value_factory()->Internalize(isolate);
-      CallPrinter printer(isolate, location.shared()->IsUserJavaScript());
-      Handle<String> str = printer.Print(info.literal(), location.start_pos());
-      *hint = printer.GetErrorHint();
-      if (str->length() > 0) return str;
-    } else {
-      isolate->clear_pending_exception();
-    }
-  }
-  return BuildDefaultCallSite(isolate, object);
-}
-
-MessageTemplate UpdateErrorTemplate(CallPrinter::ErrorHint hint,
-                                    MessageTemplate default_id) {
-  switch (hint) {
-    case CallPrinter::ErrorHint::kNormalIterator:
-      return MessageTemplate::kNotIterable;
-
-    case CallPrinter::ErrorHint::kCallAndNormalIterator:
-      return MessageTemplate::kNotCallableOrIterable;
-
-    case CallPrinter::ErrorHint::kAsyncIterator:
-      return MessageTemplate::kNotAsyncIterable;
-
-    case CallPrinter::ErrorHint::kCallAndAsyncIterator:
-      return MessageTemplate::kNotCallableOrAsyncIterable;
-
-    case CallPrinter::ErrorHint::kNone:
-      return default_id;
-  }
-  return default_id;
-}
-
-}  // namespace
-
-MaybeHandle<Object> Runtime::ThrowIteratorError(Isolate* isolate,
-                                                Handle<Object> object) {
-  CallPrinter::ErrorHint hint = CallPrinter::kNone;
-  Handle<String> callsite = RenderCallSite(isolate, object, &hint);
-  MessageTemplate id = MessageTemplate::kNotIterableNoSymbolLoad;
-
-  if (hint == CallPrinter::kNone) {
-    Handle<Symbol> iterator_symbol = isolate->factory()->iterator_symbol();
-    THROW_NEW_ERROR(isolate, NewTypeError(id, callsite, iterator_symbol),
-                    Object);
-  }
-
-  id = UpdateErrorTemplate(hint, id);
-  THROW_NEW_ERROR(isolate, NewTypeError(id, callsite), Object);
-}
-
 RUNTIME_FUNCTION(Runtime_ThrowIteratorError) {
   HandleScope scope(isolate);
   DCHECK_EQ(1, args.length());
   CONVERT_ARG_HANDLE_CHECKED(Object, object, 0);
-  RETURN_RESULT_OR_FAILURE(isolate,
-                           Runtime::ThrowIteratorError(isolate, object));
+  return isolate->Throw(*ErrorUtils::NewIteratorError(isolate, object));
 }
 
 RUNTIME_FUNCTION(Runtime_ThrowCalledNonCallable) {
   HandleScope scope(isolate);
   DCHECK_EQ(1, args.length());
   CONVERT_ARG_HANDLE_CHECKED(Object, object, 0);
-  CallPrinter::ErrorHint hint = CallPrinter::kNone;
-  Handle<String> callsite = RenderCallSite(isolate, object, &hint);
-  MessageTemplate id = MessageTemplate::kCalledNonCallable;
-  id = UpdateErrorTemplate(hint, id);
-  THROW_NEW_ERROR_RETURN_FAILURE(isolate, NewTypeError(id, callsite));
+  return isolate->Throw(
+      *ErrorUtils::NewCalledNonCallableError(isolate, object));
 }
 
 RUNTIME_FUNCTION(Runtime_ThrowConstructedNonConstructable) {
   HandleScope scope(isolate);
   DCHECK_EQ(1, args.length());
   CONVERT_ARG_HANDLE_CHECKED(Object, object, 0);
-  CallPrinter::ErrorHint hint = CallPrinter::kNone;
-  Handle<String> callsite = RenderCallSite(isolate, object, &hint);
-  MessageTemplate id = MessageTemplate::kNotConstructor;
-  THROW_NEW_ERROR_RETURN_FAILURE(isolate, NewTypeError(id, callsite));
+  return isolate->Throw(
+      *ErrorUtils::NewConstructedNonConstructable(isolate, object));
 }
 
-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);
+  DCHECK_EQ(1, args.length());
+  CONVERT_ARG_HANDLE_CHECKED(Object, object, 0);
+  return ErrorUtils::ThrowLoadFromNullOrUndefined(isolate, object,
+                                                  MaybeHandle<Object>());
 }
 
 RUNTIME_FUNCTION(Runtime_ThrowConstructorReturnedNonObject) {

src/runtime/runtime-object.cc

@@ -2,10 +2,13 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "src/ast/prettyprinter.h"
 #include "src/common/message-template.h"
 #include "src/debug/debug.h"
 #include "src/execution/arguments-inl.h"
 #include "src/execution/isolate-inl.h"
+#include "src/execution/messages.h"
+#include "src/handles/maybe-handles.h"
 #include "src/heap/heap-inl.h"  // For ToBoolean. TODO(jkummerow): Drop.
 #include "src/init/bootstrapper.h"
 #include "src/logging/counters.h"
@@ -24,13 +27,8 @@ MaybeHandle<Object> Runtime::GetObjectProperty(Isolate* isolate,
                                                Handle<Object> key,
                                                bool* is_found_out) {
   if (object->IsNullOrUndefined(isolate)) {
-    if (*key == ReadOnlyRoots(isolate).iterator_symbol()) {
-      return Runtime::ThrowIteratorError(isolate, object);
-    }
-    THROW_NEW_ERROR(
-        isolate,
-        NewTypeError(MessageTemplate::kNonObjectPropertyLoad, key, object),
-        Object);
+    ErrorUtils::ThrowLoadFromNullOrUndefined(isolate, object, key);
+    return MaybeHandle<Object>();
   }
 
   bool success = false;
@@ -778,7 +776,6 @@ RUNTIME_FUNCTION(Runtime_HasProperty) {
   return isolate->heap()->ToBoolean(maybe.FromJust());
 }
 
-
 RUNTIME_FUNCTION(Runtime_GetOwnPropertyKeys) {
   HandleScope scope(isolate);
   DCHECK_EQ(2, args.length());
@@ -795,7 +792,6 @@ RUNTIME_FUNCTION(Runtime_GetOwnPropertyKeys) {
   return *isolate->factory()->NewJSArrayWithElements(keys);
 }
 
-
 RUNTIME_FUNCTION(Runtime_ToFastProperties) {
   HandleScope scope(isolate);
   DCHECK_EQ(1, args.length());
@@ -807,14 +803,12 @@ RUNTIME_FUNCTION(Runtime_ToFastProperties) {
   return *object;
 }
 
-
 RUNTIME_FUNCTION(Runtime_AllocateHeapNumber) {
   HandleScope scope(isolate);
   DCHECK_EQ(0, args.length());
   return *isolate->factory()->NewHeapNumber(0);
 }
 
-
 RUNTIME_FUNCTION(Runtime_NewObject) {
   HandleScope scope(isolate);
   DCHECK_EQ(2, args.length());
@@ -845,7 +839,6 @@ RUNTIME_FUNCTION(Runtime_CompleteInobjectSlackTrackingForMap) {
   return ReadOnlyRoots(isolate).undefined_value();
 }
 
-
 RUNTIME_FUNCTION(Runtime_TryMigrateInstance) {
   HandleScope scope(isolate);
   DCHECK_EQ(1, args.length());
@@ -862,12 +855,10 @@ RUNTIME_FUNCTION(Runtime_TryMigrateInstance) {
   return *object;
 }
 
-
 static bool IsValidAccessor(Isolate* isolate, Handle<Object> obj) {
   return obj->IsNullOrUndefined(isolate) || obj->IsCallable();
 }
 
-
 // Implements part of 8.12.9 DefineOwnProperty.
 // There are 3 cases that lead here:
 // Step 4b - define a new accessor property.
@@ -891,7 +882,6 @@ RUNTIME_FUNCTION(Runtime_DefineAccessorPropertyUnchecked) {
   return ReadOnlyRoots(isolate).undefined_value();
 }
 
-
 RUNTIME_FUNCTION(Runtime_DefineDataPropertyInLiteral) {
   HandleScope scope(isolate);
   DCHECK_EQ(6, args.length());
@@ -983,7 +973,6 @@ RUNTIME_FUNCTION(Runtime_HasFastPackedElements) {
       IsFastPackedElementsKind(obj.map().elements_kind()));
 }
 
-
 RUNTIME_FUNCTION(Runtime_IsJSReceiver) {
   SealHandleScope shs(isolate);
   DCHECK_EQ(1, args.length());
@@ -991,7 +980,6 @@ RUNTIME_FUNCTION(Runtime_IsJSReceiver) {
   return isolate->heap()->ToBoolean(obj.IsJSReceiver());
 }
 
-
 RUNTIME_FUNCTION(Runtime_ClassOf) {
   SealHandleScope shs(isolate);
   DCHECK_EQ(1, args.length());
@@ -1068,9 +1056,9 @@ RUNTIME_FUNCTION(Runtime_CopyDataPropertiesWithExcludedProperties) {
   DCHECK_LE(1, args.length());
   CONVERT_ARG_HANDLE_CHECKED(Object, source, 0);
 
-  // 2. If source is undefined or null, let keys be an empty List.
-  if (source->IsUndefined(isolate) || source->IsNull(isolate)) {
-    return ReadOnlyRoots(isolate).undefined_value();
+  // If source is undefined or null, throw a non-coercible error.
+  if (source->IsNullOrUndefined(isolate)) {
+    return ErrorUtils::ThrowLoadFromNullOrUndefined(isolate, source);
   }
 
   ScopedVector<Handle<Object>> excluded_properties(args.length() - 1);
@@ -1140,7 +1128,6 @@ RUNTIME_FUNCTION(Runtime_ToNumeric) {
   RETURN_RESULT_OR_FAILURE(isolate, Object::ToNumeric(isolate, input));
 }
 
-
 RUNTIME_FUNCTION(Runtime_ToLength) {
   HandleScope scope(isolate);
   DCHECK_EQ(1, args.length());
@@ -1174,7 +1161,6 @@ RUNTIME_FUNCTION(Runtime_HasInPrototypeChain) {
   return isolate->heap()->ToBoolean(result.FromJust());
 }
 
-
 // ES6 section 7.4.7 CreateIterResultObject ( value, done )
 RUNTIME_FUNCTION(Runtime_CreateIterResultObject) {
   HandleScope scope(isolate);

src/runtime/runtime.h

@@ -47,7 +47,7 @@ namespace internal {
   F(NewArray, -1 /* >= 3 */, 1)        \
   F(NormalizeElements, 1, 1)           \
   F(TransitionElementsKind, 2, 1)      \
-  F(TransitionElementsKindWithKind, 2, 1) \
+  F(TransitionElementsKindWithKind, 2, 1)
 
 #define FOR_EACH_INTRINSIC_ATOMICS(F, I) \
   F(AtomicsLoad64, 2, 1)                 \
@@ -236,7 +236,7 @@ namespace internal {
   F(ThrowIteratorError, 1, 1)                        \
   F(ThrowIteratorResultNotAnObject, 1, 1)            \
   F(ThrowNotConstructor, 1, 1)                       \
-  F(ThrowPatternAssignmentNonCoercible, 0, 1)        \
+  F(ThrowPatternAssignmentNonCoercible, 1, 1)        \
   F(ThrowRangeError, -1 /* >= 1 */, 1)               \
   F(ThrowReferenceError, 1, 1)                       \
   F(ThrowAccessedUninitializedVariable, 1, 1)        \
@@ -732,7 +732,6 @@ class Runtime : public AllStatic {
       Isolate* isolate, Handle<Object> object);
 };
 
-
 class RuntimeState {
  public:
 #ifndef V8_INTL_SUPPORT

test/cctest/interpreter/bytecode_expectations/DestructuringAssignment.golden

@@ -366,18 +366,14 @@ snippet: "
   var x, a = {x:1};
   ({x} = a);
 "
-frame size: 3
+frame size: 2
 parameter count: 1
-bytecode array length: 26
+bytecode array length: 15
 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(7),
-  /*   53 E> */ B(CallRuntime), U16(Runtime::kThrowPatternAssignmentNonCoercible), R(0), U8(0),
-                B(Star), R(2),
-  /*   54 S> */ B(LdaNamedProperty), R(2), U8(1), U8(1),
+  /*   54 S> */ B(LdaNamedProperty), R(1), U8(1), U8(1),
                 B(Star), R(0),
                 B(LdaUndefined),
   /*   63 S> */ B(Return),
@@ -394,20 +390,16 @@ snippet: "
   var x={}, a = {y:1};
   ({y:x.foo} = a);
 "
-frame size: 3
+frame size: 2
 parameter count: 1
-bytecode array length: 31
+bytecode array length: 20
 bytecodes: [
   /*   30 E> */ B(StackCheck),
   /*   40 S> */ B(CreateEmptyObjectLiteral),
                 B(Star), R(0),
   /*   48 S> */ B(CreateObjectLiteral), U8(0), U8(0), U8(41),
                 B(Star), R(1),
-  /*   55 S> */ B(JumpIfNull), U8(4),
-                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),
+  /*   61 S> */ B(LdaNamedProperty), R(1), U8(1), U8(1),
                 B(StaNamedProperty), R(0), U8(2), U8(3),
                 B(LdaUndefined),
   /*   72 S> */ B(Return),
@@ -427,18 +419,15 @@ snippet: "
 "
 frame size: 4
 parameter count: 1
-bytecode array length: 41
+bytecode array length: 33
 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(7),
-  /*   63 E> */ B(CallRuntime), U16(Runtime::kThrowPatternAssignmentNonCoercible), R(0), U8(0),
-                B(Star), R(2),
   /*   64 S> */ B(LdaConstant), U8(1),
                 B(Star), R(3),
-                B(LdaNamedProperty), R(2), U8(1), U8(1),
+                B(LdaNamedProperty), R(1), U8(1), U8(1),
+                B(Mov), R(1), R(2),
                 B(JumpIfNotUndefined), U8(3),
                 B(LdaZero),
                 B(Star), R(0),

test/cctest/interpreter/bytecode_expectations/ForOfLoop.golden

@@ -362,7 +362,7 @@ snippet: "
 "
 frame size: 17
 parameter count: 2
-bytecode array length: 191
+bytecode array length: 178
 bytecodes: [
   /*   10 E> */ B(StackCheck),
   /*   41 S> */ B(GetIterator), R(arg0), U8(0),
@@ -383,26 +383,21 @@ bytecodes: [
                 B(JumpIfJSReceiver), U8(7),
                 B(CallRuntime), U16(Runtime::kThrowIteratorResultNotAnObject), R(12), U8(1),
                 B(LdaNamedProperty), R(12), U8(1), U8(8),
-                B(JumpIfToBooleanTrue), U8(50),
+                B(JumpIfToBooleanTrue), U8(37),
                 B(LdaNamedProperty), R(12), U8(2), U8(10),
                 B(Star), R(12),
                 B(LdaFalse),
                 B(Star), R(8),
                 B(Mov), R(12), R(0),
   /*   20 E> */ B(StackCheck),
-  /*   36 S> */ B(Ldar), R(12),
-                B(JumpIfNull), U8(4),
-                B(JumpIfNotUndefined), U8(7),
-  /*   29 E> */ B(CallRuntime), U16(Runtime::kThrowPatternAssignmentNonCoercible), R(0), U8(0),
-                B(Star), R(13),
-  /*   31 S> */ B(LdaNamedProperty), R(13), U8(3), U8(12),
+  /*   31 S> */ B(LdaNamedProperty), R(0), U8(3), U8(12),
                 B(Star), R(3),
-  /*   34 S> */ B(LdaNamedProperty), R(13), U8(4), U8(14),
+  /*   34 S> */ B(LdaNamedProperty), R(0), U8(4), U8(14),
                 B(Star), R(4),
   /*   56 S> */ B(Ldar), R(4),
   /*   58 E> */ B(Add), R(3), U8(16),
                 B(Star), R(5),
-                B(JumpLoop), U8(67), I8(0),
+                B(JumpLoop), U8(54), I8(0),
                 B(LdaSmi), I8(-1),
                 B(Star), R(10),
                 B(Star), R(9),
@@ -458,8 +453,8 @@ constant pool: [
   ONE_BYTE_INTERNALIZED_STRING_TYPE [""],
 ]
 handlers: [
-  [31, 101, 109],
-  [133, 166, 168],
+  [31, 88, 96],
+  [120, 153, 155],
 ]
 
 ---

test/cctest/interpreter/bytecode_expectations/StandardForLoop.golden

@@ -220,13 +220,10 @@ snippet: "
 "
 frame size: 4
 parameter count: 1
-bytecode array length: 53
+bytecode array length: 44
 bytecodes: [
   /*   10 E> */ B(StackCheck),
   /*   37 S> */ B(CreateObjectLiteral), U8(0), U8(0), U8(41),
-                B(JumpIfNull), U8(4),
-                B(JumpIfNotUndefined), U8(7),
-  /*   26 E> */ B(CallRuntime), U16(Runtime::kThrowPatternAssignmentNonCoercible), R(0), U8(0),
                 B(Star), R(3),
   /*   28 S> */ B(LdaNamedProperty), R(3), U8(1), U8(1),
                 B(Star), R(0),

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

-*%(basename)s:5: TypeError: Cannot destructure 'undefined' or 'null'.
+*%(basename)s:5: TypeError: Cannot destructure 'undefined' as it is undefined.
 var { [x] : y } = undefined;
                   ^
-TypeError: Cannot destructure 'undefined' or 'null'.
+TypeError: Cannot destructure 'undefined' as it is undefined.
     at *%(basename)s:5:5

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

-*%(basename)s:5: TypeError: Cannot destructure 'undefined' or 'null'.
+*%(basename)s:5: TypeError: Cannot destructure 'undefined' as it is undefined.
 var { 1: x } = undefined;
                ^
-TypeError: Cannot destructure 'undefined' or 'null'.
-    at *%(basename)s:5:5
+TypeError: Cannot destructure 'undefined' as it is undefined.
+    at *%(basename)s:5:10

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

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