From 09803af340130b9d5f02154a9ecd188026a667a0 Mon Sep 17 00:00:00 2001 From: modmuss Date: Mon, 19 Aug 2024 17:37:54 +0100 Subject: [PATCH] Add checkMappings task (#3875) * Add task to check for duplicate names (cherry picked from commit c9756f7dcb7e0d20e42f97ca17bf9c31f0e89e22) * 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 Co-authored-by: NebelNidas --- build.gradle | 15 +- .../task/mappingio/CheckMappingsTask.java | 136 ++++++++++++++++++ .../mappingio/CheckMergedMappingsTask.java | 114 +++++++++++++++ 3 files changed, 264 insertions(+), 1 deletion(-) create mode 100644 filament/src/main/java/net/fabricmc/filament/task/mappingio/CheckMappingsTask.java create mode 100644 filament/src/main/java/net/fabricmc/filament/task/mappingio/CheckMergedMappingsTask.java diff --git a/build.gradle b/build.gradle index 4f164858a1..f5ff31d72e 100644 --- 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 new file mode 100644 index 0000000000..4072378c0c --- /dev/null +++ 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 errors = new ArrayList<>(); + + MappingReader.read(path, new MappingVisitor() { + private final Set classes = new HashSet<>(); + private final Set members = new HashSet<>(); + private final Set argLvIndices = new HashSet<>(); + private String clsSrcName; + private String memberSrcName; + private String memberId; + + @Override + public void visitNamespaces(String srcNamespace, List 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 new file mode 100644 index 0000000000..e60b3a5d15 --- /dev/null +++ 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 errors = new ArrayList<>(); + + MappingReader.read(path, new MappingVisitor() { + private final Set mthDstNames = new HashSet<>(); + private String clsSrcName; + private String mthSrcName; + private String mthSrcDesc; + + @Override + public void visitNamespaces(String srcNamespace, List 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"); + } +}