diff --git a/src/main/java/net/minecraftforge/fml/common/asm/transformers/EventSubscriberTransformer.java b/src/main/java/net/minecraftforge/fml/common/asm/transformers/EventSubscriberTransformer.java deleted file mode 100644 index fbf702596..000000000 --- a/src/main/java/net/minecraftforge/fml/common/asm/transformers/EventSubscriberTransformer.java +++ /dev/null @@ -1,93 +0,0 @@ -/* - * Minecraft Forge - * Copyright (c) 2016-2020. - * - * 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 2.1 - * of the License. - * - * 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 net.minecraftforge.fml.common.asm.transformers; - -import java.lang.reflect.Modifier; -import java.util.List; - -import net.minecraft.launchwrapper.IClassTransformer; - -import org.objectweb.asm.ClassReader; -import org.objectweb.asm.ClassWriter; -import org.objectweb.asm.Opcodes; -import org.objectweb.asm.tree.AnnotationNode; -import org.objectweb.asm.tree.ClassNode; -import org.objectweb.asm.tree.MethodNode; - -import com.google.common.base.Predicate; -import com.google.common.collect.Iterables; - -public class EventSubscriberTransformer implements IClassTransformer -{ - @Override - public byte[] transform(String name, String transformedName, byte[] basicClass) - { - if (basicClass == null) return null; - - ClassNode classNode = new ClassNode(); - new ClassReader(basicClass).accept(classNode, 0); - - boolean isSubscriber = false; - - for (MethodNode methodNode : classNode.methods) - { - List anns = methodNode.visibleAnnotations; - - if (anns != null && Iterables.any(anns, SubscribeEventPredicate.INSTANCE)) - { - if (Modifier.isPrivate(methodNode.access)) - { - String msg = "Cannot apply @SubscribeEvent to private method %s/%s%s"; - throw new RuntimeException(String.format(msg, classNode.name, methodNode.name, methodNode.desc)); - } - - methodNode.access = toPublic(methodNode.access); - isSubscriber = true; - } - } - - if (isSubscriber) - { - classNode.access = toPublic(classNode.access); - - ClassWriter writer = new ClassWriter(ClassWriter.COMPUTE_MAXS); - classNode.accept(writer); - return writer.toByteArray(); - } - - return basicClass; - } - - private static int toPublic(int access) - { - return access & ~(Opcodes.ACC_PRIVATE | Opcodes.ACC_PROTECTED) | Opcodes.ACC_PUBLIC; - } - - private static class SubscribeEventPredicate implements Predicate - { - static final SubscribeEventPredicate INSTANCE = new SubscribeEventPredicate(); - - @Override - public boolean apply(AnnotationNode input) - { - return input.desc.equals("Lnet/minecraftforge/fml/common/eventhandler/SubscribeEvent;"); - } - } -} diff --git a/src/main/java/net/minecraftforge/fml/common/eventhandler/EventBus.java b/src/main/java/net/minecraftforge/fml/common/eventhandler/EventBus.java index 0fa262bcd..3d1a2738a 100644 --- a/src/main/java/net/minecraftforge/fml/common/eventhandler/EventBus.java +++ b/src/main/java/net/minecraftforge/fml/common/eventhandler/EventBus.java @@ -22,11 +22,10 @@ import java.lang.reflect.Constructor; import java.lang.reflect.Method; import java.lang.reflect.Modifier; -import java.util.ArrayList; -import java.util.Map; -import java.util.Objects; -import java.util.Set; +import java.util.*; import java.util.concurrent.ConcurrentHashMap; +import java.util.function.Function; +import java.util.stream.Collectors; import javax.annotation.Nonnull; @@ -79,59 +78,47 @@ public void register(Object target) } listenerOwners.put(target, activeModContainer); - boolean isStatic; - Set> supers; - Class scanTarget; + + Collection methods; if (target instanceof Class clazz) { - isStatic = true; - supers = Set.of(clazz); - scanTarget = clazz; + // static listener: subscribed methods must be declared by the class + methods = Arrays.stream(clazz.getDeclaredMethods()) + .filter(m -> !m.isSynthetic() + && Modifier.isStatic(m.getModifiers()) + // private not allowed to keep legacy behaviour + && !Modifier.isPrivate(m.getModifiers()) + && m.isAnnotationPresent(SubscribeEvent.class)) + .toList(); } else { - isStatic = false; - supers = TypeToken.of(target.getClass()).getTypes().rawTypes(); - scanTarget = target.getClass(); + // instance listener: methods overriding a subscribed method is also valid. + // + // In this case, we will register the subscribed parent method instead. JVM will + // handle it if a subclass overrides subscribed method + methods = TypeToken.of(target.getClass()) + .getTypes() + .rawTypes() + // get self & superclass & interface + .stream() + .map(Class::getDeclaredMethods) + .flatMap(Arrays::stream) + .filter(m -> !m.isSynthetic() + && !Modifier.isStatic(m.getModifiers()) + // private not allowed because it does not participate in inheritance + && !Modifier.isPrivate(m.getModifiers()) + && m.isAnnotationPresent(SubscribeEvent.class)) + // deduplicate by signature + .collect(Collectors.toMap( + m -> Map.entry(m.getName(), Arrays.asList(m.getParameterTypes())), + Function.identity(), + (a, b) -> a, + LinkedHashMap::new + )) + .values(); } - for (Method method : scanTarget.getMethods()) + for (Method method : methods) { - if (isStatic != Modifier.isStatic(method.getModifiers())) - continue; - - try { - // do `.getDeclaredMethod(...)` to force JVM to walk through declared methods and load their parameter - // types. This is for preventing shortcut below from skipping classloading - // - // mod developers should be responsible for not loading non-existent class, but :( - // related issue: https://github.com/CleanroomMC/Cleanroom/issues/349 - method.getDeclaringClass().getDeclaredMethod("forceClassLoadingForDeclaredMethods", Event.class); - } catch (NoSuchMethodException e) { - // swallow this specific exception, other exceptions, like ClassNotFoundException, will fall through - } - var parameterTypes = method.getParameterTypes(); - var matched = supers.stream() - .map(cls -> { - if (cls == method.getDeclaringClass()) { - // shortcut for most event handler classes with no explicit superclass - return method; - } - try { - return cls.getDeclaredMethod(method.getName(), parameterTypes); - } catch (NoSuchMethodException e) { - // Eat the error, this is not unexpected - return null; - } - }) - .filter(Objects::nonNull) - .filter(m -> m.isAnnotationPresent(SubscribeEvent.class)) - .findFirst() - .orElse(null); - - if (matched == null) - { - continue; - } - if (parameterTypes.length != 1) { throw new IllegalArgumentException( @@ -141,15 +128,12 @@ public void register(Object target) } Class eventType = parameterTypes[0]; - if (!Event.class.isAssignableFrom(eventType)) { throw new IllegalArgumentException("Method " + method + " has @SubscribeEvent annotation, but takes a argument that is not an Event " + eventType); } - // the method to be registered here is "matched", not "method", it should be a bug of - // the original event bus, since the exceptions above are all referencing "method" - register(eventType, target, matched, activeModContainer); + register(eventType, target, method, activeModContainer); } } diff --git a/src/main/java/net/minecraftforge/fml/common/eventhandler/EventListenerFactory.java b/src/main/java/net/minecraftforge/fml/common/eventhandler/EventListenerFactory.java index a7bfd2a43..c8900951f 100644 --- a/src/main/java/net/minecraftforge/fml/common/eventhandler/EventListenerFactory.java +++ b/src/main/java/net/minecraftforge/fml/common/eventhandler/EventListenerFactory.java @@ -39,7 +39,8 @@ private static MethodHandle createListenerFactory( Object instance ) { try { - var handle = LOOKUP.unreflect(callback); + var lookup = MethodHandles.privateLookupIn(callback.getDeclaringClass(), LOOKUP); + var handle = lookup.unreflect(callback); var factoryType = isStatic ? Constants.RETURNS_IT @@ -47,7 +48,7 @@ private static MethodHandle createListenerFactory( : Constants.RETURNS_IT.insertParameterTypes(0, instance.getClass()); var factoryHandle = LambdaMetafactory.metafactory( - LOOKUP, + lookup, Constants.METHOD_NAME, factoryType, Constants.METHOD_TYPE, diff --git a/src/main/java/net/minecraftforge/fml/relauncher/FMLCorePlugin.java b/src/main/java/net/minecraftforge/fml/relauncher/FMLCorePlugin.java index 5fd23c16e..65f13fd21 100644 --- a/src/main/java/net/minecraftforge/fml/relauncher/FMLCorePlugin.java +++ b/src/main/java/net/minecraftforge/fml/relauncher/FMLCorePlugin.java @@ -31,7 +31,6 @@ public String[] getASMTransformerClass() return new String[] { "net.minecraftforge.fml.common.asm.transformers.SideTransformer", "net.minecraftforge.fml.common.asm.transformers.EventSubscriptionTransformer", - "net.minecraftforge.fml.common.asm.transformers.EventSubscriberTransformer", "net.minecraftforge.fml.common.asm.transformers.SoundEngineFixTransformer", "net.minecraftforge.fml.common.asm.transformers.LWJGLTransformer", }; diff --git a/src/test/java/net/minecraftforge/fml/common/eventhandler/EventBusTest.java b/src/test/java/net/minecraftforge/fml/common/eventhandler/EventBusTest.java index ab87f10f8..7eb17e752 100644 --- a/src/test/java/net/minecraftforge/fml/common/eventhandler/EventBusTest.java +++ b/src/test/java/net/minecraftforge/fml/common/eventhandler/EventBusTest.java @@ -2,15 +2,13 @@ import net.minecraftforge.fml.common.Loader; import net.minecraftforge.fml.common.ModContainer; -import net.minecraftforge.fml.common.eventhandler.impl.AbnormalListeners; -import net.minecraftforge.fml.common.eventhandler.impl.ExampleEvent; -import net.minecraftforge.fml.common.eventhandler.impl.InstanceListeners; -import net.minecraftforge.fml.common.eventhandler.impl.StaticListeners; +import net.minecraftforge.fml.common.eventhandler.impl.*; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; import java.lang.reflect.Method; import java.util.List; +import java.util.Set; /** * @author ZZZank @@ -86,4 +84,32 @@ public void registerIllegalParamType() throws Exception { Assertions.assertEquals(event.id, listener.captured); } + + @Test + public void registerNonPublic() { + var bus = new EventBus(); + + // static + { + bus.register(NonPublicListeners.class); + + var event = new ExampleEvent(); + bus.post(event); + Assertions.assertEquals(Set.of("packaged static", "protected static"), event.sink); + + bus.unregister(NonPublicListeners.class); + } + + // instance + { + var listener = new NonPublicListeners.OverrideWithNoSub(); + bus.register(listener); + + var event = new ExampleEvent(); + bus.post(event); + Assertions.assertEquals(Set.of("packaged", "protected (subclass)"), event.sink); + + bus.unregister(listener); + } + } } diff --git a/src/test/java/net/minecraftforge/fml/common/eventhandler/impl/ExampleEvent.java b/src/test/java/net/minecraftforge/fml/common/eventhandler/impl/ExampleEvent.java index 93da6a024..fbae39c0d 100644 --- a/src/test/java/net/minecraftforge/fml/common/eventhandler/impl/ExampleEvent.java +++ b/src/test/java/net/minecraftforge/fml/common/eventhandler/impl/ExampleEvent.java @@ -2,6 +2,9 @@ import net.minecraftforge.fml.common.eventhandler.Event; +import java.util.HashSet; +import java.util.Set; + /** * @author ZZZank */ @@ -9,6 +12,7 @@ public class ExampleEvent extends Event { public static int CURRENT_ID = 0; public final int id; + public final Set sink = new HashSet<>(); public ExampleEvent() { this.id = CURRENT_ID++; diff --git a/src/test/java/net/minecraftforge/fml/common/eventhandler/impl/NonPublicListeners.java b/src/test/java/net/minecraftforge/fml/common/eventhandler/impl/NonPublicListeners.java new file mode 100644 index 000000000..77488a48a --- /dev/null +++ b/src/test/java/net/minecraftforge/fml/common/eventhandler/impl/NonPublicListeners.java @@ -0,0 +1,47 @@ +package net.minecraftforge.fml.common.eventhandler.impl; + +import net.minecraftforge.fml.common.eventhandler.SubscribeEvent; + +/** + * @author ZZZank + */ +public class NonPublicListeners { + + @SubscribeEvent + private static void privateStatic(ExampleEvent event) { + throw new AssertionError("private (static) method should not be registered"); + } + + @SubscribeEvent + static void packagedStatic(ExampleEvent event) { + event.sink.add("packaged static"); + } + + @SubscribeEvent + protected static void protectedStatic(ExampleEvent event) { + event.sink.add("protected static"); + } + + @SubscribeEvent + private void privateInstance(ExampleEvent event) { + throw new AssertionError("private method should not be registered"); + } + + @SubscribeEvent + void packagedInstance(ExampleEvent event) { + event.sink.add("packaged"); + } + + @SubscribeEvent + protected void protectedInstance(ExampleEvent event) { + throw new AssertionError("impl by subclass"); + } + + public static class OverrideWithNoSub extends NonPublicListeners { + + @Override + protected void protectedInstance(ExampleEvent event) { + event.sink.add("protected (subclass)"); + } + } +}