Cleanup signature fixer code + add basic integration test. Fixes #546

src/main/java/net/fabricmc/loom/configuration/providers/minecraft/MinecraftMappedProvider.java

@@ -29,15 +29,10 @@ import java.io.IOException;
 import java.nio.file.Files;
 import java.nio.file.Path;
 import java.util.Arrays;
-import java.util.HashMap;
 import java.util.Map;
-import java.util.Objects;
-import java.util.concurrent.atomic.AtomicReference;
 import java.util.function.Consumer;
 
 import org.gradle.api.Project;
-import org.objectweb.asm.ClassVisitor;
-import org.objectweb.asm.commons.Remapper;
 
 import net.fabricmc.loom.api.mappings.layered.MappingsNamespace;
 import net.fabricmc.loom.configuration.DependencyProvider;
@@ -47,7 +42,6 @@ import net.fabricmc.loom.util.Constants;
 import net.fabricmc.loom.util.TinyRemapperHelper;
 import net.fabricmc.tinyremapper.OutputConsumerPath;
 import net.fabricmc.tinyremapper.TinyRemapper;
-import net.fabricmc.tinyremapper.api.TrClass;
 
 public class MinecraftMappedProvider extends DependencyProvider {
 	private File minecraftMappedJar;
@@ -110,65 +104,16 @@ public class MinecraftMappedProvider extends DependencyProvider {
 		for (String toM : Arrays.asList(MappingsNamespace.NAMED.toString(), MappingsNamespace.INTERMEDIARY.toString())) {
 			final boolean toNamed = MappingsNamespace.NAMED.toString().equals(toM);
 			final boolean toIntermediary = MappingsNamespace.INTERMEDIARY.toString().equals(toM);
-			final boolean fixSignatures = mappingsProvider.getSignatureFixes() != null;
 			final Path output = toNamed ? outputMapped : outputIntermediary;
 
-			getProject().getLogger().lifecycle(":remapping minecraft (TinyRemapper, " + fromM + " -> " + toM + ")");
+			getProject().getLogger().info(":remapping minecraft (TinyRemapper, " + fromM + " -> " + toM + ")");
 
 			Files.deleteIfExists(output);
 
-			// Bit ugly but whatever, the whole issue is a bit ugly :)
+			final Map<String, String> remappedSignatures = SignatureFixerApplyVisitor.getRemappedSignatures(toIntermediary, mappingsProvider, getProject(), toM);
-			AtomicReference<Map<String, String>> remappedSignatures = new AtomicReference<>();
 			TinyRemapper remapper = TinyRemapperHelper.getTinyRemapper(getProject(), fromM, toM, true, (builder) -> {
-				if (!fixSignatures) {
+				builder.extraPostApplyVisitor(new SignatureFixerApplyVisitor(remappedSignatures));
-					return;
-				}
-
-				builder.extraPostApplyVisitor(new TinyRemapper.ApplyVisitorProvider() {
-					@Override
-					public ClassVisitor insertApplyVisitor(TrClass cls, ClassVisitor next) {
-						return new ClassVisitor(Constants.ASM_VERSION, next) {
-							@Override
-							public void visit(int version, int access, String name, String signature, String superName, String[] interfaces) {
-								Map<String, String> signatureFixes = Objects.requireNonNull(remappedSignatures.get(), "Could not get remapped signatures");
-
-								if (signature == null) {
-									signature = signatureFixes.getOrDefault(name, null);
-
-									if (signature != null) {
-										getProject().getLogger().info("Replaced signature for {} with {}", name, signature);
-									}
-								}
-
-								super.visit(version, access, name, signature, superName, interfaces);
-							}
-						};
-					}
 			});
-			});
-
-			if (fixSignatures) {
-				if (toIntermediary) {
-					// No need to remap, as these are already intermediary
-					remappedSignatures.set(mappingsProvider.getSignatureFixes());
-				} else {
-					// Remap the sig fixes from intermediary to the target namespace
-					final Map<String, String> remapped = new HashMap<>();
-					final TinyRemapper sigTinyRemapper = TinyRemapperHelper.getTinyRemapper(getProject(), MappingsNamespace.INTERMEDIARY.toString(), toM);
-					final Remapper sigAsmRemapper = sigTinyRemapper.getEnvironment().getRemapper();
-
-					// Remap the class names and the signatures using a new tiny remapper instance.
-					for (Map.Entry<String, String> entry : mappingsProvider.getSignatureFixes().entrySet()) {
-						remapped.put(
-								sigAsmRemapper.map(entry.getKey()),
-								sigAsmRemapper.mapSignature(entry.getValue(), false)
-						);
-					}
-
-					sigTinyRemapper.finish();
-					remappedSignatures.set(remapped);
-				}
-			}
 
 			try (OutputConsumerPath outputConsumer = new OutputConsumerPath.Builder(output).build()) {
 				outputConsumer.addNonClassFiles(input);

src/main/java/net/fabricmc/loom/configuration/providers/minecraft/SignatureFixerApplyVisitor.java

@@ -0,0 +1,85 @@
+/*
+ * This file is part of fabric-loom, licensed under the MIT License (MIT).
+ *
+ * Copyright (c) 2021 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.minecraft;
+
+import java.io.IOException;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.Map;
+
+import org.gradle.api.Project;
+import org.objectweb.asm.ClassVisitor;
+import org.objectweb.asm.commons.Remapper;
+
+import net.fabricmc.loom.api.mappings.layered.MappingsNamespace;
+import net.fabricmc.loom.configuration.providers.mappings.MappingsProviderImpl;
+import net.fabricmc.loom.util.Constants;
+import net.fabricmc.loom.util.TinyRemapperHelper;
+import net.fabricmc.tinyremapper.TinyRemapper;
+import net.fabricmc.tinyremapper.api.TrClass;
+
+public record SignatureFixerApplyVisitor(Map<String, String> signatureFixes) implements TinyRemapper.ApplyVisitorProvider {
+	@Override
+	public ClassVisitor insertApplyVisitor(TrClass cls, ClassVisitor next) {
+		return new ClassVisitor(Constants.ASM_VERSION, next) {
+			@Override
+			public void visit(int version, int access, String name, String signature, String superName, String[] interfaces) {
+				if (signature == null) {
+					signature = signatureFixes.getOrDefault(name, null);
+				}
+
+				super.visit(version, access, name, signature, superName, interfaces);
+			}
+		};
+	}
+
+	public static Map<String, String> getRemappedSignatures(boolean toIntermediary, MappingsProviderImpl mappingsProvider, Project project, String targetNamespace) throws IOException {
+		if (mappingsProvider.getSignatureFixes() == null) {
+			// No fixes
+			return Collections.emptyMap();
+		}
+
+		if (toIntermediary) {
+			// No need to remap, as these are already intermediary
+			return mappingsProvider.getSignatureFixes();
+		}
+
+		// Remap the sig fixes from intermediary to the target namespace
+		final Map<String, String> remapped = new HashMap<>();
+		final TinyRemapper sigTinyRemapper = TinyRemapperHelper.getTinyRemapper(project, MappingsNamespace.INTERMEDIARY.toString(), targetNamespace);
+		final Remapper sigAsmRemapper = sigTinyRemapper.getEnvironment().getRemapper();
+
+		// Remap the class names and the signatures using a new tiny remapper instance.
+		for (Map.Entry<String, String> entry : mappingsProvider.getSignatureFixes().entrySet()) {
+			remapped.put(
+					sigAsmRemapper.map(entry.getKey()),
+					sigAsmRemapper.mapSignature(entry.getValue(), false)
+			);
+		}
+
+		sigTinyRemapper.finish();
+		return remapped;
+	}
+}

src/test/groovy/net/fabricmc/loom/test/integration/SignatureFixesTest.groovy

@@ -0,0 +1,57 @@
+/*
+ * This file is part of fabric-loom, licensed under the MIT License (MIT).
+ *
+ * Copyright (c) 2021 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.test.integration
+
+import net.fabricmc.loom.test.util.GradleProjectTestTrait
+import spock.lang.Specification
+import spock.lang.Unroll
+
+import static net.fabricmc.loom.test.LoomTestConstants.STANDARD_TEST_VERSIONS
+import static org.gradle.testkit.runner.TaskOutcome.SUCCESS
+
+// Basic test to just ensure it builds.
+class SignatureFixesTest extends Specification implements GradleProjectTestTrait {
+	@Unroll
+	def "build (gradle #version)"() {
+		setup:
+			def gradle = gradleProject(project: "minimalBase", version: version)
+
+			gradle.buildGradle << '''
+                dependencies {
+                    minecraft "com.mojang:minecraft:1.18-pre1"
+                    mappings "net.fabricmc:yarn:1.18-pre1+build.14:v2"
+                }
+            '''
+
+		when:
+			def result = gradle.run(task: "build")
+
+		then:
+			result.task(":build").outcome == SUCCESS
+
+		where:
+			version << STANDARD_TEST_VERSIONS
+	}
+}