fix inner class detection in JarWalker (#1148)

FabricMC · Sep 3, 2024 · 7ab6e56 · 7ab6e56
1 parent 648a059
commit 7ab6e56
Show file tree

Hide file tree

Showing 3 changed files with 95 additions and 30 deletions.
diff --git a/src/main/java/net/fabricmc/loom/decompilers/cache/JarWalker.java b/src/main/java/net/fabricmc/loom/decompilers/cache/JarWalker.java
@@ -49,6 +49,8 @@
 
 import org.gradle.api.JavaVersion;
 import org.objectweb.asm.ClassReader;
+import org.objectweb.asm.tree.ClassNode;
+import org.objectweb.asm.tree.InnerClassNode;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -88,13 +90,12 @@ public static List<ClassEntry> findClasses(FileSystemUtil.Delegate fs) throws IO
 					continue;
 				}
 
-				boolean isInnerClass = fileName.contains("$");
+				String outerClass = findOuterClass(fs, fileName);
 
-				if (isInnerClass) {
-					String outerClassName = fileName.substring(0, fileName.indexOf('$')) + ".class";
-					innerClasses.computeIfAbsent(outerClassName, k -> new ArrayList<>()).add(fileName);
-				} else {
+				if (outerClass == null) {
 					outerClasses.add(fileName);
+				} else {
+					innerClasses.computeIfAbsent(outerClass + ".class", k -> new ArrayList<>()).add(fileName);
 				}
 			}
 		}
@@ -127,6 +128,45 @@ public static List<ClassEntry> findClasses(FileSystemUtil.Delegate fs) throws IO
 		}
 	}
 
+	/**
+	 * Check if the given class file denotes and inner class and find the corresponding outer class name.
+	 */
+	private static String findOuterClass(FileSystemUtil.Delegate fs, String classFile) throws IOException {
+		// this check can speed things up quite a bit, even if it does not follow the JVM spec
+		if (classFile.indexOf('$') < 0) {
+			return null;
+		}
+
+		try (InputStream is = Files.newInputStream(fs.getPath(classFile))) {
+			final ClassReader reader = new ClassReader(is);
+			final ClassNode classNode = new ClassNode();
+
+			reader.accept(classNode, ClassReader.SKIP_CODE | ClassReader.SKIP_FRAMES);
+
+			for (InnerClassNode innerClass : classNode.innerClasses) {
+				// a class file also contains references to enclosed inner classes
+				if (innerClass.name.equals(classNode.name)) {
+					// only regular inner classes have the outer class in the inner class attribute
+					if (innerClass.outerName != null) {
+						return innerClass.outerName;
+					}
+
+					// local and anonymous classes have the outer class in the enclosing method attribute
+					// we check for both attributes because both should be present for decompilers to
+					// recognize a class as an inner class
+					if (classNode.outerClass != null) {
+						return classNode.outerClass;
+					}
+
+					// there are some Minecraft versions with one attribute stripped but not the other
+					LOGGER.debug("inner class attribute is present for " + classNode.name + " but no outer class could be found, weird!");
+				}
+			}
+		}
+
+		return null;
+	}
+
 	private static CompletableFuture<ClassEntry> getClassEntry(String outerClass, List<String> innerClasses, FileSystemUtil.Delegate fs, Executor executor) {
 		List<CompletableFuture<List<String>>> parentClassesFutures = new ArrayList<>();
 

diff --git a/src/test/groovy/net/fabricmc/loom/test/unit/cache/CachedJarProcessorTest.groovy b/src/test/groovy/net/fabricmc/loom/test/unit/cache/CachedJarProcessorTest.groovy
@@ -45,12 +45,12 @@ class CachedJarProcessorTest extends Specification {
 	static Map<String, byte[]> jarEntries = [
 		"net/fabricmc/Example.class": newClass("net/fabricmc/Example"),
 		"net/fabricmc/other/Test.class": newClass("net/fabricmc/other/Test"),
-		"net/fabricmc/other/Test\$Inner.class": newClass("net/fabricmc/other/Test\$Inner"),
-		"net/fabricmc/other/Test\$1.class": newClass("net/fabricmc/other/Test\$1"),
+		"net/fabricmc/other/Test\$Inner.class": newInnerClass("net/fabricmc/other/Test\$Inner", "net/fabricmc/other/Test", "Inner"),
+		"net/fabricmc/other/Test\$1.class": newInnerClass("net/fabricmc/other/Test\$1", "net/fabricmc/other/Test"),
 	]
 
 	static String ExampleHash = "abc123/db5c3a2d04e0c6ea03aef0d217517aa0233f9b8198753d3c96574fe5825a13c4"
-	static String TestHash = "abc123/06f9f4c7dbca9baa037fbea007298ee15277d97de594bbf6e4a1ee346c079e65"
+	static String TestHash = "abc123/b49f74dc50847f8fefc0c6f850326bbe39ace0b381b827fe1a1f1ed1dea81330"
 
 	static CachedData ExampleCachedData = new CachedData("net/fabricmc/Example", "Example sources", lineNumber("net/fabricmc/Example"))
 	static CachedData TestCachedData = new CachedData("net/fabricmc/other/Test", "Test sources", lineNumber("net/fabricmc/other/Test"))
@@ -247,17 +247,17 @@ class CachedJarProcessorTest extends Specification {
 				[
 					"net/fabricmc/Example.class": newClass("net/fabricmc/Example"),
 					"net/fabricmc/other/Test.class": newClass("net/fabricmc/other/Test", ),
-					"net/fabricmc/other/Test\$Inner.class": newClass("net/fabricmc/other/Test\$Inner", ["net/fabricmc/Example"] as String[]),
-					"net/fabricmc/other/Test\$1.class": newClass("net/fabricmc/other/Test\$1"),
+					"net/fabricmc/other/Test\$Inner.class": newInnerClass("net/fabricmc/other/Test\$Inner", "net/fabricmc/other/Test", "Inner", ["net/fabricmc/Example"] as String[]),
+					"net/fabricmc/other/Test\$1.class": newInnerClass("net/fabricmc/other/Test\$1", "net/fabricmc/other/Test"),
 				]
 				)
 		// The second jar changes Example, so we would expect Test to be invalidated, thus causing a full decompile in this case
 		def jar2 = ZipTestUtils.createZipFromBytes(
 				[
 					"net/fabricmc/Example.class": newClass("net/fabricmc/Example", ["java/lang/Runnable"] as String[]),
 					"net/fabricmc/other/Test.class": newClass("net/fabricmc/other/Test", ),
-					"net/fabricmc/other/Test\$Inner.class": newClass("net/fabricmc/other/Test\$Inner", ["net/fabricmc/Example"] as String[]),
-					"net/fabricmc/other/Test\$1.class": newClass("net/fabricmc/other/Test\$1"),
+					"net/fabricmc/other/Test\$Inner.class": newInnerClass("net/fabricmc/other/Test\$Inner", "net/fabricmc/other/Test", "Inner", ["net/fabricmc/Example"] as String[]),
+					"net/fabricmc/other/Test\$1.class": newInnerClass("net/fabricmc/other/Test\$1", "net/fabricmc/other/Test"),
 				]
 				)
 
@@ -303,4 +303,14 @@ class CachedJarProcessorTest extends Specification {
 		writer.visit(Opcodes.V17, Opcodes.ACC_PUBLIC, name, null, superName, interfaces)
 		return writer.toByteArray()
 	}
+
+	private static byte[] newInnerClass(String name, String outerClass, String innerName = null, String[] interfaces = null, String superName = "java/lang/Object") {
+		def writer = new ClassWriter(0)
+		writer.visit(Opcodes.V17, Opcodes.ACC_PUBLIC, name, null, superName, interfaces)
+		writer.visitInnerClass(name, outerClass, innerName, 0)
+		if (innerName == null) {
+			writer.visitOuterClass(outerClass, null, null)
+		}
+		return writer.toByteArray()
+	}
 }
diff --git a/src/test/groovy/net/fabricmc/loom/test/unit/cache/JarWalkerTest.groovy b/src/test/groovy/net/fabricmc/loom/test/unit/cache/JarWalkerTest.groovy
@@ -38,23 +38,28 @@ class JarWalkerTest extends Specification {
 		def jar = ZipTestUtils.createZipFromBytes([
 			"net/fabricmc/Test.class": newClass("net/fabricmc/Test"),
 			"net/fabricmc/other/Test.class": newClass("net/fabricmc/other/Test"),
-			"net/fabricmc/other/Test\$Inner.class": newClass("net/fabricmc/other/Test\$Inner"),
-			"net/fabricmc/other/Test\$1.class": newClass("net/fabricmc/other/Test\$1"),
+			"net/fabricmc/other/Test\$Inner.class": newInnerClass("net/fabricmc/other/Test\$Inner", "net/fabricmc/other/Test", "Inner"),
+			"net/fabricmc/other/Test\$1.class": newInnerClass("net/fabricmc/other/Test\$1", "net/fabricmc/other/Test"),
+			"net/fabricmc/other/Test\$NotInner.class": newClass("net/fabricmc/other/Test\$NotInner"),
 		])
 		when:
 		def entries = JarWalker.findClasses(jar)
 		then:
-		entries.size() == 2
+		entries.size() == 3
 
 		entries[0].name() == "net/fabricmc/Test.class"
 		entries[0].sourcesFileName() == "net/fabricmc/Test.java"
 		entries[0].innerClasses().size() == 0
 
-		entries[1].name() == "net/fabricmc/other/Test.class"
-		entries[1].sourcesFileName() == "net/fabricmc/other/Test.java"
-		entries[1].innerClasses().size() == 2
-		entries[1].innerClasses()[0] == "net/fabricmc/other/Test\$1.class"
-		entries[1].innerClasses()[1] == "net/fabricmc/other/Test\$Inner.class"
+		entries[1].name() == "net/fabricmc/other/Test\$NotInner.class"
+		entries[1].sourcesFileName() == "net/fabricmc/other/Test\$NotInner.java"
+		entries[1].innerClasses().size() == 0
+
+		entries[2].name() == "net/fabricmc/other/Test.class"
+		entries[2].sourcesFileName() == "net/fabricmc/other/Test.java"
+		entries[2].innerClasses().size() == 2
+		entries[2].innerClasses()[0] == "net/fabricmc/other/Test\$1.class"
+		entries[2].innerClasses()[1] == "net/fabricmc/other/Test\$Inner.class"
 	}
 
 	def "Hash Classes"() {
@@ -73,17 +78,17 @@ class JarWalkerTest extends Specification {
 		"b055df8d9503b60050f6d0db387c84c47fedb4d9ed82c4f8174b4e465a9c479b" | [
 			"net/fabricmc/Test.class": newClass("net/fabricmc/Test"),
 		]
-		"3ba069bc20db1ee1b4bb69450dba3fd57a91059bd85e788d5af712aee3191792" | [
+		"b49f74dc50847f8fefc0c6f850326bbe39ace0b381b827fe1a1f1ed1dea81330" | [
 			"net/fabricmc/other/Test.class": newClass("net/fabricmc/other/Test"),
-			"net/fabricmc/other/Test\$Inner.class": newClass("net/fabricmc/other/Test\$Inner"),
-			"net/fabricmc/other/Test\$Inner\$2.class": newClass("net/fabricmc/other/Test\$Inner\$2"),
-			"net/fabricmc/other/Test\$1.class": newClass("net/fabricmc/other/Test\$1"),
+			"net/fabricmc/other/Test\$Inner.class": newInnerClass("net/fabricmc/other/Test\$Inner", "net/fabricmc/other/Test", "Inner"),
+			"net/fabricmc/other/Test\$Inner\$2.class": newInnerClass("net/fabricmc/other/Test\$Inner\$2", "net/fabricmc/other/Test\$Inner", "Inner"),
+			"net/fabricmc/other/Test\$1.class": newInnerClass("net/fabricmc/other/Test\$1", "net/fabricmc/other/Test"),
 		]
-		"3ba069bc20db1ee1b4bb69450dba3fd57a91059bd85e788d5af712aee3191792" | [
+		"b49f74dc50847f8fefc0c6f850326bbe39ace0b381b827fe1a1f1ed1dea81330" | [
 			"net/fabricmc/other/Test.class": newClass("net/fabricmc/other/Test"),
-			"net/fabricmc/other/Test\$Inner.class": newClass("net/fabricmc/other/Test\$Inner"),
-			"net/fabricmc/other/Test\$Inner\$2.class": newClass("net/fabricmc/other/Test\$Inner\$2"),
-			"net/fabricmc/other/Test\$1.class": newClass("net/fabricmc/other/Test\$1"),
+			"net/fabricmc/other/Test\$Inner.class": newInnerClass("net/fabricmc/other/Test\$Inner", "net/fabricmc/other/Test", "Inner"),
+			"net/fabricmc/other/Test\$Inner\$2.class": newInnerClass("net/fabricmc/other/Test\$Inner\$2", "net/fabricmc/other/Test\$Inner", "Inner"),
+			"net/fabricmc/other/Test\$1.class": newInnerClass("net/fabricmc/other/Test\$1", "net/fabricmc/other/Test"),
 		]
 	}
 
@@ -125,8 +130,8 @@ class JarWalkerTest extends Specification {
 		given:
 		def jarEntries = [
 			"net/fabricmc/other/Test.class": newClass("net/fabricmc/other/Test"),
-			"net/fabricmc/other/Test\$Inner.class": newClass("net/fabricmc/other/Test\$Inner", null, "net/fabricmc/other/Super"),
-			"net/fabricmc/other/Test\$1.class": newClass("net/fabricmc/other/Test\$1", ["java/lang/Runnable"] as String[]),
+			"net/fabricmc/other/Test\$Inner.class": newInnerClass("net/fabricmc/other/Test\$Inner", "net/fabricmc/other/Test", "Inner", null, "net/fabricmc/other/Super"),
+			"net/fabricmc/other/Test\$1.class": newInnerClass("net/fabricmc/other/Test\$1", "net/fabricmc/other/Test", null, ["java/lang/Runnable"] as String[]),
 		]
 		def jar = ZipTestUtils.createZipFromBytes(jarEntries)
 
@@ -151,4 +156,14 @@ class JarWalkerTest extends Specification {
 		writer.visit(Opcodes.V17, Opcodes.ACC_PUBLIC, name, null, superName, interfaces)
 		return writer.toByteArray()
 	}
+
+	private static byte[] newInnerClass(String name, String outerClass, String innerName = null, String[] interfaces = null, String superName = "java/lang/Object") {
+		def writer = new ClassWriter(0)
+		writer.visit(Opcodes.V17, Opcodes.ACC_PUBLIC, name, null, superName, interfaces)
+		writer.visitInnerClass(name, outerClass, innerName, 0)
+		if (innerName == null) {
+			writer.visitOuterClass(outerClass, null, null)
+		}
+		return writer.toByteArray()
+	}
 }