From f69e94ce52ad8e2df816003faae350a2100621d3 Mon Sep 17 00:00:00 2001 From: Jason Penilla <11360596+jpenilla@users.noreply.github.com> Date: Mon, 2 Nov 2026 15:50:38 -0700 Subject: [PATCH] Fix override annotation adder bugs - Stop skipping the rest of a class when a method's only bridges are in a subclass. The method itself is now still checked for a super method. - Don't add @Override to clone() or finalize() in interfaces unless a superinterface declares them. Interfaces don't inherit Object's non-public methods, so javac rejects the annotation (JLS 9.6.4.4). - Don't add @Override to synthetic methods, as they aren't declared in source. --- .../io/papermc/codebook/pages/FixJarPage.java | 75 +++-- .../codebook/pages/FixJarPageTest.java | 302 ++++++++++++++++++ 2 files changed, 357 insertions(+), 20 deletions(-) create mode 100644 src/test/java/io/papermc/codebook/pages/FixJarPageTest.java diff --git a/src/main/java/io/papermc/codebook/pages/FixJarPage.java b/src/main/java/io/papermc/codebook/pages/FixJarPage.java index 6b84e2a..d9d1173 100644 --- a/src/main/java/io/papermc/codebook/pages/FixJarPage.java +++ b/src/main/java/io/papermc/codebook/pages/FixJarPage.java @@ -27,6 +27,8 @@ import dev.denwav.hypo.asm.AsmMethodData; import dev.denwav.hypo.core.HypoContext; import dev.denwav.hypo.hydrate.generic.HypoHydration; +import dev.denwav.hypo.model.data.ClassData; +import dev.denwav.hypo.model.data.ClassKind; import dev.denwav.hypo.model.data.MethodData; import dev.denwav.hypo.model.data.Visibility; import jakarta.inject.Inject; @@ -55,33 +57,17 @@ protected void processClass(final AsmClassData classData) throws IOException { private static final class OverrideAnnotationAdder { private OverrideAnnotationAdder() {} - private static void addAnnotations(final AsmClassData classData) { + private static void addAnnotations(final AsmClassData classData) throws IOException { for (final MethodData method : classData.methods()) { - if (method.isStatic() || method.visibility() == Visibility.PRIVATE) { + // Synthetic methods have no declaration in source, so @Override doesn't apply to them + if (method.isStatic() || method.visibility() == Visibility.PRIVATE || method.isSynthetic()) { continue; } if (isInitializer(method)) { continue; } - final MethodData baseMethod; - final @Nullable Set syntheticSources = method.get(HypoHydration.SYNTHETIC_SOURCES); - if (syntheticSources == null) { - baseMethod = method; - } else { - outer: - { - for (final MethodData targetMethod : syntheticSources) { - if (targetMethod.parentClass().equals(method.parentClass())) { - baseMethod = targetMethod; - break outer; - } - } - return; - } - } - - if (baseMethod.superMethod() != null) { + if (overrides(method)) { final MethodNode node = ((AsmMethodData) method).getNode(); final var annoClass = "Ljava/lang/Override;"; if (node.invisibleAnnotations == null @@ -93,6 +79,55 @@ private static void addAnnotations(final AsmClassData classData) { } } + private static boolean overrides(final MethodData method) throws IOException { + if (hasSuperMethod(method)) { + return true; + } + + // Overrides with a different descriptor are only linked to the super method through a bridge in this class. + // A bridge in another class means that class inherits this method to implement something, which doesn't + // make this method an override. + final @Nullable Set syntheticSources = method.get(HypoHydration.SYNTHETIC_SOURCES); + if (syntheticSources != null) { + for (final MethodData source : syntheticSources) { + if (source.parentClass().equals(method.parentClass()) && hasSuperMethod(source)) { + return true; + } + } + } + return false; + } + + private static boolean hasSuperMethod(final MethodData method) throws IOException { + final @Nullable MethodData superMethod = method.superMethod(); + if (superMethod == null) { + return false; + } + + // Hypo treats Object as the super class of interfaces, but interfaces only have its public methods as + // members, so @Override isn't allowed for just clone() or finalize() (JLS 9.2, 9.6.4.4). superMethod() only + // gives a single method though, so the method could also be declared by a superinterface. + final ClassData parentClass = method.parentClass(); + if (parentClass.is(ClassKind.INTERFACE) && superMethod.visibility() != Visibility.PUBLIC) { + return isDeclaredBySuperInterface(method, parentClass); + } + return true; + } + + private static boolean isDeclaredBySuperInterface(final MethodData method, final ClassData classData) + throws IOException { + for (final ClassData superInterface : classData.interfaces()) { + final @Nullable MethodData superMethod = superInterface.method(method.name(), method.descriptor()); + if (superMethod != null && method.overrides(superMethod)) { + return true; + } + if (isDeclaredBySuperInterface(method, superInterface)) { + return true; + } + } + return false; + } + private static boolean isInitializer(final MethodData method) { return method.name().equals("") || method.name().equals(""); } diff --git a/src/test/java/io/papermc/codebook/pages/FixJarPageTest.java b/src/test/java/io/papermc/codebook/pages/FixJarPageTest.java new file mode 100644 index 0000000..e591bfd --- /dev/null +++ b/src/test/java/io/papermc/codebook/pages/FixJarPageTest.java @@ -0,0 +1,302 @@ +/* + * codebook is a remapper utility for the PaperMC project. + * + * Copyright (c) 2023 Kyle Wood (DenWav) + * Contributors + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; + * version 3 only, no later versions. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 + * USA + */ + +package io.papermc.codebook.pages; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import dev.denwav.hypo.asm.AsmClassDataProvider; +import dev.denwav.hypo.asm.AsmMethodData; +import dev.denwav.hypo.asm.hydrate.BridgeMethodHydrator; +import dev.denwav.hypo.core.HypoConfig; +import dev.denwav.hypo.core.HypoContext; +import dev.denwav.hypo.hydrate.HydrationManager; +import dev.denwav.hypo.model.ClassProviderRoot; +import dev.denwav.hypo.model.data.ClassData; +import dev.denwav.hypo.model.data.MethodData; +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.ArrayList; +import java.util.List; +import java.util.Map; +import javax.tools.ToolProvider; +import org.junit.jupiter.api.AfterAll; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.io.TempDir; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.CsvSource; + +class FixJarPageTest { + + private static final String OVERRIDE = "Ljava/lang/Override;"; + + private static final Map SOURCES = Map.ofEntries( + Map.entry("p/Generic.java", """ + package p; + public interface Generic { void m(T t); } + """), + Map.entry("p/Exact.java", """ + package p; + public interface Exact { void m(String s); } + """), + Map.entry("p/Unrelated.java", """ + package p; + public class Unrelated { + public void m(String s) {} + @Override public String toString() { return ""; } + } + """), + Map.entry("p/UnrelatedSub.java", """ + package p; + public class UnrelatedSub extends Unrelated implements Generic {} + """), + Map.entry("p/ImplementsExact.java", """ + package p; + public class ImplementsExact implements Exact { + @Override public void m(String s) {} + } + """), + Map.entry("p/ImplementsExactSub.java", """ + package p; + public class ImplementsExactSub extends ImplementsExact implements Generic {} + """), + Map.entry("p/Concrete.java", """ + package p; + public class Concrete implements Generic { + @Override public void m(String s) {} + } + """), + Map.entry("p/InterfaceAbstract.java", """ + package p; + public interface InterfaceAbstract extends Generic { @Override void m(String s); } + """), + Map.entry("p/InterfaceDefault.java", """ + package p; + public interface InterfaceDefault extends Generic { @Override default void m(String s) {} } + """), + Map.entry("p/AbstractClass.java", """ + package p; + public abstract class AbstractClass implements Generic { @Override public abstract void m(String s); } + """), + Map.entry("p/Bounded.java", """ + package p; + public interface Bounded extends Generic { @Override void m(U u); } + """), + Map.entry("p/Covariant.java", """ + package p; + public interface Covariant { Object get(); } + """), + Map.entry("p/CovariantInterface.java", """ + package p; + public interface CovariantInterface extends Covariant { @Override String get(); } + """), + Map.entry("p/CovariantAbstract.java", """ + package p; + public abstract class CovariantAbstract implements Covariant { @Override public abstract String get(); } + """), + Map.entry("p/Overload.java", """ + package p; + public class Overload implements Generic { + @Override public void m(String s) {} + public void m(Integer i) {} + } + """), + Map.entry("p/RecordImpl.java", """ + package p; + public record RecordImpl(String m) implements java.util.function.Supplier { + @Override public String get() { return this.m; } + } + """), + Map.entry("p/ObjectMethods.java", """ + package p; + public interface ObjectMethods { + @Override String toString(); + @Override boolean equals(Object o); + Object clone(); + void finalize(); + } + """), + Map.entry("p/CovariantClone.java", """ + package p; + public interface CovariantClone { CovariantClone clone(); } + """), + Map.entry("p/DeclaresClone.java", """ + package p; + public interface DeclaresClone { Object clone(); } + """), + Map.entry("p/InheritsClone.java", """ + package p; + public interface InheritsClone extends DeclaresClone { @Override Object clone(); } + """), + Map.entry("p/InheritsCloneCovariant.java", """ + package p; + public interface InheritsCloneCovariant extends DeclaresClone { @Override InheritsCloneCovariant clone(); } + """), + Map.entry("p/ClassClone.java", """ + package p; + public class ClassClone { @Override public Object clone() { return this; } } + """), + Map.entry("p/Members.java", """ + package p; + public class Members { + public static void hidden() {} + private void priv() {} + void packagePrivate() {} + } + """), + Map.entry("p/MembersSub.java", """ + package p; + public class MembersSub extends Members { + public static void hidden() {} + private void priv() {} + } + """), + Map.entry("q/OtherPackage.java", """ + package q; + public class OtherPackage extends p.Members { public void packagePrivate() {} } + """), + Map.entry("q/OtherPackageEmpty.java", """ + package q; + public class OtherPackageEmpty extends p.Members {} + """), + Map.entry("p/BackInPackage.java", """ + package p; + public class BackInPackage extends q.OtherPackageEmpty { @Override void packagePrivate() {} } + """)); + + @TempDir + static Path tempDir; + + private static HypoContext context; + + @BeforeAll + static void processFixtures() throws Exception { + final Path sourceDir = tempDir.resolve("src"); + final Path classesDir = tempDir.resolve("classes"); + Files.createDirectories(classesDir); + + final List args = new ArrayList<>(List.of("-proc:none", "-nowarn", "-d", classesDir.toString())); + for (final var entry : SOURCES.entrySet()) { + final Path file = sourceDir.resolve(entry.getKey()); + Files.createDirectories(file.getParent()); + Files.writeString(file, entry.getValue()); + args.add(file.toString()); + } + + // The fixtures declare @Override wherever javac allows it, so compiling them also checks the expectations below + final var compiler = ToolProvider.getSystemJavaCompiler(); + assertNotNull(compiler, "Tests must run on a JDK"); + assertEquals(0, compiler.run(null, null, null, args.toArray(new String[0])), "Fixtures failed to compile"); + + context = HypoContext.builder() + .withProvider(AsmClassDataProvider.of(ClassProviderRoot.fromDir(classesDir))) + .withContextProvider(AsmClassDataProvider.of(ClassProviderRoot.ofJdk())) + .withConfig(HypoConfig.builder().build()) + .build(); + HydrationManager.createDefault().register(BridgeMethodHydrator.create()).hydrate(context); + + new FixJarPage(context).exec(); + } + + @AfterAll + static void closeContext() throws Exception { + if (context != null) { + context.close(); + } + } + + @ParameterizedTest(name = "{0}#{1}{2}") + @CsvSource({ + // A class with a bridge in a subclass must not stop processing for the rest of the class + "p/Unrelated, toString, ()Ljava/lang/String;", + // A bridge in a subclass doesn't change that the method overrides something itself + "p/ImplementsExact, m, (Ljava/lang/String;)V", + // Different descriptors, linked by the bridge javac generates + "p/Concrete, m, (Ljava/lang/String;)V", + "p/InterfaceAbstract, m, (Ljava/lang/String;)V", + "p/InterfaceDefault, m, (Ljava/lang/String;)V", + "p/AbstractClass, m, (Ljava/lang/String;)V", + "p/Bounded, m, (Ljava/lang/Number;)V", + "p/CovariantInterface, get, ()Ljava/lang/String;", + "p/CovariantAbstract, get, ()Ljava/lang/String;", + "p/Overload, m, (Ljava/lang/String;)V", + "p/RecordImpl, get, ()Ljava/lang/String;", + "p/RecordImpl, toString, ()Ljava/lang/String;", + // Interfaces may override the public methods of Object (JLS 9.6.4.4) + "p/ObjectMethods, toString, ()Ljava/lang/String;", + "p/ObjectMethods, equals, (Ljava/lang/Object;)Z", + // Declared by a superinterface, even though Object has a matching non-public method too + "p/InheritsClone, clone, ()Ljava/lang/Object;", + "p/InheritsCloneCovariant, clone, ()Lp/InheritsCloneCovariant;", + "p/ClassClone, clone, ()Ljava/lang/Object;", + // Same package as the package-private method, with a class from another package in between (JLS 8.4.8.1) + "p/BackInPackage, packagePrivate, ()V", + }) + void annotatesOverrides(final String owner, final String name, final String descriptor) throws IOException { + assertTrue(hasOverride(owner, name, descriptor)); + } + + @ParameterizedTest(name = "{0}#{1}{2}") + @CsvSource({ + // Only implements the interface through the subclass, @Override here wouldn't compile + "p/Unrelated, m, (Ljava/lang/String;)V", + "p/Overload, m, (Ljava/lang/Integer;)V", + // Not inherited by interfaces, since they aren't public (JLS 9.2, 9.6.4.4) + "p/ObjectMethods, clone, ()Ljava/lang/Object;", + "p/ObjectMethods, finalize, ()V", + "p/CovariantClone, clone, ()Lp/CovariantClone;", + "p/DeclaresClone, clone, ()Ljava/lang/Object;", + // Hiding, private, and across packages (JLS 8.4.8.1) + "p/MembersSub, hidden, ()V", + "p/MembersSub, priv, ()V", + "q/OtherPackage, packagePrivate, ()V", + // Not declared by the fixture's source: bridges and the implicit members of the record + "p/Concrete, m, (Ljava/lang/Object;)V", + "p/ImplementsExactSub, m, (Ljava/lang/Object;)V", + "p/UnrelatedSub, m, (Ljava/lang/Object;)V", + "p/InterfaceAbstract, m, (Ljava/lang/Object;)V", + "p/CovariantInterface, get, ()Ljava/lang/Object;", + "p/RecordImpl, get, ()Ljava/lang/Object;", + "p/RecordImpl, m, ()Ljava/lang/String;", + }) + void doesNotAnnotateOtherMethods(final String owner, final String name, final String descriptor) + throws IOException { + assertFalse(hasOverride(owner, name, descriptor)); + } + + private static boolean hasOverride(final String owner, final String name, final String descriptor) + throws IOException { + final ClassData classData = context.getProvider().findClass(owner); + assertNotNull(classData, owner); + for (final MethodData method : classData.methods(name)) { + if (method.descriptorText().equals(descriptor)) { + final var annotations = ((AsmMethodData) method).getNode().invisibleAnnotations; + return annotations != null && annotations.stream().anyMatch(a -> a.desc.equals(OVERRIDE)); + } + } + throw new AssertionError("No method " + owner + "#" + name + descriptor); + } +}