Revert "[destructuring] Elide coercible check for simple keys"

This reverts commit 1fba0441.

Reason for revert: blocks V8 roll due to layout test failures caused by error message changes:
https://ci.chromium.org/p/v8/builders/ci/V8%20Blink%20Linux/347

Original change's description:
> [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/+/1773254
> Reviewed-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}

TBR=leszeks@chromium.org,verwaest@chromium.org

Change-Id: I74cf06ebd987e5b8bbe1831b0042c085edf37f5b
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1776994Reviewed-by: Adam Klein <adamk@chromium.org>
Commit-Queue: Adam Klein <adamk@chromium.org>
Cr-Commit-Position: refs/heads/master@{#63465}

src/ast/ast.h

@@ -1405,6 +1405,7 @@ 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,8 +27,6 @@ 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);
@@ -301,50 +299,24 @@ 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_) {
+  Find(node->target());
+  if (node->target()->IsArrayLiteral()) {
+    // Special case the visit for destructuring array assignment.
+    bool was_found = false;
+    if (node->value()->position() == position_) {
+      is_iterator_error_ = true;
       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.
-      bool was_found = false;
-      if (node->value()->position() == position_) {
-        is_iterator_error_ = true;
-        was_found = !found_;
-        found_ = true;
-      }
-      Find(node->value(), true);
       if (was_found) {
-        done_ = true;
-        found_ = false;
+        found_ = true;
       }
-    } else {
-      Find(node->value());
     }
-  } else {
     Find(node->value(), true);
-  }
-
-  if (was_found) {
-    done_ = true;
-    found_ = false;
+    if (was_found) {
+      done_ = true;
+      found_ = false;
+    }
+  } else {
+    Find(node->value());
   }
 }
 

src/ast/prettyprinter.h

@@ -31,12 +31,6 @@ 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);
@@ -60,8 +54,6 @@ 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 '%' as it is %.")                        \
+  T(NonCoercible, "Cannot destructure 'undefined' or 'null'.")                 \
   T(NonCoercibleWithProperty,                                                  \
-    "Cannot destructure property '%' of '%' as it is %.")                      \
+    "Cannot destructure property `%` of 'undefined' or 'null'.")               \
   T(NonExtensibleProto, "% is not extensible")                                 \
   T(NonObjectInInstanceOfCheck,                                                \
     "Right-hand side of 'instanceof' is not an object")                        \

src/execution/messages.cc

@@ -7,11 +7,8 @@
 #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"
@@ -21,9 +18,6 @@
 #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"
@@ -1172,232 +1166,5 @@ 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,18 +283,6 @@ 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,7 +14,6 @@
 #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"
@@ -29,13 +28,14 @@
 #include "src/objects/heap-number-inl.h"
 #include "src/objects/js-array-inl.h"
 #include "src/objects/module-inl.h"
-#include "src/objects/prototype.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/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,17 +391,11 @@ MaybeHandle<Object> LoadIC::Load(Handle<Object> object, Handle<Name> name) {
     }
 
     if (*name == ReadOnlyRoots(isolate()).iterator_symbol()) {
-      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 Runtime::ThrowIteratorError(isolate(), object);
     }
+    return TypeError(IsAnyHas() ? MessageTemplate::kInvalidInOperatorUse
+                                : MessageTemplate::kNonObjectPropertyLoad,
+                     object, name);
   }
 
   if (MigrateDeprecated(isolate(), object)) use_ic = false;

src/interpreter/bytecode-generator.cc

@@ -3713,6 +3713,22 @@ void BytecodeGenerator::BuildDestructuringObjectAssignment(
     LookupHoistingMode lookup_hoisting_mode) {
   RegisterAllocationScope scope(this);
 
+  // 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).
+  BytecodeLabel is_null_or_undefined, not_null_or_undefined;
+  builder()
+      ->JumpIfNull(&is_null_or_undefined)
+      .JumpIfNotUndefined(&not_null_or_undefined);
+
+  {
+    builder()->Bind(&is_null_or_undefined);
+    builder()->SetExpressionPosition(pattern);
+    builder()->CallRuntime(Runtime::kThrowPatternAssignmentNonCoercible);
+  }
+
   // Store the assignment value in a register.
   Register value;
   RegisterList rest_runtime_callargs;
@@ -3723,34 +3739,7 @@ void BytecodeGenerator::BuildDestructuringObjectAssignment(
   } else {
     value = register_allocator()->NewRegister();
   }
-  builder()->StoreAccumulatorInRegister(value);
-
-  // if (value === null || value === undefined)
-  //   throw new TypeError(kNonCoercible);
-  //
-  // 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()
-        ->JumpIfUndefinedOrNull(&is_null_or_undefined)
-        .Jump(&not_null_or_undefined);
-
-    {
-      builder()->Bind(&is_null_or_undefined);
-      builder()->SetExpressionPosition(pattern);
-      builder()->CallRuntime(Runtime::kThrowPatternAssignmentNonCoercible,
-                             value);
-    }
-    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,8 +13,6 @@
 #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"
@@ -376,35 +374,228 @@ 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 isolate->Throw(*ErrorUtils::NewIteratorError(isolate, object));
+  RETURN_RESULT_OR_FAILURE(isolate,
+                           Runtime::ThrowIteratorError(isolate, object));
 }
 
 RUNTIME_FUNCTION(Runtime_ThrowCalledNonCallable) {
   HandleScope scope(isolate);
   DCHECK_EQ(1, args.length());
   CONVERT_ARG_HANDLE_CHECKED(Object, object, 0);
-  return isolate->Throw(
-      *ErrorUtils::NewCalledNonCallableError(isolate, object));
+  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));
 }
 
 RUNTIME_FUNCTION(Runtime_ThrowConstructedNonConstructable) {
   HandleScope scope(isolate);
   DCHECK_EQ(1, args.length());
   CONVERT_ARG_HANDLE_CHECKED(Object, object, 0);
-  return isolate->Throw(
-      *ErrorUtils::NewConstructedNonConstructable(isolate, object));
+  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));
 }
 
+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(1, args.length());
-  CONVERT_ARG_HANDLE_CHECKED(Object, object, 0);
-  return ErrorUtils::ThrowLoadFromNullOrUndefined(isolate, object,
-                                                  MaybeHandle<Object>());
+  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) {

src/runtime/runtime-object.cc

@@ -2,13 +2,10 @@
 // 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"
@@ -27,8 +24,13 @@ MaybeHandle<Object> Runtime::GetObjectProperty(Isolate* isolate,
                                                Handle<Object> key,
                                                bool* is_found_out) {
   if (object->IsNullOrUndefined(isolate)) {
-    ErrorUtils::ThrowLoadFromNullOrUndefined(isolate, object, key);
-    return MaybeHandle<Object>();
+    if (*key == ReadOnlyRoots(isolate).iterator_symbol()) {
+      return Runtime::ThrowIteratorError(isolate, object);
+    }
+    THROW_NEW_ERROR(
+        isolate,
+        NewTypeError(MessageTemplate::kNonObjectPropertyLoad, key, object),
+        Object);
   }
 
   bool success = false;
@@ -776,6 +778,7 @@ RUNTIME_FUNCTION(Runtime_HasProperty) {
   return isolate->heap()->ToBoolean(maybe.FromJust());
 }
 
+
 RUNTIME_FUNCTION(Runtime_GetOwnPropertyKeys) {
   HandleScope scope(isolate);
   DCHECK_EQ(2, args.length());
@@ -792,6 +795,7 @@ RUNTIME_FUNCTION(Runtime_GetOwnPropertyKeys) {
   return *isolate->factory()->NewJSArrayWithElements(keys);
 }
 
+
 RUNTIME_FUNCTION(Runtime_ToFastProperties) {
   HandleScope scope(isolate);
   DCHECK_EQ(1, args.length());
@@ -803,12 +807,14 @@ 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());
@@ -839,6 +845,7 @@ RUNTIME_FUNCTION(Runtime_CompleteInobjectSlackTrackingForMap) {
   return ReadOnlyRoots(isolate).undefined_value();
 }
 
+
 RUNTIME_FUNCTION(Runtime_TryMigrateInstance) {
   HandleScope scope(isolate);
   DCHECK_EQ(1, args.length());
@@ -855,10 +862,12 @@ 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.
@@ -882,6 +891,7 @@ RUNTIME_FUNCTION(Runtime_DefineAccessorPropertyUnchecked) {
   return ReadOnlyRoots(isolate).undefined_value();
 }
 
+
 RUNTIME_FUNCTION(Runtime_DefineDataPropertyInLiteral) {
   HandleScope scope(isolate);
   DCHECK_EQ(6, args.length());
@@ -973,6 +983,7 @@ RUNTIME_FUNCTION(Runtime_HasFastPackedElements) {
       IsFastPackedElementsKind(obj.map().elements_kind()));
 }
 
+
 RUNTIME_FUNCTION(Runtime_IsJSReceiver) {
   SealHandleScope shs(isolate);
   DCHECK_EQ(1, args.length());
@@ -980,6 +991,7 @@ RUNTIME_FUNCTION(Runtime_IsJSReceiver) {
   return isolate->heap()->ToBoolean(obj.IsJSReceiver());
 }
 
+
 RUNTIME_FUNCTION(Runtime_ClassOf) {
   SealHandleScope shs(isolate);
   DCHECK_EQ(1, args.length());
@@ -1056,9 +1068,9 @@ RUNTIME_FUNCTION(Runtime_CopyDataPropertiesWithExcludedProperties) {
   DCHECK_LE(1, args.length());
   CONVERT_ARG_HANDLE_CHECKED(Object, source, 0);
 
-  // If source is undefined or null, throw a non-coercible error.
-  if (source->IsNullOrUndefined(isolate)) {
-    return ErrorUtils::ThrowLoadFromNullOrUndefined(isolate, source);
+  // 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();
   }
 
   ScopedVector<Handle<Object>> excluded_properties(args.length() - 1);
@@ -1128,6 +1140,7 @@ 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());
@@ -1161,6 +1174,7 @@ 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

@@ -37,17 +37,17 @@ namespace internal {
 // inline), use the F macro below. To declare the runtime version and the inline
 // version simultaneously, use the I macro below.
 
-#define FOR_EACH_INTRINSIC_ARRAY(F, I) \
-  F(ArrayIncludes_Slow, 3, 1)          \
-  F(ArrayIndexOf, 3, 1)                \
-  F(ArrayIsArray, 1, 1)                \
-  F(ArraySpeciesConstructor, 1, 1)     \
-  F(GrowArrayElements, 2, 1)           \
-  I(IsArray, 1, 1)                     \
-  F(NewArray, -1 /* >= 3 */, 1)        \
-  F(NormalizeElements, 1, 1)           \
-  F(TransitionElementsKind, 2, 1)      \
-  F(TransitionElementsKindWithKind, 2, 1)
+#define FOR_EACH_INTRINSIC_ARRAY(F, I)    \
+  F(ArrayIncludes_Slow, 3, 1)             \
+  F(ArrayIndexOf, 3, 1)                   \
+  F(ArrayIsArray, 1, 1)                   \
+  F(ArraySpeciesConstructor, 1, 1)        \
+  F(GrowArrayElements, 2, 1)              \
+  I(IsArray, 1, 1)                        \
+  F(NewArray, -1 /* >= 3 */, 1)           \
+  F(NormalizeElements, 1, 1)              \
+  F(TransitionElementsKind, 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, 1, 1)        \
+  F(ThrowPatternAssignmentNonCoercible, 0, 1)        \
   F(ThrowRangeError, -1 /* >= 1 */, 1)               \
   F(ThrowReferenceError, 1, 1)                       \
   F(ThrowAccessedUninitializedVariable, 1, 1)        \
@@ -732,6 +732,7 @@ class Runtime : public AllStatic {
       Isolate* isolate, Handle<Object> object);
 };
 
+
 class RuntimeState {
  public:
 #ifndef V8_INTL_SUPPORT

test/cctest/interpreter/bytecode_expectations/DestructuringAssignment.golden

@@ -366,14 +366,18 @@ snippet: "
   var x, a = {x:1};
   ({x} = a);
 "
-frame size: 2
+frame size: 3
 parameter count: 1
-bytecode array length: 15
+bytecode array length: 26
 bytecodes: [
   /*   30 E> */ B(StackCheck),
   /*   45 S> */ B(CreateObjectLiteral), U8(0), U8(0), U8(41),
                 B(Star), R(1),
-  /*   54 S> */ B(LdaNamedProperty), R(1), U8(1), U8(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),
                 B(Star), R(0),
                 B(LdaUndefined),
   /*   63 S> */ B(Return),
@@ -390,16 +394,20 @@ snippet: "
   var x={}, a = {y:1};
   ({y:x.foo} = a);
 "
-frame size: 2
+frame size: 3
 parameter count: 1
-bytecode array length: 20
+bytecode array length: 31
 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),
-  /*   61 S> */ B(LdaNamedProperty), R(1), U8(1), U8(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),
                 B(StaNamedProperty), R(0), U8(2), U8(3),
                 B(LdaUndefined),
   /*   72 S> */ B(Return),
@@ -419,15 +427,18 @@ snippet: "
 "
 frame size: 4
 parameter count: 1
-bytecode array length: 33
+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(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(1), U8(1), U8(1),
-                B(Mov), R(1), R(2),
+                B(LdaNamedProperty), R(2), U8(1), U8(1),
                 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: 178
+bytecode array length: 191
 bytecodes: [
   /*   10 E> */ B(StackCheck),
   /*   41 S> */ B(GetIterator), R(arg0), U8(0),
@@ -383,21 +383,26 @@ 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(37),
+                B(JumpIfToBooleanTrue), U8(50),
                 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),
-  /*   31 S> */ B(LdaNamedProperty), R(0), U8(3), U8(12),
+  /*   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),
                 B(Star), R(3),
-  /*   34 S> */ B(LdaNamedProperty), R(0), U8(4), U8(14),
+  /*   34 S> */ B(LdaNamedProperty), R(13), 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(54), I8(0),
+                B(JumpLoop), U8(67), I8(0),
                 B(LdaSmi), I8(-1),
                 B(Star), R(10),
                 B(Star), R(9),
@@ -453,8 +458,8 @@ constant pool: [
   ONE_BYTE_INTERNALIZED_STRING_TYPE [""],
 ]
 handlers: [
-  [31, 88, 96],
-  [120, 153, 155],
+  [31, 101, 109],
+  [133, 166, 168],
 ]
 
 ---

test/cctest/interpreter/bytecode_expectations/StandardForLoop.golden

@@ -220,10 +220,13 @@ snippet: "
 "
 frame size: 4
 parameter count: 1
-bytecode array length: 44
+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(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' as it is undefined.
+*%(basename)s:5: TypeError: Cannot destructure 'undefined' or 'null'.
 var { [x] : y } = undefined;
-                  ^
-TypeError: Cannot destructure 'undefined' as it is undefined.
+    ^
+TypeError: Cannot destructure 'undefined' or 'null'.
     at *%(basename)s:5:5

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

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

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

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