Skip to content

Commit

Permalink
Fix incorrect ordering of side-effects when an function inlining targ…
Browse files Browse the repository at this point in the history
…et was part of a call target expression.

Previously, the ExpressionDecomposer unconditionally extracted the arguments to a call expression in the hierarchy of the target expression.  Now, they are only extracted if they are "in the way".

PiperOrigin-RevId: 407450534
  • Loading branch information
concavelenz authored and copybara-github committed Nov 3, 2021
1 parent fb3a9f6 commit 158b9d8
Show file tree
Hide file tree
Showing 2 changed files with 77 additions and 6 deletions.
15 changes: 9 additions & 6 deletions src/com/google/javascript/jscomp/ExpressionDecomposer.java
Original file line number Diff line number Diff line change
Expand Up @@ -233,7 +233,9 @@ private void exposeExpression(Node expressionRoot, Node subExpression) {
} else if (expressionParent.isCall()
&& NodeUtil.isNormalGet(expressionParent.getFirstChild())) {
Node callee = expressionParent.getFirstChild();
decomposeSubExpressions(callee.getNext(), expressionToExpose, state);
if (callee != expressionToExpose) {
decomposeSubExpressions(callee.getNext(), expressionToExpose, state);
}

// Now handle the call expression. We only have to do this if we arrived at decomposing this
// call through one of the arguments, rather than the callee; otherwise the callee would
Expand Down Expand Up @@ -995,11 +997,12 @@ 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.
// Most declarations are split by `Normalize` but to do so in loops would change the
// behavior of the code. We don't current handle this case but it is possible:
// For this case: `for (let x = 5, y = 2 * x; ...` where `child = y`.
// As later expressions may reference the earlier expression, it would be necessary to
// rewrite references when extracting the expressions:
// `var temp1 = 5; var temp2 = 2 * temp1; for (let x = temp1, y = temp2; ...`
return DecompositionType.UNDECOMPOSABLE;
}

Expand Down
68 changes: 68 additions & 0 deletions test/com/google/javascript/jscomp/ExpressionDecomposerTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -1055,6 +1055,74 @@ public void exposeExpressionCallAtEndOfOptionalChain() {
""));
}

@Test
public void testBug117935266_expose_call_target() {
helperExposeExpression(
lines(
"function first() {",
" alert('first');",
" return '';",
"}",
// alert must be preserved before the first side-effect
"alert(first().method(alert('second')).method(alert('third')));"),
exprMatchesStr("first()"),
lines(
"function first() {",
" alert('first');",
" return '';",
"}",
"var temp_const$jscomp$0 = alert;",
"temp_const$jscomp$0(first().method(",
" alert('second')).method(alert('third')));"));
}

@Test
public void testBug117935266_move_call_target() {
helperMoveExpression(
lines(
"function first() {",
" alert('first');",
" return '';",
"}",
"var temp_const$jscomp$0 = alert;",
"temp_const$jscomp$0(first().toString(",
" alert('second')).toString(alert('third')));"),
exprMatchesStr("first()"),
lines(
"function first() {",
" alert('first');",
" return '';",
"}",
"var temp_const$jscomp$0 = alert;",
"var result$jscomp$0 = first();",
"temp_const$jscomp$0(result$jscomp$0.toString(",
" alert('second')).toString(alert('third')));"));
}

@Test
public void testBug117935266_expose_call_parameters() {
helperExposeExpression(
lines(
// alert must be preserved before the first side-effect
"alert(fn(first(), second(), third()));"),
exprMatchesStr("first()"),
lines(
"var temp_const$jscomp$1 = alert;",
"var temp_const$jscomp$0 = fn;",
"temp_const$jscomp$1(temp_const$jscomp$0(first(), second(), third()));"));

helperExposeExpression(
lines(
// alert must be preserved before the first side-effect
"alert(fn(first(), second(), third()));"),
exprMatchesStr("second()"),
lines(
"var temp_const$jscomp$2 = alert;",
"var temp_const$jscomp$1 = fn;",
"var temp_const$jscomp$0 = first();",
"temp_const$jscomp$2(temp_const$jscomp$1(temp_const$jscomp$0, second(), third()));"));
}

@Test
public void exposeExpressionAfterTwoOptionalChains() {
helperExposeExpression(
Expand Down

0 comments on commit 158b9d8

Please sign in to comment.