Revision: 10432
Author: jbrosenb...@google.com
Date: Fri Jul 8 10:47:24 2011
Log: 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
http://code.google.com/p/google-web-toolkit/source/detail?r=10432
Modified:
/trunk/dev/core/src/com/google/gwt/dev/js/JsInliner.java
/trunk/dev/core/test/com/google/gwt/dev/js/JsInlinerTest.java
=======================================
--- /trunk/dev/core/src/com/google/gwt/dev/js/JsInliner.java Tue May 31
05:03:27 2011
+++ /trunk/dev/core/src/com/google/gwt/dev/js/JsInliner.java Fri Jul 8
10:47:24 2011
@@ -762,6 +762,47 @@
return v.refersToName();
}
}
+
+ /**
+ * 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
@@ -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,10 +1170,16 @@
* 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;
@@ -1140,6 +1188,11 @@
hoisted.add(h);
}
}
+
+ /*
+ * 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
@@ -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) {
=======================================
--- /trunk/dev/core/test/com/google/gwt/dev/js/JsInlinerTest.java Tue May
31 05:03:27 2011
+++ /trunk/dev/core/test/com/google/gwt/dev/js/JsInlinerTest.java Fri Jul
8 10:47:24 2011
@@ -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);
}
}
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
