Skip to content

Commit

Permalink
Allow generic sources to have instance methods
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@SquidDev
SquidDev committed Nov 22, 2023
1 parent f8b7422 commit fe826f5
Show file tree
Hide file tree
Showing 5 changed files with 137 additions and 77 deletions.
9 changes: 4 additions & 5 deletions projects/core-api/src/main/java/dan200/computercraft/api/lua/GenericSource.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,10 @@
* 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
* methods onto objects you do not own, as well as declaring methods for a specific "trait" (for instance, a Forge
* capability or Fabric block lookup interface).
* accept their target as the first parameter. This allows you to inject methods onto objects you do not own, as well as
* declaring methods for a specific "trait" (for instance, a Forge 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()}.
Expand All @@ -25,7 +24,7 @@
* <pre>{@code
* public class InventoryMethods implements GenericSource {
* \@LuaFunction( mainThread = true )
* public static int size(IItemHandler inventory) {
* public int size(IItemHandler inventory) {
* return inventory.getSlots();
* }
*
Expand Down
149 changes: 97 additions & 52 deletions projects/core/src/main/java/dan200/computercraft/core/asm/Generator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,7 @@
import java.lang.invoke.MethodHandle;
import java.lang.invoke.MethodHandles;
import java.lang.invoke.MethodType;
import java.lang.reflect.Member;
import java.lang.reflect.Method;
import java.lang.reflect.Modifier;
import java.lang.reflect.Type;
import java.lang.reflect.*;
import java.nio.ByteBuffer;
import java.util.*;
import java.util.function.Function;
Expand Down Expand Up @@ -106,9 +103,14 @@ static void addArgType(Map<Class<?>, ArgMethods> types, Class<?> type, String na
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;
Expand All @@ -131,65 +133,94 @@ static void addArgType(Map<Class<?>, ArgMethods> types, Class<?> type, String na
}
}

Optional<T> getMethod(Method method) {
return methodCache.getUnchecked(method);
Optional<T> getInstanceMethod(Method method) {
return instanceCache.getUnchecked(method);
}

private Optional<T> build(Method method) {
var name = method.getDeclaringClass().getName() + "." + method.getName();
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);
return Optional.empty();
}
Optional<T> getGenericMethod(GenericMethod method) {
return genericCache.getUnchecked(method);
}

if (!Modifier.isPublic(method.getDeclaringClass().getModifiers())) {
LOG.error("Lua Method {} should be on a public class.", name);
return Optional.empty();
/**
* Check if a {@link LuaFunction}-annotated method can be used in this context.
*
* @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;
}

LOG.debug("Generating method wrapper for {}.", name);

// Check we don't throw additional exceptions.
var exceptions = method.getExceptionTypes();
for (var exception : exceptions) {
if (exception != LuaException.class) {
LOG.error("Lua Method {} cannot throw {}.", name, exception.getName());
return Optional.empty();
LOG.error("Lua Method {}.{} cannot throw {}.", method.getDeclaringClass().getName(), method.getName(), exception.getName());
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);
return Optional.empty();
LOG.error("Lua Method {}.{} cannot use unsafe and mainThread.", method.getDeclaringClass().getName(), method.getName());
return false;
}

try {
var originalHandle = LOOKUP.unreflect(method);
// Instance methods must be final - this prevents them being overridden and potentially exposed twice.
var modifiers = method.getModifiers();
if (!Modifier.isStatic(modifiers) && !Modifier.isFinal(modifiers)) {
LOG.warn("Lua Method {}.{} should be final.", method.getDeclaringClass().getName(), method.getName());
}

List<Type> parameters;
if (Modifier.isStatic(modifiers)) {
var allParameters = method.getGenericParameterTypes();
parameters = Arrays.asList(allParameters).subList(1, allParameters.length);
} else {
parameters = Arrays.asList(method.getGenericParameterTypes());
}
return true;
}

var handle = buildMethodHandle(method, originalHandle, parameters, annotation.unsafe());
if (handle == null) return Optional.empty();
private Optional<T> buildInstanceMethod(Method method) {
if (!checkMethod(method)) return Optional.empty();

var instance = factory.apply(handle);
return Optional.of(annotation.mainThread() ? wrap.apply(instance) : instance);
} catch (ReflectiveOperationException | RuntimeException e) {
LOG.error("Error generating wrapper for {}.", name, e);
return Optional.empty();
}
var handle = tryUnreflect(method);
if (handle == null) return Optional.empty();

return build(method, handle, Arrays.asList(method.getGenericParameterTypes()));
}

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);
}

/**
Expand All @@ -202,8 +233,7 @@ private Optional<T> build(Method method) {
* @param unsafe Whether to allow unsafe argument getters.
* @return The wrapped method handle.
*/
@Nullable
private MethodHandle buildMethodHandle(Member method, MethodHandle handle, List<Type> parameterTypes, boolean unsafe) {
private @Nullable 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");
}
Expand Down Expand Up @@ -263,8 +293,7 @@ private MethodHandle buildMethodHandle(Member method, MethodHandle handle, List<
}
}

@Nullable
private static MethodHandle loadArg(Member method, boolean unsafe, Class<?> argType, Type genericArg, int argIndex) {
private static @Nullable 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;
Expand Down Expand Up @@ -312,6 +341,22 @@ private static MethodHandle setReturn(MethodHandle handle, Class<?> retTy) {
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 -> {
Expand All @@ -320,7 +365,7 @@ static <T, U> com.google.common.base.Function<T, U> catching(Function<T, U> func
} 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;
}
};
Expand Down
8 changes: 1 addition & 7 deletions projects/core/src/main/java/dan200/computercraft/core/asm/GenericMethod.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@

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;
Expand Down Expand Up @@ -56,16 +55,11 @@ public static Stream<GenericMethod> getMethods(GenericSource source) {
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());
Expand Down
4 changes: 2 additions & 2 deletions projects/core/src/main/java/dan200/computercraft/core/asm/MethodSupplierImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ private List<NamedMethod<T>> getMethodsImpl(Class<?> klass) {
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<>();
Expand All @@ -121,7 +121,7 @@ private List<NamedMethod<T>> getMethodsImpl(Class<?> klass) {
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<>();
Expand Down
44 changes: 33 additions & 11 deletions projects/core/src/test/java/dan200/computercraft/core/asm/GeneratorTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
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;
Expand All @@ -30,7 +31,7 @@

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
Expand Down Expand Up @@ -65,8 +66,10 @@ public void testEmptyClass() {
}

@Test
public void testNonPublicClass() {
assertThat(GENERATOR.getMethods(NonPublic.class), is(empty()));
public void testNonPublicClass() throws LuaException {
var methods = GENERATOR.getMethods(NonPublic.class);
assertThat(methods, contains(named("go")));
assertThat(apply(methods, new NonPublic(), "go"), is(MethodResult.of()));
}

@Test
Expand All @@ -75,10 +78,18 @@ public void testNonInstance() {
}

@Test
public void testStaticMethod() throws LuaException {
var methods = GENERATOR.getMethods(StaticMethodTarget.class);
assertThat(methods, contains(named("go")));
assertThat(apply(methods, new StaticMethodTarget(), "go", "Hello", 123), is(MethodResult.of()));
public void testStaticGenericMethod() throws LuaException {
var methods = GENERATOR.getMethods(GenericMethodTarget.class);
assertThat(methods, hasItem(named("goStatic")));
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
Expand Down Expand Up @@ -181,17 +192,28 @@ public static void go() {
}
}

public static class StaticMethodTarget {
public static class GenericMethodTarget {
}

public static class StaticGeneric implements GenericSource {
@Override
public String id() {
return "static";
}

@LuaFunction
public static void goStatic(GenericMethodTarget target, String arg1, int arg2, ILuaContext context) {
}
}

public static class StaticMethod implements GenericSource {
public static class InstanceGeneric implements GenericSource {
@Override
public String id() {
return "source";
return "instance";
}

@LuaFunction
public static void go(StaticMethodTarget target, String arg1, int arg2, ILuaContext context) {
public void goInstance(GenericMethodTarget target, String arg1, int arg2, ILuaContext context) {
}
}

Expand Down

0 comments on commit fe826f5

Please sign in to comment.