This addresses issue #622. The problem is that CastOptimizer leaves the tree in a particular state, namely: "Cast.throwClassCastExceptionUnlessNull(value).value". The expectation is that FixDanglignRefsVisitor would replace the ".value" field reference with ".nullField". The problem is, however, that only TightenTypesVisitor can force FDRV to run; CastOptimizer cannot. My fix is to eliminate CastOptimizer altogether and move its work (mostly) into TypeTightener. Impossible casts are replaced in the tree with a (<null>) cast, which becomes a throwClassCastExceptionUnlessNull() in CastNormalizer. Review by: mmendez git-svn-id: https://google-web-toolkit.googlecode.com/svn/trunk@403 8db76d5a-ed1c-0410-87a9-c151d255dfc7
diff --git a/dev/core/src/com/google/gwt/dev/jjs/JavaToJavaScriptCompiler.java b/dev/core/src/com/google/gwt/dev/jjs/JavaToJavaScriptCompiler.java index 51988d1..5d887e7 100644 --- a/dev/core/src/com/google/gwt/dev/jjs/JavaToJavaScriptCompiler.java +++ b/dev/core/src/com/google/gwt/dev/jjs/JavaToJavaScriptCompiler.java
@@ -34,7 +34,6 @@ import com.google.gwt.dev.jjs.impl.ArrayNormalizer; import com.google.gwt.dev.jjs.impl.BuildTypeMap; import com.google.gwt.dev.jjs.impl.CastNormalizer; -import com.google.gwt.dev.jjs.impl.CastOptimizer; import com.google.gwt.dev.jjs.impl.CatchBlockNormalizer; import com.google.gwt.dev.jjs.impl.CompoundAssignmentNormalizer; import com.google.gwt.dev.jjs.impl.DeadCodeElimination; @@ -336,14 +335,12 @@ // - params based on assignment and call sites // - method bodies based on return statements // - polymorphic methods based on return types of all implementors + // - optimize casts and instanceof didChange = TypeTightener.exec(jprogram) || didChange; // tighten method call bindings didChange = MethodCallTightener.exec(jprogram) || didChange; - // remove unnecessary casts / optimize instanceof - didChange = CastOptimizer.exec(jprogram) || didChange; - // dead code removal?? didChange = DeadCodeElimination.exec(jprogram) || didChange;
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 2adf61d..417a3e1 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
@@ -43,10 +43,11 @@ } public boolean hasSideEffects() { - // technically this isn't true, but since the same cast on the same - // expression always evaluates the same way, it effectively has no side - // effects - return getExpr().hasSideEffects(); + // Any live cast operations might throw a ClassCastException + // + // TODO: revisit this when we support the concept of whether something + // can/must complete normally! + return true; } public void traverse(JVisitor visitor, Context ctx) {
diff --git a/dev/core/src/com/google/gwt/dev/jjs/impl/CastNormalizer.java b/dev/core/src/com/google/gwt/dev/jjs/impl/CastNormalizer.java index 7368151..0a9f81d 100644 --- a/dev/core/src/com/google/gwt/dev/jjs/impl/CastNormalizer.java +++ b/dev/core/src/com/google/gwt/dev/jjs/impl/CastNormalizer.java
@@ -155,8 +155,9 @@ // @Override public void endVisit(JCastOperation x, Context ctx) { - assert (x.getCastType() != program.getTypeNull()); - recordCast(x.getCastType(), x.getExpr()); + if (x.getCastType() != program.getTypeNull()) { + recordCast(x.getCastType(), x.getExpr()); + } } // @Override @@ -351,7 +352,23 @@ public void endVisit(JCastOperation x, Context ctx) { JExpression replaceExpr; JType toType = x.getCastType(); - if (toType instanceof JReferenceType) { + if (toType instanceof JNullType) { + /* + * Magic: a null type cast means the user tried a cast that couldn't + * possibly work. Typically this means either the statically resolvable + * arg type is incompatible with the target type, or the target type was + * globally uninstantiable. We handle this cast by throwing a + * ClassCastException, unless the argument is null. + */ + JMethod method = program.getSpecialMethod("Cast.throwClassCastExceptionUnlessNull"); + /* + * Override the type of the magic method with the null type. + */ + JMethodCall call = new JMethodCall(program, x.getSourceInfo(), null, + method, program.getTypeNull()); + call.getArgs().add(x.getExpr()); + replaceExpr = call; + } else if (toType instanceof JReferenceType) { JExpression curExpr = x.getExpr(); JReferenceType refType = (JReferenceType) toType; JType argType = x.getExpr().getType();
diff --git a/dev/core/src/com/google/gwt/dev/jjs/impl/CastOptimizer.java b/dev/core/src/com/google/gwt/dev/jjs/impl/CastOptimizer.java deleted file mode 100644 index 517e037..0000000 --- a/dev/core/src/com/google/gwt/dev/jjs/impl/CastOptimizer.java +++ /dev/null
@@ -1,138 +0,0 @@ -/* - * Copyright 2006 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.impl; - -import com.google.gwt.dev.jjs.ast.Context; -import com.google.gwt.dev.jjs.ast.JBinaryOperation; -import com.google.gwt.dev.jjs.ast.JBinaryOperator; -import com.google.gwt.dev.jjs.ast.JCastOperation; -import com.google.gwt.dev.jjs.ast.JInstanceOf; -import com.google.gwt.dev.jjs.ast.JMethod; -import com.google.gwt.dev.jjs.ast.JMethodCall; -import com.google.gwt.dev.jjs.ast.JModVisitor; -import com.google.gwt.dev.jjs.ast.JNullLiteral; -import com.google.gwt.dev.jjs.ast.JProgram; -import com.google.gwt.dev.jjs.ast.JReferenceType; -import com.google.gwt.dev.jjs.ast.JType; -import com.google.gwt.dev.jjs.ast.JTypeOracle; - -/** - * Optimizer that will remove all trivially computable casts and instanceof - * operations. - */ -public class CastOptimizer { - - /** - * Replaces all trivially computable casts and instanceof operations. - */ - private class ReplaceTrivialCastsVisitor extends JModVisitor { - - // @Override - public void endVisit(JCastOperation x, Context ctx) { - JType argType = x.getExpr().getType(); - if (!(x.getCastType() instanceof JReferenceType) - || !(argType instanceof JReferenceType)) { - return; - } - - JReferenceType toType = (JReferenceType) x.getCastType(); - JReferenceType fromType = (JReferenceType) argType; - - boolean triviallyTrue = false; - boolean triviallyFalse = false; - - JTypeOracle typeOracle = program.typeOracle; - if (typeOracle.canTriviallyCast(fromType, toType)) { - triviallyTrue = true; - } else if (!typeOracle.isInstantiatedType(toType)) { - triviallyFalse = true; - } else if (!typeOracle.canTheoreticallyCast(fromType, toType)) { - triviallyFalse = true; - } - - if (triviallyTrue) { - // remove the cast operation - ctx.replaceMe(x.getExpr()); - } else if (triviallyFalse) { - // throw a ClassCastException unless the argument is null - JMethod method = program.getSpecialMethod("Cast.throwClassCastExceptionUnlessNull"); - /* - * Override the type of the called method with the null type. Null flow - * will proceedeth forth from this cast operation. Assuredly, if the - * call completes normally it will return null. - */ - JMethodCall call = new JMethodCall(program, x.getSourceInfo(), null, - method, program.getTypeNull()); - call.getArgs().add(x.getExpr()); - ctx.replaceMe(call); - } - } - - // @Override - public void endVisit(JInstanceOf x, Context ctx) { - JType argType = x.getExpr().getType(); - if (!(argType instanceof JReferenceType)) { - return; - } - - JReferenceType toType = x.getTestType(); - JReferenceType fromType = (JReferenceType) argType; - - boolean triviallyTrue = false; - boolean triviallyFalse = false; - - JTypeOracle typeOracle = program.typeOracle; - if (fromType == program.getTypeNull()) { - // null is never instanceOf anything - triviallyFalse = true; - } else if (typeOracle.canTriviallyCast(fromType, toType)) { - triviallyTrue = true; - } else if (!typeOracle.isInstantiatedType(toType)) { - triviallyFalse = true; - } else if (!typeOracle.canTheoreticallyCast(fromType, toType)) { - triviallyFalse = true; - } - - if (triviallyTrue) { - // replace with a simple null test - JNullLiteral nullLit = program.getLiteralNull(); - JBinaryOperation neq = new JBinaryOperation(program, x.getSourceInfo(), - program.getTypePrimitiveBoolean(), JBinaryOperator.NEQ, - x.getExpr(), nullLit); - ctx.replaceMe(neq); - } else if (triviallyFalse) { - // replace with a false literal - ctx.replaceMe(program.getLiteralBoolean(false)); - } - } - } - - public static boolean exec(JProgram program) { - return new CastOptimizer(program).execImpl(); - } - - private final JProgram program; - - private CastOptimizer(JProgram program) { - this.program = program; - } - - private boolean execImpl() { - ReplaceTrivialCastsVisitor replacer = new ReplaceTrivialCastsVisitor(); - replacer.accept(program); - return replacer.didChange(); - } -}
diff --git a/dev/core/src/com/google/gwt/dev/jjs/impl/TypeTightener.java b/dev/core/src/com/google/gwt/dev/jjs/impl/TypeTightener.java index 9ecd657..4e13025 100644 --- a/dev/core/src/com/google/gwt/dev/jjs/impl/TypeTightener.java +++ b/dev/core/src/com/google/gwt/dev/jjs/impl/TypeTightener.java
@@ -19,10 +19,13 @@ import com.google.gwt.dev.jjs.ast.JArrayRef; import com.google.gwt.dev.jjs.ast.JArrayType; import com.google.gwt.dev.jjs.ast.JBinaryOperation; +import com.google.gwt.dev.jjs.ast.JBinaryOperator; +import com.google.gwt.dev.jjs.ast.JCastOperation; import com.google.gwt.dev.jjs.ast.JClassType; import com.google.gwt.dev.jjs.ast.JExpression; import com.google.gwt.dev.jjs.ast.JField; import com.google.gwt.dev.jjs.ast.JFieldRef; +import com.google.gwt.dev.jjs.ast.JInstanceOf; import com.google.gwt.dev.jjs.ast.JInterfaceType; import com.google.gwt.dev.jjs.ast.JLocal; import com.google.gwt.dev.jjs.ast.JLocalDeclarationStatement; @@ -31,6 +34,7 @@ import com.google.gwt.dev.jjs.ast.JMethodCall; import com.google.gwt.dev.jjs.ast.JModVisitor; import com.google.gwt.dev.jjs.ast.JNewArray; +import com.google.gwt.dev.jjs.ast.JNullLiteral; import com.google.gwt.dev.jjs.ast.JNullType; import com.google.gwt.dev.jjs.ast.JParameter; import com.google.gwt.dev.jjs.ast.JParameterRef; @@ -39,6 +43,7 @@ import com.google.gwt.dev.jjs.ast.JReturnStatement; import com.google.gwt.dev.jjs.ast.JTryStatement; import com.google.gwt.dev.jjs.ast.JType; +import com.google.gwt.dev.jjs.ast.JTypeOracle; import com.google.gwt.dev.jjs.ast.JVariable; import com.google.gwt.dev.jjs.ast.JVariableRef; import com.google.gwt.dev.jjs.ast.JVisitor; @@ -58,7 +63,7 @@ * The purpose of this pass is to record "type flow" information and then use * the information to infer places where "tighter" (that is, more specific) * types can be inferred for locals, fields, parameters, and method return - * types. + * types. We also optimize dynamic casts and instanceof operations. * * Type flow occurs automatically in most JExpressions. But locals, fields, * parameters, and method return types serve as "way points" where type @@ -162,6 +167,7 @@ } } } + /** * Record "type flow" information. Variables receive type flow via assignment. * As a special case, Parameters also receive type flow based on the types of @@ -370,16 +376,51 @@ * to change the declared type of a field, local, parameter, or method to a * more specific type. * - * We must iterate mutiple times because each way point we tighten creates - * more opportunities to do additional tightening for the things that depend - * on it. + * Also optimize dynamic casts and instanceof operations where possible. */ - public class TightenTypesVisitor extends JVisitor { + public class TightenTypesVisitor extends JModVisitor { - private boolean didChange = false; + /** + * <code>true</code> if this visitor has changed the AST apart from calls + * to Context. + */ + private boolean myDidChange = false; public boolean didChange() { - return didChange; + return myDidChange || super.didChange(); + } + + public void endVisit(JCastOperation x, Context ctx) { + JType argType = x.getExpr().getType(); + if (!(x.getCastType() instanceof JReferenceType) + || !(argType instanceof JReferenceType)) { + return; + } + + JReferenceType toType = (JReferenceType) x.getCastType(); + JReferenceType fromType = (JReferenceType) argType; + + boolean triviallyTrue = false; + boolean triviallyFalse = false; + + JTypeOracle typeOracle = program.typeOracle; + if (typeOracle.canTriviallyCast(fromType, toType)) { + triviallyTrue = true; + } else if (!typeOracle.isInstantiatedType(toType)) { + triviallyFalse = true; + } else if (!typeOracle.canTheoreticallyCast(fromType, toType)) { + triviallyFalse = true; + } + + if (triviallyTrue) { + // remove the cast operation + ctx.replaceMe(x.getExpr()); + } else if (triviallyFalse) { + // replace with a magic NULL cast + JCastOperation newOp = new JCastOperation(program, x.getSourceInfo(), + program.getTypeNull(), x.getExpr()); + ctx.replaceMe(newOp); + } } // @Override @@ -388,6 +429,45 @@ } // @Override + public void endVisit(JInstanceOf x, Context ctx) { + JType argType = x.getExpr().getType(); + if (!(argType instanceof JReferenceType)) { + // TODO: is this even possible? Replace with assert maybe. + return; + } + + JReferenceType toType = x.getTestType(); + JReferenceType fromType = (JReferenceType) argType; + + boolean triviallyTrue = false; + boolean triviallyFalse = false; + + JTypeOracle typeOracle = program.typeOracle; + if (fromType == program.getTypeNull()) { + // null is never instanceOf anything + triviallyFalse = true; + } else if (typeOracle.canTriviallyCast(fromType, toType)) { + triviallyTrue = true; + } else if (!typeOracle.isInstantiatedType(toType)) { + triviallyFalse = true; + } else if (!typeOracle.canTheoreticallyCast(fromType, toType)) { + triviallyFalse = true; + } + + if (triviallyTrue) { + // replace with a simple null test + JNullLiteral nullLit = program.getLiteralNull(); + JBinaryOperation neq = new JBinaryOperation(program, x.getSourceInfo(), + program.getTypePrimitiveBoolean(), JBinaryOperator.NEQ, + x.getExpr(), nullLit); + ctx.replaceMe(neq); + } else if (triviallyFalse) { + // replace with a false literal + ctx.replaceMe(program.getLiteralBoolean(false)); + } + } + + // @Override public void endVisit(JLocal x, Context ctx) { tighten(x); } @@ -410,7 +490,7 @@ // tighten based on non-instantiability if (!program.typeOracle.isInstantiatedType(refType)) { x.setType(typeNull); - didChange = true; + myDidChange = true; return; } @@ -443,7 +523,7 @@ resultType = program.strongerType(refType, resultType); if (refType != resultType) { x.setType(resultType); - didChange = true; + myDidChange = true; } } @@ -456,7 +536,7 @@ if (!program.typeOracle.isInstantiatedType((JReferenceType) leafType)) { arrayType = program.getTypeArray(typeNull, arrayType.getDims()); x.setType(arrayType); - didChange = true; + myDidChange = true; } } } @@ -501,7 +581,7 @@ // tighten based on non-instantiability if (!program.typeOracle.isInstantiatedType(refType)) { x.setType(typeNull); - didChange = true; + myDidChange = true; return; } @@ -551,7 +631,7 @@ resultType = program.strongerType(refType, resultType); if (refType != resultType) { x.setType(resultType); - didChange = true; + myDidChange = true; } } } @@ -586,6 +666,12 @@ private boolean execImpl() { RecordVisitor recorder = new RecordVisitor(); recorder.accept(program); + + /* + * We must iterate mutiple times because each way point we tighten creates + * more opportunities to do additional tightening for the things that depend + * on it. + */ boolean madeChanges = false; while (true) { TightenTypesVisitor tightener = new TightenTypesVisitor();