From 31a951d8755270e5ec784282f09743ada946bc8b Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Wed, 19 Jun 2019 17:59:28 +0800 Subject: [PATCH] [class] implement access of private methods This patch implements the access of private methods: - When building property loads, check whether it requires a brand check. If so, build the brand check and load the property (the method) from the context instead. - Throw type errors when there is an attempted write to private methods. Design: https://docs.google.com/document/d/1T-Ql6HOIH2U_8YjWkwK2rTfywwb7b3Qe8d3jkz72KwA/edit# Bug: v8:8330 Change-Id: Ic917d2a0030196c1940b0c0ba65a340af736c769 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1610383 Commit-Queue: Joyee Cheung Reviewed-by: Ross McIlroy Cr-Commit-Position: refs/heads/master@{#62292} --- src/execution/message-template.h | 1 + src/interpreter/bytecode-generator.cc | 56 ++++- src/interpreter/bytecode-generator.h | 1 + .../PrivateMethods.golden | 133 +++++----- .../interpreter/test-bytecode-generator.cc | 41 +-- .../fail/class-methods-private-throw-write.js | 13 + .../class-methods-private-throw-write.out | 6 + test/mjsunit/harmony/private-methods.js | 238 ++++++++++++++++-- 8 files changed, 377 insertions(+), 112 deletions(-) create mode 100644 test/message/fail/class-methods-private-throw-write.js create mode 100644 test/message/fail/class-methods-private-throw-write.out diff --git a/src/execution/message-template.h b/src/execution/message-template.h index 0bbdc691fb..cbcf5ce118 100644 --- a/src/execution/message-template.h +++ b/src/execution/message-template.h @@ -415,6 +415,7 @@ namespace internal { "Read of private field % from an object which did not contain the field") \ T(InvalidPrivateFieldWrite, \ "Write of private field % to an object which did not contain the field") \ + T(InvalidPrivateMethodWrite, "Private method '%' is not writable") \ T(JsonParseUnexpectedEOS, "Unexpected end of JSON input") \ T(JsonParseUnexpectedToken, "Unexpected token % in JSON at position %") \ T(JsonParseUnexpectedTokenNumber, "Unexpected number in JSON at position %") \ diff --git a/src/interpreter/bytecode-generator.cc b/src/interpreter/bytecode-generator.cc index ef345bcfec..1356d12d7d 100644 --- a/src/interpreter/bytecode-generator.cc +++ b/src/interpreter/bytecode-generator.cc @@ -2201,6 +2201,19 @@ void BytecodeGenerator::VisitInitializeClassMembersStatement( } } +void BytecodeGenerator::BuildThrowPrivateMethodWriteError( + const AstRawString* name) { + RegisterAllocationScope register_scope(this); + RegisterList args = register_allocator()->NewRegisterList(2); + builder() + ->LoadLiteral(Smi::FromEnum(MessageTemplate::kInvalidPrivateMethodWrite)) + .StoreAccumulatorInRegister(args[0]) + .LoadLiteral(name) + .StoreAccumulatorInRegister(args[1]) + .CallRuntime(Runtime::kNewTypeError, args) + .Throw(); +} + void BytecodeGenerator::BuildPrivateBrandInitialization(Register receiver) { RegisterList brand_args = register_allocator()->NewRegisterList(2); Variable* brand = info()->scope()->outer_scope()->AsClassScope()->brand(); @@ -3746,9 +3759,10 @@ void BytecodeGenerator::BuildAssignment( lhs_data.super_property_args()); break; } - case PRIVATE_METHOD: - // TODO(joyee): Throw TypeError because private methods are not writable. - UNREACHABLE(); + case PRIVATE_METHOD: { + BuildThrowPrivateMethodWriteError(lhs_data.name()); + break; + } } } @@ -3794,9 +3808,10 @@ void BytecodeGenerator::VisitCompoundAssignment(CompoundAssignment* expr) { lhs_data.super_property_args().Truncate(3)); break; } - case PRIVATE_METHOD: - // TODO(joyee): Throw TypeError because private methods are not writable. - UNREACHABLE(); + case PRIVATE_METHOD: { + BuildThrowPrivateMethodWriteError(lhs_data.name()); + break; + } } BinaryOperation* binop = expr->AsCompoundAssignment()->binary_operation(); FeedbackSlot slot = feedback_spec()->AddBinaryOpICSlot(); @@ -4254,8 +4269,23 @@ void BytecodeGenerator::VisitPropertyLoad(Register obj, Property* property) { case KEYED_SUPER_PROPERTY: VisitKeyedSuperPropertyLoad(property, Register::invalid_value()); break; - case PRIVATE_METHOD: - UNREACHABLE(); // TODO(joyee): implement private method access. + case PRIVATE_METHOD: { + Variable* private_name = property->key()->AsVariableProxy()->var(); + + // Perform the brand check. + DCHECK(private_name->requires_brand_check()); + ClassScope* scope = private_name->scope()->AsClassScope(); + Variable* brand = scope->brand(); + BuildVariableLoadForAccumulatorValue(brand, HoleCheckMode::kElided); + builder()->SetExpressionPosition(property); + builder()->LoadKeyedProperty( + obj, feedback_index(feedback_spec()->AddKeyedLoadICSlot())); + + // In the case of private methods, property->key() is the function to be + // loaded (stored in a context slot), so load this directly. + VisitForAccumulatorValue(property->key()); + break; + } } } @@ -4806,8 +4836,9 @@ void BytecodeGenerator::VisitCountOperation(CountOperation* expr) { break; } case PRIVATE_METHOD: { - // TODO(joyee): Throw TypeError because private methods are not writable. - UNREACHABLE(); + BuildThrowPrivateMethodWriteError( + property->key()->AsVariableProxy()->raw_name()); + break; } } @@ -4876,8 +4907,9 @@ void BytecodeGenerator::VisitCountOperation(CountOperation* expr) { break; } case PRIVATE_METHOD: { - // TODO(joyee): Throw TypeError because private methods are not writable. - UNREACHABLE(); + BuildThrowPrivateMethodWriteError( + property->key()->AsVariableProxy()->raw_name()); + break; } } diff --git a/src/interpreter/bytecode-generator.h b/src/interpreter/bytecode-generator.h index 618092c385..3732c626be 100644 --- a/src/interpreter/bytecode-generator.h +++ b/src/interpreter/bytecode-generator.h @@ -294,6 +294,7 @@ class BytecodeGenerator final : public AstVisitor { void VisitArgumentsObject(Variable* variable); void VisitRestArgumentsArray(Variable* rest); void VisitCallSuper(Call* call); + void BuildThrowPrivateMethodWriteError(const AstRawString* name); void BuildPrivateClassMemberNameAssignment(ClassLiteral::Property* property); void BuildClassLiteral(ClassLiteral* expr, Register name); void VisitClassLiteral(ClassLiteral* expr, Register name); diff --git a/test/cctest/interpreter/bytecode_expectations/PrivateMethods.golden b/test/cctest/interpreter/bytecode_expectations/PrivateMethods.golden index e783d81376..93fe47925a 100644 --- a/test/cctest/interpreter/bytecode_expectations/PrivateMethods.golden +++ b/test/cctest/interpreter/bytecode_expectations/PrivateMethods.golden @@ -11,47 +11,57 @@ snippet: " { class A { #a() { return 1; } + callA() { return this.#a(); } } - new A; + const a = new A; + a.callA(); } " -frame size: 7 +frame size: 9 parameter count: 1 -bytecode array length: 62 +bytecode array length: 80 bytecodes: [ /* 30 E> */ B(StackCheck), B(CreateBlockContext), U8(0), - B(PushContext), R(2), + B(PushContext), R(3), B(LdaTheHole), - B(Star), R(6), + B(Star), R(7), B(CreateClosure), U8(2), U8(0), U8(2), - B(Star), R(3), - B(LdaConstant), U8(1), B(Star), R(4), + B(LdaConstant), U8(1), + B(Star), R(5), B(CreateClosure), U8(3), U8(1), U8(2), B(StaCurrentContextSlot), U8(4), - B(Mov), R(3), R(5), - B(CallRuntime), U16(Runtime::kDefineClass), R(4), U8(3), - B(Star), R(4), - B(Mov), R(5), R(1), - B(LdaConstant), U8(4), + B(CreateClosure), U8(4), U8(2), U8(2), + B(Star), R(8), + B(Mov), R(4), R(6), + B(CallRuntime), U16(Runtime::kDefineClass), R(5), U8(4), B(Star), R(5), - B(CallRuntime), U16(Runtime::kCreatePrivateNameSymbol), R(5), U8(1), + B(Mov), R(6), R(2), + B(LdaConstant), U8(5), + B(Star), R(6), + B(CallRuntime), U16(Runtime::kCreatePrivateNameSymbol), R(6), U8(1), B(StaCurrentContextSlot), U8(5), - B(PopContext), R(2), - B(Mov), R(1), R(0), - /* 78 S> */ B(Ldar), R(0), - /* 78 E> */ B(Construct), R(0), R(0), U8(0), U8(0), + B(PopContext), R(3), + B(Mov), R(2), R(0), + /* 122 S> */ B(Ldar), R(0), + /* 122 E> */ B(Construct), R(0), R(0), U8(0), U8(0), + B(Star), R(1), + /* 133 S> */ B(LdaNamedProperty), R(1), U8(6), U8(2), + B(Star), R(3), + /* 133 E> */ B(CallProperty0), R(3), R(1), U8(4), B(LdaUndefined), - /* 87 S> */ B(Return), + /* 143 S> */ B(Return), ] constant pool: [ SCOPE_INFO_TYPE, FIXED_ARRAY_TYPE, SHARED_FUNCTION_INFO_TYPE, SHARED_FUNCTION_INFO_TYPE, + SHARED_FUNCTION_INFO_TYPE, ONE_BYTE_INTERNALIZED_STRING_TYPE ["A"], + ONE_BYTE_INTERNALIZED_STRING_TYPE ["callA"], ] handlers: [ ] @@ -60,79 +70,90 @@ handlers: [ snippet: " { class D { - #d() {} + #d() { return 1; } + callD() { return this.#d(); } } class E extends D { - #e() {} + #e() { return 2; } + callE() { return this.callD() + this.#e(); } } - new D; - new E; + const e = new E; + e.callE(); } " -frame size: 9 +frame size: 11 parameter count: 1 -bytecode array length: 121 +bytecode array length: 138 bytecodes: [ /* 30 E> */ B(StackCheck), B(CreateBlockContext), U8(0), - B(PushContext), R(4), + B(PushContext), R(5), B(LdaTheHole), - B(Star), R(8), + B(Star), R(9), B(CreateClosure), U8(2), U8(0), U8(2), - B(Star), R(5), - B(LdaConstant), U8(1), B(Star), R(6), + B(LdaConstant), U8(1), + B(Star), R(7), B(CreateClosure), U8(3), U8(1), U8(2), B(StaCurrentContextSlot), U8(4), - B(Mov), R(5), R(7), - B(CallRuntime), U16(Runtime::kDefineClass), R(6), U8(3), - B(Star), R(6), - B(Mov), R(7), R(3), - B(LdaConstant), U8(4), + B(CreateClosure), U8(4), U8(2), U8(2), + B(Star), R(10), + B(Mov), R(6), R(8), + B(CallRuntime), U16(Runtime::kDefineClass), R(7), U8(4), B(Star), R(7), - B(CallRuntime), U16(Runtime::kCreatePrivateNameSymbol), R(7), U8(1), + B(Mov), R(8), R(4), + B(LdaConstant), U8(5), + B(Star), R(8), + B(CallRuntime), U16(Runtime::kCreatePrivateNameSymbol), R(8), U8(1), B(StaCurrentContextSlot), U8(5), - B(PopContext), R(4), - B(Mov), R(3), R(0), - /* 38 E> */ B(CreateBlockContext), U8(5), - B(PushContext), R(4), - /* 83 E> */ B(CreateClosure), U8(7), U8(2), U8(2), - B(Star), R(5), - B(LdaConstant), U8(6), + B(PopContext), R(5), + B(Mov), R(4), R(0), + /* 38 E> */ B(CreateBlockContext), U8(6), + B(PushContext), R(5), + /* 128 E> */ B(CreateClosure), U8(8), U8(3), U8(2), B(Star), R(6), - B(CreateClosure), U8(8), U8(3), U8(2), + B(LdaConstant), U8(7), + B(Star), R(7), + B(CreateClosure), U8(9), U8(4), U8(2), B(StaCurrentContextSlot), U8(4), - B(Mov), R(5), R(7), - B(Mov), R(3), R(8), - B(CallRuntime), U16(Runtime::kDefineClass), R(6), U8(3), - B(Star), R(6), - B(Mov), R(7), R(2), - B(LdaConstant), U8(9), + B(CreateClosure), U8(10), U8(5), U8(2), + B(Star), R(10), + B(Mov), R(6), R(8), + B(Mov), R(4), R(9), + B(CallRuntime), U16(Runtime::kDefineClass), R(7), U8(4), B(Star), R(7), - B(CallRuntime), U16(Runtime::kCreatePrivateNameSymbol), R(7), U8(1), + B(Mov), R(8), R(3), + B(LdaConstant), U8(11), + B(Star), R(8), + B(CallRuntime), U16(Runtime::kCreatePrivateNameSymbol), R(8), U8(1), B(StaCurrentContextSlot), U8(5), - B(PopContext), R(4), - B(Mov), R(2), R(1), - /* 106 S> */ B(Ldar), R(3), - /* 106 E> */ B(Construct), R(3), R(0), U8(0), U8(0), - /* 115 S> */ B(Ldar), R(2), - /* 115 E> */ B(Construct), R(2), R(0), U8(0), U8(2), + B(PopContext), R(5), + B(Mov), R(3), R(1), + /* 221 S> */ B(Ldar), R(1), + /* 221 E> */ B(Construct), R(1), R(0), U8(0), U8(0), + B(Star), R(2), + /* 232 S> */ B(LdaNamedProperty), R(2), U8(12), U8(2), + B(Star), R(5), + /* 232 E> */ B(CallProperty0), R(5), R(2), U8(4), B(LdaUndefined), - /* 124 S> */ B(Return), + /* 242 S> */ B(Return), ] constant pool: [ SCOPE_INFO_TYPE, FIXED_ARRAY_TYPE, SHARED_FUNCTION_INFO_TYPE, SHARED_FUNCTION_INFO_TYPE, + SHARED_FUNCTION_INFO_TYPE, ONE_BYTE_INTERNALIZED_STRING_TYPE ["D"], SCOPE_INFO_TYPE, FIXED_ARRAY_TYPE, SHARED_FUNCTION_INFO_TYPE, SHARED_FUNCTION_INFO_TYPE, + SHARED_FUNCTION_INFO_TYPE, ONE_BYTE_INTERNALIZED_STRING_TYPE ["E"], + ONE_BYTE_INTERNALIZED_STRING_TYPE ["callE"], ] handlers: [ ] diff --git a/test/cctest/interpreter/test-bytecode-generator.cc b/test/cctest/interpreter/test-bytecode-generator.cc index 3a4d089786..d9882a4ac8 100644 --- a/test/cctest/interpreter/test-bytecode-generator.cc +++ b/test/cctest/interpreter/test-bytecode-generator.cc @@ -2764,26 +2764,29 @@ TEST(PrivateMethods) { BytecodeExpectationsPrinter printer(CcTest::isolate()); const char* snippets[] = { - "{\n" - " class A {\n" - " #a() { return 1; }\n" - " }\n" - "\n" - " new A;\n" - "}\n", + R"({ + class A { + #a() { return 1; } + callA() { return this.#a(); } + } - "{\n" - " class D {\n" - " #d() {}\n" - " }\n" - "\n" - " class E extends D {\n" - " #e() {}\n" - " }\n" - "\n" - " new D;\n" - " new E;\n" - "}\n"}; + const a = new A; + a.callA(); +})", + R"({ + class D { + #d() { return 1; } + callD() { return this.#d(); } + } + + class E extends D { + #e() { return 2; } + callE() { return this.callD() + this.#e(); } + } + + const e = new E; + e.callE(); +})"}; CHECK(CompareTexts(BuildActual(printer, snippets), LoadGolden("PrivateMethods.golden"))); i::FLAG_harmony_private_methods = old_methods_flag; diff --git a/test/message/fail/class-methods-private-throw-write.js b/test/message/fail/class-methods-private-throw-write.js new file mode 100644 index 0000000000..3181fea1b6 --- /dev/null +++ b/test/message/fail/class-methods-private-throw-write.js @@ -0,0 +1,13 @@ +// 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. + +// Flags: --harmony-private-methods + +class C { + #a() {} + constructor() { + this.#a = 1; + } +} +new C; diff --git a/test/message/fail/class-methods-private-throw-write.out b/test/message/fail/class-methods-private-throw-write.out new file mode 100644 index 0000000000..2aadad9c3c --- /dev/null +++ b/test/message/fail/class-methods-private-throw-write.out @@ -0,0 +1,6 @@ +*%(basename)s:10: TypeError: Private method '#a' is not writable + this.#a = 1; + ^ +TypeError: Private method '#a' is not writable + at new C (*%(basename)s:10:13) + at *%(basename)s:13:1 \ No newline at end of file diff --git a/test/mjsunit/harmony/private-methods.js b/test/mjsunit/harmony/private-methods.js index e7784a29f5..360b065f17 100644 --- a/test/mjsunit/harmony/private-methods.js +++ b/test/mjsunit/harmony/private-methods.js @@ -6,57 +6,211 @@ "use strict"; +// Basic private method test { + let calledWith; class C { - #a() {} + #a(arg) { calledWith = arg; } + callA(arg) { this.#a(arg); } } - new C; + + const c = new C; + assertEquals(undefined, c.a); + assertEquals(undefined, calledWith); + c.callA(1); + assertEquals(1, calledWith); } +// Call private method in another instance { class C { - #a() { - class B { - #a() { } - } - new B; + #a(arg) { this.calledWith = arg; } + callAIn(obj, arg) { obj.#a(arg); } + } + + const c = new C; + const c2 = new C; + + assertEquals(undefined, c.a); + assertEquals(undefined, c.calledWith); + assertEquals(undefined, c2.calledWith); + + c2.callAIn(c, 'fromC2'); + assertEquals('fromC2', c.calledWith); + assertEquals(undefined, c2.calledWith); + + c2.callAIn(c2, 'c2'); + assertEquals('fromC2', c.calledWith); + assertEquals('c2', c2.calledWith); + + assertThrows(() => { c2.callAIn({}); }, TypeError); +} + +// Private methods and private fields +{ + class C { + #a; + constructor(a) { + this.#a = a; + } + #getAPlus1() { + return this.#a + 1; + } + equals(obj) { + return this.#getAPlus1() === obj.#getAPlus1(); } } - new C; + const c = new C(0); + const c2 = new C(2); + const c3 = new C(2); + assertEquals(true, c2.equals(c3)); + assertEquals(false, c2.equals(c)); + assertEquals(false, c3.equals(c)); } +// Class inheritance { class A { - #a() { - class C extends A { - #c() { } - } - new C; + #val; + constructor(a) { + this.#val = a; } + #a() { return this.#val; } + getA() { return this.#a(); } } - - new A; + class B extends A { + constructor(b) { + super(b); + } + b() { return this.getA() } + } + const b = new B(1); + assertEquals(1, b.b()); } +// Private members should be accessed according to the class the +// invoked method is in. { - const C = class { - #a() { } + class A { + #val; + constructor(a) { + this.#val = a; + } + #getVal() { return this.#val; } + getA() { return this.#getVal(); } + getVal() { return this.#getVal(); } } - new C; + + class B extends A { + #val; + constructor(a, b) { + super(a); + this.#val = b; + } + #getVal() { return this.#val; } + getB() { return this.#getVal(); } + getVal() { return this.#getVal(); } + } + + const b = new B(1, 2); + assertEquals(1, b.getA()); + assertEquals(2, b.getB()); + assertEquals(1, A.prototype.getVal.call(b)); + assertEquals(2, B.prototype.getVal.call(b)); + const a = new A(1); + assertEquals(1, a.getA()); + assertThrows(() => B.prototype.getB.call(a), TypeError); } +// Private methods in nested classes. +{ + class C { + #b() { + class B { + #foo(arg) { return arg; } + callFoo(arg) { return this.#foo(arg); } + } + return new B(); + } + createB() { return this.#b(); } + } + const c = new C; + const b = c.createB(); + assertEquals(1, b.callFoo(1)); +} + +// Private methods in nested classes with inheritance. +{ + class C { + #b() { + class B extends C { + #foo(arg) { return arg; } + callFoo(arg) { return this.#foo(arg); } + } + return new B(); + } + createB() { return this.#b(); } + } + + const c = new C; + const b = c.createB(); + assertEquals(1, b.callFoo(1)); + const b2 = b.createB(); + assertEquals(1, b2.callFoo(1)); +} + +// Class expressions. { const C = class { - #a() { + #a() { return 1; } + callA(obj) { return obj.#a() } + }; + const c = new C; + const c2 = new C; + assertEquals(1, c.callA(c)); + assertEquals(1, c.callA(c2)); +} + +// Nested class expressions. +{ + const C = class { + #b() { const B = class { - #a() { } - } - new B; + #foo(arg) { return arg; } + callFoo(arg) { return this.#foo(arg); } + }; + return new B(); } - } - new C; + createB() { return this.#b(); } + }; + + const c = new C; + const b = c.createB(); + assertEquals(1, b.callFoo(1)); } + +// Nested class expressions with hierarchy. +{ + const C = class { + #b() { + const B = class extends C { + #foo(arg) { return arg; } + callFoo(arg) { return this.#foo(arg); } + } + return new B(); + } + createB() { return this.#b(); } + } + + const c = new C; + const b = c.createB(); + assertEquals(1, b.callFoo(1)); + const b2 = b.createB(); + assertEquals(1, b2.callFoo(1)); +} + +// Adding the brand twice on the same object should throw. { class A { constructor(arg) { @@ -72,11 +226,45 @@ } } - // Add the brand twice on the same object. let c1 = new C({}); assertThrows(() => new C(c1), TypeError); } +// Private methods should be not visible to proxies. +{ + class X { + #x() {} + x() { this.#x(); }; + callX(obj) { obj.#x(); } + } + let handlerCalled = false; + const x = new X(); + let p = new Proxy(new X, { + apply(target, thisArg, argumentsList) { + handlerCalled = true; + Reflect.apply(target, thisArg, argumentsList); + } + }); + assertThrows(() => p.x(), TypeError); + assertThrows(() => x.callX(p), TypeError); + assertThrows(() => X.prototype.x.call(p), TypeError); + assertThrows(() => X.prototype.callX(p), TypeError); + assertEquals(false, handlerCalled); +} + +// Reference outside of class. +{ + class C { + #a() {} + } + assertThrows('new C().#a()'); +} + +// Duplicate private names. +{ + assertThrows('class C { #a = 1; #a() {} }'); +} + { // TODO(v8:9177): test extending a class expression that does not have // a private method.