Skip to content

Commit bf776ec

Browse files
Closure Teamcopybara-github
Closure Team
authored and
copybara-github
committed
Make ExpressionDecomposer#exposeExpression() private
The correct method to use to expose an expression so it can be moved is `ExpressionDecomposer#maybeExposeExpression()`. It loops until the expression is actually moveable. The `exposeExpression()` method may only partially expose the expression. PiperOrigin-RevId: 321685407
1 parent 392f9ff commit bf776ec

File tree

4 files changed

+71
-79
lines changed

4 files changed

+71
-79
lines changed

‎src/com/google/javascript/jscomp/Es6ExtractClasses.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -179,7 +179,7 @@ private boolean shouldExtractClass(Node classNode) {
179179

180180
private void extractClass(NodeTraversal t, Node classNode) {
181181
if (expressionDecomposer.canExposeExpression(classNode) == DecompositionType.DECOMPOSABLE) {
182-
expressionDecomposer.maybeExposeExpression(classNode);
182+
expressionDecomposer.exposeExpression(classNode);
183183
}
184184
Node parent = classNode.getParent();
185185

‎src/com/google/javascript/jscomp/Es6RewriteGenerators.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -191,7 +191,7 @@ void visitYield(Node n) {
191191
}
192192
if (decomposer.canExposeExpression(n)
193193
!= ExpressionDecomposer.DecompositionType.UNDECOMPOSABLE) {
194-
decomposer.maybeExposeExpression(n);
194+
decomposer.exposeExpression(n);
195195
} else {
196196
String link =
197197
"https://github.com/google/closure-compiler/wiki/FAQ"

‎src/com/google/javascript/jscomp/ExpressionDecomposer.java

Lines changed: 14 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -98,16 +98,10 @@ enum DecompositionType {
9898
private static final int MAX_ITERATIONS = 100;
9999

100100
/**
101-
* Perform any rewriting necessary so that the specified expression is {@code MOVABLE}.
102-
*
103-
* <p>This method is a primary entrypoint into this class. It performs expression decomposition
104-
* such that {@code expression} can be moved to a preceding statement without changing behaviour.
101+
* If required, rewrite the statement containing the expression.
105102
*
106-
* <p>Exposing {@code expression} generally doesn't mean that {@code expression} itself will
107-
* moved. An expression is exposed within a larger statement if no preceding expression would
108-
* interact with it.
109-
*
110-
* @see {@link #canExposeExpression}
103+
* @param expression The expression to be exposed.
104+
* @see #canExposeExpression
111105
*/
112106
void maybeExposeExpression(Node expression) {
113107
// If the expression needs to exposed.
@@ -123,12 +117,19 @@ void maybeExposeExpression(Node expression) {
123117
}
124118

125119
/**
126-
* Perform partial decomposition to get the given expression closer to being {@code MOVEABLE}.
120+
* Perform any rewriting necessary so that the specified expression is {@code MOVABLE}.
127121
*
128-
* <p>This method should not be called from outside of this class. Instead call {@link
129-
* #maybeExposeExpression(Node)}.
122+
* <p>This method is a primary entrypoint into this class. It performs a partial expression
123+
* decomposition such that {@code expression} can be moved to a preceding statement without
124+
* changing behaviour.
125+
*
126+
* <p>Exposing {@code expression} generally doesn't mean that {@code expression} itself will
127+
* moved. An expression is exposed within a larger statement if no preceding expression would
128+
* interact with it.
129+
*
130+
* @see {@link #canExposeExpression}
130131
*/
131-
private void exposeExpression(Node expression) {
132+
void exposeExpression(Node expression) {
132133
Node expressionRoot = findExpressionRoot(expression);
133134
checkNotNull(expressionRoot);
134135
checkState(NodeUtil.isStatement(expressionRoot), expressionRoot);
@@ -1236,16 +1237,6 @@ private boolean isSafeAssign(Node n, boolean seenSideEffects) {
12361237
* incorrect.
12371238
*/
12381239
private boolean isExpressionTreeUnsafe(Node tree, boolean followingSideEffectsExist) {
1239-
if (isTempConstantValueName(tree)) {
1240-
// Constant temporary values we created as part of decomposition are known to be safe.
1241-
// NOTE: We also put these variables into our list of known constants, so you might think
1242-
// explicitly checking the name is redundant with the use of this.knownConstants below.
1243-
// However, there are cases where one ExpressionDecomposer creates the variable, then another
1244-
// one (which doesn't have that variable in its list of known constants) sees it later when
1245-
// doing further decomposition.
1246-
return false;
1247-
}
1248-
12491240
if (tree.isSpread()) {
12501241
// Spread expressions would cause recursive rewriting if not special cased here.
12511242
// When extracted, spreads can't be assigned to a single variable and instead are put into

‎test/com/google/javascript/jscomp/ExpressionDecomposerTest.java

Lines changed: 55 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,7 @@ public final class ExpressionDecomposerTest {
5858
public void setUp() {
5959
allowMethodCallDecomposing = false;
6060
knownConstants.clear();
61+
times = 1;
6162
shouldTestTypes = true;
6263
}
6364

@@ -649,8 +650,7 @@ public void exposeExpressionOptionalGetElem() {
649650
lines(
650651
"var temp$jscomp$1 = x;",
651652
"if (temp$jscomp$1 != null) {",
652-
" var temp_const$jscomp$2 = temp$jscomp$1;",
653-
" var temp$jscomp$0 = temp_const$jscomp$2[foo()];",
653+
" var temp$jscomp$0 = temp$jscomp$1[foo()];",
654654
"}",
655655
"a = temp$jscomp$0"));
656656
}
@@ -663,8 +663,7 @@ public void exposeExpressionOptionalCallChain() {
663663
lines(
664664
"var temp$jscomp$1 = x;",
665665
"if (temp$jscomp$1 != null) {",
666-
" var temp_const$jscomp$2 = temp$jscomp$1(a).y.z;",
667-
" var temp$jscomp$0 = temp_const$jscomp$2[foo()];",
666+
" var temp$jscomp$0 = temp$jscomp$1(a).y.z[foo()];",
668667
"}",
669668
"a = temp$jscomp$0"));
670669
}
@@ -677,8 +676,7 @@ public void exposeExpressionOptionalCallChainNoResult() {
677676
lines(
678677
"var temp$jscomp$0 = x;",
679678
"if (temp$jscomp$0 != null) {",
680-
" var temp_const$jscomp$1 = temp$jscomp$0(a)[y].z;",
681-
" temp_const$jscomp$1[foo()];",
679+
" temp$jscomp$0(a)[y].z[foo()];",
682680
"}"));
683681
}
684682

@@ -690,8 +688,7 @@ public void exposeExpressionOptionalGetPropChain() {
690688
lines(
691689
"var temp$jscomp$1 = x;",
692690
"if (temp$jscomp$1 != null) {",
693-
" var temp_const$jscomp$2 = temp$jscomp$1.y.z;",
694-
" var temp$jscomp$0 = temp_const$jscomp$2[foo()];",
691+
" var temp$jscomp$0 = temp$jscomp$1.y.z[foo()];",
695692
"}",
696693
"a = temp$jscomp$0"));
697694
}
@@ -704,21 +701,19 @@ public void exposeExpressionOptionalGetPropChainNoResult() {
704701
lines(
705702
"var temp$jscomp$0 = x;",
706703
"if (temp$jscomp$0 != null) {",
707-
" var temp_const$jscomp$1 = temp$jscomp$0.y.z;",
708-
" temp_const$jscomp$1[foo()];",
704+
" temp$jscomp$0.y.z[foo()];",
709705
"}"));
710706
}
711707

712708
@Test
713709
public void exposeExpressionOptionalGetElemChain() {
714710
helperExposeExpression(
715-
"a = x?.[y].z[foo()];",
711+
"a = x?.[y].z[foo()]",
716712
"foo",
717713
lines(
718714
"var temp$jscomp$1 = x;",
719715
"if (temp$jscomp$1 != null) {",
720-
" var temp_const$jscomp$2 = temp$jscomp$1[y].z;",
721-
" var temp$jscomp$0 = temp_const$jscomp$2[foo()];",
716+
" var temp$jscomp$0 = temp$jscomp$1[y].z[foo()];",
722717
"}",
723718
"a = temp$jscomp$0"));
724719
}
@@ -731,8 +726,7 @@ public void exposeExpressionOptionalGetElemChainNoResult() {
731726
lines(
732727
"var temp$jscomp$0 = x;",
733728
"if (temp$jscomp$0 != null) {",
734-
" var temp_const$jscomp$1 = temp$jscomp$0[y].z;",
735-
" temp_const$jscomp$1[foo()];",
729+
" temp$jscomp$0[y].z[foo()];",
736730
"}"));
737731
}
738732

@@ -745,9 +739,7 @@ public void exposeExpressionOptionalGetElemWithCall() {
745739
lines(
746740
"var temp$jscomp$1 = x.y;",
747741
"if (temp$jscomp$1 != null) {",
748-
" var temp_const$jscomp$3 = temp$jscomp$1;",
749-
" var temp_const$jscomp$2 = temp_const$jscomp$3[z];",
750-
" var temp$jscomp$0 = temp_const$jscomp$2.call(temp_const$jscomp$3, foo());",
742+
" var temp$jscomp$0 = temp$jscomp$1[z](foo())",
751743
"}",
752744
"a = temp$jscomp$0"));
753745
}
@@ -762,9 +754,7 @@ public void exposeExpressionOptionalGetElemWithCallTwiceRewriteCall() {
762754
lines(
763755
"var temp$jscomp$1 = x.y;",
764756
"if (temp$jscomp$1 != null) {",
765-
" var temp_const$jscomp$3 = temp$jscomp$1;",
766-
" var temp_const$jscomp$2 = temp_const$jscomp$3[z];",
767-
" var temp$jscomp$0 = temp_const$jscomp$2.call(temp_const$jscomp$3, foo());",
757+
" var temp$jscomp$0 = temp$jscomp$1[z](foo())",
768758
"}",
769759
"a = temp$jscomp$0");
770760
String secondTimeExpose =
@@ -779,6 +769,7 @@ public void exposeExpressionOptionalGetElemWithCallTwiceRewriteCall() {
779769

780770
helperExposeExpression(originalSource, "foo", firstTimeExpose);
781771

772+
times = 2;
782773
helperExposeExpression(originalSource, "foo", secondTimeExpose);
783774
}
784775

@@ -792,11 +783,7 @@ public void exposeExpressionGetElemWithOptionalCall() {
792783
"var temp$jscomp$2 = x.y;",
793784
"var temp$jscomp$1 = temp$jscomp$2[z];",
794785
"if(temp$jscomp$1 != null) {",
795-
" var temp_const$jscomp$5 = temp$jscomp$1;",
796-
" var temp_const$jscomp$4 = temp_const$jscomp$5.call;",
797-
" var temp_const$jscomp$3 = temp$jscomp$2;",
798-
" var temp$jscomp$0 = ",
799-
" temp_const$jscomp$4.call(temp_const$jscomp$5, temp_const$jscomp$3, foo(), d);",
786+
" var temp$jscomp$0 = temp$jscomp$1.call(temp$jscomp$2, foo(), d);",
800787
"}",
801788
"a = temp$jscomp$0"));
802789
}
@@ -810,9 +797,7 @@ public void exposeExpressionOptionalGetPropWithCall() {
810797
lines(
811798
"var temp$jscomp$1 = x.y;",
812799
"if (temp$jscomp$1 != null) {",
813-
" var temp_const$jscomp$3 = temp$jscomp$1;",
814-
" var temp_const$jscomp$2 = temp_const$jscomp$3.z;",
815-
" var temp$jscomp$0 = temp_const$jscomp$2.call(temp_const$jscomp$3, foo(1));",
800+
" var temp$jscomp$0 = temp$jscomp$1.z(foo(1));",
816801
"}",
817802
"a = temp$jscomp$0"));
818803
}
@@ -821,17 +806,29 @@ public void exposeExpressionOptionalGetPropWithCall() {
821806
public void exposeExpressionOptionalGetPropWithCallTwiceRewriteCall() {
822807
allowMethodCallDecomposing = true;
823808

824-
helperExposeExpression(
825-
"a = x.y?.z(foo(1))",
826-
"foo",
809+
// 2 calls to exposeExpression() are needed to get full exposure
810+
String originalSource = "a = x.y?.z(foo(1))";
811+
String firstTimeExpose =
812+
lines(
813+
"var temp$jscomp$1 = x.y;",
814+
"if (temp$jscomp$1 != null) {",
815+
" var temp$jscomp$0 = temp$jscomp$1.z(foo(1));",
816+
"}",
817+
"a = temp$jscomp$0");
818+
String secondTimeExpose =
827819
lines(
828820
"var temp$jscomp$1 = x.y;",
829821
"if (temp$jscomp$1 != null) {",
830822
" var temp_const$jscomp$3 = temp$jscomp$1;",
831823
" var temp_const$jscomp$2 = temp_const$jscomp$3.z;",
832824
" var temp$jscomp$0 = temp_const$jscomp$2.call(temp_const$jscomp$3, foo(1));",
833825
"}",
834-
"a = temp$jscomp$0"));
826+
"a = temp$jscomp$0");
827+
828+
helperExposeExpression(originalSource, "foo", firstTimeExpose);
829+
830+
times = 2;
831+
helperExposeExpression(originalSource, "foo", secondTimeExpose);
835832
}
836833

837834
@Test
@@ -844,11 +841,7 @@ public void exposeExpressionGetPropWithOptionalCall() {
844841
"var temp$jscomp$2 = x.y;",
845842
"var temp$jscomp$1 = temp$jscomp$2.z;",
846843
"if (temp$jscomp$1 != null) {",
847-
" var temp_const$jscomp$5 = temp$jscomp$1;",
848-
" var temp_const$jscomp$4 = temp_const$jscomp$5.call;",
849-
" var temp_const$jscomp$3 = temp$jscomp$2;",
850-
" var temp$jscomp$0 =",
851-
" temp_const$jscomp$4.call(temp_const$jscomp$5, temp_const$jscomp$3, foo());",
844+
" var temp$jscomp$0 = temp$jscomp$1.call(temp$jscomp$2, foo())",
852845
"}",
853846
"a = temp$jscomp$0"));
854847
}
@@ -862,9 +855,7 @@ public void exposeExpressionNewOptChainAfterRewriteCall() {
862855
lines(
863856
"var temp$jscomp$1 = x;",
864857
"if (temp$jscomp$1 != null) {",
865-
" var temp_const$jscomp$3 = temp$jscomp$1;",
866-
" var temp_const$jscomp$2 = temp_const$jscomp$3.y;",
867-
" var temp$jscomp$0 = temp_const$jscomp$2.call(temp_const$jscomp$3, foo());",
858+
" var temp$jscomp$0 = temp$jscomp$1.y(foo())",
868859
"}",
869860
"a = temp$jscomp$0?.z.q"));
870861
}
@@ -878,8 +869,7 @@ public void exposeExpressionNewOptChainAfter() {
878869
lines(
879870
"var temp$jscomp$1 = x;",
880871
"if (temp$jscomp$1 != null) {",
881-
" var temp_const$jscomp$2 = temp$jscomp$1.y;",
882-
" var temp$jscomp$0 = temp_const$jscomp$2[foo()];",
872+
" var temp$jscomp$0 = temp$jscomp$1.y[foo()]",
883873
"}",
884874
"a = temp$jscomp$0?.z.q"));
885875
}
@@ -893,8 +883,7 @@ public void exposeExpressionNotImmediatelyFollowedByNewChain() {
893883
lines(
894884
"var temp$jscomp$1 = x;",
895885
"if (temp$jscomp$1 != null) {",
896-
" var temp_const$jscomp$2 = temp$jscomp$1.y;",
897-
" var temp$jscomp$0 = temp_const$jscomp$2[foo()].z.q;",
886+
" var temp$jscomp$0 = temp$jscomp$1.y[foo()].z.q",
898887
"}",
899888
"a = temp$jscomp$0?.b.c"));
900889
}
@@ -908,8 +897,7 @@ public void exposeExpressionBreakingOutOfOptionalChain() {
908897
lines(
909898
"var temp$jscomp$1 = x;",
910899
"if (temp$jscomp$1 != null) {",
911-
" var temp_const$jscomp$2 = temp$jscomp$1.y;",
912-
" var temp$jscomp$0 = temp_const$jscomp$2[foo()];",
900+
" var temp$jscomp$0 = temp$jscomp$1.y[foo()]",
913901
"}",
914902
"a = temp$jscomp$0.z.q"));
915903
}
@@ -923,9 +911,7 @@ public void exposeExpressionAfterTwoOptionalChains() {
923911
lines(
924912
"var temp$jscomp$1 = x?.y.z;",
925913
"if (temp$jscomp$1 != null) {",
926-
" var temp_const$jscomp$3 = temp$jscomp$1;",
927-
" var temp_const$jscomp$2 = temp_const$jscomp$3.q;",
928-
" var temp$jscomp$0 = temp_const$jscomp$2.call(temp_const$jscomp$3, foo());",
914+
" var temp$jscomp$0 = temp$jscomp$1.q(foo());",
929915
"}",
930916
"a = temp$jscomp$0"));
931917
}
@@ -1241,6 +1227,11 @@ public void testExposeYieldExpression3() {
12411227
" return temp_const$jscomp$0.call(temp_const$jscomp$1, yield 1);",
12421228
"}");
12431229
helperExposeExpression(before, "yield", after);
1230+
1231+
// Check that we don't decompose again, which would result in an infinite loop when inlining
1232+
// functions.
1233+
times = 2;
1234+
helperExposeExpression(before, "yield", after);
12441235
}
12451236

12461237
@Test
@@ -1541,6 +1532,7 @@ public void testMoveSuperCall_noSideEffects() {
15411532

15421533
@Test
15431534
public void testExposeSuperCall() {
1535+
times = 2;
15441536
helperExposeExpression(
15451537
"class A { constructor() { super(goo(), foo()) } }",
15461538
"foo",
@@ -1553,6 +1545,7 @@ public void testExposeSuperCall() {
15531545

15541546
@Test
15551547
public void testExposeSuperCall_noSideEffects() {
1548+
times = 2;
15561549
// String() is being used since it's known to not have side-effects.
15571550
helperExposeExpression(
15581551
"class A { constructor() { super(goo(), String()) } }",
@@ -1633,7 +1626,9 @@ private Node helperExposeExpressionThenTypeCheck(String code, Function<Node, Nod
16331626
Node expr = nodeFinder.apply(tree);
16341627

16351628
compiler.resetUniqueNameId();
1636-
decomposer.maybeExposeExpression(expr);
1629+
for (int i = 0; i < times; i++) {
1630+
decomposer.exposeExpression(expr);
1631+
}
16371632
processForTypecheck(compiler, tree);
16381633

16391634
return tree;
@@ -1670,7 +1665,9 @@ private void helperExposeExpression(
16701665
assertThat(result).isEqualTo(DecompositionType.DECOMPOSABLE);
16711666

16721667
compiler.resetUniqueNameId();
1673-
decomposer.maybeExposeExpression(expr);
1668+
for (int i = 0; i < times; i++) {
1669+
decomposer.exposeExpression(expr);
1670+
}
16741671
validateSourceInfo(compiler, tree);
16751672
assertNode(tree).usingSerializer(compiler::toSource).isEqualTo(expectedRoot);
16761673

@@ -1716,7 +1713,9 @@ private void helperMoveExpression(
17161713
assertWithMessage("Expected node was not found.").that(expr).isNotNull();
17171714

17181715
compiler.resetUniqueNameId();
1719-
decomposer.moveExpression(expr);
1716+
for (int i = 0; i < times; i++) {
1717+
decomposer.moveExpression(expr);
1718+
}
17201719
validateSourceInfo(compiler, tree);
17211720
assertNode(tree).usingSerializer(compiler::toSource).isEqualTo(expectedRoot);
17221721

@@ -1725,7 +1724,9 @@ private void helperMoveExpression(
17251724
Node originalExpr = nodeFinder.apply(originalTree);
17261725

17271726
compiler.resetUniqueNameId();
1728-
decomposer.moveExpression(originalExpr);
1727+
for (int i = 0; i < times; i++) {
1728+
decomposer.moveExpression(originalExpr);
1729+
}
17291730
processForTypecheck(compiler, originalTree);
17301731

17311732
// TODO(bradfordcsmith): Don't assume type check + decompose gives the same results as

0 commit comments

Comments
 (0)