Improve MemoryMappingTree merging capabilities

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
 

src/main/java/net/fabricmc/mappingio/MappingVisitor.java

@@ -47,6 +47,9 @@
  * something else after a {@link #reset()}.
  *
  * <p>The same element may be visited more than once unless the flags contain {@link MappingFlag#NEEDS_ELEMENT_UNIQUENESS}.
+ *
+ * <p>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<MappingFlag> getFlags() {

src/main/java/net/fabricmc/mappingio/tree/MappingTree.java

@@ -23,10 +23,21 @@
 
 /**
  * Mutable mapping tree.
+ *
+ * <p>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<String> setDstNamespaces(List<String> namespaces);
 
 	/**
@@ -37,8 +48,7 @@ public interface MappingTree extends MappingTreeView {
 	List<? extends MetadataEntry> 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);
 	}

src/main/java/net/fabricmc/mappingio/tree/MappingTreeView.java

@@ -26,6 +26,9 @@
 
 /**
  * Read-only mapping tree.
+ *
+ * <p>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<? extends MetadataEntryView> 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<? extends MetadataEntryView> getMetadata(String key);
 
 	Collection<? extends ClassMappingView> getClasses();