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; + } + } + }-*/; +}