Skip to content

Enhance constructor invocation handling and matching logic #261

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
twisti-dev wants to merge 1 commit into from
Closed

Enhance constructor invocation handling and matching logic #261

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion gradle.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,6 @@ org.jetbrains.dokka.experimental.gradle.pluginMode=V2Enabled
javaVersion=25
mcVersion=1.21.11
group=dev.slne.surf
version=1.21.11-2.70.2
version=1.21.11-2.70.3
relocationPrefix=dev.slne.surf.surfapi.libs
snapshot=false
159 changes: 149 additions & 10 deletions ...ain/java/dev/slne/surf/surfapi/core/server/impl/reflection/SurfInvocationHandlerJava.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,10 +115,7 @@ private Invokable createInvokable(final Method method) {
}

if (constructorAnnotation != null) {
final var handle = sneaky(
() -> privateLookup.unreflectConstructor(findConstructor(proxiedClass, method)));
final boolean hasParams = handle.type().parameterCount() > 0;
return new HandleInvokable(normalizeMethodHandleType(handle), hasParams);
return new ConstructorInvokable(proxiedClass, method, privateLookup);
}

if (staticAnnotation == null && method.getParameterCount() == 0) {
Expand Down Expand Up @@ -228,11 +225,132 @@ private static Field findField(final Class<?> clazz, final String name)
return field;
}

private static Constructor<?> findConstructor(final Class<?> clazz, final Method method)
throws NoSuchMethodException {
final Constructor<?> constructor = clazz.getDeclaredConstructor(method.getParameterTypes());
constructor.setAccessible(true);
return constructor;
private static Constructor<?> findConstructor(
final Class<?> clazz,
final Method method,
final Object[] args
) throws NoSuchMethodException {

final Class<?>[] declaredTypes = method.getParameterTypes();

try {
final Constructor<?> exact = clazz.getDeclaredConstructor(declaredTypes);
exact.trySetAccessible();
return exact;
} catch (final NoSuchMethodException ignored) {
}

final Constructor<?>[] constructors = clazz.getDeclaredConstructors();
Constructor<?> best = null;
int bestScore = Integer.MIN_VALUE;

for (final Constructor<?> constructor : constructors) {
final Class<?>[] ctorTypes = constructor.getParameterTypes();

if (ctorTypes.length != declaredTypes.length) {
continue;
}

final int score = scoreExecutableMatch(ctorTypes, declaredTypes, args);
if (score > bestScore) {
best = constructor;
bestScore = score;
}
Comment on lines +254 to +258
Copy link

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Constructor selection uses a score, but ties (equal score) currently fall back to the first constructor returned by reflection. Since reflection order isn’t specified, this can make constructor choice nondeterministic/unstable when multiple overloads are equally compatible (e.g., multiple interface-typed params that the runtime arg implements). Consider adding a deterministic tie-breaker (e.g., prefer the most specific parameter types) or detect ties and throw an explicit ambiguity error requiring a more specific proxy method signature.

Copilot uses AI. Check for mistakes.
}

if (best == null) {
throw new NoSuchMethodException(
"No compatible constructor found for " + clazz.getName()
+ " with declared parameters " + Arrays.toString(declaredTypes)
+ " and runtime arguments " + runtimeTypesToString(args)
);
}

best.setAccessible(true);
Copy link

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

best.setAccessible(true) can throw InaccessibleObjectException under strong encapsulation, and it’s not clear it’s needed here since invocation uses a privateLookup + unreflectConstructor. Prefer trySetAccessible() (and handle failure) or remove the reflective accessibility change if Lookup access is sufficient.

Suggested change
best.setAccessible(true);
best.trySetAccessible();

Copilot uses AI. Check for mistakes.
return best;
}

private static int scoreExecutableMatch(
final Class<?>[] targetTypes,
final Class<?>[] declaredTypes,
final Object @Nullable [] args
) {
int score = 0;

for (int i = 0; i < targetTypes.length; i++) {
final Class<?> target = wrap(targetTypes[i]);
final Class<?> declared = wrap(declaredTypes[i]);
final Object arg = args != null && i < args.length ? args[i] : null;

if (arg == null) {
if (targetTypes[i].isPrimitive()) {
return Integer.MIN_VALUE;
}

if (declared.equals(target)) {
score += 20;
} else if (target.isAssignableFrom(declared)) {
score += 10;
} else {
score += 1;
}
continue;
}

final Class<?> runtime = wrap(arg.getClass());

if (!target.isAssignableFrom(runtime)) {
return Integer.MIN_VALUE;
}

if (target.equals(runtime)) {
score += 100;
} else if (target.equals(declared)) {
score += 50;
} else if (target.isAssignableFrom(declared)) {
score += 25;
} else {
score += 10;
}
}

return score;
}

private static Class<?> wrap(final Class<?> type) {
if (!type.isPrimitive()) {
return type;
}

if (type == boolean.class) return Boolean.class;
if (type == byte.class) return Byte.class;
if (type == short.class) return Short.class;
if (type == char.class) return Character.class;
if (type == int.class) return Integer.class;
if (type == long.class) return Long.class;
if (type == float.class) return Float.class;
if (type == double.class) return Double.class;
if (type == void.class) return Void.class;

return type;
}

private static String runtimeTypesToString(final @Nullable Object @Nullable [] args) {
if (args == null) {
return "[]";
}

final StringBuilder builder = new StringBuilder("[");
for (int i = 0; i < args.length; i++) {
if (i > 0) {
builder.append(", ");
}

final Object arg = args[i];
builder.append(arg == null ? "null" : arg.getClass().getName());
}
builder.append(']');
return builder.toString();
}

private static Method findMethod(
Expand Down Expand Up @@ -348,7 +466,7 @@ private static boolean isEqualsHashOrToStringMethod(final Method method) {
return isEqualsMethod(method) || isHashCodeMethod(method) || isToStringMethod(method);
}

private sealed interface Invokable permits HandleInvokable, ReflectionSetterInvokable, VarHandleSetterInvokable {
private sealed interface Invokable permits ConstructorInvokable, HandleInvokable, ReflectionSetterInvokable, VarHandleSetterInvokable {

@Nullable
Object invoke(final Object[] args) throws Throwable;
Expand All @@ -367,6 +485,27 @@ private record HandleInvokable(MethodHandle handle, boolean hasParams) implement
}
}

private record ConstructorInvokable(
Class<?> proxiedClass,
Method method,
MethodHandles.Lookup privateLookup
) implements Invokable {

@Override
public @Nullable Object invoke(final Object[] args) throws Throwable {
final Constructor<?> constructor = findConstructor(proxiedClass, method, args);
final MethodHandle handle = normalizeMethodHandleType(
privateLookup.unreflectConstructor(constructor)
);

Comment on lines +494 to +500
Copy link

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ConstructorInvokable.invoke resolves the constructor, unreflects it, and normalizes the method handle on every call. Compared to the previous approach (creating the handle once in createInvokable), this is a noticeable performance regression for hot paths. Consider caching the selected MethodHandle (at least for the exact declared signature, and optionally per runtime-arg-type signature) inside ConstructorInvokable to avoid repeated reflective scans and handle creation.

Suggested change
@Override
public @Nullable Object invoke(final Object[] args) throws Throwable {
final Constructor<?> constructor = findConstructor(proxiedClass, method, args);
final MethodHandle handle = normalizeMethodHandleType(
privateLookup.unreflectConstructor(constructor)
);
/**
* Cached method handle for the last-resolved constructor signature.
* This avoids repeated reflective scans and handle creation on hot paths.
*/
private volatile MethodHandle cachedHandle;
/**
* Runtime parameter types corresponding to {@link #cachedHandle}.
*/
private volatile Class<?>[] cachedParamTypes;
@Override
public @Nullable Object invoke(final Object[] args) throws Throwable {
// Determine the current runtime argument types (nulls are represented as Object.class)
final Class<?>[] currentTypes;
if (args.length == 0) {
currentTypes = new Class<?>[0];
} else {
currentTypes = new Class<?>[args.length];
for (int i = 0; i < args.length; i++) {
final Object arg = args[i];
currentTypes[i] = (arg != null) ? arg.getClass() : Object.class;
}
}
MethodHandle handle = cachedHandle;
final Class<?>[] cachedTypes = cachedParamTypes;
// Recompute the handle if we have no cache yet or the runtime signature differs
if (handle == null || cachedTypes == null || !Arrays.equals(cachedTypes, currentTypes)) {
final Constructor<?> constructor = findConstructor(proxiedClass, method, args);
handle = normalizeMethodHandleType(
privateLookup.unreflectConstructor(constructor)
);
// Publish the new cache (benign races between threads are acceptable)
cachedHandle = handle;
cachedParamTypes = currentTypes;
}

Copilot uses AI. Check for mistakes.
if (handle.type().parameterCount() > 0) {
return handle.invokeExact(args);
} else {
return handle.invokeExact();
}
}
}

private record ReflectionSetterInvokable(Field field, boolean isStatic) implements Invokable {

@Override
Expand Down
Loading