Fix bug in nullness analysis

core/src/main/java/org/teavm/model/analysis/NullnessInformationBuilder.java

@@ -24,6 +24,7 @@ import com.carrotsearch.hppc.IntSet;
 import java.util.ArrayList;
 import java.util.BitSet;
 import java.util.List;
+import org.teavm.common.DominatorTree;
 import org.teavm.common.Graph;
 import org.teavm.common.GraphBuilder;
 import org.teavm.common.GraphUtils;
@@ -190,6 +191,12 @@ class NullnessInformationBuilder {
         BasicBlock currentBlock;
         IntIntMap nullSuccessors = new IntIntOpenHashMap();
         IntIntMap notNullSuccessors = new IntIntOpenHashMap();
+        private DominatorTree dom;
+
+        @Override
+        public void setDomTree(DominatorTree domTree) {
+            dom = domTree;
+        }
 
         @Override
         public State visit(BasicBlock block) {
@@ -317,12 +324,12 @@ class NullnessInformationBuilder {
         public void visit(BranchingInstruction insn) {
             switch (insn.getCondition()) {
                 case NOT_NULL:
-                    notNullSuccessors.put(insn.getConsequent().getIndex(), insn.getOperand().getIndex());
+                    setNotNullSuccessor(insn.getConsequent(), insn.getOperand());
-                    nullSuccessors.put(insn.getAlternative().getIndex(), insn.getOperand().getIndex());
+                    setNullSuccessor(insn.getAlternative(), insn.getOperand());
                     break;
                 case NULL:
-                    nullSuccessors.put(insn.getConsequent().getIndex(), insn.getOperand().getIndex());
+                    setNullSuccessor(insn.getConsequent(), insn.getOperand());
-                    notNullSuccessors.put(insn.getAlternative().getIndex(), insn.getOperand().getIndex());
+                    setNotNullSuccessor(insn.getAlternative(), insn.getOperand());
                     break;
                 default:
                     break;
@@ -341,18 +348,39 @@ class NullnessInformationBuilder {
 
             switch (insn.getCondition()) {
                 case REFERENCE_EQUAL:
-                    notNullSuccessors.put(insn.getConsequent().getIndex(), first.getIndex());
+                    setNotNullSuccessor(insn.getConsequent(), first);
-                    nullSuccessors.put(insn.getAlternative().getIndex(), first.getIndex());
+                    setNullSuccessor(insn.getAlternative(), first);
                     break;
                 case REFERENCE_NOT_EQUAL:
-                    nullSuccessors.put(insn.getConsequent().getIndex(), first.getIndex());
+                    setNullSuccessor(insn.getConsequent(), first);
-                    notNullSuccessors.put(insn.getAlternative().getIndex(), first.getIndex());
+                    setNotNullSuccessor(insn.getAlternative(), first);
                     break;
                 default:
                     break;
             }
         }
 
+        private void setNullSuccessor(BasicBlock successor, Variable value) {
+            if (shouldSetSuccessor(successor, value)) {
+                nullSuccessors.put(successor.getIndex(), value.getIndex());
+            }
+        }
+
+        private void setNotNullSuccessor(BasicBlock successor, Variable value) {
+            if (shouldSetSuccessor(successor, value)) {
+                notNullSuccessors.put(successor.getIndex(), value.getIndex());
+            }
+        }
+
+        private boolean shouldSetSuccessor(BasicBlock successor, Variable value) {
+            for (Phi phi : successor.getPhis()) {
+                if (phi.getIncomings().stream().anyMatch(incoming -> incoming.getValue() == value)) {
+                    return false;
+                }
+            }
+            return true;
+        }
+
         private void insertNotNullInstruction(Instruction currentInstruction, Variable var) {
             if (notNullVariables.get(var.getIndex())) {
                 return;

core/src/main/java/org/teavm/model/instructions/NullCheckInstruction.java

@@ -18,10 +18,6 @@ package org.teavm.model.instructions;
 import org.teavm.model.Instruction;
 import org.teavm.model.Variable;
 
-/**
- *
- * @author Alexey Andreev
- */
 public class NullCheckInstruction extends Instruction {
     private Variable value;
     private Variable receiver;

core/src/main/java/org/teavm/model/util/DominatorWalker.java

@@ -23,12 +23,13 @@ import org.teavm.model.Program;
 
 public class DominatorWalker {
     private Program program;
+    private DominatorTree dom;
     private Graph domGraph;
 
     public DominatorWalker(Program program) {
         this.program = program;
         Graph cfg = ProgramUtils.buildControlFlowGraph(program);
-        DominatorTree dom = GraphUtils.buildDominatorTree(cfg);
+        dom = GraphUtils.buildDominatorTree(cfg);
         domGraph = GraphUtils.buildDominatorGraph(dom, cfg.size());
     }
 
@@ -36,6 +37,7 @@ public class DominatorWalker {
         int[] stack = new int[program.basicBlockCount() * 2];
         Object[] stateStack = new Object[stack.length];
         boolean[] backward = new boolean[stack.length];
+        callback.setDomTree(dom);
 
         int head = 1;
         while (head > 0) {

core/src/main/java/org/teavm/model/util/DominatorWalkerCallback.java

@@ -15,6 +15,7 @@
  */
 package org.teavm.model.util;
 
+import org.teavm.common.DominatorTree;
 import org.teavm.model.BasicBlock;
 
 /**
@@ -23,6 +24,9 @@ import org.teavm.model.BasicBlock;
  * @param <T> type of state that can be saved for each visited node.
  */
 public interface DominatorWalkerCallback<T> {
+    default void setDomTree(DominatorTree domTree) {
+    }
+
     /**
      * Called before visiting block. This method should tell whether this block and all of its descendant blocks
      * should be visited.

core/src/test/java/org/teavm/model/analysis/test/NullnessAnalysisTest.java

@@ -75,6 +75,11 @@ public class NullnessAnalysisTest {
         test();
     }
 
+    @Test
+    public void nonDominatedBranch() {
+        test();
+    }
+
     private void test() {
         String baseName = "model/analysis/nullness/" + name.getMethodName();
         String originalResourceName = baseName + ".original.txt";

core/src/test/resources/model/analysis/nullness/nonDominatedBranch.extended.txt

@@ -0,0 +1,15 @@
+var @this as this
+
+$start
+    @a := invokeStatic `Foo.f()LBar;`
+    if @a === null then goto $joint else goto $ifNonNull
+$ifNonNull
+    @a_1 := nullCheck @a
+    @b := invokeStatic `Foo.g()LBar;`
+    if @b !== null then goto $joint else goto $ifNull
+$ifNull
+    @b_1 := null
+    return @a_1
+$joint
+    @c := phi @a from $start, @b from $ifNonNull
+    return @c

core/src/test/resources/model/analysis/nullness/nonDominatedBranch.original.txt

@@ -0,0 +1,13 @@
+var @this as this
+
+$start
+    @a := invokeStatic `Foo.f()LBar;`
+    if @a === null then goto $joint else goto $ifNonNull
+$ifNonNull
+    @b := invokeStatic `Foo.g()LBar;`
+    if @b !== null then goto $joint else goto $ifNull
+$ifNull
+    return @a
+$joint
+    @c := phi @a from $start, @b from $ifNonNull
+    return @c