Reviewers: Jasvir, metaweta,
Description:
this fixes issue 1452
http://code.google.com/p/google-caja/issues/detail?id=1452
the method reuse() creates a temporary var for an expression, so that
the value can be used multiple times. When reuse() notices the
expression is simple enough, it doesn't create a temporary.
The method reuseAll() is reuse() applied to an argument list.
Currently it just calls reuse() on each argument, which is wrong.
When an argument list is something like this:
o.m(i, i++)
the result is something like this:
var x1___ = i++;
o.m(i, x1___);
This change ensures reuseAll() creates temporaries for all its args
if any of its args needs a temporary.
Please review this at http://codereview.appspot.com/5820071/
Affected files:
M src/com/google/caja/parser/quasiliteral/Rule.java
M tests/com/google/caja/parser/quasiliteral/ES53RewriterTest.java
Index: src/com/google/caja/parser/quasiliteral/Rule.java
===================================================================
--- src/com/google/caja/parser/quasiliteral/Rule.java (revision 4807)
+++ src/com/google/caja/parser/quasiliteral/Rule.java (working copy)
@@ -300,10 +300,11 @@
protected Pair<Expression, Expression> reuse(
ParseTreeNode value, Scope scope) {
Expression rhs = (Expression) rewriter.expand(value, scope);
- if (rhs instanceof Reference || rhs instanceof Literal) {
+ if (noSideEffects(rhs)) {
return new Pair<Expression, Expression>(
rhs, Operation.undefined(FilePosition.UNKNOWN));
}
+
Expression tempRef = new Reference(
scope.declareStartOfScopeTempVariable());
Expression tempInit = (Expression) QuasiBuilder.substV(
@@ -313,6 +314,17 @@
return new Pair<Expression, Expression>(tempRef, tempInit);
}
+ private boolean noSideEffects(Expression e) {
+ if (e instanceof Reference) {
+ return true;
+ } else if (e instanceof Literal) {
+ return true;
+ } else {
+ // conservative answer
+ return false;
+ }
+ }
+
/**
* Returns a pair of a reusable expression list and an initializing
* expression that together represent the reusable expansion of the
@@ -323,19 +335,40 @@
*/
protected Pair<ParseTreeNodeContainer, Expression> reuseAll(
ParseTreeNode arguments, Scope scope) {
+ Expression[] exp = new Expression[arguments.children().size()];
List<ParseTreeNode> refs = Lists.newArrayList();
- Expression[] inits = new Expression[arguments.children().size()];
- for (int i = 0; i < arguments.children().size(); i++) {
- Pair<Expression, Expression> p = reuse(
- arguments.children().get(i), scope);
- refs.add(p.a);
- inits[i] = p.b;
+ boolean pure = true;
+ for (int i = 0; i < exp.length; i++) {
+ ParseTreeNode arg = arguments.children().get(i);
+ exp[i] = (Expression) rewriter.expand(arg, scope);
+ pure = pure && noSideEffects(exp[i]);
}
+ if (pure) {
+ // No arg has side-effects, so we can use the expansions directly.
+ for (int i = 0; i < exp.length; i++) {
+ refs.add(exp[i]);
+ }
+ return new Pair<ParseTreeNodeContainer, Expression>(
+ new ParseTreeNodeContainer(refs),
+ Operation.undefined(FilePosition.UNKNOWN));
+ }
+
+ for (int i = 0; i < exp.length; i++) {
+ Expression tempRef = new Reference(
+ scope.declareStartOfScopeTempVariable());
+ Expression tempInit = (Expression) QuasiBuilder.substV(
+ "@ref = @rhs;",
+ "ref", tempRef,
+ "rhs", exp[i]);
+ refs.add(tempRef);
+ exp[i] = tempInit;
+ }
+
return new Pair<ParseTreeNodeContainer, Expression>(
new ParseTreeNodeContainer(refs),
- commas(inits));
+ commas(exp));
}
/**
Index: tests/com/google/caja/parser/quasiliteral/ES53RewriterTest.java
===================================================================
--- tests/com/google/caja/parser/quasiliteral/ES53RewriterTest.java
(revision 4807)
+++ tests/com/google/caja/parser/quasiliteral/ES53RewriterTest.java
(working copy)
@@ -14,14 +14,6 @@
package com.google.caja.parser.quasiliteral;
-import java.io.IOException;
-import java.net.URI;
-import java.util.Arrays;
-import java.util.EnumSet;
-import java.util.List;
-
-import junit.framework.AssertionFailedError;
-
import com.google.caja.lexer.ExternalReference;
import com.google.caja.lexer.FetchedData;
import com.google.caja.lexer.FilePosition;
@@ -45,10 +37,17 @@
import com.google.caja.reporting.MessageType;
import com.google.caja.reporting.TestBuildInfo;
import com.google.caja.util.Executor;
-import com.google.caja.util.FailureIsAnOption;
import com.google.caja.util.Lists;
import com.google.caja.util.RhinoTestBed;
+import java.io.IOException;
+import java.net.URI;
+import java.util.Arrays;
+import java.util.EnumSet;
+import java.util.List;
+
+import junit.framework.AssertionFailedError;
+
public class ES53RewriterTest extends CommonJsRewriterTestCase {
protected class TestUriFetcher implements UriFetcher {
public FetchedData fetch(ExternalReference ref, String mimeType)
@@ -201,25 +200,41 @@
assertConsistent("({ x: 1, y: 2 });");
}
- //TODO(erights): Fix these tests to test for the output we now expect.
- @FailureIsAnOption
+ public final void testFunctionArgsWithSideEffects() throws Exception {
+ assertConsistent(
+ "(function() {\n" +
+ " function f(a, b) { return a + ',' + b; }\n" +
+ " var i = 0;\n" +
+ " return f(i, i++);\n" +
+ "})();");
+ }
+
+ public final void testMethodArgsWithSideEffects() throws Exception {
+ assertConsistent(
+ "(function () {\n" +
+ " var o = { f: function(a, b) { return a + ',' + b; } };\n" +
+ " var i = 0;\n" +
+ " return o.f(i, i++);\n" +
+ "})();");
+ }
+
public final void testFunctionToStringCall() throws Exception {
rewriteAndExecute(
"function foo() {}\n"
- + "assertEquals('function foo() {\\n [cajoled code]\\n}',\n"
+ + "assertEquals('\\nfunction foo() {\\n}\\n',\n"
+ " foo.toString());");
rewriteAndExecute(
- "function foo (a, b) { xx; }\n"
- + "assertEquals('function foo(a, b) {\\n [cajoled code]\\n}',\n"
+ "function foo (a, b) { 1; }\n"
+ + "assertEquals('\\nfunction foo(a, b) {\\n 1;\\n}\\n',\n"
+ " foo.toString());");
rewriteAndExecute(
"function foo() {}\n"
- + "assertEquals('function foo() {\\n [cajoled code]\\n}',\n"
+ + "assertEquals('\\nfunction foo() {\\n}\\n',\n"
+ " Function.prototype.toString.call(foo));");
rewriteAndExecute(
"var foo = function (x$x, y_y) {};\n"
+ "assertEquals("
- + " 'function foo$_var(x$x, y_y) {\\n [cajoled code]\\n}',\n"
+ + " '\\nfunction foo$_var(x$x, y_y) {\\n}\\n',\n"
+ " Function.prototype.toString.call(foo));");
}
