Make OuterClassNamePropagator configurable, refactor tests (#119)

CHANGELOG.md

@@ -4,9 +4,11 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
 ## [Unreleased]
+- Made `OuterClassNamePropagator` configurable
+- Added a simplified `MappingNsCompleter` constructor for completing all destination names with the source names
 
 ## [0.7.1] - 2025-01-07
-- Restore the ability to read source-namespace-only mapping files, even if not spec-compliant
+- Restored the ability to read source-namespace-only mapping files, even if not spec-compliant
 
 ## [0.7.0] - 2025-01-01
 - Added IntelliJ IDEA migration map reader and writer

build.gradle

@@ -202,12 +202,7 @@ tasks.register("generateTestMappings", JavaExec) {
 	dependsOn("testClasses")
 	classpath = sourceSets.test.runtimeClasspath
 	mainClass = 'net.fabricmc.mappingio.test.TestFileUpdater'
-}
-
-tasks.register("updateTestMappings", Copy) {
-	dependsOn("generateTestMappings")
-	from 'build/resources/test/'
-	into 'src/test/resources/'
+	args = [file('src/test/resources/').getAbsolutePath()]
 }
 
 // A task to ensure that the version being released has not already been released.

mapping-io-extras/src/test/java/net/fabricmc/mappingio/extras/MappingTreeRemapperTest.java

@@ -25,7 +25,7 @@
 import org.junit.jupiter.api.Test;
 import org.objectweb.asm.Type;
 
-import net.fabricmc.mappingio.test.TestUtil;
+import net.fabricmc.mappingio.test.TestMappings;
 import net.fabricmc.mappingio.tree.MappingTree;
 import net.fabricmc.mappingio.tree.MappingTree.ClassMapping;
 import net.fabricmc.mappingio.tree.MappingTree.FieldMapping;
@@ -50,7 +50,7 @@ public class MappingTreeRemapperTest {
 
 	@BeforeAll
 	public static void setup() throws IOException {
-		mappingTree = TestUtil.acceptTestMappings(new MemoryMappingTree());
+		mappingTree = TestMappings.generateValid(new MemoryMappingTree());
 		srcNs = mappingTree.getSrcNamespace();
 		dstNs = mappingTree.getDstNamespaces().get(0);
 

src/main/java/net/fabricmc/mappingio/adapter/MappingNsCompleter.java

@@ -19,6 +19,7 @@
 import java.io.IOException;
 import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 
@@ -34,20 +35,31 @@
  * This visitor fills in these "holes" by copying names from another namespace.
  */
 public final class MappingNsCompleter extends ForwardingMappingVisitor {
+	/**
+	 * Constructs a new {@link MappingNsCompleter} which completes all destination namespaces.
+	 *
+	 * @param next The next visitor to forward the data to.
+	 */
+	public MappingNsCompleter(MappingVisitor next) {
+		this(next, null, false);
+	}
+
 	/**
 	 * @param next The next visitor to forward the data to.
 	 * @param alternatives A map of which namespaces should copy from which others.
+	 * Passing {@code null} causes all destination namespaces to be completed.
 	 */
-	public MappingNsCompleter(MappingVisitor next, Map<String, String> alternatives) {
+	public MappingNsCompleter(MappingVisitor next, @Nullable Map<String, String> alternatives) {
 		this(next, alternatives, false);
 	}
 
 	/**
 	 * @param next The next visitor to forward the data to.
 	 * @param alternatives A map of which namespaces should copy from which others.
+	 * Passing {@code null} causes all destination namespaces to be completed.
 	 * @param addMissingNs Whether to copy namespaces from the alternatives key set if not already present.
 	 */
-	public MappingNsCompleter(MappingVisitor next, Map<String, String> alternatives, boolean addMissingNs) {
+	public MappingNsCompleter(MappingVisitor next, @Nullable Map<String, String> alternatives, boolean addMissingNs) {
 		super(next);
 
 		this.alternatives = alternatives;
@@ -63,6 +75,14 @@ public boolean visitHeader() throws IOException {
 
 	@Override
 	public void visitNamespaces(String srcNamespace, List<String> dstNamespaces) throws IOException {
+		if (alternatives == null) {
+			alternatives = new HashMap<>(dstNamespaces.size());
+
+			for (String ns : dstNamespaces) {
+				alternatives.put(ns, srcNamespace);
+			}
+		}
+
 		if (addMissingNs) {
 			boolean copied = false;
 
@@ -192,8 +212,8 @@ public boolean visitElementContent(MappedElementKind targetKind) throws IOExcept
 		return next.visitElementContent(targetKind);
 	}
 
-	private final Map<String, String> alternatives;
 	private final boolean addMissingNs;
+	private Map<String, String> alternatives;
 	private int[] alternativesMapping;
 
 	private String srcName;

src/main/java/net/fabricmc/mappingio/adapter/OuterClassNamePropagator.java

@@ -17,7 +17,9 @@
 package net.fabricmc.mappingio.adapter;
 
 import java.io.IOException;
+import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.Collection;
 import java.util.EnumSet;
 import java.util.HashMap;
 import java.util.HashSet;
@@ -34,8 +36,9 @@
 import net.fabricmc.mappingio.MappingVisitor;
 
 /**
- * Searches for inner classes with no mapped name, whose enclosing classes do have mapped names,
- * and applies those to the outer part of the inner classes' fully qualified name.
+ * Searches for inner classes whose effective destination name contains outer classes referenced via their source name,
+ * waits for mappings for these enclosing classes, and applies the latters' destination names
+ * to the formers' fully qualified name.
  *
  * <p>For example, it takes a class {@code class_1$class_2} that doesn't have a mapping,
  * tries to find {@code class_1}, which let's say has the mapping {@code SomeClass},
@@ -45,8 +48,24 @@
  * the other to actually apply the outer names. The third pass onwards will then emit the final mappings.
  */
 public class OuterClassNamePropagator extends ForwardingMappingVisitor {
+	/**
+	 * Constructs a new {@link OuterClassNamePropagator} which processes all destination namespaces,
+	 * including already remapped class destination names therein.
+	 */
 	public OuterClassNamePropagator(MappingVisitor next) {
+		this(next, null, true);
+	}
+
+	/**
+	 * Constructs a new {@link OuterClassNamePropagator} which processes the selected destination namespaces.
+	 *
+	 * @param namespaces The destination namespaces where outer class names shall be propagated. Pass {@code null} to process all destination namespaces.
+	 * @param processRemappedDstNames Whether already remapped destination names should also get their unmapped outer classes replaced.
+	 */
+	public OuterClassNamePropagator(MappingVisitor next, @Nullable Collection<String> namespaces, boolean processRemappedDstNames) {
 		super(next);
+		this.dstNamespacesToProcess = namespaces;
+		this.processRemappedDstNames = processRemappedDstNames;
 	}
 
 	@Override
@@ -60,34 +79,61 @@ public Set<MappingFlag> getFlags() {
 
 	@Override
 	public boolean visitHeader() throws IOException {
-		if (pass < firstEmitPass) return true;
+		if (pass < FIST_EMIT_PASS) return true;
 
 		return super.visitHeader();
 	}
 
 	@Override
 	@SuppressWarnings("unchecked")
 	public void visitNamespaces(String srcNamespace, List<String> dstNamespaces) throws IOException {
-		dstNsCount = dstNamespaces.size();
+		if (pass == COLLECT_CLASSES_PASS) {
+			if (dstNamespacesToProcess == null) {
+				dstNamespacesToProcess = dstNamespaces;
+			} else {
+				if (dstNamespacesToProcess.contains(srcNamespace)) {
+					throw new UnsupportedOperationException(srcNamespace + " was passed as a destination namespace"
+							+ " to propagate outer class names in, but has been visited as the source namespace.");
+				}
+
+				for (String ns : dstNamespacesToProcess) {
+					if (!dstNamespaces.contains(ns)) {
+						throw new IllegalArgumentException(ns + " was passed as a destination namespace to propagate outer class names in,"
+								+ " but is not present in the namespaces of the current visitation pass.");
+					}
+				}
+			}
+
+			this.dstNamespaces = dstNamespaces;
+			this.dstNsCount = dstNamespaces.size();
+
+			if (dstNamespaceIndicesToProcess == null) {
+				dstNamespaceIndicesToProcess = new ArrayList<>();
+
+				for (int i = 0; i < dstNsCount; i++) {
+					if (dstNamespacesToProcess.contains(dstNamespaces.get(i))) {
+						dstNamespaceIndicesToProcess.add(i);
+					}
+				}
+			}
 
-		if (pass == collectClassesPass) {
 			visitedDstName = new boolean[dstNsCount];
 			dstNameBySrcNameByNamespace = new HashMap[dstNsCount];
-		} else if (pass >= firstEmitPass) {
+		} else if (pass >= FIST_EMIT_PASS) {
 			super.visitNamespaces(srcNamespace, dstNamespaces);
 		}
 	}
 
 	@Override
 	public void visitMetadata(String key, @Nullable String value) throws IOException {
-		if (pass < firstEmitPass) return;
+		if (pass < FIST_EMIT_PASS) return;
 
 		super.visitMetadata(key, value);
 	}
 
 	@Override
 	public boolean visitContent() throws IOException {
-		if (pass < firstEmitPass) return true;
+		if (pass < FIST_EMIT_PASS) return true;
 
 		return super.visitContent();
 	}
@@ -96,9 +142,9 @@ public boolean visitContent() throws IOException {
 	public boolean visitClass(String srcName) throws IOException {
 		this.srcName = srcName;
 
-		if (pass == collectClassesPass) {
+		if (pass == COLLECT_CLASSES_PASS) {
 			dstNamesBySrcName.putIfAbsent(srcName, new String[dstNsCount]);
-		} else if (pass >= firstEmitPass) {
+		} else if (pass >= FIST_EMIT_PASS) {
 			super.visitClass(srcName);
 		}
 
@@ -107,11 +153,11 @@ public boolean visitClass(String srcName) throws IOException {
 
 	@Override
 	public void visitDstName(MappedElementKind targetKind, int namespace, String name) throws IOException {
-		if (pass == collectClassesPass) {
+		if (pass == COLLECT_CLASSES_PASS) {
 			if (targetKind != MappedElementKind.CLASS) return;
 
 			dstNamesBySrcName.get(srcName)[namespace] = name;
-		} else if (pass >= firstEmitPass) {
+		} else if (pass >= FIST_EMIT_PASS) {
 			if (targetKind == MappedElementKind.CLASS) {
 				visitedDstName[namespace] = true;
 				name = dstNamesBySrcName.get(srcName)[namespace];
@@ -123,7 +169,7 @@ public void visitDstName(MappedElementKind targetKind, int namespace, String nam
 
 	@Override
 	public void visitDstDesc(MappedElementKind targetKind, int namespace, String desc) throws IOException {
-		if (pass < firstEmitPass) return;
+		if (pass < FIST_EMIT_PASS) return;
 
 		if (modifiedClasses.contains(srcName)) {
 			Map<String, String> nsDstNameBySrcName = dstNameBySrcNameByNamespace[namespace];
@@ -143,23 +189,37 @@ public void visitDstDesc(MappedElementKind targetKind, int namespace, String des
 
 	@Override
 	public boolean visitElementContent(MappedElementKind targetKind) throws IOException {
-		if (targetKind == MappedElementKind.CLASS && pass > collectClassesPass) {
+		if (targetKind == MappedElementKind.CLASS && pass > COLLECT_CLASSES_PASS) {
 			String[] dstNames = dstNamesBySrcName.get(srcName);
 
 			for (int ns = 0; ns < dstNames.length; ns++) {
+				if (!dstNamespacesToProcess.contains(dstNamespaces.get(ns))) {
+					continue;
+				}
+
 				String dstName = dstNames[ns];
 
-				if (pass == fixOuterClassesPass) {
-					if (dstName != null) continue; // skip if already mapped
+				if (pass == FIX_OUTER_CLASSES_PASS) {
+					if (!processRemappedDstNames && dstName != null && !dstName.equals(srcName)) {
+						continue;
+					}
+
+					String[] srcParts = srcName.split(Pattern.quote("$"));
+					String[] dstParts = dstName == null ? srcParts : dstName.split(Pattern.quote("$"));
+					assert dstParts.length == srcParts.length;
 
-					String[] parts = srcName.split(Pattern.quote("$"));
+					for (int pos = srcParts.length - 2; pos >= 0; pos--) {
+						String outerSrcName = String.join("$", Arrays.copyOfRange(srcParts, 0, pos + 1));
+
+						if (dstName != null && !dstParts[pos].equals(srcParts[pos])) {
+							// That part already has a different mapping
+							continue;
+						}
 
-					for (int pos = parts.length - 2; pos >= 0; pos--) {
-						String outerSrcName = String.join("$", Arrays.copyOfRange(parts, 0, pos + 1));
 						String outerDstName = dstNamesBySrcName.get(outerSrcName)[ns];
 
-						if (outerDstName != null) {
-							dstName = outerDstName + "$" + String.join("$", Arrays.copyOfRange(parts, pos + 1, parts.length));
+						if (outerDstName != null && !outerDstName.equals(outerSrcName)) {
+							dstName = outerDstName + "$" + String.join("$", Arrays.copyOfRange(dstParts, pos + 1, dstParts.length));
 
 							dstNames[ns] = dstName;
 							modifiedClasses.add(srcName);
@@ -176,7 +236,7 @@ public boolean visitElementContent(MappedElementKind targetKind) throws IOExcept
 			}
 		}
 
-		if (pass < firstEmitPass) {
+		if (pass < FIST_EMIT_PASS) {
 			return false; // prevent other element visits, we only care about classes here
 		}
 
@@ -186,18 +246,24 @@ public boolean visitElementContent(MappedElementKind targetKind) throws IOExcept
 
 	@Override
 	public boolean visitEnd() throws IOException {
-		if (pass++ < firstEmitPass) {
+		if (pass++ < FIST_EMIT_PASS) {
 			return false;
 		}
 
 		return super.visitEnd();
 	}
 
-	private static final int collectClassesPass = 1;
-	private static final int fixOuterClassesPass = 2;
-	private static final int firstEmitPass = 3;
+	private static final int COLLECT_CLASSES_PASS = 1;
+	private static final int FIX_OUTER_CLASSES_PASS = 2;
+	private static final int FIST_EMIT_PASS = 3;
+	private final boolean processRemappedDstNames;
 	private final Map<String, String[]> dstNamesBySrcName = new HashMap<>();
 	private final Set<String> modifiedClasses = new HashSet<>();
+	private String srcNamespaceToProcess;
+	private int srcNsId;
+	private List<String> dstNamespaces;
+	private Collection<String> dstNamespacesToProcess;
+	private Collection<Integer> dstNamespaceIndicesToProcess;
 	private int pass = 1;
 	private int dstNsCount = -1;
 	private String srcName;