[parser] Provide better error when destructuring callable

The patch changes CallPrinter's AST traversal to continue even after the first positive match for an AST node. This helps us check for the subsequent GetIterator AST node in case of destructuring. We can not differentiate between the function call failing and the GetIterator failing based on source position info. This would involve runtime checks costing performance. Instead of providing an incorrect error, we provide both the possiblities to user and allow them to disambiguate. Previously, d8> function f() { return 5; } undefined d8> var [a] = f(); (d8):1: TypeError: f is not a function var [a] = f(); ^ TypeError: f is not a function at (d8):1:11 Now, d8> function f() { return 5; } undefined d8> var [a] = f(); (d8):1: TypeError: f is not a function or its return value is not iterable var [a] = f(); ^ TypeError: f is not a function or its return value is not iterable at (d8):1:11 Bug: v8:6616, v8:6513 Change-Id: I3d6427f10cae54951b0ad0e5ddcbe802bb7191c1 Reviewed-on: https://chromium-review.googlesource.com/594894 Commit-Queue: Sathya Gunasekaran <gsathya@chromium.org> Reviewed-by: Adam Klein <adamk@chromium.org> Cr-Commit-Position: refs/heads/master@{#47025}

[parser] Provide better error when destructuring callable
The patch changes CallPrinter's AST traversal to continue even after the first positive match for an AST node. This helps us check for the subsequent GetIterator AST node in case of destructuring. We can not differentiate between the function call failing and the GetIterator failing based on source position info. This would involve runtime checks costing performance. Instead of providing an incorrect error, we provide both the possiblities to user and allow them to disambiguate. Previously, d8> function f() { return 5; } undefined d8> var [a] = f(); (d8):1: TypeError: f is not a function var [a] = f(); ^ TypeError: f is not a function at (d8):1:11 Now, d8> function f() { return 5; } undefined d8> var [a] = f(); (d8):1: TypeError: f is not a function or its return value is not iterable var [a] = f(); ^ TypeError: f is not a function or its return value is not iterable at (d8):1:11 Bug: v8:6616, v8:6513 Change-Id: I3d6427f10cae54951b0ad0e5ddcbe802bb7191c1 Reviewed-on: https://chromium-review.googlesource.com/594894 Commit-Queue: Sathya Gunasekaran <gsathya@chromium.org> Reviewed-by: Adam Klein <adamk@chromium.org> Cr-Commit-Position: refs/heads/master@{#47025}
c805d5e3 · Sathya Gunasekaran · Commit Bot · 473a6f5b · c805d5e3 · c805d5e3
Commit c805d5e3 authored Aug 01, 2017 by Sathya Gunasekaran Committed by Commit Bot Aug 01, 2017
16 changed files
--- a/src/ast/prettyprinter.cc
+++ b/src/ast/prettyprinter.cc
@@ -22,13 +22,22 @@ CallPrinter::CallPrinter(Isolate* isolate, bool is_user_js)
  num_prints_ = 0;
  found_ = false;
  done_ = false;
-  iterator_hint_ = IteratorHint::kNone;
+  is_call_error_ = false;
+  is_iterator_error_ = false;
+  is_async_iterator_error_ = false;
  is_user_js_ = is_user_js;
  InitializeAstVisitor(isolate);
 }
-CallPrinter::IteratorHint CallPrinter::GetIteratorHint() const {
+CallPrinter::ErrorHint CallPrinter::GetErrorHint() const {
-  return iterator_hint_;
+  if (is_call_error_) {
+    if (is_iterator_error_) return ErrorHint::kCallAndNormalIterator;
+    if (is_async_iterator_error_) return ErrorHint::kCallAndAsyncIterator;
+  } else {
+    if (is_iterator_error_) return ErrorHint::kNormalIterator;
+    if (is_async_iterator_error_) return ErrorHint::kAsyncIterator;
+  }
+  return ErrorHint::kNone;
 }
 Handle<String> CallPrinter::Print(FunctionLiteral* program, int position) {
@@ -40,7 +49,6 @@ Handle<String> CallPrinter::Print(FunctionLiteral* program, int position) {
 void CallPrinter::Find(AstNode* node, bool print) {
-  if (done_) return;
  if (found_) {
    if (print) {
      int prev_num_prints = num_prints_;
@@ -287,7 +295,11 @@ void CallPrinter::VisitProperty(Property* node) {
 void CallPrinter::VisitCall(Call* node) {
-  bool was_found = !found_ && node->position() == position_;
+  bool was_found = false;
+  if (node->position() == position_) {
+    is_call_error_ = true;
+    was_found = !found_;
+  }
  if (was_found) {
    // Bail out if the error is caused by a direct call to a variable in
    // non-user JS code. The variable name is meaningless due to minification.
@@ -300,12 +312,19 @@ void CallPrinter::VisitCall(Call* node) {
  Find(node->expression(), true);
  if (!was_found) Print("(...)");
  FindArguments(node->arguments());
-  if (was_found) done_ = true;
+  if (was_found) {
+    done_ = true;
+    found_ = false;
+  }
 }
 void CallPrinter::VisitCallNew(CallNew* node) {
-  bool was_found = !found_ && node->position() == position_;
+  bool was_found = false;
+  if (node->position() == position_) {
+    is_call_error_ = true;
+    was_found = !found_;
+  }
  if (was_found) {
    // Bail out if the error is caused by a direct call to a variable in
    // non-user JS code. The variable name is meaningless due to minification.
@@ -317,7 +336,10 @@ void CallPrinter::VisitCallNew(CallNew* node) {
  }
  Find(node->expression(), was_found);
  FindArguments(node->arguments());
-  if (was_found) done_ = true;
+  if (was_found) {
+    done_ = true;
+    found_ = false;
+  }
 }
@@ -381,15 +403,20 @@ void CallPrinter::VisitEmptyParentheses(EmptyParentheses* node) {
 }
 void CallPrinter::VisitGetIterator(GetIterator* node) {
-  bool was_found = !found_ && node->position() == position_;
+  bool was_found = false;
-  if (was_found) {
+  if (node->position() == position_) {
-    found_ = true;
+    is_async_iterator_error_ = node->hint() == IteratorType::kAsync;
-    iterator_hint_ = node->hint() == IteratorType::kNormal
+    is_iterator_error_ = !is_async_iterator_error_;
-                         ? IteratorHint::kNormal
+    was_found = !found_;
-                         : IteratorHint::kAsync;
+    if (was_found) {
+      found_ = true;
+    }
  }
  Find(node->iterable_for_call_printer(), true);
-  if (was_found) done_ = true;
+  if (was_found) {
+    done_ = true;
+    found_ = false;
+  }
 }
 void CallPrinter::VisitImportCallExpression(ImportCallExpression* node) {

--- a/src/ast/prettyprinter.h
+++ b/src/ast/prettyprinter.h
@@ -20,8 +20,14 @@ class CallPrinter final : public AstVisitor<CallPrinter> {
  // The following routine prints the node with position |position| into a
  // string.
  Handle<String> Print(FunctionLiteral* program, int position);
-  enum IteratorHint { kNone, kNormal, kAsync };
+  enum ErrorHint {
-  IteratorHint GetIteratorHint() const;
+    kNone,
+    kNormalIterator,
+    kAsyncIterator,
+    kCallAndNormalIterator,
+    kCallAndAsyncIterator
+  };
+  ErrorHint GetErrorHint() const;
 // Individual nodes
 #define DECLARE_VISIT(type) void Visit##type(type* node);
@@ -41,8 +47,9 @@ class CallPrinter final : public AstVisitor<CallPrinter> {
  bool found_;
  bool done_;
  bool is_user_js_;
-  IteratorHint iterator_hint_;
+  bool is_iterator_error_;
+  bool is_async_iterator_error_;
+  bool is_call_error_;
  DEFINE_AST_VISITOR_SUBCLASS_MEMBERS();
 protected:

--- a/src/messages.h
+++ b/src/messages.h
@@ -345,6 +345,10 @@ class ErrorUtils : public AllStatic {
  T(NotConstructor, "% is not a constructor")                                  \
  T(NotDateObject, "this is not a Date object.")                               \
  T(NotGeneric, "% requires that 'this' be a %")                               \
+  T(NotCallableOrIterable,                                                     \
+    "% is not a function or its return value is not iterable")                 \
+  T(NotCallableOrAsyncIterable,                                                \
+    "% is not a function or its return value is not async iterable")           \
  T(NotIterable, "% is not iterable")                                          \
  T(NotAsyncIterable, "% is not async iterable")                               \
  T(NotPropertyName, "% is not a valid property name")                         \

--- a/src/runtime/runtime-internal.cc
+++ b/src/runtime/runtime-internal.cc
@@ -393,7 +393,7 @@ bool ComputeLocation(Isolate* isolate, MessageLocation* target) {
 }
 Handle<String> RenderCallSite(Isolate* isolate, Handle<Object> object,
-                              CallPrinter::IteratorHint* hint) {
+                              CallPrinter::ErrorHint* hint) {
  MessageLocation location;
  if (ComputeLocation(isolate, &location)) {
    ParseInfo info(location.shared());
@@ -401,7 +401,7 @@ Handle<String> RenderCallSite(Isolate* isolate, Handle<Object> object,
      info.ast_value_factory()->Internalize(isolate);
      CallPrinter printer(isolate, location.shared()->IsUserJavaScript());
      Handle<String> str = printer.Print(info.literal(), location.start_pos());
-      *hint = printer.GetIteratorHint();
+      *hint = printer.GetErrorHint();
      if (str->length() > 0) return str;
    } else {
      isolate->clear_pending_exception();
@@ -410,22 +410,32 @@ Handle<String> RenderCallSite(Isolate* isolate, Handle<Object> object,
  return Object::TypeOf(isolate, object);
 }
-void UpdateIteratorTemplate(CallPrinter::IteratorHint hint,
+MessageTemplate::Template UpdateErrorTemplate(
-                            MessageTemplate::Template* id) {
+    CallPrinter::ErrorHint hint, MessageTemplate::Template default_id) {
-  if (hint == CallPrinter::IteratorHint::kNormal) {
+  switch (hint) {
-    *id = MessageTemplate::kNotIterable;
+    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;
-  if (hint == CallPrinter::IteratorHint::kAsync) {
+    case CallPrinter::ErrorHint::kNone:
-    *id = MessageTemplate::kNotAsyncIterable;
+      return default_id;
  }
+  return default_id;
 }
 }  // namespace
 MaybeHandle<Object> Runtime::ThrowIteratorError(Isolate* isolate,
                                                Handle<Object> object) {
-  CallPrinter::IteratorHint hint = CallPrinter::kNone;
+  CallPrinter::ErrorHint hint = CallPrinter::kNone;
  Handle<String> callsite = RenderCallSite(isolate, object, &hint);
  MessageTemplate::Template id = MessageTemplate::kNonObjectPropertyLoad;
@@ -435,7 +445,7 @@ MaybeHandle<Object> Runtime::ThrowIteratorError(Isolate* isolate,
                    Object);
  }
-  UpdateIteratorTemplate(hint, &id);
+  id = UpdateErrorTemplate(hint, id);
  THROW_NEW_ERROR(isolate, NewTypeError(id, callsite), Object);
 }
@@ -443,10 +453,10 @@ RUNTIME_FUNCTION(Runtime_ThrowCalledNonCallable) {
  HandleScope scope(isolate);
  DCHECK_EQ(1, args.length());
  CONVERT_ARG_HANDLE_CHECKED(Object, object, 0);
-  CallPrinter::IteratorHint hint = CallPrinter::kNone;
+  CallPrinter::ErrorHint hint = CallPrinter::kNone;
  Handle<String> callsite = RenderCallSite(isolate, object, &hint);
  MessageTemplate::Template id = MessageTemplate::kCalledNonCallable;
-  UpdateIteratorTemplate(hint, &id);
+  id = UpdateErrorTemplate(hint, id);
  THROW_NEW_ERROR_RETURN_FAILURE(isolate, NewTypeError(id, callsite));
 }
@@ -462,7 +472,7 @@ RUNTIME_FUNCTION(Runtime_ThrowConstructedNonConstructable) {
  HandleScope scope(isolate);
  DCHECK_EQ(1, args.length());
  CONVERT_ARG_HANDLE_CHECKED(Object, object, 0);
-  CallPrinter::IteratorHint hint = CallPrinter::kNone;
+  CallPrinter::ErrorHint hint = CallPrinter::kNone;
  Handle<String> callsite = RenderCallSite(isolate, object, &hint);
  MessageTemplate::Template id = MessageTemplate::kNotConstructor;
  THROW_NEW_ERROR_RETURN_FAILURE(isolate, NewTypeError(id, callsite));

--- a/test/cctest/interpreter/bytecode_expectations/AsyncGenerators.golden
+++ b/test/cctest/interpreter/bytecode_expectations/AsyncGenerators.golden
@@ -419,7 +419,7 @@ bytecodes: [
                B(TestTypeOf), U8(5),
                B(JumpIfFalse), U8(4),
                B(Jump), U8(18),
-                B(Wide), B(LdaSmi), I16(132),
+                B(Wide), B(LdaSmi), I16(134),
                B(Star), R(19),
                B(LdaConstant), U8(15),
                B(Star), R(20),
@@ -720,7 +720,7 @@ bytecodes: [
                B(TestTypeOf), U8(5),
                B(JumpIfFalse), U8(4),
                B(Jump), U8(18),
-                B(Wide), B(LdaSmi), I16(132),
+                B(Wide), B(LdaSmi), I16(134),
                B(Star), R(16),
                B(LdaConstant), U8(21),
                B(Star), R(17),
@@ -784,7 +784,7 @@ bytecodes: [
                B(JumpIfToBooleanFalse), U8(4),
                B(Jump), U8(7),
                B(CallRuntime), U16(Runtime::kThrowIteratorResultNotAnObject), R(7), U8(1),
-                B(Wide), B(LdaSmi), I16(146),
+                B(Wide), B(LdaSmi), I16(148),
                B(Star), R(16),
                B(LdaConstant), U8(21),
                B(Star), R(17),

--- a/test/cctest/interpreter/bytecode_expectations/ForAwaitOf.golden
+++ b/test/cctest/interpreter/bytecode_expectations/ForAwaitOf.golden
@@ -152,7 +152,7 @@ bytecodes: [
                B(TestTypeOf), U8(5),
                B(JumpIfFalse), U8(4),
                B(Jump), U8(18),
-                B(Wide), B(LdaSmi), I16(132),
+                B(Wide), B(LdaSmi), I16(134),
                B(Star), R(20),
                B(LdaConstant), U8(13),
                B(Star), R(21),
@@ -456,7 +456,7 @@ bytecodes: [
                B(TestTypeOf), U8(5),
                B(JumpIfFalse), U8(4),
                B(Jump), U8(18),
-                B(Wide), B(LdaSmi), I16(132),
+                B(Wide), B(LdaSmi), I16(134),
                B(Star), R(20),
                B(LdaConstant), U8(13),
                B(Star), R(21),
@@ -784,7 +784,7 @@ bytecodes: [
                B(TestTypeOf), U8(5),
                B(JumpIfFalse), U8(4),
                B(Jump), U8(18),
-                B(Wide), B(LdaSmi), I16(132),
+                B(Wide), B(LdaSmi), I16(134),
                B(Star), R(20),
                B(LdaConstant), U8(13),
                B(Star), R(21),
@@ -1039,7 +1039,7 @@ bytecodes: [
                B(TestTypeOf), U8(5),
                B(JumpIfFalse), U8(4),
                B(Jump), U8(18),
-                B(Wide), B(LdaSmi), I16(132),
+                B(Wide), B(LdaSmi), I16(134),
                B(Star), R(16),
                B(LdaConstant), U8(10),
                B(Star), R(17),

--- a/test/cctest/interpreter/bytecode_expectations/ForOf.golden
+++ b/test/cctest/interpreter/bytecode_expectations/ForOf.golden
@@ -85,7 +85,7 @@ bytecodes: [
                B(TestTypeOf), U8(5),
                B(JumpIfFalse), U8(4),
                B(Jump), U8(18),
-                B(Wide), B(LdaSmi), I16(132),
+                B(Wide), B(LdaSmi), I16(134),
                B(Star), R(11),
                B(LdaConstant), U8(8),
                B(Star), R(12),
@@ -225,7 +225,7 @@ bytecodes: [
                B(TestTypeOf), U8(5),
                B(JumpIfFalse), U8(4),
                B(Jump), U8(18),
-                B(Wide), B(LdaSmi), I16(132),
+                B(Wide), B(LdaSmi), I16(134),
                B(Star), R(12),
                B(LdaConstant), U8(8),
                B(Star), R(13),
@@ -377,7 +377,7 @@ bytecodes: [
                B(TestTypeOf), U8(5),
                B(JumpIfFalse), U8(4),
                B(Jump), U8(18),
-                B(Wide), B(LdaSmi), I16(132),
+                B(Wide), B(LdaSmi), I16(134),
                B(Star), R(11),
                B(LdaConstant), U8(8),
                B(Star), R(12),
@@ -519,7 +519,7 @@ bytecodes: [
                B(TestTypeOf), U8(5),
                B(JumpIfFalse), U8(4),
                B(Jump), U8(18),
-                B(Wide), B(LdaSmi), I16(132),
+                B(Wide), B(LdaSmi), I16(134),
                B(Star), R(10),
                B(LdaConstant), U8(10),
                B(Star), R(11),

--- a/test/cctest/interpreter/bytecode_expectations/ForOfLoop.golden
+++ b/test/cctest/interpreter/bytecode_expectations/ForOfLoop.golden
@@ -91,7 +91,7 @@ bytecodes: [
                B(TestTypeOf), U8(5),
                B(JumpIfFalse), U8(4),
                B(Jump), U8(18),
-                B(Wide), B(LdaSmi), I16(132),
+                B(Wide), B(LdaSmi), I16(134),
                B(Star), R(13),
                B(LdaConstant), U8(7),
                B(Star), R(14),
@@ -268,7 +268,7 @@ bytecodes: [
                B(TestTypeOf), U8(5),
                B(JumpIfFalse), U8(4),
                B(Jump), U8(18),
-                B(Wide), B(LdaSmi), I16(132),
+                B(Wide), B(LdaSmi), I16(134),
                B(Star), R(12),
                B(LdaConstant), U8(11),
                B(Star), R(13),
@@ -423,7 +423,7 @@ bytecodes: [
                B(TestTypeOf), U8(5),
                B(JumpIfFalse), U8(4),
                B(Jump), U8(18),
-                B(Wide), B(LdaSmi), I16(132),
+                B(Wide), B(LdaSmi), I16(134),
                B(Star), R(11),
                B(LdaConstant), U8(9),
                B(Star), R(12),
@@ -584,7 +584,7 @@ bytecodes: [
                B(TestTypeOf), U8(5),
                B(JumpIfFalse), U8(4),
                B(Jump), U8(18),
-                B(Wide), B(LdaSmi), I16(132),
+                B(Wide), B(LdaSmi), I16(134),
                B(Star), R(16),
                B(LdaConstant), U8(9),
                B(Star), R(17),
@@ -763,7 +763,7 @@ bytecodes: [
                B(TestTypeOf), U8(5),
                B(JumpIfFalse), U8(4),
                B(Jump), U8(18),
-                B(Wide), B(LdaSmi), I16(132),
+                B(Wide), B(LdaSmi), I16(134),
                B(Star), R(17),
                B(LdaConstant), U8(10),
                B(Star), R(18),
@@ -969,7 +969,7 @@ bytecodes: [
                B(TestTypeOf), U8(5),
                B(JumpIfFalse), U8(4),
                B(Jump), U8(18),
-                B(Wide), B(LdaSmi), I16(132),
+                B(Wide), B(LdaSmi), I16(134),
                B(Star), R(16),
                B(LdaConstant), U8(14),
                B(Star), R(17),
@@ -1135,7 +1135,7 @@ bytecodes: [
                B(TestTypeOf), U8(5),
                B(JumpIfFalse), U8(4),
                B(Jump), U8(18),
-                B(Wide), B(LdaSmi), I16(132),
+                B(Wide), B(LdaSmi), I16(134),
                B(Star), R(20),
                B(LdaConstant), U8(7),
                B(Star), R(21),
@@ -1392,7 +1392,7 @@ bytecodes: [
                B(TestTypeOf), U8(5),
                B(JumpIfFalse), U8(4),
                B(Jump), U8(18),
-                B(Wide), B(LdaSmi), I16(132),
+                B(Wide), B(LdaSmi), I16(134),
                B(Star), R(21),
                B(LdaConstant), U8(9),
                B(Star), R(22),

--- a/test/cctest/interpreter/bytecode_expectations/Generators.golden
+++ b/test/cctest/interpreter/bytecode_expectations/Generators.golden
@@ -272,7 +272,7 @@ bytecodes: [
                B(TestTypeOf), U8(5),
                B(JumpIfFalse), U8(4),
                B(Jump), U8(18),
-                B(Wide), B(LdaSmi), I16(132),
+                B(Wide), B(LdaSmi), I16(134),
                B(Star), R(15),
                B(LdaConstant), U8(15),
                B(Star), R(16),

--- a/test/message/destructuring-function-non-iterable.js
+++ b/test/message/destructuring-function-non-iterable.js
+// Copyright 2017 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+function f() { return 5; }
+var [a] = f();
--- a/test/message/destructuring-function-non-iterable.out
+++ b/test/message/destructuring-function-non-iterable.out
+*%(basename)s:6: TypeError: f is not a function or its return value is not iterable
+var [a] = f();
+          ^
+TypeError: f is not a function or its return value is not iterable
+    at *%(basename)s:6:11
--- a/test/message/destructuring-new-callable-non-iterable.js
+++ b/test/message/destructuring-new-callable-non-iterable.js
+// Copyright 2017 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+function f() { return 5; }
+var [a] = new f;
--- a/test/message/destructuring-new-callable-non-iterable.out
+++ b/test/message/destructuring-new-callable-non-iterable.out
+*%(basename)s:6: TypeError: f is not a function or its return value is not iterable
+var [a] = new f;
+          ^
+TypeError: f is not a function or its return value is not iterable
+    at *%(basename)s:6:11
--- a/test/message/destructuring-non-function-non-iterable.js
+++ b/test/message/destructuring-non-function-non-iterable.js
+// Copyright 2017 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+var f = undefined;
+var [a] = f();
--- a/test/message/destructuring-non-function-non-iterable.out
+++ b/test/message/destructuring-non-function-non-iterable.out
+*%(basename)s:6: TypeError: f is not a function or its return value is not iterable
+var [a] = f();
+          ^
+TypeError: f is not a function or its return value is not iterable
+    at *%(basename)s:6:11
--- a/test/message/get-iterator1.out
+++ b/test/message/get-iterator1.out
-*%(basename)s:7: TypeError: nonIterable(...) is not iterable
+*%(basename)s:7: TypeError: nonIterable(...) is not a function or its return value is not iterable
 [...nonIterable()];
    ^
-TypeError: nonIterable(...) is not iterable
+TypeError: nonIterable(...) is not a function or its return value is not iterable
    at *%(basename)s:7:5