Fix nullness analysis for JSOs and propagate through casts.
For a long time our library and some use code has been relying
on the absence of NPE on null JSO dispatches. Subtle changes to
the optimization framework (like propagating nullness information
through casts) break due to code following this assumption.
This patches makes JSO types more conservative, by not letting
the compiler model them as non null, and improves optimizations
by propagating nullness through casts.
Change-Id: I202d889ef5b686b272aa95a5dd9ddf1e8644f938
diff --git a/dev/core/src/com/google/gwt/dev/jjs/ast/JCastOperation.java b/dev/core/src/com/google/gwt/dev/jjs/ast/JCastOperation.java
index 4fd6dd1..8c3de56 100644
--- a/dev/core/src/com/google/gwt/dev/jjs/ast/JCastOperation.java
+++ b/dev/core/src/com/google/gwt/dev/jjs/ast/JCastOperation.java
@@ -41,6 +41,11 @@
@Override
public JType getType() {
+ if (!expr.getType().canBeNull() && !castType.isNullType()) {
+ // Strengthen type to non null unless it has been determined that the type is not instantiable
+ // (and that is reflected by replacing the cast type by the null type).
+ return castType.strengthenToNonNull();
+ }
return castType;
}
diff --git a/dev/core/src/com/google/gwt/dev/jjs/ast/JPrimitiveType.java b/dev/core/src/com/google/gwt/dev/jjs/ast/JPrimitiveType.java
index c13dae2..8cd9505 100644
--- a/dev/core/src/com/google/gwt/dev/jjs/ast/JPrimitiveType.java
+++ b/dev/core/src/com/google/gwt/dev/jjs/ast/JPrimitiveType.java
@@ -128,6 +128,11 @@
return null;
}
+ @Override
+ public JPrimitiveType strengthenToNonNull() {
+ return this;
+ }
+
public String getWrapperTypeName() {
return wrapperTypeName;
}
diff --git a/dev/core/src/com/google/gwt/dev/jjs/ast/JProgram.java b/dev/core/src/com/google/gwt/dev/jjs/ast/JProgram.java
index fda68a7..4e37f8b 100644
--- a/dev/core/src/com/google/gwt/dev/jjs/ast/JProgram.java
+++ b/dev/core/src/com/google/gwt/dev/jjs/ast/JProgram.java
@@ -515,9 +515,16 @@
return JReferenceType.NULL_TYPE;
}
- if (thisType.canBeNull() != thatType.canBeNull()) {
- // If either is non-nullable, the result should be non-nullable.
- return strengthenType(thisType.strengthenToNonNull(), thatType.strengthenToNonNull());
+ if (!thisType.canBeNull() || !thatType.canBeNull()) {
+ JReferenceType thisTypeNonNull = thisType.strengthenToNonNull();
+ JReferenceType thatTypeNonNull = thatType.strengthenToNonNull();
+ // .strengthenToNonNull does not guarantee that the resulting type is non null (e.g. JSOs).
+
+ // If either is non-nullable, the result should be non-nullable, unless it is a type that
+ // can not be made non-nullable, like a JSO.
+ if (thisType != thisTypeNonNull || thatType != thatTypeNonNull) {
+ return strengthenType(thisTypeNonNull, thatTypeNonNull);
+ }
}
if (typeOracle.castSucceedsTrivially(thisType, thatType)) {
diff --git a/dev/core/src/com/google/gwt/dev/jjs/ast/JReferenceType.java b/dev/core/src/com/google/gwt/dev/jjs/ast/JReferenceType.java
index 8398ab3..886c80d 100644
--- a/dev/core/src/com/google/gwt/dev/jjs/ast/JReferenceType.java
+++ b/dev/core/src/com/google/gwt/dev/jjs/ast/JReferenceType.java
@@ -325,7 +325,7 @@
}
public JReferenceType weakenToNonExact() {
- if (getUnderlyingType() == this || !getUnderlyingType().canBeSubclass()) {
+ if (getUnderlyingType() == this) {
// Underlying types cannot be weakened.
return this;
}
@@ -343,7 +343,12 @@
throw new AssertionError("Unknown AnalysisResult " + getAnalysisResult().toString());
}
+ @Override
public JReferenceType strengthenToNonNull() {
+ if (isJsoType()) {
+ // JSOs can not be strengthened.
+ return this;
+ }
switch (getAnalysisResult()) {
case NULLABLE_NOT_EXACT:
return getAnalysisDecoratedTypePool().getAnalysisDecoratedType(
@@ -360,7 +365,7 @@
public JReferenceType strengthenToExact() {
if (isJsoType()) {
- // JSOs can not be strengthened to EXACT.
+ // JSOs can not be strengthened.
return this;
}
switch (getAnalysisResult()) {
diff --git a/dev/core/src/com/google/gwt/dev/jjs/ast/JType.java b/dev/core/src/com/google/gwt/dev/jjs/ast/JType.java
index a645b8b..7a62af4 100644
--- a/dev/core/src/com/google/gwt/dev/jjs/ast/JType.java
+++ b/dev/core/src/com/google/gwt/dev/jjs/ast/JType.java
@@ -95,6 +95,13 @@
*/
public abstract boolean canBeSubclass();
+ /**
+ * Returns a non nullable version of this type if possible.
+ *
+ * @see JAnalysisDecoratedType
+ */
+ public abstract JType strengthenToNonNull();
+
public abstract JLiteral getDefaultValue();
public abstract String getJavahSignatureName();
diff --git a/user/test/com/google/gwt/dev/jjs/optimized/CastOptimizationTest.java b/user/test/com/google/gwt/dev/jjs/optimized/CastOptimizationTest.java
index 1891953..500b198 100644
--- a/user/test/com/google/gwt/dev/jjs/optimized/CastOptimizationTest.java
+++ b/user/test/com/google/gwt/dev/jjs/optimized/CastOptimizationTest.java
@@ -42,6 +42,8 @@
private static Object field;
+ private static int randomNumber = new Random().nextInt(42);
+
@Override
protected void gwtSetUp() throws Exception {
field = createField();
@@ -50,7 +52,7 @@
private static Object createField() {
// Makes sure that field type is not upgradable even the compiler becomes really smart and also
// no types are pruned otherwise casts can be statically evaluated.
- switch (new Random().nextInt(42)) {
+ switch (randomNumber) {
case 0:
return new TestObject();
case 1:
@@ -84,7 +86,7 @@
return ((String) field);
}
- private static native String getGeneratedFunctionDefinition() /*-{
+ private static native String getGeneratedCastFunctionDefinition() /*-{
return function() {
@CastOptimizationTest::castOp()();
@CastOptimizationTest::castOpJso()();
@@ -95,7 +97,25 @@
}-*/;
public void testCastsAreRemoved() throws Exception {
- String functionDef = getGeneratedFunctionDefinition();
+ String functionDef = getGeneratedCastFunctionDefinition();
assertFunctionMatches(functionDef, "");
}
+
+ private static native String getGeneratedNullnessPropagationFunctionDefinition() /*-{
+ return function() {
+ @CastOptimizationTest::castNonNullnessCheck()();
+ }.toString();
+ }-*/;
+
+ public static void castNonNullnessCheck() {
+ if ((String) (randomNumber == 1 ? "uniqueCastString" : new Object()) == null) {
+ throw new RuntimeException();
+ }
+ }
+
+ public void testCastsPropagatesNullness() throws Exception {
+ String functionDef = getGeneratedNullnessPropagationFunctionDefinition();
+ // Means that the function was inlined but the throw was optimized away.
+ assertFunctionMatches(functionDef, "<obf>==1?'uniqueCastString':new<obf>()");
+ }
}
diff --git a/user/test/com/google/gwt/dev/jjs/optimized/OptimizationTestBase.java b/user/test/com/google/gwt/dev/jjs/optimized/OptimizationTestBase.java
index 9465ae3..68ea4dc 100644
--- a/user/test/com/google/gwt/dev/jjs/optimized/OptimizationTestBase.java
+++ b/user/test/com/google/gwt/dev/jjs/optimized/OptimizationTestBase.java
@@ -30,7 +30,8 @@
public static void assertFunctionMatches(String functionDef, String pattern) {
String content = getFunctionContent(functionDef);
String regex = createRegex(pattern);
- assertTrue("content: " + content, content.matches(regex));
+ assertTrue("content: \"" + content + "\" does not match pattern: " + regex,
+ content.matches(regex));
}
private static String getFunctionContent(String functionDef) {
@@ -45,9 +46,11 @@
}
private static String createRegex(String pattern) {
- for (char toBeEscaped : ".[]+".toCharArray()) {
+ for (char toBeEscaped : ".[](){}+=?".toCharArray()) {
pattern = pattern.replace("" + toBeEscaped, "\\" + toBeEscaped);
}
+ pattern = pattern.replace("\\(\\)", "(\\(\\))?"); // to account for the removal of ()
+ // in new operations.
pattern = pattern.replace("<obf>", "[\\w$_]+");
return pattern + ";?";
}
diff --git a/user/test/com/google/gwt/dev/jjs/test/JsoTest.java b/user/test/com/google/gwt/dev/jjs/test/JsoTest.java
index d6a5f4a..e9f3f5f 100644
--- a/user/test/com/google/gwt/dev/jjs/test/JsoTest.java
+++ b/user/test/com/google/gwt/dev/jjs/test/JsoTest.java
@@ -837,6 +837,21 @@
assertTrue(i == (I) (trueFn() ? finalJso2 : (JavaScriptObject) null));
}
+ /**
+ *
+ * Test that nullability optimizations don't break the assumption that null is considered a JSO.
+ */
+ public void testNonNullableOptimization() {
+ // TODO(rluble): fix semantics to be more coherent by adding a nullness check for devirtualized
+ // methods in draft mode.
+ JavaScriptObject nullJso = (trueFn() ? null : JavaScriptObject.createArray());
+ // This statement should throw NPE (at least in draft mode) but it does not due to JSO
+ // devirtualization. These semantics are exploited in GWT standard library. Improvemnts and
+ // fixes to optimization passes might break these assumptions.
+ JSO1 someJso = nullJso.cast();
+ assertTrue(someJso == null);
+ }
+
private boolean trueFn() {
return true;
}
