RemoveUnusedCode: support optional chaining

PiperOrigin-RevId: 326108121

Author: brad4d

src/com/google/javascript/jscomp/NodeUtil.java

@@ -1930,6 +1930,11 @@ public static boolean isNormalOrOptChainGet(Node n) {
     return isNormalGet(n) || isOptChainGet(n);
   }
 
+  /** Is this a GETPROP or OPTCHAIN_GETPROP? */
+  public static boolean isNormalOrOptChainGetProp(Node n) {
+    return n.isGetProp() || n.isOptChainGetProp();
+  }
+
   /** Is this a GETPROP or GETELEM node? */
   public static boolean isNormalGet(Node n) {
     return n.isGetProp() || n.isGetElem();

src/com/google/javascript/jscomp/RemoveUnusedCode.java

@@ -469,6 +469,7 @@ private void traverseNode(Node n, Scope scope) {
         break;
 
       case CALL:
+      case OPTCHAIN_CALL:
         traverseCall(n, scope);
         break;
 
@@ -546,7 +547,8 @@ private void traverseNode(Node n, Scope scope) {
         break;
 
       case GETPROP:
-        traverseGetProp(n, scope);
+      case OPTCHAIN_GETPROP:
+        traverseNormalOrOptChainGetProp(n, scope);
         break;
 
       default:
@@ -569,7 +571,17 @@ private void traverseInstanceof(Node instanceofNode, Scope scope) {
     }
   }
 
-  private void traverseGetProp(Node getProp, Scope scope) {
+  /**
+   * Traverse `expr.prop` or `expr?.prop`.
+   *
+   * <p>Note that this method is called only for RHS nodes. Property references that are being
+   * assigned to are handled by the logic traversing their parent (e.g. ASSIGN) node.
+   *
+   * <p>The primary purpose of this method is to make sure the property reference is correctly
+   * recorded.
+   */
+  private void traverseNormalOrOptChainGetProp(Node getProp, Scope scope) {
+    checkState(NodeUtil.isNormalOrOptChainGetProp(getProp), getProp);
     Node objectNode = getProp.getFirstChild();
     Node propertyNameNode = objectNode.getNext();
     String propertyName = propertyNameNode.getString();
@@ -586,7 +598,10 @@ private void traverseGetProp(Node getProp, Scope scope) {
       markPropertyNameAsPinned(propertyName);
       traverseNode(objectNode, scope);
     } else if (objectNode.isThis()) {
+      // This is probably the declaration of a class field in a constructor.
+      // /** @private {number} */
       // this.propName;
+      // We don't want to consider this a real usage that should prevent removal.
       RemovableBuilder builder = new RemovableBuilder().setIsThisDotPropertyReference(true);
       considerForIndependentRemoval(builder.buildUnusedReadReference(getProp, propertyNameNode));
     } else if (isDotPrototype(objectNode)) {
@@ -909,7 +924,8 @@ private boolean isAssignmentToPrototype(Node n) {
 
   /** True for `someExpression.prototype`. */
   private static boolean isDotPrototype(Node n) {
-    return n.isGetProp() && n.getLastChild().getString().equals("prototype");
+    return NodeUtil.isNormalOrOptChainGetProp(n)
+        && n.getLastChild().getString().equals("prototype");
   }
 
   private void traverseCatch(Node catchNode, Scope scope) {
@@ -1463,8 +1479,8 @@ private void considerForIndependentRemoval(Removable removable) {
 
   /** @return Whether or not accessor side-effect are a possibility. */
   private boolean considerForAccessorSideEffects(Node getprop, PropertyAccessKind usage) {
-    checkState(getprop.isGetProp(), getprop); // Other node types may make sense in the future.
-
+    // Other node types may make sense in the future.
+    checkState(NodeUtil.isNormalOrOptChainGetProp(getprop), getprop);
     String propName = getprop.getSecondChild().getString();
     PropertyAccessKind recorded = compiler.getAccessorSummary().getKind(propName);
     if ((recorded.hasGetter() && usage.hasGetter() && !assumeGettersArePure)
@@ -2072,12 +2088,12 @@ public String toString() {
 
   /** True for `this.propertyName` */
   private static boolean isThisDotProperty(Node n) {
-    return n.isGetProp() && n.getFirstChild().isThis();
+    return NodeUtil.isNormalOrOptChainGetProp(n) && n.getFirstChild().isThis();
   }
 
   /** True for `(something).prototype.propertyName` */
   private static boolean isDotPrototypeDotProperty(Node n) {
-    return n.isGetProp() && isDotPrototype(n.getFirstChild());
+    return NodeUtil.isNormalOrOptChainGetProp(n) && isDotPrototype(n.getFirstChild());
   }
 
   private class IndirectAssign extends Removable {
@@ -2854,7 +2870,7 @@ void considerPossibleReferenceInternal(Node possiblyReferencingNode) {
         // A matching NAME node must be a reference (there's no need to check that the referenced
         // Var is global, since local variables have all been renamed by normalization).
         isRemovable = false;
-      } else if (possiblyReferencingNode.isGetProp()) {
+      } else if (NodeUtil.isNormalOrOptChainGetProp(possiblyReferencingNode)) {
         // Assume that the owner is possibly the global `this` and skip removal.
         isRemovable = false;
       }
@@ -2877,10 +2893,9 @@ String getName() {
 
     @Override
     void considerPossibleReferenceInternal(Node possiblyReferencingNode) {
-      if (!possiblyReferencingNode.isGetProp()) {
-        return;
+      if (NodeUtil.isNormalOrOptChainGetProp(possiblyReferencingNode)) {
+        isRemovable = false;
       }
-      isRemovable = false;
     }
   }
 
@@ -2900,11 +2915,10 @@ String getName() {
 
     @Override
     void considerPossibleReferenceInternal(Node possiblyReferencingNode) {
-      if (!possiblyReferencingNode.isGetProp()) {
-        return;
+      if (NodeUtil.isNormalOrOptChainGetProp(possiblyReferencingNode)) {
+        // Prototype properties are simply not removable.
+        isRemovable = false;
       }
-      // Prototype properties are simply not removable.
-      isRemovable = false;
     }
   }
 

test/com/google/javascript/jscomp/AstChainOptionalizer.java

@@ -138,10 +138,17 @@ public void visit(NodeTraversal t, Node n, Node parent) {
   /** Special function qualified names that really should never be called with ?. */
   private static final ImmutableList<String> SPECIAL_CALLEE_QNAMES =
       ImmutableList.of(
+          "Array.from",
           "Object.defineProperty",
           "Object.defineProperties",
+          "goog.addSingletonGetter",
           "goog.inherits",
-          "goog.reflect.cache");
+          "goog.mixin",
+          "goog$mixin",
+          "goog.reflect.cache",
+          "goog.reflect.objectProperty",
+          "Math.random",
+          "$jscomp.polyfill");
 
   /**
    * Is the callee a special function name that we would never expect to be called using optional

test/com/google/javascript/jscomp/RemoveUnusedCodeClassPropertiesTest.java

@@ -16,6 +16,7 @@
 
 package com.google.javascript.jscomp;
 
+import com.google.javascript.jscomp.CompilerOptions.LanguageMode;
 import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -96,6 +97,8 @@ protected CompilerPass getProcessor(Compiler compiler) {
   @Before
   public void setUp() throws Exception {
     super.setUp();
+    // Allow testing of features that aren't fully supported for output yet.
+    setAcceptedLanguage(LanguageMode.ECMASCRIPT_NEXT_IN);
     enableNormalize();
     enableGatherExternProperties();
     onlyValidateNoNewGettersAndSetters();
@@ -213,6 +216,10 @@ public void testDestructuringRest() {
   public void testExprResult() {
     test("this.x", "");
     test("externFunction().prototype.x", "externFunction()");
+    // It doesn't make much sense to use optional chaining in these cases, but if you do,
+    // it shouldn't prevent unused property removal
+    test("this?.x", "");
+    test("externFunction()?.prototype.x", "externFunction()");
   }
 
   @Test

test/com/google/javascript/jscomp/RemoveUnusedCodeNameAnalyzerTest.java

@@ -107,7 +107,8 @@ public void process(Node externs, Node root) {
   @Before
   public void setUp() throws Exception {
     super.setUp();
-    setAcceptedLanguage(LanguageMode.ECMASCRIPT_2017);
+    // Allow testing of features that aren't fully supported for output yet.
+    setAcceptedLanguage(LanguageMode.ECMASCRIPT_NEXT_IN);
     enableNormalize();
     enableGatherExternProperties();
     onlyValidateNoNewGettersAndSetters();

test/com/google/javascript/jscomp/RemoveUnusedCodePrototypePropertiesTest.java

@@ -16,6 +16,7 @@
 
 package com.google.javascript.jscomp;
 
+import com.google.javascript.jscomp.CompilerOptions.LanguageMode;
 import com.google.javascript.rhino.Node;
 import org.junit.Before;
 import org.junit.Test;
@@ -85,6 +86,8 @@ public CompilerOptions getOptions() {
   @Before
   public void setUp() throws Exception {
     super.setUp();
+    // Allow testing of features that aren't fully supported for output yet.
+    setAcceptedLanguage(LanguageMode.ECMASCRIPT_NEXT_IN);
     enableNormalize();
     enableGatherExternProperties();
     onlyValidateNoNewGettersAndSetters();
@@ -818,6 +821,39 @@ public void testDestructuringRest() {
             "({ ...new Foo().a.b } = 0);"));
   }
 
+  @Test
+  public void testOptionalGetPropPreventsRemoval() {
+    test(
+        lines(
+            "class C {",
+            "  constructor() {",
+            "    this.x = 1;",
+            "  }",
+            "  optGetPropRef() {}",
+            "  optCallRef() {}",
+            "  unreferenced() {}",
+            "}",
+            "var c = new C;",
+            "c?.optGetPropRef()",
+            "c.optCallRef?.()",
+            // no call to unreferenced()
+            ""),
+        lines(
+            "class C {",
+            "  constructor() {",
+            "    this.x = 1;",
+            "  }",
+            "  optGetPropRef() {}", // kept
+            "  optCallRef() {}", // kept
+            // unreferenced() removed
+            "}",
+            "var c = new C;",
+            "c?.optGetPropRef()",
+            "c.optCallRef?.()",
+            // no call to unreferenced()
+            ""));
+  }
+
   @Test
   public void testEs6Class() {
     testSame(