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
--
---
You received this message because you are subscribed to the Google Groups "Google Caja Discuss" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/d/optout.
