Fixes a bug with JsStaticEval that was preventing unreachable functions from being declared. Gotta love JavaScript, eh?
Found by: cromwellian
Review by: spoon
git-svn-id: https://google-web-toolkit.googlecode.com/svn/trunk@2392 8db76d5a-ed1c-0410-87a9-c151d255dfc7
diff --git a/dev/core/src/com/google/gwt/dev/js/JsInliner.java b/dev/core/src/com/google/gwt/dev/js/JsInliner.java
index 3d40811..d6ae5ac 100644
--- a/dev/core/src/com/google/gwt/dev/js/JsInliner.java
+++ b/dev/core/src/com/google/gwt/dev/js/JsInliner.java
@@ -839,6 +839,22 @@
boolean sawReturnStatement = false;
for (JsStatement statement : statements) {
+ if (sawReturnStatement) {
+ /*
+ * We've already seen a return statement, but there are still more
+ * statements. The target is unsafe to inline, so bail. Note: in most
+ * cases JsStaticEval will have removed any statements following a
+ * return statement.
+ *
+ * The reason we have to bail is that the return statement's
+ * expression MUST be the last thing evaluated.
+ *
+ * TODO(bobv): maybe it could still be inlined with smart
+ * transformation?
+ */
+ return;
+ }
+
/*
* Create replacement expressions to use in place of the original
* statements. It is important that the replacement is newly-minted and
@@ -857,7 +873,6 @@
if (isReturnStatement(statement)) {
sawReturnStatement = true;
- break;
}
}
@@ -1461,6 +1476,9 @@
// Extract the return value
JsReturn ret = (JsReturn) statement;
expression = ret.getExpr();
+ if (expression == null) {
+ expression = program.getUndefinedLiteral();
+ }
} else if (statement instanceof JsVars) {
// Create a comma expression for variable initializers
diff --git a/dev/core/src/com/google/gwt/dev/js/JsStaticEval.java b/dev/core/src/com/google/gwt/dev/js/JsStaticEval.java
index 48fa96e..daaa3b3 100644
--- a/dev/core/src/com/google/gwt/dev/js/JsStaticEval.java
+++ b/dev/core/src/com/google/gwt/dev/js/JsStaticEval.java
@@ -1,5 +1,5 @@
/*
- * Copyright 2007 Google Inc.
+ * Copyright 2008 Google Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License"); you may not
* use this file except in compliance with the License. You may obtain a copy of
@@ -27,14 +27,22 @@
import com.google.gwt.dev.js.ast.JsExprStmt;
import com.google.gwt.dev.js.ast.JsExpression;
import com.google.gwt.dev.js.ast.JsFor;
+import com.google.gwt.dev.js.ast.JsFunction;
import com.google.gwt.dev.js.ast.JsIf;
import com.google.gwt.dev.js.ast.JsModVisitor;
import com.google.gwt.dev.js.ast.JsProgram;
import com.google.gwt.dev.js.ast.JsStatement;
+import com.google.gwt.dev.js.ast.JsVars;
import com.google.gwt.dev.js.ast.JsVisitor;
import com.google.gwt.dev.js.ast.JsWhile;
+import com.google.gwt.dev.js.ast.JsVars.JsVar;
+import java.util.ArrayList;
+import java.util.HashSet;
import java.util.List;
+import java.util.ListIterator;
+import java.util.Set;
+import java.util.Stack;
/**
* Removes JsFunctions that are never referenced in the program.
@@ -68,6 +76,140 @@
}
/**
+ * Force all functions to be evaluated at the top of the lexical scope in
+ * which they reside. This makes {@link StaticEvalVisitor} simpler in that we
+ * no longer have to worry about function declarations within expressions.
+ * After this runs, only statements can contain declarations. Moved functions
+ * will end up just before the statement in which they presently reside.
+ */
+ private class EvalFunctionsAtTopScope extends JsModVisitor {
+ private final Set<JsFunction> dontMove = new HashSet<JsFunction>();
+ private final Stack<JsBlock> scopeStack = new Stack<JsBlock>();
+ private final Stack<ListIterator<JsStatement>> itrStack = new Stack<ListIterator<JsStatement>>();
+
+ @Override
+ public void endVisit(JsFunction x, JsContext<JsExpression> ctx) {
+ scopeStack.pop();
+ }
+
+ @Override
+ public void endVisit(JsProgram x, JsContext<JsProgram> ctx) {
+ scopeStack.pop();
+ }
+
+ @Override
+ public boolean visit(JsBlock x, JsContext<JsStatement> ctx) {
+ if (x == scopeStack.peek()) {
+ ListIterator<JsStatement> itr = x.getStatements().listIterator();
+ itrStack.push(itr);
+ while (itr.hasNext()) {
+ JsStatement stmt = itr.next();
+ JsFunction func = isFunctionDecl(stmt);
+ // Already at the top level.
+ if (func != null) {
+ dontMove.add(func);
+ }
+ accept(stmt);
+ if (func != null) {
+ dontMove.remove(func);
+ }
+ }
+ itrStack.pop();
+ // Already visited.
+ return false;
+ } else {
+ // Just do normal visitation.
+ return true;
+ }
+ }
+
+ @Override
+ public boolean visit(JsFunction x, JsContext<JsExpression> ctx) {
+ /*
+ * We do this during visit() to preserve first-to-last evaluation order.
+ */
+ if (x.getName() != null && !dontMove.contains(x)) {
+ /*
+ * Reinsert this function into the statement immediately before the
+ * current statement. The current statement will have already been
+ * returned from the current iterator's next(), so we have to
+ * backshuffle one step to get in front of it.
+ */
+ ListIterator<JsStatement> itr = itrStack.peek();
+ itr.previous();
+ itr.add(x.makeStmt());
+ itr.next();
+ ctx.replaceMe(x.getName().makeRef());
+ }
+
+ // Dive into the function itself.
+ scopeStack.push(x.getBody());
+ return true;
+ }
+
+ @Override
+ public boolean visit(JsProgram x, JsContext<JsProgram> ctx) {
+ scopeStack.push(x.getGlobalBlock());
+ return true;
+ }
+ }
+
+ /**
+ * Creates a minimalist list of statements that must be run in order to
+ * achieve the same declaration effect as the visited statements.
+ *
+ * For example, a JsFunction declaration should be run as a JsExprStmt. JsVars
+ * should be run without any initializers.
+ *
+ * This visitor is called from
+ * {@link StaticEvalVisitor#ensureDeclarations(JsStatement)} on any statements
+ * that are removed from a function.
+ */
+ private static class MustExecVisitor extends JsVisitor {
+
+ private final List<JsStatement> mustExec = new ArrayList<JsStatement>();
+
+ public MustExecVisitor() {
+ }
+
+ @Override
+ public void endVisit(JsExprStmt x, JsContext<JsStatement> ctx) {
+ JsFunction func = isFunctionDecl(x);
+ if (func != null) {
+ mustExec.add(x);
+ }
+ }
+
+ @Override
+ public void endVisit(JsVars x, JsContext<JsStatement> ctx) {
+ JsVars strippedVars = new JsVars();
+ boolean mustReplace = false;
+ for (JsVar var : x) {
+ JsVar strippedVar = new JsVar(var.getName());
+ strippedVars.add(strippedVar);
+ if (var.getInitExpr() != null) {
+ mustReplace = true;
+ }
+ }
+ if (mustReplace) {
+ mustExec.add(strippedVars);
+ } else {
+ mustExec.add(x);
+ }
+ }
+
+ public List<JsStatement> getStatements() {
+ return mustExec;
+ }
+
+ @Override
+ public boolean visit(JsFunction x, JsContext<JsExpression> ctx) {
+ // Don't dive into nested functions.
+ return false;
+ }
+ }
+
+ /**
* Does static evals.
*
* TODO: borrow more concepts from
@@ -118,8 +260,17 @@
if (stmt.unconditionalControlBreak()) {
// Abrupt change in flow, chop the remaining items from this block
for (int j = i + 1; j < stmts.size();) {
- stmts.remove(j);
- didChange = true;
+ JsStatement toRemove = stmts.get(j);
+ JsStatement toReplace = ensureDeclarations(toRemove);
+ if (toReplace == null) {
+ stmts.remove(j);
+ didChange = true;
+ } else if (toReplace == toRemove) {
+ ++j;
+ } else {
+ stmts.set(j, toReplace);
+ didChange = true;
+ }
}
}
}
@@ -169,7 +320,7 @@
JsBlock block = new JsBlock();
block.getStatements().add(x.getBody());
block.getStatements().add(expr.makeStmt());
- ctx.replaceMe(block);
+ ctx.replaceMe(accept(block));
}
}
}
@@ -205,7 +356,11 @@
block.getStatements().add(x.getInitVars());
}
block.getStatements().add(expr.makeStmt());
- ctx.replaceMe(block);
+ JsStatement decls = ensureDeclarations(x.getBody());
+ if (decls != null) {
+ block.getStatements().add(decls);
+ }
+ ctx.replaceMe(accept(block));
}
}
}
@@ -221,10 +376,13 @@
if (expr instanceof CanBooleanEval) {
CanBooleanEval cond = (CanBooleanEval) expr;
JsStatement onlyStmtToExecute;
+ JsStatement removed;
if (cond.isBooleanTrue()) {
onlyStmtToExecute = thenStmt;
+ removed = elseStmt;
} else if (cond.isBooleanFalse()) {
onlyStmtToExecute = elseStmt;
+ removed = thenStmt;
} else {
return;
}
@@ -236,7 +394,11 @@
block.getStatements().add(onlyStmtToExecute);
}
- ctx.replaceMe(block);
+ JsStatement decls = ensureDeclarations(removed);
+ if (decls != null) {
+ block.getStatements().add(decls);
+ }
+ ctx.replaceMe(accept(block));
} else if (isEmpty(thenStmt) && isEmpty(elseStmt)) {
ctx.replaceMe(expr.makeStmt());
}
@@ -253,10 +415,46 @@
// If false, replace with condition.
if (cond.isBooleanFalse()) {
- ctx.replaceMe(expr.makeStmt());
+ JsBlock block = new JsBlock();
+ block.getStatements().add(expr.makeStmt());
+ JsStatement decls = ensureDeclarations(x.getBody());
+ if (decls != null) {
+ block.getStatements().add(decls);
+ }
+ ctx.replaceMe(accept(block));
}
}
}
+
+ /**
+ * This method MUST be called whenever any statements are removed from a
+ * function. This is because some statements, such as JsVars or JsFunction
+ * have the effect of defining local variables, no matter WHERE they are in
+ * the function. The returned statement (if any), must be executed. It is
+ * also possible for stmt to be directly returned, in which case the caller
+ * should not perform AST changes that would cause an infinite optimization
+ * loop.
+ *
+ * Note: EvalFunctionsAtTopScope will have changed any JsFunction
+ * declarations into statements before this visitor runs.
+ */
+ private JsStatement ensureDeclarations(JsStatement stmt) {
+ if (stmt == null) {
+ return null;
+ }
+ MustExecVisitor mev = new MustExecVisitor();
+ mev.accept(stmt);
+ List<JsStatement> stmts = mev.getStatements();
+ if (stmts.isEmpty()) {
+ return null;
+ } else if (stmts.size() == 1) {
+ return stmts.get(0);
+ } else {
+ JsBlock jsBlock = new JsBlock();
+ jsBlock.getStatements().addAll(stmts);
+ return jsBlock;
+ }
+ }
}
public static boolean exec(JsProgram program) {
@@ -271,6 +469,24 @@
}
/**
+ * If the statement is a JsExprStmt that declares a function with no other
+ * side effects, returns that function; otherwise <code>null</code>.
+ */
+ protected static JsFunction isFunctionDecl(JsStatement stmt) {
+ if (stmt instanceof JsExprStmt) {
+ JsExprStmt exprStmt = (JsExprStmt) stmt;
+ JsExpression expr = exprStmt.getExpression();
+ if (expr instanceof JsFunction) {
+ JsFunction func = (JsFunction) expr;
+ if (func.getName() != null) {
+ return func;
+ }
+ }
+ }
+ return null;
+ }
+
+ /**
* Simplify short circuit AND expressions.
*
* <pre>
@@ -324,10 +540,11 @@
}
public boolean execImpl() {
- // Remove the unused functions from the JsProgram
- StaticEvalVisitor evalVisitor = new StaticEvalVisitor();
- evalVisitor.accept(program);
+ EvalFunctionsAtTopScope fev = new EvalFunctionsAtTopScope();
+ fev.accept(program);
+ StaticEvalVisitor sev = new StaticEvalVisitor();
+ sev.accept(program);
- return evalVisitor.didChange();
+ return fev.didChange() || sev.didChange();
}
}
diff --git a/user/test/com/google/gwt/dev/jjs/CompilerSuite.java b/user/test/com/google/gwt/dev/jjs/CompilerSuite.java
index 37775c7..7df07d3 100644
--- a/user/test/com/google/gwt/dev/jjs/CompilerSuite.java
+++ b/user/test/com/google/gwt/dev/jjs/CompilerSuite.java
@@ -27,6 +27,7 @@
import com.google.gwt.dev.jjs.test.HostedTest;
import com.google.gwt.dev.jjs.test.InnerClassTest;
import com.google.gwt.dev.jjs.test.InnerOuterSuperTest;
+import com.google.gwt.dev.jjs.test.JsStaticEvalTest;
import com.google.gwt.dev.jjs.test.JsniConstructorTest;
import com.google.gwt.dev.jjs.test.JsoTest;
import com.google.gwt.dev.jjs.test.MemberShadowingTest;
@@ -63,6 +64,7 @@
suite.addTestSuite(InnerOuterSuperTest.class);
suite.addTestSuite(JsniConstructorTest.class);
suite.addTestSuite(JsoTest.class);
+ suite.addTestSuite(JsStaticEvalTest.class);
suite.addTestSuite(MemberShadowingTest.class);
suite.addTestSuite(MethodBindTest.class);
suite.addTestSuite(MethodCallTest.class);
diff --git a/user/test/com/google/gwt/dev/jjs/test/JsStaticEvalTest.java b/user/test/com/google/gwt/dev/jjs/test/JsStaticEvalTest.java
new file mode 100644
index 0000000..a4e5ac2
--- /dev/null
+++ b/user/test/com/google/gwt/dev/jjs/test/JsStaticEvalTest.java
@@ -0,0 +1,220 @@
+/*
+ * Copyright 2008 Google Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License"); you may not
+ * use this file except in compliance with the License. You may obtain a copy of
+ * the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package com.google.gwt.dev.jjs.test;
+
+import com.google.gwt.core.client.GWT;
+import com.google.gwt.junit.client.GWTTestCase;
+
+/**
+ * Tests that declarations in pruned code still happen. NOTE: this test does not
+ * run in hosted mode due to browser inconsistencies; however it should run in
+ * web mode due to our normalizations.
+ */
+public class JsStaticEvalTest extends GWTTestCase {
+
+ @Override
+ public String getModuleName() {
+ return "com.google.gwt.dev.jjs.CompilerSuite";
+ }
+
+ public void testAfterReturn() {
+ if (GWT.isScript()) {
+ doTestAfterReturn();
+ }
+ }
+
+ public void testAfterReturnInBlock() {
+ if (GWT.isScript()) {
+ doTestAfterReturnInBlock();
+ }
+ }
+
+ public void testConditionalFalse() {
+ if (GWT.isScript()) {
+ doTestConditionalFalse();
+ }
+ }
+
+ public void testConditionalTrue() {
+ if (GWT.isScript()) {
+ doTestConditionalTrue();
+ }
+ }
+
+ public void testForFalse() {
+ if (GWT.isScript()) {
+ doTestForFalse();
+ }
+ }
+
+ public void testIfFalse() {
+ if (GWT.isScript()) {
+ doTestIfFalse();
+ }
+ }
+
+ public void testOrder1() {
+ if (GWT.isScript()) {
+ doTestOrder1();
+ }
+ }
+
+ public void testOrder2() {
+ if (GWT.isScript()) {
+ doTestOrder2();
+ }
+ }
+
+ public void testShortCircuitAnd() {
+ if (GWT.isScript()) {
+ doTestShortCircuitAnd();
+ }
+ }
+
+ public void testShortCircuitOr() {
+ if (GWT.isScript()) {
+ doTestShortCircuitOr();
+ }
+ }
+
+ public void testWhileFalse() {
+ if (GWT.isScript()) {
+ doTestWhileFalse();
+ }
+ }
+
+ private native void doTestAfterReturn() /*-{
+ func();
+ @junit.framework.Assert::assertTrue(Z)(result);
+ return;
+ var result;
+ function func() {
+ result = true;
+ }
+ }-*/;
+
+ private native void doTestAfterReturnInBlock() /*-{
+ func();
+ @junit.framework.Assert::assertTrue(Z)(result);
+ return;
+ {
+ var result;
+ function func() {
+ result = true;
+ }
+ }
+ }-*/;
+
+ private native void doTestConditionalFalse() /*-{
+ func();
+ @junit.framework.Assert::assertTrue(Z)(result);
+ return;
+ var result;
+ var toss = false ? function func() {
+ result = true;
+ } : false;
+ }-*/;
+
+ private native void doTestConditionalTrue() /*-{
+ func();
+ @junit.framework.Assert::assertTrue(Z)(result);
+ return;
+ var result;
+ var toss = true ? true : function func() {
+ result = true;
+ };
+ }-*/;
+
+ private native void doTestForFalse() /*-{
+ func();
+ @junit.framework.Assert::assertTrue(Z)(result);
+ return;
+ for (;false;) {
+ var result;
+ function func() {
+ result = true;
+ }
+ }
+ }-*/;
+
+ private native void doTestIfFalse() /*-{
+ func();
+ @junit.framework.Assert::assertTrue(Z)(result);
+ return;
+ if (false) {
+ var result;
+ function func() {
+ result = true;
+ }
+ }
+ }-*/;
+
+ private native void doTestOrder1() /*-{
+ func();
+ @junit.framework.Assert::assertTrue(Z)(result);
+ return;
+ var result;
+ var toss = function func() {
+ };
+ var toss = function func() {
+ result = true;
+ };
+ }-*/;
+
+ private native void doTestOrder2() /*-{
+ var toss = function func() {
+ };
+ func();
+ @junit.framework.Assert::assertTrue(Z)(result);
+ return;
+ var result;
+ var toss = function func() {
+ result = true;
+ };
+ }-*/;
+
+ private native void doTestShortCircuitAnd() /*-{
+ func();
+ @junit.framework.Assert::assertTrue(Z)(result);
+ return;
+ var result;
+ var toss = false && function func() {
+ result = true;
+ };
+ }-*/;
+
+ private native void doTestShortCircuitOr() /*-{
+ func();
+ @junit.framework.Assert::assertTrue(Z)(result);
+ return;
+ var result;
+ var toss = true || function func() {
+ result = true;
+ };
+ }-*/;
+
+ private native void doTestWhileFalse() /*-{
+ func();
+ @junit.framework.Assert::assertTrue(Z)(result);
+ return;
+ while (false) {
+ var result;
+ function func() {
+ result = true;
+ }
+ }
+ }-*/;
+}
