Allow generic sources to have instance methods

Rather than assuming static methods are generic, and instance methods
are direct, the Generator now has separate entrypoints for handling
instance and generic methods.

As a result of this change, we've also relaxed some of the validation
code. As a result, we now allow calling private/protected methods
which are annotated with @LuaFunction.

projects/core-api/src/main/java/dan200/computercraft/api/lua/GenericSource.java

@@ -11,11 +11,10 @@ import dan200.computercraft.api.peripheral.IPeripheral;
  * A generic source of {@link LuaFunction} functions.
  * <p>
  * Unlike normal objects ({@link IDynamicLuaObject} or {@link IPeripheral}), methods do not target this object but
- * instead are defined as {@code static} and accept their target as the first parameter. This allows you to inject
+ * accept their target as the first parameter. This allows you to inject methods onto objects you do not own, as well as
- * methods onto objects you do not own, as well as declaring methods for a specific "trait" (for instance, a Forge
+ * declaring methods for a specific "trait" (for instance, a Forge capability or Fabric block lookup interface).
- * capability or Fabric block lookup interface).
  * <p>
- * Currently the "generic peripheral" system is incompatible with normal peripherals. Peripherals explicitly provided
+ * Currently, the "generic peripheral" system is incompatible with normal peripherals. Peripherals explicitly provided
  * by capabilities/the block lookup API take priority. Block entities which use this system are given a peripheral name
  * determined by their id, rather than any peripheral provider, though additional types may be provided by overriding
  * {@link GenericPeripheral#getType()}.
@@ -25,7 +24,7 @@ import dan200.computercraft.api.peripheral.IPeripheral;
  * <pre>{@code
  * public class InventoryMethods implements GenericSource {
  *     \@LuaFunction( mainThread = true )
- *     public static int size(IItemHandler inventory) {
+ *     public int size(IItemHandler inventory) {
  *         return inventory.getSlots();
  *     }
  *

projects/core/src/main/java/dan200/computercraft/core/asm/Generator.java

@@ -18,10 +18,7 @@ import javax.annotation.Nullable;
 import java.lang.invoke.MethodHandle;
 import java.lang.invoke.MethodHandles;
 import java.lang.invoke.MethodType;
-import java.lang.reflect.Member;
+import java.lang.reflect.*;
-import java.lang.reflect.Method;
-import java.lang.reflect.Modifier;
-import java.lang.reflect.Type;
 import java.nio.ByteBuffer;
 import java.util.*;
 import java.util.function.Function;
@@ -106,9 +103,14 @@ final class Generator<T> {
     private final Function<MethodHandle, T> factory;
     private final Function<T, T> wrap;
 
-    private final LoadingCache<Method, Optional<T>> methodCache = CacheBuilder
+    private final LoadingCache<Method, Optional<T>> instanceCache = CacheBuilder
         .newBuilder()
-        .build(CacheLoader.from(catching(this::build, Optional.empty())));
+        .build(CacheLoader.from(catching(this::buildInstanceMethod, Optional.empty())));
+
+    private final LoadingCache<GenericMethod, Optional<T>> genericCache = CacheBuilder
+        .newBuilder()
+        .weakKeys()
+        .build(CacheLoader.from(catching(this::buildGenericMethod, Optional.empty())));
 
     Generator(List<Class<?>> context, Function<MethodHandle, T> factory, Function<T, T> wrap) {
         this.context = context;
@@ -131,65 +133,94 @@ final class Generator<T> {
         }
     }
 
-    Optional<T> getMethod(Method method) {
+    Optional<T> getInstanceMethod(Method method) {
-        return methodCache.getUnchecked(method);
+        return instanceCache.getUnchecked(method);
     }
 
-    private Optional<T> build(Method method) {
+    Optional<T> getGenericMethod(GenericMethod method) {
-        var name = method.getDeclaringClass().getName() + "." + method.getName();
+        return genericCache.getUnchecked(method);
-        var modifiers = method.getModifiers();
-
-        // Instance methods must be final - this prevents them being overridden and potentially exposed twice.
-        if (!Modifier.isStatic(modifiers) && !Modifier.isFinal(modifiers)) {
-            LOG.warn("Lua Method {} should be final.", name);
     }
 
-        if (!Modifier.isPublic(modifiers)) {
+    /**
-            LOG.error("Lua Method {} should be a public method.", name);
+     * Check if a {@link LuaFunction}-annotated method can be used in this context.
-            return Optional.empty();
+     *
+     * @param method The method to check.
+     * @return Whether the method is valid.
+     */
+    private boolean checkMethod(Method method) {
+        if (method.isBridge()) {
+            LOG.debug("Skipping bridge Lua Method {}.{}", method.getDeclaringClass().getName(), method.getName());
+            return false;
         }
 
-        if (!Modifier.isPublic(method.getDeclaringClass().getModifiers())) {
+        // Check we don't throw additional exceptions.
-            LOG.error("Lua Method {} should be on a public class.", name);
-            return Optional.empty();
-        }
-
-        LOG.debug("Generating method wrapper for {}.", name);
-
         var exceptions = method.getExceptionTypes();
         for (var exception : exceptions) {
             if (exception != LuaException.class) {
-                LOG.error("Lua Method {} cannot throw {}.", name, exception.getName());
+                LOG.error("Lua Method {}.{} cannot throw {}.", method.getDeclaringClass().getName(), method.getName(), exception.getName());
-                return Optional.empty();
+                return false;
             }
         }
 
+        // unsafe can only be used on the computer thread, so reject it for mainThread functions.
         var annotation = method.getAnnotation(LuaFunction.class);
         if (annotation.unsafe() && annotation.mainThread()) {
-            LOG.error("Lua Method {} cannot use unsafe and mainThread", name);
+            LOG.error("Lua Method {}.{} cannot use unsafe and mainThread.", method.getDeclaringClass().getName(), method.getName());
-            return Optional.empty();
+            return false;
         }
 
-        try {
+        // Instance methods must be final - this prevents them being overridden and potentially exposed twice.
-            var originalHandle = LOOKUP.unreflect(method);
+        var modifiers = method.getModifiers();
-
+        if (!Modifier.isStatic(modifiers) && !Modifier.isFinal(modifiers)) {
-            List<Type> parameters;
+            LOG.warn("Lua Method {}.{} should be final.", method.getDeclaringClass().getName(), method.getName());
-            if (Modifier.isStatic(modifiers)) {
-                var allParameters = method.getGenericParameterTypes();
-                parameters = Arrays.asList(allParameters).subList(1, allParameters.length);
-            } else {
-                parameters = Arrays.asList(method.getGenericParameterTypes());
         }
 
-            var handle = buildMethodHandle(method, originalHandle, parameters, annotation.unsafe());
+        return true;
+    }
+
+    private Optional<T> buildInstanceMethod(Method method) {
+        if (!checkMethod(method)) return Optional.empty();
+
+        var handle = tryUnreflect(method);
         if (handle == null) return Optional.empty();
 
-            var instance = factory.apply(handle);
+        return build(method, handle, Arrays.asList(method.getGenericParameterTypes()));
-            return Optional.of(annotation.mainThread() ? wrap.apply(instance) : instance);
-        } catch (ReflectiveOperationException | RuntimeException e) {
-            LOG.error("Error generating wrapper for {}.", name, e);
-            return Optional.empty();
     }
+
+    private Optional<T> buildGenericMethod(GenericMethod method) {
+        if (!checkMethod(method.method)) return Optional.empty();
+
+        var handle = tryUnreflect(method.method);
+        if (handle == null) return Optional.empty();
+
+        var parameters = Arrays.asList(method.method.getGenericParameterTypes());
+        return build(
+            method.method,
+            Modifier.isStatic(method.method.getModifiers()) ? handle : handle.bindTo(method.source),
+            parameters.subList(1, parameters.size()) // Drop the instance argument.
+        );
+    }
+
+    /**
+     * Generate our {@link T} instance for a specific method.
+     * <p>
+     * This {@linkplain #buildMethodHandle(Member, MethodHandle, List, boolean)} builds the method handle, and then
+     * wraps it with {@link #factory}.
+     *
+     * @param method     The original method, for reflection and error reporting.
+     * @param handle     The method handle to execute.
+     * @param parameters The generic parameters to this method handle.
+     * @return The generated method, or {@link Optional#empty()} if an error occurred.
+     */
+    private Optional<T> build(Method method, MethodHandle handle, List<Type> parameters) {
+        LOG.debug("Generating method wrapper for {}.{}.", method.getDeclaringClass().getName(), method.getName());
+
+        var annotation = method.getAnnotation(LuaFunction.class);
+        var wrappedHandle = buildMethodHandle(method, handle, parameters, annotation.unsafe());
+        if (wrappedHandle == null) return Optional.empty();
+
+        var instance = factory.apply(wrappedHandle);
+        return Optional.of(annotation.mainThread() ? wrap.apply(instance) : instance);
     }
 
     /**
@@ -202,8 +233,7 @@ final class Generator<T> {
      * @param unsafe         Whether to allow unsafe argument getters.
      * @return The wrapped method handle.
      */
-    @Nullable
+    private @Nullable MethodHandle buildMethodHandle(Member method, MethodHandle handle, List<Type> parameterTypes, boolean unsafe) {
-    private MethodHandle buildMethodHandle(Member method, MethodHandle handle, List<Type> parameterTypes, boolean unsafe) {
         if (handle.type().parameterCount() != parameterTypes.size() + 1) {
             throw new IllegalArgumentException("Argument lists are mismatched");
         }
@@ -263,8 +293,7 @@ final class Generator<T> {
         }
     }
 
-    @Nullable
+    private static @Nullable MethodHandle loadArg(Member method, boolean unsafe, Class<?> argType, Type genericArg, int argIndex) {
-    private static MethodHandle loadArg(Member method, boolean unsafe, Class<?> argType, Type genericArg, int argIndex) {
         if (argType == Coerced.class) {
             var klass = Reflect.getRawType(method, TypeToken.of(genericArg).resolveType(Reflect.COERCED_IN).getType(), false);
             if (klass == null) return null;
@@ -312,6 +341,22 @@ final class Generator<T> {
         return null;
     }
 
+    /**
+     * A wrapper over {@link MethodHandles.Lookup#unreflect(Method)} which discards errors.
+     *
+     * @param method The method to unreflect.
+     * @return The resulting handle, or {@code null} if it cannot be unreflected.
+     */
+    private static @Nullable MethodHandle tryUnreflect(Method method) {
+        try {
+            method.setAccessible(true);
+            return LOOKUP.unreflect(method);
+        } catch (SecurityException | InaccessibleObjectException | IllegalAccessException e) {
+            LOG.error("Lua Method {}.{} is not accessible.", method.getDeclaringClass().getName(), method.getName());
+            return null;
+        }
+    }
+
     @SuppressWarnings("Guava")
     static <T, U> com.google.common.base.Function<T, U> catching(Function<T, U> function, U def) {
         return x -> {
@@ -320,7 +365,7 @@ final class Generator<T> {
             } catch (Exception | LinkageError e) {
                 // LinkageError due to possible codegen bugs and NoClassDefFoundError. The latter occurs when fetching
                 // methods on a class which references non-existent (i.e. client-only) types.
-                LOG.error("Error generating @LuaFunctions", e);
+                LOG.error("Error generating @LuaFunction for {}", x, e);
                 return def;
             }
         };

projects/core/src/main/java/dan200/computercraft/core/asm/GenericMethod.java

@@ -13,7 +13,6 @@ import org.slf4j.LoggerFactory;
 
 import javax.annotation.Nullable;
 import java.lang.reflect.Method;
-import java.lang.reflect.Modifier;
 import java.util.Arrays;
 import java.util.Objects;
 import java.util.stream.Stream;
@@ -56,16 +55,11 @@ public final class GenericMethod {
         Class<?> klass = source.getClass();
         var type = source instanceof GenericPeripheral generic ? generic.getType() : null;
 
-        return Arrays.stream(klass.getDeclaredMethods())
+        return Arrays.stream(klass.getMethods())
             .map(method -> {
                 var annotation = method.getAnnotation(LuaFunction.class);
                 if (annotation == null) return null;
 
-                if (!Modifier.isStatic(method.getModifiers())) {
-                    LOG.error("GenericSource method {}.{} should be static.", method.getDeclaringClass(), method.getName());
-                    return null;
-                }
-
                 var types = method.getGenericParameterTypes();
                 if (types.length == 0) {
                     LOG.error("GenericSource method {}.{} has no parameters.", method.getDeclaringClass(), method.getName());

projects/core/src/main/java/dan200/computercraft/core/asm/MethodSupplierImpl.java

@@ -110,7 +110,7 @@ final class MethodSupplierImpl<T> implements MethodSupplier<T> {
                 continue;
             }
 
-            var instance = generator.getMethod(method).orElse(null);
+            var instance = generator.getInstanceMethod(method).orElse(null);
             if (instance == null) continue;
 
             if (methods == null) methods = new ArrayList<>();
@@ -121,7 +121,7 @@ final class MethodSupplierImpl<T> implements MethodSupplier<T> {
         for (var method : genericMethods) {
             if (!method.target.isAssignableFrom(klass)) continue;
 
-            var instance = generator.getMethod(method.method).orElse(null);
+            var instance = generator.getGenericMethod(method).orElse(null);
             if (instance == null) continue;
 
             if (methods == null) methods = new ArrayList<>();

projects/core/src/test/java/dan200/computercraft/core/asm/GeneratorTest.java

@@ -22,6 +22,7 @@ import java.util.Collection;
 import java.util.Map;
 import java.util.Objects;
 import java.util.Optional;
+import java.util.stream.Stream;
 
 import static dan200.computercraft.test.core.ContramapMatcher.contramap;
 import static org.hamcrest.MatcherAssert.assertThat;
@@ -30,7 +31,7 @@ import static org.junit.jupiter.api.Assertions.assertThrows;
 
 public class GeneratorTest {
     private static final MethodSupplierImpl<LuaMethod> GENERATOR = (MethodSupplierImpl<LuaMethod>) LuaMethodSupplier.create(
-        GenericMethod.getMethods(new StaticMethod()).toList()
+        Stream.of(new StaticGeneric(), new InstanceGeneric()).flatMap(GenericMethod::getMethods).toList()
     );
 
     @Test
@@ -65,8 +66,10 @@ public class GeneratorTest {
     }
 
     @Test
-    public void testNonPublicClass() {
+    public void testNonPublicClass() throws LuaException {
-        assertThat(GENERATOR.getMethods(NonPublic.class), is(empty()));
+        var methods = GENERATOR.getMethods(NonPublic.class);
+        assertThat(methods, contains(named("go")));
+        assertThat(apply(methods, new NonPublic(), "go"), is(MethodResult.of()));
     }
 
     @Test
@@ -75,10 +78,18 @@ public class GeneratorTest {
     }
 
     @Test
-    public void testStaticMethod() throws LuaException {
+    public void testStaticGenericMethod() throws LuaException {
-        var methods = GENERATOR.getMethods(StaticMethodTarget.class);
+        var methods = GENERATOR.getMethods(GenericMethodTarget.class);
-        assertThat(methods, contains(named("go")));
+        assertThat(methods, hasItem(named("goStatic")));
-        assertThat(apply(methods, new StaticMethodTarget(), "go", "Hello", 123), is(MethodResult.of()));
+        assertThat(apply(methods, new GenericMethodTarget(), "goStatic", "Hello", 123), is(MethodResult.of()));
+    }
+
+
+    @Test
+    public void testInstanceGenericrMethod() throws LuaException {
+        var methods = GENERATOR.getMethods(GenericMethodTarget.class);
+        assertThat(methods, hasItem(named("goInstance")));
+        assertThat(apply(methods, new GenericMethodTarget(), "goInstance", "Hello", 123), is(MethodResult.of()));
     }
 
     @Test
@@ -181,17 +192,28 @@ public class GeneratorTest {
         }
     }
 
-    public static class StaticMethodTarget {
+    public static class GenericMethodTarget {
     }
 
-    public static class StaticMethod implements GenericSource {
+    public static class StaticGeneric implements GenericSource {
         @Override
         public String id() {
-            return "source";
+            return "static";
         }
 
         @LuaFunction
-        public static void go(StaticMethodTarget target, String arg1, int arg2, ILuaContext context) {
+        public static void goStatic(GenericMethodTarget target, String arg1, int arg2, ILuaContext context) {
+        }
+    }
+
+    public static class InstanceGeneric implements GenericSource {
+        @Override
+        public String id() {
+            return "instance";
+        }
+
+        @LuaFunction
+        public void goInstance(GenericMethodTarget target, String arg1, int arg2, ILuaContext context) {
         }
     }