Add checkMappings task (#3875)

* Add task to check for duplicate names (cherry picked from commit c9756f7) * Fixes * Use custom visitor instead of an intermediate tree, check for more duplicate names * Address review * Reimplement previous check for mappings targeting non-existent elements * Fix comment * Fix typo * Fix import order --------- Co-authored-by: Shnupbups <[email protected]> Co-authored-by: NebelNidas <[email protected]>
FabricMC · Aug 19, 2024 · 09803af · 09803af
1 parent 889bf3f
commit 09803af
Show file tree

Hide file tree

Showing 3 changed files with 264 additions and 1 deletion.
diff --git a/build.gradle b/build.gradle
@@ -104,6 +104,8 @@ import net.fabricmc.filament.task.mappingio.CompleteMappingsTask
 import net.fabricmc.filament.task.mappingio.ConvertMappingsTask
 import net.fabricmc.filament.task.mappingio.FormatMappingsTask
 import net.fabricmc.filament.task.mappingio.MergeMappingsTask
+import net.fabricmc.filament.task.mappingio.CheckMappingsTask
+import net.fabricmc.filament.task.mappingio.CheckMergedMappingsTask
 import net.fabricmc.filament.nameproposal.MappingNameCompleter
 import net.fabricmc.mappingio.format.MappingFormat
 import org.gradle.work.DisableCachingByDefault
@@ -153,7 +155,7 @@ tasks.register('yarnCommon', EnigmaTask) {
 	mappings = mappingsDir
 }
 
-tasks.register('checkMappings') {
+def checkPackages = tasks.register('checkPackages') {
 	group = buildMappingGroup
 	inputs.dir mappingsDir
 	inputs.file mapIntermediaryJar.output
@@ -554,6 +556,17 @@ javadocLint {
 	mappingDirectory = file("mappings")
 }
 
+def checkMappings = tasks.register("checkMappings", CheckMappingsTask) {
+	input = mappingsDir
+}
+
+def checkMergedMappings = tasks.register("checkMergedMappings", CheckMergedMappingsTask) {
+	input = mergeV2.output
+}
+
+check.dependsOn checkPackages
+check.dependsOn checkMappings
+check.dependsOn checkMergedMappings
 check.dependsOn javadocLint
 
 publishing {

diff --git a/filament/src/main/java/net/fabricmc/filament/task/mappingio/CheckMappingsTask.java b/filament/src/main/java/net/fabricmc/filament/task/mappingio/CheckMappingsTask.java
@@ -0,0 +1,136 @@
+package net.fabricmc.filament.task.mappingio;
+
+import java.io.IOException;
+import java.nio.file.Path;
+import java.util.ArrayList;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+
+import org.gradle.api.file.DirectoryProperty;
+import org.gradle.api.tasks.InputDirectory;
+import org.gradle.api.tasks.TaskAction;
+import org.jetbrains.annotations.Nullable;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import net.fabricmc.mappingio.MappedElementKind;
+import net.fabricmc.mappingio.MappingVisitor;
+import net.fabricmc.mappingio.MappingReader;
+import net.fabricmc.filament.task.base.FilamentTask;
+
+public abstract class CheckMappingsTask extends FilamentTask {
+	private static final Logger LOGGER = LoggerFactory.getLogger(CheckMappingsTask.class);
+
+	@InputDirectory
+	abstract DirectoryProperty getInput();
+
+	@TaskAction
+	public final void run() throws IOException {
+		Path path = getInput().get().getAsFile().toPath();
+		List<String> errors = new ArrayList<>();
+
+		MappingReader.read(path, new MappingVisitor() {
+			private final Set<String> classes = new HashSet<>();
+			private final Set<String> members = new HashSet<>();
+			private final Set<Integer> argLvIndices = new HashSet<>();
+			private String clsSrcName;
+			private String memberSrcName;
+			private String memberId;
+
+			@Override
+			public void visitNamespaces(String srcNamespace, List<String> dstNamespaces) throws IOException {
+			}
+
+			@Override
+			public boolean visitClass(String srcName) throws IOException {
+				if (classes.contains(srcName)) {
+					errors.add("Duplicate class mapping for " + srcName);
+				} else {
+					classes.add(srcName);
+				}
+
+				members.clear();
+				clsSrcName = srcName;
+				return true;
+			}
+
+			@Override
+			public boolean visitField(String srcName, @Nullable String srcDesc) throws IOException {
+				memberId = srcName + ":" + srcDesc;
+
+				if (members.contains(memberId)) {
+					errors.add("Duplicate field mapping for " + clsSrcName + "#" + memberId);
+				} else {
+					members.add(memberId);
+				}
+
+				memberSrcName = srcName;
+				return true;
+			}
+
+			@Override
+			public boolean visitMethod(String srcName, @Nullable String srcDesc) throws IOException {
+				memberId = srcName + srcDesc;
+
+				if (members.contains(memberId)) {
+					errors.add("Duplicate method mapping for " + clsSrcName + "#" + memberId);
+				} else {
+					members.add(memberId);
+				}
+
+				argLvIndices.clear();
+				memberSrcName = srcName;
+				return true;
+			}
+
+			@Override
+			public boolean visitMethodArg(int argPosition, int lvIndex, @Nullable String srcName) throws IOException {
+				if (argLvIndices.contains(lvIndex)) {
+					errors.add("Duplicate parameter mapping for " + clsSrcName + "#" + memberId + ", slot " + lvIndex);
+				}
+
+				argLvIndices.add(lvIndex);
+				return true;
+			}
+
+			@Override
+			public boolean visitMethodVar(int lvtRowIndex, int lvIndex, int startOpIdx, int endOpIdx, @Nullable String srcName) throws IOException {
+				throw new UnsupportedOperationException();
+			}
+
+			@Override
+			public void visitDstName(MappedElementKind targetKind, int namespace, String name) throws IOException {
+				switch (targetKind) {
+					case CLASS:
+						if (clsSrcName.equals(name)) {
+							errors.add("Same name in both namespaces for class " + clsSrcName);
+						}
+
+						break;
+					case FIELD:
+					case METHOD:
+						if (memberSrcName.equals(name) && (name.startsWith("method_") || name.startsWith("field_"))) {
+							errors.add("Same name in both namespaces for member " + clsSrcName + "#" + memberId);
+						}
+
+						break;
+				}
+			}
+
+			@Override
+			public void visitComment(MappedElementKind mappedElementKind, String comment) throws IOException {
+			}
+		});
+
+		if (errors.isEmpty()) {
+			return;
+		}
+
+		for (String error : errors) {
+			LOGGER.error(error);
+		}
+
+		throw new RuntimeException("Duplicate mappings detected");
+	}
+}
diff --git a/filament/src/main/java/net/fabricmc/filament/task/mappingio/CheckMergedMappingsTask.java b/filament/src/main/java/net/fabricmc/filament/task/mappingio/CheckMergedMappingsTask.java
@@ -0,0 +1,114 @@
+package net.fabricmc.filament.task.mappingio;
+
+import java.io.IOException;
+import java.nio.file.Path;
+import java.util.ArrayList;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+
+import org.gradle.api.tasks.TaskAction;
+import org.jetbrains.annotations.Nullable;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import net.fabricmc.filament.task.base.FilamentTask;
+import net.fabricmc.filament.task.base.WithFileInput;
+import net.fabricmc.mappingio.MappedElementKind;
+import net.fabricmc.mappingio.MappingReader;
+import net.fabricmc.mappingio.MappingVisitor;
+
+public abstract class CheckMergedMappingsTask extends FilamentTask implements WithFileInput {
+	private static final Logger LOGGER = LoggerFactory.getLogger(CheckMergedMappingsTask.class);
+
+	@TaskAction
+	public final void run() throws IOException {
+		Path path = getInput().get().getAsFile().toPath();
+		List<String> errors = new ArrayList<>();
+
+		MappingReader.read(path, new MappingVisitor() {
+			private final Set<String> mthDstNames = new HashSet<>();
+			private String clsSrcName;
+			private String mthSrcName;
+			private String mthSrcDesc;
+
+			@Override
+			public void visitNamespaces(String srcNamespace, List<String> dstNamespaces) throws IOException {
+			}
+
+			@Override
+			public boolean visitClass(String srcName) throws IOException {
+				if (srcName.startsWith("net/minecraft/class_")) {
+					errors.add("Encountered mapping for non-existent class " + srcName);
+				}
+
+				clsSrcName = srcName;
+				return true;
+			}
+
+			@Override
+			public boolean visitField(String srcName, @Nullable String srcDesc) throws IOException {
+				if (srcName.startsWith("field_")) {
+					errors.add("Encountered mapping for non-existent field " + clsSrcName + "#" + srcName + ":" + srcDesc);
+				}
+
+				return true;
+			}
+
+			@Override
+			public boolean visitMethod(String srcName, @Nullable String srcDesc) throws IOException {
+				mthSrcName = srcName;
+				mthSrcDesc = srcDesc;
+				mthDstNames.clear();
+				return true;
+			}
+
+			@Override
+			public boolean visitMethodArg(int argPosition, int lvIndex, @Nullable String srcName) throws IOException {
+				return true;
+			}
+
+			@Override
+			public boolean visitMethodVar(int lvtRowIndex, int lvIndex, int startOpIdx, int endOpIdx, @Nullable String srcName) throws IOException {
+				throw new UnsupportedOperationException();
+			}
+
+			@Override
+			public void visitDstName(MappedElementKind targetKind, int namespace, String name) throws IOException {
+				if (targetKind == MappedElementKind.METHOD) {
+					mthDstNames.add(name);
+				}
+			}
+
+			@Override
+			public boolean visitElementContent(MappedElementKind targetKind) throws IOException {
+				if (targetKind == MappedElementKind.METHOD) {
+					// Checking if the srcName is an intermediary name (like for classes and fields) would be the more correct option,
+					// but Enigma's bridge method mapper behaves weirdly and injects copies of the mapping into the whole hierarchy.
+					// We haven't been able to fix this yet, so in the meantime, we're using the following workaround,
+					// which ignores any Enigma-bridge-mapper generated mappings, but should still catch all other
+					// mappings without official names.
+					if (mthDstNames.size() == 1 && mthDstNames.contains(mthSrcName) && mthSrcName.startsWith("method_")) {
+						errors.add("Encountered mapping for non-existent method " + clsSrcName + "#" + mthSrcName + mthSrcDesc);
+					}
+				}
+
+				return true;
+			}
+
+			@Override
+			public void visitComment(MappedElementKind mappedElementKind, String comment) throws IOException {
+			}
+		});
+
+		if (errors.isEmpty()) {
+			return;
+		}
+
+		for (String error : errors) {
+			LOGGER.error(error);
+		}
+
+		throw new RuntimeException("Mappings for non-existent elements detected");
+	}
+}