Revision: 3999
Author: [email protected]
Date: Tue Mar 2 14:43:56 2010
Log: Fix problem with Expression.fold changing meaning of code.
http://codereview.appspot.com/224054
There are a number of places in the ES spec where the meaning of a
code construct is dependent on the context in which it appears.
Specifically,
eval
and
foo.bar
both have different meanings when they appear are the first operand to
the function call operator.
[email protected]
http://code.google.com/p/google-caja/source/detail?r=3999
Modified:
/trunk/src/com/google/caja/ancillary/opt/JsOptimizer.java
/trunk/src/com/google/caja/ancillary/opt/ParseTreeKB.java
/trunk/src/com/google/caja/ancillary/opt/StatementSimplifier.java
/trunk/src/com/google/caja/parser/js/AbstractExpression.java
/trunk/src/com/google/caja/parser/js/Expression.java
/trunk/src/com/google/caja/parser/js/Operation.java
/trunk/tests/com/google/caja/ancillary/opt/ParseTreeKBTest.java
/trunk/tests/com/google/caja/parser/js/ExpressionTest.java
=======================================
--- /trunk/src/com/google/caja/ancillary/opt/JsOptimizer.java Mon Jan 18
15:50:44 2010
+++ /trunk/src/com/google/caja/ancillary/opt/JsOptimizer.java Tue Mar 2
14:43:56 2010
@@ -74,7 +74,7 @@
if (optimizer == null) { optimizer = new ParseTreeKB(); }
List<? extends Expression> parts = envJson.children();
for (int i = 0, n = parts.size(); i < n; i += 2) {
- Expression value = parts.get(i + 1).fold(); // fold negative nums
+ Expression value = parts.get(i + 1).fold(false); // fold negative
nums
if (!(value instanceof Literal)) {
// True for "*useragent*" property inserted by JSKB.
continue;
=======================================
--- /trunk/src/com/google/caja/ancillary/opt/ParseTreeKB.java Mon Jan 18
15:50:44 2010
+++ /trunk/src/com/google/caja/ancillary/opt/ParseTreeKB.java Tue Mar 2
14:43:56 2010
@@ -91,7 +91,7 @@
while (true) {
Result out = new Result();
Scope s = Scope.fromProgram(js, mq);
- optimize(s, js, false, false, false, out);
+ optimize(s, js, false, false, false, false, out);
Block optimized = ConstLocalOptimization.optimize((Block) out.node);
if (optimized == js) { return optimized; }
js = optimized;
@@ -106,7 +106,7 @@
// Recursively fold e, since that is how the optimizer will compare it.
// This has the side effect of turning complex expressions like (1/0),
// (0/0), and (-0) into NumberLiterals.
- addFactInt(rfold(e), fact);
+ addFactInt(rfold(e, false), fact);
}
private void addFactInt(Expression e, Fact fact) {
@@ -371,7 +371,7 @@
private void optimize(
Scope s, ParseTreeNode node, boolean isFuzzy, boolean isLhs,
- boolean throwsOnUndefined, Result out) {
+ boolean throwsOnUndefined, boolean isFn, Result out) {
if (node instanceof Conditional) {
// Handle conditionals specially since the goal of this code is to
cut
// bits out of them.
@@ -414,6 +414,8 @@
// The number of operands that will cause the exception to fail with
// an error if undefined.
int touLimit = 0;
+ // The number of operands that are functions.
+ int fnLimit = 0;
if (node instanceof Operation) {
Operator op = ((Operation) node).getOperator();
@@ -427,7 +429,10 @@
case NOT:
fuzzyLimit = 1;
break;
- case FUNCTION_CALL: case MEMBER_ACCESS: case CONSTRUCTOR:
+ case FUNCTION_CALL:
+ fnLimit = 1;
+ // $FALL-THROUGH$
+ case MEMBER_ACCESS: case CONSTRUCTOR:
case SQUARE_BRACKET:
touLimit = 1;
break;
@@ -447,7 +452,9 @@
ParseTreeNode[] newChildren = null;
for (int i = 0; i < n; ++i) {
ParseTreeNode child = children.get(i);
- optimize(s, child, i < fuzzyLimit, i < lhsLimit, i < touLimit,
out);
+ optimize(
+ s, child, i < fuzzyLimit, i < lhsLimit, i < touLimit,
+ i < fnLimit, out);
sb = addDigest(out.digest, sb);
if (out.node != child) {
if (newChildren == null) {
@@ -483,7 +490,7 @@
}
if (node instanceof Expression) {
- Expression folded = normNum(((Expression) node).fold());
+ Expression folded = normNum(((Expression) node).fold(isFn));
if (folded != node) {
node = folded;
digest = optNodeDigest(folded);
@@ -505,7 +512,7 @@
}
while (i < n) {
ParseTreeNode child = children.get(i);
- optimize(s, child, true, false, false, out);
+ optimize(s, child, true, false, false, false, out);
ParseTreeNode newChild = out.node;
String newDigest = out.digest;
Boolean optCond = (i & 1) == 0 && i + 1 < n
@@ -531,7 +538,7 @@
} else {
if (optCond) {
// if (foo() || true) { ... } => { foo(); ... }
- optimize(s, children.get(i + 1), false, false, false, out);
+ optimize(s, children.get(i + 1), false, false, false, false,
out);
// if (foo() && 0) { ... } else { baz(); } => { foo();
baz(); }
} else {
optimizeConditional(s, c, i + 2, out);
@@ -587,7 +594,7 @@
StringBuilder sb = new StringBuilder();
sb.append('(');
Expression obj = ma.children().get(0);
- optimize(s, obj, false, false, false, out);
+ optimize(s, obj, false, false, false, false, out);
Reference prop = (Reference) ma.children().get(1);
if (out.node != obj) {
ma = Operation.createInfix(
@@ -814,14 +821,15 @@
e.getFilePosition(), op.getOperator(), withoutTopRef(obj), prop);
}
- private static Expression rfold(Expression e) {
+ private static Expression rfold(Expression e, boolean isFn) {
if (e instanceof Operation) {
Operation o = (Operation) e;
List<? extends Expression> children = o.children();
Expression[] newChildren = null;
+ boolean oIsFn = o.getOperator() == Operator.FUNCTION_CALL;
for (int i = 0, n = children.size(); i < n; ++i) {
Expression operand = children.get(i);
- Expression newOperand = operand.fold();
+ Expression newOperand = rfold(operand, oIsFn && i == 0);
if (operand == newOperand) { continue; }
if (newChildren == null) {
newChildren = children.toArray(new Expression[n]);
@@ -832,7 +840,7 @@
e = Operation.create(e.getFilePosition(), o.getOperator(),
newChildren);
}
}
- return normNum(e.fold());
+ return normNum(e.fold(isFn));
}
private static Expression normNum(Expression e) {
=======================================
--- /trunk/src/com/google/caja/ancillary/opt/StatementSimplifier.java Mon
Jan 18 15:50:44 2010
+++ /trunk/src/com/google/caja/ancillary/opt/StatementSimplifier.java Tue
Mar 2 14:43:56 2010
@@ -250,7 +250,7 @@
n = ParseTreeNodes.newNodeInstance(
n.getClass(), n.getFilePosition(), n.getValue(), newChildren);
}
- return n instanceof Expression ? ((Expression) n).fold() : n;
+ return n instanceof Expression ? ((Expression) n).fold(false) : n;
}
}
@@ -487,7 +487,7 @@
BooleanLiteral a = (BooleanLiteral) clause,
b = (BooleanLiteral) e;
if (a.value == b.value) {
- e = commaOp(cond, a).fold();
+ e = commaOp(cond, a).fold(false);
} else {
// cond ? true : false -> !!cond
int nNotsNeeded = a.value ? 2 : 1;
@@ -496,7 +496,8 @@
}
e = cond;
while (--nNotsNeeded >= 0) {
- e = Operation.create(e.getFilePosition(), Operator.NOT,
e).fold();
+ e = Operation.create(e.getFilePosition(), Operator.NOT, e)
+ .fold(false);
}
}
} else if (Operation.is(cond, Operator.NOT)) {
=======================================
--- /trunk/src/com/google/caja/parser/js/AbstractExpression.java Thu Oct 22
14:07:39 2009
+++ /trunk/src/com/google/caja/parser/js/AbstractExpression.java Tue Mar 2
14:43:56 2010
@@ -47,5 +47,5 @@
public Boolean conditionResult() { return null; }
- public Expression fold() { return this; }
-}
+ public Expression fold(boolean isFn) { return this; }
+}
=======================================
--- /trunk/src/com/google/caja/parser/js/Expression.java Thu Oct 22
14:07:39 2009
+++ /trunk/src/com/google/caja/parser/js/Expression.java Tue Mar 2
14:43:56 2010
@@ -54,6 +54,12 @@
/**
* This expression or a semantically equivalent simpler expression.
* This method does not recurse.
+ * @param isFn true if the expression is the first operand to a function
call.
+ * Parts of JS are semantics specified in terms of the syntactic
structure
+ * of sub-expressions, such as the value of {...@code this} in a method
call
+ * or the {...@code eval} function/operator distinction. This
parameter is
+ * used to ensure the semantics don't change even when the simplified
+ * sub-expression is called as a function.
*/
- Expression fold();
-}
+ Expression fold(boolean isFn);
+}
=======================================
--- /trunk/src/com/google/caja/parser/js/Operation.java Thu Jan 21 12:58:35
2010
+++ /trunk/src/com/google/caja/parser/js/Operation.java Tue Mar 2 14:43:56
2010
@@ -458,14 +458,22 @@
}
@Override
- public Expression fold() {
+ public Expression fold(boolean isFn) {
if (getOperator() == Operator.FUNCTION_CALL) { return foldCall(); }
+ Expression folded;
switch (children().size()) {
- case 1: return foldUnaryOp();
- case 2: return foldBinaryOp();
- case 3: return foldTernaryOp();
+ case 1: folded = foldUnaryOp(); break;
+ case 2: folded = foldBinaryOp(); break;
+ case 3: folded = foldTernaryOp(); break;
default: return this;
}
+ if (isFn && isFnSpecialForm(folded) && !isFnSpecialForm(this)) {
+ FilePosition pos = getFilePosition();
+ folded = Operation.create(
+ pos, Operator.COMMA,
+ new IntegerLiteral(FilePosition.startOf(pos), 0), folded);
+ }
+ return folded;
}
private static final Expression[] NO_EXPRESSIONS = new Expression[0];
@@ -592,6 +600,8 @@
((StringLiteral) left).getUnquotedValue().length());
}
}
+ } else if (op == Operator.COMMA) {
+ if (left.simplifyForSideEffect() == null) { return right; }
}
Object lhs = toLiteralValue(left);
Object rhs = toLiteralValue(right);
@@ -797,4 +807,16 @@
}
return false;
}
-}
+
+ private static boolean isFnSpecialForm(Expression e) {
+ if (e instanceof Reference) {
+ Reference r = (Reference) e;
+ return "eval".equals(r.getIdentifierName());
+ } else if (e instanceof Operation) {
+ Operator op = ((Operation) e).getOperator();
+ return Operator.MEMBER_ACCESS == op || Operator.SQUARE_BRACKET == op;
+ } else {
+ return false;
+ }
+ }
+}
=======================================
--- /trunk/tests/com/google/caja/ancillary/opt/ParseTreeKBTest.java Mon Nov
30 20:13:04 2009
+++ /trunk/tests/com/google/caja/ancillary/opt/ParseTreeKBTest.java Tue
Mar 2 14:43:56 2010
@@ -942,7 +942,7 @@
private void addFact(String expr, String value) throws ParseException {
kb.addFact(jsExpr(fromString(expr)),
- Fact.is((Literal) jsExpr(fromString(value)).fold()));
+ Fact.is((Literal) jsExpr(fromString(value)).fold(false)));
}
private void addFuzzyFact(String expr, boolean truthy) throws
ParseException {
=======================================
--- /trunk/tests/com/google/caja/parser/js/ExpressionTest.java Thu Jan 21
12:58:35 2010
+++ /trunk/tests/com/google/caja/parser/js/ExpressionTest.java Tue Mar 2
14:43:56 2010
@@ -282,6 +282,14 @@
assertFolded(
"(function () {\n arguments.callee();\n })()",
"(function () { arguments.callee(); })()");
+ assertFolded("eval", "0,eval", false);
+ assertFolded("0, eval", "0,eval", true);
+ assertFolded("foo.bar", "0,foo.bar", false);
+ assertFolded("0, foo.bar", "0,foo.bar", true);
+ assertFolded("foo[ bar ]", "0,foo[bar]", false);
+ assertFolded("0, foo[ bar ]", "0,foo[bar]", true);
+ assertFolded("foo[ 'bar' ]", "0,foo['bar']", false);
+ assertFolded("0, foo[ 'bar' ]", "0,foo['bar']", true);
}
public final void testToInt32() {
@@ -371,20 +379,25 @@
}
private void assertFolded(String result, String expr) throws
ParseException {
+ assertFolded(result, expr, false);
+ }
+
+ private void assertFolded(String result, String expr, boolean isFn)
+ throws ParseException {
Expression input = jsExpr(fromString(expr));
- if (input instanceof Operation) {
- Operation op = (Operation) input;
- for (Expression operand : op.children()) {
- // Fold some operands so we can test negative numbers.
- if ((Operation.is(operand, Operator.NEGATION)
- // and so that we can test corner cases around NaN and
Infinity.
- || Operation.is(operand, Operator.DIVISION))
- && operand.children().get(0) instanceof NumberLiteral) {
- op.replaceChild(operand.fold(), operand);
- }
+ if (input instanceof Operation) {
+ Operation op = (Operation) input;
+ for (Expression operand : op.children()) {
+ // Fold some operands so we can test negative numbers.
+ if ((Operation.is(operand, Operator.NEGATION)
+ // and so that we can test corner cases around NaN and
Infinity.
+ || Operation.is(operand, Operator.DIVISION))
+ && operand.children().get(0) instanceof NumberLiteral) {
+ op.replaceChild(operand.fold(false), operand);
}
}
- Expression actual = input.fold();
+ }
+ Expression actual = input.fold(isFn);
assertEquals(expr, result, actual != null ? render(actual) : null);
}
}
