Skip to content

Commit

Permalink
deps: V8: cherry-pick ad21d212fc14
Browse files Browse the repository at this point in the history
Original commit message:

    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/+/3565724
    Reviewed-by: Yang Guo <[email protected]>
    Commit-Queue: Benedikt Meurer <[email protected]>
    Cr-Commit-Position: refs/heads/main@{#79748}

Refs: v8/v8@ad21d21

PR-URL: #42657
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
  • Loading branch information
targos committed Apr 12, 2022
1 parent 004137e commit eba7d2d
Show file tree
Hide file tree
Showing 14 changed files with 102 additions and 65 deletions.
2 changes: 1 addition & 1 deletion common.gypi
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@

# Reset this number to 0 on major V8 upgrades.
# Increment by one for each non-official patch applied to deps/v8.
'v8_embedder_string': '-node.11',
'v8_embedder_string': '-node.12',

##### V8 defaults for Node.js #####

Expand Down
25 changes: 1 addition & 24 deletions deps/v8/src/api/api.cc
Original file line number Diff line number Diff line change
Expand Up @@ -2537,29 +2537,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,
Expand Down Expand Up @@ -2608,7 +2585,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);
}

Expand Down
11 changes: 2 additions & 9 deletions deps/v8/src/objects/call-site-info.cc
Original file line number Diff line number Diff line change
Expand Up @@ -629,12 +629,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.
Expand Down Expand Up @@ -676,9 +670,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('.');
}
Expand Down
29 changes: 29 additions & 0 deletions deps/v8/src/objects/string.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
3 changes: 3 additions & 0 deletions deps/v8/src/objects/string.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 0 additions & 2 deletions deps/v8/test/message/fail/class-fields-static-throw.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
// 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();
Expand Down
4 changes: 2 additions & 2 deletions deps/v8/test/message/fail/class-fields-static-throw.out
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
*%(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
4 changes: 2 additions & 2 deletions deps/v8/test/message/fail/class-fields-throw.out
Original file line number Diff line number Diff line change
Expand Up @@ -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
at *%(basename)s:9:1
Original file line number Diff line number Diff line change
Expand Up @@ -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
12 changes: 12 additions & 0 deletions deps/v8/test/message/fail/stack-trace-method-name-configured.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
// 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();
11 changes: 11 additions & 0 deletions deps/v8/test/message/fail/stack-trace-method-name-configured.out
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
# 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

12 changes: 12 additions & 0 deletions deps/v8/test/message/fail/stack-trace-method-name-renaming.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
// 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();
11 changes: 11 additions & 0 deletions deps/v8/test/message/fail/stack-trace-method-name-renaming.out
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
# 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

39 changes: 15 additions & 24 deletions deps/v8/test/mjsunit/stack-traces-class-fields.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 {}
Expand All @@ -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 {}
Expand All @@ -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 {
Expand All @@ -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 {
Expand Down

0 comments on commit eba7d2d

Please sign in to comment.