Preserve "proper method names" as-is in error.stack.

This changes the logic for generating method names in `error.stack` to
prepend an inferred type name only when the function name is a valid
ECMAScript identifiers and does not equal the inferred type name, to

(1) give developers more control over the exact name shown in
    `error.stack`, as well as
(2) avoid confusion in the presence of renaming of local variables.

Previously we'd leave the function name as-is if it was prefixed by the
inferred type name, but that condition is unnecessarily strict, and led
to a bunch of inconsistencies around special names like
`<instance_member_initializer>` where this dynamic approached often
prefixed it with the correct type name, but also sometimes got it wrong
and prepended `Object.`, which is very unfortunate and misleading.
Specifically for these special names, we'll add logic later in the
parser to infer a useful (complete) name.

The design doc (https://bit.ly/devtools-method-names-in-stack-traces)
contains more background and examples of why we do this change.

Doc: https://bit.ly/devtools-method-names-in-stack-traces
Fixed: chromium:1294619
Bug: chromium:1283435
Change-Id: Ib8b528ba25255dcd07e9d11044c562c11d699bcb
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3565724Reviewed-by: Yang Guo <yangguo@chromium.org>
Commit-Queue: Benedikt Meurer <bmeurer@chromium.org>
Cr-Commit-Position: refs/heads/main@{#79748}

src/api/api.cc

@@ -2581,29 +2581,6 @@ MaybeLocal<Module> ScriptCompiler::CompileModule(
   return ToApiHandle<Module>(i_isolate->factory()->NewSourceTextModule(shared));
 }
 
-namespace {
-bool IsIdentifier(i::Isolate* isolate, i::Handle<i::String> string) {
-  string = i::String::Flatten(isolate, string);
-  const int length = string->length();
-  if (length == 0) return false;
-  if (!i::IsIdentifierStart(string->Get(0))) return false;
-  i::DisallowGarbageCollection no_gc;
-  i::String::FlatContent flat = string->GetFlatContent(no_gc);
-  if (flat.IsOneByte()) {
-    auto vector = flat.ToOneByteVector();
-    for (int i = 1; i < length; i++) {
-      if (!i::IsIdentifierPart(vector[i])) return false;
-    }
-  } else {
-    auto vector = flat.ToUC16Vector();
-    for (int i = 1; i < length; i++) {
-      if (!i::IsIdentifierPart(vector[i])) return false;
-    }
-  }
-  return true;
-}
-}  // namespace
-
 // static
 V8_WARN_UNUSED_RESULT MaybeLocal<Function> ScriptCompiler::CompileFunction(
     Local<Context> context, Source* source, size_t arguments_count,
@@ -2652,7 +2629,7 @@ MaybeLocal<Function> ScriptCompiler::CompileFunctionInternal(
         isolate->factory()->NewFixedArray(static_cast<int>(arguments_count));
     for (int i = 0; i < static_cast<int>(arguments_count); i++) {
       i::Handle<i::String> argument = Utils::OpenHandle(*arguments[i]);
-      if (!IsIdentifier(isolate, argument)) return Local<Function>();
+      if (!i::String::IsIdentifier(isolate, argument)) return Local<Function>();
       arguments_list->set(i, *argument);
     }
 

src/objects/call-site-info.cc

@@ -636,12 +636,6 @@ void AppendFileLocation(Isolate* isolate, Handle<CallSiteInfo> frame,
   }
 }
 
-int StringIndexOf(Isolate* isolate, Handle<String> subject,
-                  Handle<String> pattern) {
-  if (pattern->length() > subject->length()) return -1;
-  return String::IndexOf(isolate, subject, pattern, 0);
-}
-
 // Returns true iff
 // 1. the subject ends with '.' + pattern, or
 // 2. subject == pattern.
@@ -683,9 +677,8 @@ void AppendMethodCall(Isolate* isolate, Handle<CallSiteInfo> frame,
     Handle<String> function_string = Handle<String>::cast(function_name);
     if (IsNonEmptyString(type_name)) {
       Handle<String> type_string = Handle<String>::cast(type_name);
-      bool starts_with_type_name =
-          (StringIndexOf(isolate, function_string, type_string) == 0);
-      if (!starts_with_type_name) {
+      if (String::IsIdentifier(isolate, function_string) &&
+          !String::Equals(isolate, function_string, type_string)) {
         builder->AppendString(type_string);
         builder->AppendCharacter('.');
       }

src/objects/string.cc

@@ -1558,6 +1558,35 @@ bool String::HasOneBytePrefix(base::Vector<const char> str) {
 
 namespace {
 
+template <typename Char>
+bool IsIdentifierVector(const base::Vector<Char>& vec) {
+  if (vec.empty()) {
+    return false;
+  }
+  if (!IsIdentifierStart(vec[0])) {
+    return false;
+  }
+  for (size_t i = 1; i < vec.size(); ++i) {
+    if (!IsIdentifierPart(vec[i])) {
+      return false;
+    }
+  }
+  return true;
+}
+
+}  // namespace
+
+// static
+bool String::IsIdentifier(Isolate* isolate, Handle<String> str) {
+  str = String::Flatten(isolate, str);
+  DisallowGarbageCollection no_gc;
+  String::FlatContent flat = str->GetFlatContent(no_gc);
+  return flat.IsOneByte() ? IsIdentifierVector(flat.ToOneByteVector())
+                          : IsIdentifierVector(flat.ToUC16Vector());
+}
+
+namespace {
+
 template <typename Char>
 uint32_t HashString(String string, size_t start, int length, uint64_t seed,
                     PtrComprCageBase cage_base,

src/objects/string.h

@@ -391,6 +391,9 @@ class String : public TorqueGeneratedString<String, Name> {
   V8_EXPORT_PRIVATE bool HasOneBytePrefix(base::Vector<const char> str);
   V8_EXPORT_PRIVATE inline bool IsOneByteEqualTo(base::Vector<const char> str);
 
+  // Returns true if the |str| is a valid ECMAScript identifier.
+  static bool IsIdentifier(Isolate* isolate, Handle<String> str);
+
   // Return a UTF8 representation of the string.  The string is null
   // terminated but may optionally contain nulls.  Length is returned
   // in length_output if length_output is not a null pointer  The string

test/message/fail/class-fields-static-throw.js

 // Copyright 2018 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.
-//
-// TODO(gsathya): Remove 'Function' from stack trace.
 
 class X {
   static x = foo();

test/message/fail/class-fields-static-throw.out

-*%(basename)s:8: ReferenceError: foo is not defined
+*%(basename)s:6: ReferenceError: foo is not defined
   static x = foo();
              ^
 ReferenceError: foo is not defined
-    at Function.<static_initializer> (*%(basename)s:8:14)
+    at <static_initializer> (*%(basename)s:6:14)
     at *%(basename)s:1:1

test/message/fail/class-fields-throw.out

@@ -2,6 +2,6 @@
   x = foo();
       ^
 ReferenceError: foo is not defined
-    at X.<instance_members_initializer> (*%(basename)s:6:7)
+    at <instance_members_initializer> (*%(basename)s:6:7)
     at new X (*%(basename)s:5:1)
-    at *%(basename)s:9:1
\ No newline at end of file
+    at *%(basename)s:9:1

test/message/fail/class-private-field-reinitialization.out

@@ -2,6 +2,6 @@
 class B extends A { #x; }
                     ^
 TypeError: Cannot initialize #x twice on the same object
-    at Object.<instance_members_initializer> (*%(basename)s:7:21)
+    at <instance_members_initializer> (*%(basename)s:7:21)
     at new B (*%(basename)s:7:1)
     at *%(basename)s:10:1

test/message/fail/stack-trace-method-name-configured.js

+// Copyright 2022 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.
+
+// Check that the user configured method name ('SomeClass.someMethod') is used
+// as-is in error.stack output without prepending the inferred type name ('A').
+const A = function() {};
+A.prototype.a = function() {
+  throw new Error;
+};
+Object.defineProperty(A.prototype.a, 'name', {value: 'SomeClass.someMethod'});
+(new A).a();

test/message/fail/stack-trace-method-name-configured.out

+# Copyright 2022 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.
+
+*%(basename)s:9: Error
+  throw new Error;
+  ^
+Error
+    at SomeClass.someMethod [as a] (*%(basename)s:9:9)
+    at *%(basename)s:12:9
+

test/message/fail/stack-trace-method-name-renaming.js

+// Copyright 2022 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.
+
+// Check that the dynamic type name ('A') is not prepended to the inferred
+// method name ('C.a') in error.stack output.
+const A = function() {};
+const C = A;
+C.prototype.a = function() {
+  throw new Error;
+};
+(new A).a();

test/message/fail/stack-trace-method-name-renaming.out

+# Copyright 2022 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.
+
+*%(basename)s:10: Error
+  throw new Error;
+  ^
+Error
+    at C.a (*%(basename)s:10:9)
+    at *%(basename)s:12:9
+

test/mjsunit/stack-traces-class-fields.js

@@ -75,16 +75,13 @@ function testClassInstantiation() {
 
 // ReferenceError: FAIL is not defined
 //     at thrower
-//     at X.<instance_members_initializer>
+//     at <instance_members_initializer>
 //     at new X
 //     at testClassInstantiation
 //     at testTrace
 testTrace(
-  "during class instantiation",
-  testClassInstantiation,
-  ["thrower", "X.<instance_members_initializer>", "new X"],
-  ["anonymous"]
-);
+    'during class instantiation', testClassInstantiation,
+    ['thrower', '<instance_members_initializer>', 'new X'], ['anonymous']);
 
 function testClassInstantiationWithSuper() {
   class Base {}
@@ -98,16 +95,14 @@ function testClassInstantiationWithSuper() {
 
 // ReferenceError: FAIL is not defined
 //     at thrower
-//     at X.<instance_members_initializer>
+//     at <instance_members_initializer>
 //     at new X
 //     at testClassInstantiation
 //     at testTrace
 testTrace(
-  "during class instantiation with super",
-  testClassInstantiationWithSuper,
-  ["thrower", "X.<instance_members_initializer>", "new X"],
-  ["Base", "anonymous"]
-);
+    'during class instantiation with super', testClassInstantiationWithSuper,
+    ['thrower', '<instance_members_initializer>', 'new X'],
+    ['Base', 'anonymous']);
 
 function testClassInstantiationWithSuper2() {
   class Base {}
@@ -124,16 +119,14 @@ function testClassInstantiationWithSuper2() {
 
 // ReferenceError: FAIL is not defined
 //     at thrower
-//     at X.<instance_members_initializer>
+//     at <instance_members_initializer>
 //     at new X
 //     at testClassInstantiation
 //     at testTrace
 testTrace(
-  "during class instantiation with super2",
-  testClassInstantiationWithSuper2,
-  ["thrower", "X.<instance_members_initializer>", "new X"],
-  ["Base", "anonymous"]
-);
+    'during class instantiation with super2', testClassInstantiationWithSuper2,
+    ['thrower', '<instance_members_initializer>', 'new X'],
+    ['Base', 'anonymous']);
 
 function testClassInstantiationWithSuper3() {
   class Base {
@@ -151,17 +144,15 @@ function testClassInstantiationWithSuper3() {
 
 // ReferenceError: FAIL is not defined
 //     at thrower
-//     at X.<instance_members_initializer>
+//     at <instance_members_initializer>
 //     at new Base
 //     at new X
 //     at testClassInstantiationWithSuper3
 //     at testTrace
 testTrace(
-  "during class instantiation with super3",
-  testClassInstantiationWithSuper3,
-  ["thrower", "X.<instance_members_initializer>", "new Base", "new X"],
-  ["anonymous"]
-);
+    'during class instantiation with super3', testClassInstantiationWithSuper3,
+    ['thrower', '<instance_members_initializer>', 'new Base', 'new X'],
+    ['anonymous']);
 
 function testClassFieldCall() {
   class X {