From b3b9466a051e04778d6fb532df96b99c8b3e175f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=B2=20Ribaudo?= Date: Wed, 11 Aug 2021 16:32:58 +0200 Subject: [PATCH] [class] Improve errors for reinitialized private elements Previously V8 was reusing the error fur duplicate declarations, using the private name for class fields or the class name for class methods as the redeclared identifier. class A { constructor(o) { return o } } class B extends A { #x } class C extends A { #x() {} } let D = (0, class extends A { #x() {} }); new B(new B({})) // Identifier '#x' has already been declared new C(new C({})) // Identifier 'C' has already been declared new D(new D({})) // Identifier '' has already been declared This patch changes it to use error messages that better explain what's happening: new B(new B({})) // Cannot initialize #x twice on the same object new C(new C({})) // Cannot initialize private methods of // class C twice on the same object new D(new D({})) // Cannot initialize private methods of // class anonymous twice on the same object I initially tried to use the same message for both fields and methods, but the problem with that is that when initializing fields we only have access to the field name, while when initializing methods we only have access to the class name (using the "private brand" symbol). However, almost all the error messages are different for private fields and for methods so this shouldn't be a problem. Bug: v8:12042 Change-Id: Iaa50c16e4fa5c0646ad9ef2aa7e65bb649b3fce2 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3078362 Reviewed-by: Leszek Swirski Reviewed-by: Joyee Cheung Commit-Queue: Leszek Swirski Cr-Commit-Position: refs/heads/master@{#76279} --- AUTHORS | 1 + src/common/message-template.h | 4 +++ src/interpreter/bytecode-generator.cc | 2 +- src/runtime/runtime-object.cc | 27 +++++++------------ .../PrivateAccessorAccess.golden | 8 +++--- .../PrivateMethodAccess.golden | 4 +-- .../StaticPrivateMethodAccess.golden | 20 +++++++------- ...rivate-brand-reinitialization-anonymous.js | 10 +++++++ ...ivate-brand-reinitialization-anonymous.out | 6 +++++ .../class-private-brand-reinitialization.js | 10 +++++++ .../class-private-brand-reinitialization.out | 6 +++++ .../class-private-field-reinitialization.js | 10 +++++++ .../class-private-field-reinitialization.out | 7 +++++ 13 files changed, 80 insertions(+), 35 deletions(-) create mode 100644 test/message/fail/class-private-brand-reinitialization-anonymous.js create mode 100644 test/message/fail/class-private-brand-reinitialization-anonymous.out create mode 100644 test/message/fail/class-private-brand-reinitialization.js create mode 100644 test/message/fail/class-private-brand-reinitialization.out create mode 100644 test/message/fail/class-private-field-reinitialization.js create mode 100644 test/message/fail/class-private-field-reinitialization.out diff --git a/AUTHORS b/AUTHORS index d9eb05985c..7a276e5758 100644 --- a/AUTHORS +++ b/AUTHORS @@ -172,6 +172,7 @@ Milton Chiang Mu Tao Myeong-bo Shim Nicolas Antonius Ernst Leopold Maria Kaiser +Nicolò Ribaudo Niek van der Maas Niklas Hambüchen Noj Vek diff --git a/src/common/message-template.h b/src/common/message-template.h index 89ef319db1..1057676ce2 100644 --- a/src/common/message-template.h +++ b/src/common/message-template.h @@ -439,6 +439,10 @@ namespace internal { T(InvalidRegExpFlags, "Invalid flags supplied to RegExp constructor '%'") \ T(InvalidOrUnexpectedToken, "Invalid or unexpected token") \ T(InvalidPrivateBrand, "Object must be an instance of class %") \ + T(InvalidPrivateBrandReinitialization, \ + "Cannot initialize private methods of class % twice on the same object") \ + T(InvalidPrivateFieldReitialization, \ + "Cannot initialize % twice on the same object") \ T(InvalidPrivateFieldResolution, \ "Private field '%' must be declared in an enclosing class") \ T(InvalidPrivateMemberRead, \ diff --git a/src/interpreter/bytecode-generator.cc b/src/interpreter/bytecode-generator.cc index f78330bea1..460529a5c8 100644 --- a/src/interpreter/bytecode-generator.cc +++ b/src/interpreter/bytecode-generator.cc @@ -2525,7 +2525,7 @@ void BytecodeGenerator::BuildClassLiteral(ClassLiteral* expr, Register name) { const AstRawString* class_name = expr->scope()->class_variable() != nullptr ? expr->scope()->class_variable()->raw_name() - : ast_string_constants()->empty_string(); + : ast_string_constants()->anonymous_string(); builder() ->LoadLiteral(class_name) .StoreAccumulatorInRegister(brand) diff --git a/src/runtime/runtime-object.cc b/src/runtime/runtime-object.cc index 42bbb10d92..bec54bd8d4 100644 --- a/src/runtime/runtime-object.cc +++ b/src/runtime/runtime-object.cc @@ -49,22 +49,10 @@ MaybeHandle Runtime::GetObjectProperty( if (!it.IsFound() && key->IsSymbol() && Symbol::cast(*key).is_private_name()) { - Handle sym = Handle::cast(key); - Handle name(sym->description(), isolate); - DCHECK(name->IsString()); - Handle name_string = Handle::cast(name); - if (sym->IsPrivateBrand()) { - Handle class_name = (name_string->length() == 0) - ? isolate->factory()->anonymous_string() - : name_string; - THROW_NEW_ERROR(isolate, - NewTypeError(MessageTemplate::kInvalidPrivateBrand, - class_name, lookup_start_object), - Object); - } - THROW_NEW_ERROR(isolate, - NewTypeError(MessageTemplate::kInvalidPrivateMemberRead, - name_string, lookup_start_object), + MessageTemplate message = Symbol::cast(*key).IsPrivateBrand() + ? MessageTemplate::kInvalidPrivateBrand + : MessageTemplate::kInvalidPrivateMemberRead; + THROW_NEW_ERROR(isolate, NewTypeError(message, key, lookup_start_object), Object); } return result; @@ -1424,7 +1412,9 @@ RUNTIME_FUNCTION(Runtime_AddPrivateBrand) { if (it.IsFound()) { THROW_NEW_ERROR_RETURN_FAILURE( - isolate, NewTypeError(MessageTemplate::kVarRedeclaration, brand)); + isolate, + NewTypeError(MessageTemplate::kInvalidPrivateBrandReinitialization, + brand)); } PropertyAttributes attributes = @@ -1447,7 +1437,8 @@ RUNTIME_FUNCTION(Runtime_AddPrivateField) { if (it.IsFound()) { THROW_NEW_ERROR_RETURN_FAILURE( - isolate, NewTypeError(MessageTemplate::kVarRedeclaration, key)); + isolate, + NewTypeError(MessageTemplate::kInvalidPrivateFieldReitialization, key)); } CHECK(Object::AddDataProperty(&it, value, NONE, Just(kDontThrow), diff --git a/test/cctest/interpreter/bytecode_expectations/PrivateAccessorAccess.golden b/test/cctest/interpreter/bytecode_expectations/PrivateAccessorAccess.golden index 819338ad80..df6ec8d95f 100644 --- a/test/cctest/interpreter/bytecode_expectations/PrivateAccessorAccess.golden +++ b/test/cctest/interpreter/bytecode_expectations/PrivateAccessorAccess.golden @@ -83,7 +83,7 @@ bytecodes: [ B(Mov), R(this), R(0), B(Mov), R(context), R(2), /* 48 E> */ B(CallRuntime), U16(Runtime::kAddPrivateBrand), R(0), U8(3), - /* 53 S> */ B(Wide), B(LdaSmi), I16(281), + /* 53 S> */ B(Wide), B(LdaSmi), I16(283), B(Star3), B(LdaConstant), U8(0), B(Star4), @@ -114,7 +114,7 @@ bytecodes: [ B(Mov), R(this), R(0), B(Mov), R(context), R(2), /* 41 E> */ B(CallRuntime), U16(Runtime::kAddPrivateBrand), R(0), U8(3), - /* 46 S> */ B(Wide), B(LdaSmi), I16(280), + /* 46 S> */ B(Wide), B(LdaSmi), I16(282), B(Star3), B(LdaConstant), U8(0), B(Star4), @@ -145,7 +145,7 @@ bytecodes: [ B(Mov), R(this), R(0), B(Mov), R(context), R(2), /* 48 E> */ B(CallRuntime), U16(Runtime::kAddPrivateBrand), R(0), U8(3), - /* 53 S> */ B(Wide), B(LdaSmi), I16(281), + /* 53 S> */ B(Wide), B(LdaSmi), I16(283), B(Star3), B(LdaConstant), U8(0), B(Star4), @@ -176,7 +176,7 @@ bytecodes: [ B(Mov), R(this), R(0), B(Mov), R(context), R(2), /* 41 E> */ B(CallRuntime), U16(Runtime::kAddPrivateBrand), R(0), U8(3), - /* 46 S> */ B(Wide), B(LdaSmi), I16(280), + /* 46 S> */ B(Wide), B(LdaSmi), I16(282), B(Star4), B(LdaConstant), U8(0), B(Star5), diff --git a/test/cctest/interpreter/bytecode_expectations/PrivateMethodAccess.golden b/test/cctest/interpreter/bytecode_expectations/PrivateMethodAccess.golden index 855d8919f3..6ad32d35be 100644 --- a/test/cctest/interpreter/bytecode_expectations/PrivateMethodAccess.golden +++ b/test/cctest/interpreter/bytecode_expectations/PrivateMethodAccess.golden @@ -56,7 +56,7 @@ bytecodes: [ B(Mov), R(this), R(0), B(Mov), R(context), R(2), /* 44 E> */ B(CallRuntime), U16(Runtime::kAddPrivateBrand), R(0), U8(3), - /* 49 S> */ B(Wide), B(LdaSmi), I16(279), + /* 49 S> */ B(Wide), B(LdaSmi), I16(281), B(Star3), B(LdaConstant), U8(0), B(Star4), @@ -88,7 +88,7 @@ bytecodes: [ B(Mov), R(this), R(0), B(Mov), R(context), R(2), /* 44 E> */ B(CallRuntime), U16(Runtime::kAddPrivateBrand), R(0), U8(3), - /* 49 S> */ B(Wide), B(LdaSmi), I16(279), + /* 49 S> */ B(Wide), B(LdaSmi), I16(281), B(Star3), B(LdaConstant), U8(0), B(Star4), diff --git a/test/cctest/interpreter/bytecode_expectations/StaticPrivateMethodAccess.golden b/test/cctest/interpreter/bytecode_expectations/StaticPrivateMethodAccess.golden index df2bdc2a09..b409a183b3 100644 --- a/test/cctest/interpreter/bytecode_expectations/StaticPrivateMethodAccess.golden +++ b/test/cctest/interpreter/bytecode_expectations/StaticPrivateMethodAccess.golden @@ -24,7 +24,7 @@ bytecodes: [ B(TestReferenceEqual), R(this), B(Mov), R(this), R(1), B(JumpIfTrue), U8(16), - B(Wide), B(LdaSmi), I16(277), + B(Wide), B(LdaSmi), I16(279), B(Star2), B(LdaConstant), U8(0), B(Star3), @@ -55,7 +55,7 @@ frame size: 2 parameter count: 1 bytecode array length: 14 bytecodes: [ - /* 56 S> */ B(Wide), B(LdaSmi), I16(279), + /* 56 S> */ B(Wide), B(LdaSmi), I16(281), B(Star0), B(LdaConstant), U8(0), B(Star1), @@ -82,7 +82,7 @@ frame size: 2 parameter count: 1 bytecode array length: 14 bytecodes: [ - /* 56 S> */ B(Wide), B(LdaSmi), I16(279), + /* 56 S> */ B(Wide), B(LdaSmi), I16(281), B(Star0), B(LdaConstant), U8(0), B(Star1), @@ -121,7 +121,7 @@ bytecodes: [ B(TestReferenceEqual), R(this), B(Mov), R(this), R(0), B(JumpIfTrue), U8(16), - B(Wide), B(LdaSmi), I16(277), + B(Wide), B(LdaSmi), I16(279), B(Star2), B(LdaConstant), U8(0), B(Star3), @@ -143,7 +143,7 @@ bytecodes: [ B(TestReferenceEqual), R(this), B(Mov), R(this), R(1), B(JumpIfTrue), U8(16), - B(Wide), B(LdaSmi), I16(278), + B(Wide), B(LdaSmi), I16(280), B(Star3), B(LdaConstant), U8(0), B(Star4), @@ -158,7 +158,7 @@ bytecodes: [ B(TestReferenceEqual), R(this), B(Mov), R(this), R(0), B(JumpIfTrue), U8(16), - B(Wide), B(LdaSmi), I16(277), + B(Wide), B(LdaSmi), I16(279), B(Star2), B(LdaConstant), U8(0), B(Star3), @@ -188,7 +188,7 @@ frame size: 2 parameter count: 1 bytecode array length: 14 bytecodes: [ - /* 60 S> */ B(Wide), B(LdaSmi), I16(281), + /* 60 S> */ B(Wide), B(LdaSmi), I16(283), B(Star0), B(LdaConstant), U8(0), B(Star1), @@ -214,7 +214,7 @@ frame size: 2 parameter count: 1 bytecode array length: 14 bytecodes: [ - /* 53 S> */ B(Wide), B(LdaSmi), I16(280), + /* 53 S> */ B(Wide), B(LdaSmi), I16(282), B(Star0), B(LdaConstant), U8(0), B(Star1), @@ -240,7 +240,7 @@ frame size: 2 parameter count: 1 bytecode array length: 14 bytecodes: [ - /* 60 S> */ B(Wide), B(LdaSmi), I16(281), + /* 60 S> */ B(Wide), B(LdaSmi), I16(283), B(Star0), B(LdaConstant), U8(0), B(Star1), @@ -266,7 +266,7 @@ frame size: 3 parameter count: 1 bytecode array length: 14 bytecodes: [ - /* 46 S> */ B(Wide), B(LdaSmi), I16(280), + /* 46 S> */ B(Wide), B(LdaSmi), I16(282), B(Star1), B(LdaConstant), U8(0), B(Star2), diff --git a/test/message/fail/class-private-brand-reinitialization-anonymous.js b/test/message/fail/class-private-brand-reinitialization-anonymous.js new file mode 100644 index 0000000000..95b54396a6 --- /dev/null +++ b/test/message/fail/class-private-brand-reinitialization-anonymous.js @@ -0,0 +1,10 @@ +// 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. + +class A { constructor(o) { return o } } + +let B = (0, class extends A { #x() { } }); + +let o = new B({}); +new B(o); diff --git a/test/message/fail/class-private-brand-reinitialization-anonymous.out b/test/message/fail/class-private-brand-reinitialization-anonymous.out new file mode 100644 index 0000000000..2005f5f716 --- /dev/null +++ b/test/message/fail/class-private-brand-reinitialization-anonymous.out @@ -0,0 +1,6 @@ +*%(basename)s:7: TypeError: Cannot initialize private methods of class anonymous twice on the same object +let B = (0, class extends A { #x() { } }); + ^ +TypeError: Cannot initialize private methods of class anonymous twice on the same object + at new B (*%(basename)s:7:13) + at *%(basename)s:10:1 diff --git a/test/message/fail/class-private-brand-reinitialization.js b/test/message/fail/class-private-brand-reinitialization.js new file mode 100644 index 0000000000..55f3fe73db --- /dev/null +++ b/test/message/fail/class-private-brand-reinitialization.js @@ -0,0 +1,10 @@ +// 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. + +class A { constructor(o) { return o } } + +class B extends A { #x() {} } + +let o = new B({}); +new B(o); diff --git a/test/message/fail/class-private-brand-reinitialization.out b/test/message/fail/class-private-brand-reinitialization.out new file mode 100644 index 0000000000..6241c334bd --- /dev/null +++ b/test/message/fail/class-private-brand-reinitialization.out @@ -0,0 +1,6 @@ +*%(basename)s:7: TypeError: Cannot initialize private methods of class B twice on the same object +class B extends A { #x() {} } +^ +TypeError: Cannot initialize private methods of class B twice on the same object + at new B (*%(basename)s:7:1) + at *%(basename)s:10:1 diff --git a/test/message/fail/class-private-field-reinitialization.js b/test/message/fail/class-private-field-reinitialization.js new file mode 100644 index 0000000000..8202948c95 --- /dev/null +++ b/test/message/fail/class-private-field-reinitialization.js @@ -0,0 +1,10 @@ +// 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. + +class A { constructor(o) { return o } } + +class B extends A { #x; } + +let o = new B({}); +new B(o); diff --git a/test/message/fail/class-private-field-reinitialization.out b/test/message/fail/class-private-field-reinitialization.out new file mode 100644 index 0000000000..6e8024f587 --- /dev/null +++ b/test/message/fail/class-private-field-reinitialization.out @@ -0,0 +1,7 @@ +*%(basename)s:7: TypeError: Cannot initialize #x twice on the same object +class B extends A { #x; } + ^ +TypeError: Cannot initialize #x twice on the same object + at Object. (*%(basename)s:7:21) + at new B (*%(basename)s:7:1) + at *%(basename)s:10:1