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 <[email protected]> * fix method name * update description * add a test to run the help command --------- Co-authored-by: Iota <[email protected]>
QuiltMC · Oct 16, 2024 · ba80b7e · ba80b7e
1 parent a38a3e4
commit ba80b7e
Show file tree

Hide file tree

Showing 10 changed files with 182 additions and 41 deletions.
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
@@ -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
@@ -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
@@ -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
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
@@ -0,0 +1,5 @@
+CLASS a InvalidMappings
+	FIELD a slayField Ljava/lang/String;
+	METHOD <init> (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
@@ -0,0 +1,5 @@
+CLASS a
+	FIELD a Ljava/lang/String;
+	METHOD <init> (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
@@ -0,0 +1,12 @@
+CLASS a InvalidMappings
+	FIELD a slayField Ljava/lang/String;
+	FIELD b fakeField I
+	METHOD <init> (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
@@ -131,18 +131,20 @@ public EntryRemapper getRemapper() {
 		return this.remapper;
 	}
 
-	public void dropMappings(ProgressListener progress) {
+	public Collection<Entry<?>> dropMappings(ProgressListener progress) {
 		DeltaTrackingTree<EntryMapping> mappings = this.remapper.getMappings();
 
 		Collection<Entry<?>> dropped = this.dropMappings(mappings, progress);
 		for (Entry<?> entry : dropped) {
 			mappings.trackChange(entry);
 		}
+
+		return dropped;
 	}
 
 	private Collection<Entry<?>> dropMappings(EntryTree<EntryMapping> 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<Entry<?>, 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
@@ -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<EntryMapping> 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.
+	 * <br>
+	 * 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.
+	 * <br>
+	 * 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
@@ -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<EntryMapping> mappings;
 
-	public MappingsChecker(JarIndex index, EntryTree<EntryMapping> mappings) {
+	public MappingsChecker(EnigmaProject project, JarIndex index, EntryTree<EntryMapping> mappings) {
+		this.project = project;
 		this.index = index;
 		this.mappings = mappings;
 	}
@@ -50,16 +53,16 @@ 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);
 			}
 		}
 	}
 
-	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,26 +87,42 @@ 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);
 			}
 		}
 	}
 
-	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<Entry<?>, String> droppedMappings = new HashMap<>();