Roll forward: Make Normalize pass add the IS_CONSTANT_NAME property t…

…o names of const variables. Before this change, Normalize did not set the IS_CONSTANT_NAME property to true on the name nodes tied to const declarations which are constant by nature. Adding this property allows the compiler to optimize constant variables and reduce code size. PiperOrigin-RevId: 542693869
google · Jun 22, 2023 · 2197fe8 · 2197fe8
1 parent 5f82d52
commit 2197fe8
Show file tree

Hide file tree

Showing 14 changed files with 182 additions and 54 deletions.
diff --git a/src/com/google/javascript/jscomp/AstFactory.java b/src/com/google/javascript/jscomp/AstFactory.java
@@ -44,7 +44,6 @@
 import com.google.javascript.rhino.jstype.TemplateTypeReplacer;
 import java.util.Arrays;
 import java.util.List;
-import java.util.Objects;
 import java.util.function.Supplier;
 import org.jspecify.nullness.Nullable;
 
@@ -450,9 +449,7 @@ private FunctionType getFunctionType(Node functionNode) {
    * classes but not generic functions annotated @this.
    */
   Node createThisAliasReferenceForEs6Class(String aliasName, Node functionNode) {
-    final Node result = IR.name(aliasName);
-    setJSTypeOrColor(getTypeOfThisForEs6Class(functionNode), result);
-    return result;
+    return createName(aliasName, getTypeOfThisForEs6Class(functionNode));
   }
 
   /**
@@ -506,7 +503,8 @@ Node createSingleVarNameDeclaration(String variableName, Node value) {
    * <p>e.g. `const variableName = value;`
    */
   Node createSingleConstNameDeclaration(String variableName, Node value) {
-    return IR.constNode(createName(variableName, type(value.getJSType(), value.getColor())), value);
+    Node nameNode = createConstantName(variableName, type(value.getJSType(), value.getColor()));
+    return IR.constNode(nameNode, value);
   }
 
   /**
@@ -563,14 +561,24 @@ Node createArgumentsAliasDeclaration(String aliasName) {
   Node createName(String name, Type type) {
     Node result = IR.name(name);
     setJSTypeOrColor(type, result);
-    if (lifeCycleStage.isNormalized() && Objects.equals(name, "$jscomp")) {
+    if (lifeCycleStage.isNormalized() && name.startsWith("$jscomp")) {
       // $jscomp will always be a constant and needs to be marked that way to satisfy
       // the normalization invariants.
       result.putBooleanProp(Node.IS_CONSTANT_NAME, true);
     }
     return result;
   }
 
+  /** Use this when you know you need to create a constant name for const declarations */
+  Node createConstantName(String name, Type type) {
+    Node result = IR.name(name);
+    setJSTypeOrColor(type, result);
+    if (lifeCycleStage.isNormalized()) {
+      result.putBooleanProp(Node.IS_CONSTANT_NAME, true);
+    }
+    return result;
+  }
+
   Node createName(@Nullable StaticScope scope, String name) {
     final Node result = IR.name(name);
 

diff --git a/src/com/google/javascript/jscomp/CrossChunkMethodMotion.java b/src/com/google/javascript/jscomp/CrossChunkMethodMotion.java
@@ -473,17 +473,17 @@ private void tryToMoveMemberFunction(
       }
 
       Node destinationParent = compiler.getNodeForCodeInsertion(deepestCommonModuleRef);
-      String className = rootVar.getName();
+      Node classNameNode = rootVar.getNameNode();
       if (noStubFunctions) {
-        moveClassInstanceMethodWithoutStub(className, definitionNode, destinationParent);
+        moveClassInstanceMethodWithoutStub(classNameNode, definitionNode, destinationParent);
       } else {
-        moveClassInstanceMethodWithStub(className, definitionNode, destinationParent);
+        moveClassInstanceMethodWithStub(classNameNode, definitionNode, destinationParent);
       }
     }
   }
 
   private void moveClassInstanceMethodWithoutStub(
-      String className, Node methodDefinition, Node destinationParent) {
+      Node classNameNode, Node methodDefinition, Node destinationParent) {
     checkArgument(methodDefinition.isMemberFunctionDef(), methodDefinition);
     Node classMembers = checkNotNull(methodDefinition.getParent());
     checkState(classMembers.isClassMembers(), classMembers);
@@ -496,9 +496,10 @@ private void moveClassInstanceMethodWithoutStub(
 
     // ClassName.prototype.propertyName = function() {};
     Node functionNode = checkNotNull(methodDefinition.getOnlyChild());
+    Node clonedNameNode = classNameNode.cloneNode().copyTypeFrom(classNode);
     Node classNameDotPrototypeDotPropName =
         astFactory.createGetProp(
-            astFactory.createPrototypeAccess(astFactory.createName(className, type(classNode))),
+            astFactory.createPrototypeAccess(clonedNameNode),
             methodDefinition.getString(),
             type(functionNode));
     functionNode.detach();
@@ -513,7 +514,7 @@ private void moveClassInstanceMethodWithoutStub(
   }
 
   private void moveClassInstanceMethodWithStub(
-      String className, Node methodDefinition, Node destinationParent) {
+      Node classNameNode, Node methodDefinition, Node destinationParent) {
     checkArgument(methodDefinition.isMemberFunctionDef(), methodDefinition);
     Node classMembers = checkNotNull(methodDefinition.getParent());
     checkState(classMembers.isClassMembers(), classMembers);
@@ -524,9 +525,10 @@ private void moveClassInstanceMethodWithStub(
 
     // Put a stub definition after the class
     // ClassName.prototype.propertyName = JSCompiler_stubMethod(id);
+    Node clonedNameNode = classNameNode.cloneNode().copyTypeFrom(classNode);
     Node classNameDotPrototypeDotPropName =
         astFactory.createGetProp(
-            astFactory.createPrototypeAccess(astFactory.createName(className, type(classNode))),
+            astFactory.createPrototypeAccess(clonedNameNode),
             methodDefinition.getString(),
             type(methodDefinition));
     Node stubCall = createStubCall(methodDefinition, stubId);

diff --git a/src/com/google/javascript/jscomp/Es6ExtractClasses.java b/src/com/google/javascript/jscomp/Es6ExtractClasses.java
@@ -249,7 +249,7 @@ private void extractClass(NodeTraversal t, Node classNode) {
 
     Node statement = NodeUtil.getEnclosingStatement(parent);
     // class name node used as LHS in newly created assignment
-    Node classNameLhs = astFactory.createName(name, type(classNode));
+    Node classNameLhs = astFactory.createConstantName(name, type(classNode));
     // class name node that replaces the class literal in the original statement
     Node classNameRhs = classNameLhs.cloneTree();
     classNode.replaceWith(classNameRhs);

diff --git a/src/com/google/javascript/jscomp/Es6RewriteClassExtendsExpressions.java b/src/com/google/javascript/jscomp/Es6RewriteClassExtendsExpressions.java
@@ -137,8 +137,9 @@ private void extractExtends(NodeTraversal t, Node classNode) {
 
     Node statement = NodeUtil.getEnclosingStatement(classNode);
     Node originalExtends = classNode.getSecondChild();
-    originalExtends.replaceWith(
-        astFactory.createName(name, type(originalExtends)).srcref(originalExtends));
+    Node nameNode =
+        astFactory.createConstantName(name, type(originalExtends)).srcref(originalExtends);
+    originalExtends.replaceWith(nameNode);
     Node extendsAlias =
         astFactory
             .createSingleConstNameDeclaration(name, originalExtends)

diff --git a/src/com/google/javascript/jscomp/Es6RewriteDestructuring.java b/src/com/google/javascript/jscomp/Es6RewriteDestructuring.java
@@ -320,11 +320,7 @@ private Node replacePatternParamWithTempVar(
    * (the compiler only assigns $jscomp$destructuring$var[num] once)
    */
   private Node createTempVarNameNode(String name, AstFactory.Type type) {
-    Node nameNode = astFactory.createName(name, type);
-    if (compiler.getLifeCycleStage().isNormalized()) {
-      nameNode.putBooleanProp(Node.IS_CONSTANT_NAME, true);
-    }
-    return nameNode;
+    return astFactory.createConstantName(name, type);
   }
 
   /** Creates a new unique name to use for a pattern we need to rewrite. */
@@ -542,6 +538,9 @@ private void replaceObjectPattern(
 
       Node newNode;
       if (NodeUtil.isNameDeclaration(parent)) {
+        if (parent.isConst()) {
+          newLHS.putBooleanProp(Node.IS_CONSTANT_NAME, true);
+        }
         newNode = IR.declaration(newLHS, newRHS, parent.getToken());
       } else if (parent.isAssign()) {
         newNode = IR.exprResult(astFactory.createAssign(newLHS, newRHS));

diff --git a/src/com/google/javascript/jscomp/InlineAndCollapseProperties.java b/src/com/google/javascript/jscomp/InlineAndCollapseProperties.java
@@ -34,7 +34,7 @@
 import com.google.javascript.jscomp.GlobalNamespace.Ref;
 import com.google.javascript.jscomp.NodeTraversal.AbstractPostOrderCallback;
 import com.google.javascript.jscomp.NodeTraversal.ExternsSkippingCallback;
-import com.google.javascript.jscomp.Normalize.PropagateConstantAnnotationsOverVars;
+import com.google.javascript.jscomp.Normalize.PropagateConstantPropertyOverVars;
 import com.google.javascript.jscomp.base.format.SimpleFormat;
 import com.google.javascript.jscomp.deps.ModuleLoader.ResolutionMode;
 import com.google.javascript.jscomp.diagnostic.LogFile;
@@ -1301,7 +1301,7 @@ public void process(Node externs, Node root) {
       // This shouldn't be necessary, this pass should already be setting new constants as
       // constant.
       // TODO(b/64256754): Investigate.
-      new PropagateConstantAnnotationsOverVars(compiler, false).process(externs, root);
+      new PropagateConstantPropertyOverVars(compiler, false).process(externs, root);
     }
 
     private boolean canCollapse(Name name) {

diff --git a/src/com/google/javascript/jscomp/MakeDeclaredNamesUnique.java b/src/com/google/javascript/jscomp/MakeDeclaredNamesUnique.java
@@ -180,7 +180,7 @@ private void visitNameOrImportStar(NodeTraversal t, Node n, Node parent) {
     if (markChanges) {
       t.reportCodeChange();
       // If we are renaming a function declaration, make sure the containing scope
-      // has the opporunity to act on the change.
+      // has the opportunity to act on the change.
       if (parent.isFunction() && NodeUtil.isFunctionDeclaration(parent)) {
         t.getCompiler().reportChangeToEnclosingScope(parent);
       }
@@ -479,9 +479,7 @@ public Renamer createForChildScope(Node scopeRoot, boolean hoistingTargetScope)
       return new ContextualRenamer(scopeRoot, nameUsage, hoistingTargetScope, this);
     }
 
-    /**
-     * Adds a name to the map of names declared in this scope.
-     */
+    /** Adds a name to the map of names declared in this scope. */
     @Override
     public void addDeclaredName(String name, boolean hoisted) {
       if (hoisted && hoistRenamer != this) {

diff --git a/src/com/google/javascript/jscomp/Normalize.java b/src/com/google/javascript/jscomp/Normalize.java
@@ -159,20 +159,20 @@ public void process(Node externs, Node root) {
 
     removeDuplicateDeclarations(externs, root);
 
-    new PropagateConstantAnnotationsOverVars(compiler, assertOnChange).process(externs, root);
+    new PropagateConstantPropertyOverVars(compiler, assertOnChange).process(externs, root);
 
     if (!compiler.getLifeCycleStage().isNormalized()) {
       compiler.setLifeCycleStage(LifeCycleStage.NORMALIZED);
     }
   }
 
-  /** Propagate constant annotations over the Var graph. */
-  static class PropagateConstantAnnotationsOverVars extends AbstractPostOrderCallback
+  /** Propagate constant annotations and IS_CONSTANT_NAME property over the Var graph. */
+  static class PropagateConstantPropertyOverVars extends AbstractPostOrderCallback
       implements CompilerPass {
     private final AbstractCompiler compiler;
     private final boolean assertOnChange;
 
-    PropagateConstantAnnotationsOverVars(AbstractCompiler compiler, boolean forbidChanges) {
+    PropagateConstantPropertyOverVars(AbstractCompiler compiler, boolean forbidChanges) {
       this.compiler = compiler;
       this.assertOnChange = forbidChanges;
     }
@@ -194,7 +194,8 @@ public void visit(NodeTraversal t, Node n, Node parent) {
       JSDocInfo info = (var != null) ? var.getJSDocInfo() : null;
 
       boolean shouldBeConstant =
-          (info != null && info.isConstant())
+          (var != null && var.isConst())
+              || (info != null && info.isConstant())
               || NodeUtil.isConstantByConvention(compiler.getCodingConvention(), n);
       boolean isMarkedConstant = n.getBooleanProp(Node.IS_CONSTANT_NAME);
       if (shouldBeConstant && !isMarkedConstant) {

diff --git a/src/com/google/javascript/jscomp/PureFunctionIdentifier.java b/src/com/google/javascript/jscomp/PureFunctionIdentifier.java
@@ -843,7 +843,7 @@ private void updateSideEffectsForNode(
           break;
 
         case DYNAMIC_IMPORT:
-          // Modules may be imported for side-effecfts only. This is frequently
+          // Modules may be imported for side-effects only. This is frequently
           // a pattern used to load polyfills.
           encloserSummary.setMutatesGlobalStateAndAllOtherFlags();
           break;

diff --git a/src/com/google/javascript/jscomp/RewriteAsyncFunctions.java b/src/com/google/javascript/jscomp/RewriteAsyncFunctions.java
@@ -351,6 +351,9 @@ public void visit(NodeTraversal t, Node n) {
           case NAME:
             if (n.matchesName("arguments")) {
               n.setString(ASYNC_ARGUMENTS);
+              if (compiler.getLifeCycleStage().isNormalized()) {
+                n.putBooleanProp(Node.IS_CONSTANT_NAME, true);
+              }
               asyncThisAndArgumentsContext.recordAsyncArgumentsReplacementWasDone();
               compiler.reportChangeToChangeScope(contextRootNode);
             }

diff --git a/test/com/google/javascript/jscomp/InlineVariablesTest.java b/test/com/google/javascript/jscomp/InlineVariablesTest.java
@@ -2199,7 +2199,7 @@ public void testLetConst() {
         lines(
             "", //
             "function f(x) {",
-            "  x; const g = 2;",
+            "  x;",
             "  {3;}",
             "}"));
   }

diff --git a/test/com/google/javascript/jscomp/MakeDeclaredNamesUniqueTest.java b/test/com/google/javascript/jscomp/MakeDeclaredNamesUniqueTest.java
@@ -83,10 +83,8 @@ protected int getNumRepetitions() {
     return 1;
   }
 
-  @Override
   @Before
-  public void setUp() throws Exception {
-    super.setUp();
+  public void customSetUp() throws Exception {
     removeConst = false;
     invert = false;
     useDefaultRenamer = false;
@@ -441,17 +439,23 @@ public void testMakeLocalNamesUniqueWithoutContext() {
     testSame("a;");
 
     // Local names are made unique.
-    test("var a;"
-         + "function foo(a){var b;a}",
-         "var a$jscomp$unique_0;"
-         + "function foo$jscomp$unique_1(a$jscomp$unique_2){"
-         + "  var b$jscomp$unique_3;a$jscomp$unique_2}");
-    test("var a;"
-         + "function foo(){var b;a}"
-         + "function boo(){var b;a}",
-         "var a$jscomp$unique_0;" +
-         "function foo$jscomp$unique_1(){var b$jscomp$unique_3;a$jscomp$unique_0}"
-         + "function boo$jscomp$unique_2(){var b$jscomp$unique_4;a$jscomp$unique_0}");
+    test(
+        lines(
+            "var a;", //
+            "function foo(a){var b;a}"),
+        lines(
+            "var a$jscomp$unique_0;",
+            "function foo$jscomp$unique_1(a$jscomp$unique_2){",
+            "  var b$jscomp$unique_3;a$jscomp$unique_2}"));
+    test(
+        lines(
+            "var a;", //
+            "function foo(){var b;a}",
+            "function boo(){var b;a}"),
+        lines(
+            "var a$jscomp$unique_0;",
+            "function foo$jscomp$unique_1(){var b$jscomp$unique_3;a$jscomp$unique_0}",
+            "function boo$jscomp$unique_2(){var b$jscomp$unique_4;a$jscomp$unique_0}"));
 
     test(
         "let a; function foo(a) {let b; a; }",