[callprinter] Correctly point to the incorrect spread arg

The source position is set to the function call (console.log) not the
spread (..x), in the bytecode generator, as the spread operation is
done as part of the CallWithSpread bytecode.

The CallPrinter stops at the function call and doesn't look at the
arguments as well (in CallPrinter::VisitCall) to see if the error is
from an incorrect spread operation.


With this patch, we pass some state to the CallPrinter in the
CallWithSpread error case and check that in CallPrinter::VisitCall
before returning.

For the given source string:
```
x = undefined;
console.log(1, ...x);
```

Previously, the error was -

```
test.js:2: TypeError: console.log is not iterable (cannot read property Symbol(Symbol.iterator))
console.log(1, ...x);
        ^
TypeError: console.log is not iterable (cannot read property Symbol(Symbol.iterator))
    at test.js:2:9
```


Now, the error is -

```
_test.js:2: TypeError: x is not iterable (cannot read property undefined)
console.log(1, ...x);
                  ^
TypeError: x is not iterable (cannot read property undefined)
    at _test.js:2:9
```

Bug: v8:10038
Change-Id: I199de9997f1d949c6f9b7b4f41d51f422b8b5131
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2037431Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
Reviewed-by: Leszek Swirski <leszeks@chromium.org>
Commit-Queue: Sathya Gunasekaran  <gsathya@chromium.org>
Cr-Commit-Position: refs/heads/master@{#66131}

src/ast/prettyprinter.cc

@@ -17,7 +17,8 @@
 namespace v8 {
 namespace internal {
 
-CallPrinter::CallPrinter(Isolate* isolate, bool is_user_js)
+CallPrinter::CallPrinter(Isolate* isolate, bool is_user_js,
+                         SpreadErrorInArgsHint error_in_spread_args)
     : builder_(new IncrementalStringBuilder(isolate)) {
   isolate_ = isolate;
   position_ = 0;
@@ -30,6 +31,8 @@ CallPrinter::CallPrinter(Isolate* isolate, bool is_user_js)
   destructuring_prop_ = nullptr;
   destructuring_assignment_ = nullptr;
   is_user_js_ = is_user_js;
+  error_in_spread_args_ = error_in_spread_args;
+  spread_arg_ = nullptr;
   function_kind_ = kNormalFunction;
   InitializeAstVisitor(isolate);
 }
@@ -404,9 +407,20 @@ void CallPrinter::VisitProperty(Property* node) {
 void CallPrinter::VisitCall(Call* node) {
   bool was_found = false;
   if (node->position() == position_) {
+    if (error_in_spread_args_ == SpreadErrorInArgsHint::kErrorInArgs) {
+      found_ = true;
+      spread_arg_ = node->arguments()->last()->AsSpread()->expression();
+      Find(spread_arg_, true);
+
+      done_ = true;
+      found_ = false;
+      return;
+    }
+
     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.
@@ -429,6 +443,16 @@ void CallPrinter::VisitCall(Call* node) {
 void CallPrinter::VisitCallNew(CallNew* node) {
   bool was_found = false;
   if (node->position() == position_) {
+    if (error_in_spread_args_ == SpreadErrorInArgsHint::kErrorInArgs) {
+      found_ = true;
+      spread_arg_ = node->arguments()->last()->AsSpread()->expression();
+      Find(spread_arg_, true);
+
+      done_ = true;
+      found_ = false;
+      return;
+    }
+
     is_call_error_ = true;
     was_found = !found_;
   }

src/ast/prettyprinter.h

@@ -19,7 +19,11 @@ class IncrementalStringBuilder;  // to avoid including string-builder-inl.h
 
 class CallPrinter final : public AstVisitor<CallPrinter> {
  public:
-  explicit CallPrinter(Isolate* isolate, bool is_user_js);
+  enum class SpreadErrorInArgsHint { kErrorInArgs, kNoErrorInArgs };
+
+  explicit CallPrinter(Isolate* isolate, bool is_user_js,
+                       SpreadErrorInArgsHint error_in_spread_args =
+                           SpreadErrorInArgsHint::kNoErrorInArgs);
   ~CallPrinter();
 
   // The following routine prints the node with position |position| into a
@@ -32,7 +36,9 @@ class CallPrinter final : public AstVisitor<CallPrinter> {
     kCallAndNormalIterator,
     kCallAndAsyncIterator
   };
+
   ErrorHint GetErrorHint() const;
+  Expression* spread_arg() const { return spread_arg_; }
   ObjectLiteralProperty* destructuring_prop() const {
     return destructuring_prop_;
   }
@@ -62,8 +68,10 @@ class CallPrinter final : public AstVisitor<CallPrinter> {
   bool is_iterator_error_;
   bool is_async_iterator_error_;
   bool is_call_error_;
+  SpreadErrorInArgsHint error_in_spread_args_;
   ObjectLiteralProperty* destructuring_prop_;
   Assignment* destructuring_assignment_;
+  Expression* spread_arg_;
   FunctionKind function_kind_;
   DEFINE_AST_VISITOR_SUBCLASS_MEMBERS();
 

src/builtins/builtins-call-gen.cc

@@ -316,7 +316,11 @@ void CallOrConstructBuiltinsAssembler::CallOrConstructWithSpread(
 
   BIND(&if_generic);
   {
-    Label if_iterator_fn_not_callable(this, Label::kDeferred);
+    Label if_iterator_fn_not_callable(this, Label::kDeferred),
+        if_iterator_is_null_or_undefined(this, Label::kDeferred);
+
+    GotoIf(IsNullOrUndefined(spread), &if_iterator_is_null_or_undefined);
+
     TNode<Object> iterator_fn =
         GetProperty(context, spread, IteratorSymbolConstant());
     GotoIfNot(TaggedIsCallable(iterator_fn), &if_iterator_fn_not_callable);
@@ -333,6 +337,10 @@ void CallOrConstructBuiltinsAssembler::CallOrConstructWithSpread(
 
     BIND(&if_iterator_fn_not_callable);
     ThrowTypeError(context, MessageTemplate::kIteratorSymbolNonCallable);
+
+    BIND(&if_iterator_is_null_or_undefined);
+    CallRuntime(Runtime::kThrowSpreadArgIsNullOrUndefined, context, spread);
+    Unreachable();
   }
 
   BIND(&if_smiorobject);

src/execution/messages.cc

@@ -1284,6 +1284,38 @@ Handle<Object> ErrorUtils::NewIteratorError(Isolate* isolate,
   return isolate->factory()->NewTypeError(id, callsite);
 }
 
+Object ErrorUtils::ThrowSpreadArgIsNullOrUndefinedError(Isolate* isolate,
+                                                        Handle<Object> object) {
+  MessageLocation location;
+  Handle<String> callsite;
+  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(),
+                          CallPrinter::SpreadErrorInArgsHint::kErrorInArgs);
+      Handle<String> str = printer.Print(info.literal(), location.start_pos());
+      callsite =
+          str->length() > 0 ? str : BuildDefaultCallSite(isolate, object);
+
+      if (printer.spread_arg() != nullptr) {
+        // Change the message location to point at the property name.
+        int pos = printer.spread_arg()->position();
+        location =
+            MessageLocation(location.script(), pos, pos + 1, location.shared());
+      }
+    } else {
+      isolate->clear_pending_exception();
+      callsite = BuildDefaultCallSite(isolate, object);
+    }
+  }
+
+  MessageTemplate id = MessageTemplate::kNotIterableNoSymbolLoad;
+  Handle<Object> exception =
+      isolate->factory()->NewTypeError(id, callsite, object);
+  return isolate->Throw(*exception, &location);
+}
+
 Handle<Object> ErrorUtils::NewCalledNonCallableError(Isolate* isolate,
                                                      Handle<Object> source) {
   MessageLocation location;

src/execution/messages.h

@@ -293,6 +293,8 @@ class ErrorUtils : public AllStatic {
                                                   Handle<Object> source);
   static Handle<Object> NewConstructedNonConstructable(Isolate* isolate,
                                                        Handle<Object> source);
+  static Object ThrowSpreadArgIsNullOrUndefinedError(Isolate* isolate,
+                                                     Handle<Object> object);
   static Object ThrowLoadFromNullOrUndefined(Isolate* isolate,
                                              Handle<Object> object);
   static Object ThrowLoadFromNullOrUndefined(Isolate* isolate,

src/runtime/runtime-internal.cc

@@ -400,6 +400,13 @@ RUNTIME_FUNCTION(Runtime_ThrowIteratorError) {
   return isolate->Throw(*ErrorUtils::NewIteratorError(isolate, object));
 }
 
+RUNTIME_FUNCTION(Runtime_ThrowSpreadArgIsNullOrUndefined) {
+  HandleScope scope(isolate);
+  DCHECK_EQ(1, args.length());
+  CONVERT_ARG_HANDLE_CHECKED(Object, object, 0);
+  return ErrorUtils::ThrowSpreadArgIsNullOrUndefinedError(isolate, object);
+}
+
 RUNTIME_FUNCTION(Runtime_ThrowCalledNonCallable) {
   HandleScope scope(isolate);
   DCHECK_EQ(1, args.length());

src/runtime/runtime.h

@@ -238,6 +238,7 @@ namespace internal {
   F(ThrowInvalidStringLength, 0, 1)                  \
   F(ThrowInvalidTypedArrayAlignment, 2, 1)           \
   F(ThrowIteratorError, 1, 1)                        \
+  F(ThrowSpreadArgIsNullOrUndefined, 1, 1)           \
   F(ThrowIteratorResultNotAnObject, 1, 1)            \
   F(ThrowNotConstructor, 1, 1)                       \
   F(ThrowPatternAssignmentNonCoercible, 1, 1)        \

test/message/fail/spread-call-2.js

+// Copyright 2019 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.
+x = undefined;
+console.log(1, ...x);

test/message/fail/spread-call-2.out

+*%(basename)s:5: TypeError: x is not iterable (cannot read property undefined)
+console.log(1, ...x);
+                  ^
+TypeError: x is not iterable (cannot read property undefined)
+    at *%(basename)s:5:9

test/message/fail/spread-call-3.js

+// Copyright 2019 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.
+p = undefined;
+f = () => 3;
+var [x] = f(...p);

test/message/fail/spread-call-3.out

+*%(basename)s:6: TypeError: p is not iterable (cannot read property undefined)
+var [x] = f(...p);
+               ^
+TypeError: p is not iterable (cannot read property undefined)
+    at *%(basename)s:6:11

test/message/fail/spread-call.js

+// Copyright 2019 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.
+x = null;
+console.log(1, ...x);

test/message/fail/spread-call.out

+*%(basename)s:5: TypeError: x is not iterable (cannot read property null)
+console.log(1, ...x);
+                  ^
+TypeError: x is not iterable (cannot read property null)
+    at *%(basename)s:5:9

test/message/fail/spread-construct-2.js

+// Copyright 2019 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.
+x = undefined;
+function p() {};
+p(1, ...x);

test/message/fail/spread-construct-2.out

+*%(basename)s:6: TypeError: x is not iterable (cannot read property undefined)
+p(1, ...x);
+        ^
+TypeError: x is not iterable (cannot read property undefined)
+    at *%(basename)s:6:1

test/message/fail/spread-construct-3.js

+// Copyright 2019 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.
+p = undefined;
+f = () => 3;
+function t() {};
+var [x] = new t(...p);

test/message/fail/spread-construct-3.out

+*%(basename)s:7: TypeError: p is not iterable (cannot read property undefined)
+var [x] = new t(...p);
+                   ^
+TypeError: p is not iterable (cannot read property undefined)
+    at *%(basename)s:7:11

test/message/fail/spread-construct.js

+// Copyright 2019 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.
+x = null;
+function p() {};
+new p(1, ...x);

test/message/fail/spread-construct.out

+*%(basename)s:6: TypeError: x is not iterable (cannot read property null)
+new p(1, ...x);
+            ^
+TypeError: x is not iterable (cannot read property null)
+    at *%(basename)s:6:1