[class] fix read-only private references in logical assignments

Since assignments to read-only private references can be skipped due
to short-circuiting in logical assignments, we should not eagerly
emit the error of invalid writes, and should instead load the values
as usual, only emitting an error when the assignment happens,
which can be handled by BytecodeGenerator::BuildAssignment().

Bug: v8:12680, v8:8330, v8:10372
Change-Id: Ia5fea9090bc48b0af8a9c8d6f95174f7aa2d86f8
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3509298Reviewed-by: Shu-yu Guo <syg@chromium.org>
Reviewed-by: Marja Hölttä <marja@chromium.org>
Commit-Queue: Joyee Cheung <joyee@igalia.com>
Cr-Commit-Position: refs/heads/main@{#79583}

src/ast/ast.h

@@ -1598,14 +1598,14 @@ class OptionalChain final : public Expression {
 // Otherwise, the assignment is to a non-property (a global, a local slot, a
 // parameter slot, or a destructuring pattern).
 enum AssignType {
-  NON_PROPERTY,              // destructuring
-  NAMED_PROPERTY,            // obj.key
-  KEYED_PROPERTY,            // obj[key]
-  NAMED_SUPER_PROPERTY,      // super.key
-  KEYED_SUPER_PROPERTY,      // super[key]
-  PRIVATE_METHOD,            // obj.#key: #key is a private method
-  PRIVATE_GETTER_ONLY,       // obj.#key: #key only has a getter defined
-  PRIVATE_SETTER_ONLY,       // obj.#key: #key only has a setter defined
+  NON_PROPERTY,          // destructuring
+  NAMED_PROPERTY,        // obj.key
+  KEYED_PROPERTY,        // obj[key] and obj.#key when #key is a private field
+  NAMED_SUPER_PROPERTY,  // super.key
+  KEYED_SUPER_PROPERTY,  // super[key]
+  PRIVATE_METHOD,        // obj.#key: #key is a private method
+  PRIVATE_GETTER_ONLY,   // obj.#key: #key only has a getter defined
+  PRIVATE_SETTER_ONLY,   // obj.#key: #key only has a setter defined
   PRIVATE_GETTER_AND_SETTER  // obj.#key: #key has both accessors defined
 };
 

src/interpreter/bytecode-generator.cc

@@ -3952,10 +3952,7 @@ BytecodeGenerator::AssignmentLhsData BytecodeGenerator::PrepareAssignmentLhs(
       DCHECK(!property->IsSuperAccess());
       AccumulatorPreservingScope scope(this, accumulator_preserving_mode);
       Register object = VisitForRegisterValue(property->obj());
-      Register key =
-          assign_type == PRIVATE_GETTER_ONLY || assign_type == PRIVATE_METHOD
-              ? Register()
-              : VisitForRegisterValue(property->key());
+      Register key = VisitForRegisterValue(property->key());
       return AssignmentLhsData::PrivateMethodOrAccessor(assign_type, property,
                                                         object, key);
     }
@@ -4588,23 +4585,21 @@ void BytecodeGenerator::VisitCompoundAssignment(CompoundAssignment* expr) {
                              lhs_data.super_property_args().Truncate(3));
       break;
     }
+    // BuildAssignment() will throw an error about the private method being
+    // read-only.
     case PRIVATE_METHOD: {
-      // The property access is invalid, but if the brand check fails too, we
-      // need to return the error from the brand check.
       Property* property = lhs_data.expr()->AsProperty();
       BuildPrivateBrandCheck(property, lhs_data.object());
-      BuildInvalidPropertyAccess(MessageTemplate::kInvalidPrivateMethodWrite,
-                                 lhs_data.expr()->AsProperty());
+      builder()->LoadAccumulatorWithRegister(lhs_data.key());
       break;
     }
-    case PRIVATE_GETTER_ONLY: {
-      // The property access is invalid, but if the brand check fails too, we
-      // need to return the error from the brand check.
+    // For read-only properties, BuildAssignment() will throw an error about
+    // the missing setter.
+    case PRIVATE_GETTER_ONLY:
+    case PRIVATE_GETTER_AND_SETTER: {
       Property* property = lhs_data.expr()->AsProperty();
       BuildPrivateBrandCheck(property, lhs_data.object());
-      BuildInvalidPropertyAccess(MessageTemplate::kInvalidPrivateSetterAccess,
-                                 lhs_data.expr()->AsProperty());
-
+      BuildPrivateGetterAccess(lhs_data.object(), lhs_data.key());
       break;
     }
     case PRIVATE_SETTER_ONLY: {
@@ -4616,12 +4611,6 @@ void BytecodeGenerator::VisitCompoundAssignment(CompoundAssignment* expr) {
                                  lhs_data.expr()->AsProperty());
       break;
     }
-    case PRIVATE_GETTER_AND_SETTER: {
-      Property* property = lhs_data.expr()->AsProperty();
-      BuildPrivateBrandCheck(property, lhs_data.object());
-      BuildPrivateGetterAccess(lhs_data.object(), lhs_data.key());
-      break;
-    }
   }
 
   BinaryOperation* binop = expr->binary_operation();
@@ -4651,6 +4640,7 @@ void BytecodeGenerator::VisitCompoundAssignment(CompoundAssignment* expr) {
     builder()->BinaryOperation(binop->op(), old_value, feedback_index(slot));
   }
   builder()->SetExpressionPosition(expr);
+
   BuildAssignment(lhs_data, expr->op(), expr->lookup_hoisting_mode());
   builder()->Bind(&short_circuit);
 }

src/interpreter/bytecode-generator.h

@@ -128,7 +128,8 @@ class BytecodeGenerator final : public AstVisitor<BytecodeGenerator> {
       return object_;
     }
     Register key() const {
-      DCHECK(assign_type_ == KEYED_PROPERTY ||
+      DCHECK(assign_type_ == KEYED_PROPERTY || assign_type_ == PRIVATE_METHOD ||
+             assign_type_ == PRIVATE_GETTER_ONLY ||
              assign_type_ == PRIVATE_SETTER_ONLY ||
              assign_type_ == PRIVATE_GETTER_AND_SETTER);
       return key_;

test/cctest/interpreter/bytecode_expectations/PrivateAccessorAccess.golden

@@ -137,21 +137,23 @@ snippet: "
   var test = D;
   new test;
 "
-frame size: 4
+frame size: 5
 parameter count: 1
-bytecode array length: 28
+bytecode array length: 31
 bytecodes: [
                 B(LdaImmutableCurrentContextSlot), U8(3),
                 B(Star0),
                 B(Ldar), R(context),
   /*   48 E> */ B(DefineKeyedOwnProperty), R(this), R(0), U8(0),
-  /*   53 S> */ B(LdaImmutableCurrentContextSlot), U8(3),
+  /*   53 S> */ B(LdaImmutableCurrentContextSlot), U8(2),
+                B(Star2),
+                B(LdaImmutableCurrentContextSlot), U8(3),
   /*   58 E> */ B(GetKeyedProperty), R(this), U8(2),
                 B(Wide), B(LdaSmi), I16(296),
-                B(Star2),
-                B(LdaConstant), U8(0),
                 B(Star3),
-                B(CallRuntime), U16(Runtime::kNewTypeError), R(2), U8(2),
+                B(LdaConstant), U8(0),
+                B(Star4),
+                B(CallRuntime), U16(Runtime::kNewTypeError), R(3), U8(2),
                 B(Throw),
 ]
 constant pool: [

test/cctest/interpreter/bytecode_expectations/PrivateMethodAccess.golden

@@ -46,21 +46,23 @@ snippet: "
   var test = B;
   new test;
 "
-frame size: 4
+frame size: 5
 parameter count: 1
-bytecode array length: 28
+bytecode array length: 31
 bytecodes: [
                 B(LdaImmutableCurrentContextSlot), U8(3),
                 B(Star0),
                 B(Ldar), R(context),
   /*   44 E> */ B(DefineKeyedOwnProperty), R(this), R(0), U8(0),
-  /*   49 S> */ B(LdaImmutableCurrentContextSlot), U8(3),
+  /*   49 S> */ B(LdaImmutableCurrentContextSlot), U8(2),
+                B(Star2),
+                B(LdaImmutableCurrentContextSlot), U8(3),
   /*   54 E> */ B(GetKeyedProperty), R(this), U8(2),
                 B(Wide), B(LdaSmi), I16(294),
-                B(Star2),
-                B(LdaConstant), U8(0),
                 B(Star3),
-                B(CallRuntime), U16(Runtime::kNewTypeError), R(2), U8(2),
+                B(LdaConstant), U8(0),
+                B(Star4),
+                B(CallRuntime), U16(Runtime::kNewTypeError), R(3), U8(2),
                 B(Throw),
 ]
 constant pool: [

test/cctest/interpreter/bytecode_expectations/StaticPrivateMethodAccess.golden

@@ -51,25 +51,27 @@ snippet: "
   var test = B.test;
   test();
 "
-frame size: 3
+frame size: 4
 parameter count: 1
-bytecode array length: 37
+bytecode array length: 40
 bytecodes: [
-  /*   56 S> */ B(LdaCurrentContextSlot), U8(3),
+  /*   56 S> */ B(LdaImmutableCurrentContextSlot), U8(2),
+                B(Star1),
+                B(LdaCurrentContextSlot), U8(3),
                 B(TestReferenceEqual), R(this),
                 B(Mov), R(this), R(0),
                 B(JumpIfTrue), U8(16),
                 B(Wide), B(LdaSmi), I16(288),
-                B(Star1),
-                B(LdaConstant), U8(0),
                 B(Star2),
-  /*   61 E> */ B(CallRuntime), U16(Runtime::kNewTypeError), R(1), U8(2),
+                B(LdaConstant), U8(0),
+                B(Star3),
+  /*   61 E> */ B(CallRuntime), U16(Runtime::kNewTypeError), R(2), U8(2),
                 B(Throw),
                 B(Wide), B(LdaSmi), I16(294),
-                B(Star1),
-                B(LdaConstant), U8(1),
                 B(Star2),
-                B(CallRuntime), U16(Runtime::kNewTypeError), R(1), U8(2),
+                B(LdaConstant), U8(1),
+                B(Star3),
+                B(CallRuntime), U16(Runtime::kNewTypeError), R(2), U8(2),
                 B(Throw),
 ]
 constant pool: [
@@ -280,25 +282,27 @@ snippet: "
   var test = G.test;
   test();
 "
-frame size: 3
+frame size: 4
 parameter count: 1
-bytecode array length: 37
+bytecode array length: 40
 bytecodes: [
-  /*   60 S> */ B(LdaCurrentContextSlot), U8(3),
+  /*   60 S> */ B(LdaImmutableCurrentContextSlot), U8(2),
+                B(Star1),
+                B(LdaCurrentContextSlot), U8(3),
                 B(TestReferenceEqual), R(this),
                 B(Mov), R(this), R(0),
                 B(JumpIfTrue), U8(16),
                 B(Wide), B(LdaSmi), I16(288),
-                B(Star1),
-                B(LdaConstant), U8(0),
                 B(Star2),
-  /*   65 E> */ B(CallRuntime), U16(Runtime::kNewTypeError), R(1), U8(2),
+                B(LdaConstant), U8(0),
+                B(Star3),
+  /*   65 E> */ B(CallRuntime), U16(Runtime::kNewTypeError), R(2), U8(2),
                 B(Throw),
                 B(Wide), B(LdaSmi), I16(296),
-                B(Star1),
-                B(LdaConstant), U8(1),
                 B(Star2),
-                B(CallRuntime), U16(Runtime::kNewTypeError), R(1), U8(2),
+                B(LdaConstant), U8(1),
+                B(Star3),
+                B(CallRuntime), U16(Runtime::kNewTypeError), R(2), U8(2),
                 B(Throw),
 ]
 constant pool: [

test/mjsunit/harmony/private-reference-logical-assignment-short-circuit.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.
+
+'use strict';
+
+function doNotCall() {
+  throw new Error("The right-hand side should not be evaluated");
+}
+
+{
+  class C {
+    get #readOnlyFalse() {
+      return false;
+    }
+
+    get #readOnlyTrue() {
+      return true;
+    }
+
+    get #readOnlyOne() {
+      return 1;
+    }
+
+    get #readOnlyNull() {
+      return null;
+    }
+
+    get #readOnlyUndefined() {
+      return undefined;
+    }
+
+    shortCircuitedAndFalse() {
+      return this.#readOnlyFalse &&= doNotCall();
+    }
+
+    shortCircuitedOrTrue() {
+      return this.#readOnlyTrue ||= doNotCall();
+    }
+
+    shortCircuitedNullishOne() {
+      return this.#readOnlyOne ??= doNotCall();
+    }
+
+    andAssignReadOnly() {
+      return this.#readOnlyTrue &&= 1;
+    }
+
+    orAssignReadOnly() {
+      return this.#readOnlyFalse ||= 0;
+    }
+
+    nullishAssignReadOnlyNull() {
+      return this.#readOnlyNull ??= 1;
+    }
+
+    nullishAssignReadOnlyUndefined() {
+      return this.#readOnlyUndefined ??= 1;
+    }
+  }
+
+  const o = new C();
+  assertEquals(
+    o.shortCircuitedAndFalse(),
+    false,
+    "The expression should evaluate to the short-circuited value");
+  assertEquals(
+    o.shortCircuitedOrTrue(),
+    true,
+    "The expression should evaluate to the short-circuited value");
+  assertEquals(
+    o.shortCircuitedNullishOne(),
+    1,
+    "The expression should evaluate to the short-circuited value");
+
+  assertThrows(
+    () => o.andAssignReadOnly(),
+    TypeError,
+    /'#readOnlyTrue' was defined without a setter/
+  );
+  assertThrows(
+    () => o.orAssignReadOnly(),
+    TypeError,
+    /'#readOnlyFalse' was defined without a setter/
+  );
+  assertThrows(
+    () => o.nullishAssignReadOnlyNull(),
+    TypeError,
+    /'#readOnlyNull' was defined without a setter/
+  );
+  assertThrows(
+    () => o.nullishAssignReadOnlyUndefined(),
+    TypeError,
+    /'#readOnlyUndefined' was defined without a setter/
+  );
+}
+
+{
+  class C {
+    #privateMethod() { }
+
+    getPrivateMethod() {
+      return this.#privateMethod;
+    }
+
+    shortCircuitedNullishPrivateMethod() {
+      return this.#privateMethod ??= doNotCall();
+    }
+
+    shortCircuitedOrPrivateMethod() {
+      return this.#privateMethod ||= doNotCall();
+    }
+
+    andAssignReadOnly() {
+      return this.#privateMethod &&= 1;
+    }
+  }
+
+  const o = new C();
+  assertEquals(
+    o.shortCircuitedNullishPrivateMethod(),
+    o.getPrivateMethod(),
+    "The expression should evaluate to the short-circuited value");
+
+  assertEquals(
+    o.shortCircuitedNullishPrivateMethod(),
+    o.getPrivateMethod(),
+    "The expression should evaluate to the short-circuited value");
+
+  assertThrows(
+    () => o.andAssignReadOnly(),
+    TypeError,
+    /Private method '#privateMethod' is not writable/
+  );
+}

test/test262/test262.status

@@ -2822,13 +2822,6 @@
   'built-ins/Date/prototype/setUTCMonth/arg-coercion-order': [FAIL],
   'built-ins/Date/prototype/setUTCSeconds/arg-coercion-order': [FAIL],
 
-  # https://bugs.chromium.org/p/v8/issues/detail?id=12680
-  'language/expressions/logical-assignment/left-hand-side-private-reference-method-short-circuit-nullish': [FAIL],
-  'language/expressions/logical-assignment/left-hand-side-private-reference-method-short-circuit-or': [FAIL],
-  'language/expressions/logical-assignment/left-hand-side-private-reference-readonly-accessor-property-short-circuit-and': [FAIL],
-  'language/expressions/logical-assignment/left-hand-side-private-reference-readonly-accessor-property-short-circuit-nullish': [FAIL],
-  'language/expressions/logical-assignment/left-hand-side-private-reference-readonly-accessor-property-short-circuit-or': [FAIL],
-
   # https://bugs.chromium.org/p/v8/issues/detail?id=12044
   'built-ins/Array/prototype/Symbol.unscopables/value': [FAIL],