diff --git a/CHANGELOG.md b/CHANGELOG.md index bc08c27d..ff8e5db5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,7 +12,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Made some internal methods in Enigma and TSRG readers actually private - Made all writers for formats which can't represent empty destination names skip such elements entirely, unless mapped child elements are present - Added missing `visitElementContent` calls to CSRG and Recaf Simple readers -- Fixed member mapping merging via tree-API in `MemoryMappingTree` +- Added protection to `MemoryMappingTree` guarding against external data modification while a visitation pass is in progress +- Clearly defined Tree-API contracts regarding returned collections' mutability +- Fixed `MemoryMappingTree#reset` to actually reset all its internal state related to the current visitation pass +- Fixed and improved `MemoryMappingTree`'s merging capabilities: + - Fixed broken member mapping merging via tree-API in `MemoryMappingTree` + - Fixed existing entries' data not getting overridden when merging elements into `MemoryMappingTree` via tree-API + - Fixed NPE when visiting with flipped namespaces ([issue 68](https://github.com/FabricMC/mapping-io/issues/68)) + - Made merging with flipped namespaces actually work and handle both names and descriptors + - Fixed potentially incorrect descriptor computation by delaying until all classes are present and merged - Fixed duplicate mapping definitions not being handled correctly in multiple readers - Removed ASM dependency from core project diff --git a/src/main/java/net/fabricmc/mappingio/MappingVisitor.java b/src/main/java/net/fabricmc/mappingio/MappingVisitor.java index 24f78dec..8165071b 100644 --- a/src/main/java/net/fabricmc/mappingio/MappingVisitor.java +++ b/src/main/java/net/fabricmc/mappingio/MappingVisitor.java @@ -47,6 +47,9 @@ * something else after a {@link #reset()}. * *

The same element may be visited more than once unless the flags contain {@link MappingFlag#NEEDS_ELEMENT_UNIQUENESS}. + * + *

If an exception is thrown during visitation, the visitor is to be considered in an invalid state. + * {@link #reset()} must be called clear the internal state before further visitations can happen. */ public interface MappingVisitor { default Set getFlags() { diff --git a/src/main/java/net/fabricmc/mappingio/tree/MappingTree.java b/src/main/java/net/fabricmc/mappingio/tree/MappingTree.java index df8db66d..de5703c8 100644 --- a/src/main/java/net/fabricmc/mappingio/tree/MappingTree.java +++ b/src/main/java/net/fabricmc/mappingio/tree/MappingTree.java @@ -23,10 +23,21 @@ /** * Mutable mapping tree. + * + *

All returned collections are to be assumed unmodifiable, unless explicitly stated otherwise. */ public interface MappingTree extends MappingTreeView { + /** + * Sets the tree's and all of its contained elements' source namespace. + * + * @return The previous source namespace, if present. + */ @Nullable String setSrcNamespace(String namespace); + + /** + * @return The previous destination namespaces. + */ List setDstNamespaces(List namespaces); /** @@ -37,8 +48,7 @@ public interface MappingTree extends MappingTreeView { List getMetadata(); /** - * @return An unmodifiable list of all metadata entries currently present - * in the tree whose key is equal to the passed one. + * @return An unmodifiable list of all currently present metadata entries whose key is equal to the passed one. * The list's order is equal to the order in which the entries have been originally added. */ @Override @@ -65,7 +75,19 @@ default ClassMapping getClass(String name, int namespace) { return (ClassMapping) MappingTreeView.super.getClass(name, namespace); } + /** + * Merges a class mapping into the tree. + * + * @return The {@link ClassMapping} instance present in the tree after the merge has occurred. + * May or may not be the passed instance. + */ ClassMapping addClass(ClassMapping cls); + + /** + * Removes a class mapping from the tree. + * + * @return The removed class mapping, if any. + */ @Nullable ClassMapping removeClass(String srcName); @@ -117,7 +139,19 @@ default FieldMapping getField(String name, @Nullable String desc, int namespace) return (FieldMapping) ClassMappingView.super.getField(name, desc, namespace); } + /** + * Merges a field mapping into the class. + * + * @return The {@link FieldMapping} instance present in the parent {@link ClassMapping} after the merge has occurred. + * May or may not be the passed instance. + */ FieldMapping addField(FieldMapping field); + + /** + * Removes a field mapping from the class. + * + * @return The removed field mapping, if any. + */ @Nullable FieldMapping removeField(String srcName, @Nullable String srcDesc); @@ -133,7 +167,19 @@ default MethodMapping getMethod(String name, @Nullable String desc, int namespac return (MethodMapping) ClassMappingView.super.getMethod(name, desc, namespace); } + /** + * Merges a method mapping into the class. + * + * @return The {@link MethodMapping} instance present in the parent {@link ClassMapping} after the merge has occurred. + * May or may not be the passed instance. + */ MethodMapping addMethod(MethodMapping method); + + /** + * Removes a method mapping from the class. + * + * @return The removed method mapping, if any. + */ @Nullable MethodMapping removeMethod(String srcName, @Nullable String srcDesc); } @@ -153,6 +199,12 @@ interface MethodMapping extends MemberMapping, MethodMappingView { @Nullable MethodArgMapping getArg(int argPosition, int lvIndex, @Nullable String srcName); MethodArgMapping addArg(MethodArgMapping arg); + + /** + * Removes an argument mapping from the method. + * + * @return The removed argument mapping, if any. + */ @Nullable MethodArgMapping removeArg(int argPosition, int lvIndex, @Nullable String srcName); @@ -161,7 +213,20 @@ interface MethodMapping extends MemberMapping, MethodMappingView { @Override @Nullable MethodVarMapping getVar(int lvtRowIndex, int lvIndex, int startOpIdx, int endOpIdx, @Nullable String srcName); + + /** + * Merges a variable mapping into the method. + * + * @return The {@link MethodVarMapping} instance present in the parent {@link MethodMapping} after the merge has occurred. + * May or may not be the passed instance. + */ MethodVarMapping addVar(MethodVarMapping var); + + /** + * Removes a variable mapping from the method. + * + * @return The removed variable mapping, if any. + */ @Nullable MethodVarMapping removeVar(int lvtRowIndex, int lvIndex, int startOpIdx, int endOpIdx, @Nullable String srcName); } diff --git a/src/main/java/net/fabricmc/mappingio/tree/MappingTreeView.java b/src/main/java/net/fabricmc/mappingio/tree/MappingTreeView.java index d6f3eac4..3dacb7b1 100644 --- a/src/main/java/net/fabricmc/mappingio/tree/MappingTreeView.java +++ b/src/main/java/net/fabricmc/mappingio/tree/MappingTreeView.java @@ -26,6 +26,9 @@ /** * Read-only mapping tree. + * + *

All returned collections are to be assumed unmodifiable, + * unless implementation-specific documentation states otherwise. */ public interface MappingTreeView { /** @@ -70,7 +73,16 @@ default String getNamespaceName(int id) { return getDstNamespaces().get(id); } + /** + * @return A list of all metadata entries currently present in the tree. + * The list's order is equal to the order in which the entries have been originally added. + */ List getMetadata(); + + /** + * @return A list of all currently present metadata entries whose key is equal to the passed one. + * The list's order is equal to the order in which the entries have been originally added. + */ List getMetadata(String key); Collection getClasses(); diff --git a/src/main/java/net/fabricmc/mappingio/tree/MemoryMappingTree.java b/src/main/java/net/fabricmc/mappingio/tree/MemoryMappingTree.java index 724aa1d1..c5fa15ec 100644 --- a/src/main/java/net/fabricmc/mappingio/tree/MemoryMappingTree.java +++ b/src/main/java/net/fabricmc/mappingio/tree/MemoryMappingTree.java @@ -69,7 +69,13 @@ public MemoryMappingTree(MappingTree src) { } } + /** + * Whether or not to index classes by their destination names, in addition to their source names. + * + *

Trades higher memory consumption for faster lookups by destination name. + */ public void setIndexByDstNames(boolean indexByDstNames) { + assertNotInVisitPass(); if (indexByDstNames == this.indexByDstNames) return; if (!indexByDstNames) { @@ -115,6 +121,7 @@ public String getSrcNamespace() { @Override @Nullable public String setSrcNamespace(String namespace) { + assertNotInVisitPass(); String ret = srcNamespace; srcNamespace = namespace; @@ -128,6 +135,8 @@ public List getDstNamespaces() { @Override public List setDstNamespaces(List namespaces) { + assertNotInVisitPass(); + if (!classesBySrcName.isEmpty()) { // classes present, update existing dstNames int newSize = namespaces.size(); int[] nameMap = new int[newSize]; @@ -239,7 +248,7 @@ public boolean removeMetadata(String key) { @Override public Collection getClasses() { - return classesBySrcName.values(); + return classesView; } @Override @@ -260,11 +269,12 @@ public ClassMapping getClass(String name, int namespace) { @Override public ClassMapping addClass(ClassMapping cls) { + assertNotInVisitPass(); ClassEntry entry = cls instanceof ClassEntry && cls.getTree() == this ? (ClassEntry) cls : new ClassEntry(this, cls, getSrcNsEquivalent(cls)); ClassEntry ret = classesBySrcName.putIfAbsent(cls.getSrcName(), entry); if (ret != null) { - ret.copyFrom(entry, false); + ret.copyFrom(entry, true); entry = ret; } @@ -288,6 +298,7 @@ private int getSrcNsEquivalent(ElementMapping mapping) { @Override @Nullable public ClassMapping removeClass(String srcName) { + assertNotInVisitPass(); ClassEntry ret = classesBySrcName.remove(srcName); if (ret != null && indexByDstNames) { @@ -343,20 +354,30 @@ public void accept(MappingVisitor visitor, VisitOrder order) throws IOException @Override public void reset() { + inVisitPass = false; + srcNsMap = SRC_NAMESPACE_ID; + dstNameMap = null; currentEntry = null; currentClass = null; currentMethod = null; + pendingClasses = null; + pendingMembers = null; } @Override public void visitNamespaces(String srcNamespace, List dstNamespaces) { + inVisitPass = true; srcNsMap = SRC_NAMESPACE_ID; dstNameMap = new int[dstNamespaces.size()]; if (this.srcNamespace != null) { // ns already set, try to merge if (!srcNamespace.equals(this.srcNamespace)) { srcNsMap = this.dstNamespaces.indexOf(srcNamespace); - if (srcNsMap < 0) throw new UnsupportedOperationException("can't merge with disassociated src namespace"); // srcNamespace must already be present + + if (srcNsMap < 0) { + reset(); + throw new IllegalArgumentException("can't merge with disassociated src namespace"); // srcNamespace must already be present + } } int newDstNamespaces = 0; @@ -365,13 +386,15 @@ public void visitNamespaces(String srcNamespace, List dstNamespaces) { String dstNs = dstNamespaces.get(i); int idx; - if (dstNs.equals(srcNamespace)) { - idx = -1; + if (dstNs.equals(this.srcNamespace)) { + idx = SRC_NAMESPACE_ID; + } else if (dstNs.equals(srcNamespace)) { + reset(); + throw new IllegalArgumentException("namespace \"" + srcNamespace + "\" is present on both source and destination side simultaneously"); } else { idx = this.dstNamespaces.indexOf(dstNs); if (idx < 0) { - if (dstNs.equals(this.srcNamespace)) throw new UnsupportedOperationException("can't merge with existing src namespace in new dst namespaces"); if (newDstNamespaces == 0) this.dstNamespaces = new ArrayList<>(this.dstNamespaces); idx = this.dstNamespaces.size(); @@ -400,7 +423,12 @@ public void visitNamespaces(String srcNamespace, List dstNamespaces) { this.dstNamespaces = dstNamespaces; for (int i = 0; i < dstNameMap.length; i++) { - dstNameMap[i] = dstNamespaces.get(i).equals(srcNamespace) ? -1 : i; + if (dstNamespaces.get(i).equals(srcNamespace)) { + reset(); + throw new IllegalArgumentException("namespace \"" + srcNamespace + "\" is present on both source and destination side simultaneously"); + } + + dstNameMap[i] = i; } if (indexByDstNames) { @@ -412,7 +440,7 @@ public void visitNamespaces(String srcNamespace, List dstNamespaces) { @Override public void visitMetadata(String key, @Nullable String value) { MetadataEntryImpl entry = new MetadataEntryImpl(key, value); - addMetadata(entry); + metadata.add(entry); } @Override @@ -423,8 +451,7 @@ public boolean visitClass(String srcName) { if (cls == null) { if (srcNsMap >= 0) { // tree-side srcName unknown - cls = new ClassEntry(this, null); - cls.setDstName(srcName, srcNsMap); + cls = queuePendingClass(srcName); } else { cls = new ClassEntry(this, srcName); classesBySrcName.put(srcName, cls); @@ -449,10 +476,15 @@ public boolean visitField(String srcName, @Nullable String srcDesc) { field = (FieldEntry) queuePendingMember(srcName, srcDesc, true); } else { field = new FieldEntry(currentClass, srcName, srcDesc); - field = currentClass.addField(field); + field = currentClass.addFieldInternal(field); } } else if (srcDesc != null && field.srcDesc == null) { - field.setSrcDesc(mapDesc(srcDesc, srcNsMap, SRC_NAMESPACE_ID)); // assumes the class mapping is already sufficiently present.. + if (srcNsMap >= 0) { + // delay descriptor computation until all classes have been supplied + queuePendingMember(srcName, srcDesc, true).setSrcName(field.getSrcName()); + } else { + field.setSrcDescInternal(srcDesc); + } } currentEntry = field; @@ -471,10 +503,15 @@ public boolean visitMethod(String srcName, @Nullable String srcDesc) { method = (MethodEntry) queuePendingMember(srcName, srcDesc, false); } else { method = new MethodEntry(currentClass, srcName, srcDesc); - method = currentClass.addMethod(method); + method = currentClass.addMethodInternal(method); + } + } else if (isValidDescriptor(srcDesc, true) && !isValidDescriptor(method.srcDesc, true)) { + if (srcNsMap >= 0) { + // delay descriptor computation until all classes have been supplied + queuePendingMember(srcName, srcDesc, false).setSrcName(method.getSrcName()); + } else { + method.setSrcDescInternal(srcDesc); } - } else if (srcDesc != null && (method.srcDesc == null || method.srcDesc.endsWith(")") && !srcDesc.endsWith(")"))) { - method.setSrcDesc(mapDesc(srcDesc, srcNsMap, SRC_NAMESPACE_ID)); // assumes the class mapping is already sufficiently present.. } currentEntry = currentMethod = method; @@ -482,6 +519,21 @@ public boolean visitMethod(String srcName, @Nullable String srcDesc) { return true; } + private ClassEntry queuePendingClass(String name) { + if (pendingClasses == null) pendingClasses = new HashMap<>(); + ClassEntry cls = pendingClasses.get(name); + + if (cls == null) { + cls = new ClassEntry(this, null); + 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<>(); GlobalMemberKey key = new GlobalMemberKey(currentClass, name, desc, isField); @@ -489,7 +541,7 @@ private MemberEntry queuePendingMember(String name, @Nullable String desc, bo if (member == null) { if (isField) { - member = new FieldEntry(currentClass, null, desc); + member = new FieldEntry(currentClass, null, desc); // we're misusing the srcDesc field to store the dstDesc (as there is no dstDesc field) } else { member = new MethodEntry(currentClass, null, desc); } @@ -497,39 +549,63 @@ private MemberEntry queuePendingMember(String name, @Nullable String desc, bo pendingMembers.put(key, member); } - if (srcNsMap >= 0) { - member.setDstName(name, srcNsMap); - } + assert srcNsMap >= 0; + member.setDstNameInternal(name, srcNsMap); return member; } - private void addPendingMember(MemberEntry member) { - String name = member.getName(srcNsMap); + private void addPendingClass(ClassEntry cls) { + if (cls.isSrcNameMissing()) { + return; + } + + String srcName = cls.getSrcName(); + ClassEntry existing = classesBySrcName.get(srcName); + + if (existing == null) { + classesBySrcName.put(srcName, cls); + } else { // copy remaining data + existing.copyFrom(cls, true); + } + } - if (name == null) { + private void addPendingMember(MemberEntry member) { + if (member.isSrcNameMissing() || member.getOwner().isSrcNameMissing()) { return; } - String desc = member.getDesc(srcNsMap); + // 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 + String srcDesc = null; + + if (isValidDescriptor(dstDesc, !isField)) { + srcDesc = mapDesc(dstDesc, srcNsMap, SRC_NAMESPACE_ID); + } + + member.setSrcDescInternal(srcDesc); - if (member.getKind() == MappedElementKind.FIELD) { - FieldEntry field = member.getOwner().getField(name, desc); + if (isField) { + FieldEntry queuedField = (FieldEntry) member; + FieldEntry existingField = owner.getField(srcName, srcDesc); - if (field == null) { - member.srcName = name; - member.setSrcDesc(desc); + if (existingField == null) { + owner.addFieldInternal(queuedField); } else { // copy remaining data - field.copyFrom((FieldEntry) member, false); + existingField.copyFrom(queuedField, true); } } else { - MethodEntry method = member.getOwner().getMethod(name, desc); + MethodEntry queuedMethod = (MethodEntry) member; + MethodEntry existingMethod = owner.getMethod(srcName, srcDesc); - if (method == null) { - member.srcName = name; - member.setSrcDesc(desc); + if (existingMethod == null) { + owner.addMethodInternal(queuedMethod); } else { // copy remaining data - method.copyFrom((MethodEntry) member, false); + existingMethod.copyFrom(queuedMethod, true); } } } @@ -542,10 +618,10 @@ public boolean visitMethodArg(int argPosition, int lvIndex, @Nullable String src if (arg == null) { arg = new MethodArgEntry(currentMethod, argPosition, lvIndex, srcName); - arg = currentMethod.addArg(arg); + arg = currentMethod.addArgInternal(arg); } else { - if (argPosition >= 0 && arg.argPosition < 0) arg.setArgPosition(argPosition); - if (lvIndex >= 0 && arg.lvIndex < 0) arg.setLvIndex(lvIndex); + if (argPosition >= 0 && arg.argPosition < 0) arg.setArgPositionInternal(argPosition); + if (lvIndex >= 0 && arg.lvIndex < 0) arg.setLvIndexInternal(lvIndex); if (srcName != null) { assert !srcName.isEmpty(); @@ -566,10 +642,10 @@ public boolean visitMethodVar(int lvtRowIndex, int lvIndex, int startOpIdx, int if (var == null) { var = new MethodVarEntry(currentMethod, lvtRowIndex, lvIndex, startOpIdx, endOpIdx, srcName); - var = currentMethod.addVar(var); + var = currentMethod.addVarInternal(var); } else { - if (lvtRowIndex >= 0 && var.lvtRowIndex < 0) var.setLvtRowIndex(lvtRowIndex); - if (lvIndex >= 0 && startOpIdx >= 0 && (var.lvIndex < 0 || var.startOpIdx < 0)) var.setLvIndex(lvIndex, startOpIdx, endOpIdx); + if (lvtRowIndex >= 0 && var.lvtRowIndex < 0) var.setLvtRowIndexInternal(lvtRowIndex); + if (lvIndex >= 0 && startOpIdx >= 0 && (var.lvIndex < 0 || var.startOpIdx < 0)) var.setLvIndexInternal(lvIndex, startOpIdx, endOpIdx); if (srcName != null) { assert !srcName.isEmpty(); @@ -584,9 +660,14 @@ public boolean visitMethodVar(int lvtRowIndex, int lvIndex, int startOpIdx, int @Override public boolean visitEnd() { - currentEntry = null; - currentClass = null; - currentMethod = null; + // TODO: Don't discard pending elements which are still missing their tree-side src names, upcoming visit passes might provide them + if (pendingClasses != null) { + for (ClassEntry cls : pendingClasses.values()) { + addPendingClass(cls); + } + + pendingClasses = null; + } if (pendingMembers != null) { for (MemberEntry member : pendingMembers.values()) { @@ -596,6 +677,8 @@ public boolean visitEnd() { pendingMembers = null; } + reset(); + if (hierarchyInfo != null) { propagateNames(hierarchyInfo); } @@ -659,38 +742,33 @@ public void visitDstName(MappedElementKind targetKind, int namespace, String nam if (currentEntry == null) throw new UnsupportedOperationException("Tried to visit mapped name before owner"); if (namespace < 0) { - if (name.equals(currentEntry.getSrcName())) return; + if (name.equals(currentEntry.getSrcNameUnchecked())) return; switch (currentEntry.getKind()) { case CLASS: assert currentClass == currentEntry; - - if (currentClass.srcName == null) { - currentClass.srcName = name; - } else { - throw new UnsupportedOperationException("can't change src name for "+currentEntry.getKind()); + case FIELD: + case METHOD: + if (currentEntry.isSrcNameMissing()) { + currentEntry.setSrcName(name); + return; } break; case METHOD_ARG: ((MethodArgEntry) currentEntry).setSrcName(name); - break; + return; case METHOD_VAR: ((MethodVarEntry) currentEntry).setSrcName(name); - break; - default: - throw new UnsupportedOperationException("can't change src name for "+currentEntry.getKind()); + return; } + + throw new UnsupportedOperationException("can't change src name for "+currentEntry.getKind()); } else { - currentEntry.setDstName(name, namespace); + currentEntry.setDstNameInternal(name, namespace); } } - @Override - public boolean visitElementContent(MappedElementKind targetKind) throws IOException { - return targetKind != MappedElementKind.CLASS || currentClass.getSrcName() != null; // reject classes that never received a src name - } - @Override public void visitComment(MappedElementKind targetKind, String comment) { Entry entry; @@ -707,7 +785,25 @@ public void visitComment(MappedElementKind targetKind, String comment) { } if (entry == null) throw new UnsupportedOperationException("Tried to visit comment before owning target"); - entry.setComment(comment); + entry.setCommentInternal(comment); + } + + private static boolean isValidDescriptor(String descriptor, boolean possiblyMethod) { + if (descriptor == null) { + return false; + } + + if (possiblyMethod && descriptor.endsWith(")")) { + return false; // Parameter-only descriptor (Proguard?) + } + + return true; + } + + void assertNotInVisitPass() { + if (inVisitPass) { + throw new UnsupportedOperationException("Attempted illegal tree interaction via tree-API during an ongoing visitation pass"); + } } abstract static class Entry> implements ElementMapping { @@ -724,20 +820,46 @@ protected Entry(MemoryMappingTree tree, ElementMapping src, int srcNsEquivalent) int dstNsEquivalent = src.getTree().getNamespaceId(tree.dstNamespaces.get(i)); if (dstNsEquivalent != NULL_NAMESPACE_ID) { - setDstName(src.getDstName(dstNsEquivalent), i); + setDstNameInternal(src.getDstName(dstNsEquivalent), i); } } - setComment(src.getComment()); + setCommentInternal(src.getComment()); } public abstract MappedElementKind getKind(); + final boolean isSrcNameMissing() { + return srcName == null; + } + + String getSrcNameUnchecked() { + return srcName; + } + @Override public final String getSrcName() { + if (!missingSrcNameAllowed) { + assertSrcNamePresent(); + } + return srcName; } + protected final void assertSrcNamePresent() { + if (isSrcNameMissing()) { + throw new UnsupportedOperationException("Attempted illegal interaction with a pending entry still missing its tree-side source name"); + } + } + + void setSrcName(String name) { + if (!missingSrcNameAllowed && name == null) { + throw new UnsupportedOperationException("Source name cannot be null"); + } + + srcName = name; + } + @Override @Nullable public final String getDstName(int namespace) { @@ -745,7 +867,12 @@ public final String getDstName(int namespace) { } @Override - public void setDstName(String name, int namespace) { + public final void setDstName(String name, int namespace) { + tree.assertNotInVisitPass(); + setDstNameInternal(name, namespace); + } + + void setDstNameInternal(String name, int namespace) { dstNames[namespace] = name; } @@ -775,6 +902,11 @@ public final String getComment() { @Override public final void setComment(String comment) { + tree.assertNotInVisitPass(); + setCommentInternal(comment); + } + + void setCommentInternal(String comment) { this.comment = comment; } @@ -814,12 +946,11 @@ protected void copyFrom(T o, boolean replace) { if (o.comment != null && (replace || comment == null)) { comment = o.comment; } - - // TODO: copy args+vars } + private final boolean missingSrcNameAllowed = getKind().level > MappedElementKind.METHOD.level; // args and vars protected final MemoryMappingTree tree; - protected String srcName; + private String srcName; protected String[] dstNames; protected String comment; } @@ -833,11 +964,11 @@ static final class ClassEntry extends Entry implements ClassMapping super(tree, src, srcNsEquivalent); for (FieldMapping field : src.getFields()) { - addField(field); + addFieldInternal(field); } for (MethodMapping method : src.getMethods()) { - addMethod(method); + addMethodInternal(method); } } @@ -852,7 +983,7 @@ public MemoryMappingTree getTree() { } @Override - public void setDstName(String name, int namespace) { + void setDstNameInternal(String name, int namespace) { if (tree.indexByDstNames) { String oldName = dstNames[namespace]; @@ -868,14 +999,14 @@ public void setDstName(String name, int namespace) { } } - super.setDstName(name, namespace); + super.setDstNameInternal(name, namespace); } @Override public Collection getFields() { if (fields == null) return Collections.emptyList(); - return fields.values(); + return fieldsView; } @Override @@ -892,9 +1023,17 @@ public FieldEntry getField(String name, @Nullable String desc, int namespace) { @Override public FieldEntry addField(FieldMapping field) { + tree.assertNotInVisitPass(); + return addFieldInternal(field); + } + + FieldEntry addFieldInternal(FieldMapping field) { FieldEntry entry = field instanceof FieldEntry && field.getOwner() == this ? (FieldEntry) field : new FieldEntry(this, field, tree.getSrcNsEquivalent(field)); - if (fields == null) fields = new LinkedHashMap<>(); + if (fields == null) { + fields = new LinkedHashMap<>(); + fieldsView = Collections.unmodifiableCollection(fields.values()); + } return addMember(entry, fields, FLAG_HAS_ANY_FIELD_DESC, FLAG_MISSES_ANY_FIELD_DESC); } @@ -902,8 +1041,10 @@ public FieldEntry addField(FieldMapping field) { @Override @Nullable public FieldEntry removeField(String srcName, @Nullable String srcDesc) { + tree.assertNotInVisitPass(); + FieldEntry ret = getField(srcName, srcDesc); - if (ret != null) fields.remove(ret.key); + if (ret != null) fields.remove(ret.getKey()); return ret; } @@ -912,7 +1053,7 @@ public FieldEntry removeField(String srcName, @Nullable String srcDesc) { public Collection getMethods() { if (methods == null) return Collections.emptyList(); - return methods.values(); + return methodsView; } @Override @@ -929,9 +1070,17 @@ public MethodEntry getMethod(String name, @Nullable String desc, int namespace) @Override public MethodEntry addMethod(MethodMapping method) { + tree.assertNotInVisitPass(); + return addMethodInternal(method); + } + + MethodEntry addMethodInternal(MethodMapping method) { MethodEntry entry = method instanceof MethodEntry && method.getOwner() == this ? (MethodEntry) method : new MethodEntry(this, method, tree.getSrcNsEquivalent(method)); - if (methods == null) methods = new LinkedHashMap<>(); + if (methods == null) { + methods = new LinkedHashMap<>(); + methodsView = Collections.unmodifiableCollection(methods.values()); + } return addMember(entry, methods, FLAG_HAS_ANY_METHOD_DESC, FLAG_MISSES_ANY_METHOD_DESC); } @@ -939,8 +1088,10 @@ public MethodEntry addMethod(MethodMapping method) { @Override @Nullable public MethodEntry removeMethod(String srcName, @Nullable String srcDesc) { + tree.assertNotInVisitPass(); + MethodEntry ret = getMethod(srcName, srcDesc); - if (ret != null) methods.remove(ret.key); + if (ret != null) methods.remove(ret.getKey()); return ret; } @@ -960,7 +1111,7 @@ private static > T getMember(String srcName, @Nullable if (hasAnyDesc) { // may have name match [no desc] -> [full desc/partial desc] for (T entry : map.values()) { - if (entry.srcName.equals(srcName)) return entry; + if (entry.getSrcName().equals(srcName)) return entry; } } } else if (srcDesc.endsWith(")")) { // parameter-only desc @@ -974,7 +1125,7 @@ private static > T getMember(String srcName, @Nullable if (hasAnyDesc) { // may have partial-desc match [partial desc] -> [full desc] for (T entry : map.values()) { - if (entry.srcName.equals(srcName) + if (entry.getSrcName().equals(srcName) && entry.srcDesc.startsWith(srcDesc)) { return entry; } @@ -992,7 +1143,7 @@ private static > T getMember(String srcName, @Nullable if (srcDesc.indexOf(')') >= 0) { for (T entry : map.values()) { - if (entry.srcName.equals(srcName) + if (entry.getSrcName().equals(srcName) && srcDesc.startsWith(entry.srcDesc)) { // entry.srcDesc can't be null here return entry; } @@ -1005,23 +1156,23 @@ private static > T getMember(String srcName, @Nullable } private > T addMember(T entry, Map map, int flagHasAny, int flagMissesAny) { - T ret = map.putIfAbsent(entry.key, entry); + T ret = map.putIfAbsent(entry.getKey(), entry); if (ret != null) { // same desc - ret.copyFrom(entry, false); + ret.copyFrom(entry, true); return ret; - } else if (entry.srcDesc != null && !entry.srcDesc.endsWith(")")) { // may have replaced desc-less + } else if (isValidDescriptor(entry.srcDesc, true)) { // may have replaced desc-less flags |= flagHasAny; if ((flags & flagMissesAny) != 0) { - ret = map.remove(new MemberKey(entry.srcName, null)); + ret = map.remove(new MemberKey(entry.getSrcName(), null)); if (ret != null) { // compatible entry exists, copy desc + extra content - ret.key = entry.key; + ret.setKey(entry.getKey()); ret.srcDesc = entry.srcDesc; - map.put(ret.key, ret); - ret.copyFrom(entry, false); + map.put(ret.getKey(), ret); + ret.copyFrom(entry, true); entry = ret; } } @@ -1030,9 +1181,9 @@ private > T addMember(T entry, Map map, i } else { // entry.srcDesc == null, may have replaced desc-containing if ((flags & flagHasAny) != 0) { for (T prevEntry : map.values()) { - if (prevEntry != entry && prevEntry.srcName.equals(entry.srcName) && (entry.srcDesc == null || prevEntry.srcDesc.startsWith(entry.srcDesc))) { - map.remove(entry.key); - prevEntry.copyFrom(entry, false); + if (prevEntry != entry && prevEntry.getSrcName().equals(entry.getSrcName()) && (entry.srcDesc == null || prevEntry.srcDesc.startsWith(entry.srcDesc))) { + map.remove(entry.getKey()); + prevEntry.copyFrom(entry, true); return prevEntry; } @@ -1046,7 +1197,7 @@ private > T addMember(T entry, Map map, i } void accept(MappingVisitor visitor, VisitOrder order, boolean supplyFieldDstDescs, boolean supplyMethodDstDescs) throws IOException { - if (visitor.visitClass(srcName) && acceptElement(visitor, null)) { + if (visitor.visitClass(getSrcName()) && acceptElement(visitor, null)) { boolean methodsFirst = order.isMethodsFirst() && fields != null && methods != null; if (!methodsFirst && fields != null) { @@ -1075,16 +1226,16 @@ protected void copyFrom(ClassEntry o, boolean replace) { if (o.fields != null) { for (FieldEntry oField : o.fields.values()) { - FieldEntry field = getField(oField.srcName, oField.srcDesc); + FieldEntry field = getField(oField.getSrcName(), oField.srcDesc); if (field == null) { // missing - addField(oField); + addFieldInternal(oField); } else { if (oField.srcDesc != null && field.srcDesc == null) { // extra location info - fields.remove(field.key); - field.key = oField.key; + fields.remove(field.getKey()); + field.setKey(oField.getKey()); field.srcDesc = oField.srcDesc; - fields.put(field.key, field); + fields.put(field.getKey(), field); flags |= FLAG_HAS_ANY_FIELD_DESC; } @@ -1096,16 +1247,16 @@ protected void copyFrom(ClassEntry o, boolean replace) { if (o.methods != null) { for (MethodEntry oMethod : o.methods.values()) { - MethodEntry method = getMethod(oMethod.srcName, oMethod.srcDesc); + MethodEntry method = getMethod(oMethod.getSrcName(), oMethod.srcDesc); if (method == null) { // missing - addMethod(oMethod); + addMethodInternal(oMethod); } else { if (oMethod.srcDesc != null && method.srcDesc == null) { // extra location info - methods.remove(method.key); - method.key = oMethod.key; + methods.remove(method.getKey()); + method.setKey(oMethod.getKey()); method.srcDesc = oMethod.srcDesc; - methods.put(method.key, method); + methods.put(method.getKey(), method); flags |= FLAG_HAS_ANY_METHOD_DESC; } @@ -1118,7 +1269,7 @@ protected void copyFrom(ClassEntry o, boolean replace) { @Override public String toString() { - return srcName; + return getSrcNameUnchecked(); } private static final byte FLAG_HAS_ANY_FIELD_DESC = 1; @@ -1128,6 +1279,8 @@ public String toString() { private Map fields = null; private Map methods = null; + private Collection fieldsView = null; + private Collection methodsView = null; private byte flags; } @@ -1145,7 +1298,7 @@ protected MemberEntry(ClassEntry owner, MemberMapping src, int srcNsEquivalent) this.owner = owner; this.srcDesc = src.getDesc(srcNsEquivalent); - this.key = new MemberKey(srcName, srcDesc); + this.key = new MemberKey(getSrcName(), srcDesc); } @Override @@ -1158,12 +1311,36 @@ public final ClassEntry getOwner() { return owner; } + void setOwner(ClassEntry owner) { + assert tree.inVisitPass; + assert owner.getSrcName().equals(this.owner.getSrcName()); + this.owner = owner; + } + + @Override + void setSrcName(String name) { + assert tree.inVisitPass; + super.setSrcName(name); + key = new MemberKey(name, srcDesc); + } + @Override @Nullable public final String getSrcDesc() { return srcDesc; } + abstract void setSrcDescInternal(@Nullable String desc); + + MemberKey getKey() { + assertSrcNamePresent(); + return key; + } + + void setKey(MemberKey key) { + this.key = key; + } + protected final boolean acceptMember(MappingVisitor visitor, boolean supplyDstDescs) throws IOException { String[] dstDescs; @@ -1181,9 +1358,9 @@ protected final boolean acceptMember(MappingVisitor visitor, boolean supplyDstDe return acceptElement(visitor, dstDescs); } - protected final ClassEntry owner; + protected ClassEntry owner; protected String srcDesc; - MemberKey key; + private MemberKey key; } static final class FieldEntry extends MemberEntry implements FieldMapping { @@ -1202,15 +1379,27 @@ public MappedElementKind getKind() { @Override public void setSrcDesc(@Nullable String desc) { + tree.assertNotInVisitPass(); + setSrcDescInternal(desc); + } + + @Override + void setSrcDescInternal(@Nullable String desc) { if (Objects.equals(desc, srcDesc)) return; - MemberKey newKey = new MemberKey(srcName, desc); - if (owner.fields.containsKey(newKey)) throw new IllegalArgumentException("conflicting name+desc after changing desc to "+desc+" for "+this); + 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); + owner.fields.remove(getKey()); + } - owner.fields.remove(key); srcDesc = desc; - key = newKey; - owner.fields.put(newKey, this); + setKey(newKey); + + if (owner.fields != null) { + owner.fields.put(newKey, this); + } if (desc != null) { owner.flags |= ClassEntry.FLAG_HAS_ANY_FIELD_DESC; @@ -1220,14 +1409,14 @@ public void setSrcDesc(@Nullable String desc) { } void accept(MappingVisitor visitor, boolean supplyDstDescs) throws IOException { - if (visitor.visitField(srcName, srcDesc)) { + if (visitor.visitField(getSrcName(), srcDesc)) { acceptMember(visitor, supplyDstDescs); } } @Override public String toString() { - return String.format("%s;;%s", srcName, srcDesc); + return String.format("%s;;%s", getSrcNameUnchecked(), srcDesc); } } @@ -1240,11 +1429,11 @@ static final class MethodEntry extends MemberEntry implements Metho super(owner, src, srcNsEquivalent); for (MethodArgMapping arg : src.getArgs()) { - addArg(arg); + addArgInternal(arg); } for (MethodVarMapping var : src.getVars()) { - addVar(var); + addVarInternal(var); } } @@ -1255,17 +1444,29 @@ public MappedElementKind getKind() { @Override public void setSrcDesc(@Nullable String desc) { + tree.assertNotInVisitPass(); + setSrcDescInternal(desc); + } + + @Override + void setSrcDescInternal(@Nullable String desc) { if (Objects.equals(desc, srcDesc)) return; - MemberKey newKey = new MemberKey(srcName, desc); - if (owner.methods.containsKey(newKey)) throw new IllegalArgumentException("conflicting name+desc after changing desc to "+desc+" for "+this); + MemberKey newKey = new MemberKey(getSrcName(), desc); + + if (owner.methods != null) { // pending member + if (owner.methods.containsKey(newKey)) throw new IllegalArgumentException("conflicting name+desc after changing desc to "+desc+" for "+this); + owner.methods.remove(getKey()); + } - owner.methods.remove(key); srcDesc = desc; - key = newKey; - owner.methods.put(newKey, this); + setKey(newKey); + + if (owner.methods != null) { + owner.methods.put(newKey, this); + } - if (desc != null && !desc.endsWith(")")) { + if (isValidDescriptor(desc, true)) { owner.flags |= ClassEntry.FLAG_HAS_ANY_METHOD_DESC; } else { owner.flags |= ClassEntry.FLAG_MISSES_ANY_METHOD_DESC; @@ -1276,7 +1477,7 @@ public void setSrcDesc(@Nullable String desc) { public Collection getArgs() { if (args == null) return Collections.emptyList(); - return args; + return argsView; } @Override @@ -1288,7 +1489,7 @@ public MethodArgEntry getArg(int argPosition, int lvIndex, @Nullable String srcN for (MethodArgEntry entry : args) { if (argPosition >= 0 && entry.argPosition == argPosition || lvIndex >= 0 && entry.lvIndex == lvIndex) { - if (srcName != null && entry.srcName != null && !srcName.equals(entry.srcName)) continue; // both srcNames are present but not equal + if (srcName != null && entry.getSrcName() != null && !srcName.equals(entry.getSrcName())) continue; // both srcNames are present but not equal return entry; } } @@ -1296,7 +1497,7 @@ public MethodArgEntry getArg(int argPosition, int lvIndex, @Nullable String srcN if (srcName != null) { for (MethodArgEntry entry : args) { - if (srcName.equals(entry.srcName) + if (srcName.equals(entry.getSrcName()) && (argPosition < 0 || entry.argPosition < 0) && (lvIndex < 0 || entry.lvIndex < 0)) { return entry; @@ -1309,29 +1510,33 @@ public MethodArgEntry getArg(int argPosition, int lvIndex, @Nullable String srcN @Override public MethodArgEntry addArg(MethodArgMapping arg) { + tree.assertNotInVisitPass(); + return addArgInternal(arg); + } + + MethodArgEntry addArgInternal(MethodArgMapping arg) { MethodArgEntry entry = arg instanceof MethodArgEntry && arg.getMethod() == this ? (MethodArgEntry) arg : new MethodArgEntry(this, arg, owner.tree.getSrcNsEquivalent(arg)); MethodArgEntry prev = getArg(arg.getArgPosition(), arg.getLvIndex(), arg.getSrcName()); if (prev == null) { - if (args == null) args = new ArrayList<>(); + if (args == null) { + args = new ArrayList<>(); + argsView = Collections.unmodifiableList(args); + } + args.add(entry); } else { - updateArg(prev, entry, false); + prev.copyFrom(entry, true); } return entry; } - private void updateArg(MethodArgEntry existing, MethodArgEntry toAdd, boolean replace) { - if (toAdd.argPosition >= 0 && existing.argPosition < 0) existing.setArgPosition(toAdd.argPosition); - if (toAdd.lvIndex >= 0 && existing.lvIndex < 0) existing.setLvIndex(toAdd.getLvIndex()); - - existing.copyFrom(toAdd, replace); - } - @Override @Nullable public MethodArgEntry removeArg(int argPosition, int lvIndex, @Nullable String srcName) { + tree.assertNotInVisitPass(); + MethodArgEntry ret = getArg(argPosition, lvIndex, srcName); if (ret != null) args.remove(ret); @@ -1342,7 +1547,7 @@ public MethodArgEntry removeArg(int argPosition, int lvIndex, @Nullable String s public Collection getVars() { if (vars == null) return Collections.emptyList(); - return vars; + return varsView; } @Override @@ -1371,7 +1576,7 @@ public MethodVarEntry getVar(int lvtRowIndex, int lvIndex, int startOpIdx, int e for (MethodVarEntry entry : vars) { // skip otherwise mismatched candidates if (lvtRowIndex >= 0 && entry.lvtRowIndex >= 0 && lvtRowIndex != entry.lvtRowIndex // different lvtRowIndex - || srcName != null && entry.srcName != null && !srcName.equals(entry.srcName)) { // different srcName + || srcName != null && entry.getSrcName() != null && !srcName.equals(entry.getSrcName())) { // different srcName continue; } @@ -1409,7 +1614,7 @@ public MethodVarEntry getVar(int lvtRowIndex, int lvIndex, int startOpIdx, int e if (srcName != null) { for (MethodVarEntry entry : vars) { - if (srcName.equals(entry.srcName) + if (srcName.equals(entry.getSrcName()) && (lvtRowIndex < 0 || entry.lvtRowIndex < 0) && (lvIndex < 0 || entry.lvIndex < 0)) { return entry; @@ -1422,32 +1627,33 @@ public MethodVarEntry getVar(int lvtRowIndex, int lvIndex, int startOpIdx, int e @Override public MethodVarEntry addVar(MethodVarMapping var) { + tree.assertNotInVisitPass(); + return addVarInternal(var); + } + + MethodVarEntry addVarInternal(MethodVarMapping var) { MethodVarEntry entry = var instanceof MethodVarEntry && var.getMethod() == this ? (MethodVarEntry) var : new MethodVarEntry(this, var, owner.tree.getSrcNsEquivalent(var)); MethodVarEntry prev = getVar(var.getLvtRowIndex(), var.getLvIndex(), var.getStartOpIdx(), var.getEndOpIdx(), var.getSrcName()); if (prev == null) { - if (vars == null) vars = new ArrayList<>(); + if (vars == null) { + vars = new ArrayList<>(); + varsView = Collections.unmodifiableList(vars); + } + vars.add(entry); } else { - updateVar(prev, entry, false); + prev.copyFrom(entry, true); } return entry; } - private void updateVar(MethodVarEntry existing, MethodVarEntry toAdd, boolean replace) { - if (toAdd.lvtRowIndex >= 0 && existing.lvtRowIndex < 0) existing.setLvtRowIndex(toAdd.lvtRowIndex); - - if (toAdd.lvIndex >= 0 && toAdd.startOpIdx >= 0 && (existing.lvIndex < 0 || existing.startOpIdx < 0)) { - existing.setLvIndex(toAdd.lvIndex, toAdd.startOpIdx, toAdd.endOpIdx); - } - - existing.copyFrom(toAdd, replace); - } - @Override @Nullable public MethodVarEntry removeVar(int lvtRowIndex, int lvIndex, int startOpIdx, int endOpIdx, @Nullable String srcName) { + tree.assertNotInVisitPass(); + MethodVarEntry ret = getVar(lvtRowIndex, lvIndex, startOpIdx, endOpIdx, srcName); if (ret != null) vars.remove(ret); @@ -1455,7 +1661,7 @@ public MethodVarEntry removeVar(int lvtRowIndex, int lvIndex, int startOpIdx, in } void accept(MappingVisitor visitor, VisitOrder order, boolean supplyDstDescs) throws IOException { - if (visitor.visitMethod(srcName, srcDesc) && acceptMember(visitor, supplyDstDescs)) { + if (visitor.visitMethod(getSrcName(), srcDesc) && acceptMember(visitor, supplyDstDescs)) { boolean varsFirst = order.isMethodVarsFirst() && args != null && vars != null; if (!varsFirst && args != null) { @@ -1484,24 +1690,24 @@ protected void copyFrom(MethodEntry o, boolean replace) { if (o.args != null) { for (MethodArgEntry oArg : o.args) { - MethodArgEntry arg = getArg(oArg.argPosition, oArg.lvIndex, oArg.srcName); + MethodArgEntry arg = getArg(oArg.argPosition, oArg.lvIndex, oArg.getSrcName()); if (arg == null) { // missing - addArg(oArg); + addArgInternal(oArg); } else { - updateArg(arg, oArg, replace); + arg.copyFrom(oArg, replace); } } } if (o.vars != null) { for (MethodVarEntry oVar : o.vars) { - MethodVarEntry var = getVar(oVar.lvtRowIndex, oVar.lvIndex, oVar.startOpIdx, oVar.endOpIdx, oVar.srcName); + MethodVarEntry var = getVar(oVar.lvtRowIndex, oVar.lvIndex, oVar.startOpIdx, oVar.endOpIdx, oVar.getSrcName()); if (var == null) { // missing - addVar(oVar); + addVarInternal(oVar); } else { - updateVar(var, oVar, replace); + var.copyFrom(oVar, replace); } } } @@ -1509,11 +1715,13 @@ protected void copyFrom(MethodEntry o, boolean replace) { @Override public String toString() { - return String.format("%s%s", srcName, srcDesc); + return String.format("%s%s", getSrcNameUnchecked(), srcDesc); } private List args = null; private List vars = null; + private List argsView = null; + private List varsView = null; } static final class MethodArgEntry extends Entry implements MethodArgMapping { @@ -1555,6 +1763,11 @@ public int getArgPosition() { @Override public void setArgPosition(int position) { + tree.assertNotInVisitPass(); + setArgPositionInternal(position); + } + + void setArgPositionInternal(int position) { this.argPosition = position; } @@ -1565,31 +1778,40 @@ public int getLvIndex() { @Override public void setLvIndex(int index) { - this.lvIndex = index; + tree.assertNotInVisitPass(); + setLvIndexInternal(index); } - public void setSrcName(@Nullable String name) { - this.srcName = name; + void setLvIndexInternal(int index) { + this.lvIndex = index; } void accept(MappingVisitor visitor) throws IOException { - if (visitor.visitMethodArg(argPosition, lvIndex, srcName)) { + if (visitor.visitMethodArg(argPosition, lvIndex, getSrcName())) { acceptElement(visitor, null); } } @Override protected void copyFrom(MethodArgEntry o, boolean replace) { - super.copyFrom(o, replace); + if (o.argPosition >= 0 && (replace || argPosition < 0)) { + setArgPositionInternal(o.argPosition); + } + + if (o.lvIndex >= 0 && (replace || lvIndex < 0)) { + setLvIndexInternal(o.getLvIndex()); + } - if (o.srcName != null && (replace || srcName == null)) { - srcName = o.srcName; + if (o.getSrcName() != null && (replace || getSrcName() == null)) { + setSrcName(o.getSrcName()); } + + super.copyFrom(o, replace); } @Override public String toString() { - return String.format("%d/%d:%s", argPosition, lvIndex, srcName); + return String.format("%d/%d:%s", argPosition, lvIndex, getSrcName()); } private final MethodEntry method; @@ -1640,6 +1862,11 @@ public int getLvtRowIndex() { @Override public void setLvtRowIndex(int index) { + tree.assertNotInVisitPass(); + setLvtRowIndexInternal(index); + } + + void setLvtRowIndexInternal(int index) { this.lvtRowIndex = index; } @@ -1660,33 +1887,42 @@ public int getEndOpIdx() { @Override public void setLvIndex(int lvIndex, int startOpIdx, int endOpIdx) { + tree.assertNotInVisitPass(); + setLvIndexInternal(lvIndex, startOpIdx, endOpIdx); + } + + void setLvIndexInternal(int lvIndex, int startOpIdx, int endOpIdx) { this.lvIndex = lvIndex; this.startOpIdx = startOpIdx; this.endOpIdx = endOpIdx; } - public void setSrcName(@Nullable String name) { - this.srcName = name; - } - void accept(MappingVisitor visitor) throws IOException { - if (visitor.visitMethodVar(lvtRowIndex, lvIndex, startOpIdx, endOpIdx, srcName)) { + if (visitor.visitMethodVar(lvtRowIndex, lvIndex, startOpIdx, endOpIdx, getSrcName())) { acceptElement(visitor, null); } } @Override protected void copyFrom(MethodVarEntry o, boolean replace) { - super.copyFrom(o, replace); + if (o.lvtRowIndex >= 0 && (replace || lvtRowIndex < 0)) { + setLvtRowIndexInternal(o.lvtRowIndex); + } - if (o.srcName != null && (replace || srcName == null)) { - srcName = o.srcName; + if (o.lvIndex >= 0 && o.startOpIdx >= 0 && (replace || lvIndex < 0 || startOpIdx < 0)) { + setLvIndexInternal(o.lvIndex, o.startOpIdx, o.endOpIdx); } + + if (o.getSrcName() != null && (replace || getSrcName() == null)) { + setSrcName(o.getSrcName()); + } + + super.copyFrom(o, replace); } @Override public String toString() { - return String.format("%d/%d@%d-%d:%s", lvtRowIndex, lvIndex, startOpIdx, endOpIdx, srcName); + return String.format("%d/%d@%d-%d:%s", lvtRowIndex, lvIndex, startOpIdx, endOpIdx, getSrcName()); } private final MethodEntry method; @@ -1697,11 +1933,13 @@ public String toString() { } static final class MemberKey { - MemberKey(String name, @Nullable String desc) { + MemberKey(@Nullable String name, @Nullable String desc) { this.name = name; this.desc = desc; - if (desc == null) { + if (name == null) { + hash = super.hashCode(); + } else if (desc == null) { hash = name.hashCode(); } else { hash = name.hashCode() * 257 + desc.hashCode(); @@ -1710,11 +1948,17 @@ static final class MemberKey { @Override public boolean equals(Object obj) { - if (obj == null || obj.getClass() != MemberKey.class) return false; + if (obj == null || obj.getClass() != MemberKey.class) { + return false; + } MemberKey o = (MemberKey) obj; - return name.equals(o.name) && Objects.equals(desc, o.desc); + if (name == null || o.name == null) { + return false; + } + + return Objects.equals(name, o.name) && Objects.equals(desc, o.desc); } @Override @@ -1812,19 +2056,24 @@ public String toString() { private final boolean isField; } + private boolean inVisitPass; private boolean indexByDstNames; 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; + /** The incoming source namespace's namespace index on the tree side. */ private int srcNsMap; private int[] dstNameMap; private Entry currentEntry; private ClassEntry currentClass; private MethodEntry currentMethod; + /** originalSrcName -> clsEntry. */ + private Map pendingClasses; private Map> pendingMembers; } diff --git a/src/main/java/net/fabricmc/mappingio/tree/VisitableMappingTree.java b/src/main/java/net/fabricmc/mappingio/tree/VisitableMappingTree.java index 4e9b3a97..bd035f68 100644 --- a/src/main/java/net/fabricmc/mappingio/tree/VisitableMappingTree.java +++ b/src/main/java/net/fabricmc/mappingio/tree/VisitableMappingTree.java @@ -20,6 +20,11 @@ /** * {@link MappingTree} that can be visited. + * + *

Accessing data manipulating {@link MappingTree} methods during an ongoing visitation pass + * may lead to undefined behavior and data corruption. + * + *

Visited data isn't guaranteed to be available until the visitation pass is complete. */ public interface VisitableMappingTree extends MappingTree, MappingVisitor { } diff --git a/src/test/java/net/fabricmc/mappingio/TestHelper.java b/src/test/java/net/fabricmc/mappingio/TestHelper.java index 7411a35d..b2cbc567 100644 --- a/src/test/java/net/fabricmc/mappingio/TestHelper.java +++ b/src/test/java/net/fabricmc/mappingio/TestHelper.java @@ -625,6 +625,7 @@ public static MemoryMappingTree getCorrespondingTree(Path dir) throws IOExceptio public static final Path VALID = getResource("/read/valid/"); public static final Path REPEATED_ELEMENTS = getResource("/read/repeated-elements/"); public static final Path VALID_WITH_HOLES = getResource("/read/valid-with-holes/"); + public static final Path MERGING = getResource("/merging/"); } private static final List fldDescs = Arrays.asList("I", "Lcls;", "Lpkg/cls;", "[I"); diff --git a/src/test/java/net/fabricmc/mappingio/VisitOrderVerifyingVisitor.java b/src/test/java/net/fabricmc/mappingio/VisitOrderVerifyingVisitor.java index cbdb6211..02732191 100644 --- a/src/test/java/net/fabricmc/mappingio/VisitOrderVerifyingVisitor.java +++ b/src/test/java/net/fabricmc/mappingio/VisitOrderVerifyingVisitor.java @@ -58,6 +58,12 @@ private void init() { resetLastSrcInfoDownTo(0); } + @Override + public void reset() { + init(); + super.reset(); + } + private void resetVisitedElementContentDownTo(int inclusiveLevel) { for (int i = visitedElementContent.length - 1; i >= inclusiveLevel; i--) { visitedElementContent[i] = false; diff --git a/src/test/java/net/fabricmc/mappingio/read/EmptyContentReadTest.java b/src/test/java/net/fabricmc/mappingio/read/EmptyContentReadTest.java index a1a9f777..2783ee52 100644 --- a/src/test/java/net/fabricmc/mappingio/read/EmptyContentReadTest.java +++ b/src/test/java/net/fabricmc/mappingio/read/EmptyContentReadTest.java @@ -110,11 +110,11 @@ public void emptyTsrgFile() throws Exception { String header5 = header4 + "dstNs"; TsrgFileReader.read(new StringReader(header0), target); // interpreted as TSRG v1 + instantiateTree(); assertThrows(IOException.class, () -> TsrgFileReader.read(new StringReader(header1), target)); assertThrows(IOException.class, () -> TsrgFileReader.read(new StringReader(header2), target)); assertThrows(IOException.class, () -> TsrgFileReader.read(new StringReader(header3), target)); assertThrows(IOException.class, () -> TsrgFileReader.read(new StringReader(header4), target)); - instantiateTree(); TsrgFileReader.read(new StringReader(header5), target); } @@ -122,10 +122,10 @@ public void emptyTsrgFile() throws Exception { public void emptyMigrationMapFile() throws Exception { assertThrows(IOException.class, () -> MigrationMapFileReader.read(new StringReader(""), target)); - instantiateTree(); + target.reset(); assertThrows(IOException.class, () -> MigrationMapFileReader.read(new StringReader(""), target)); - instantiateTree(); + target.reset(); MigrationMapFileReader.read( new StringReader("\n" + "\n" diff --git a/src/test/java/net/fabricmc/mappingio/tree/MergeTest.java b/src/test/java/net/fabricmc/mappingio/tree/MergeTest.java index c87d87cf..facff314 100644 --- a/src/test/java/net/fabricmc/mappingio/tree/MergeTest.java +++ b/src/test/java/net/fabricmc/mappingio/tree/MergeTest.java @@ -17,27 +17,44 @@ package net.fabricmc.mappingio.tree; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertThrows; +import java.io.IOException; +import java.nio.file.Path; +import java.util.Arrays; import java.util.Collections; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import net.fabricmc.mappingio.MappedElementKind; +import net.fabricmc.mappingio.MappingReader; import net.fabricmc.mappingio.MappingVisitor; +import net.fabricmc.mappingio.SubsetAssertingVisitor; +import net.fabricmc.mappingio.TestHelper; import net.fabricmc.mappingio.VisitOrderVerifyingVisitor; +import net.fabricmc.mappingio.adapter.FlatAsRegularMappingVisitor; import net.fabricmc.mappingio.tree.MappingTree.ClassMapping; import net.fabricmc.mappingio.tree.MappingTree.FieldMapping; import net.fabricmc.mappingio.tree.MemoryMappingTree.ClassEntry; import net.fabricmc.mappingio.tree.MemoryMappingTree.FieldEntry; public class MergeTest { - private static final String srcNs = "ns1"; - private static final String dstNs = "ns2"; - private static final String clsName = "class1Ns1Name"; - private static final String fldName = "field1Ns1Name"; - private static final String fldComment = "field1Ns1Comment"; - private static final String fldDesc = "I"; + private static final Path dir = TestHelper.MappingDirs.MERGING; + private static final String ns1 = "ns1"; + private static final String ns2 = "ns2"; + private static final String ns3 = "ns3"; + private static final String ns4 = "ns4"; + private static final String cls1Ns1Name = "cls1Ns1Name"; + private static final String cls1Ns2Name = "cls1Ns2Name"; + private static final String cls2Ns1Name = "cls2Ns1Name"; + private static final String cls2Ns2Name = "cls2Ns2Name"; + private static final String fld1Ns1Name = "fld1Ns1Name"; + private static final String fld1Ns2Name = "fld1Ns2Name"; + private static final String fld1Ns1Desc = "L" + cls1Ns1Name + ";"; + private static final String fld1Ns2Desc = "L" + cls1Ns2Name + ";"; + private static final String fld1Comment = "fld1Comment"; private MemoryMappingTree tree; private MappingVisitor delegate; @@ -50,37 +67,37 @@ public void setup() { @Test public void descHoldingMemberIntoDescless() throws Exception { delegate.visitHeader(); - delegate.visitNamespaces(srcNs, Collections.singletonList(dstNs)); + delegate.visitNamespaces(ns1, Collections.singletonList(ns2)); delegate.visitContent(); - delegate.visitClass(clsName); + delegate.visitClass(cls1Ns1Name); delegate.visitElementContent(MappedElementKind.CLASS); - delegate.visitField(fldName, null); + delegate.visitField(fld1Ns1Name, null); delegate.visitElementContent(MappedElementKind.FIELD); - delegate.visitComment(MappedElementKind.FIELD, fldComment); + delegate.visitComment(MappedElementKind.FIELD, fld1Comment); delegate.visitEnd(); - ClassMapping cls = tree.getClass(clsName); - FieldMapping fld = cls.addField(fieldMappingOf(cls, fldName, fldDesc)); + ClassMapping cls = tree.getClass(cls1Ns1Name); + FieldMapping fld = cls.addField(fieldMappingOf(cls, fld1Ns1Name, fld1Ns1Desc)); - assertEquals(fldComment, fld.getComment()); + assertEquals(fld1Comment, fld.getComment()); } @Test public void desclessMemberIntoDescHolder() throws Exception { delegate.visitHeader(); - delegate.visitNamespaces(srcNs, Collections.singletonList(dstNs)); + delegate.visitNamespaces(ns1, Collections.singletonList(ns2)); delegate.visitContent(); - delegate.visitClass(clsName); + delegate.visitClass(cls1Ns1Name); delegate.visitElementContent(MappedElementKind.CLASS); - delegate.visitField(fldName, fldDesc); + delegate.visitField(fld1Ns1Name, fld1Ns1Desc); delegate.visitElementContent(MappedElementKind.FIELD); - delegate.visitComment(MappedElementKind.FIELD, fldComment); + delegate.visitComment(MappedElementKind.FIELD, fld1Comment); delegate.visitEnd(); - ClassMapping cls = tree.getClass(clsName); - FieldMapping fld = cls.addField(fieldMappingOf(cls, fldName, fldDesc)); + ClassMapping cls = tree.getClass(cls1Ns1Name); + FieldMapping fld = cls.addField(fieldMappingOf(cls, fld1Ns1Name, fld1Ns1Desc)); - assertEquals(fldDesc, fld.getSrcDesc()); + assertEquals(fld1Ns1Desc, fld.getSrcDesc()); } private FieldMapping fieldMappingOf(ClassMapping cls, String name, String desc) throws Exception { @@ -88,5 +105,191 @@ private FieldMapping fieldMappingOf(ClassMapping cls, String name, String desc) .getDeclaredConstructor(ClassEntry.class, String.class, String.class) .newInstance(cls, name, desc); } + + @Test + public void ns1ToNs2ThenNs2ToNs3() throws Exception { + String cls1Ns3Name = "cls1Ns3Name"; + + delegate.visitHeader(); + delegate.visitNamespaces(ns1, Collections.singletonList(ns2)); + delegate.visitContent(); + delegate.visitClass(cls1Ns1Name); + delegate.visitDstName(MappedElementKind.CLASS, 0, cls1Ns2Name); + delegate.visitElementContent(MappedElementKind.CLASS); + delegate.visitEnd(); + + delegate.visitHeader(); + delegate.visitNamespaces(ns2, Arrays.asList(ns3, ns1)); + delegate.visitContent(); + delegate.visitClass(cls1Ns2Name); + delegate.visitDstName(MappedElementKind.CLASS, 0, cls1Ns3Name); + delegate.visitElementContent(MappedElementKind.CLASS); + delegate.visitField(fld1Ns2Name, fld1Ns2Desc); + delegate.visitDstName(MappedElementKind.FIELD, 1, fld1Ns1Name); + delegate.visitElementContent(MappedElementKind.FIELD); + delegate.visitEnd(); + + ClassMapping cls = tree.getClass(cls1Ns1Name); + assertNotNull(cls); + assertEquals(cls1Ns2Name, cls.getDstName(0)); + assertEquals(cls1Ns3Name, cls.getDstName(1)); + + FieldMapping fld = cls.getField(fld1Ns1Name, fld1Ns1Desc); + assertNotNull(fld); + assertEquals(fld1Ns2Name, fld.getDstName(0)); + assertEquals(null, fld.getDstName(1)); + assertEquals(fld1Ns1Desc, fld.getSrcDesc()); + } + + @Test + public void disassociatedNamespaces() throws Exception { + delegate.visitHeader(); + delegate.visitNamespaces(ns1, Collections.singletonList(ns2)); + delegate.visitContent(); + delegate.visitEnd(); + + delegate.visitHeader(); + assertThrows(IllegalArgumentException.class, () -> delegate.visitNamespaces(ns3, Collections.singletonList(ns4))); + + delegate.reset(); + delegate.visitHeader(); + assertThrows(IllegalArgumentException.class, () -> delegate.visitNamespaces(ns4, Collections.singletonList(ns3))); + } + + @Test + public void srcNsDuplicatedToDstSide() throws Exception { + // Uninitialized tree + delegate.visitHeader(); + assertThrows(IllegalArgumentException.class, () -> delegate.visitNamespaces(ns1, Collections.singletonList(ns1))); + + // Initialized tree with subsequent incorrect visit + setup(); + delegate.visitHeader(); + delegate.visitNamespaces(ns1, Collections.singletonList(ns2)); + delegate.visitContent(); + delegate.visitEnd(); + delegate.visitHeader(); + assertThrows(IllegalArgumentException.class, () -> delegate.visitNamespaces(ns2, Collections.singletonList(ns2))); + } + + @Test + public void pendingElementsQueue() throws Exception { + pendingElementsQueue0(false); + pendingElementsQueue0(true); + } + + private void pendingElementsQueue0(boolean visitDstNames) throws IOException { + delegate.visitHeader(); + delegate.visitNamespaces(ns1, Collections.singletonList(ns2)); + delegate.visitContent(); + delegate.visitEnd(); + + delegate.visitHeader(); + delegate.visitNamespaces(ns2, Collections.singletonList(ns1)); + delegate.visitContent(); + delegate.visitClass(cls1Ns2Name); + + if (visitDstNames) { + delegate.visitDstName(MappedElementKind.CLASS, 0, cls1Ns1Name); + } + + delegate.visitElementContent(MappedElementKind.CLASS); + delegate.visitField(fld1Ns2Name, fld1Ns1Desc); + + if (visitDstNames) { + delegate.visitDstName(MappedElementKind.FIELD, 0, fld1Ns1Name); + } + + delegate.visitElementContent(MappedElementKind.FIELD); + delegate.visitEnd(); + + assertEquals(tree.getClass(cls1Ns1Name) != null, visitDstNames); + assertEquals(tree.getField(cls1Ns1Name, fld1Ns1Name, fld1Ns1Desc) != null, visitDstNames); + } + + /* + * Destination tree: + * ns1 | ns2 + * ---------------------------------|---------------- + * cls1Ns1Name | cls1Ns2Name + * + * + * To-be-merged tree: + * ns2 | ns1 + * ---------------------------------|---------------- + * cls2Ns2Name | cls2Ns1Name + * \-- fldNs2Name : Lcls1Ns2Name; | \-- fldNs1Name + * + * + * To-be-merged tree, primitively flipped (e.g. via MappingSourceNsSwitch): + * ns1 | ns2 + * ---------------------------------|---------------- + * cls2Ns1Name | cls2Ns2Name + * \-- fldNs1Name : Lcls1Ns2Name; | \-- fldNs2Name + * + * fld's ns1 descriptor still references ns2's name of cls1, + * since the tree didn't contain any data for cls1. + * + * + * When merged into destination tree: + * ns1 | ns2 + * ---------------------------------|---------------- + * cls1Ns1Name | cls1Ns2Name + * cls2Ns1Name | cls2Ns2Name + * \-- fldNs1Name : Lcls1Ns2Name; | \-- fldNs2Name + * + * Incorrect, there's now no way of knowing that fld's descriptor was originally referencing cls1. + * + * + * When merging to-be-merged tree directly into destination tree: + * ns1 | ns2 + * ---------------------------------|---------------- + * cls1Ns1Name | cls1Ns2Name + * cls2Ns1Name | cls2Ns2Name + * \-- fldNs1Name : Lcls1Ns1Name; | \-- fldNs2Name + * + * Correct thanks to MemoryMappingTree's advanced merging capabilities. + */ + @Test + public void descriptorCompletion() throws IOException { + delegate.visitHeader(); + delegate.visitNamespaces(ns1, Collections.singletonList(ns2)); + delegate.visitContent(); + delegate.visitClass(cls1Ns1Name); + delegate.visitDstName(MappedElementKind.CLASS, 0, cls1Ns2Name); + delegate.visitElementContent(MappedElementKind.CLASS); + delegate.visitEnd(); + + delegate.visitHeader(); + delegate.visitNamespaces(ns2, Collections.singletonList(ns1)); + delegate.visitContent(); + delegate.visitClass(cls2Ns2Name); + delegate.visitDstName(MappedElementKind.CLASS, 0, cls2Ns1Name); + delegate.visitElementContent(MappedElementKind.CLASS); + delegate.visitField(fld1Ns2Name, fld1Ns2Desc); + delegate.visitDstName(MappedElementKind.FIELD, 0, fld1Ns1Name); + delegate.visitElementContent(MappedElementKind.FIELD); + delegate.visitEnd(); + + assertEquals(fld1Ns1Desc, tree.getField(cls2Ns1Name, fld1Ns1Name, null).getSrcDesc()); + } + + @Test + public void diskMappings() throws IOException { + MappingReader.read(dir.resolve("tree1.tiny"), delegate); + MappingReader.read(dir.resolve("tree2.tiny"), delegate); + + MemoryMappingTree referenceTree = new MemoryMappingTree(); + 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))); + + MappingReader.read(dir.resolve("tree3.tiny"), delegate); + + referenceTree = new MemoryMappingTree(); + 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))); + } } diff --git a/src/test/resources/merging/tree1+2+3.tiny b/src/test/resources/merging/tree1+2+3.tiny new file mode 100644 index 00000000..1f346bde --- /dev/null +++ b/src/test/resources/merging/tree1+2+3.tiny @@ -0,0 +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 diff --git a/src/test/resources/merging/tree1+2.tiny b/src/test/resources/merging/tree1+2.tiny new file mode 100644 index 00000000..0f4b2788 --- /dev/null +++ b/src/test/resources/merging/tree1+2.tiny @@ -0,0 +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 diff --git a/src/test/resources/merging/tree1.tiny b/src/test/resources/merging/tree1.tiny new file mode 100644 index 00000000..f90f9c5e --- /dev/null +++ b/src/test/resources/merging/tree1.tiny @@ -0,0 +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 diff --git a/src/test/resources/merging/tree2.tiny b/src/test/resources/merging/tree2.tiny new file mode 100644 index 00000000..bc96deb3 --- /dev/null +++ b/src/test/resources/merging/tree2.tiny @@ -0,0 +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 diff --git a/src/test/resources/merging/tree3.tiny b/src/test/resources/merging/tree3.tiny new file mode 100644 index 00000000..b8669535 --- /dev/null +++ b/src/test/resources/merging/tree3.tiny @@ -0,0 +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