Fix JsInliner, to properly handle name scope tracking after multi-level inlining. Was causing local variable name collisions after obfuscation. Fixes issue 5936. Review at http://gwt-code-reviews.appspot.com/1472803 git-svn-id: https://google-web-toolkit.googlecode.com/svn/trunk@10432 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 00c2e0a..fc1fa40 100644 --- a/dev/core/src/com/google/gwt/dev/js/JsInliner.java +++ b/dev/core/src/com/google/gwt/dev/js/JsInliner.java
@@ -764,6 +764,47 @@ } /** + * Collect names in a hoisted statement that are local to the original + * scope. These names will need to be copied to the destination scope + * once the statement becomes hoisted. + */ + private static class HoistedNameVisitor extends JsVisitor { + private final JsScope toScope; + private final JsScope fromScope; + private final List<JsName> hoistedNames; + + public HoistedNameVisitor(JsScope toScope, JsScope fromScope) { + this.toScope = toScope; + this.fromScope = fromScope; + this.hoistedNames = new ArrayList<JsName>(); + } + + public List<JsName> getHoistedNames() { + return hoistedNames; + } + + /* + * We need to hoist names that are only visible in fromScope, but not in + * toScope (i.e. we don't want to hoist names that are visible to both + * scopes, such as a global). Also, we don't want to hoist names that have a + * staticRef, which indicates a formal parameter, or a function name. + */ + @Override + public boolean visit(JsNameRef nameRef, JsContext ctx) { + JsName name = nameRef.getName(); + JsName fromScopeName = fromScope.findExistingName(name.getIdent()); + JsName toScopeName = toScope.findExistingName(name.getIdent()); + if (name.getStaticRef() == null + && name == fromScopeName + && name != toScopeName + && !hoistedNames.contains(name)) { + hoistedNames.add(name); + } + return true; + } + } + + /** * Collect all of the idents used in an AST node. The collector can be * configured to collect idents from qualified xor unqualified JsNameRefs. */ @@ -1097,10 +1138,11 @@ statements = Collections.emptyList(); } - List<JsExpression> hoisted = new ArrayList<JsExpression>( - statements.size()); + List<JsExpression> hoisted = new ArrayList<JsExpression>(statements.size()); JsExpression thisExpr = ((JsNameRef) x.getQualifier()).getQualifier(); - List<JsName> localVariableNames = new ArrayList<JsName>(); + HoistedNameVisitor hoistedNameVisitor = + new HoistedNameVisitor(callerFunction.getScope(), invokedFunction.getScope()); + boolean sawReturnStatement = false; for (JsStatement statement : statements) { @@ -1128,11 +1170,17 @@ * distinct objects, it would not be possible to substitute different * JsNameRefs at different call sites. */ - JsExpression h = hoistedExpression(statement, localVariableNames); + JsExpression h = hoistedExpression(statement); if (h == null) { return x; } + /* + * Visit the statement to find names that will be moved to the caller's + * scope from the invoked function. + */ + hoistedNameVisitor.accept(statement); + if (isReturnStatement(statement)) { sawReturnStatement = true; hoisted.add(h); @@ -1142,6 +1190,11 @@ } /* + * Get the referenced names that need to be copied to the caller's scope. + */ + List<JsName> hoistedNames = hoistedNameVisitor.getHoistedNames(); + + /* * If the inlined method has no return statement, synthesize an undefined * reference. It will be reclaimed if the method call is from a * JsExprStmt. @@ -1179,7 +1232,7 @@ // Perform the name replacement NameRefReplacerVisitor v = new NameRefReplacerVisitor(thisExpr, x.getArguments(), invokedFunction.getParameters()); - for (ListIterator<JsName> nameIterator = localVariableNames.listIterator(); nameIterator.hasNext();) { + for (ListIterator<JsName> nameIterator = hoistedNames.listIterator(); nameIterator.hasNext();) { JsName name = nameIterator.next(); /* @@ -1210,7 +1263,7 @@ op = v.accept(op); // Normalize any nested comma expressions that we may have generated. - op = (new CommaNormalizer(localVariableNames)).accept(op); + op = (new CommaNormalizer(hoistedNames)).accept(op); /* * Compare the relative complexity of the original invocation versus the @@ -1224,13 +1277,13 @@ return x; } - if (callerFunction == programFunction && localVariableNames.size() > 0) { + if (callerFunction == programFunction && hoistedNames.size() > 0) { // Don't add additional variables to the top-level program. return x; } // We've committed to the inlining, ensure the vars are created - newLocalVariableStack.peek().addAll(localVariableNames); + newLocalVariableStack.peek().addAll(hoistedNames); // update invocation counts according to this inlining invocationCountingVisitor.removeCountsFor(x); @@ -1558,7 +1611,6 @@ private static class RefersToNameVisitor extends JsVisitor { private final Collection<JsName> names; private boolean refersToName; - private boolean refersToUnbound; public RefersToNameVisitor(Collection<JsName> names) { this.names = names; @@ -1568,9 +1620,7 @@ public void endVisit(JsNameRef x, JsContext ctx) { JsName name = x.getName(); - if (name == null) { - refersToUnbound = true; - } else { + if (name != null) { refersToName = refersToName || names.contains(name); } } @@ -1578,10 +1628,6 @@ public boolean refersToName() { return refersToName; } - - public boolean refersToUnbound() { - return refersToUnbound; - } } /** @@ -1701,7 +1747,7 @@ /** * @param program - * @return + * @return stats */ private static OptimizerStats execImpl(JsProgram program) { OptimizerStats stats = new OptimizerStats(NAME); @@ -1783,15 +1829,12 @@ * constructs a mutable copy of the expression that can be manipulated * at-will. * - * @param program the enclosing JsProgram * @param statement the statement from which to extract the expressions - * @param localVariableNames accumulates any local varables declared by - * <code>statement</code> * @return a JsExpression representing all expressions that would have been * evaluated by the statement */ - private static JsExpression hoistedExpression(JsStatement statement, - List<JsName> localVariableNames) { + private static JsExpression hoistedExpression(JsStatement statement) { + JsExpression expression; if (statement instanceof JsExprStmt) { // Extract the expression @@ -1815,9 +1858,6 @@ expression = JsNullLiteral.INSTANCE; for (JsVar var : vars) { - // Record the locally-defined variable - localVariableNames.add(var.getName()); - // Extract the initialization expression JsExpression init = var.getInitExpr(); if (init != null) { @@ -1856,7 +1896,7 @@ } /** - * Given an expression, determine if it it is a JsNameRef that refers to a + * Given an expression, determine if it is a JsNameRef that refers to a * statically-defined JsFunction. */ private static JsFunction isFunction(JsExpression e) {
diff --git a/dev/core/test/com/google/gwt/dev/js/JsInlinerTest.java b/dev/core/test/com/google/gwt/dev/js/JsInlinerTest.java index 9bdae0f..ad321ff 100644 --- a/dev/core/test/com/google/gwt/dev/js/JsInlinerTest.java +++ b/dev/core/test/com/google/gwt/dev/js/JsInlinerTest.java
@@ -48,22 +48,22 @@ public void testInlineArrayLiterals() throws Exception { String input = "function a1(arg, x) { arg.x = x; return arg; }" + "function b1() { var x=a1([], 10); } b1();"; - compare(input, input); + verifyNoChange(input); } public void testInlineFunctionLiterals() throws Exception { String input = "function a1(arg, x) { arg.x = x; return arg; }" + "function b1() { var x=a1(function (){}, 10); } b1();"; - compare(input, input); + verifyNoChange(input); String input2 = "function a1(arg, x) { arg.x = x; return arg; }" + "function b1() { var x=a1(function blah(){}, 10); } b1();"; - compare(input2, input2); + verifyNoChange(input2); } public void testInlineObjectLiterals() throws Exception { String input = "function a1(arg, x) { arg.x = x; return arg; }" + "function b1() { var x=a1({}, 10); } b1();"; - compare(input, input); + verifyNoChange(input); } /** @@ -81,7 +81,7 @@ + "function c1() { return ex2? a1():c1(); } c1()"; String expected = "function a1() { return ex ? (ex2 ? a1() : c1()) : c1() }" + "function c1() { return ex2 ? a1() :c1(); } c1()"; - compare(expected, input); + verifyOptimized(expected, input); } /** @@ -102,7 +102,7 @@ // bootstrap the program code.append("caller();"); - compare(code.toString(), code.toString()); + verifyNoChange(code.toString()); } /** @@ -127,7 +127,7 @@ expected.append("function caller() { var array; array[0] + (clinit(), 2); }"); expected.append("caller();"); - compare(expected.toString(), code.toString()); + verifyOptimized(expected.toString(), code.toString()); } /** @@ -147,7 +147,7 @@ // bootstrap the program code.append("caller();"); - compare(code.toString(), code.toString()); + verifyNoChange(code.toString()); } /** @@ -170,7 +170,7 @@ // bootstrap the program code.append("caller();"); - compare(code.toString(), code.toString()); + verifyNoChange(code.toString()); } /** @@ -195,7 +195,7 @@ expected.append("function clinit() { clinit = null; }"); expected.append("function caller() {var y; return y=2,clinit(),3;}"); expected.append("caller();"); - compare(expected.toString(), code.toString()); + verifyOptimized(expected.toString(), code.toString()); } /** @@ -218,7 +218,7 @@ // bootstrap the program code.append("caller();"); - compare(code.toString(), code.toString()); + verifyNoChange(code.toString()); } public void testSelfRecursion() throws Exception { @@ -228,13 +228,58 @@ String expected = "function a1() { return blah && bar && a1() }" + "function c() { a1() } c()"; - compare(expected, input); + verifyOptimized(expected, input); } - private void compare(String expected, String input) throws Exception { - input = optimize(input, JsSymbolResolver.class, FixStaticRefsVisitor.class, + /* + * This is inspired by issue 5936: + * @see http://code.google.com/p/google-web-toolkit/issues/detail?id=5936 + */ + public void testPreserveNameScopeWithDoubleInliningAndObfuscation() throws Exception { + StringBuffer code = new StringBuffer(); + + code.append("function getA(){" + + "var s;" + + "s = getB();" + + "return s;" + + "}"); + + code.append("function getB(){" + + "var t;" + + "t = 't';" + + "t = t + '';" + + "return t;" + + "}"); + + code.append("function start(y){" + + "getA();" + + "if (y != 10) {$wnd.alert('y != 10');}" + + "}"); + + code.append("var x = 10; start(x);"); + + StringBuffer expected = new StringBuffer(); + expected.append("function c(a){var b;b='t';if(a!=10){$wnd.alert('y != 10')}}"); + expected.append("var d=10;c(d);"); + + verifyOptimizedObfuscated(expected.toString(), code.toString()); + } + + private void verifyNoChange(String input) throws Exception { + verifyOptimized(input, input); + } + + private void verifyOptimized(String expected, String input) throws Exception { + String actual = optimize(input, JsSymbolResolver.class, FixStaticRefsVisitor.class, JsInliner.class, JsUnusedFunctionRemover.class); - expected = optimize(expected); - assertEquals(expected, input); + String expectedAfterParse = optimize(expected); + assertEquals(expectedAfterParse, actual); + } + + private void verifyOptimizedObfuscated(String expected, String input) throws Exception { + String actual = optimize(input, JsSymbolResolver.class, FixStaticRefsVisitor.class, + JsInliner.class, JsUnusedFunctionRemover.class, JsObfuscateNamer.class); + String expectedAfterParse = optimize(expected); + assertEquals(expectedAfterParse, actual); } }