improve removeEdge

- doesn't always remove all edges when removing at the tail
- returns a boolean indicating if an edge was removed

sootup.core/src/main/java/sootup/core/graph/MutableBasicBlock.java

@@ -82,12 +82,12 @@ public void addSuccessorBlock(@Nonnull MutableBasicBlock block) {
     successorBlocks.add(block);
   }
 
-  public void removePredecessorBlock(@Nonnull MutableBasicBlock b) {
-    predecessorBlocks.remove(b);
+  public boolean removePredecessorBlock(@Nonnull MutableBasicBlock b) {
+    return predecessorBlocks.remove(b);
   }
 
-  public void removeSuccessorBlock(@Nonnull MutableBasicBlock b) {
-    successorBlocks.remove(b);
+  public boolean removeSuccessorBlock(@Nonnull MutableBasicBlock b) {
+    return successorBlocks.remove(b);
   }
 
   public void addExceptionalSuccessorBlock(@Nonnull ClassType exception, MutableBasicBlock b) {

sootup.core/src/main/java/sootup/core/graph/MutableBlockStmtGraph.java

@@ -958,20 +958,15 @@ private void linkBlocks(@Nonnull MutableBasicBlock blockA, @Nonnull MutableBasic
   }
 
   @Override
-  public void removeEdge(@Nonnull Stmt from, @Nonnull Stmt to) {
-    // FIXME: how to handle "partial" removals of targets of flows starting from a Branching Stmt..
-    // e.g. because one of the targets are removed.. that changes the whole logic there..
-
+  public boolean removeEdge(@Nonnull Stmt from, @Nonnull Stmt to) {
     MutableBasicBlock blockOfFrom = stmtToBlock.get(from);
     MutableBasicBlock blockOfTo = stmtToBlock.get(to);
 
     if (blockOfFrom == null || blockOfTo == null) {
       // one of the Stmts is not existing anymore in this graph - so neither a connection.
-      return;
+      return false;
     }
 
-    removeBlockBorderEdgesInternal(from, blockOfFrom);
-
     // divide block if from and to are from the same block
     if (blockOfFrom == blockOfTo) {
       // divide block and don't link them
@@ -985,61 +980,35 @@ public void removeEdge(@Nonnull Stmt from, @Nonnull Stmt to) {
         blockOfFrom.clearSuccessorBlocks();
         blocks.add(newBlock);
         newBlock.getStmts().forEach(s -> stmtToBlock.put(s, newBlock));
+        return true;
       } else {
-        // throw new IllegalArgumentException("Can't seperate the flow from '"+from+"' to '"+to+"'.
-        // The Stmts are not connected in this graph!");
+        // `from` and `to` are not successive statements in the block
+        return false;
       }
-    }
-  }
+    } else {
+      // `from` and `to` are part of different blocks
 
-  protected void removeBlockBorderEdgesInternal(
-      @Nonnull Stmt from, @Nonnull MutableBasicBlock blockOfFrom) {
-    // TODO: is it intuitive to remove connections to the BasicBlock in the case we cant merge the
-    // blocks?
-    // TODO: reuse tryMerge*Block?
-
-    // add BlockB to BlockA if blockA has no branchingstmt as tail && same traps
-    if (!blockOfFrom.getStmts().isEmpty() && from == blockOfFrom.getTail()) {
-      if (blockOfFrom.getPredecessors().size() == 1) {
-        MutableBasicBlock singlePreviousBlock = blockOfFrom.getPredecessors().get(0);
-        if (!singlePreviousBlock.getTail().branches() && singlePreviousBlock != blockOfFrom) {
-          if (singlePreviousBlock
-              .getExceptionalSuccessors()
-              .equals(blockOfFrom.getExceptionalSuccessors())) {
-            blockOfFrom
-                .getStmts()
-                .forEach(
-                    k -> {
-                      addNodeToBlock(blockOfFrom, k);
-                    });
-            return;
-          }
-        }
+      if (blockOfFrom.getTail() != from || blockOfTo.getHead() != to) {
+        // `from` and `to` aren't the tail and head of their respective blocks,
+        // which means they aren't connected
+        return false;
       }
 
-      // remove outgoing connections from blockA if from stmt is the tail
-      if (!from.branches()) {
-        if (!blockOfFrom.getStmts().isEmpty() && blockOfFrom.getSuccessors().size() == 1) {
-          // merge previous block if possible i.e. no branchingstmt as tail && same traps && no
-          // other predesccorblocks
-          MutableBasicBlock singleSuccessorBlock = blockOfFrom.getSuccessors().get(0);
-          if (singleSuccessorBlock.getPredecessors().size() == 1
-              && singleSuccessorBlock.getPredecessors().get(0) == blockOfFrom) {
-            if (singleSuccessorBlock
-                .getExceptionalSuccessors()
-                .equals(blockOfFrom.getExceptionalSuccessors())) {
-              singleSuccessorBlock
-                  .getStmts()
-                  .forEach(
-                      k -> {
-                        addNodeToBlock(blockOfFrom, k);
-                      });
-            }
-          }
-        }
-      } else {
-        blockOfFrom.clearSuccessorBlocks();
+      // remove the connection between the two blocks
+      boolean predecessorRemoved = blockOfTo.removePredecessorBlock(blockOfFrom);
+      boolean successorRemoved = blockOfFrom.removeSuccessorBlock(blockOfTo);
+      assert predecessorRemoved == successorRemoved;
+
+      if (!predecessorRemoved) {
+        // the blocks weren't connected
+        return false;
       }
+
+      // the removal of the edge between `from` and `to` might have created blocks that can be merged
+      tryMergeWithPredecessorBlock(blockOfTo);
+      tryMergeWithSuccessorBlock(blockOfFrom);
+
+      return true;
     }
   }
 

sootup.core/src/main/java/sootup/core/graph/MutableStmtGraph.java

@@ -96,8 +96,12 @@ public void setEdges(@Nonnull Stmt from, @Nonnull Stmt... targets) {
     setEdges(from, Arrays.asList(targets));
   }
 
-  /** removes the current outgoing flows of "from" to "targets" */
-  public abstract void removeEdge(@Nonnull Stmt from, @Nonnull Stmt to);
+  /**
+   * removes the current outgoing flows of "from" to "to"
+   *
+   * @return true if the edge existed and was removed; false if the edge didn't exist
+   */
+  public abstract boolean removeEdge(@Nonnull Stmt from, @Nonnull Stmt to);
 
   /** Modifications of exceptional flows removes all exceptional flows from "stmt" */
   public abstract void clearExceptionalEdges(@Nonnull Stmt stmt);

sootup.core/src/test/java/sootup/core/graph/MutableBlockStmtGraphTest.java

@@ -256,19 +256,19 @@ public void modifyStmtToBlockAtTail() {
     assertEquals(1, graph.getBlocksSorted().get(1).getSuccessors().size());
 
     // remove non-existing edge
-    graph.removeEdge(firstNop, conditionalStmt);
+    assertFalse(graph.removeEdge(firstNop, conditionalStmt));
     assertEquals(2, graph.getBlocksSorted().size());
 
     // remove branchingstmt at end -> edge across blocks
-    graph.removeEdge(conditionalStmt, firstNop);
+    assertTrue(graph.removeEdge(conditionalStmt, firstNop));
     assertEquals(2, graph.getBlocksSorted().size());
 
     assertEquals(4, graph.getNodes().size());
     graph.removeNode(firstNop);
     assertEquals(3, graph.getNodes().size());
 
     // remove branchingstmt at head
-    graph.removeEdge(conditionalStmt, secondNop);
+    assertTrue(graph.removeEdge(conditionalStmt, secondNop));
     assertEquals(1, graph.getBlocksSorted().size());
     assertEquals(3, graph.getNodes().size());
 
@@ -402,22 +402,22 @@ public void linkDirectlyAddedBlocks() {
     assertEquals(1, graph.successors(firstNop).size());
     assertEquals(1, graph.successors(secondNop).size());
 
-    graph.removeEdge(secondNop, thirdNop);
+    assertTrue(graph.removeEdge(secondNop, thirdNop));
     assertEquals(2, graph.getBlocksSorted().size());
     assertEquals(1, graph.successors(firstNop).size());
     assertEquals(0, graph.successors(secondNop).size());
 
-    graph.removeEdge(secondNop, thirdNop); // empty operation
+    assertFalse(graph.removeEdge(secondNop, thirdNop)); // empty operation
     assertEquals(2, graph.getBlocksSorted().size());
     assertEquals(1, graph.successors(firstNop).size());
     assertEquals(0, graph.successors(secondNop).size());
 
-    graph.removeEdge(firstNop, thirdNop); // empty operation
+    assertFalse(graph.removeEdge(firstNop, thirdNop)); // empty operation
     assertEquals(2, graph.getBlocksSorted().size());
     assertEquals(1, graph.successors(firstNop).size());
     assertEquals(0, graph.successors(secondNop).size());
 
-    graph.removeEdge(firstNop, secondNop);
+    assertTrue(graph.removeEdge(firstNop, secondNop));
     assertEquals(3, graph.getBlocksSorted().size());
   }
 
@@ -929,7 +929,7 @@ public void removeEdge() {
     assertEquals(1, graph.successors(stmt1).size());
     assertTrue(graph.hasEdgeConnecting(stmt1, stmt2));
 
-    graph.removeEdge(stmt1, stmt2);
+    assertTrue(graph.removeEdge(stmt1, stmt2));
     assertEquals(0, graph.successors(stmt1).size());
     assertFalse(graph.hasEdgeConnecting(stmt1, stmt2));
   }
@@ -960,7 +960,7 @@ public void removeImpossibleEdge() {
     Stmt stmt2 = new JNopStmt(StmtPositionInfo.createNoStmtPositionInfo());
     MutableStmtGraph graph = new MutableBlockStmtGraph();
     // nodes are not in the graph!
-    graph.removeEdge(stmt1, stmt2);
+    assertFalse(graph.removeEdge(stmt1, stmt2));
   }
 
   @Test