From c69c3b8eccc37a0ca206aeef7c98e887fb5483a4 Mon Sep 17 00:00:00 2001 From: slarse Date: Sat, 23 May 2020 11:14:05 +0200 Subject: [PATCH 1/9] Get initial idea to work --- .../java/se/kth/spork/base3dm/ChangeSet.java | 10 +- .../java/se/kth/spork/base3dm/ListNode.java | 7 + src/main/java/se/kth/spork/base3dm/Pcs.java | 4 + .../java/se/kth/spork/spoon/PcsBuilder.java | 54 ++++--- .../se/kth/spork/spoon/Spoon3dmMerge.java | 10 +- .../spoon/matching/ClassRepresentatives.java | 31 +++- .../spork/spoon/matching/SpoonMapping.java | 4 + .../pcsinterpreter/SporkTreeBuilder.java | 46 +++--- .../kth/spork/spoon/wrappers/NodeFactory.java | 145 ++++++++++++++++-- .../kth/spork/spoon/wrappers/SpoonNode.java | 4 + 10 files changed, 251 insertions(+), 64 deletions(-) diff --git a/src/main/java/se/kth/spork/base3dm/ChangeSet.java b/src/main/java/se/kth/spork/base3dm/ChangeSet.java index e3c28af0..5e653f6c 100644 --- a/src/main/java/se/kth/spork/base3dm/ChangeSet.java +++ b/src/main/java/se/kth/spork/base3dm/ChangeSet.java @@ -196,7 +196,7 @@ private void add(Set> tree, Function getContent) { addToLookupTable(classRepPred, classRepPcs, predecessors); addToLookupTable(classRepSucc, classRepPcs, successors); } - if (!pred.isListEdge()) { + if (!pred.isVirtual()) { Content c = new Content(pcs, getContent.apply(pred)); addToLookupTable(classRepPred, c, content); } @@ -204,10 +204,10 @@ private void add(Set> tree, Function getContent) { } private Pcs addToStar(Pcs pcs) { - T root = pcs.getRoot(); - T pred = pcs.getPredecessor(); - T succ = pcs.getSuccessor(); - Pcs classRepPcs = new Pcs(classRepMap.get(root), classRepMap.get(pred), classRepMap.get(succ), pcs.getRevision()); + T root = classRepMap.get(pcs.getRoot()); + T pred = classRepMap.get(pcs.getPredecessor()); + T succ = classRepMap.get(pcs.getSuccessor()); + Pcs classRepPcs = new Pcs(root, pred, succ, pcs.getRevision()); pcsSet.add(classRepPcs); return classRepPcs; } diff --git a/src/main/java/se/kth/spork/base3dm/ListNode.java b/src/main/java/se/kth/spork/base3dm/ListNode.java index 2ecfea28..16503259 100644 --- a/src/main/java/se/kth/spork/base3dm/ListNode.java +++ b/src/main/java/se/kth/spork/base3dm/ListNode.java @@ -27,4 +27,11 @@ default boolean isEndOfList() { default boolean isListEdge() { return isStartOfList() || isEndOfList(); } + + /** + * @return true iff this node is a virtual node. + */ + default boolean isVirtual() { + return isListEdge(); + } } diff --git a/src/main/java/se/kth/spork/base3dm/Pcs.java b/src/main/java/se/kth/spork/base3dm/Pcs.java index bc2e1428..615f8ca6 100644 --- a/src/main/java/se/kth/spork/base3dm/Pcs.java +++ b/src/main/java/se/kth/spork/base3dm/Pcs.java @@ -21,6 +21,10 @@ public class Pcs { * @param revision The revision this PCS is related to. */ public Pcs(T root, T predecessor, T successor, Revision revision) { + if (root == null || predecessor == null || successor == null) { + throw new IllegalArgumentException("nodes may not be null"); + } + this.root = root; this.predecessor = predecessor; this.successor = successor; diff --git a/src/main/java/se/kth/spork/spoon/PcsBuilder.java b/src/main/java/se/kth/spork/spoon/PcsBuilder.java index 8e9b669a..31506ebb 100644 --- a/src/main/java/se/kth/spork/spoon/PcsBuilder.java +++ b/src/main/java/se/kth/spork/spoon/PcsBuilder.java @@ -15,7 +15,7 @@ * @author Simon Larsén */ class PcsBuilder extends CtScanner { - private Map rootTolastSibling = new HashMap<>(); + private Map parentToLastSibling = new HashMap<>(); private Set> pcses = new HashSet<>(); private SpoonNode root = null; private Revision revision; @@ -23,7 +23,7 @@ class PcsBuilder extends CtScanner { public PcsBuilder(Revision revision) { super(); this.revision = revision; - rootTolastSibling.put(NodeFactory.ROOT, NodeFactory.startOfChildList(NodeFactory.ROOT)); + parentToLastSibling.put(NodeFactory.ROOT, NodeFactory.startOfChildList(NodeFactory.ROOT)); } /** @@ -36,33 +36,51 @@ public PcsBuilder(Revision revision) { public static Set> fromSpoon(CtElement spoonClass, Revision revision) { PcsBuilder scanner = new PcsBuilder(revision); scanner.scan(spoonClass); + scanner.finishPcses(); return scanner.getPcses(); } @Override protected void enter(CtElement e) { - SpoonNode wrapped = NodeFactory.wrap(e); - if (root == null) + SpoonNode wrapped; + SpoonNode parent; + + if (root == null) { + parent = NodeFactory.ROOT; + wrapped = NodeFactory.initializeWrapper(e, parent); root = wrapped; + } else { + NodeFactory.Node actualParent = NodeFactory.wrap(e.getParent()); + wrapped = NodeFactory.initializeRoledWrapper(e, actualParent); + parent = wrapped.getParent(); + } - rootTolastSibling.put(wrapped, NodeFactory.startOfChildList(wrapped)); + parentToLastSibling.put(wrapped, NodeFactory.startOfChildList(wrapped)); - SpoonNode parent = wrapped.getParent(); - SpoonNode predecessor = rootTolastSibling.get(parent); + SpoonNode predecessor = parentToLastSibling.getOrDefault(parent, NodeFactory.startOfChildList(parent)); pcses.add(new Pcs<>(parent, predecessor, wrapped, revision)); - rootTolastSibling.put(parent, wrapped); + parentToLastSibling.put(parent, wrapped); } - @Override - protected void exit(CtElement e) { - SpoonNode current = NodeFactory.wrap(e); - SpoonNode predecessor = rootTolastSibling.get(current); - SpoonNode successor = NodeFactory.endOfChildList(current); - pcses.add(new Pcs<>(current, predecessor, successor, revision)); - - if (current.getParent() == NodeFactory.ROOT) { - // need to finish the virtual root's child list artificially as it is not a real node - pcses.add(new Pcs<>(NodeFactory.ROOT, current, NodeFactory.endOfChildList(NodeFactory.ROOT), revision)); + private void finishPcses() { + for (Map.Entry nodePair : parentToLastSibling.entrySet()) { + SpoonNode parent = nodePair.getKey(); + SpoonNode lastSibling = nodePair.getValue(); + if (parent.isVirtual()) { + // this is either the virtual root, or a RoleNode + // we just need to close their child lists + pcses.add(new Pcs<>(parent, lastSibling, NodeFactory.endOfChildList(parent), revision)); + } else { + // this is a concrete node, we must add all of its virtual children to the PCS structure + List virtualNodes = parent.getVirtualNodes(); + assert lastSibling.isStartOfList(); + assert virtualNodes.get(0).equals(lastSibling); + SpoonNode pred = lastSibling; + for (SpoonNode succ : virtualNodes.subList(1, virtualNodes.size())) { + pcses.add(new Pcs<>(parent, pred, succ, revision)); + pred = succ; + } + } } } diff --git a/src/main/java/se/kth/spork/spoon/Spoon3dmMerge.java b/src/main/java/se/kth/spork/spoon/Spoon3dmMerge.java index b7d0f99a..bf7ec145 100644 --- a/src/main/java/se/kth/spork/spoon/Spoon3dmMerge.java +++ b/src/main/java/se/kth/spork/spoon/Spoon3dmMerge.java @@ -77,6 +77,11 @@ public static Pair merge(T base, T left, T rig Matcher baseRightGumtreeMatch = matchTrees(baseGumtree, rightGumtree); Matcher leftRightGumtreeMatch = matchTreesXY(leftGumtree, rightGumtree); + LOGGER.info(() -> "Converting Spoon trees to PCS triples"); + Set> t0 = PcsBuilder.fromSpoon(base, Revision.BASE); + Set> t1 = PcsBuilder.fromSpoon(left, Revision.LEFT); + Set> t2 = PcsBuilder.fromSpoon(right, Revision.RIGHT); + LOGGER.info(() -> "Converting GumTree matches to Spoon matches"); SpoonMapping baseLeft = SpoonMapping.fromGumTreeMapping(baseLeftGumtreeMatch.getMappings()); SpoonMapping baseRight = SpoonMapping.fromGumTreeMapping(baseRightGumtreeMatch.getMappings()); @@ -87,11 +92,6 @@ public static Pair merge(T base, T left, T rig Map classRepMap = ClassRepresentatives.createClassRepresentativesMapping( base, left, right, baseLeft, baseRight, leftRight); - LOGGER.info(() -> "Converting Spoon trees to PCS triples"); - Set> t0 = PcsBuilder.fromSpoon(base, Revision.BASE); - Set> t1 = PcsBuilder.fromSpoon(left, Revision.LEFT); - Set> t2 = PcsBuilder.fromSpoon(right, Revision.RIGHT); - LOGGER.info(() -> "Computing raw PCS merge"); ChangeSet delta = new ChangeSet<>(classRepMap, new ContentResolver(), t0, t1, t2); ChangeSet t0Star = new ChangeSet<>(classRepMap, new ContentResolver(), t0); diff --git a/src/main/java/se/kth/spork/spoon/matching/ClassRepresentatives.java b/src/main/java/se/kth/spork/spoon/matching/ClassRepresentatives.java index c5659176..f6cfa587 100644 --- a/src/main/java/se/kth/spork/spoon/matching/ClassRepresentatives.java +++ b/src/main/java/se/kth/spork/spoon/matching/ClassRepresentatives.java @@ -5,10 +5,12 @@ import se.kth.spork.spoon.wrappers.SpoonNode; import se.kth.spork.spoon.wrappers.NodeFactory; import spoon.reflect.declaration.CtElement; +import spoon.reflect.reference.CtReference; import spoon.reflect.visitor.CtScanner; import java.util.HashMap; import java.util.Iterator; +import java.util.List; import java.util.Map; /** @@ -99,6 +101,17 @@ private static void mapToClassRepresentatives(CtElement tree, SpoonMapping mappi CtElement t = descIt.next(); mapToClassRep(mappings, classRepMap, rev, t); } + + descIt = tree.descendantIterator(); + while (descIt.hasNext()) { + CtElement t = descIt.next(); + SpoonNode node = NodeFactory.wrap(t); + SpoonNode parent = node.getParent(); + SpoonNode parentClassRep = classRepMap.get(parent); + if (parentClassRep == null) { + System.out.println(); + } + } } private static void mapToClassRep(SpoonMapping mappings, Map classRepMap, Revision rev, CtElement t) { @@ -126,13 +139,19 @@ private static void mapNodes(SpoonNode from, SpoonNode to, Map fromVirtualNodes = from.getVirtualNodes(); + List toVirtualNodes = to.getVirtualNodes(); + + for (int i = 0; i < fromVirtualNodes.size(); i++) { + SpoonNode fromVirt = fromVirtualNodes.get(i); + SpoonNode toVirt = toVirtualNodes.get(i); - classRepMap.put(fromSOL, toSOL); - classRepMap.put(fromEOL, toEOL); + if (fromVirt.isListEdge()) { + classRepMap.put(fromVirt, toVirt); + } else { + mapNodes(fromVirt, toVirt, classRepMap); + } + } } /** diff --git a/src/main/java/se/kth/spork/spoon/matching/SpoonMapping.java b/src/main/java/se/kth/spork/spoon/matching/SpoonMapping.java index 7947da5a..36244402 100644 --- a/src/main/java/se/kth/spork/spoon/matching/SpoonMapping.java +++ b/src/main/java/se/kth/spork/spoon/matching/SpoonMapping.java @@ -4,6 +4,7 @@ import com.github.gumtreediff.matchers.MappingStore; import com.github.gumtreediff.tree.ITree; import com.github.gumtreediff.utils.Pair; +import gumtree.spoon.builder.CtWrapper; import gumtree.spoon.builder.SpoonGumTreeBuilder; import se.kth.spork.spoon.wrappers.SpoonNode; import se.kth.spork.spoon.wrappers.NodeFactory; @@ -98,6 +99,9 @@ private static boolean ignoreMapping(CtElement src, CtElement dst) { return true; } else if (isPrimitiveType(src) != isPrimitiveType(dst)) { return true; + } else if (src instanceof CtWrapper || dst instanceof CtWrapper) { + // the CtWrapper elements do not represent real Spoon nodes, and so are just noise + return true; } return false; } diff --git a/src/main/java/se/kth/spork/spoon/pcsinterpreter/SporkTreeBuilder.java b/src/main/java/se/kth/spork/spoon/pcsinterpreter/SporkTreeBuilder.java index 8b68b50a..c61b107d 100644 --- a/src/main/java/se/kth/spork/spoon/pcsinterpreter/SporkTreeBuilder.java +++ b/src/main/java/se/kth/spork/spoon/pcsinterpreter/SporkTreeBuilder.java @@ -107,7 +107,6 @@ public SporkTree buildTree() { public SporkTree build(SpoonNode currentRoot) { Map> children = rootToChildren.get(currentRoot); - SpoonNode next = NodeFactory.startOfChildList(currentRoot); Set> currentContent = contents.getOrDefault(currentRoot, Collections.emptySet()); SporkTree tree = new SporkTree(currentRoot, currentContent); @@ -116,15 +115,35 @@ public SporkTree build(SpoonNode currentRoot) { return tree; try { - while (true) { - Pcs nextPcs = children.get(next); - tree.addRevision(nextPcs.getRevision()); + build(NodeFactory.startOfChildList(currentRoot), tree, children); + } catch (NullPointerException | ConflictException e) { + // could not resolve the child list + // TODO improve design, should not have to catch exceptions like this + LOGGER.warn(() -> + "Failed to resolve child list of " + currentRoot.getElement().getShortRepresentation() + + ". Falling back to line-based merge of this element."); + StructuralConflict conflict = approximateConflict(currentRoot); + tree = new SporkTree(currentRoot, currentContent, conflict); + tree.setRevisions(Arrays.asList(Revision.BASE, Revision.LEFT, Revision.RIGHT)); + } - next = nextPcs.getSuccessor(); - if (next.isEndOfList()) { - break; - } + return tree; + } + + private void build(SpoonNode start, SporkTree tree, Map> children) { + SpoonNode next = start; + while (true) { + Pcs nextPcs = children.get(next); + tree.addRevision(nextPcs.getRevision()); + next = nextPcs.getSuccessor(); + if (next.isEndOfList()) { + break; + } + + if (next.isVirtual()) { + build(NodeFactory.startOfChildList(next), tree, rootToChildren.get(next)); + } else { Set> conflicts = structuralConflicts.get(nextPcs); Optional> successorConflict = conflicts == null ? Optional.empty() : conflicts.stream().filter(confPcs -> @@ -144,18 +163,7 @@ public SporkTree build(SpoonNode currentRoot) { throw new ConflictException("Missed conflict: " + inconsistent); } } - } catch (NullPointerException | ConflictException e) { - // could not resolve the child list - // TODO improve design, should not have to catch exceptions like this - LOGGER.warn(() -> - "Failed to resolve child list of " + currentRoot.getElement().getShortRepresentation() - + ". Falling back to line-based merge of this element."); - StructuralConflict conflict = approximateConflict(currentRoot); - tree = new SporkTree(currentRoot, currentContent, conflict); - tree.setRevisions(Arrays.asList(Revision.BASE, Revision.LEFT, Revision.RIGHT)); } - - return tree; } /** diff --git a/src/main/java/se/kth/spork/spoon/wrappers/NodeFactory.java b/src/main/java/se/kth/spork/spoon/wrappers/NodeFactory.java index 752cfd53..d5024ef7 100644 --- a/src/main/java/se/kth/spork/spoon/wrappers/NodeFactory.java +++ b/src/main/java/se/kth/spork/spoon/wrappers/NodeFactory.java @@ -4,8 +4,17 @@ import se.kth.spork.base3dm.TdmMerge; import spoon.reflect.declaration.CtElement; import spoon.reflect.factory.ModuleFactory; +import spoon.reflect.path.CtRole; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collections; +import java.util.List; +import java.util.Map; import java.util.Objects; +import java.util.TreeMap; +import java.util.stream.Collectors; +import java.util.stream.Stream; /** * Wraps a CtElement and stores the wrapper in the CtElement's metadata. @@ -25,15 +34,27 @@ public class NodeFactory { * @param elem An element to wrap. * @return A wrapper around the CtElement that is more practical for hashing purposes. */ - public static SpoonNode wrap(CtElement elem) { + public static Node wrap(CtElement elem) { Object wrapper = elem.getMetadata(WRAPPER_METADATA); if (wrapper == null) { - wrapper = new Node(elem, currentKey++); - elem.putMetadata(WRAPPER_METADATA, wrapper); + //throw new IllegalStateException("wrapper not initialized for " + elem); + CtElement parent = elem.getParent(); + return initializeWrapper(elem, parent == null ? ROOT : wrap(parent)); } - return (SpoonNode) wrapper; + return (Node) wrapper; + } + + public static Node initializeRoledWrapper(CtElement elem, Node parent) { + SpoonNode effectiveParent = parent.getRoleNode(elem.getRoleInParent()); + return initializeWrapper(elem, effectiveParent); + } + + public static Node initializeWrapper(CtElement elem, SpoonNode parent) { + Node node = new Node(elem, parent, currentKey++); + elem.putMetadata(WRAPPER_METADATA, node); + return node; } /** @@ -60,16 +81,22 @@ public static SpoonNode endOfChildList(SpoonNode elem) { * A simple wrapper class for a Spoon CtElement. The reason it is needed is that the 3DM merge implementation * uses lookup tables, and CtElements have very heavy-duty equals and hash functions. For the purpose of 3DM merge, * only reference equality is needed, not deep equality. - * - * This class should only be instantiated with the CtWrapperFactory singleton. + *

+ * This class should only be instantiated {@link NodeFactory#wrap(CtElement)}. */ - private static class Node implements SpoonNode { + public static class Node implements SpoonNode { private final CtElement element; private final long key; + private final SpoonNode parent; + private final Map childRoles; + private final CtRole role; - Node(CtElement element, long key) { + Node(CtElement element, SpoonNode parent, long key) { this.element = element; this.key = key; + childRoles = new TreeMap<>(); + this.role = element.getRoleInParent(); + this.parent = parent; } @Override @@ -79,9 +106,7 @@ public CtElement getElement() { @Override public SpoonNode getParent() { - if (element instanceof ModuleFactory.CtUnnamedModule) - return NodeFactory.ROOT; - return wrap(element.getParent()); + return parent; } @Override @@ -102,6 +127,22 @@ public Revision getRevision() { return (Revision) element.getMetadata(TdmMerge.REV); } + @Override + public boolean isVirtual() { + return false; + } + + @Override + public List getVirtualNodes() { + return Stream.concat( + Stream.concat( + Stream.of(NodeFactory.startOfChildList(this)), + childRoles.values().stream() + ), + Stream.of(NodeFactory.endOfChildList(this))) + .collect(Collectors.toList()); + } + @Override public boolean equals(Object o) { if (this == o) return true; @@ -114,6 +155,19 @@ public boolean equals(Object o) { public int hashCode() { return Objects.hash(key); } + + public RoleNode getRoleNode(CtRole role) { + RoleNode roleNode = childRoles.get(role); + if (roleNode == null) { + roleNode = new RoleNode(role, this); + childRoles.put(role, roleNode); + } + return roleNode; + } + + public List getChildRoles() { + return new ArrayList<>(childRoles.values()); + } } private static class Root implements SpoonNode { @@ -136,6 +190,16 @@ public String toString() { public Revision getRevision() { throw new UnsupportedOperationException("The virtual root has no revision"); } + + @Override + public boolean isVirtual() { + return true; + } + + @Override + public List getVirtualNodes() { + return Arrays.asList(NodeFactory.startOfChildList(this), NodeFactory.endOfChildList(this)); + } } /** @@ -170,6 +234,11 @@ public Revision getRevision() { return parent.getRevision(); } + @Override + public List getVirtualNodes() { + return null; + } + @Override public boolean isEndOfList() { return side == Side.END; @@ -200,4 +269,58 @@ public String toString() { return side.toString(); } } + + public static class RoleNode implements SpoonNode { + private final Node parent; + private final CtRole role; + + RoleNode(CtRole role, Node parent) { + this.role = role; + this.parent = parent; + } + + @Override + public CtElement getElement() { + throw new UnsupportedOperationException("Can't get element from a RoleNode"); + } + + @Override + public SpoonNode getParent() { + return parent; + } + + @Override + public Revision getRevision() { + return parent.getRevision(); + } + + @Override + public List getVirtualNodes() { + return Arrays.asList(NodeFactory.startOfChildList(this), NodeFactory.endOfChildList(this)); + } + + @Override + public String toString() { + return "RoleNode#" + role.toString(); + } + + @Override + public boolean isVirtual() { + return true; + } + + @Override + public boolean equals(Object o) { + if (this == o) return true; + if (o == null || getClass() != o.getClass()) return false; + RoleNode roleNode = (RoleNode) o; + return Objects.equals(parent, roleNode.parent) && + role == roleNode.role; + } + + @Override + public int hashCode() { + return Objects.hash(parent, role); + } + } } diff --git a/src/main/java/se/kth/spork/spoon/wrappers/SpoonNode.java b/src/main/java/se/kth/spork/spoon/wrappers/SpoonNode.java index a42ec72d..331e1038 100644 --- a/src/main/java/se/kth/spork/spoon/wrappers/SpoonNode.java +++ b/src/main/java/se/kth/spork/spoon/wrappers/SpoonNode.java @@ -4,10 +4,14 @@ import se.kth.spork.base3dm.Revision; import spoon.reflect.declaration.CtElement; +import java.util.List; + public interface SpoonNode extends ListNode { CtElement getElement(); SpoonNode getParent(); Revision getRevision(); + + List getVirtualNodes(); } From 8fa60ea9c8437342c39f22227c4b9556b536e3a2 Mon Sep 17 00:00:00 2001 From: slarse Date: Sat, 23 May 2020 14:53:28 +0200 Subject: [PATCH 2/9] Refine implementation --- .../java/se/kth/spork/spoon/PcsBuilder.java | 19 ++--- .../se/kth/spork/spoon/Spoon3dmMerge.java | 10 +-- .../pcsinterpreter/SporkTreeBuilder.java | 17 ++-- .../kth/spork/spoon/wrappers/NodeFactory.java | 85 ++++++++++++++++--- .../se/kth/spork/spoon/Spoon3dmMergeTest.java | 1 + .../generify_method/Expected.java | 2 +- .../left_modified/generify_method/Left.java | 2 +- 7 files changed, 99 insertions(+), 37 deletions(-) diff --git a/src/main/java/se/kth/spork/spoon/PcsBuilder.java b/src/main/java/se/kth/spork/spoon/PcsBuilder.java index 31506ebb..a7aea743 100644 --- a/src/main/java/se/kth/spork/spoon/PcsBuilder.java +++ b/src/main/java/se/kth/spork/spoon/PcsBuilder.java @@ -5,6 +5,7 @@ import se.kth.spork.spoon.wrappers.NodeFactory; import se.kth.spork.spoon.wrappers.SpoonNode; import spoon.reflect.declaration.CtElement; +import spoon.reflect.declaration.CtMethod; import spoon.reflect.visitor.CtScanner; import java.util.*; @@ -42,18 +43,11 @@ public static Set> fromSpoon(CtElement spoonClass, Revision revis @Override protected void enter(CtElement e) { - SpoonNode wrapped; - SpoonNode parent; + SpoonNode wrapped = NodeFactory.wrap(e); + SpoonNode parent = wrapped.getParent(); - if (root == null) { - parent = NodeFactory.ROOT; - wrapped = NodeFactory.initializeWrapper(e, parent); + if (root == null) root = wrapped; - } else { - NodeFactory.Node actualParent = NodeFactory.wrap(e.getParent()); - wrapped = NodeFactory.initializeRoledWrapper(e, actualParent); - parent = wrapped.getParent(); - } parentToLastSibling.put(wrapped, NodeFactory.startOfChildList(wrapped)); @@ -71,10 +65,9 @@ private void finishPcses() { // we just need to close their child lists pcses.add(new Pcs<>(parent, lastSibling, NodeFactory.endOfChildList(parent), revision)); } else { - // this is a concrete node, we must add all of its virtual children to the PCS structure + // this is a concrete node, we must add all of its virtual children to the PCS structure, except for + // the start of the child list as it has all ready been added List virtualNodes = parent.getVirtualNodes(); - assert lastSibling.isStartOfList(); - assert virtualNodes.get(0).equals(lastSibling); SpoonNode pred = lastSibling; for (SpoonNode succ : virtualNodes.subList(1, virtualNodes.size())) { pcses.add(new Pcs<>(parent, pred, succ, revision)); diff --git a/src/main/java/se/kth/spork/spoon/Spoon3dmMerge.java b/src/main/java/se/kth/spork/spoon/Spoon3dmMerge.java index bf7ec145..b7d0f99a 100644 --- a/src/main/java/se/kth/spork/spoon/Spoon3dmMerge.java +++ b/src/main/java/se/kth/spork/spoon/Spoon3dmMerge.java @@ -77,11 +77,6 @@ public static Pair merge(T base, T left, T rig Matcher baseRightGumtreeMatch = matchTrees(baseGumtree, rightGumtree); Matcher leftRightGumtreeMatch = matchTreesXY(leftGumtree, rightGumtree); - LOGGER.info(() -> "Converting Spoon trees to PCS triples"); - Set> t0 = PcsBuilder.fromSpoon(base, Revision.BASE); - Set> t1 = PcsBuilder.fromSpoon(left, Revision.LEFT); - Set> t2 = PcsBuilder.fromSpoon(right, Revision.RIGHT); - LOGGER.info(() -> "Converting GumTree matches to Spoon matches"); SpoonMapping baseLeft = SpoonMapping.fromGumTreeMapping(baseLeftGumtreeMatch.getMappings()); SpoonMapping baseRight = SpoonMapping.fromGumTreeMapping(baseRightGumtreeMatch.getMappings()); @@ -92,6 +87,11 @@ public static Pair merge(T base, T left, T rig Map classRepMap = ClassRepresentatives.createClassRepresentativesMapping( base, left, right, baseLeft, baseRight, leftRight); + LOGGER.info(() -> "Converting Spoon trees to PCS triples"); + Set> t0 = PcsBuilder.fromSpoon(base, Revision.BASE); + Set> t1 = PcsBuilder.fromSpoon(left, Revision.LEFT); + Set> t2 = PcsBuilder.fromSpoon(right, Revision.RIGHT); + LOGGER.info(() -> "Computing raw PCS merge"); ChangeSet delta = new ChangeSet<>(classRepMap, new ContentResolver(), t0, t1, t2); ChangeSet t0Star = new ChangeSet<>(classRepMap, new ContentResolver(), t0); diff --git a/src/main/java/se/kth/spork/spoon/pcsinterpreter/SporkTreeBuilder.java b/src/main/java/se/kth/spork/spoon/pcsinterpreter/SporkTreeBuilder.java index c61b107d..7514673e 100644 --- a/src/main/java/se/kth/spork/spoon/pcsinterpreter/SporkTreeBuilder.java +++ b/src/main/java/se/kth/spork/spoon/pcsinterpreter/SporkTreeBuilder.java @@ -107,7 +107,6 @@ public SporkTree buildTree() { public SporkTree build(SpoonNode currentRoot) { Map> children = rootToChildren.get(currentRoot); - Set> currentContent = contents.getOrDefault(currentRoot, Collections.emptySet()); SporkTree tree = new SporkTree(currentRoot, currentContent); @@ -116,6 +115,12 @@ public SporkTree build(SpoonNode currentRoot) { try { build(NodeFactory.startOfChildList(currentRoot), tree, children); + + for (Pcs inconsistent : remainingInconsistencies) { + if (inconsistent.getRoot().equals(tree.getNode())) { + throw new ConflictException("Missed conflict: " + inconsistent); + } + } } catch (NullPointerException | ConflictException e) { // could not resolve the child list // TODO improve design, should not have to catch exceptions like this @@ -131,6 +136,10 @@ public SporkTree build(SpoonNode currentRoot) { } private void build(SpoonNode start, SporkTree tree, Map> children) { + if (children == null) // leaf node + return; + + SpoonNode next = start; while (true) { Pcs nextPcs = children.get(next); @@ -157,12 +166,6 @@ private void build(SpoonNode start, SporkTree tree, Map inconsistent : remainingInconsistencies) { - if (inconsistent.getRoot().equals(tree.getNode())) { - throw new ConflictException("Missed conflict: " + inconsistent); - } - } } } diff --git a/src/main/java/se/kth/spork/spoon/wrappers/NodeFactory.java b/src/main/java/se/kth/spork/spoon/wrappers/NodeFactory.java index d5024ef7..9e0c4b8b 100644 --- a/src/main/java/se/kth/spork/spoon/wrappers/NodeFactory.java +++ b/src/main/java/se/kth/spork/spoon/wrappers/NodeFactory.java @@ -3,15 +3,21 @@ import se.kth.spork.base3dm.Revision; import se.kth.spork.base3dm.TdmMerge; import spoon.reflect.declaration.CtElement; +import spoon.reflect.declaration.CtExecutable; import spoon.reflect.factory.ModuleFactory; +import spoon.reflect.meta.RoleHandler; +import spoon.reflect.meta.impl.RoleHandlerHelper; import spoon.reflect.path.CtRole; +import spoon.reflect.reference.CtExecutableReference; import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; +import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.Objects; +import java.util.Set; import java.util.TreeMap; import java.util.stream.Collectors; import java.util.stream.Stream; @@ -26,6 +32,37 @@ public class NodeFactory { private static long currentKey = 0; public static final SpoonNode ROOT = new Root(); + private static final Map, List> EXPLODED_TYPE_ROLES; + private static final List> EXPLODED_TYPES = Arrays.asList( + CtExecutableReference.class, CtExecutable.class + ); + + // These are roles that are present in the EXPLODED_TYPES types, but are not structural and therefore + // do not add any value in the PCS structure. When adding a new exploded type, + private static final Set IGNORED_ROLES = Stream.of( + CtRole.IS_IMPLICIT, + CtRole.IS_DEFAULT, + CtRole.IS_VARARGS, + CtRole.IS_FINAL, + CtRole.IS_SHADOW, + CtRole.IS_STATIC, + CtRole.DECLARING_TYPE, + CtRole.MODIFIER, + CtRole.EMODIFIER, + CtRole.COMMENT, + CtRole.NAME, + CtRole.BODY, + CtRole.POSITION + ).collect(Collectors.toSet()); + + static { + Map, List> rolesPerClass = new HashMap<>(); + for (Class cls : EXPLODED_TYPES) { + rolesPerClass.put(cls, getRoles(cls).filter(role -> !IGNORED_ROLES.contains(role)).collect(Collectors.toList())); + } + EXPLODED_TYPE_ROLES = Collections.unmodifiableMap(rolesPerClass); + } + /** * Wrap a CtElement in a CtWrapper. The wrapper is stored in the CtElement's metadata. If a CtElement that has * already been wrapped is passed in, then its existing wrapper is returned. In other words, each CtElement gets @@ -38,25 +75,45 @@ public static Node wrap(CtElement elem) { Object wrapper = elem.getMetadata(WRAPPER_METADATA); if (wrapper == null) { - //throw new IllegalStateException("wrapper not initialized for " + elem); - CtElement parent = elem.getParent(); - return initializeWrapper(elem, parent == null ? ROOT : wrap(parent)); + return initializeWrapper(elem); } return (Node) wrapper; } - public static Node initializeRoledWrapper(CtElement elem, Node parent) { - SpoonNode effectiveParent = parent.getRoleNode(elem.getRoleInParent()); + private static Node initializeWrapper(CtElement elem) { + if (elem instanceof ModuleFactory.CtUnnamedModule) + return initializeWrapper(elem, ROOT); + + CtElement spoonParent = elem.getParent(); + CtRole roleInParent = elem.getRoleInParent(); + Node actualParent = wrap(spoonParent); + SpoonNode effectiveParent = actualParent.hasRoleNodeFor(roleInParent) ? + actualParent.getRoleNode(roleInParent) : actualParent; return initializeWrapper(elem, effectiveParent); } - public static Node initializeWrapper(CtElement elem, SpoonNode parent) { - Node node = new Node(elem, parent, currentKey++); + private static Node initializeWrapper(CtElement elem, SpoonNode parent) { + + List availableChildRoles = Collections.emptyList(); + Class cls = elem.getClass(); + for (Class explodedType : EXPLODED_TYPES) { + if (explodedType.isAssignableFrom(cls)) { + availableChildRoles = EXPLODED_TYPE_ROLES.get(explodedType); + break; + } + } + + + Node node = new Node(elem, parent, currentKey++, availableChildRoles); elem.putMetadata(WRAPPER_METADATA, node); return node; } + private static Stream getRoles(Class cls) { + return RoleHandlerHelper.getRoleHandlers(cls).stream().map(RoleHandler::getRole); + } + /** * Create a node that represents the start of the child list of the provided node. * @@ -91,10 +148,15 @@ public static class Node implements SpoonNode { private final Map childRoles; private final CtRole role; - Node(CtElement element, SpoonNode parent, long key) { + Node(CtElement element, SpoonNode parent, long key, List availableChildRoles) { this.element = element; this.key = key; + childRoles = new TreeMap<>(); + for (CtRole role : availableChildRoles) { + childRoles.put(role, new RoleNode(role, this)); + } + this.role = element.getRoleInParent(); this.parent = parent; } @@ -159,12 +221,15 @@ public int hashCode() { public RoleNode getRoleNode(CtRole role) { RoleNode roleNode = childRoles.get(role); if (roleNode == null) { - roleNode = new RoleNode(role, this); - childRoles.put(role, roleNode); + throw new IllegalArgumentException("No role node for " + role); } return roleNode; } + boolean hasRoleNodeFor(CtRole role) { + return role != null && childRoles.containsKey(role); + } + public List getChildRoles() { return new ArrayList<>(childRoles.values()); } diff --git a/src/test/java/se/kth/spork/spoon/Spoon3dmMergeTest.java b/src/test/java/se/kth/spork/spoon/Spoon3dmMergeTest.java index 7d42be4b..e6d2975f 100644 --- a/src/test/java/se/kth/spork/spoon/Spoon3dmMergeTest.java +++ b/src/test/java/se/kth/spork/spoon/Spoon3dmMergeTest.java @@ -4,6 +4,7 @@ import org.junit.jupiter.params.ParameterizedTest; 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.*; diff --git a/src/test/resources/clean/left_modified/generify_method/Expected.java b/src/test/resources/clean/left_modified/generify_method/Expected.java index 576dbf9c..6a2b3137 100644 --- a/src/test/resources/clean/left_modified/generify_method/Expected.java +++ b/src/test/resources/clean/left_modified/generify_method/Expected.java @@ -1,5 +1,5 @@ public class Cls { - public void method(List list) { + public void method(List list) { System.out.println(list); } } \ No newline at end of file diff --git a/src/test/resources/clean/left_modified/generify_method/Left.java b/src/test/resources/clean/left_modified/generify_method/Left.java index 576dbf9c..6a2b3137 100644 --- a/src/test/resources/clean/left_modified/generify_method/Left.java +++ b/src/test/resources/clean/left_modified/generify_method/Left.java @@ -1,5 +1,5 @@ public class Cls { - public void method(List list) { + public void method(List list) { System.out.println(list); } } \ No newline at end of file From 59ffb00bd6d129069afa6189ade0e3dd84745787 Mon Sep 17 00:00:00 2001 From: slarse Date: Sat, 23 May 2020 15:28:50 +0200 Subject: [PATCH 3/9] Fix flaky conflict resolution --- .../pcsinterpreter/SporkTreeBuilder.java | 30 +++++++++++++------ 1 file changed, 21 insertions(+), 9 deletions(-) diff --git a/src/main/java/se/kth/spork/spoon/pcsinterpreter/SporkTreeBuilder.java b/src/main/java/se/kth/spork/spoon/pcsinterpreter/SporkTreeBuilder.java index 7514673e..0956a4d5 100644 --- a/src/main/java/se/kth/spork/spoon/pcsinterpreter/SporkTreeBuilder.java +++ b/src/main/java/se/kth/spork/spoon/pcsinterpreter/SporkTreeBuilder.java @@ -146,29 +146,38 @@ private void build(SpoonNode start, SporkTree tree, Map traverseConflict(nextPcs, conflicting, children, tree)); break; } - if (next.isVirtual()) { + if (next.isVirtual() && !next.isListEdge()) { build(NodeFactory.startOfChildList(next), tree, rootToChildren.get(next)); } else { - Set> conflicts = structuralConflicts.get(nextPcs); - Optional> successorConflict = conflicts == null ? Optional.empty() : - conflicts.stream().filter(confPcs -> - StructuralConflict.isSuccessorConflict(nextPcs, confPcs)).findFirst(); - - // successor conflicts mark the start of a conflict, any other conflict is to be ignored + Optional> successorConflict = getSuccessorConflict(nextPcs); if (successorConflict.isPresent()) { Pcs conflicting = successorConflict.get(); next = traverseConflict(nextPcs, conflicting, children, tree); } else { addChild(tree, build(next)); } + + if (next.isEndOfList()) { + break; + } } } } + private Optional> getSuccessorConflict(Pcs pcs) { + Set> conflicts = structuralConflicts.get(pcs); + return conflicts == null ? Optional.empty() : + conflicts.stream().filter(confPcs -> + StructuralConflict.isSuccessorConflict(pcs, confPcs)).findFirst(); + } + /** * When a conflict in the child list of a node is not possible to resolve, we approximate the conflict by finding * the node's matches in the left and right revisions and have them make up the conflict instead. This is a rough @@ -222,6 +231,9 @@ private SpoonNode traverseConflict( List rightNodes = extractConflictList(rightPcs, children); Optional> resolved = tryResolveConflict(leftNodes, rightNodes); + + // if nextPcs happens to be the final PCS of a child list, next may be a virtual node + next = leftNodes.isEmpty() ? rightNodes.get(rightNodes.size() - 1) : leftNodes.get(leftNodes.size() - 1); if (resolved.isPresent()) { resolved.get().forEach(child -> addChild(tree, build(child)) @@ -236,7 +248,7 @@ private SpoonNode traverseConflict( tree.addChild(new SporkTree(next, contents.get(next), conflict)); } // by convention, build left tree - return leftNodes.isEmpty() ? next : leftNodes.get(leftNodes.size() - 1); + return next; } private void addChild(SporkTree tree, SporkTree child) { From c744db69054b5e1b2579e84ed80ceb3aefce51cd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20Lars=C3=A9n?= Date: Sat, 23 May 2020 18:36:46 +0200 Subject: [PATCH 4/9] Set the merge parent unconditionally --- .../se/kth/spork/spoon/pcsinterpreter/SpoonTreeBuilder.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/main/java/se/kth/spork/spoon/pcsinterpreter/SpoonTreeBuilder.java b/src/main/java/se/kth/spork/spoon/pcsinterpreter/SpoonTreeBuilder.java index 1f307f87..c7490271 100644 --- a/src/main/java/se/kth/spork/spoon/pcsinterpreter/SpoonTreeBuilder.java +++ b/src/main/java/se/kth/spork/spoon/pcsinterpreter/SpoonTreeBuilder.java @@ -187,6 +187,9 @@ private CtElement visit(SporkTree sporkParent, SporkTree sporkChild) { } } + // NOTE: Super important that the parent of the merge tree is set no matter what, as wrapping a spoon CtElement + // in a SpoonNode requires access to its parent. + mergeTree.setParent(mergeParent); nodes.put(origTreeNode, NodeFactory.wrap(mergeTree)); return mergeTree; From cb8aedacfc13fb46ea42a5f17c76b6dfe359860d Mon Sep 17 00:00:00 2001 From: slarse Date: Sun, 24 May 2020 12:13:49 +0200 Subject: [PATCH 5/9] Ensure that virtual nodes get leaf child lists when empty --- src/main/java/se/kth/spork/spoon/PcsBuilder.java | 12 ++++++++++++ .../left_modified/empty_parameter_list/Base.java | 5 +++++ .../left_modified/empty_parameter_list/Expected.java | 5 +++++ .../left_modified/empty_parameter_list/Left.java | 5 +++++ .../left_modified/empty_parameter_list/Right.java | 5 +++++ .../clean/left_modified/empty_thrown_types/Base.java | 7 +++++++ .../left_modified/empty_thrown_types/Expected.java | 4 ++++ .../clean/left_modified/empty_thrown_types/Left.java | 4 ++++ .../left_modified/empty_thrown_types/Right.java | 7 +++++++ 9 files changed, 54 insertions(+) create mode 100644 src/test/resources/clean/left_modified/empty_parameter_list/Base.java create mode 100644 src/test/resources/clean/left_modified/empty_parameter_list/Expected.java create mode 100644 src/test/resources/clean/left_modified/empty_parameter_list/Left.java create mode 100644 src/test/resources/clean/left_modified/empty_parameter_list/Right.java create mode 100644 src/test/resources/clean/left_modified/empty_thrown_types/Base.java create mode 100644 src/test/resources/clean/left_modified/empty_thrown_types/Expected.java create mode 100644 src/test/resources/clean/left_modified/empty_thrown_types/Left.java create mode 100644 src/test/resources/clean/left_modified/empty_thrown_types/Right.java diff --git a/src/main/java/se/kth/spork/spoon/PcsBuilder.java b/src/main/java/se/kth/spork/spoon/PcsBuilder.java index a7aea743..19002d3f 100644 --- a/src/main/java/se/kth/spork/spoon/PcsBuilder.java +++ b/src/main/java/se/kth/spork/spoon/PcsBuilder.java @@ -71,12 +71,24 @@ private void finishPcses() { SpoonNode pred = lastSibling; for (SpoonNode succ : virtualNodes.subList(1, virtualNodes.size())) { pcses.add(new Pcs<>(parent, pred, succ, revision)); + + // also need to create "leaf child lists" for any non-list-edge virtual node that does not have any + // children, or Spork will not discover removals that entirely empty the child list. + // The problem is detailed in https://github.com/KTH/spork/issues/116 + if (!pred.isListEdge() && !parentToLastSibling.containsKey(pred)) { + pcses.add(createLeafPcs(pred)); + } + pred = succ; } } } } + private Pcs createLeafPcs(SpoonNode node) { + return new Pcs<>(node, NodeFactory.startOfChildList(node), NodeFactory.endOfChildList(node), revision); + } + public Set> getPcses() { return pcses; } diff --git a/src/test/resources/clean/left_modified/empty_parameter_list/Base.java b/src/test/resources/clean/left_modified/empty_parameter_list/Base.java new file mode 100644 index 00000000..ff63f866 --- /dev/null +++ b/src/test/resources/clean/left_modified/empty_parameter_list/Base.java @@ -0,0 +1,5 @@ +public class Main { + int generateNumber(int a, int b) { + return a + b; + } +} \ No newline at end of file diff --git a/src/test/resources/clean/left_modified/empty_parameter_list/Expected.java b/src/test/resources/clean/left_modified/empty_parameter_list/Expected.java new file mode 100644 index 00000000..e779dfea --- /dev/null +++ b/src/test/resources/clean/left_modified/empty_parameter_list/Expected.java @@ -0,0 +1,5 @@ +public class Main { + int generateNumber() { + return 42; + } +} \ No newline at end of file diff --git a/src/test/resources/clean/left_modified/empty_parameter_list/Left.java b/src/test/resources/clean/left_modified/empty_parameter_list/Left.java new file mode 100644 index 00000000..e779dfea --- /dev/null +++ b/src/test/resources/clean/left_modified/empty_parameter_list/Left.java @@ -0,0 +1,5 @@ +public class Main { + int generateNumber() { + return 42; + } +} \ No newline at end of file diff --git a/src/test/resources/clean/left_modified/empty_parameter_list/Right.java b/src/test/resources/clean/left_modified/empty_parameter_list/Right.java new file mode 100644 index 00000000..ff63f866 --- /dev/null +++ b/src/test/resources/clean/left_modified/empty_parameter_list/Right.java @@ -0,0 +1,5 @@ +public class Main { + int generateNumber(int a, int b) { + return a + b; + } +} \ No newline at end of file diff --git a/src/test/resources/clean/left_modified/empty_thrown_types/Base.java b/src/test/resources/clean/left_modified/empty_thrown_types/Base.java new file mode 100644 index 00000000..7a3cfac7 --- /dev/null +++ b/src/test/resources/clean/left_modified/empty_thrown_types/Base.java @@ -0,0 +1,7 @@ +import java.io.IOException; +import java.util.EmptyStackException; + +public class Main { + int method() throws EmptyStackException, IOException { + } +} \ No newline at end of file diff --git a/src/test/resources/clean/left_modified/empty_thrown_types/Expected.java b/src/test/resources/clean/left_modified/empty_thrown_types/Expected.java new file mode 100644 index 00000000..c5eb794b --- /dev/null +++ b/src/test/resources/clean/left_modified/empty_thrown_types/Expected.java @@ -0,0 +1,4 @@ +public class Main { + int method() { + } +} \ No newline at end of file diff --git a/src/test/resources/clean/left_modified/empty_thrown_types/Left.java b/src/test/resources/clean/left_modified/empty_thrown_types/Left.java new file mode 100644 index 00000000..c5eb794b --- /dev/null +++ b/src/test/resources/clean/left_modified/empty_thrown_types/Left.java @@ -0,0 +1,4 @@ +public class Main { + int method() { + } +} \ No newline at end of file diff --git a/src/test/resources/clean/left_modified/empty_thrown_types/Right.java b/src/test/resources/clean/left_modified/empty_thrown_types/Right.java new file mode 100644 index 00000000..7a3cfac7 --- /dev/null +++ b/src/test/resources/clean/left_modified/empty_thrown_types/Right.java @@ -0,0 +1,7 @@ +import java.io.IOException; +import java.util.EmptyStackException; + +public class Main { + int method() throws EmptyStackException, IOException { + } +} \ No newline at end of file From dfe3d6b2941bb52aee3049f948a9ac2350ea6611 Mon Sep 17 00:00:00 2001 From: slarse Date: Sun, 24 May 2020 12:47:15 +0200 Subject: [PATCH 6/9] Also explode types --- .../kth/spork/spoon/wrappers/NodeFactory.java | 17 +++++++++++------ .../add_parameters_and_thrown_types/Base.java | 5 +++++ .../Expected.java | 5 +++++ .../add_parameters_and_thrown_types/Left.java | 5 +++++ .../add_parameters_and_thrown_types/Right.java | 5 +++++ .../add_type_member_and_comment/Base.java | 5 +++++ .../add_type_member_and_comment/Expected.java | 6 ++++++ .../add_type_member_and_comment/Left.java | 5 +++++ .../add_type_member_and_comment/Right.java | 6 ++++++ 9 files changed, 53 insertions(+), 6 deletions(-) create mode 100644 src/test/resources/clean/both_modified/add_parameters_and_thrown_types/Base.java create mode 100644 src/test/resources/clean/both_modified/add_parameters_and_thrown_types/Expected.java create mode 100644 src/test/resources/clean/both_modified/add_parameters_and_thrown_types/Left.java create mode 100644 src/test/resources/clean/both_modified/add_parameters_and_thrown_types/Right.java create mode 100644 src/test/resources/clean/both_modified/add_type_member_and_comment/Base.java create mode 100644 src/test/resources/clean/both_modified/add_type_member_and_comment/Expected.java create mode 100644 src/test/resources/clean/both_modified/add_type_member_and_comment/Left.java create mode 100644 src/test/resources/clean/both_modified/add_type_member_and_comment/Right.java diff --git a/src/main/java/se/kth/spork/spoon/wrappers/NodeFactory.java b/src/main/java/se/kth/spork/spoon/wrappers/NodeFactory.java index 9e0c4b8b..55a1ea5e 100644 --- a/src/main/java/se/kth/spork/spoon/wrappers/NodeFactory.java +++ b/src/main/java/se/kth/spork/spoon/wrappers/NodeFactory.java @@ -4,6 +4,7 @@ import se.kth.spork.base3dm.TdmMerge; import spoon.reflect.declaration.CtElement; import spoon.reflect.declaration.CtExecutable; +import spoon.reflect.declaration.CtType; import spoon.reflect.factory.ModuleFactory; import spoon.reflect.meta.RoleHandler; import spoon.reflect.meta.impl.RoleHandlerHelper; @@ -34,12 +35,13 @@ public class NodeFactory { private static final Map, List> EXPLODED_TYPE_ROLES; private static final List> EXPLODED_TYPES = Arrays.asList( - CtExecutableReference.class, CtExecutable.class + CtExecutableReference.class, CtExecutable.class, CtType.class ); - // These are roles that are present in the EXPLODED_TYPES types, but are not structural and therefore - // do not add any value in the PCS structure. When adding a new exploded type, + // These are roles that are present in the EXPLODED_TYPES types, but are either not structural + // or are always present as a single node (such as a method body) private static final Set IGNORED_ROLES = Stream.of( + /* START NON-STRUCTURAL ROLES */ CtRole.IS_IMPLICIT, CtRole.IS_DEFAULT, CtRole.IS_VARARGS, @@ -49,10 +51,13 @@ public class NodeFactory { CtRole.DECLARING_TYPE, CtRole.MODIFIER, CtRole.EMODIFIER, - CtRole.COMMENT, CtRole.NAME, - CtRole.BODY, - CtRole.POSITION + CtRole.POSITION, + /* END NON-STRUCTURAL ROLES */ + CtRole.BODY, // always present as a single node + CtRole.NESTED_TYPE, // falls under type member + CtRole.FIELD, // falls under type member + CtRole.METHOD // falls under type member ).collect(Collectors.toSet()); static { diff --git a/src/test/resources/clean/both_modified/add_parameters_and_thrown_types/Base.java b/src/test/resources/clean/both_modified/add_parameters_and_thrown_types/Base.java new file mode 100644 index 00000000..81214dfa --- /dev/null +++ b/src/test/resources/clean/both_modified/add_parameters_and_thrown_types/Base.java @@ -0,0 +1,5 @@ +public class Main { + public int add(int a, int b) { + return a + b; + } +} diff --git a/src/test/resources/clean/both_modified/add_parameters_and_thrown_types/Expected.java b/src/test/resources/clean/both_modified/add_parameters_and_thrown_types/Expected.java new file mode 100644 index 00000000..70be0d9b --- /dev/null +++ b/src/test/resources/clean/both_modified/add_parameters_and_thrown_types/Expected.java @@ -0,0 +1,5 @@ +public class Main { + public int add(int a, int b, int c) throws IllegalArgumentException { + return a + b + c; + } +} diff --git a/src/test/resources/clean/both_modified/add_parameters_and_thrown_types/Left.java b/src/test/resources/clean/both_modified/add_parameters_and_thrown_types/Left.java new file mode 100644 index 00000000..963c829c --- /dev/null +++ b/src/test/resources/clean/both_modified/add_parameters_and_thrown_types/Left.java @@ -0,0 +1,5 @@ +public class Main { + public int add(int a, int b) throws IllegalArgumentException { + return a + b; + } +} diff --git a/src/test/resources/clean/both_modified/add_parameters_and_thrown_types/Right.java b/src/test/resources/clean/both_modified/add_parameters_and_thrown_types/Right.java new file mode 100644 index 00000000..d22a10ab --- /dev/null +++ b/src/test/resources/clean/both_modified/add_parameters_and_thrown_types/Right.java @@ -0,0 +1,5 @@ +public class Main { + public int add(int a, int b, int c) { + return a + b + c; + } +} diff --git a/src/test/resources/clean/both_modified/add_type_member_and_comment/Base.java b/src/test/resources/clean/both_modified/add_type_member_and_comment/Base.java new file mode 100644 index 00000000..32636d0b --- /dev/null +++ b/src/test/resources/clean/both_modified/add_type_member_and_comment/Base.java @@ -0,0 +1,5 @@ +package main; + +class Main { + +} \ No newline at end of file diff --git a/src/test/resources/clean/both_modified/add_type_member_and_comment/Expected.java b/src/test/resources/clean/both_modified/add_type_member_and_comment/Expected.java new file mode 100644 index 00000000..953d4154 --- /dev/null +++ b/src/test/resources/clean/both_modified/add_type_member_and_comment/Expected.java @@ -0,0 +1,6 @@ +package main; + +// this is a comment that will appear at the end of the direct children of this class +class Main { + int a = 2; +} diff --git a/src/test/resources/clean/both_modified/add_type_member_and_comment/Left.java b/src/test/resources/clean/both_modified/add_type_member_and_comment/Left.java new file mode 100644 index 00000000..c095ec38 --- /dev/null +++ b/src/test/resources/clean/both_modified/add_type_member_and_comment/Left.java @@ -0,0 +1,5 @@ +package main; + +class Main { + int a = 2; +} \ No newline at end of file diff --git a/src/test/resources/clean/both_modified/add_type_member_and_comment/Right.java b/src/test/resources/clean/both_modified/add_type_member_and_comment/Right.java new file mode 100644 index 00000000..a139acff --- /dev/null +++ b/src/test/resources/clean/both_modified/add_type_member_and_comment/Right.java @@ -0,0 +1,6 @@ +package main; + +// this is a comment that will appear at the end of the direct children of this class +class Main { + +} \ No newline at end of file From 12457c182d194cc8d22c2fa83e7c0da1c34f5eb0 Mon Sep 17 00:00:00 2001 From: slarse Date: Sun, 24 May 2020 13:22:02 +0200 Subject: [PATCH 7/9] Do not explode type parameters --- .../kth/spork/spoon/wrappers/NodeFactory.java | 48 +++++++++++-------- 1 file changed, 28 insertions(+), 20 deletions(-) diff --git a/src/main/java/se/kth/spork/spoon/wrappers/NodeFactory.java b/src/main/java/se/kth/spork/spoon/wrappers/NodeFactory.java index 55a1ea5e..e679ba6e 100644 --- a/src/main/java/se/kth/spork/spoon/wrappers/NodeFactory.java +++ b/src/main/java/se/kth/spork/spoon/wrappers/NodeFactory.java @@ -5,6 +5,8 @@ import spoon.reflect.declaration.CtElement; import spoon.reflect.declaration.CtExecutable; import spoon.reflect.declaration.CtType; +import spoon.reflect.declaration.CtTypeMember; +import spoon.reflect.declaration.CtTypeParameter; import spoon.reflect.factory.ModuleFactory; import spoon.reflect.meta.RoleHandler; import spoon.reflect.meta.impl.RoleHandlerHelper; @@ -99,20 +101,30 @@ private static Node initializeWrapper(CtElement elem) { } private static Node initializeWrapper(CtElement elem, SpoonNode parent) { + List availableChildRoles = getVirtualNodeChildRoles(elem); + Node node = new Node(elem, parent, currentKey++, availableChildRoles); + elem.putMetadata(WRAPPER_METADATA, node); + return node; + } + + /** + * Return a list of child nodes that should be exploded into virtual types for the given element. + * + * Note that for most types, the list will be empty. + */ + private static List getVirtualNodeChildRoles(CtElement elem) { + if (CtTypeParameter.class.isAssignableFrom(elem.getClass())) { + // we ignore any subtype of CtTypeParameter as exploding them causes a large performance hit + return Collections.emptyList(); + } - List availableChildRoles = Collections.emptyList(); Class cls = elem.getClass(); for (Class explodedType : EXPLODED_TYPES) { if (explodedType.isAssignableFrom(cls)) { - availableChildRoles = EXPLODED_TYPE_ROLES.get(explodedType); - break; + return EXPLODED_TYPE_ROLES.get(explodedType); } } - - - Node node = new Node(elem, parent, currentKey++, availableChildRoles); - elem.putMetadata(WRAPPER_METADATA, node); - return node; + return Collections.emptyList(); } private static Stream getRoles(Class cls) { @@ -150,16 +162,16 @@ public static class Node implements SpoonNode { private final CtElement element; private final long key; private final SpoonNode parent; - private final Map childRoles; + private final Map virtualRoleChildNodes; private final CtRole role; - Node(CtElement element, SpoonNode parent, long key, List availableChildRoles) { + Node(CtElement element, SpoonNode parent, long key, List virtualNodeChildRoles) { this.element = element; this.key = key; - childRoles = new TreeMap<>(); - for (CtRole role : availableChildRoles) { - childRoles.put(role, new RoleNode(role, this)); + virtualRoleChildNodes = new TreeMap<>(); + for (CtRole role : virtualNodeChildRoles) { + virtualRoleChildNodes.put(role, new RoleNode(role, this)); } this.role = element.getRoleInParent(); @@ -204,7 +216,7 @@ public List getVirtualNodes() { return Stream.concat( Stream.concat( Stream.of(NodeFactory.startOfChildList(this)), - childRoles.values().stream() + virtualRoleChildNodes.values().stream() ), Stream.of(NodeFactory.endOfChildList(this))) .collect(Collectors.toList()); @@ -224,7 +236,7 @@ public int hashCode() { } public RoleNode getRoleNode(CtRole role) { - RoleNode roleNode = childRoles.get(role); + RoleNode roleNode = virtualRoleChildNodes.get(role); if (roleNode == null) { throw new IllegalArgumentException("No role node for " + role); } @@ -232,11 +244,7 @@ public RoleNode getRoleNode(CtRole role) { } boolean hasRoleNodeFor(CtRole role) { - return role != null && childRoles.containsKey(role); - } - - public List getChildRoles() { - return new ArrayList<>(childRoles.values()); + return role != null && virtualRoleChildNodes.containsKey(role); } } From 716249d2ae92287f6d5abb9bd0527077d8c2831f Mon Sep 17 00:00:00 2001 From: slarse Date: Sun, 24 May 2020 14:07:03 +0200 Subject: [PATCH 8/9] Remove forgotten debug loop --- .../spork/spoon/matching/ClassRepresentatives.java | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/src/main/java/se/kth/spork/spoon/matching/ClassRepresentatives.java b/src/main/java/se/kth/spork/spoon/matching/ClassRepresentatives.java index f6cfa587..1031bfd4 100644 --- a/src/main/java/se/kth/spork/spoon/matching/ClassRepresentatives.java +++ b/src/main/java/se/kth/spork/spoon/matching/ClassRepresentatives.java @@ -101,17 +101,6 @@ private static void mapToClassRepresentatives(CtElement tree, SpoonMapping mappi CtElement t = descIt.next(); mapToClassRep(mappings, classRepMap, rev, t); } - - descIt = tree.descendantIterator(); - while (descIt.hasNext()) { - CtElement t = descIt.next(); - SpoonNode node = NodeFactory.wrap(t); - SpoonNode parent = node.getParent(); - SpoonNode parentClassRep = classRepMap.get(parent); - if (parentClassRep == null) { - System.out.println(); - } - } } private static void mapToClassRep(SpoonMapping mappings, Map classRepMap, Revision rev, CtElement t) { From b70ad8248456faa1802bbb5f6775fca8dfaab342 Mon Sep 17 00:00:00 2001 From: slarse Date: Sun, 24 May 2020 14:29:26 +0200 Subject: [PATCH 9/9] Clean up in NodeFactory --- .../kth/spork/spoon/wrappers/NodeFactory.java | 27 +++++++++++++------ 1 file changed, 19 insertions(+), 8 deletions(-) diff --git a/src/main/java/se/kth/spork/spoon/wrappers/NodeFactory.java b/src/main/java/se/kth/spork/spoon/wrappers/NodeFactory.java index e679ba6e..52a83cdd 100644 --- a/src/main/java/se/kth/spork/spoon/wrappers/NodeFactory.java +++ b/src/main/java/se/kth/spork/spoon/wrappers/NodeFactory.java @@ -78,7 +78,11 @@ public class NodeFactory { * @param elem An element to wrap. * @return A wrapper around the CtElement that is more practical for hashing purposes. */ - public static Node wrap(CtElement elem) { + public static SpoonNode wrap(CtElement elem) { + return wrapInternal(elem); + } + + private static Node wrapInternal(CtElement elem) { Object wrapper = elem.getMetadata(WRAPPER_METADATA); if (wrapper == null) { @@ -94,7 +98,7 @@ private static Node initializeWrapper(CtElement elem) { CtElement spoonParent = elem.getParent(); CtRole roleInParent = elem.getRoleInParent(); - Node actualParent = wrap(spoonParent); + Node actualParent = wrapInternal(spoonParent); SpoonNode effectiveParent = actualParent.hasRoleNodeFor(roleInParent) ? actualParent.getRoleNode(roleInParent) : actualParent; return initializeWrapper(elem, effectiveParent); @@ -155,10 +159,10 @@ public static SpoonNode endOfChildList(SpoonNode elem) { * A simple wrapper class for a Spoon CtElement. The reason it is needed is that the 3DM merge implementation * uses lookup tables, and CtElements have very heavy-duty equals and hash functions. For the purpose of 3DM merge, * only reference equality is needed, not deep equality. - *

- * This class should only be instantiated {@link NodeFactory#wrap(CtElement)}. + * + * This class should only be instantiated by {@link #wrap(CtElement)}. */ - public static class Node implements SpoonNode { + private static class Node implements SpoonNode { private final CtElement element; private final long key; private final SpoonNode parent; @@ -248,6 +252,9 @@ boolean hasRoleNodeFor(CtRole role) { } } + /** + * The root virtual node. This is a singleton, there should only be the one that exists in {@link #ROOT}. + */ private static class Root implements SpoonNode { @Override public CtElement getElement() { @@ -256,7 +263,7 @@ public CtElement getElement() { @Override public SpoonNode getParent() { - return null; + throw new UnsupportedOperationException("The virtual root has no parent"); } @Override @@ -314,7 +321,7 @@ public Revision getRevision() { @Override public List getVirtualNodes() { - return null; + throw new UnsupportedOperationException("Can't get virtual nodes from a list edge"); } @Override @@ -348,7 +355,11 @@ public String toString() { } } - public static class RoleNode implements SpoonNode { + /** + * A RoleNode is a virtual node used to separate child lists in nodes with multiple types of child lists. See + * https://github.com/KTH/spork/issues/132 for details. + */ + private static class RoleNode implements SpoonNode { private final Node parent; private final CtRole role;