diff --git a/src/com/google/javascript/jscomp/ExpressionDecomposer.java b/src/com/google/javascript/jscomp/ExpressionDecomposer.java index b16322e815f..f64d76d399d 100644 --- a/src/com/google/javascript/jscomp/ExpressionDecomposer.java +++ b/src/com/google/javascript/jscomp/ExpressionDecomposer.java @@ -699,16 +699,17 @@ private static boolean isConditionalOp(Node n) { } /** - * @return The statement containing the expression or null if the subExpression - * is not contain in a Node where inlining is known to be possible. - * For example, a WHILE node condition expression. + * Finds the statement containing {@code subExpression}. + * + *

If {@code subExpression} is not contained by a statement where inlining is known to be + * possible, {@code null} is returned. For example, the condition expression of a WHILE loop. */ @Nullable - static Node findExpressionRoot(Node subExpression) { + private static Node findExpressionRoot(Node subExpression) { Node child = subExpression; - for (Node parent : child.getAncestors()) { - Token parentType = parent.getToken(); - switch (parentType) { + for (Node current : child.getAncestors()) { + Node parent = current.getParent(); + switch (current.getToken()) { // Supported expression roots: // SWITCH and IF can have multiple children, but the CASE, DEFAULT, // or BLOCK will be encountered first for any of the children other @@ -718,26 +719,30 @@ static Node findExpressionRoot(Node subExpression) { case SWITCH: case RETURN: case THROW: - Preconditions.checkState(child == parent.getFirstChild()); - return parent; + Preconditions.checkState(child.isFirstChildOf(current)); + return current; + case VAR: - case CONST: + // Normalization will remove LABELs from VARs. case LET: - Preconditions.checkState(child == parent.getFirstChild()); - if (parent.getParent().isVanillaFor() - && parent == parent.getParent().getFirstChild()) { - return parent.getParent(); - } else { - return parent; + case CONST: + if (NodeUtil.isAnyFor(parent)) { + break; // Name declarations may not be roots if they're for-loop initializers. } + return current; + // Any of these indicate an unsupported expression: case FOR: - if (child == parent.getFirstChild()) { - return parent; + if (child.isFirstChildOf(current)) { + // Only the initializer of a for-loop could possibly be decomposed since the other + // statements need to execute each iteration. + return current; } // fall through case FOR_IN: case FOR_OF: + case DO: + case WHILE: case SCRIPT: case BLOCK: case LABEL: @@ -745,10 +750,11 @@ static Node findExpressionRoot(Node subExpression) { case DEFAULT_CASE: case PARAM_LIST: return null; + default: break; } - child = parent; + child = current; } throw new IllegalStateException("Unexpected AST structure."); @@ -839,6 +845,15 @@ private DecompositionType isSubexpressionMovable(Node expressionRoot, Node subEx Node child = subExpression; for (Node parent : child.getAncestors()) { + if (NodeUtil.isNameDeclaration(parent) && !child.isFirstChildOf(parent)) { + // Case: `let x = 5, y = 2 * x;` where `child = y`. + // Compound declarations cannot generally be decomposed. Later declarations might reference + // earlier ones and if it were possible to separate them, `Normalize` would already have + // done so. Therefore, we only support decomposing the first declaration. + // TODO(b/121157467): FOR initializers are probably the only source of these cases. + return DecompositionType.UNDECOMPOSABLE; + } + if (parent == expressionRoot) { // Done. The walk back to the root of the expression is complete, and // nothing was encountered that blocks the call from being moved. diff --git a/src/com/google/javascript/jscomp/FunctionInjector.java b/src/com/google/javascript/jscomp/FunctionInjector.java index 8a20ea0875e..c4233b1a983 100644 --- a/src/com/google/javascript/jscomp/FunctionInjector.java +++ b/src/com/google/javascript/jscomp/FunctionInjector.java @@ -25,7 +25,6 @@ import com.google.common.base.Supplier; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; -import com.google.javascript.jscomp.ExpressionDecomposer.DecompositionType; import com.google.javascript.rhino.Node; import com.google.javascript.rhino.Token; import com.google.javascript.rhino.jstype.JSType; @@ -467,17 +466,14 @@ private CallSiteType classifyCallSite(Reference ref) { // left-hand-side of the assignments and handling them as EXPRESSION? return CallSiteType.VAR_DECL_SIMPLE_ASSIGNMENT; } else { - Node expressionRoot = ExpressionDecomposer.findExpressionRoot(callNode); - if (expressionRoot != null) { - ExpressionDecomposer decomposer = getDecomposer(ref.scope); - DecompositionType type = decomposer.canExposeExpression(callNode); - if (type == DecompositionType.MOVABLE) { + ExpressionDecomposer decomposer = getDecomposer(ref.scope); + switch (decomposer.canExposeExpression(callNode)) { + case MOVABLE: return CallSiteType.EXPRESSION; - } else if (type == DecompositionType.DECOMPOSABLE) { + case DECOMPOSABLE: return CallSiteType.DECOMPOSABLE_EXPRESSION; - } else { - checkState(type == DecompositionType.UNDECOMPOSABLE); - } + case UNDECOMPOSABLE: + break; } } diff --git a/test/com/google/javascript/jscomp/ExpressionDecomposerTest.java b/test/com/google/javascript/jscomp/ExpressionDecomposerTest.java index bef70094abc..92af3fd48b1 100644 --- a/test/com/google/javascript/jscomp/ExpressionDecomposerTest.java +++ b/test/com/google/javascript/jscomp/ExpressionDecomposerTest.java @@ -16,13 +16,11 @@ package com.google.javascript.jscomp; -import static com.google.common.base.Preconditions.checkNotNull; -import static com.google.common.base.Preconditions.checkState; import static com.google.common.truth.Truth.assertThat; import static com.google.common.truth.Truth.assertWithMessage; import static com.google.javascript.jscomp.CompilerTestCase.lines; -import static com.google.javascript.rhino.testing.NodeSubject.assertNode; +import com.google.common.collect.ImmutableList; import com.google.javascript.jscomp.CompilerOptions.LanguageMode; import com.google.javascript.jscomp.ExpressionDecomposer.DecompositionType; import com.google.javascript.jscomp.type.SemanticReverseAbstractInterpreter; @@ -164,6 +162,31 @@ public void testCanExposeExpression3() { "function f(){ return goo() && foo();}", "foo"); } + @Test + public void testCanExposeExpression_compoundDeclaration_inForInitializer_firstElement() { + // VAR will already be hoisted by `Normalize`. + helperCanExposeExpression(DecompositionType.MOVABLE, "for (var x = foo(), y = 5;;) {}", "foo"); + + helperCanExposeExpression(DecompositionType.MOVABLE, "for (let x = foo(), y = 5;;) {}", "foo"); + helperCanExposeExpression( + DecompositionType.MOVABLE, "for (const x = foo(), y = 5;;) {}", "foo"); + } + + @Test + public void testCanExposeExpression_compoundDeclaration_inForInitializer_nthElement() { + // TODO(b/121157467) FOR introduces complex scoping that isn't currently `Normalize`d. + // Since in some cases we'd effectively end up having to `Normalize` these, decomposition just + // bails for now. + + // VAR will already be hoisted by `Normalize`. + helperCanExposeExpression(DecompositionType.MOVABLE, "for (var x = 8, y = foo();;) {}", "foo"); + + helperCanExposeExpression( + DecompositionType.UNDECOMPOSABLE, "for (let x = 8, y = foo();;) {}", "foo"); + helperCanExposeExpression( + DecompositionType.UNDECOMPOSABLE, "for (const x = 8, y = foo();;) {}", "foo"); + } + @Test public void testCannotExpose_expression4a() { // 'this' must be preserved in call. @@ -624,6 +647,50 @@ public void testExposeExpression13() { "var temp_const$jscomp$0 = 1 + goo(); switch(temp_const$jscomp$0 + foo()){}"); } + @Test + public void testExposeExpression_inVanillaForInitializer_simpleExpression() { + helperExposeExpression( + "for (x = goo() + foo();;) {}", + "foo", + lines( + "var temp_const$jscomp$0 = goo();", // + "for (x = temp_const$jscomp$0 + foo();;) {}")); + } + + @Test + public void testExposeExpression_inVanillaForInitializer_usingLabel() { + helperExposeExpression( + "LABEL: for (x = goo() + foo();;) {}", + "foo", + lines( + "var temp_const$jscomp$0 = goo();", // + "LABEL: for (x = temp_const$jscomp$0 + foo();;) {}")); + } + + @Test + public void testExposeExpression_inVanillaForInitializer_singleDeclaration_withLetOrConst() { + for (String dec : ImmutableList.of("let", "const")) { + helperExposeExpression( + "for (" + dec + " x = goo() + foo();;) {}", + "foo", + lines( + "var temp_const$jscomp$0 = goo();", // + "for (" + dec + " x = temp_const$jscomp$0 + foo();;) {}")); + } + } + + @Test + public void testExposeExpression_inVanillaForInitializer_firstDeclaration_withLetOrConst() { + for (String dec : ImmutableList.of("let", "const")) { + helperExposeExpression( + "for (" + dec + " x = goo() + foo(), y = 5;;) {}", + "foo", + lines( + "var temp_const$jscomp$0 = goo();", // + "for (" + dec + " x = temp_const$jscomp$0 + foo(), y = 5;;) {}")); + } + } + @Test public void testExposeExpression14() { helperExposeExpression( @@ -932,31 +999,6 @@ public void testExposeObjectLit1() { "foo", "var result$jscomp$0=foo();var x = {set a(p) {}, b: result$jscomp$0};"); } - - @Test - public void testExposeExpressionInTemplateLibSub() { - helperExposeExpression( - "` ${ foo() } ${ goo() } `;", - "goo", - "var temp_const$jscomp$0 = foo(); ` ${ temp_const$jscomp$0 } ${ goo() } `;"); - } - - @Test - public void testExposeSubExpressionInTemplateLibSub() { - helperExposeExpression( - "` ${ foo() + goo() } `;", - "goo", - "var temp_const$jscomp$0 = foo(); ` ${ temp_const$jscomp$0 + goo() } `;"); - } - - @Test - public void testMoveExpressionInTemplateLibSub() { - helperMoveExpression( - "` ${ foo() } ${ goo() } `;", - "foo", - "var result$jscomp$0 = foo(); ` ${ result$jscomp$0 } ${ goo() } `;"); - } - @Test public void testMoveSpread_siblingOfCall_outOfArrayLiteral_usesTempArray() { shouldTestTypes = false; // TODO(nickreid): Enable this when tests support typed `AstFactory`. @@ -998,50 +1040,31 @@ public void testMoveSpreadParent_siblingOfCall_outOfFunctionCall_usesNoTempArray } @Test - public void testFindExpressionRoot1() { - assertNode(findExpressionRoot("var x = f()", "f")).hasType(Token.VAR); - } - - @Test - public void testFindExpressionRoot2() { - assertNode(findExpressionRoot("foo(bar(f()));", "f")).hasType(Token.EXPR_RESULT); - } - - @Test - public void testFindExpressionRoot3() { - assertThat(findExpressionRoot("for (let x of f()) {}", "f")).isNull(); + public void testExposeExpressionInTemplateLibSub() { + helperExposeExpression( + "` ${ foo() } ${ goo() } `;", + "goo", + "var temp_const$jscomp$0 = foo(); ` ${ temp_const$jscomp$0 } ${ goo() } `;"); } @Test - public void testFindExpressionRoot4() { - assertThat(findExpressionRoot("for (let x in f()) {}", "f")).isNull(); + public void testExposeSubExpressionInTemplateLibSub() { + helperExposeExpression( + "` ${ foo() + goo() } `;", + "goo", + "var temp_const$jscomp$0 = foo(); ` ${ temp_const$jscomp$0 + goo() } `;"); } @Test - public void testFindExpressionRoot5() { - assertNode(findExpressionRoot("for (let x = f();;) {}", "f")).hasType(Token.FOR); + public void testMoveExpressionInTemplateLibSub() { + helperMoveExpression( + "` ${ foo() } ${ goo() } `;", + "foo", + "var result$jscomp$0 = foo(); ` ${ result$jscomp$0 } ${ goo() } `;"); } /** Test case helpers. */ - /** - * @return The result of calling {@link ExpressionDecomposer#findExpressionRoot} on the CALL - * node in {@code js} whose callee is a NAME matching {@code name}. - */ - @Nullable - private Node findExpressionRoot(String js, String name) { - Compiler compiler = getCompiler(); - Node tree = parse(compiler, js); - Node call = findCall(tree, name); - checkNotNull(call); - - Node root = ExpressionDecomposer.findExpressionRoot(call); - if (root != null) { - checkState(NodeUtil.isStatement(root), root); - } - return root; - } - private void helperCanExposeFunctionExpression( DecompositionType expectedResult, String code, int call) { Compiler compiler = getCompiler();