From 2197fe8fab45052894bbf23e9816faa1b83d4aa5 Mon Sep 17 00:00:00 2001 From: Closure Team Date: Thu, 22 Jun 2023 16:06:19 -0700 Subject: [PATCH] Roll forward: Make Normalize pass add the IS_CONSTANT_NAME property to 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/javascript/jscomp/AstFactory.java | 20 ++- .../jscomp/CrossChunkMethodMotion.java | 16 ++- .../javascript/jscomp/Es6ExtractClasses.java | 2 +- .../Es6RewriteClassExtendsExpressions.java | 5 +- .../jscomp/Es6RewriteDestructuring.java | 9 +- .../jscomp/InlineAndCollapseProperties.java | 4 +- .../jscomp/MakeDeclaredNamesUnique.java | 6 +- .../google/javascript/jscomp/Normalize.java | 11 +- .../jscomp/PureFunctionIdentifier.java | 2 +- .../jscomp/RewriteAsyncFunctions.java | 3 + .../jscomp/InlineVariablesTest.java | 2 +- .../jscomp/MakeDeclaredNamesUniqueTest.java | 32 +++-- .../javascript/jscomp/NormalizeTest.java | 122 +++++++++++++++++- .../jscomp/PureFunctionIdentifierTest.java | 2 +- 14 files changed, 182 insertions(+), 54 deletions(-) diff --git a/src/com/google/javascript/jscomp/AstFactory.java b/src/com/google/javascript/jscomp/AstFactory.java index 42c4f620f78..25c02b3f5b6 100644 --- 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) { *

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,7 +561,7 @@ 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); @@ -571,6 +569,16 @@ Node createName(String name, Type type) { 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 index 785856ad918..270361abcaa 100644 --- 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 index e87ad37abfb..376ec4592e8 100644 --- 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 index 72e781d27ca..24cbb786b3f 100644 --- 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 index f3a633baff1..cb27765b483 100644 --- 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 index 937ca8d74ed..d5d836015f2 100644 --- 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 index 93e8337620c..83573ab2153 100644 --- 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 index 222affacef5..50fce07eca3 100644 --- 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 index c204c0dc8bd..8fbf3e251c3 100644 --- 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 index 577d5584e3d..c1a664c790b 100644 --- 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 index 1a535a50418..30ac69f101a 100644 --- 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 index 822324e00d4..8062e2f0b92 100644 --- 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; }", diff --git a/test/com/google/javascript/jscomp/NormalizeTest.java b/test/com/google/javascript/jscomp/NormalizeTest.java index 0e3b310c2c3..271d4cb6e46 100644 --- a/test/com/google/javascript/jscomp/NormalizeTest.java +++ b/test/com/google/javascript/jscomp/NormalizeTest.java @@ -28,6 +28,7 @@ import java.util.HashSet; import java.util.Set; import java.util.function.Predicate; +import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; @@ -62,15 +63,125 @@ protected int getNumRepetitions() { return 1; } - @Override - public void setUp() throws Exception { - super.setUp(); + @Before + public void customSetUp() throws Exception { // Validate that Normalize copies colors onto any nodes it synthesizes enableTypeInfoValidation(); enableTypeCheck(); replaceTypesWithColors(); } + @Test + public void testConstAnnotationPropagation() { + test( + "const x = 3; var a, b; var y = x + 2;", // + "const x = 3; var a; var b; var y = x + 2;"); + Node root = getLastCompiler().getRoot(); + Node scriptNode = + root.getLastChild() // ROOT of input sources + .getLastChild(); + + // `const x = 3;` + Node constNode = scriptNode.getFirstChild(); + + // `x` + Node constName = constNode.getOnlyChild(); + assertThat(constName.getBooleanProp(Node.IS_CONSTANT_NAME)).isTrue(); + + // `var y = x + 2;` + Node lastStatement = scriptNode.getLastChild(); + // `y = x + 2` + Node yVar = lastStatement.getOnlyChild(); + Node secondNameNodeOfX = + yVar.getFirstChild() // `x + 2` + .getFirstChild(); + + assertThat(secondNameNodeOfX.getBooleanProp(Node.IS_CONSTANT_NAME)).isTrue(); + } + + @Test + public void testRestConstAnnotationPropagation() { + testSame( + lines( + "const {...x} = {a: 3};", // + "var y = x;")); + Node root = getLastCompiler().getRoot(); + Node scriptNode = + root.getLastChild() // ROOT of input sources + .getLastChild(); + + // `const {...x} = {a: 3};` + Node constNode = scriptNode.getFirstChild(); + + // `{...x}` + Node objectPattern = + constNode + .getFirstChild() // DESTRUCTURING_LHS + .getFirstChild(); + + // `x` + Node constName = + objectPattern + .getFirstChild() // OBJECT_REST + .getFirstChild(); + assertThat(constName.getBooleanProp(Node.IS_CONSTANT_NAME)).isTrue(); + + // `var y = x` + Node lastStatement = scriptNode.getLastChild(); + // `y = x` + Node yVar = lastStatement.getOnlyChild(); + Node secondNameNodeOfX = yVar.getFirstChild(); + assertThat(secondNameNodeOfX.getBooleanProp(Node.IS_CONSTANT_NAME)).isTrue(); + } + + @Test + public void testRestConstAnnotationPropagation_onlyConstVars() { + testSame( + lines( + "const obj = {a: 3, b: 'string', c: null};", + "const {...rest} = obj;", + "const y = rest;")); + Node root = getLastCompiler().getRoot(); + Node scriptNode = + root.getLastChild() // ROOT of input sources + .getLastChild(); + Node firstStatement = scriptNode.getFirstChild(); + + // `obj` from `const obj = {a: 3, b: 'string', c: null}` + Node objName = firstStatement.getFirstChild(); + assertThat(objName.getBooleanProp(Node.IS_CONSTANT_NAME)).isTrue(); + + // `const {...rest} = obj` + Node constNode = firstStatement.getNext(); + + // `{...rest} = obj + Node destructuringNode = constNode.getFirstChild(); + + // `obj` from previous comment + Node secondObjName = destructuringNode.getLastChild(); + assertThat(secondObjName.getBooleanProp(Node.IS_CONSTANT_NAME)).isTrue(); + + // `{...rest} + Node objectPattern = destructuringNode.getFirstChild(); + + // `rest` + Node constName = + objectPattern + .getFirstChild() // OBJECT_REST + .getFirstChild(); + assertThat(constName.getBooleanProp(Node.IS_CONSTANT_NAME)).isTrue(); + + // `const y = rest` + Node secondConstNode = constNode.getNext(); + + // `y = rest` + Node yVar = secondConstNode.getOnlyChild(); + assertThat(yVar.getBooleanProp(Node.IS_CONSTANT_NAME)).isTrue(); + + Node secondConstName = yVar.getFirstChild(); + assertThat(secondConstName.getBooleanProp(Node.IS_CONSTANT_NAME)).isTrue(); + } + @Test public void testNullishCoalesce() { test("var a = x ?? y, b = foo()", "var a = x ?? y; var b = foo()"); @@ -982,11 +1093,12 @@ public void testIsConstant() { @Test public void testIsConstantByDestructuring() { test( - "const {CONST} = {CONST:3}; const b = CONST;", - "const {CONST: CONST} = {CONST:3}; const b = CONST;"); + "const {CONST} = {CONST:3}; let b = CONST;", + "const {CONST: CONST} = {CONST:3}; let b = CONST;"); Node n = getLastCompiler().getRoot(); Set constantNodes = findNodesWithProperty(n, IS_CONSTANT_NAME); + // There are 3 name nodes which should all be marked with IS_CONSTANT_NAME assertThat(constantNodes).hasSize(2); for (Node hasProp : constantNodes) { assertThat(hasProp.getString()).isEqualTo("CONST"); diff --git a/test/com/google/javascript/jscomp/PureFunctionIdentifierTest.java b/test/com/google/javascript/jscomp/PureFunctionIdentifierTest.java index 7ef859f05a6..84a55b74fcc 100644 --- a/test/com/google/javascript/jscomp/PureFunctionIdentifierTest.java +++ b/test/com/google/javascript/jscomp/PureFunctionIdentifierTest.java @@ -2815,7 +2815,7 @@ public void testIterableIteration_arrayPatternRest_makesFunctionImpure() { assertNoPureCalls( lines( "function foo(a) { const [...x] = a; }", // - "foo(x);")); + "foo(v);")); } @Test