[fix] Properly handle modifier merge conflict #43

Author: slarse

src/main/java/se/kth/spork/cli/PrinterPreprocessor.java

@@ -2,18 +2,19 @@
 
 import se.kth.spork.base3dm.Revision;
 import se.kth.spork.spoon.ContentConflict;
+import se.kth.spork.spoon.ContentMerger;
 import se.kth.spork.spoon.RoledValue;
 import se.kth.spork.util.LineBasedMerge;
 import se.kth.spork.util.Pair;
 import spoon.reflect.code.CtComment;
 import spoon.reflect.cu.SourcePosition;
 import spoon.reflect.declaration.CtElement;
+import spoon.reflect.declaration.ModifierKind;
+import spoon.reflect.path.CtRole;
 import spoon.reflect.visitor.CtScanner;
 import spoon.reflect.visitor.printer.CommentOffset;
 
-import java.util.HashMap;
-import java.util.List;
-import java.util.Map;
+import java.util.*;
 
 /**
  * A pre-processor that must run before pretty-printing a merged tree. It does things like embedding conflict values
@@ -72,6 +73,7 @@ private static void unsetSourcePosition(CtElement element) {
      * @param conflict A content conflict.
      * @param element The element associated with the conflict.
      */
+    @SuppressWarnings("unchecked")
     private void processConflict(ContentConflict conflict, CtElement element) {
         Object leftVal = conflict.getLeft().getValue();
         Object rightVal = conflict.getRight().getValue();
@@ -106,6 +108,25 @@ private void processConflict(ContentConflict conflict, CtElement element) {
                     printerMap.put("super", Pair.of(Revision.RIGHT, "extends"));
                 }
                 break;
+            case MODIFIER:
+                Collection<ModifierKind> leftMods = (Collection<ModifierKind>) leftVal;
+                Collection<ModifierKind> rightMods = (Collection<ModifierKind>) rightVal;
+                Set<ModifierKind> leftVisibilities = ContentMerger.categorizeModifiers(leftMods).first;
+                Set<ModifierKind> rightVisibilities = ContentMerger.categorizeModifiers(rightMods).first;
+
+                if (leftVisibilities.isEmpty()) {
+                    // use the right-hand visibility in actual tree to force something to be printed
+                    Collection<ModifierKind> mods = element.getValueByRole(CtRole.MODIFIER);
+                    ModifierKind rightVis = rightVisibilities.iterator().next();
+                    mods.add(rightVis);
+                    element.setValueByRole(CtRole.MODIFIER, mods);
+                    printerMap.put(rightVis.toString(), Pair.of(Revision.LEFT, ""));
+                } else {
+                    String leftVisStr = leftVisibilities.iterator().next().toString();
+                    String rightVisStr = rightVisibilities.isEmpty()
+                            ? "" : rightVisibilities.iterator().next().toString();
+                    printerMap.put(leftVisStr, Pair.of(Revision.RIGHT, rightVisStr));
+                }
         }
 
         if (!printerMap.isEmpty()) {

src/main/java/se/kth/spork/cli/SporkPrettyPrinter.java

@@ -32,6 +32,10 @@ public SporkPrettyPrinter(Environment env) {
         printerConflictMaps = new ArrayDeque<>();
         setPrinterTokenWriter(new DefaultTokenWriter(printerHelper));
 
+        // This is required to avoid NullPointerExceptions when debugging, as the debugger calls toString from time
+        // to time
+        printerConflictMaps.push(Optional.empty());
+
         // this line is SUPER important because without it implicit elements will be printed. For example,
         // instead of just String, it will print java.lang.String. Which isn't great.
         setIgnoreImplicit(false);

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

@@ -76,6 +76,10 @@ static void handleContentConflicts(TStar<SpoonNode, RoledValues> delta) {
 
                     Optional<?> merged = Optional.empty();
 
+                    // sometimes a value can be partially merged (e.g. modifiers), and then we want to be
+                    // able to set the merged value, AND flag a conflict.
+                    boolean conflictPresent = false;
+
                     // if either value is equal to base, we keep THE OTHER one
                     if (baseValOpt.isPresent() && baseValOpt.get().equals(leftVal)) {
                         merged = Optional.of(rightVal);
@@ -93,10 +97,12 @@ static void handleContentConflicts(TStar<SpoonNode, RoledValues> delta) {
                                 }
                                 break;
                             case MODIFIER:
-                                merged = mergeModifierKinds(
+                                Pair<Boolean, Optional<Set<ModifierKind>>> mergePair = mergeModifierKinds(
                                         baseValOpt.map(o -> (Set<ModifierKind>) o),
                                         (Set<ModifierKind>) leftVal,
                                         (Set<ModifierKind>) rightVal);
+                                conflictPresent = mergePair.first;
+                                merged = mergePair.second;
                                 break;
                             case COMMENT_CONTENT:
                                 merged = mergeComments(baseValOpt.orElse(""), leftVal, rightVal);
@@ -116,7 +122,9 @@ static void handleContentConflicts(TStar<SpoonNode, RoledValues> delta) {
 
                     if (merged.isPresent()) {
                         mergedRoledValues.set(i, role, merged.get());
-                        unresolvedConflicts.pop();
+
+                        if (!conflictPresent)
+                            unresolvedConflicts.pop();
                     }
                 }
 
@@ -167,6 +175,28 @@ static void handleContentConflicts(TStar<SpoonNode, RoledValues> delta) {
         return Triple.of(visibility, keywords, other);
     }
 
+    /**
+     * Separate modifiers into visibility (public, private, protected), keywords (static, final) and all
+     * others.
+     *
+     * @param modifiers A collection of modifiers.
+     * @return A triple with visibility in first, keywords in second and other in third.
+     */
+    public static Triple<Set<ModifierKind>, Set<ModifierKind>, Set<ModifierKind>>
+    categorizeModifiers(Collection<ModifierKind> modifiers) {
+        return categorizeModifiers(modifiers.stream());
+    }
+
+    /**
+     * Extract the visibility modifier(s).
+     *
+     * @param modifiers A collection of modifiers.
+     * @return A possibly empty set of visibility modifiers.
+     */
+    public static Set<ModifierKind> getVisibility(Collection<ModifierKind> modifiers) {
+        return categorizeModifiers(modifiers).first;
+    }
+
     private static Optional<?> mergeIsUpper(Optional<CtElement> baseElem, CtElement leftElem, CtElement rightElem) {
         CtWildcardReference left = (CtWildcardReference) leftElem;
         CtWildcardReference right = (CtWildcardReference) rightElem;
@@ -201,16 +231,23 @@ private static Optional<?> mergeComments(Object base, Object left, Object right)
         return Optional.of(merge.first);
     }
 
-    private static Optional<Set<ModifierKind>>
+    /**
+     * Return a pair (conflict, mergedModifiers).
+     * If the conflict value is true, there is a conflict in the visibility modifiers, and the merged value
+     * will always be the left one.
+     */
+    private static Pair<Boolean, Optional<Set<ModifierKind>>>
     mergeModifierKinds(Optional<Set<ModifierKind>> base, Set<ModifierKind> left, Set<ModifierKind> right) {
-        // all revisions must have the same primary value
-
         Set<ModifierKind> baseModifiers = base.orElseGet(HashSet::new);
 
         Stream<ModifierKind> modifiers = Stream.of(baseModifiers, left, right).flatMap(Set::stream);
         Triple<Set<ModifierKind>, Set<ModifierKind>, Set<ModifierKind>>
                 categorizedMods = categorizeModifiers(modifiers);
 
+        Set<ModifierKind> baseVis = getVisibility(baseModifiers);
+        Set<ModifierKind> leftVis = getVisibility(left);
+        Set<ModifierKind> rightVis = getVisibility(right);
+
         Set<ModifierKind> visibility = categorizedMods.first;
         Set<ModifierKind> keywords = categorizedMods.second;
         Set<ModifierKind> other = categorizedMods.third;
@@ -221,8 +258,12 @@ private static Optional<?> mergeComments(Object base, Object left, Object right)
 
         // visibility is the only place where we can have obvious addition conflicts
         // TODO further analyze conflicts among other modifiers (e.g. you can't combine static and volatile)
-        if (visibility.size() != 1) {
-            return Optional.empty();
+        boolean conflict = visibility.size() != 1 ||
+                !leftVis.equals(rightVis) && !leftVis.equals(baseVis) && !rightVis.equals(baseVis);
+
+        if (conflict) {
+            // use left version on conflict to follow the convention
+            visibility = leftVis;
         }
 
         Set<ModifierKind> mods = Stream.of(visibility, keywords, other).flatMap(Set::stream)
@@ -235,7 +276,7 @@ private static Optional<?> mergeComments(Object base, Object left, Object right)
                 )
                 .collect(Collectors.toSet());
 
-        return Optional.of(mods);
+        return Pair.of(conflict,Optional.of(mods));
     }
 
     private static _ContentTriple getContentRevisions(Set<Content<SpoonNode, RoledValues>> contents) {

src/test/resources/conflict/conflicting_class_visibility/Base.java

@@ -0,0 +1 @@
+static class cls {}

src/test/resources/conflict/conflicting_class_visibility/Expected.java

@@ -0,0 +1,6 @@
+<<<<<<< LEFT
+public
+=======
+private
+>>>>>>> RIGHT
+ static class Cls {}

src/test/resources/conflict/conflicting_class_visibility/Left.java

@@ -0,0 +1 @@
+public static class Cls {}

src/test/resources/conflict/conflicting_class_visibility/Right.java

@@ -0,0 +1 @@
+private static class Cls {}

src/test/resources/conflict/conflicting_method_visibility/Base.java

@@ -0,0 +1,3 @@
+class Cls {
+    void method() {}
+}

src/test/resources/conflict/conflicting_method_visibility/Expected.java

@@ -0,0 +1,8 @@
+class Cls {
+<<<<<<< LEFT
+public
+=======
+private
+>>>>>>> RIGHT
+    void method() {}
+}

src/test/resources/conflict/conflicting_method_visibility/Left.java

@@ -0,0 +1,3 @@
+class Cls {
+    public void method() {}
+}

src/test/resources/conflict/conflicting_method_visibility/Right.java

@@ -0,0 +1,3 @@
+class Cls {
+    private void method() {}
+}

src/test/resources/conflict/method_visibility_left_empty/Base.java

@@ -0,0 +1,3 @@
+class Cls {
+    public void method() {}
+}

src/test/resources/conflict/method_visibility_left_empty/Expected.java

@@ -0,0 +1,7 @@
+class Cls {
+<<<<<<< LEFT
+=======
+protected
+>>>>>>> RIGHT
+     final static void method() {}
+}

src/test/resources/conflict/method_visibility_left_empty/Left.java

@@ -0,0 +1,3 @@
+class Cls {
+    final void method() {}
+}

src/test/resources/conflict/method_visibility_left_empty/Right.java

@@ -0,0 +1,3 @@
+class Cls {
+    protected static void method() {}
+}