[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 <joyee@igalia.com>
Reviewed-by: Ross McIlroy <rmcilroy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#62292}

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 %") \

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:
+    case PRIVATE_METHOD: {
-      // TODO(joyee): Throw TypeError because private methods are not writable.
+      BuildThrowPrivateMethodWriteError(lhs_data.name());
-      UNREACHABLE();
+      break;
+    }
   }
 }
 
@@ -3794,9 +3808,10 @@ void BytecodeGenerator::VisitCompoundAssignment(CompoundAssignment* expr) {
                              lhs_data.super_property_args().Truncate(3));
       break;
     }
-    case PRIVATE_METHOD:
+    case PRIVATE_METHOD: {
-      // TODO(joyee): Throw TypeError because private methods are not writable.
+      BuildThrowPrivateMethodWriteError(lhs_data.name());
-      UNREACHABLE();
+      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:
+    case PRIVATE_METHOD: {
-      UNREACHABLE();  // TODO(joyee): implement private method access.
+      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.
+      BuildThrowPrivateMethodWriteError(
-      UNREACHABLE();
+          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.
+      BuildThrowPrivateMethodWriteError(
-      UNREACHABLE();
+          property->key()->AsVariableProxy()->raw_name());
+      break;
     }
   }
 

src/interpreter/bytecode-generator.h

@@ -294,6 +294,7 @@ class BytecodeGenerator final : public AstVisitor<BytecodeGenerator> {
   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);

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(CreateClosure), U8(4), U8(2), U8(2),
-                B(CallRuntime), U16(Runtime::kDefineClass), R(4), U8(3),
+                B(Star), R(8),
-                B(Star), R(4),
+                B(Mov), R(4), R(6),
-                B(Mov), R(5), R(1),
+                B(CallRuntime), U16(Runtime::kDefineClass), R(5), U8(4),
-                B(LdaConstant), 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(PopContext), R(3),
-                B(Mov), R(1), R(0),
+                B(Mov), R(2), R(0),
-  /*   78 S> */ B(Ldar), R(0),
+  /*  122 S> */ B(Ldar), R(0),
-  /*   78 E> */ B(Construct), R(0), R(0), U8(0), U8(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;
+    const e = new 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(CreateClosure), U8(4), U8(2), U8(2),
-                B(CallRuntime), U16(Runtime::kDefineClass), R(6), U8(3),
+                B(Star), R(10),
-                B(Star), R(6),
+                B(Mov), R(6), R(8),
-                B(Mov), R(7), R(3),
+                B(CallRuntime), U16(Runtime::kDefineClass), R(7), U8(4),
-                B(LdaConstant), 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(PopContext), R(5),
-                B(Mov), R(3), R(0),
+                B(Mov), R(4), R(0),
-  /*   38 E> */ B(CreateBlockContext), U8(5),
+  /*   38 E> */ B(CreateBlockContext), U8(6),
-                B(PushContext), R(4),
+                B(PushContext), R(5),
-  /*   83 E> */ B(CreateClosure), U8(7), U8(2), U8(2),
+  /*  128 E> */ B(CreateClosure), U8(8), U8(3), U8(2),
-                B(Star), R(5),
-                B(LdaConstant), U8(6),
                 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(CreateClosure), U8(10), U8(5), U8(2),
-                B(Mov), R(3), R(8),
+                B(Star), R(10),
-                B(CallRuntime), U16(Runtime::kDefineClass), R(6), U8(3),
+                B(Mov), R(6), R(8),
-                B(Star), R(6),
+                B(Mov), R(4), R(9),
-                B(Mov), R(7), R(2),
+                B(CallRuntime), U16(Runtime::kDefineClass), R(7), U8(4),
-                B(LdaConstant), U8(9),
                 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(PopContext), R(5),
-                B(Mov), R(2), R(1),
+                B(Mov), R(3), R(1),
-  /*  106 S> */ B(Ldar), R(3),
+  /*  221 S> */ B(Ldar), R(1),
-  /*  106 E> */ B(Construct), R(3), R(0), U8(0), U8(0),
+  /*  221 E> */ B(Construct), R(1), R(0), U8(0), U8(0),
-  /*  115 S> */ B(Ldar), R(2),
+                B(Star), R(2),
-  /*  115 E> */ B(Construct), R(2), R(0), U8(0), U8(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: [
 ]

test/cctest/interpreter/test-bytecode-generator.cc

@@ -2764,26 +2764,29 @@ TEST(PrivateMethods) {
   BytecodeExpectationsPrinter printer(CcTest::isolate());
 
   const char* snippets[] = {
-      "{\n"
+      R"({
-      "  class A {\n"
+  class A {
-      "    #a() { return 1; }\n"
+    #a() { return 1; }
-      "  }\n"
+    callA() { return this.#a(); }
-      "\n"
+  }
-      "  new A;\n"
-      "}\n",
 
-      "{\n"
+  const a = new A;
-      "  class D {\n"
+  a.callA();
-      "    #d() {}\n"
+})",
-      "  }\n"
+      R"({
-      "\n"
+  class D {
-      "  class E extends D {\n"
+    #d() { return 1; }
-      "    #e() {}\n"
+    callD() { return this.#d(); }
-      "  }\n"
+  }
-      "\n"
+
-      "  new D;\n"
+  class E extends D {
-      "  new E;\n"
+    #e() { return 2; }
-      "}\n"};
+    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;

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;

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

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() {
+    #a(arg) { this.calledWith = arg; }
-      class B {
+    callAIn(obj, arg) { obj.#a(arg); }
-        #a() {  }
+  }
-      }
+
-      new B;
+  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() {
+    #val;
-      class C extends A {
+    constructor(a) {
-        #c() { }
+      this.#val = a;
-      }
-      new C;
     }
+    #a() { return this.#val; }
+    getA() { return this.#a(); }
   }
-
+  class B extends A {
-  new 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 {
+  class A {
-    #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() {  }
+        #foo(arg) { return arg; }
-      }
+        callFoo(arg) { return this.#foo(arg); }
-      new B;
+      };
+      return new B();
     }
-  }
+    createB() { return this.#b(); }
-  new C;
+  };
+
+  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.