[fix] Fix flaky inconsistency detection (#125)

Author: slarse

src/main/java/se/kth/spork/base3dm/TdmMerge.java

@@ -1,5 +1,6 @@
 package se.kth.spork.base3dm;
 
+import se.kth.spork.exception.ConflictException;
 import se.kth.spork.util.LazyLogger;
 
 import java.util.*;
@@ -39,10 +40,8 @@ public static <T extends ListNode,V> void resolveRawMerge(ChangeSet<T,V> base, C
             mergeContent(pcs.getSuccessor(), base, delta);
 
             List<Pcs<T>> others = delta.getOtherRoots(pcs);
-            if (others.isEmpty())
-                others = delta.getOtherPredecessors(pcs);
-            if (others.isEmpty())
-                others = delta.getOtherSuccessors(pcs);
+            others.addAll(delta.getOtherPredecessors(pcs));
+            others.addAll(delta.getOtherSuccessors(pcs));
 
             for (Pcs<T> otherPcs : others) {
                 if (base.contains(otherPcs)) {
@@ -102,7 +101,7 @@ private static <T extends ListNode,V> Set<Content<T,V>> handleContentConflict(Se
         } else if (newContent.size() > 2) {
             // This should never happen, as there are at most 3 pieces of content to begin with and base has been
             // removed.
-            throw new IllegalStateException("Unexpected amount of conflicting content: " + newContent);
+            throw new ConflictException("Unexpected amount of conflicting content: " + newContent);
         }
 
         if (newContent.size() != 1) {

src/main/java/se/kth/spork/exception/ConflictException.java

@@ -0,0 +1,15 @@
+package se.kth.spork.exception;
+
+/**
+ * Thrown when Spork encounters a conflict that cannot be handled properly.
+ *
+ * @author Simon Larsén
+ */
+public class ConflictException extends RuntimeException {
+    public ConflictException(String s) {
+        super(s);
+    }
+    public ConflictException(String s, Throwable throwable) {
+        super(s, throwable);
+    }
+}

src/main/java/se/kth/spork/spoon/pcsinterpreter/ContentMerger.java

@@ -291,7 +291,7 @@ private static _ContentTriple getContentRevisions(Set<Content<SpoonNode, RoledVa
         }
 
         if (left == null || right == null)
-            throw new IllegalStateException("Expected at least left and right revisions, got: " + contents);
+            throw new IllegalArgumentException("Expected at least left and right revisions, got: " + contents);
 
         return new _ContentTriple(Optional.ofNullable(base), left, right);
     }

src/main/java/se/kth/spork/spoon/pcsinterpreter/PcsInterpreter.java

@@ -27,7 +27,7 @@ public static Pair<CtElement, Integer> fromMergedPcs(
             SpoonMapping baseLeft,
             SpoonMapping baseRight) {
         SporkTreeBuilder sporkTreeBuilder = new SporkTreeBuilder(delta);
-        SporkTree sporkTreeRoot = sporkTreeBuilder.build(NodeFactory.ROOT);
+        SporkTree sporkTreeRoot = sporkTreeBuilder.buildTree();
 
         // this is a bit of a hack, get any used environment such that the SpoonTreeBuilder can copy environment
         // details

src/main/java/se/kth/spork/spoon/pcsinterpreter/SporkTreeBuilder.java

@@ -1,6 +1,7 @@
 package se.kth.spork.spoon.pcsinterpreter;
 
 import se.kth.spork.base3dm.*;
+import se.kth.spork.exception.ConflictException;
 import se.kth.spork.spoon.wrappers.NodeFactory;
 import se.kth.spork.spoon.wrappers.RoledValues;
 import se.kth.spork.spoon.wrappers.SpoonNode;
@@ -27,6 +28,10 @@ class SporkTreeBuilder {
     // if any node is added twice, there's an unresolved move conflict
     private Set<SpoonNode> usedNodes;
 
+    // keeps track of all structural inconsistencies that are used
+    // if any have not been used when the tree has been built, there's something wrong
+    private Set<Pcs<SpoonNode>> remainingInconsistencies;
+
     /**
      * Create a builder.
      *
@@ -38,6 +43,8 @@ public SporkTreeBuilder(ChangeSet<SpoonNode, RoledValues> delta) {
         contents = delta.getContents();
         numStructuralConflicts = 0;
         usedNodes = new HashSet<>();
+        remainingInconsistencies = new HashSet<>();
+        structuralConflicts.values().forEach(remainingInconsistencies::addAll);
     }
 
     private static <T extends ListNode> Map<T, Map<T, Pcs<T>>> buildRootToChildren(Set<Pcs<T>> pcses) {
@@ -75,6 +82,14 @@ public int numStructuralConflicts() {
         return numStructuralConflicts;
     }
 
+    public SporkTree buildTree() {
+        SporkTree tree = build(NodeFactory.ROOT);
+        if (!remainingInconsistencies.isEmpty()) {
+            throw new ConflictException("Unhandled inconsistencies remain: " + remainingInconsistencies);
+        }
+        return tree;
+    }
+
     /**
      * Build a subtree of the {@link ChangeSet} contained in this builder. The {@link SporkTree} that's returned
      * contains all information required to build the final Spoon tree, including structural conflict information.
@@ -124,6 +139,9 @@ private SpoonNode traverseConflict(
             Pcs<SpoonNode> conflicting,
             Map<SpoonNode, Pcs<SpoonNode>> children,
             SporkTree tree) {
+        remainingInconsistencies.remove(nextPcs);
+        remainingInconsistencies.remove(conflicting);
+
         SpoonNode next = nextPcs.getSuccessor();
         Arrays.asList(Revision.LEFT, Revision.RIGHT).forEach(tree::addRevision);
         Pcs<SpoonNode> leftPcs = nextPcs.getRevision() == Revision.LEFT ? nextPcs : conflicting;
@@ -152,7 +170,7 @@ private SpoonNode traverseConflict(
     private void addChild(SporkTree tree, SporkTree child) {
         if (usedNodes.contains(child.getNode())) {
             // if this happens, then there is a duplicate node in the tree, indicating a move conflict
-            throw new IllegalStateException("Move conflict detected");
+            throw new ConflictException("Move conflict detected");
         }
         tree.addChild(child);
         usedNodes.add(child.getNode());
@@ -174,14 +192,15 @@ private List<SpoonNode> extractConflictList(Pcs<SpoonNode> pcs, Map<SpoonNode, P
                         .filter(confPcs -> StructuralConflict.isPredecessorConflict(finalPcs, confPcs)).findFirst();
 
                 if (predConflict.isPresent()) {
+                    remainingInconsistencies.remove(predConflict.get());
                     return nodes;
                 }
             }
 
             SpoonNode nextNode = pcs.getSuccessor();
 
             if (nextNode.isEndOfList())
-                throw new IllegalStateException(
+                throw new ConflictException(
                         "Reached the end of the child list without finding a predecessor conflict");
 
             nodes.add(nextNode);

src/main/java/se/kth/spork/spoon/printer/PrinterPreprocessor.java

@@ -1,5 +1,6 @@
 package se.kth.spork.spoon.printer;
 
+import se.kth.spork.exception.ConflictException;
 import se.kth.spork.spoon.ContentConflict;
 import se.kth.spork.spoon.pcsinterpreter.ContentMerger;
 import se.kth.spork.spoon.wrappers.RoledValue;
@@ -182,7 +183,7 @@ private void processConflict(ContentConflict conflict, CtElement element) {
                 localPrinterMap.put(leftStr, Pair.of(leftStr, rightStr));
                 break;
             default:
-                throw new IllegalStateException("Unhandled conflict: " + leftVal + ", " + rightVal);
+                throw new ConflictException("Unhandled conflict: " + leftVal + ", " + rightVal);
         }
 
         if (!localPrinterMap.isEmpty()) {

src/test/java/se/kth/spork/Util.java

@@ -15,6 +15,7 @@
 import java.nio.file.Path;
 import java.nio.file.Paths;
 import java.util.*;
+import java.util.function.Function;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
 import java.util.stream.Stream;
@@ -29,61 +30,18 @@ public class Util {
     public static final Path BOTH_MODIFIED_DIRPATH = CLEAN_MERGE_DIRPATH.resolve("both_modified");
     public static final Path LEFT_MODIFIED_DIRPATH = CLEAN_MERGE_DIRPATH.resolve("left_modified");
     public static final Path CONFLICT_DIRPATH = Paths.get("src/test/resources/conflict");
+    public static final Path UNHANDLED_INCONSISTENCY_PATH = Paths.get("src/test/resources/unhandled_inconsistency");
 
-    /**
-     * Provides test sources for scenarios where both left and right revisions are modified.
-     */
-    public static class BothModifiedSourceProvider implements ArgumentsProvider {
-        @Override
-        public Stream<? extends Arguments> provideArguments(ExtensionContext extensionContext) {
-            return getArgumentSourcesStream(BOTH_MODIFIED_DIRPATH.toFile());
-        }
-    }
-
-    /**
-     * Provides test sources for scenarios where left is modified.
-     */
-    public static class LeftModifiedSourceProvider implements ArgumentsProvider {
-        @Override
-        public Stream<? extends Arguments> provideArguments(ExtensionContext extensionContext) {
-            return getArgumentSourcesStream(LEFT_MODIFIED_DIRPATH.toFile());
-        }
-    }
-
-    /**
-     * Provides test sources for scenarios where right is modified.
-     */
-    public static class RightModifiedSourceProvider implements ArgumentsProvider {
-        @Override
-        public Stream<? extends Arguments> provideArguments(ExtensionContext extensionContext) {
-            return getArgumentSourcesStream(LEFT_MODIFIED_DIRPATH.toFile()).map(
-                    arg -> {
-                        TestSources sources = (TestSources) arg.get()[0];
-                        // swap left and right around to make this a "right modified" test case
-                        Path left = sources.left;
-                        sources.left = sources.right;
-                        sources.right = left;
-                        return Arguments.of(sources);
-                    }
-            );
-        }
-    }
 
-    /**
-     * Provides test sources for scenarios where there are conflicts.
-     */
-    public static class ConflictSourceProvider implements ArgumentsProvider {
-        @Override
-        public Stream<? extends Arguments> provideArguments(ExtensionContext extensionContext) {
-            return getArgumentSourcesStream(CONFLICT_DIRPATH.toFile());
-        }
+    private static Stream<? extends Arguments> getArgumentSourcesStream(File testDir) {
+        return getArgumentSourcesStream(testDir, TestSources::fromTestDirectory);
     }
 
-    private static Stream<? extends Arguments> getArgumentSourcesStream(File testDir) {
+    private static Stream<? extends Arguments> getArgumentSourcesStream(File testDir, Function<File, TestSources> sourceGetter) {
         return Arrays.stream(testDir.listFiles())
                 .filter(File::isDirectory)
                 .filter(f -> !f.getName().startsWith("IGNORE"))
-                .map(TestSources::fromTestDirectory)
+                .map(sourceGetter)
                 .map(Arguments::of);
     }
 
@@ -142,6 +100,62 @@ public static String keepLeftConflict(String string) {
         return leftMarkerMatcher.replaceAll("");
     }
 
+    /**
+     * Provides test sources for scenarios where both left and right revisions are modified.
+     */
+    public static class BothModifiedSourceProvider implements ArgumentsProvider {
+        @Override
+        public Stream<? extends Arguments> provideArguments(ExtensionContext extensionContext) {
+            return getArgumentSourcesStream(BOTH_MODIFIED_DIRPATH.toFile());
+        }
+    }
+
+    /**
+     * Provides test sources for scenarios where left is modified.
+     */
+    public static class LeftModifiedSourceProvider implements ArgumentsProvider {
+        @Override
+        public Stream<? extends Arguments> provideArguments(ExtensionContext extensionContext) {
+            return getArgumentSourcesStream(LEFT_MODIFIED_DIRPATH.toFile());
+        }
+    }
+
+    /**
+     * Provides test sources for scenarios where right is modified.
+     */
+    public static class RightModifiedSourceProvider implements ArgumentsProvider {
+        @Override
+        public Stream<? extends Arguments> provideArguments(ExtensionContext extensionContext) {
+            return getArgumentSourcesStream(LEFT_MODIFIED_DIRPATH.toFile()).map(
+                    arg -> {
+                        TestSources sources = (TestSources) arg.get()[0];
+                        // swap left and right around to make this a "right modified" test case
+                        Path left = sources.left;
+                        sources.left = sources.right;
+                        sources.right = left;
+                        return Arguments.of(sources);
+                    }
+            );
+        }
+    }
+
+    public static class UnhandledInconsistencyProvider implements ArgumentsProvider {
+        @Override
+        public Stream<? extends Arguments> provideArguments(ExtensionContext extensionContext) throws Exception {
+            return getArgumentSourcesStream(UNHANDLED_INCONSISTENCY_PATH.toFile(), TestSources::fromTestDirectoryWithoutExpected);
+        }
+    }
+
+    /**
+     * Provides test sources for scenarios where there are conflicts.
+     */
+    public static class ConflictSourceProvider implements ArgumentsProvider {
+        @Override
+        public Stream<? extends Arguments> provideArguments(ExtensionContext extensionContext) {
+            return getArgumentSourcesStream(CONFLICT_DIRPATH.toFile());
+        }
+    }
+
     public static class Conflict {
         String left;
         String right;
@@ -169,7 +183,6 @@ public int hashCode() {
         }
     }
 
-
     public static class TestSources {
         public Path base;
         public Path left;
@@ -194,9 +207,20 @@ public static TestSources fromTestDirectory(File testDir) {
             );
         }
 
+        public static TestSources fromTestDirectoryWithoutExpected(File testDir) {
+            Path path = testDir.toPath();
+            return new TestSources(
+                    path.resolve("Base.java"),
+                    path.resolve("Left.java"),
+                    path.resolve("Right.java"),
+                    null
+            );
+        }
+
         @Override
         public String toString() {
             return base.getParent().getFileName().toString();
         }
     }
+
 }

src/test/java/se/kth/spork/spoon/Spoon3dmMergeTest.java

@@ -4,6 +4,7 @@
 import org.junit.jupiter.params.provider.ArgumentsSource;
 import se.kth.spork.Util;
 import se.kth.spork.cli.Cli;
+import se.kth.spork.exception.ConflictException;
 import se.kth.spork.util.Pair;
 import spoon.reflect.declaration.*;
 
@@ -31,6 +32,15 @@ void mergeToTree_shouldReturnExpectedTree_whenBothVersionsAreModified(Util.TestS
         runTestMerge(sources);
     }
 
+    @ParameterizedTest
+    @ArgumentsSource(Util.UnhandledInconsistencyProvider.class)
+    void merge_shouldThrow_onUnhandledInconsistencies(Util.TestSources sources) {
+        assertThrows(
+                ConflictException.class,
+                () -> Spoon3dmMerge.merge(sources.base, sources.left, sources.right)
+        );
+    }
+
     private static void runTestMerge(Util.TestSources sources) {
         CtModule expected = Parser.parse(sources.expected);
         Object expectedImports = expected.getMetadata(Parser.IMPORT_STATEMENTS);

src/test/resources/unhandled_inconsistency/intra_parent_move_conflict/Base.java

@@ -0,0 +1,9 @@
+class Main {
+    public static void main(String[] args) {
+        if (true) {
+        }
+        int a = 2;
+        if (true) {
+        }
+    }
+}

src/test/resources/unhandled_inconsistency/intra_parent_move_conflict/Left.java

@@ -0,0 +1,9 @@
+class Main {
+    public static void main(String[] args) {
+        int a = 2;
+        if (true) {
+        }
+        if (true) {
+        }
+    }
+}

src/test/resources/unhandled_inconsistency/intra_parent_move_conflict/Right.java

@@ -0,0 +1,9 @@
+class Main {
+    public static void main(String[] args) {
+        if (true) {
+        }
+        if (true) {
+        }
+        int a = 2;
+    }
+}