Skip to content

Commit 6963812

Browse files
brad4dcopybara-github
brad4d
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: 321663592
1 parent 2d01716 commit 6963812

File tree

4 files changed

+79
-71
lines changed

4 files changed

+79
-71
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.exposeExpression(classNode);
182+
expressionDecomposer.maybeExposeExpression(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.exposeExpression(n);
194+
decomposer.maybeExposeExpression(n);
195195
} else {
196196
String link =
197197
"https://github.com/google/closure-compiler/wiki/FAQ"

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

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

100100
/**
101-
* If required, rewrite the statement containing the expression.
101+
* Perform any rewriting necessary so that the specified expression is {@code MOVABLE}.
102102
*
103-
* @param expression The expression to be exposed.
104-
* @see #canExposeExpression
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.
105+
*
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}
105111
*/
106112
void maybeExposeExpression(Node expression) {
107113
// If the expression needs to exposed.
@@ -117,19 +123,12 @@ void maybeExposeExpression(Node expression) {
117123
}
118124

119125
/**
120-
* Perform any rewriting necessary so that the specified expression is {@code MOVABLE}.
126+
* Perform partial decomposition to get the given expression closer to being {@code MOVEABLE}.
121127
*
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}
128+
* <p>This method should not be called from outside of this class. Instead call {@link
129+
* #maybeExposeExpression(Node)}.
131130
*/
132-
void exposeExpression(Node expression) {
131+
private void exposeExpression(Node expression) {
133132
Node expressionRoot = findExpressionRoot(expression);
134133
checkNotNull(expressionRoot);
135134
checkState(NodeUtil.isStatement(expressionRoot), expressionRoot);
@@ -1237,6 +1236,16 @@ private boolean isSafeAssign(Node n, boolean seenSideEffects) {
12371236
* incorrect.
12381237
*/
12391238
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+
12401249
if (tree.isSpread()) {
12411250
// Spread expressions would cause recursive rewriting if not special cased here.
12421251
// 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: 54 additions & 55 deletions
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,6 @@ public final class ExpressionDecomposerTest {
5858
public void setUp() {
5959
allowMethodCallDecomposing = false;
6060
knownConstants.clear();
61-
times = 1;
6261
shouldTestTypes = true;
6362
}
6463

@@ -650,7 +649,8 @@ public void exposeExpressionOptionalGetElem() {
650649
lines(
651650
"var temp$jscomp$1 = x;",
652651
"if (temp$jscomp$1 != null) {",
653-
" var temp$jscomp$0 = temp$jscomp$1[foo()];",
652+
" var temp_const$jscomp$2 = temp$jscomp$1;",
653+
" var temp$jscomp$0 = temp_const$jscomp$2[foo()];",
654654
"}",
655655
"a = temp$jscomp$0"));
656656
}
@@ -663,7 +663,8 @@ public void exposeExpressionOptionalCallChain() {
663663
lines(
664664
"var temp$jscomp$1 = x;",
665665
"if (temp$jscomp$1 != null) {",
666-
" var temp$jscomp$0 = temp$jscomp$1(a).y.z[foo()];",
666+
" var temp_const$jscomp$2 = temp$jscomp$1(a).y.z;",
667+
" var temp$jscomp$0 = temp_const$jscomp$2[foo()];",
667668
"}",
668669
"a = temp$jscomp$0"));
669670
}
@@ -676,7 +677,8 @@ public void exposeExpressionOptionalCallChainNoResult() {
676677
lines(
677678
"var temp$jscomp$0 = x;",
678679
"if (temp$jscomp$0 != null) {",
679-
" temp$jscomp$0(a)[y].z[foo()];",
680+
" var temp_const$jscomp$1 = temp$jscomp$0(a)[y].z;",
681+
" temp_const$jscomp$1[foo()];",
680682
"}"));
681683
}
682684

@@ -688,7 +690,8 @@ public void exposeExpressionOptionalGetPropChain() {
688690
lines(
689691
"var temp$jscomp$1 = x;",
690692
"if (temp$jscomp$1 != null) {",
691-
" var temp$jscomp$0 = temp$jscomp$1.y.z[foo()];",
693+
" var temp_const$jscomp$2 = temp$jscomp$1.y.z;",
694+
" var temp$jscomp$0 = temp_const$jscomp$2[foo()];",
692695
"}",
693696
"a = temp$jscomp$0"));
694697
}
@@ -701,19 +704,21 @@ public void exposeExpressionOptionalGetPropChainNoResult() {
701704
lines(
702705
"var temp$jscomp$0 = x;",
703706
"if (temp$jscomp$0 != null) {",
704-
" temp$jscomp$0.y.z[foo()];",
707+
" var temp_const$jscomp$1 = temp$jscomp$0.y.z;",
708+
" temp_const$jscomp$1[foo()];",
705709
"}"));
706710
}
707711

708712
@Test
709713
public void exposeExpressionOptionalGetElemChain() {
710714
helperExposeExpression(
711-
"a = x?.[y].z[foo()]",
715+
"a = x?.[y].z[foo()];",
712716
"foo",
713717
lines(
714718
"var temp$jscomp$1 = x;",
715719
"if (temp$jscomp$1 != null) {",
716-
" var temp$jscomp$0 = temp$jscomp$1[y].z[foo()];",
720+
" var temp_const$jscomp$2 = temp$jscomp$1[y].z;",
721+
" var temp$jscomp$0 = temp_const$jscomp$2[foo()];",
717722
"}",
718723
"a = temp$jscomp$0"));
719724
}
@@ -726,7 +731,8 @@ public void exposeExpressionOptionalGetElemChainNoResult() {
726731
lines(
727732
"var temp$jscomp$0 = x;",
728733
"if (temp$jscomp$0 != null) {",
729-
" temp$jscomp$0[y].z[foo()];",
734+
" var temp_const$jscomp$1 = temp$jscomp$0[y].z;",
735+
" temp_const$jscomp$1[foo()];",
730736
"}"));
731737
}
732738

@@ -739,7 +745,9 @@ public void exposeExpressionOptionalGetElemWithCall() {
739745
lines(
740746
"var temp$jscomp$1 = x.y;",
741747
"if (temp$jscomp$1 != null) {",
742-
" var temp$jscomp$0 = temp$jscomp$1[z](foo())",
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());",
743751
"}",
744752
"a = temp$jscomp$0"));
745753
}
@@ -754,7 +762,9 @@ public void exposeExpressionOptionalGetElemWithCallTwiceRewriteCall() {
754762
lines(
755763
"var temp$jscomp$1 = x.y;",
756764
"if (temp$jscomp$1 != null) {",
757-
" var temp$jscomp$0 = temp$jscomp$1[z](foo())",
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());",
758768
"}",
759769
"a = temp$jscomp$0");
760770
String secondTimeExpose =
@@ -769,7 +779,6 @@ public void exposeExpressionOptionalGetElemWithCallTwiceRewriteCall() {
769779

770780
helperExposeExpression(originalSource, "foo", firstTimeExpose);
771781

772-
times = 2;
773782
helperExposeExpression(originalSource, "foo", secondTimeExpose);
774783
}
775784

@@ -783,7 +792,11 @@ public void exposeExpressionGetElemWithOptionalCall() {
783792
"var temp$jscomp$2 = x.y;",
784793
"var temp$jscomp$1 = temp$jscomp$2[z];",
785794
"if(temp$jscomp$1 != null) {",
786-
" var temp$jscomp$0 = temp$jscomp$1.call(temp$jscomp$2, foo(), d);",
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);",
787800
"}",
788801
"a = temp$jscomp$0"));
789802
}
@@ -797,7 +810,9 @@ public void exposeExpressionOptionalGetPropWithCall() {
797810
lines(
798811
"var temp$jscomp$1 = x.y;",
799812
"if (temp$jscomp$1 != null) {",
800-
" var temp$jscomp$0 = temp$jscomp$1.z(foo(1));",
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));",
801816
"}",
802817
"a = temp$jscomp$0"));
803818
}
@@ -806,29 +821,17 @@ public void exposeExpressionOptionalGetPropWithCall() {
806821
public void exposeExpressionOptionalGetPropWithCallTwiceRewriteCall() {
807822
allowMethodCallDecomposing = true;
808823

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 =
824+
helperExposeExpression(
825+
"a = x.y?.z(foo(1))",
826+
"foo",
819827
lines(
820828
"var temp$jscomp$1 = x.y;",
821829
"if (temp$jscomp$1 != null) {",
822830
" var temp_const$jscomp$3 = temp$jscomp$1;",
823831
" var temp_const$jscomp$2 = temp_const$jscomp$3.z;",
824832
" var temp$jscomp$0 = temp_const$jscomp$2.call(temp_const$jscomp$3, foo(1));",
825833
"}",
826-
"a = temp$jscomp$0");
827-
828-
helperExposeExpression(originalSource, "foo", firstTimeExpose);
829-
830-
times = 2;
831-
helperExposeExpression(originalSource, "foo", secondTimeExpose);
834+
"a = temp$jscomp$0"));
832835
}
833836

834837
@Test
@@ -841,7 +844,11 @@ public void exposeExpressionGetPropWithOptionalCall() {
841844
"var temp$jscomp$2 = x.y;",
842845
"var temp$jscomp$1 = temp$jscomp$2.z;",
843846
"if (temp$jscomp$1 != null) {",
844-
" var temp$jscomp$0 = temp$jscomp$1.call(temp$jscomp$2, foo())",
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());",
845852
"}",
846853
"a = temp$jscomp$0"));
847854
}
@@ -855,7 +862,9 @@ public void exposeExpressionNewOptChainAfterRewriteCall() {
855862
lines(
856863
"var temp$jscomp$1 = x;",
857864
"if (temp$jscomp$1 != null) {",
858-
" var temp$jscomp$0 = temp$jscomp$1.y(foo())",
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());",
859868
"}",
860869
"a = temp$jscomp$0?.z.q"));
861870
}
@@ -869,7 +878,8 @@ public void exposeExpressionNewOptChainAfter() {
869878
lines(
870879
"var temp$jscomp$1 = x;",
871880
"if (temp$jscomp$1 != null) {",
872-
" var temp$jscomp$0 = temp$jscomp$1.y[foo()]",
881+
" var temp_const$jscomp$2 = temp$jscomp$1.y;",
882+
" var temp$jscomp$0 = temp_const$jscomp$2[foo()];",
873883
"}",
874884
"a = temp$jscomp$0?.z.q"));
875885
}
@@ -883,7 +893,8 @@ public void exposeExpressionNotImmediatelyFollowedByNewChain() {
883893
lines(
884894
"var temp$jscomp$1 = x;",
885895
"if (temp$jscomp$1 != null) {",
886-
" var temp$jscomp$0 = temp$jscomp$1.y[foo()].z.q",
896+
" var temp_const$jscomp$2 = temp$jscomp$1.y;",
897+
" var temp$jscomp$0 = temp_const$jscomp$2[foo()].z.q;",
887898
"}",
888899
"a = temp$jscomp$0?.b.c"));
889900
}
@@ -897,7 +908,8 @@ public void exposeExpressionBreakingOutOfOptionalChain() {
897908
lines(
898909
"var temp$jscomp$1 = x;",
899910
"if (temp$jscomp$1 != null) {",
900-
" var temp$jscomp$0 = temp$jscomp$1.y[foo()]",
911+
" var temp_const$jscomp$2 = temp$jscomp$1.y;",
912+
" var temp$jscomp$0 = temp_const$jscomp$2[foo()];",
901913
"}",
902914
"a = temp$jscomp$0.z.q"));
903915
}
@@ -911,7 +923,9 @@ public void exposeExpressionAfterTwoOptionalChains() {
911923
lines(
912924
"var temp$jscomp$1 = x?.y.z;",
913925
"if (temp$jscomp$1 != null) {",
914-
" var temp$jscomp$0 = temp$jscomp$1.q(foo());",
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());",
915929
"}",
916930
"a = temp$jscomp$0"));
917931
}
@@ -1227,11 +1241,6 @@ public void testExposeYieldExpression3() {
12271241
" return temp_const$jscomp$0.call(temp_const$jscomp$1, yield 1);",
12281242
"}");
12291243
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);
12351244
}
12361245

12371246
@Test
@@ -1532,7 +1541,6 @@ public void testMoveSuperCall_noSideEffects() {
15321541

15331542
@Test
15341543
public void testExposeSuperCall() {
1535-
times = 2;
15361544
helperExposeExpression(
15371545
"class A { constructor() { super(goo(), foo()) } }",
15381546
"foo",
@@ -1545,7 +1553,6 @@ public void testExposeSuperCall() {
15451553

15461554
@Test
15471555
public void testExposeSuperCall_noSideEffects() {
1548-
times = 2;
15491556
// String() is being used since it's known to not have side-effects.
15501557
helperExposeExpression(
15511558
"class A { constructor() { super(goo(), String()) } }",
@@ -1626,9 +1633,7 @@ private Node helperExposeExpressionThenTypeCheck(String code, Function<Node, Nod
16261633
Node expr = nodeFinder.apply(tree);
16271634

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

16341639
return tree;
@@ -1665,9 +1670,7 @@ private void helperExposeExpression(
16651670
assertThat(result).isEqualTo(DecompositionType.DECOMPOSABLE);
16661671

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

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

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

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

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

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

0 commit comments

Comments
 (0)