Fix conflicts caused by MappingsMerger (#1171)

SpaceWalkerRS · web-flow · commit bc58b6f05847 · 2024-09-24T09:51:40.000+01:00
* only complete official namespaces for unobfuscated members

* TinyRemapperHelper.create does not like null names

* add MappingsMergerTest
diff --git a/src/main/java/net/fabricmc/loom/configuration/providers/mappings/tiny/MappingsMerger.java b/src/main/java/net/fabricmc/loom/configuration/providers/mappings/tiny/MappingsMerger.java
@@ -34,13 +34,13 @@
 import java.util.regex.Pattern;
 
 import com.google.common.base.Stopwatch;
+import org.jetbrains.annotations.VisibleForTesting;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
 import net.fabricmc.loom.api.mappings.layered.MappingsNamespace;
 import net.fabricmc.loom.configuration.providers.mappings.IntermediateMappingsService;
 import net.fabricmc.loom.configuration.providers.minecraft.MinecraftProvider;
-import net.fabricmc.mappingio.adapter.MappingNsCompleter;
 import net.fabricmc.mappingio.adapter.MappingSourceNsSwitch;
 import net.fabricmc.mappingio.format.tiny.Tiny2FileReader;
 import net.fabricmc.mappingio.format.tiny.Tiny2FileWriter;
@@ -63,7 +63,8 @@ public static void mergeAndSaveMappings(Path from, Path out, MinecraftProvider m
 		LOGGER.info(":merged mappings in " + stopwatch.stop());
 	}
 
-	private static void mergeAndSaveMappings(Path from, Path out, IntermediateMappingsService intermediateMappingsService) throws IOException {
+	@VisibleForTesting
+	public static void mergeAndSaveMappings(Path from, Path out, IntermediateMappingsService intermediateMappingsService) throws IOException {
 		MemoryMappingTree intermediaryTree = new MemoryMappingTree();
 		intermediateMappingsService.getMemoryMappingTree().accept(new MappingSourceNsSwitch(intermediaryTree, MappingsNamespace.INTERMEDIARY.toString()));
 
@@ -72,7 +73,7 @@ private static void mergeAndSaveMappings(Path from, Path out, IntermediateMappin
 		}
 
 		MemoryMappingTree officialTree = new MemoryMappingTree();
-		MappingNsCompleter nsCompleter = new MappingNsCompleter(officialTree, Map.of(MappingsNamespace.OFFICIAL.toString(), MappingsNamespace.INTERMEDIARY.toString()));
+		UnobfuscatedMappingNsCompleter nsCompleter = new UnobfuscatedMappingNsCompleter(officialTree, MappingsNamespace.NAMED.toString(), Map.of(MappingsNamespace.OFFICIAL.toString(), MappingsNamespace.INTERMEDIARY.toString()));
 		MappingSourceNsSwitch nsSwitch = new MappingSourceNsSwitch(nsCompleter, MappingsNamespace.OFFICIAL.toString());
 		intermediaryTree.accept(nsSwitch);
 
@@ -83,7 +84,8 @@ private static void mergeAndSaveMappings(Path from, Path out, IntermediateMappin
 		}
 	}
 
-	private static void legacyMergeAndSaveMappings(Path from, Path out, IntermediateMappingsService intermediateMappingsService) throws IOException {
+	@VisibleForTesting
+	public static void legacyMergeAndSaveMappings(Path from, Path out, IntermediateMappingsService intermediateMappingsService) throws IOException {
 		MemoryMappingTree intermediaryTree = new MemoryMappingTree();
 		intermediateMappingsService.getMemoryMappingTree().accept(intermediaryTree);
 
@@ -92,7 +94,7 @@ private static void legacyMergeAndSaveMappings(Path from, Path out, Intermediate
 		}
 
 		MemoryMappingTree officialTree = new MemoryMappingTree();
-		MappingNsCompleter nsCompleter = new MappingNsCompleter(officialTree, Map.of(MappingsNamespace.CLIENT_OFFICIAL.toString(), MappingsNamespace.INTERMEDIARY.toString(), MappingsNamespace.SERVER_OFFICIAL.toString(), MappingsNamespace.INTERMEDIARY.toString()));
+		UnobfuscatedMappingNsCompleter nsCompleter = new UnobfuscatedMappingNsCompleter(officialTree, MappingsNamespace.NAMED.toString(), Map.of(MappingsNamespace.CLIENT_OFFICIAL.toString(), MappingsNamespace.INTERMEDIARY.toString(), MappingsNamespace.SERVER_OFFICIAL.toString(), MappingsNamespace.INTERMEDIARY.toString()));
 		intermediaryTree.accept(nsCompleter);
 
 		// versions this old strip inner class attributes
diff --git a/src/main/java/net/fabricmc/loom/configuration/providers/mappings/tiny/UnobfuscatedMappingNsCompleter.java b/src/main/java/net/fabricmc/loom/configuration/providers/mappings/tiny/UnobfuscatedMappingNsCompleter.java
@@ -0,0 +1,231 @@
+/*
+ * This file is part of fabric-loom, licensed under the MIT License (MIT).
+ *
+ * Copyright (c) 2024 FabricMC
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in all
+ * copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ */
+
+package net.fabricmc.loom.configuration.providers.mappings.tiny;
+
+import java.io.IOException;
+import java.util.Arrays;
+import java.util.List;
+import java.util.Map;
+
+import org.jetbrains.annotations.Nullable;
+
+import net.fabricmc.mappingio.MappedElementKind;
+import net.fabricmc.mappingio.MappingVisitor;
+import net.fabricmc.mappingio.adapter.ForwardingMappingVisitor;
+
+/**
+ * Adapted from {@link net.fabricmc.mappingio.adapter.MappingNsCompleter}.
+ * This visitor completes any empty namespace with some alternative namespace
+ * only if that alternative namespace is equal to some test namespace.
+ * Mostly this will be used to complete official namespaces with intermediary
+ * names only if those intermediary names are equal to the named names.
+ */
+public final class UnobfuscatedMappingNsCompleter extends ForwardingMappingVisitor {
+	private final String testNs;
+	private final Map<String, String> alternatives;
+	private int testNsId;
+	private int[] alternativesMapping;
+
+	private String srcName;
+	private String[] dstNames;
+	private boolean[] unobf;
+	private boolean[] lastMethodUnobf;
+
+	private boolean relayHeaderOrMetadata;
+
+	public UnobfuscatedMappingNsCompleter(MappingVisitor next, String testNs, Map<String, String> alternatives) {
+		super(next);
+
+		this.testNs = testNs;
+		this.alternatives = alternatives;
+	}
+
+	@Override
+	public boolean visitHeader() throws IOException {
+		relayHeaderOrMetadata = next.visitHeader();
+
+		return true;
+	}
+
+	@Override
+	public void visitNamespaces(String srcNamespace, List<String> dstNamespaces) throws IOException {
+		int count = dstNamespaces.size();
+		testNsId = -1;
+		alternativesMapping = new int[count];
+		dstNames = new String[count];
+		unobf = new boolean[count + 1]; // contains src ns as well
+		lastMethodUnobf = new boolean[count + 1]; // contains src ns as well
+
+		for (int i = 0; i < count; i++) {
+			String dst = dstNamespaces.get(i);
+
+			if (testNs.equals(dst)) {
+				testNsId = i;
+			}
+
+			String src = alternatives.get(dst);
+			int srcIdx;
+
+			if (src == null) {
+				srcIdx = i;
+			} else if (src.equals(srcNamespace)) {
+				srcIdx = -1;
+			} else {
+				srcIdx = dstNamespaces.indexOf(src);
+				if (srcIdx < 0) throw new RuntimeException("invalid alternative mapping ns "+src+": not in "+dstNamespaces+" or "+srcNamespace);
+			}
+
+			alternativesMapping[i] = srcIdx;
+		}
+
+		if (testNsId == -1 && !testNs.equals(srcNamespace)) throw new RuntimeException("test namespace " + testNs + " not present in src and dst namespaces!");
+
+		if (relayHeaderOrMetadata) next.visitNamespaces(srcNamespace, dstNamespaces);
+	}
+
+	@Override
+	public void visitMetadata(String key, @Nullable String value) throws IOException {
+		if (relayHeaderOrMetadata) next.visitMetadata(key, value);
+	}
+
+	@Override
+	public boolean visitContent() throws IOException {
+		relayHeaderOrMetadata = true; // for in-content metadata
+
+		return next.visitContent();
+	}
+
+	@Override
+	public boolean visitClass(String srcName) throws IOException {
+		this.srcName = srcName;
+
+		return next.visitClass(srcName);
+	}
+
+	@Override
+	public boolean visitField(String srcName, @Nullable String srcDesc) throws IOException {
+		this.srcName = srcName;
+
+		return next.visitField(srcName, srcDesc);
+	}
+
+	@Override
+	public boolean visitMethod(String srcName, @Nullable String srcDesc) throws IOException {
+		this.srcName = srcName;
+
+		return next.visitMethod(srcName, srcDesc);
+	}
+
+	@Override
+	public boolean visitMethodArg(int argPosition, int lvIndex, @Nullable String srcName) throws IOException {
+		this.srcName = srcName;
+
+		return next.visitMethodArg(argPosition, lvIndex, srcName);
+	}
+
+	@Override
+	public boolean visitMethodVar(int lvtRowIndex, int lvIndex, int startOpIdx, int endOpIdx, @Nullable String srcName) throws IOException {
+		this.srcName = srcName;
+
+		return next.visitMethodVar(lvtRowIndex, lvIndex, startOpIdx, endOpIdx, srcName);
+	}
+
+	@Override
+	public void visitDstName(MappedElementKind targetKind, int namespace, String name) {
+		dstNames[namespace] = name;
+	}
+
+	@Override
+	public boolean visitElementContent(MappedElementKind targetKind) throws IOException {
+		for (int ns : alternativesMapping) {
+			int idx = ns + 1; // offset by 1 bc src ns is -1
+
+			if (targetKind == MappedElementKind.METHOD_ARG || targetKind == MappedElementKind.METHOD_VAR) {
+				unobf[idx] = lastMethodUnobf[idx];
+			} else if (ns == testNsId) {
+				unobf[idx] = true;
+
+				if (targetKind == MappedElementKind.METHOD) {
+					lastMethodUnobf[idx] = true;
+				}
+			} else if (!unobf[idx]) { // only check each ns once
+				String name = ns == -1 ? srcName : dstNames[ns];
+				String testName = dstNames[testNsId];
+
+				if (testName != null && testName.equals(name)) {
+					unobf[idx] = true;
+
+					if (targetKind == MappedElementKind.METHOD) {
+						lastMethodUnobf[idx] = true;
+					}
+				}
+			}
+		}
+
+		nsLoop: for (int i = 0; i < dstNames.length; i++) {
+			String name = dstNames[i];
+
+			if (name == null) {
+				int src = i;
+				long visited = 1L << src;
+
+				do {
+					int newSrc = alternativesMapping[src];
+
+					if (newSrc < 0) { // mapping to src name
+						if (unobf[newSrc + 1]) {
+							name = srcName;
+							break; // srcName must never be null
+						} else {
+							continue nsLoop;
+						}
+					} else if (newSrc == src) { // no-op (identity) mapping, explicit in case src > 64
+						continue nsLoop; // always null
+					} else if ((visited & 1L << newSrc) != 0) { // cyclic mapping
+						continue nsLoop; // always null
+					} else {
+						if (unobf[newSrc + 1]) {
+							src = newSrc;
+							name = dstNames[src];
+							visited |= 1L << src;
+						} else {
+							continue nsLoop;
+						}
+					}
+				} while (name == null);
+
+				assert name != null;
+			}
+
+			next.visitDstName(targetKind, i, name);
+		}
+
+		Arrays.fill(dstNames, null);
+		Arrays.fill(unobf, false);
+		Arrays.fill(lastMethodUnobf, false);
+
+		return next.visitElementContent(targetKind);
+	}
+}
diff --git a/src/main/java/net/fabricmc/loom/util/TinyRemapperHelper.java b/src/main/java/net/fabricmc/loom/util/TinyRemapperHelper.java
@@ -111,22 +111,51 @@ public static IMappingProvider create(MappingTree mappings, String from, String
 
 			for (MappingTree.ClassMapping classDef : mappings.getClasses()) {
 				String className = classDef.getName(fromId);
-				String dstName = classDef.getName(toId);
 
-				if (dstName == null) {
+				if (className == null) {
+					continue;
+				}
+
+				String dstClassName = classDef.getName(toId);
+
+				if (dstClassName == null) {
 					// Unsure if this is correct, should be better than crashing tho.
-					dstName = className;
+					dstClassName = className;
 				}
 
-				acceptor.acceptClass(className, dstName);
+				acceptor.acceptClass(className, dstClassName);
 
 				for (MappingTree.FieldMapping field : classDef.getFields()) {
-					acceptor.acceptField(memberOf(className, field.getName(fromId), field.getDesc(fromId)), field.getName(toId));
+					String fieldName = field.getName(fromId);
+
+					if (fieldName == null) {
+						continue;
+					}
+
+					String dstFieldName = field.getName(toId);
+
+					if (dstFieldName == null) {
+						dstFieldName = fieldName;
+					}
+
+					acceptor.acceptField(memberOf(className, fieldName, field.getDesc(fromId)), dstFieldName);
 				}
 
 				for (MappingTree.MethodMapping method : classDef.getMethods()) {
-					IMappingProvider.Member methodIdentifier = memberOf(className, method.getName(fromId), method.getDesc(fromId));
-					acceptor.acceptMethod(methodIdentifier, method.getName(toId));
+					String methodName = method.getName(fromId);
+
+					if (methodName == null) {
+						continue;
+					}
+
+					String dstMethodName = method.getName(toId);
+
+					if (dstMethodName == null) {
+						dstMethodName = methodName;
+					}
+
+					IMappingProvider.Member methodIdentifier = memberOf(className, methodName, method.getDesc(fromId));
+					acceptor.acceptMethod(methodIdentifier, dstMethodName);
 
 					if (remapLocalVariables) {
 						for (MappingTree.MethodArgMapping parameter : method.getArgs()) {
diff --git a/src/test/groovy/net/fabricmc/loom/test/unit/LoomMocks.groovy b/src/test/groovy/net/fabricmc/loom/test/unit/LoomMocks.groovy
@@ -24,9 +24,11 @@
 
 package net.fabricmc.loom.test.unit
 
+import java.nio.file.Path
 import java.util.function.Function
 
 import net.fabricmc.loom.configuration.providers.mappings.IntermediaryMappingsProvider
+import net.fabricmc.loom.configuration.providers.mappings.IntermediateMappingsService
 import net.fabricmc.loom.test.util.GradleTestUtil
 import net.fabricmc.loom.util.download.Download
 
@@ -49,4 +51,20 @@ class LoomMocks {
 		when(mock.getRefreshDeps()).thenReturn(refreshDeps)
 		return mock
 	}
+
+	static IntermediateMappingsService.Options intermediateMappingsServiceOptionsMock(Path intermediaryTiny, String expectedSrcNs) {
+		def intermediaryTinyProperty = GradleTestUtil.mockProperty(intermediaryTiny)
+		def expectedSrcNsProperty = GradleTestUtil.mockProperty(expectedSrcNs)
+
+		def mock = spy(IntermediateMappingsService.Options.class)
+		when(mock.getIntermediaryTiny()).thenReturn(intermediaryTinyProperty)
+		when(mock.getExpectedSrcNs()).thenReturn(expectedSrcNsProperty)
+		return mock
+	}
+
+	static IntermediateMappingsService intermediateMappingsServiceMock(IntermediateMappingsService.Options options) {
+		def mock = spy(IntermediateMappingsService.class)
+		when(mock.getOptions()).thenReturn(options)
+		return mock
+	}
 }
diff --git a/src/test/groovy/net/fabricmc/loom/test/unit/MappingsMergerTest.groovy b/src/test/groovy/net/fabricmc/loom/test/unit/MappingsMergerTest.groovy
