Fix optimization of function calls to not leak this. (issue 202360043 by msamuel@google.com)

[re...@codereview-hr.appspotmail.com]

Reviewers: kpreid2,

Description:
The optimizer was propagating an isFn bit improperly when folding
expressions
which caused (0, a.b)() to simplify to (a.b()) which leaks (a.b) as the
"this"
value for the call.

There was another problem in folding which caused an infinite loop on
the test
cases added to JsOptimizerTest. That was fixed in Operation.fold.

Please review this at https://codereview.appspot.com/202360043/

Affected files (+-3, --3 lines):
M src/com/google/caja/ancillary/opt/StatementSimplifier.java
M src/com/google/caja/parser/js/Operation.java
M tests/com/google/caja/ancillary/opt/JsOptimizerTest.java


Index: src/com/google/caja/ancillary/opt/StatementSimplifier.java
===================================================================
[1;31m--- src/com/google/caja/ancillary/opt/StatementSimplifier.java
(revision 5712) [0;0m
[1;34m+++ src/com/google/caja/ancillary/opt/StatementSimplifier.java
(working copy) [0;0m
[1;35m@@ -188,14 +188,19 @@ [0;0m
[0;0m return new ReturnStmt(rs.getFilePosition(),
optReturnValue); [0;0m
[0;0m } [0;0m
[0;0m return rs; [0;0m
[1;34m+ } else if (n instanceof Expression) { [0;0m
[1;34m+ // We need to recurse into it to find function constructor
bodies, [0;0m
[1;34m+ // and we want to fold the result, but we don't want to fold
recursively [0;0m
[1;34m+ // with isFn=false, so we separate the recursion through the
expression [0;0m
[1;34m+ // for statements out. [0;0m
[1;34m+ return optimizeEmbeddedExpressions((Expression) n,
false); [0;0m
[0;0m } else { [0;0m
[0;0m List<? extends ParseTreeNode> children = n.children(); [0;0m
[0;0m int nChildren = children.size(); [0;0m
[0;0m List<ParseTreeNode> newChildren = null; [0;0m
[0;0m boolean childNeedsBlock = ( [0;0m
[1;31m- n instanceof FunctionConstructor || n instanceof
TryStmt [0;0m
[1;31m- || n instanceof CatchStmt || n instanceof
FinallyStmt [0;0m
[1;31m- || n instanceof SwitchCase); [0;0m
[1;34m+ n instanceof TryStmt || n instanceof CatchStmt [0;0m
[1;34m+ || n instanceof FinallyStmt || n instanceof
SwitchCase); [0;0m
[0;0m for (int i = 0; i < nChildren; ++i) { [0;0m
[0;0m ParseTreeNode child = children.get(i); [0;0m
[0;0m ParseTreeNode newChild = optimize(child,
childNeedsBlock); [0;0m
[1;35m@@ -262,10 +267,41 @@ [0;0m
[0;0m n = ParseTreeNodes.newNodeInstance( [0;0m
[0;0m n.getClass(), n.getFilePosition(), n.getValue(),
newChildren); [0;0m
[0;0m } [0;0m
[1;31m- return n instanceof Expression ? ((Expression)
n).fold(false) : n; [0;0m
[1;34m+ return n; [0;0m
[0;0m } [0;0m
[0;0m } [0;0m
[0;0m [0;0m
[1;34m+ private Expression optimizeEmbeddedExpressions(Expression e,
boolean isFn) { [0;0m
[1;34m+ List<? extends ParseTreeNode> children = e.children(); [0;0m
[1;34m+ int nChildren = children.size(); [0;0m
[1;34m+ List<ParseTreeNode> newChildren = null; [0;0m
[1;34m+ for (int i = 0; i < nChildren; ++i) { [0;0m
[1;34m+ ParseTreeNode child = children.get(i); [0;0m
[1;34m+ ParseTreeNode newChild; [0;0m
[1;34m+ if (child instanceof Expression) { [0;0m
[1;34m+ boolean childIsFn = i == 0 && Operation.is(e,
Operator.FUNCTION_CALL); [0;0m
[1;34m+ newChild = optimizeEmbeddedExpressions((Expression) child,
childIsFn); [0;0m
[1;34m+ } else if (child instanceof Statement) { [0;0m
[1;34m+ newChild = optimize(child, e instanceof
FunctionConstructor); [0;0m
[1;34m+ } else { [0;0m
[1;34m+ newChild = child; [0;0m
[1;34m+ } [0;0m
[1;34m+ if (child != newChild) { [0;0m
[1;34m+ if (newChildren == null) { [0;0m
[1;34m+ newChildren =
Lists.newArrayListWithCapacity(nChildren); [0;0m
[1;34m+ } [0;0m
[1;34m+ newChildren.addAll(children.subList(newChildren.size(),
i)); [0;0m
[1;34m+ newChildren.add(newChild); [0;0m
[1;34m+ } [0;0m
[1;34m+ } [0;0m
[1;34m+ if (newChildren != null) { [0;0m
[1;34m+ newChildren.addAll(children.subList(newChildren.size(),
nChildren)); [0;0m
[1;34m+ e = (Expression) ParseTreeNodes.newNodeInstance( [0;0m
[1;34m+ e.getClass(), e.getFilePosition(), e.getValue(),
newChildren); [0;0m
[1;34m+ } [0;0m
[1;34m+ return e.fold(isFn); [0;0m
[1;34m+ } [0;0m
[1;34m+ [0;0m
[0;0m /** [0;0m
[0;0m * <code>{ a; { b; c; } ; ; d }</code> -> <code>{ a; b; c; d;
}</code> [0;0m
[0;0m * @return {@code null} if there are no optimizations to perform
on the input. [0;0m
[1;35m@@ -592,7 +628,7 @@ [0;0m
[0;0m } [0;0m
[0;0m } [0;0m
[0;0m // TODO(mikesamuel): if c is simple and not a global reference,
optimize [0;0m
[1;31m- // out he common head as well. [0;0m
[1;34m+ // out the common head as well. [0;0m
[0;0m CommaCommonalities opt = commaCommonalities(x, y); [0;0m
[0;0m if (opt != null) { [0;0m
[0;0m // Both reduced sides can't be null since we checked above
whether [0;0m
Index: src/com/google/caja/parser/js/Operation.java
===================================================================
[1;31m--- src/com/google/caja/parser/js/Operation.java (revision
5712) [0;0m
[1;34m+++ src/com/google/caja/parser/js/Operation.java (working copy) [0;0m
[1;35m@@ -20,6 +20,7 @@ [0;0m
[0;0m import com.google.caja.lexer.TokenConsumer; [0;0m
[0;0m import com.google.caja.parser.ParseTreeNode; [0;0m
[0;0m import com.google.caja.reporting.RenderContext; [0;0m
[1;34m+import com.google.caja.util.Lists; [0;0m
[0;0m import java.util.Arrays; [0;0m
[0;0m import java.util.List; [0;0m
[0;0m [0;0m
[1;35m@@ -519,9 +520,18 @@ [0;0m
[0;0m case 1: folded = foldUnaryOp(); break; [0;0m
[0;0m case 2: folded = foldBinaryOp(); break; [0;0m
[0;0m case 3: folded = foldTernaryOp(); break; [0;0m
[1;31m- default: return this; [0;0m
[1;34m+ default: folded = this; [0;0m
[0;0m } [0;0m
[0;0m if (isFn && isFnSpecialForm(folded) && !isFnSpecialForm(this))
{ [0;0m
[1;34m+ if (is(this, Operator.COMMA)) { [0;0m
[1;34m+ List<? extends Expression> operands = children(); [0;0m
[1;34m+ Expression left = operands.get(0); [0;0m
[1;34m+ Expression right = operands.get(1); [0;0m
[1;34m+ if (right == folded [0;0m
[1;34m+ && left instanceof IntegerLiteral &&
left.getValue().equals(0L)) { [0;0m
[1;34m+ return this; [0;0m
[1;34m+ } [0;0m
[1;34m+ } [0;0m
[0;0m FilePosition pos = getFilePosition(); [0;0m
[0;0m folded = Operation.create( [0;0m
[0;0m pos, Operator.COMMA, [0;0m
[1;35m@@ -739,7 +749,7 @@ [0;0m
[0;0m } [0;0m
[0;0m } [0;0m
[0;0m } else if (is(fn, Operator.MEMBER_ACCESS) [0;0m
[1;31m- && fn.children().get(0) instanceof StringLiteral) { [0;0m
[1;34m+ && fn.children().get(0) instanceof StringLiteral)
{ [0;0m
[0;0m StringLiteral sl = (StringLiteral) fn.children().get(0); [0;0m
[0;0m String methodName = ((Reference) fn.children().get(1)) [0;0m
[0;0m .getIdentifierName(); [0;0m
Index: tests/com/google/caja/ancillary/opt/JsOptimizerTest.java
===================================================================
[1;31m--- tests/com/google/caja/ancillary/opt/JsOptimizerTest.java
(revision 5712) [0;0m
[1;34m+++ tests/com/google/caja/ancillary/opt/JsOptimizerTest.java
(working copy) [0;0m
[1;35m@@ -84,6 +84,24 @@ [0;0m
[0;0m + "else return boo(); }());"))); [0;0m
[0;0m } [0;0m
[0;0m [0;0m
[1;34m+ public final void testThisNotUnintentionallyPassed1() throws
Exception { [0;0m
[1;34m+ assertOptimized( [0;0m
[1;34m+ js(fromString("var x = a.b")), [0;0m
[1;34m+ js(fromString("var x = (0,a.b)"))); [0;0m
[1;34m+ } [0;0m
[1;34m+ [0;0m
[1;34m+ public final void testThisNotUnintentionallyPassed2() throws
Exception { [0;0m
[1;34m+ assertOptimized( [0;0m
[1;34m+ js(fromString("x=(0,a.b)()")), // Does not pass a.b as
this [0;0m
[1;34m+ js(fromString("x = (1,a.b)()"))); [0;0m
[1;34m+ } [0;0m
[1;34m+ [0;0m
[1;34m+ public final void testThisNotUnintentionallyPassed3() throws
Exception { [0;0m
[1;34m+ assertOptimized( [0;0m
[1;34m+ js(fromString("var x=(0,a.b)()")), // Does not pass a.b as
this [0;0m
[1;34m+ js(fromString("var x = (1,a.b)()"))); [0;0m
[1;34m+ } [0;0m
[1;34m+ [0;0m
[0;0m public final void testIssue1348() throws Exception { [0;0m
[0;0m String input = Join.join( [0;0m
[0;0m "\n", [0;0m

[eri...@gmail.com]

codereview.appspot is not showing me any differences.


https://codereview.appspot.com/202360043/

[☻Mike Samuel]

Yeah. I had diff-cmd=colordiff in my .subversion/config and our myvn
script doesn't know to ignore it.

[kpr...@google.com]

LGTM


https://codereview.appspot.com/202360043/diff/2/tests/com/google/caja/ancillary/opt/JsOptimizerTest.java
File tests/com/google/caja/ancillary/opt/JsOptimizerTest.java (right):

https://codereview.appspot.com/202360043/diff/2/tests/com/google/caja/ancillary/opt/JsOptimizerTest.java#newcode87
tests/com/google/caja/ancillary/opt/JsOptimizerTest.java:87: public

final void testThisNotUnintentionallyPassed1() throws Exception {

This test isn't quite testing what its name suggests, but I agree the
name parallelism is relevant. How about a comment like "/** Test that
(0,...) is not inserted when it is not necessary */"?

https://codereview.appspot.com/202360043/

[msa...@google.com]

https://codereview.appspot.com/202360043/diff/2/tests/com/google/caja/ancillary/opt/JsOptimizerTest.java
File tests/com/google/caja/ancillary/opt/JsOptimizerTest.java (right):

https://codereview.appspot.com/202360043/diff/2/tests/com/google/caja/ancillary/opt/JsOptimizerTest.java#newcode87
tests/com/google/caja/ancillary/opt/JsOptimizerTest.java:87: public
final void testThisNotUnintentionallyPassed1() throws Exception {

On 2015/02/26 18:32:38, kpreid_google wrote:
> This test isn't quite testing what its name suggests, but I agree the
name
> parallelism is relevant. How about a comment like "/** Test that
(0,...) is not
> inserted when it is not necessary */"?

Done.

https://codereview.appspot.com/202360043/

[kpr...@google.com]

LGTM

https://codereview.appspot.com/202360043/