Bug 1642476 - Make deleting a private field an error r=jorendorff

Differential Revision: https://phabricator.services.mozilla.com/D77931

js/src/frontend/BytecodeEmitter.cpp

@@ -7985,6 +7985,7 @@ bool BytecodeEmitter::emitOptionalTree(
       // https://tc39.es/ecma262/#sec-primary-expression
       bool isPrimaryExpression =
           kind == ParseNodeKind::ThisExpr || kind == ParseNodeKind::Name ||
+          kind == ParseNodeKind::PrivateName ||
           kind == ParseNodeKind::NullExpr || kind == ParseNodeKind::TrueExpr ||
           kind == ParseNodeKind::FalseExpr ||
           kind == ParseNodeKind::NumberExpr ||

js/src/frontend/FullParseHandler.h

@@ -1037,6 +1037,20 @@ class FullParseHandler {
     return node->isKind(ParseNodeKind::PrivateName);
   }
 
+  bool isPrivateField(Node node) {
+    if (node->isKind(ParseNodeKind::ElemExpr) ||
+        node->isKind(ParseNodeKind::OptionalElemExpr)) {
+      PropertyByValueBase& pbv = node->as<PropertyByValueBase>();
+      if (isPrivateName(&pbv.key())) {
+        return true;
+      }
+    }
+    if (node->isKind(ParseNodeKind::OptionalChain)) {
+      return isPrivateField(node->as<UnaryNode>().kid());
+    }
+    return false;
+  }
+
   PropertyName* maybeDottedProperty(Node pn) {
     return pn->is<PropertyAccessBase>() ? &pn->as<PropertyAccessBase>().name()
                                         : nullptr;

js/src/frontend/Parser.cpp

@@ -9244,8 +9244,10 @@ typename ParseHandler::Node GeneralParser<ParseHandler, Unit>::unaryExpr(
         return null();
       }
 
-      // Per spec, deleting any unary expression is valid -- it simply
-      // returns true -- except for one case that is illegal in strict mode.
+      // Per spec, deleting most unary expressions is valid -- it simply
+      // returns true -- except for two cases:
+      // 1. `var x; ...; delete x` is a syntax error in strict mode.
+      // 2. Private fields cannot be deleted.
       if (handler_.isName(expr)) {
         if (!strictModeErrorAt(exprOffset, JSMSG_DEPRECATED_DELETE_OPERAND)) {
           return null();
@@ -9254,6 +9256,12 @@ typename ParseHandler::Node GeneralParser<ParseHandler, Unit>::unaryExpr(
         pc_->sc()->setBindingsAccessedDynamically();
       }
 
+      if (handler_.isPrivateField(expr)) {
+        // should always be in strict mode if we're talking private names.
+        MOZ_ALWAYS_FALSE(strictModeErrorAt(exprOffset, JSMSG_PRIVATE_DELETE));
+        return null();
+      }
+
       return handler_.newDelete(begin, expr);
     }
 

js/src/frontend/SyntaxParseHandler.h

@@ -97,6 +97,9 @@ class SyntaxParseHandler {
     NodeOptionalDottedProperty,
     NodeElement,
     NodeOptionalElement,
+    // A distinct node for [PrivateName], to make detecting delete this.#x
+    // detectable in syntax parse
+    NodePrivateElement,
 
     // Destructuring target patterns can't be parenthesized: |([a]) = [3];|
     // must be a syntax error.  (We can't use NodeGeneric instead of these
@@ -149,7 +152,8 @@ class SyntaxParseHandler {
   }
 
   bool isPropertyAccess(Node node) {
-    return node == NodeDottedProperty || node == NodeElement;
+    return node == NodeDottedProperty || node == NodeElement ||
+           node == NodePrivateElement;
   }
 
   bool isOptionalPropertyAccess(Node node) {
@@ -498,6 +502,9 @@ class SyntaxParseHandler {
   }
 
   PropertyByValueType newPropertyByValue(Node lhs, Node index, uint32_t end) {
+    if (isPrivateName(index)) {
+      return NodePrivateElement;
+    }
     return NodeElement;
   }
 
@@ -688,6 +695,7 @@ class SyntaxParseHandler {
   }
 
   bool isPrivateName(Node node) { return node == NodePrivateName; }
+  bool isPrivateField(Node node) { return node == NodePrivateElement; }
 
   PropertyName* maybeDottedProperty(Node node) {
     // Note: |super.apply(...)| is a special form that calls an "apply"

js/src/js.msg

@@ -275,6 +275,7 @@ MSG_DEF(JSMSG_GARBAGE_AFTER_INPUT,     2, JSEXN_SYNTAXERR, "unexpected garbage a
 MSG_DEF(JSMSG_IDSTART_AFTER_NUMBER,    0, JSEXN_SYNTAXERR, "identifier starts immediately after numeric literal")
 MSG_DEF(JSMSG_BAD_ESCAPE,              0, JSEXN_SYNTAXERR, "invalid escape sequence")
 MSG_DEF(JSMSG_MISSING_PRIVATE_NAME,    0, JSEXN_SYNTAXERR, "'#' not followed by identifier")
+MSG_DEF(JSMSG_PRIVATE_DELETE,          0, JSEXN_SYNTAXERR, "private fields can't be deleted")
 MSG_DEF(JSMSG_ILLEGAL_CHARACTER,       0, JSEXN_SYNTAXERR, "illegal character")
 MSG_DEF(JSMSG_IMPORT_META_OUTSIDE_MODULE, 0, JSEXN_SYNTAXERR, "import.meta may only appear in a module")
 MSG_DEF(JSMSG_IMPORT_DECL_AT_TOP_LEVEL, 0, JSEXN_SYNTAXERR, "import declarations may only appear at top level of a module")

js/src/tests/non262/PrivateName/illegal-delete.js

@@ -0,0 +1,94 @@
+// |reftest| skip-if(!xulRuntime.shell) shell-option(--enable-private-fields)
+
+class A {
+  #x = {a: 1};
+  b = null;
+  es(s) {
+    eval(s);
+  }
+}
+
+var a = new A;
+a.b = new A;
+
+assertThrowsInstanceOf(() => a.es('delete this.#x'), SyntaxError);
+assertThrowsInstanceOf(() => a.es('delete (this.#x)'), SyntaxError);
+assertThrowsInstanceOf(() => a.es('delete this?.#x'), SyntaxError);
+assertThrowsInstanceOf(() => a.es('delete this?.b.#x'), SyntaxError);
+// Should be OK
+a.es('delete (0, this.#x.a)')
+a.es('delete this?.b.#x.a')
+
+
+// Make sure the above works in a different context, with emphasis on
+// lazy/syntax parsing.
+function eval_and_report(str) {
+  var classTest = `
+  class B {
+    #x = {a: 1};
+    b = null;
+    test() {
+      ${str};
+    }
+  }
+  var b = new B;
+  b.b = new B;
+  b.test();
+ `;
+
+  var str = `
+  function f(run) {
+    if (run) {
+      ${classTest}
+    }
+  }
+  f(run)`;
+
+
+  var throws = [];
+  // Evalutate in a new global; has the advantage that it makes successes
+  // idempotent.
+  var g = newGlobal();
+  g.run = false;
+
+  try {
+    // The idea is that this is a full parse
+    evaluate(classTest, {global: g});
+  } catch (e) {
+    throws.push(e);
+  }
+
+  try {
+    // The idea is this is a lazy parse; however, fields currently
+    // disable lazy parsing, so right now
+    evaluate(str, {global: g});
+  } catch (e) {
+    throws.push(e);
+  }
+
+  return throws;
+}
+
+function assertSyntaxError(str) {
+  var exceptions = eval_and_report(str);
+  assertEq(exceptions.length, 2);
+  for (var e of exceptions) {
+    assertEq(/SyntaxError/.test(e.name), true);
+  }
+}
+
+function assertNoSyntaxError(str) {
+  var exceptions = eval_and_report(str);
+  assertEq(exceptions.length, 0);
+}
+
+assertSyntaxError('delete this.#x');
+assertSyntaxError('delete (this.#x)');
+assertSyntaxError('delete this?.#x');
+assertSyntaxError('delete this?.b.#x');
+// Should be OK
+assertNoSyntaxError('delete (0, this.#x.a)')
+assertNoSyntaxError('delete this?.b.#x.a')
+
+
+if (typeof reportCompare === 'function') reportCompare(0, 0);