refactor: Use identity for equals in nodes

Effectively, the semantic of `isSameAs` and `equals` has been exchanged.

There is a trade off here: We can't just copy nodes arbitrarily anymore
but we gain simpler equality reasoning and some performance.

The `FormalDiffGraph` was originally introduced to simplify comparisons
but that is now no longer the case. If no future use cases emerge we
should consider removing it again.

Author: ibbem

src/main/java/org/variantsync/diffdetective/variation/diff/DiffNode.java

@@ -104,8 +104,7 @@ public String toString() {
      * It stores the projection node at each time so that only one instance of {@link Projection}
      * per {@link Time} is ever created. This array has to be indexed by {@code Time.ordinal()}
      *
-     * <p>This field is required to allow identity tests of {@link Projection}s with {@code ==} instead
-     * of {@link Projection#isSameAs}.
+     * <p>This field is required to allow identity tests of {@link Projection}s with {@code ==}.
      */
     private Projection[] projections = new Projection[2];
 
@@ -824,6 +823,41 @@ public static <T extends VariationNode<T>> DiffNode unchanged(T variationNode) {
         return unchanged(DiffNode::unchangedFlat, variationNode);
     }
 
+    /**
+     * Returns true if this subtree is exactly equal to {@code other}.
+     * This check uses equality checks instead of identity.
+     */
+    public boolean isSameAs(DiffNode other) {
+        return isSameAs(this, other, new HashSet<>());
+    }
+
+    private static boolean isSameAs(DiffNode a, DiffNode b, Set<DiffNode> visited) {
+        if (!visited.add(a)) {
+            return true;
+        }
+
+        if (!(
+                a.getDiffType().equals(b.getDiffType()) &&
+                a.getNodeType().equals(b.getNodeType()) &&
+                a.getFromLine().equals(b.getFromLine()) &&
+                a.getToLine().equals(b.getToLine()) &&
+                (a.getFormula() == null ? b.getFormula() == null : a.getFormula().equals(b.getFormula())) &&
+                a.getLabel().equals(b.getLabel())
+        )) {
+            return false;
+        }
+
+        Iterator<DiffNode> aIt = a.getAllChildren().iterator();
+        Iterator<DiffNode> bIt = b.getAllChildren().iterator();
+        while (aIt.hasNext() && bIt.hasNext()) {
+            if (!isSameAs(aIt.next(), bIt.next(), visited)) {
+                return false;
+            }
+        }
+
+        return aIt.hasNext() == bIt.hasNext();
+    }
+
     @Override
     public String toString() {
         String s;
@@ -837,25 +871,4 @@ public String toString() {
         }
         return s;
     }
-
-    @Override
-    public boolean equals(Object o) {
-        if (this == o) return true;
-        if (o == null || getClass() != o.getClass()) return false;
-        DiffNode diffNode = (DiffNode) o;
-        return diffType == diffNode.diffType && nodeType == diffNode.nodeType && from.equals(diffNode.from) && to.equals(diffNode.to) && Objects.equals(featureMapping, diffNode.featureMapping) && label.equals(diffNode.label);
-    }
-
-    /**
-     * Compute a hash using all available attributes.
-     *
-     * This implementation doesn't strictly adhere to the contract required by {@code Object},
-     * because some attributes (for example the line numbers) can be changed during the lifetime of
-     * a {@code DiffNode}. So when using something like a {@code HashSet} the user of {@code
-     * DiffNode} has to be careful not to change any attributes of a stored {@code DiffNode}.
-     */
-    @Override
-    public int hashCode() {
-        return Objects.hash(diffType, nodeType, from, to, featureMapping, label);
-    }
 }

src/main/java/org/variantsync/diffdetective/variation/diff/DiffTree.java

@@ -437,6 +437,14 @@ public ConsistencyResult isConsistent() {
         return ConsistencyResult.Success();
     }
 
+    /**
+     * Returns true if this {@code DiffTree} is exactly equal to {@code other}.
+     * This check uses equality checks instead of identity.
+     */
+    public boolean isSameAs(DiffTree other) {
+        return this.getRoot().isSameAs(other.getRoot());
+    }
+
     @Override
     public String toString() {
         return "DiffTree of " + source;

src/main/java/org/variantsync/diffdetective/variation/diff/Projection.java

@@ -18,12 +18,6 @@
  *
  * <p>This class has to be instantiated using {@link DiffNode#projection}.
  *
- * <p>Implementation note: It's ensured that identity can be checked using {@code ==}. This
- * prevents unexpected behaviour if some code uses {@code ==} instead of {@link isSameAs} as
- * documented in {@link VariationNode}. Although this is currently guaranteed by all
- * implementations of {@link VariationNode} it should still be considered a bug if {@code ==} is
- * used to check for identity ({@code null} checks are still allowed).
- *
  * @see DiffNode#projection
  */
 public class Projection extends VariationNode<Projection> {
@@ -148,22 +142,4 @@ public Node getFormula() {
     public int getID() {
         return getBackingNode().getID();
     }
-
-    @Override
-    public boolean isSameAs(Projection other) {
-        if (other != null && getClass() == other.getClass()) {
-            Projection otherProjection = (Projection) other;
-            return time.equals(otherProjection.time) && getBackingNode() == otherProjection.getBackingNode();
-        } else {
-            return false;
-        }
-    }
-
-    @Override
-    public boolean equals(Object o) {
-        if (this == o) return true;
-        if (o == null || getClass() != o.getClass()) return false;
-        Projection projection = (Projection) o;
-        return time.equals(projection.time) && getBackingNode().equals(projection.getBackingNode());
-    }
 };

src/main/java/org/variantsync/diffdetective/variation/diff/graph/FormalDiffGraph.java

@@ -68,20 +68,5 @@ public String toString() {
 
         return b.toString();
     }
-
-    @Override
-    public boolean equals(Object o) {
-        if (this == o) return true;
-        if (o == null || getClass() != o.getClass()) return false;
-        final FormalDiffGraph graph = (FormalDiffGraph) o;
-
-        return     nodes.equals(graph.nodes)
-                && edges.equals(graph.edges);
-    }
-
-    @Override
-    public int hashCode() {
-        return Objects.hash(nodes, edges);
-    }
 }
 

src/main/java/org/variantsync/diffdetective/variation/tree/VariationNode.java

@@ -35,11 +35,6 @@
  * quite common or because the calling syntax {@code node.algorithm()} makes more sense than the
  * alternative {@code Algorithm.run(node)}).
  *
- * <p>Identity of {@code VariationNode}s shouldn't be tested using {@code ==} because this might be
- * a view which is instantiated multiple times for the same node (e.g., it might be that
- * {@code getParent() != getParent()}). Instead the method {@link isSameAs} should be used to test
- * for identity.
- *
  * @param <T> the derived type (the type extending this class)
  *
  * @see assertConsistency
@@ -124,7 +119,7 @@ public List<String> getLabelLines() {
      * <p>The following invariant has to hold for all {@code node}s:
      * <code>
      *   for (var child : node.getChildren()) {
-     *     Assert.assertTrue(node.isSameAs(child.getParent(node)))
+     *     Assert.assertTrue(node == child.getParent(node))
      *   }
      * </code>
      *
@@ -190,7 +185,7 @@ public T getIfNode() {
      * @see getChildren
      */
     public boolean isChild(T child) {
-        return child.getParent().isSameAs(this.upCast());
+        return child.getParent() == this.upCast();
     }
 
     /**
@@ -560,7 +555,7 @@ public void assertConsistency() {
         // check consistency of children lists and edges
         for (var child : getChildren()) {
             Assert.assertTrue(
-                child.getParent().isSameAs(this.upCast()), () ->
+                child.getParent() == this.upCast(), () ->
                 "The parent (" + this + ") of " + child + " is not set correctly");
         }
     }
@@ -578,19 +573,6 @@ public void assertConsistency() {
      */
     public abstract int getID();
 
-    /**
-     * Checks if {@code other} represents the same node as this node.
-     *
-     * <p>The difference between this method and {@link equals} is the same as the difference
-     * between {@code ==} and {@link equals}: {@code isSameAs} respects the identity of the backing
-     * node and {@link equals} does not.
-     *
-     * <p>This method has to be used instead of {@code ==} because multiple instances of this view
-     * representing the same node might be created (i.e., {@code getParent() == getParent()} might
-     * not always hold).
-     */
-    public abstract boolean isSameAs(T other);
-
     /**
      * Unparses the labels of this subtree into {@code output}.
      *
@@ -614,4 +596,35 @@ public void printSourceCode(BufferedWriter output) throws IOException {
             output.newLine();
         }
     }
+
+    /**
+     * Returns true if this subtree is exactly equal to {@code other}.
+     * This check uses equality checks instead of identity.
+     */
+    public boolean isSameAs(VariationNode<T> other) {
+        if (!shallowIsSameAs(other)) {
+            return false;
+        }
+
+        var childIt = getChildren().iterator();
+        var otherChildIt = other.getChildren().iterator();
+        while (childIt.hasNext() && otherChildIt.hasNext()) {
+            if (!childIt.next().isSameAs(otherChildIt.next())) {
+                return false;
+            }
+        }
+
+        return childIt.hasNext() == otherChildIt.hasNext();
+    }
+
+    /**
+     * Returns true if this node is exactly equal to {@code other} without checking any children.
+     * This check uses equality checks instead of identity.
+     */
+    protected boolean shallowIsSameAs(VariationNode<T> other) {
+        return
+            this.getNodeType().equals(other.getNodeType()) &&
+            this.getLabel().equals(other.getLabel()) &&
+            this.getLineRange().equals(other.getLineRange());
+    }
 }

src/main/java/org/variantsync/diffdetective/variation/tree/VariationTreeNode.java

@@ -357,32 +357,6 @@ public VariationTreeNode deepCopy(final Map<VariationTreeNode, VariationTreeNode
         return toVariationTree(oldToNew);
     }
 
-    @Override
-    public boolean isSameAs(VariationTreeNode other) {
-        return this == other;
-    }
-
-    @Override
-    public boolean equals(Object o) {
-        if (this == o) return true;
-        if (o == null || getClass() != o.getClass()) return false;
-        var other = (VariationTreeNode) o;
-        return nodeType == other.nodeType && lineRange.equals(other.lineRange) && Objects.equals(featureMapping, other.featureMapping) && label.equals(other.label);
-    }
-
-    /**
-     * Compute a hash using all available attributes.
-     *
-     * <p>This implementation doesn't strictly adhere to the contract required by {@code Object},
-     * because some attributes (for example the line numbers) can be changed during the lifetime of
-     * a node. So when using something like a {@code HashSet} the user of this class has to be
-     * careful with any modifications of attributes.
-     */
-    @Override
-    public int hashCode() {
-        return Objects.hash(nodeType, lineRange, featureMapping, label);
-    }
-
     @Override
     public String toString() {
         String s;

src/test/java/BadVDiffTest.java

@@ -28,18 +28,16 @@ public void toGood_after_fromGood_idempotency(String filename) throws IOExceptio
         Logger.debug("Testing " + testfile);
 
         final DiffTree initialVDiff = DiffTree.fromFile(testfile, new DiffTreeParseOptions(false, false));
-        final FormalDiffGraph initialVDiffAsGraph = FormalDiffGraph.fromDiffTree(initialVDiff);
-        Logger.debug("Initial:" + StringUtils.LINEBREAK + initialVDiffAsGraph);
+        Logger.debug("Initial:" + StringUtils.LINEBREAK + initialVDiff);
         initialVDiff.assertConsistency();
 
         final BadVDiff badDiff = BadVDiff.fromGood(initialVDiff);
         Logger.debug("Bad:" + StringUtils.LINEBREAK + badDiff.prettyPrint());
 
         final DiffTree goodDiff = badDiff.toGood();
-        final FormalDiffGraph goodDiffAsGraph = FormalDiffGraph.fromDiffTree(goodDiff);
-        Logger.debug("Good:" + StringUtils.LINEBREAK + goodDiffAsGraph);
+        Logger.debug("Good:" + StringUtils.LINEBREAK + goodDiff);
         goodDiff.assertConsistency();
 
-        Assertions.assertEquals(initialVDiffAsGraph, goodDiffAsGraph);
+        Assertions.assertTrue(initialVDiff.isSameAs(goodDiff));
     }
 }