From ba80b7eca18222bbbf8f9b82fe64c42b74c36348 Mon Sep 17 00:00:00 2001 From: ix0rai Date: Wed, 16 Oct 2024 11:11:24 -0500 Subject: [PATCH] better logging and smarter behaviour for drop invalid mappings (#233) * improve logging for drop invalid mappings * add a simple test for drop invalid mappings * fix empty mappings test * fix drop invalid mappings test * remove debug print * add another out of bounds parameter * write some docs * suggestions! Co-authored-by: Iota <47987888+IotaBread@users.noreply.github.com> * fix method name * update description * add a test to run the help command --------- Co-authored-by: Iota <47987888+IotaBread@users.noreply.github.com> --- .../command/DropInvalidMappingsCommand.java | 55 ++++++++++--------- .../command/DropInvalidMappingsTest.java | 42 ++++++++++++++ .../enigma/command/HelpCommandTest.java | 11 ++++ .../expected/EmptyMappings.mapping | 0 .../expected/InvalidMappings.mapping | 5 ++ .../input/EmptyMappings.mapping | 5 ++ .../input/InvalidMappings.mapping | 12 ++++ .../org/quiltmc/enigma/api/EnigmaProject.java | 12 ++-- .../api/analysis/index/jar/EntryIndex.java | 44 ++++++++++++++- .../translation/mapping/MappingsChecker.java | 37 ++++++++++--- 10 files changed, 182 insertions(+), 41 deletions(-) create mode 100644 enigma-cli/src/test/java/org/quiltmc/enigma/command/DropInvalidMappingsTest.java create mode 100644 enigma-cli/src/test/java/org/quiltmc/enigma/command/HelpCommandTest.java create mode 100644 enigma-cli/src/test/resources/drop_invalid_mappings/expected/EmptyMappings.mapping create mode 100644 enigma-cli/src/test/resources/drop_invalid_mappings/expected/InvalidMappings.mapping create mode 100644 enigma-cli/src/test/resources/drop_invalid_mappings/input/EmptyMappings.mapping create mode 100644 enigma-cli/src/test/resources/drop_invalid_mappings/input/InvalidMappings.mapping diff --git a/enigma-cli/src/main/java/org/quiltmc/enigma/command/DropInvalidMappingsCommand.java b/enigma-cli/src/main/java/org/quiltmc/enigma/command/DropInvalidMappingsCommand.java index 496e3049c..fabe1b11d 100644 --- a/enigma-cli/src/main/java/org/quiltmc/enigma/command/DropInvalidMappingsCommand.java +++ b/enigma-cli/src/main/java/org/quiltmc/enigma/command/DropInvalidMappingsCommand.java @@ -37,7 +37,7 @@ public String getName() { @Override public String getDescription() { - return "Removes all invalid mapping entries (entries whose obfuscated name is not found in the jar) from the provided mappings."; + return "Removes all invalid mapping entries (entries whose obfuscated name is not found in the jar) and empty mappings (garbage lines that don't add anything to the mappings) from the provided mappings."; } public static void run(Path jarIn, Path mappingsIn, Path mappingsOut) throws Exception { @@ -51,30 +51,35 @@ public static void run(Path jarIn, Path mappingsIn, Path mappingsOut) throws Exc Logger.info("Dropping invalid mappings..."); - project.dropMappings(ProgressListener.createEmpty()); - - Logger.info("Writing mappings..."); - - if (mappingsOut == mappingsIn) { - Logger.info("Overwriting input mappings"); - Files.walkFileTree(mappingsIn, new SimpleFileVisitor<>() { - @Override - public FileVisitResult postVisitDirectory(Path dir, IOException exc) throws IOException { - Files.delete(dir); - return FileVisitResult.CONTINUE; - } - - @Override - public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) throws IOException { - Files.delete(file); - return FileVisitResult.CONTINUE; - } - }); - - Files.deleteIfExists(mappingsIn); + var droppedMappings = project.dropMappings(ProgressListener.createEmpty()); + + if (!droppedMappings.isEmpty()) { + Logger.info("Found and dropped {} invalid mappings.", droppedMappings.size()); + Logger.info("Writing mappings..."); + + if (mappingsOut == mappingsIn) { + Logger.info("Overwriting input mappings"); + Files.walkFileTree(mappingsIn, new SimpleFileVisitor<>() { + @Override + public FileVisitResult postVisitDirectory(Path dir, IOException exc) throws IOException { + Files.delete(dir); + return FileVisitResult.CONTINUE; + } + + @Override + public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) throws IOException { + Files.delete(file); + return FileVisitResult.CONTINUE; + } + }); + + Files.deleteIfExists(mappingsIn); + } + + MappingSaveParameters saveParameters = project.getEnigma().getProfile().getMappingSaveParameters(); + writer.write(project.getRemapper().getMappings(), mappingsOut, ProgressListener.createEmpty(), saveParameters); + } else { + Logger.info("No invalid mappings found."); } - - MappingSaveParameters saveParameters = project.getEnigma().getProfile().getMappingSaveParameters(); - writer.write(project.getRemapper().getMappings(), mappingsOut, ProgressListener.createEmpty(), saveParameters); } } diff --git a/enigma-cli/src/test/java/org/quiltmc/enigma/command/DropInvalidMappingsTest.java b/enigma-cli/src/test/java/org/quiltmc/enigma/command/DropInvalidMappingsTest.java new file mode 100644 index 000000000..cc2dbc129 --- /dev/null +++ b/enigma-cli/src/test/java/org/quiltmc/enigma/command/DropInvalidMappingsTest.java @@ -0,0 +1,42 @@ +package org.quiltmc.enigma.command; + +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; +import org.quiltmc.enigma.TestUtil; + +import java.nio.file.Files; +import java.nio.file.Path; + +public class DropInvalidMappingsTest extends CommandTest { + private static final Path JAR = TestUtil.obfJar("lone_class"); + private static final Path INPUT_DIR = getResource("/drop_invalid_mappings/input/"); + private static final Path EXPECTED_DIR = getResource("/drop_invalid_mappings/expected/"); + private static final Path INVALID_MAPPINGS_INPUT = INPUT_DIR.resolve("InvalidMappings.mapping"); + private static final Path INVALID_MAPPINGS_EXPECTED = EXPECTED_DIR.resolve("InvalidMappings.mapping"); + private static final Path EMPTY_MAPPINGS_INPUT = INPUT_DIR.resolve("EmptyMappings.mapping"); + private static final Path EMPTY_MAPPINGS_EXPECTED = EXPECTED_DIR.resolve("EmptyMappings.mapping"); + + @Test + public void testInvalidMappings() throws Exception { + Path resultFile = Files.createTempFile("invalidMappingsResult", ".mapping"); + + DropInvalidMappingsCommand.run(JAR, INVALID_MAPPINGS_INPUT, resultFile); + + String expectedLines = Files.readString(INVALID_MAPPINGS_EXPECTED); + String actualLines = Files.readString(resultFile); + + Assertions.assertEquals(expectedLines, actualLines); + } + + @Test + public void testEmptyMappings() throws Exception { + Path resultFile = Files.createTempFile("emptyMappingsResult", ".mapping"); + + DropInvalidMappingsCommand.run(JAR, EMPTY_MAPPINGS_INPUT, resultFile); + + String expectedLines = Files.readString(EMPTY_MAPPINGS_EXPECTED); + String actualLines = Files.readString(resultFile); + + Assertions.assertEquals(expectedLines, actualLines); + } +} diff --git a/enigma-cli/src/test/java/org/quiltmc/enigma/command/HelpCommandTest.java b/enigma-cli/src/test/java/org/quiltmc/enigma/command/HelpCommandTest.java new file mode 100644 index 000000000..96b2f1beb --- /dev/null +++ b/enigma-cli/src/test/java/org/quiltmc/enigma/command/HelpCommandTest.java @@ -0,0 +1,11 @@ +package org.quiltmc.enigma.command; + +import org.junit.jupiter.api.Test; + +public class HelpCommandTest { + @Test + void test() throws Exception { + // for manually verifying output + new HelpCommand().run(); + } +} diff --git a/enigma-cli/src/test/resources/drop_invalid_mappings/expected/EmptyMappings.mapping b/enigma-cli/src/test/resources/drop_invalid_mappings/expected/EmptyMappings.mapping new file mode 100644 index 000000000..e69de29bb diff --git a/enigma-cli/src/test/resources/drop_invalid_mappings/expected/InvalidMappings.mapping b/enigma-cli/src/test/resources/drop_invalid_mappings/expected/InvalidMappings.mapping new file mode 100644 index 000000000..1877d7c78 --- /dev/null +++ b/enigma-cli/src/test/resources/drop_invalid_mappings/expected/InvalidMappings.mapping @@ -0,0 +1,5 @@ +CLASS a InvalidMappings + FIELD a slayField Ljava/lang/String; + METHOD (Ljava/lang/String;)V + ARG 1 coolParameter + METHOD a slayMethod ()Ljava/lang/String; diff --git a/enigma-cli/src/test/resources/drop_invalid_mappings/input/EmptyMappings.mapping b/enigma-cli/src/test/resources/drop_invalid_mappings/input/EmptyMappings.mapping new file mode 100644 index 000000000..91bc828b5 --- /dev/null +++ b/enigma-cli/src/test/resources/drop_invalid_mappings/input/EmptyMappings.mapping @@ -0,0 +1,5 @@ +CLASS a + FIELD a Ljava/lang/String; + METHOD (Ljava/lang/String;)V + ARG 1 + METHOD a ()Ljava/lang/String; diff --git a/enigma-cli/src/test/resources/drop_invalid_mappings/input/InvalidMappings.mapping b/enigma-cli/src/test/resources/drop_invalid_mappings/input/InvalidMappings.mapping new file mode 100644 index 000000000..532c59349 --- /dev/null +++ b/enigma-cli/src/test/resources/drop_invalid_mappings/input/InvalidMappings.mapping @@ -0,0 +1,12 @@ +CLASS a InvalidMappings + FIELD a slayField Ljava/lang/String; + FIELD b fakeField I + METHOD (Ljava/lang/String;)V + ARG 0 outOfBoundsParameter1 + ARG 1 coolParameter + ARG 2 outOfBoundsParameter2 + ARG 3 outOfBoundsParameter3 + METHOD a slayMethod ()Ljava/lang/String; + ARG 1 fakeParameter + METHOD b fakeMethod ()V + CLASS b NonExistentClass diff --git a/enigma/src/main/java/org/quiltmc/enigma/api/EnigmaProject.java b/enigma/src/main/java/org/quiltmc/enigma/api/EnigmaProject.java index 4d1384e64..67e7b4989 100644 --- a/enigma/src/main/java/org/quiltmc/enigma/api/EnigmaProject.java +++ b/enigma/src/main/java/org/quiltmc/enigma/api/EnigmaProject.java @@ -131,18 +131,20 @@ public EntryRemapper getRemapper() { return this.remapper; } - public void dropMappings(ProgressListener progress) { + public Collection> dropMappings(ProgressListener progress) { DeltaTrackingTree mappings = this.remapper.getMappings(); Collection> dropped = this.dropMappings(mappings, progress); for (Entry entry : dropped) { mappings.trackChange(entry); } + + return dropped; } private Collection> dropMappings(EntryTree mappings, ProgressListener progress) { // drop mappings that don't match the jar - MappingsChecker checker = new MappingsChecker(this.jarIndex, mappings); + MappingsChecker checker = new MappingsChecker(this, this.jarIndex, mappings); MappingsChecker.Dropped droppedBroken = checker.dropBrokenMappings(progress); Map, String> droppedBrokenMappings = droppedBroken.getDroppedMappings(); @@ -168,7 +170,7 @@ public boolean isNavigable(Entry obfEntry) { return false; } - return this.jarIndex.getIndex(EntryIndex.class).hasEntry(obfEntry); + return this.jarIndex.getIndex(EntryIndex.class).hasEntry(obfEntry, this); } public boolean isRenamable(Entry obfEntry) { @@ -209,7 +211,7 @@ public boolean isRenamable(Entry obfEntry) { return false; } - return this.jarIndex.getIndex(EntryIndex.class).hasEntry(obfEntry); + return this.jarIndex.getIndex(EntryIndex.class).hasEntry(obfEntry, this); } private static boolean isEnumValueOfMethod(ClassDefEntry parent, MethodEntry method) { @@ -235,7 +237,7 @@ public boolean isObfuscated(Entry entry) { } public boolean isSynthetic(Entry entry) { - return this.jarIndex.getIndex(EntryIndex.class).hasEntry(entry) && this.jarIndex.getIndex(EntryIndex.class).getEntryAccess(entry).isSynthetic(); + return this.jarIndex.getIndex(EntryIndex.class).hasEntry(entry, this) && this.jarIndex.getIndex(EntryIndex.class).getEntryAccess(entry).isSynthetic(); } public boolean isAnonymousOrLocal(ClassEntry classEntry) { diff --git a/enigma/src/main/java/org/quiltmc/enigma/api/analysis/index/jar/EntryIndex.java b/enigma/src/main/java/org/quiltmc/enigma/api/analysis/index/jar/EntryIndex.java index 7fb4e14b3..817507ce6 100644 --- a/enigma/src/main/java/org/quiltmc/enigma/api/analysis/index/jar/EntryIndex.java +++ b/enigma/src/main/java/org/quiltmc/enigma/api/analysis/index/jar/EntryIndex.java @@ -1,5 +1,7 @@ package org.quiltmc.enigma.api.analysis.index.jar; +import org.objectweb.asm.tree.ClassNode; +import org.quiltmc.enigma.api.EnigmaProject; import org.quiltmc.enigma.api.translation.mapping.EntryMapping; import org.quiltmc.enigma.api.translation.mapping.tree.EntryTree; import org.quiltmc.enigma.api.translation.mapping.tree.HashEntryTree; @@ -17,6 +19,7 @@ import java.util.Collection; import java.util.HashMap; import java.util.Map; +import java.util.concurrent.atomic.AtomicInteger; public class EntryIndex implements JarIndexer { private final EntryTree tree = new HashEntryTree<>(); @@ -67,7 +70,29 @@ public boolean hasField(FieldEntry entry) { return this.fieldDefinitions.containsKey(entry); } + /** + * Checks whether the entry has been indexed and therefore exists in the JAR file. + *
+ * Parameters are not indexed, and this method does not fully verify validity of parameter indices. + * Therefore, it is possible that this method returns {@code true} for an invalid parameter. + * @param entry the entry to check + * @return whether the entry exists + * @see #hasEntry(Entry, EnigmaProject) + */ public boolean hasEntry(Entry entry) { + return this.hasEntry(entry, null); + } + + /** + * Checks whether the entry has been indexed and therefore exists in the JAR file. + *
+ * For parameters, which are not indexed, verifies that they have a valid index and therefore could exist. + * @param entry the entry to check + * @param project the current project + * @return whether the entry exists + */ + @SuppressWarnings("ConstantConditions") + public boolean hasEntry(Entry entry, @Nullable EnigmaProject project) { if (entry instanceof ClassEntry classEntry) { return this.hasClass(classEntry); } else if (entry instanceof MethodEntry methodEntry) { @@ -77,10 +102,25 @@ public boolean hasEntry(Entry entry) { } else if (entry instanceof LocalVariableEntry localVariableEntry) { MethodEntry parent = localVariableEntry.getParent(); if (this.hasMethod(parent)) { - // TODO: Check using max_locals from the Code attribute (JVMS§4.7.3) + AtomicInteger maxLocals = new AtomicInteger(-1); + ClassEntry parentClass = parent != null ? parent.getParent() : null; + + if (project != null) { + // find max_locals for method, representing the number of parameters it receives (JVMS§4.7.3) + // note: parent class cannot be null, warning suppressed + ClassNode classNode = project.getClassProvider().get(parentClass.getFullName()); + if (classNode != null) { + classNode.methods.stream() + .filter(node -> node.name.equals(parent.getName()) && node.desc.equals(parent.getDesc().toString())) + .findFirst().ifPresent(node -> maxLocals.set(node.maxLocals)); + } + } + AccessFlags parentAccess = this.getMethodAccess(parent); int startIndex = parentAccess != null && parentAccess.isStatic() ? 0 : 1; - return localVariableEntry.getIndex() >= startIndex; + + // if maxLocals is -1 it's not found for the method and should be ignored + return localVariableEntry.getIndex() >= startIndex && (maxLocals.get() == -1 || localVariableEntry.getIndex() <= maxLocals.get() - 1); } } diff --git a/enigma/src/main/java/org/quiltmc/enigma/impl/translation/mapping/MappingsChecker.java b/enigma/src/main/java/org/quiltmc/enigma/impl/translation/mapping/MappingsChecker.java index 5fe858726..c1e9d1893 100644 --- a/enigma/src/main/java/org/quiltmc/enigma/impl/translation/mapping/MappingsChecker.java +++ b/enigma/src/main/java/org/quiltmc/enigma/impl/translation/mapping/MappingsChecker.java @@ -1,5 +1,6 @@ package org.quiltmc.enigma.impl.translation.mapping; +import org.quiltmc.enigma.api.EnigmaProject; import org.quiltmc.enigma.api.ProgressListener; import org.quiltmc.enigma.api.analysis.index.jar.EntryIndex; import org.quiltmc.enigma.api.analysis.index.jar.JarIndex; @@ -18,10 +19,12 @@ import java.util.stream.StreamSupport; public class MappingsChecker { + private final EnigmaProject project; private final JarIndex index; private final EntryTree mappings; - public MappingsChecker(JarIndex index, EntryTree mappings) { + public MappingsChecker(EnigmaProject project, JarIndex index, EntryTree mappings) { + this.project = project; this.index = index; this.mappings = mappings; } @@ -50,7 +53,7 @@ public Dropped dropBrokenMappings(ProgressListener progress) { } private void tryDropBrokenEntry(Dropped dropped, Entry entry) { - if (this.shouldDropBrokenEntry(entry)) { + if (this.shouldDropBrokenEntry(dropped, entry)) { EntryMapping mapping = this.mappings.get(entry); if (mapping != null) { dropped.drop(entry, mapping); @@ -58,8 +61,8 @@ private void tryDropBrokenEntry(Dropped dropped, Entry entry) { } } - private boolean shouldDropBrokenEntry(Entry entry) { - if (!this.index.getIndex(EntryIndex.class).hasEntry(entry)) { + private boolean shouldDropBrokenEntry(Dropped dropped, Entry entry) { + if (!this.index.getIndex(EntryIndex.class).hasEntry(entry, this.project)) { return true; } @@ -74,7 +77,7 @@ private boolean shouldDropBrokenEntry(Entry entry) { } // Method entry has parameter names, keep it even though it's not the root. - return !(entry instanceof MethodEntry) || this.mappings.getChildren(entry).isEmpty(); + return !(entry instanceof MethodEntry) || this.hasNoChildren(entry, dropped); // Entry is not the root, and is not a method with params } @@ -84,7 +87,7 @@ public Dropped dropEmptyMappings(ProgressListener progress) { } private void tryDropEmptyEntry(Dropped dropped, Entry entry) { - if (this.shouldDropEmptyMapping(entry)) { + if (this.shouldDropEmptyMapping(dropped, entry)) { EntryMapping mapping = this.mappings.get(entry); if (mapping != null) { dropped.drop(entry, mapping); @@ -92,18 +95,34 @@ private void tryDropEmptyEntry(Dropped dropped, Entry entry) { } } - private boolean shouldDropEmptyMapping(Entry entry) { + private boolean shouldDropEmptyMapping(Dropped dropped, Entry entry) { EntryMapping mapping = this.mappings.get(entry); if (mapping != null) { - boolean isEmpty = mapping.targetName() == null && mapping.javadoc() == null; + boolean isEmpty = (mapping.targetName() == null && mapping.javadoc() == null) || !this.project.isRenamable(entry); + if (isEmpty) { - return this.mappings.getChildren(entry).isEmpty(); + return this.hasNoChildren(entry, dropped); } } return false; } + private boolean hasNoChildren(Entry entry, Dropped dropped) { + var children = this.mappings.getChildren(entry); + + // account for child mappings that have been dropped already + if (!children.isEmpty()) { + for (Entry child : children) { + if (!dropped.getDroppedMappings().containsKey(child) && !this.hasNoChildren(child, dropped)) { + return true; + } + } + } + + return children.isEmpty(); + } + public static class Dropped { private final Map, String> droppedMappings = new HashMap<>();