diff --git a/mapping-io-extras/src/test/java/net/fabricmc/mappingio/extras/MappingTreeRemapperTest.java b/mapping-io-extras/src/test/java/net/fabricmc/mappingio/extras/MappingTreeRemapperTest.java index 90213e1f..df17a6ff 100644 --- a/mapping-io-extras/src/test/java/net/fabricmc/mappingio/extras/MappingTreeRemapperTest.java +++ b/mapping-io-extras/src/test/java/net/fabricmc/mappingio/extras/MappingTreeRemapperTest.java @@ -16,6 +16,7 @@ package net.fabricmc.mappingio.extras; +import static net.fabricmc.mappingio.test.TestUtil.createTree; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertThrows; @@ -30,7 +31,6 @@ import net.fabricmc.mappingio.tree.MappingTree.ClassMapping; import net.fabricmc.mappingio.tree.MappingTree.FieldMapping; import net.fabricmc.mappingio.tree.MappingTree.MethodMapping; -import net.fabricmc.mappingio.tree.MemoryMappingTree; public class MappingTreeRemapperTest { private static final String cls1SrcName = "class_1"; @@ -50,7 +50,7 @@ public class MappingTreeRemapperTest { @BeforeAll public static void setup() throws IOException { - mappingTree = TestMappings.generateValid(new MemoryMappingTree()); + mappingTree = TestMappings.generateValid(createTree()); srcNs = mappingTree.getSrcNamespace(); dstNs = mappingTree.getDstNamespaces().get(0); diff --git a/src/main/java/net/fabricmc/mappingio/tree/MemoryMappingTree.java b/src/main/java/net/fabricmc/mappingio/tree/MemoryMappingTree.java index 67f5c03a..55d3ad34 100644 --- a/src/main/java/net/fabricmc/mappingio/tree/MemoryMappingTree.java +++ b/src/main/java/net/fabricmc/mappingio/tree/MemoryMappingTree.java @@ -17,6 +17,7 @@ package net.fabricmc.mappingio.tree; import java.io.IOException; +import java.util.AbstractMap; import java.util.ArrayDeque; import java.util.ArrayList; import java.util.Arrays; @@ -99,14 +100,84 @@ private void initClassesByDstNames() { classesByDstNames[i] = new HashMap<>(classesBySrcName.size()); } + Map>> duplicatesByNameByNs = null; + for (ClassEntry cls : classesBySrcName.values()) { for (int i = 0; i < cls.dstNames.length; i++) { String dstName = cls.dstNames[i]; - if (dstName != null) classesByDstNames[i].put(dstName, cls); + + if (dstName == null) { + continue; + } + + ClassEntry prev = classesByDstNames[i].put(dstName, cls); + + if (inDebugMode && prev != null) { + if (duplicatesByNameByNs == null) { + duplicatesByNameByNs = new LinkedHashMap<>(); + } + + Map> duplicatesByName = duplicatesByNameByNs.computeIfAbsent(i, k -> new HashMap<>()); + Set duplicates = duplicatesByName.computeIfAbsent(dstName, k -> new HashSet<>()); + duplicates.add(prev); + duplicates.add(cls); + } + } + } + + if (duplicatesByNameByNs != null) { + StringBuilder errorBuilder = new StringBuilder("Found destination names occurring multiple times per namespace:"); + + for (Map.Entry>> duplicateByNs : duplicatesByNameByNs.entrySet()) { + int ns = duplicateByNs.getKey(); + Map> duplicatesByName = duplicateByNs.getValue(); + + for (Map.Entry> duplicate : duplicatesByName.entrySet()) { + String name = duplicate.getKey(); + Set duplicates = duplicate.getValue(); + + errorBuilder.append("\n - \"") + .append(name) + .append("\" in namespace ") + .append(getNamespaceName(ns)) + .append(" for classes ") + .append(duplicates.stream() + .map(ClassEntry::toString) + .collect(Collectors.joining(", "))); + } } + + throw new IllegalStateException(errorBuilder.append("\nContinuing to use this tree may lead to unexpected behavior.").toString()); } } + public boolean doesIndexByDstNames() { + return indexByDstNames; + } + + public void setSrcNameDevoidEntryMergingStrategy(SrcNameDevoidEntryMergingStrategy strategy) { + this.srcNameDevoidEntryMergingStrategy = strategy; + } + + public SrcNameDevoidEntryMergingStrategy getSrcNameDevoidEntryMergingStrategy() { + return srcNameDevoidEntryMergingStrategy; + } + + /** + * Whether additional assertions and safety checks should be enabled + * at the expense of performance. Useful for debugging or input + * data validation. + */ + @ApiStatus.Internal + public void setInDebugMode(boolean inDebugMode) { + this.inDebugMode = inDebugMode; + } + + @ApiStatus.Internal + public boolean isInDebugMode() { + return inDebugMode; + } + @ApiStatus.Experimental public void setHierarchyInfoProvider(@Nullable HierarchyInfoProvider provider) { hierarchyInfo = provider; @@ -300,7 +371,19 @@ public ClassMapping getClass(String name, int namespace) { if (namespace < 0 || !indexByDstNames) { return VisitableMappingTree.super.getClass(name, namespace); } else { - return classesByDstNames[namespace].get(name); + ClassMapping ret = classesByDstNames[namespace].get(name); + + if (inDebugMode) { + ClassMapping expected = VisitableMappingTree.super.getClass(name, namespace); + + if (ret != expected) { + throw new IllegalStateException("Class name \"" + name + + "\" in destination namespace " + getNamespaceName(namespace) + " is not unique: " + + ret + " conflicts with at least " + expected); + } + } + + return ret; } } @@ -308,6 +391,53 @@ public ClassMapping getClass(String name, int namespace) { public ClassMapping addClass(ClassMapping cls) { assertNotInVisitPass(); ClassEntry entry = cls instanceof ClassEntry && cls.getTree() == this ? (ClassEntry) cls : new ClassEntry(this, cls, getSrcNsEquivalent(cls)); + + if (inDebugMode) { + ClassEntry[] duplicates = null; + + for (int i = 0; i < entry.dstNames.length; i++) { + String dstName = entry.dstNames[i]; + + if (dstName == null) { + continue; + } + + ClassEntry existing = (ClassEntry) getClass(dstName, i); + + if (existing != null) { + if (duplicates == null) { + duplicates = new ClassEntry[entry.dstNames.length]; + } + + duplicates[i] = existing; + } + } + + if (duplicates != null) { + StringBuilder errorBuilder = new StringBuilder("Can't add \"") + .append(entry.toString(true)) + .append("\", some of its destination names are already assigned to other classes:"); + + for (int ns = 0; ns < duplicates.length; ns++) { + ClassEntry prev = duplicates[ns]; + + if (prev == null) { + continue; + } + + errorBuilder.append("\n - \"") + .append(entry.dstNames[ns]) + .append("\" in namespace \"") + .append(getNamespaceName(ns)) + .append("\" by \"") + .append(prev.toString(false)) + .append("\""); + } + + throw new IllegalStateException(errorBuilder.toString()); + } + } + ClassEntry ret = classesBySrcName.putIfAbsent(cls.getSrcName(), entry); if (ret != null) { @@ -394,17 +524,19 @@ public void reset() { inVisitPass = false; srcNsMap = SRC_NAMESPACE_ID; dstNameMap = null; + disassociatedSourceNs = false; currentEntry = null; currentClass = null; currentMethod = null; pendingClasses = null; pendingMembers = null; + pendingMemberDescs = null; } @Override public void visitNamespaces(String srcNamespace, List dstNamespaces) { + reset(); // Just in case we never reached visitEnd in the previous pass due to an unexpected exception inVisitPass = true; - srcNsMap = SRC_NAMESPACE_ID; dstNameMap = new int[dstNamespaces.size()]; if (this.srcNamespace != null) { // ns already set, try to merge @@ -412,12 +544,13 @@ public void visitNamespaces(String srcNamespace, List dstNamespaces) { srcNsMap = this.dstNamespaces.indexOf(srcNamespace); if (srcNsMap < 0) { - reset(); - throw new IllegalArgumentException("can't merge with disassociated src namespace"); // srcNamespace must already be present + disassociatedSourceNs = true; + srcNsMap = NULL_NAMESPACE_ID; } } int newDstNamespaces = 0; + List treeSideDstNamespaces = this.dstNamespaces; for (int i = 0; i < dstNameMap.length; i++) { String dstNs = dstNamespaces.get(i); @@ -429,13 +562,15 @@ public void visitNamespaces(String srcNamespace, List dstNamespaces) { reset(); throw new IllegalArgumentException("namespace \"" + srcNamespace + "\" is present on both source and destination side simultaneously"); } else { - idx = this.dstNamespaces.indexOf(dstNs); + idx = treeSideDstNamespaces.indexOf(dstNs); if (idx < 0) { - if (newDstNamespaces == 0) this.dstNamespaces = new ArrayList<>(this.dstNamespaces); + if (newDstNamespaces == 0) { + treeSideDstNamespaces = new ArrayList<>(treeSideDstNamespaces); + } - idx = this.dstNamespaces.size(); - this.dstNamespaces.add(dstNs); + idx = treeSideDstNamespaces.size(); + treeSideDstNamespaces.add(dstNs); newDstNamespaces++; } } @@ -443,6 +578,23 @@ public void visitNamespaces(String srcNamespace, List dstNamespaces) { dstNameMap[i] = idx; } + if (disassociatedSourceNs) { + if (newDstNamespaces == dstNameMap.length) { + reset(); + throw new IllegalArgumentException("none of the incoming namespaces are present in the non-empty tree, can't merge"); + } + + if (newDstNamespaces == 0) { + treeSideDstNamespaces = new ArrayList<>(treeSideDstNamespaces); + } + + srcNsMap = treeSideDstNamespaces.size(); + treeSideDstNamespaces.add(srcNamespace); + newDstNamespaces++; + } + + this.dstNamespaces = treeSideDstNamespaces; + if (newDstNamespaces > 0) { int newSize = this.dstNamespaces.size(); resizeDstNames(newSize); @@ -557,6 +709,8 @@ public boolean visitMethod(String srcName, @Nullable String srcDesc) { } private ClassEntry queuePendingClass(String name) { + assert srcNsMap >= 0; + if (pendingClasses == null) pendingClasses = new HashMap<>(); ClassEntry cls = pendingClasses.get(name); @@ -565,84 +719,261 @@ private ClassEntry queuePendingClass(String name) { pendingClasses.put(name, cls); } - assert srcNsMap >= 0; cls.setDstNameInternal(name, srcNsMap); return cls; } private MemberEntry queuePendingMember(String name, @Nullable String desc, boolean isField) { - if (pendingMembers == null) pendingMembers = new HashMap<>(); + assert srcNsMap >= 0; + + if (pendingMembers == null) { + pendingMembers = new HashMap<>(); + pendingMemberDescs = new HashMap<>(); + } + GlobalMemberKey key = new GlobalMemberKey(currentClass, name, desc, isField); MemberEntry member = pendingMembers.get(key); if (member == null) { if (isField) { - member = new FieldEntry(currentClass, null, desc); // we're misusing the srcDesc field to store the dstDesc (as there is no dstDesc field) + member = new FieldEntry(currentClass, null, null); } else { - member = new MethodEntry(currentClass, null, desc); + member = new MethodEntry(currentClass, null, null); } pendingMembers.put(key, member); + pendingMemberDescs.computeIfAbsent(member, m -> new String[dstNamespaces.size()])[srcNsMap] = desc; } - assert srcNsMap >= 0; member.setDstNameInternal(name, srcNsMap); return member; } private void addPendingClass(ClassEntry cls) { - if (cls.isSrcNameMissing()) { + int startIdx = cls.isSrcNameMissing() ? 0 : SRC_NAMESPACE_ID; + Set matches = null; + + for (int i = startIdx; i <= dstNameMap.length; i++) { + int ns = i; + + if (ns != SRC_NAMESPACE_ID) { + if (ns == 0) { + ns = srcNsMap; + + if (disassociatedSourceNs) { + continue; // no existing entries' names can be found here, ns didn't exist before this pass + } + } else { + ns = dstNameMap[ns - 1]; + } + } + + String name = ns == SRC_NAMESPACE_ID + ? cls.getSrcNameUnchecked() + : cls.getDstName(ns); + + if (name == null) { + continue; + } + + ClassEntry existing = (ClassEntry) getClass(name, ns); + + if (existing != null) { + if (srcNameDevoidEntryMergingStrategy == SrcNameDevoidEntryMergingStrategy.FIRST_MATCH) { + matches = Collections.singleton(existing); + break; + } else { + if (matches == null) { + matches = Collections.newSetFromMap(new LinkedHashMap<>()); + } + + matches.add(existing); + } + } else if (ns == SRC_NAMESPACE_ID) { + classesBySrcName.put(name, cls); + cls.updateIndexByDstNames(); + return; + } + } + + if (matches == null) { return; } - String srcName = cls.getSrcName(); - ClassEntry existing = classesBySrcName.get(srcName); + if (matches.size() > 1 && srcNameDevoidEntryMergingStrategy == SrcNameDevoidEntryMergingStrategy.REJECT_AMBIGUOUS) { + // TODO: Add a way to report this to the user (PR 94?) + + if (inDebugMode) { + throw new IllegalStateException("Found multiple matches for " + cls.toString(true)); + } - if (existing == null) { - classesBySrcName.put(srcName, cls); - } else { // copy remaining data - existing.copyFrom(cls, true); + return; + } + + ClassEntry latestSubmittedMatch = null; + + for (ClassEntry treeCls : classesBySrcName.values()) { + if (matches.contains(treeCls)) { + latestSubmittedMatch = treeCls; + } + } + + matches.remove(latestSubmittedMatch); + matches.add(cls); + + for (ClassEntry match : matches) { + latestSubmittedMatch.copyFrom(match, true); + } + + if (cls.isSrcNameMissing()) { + // Copy source name so cls's pending members can be processed later on + cls.setSrcName(latestSubmittedMatch.getSrcName()); } } private void addPendingMember(MemberEntry member) { - if (member.isSrcNameMissing() || member.getOwner().isSrcNameMissing()) { + if (member.getOwner().isSrcNameMissing()) { return; } // Make sure the owner reference is pointing to an in-tree entry ClassEntry owner = classesBySrcName.get(member.getOwner().getSrcName()); member.setOwner(owner); + boolean isField = member.getKind() == MappedElementKind.FIELD; - String srcName = member.getSrcName(); - String dstDesc = member.getSrcDesc(); // pending members' srcDesc is actually their dst desc + int startIdx = member.isSrcNameMissing() && member.getSrcDesc() == null ? 0 : SRC_NAMESPACE_ID; + Map.Entry anyDesc = null; + Set> matches = null; + int findDescPhase = 0; + int addToTreePhase = 1; String srcDesc = null; - if (isValidDescriptor(dstDesc, !isField)) { - srcDesc = mapDesc(dstDesc, srcNsMap, SRC_NAMESPACE_ID); + for (int phase = findDescPhase; phase <= addToTreePhase; phase++) { + for (int i = startIdx; i <= dstNameMap.length; i++) { + int ns = i; + + if (ns != SRC_NAMESPACE_ID) { + if (ns == 0) { + ns = srcNsMap; + + if (disassociatedSourceNs && phase != findDescPhase) { + continue; // no existing entries' names can be found here, ns didn't exist before this pass + } + } else { + ns = dstNameMap[ns - 1]; + } + + if (ns == SRC_NAMESPACE_ID && startIdx == SRC_NAMESPACE_ID) { + continue; // already checked in the first iteration + } + } + + String desc = ns == SRC_NAMESPACE_ID + ? member.getSrcDesc() + : pendingMemberDescs.get(member)[ns]; + + if (phase == findDescPhase) { + if (isValidDescriptor(desc, !isField)) { + anyDesc = new AbstractMap.SimpleEntry<>(ns, desc); + break; + } + + continue; + } + + String name = ns == SRC_NAMESPACE_ID + ? member.getSrcNameUnchecked() + : member.getDstName(ns); + + if (name == null) { + continue; + } + + if (desc == null && anyDesc != null) { + desc = mapDesc(anyDesc.getValue(), anyDesc.getKey(), ns); + } + + if (ns == SRC_NAMESPACE_ID) { + assert srcDesc == null; + srcDesc = desc; + } + + MemberEntry existing = isField + ? owner.getField(name, desc, ns) + : owner.getMethod(name, desc, ns); + + if (existing != null) { + if (srcNameDevoidEntryMergingStrategy == SrcNameDevoidEntryMergingStrategy.FIRST_MATCH) { + matches = Collections.singleton(existing); + break; + } else { + if (matches == null) { + matches = Collections.newSetFromMap(new LinkedHashMap<>()); + } + + matches.add(existing); + } + } else if (ns == SRC_NAMESPACE_ID) { + if (member.getSrcDesc() == null && srcDesc != null) { + member.setSrcDescInternal(srcDesc); + } + + if (isField) { + owner.addFieldInternal((FieldEntry) member); + } else { + owner.addMethodInternal((MethodEntry) member); + } + + return; + } + } } - member.setSrcDescInternal(srcDesc); + if (matches == null) { + return; + } - if (isField) { - FieldEntry queuedField = (FieldEntry) member; - FieldEntry existingField = owner.getField(srcName, srcDesc); + if (matches.size() > 1 && srcNameDevoidEntryMergingStrategy == SrcNameDevoidEntryMergingStrategy.REJECT_AMBIGUOUS) { + // TODO: Add a way to report this to the user (PR 94?) - if (existingField == null) { - owner.addFieldInternal(queuedField); - } else { // copy remaining data - existingField.copyFrom(queuedField, true); + if (inDebugMode) { + throw new IllegalStateException("Found multiple matches for " + member.toString(true) + " in " + member.getOwner().toString(true)); + } + + return; + } + + MemberEntry latestSubmittedMatch = null; + Collection> treeMembers = isField + ? owner.getFields() + : owner.getMethods(); + + for (MemberEntry treeMember : treeMembers) { + if (matches.contains(treeMember)) { + latestSubmittedMatch = treeMember; + } + } + + matches.remove(latestSubmittedMatch); + matches.add(member); + + for (MemberEntry match : matches) { + if (isField) { + ((FieldEntry) latestSubmittedMatch).copyFrom((FieldEntry) match, true); + } else { + ((MethodEntry) latestSubmittedMatch).copyFrom((MethodEntry) match, true); + } + } + + if (latestSubmittedMatch.getSrcDesc() == null) { + if (srcDesc == null && anyDesc != null) { + srcDesc = mapDesc(anyDesc.getValue(), anyDesc.getKey(), SRC_NAMESPACE_ID); } - } else { - MethodEntry queuedMethod = (MethodEntry) member; - MethodEntry existingMethod = owner.getMethod(srcName, srcDesc); - if (existingMethod == null) { - owner.addMethodInternal(queuedMethod); - } else { // copy remaining data - existingMethod.copyFrom(queuedMethod, true); + if (srcDesc != null) { + latestSubmittedMatch.setSrcDescInternal(srcDesc); } } } @@ -804,6 +1135,25 @@ public void visitDstName(MappedElementKind targetKind, int namespace, String nam } } + @Override + public void visitDstDesc(MappedElementKind targetKind, int namespace, String desc) throws IOException { + namespace = dstNameMap[namespace]; + + if (currentEntry == null) throw new UnsupportedOperationException("Tried to visit mapped descriptor before owner"); + + MemberEntry currentMember = (MemberEntry) currentEntry; + + if (pendingMemberDescs == null || !pendingMemberDescs.containsKey(currentMember)) { + return; + } + + if (namespace < 0) { + currentMember.setSrcDescInternal(desc); + } else { + pendingMemberDescs.get(currentMember)[namespace] = desc; + } + } + @Override public void visitComment(MappedElementKind targetKind, String comment) { Entry entry; @@ -841,6 +1191,32 @@ void assertNotInVisitPass() { } } + /** + * Strategy for merging incoming entries which don't supply a name for the tree-side source namespace, + * but do supply a name for a tree-side destination namespace, + * which might already be in use by an existing entry (= match). + */ + public enum SrcNameDevoidEntryMergingStrategy { + /** + * Pending data will be merged into the first matching entry, + * there will be no checks if further matches might exist. + */ + FIRST_MATCH, + + /** + * Checks if multiple matches exist. If not, same as {@link #FIRST_MATCH}, + * otherwise merges all matching entries into the last submitted entry, + * merges pending data into there and then deletes the other matches from the tree. + */ + ALL_MATCHES, + + /** + * Checks if multiple matches exist. If not, same as {@link #FIRST_MATCH}, + * otherwise ignores the pending data. + */ + REJECT_AMBIGUOUS + } + abstract static class Entry> implements ElementMapping { protected Entry(MemoryMappingTree tree, String srcName) { this.tree = tree; @@ -851,6 +1227,17 @@ protected Entry(MemoryMappingTree tree, String srcName) { protected Entry(MemoryMappingTree tree, ElementMapping src, int srcNsEquivalent) { this(tree, src.getName(srcNsEquivalent)); + if (!(this instanceof MemberEntry)) { + constructorCopyFrom(src); + } + } + + /** + * Ideally we would just always do this in the constructor above, + * but {@link MemberEntry#setDstNameInternal(String, int)} requires {@link MemberEntry#owner} to be set, + * which is not possible due to the compiler forcing us to call the super constructor above first. + */ + protected final void constructorCopyFrom(ElementMapping src) { for (int i = 0; i < dstNames.length; i++) { int dstNsEquivalent = src.getTree().getNamespaceId(tree.dstNamespaces.get(i)); @@ -974,7 +1361,13 @@ protected final boolean acceptElement(MappingVisitor visitor, @Nullable String[] protected void copyFrom(T o, boolean replace) { for (int i = 0; i < dstNames.length; i++) { if (o.dstNames[i] != null && (replace || dstNames[i] == null)) { - dstNames[i] = o.dstNames[i]; + String oDstName = o.dstNames[i]; + + if (this instanceof ClassEntry) { + ((ClassEntry) this).updateIndexByDstNames(oDstName, i, false); + } + + dstNames[i] = oDstName; } } @@ -983,6 +1376,13 @@ protected void copyFrom(T o, boolean replace) { } } + protected abstract String toString(boolean withOwners); + + @Override + public final String toString() { + return toString(false); + } + private final boolean missingSrcNameAllowed = getKind().level > MappedElementKind.METHOD.level; // args and vars protected final MemoryMappingTree tree; private String srcName; @@ -1019,24 +1419,48 @@ public MemoryMappingTree getTree() { @Override void setDstNameInternal(String name, int namespace) { - if (tree.indexByDstNames) { - String oldName = dstNames[namespace]; - - if (!Objects.equals(name, oldName)) { - Map map = tree.classesByDstNames[namespace]; - if (oldName != null) map.remove(oldName); - - if (name != null) { - map.put(name, this); - } else { - map.remove(oldName); - } + if (tree.inDebugMode) { + ClassMapping existing = tree.getClass(name, namespace); + + if (existing != null && existing != this && getSrcNameUnchecked() != null) { + throw new IllegalArgumentException("Destination name \"" + + name + "\" in namespace " + tree.getNamespaceName(namespace) + + " in the process to be set for class " + toString() + + " is already in use by " + existing.toString() + + ", aborting"); } } + updateIndexByDstNames(name, namespace, false); + super.setDstNameInternal(name, namespace); } + private void updateIndexByDstNames() { + for (int ns = 0; ns < dstNames.length; ns++) { + updateIndexByDstNames(dstNames[ns], ns, true); + } + } + + private void updateIndexByDstNames(String name, int namespace, boolean skipEqualityCheck) { + if (!tree.indexByDstNames || tree.getClass(getSrcNameUnchecked()) != this /* pending */) { + return; + } + + String oldName = dstNames[namespace]; + + if (skipEqualityCheck || !Objects.equals(name, oldName)) { + Map map = tree.classesByDstNames[namespace]; + if (oldName != null) map.remove(oldName); + + if (name != null) { + map.put(name, this); + } else { + map.remove(oldName); + } + } + } + @Override public Collection getFields() { if (fields == null) return Collections.emptyList(); @@ -1191,43 +1615,116 @@ private static > T getMember(String srcName, @Nullable } private > T addMember(T entry, Map map, int flagHasAny, int flagMissesAny) { - T ret = map.putIfAbsent(entry.getKey(), entry); + T existing = map.get(entry.getKey()); + T existingDescless = null; + T existingDescContaining = null; + boolean validDesc = false; + MemberKey existingDesclessKey = null; + byte flags = this.flags; + + if (existing == null) { + if (isValidDescriptor(entry.srcDesc, true)) { // entry has desc, check for desc-less match + validDesc = true; + flags |= flagHasAny; + + if ((flags & flagMissesAny) != 0) { + existingDesclessKey = new MemberKey(entry.getSrcName(), null); + existingDescless = map.get(existingDesclessKey); + } + } else if ((flags & flagHasAny) != 0) { // entry has no desc, check for desc-containing match + for (T mapEntry : map.values()) { + if (mapEntry != entry && mapEntry.getSrcName().equals(entry.getSrcName()) && (entry.srcDesc == null || mapEntry.srcDesc.startsWith(entry.srcDesc))) { + existingDescContaining = mapEntry; + break; + } + } + } + } - if (ret != null) { // same desc - ret.copyFrom(entry, true); + if (tree.inDebugMode) { + MemberEntry[] duplicates = null; - return ret; - } else if (isValidDescriptor(entry.srcDesc, true)) { // may have replaced desc-less - flags |= flagHasAny; - - if ((flags & flagMissesAny) != 0) { - ret = map.remove(new MemberKey(entry.getSrcName(), null)); - - if (ret != null) { // compatible entry exists, copy desc + extra content - ret.setKey(entry.getKey()); - ret.srcDesc = entry.srcDesc; - map.put(ret.getKey(), ret); - ret.copyFrom(entry, true); - entry = ret; + for (int ns = 0; ns < entry.dstNames.length; ns++) { + String dstName = entry.getDstName(ns); + + if (dstName == null) { + continue; + } + + MemberEntry match = entry.getKind() == MappedElementKind.FIELD + ? getField(dstName, entry.getDstDesc(ns), ns) + : getMethod(dstName, entry.getDstDesc(ns), ns); + + if (match != existing && match != existingDescless && match != existingDescContaining) { + if (duplicates == null) { + duplicates = new MemberEntry[entry.dstNames.length]; + } + + duplicates[ns] = match; } } - return entry; - } else { // entry.srcDesc == null, may have replaced desc-containing - if ((flags & flagHasAny) != 0) { - for (T prevEntry : map.values()) { - if (prevEntry != entry && prevEntry.getSrcName().equals(entry.getSrcName()) && (entry.srcDesc == null || prevEntry.srcDesc.startsWith(entry.srcDesc))) { - map.remove(entry.getKey()); - prevEntry.copyFrom(entry, true); + if (duplicates != null) { + StringBuilder errorBuilder = new StringBuilder("Can't add \"") + .append(entry.toString(true)) + .append("\", some of its destination names are already assigned to other entries:"); + + for (int ns = 0; ns < duplicates.length; ns++) { + MemberEntry prev = duplicates[ns]; - return prevEntry; + if (prev == null) { + continue; } + + errorBuilder.append("\n - \"") + .append(entry.dstNames[ns]) + .append("\" in namespace \"") + .append(tree.getNamespaceName(ns)) + .append("\" by \"") + .append(prev.toString(false)) + .append("\""); } + + throw new IllegalStateException(errorBuilder.toString()); } + } + + if (existing != null) { // same desc + existing.copyFrom(entry, true); + + return existing; + } else { + T ret = entry; - flags |= flagMissesAny; + if (validDesc) { + flags |= flagHasAny; - return entry; + existing = map.put(entry.getKey(), entry); + assert existing == null; + + if (existingDescless != null) { // compatible desc-less entry exists, copy desc + extra content + assert existingDesclessKey != null; + + map.remove(existingDesclessKey); + + existingDescless.copyFrom(entry, true); + ret = existingDescless; + } + } else { + if (existingDescContaining != null) { + existingDescContaining.copyFrom(entry, true); + + ret = existingDescContaining; + } else { + existing = map.put(entry.getKey(), entry); + assert existing == null; + } + + flags |= flagMissesAny; + } + + this.flags = flags; + return ret; } } @@ -1303,7 +1800,7 @@ protected void copyFrom(ClassEntry o, boolean replace) { } @Override - public String toString() { + protected String toString(boolean withOwners) { return getSrcNameUnchecked(); } @@ -1334,6 +1831,8 @@ protected MemberEntry(ClassEntry owner, MemberMapping src, int srcNsEquivalent) this.owner = owner; this.srcDesc = src.getDesc(srcNsEquivalent); this.key = new MemberKey(getSrcName(), srcDesc); + + constructorCopyFrom(src); } @Override @@ -1367,6 +1866,21 @@ public final String getSrcDesc() { abstract void setSrcDescInternal(@Nullable String desc); + @Override + void setDstNameInternal(String name, int namespace) { + if (tree.inDebugMode) { + MemberEntry existing = getKind() == MappedElementKind.FIELD + ? owner.getField(name, getDesc(namespace), namespace) + : owner.getMethod(name, getDesc(namespace), namespace); + + if (existing != null && existing != this) { + throw new IllegalArgumentException("Destination name \"" + name + "\" in namespace " + tree.getNamespaceName(namespace) + " is already in use by " + existing.toString(true)); + } + } + + super.setDstNameInternal(name, namespace); + } + MemberKey getKey() { assertSrcNamePresent(); return key; @@ -1393,6 +1907,19 @@ protected final boolean acceptMember(MappingVisitor visitor, boolean supplyDstDe return acceptElement(visitor, dstDescs); } + protected String toString(boolean completeHierarchy) { + String toFormat = getKind() == MappedElementKind.FIELD + ? "%s:%s" + : "%s%s"; + String ret = String.format(toFormat, getSrcNameUnchecked(), srcDesc); + + if (completeHierarchy) { + ret = String.format("%s.%s", owner.toString(true), ret); + } + + return ret; + } + protected ClassEntry owner; protected String srcDesc; private MemberKey key; @@ -1425,7 +1952,10 @@ void setSrcDescInternal(@Nullable String desc) { MemberKey newKey = new MemberKey(getSrcName(), desc); if (owner.fields != null) { // pending member - if (owner.fields.containsKey(newKey)) throw new IllegalArgumentException("conflicting name+desc after changing desc to "+desc+" for "+this); + if (owner.fields.containsKey(newKey)) { + throw new IllegalArgumentException("conflicting name+desc after changing desc to "+desc+" for "+toString(true)+", skipping"); + } + owner.fields.remove(getKey()); } @@ -1448,11 +1978,6 @@ void accept(MappingVisitor visitor, boolean supplyDstDescs) throws IOException { acceptMember(visitor, supplyDstDescs); } } - - @Override - public String toString() { - return String.format("%s;;%s", getSrcNameUnchecked(), srcDesc); - } } static final class MethodEntry extends MemberEntry implements MethodMapping { @@ -1748,11 +2273,6 @@ protected void copyFrom(MethodEntry o, boolean replace) { } } - @Override - public String toString() { - return String.format("%s%s", getSrcNameUnchecked(), srcDesc); - } - private List args = null; private List vars = null; private List argsView = null; @@ -1845,8 +2365,14 @@ protected void copyFrom(MethodArgEntry o, boolean replace) { } @Override - public String toString() { - return String.format("%d/%d:%s", argPosition, lvIndex, getSrcName()); + protected String toString(boolean withOwners) { + String ret = String.format("%s:%d/%d", getSrcName(), argPosition, lvIndex); + + if (withOwners) { + ret = String.format("%s.%s", method.toString(true), ret); + } + + return ret; } private final MethodEntry method; @@ -1956,8 +2482,14 @@ protected void copyFrom(MethodVarEntry o, boolean replace) { } @Override - public String toString() { - return String.format("%d/%d@%d-%d:%s", lvtRowIndex, lvIndex, startOpIdx, endOpIdx, getSrcName()); + protected String toString(boolean withOwners) { + String ret = String.format("%s:%d/%d@%d-%d", getSrcName(), lvtRowIndex, lvIndex, startOpIdx, endOpIdx); + + if (withOwners) { + ret = String.format("%s.%s", method.toString(true), ret); + } + + return ret; } private final MethodEntry method; @@ -2098,24 +2630,33 @@ public String toString() { private final boolean isField; } - private boolean inVisitPass; + // --- Configuration --- private boolean indexByDstNames; + private SrcNameDevoidEntryMergingStrategy srcNameDevoidEntryMergingStrategy = SrcNameDevoidEntryMergingStrategy.ALL_MATCHES; + private boolean inDebugMode; + + // --- Internal state --- + private boolean inVisitPass; private String srcNamespace; private List dstNamespaces = Collections.emptyList(); private final List metadata = new ArrayList<>(); private final Map classesBySrcName = new LinkedHashMap<>(); private final Collection classesView = Collections.unmodifiableCollection(classesBySrcName.values()); private Map[] classesByDstNames; - private HierarchyInfoProvider hierarchyInfo; + // --- Current visit pass --- /** The incoming source namespace's namespace index on the tree side. */ private int srcNsMap; + /** Incoming destination namespaces' namespace indices on the tree side. dstNameMap[incomingNsIdx] = treeSideNsIdx. */ private int[] dstNameMap; + /** Whether the incoming source namespace was not in the tree prior to the current visit pass. */ + private boolean disassociatedSourceNs; private Entry currentEntry; private ClassEntry currentClass; private MethodEntry currentMethod; /** originalSrcName -> clsEntry. */ private Map pendingClasses; private Map> pendingMembers; + private Map, String[]> pendingMemberDescs; } diff --git a/src/test/java/net/fabricmc/mappingio/test/TestUtil.java b/src/test/java/net/fabricmc/mappingio/test/TestUtil.java index 321d5aa7..d4c8322d 100644 --- a/src/test/java/net/fabricmc/mappingio/test/TestUtil.java +++ b/src/test/java/net/fabricmc/mappingio/test/TestUtil.java @@ -28,7 +28,9 @@ import net.fabricmc.mappingio.MappingWriter; import net.fabricmc.mappingio.format.MappingFormat; +import net.fabricmc.mappingio.tree.MappingTree; import net.fabricmc.mappingio.tree.MappingTreeView; +import net.fabricmc.mappingio.tree.MemoryMappingTree; public final class TestUtil { public static void setResourceRoot(Path path) { @@ -53,6 +55,19 @@ public static Path getResource(String slashPrefixedResourcePath) { } } + public static MemoryMappingTree createTree() { + return createTree(null); + } + + public static MemoryMappingTree createTree(MappingTree src) { + MemoryMappingTree tree = src == null + ? new MemoryMappingTree() + : new MemoryMappingTree(src); + tree.setInDebugMode(true); + tree.setIndexByDstNames(true); + return tree; + } + @Nullable public static String getFileName(MappingFormat format) { switch (format) { diff --git a/src/test/java/net/fabricmc/mappingio/test/tests/OuterClassNamePropagationTest.java b/src/test/java/net/fabricmc/mappingio/test/tests/OuterClassNamePropagationTest.java index 8bc1128c..4a140665 100644 --- a/src/test/java/net/fabricmc/mappingio/test/tests/OuterClassNamePropagationTest.java +++ b/src/test/java/net/fabricmc/mappingio/test/tests/OuterClassNamePropagationTest.java @@ -16,6 +16,7 @@ package net.fabricmc.mappingio.test.tests; +import static net.fabricmc.mappingio.test.TestUtil.createTree; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertThrows; @@ -39,7 +40,6 @@ import net.fabricmc.mappingio.test.visitors.NopMappingVisitor; import net.fabricmc.mappingio.test.visitors.SubsetAssertingVisitor; import net.fabricmc.mappingio.tree.MappingTreeView; -import net.fabricmc.mappingio.tree.MemoryMappingTree; import net.fabricmc.mappingio.tree.VisitableMappingTree; public class OuterClassNamePropagationTest { @@ -49,7 +49,7 @@ public class OuterClassNamePropagationTest { @BeforeAll public static void setup() throws IOException { - MappingTreeView tree = acceptMappings(new MemoryMappingTree()); + MappingTreeView tree = acceptMappings(createTree()); srcNamespace = tree.getSrcNamespace(); dstNamespaces = tree.getDstNamespaces(); @@ -90,7 +90,7 @@ public void visitorThroughTree() throws IOException { for (int pass = 1; pass <= 2; pass++) { boolean processRemappedDstNames = pass == 1; - VisitableMappingTree tree = new MemoryMappingTree(); + VisitableMappingTree tree = createTree(); acceptMappings(new OuterClassNamePropagator(tree, dstNamespaces, processRemappedDstNames)); tree.accept(new OuterClassNameChecker(true, dstNamespaces, processRemappedDstNames)); @@ -103,14 +103,14 @@ public void tree() throws IOException { for (int pass = 1; pass <= 2; pass++) { boolean processRemappedDstNames = pass == 1; - VisitableMappingTree tree = acceptMappings(new MemoryMappingTree()); + VisitableMappingTree tree = acceptMappings(createTree()); tree.propagateOuterClassNames(processRemappedDstNames); tree.accept(new OuterClassNameChecker(true, dstNamespaces, processRemappedDstNames)); checkDiskEquivalence(tree, processRemappedDstNames); } - VisitableMappingTree tree = acceptMappings(new MemoryMappingTree()); + VisitableMappingTree tree = acceptMappings(createTree()); assertThrows(UnsupportedOperationException.class, () -> tree.propagateOuterClassNames( dstNamespaces.get(0), @@ -132,7 +132,7 @@ private void checkDiskEquivalence(VisitableMappingTree tree, boolean processRema ? TestMappings.PROPAGATION.PROPAGATED : TestMappings.PROPAGATION.PROPAGATED_EXCEPT_REMAPPED_DST; - VisitableMappingTree diskTree = dir.read(format, new MemoryMappingTree()); + VisitableMappingTree diskTree = dir.read(format, createTree()); tree.accept(new FlatAsRegularMappingVisitor(new SubsetAssertingVisitor(diskTree, format, null))); diskTree.accept(new FlatAsRegularMappingVisitor(new SubsetAssertingVisitor(tree, null, format))); diff --git a/src/test/java/net/fabricmc/mappingio/test/tests/reading/EmptyContentReadTest.java b/src/test/java/net/fabricmc/mappingio/test/tests/reading/EmptyContentReadTest.java index 491f9e55..a4a9c269 100644 --- a/src/test/java/net/fabricmc/mappingio/test/tests/reading/EmptyContentReadTest.java +++ b/src/test/java/net/fabricmc/mappingio/test/tests/reading/EmptyContentReadTest.java @@ -16,6 +16,7 @@ package net.fabricmc.mappingio.test.tests.reading; +import static net.fabricmc.mappingio.test.TestUtil.createTree; import static org.junit.jupiter.api.Assertions.assertThrows; import java.io.IOException; @@ -36,14 +37,13 @@ import net.fabricmc.mappingio.format.tiny.Tiny1FileReader; import net.fabricmc.mappingio.format.tiny.Tiny2FileReader; import net.fabricmc.mappingio.test.visitors.VisitOrderVerifyingVisitor; -import net.fabricmc.mappingio.tree.MemoryMappingTree; public class EmptyContentReadTest { private MappingVisitor target; @BeforeEach public void instantiateTree() { - target = new VisitOrderVerifyingVisitor(new MemoryMappingTree()); + target = new VisitOrderVerifyingVisitor(createTree()); } @Test diff --git a/src/test/java/net/fabricmc/mappingio/test/tests/reading/ValidContentReadTest.java b/src/test/java/net/fabricmc/mappingio/test/tests/reading/ValidContentReadTest.java index 10eac065..a3274403 100644 --- a/src/test/java/net/fabricmc/mappingio/test/tests/reading/ValidContentReadTest.java +++ b/src/test/java/net/fabricmc/mappingio/test/tests/reading/ValidContentReadTest.java @@ -16,6 +16,8 @@ package net.fabricmc.mappingio.test.tests.reading; +import static net.fabricmc.mappingio.test.TestUtil.createTree; + import java.nio.file.Files; import org.jetbrains.annotations.Nullable; @@ -31,7 +33,6 @@ import net.fabricmc.mappingio.test.visitors.SubsetAssertingVisitor; import net.fabricmc.mappingio.test.visitors.VisitOrderVerifyingVisitor; import net.fabricmc.mappingio.tree.MappingTreeView; -import net.fabricmc.mappingio.tree.MemoryMappingTree; import net.fabricmc.mappingio.tree.VisitableMappingTree; public class ValidContentReadTest { @@ -59,8 +60,8 @@ private void check(MappingDir dir, MappingFormat format) throws Exception { // TODO: The Tiny v2 spec also disallows repeated elements, there should at least be warnings boolean allowConsecutiveDuplicateElementVisits = dir == TestMappings.READING.REPEATED_ELEMENTS; - VisitableMappingTree referenceTree = dir.generate(new MemoryMappingTree()); - VisitableMappingTree tree = new MemoryMappingTree(); + VisitableMappingTree referenceTree = dir.generate(createTree()); + VisitableMappingTree tree = createTree(); dir.read(format, new VisitOrderVerifyingVisitor(tree, allowConsecutiveDuplicateElementVisits)); assertEqual(tree, format, referenceTree, allowConsecutiveDuplicateElementVisits); @@ -69,7 +70,7 @@ private void check(MappingDir dir, MappingFormat format) throws Exception { return; } - tree = new MemoryMappingTree(); + tree = createTree(); String newSrcNs = format.features().hasNamespaces() ? referenceTree.getDstNamespaces().get(0) : MappingUtil.NS_TARGET_FALLBACK; diff --git a/src/test/java/net/fabricmc/mappingio/test/tests/tree/DebugModeTest.java b/src/test/java/net/fabricmc/mappingio/test/tests/tree/DebugModeTest.java new file mode 100644 index 00000000..2f819186 --- /dev/null +++ b/src/test/java/net/fabricmc/mappingio/test/tests/tree/DebugModeTest.java @@ -0,0 +1,201 @@ +/* + * Copyright (c) 2024 FabricMC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package net.fabricmc.mappingio.test.tests.tree; + +import static net.fabricmc.mappingio.test.TestUtil.createTree; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; + +import java.util.Arrays; + +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +import net.fabricmc.mappingio.MappedElementKind; +import net.fabricmc.mappingio.MappingVisitor; +import net.fabricmc.mappingio.test.visitors.VisitOrderVerifyingVisitor; +import net.fabricmc.mappingio.tree.MappingTree.ClassMapping; +import net.fabricmc.mappingio.tree.MappingTree.FieldMapping; +import net.fabricmc.mappingio.tree.MappingTree.MethodMapping; +import net.fabricmc.mappingio.tree.MemoryMappingTree; + +public class DebugModeTest { + private static final String nsA = "nsA"; + private static final String nsB = "nsB"; + private static final String nsC = "nsC"; + private static final String cls1NsAName = "cls1NsAName"; + private static final String cls1NsBName = "cls1NsBName"; + private static final String cls1NsCName = "cls1NsCName"; + private static final String cls2NsAName = "cls2NsAName"; + private static final String fld1NsAName = "fld1NsAName"; + private static final String fld1NsBName = "fld1NsBName"; + private static final String fldNsADesc = "L" + cls1NsAName + ";"; + private static final String fld2NsAName = "fld2NsAName"; + private static final String mth1NsAName = "mth1NsAName"; + private static final String mth1NsBName = "mth1NsBName"; + private static final String mth2NsAName = "mth2NsAName"; + private static final String mthNsADesc = "(L" + cls1NsAName + ";)V"; + private static final int classPhase = 0; + private static final int fieldPhase = 1; + private static final int methodPhase = 2; + private MemoryMappingTree tree; + private MappingVisitor delegate; + + @BeforeEach + public void setup() { + tree = createTree(); + delegate = new VisitOrderVerifyingVisitor(tree); + } + + /** + * Tests {@link MemoryMappingTree#initClassesByDstNames()}. + */ + @Test + public void duplicateClsDstNameWithinNsAlreadyPresent() throws Exception { + for (int i = 0; i <= 2; i++) { + tree.setInDebugMode(false); + tree.setIndexByDstNames(false); + + delegate.visitHeader(); + delegate.visitNamespaces(nsA, Arrays.asList(nsB, nsC)); + delegate.visitContent(); + delegate.visitClass(cls1NsAName); + if (i != 1) delegate.visitDstName(MappedElementKind.CLASS, 0, cls1NsBName); + if (i != 0) delegate.visitDstName(MappedElementKind.CLASS, 1, cls1NsCName); + delegate.visitElementContent(MappedElementKind.CLASS); + delegate.visitClass(cls2NsAName); + if (i != 1) delegate.visitDstName(MappedElementKind.CLASS, 0, cls1NsBName); + if (i != 0) delegate.visitDstName(MappedElementKind.CLASS, 1, cls1NsCName); + delegate.visitElementContent(MappedElementKind.CLASS); + delegate.visitEnd(); + + tree.setInDebugMode(true); + assertThrows(IllegalStateException.class, () -> tree.setIndexByDstNames(true)); + setup(); + } + } + + /** + * Tests {@link MemoryMappingTree#visitDstName(MappedElementKind, int, String)}. + */ + @Test + public void insertDuplicateDstNameIntoNsViaVisitation() throws Exception { + int ns = 0; + + for (int phase = classPhase; phase <= methodPhase; phase++) { + delegate.visitHeader(); + delegate.visitNamespaces(nsA, Arrays.asList(nsB, nsC)); + delegate.visitContent(); + delegate.visitClass(cls1NsAName); + delegate.visitDstName(MappedElementKind.CLASS, ns, cls1NsBName); + delegate.visitElementContent(MappedElementKind.CLASS); + ClassMapping cls1 = tree.getClass(cls1NsAName); + + if (phase == classPhase) { + delegate.visitClass(cls2NsAName); + assertThrows(IllegalArgumentException.class, () -> delegate.visitDstName(MappedElementKind.CLASS, ns, cls1NsBName)); + assertEquals(cls1, tree.getClass(cls1NsBName, ns)); + } else if (phase == fieldPhase) { + delegate.visitField(fld1NsAName, fldNsADesc); + delegate.visitDstName(MappedElementKind.FIELD, ns, fld1NsBName); + delegate.visitElementContent(MappedElementKind.FIELD); + delegate.visitField(fld2NsAName, fldNsADesc); + assertThrows(IllegalArgumentException.class, () -> delegate.visitDstName(MappedElementKind.FIELD, ns, fld1NsBName)); + assertEquals(cls1.getField(fld1NsAName, fldNsADesc), cls1.getField(fld1NsBName, tree.mapDesc(fldNsADesc, ns), ns)); + } else { + delegate.visitMethod(mth1NsAName, mthNsADesc); + delegate.visitDstName(MappedElementKind.METHOD, ns, mth1NsBName); + delegate.visitElementContent(MappedElementKind.METHOD); + MethodMapping mth1 = cls1.getMethod(mth1NsAName, mthNsADesc); + + if (phase == methodPhase) { + delegate.visitMethod(mth2NsAName, mthNsADesc); + + assertThrows(IllegalArgumentException.class, () -> delegate.visitDstName(MappedElementKind.METHOD, ns, mth1NsBName)); + assertEquals(mth1, cls1.getMethod(mth1NsBName, tree.mapDesc(mthNsADesc, ns), ns)); + } + + // TODO: args and vars + } + + delegate.reset(); + } + } + + /** + * Tests {@link MemoryMappingTree#addClass(ClassMapping)}, + * {@link ClassMapping#addField(FieldMapping)} and + * {@link ClassMapping#addMethod(MethodMapping)}. + */ + @Test + public void insertDuplicateClsDstNameIntoNsViaTreeApi() throws Exception { + MemoryMappingTree treeToCopyFrom = createTree(); + treeToCopyFrom.visitHeader(); + treeToCopyFrom.visitNamespaces(nsA, Arrays.asList(nsB, nsC)); + treeToCopyFrom.visitContent(); + treeToCopyFrom.visitClass(cls2NsAName); + treeToCopyFrom.visitDstName(MappedElementKind.CLASS, 0, cls1NsBName); + treeToCopyFrom.visitDstName(MappedElementKind.CLASS, 1, cls1NsCName); + treeToCopyFrom.visitElementContent(MappedElementKind.CLASS); + treeToCopyFrom.visitField(fld2NsAName, fldNsADesc); + treeToCopyFrom.visitDstName(MappedElementKind.FIELD, 0, fld1NsBName); + treeToCopyFrom.visitElementContent(MappedElementKind.FIELD); + treeToCopyFrom.visitMethod(mth2NsAName, mthNsADesc); + treeToCopyFrom.visitDstName(MappedElementKind.METHOD, 0, mth1NsBName); + treeToCopyFrom.visitElementContent(MappedElementKind.METHOD); + treeToCopyFrom.visitEnd(); + + ClassMapping cls2ToAdd = treeToCopyFrom.getClass(cls2NsAName); + FieldMapping fld2ToAdd = cls2ToAdd.getField(fld2NsAName, fldNsADesc); + MethodMapping mth2ToAdd = cls2ToAdd.getMethod(mth2NsAName, mthNsADesc); + + for (int phase = classPhase; phase <= methodPhase; phase++) { + delegate.visitHeader(); + delegate.visitNamespaces(nsA, Arrays.asList(nsB, nsC)); + delegate.visitContent(); + delegate.visitClass(cls1NsAName); + delegate.visitDstName(MappedElementKind.CLASS, 0, cls1NsBName); + delegate.visitDstName(MappedElementKind.CLASS, 1, cls1NsCName); + delegate.visitElementContent(MappedElementKind.CLASS); + + if (phase == fieldPhase) { + delegate.visitField(fld1NsAName, fldNsADesc); + delegate.visitDstName(MappedElementKind.FIELD, 0, fld1NsBName); + delegate.visitElementContent(MappedElementKind.FIELD); + } else { + delegate.visitMethod(mth1NsAName, mthNsADesc); + delegate.visitDstName(MappedElementKind.METHOD, 0, mth1NsBName); + delegate.visitElementContent(MappedElementKind.METHOD); + } + + delegate.visitEnd(); + ClassMapping cls1 = tree.getClass(cls1NsAName); + + if (phase == classPhase) { + assertThrows(IllegalArgumentException.class, () -> tree.addClass(cls2ToAdd)); + } else if (phase == fieldPhase) { + assertThrows(IllegalArgumentException.class, () -> cls1.addField(fld2ToAdd)); + } else { + if (phase == methodPhase) { + assertThrows(IllegalArgumentException.class, () -> cls1.addMethod(mth2ToAdd)); + } + + // TODO: args and vars + } + } + } +} diff --git a/src/test/java/net/fabricmc/mappingio/test/tests/tree/MergeTest.java b/src/test/java/net/fabricmc/mappingio/test/tests/tree/MergeTest.java index 1924316b..ec3140c5 100644 --- a/src/test/java/net/fabricmc/mappingio/test/tests/tree/MergeTest.java +++ b/src/test/java/net/fabricmc/mappingio/test/tests/tree/MergeTest.java @@ -16,6 +16,7 @@ package net.fabricmc.mappingio.test.tests.tree; +import static net.fabricmc.mappingio.test.TestUtil.createTree; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertThrows; @@ -39,6 +40,7 @@ import net.fabricmc.mappingio.tree.MappingTree.ClassMapping; import net.fabricmc.mappingio.tree.MappingTree.FieldMapping; import net.fabricmc.mappingio.tree.MemoryMappingTree; +import net.fabricmc.mappingio.tree.MemoryMappingTree.SrcNameDevoidEntryMergingStrategy; public class MergeTest { private static final Path dir = TestMappings.MERGING; @@ -60,7 +62,7 @@ public class MergeTest { @BeforeEach public void setup() { - tree = new MemoryMappingTree(); + tree = createTree(); delegate = new VisitOrderVerifyingVisitor(tree); } @@ -285,19 +287,30 @@ public void descriptorCompletion() throws IOException { @Test public void diskMappings() throws IOException { + tree.setSrcNameDevoidEntryMergingStrategy(SrcNameDevoidEntryMergingStrategy.REJECT_AMBIGUOUS); + // Same namespaces, same order MappingReader.read(dir.resolve("tree1.tiny"), delegate); MappingReader.read(dir.resolve("tree2.tiny"), delegate); - MemoryMappingTree referenceTree = new MemoryMappingTree(); + MemoryMappingTree referenceTree = createTree(); MappingReader.read(dir.resolve("tree1+2.tiny"), referenceTree); tree.accept(new FlatAsRegularMappingVisitor(new SubsetAssertingVisitor(referenceTree, null, null))); referenceTree.accept(new FlatAsRegularMappingVisitor(new SubsetAssertingVisitor(tree, null, null))); + // Same namespaces, different order MappingReader.read(dir.resolve("tree3.tiny"), delegate); - referenceTree = new MemoryMappingTree(); + referenceTree = createTree(); MappingReader.read(dir.resolve("tree1+2+3.tiny"), referenceTree); tree.accept(new FlatAsRegularMappingVisitor(new SubsetAssertingVisitor(referenceTree, null, null))); referenceTree.accept(new FlatAsRegularMappingVisitor(new SubsetAssertingVisitor(tree, null, null))); + + // New namespace as source, some existing ones as destination + MappingReader.read(dir.resolve("tree4.tiny"), delegate); + + referenceTree = createTree(); + MappingReader.read(dir.resolve("tree1+2+3+4.tiny"), referenceTree); + tree.accept(new FlatAsRegularMappingVisitor(new SubsetAssertingVisitor(referenceTree, null, null))); + referenceTree.accept(new FlatAsRegularMappingVisitor(new SubsetAssertingVisitor(tree, null, null))); } } diff --git a/src/test/java/net/fabricmc/mappingio/test/tests/tree/MetadataTest.java b/src/test/java/net/fabricmc/mappingio/test/tests/tree/MetadataTest.java index b652fa46..5c9c490d 100644 --- a/src/test/java/net/fabricmc/mappingio/test/tests/tree/MetadataTest.java +++ b/src/test/java/net/fabricmc/mappingio/test/tests/tree/MetadataTest.java @@ -16,6 +16,7 @@ package net.fabricmc.mappingio.test.tests.tree; +import static net.fabricmc.mappingio.test.TestUtil.createTree; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; @@ -33,7 +34,6 @@ import net.fabricmc.mappingio.MappingFlag; import net.fabricmc.mappingio.test.TestMappings; import net.fabricmc.mappingio.test.visitors.NopMappingVisitor; -import net.fabricmc.mappingio.tree.MemoryMappingTree; import net.fabricmc.mappingio.tree.VisitableMappingTree; public class MetadataTest { @@ -44,7 +44,7 @@ public class MetadataTest { @BeforeAll public static void setup() throws Exception { - tree = TestMappings.generateValid(new MemoryMappingTree()); + tree = TestMappings.generateValid(createTree()); tree.getMetadata().clear(); for (int i = 0; i < 40; i++) { diff --git a/src/test/java/net/fabricmc/mappingio/test/tests/tree/SetNamespacesTest.java b/src/test/java/net/fabricmc/mappingio/test/tests/tree/SetNamespacesTest.java index b7fe17b9..3a323b08 100644 --- a/src/test/java/net/fabricmc/mappingio/test/tests/tree/SetNamespacesTest.java +++ b/src/test/java/net/fabricmc/mappingio/test/tests/tree/SetNamespacesTest.java @@ -16,6 +16,7 @@ package net.fabricmc.mappingio.test.tests.tree; +import static net.fabricmc.mappingio.test.TestUtil.createTree; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertThrows; @@ -36,7 +37,7 @@ public class SetNamespacesTest { @BeforeEach public void setup() throws IOException { - tree = TestMappings.generateValid(new MemoryMappingTree()); + tree = TestMappings.generateValid(createTree()); srcNs = tree.getSrcNamespace(); dstNs0 = tree.getDstNamespaces().get(0); diff --git a/src/test/java/net/fabricmc/mappingio/test/tests/visiting/VisitEndTest.java b/src/test/java/net/fabricmc/mappingio/test/tests/visiting/VisitEndTest.java index 253af226..809ae144 100644 --- a/src/test/java/net/fabricmc/mappingio/test/tests/visiting/VisitEndTest.java +++ b/src/test/java/net/fabricmc/mappingio/test/tests/visiting/VisitEndTest.java @@ -16,6 +16,7 @@ package net.fabricmc.mappingio.test.tests.visiting; +import static net.fabricmc.mappingio.test.TestUtil.createTree; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -57,7 +58,7 @@ private void check(MappingDir dir, MappingFormat format) throws Exception { } MappingTreeView supTree = dir.supportsGeneration() - ? dir.generate(new MemoryMappingTree()) + ? dir.generate(createTree()) : null; checkCompliance(dir, format, 1, true, supTree); @@ -92,7 +93,7 @@ private VisitEndComplianceChecker(int visitPassCountToFinish, boolean setFlag, M this.supTree = supTree; this.subFormat = subFormat; this.dir = dir; - this.tree = new MemoryMappingTree(); + this.tree = createTree(); this.oldTrees = new MappingTree[visitPassCountToFinish - 1]; } @@ -200,8 +201,8 @@ public boolean visitEnd() throws IOException { return true; } - oldTrees[finishedVisitPassCount - 1] = new MemoryMappingTree(tree); - tree = new MemoryMappingTree(); + oldTrees[finishedVisitPassCount - 1] = createTree(tree); + tree = createTree(); return false; } diff --git a/src/test/java/net/fabricmc/mappingio/test/tests/writing/WriteTest.java b/src/test/java/net/fabricmc/mappingio/test/tests/writing/WriteTest.java index 6fd5a540..8f742bd4 100644 --- a/src/test/java/net/fabricmc/mappingio/test/tests/writing/WriteTest.java +++ b/src/test/java/net/fabricmc/mappingio/test/tests/writing/WriteTest.java @@ -16,6 +16,8 @@ package net.fabricmc.mappingio.test.tests.writing; +import static net.fabricmc.mappingio.test.TestUtil.createTree; + import java.io.IOException; import java.nio.file.Path; @@ -37,7 +39,6 @@ import net.fabricmc.mappingio.test.TestUtil; import net.fabricmc.mappingio.test.visitors.SubsetAssertingVisitor; import net.fabricmc.mappingio.tree.MappingTreeView; -import net.fabricmc.mappingio.tree.MemoryMappingTree; import net.fabricmc.mappingio.tree.VisitableMappingTree; public class WriteTest { @@ -59,7 +60,7 @@ private void check(MappingDir dir, MappingFormat format) throws Exception { } Path path = targetDir.resolve(TestUtil.getFileName(format)); - MappingTreeView tree = dir.generate(new MemoryMappingTree()); + MappingTreeView tree = dir.generate(createTree()); MappingVisitor target = MappingWriter.create(path, format); if (dir.isIn(TestMappings.PROPAGATION.BASE_DIR) && !format.features().hasNamespaces()) { @@ -83,7 +84,7 @@ private void check(MappingDir dir, MappingFormat format) throws Exception { } private void readWithMio(MappingTreeView origTree, Path outputPath, MappingFormat outputFormat) throws Exception { - VisitableMappingTree writtenTree = new MemoryMappingTree(); + VisitableMappingTree writtenTree = createTree(); MappingReader.read(outputPath, outputFormat, writtenTree); writtenTree.accept(new FlatAsRegularMappingVisitor(new SubsetAssertingVisitor(origTree, null, outputFormat))); @@ -104,7 +105,7 @@ private void readWithSrgUtils(MappingTreeView tree, MappingFormat format) throws if (format == MappingFormat.PROGUARD_FILE) return; // SrgUtils can't handle empty dst names - VisitableMappingTree dstNsCompTree = new MemoryMappingTree(); + VisitableMappingTree dstNsCompTree = createTree(); tree.accept( // TODO: Remove once https://github.com/neoforged/SRGUtils/issues/9 is fixed new MappingNsCompleter( diff --git a/src/test/resources/merging/tree1+2+3+4.tiny b/src/test/resources/merging/tree1+2+3+4.tiny new file mode 100644 index 00000000..27de3872 --- /dev/null +++ b/src/test/resources/merging/tree1+2+3+4.tiny @@ -0,0 +1,9 @@ +tiny 2 0 nsA nsB nsC nsD +c cls1NsAName cls1NsBName cls1NsCName2 cls1NsDName + f I fld1NsAName fld1NsCName + c fld1 comment + f I fld2NsAName fld2NsBName fld2NsCName fld2NsDName + f I fld3NsAName fld3NsBName fld3NsCName fld3NsDName + m ()I mth1NsAName mth1NsBName2 mth1NsCName mth1NsDName + p 1 arg1NsAName arg1NsBName arg1NsCName + v 2 2 2 var1NsAName var1NsBName var1NsCName diff --git a/src/test/resources/merging/tree1+2+3.tiny b/src/test/resources/merging/tree1+2+3.tiny index 1f346bde..00bcb242 100644 --- a/src/test/resources/merging/tree1+2+3.tiny +++ b/src/test/resources/merging/tree1+2+3.tiny @@ -1,9 +1,9 @@ -tiny 2 0 source target target2 -c class_1 class1Ns0Rename class1Ns1Rename2 - f I field_1 field1Ns1Rename - c field_1 comment - f I field_2 field2Ns0Rename - f I field_3 field3Ns0Rename field3Ns1Rename - m ()I method_1 method1Ns0Rename method1Ns1Rename - p 1 param_1 param1Ns0Rename param1Ns1Rename - v 2 2 2 var_1 var1Ns0Rename var1Ns1Rename +tiny 2 0 nsA nsB nsC +c cls1NsAName cls1NsBName cls1NsCName2 + f I fld1NsAName fld1NsCName + c fld1 comment + f I fld2NsAName fld2NsBName fld2NsCName + f I fld3NsAName fld3NsBName fld3NsCName + m ()I mth1NsAName mth1NsBName mth1NsCName + p 1 arg1NsAName arg1NsBName arg1NsCName + v 2 2 2 var1NsAName var1NsBName var1NsCName diff --git a/src/test/resources/merging/tree1+2.tiny b/src/test/resources/merging/tree1+2.tiny index 0f4b2788..81847fb9 100644 --- a/src/test/resources/merging/tree1+2.tiny +++ b/src/test/resources/merging/tree1+2.tiny @@ -1,7 +1,7 @@ -tiny 2 0 source target target2 -c class_1 class1Ns0Rename class1Ns1Rename - f I field_1 field1Ns1Rename - f I field_2 field2Ns0Rename - m ()I method_1 method1Ns0Rename method1Ns1Rename - p 1 param_1 param1Ns0Rename param1Ns1Rename - v 2 2 2 var_1 var1Ns0Rename var1Ns1Rename +tiny 2 0 nsA nsB nsC +c cls1NsAName cls1NsBName cls1NsCName + f I fld1NsAName fld1NsCName + f I fld2NsAName fld2NsBName + m ()I mth1NsAName mth1NsBName mth1NsCName + p 1 arg1NsAName arg1NsBName arg1NsCName + v 2 2 2 var1NsAName var1NsBName var1NsCName diff --git a/src/test/resources/merging/tree1.tiny b/src/test/resources/merging/tree1.tiny index f90f9c5e..e06c13ea 100644 --- a/src/test/resources/merging/tree1.tiny +++ b/src/test/resources/merging/tree1.tiny @@ -1,6 +1,6 @@ -tiny 2 0 source target target2 -c class_1 class1Ns0Rename - f I field_1 field1Ns1Rename - m ()I method_1 method1Ns1Rename - p 1 param_1 param1Ns1Rename - v 2 2 -1 var_1 var1Ns0Rename +tiny 2 0 nsA nsB nsC +c cls1NsAName cls1NsBName + f I fld1NsAName fld1NsCName + m ()I mth1NsAName mth1NsCName + p 1 arg1NsAName arg1NsCName + v 2 2 -1 var1NsAName var1NsBName diff --git a/src/test/resources/merging/tree2.tiny b/src/test/resources/merging/tree2.tiny index bc96deb3..3bb79dd5 100644 --- a/src/test/resources/merging/tree2.tiny +++ b/src/test/resources/merging/tree2.tiny @@ -1,7 +1,7 @@ -tiny 2 0 source target target2 -c class_1 class1Ns0Rename class1Ns1Rename - f I field_1 field1Ns1Rename - f I field_2 field2Ns0Rename - m ()I method_1 method1Ns0Rename - p 1 param_1 param1Ns0Rename - v 2 2 2 var_1 var1Ns1Rename +tiny 2 0 nsA nsB nsC +c cls1NsAName cls1NsBName cls1NsCName + f I fld1NsAName fld1NsCName + f I fld2NsAName fld2NsBName + m ()I mth1NsAName mth1NsBName + p 1 arg1NsAName arg1NsBName + v 2 2 2 var1NsAName var1NsCName diff --git a/src/test/resources/merging/tree3.tiny b/src/test/resources/merging/tree3.tiny index b8669535..5cb3cb69 100644 --- a/src/test/resources/merging/tree3.tiny +++ b/src/test/resources/merging/tree3.tiny @@ -1,6 +1,6 @@ -tiny 2 0 target2 target source -c class1Ns1Rename2 class_1 - f I field1Ns1Rename field_1 - c field_1 comment - f I field2Ns1Rename field2Ns0Rename - f I field3Ns1Rename field3Ns0Rename field_3 +tiny 2 0 nsC nsB nsA +c cls1NsCName2 cls1NsAName + f I fld1NsCName fld1NsAName + c fld1 comment + f I fld2NsCName fld2NsBName + f I fld3NsCName fld3NsBName fld3NsAName diff --git a/src/test/resources/merging/tree4.tiny b/src/test/resources/merging/tree4.tiny new file mode 100644 index 00000000..6721f6a3 --- /dev/null +++ b/src/test/resources/merging/tree4.tiny @@ -0,0 +1,6 @@ +tiny 2 0 nsD nsC nsB +c cls1NsDName cls1NsBName + f I fld1NsDName fld1NsBName + f I fld2NsDName fld2NsBName + f I fld3NsDName fld3NsCName + m ()I mth1NsDName mth1NsCName mth1NsBName2