Skip to content

Commit

Permalink
Enforce need for type reflection in RuntimeHintsAgent
Browse files Browse the repository at this point in the history 
Prior to this commit, the AOT infrastructure would rely on the fact that
native runtime reflection on a type would only consider
methods/fields/constructors that had specific hints contributed. When
listing them through the reflection API on the type, the native image
would only return those for which we had hints contributed.
This behavior will soon change in GraalVM and will better align with the
JVM behavior: when asking for all declared methods on a type in a native
image, we should get all existing methods, not just the ones registered
previously in the native image.

This commit aligns the behavior of the `RuntimeHintsAgent` and removes
the now misleading predicates as a consequence.

Closes gh-29205
  • Loading branch information
@bclozel
bclozel committed Oct 3, 2022
1 parent ce46170 commit 43d39d4
Show file tree
Hide file tree
Showing 6 changed files with 18 additions and 158 deletions.
16 changes: 0 additions & 16 deletions integration-tests/src/test/java/org/springframework/aot/RuntimeHintsAgentTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -164,22 +164,6 @@ private static Stream<Arguments> instrumentedReflectionMethods() {
throw new RuntimeException(e);
}
}, MethodReference.of(Method.class, "invoke")),
Arguments.of((Runnable) () -> {
try {
toStringMethod.getAnnotations();
}
catch (Exception e) {
throw new RuntimeException(e);
}
}, MethodReference.of(Method.class, "getAnnotations")),
Arguments.of((Runnable) () -> {
try {
toStringMethod.getParameterTypes();
}
catch (Exception e) {
throw new RuntimeException(e);
}
}, MethodReference.of(Method.class, "getParameterTypes")),
Arguments.of((Runnable) () -> {
try {
privateGreetMethod.invoke(new PrivateClass());
Expand Down
27 changes: 0 additions & 27 deletions spring-core-test/src/main/java/org/springframework/aot/agent/InstrumentedBridgeMethods.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@

import java.io.IOException;
import java.io.InputStream;
import java.lang.annotation.Annotation;
import java.lang.reflect.Constructor;
import java.lang.reflect.Field;
import java.lang.reflect.InvocationHandler;
Expand Down Expand Up @@ -336,32 +335,6 @@ public static Object constructornewInstance(Constructor<?> constructor, Object..
* Bridge methods for java.lang.reflect.Method
*/

public static Annotation[] methodgetAnnotations(Method method) {
Annotation[] result = null;
try {
result = method.getAnnotations();
}
finally {
RecordedInvocation invocation = RecordedInvocation.of(InstrumentedMethod.METHOD_GETANNOTATIONS)
.onInstance(method).returnValue(result).build();
RecordedInvocationsPublisher.publish(invocation);
}
return result;
}

public static Class<?>[] methodgetParameterTypes(Method method) {
Class<?>[] result = null;
try {
result = method.getParameterTypes();
}
finally {
RecordedInvocation invocation = RecordedInvocation.of(InstrumentedMethod.METHOD_GETPARAMETERTYPES)
.onInstance(method).returnValue(result).build();
RecordedInvocationsPublisher.publish(invocation);
}
return result;
}

public static Object methodinvoke(Method method, Object object, Object... arguments) throws InvocationTargetException, IllegalAccessException {
Object result = null;
boolean accessibilityChanged = false;
Expand Down
27 changes: 6 additions & 21 deletions spring-core-test/src/main/java/org/springframework/aot/agent/InstrumentedMethod.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -93,8 +93,7 @@ enum InstrumentedMethod {
Class<?> thisClass = invocation.getInstance();
return reflection().onType(TypeReference.of(thisClass)).withAnyMemberCategory(
MemberCategory.INTROSPECT_PUBLIC_CONSTRUCTORS, MemberCategory.INTROSPECT_DECLARED_CONSTRUCTORS,
MemberCategory.INVOKE_PUBLIC_CONSTRUCTORS, MemberCategory.INVOKE_DECLARED_CONSTRUCTORS)
.or(reflection().onType(TypeReference.of(thisClass)).withAnyConstructor());
MemberCategory.INVOKE_PUBLIC_CONSTRUCTORS, MemberCategory.INVOKE_DECLARED_CONSTRUCTORS);
}
),

Expand Down Expand Up @@ -130,8 +129,7 @@ enum InstrumentedMethod {
invocation -> {
Class<?> thisClass = invocation.getInstance();
return reflection().onType(TypeReference.of(thisClass))
.withAnyMemberCategory(MemberCategory.INTROSPECT_DECLARED_CONSTRUCTORS, MemberCategory.INVOKE_DECLARED_CONSTRUCTORS)
.or(reflection().onType(TypeReference.of(thisClass)).withAnyConstructor());
.withAnyMemberCategory(MemberCategory.INTROSPECT_DECLARED_CONSTRUCTORS, MemberCategory.INVOKE_DECLARED_CONSTRUCTORS);
}),

/**
Expand All @@ -155,8 +153,7 @@ enum InstrumentedMethod {
CLASS_GETDECLAREDFIELDS(Class.class, "getDeclaredFields", HintType.REFLECTION,
invocation -> {
Class<?> thisClass = invocation.getInstance();
return reflection().onType(TypeReference.of(thisClass)).withMemberCategory(MemberCategory.DECLARED_FIELDS)
.or(reflection().onType(TypeReference.of(thisClass)).withAnyField());
return reflection().onType(TypeReference.of(thisClass)).withMemberCategory(MemberCategory.DECLARED_FIELDS);
}
),

Expand All @@ -183,8 +180,7 @@ enum InstrumentedMethod {
invocation -> {
Class<?> thisClass = invocation.getInstance();
return reflection().onType(TypeReference.of(thisClass))
.withAnyMemberCategory(MemberCategory.INTROSPECT_DECLARED_METHODS, MemberCategory.INVOKE_DECLARED_METHODS)
.or(reflection().onType(TypeReference.of(thisClass)).withAnyMethod());
.withAnyMemberCategory(MemberCategory.INTROSPECT_DECLARED_METHODS, MemberCategory.INVOKE_DECLARED_METHODS);
}
),

Expand All @@ -211,8 +207,7 @@ enum InstrumentedMethod {
invocation -> {
Class<?> thisClass = invocation.getInstance();
return reflection().onType(TypeReference.of(thisClass))
.withAnyMemberCategory(MemberCategory.PUBLIC_FIELDS, MemberCategory.DECLARED_FIELDS)
.or(reflection().onType(TypeReference.of(thisClass)).withAnyField());
.withAnyMemberCategory(MemberCategory.PUBLIC_FIELDS, MemberCategory.DECLARED_FIELDS);
}
),

Expand Down Expand Up @@ -242,8 +237,7 @@ enum InstrumentedMethod {
Class<?> thisClass = invocation.getInstance();
return reflection().onType(TypeReference.of(thisClass)).withAnyMemberCategory(
MemberCategory.INTROSPECT_PUBLIC_METHODS, MemberCategory.INTROSPECT_DECLARED_METHODS,
MemberCategory.INVOKE_PUBLIC_METHODS, MemberCategory.INVOKE_DECLARED_METHODS)
.or(reflection().onType(TypeReference.of(thisClass)).withAnyMethod());
MemberCategory.INVOKE_PUBLIC_METHODS, MemberCategory.INVOKE_DECLARED_METHODS);
}
),

Expand All @@ -265,15 +259,6 @@ enum InstrumentedMethod {
CONSTRUCTOR_NEWINSTANCE(Constructor.class, "newInstance", HintType.REFLECTION,
invocation -> reflection().onConstructor(invocation.getInstance()).invoke()),

/**
* {@link Method#getParameterTypes()}.
*/
METHOD_GETANNOTATIONS(Method.class, "getAnnotations", HintType.REFLECTION,
invocation -> reflection().onMethod(invocation.getInstance())),

METHOD_GETPARAMETERTYPES(Method.class, "getParameterTypes", HintType.REFLECTION,
invocation -> reflection().onMethod(invocation.getInstance())),

/**
* {@link Method#invoke(Object, Object...)}.
*/
Expand Down
47 changes: 12 additions & 35 deletions spring-core-test/src/test/java/org/springframework/aot/agent/InstrumentedMethodTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@

package org.springframework.aot.agent;

import java.lang.reflect.Method;
import java.lang.reflect.Proxy;
import java.util.Collections;
import java.util.Comparator;
Expand Down Expand Up @@ -193,9 +192,9 @@ void classGetConstructorsShouldNotMatchTypeReflectionHint() {
}

@Test
void classGetConstructorsShouldMatchConstructorReflectionHint() throws Exception {
void classGetConstructorsShouldNotMatchConstructorReflectionHint() throws Exception {
hints.reflection().registerConstructor(String.class.getConstructor(), ExecutableMode.INVOKE);
assertThatInvocationMatches(InstrumentedMethod.CLASS_GETCONSTRUCTORS, this.stringGetConstructors);
assertThatInvocationDoesNotMatch(InstrumentedMethod.CLASS_GETCONSTRUCTORS, this.stringGetConstructors);
}

@Test
Expand Down Expand Up @@ -249,9 +248,9 @@ void classGetDeclaredConstructorsShouldNotMatchTypeReflectionHint() {
}

@Test
void classGetDeclaredConstructorsShouldMatchConstructorReflectionHint() throws Exception {
void classGetDeclaredConstructorsShouldNotMatchConstructorReflectionHint() throws Exception {
hints.reflection().registerConstructor(String.class.getConstructor(), ExecutableMode.INVOKE);
assertThatInvocationMatches(InstrumentedMethod.CLASS_GETDECLAREDCONSTRUCTORS, this.stringGetDeclaredConstructors);
assertThatInvocationDoesNotMatch(InstrumentedMethod.CLASS_GETDECLAREDCONSTRUCTORS, this.stringGetDeclaredConstructors);
}

@Test
Expand Down Expand Up @@ -349,9 +348,9 @@ void classGetDeclaredMethodsShouldNotMatchTypeReflectionHint() {
}

@Test
void classGetDeclaredMethodsShouldMatchMethodReflectionHint() throws Exception {
void classGetDeclaredMethodsShouldNotMatchMethodReflectionHint() throws Exception {
hints.reflection().registerMethod(String.class.getMethod("toString"), ExecutableMode.INVOKE);
assertThatInvocationMatches(InstrumentedMethod.CLASS_GETDECLAREDMETHODS, this.stringGetScaleMethod);
assertThatInvocationDoesNotMatch(InstrumentedMethod.CLASS_GETDECLAREDMETHODS, this.stringGetScaleMethod);
}

@Test
Expand Down Expand Up @@ -391,9 +390,9 @@ void classGetMethodsShouldNotMatchTypeReflectionHint() {
}

@Test
void classGetMethodsShouldMatchMethodReflectionHint() throws Exception {
void classGetMethodsShouldNotMatchMethodReflectionHint() throws Exception {
hints.reflection().registerMethod(String.class.getMethod("toString"), ExecutableMode.INVOKE);
assertThatInvocationMatches(InstrumentedMethod.CLASS_GETMETHODS, this.stringGetMethods);
assertThatInvocationDoesNotMatch(InstrumentedMethod.CLASS_GETMETHODS, this.stringGetMethods);
}

@Test
Expand Down Expand Up @@ -452,28 +451,6 @@ void classGetMethodShouldNotMatchForWrongType() {
assertThatInvocationDoesNotMatch(InstrumentedMethod.CLASS_GETMETHOD, this.stringGetToStringMethod);
}

@Test
void methodGetAnnotationsShouldMatchIntrospectHintOnMethod() throws NoSuchMethodException {
Method toString = String.class.getMethod("toString");
RecordedInvocation invocation = RecordedInvocation.of(InstrumentedMethod.METHOD_GETANNOTATIONS)
.onInstance(toString).withArguments("testString")
.returnValue(toString.getAnnotations()).build();
hints.reflection().registerType(String.class, typeHint -> typeHint.withMethod("toString",
Collections.emptyList(), ExecutableMode.INTROSPECT));
assertThatInvocationMatches(InstrumentedMethod.METHOD_GETANNOTATIONS, invocation);
}

@Test
void methodGetParameterTypesShouldMatchIntrospectHintOnMethod() throws NoSuchMethodException {
Method toString = String.class.getMethod("toString");
RecordedInvocation invocation = RecordedInvocation.of(InstrumentedMethod.METHOD_GETPARAMETERTYPES)
.onInstance(toString).withArguments("testString")
.returnValue(toString.getParameterTypes()).build();
hints.reflection().registerType(String.class, typeHint -> typeHint.withMethod("toString",
Collections.emptyList(), ExecutableMode.INTROSPECT));
assertThatInvocationMatches(InstrumentedMethod.METHOD_GETPARAMETERTYPES, invocation);
}

@Test
void methodInvokeShouldMatchInvokeHintOnMethod() throws NoSuchMethodException {
RecordedInvocation invocation = RecordedInvocation.of(InstrumentedMethod.METHOD_INVOKE)
Expand Down Expand Up @@ -555,9 +532,9 @@ void classGetDeclaredFieldsShouldNotMatchTypeHint() {
}

@Test
void classGetDeclaredFieldsShouldMatchFieldHint() throws Exception {
void classGetDeclaredFieldsShouldNotMatchFieldHint() throws Exception {
hints.reflection().registerField(String.class.getDeclaredField("value"));
assertThatInvocationMatches(InstrumentedMethod.CLASS_GETDECLAREDFIELDS, this.stringGetDeclaredFields);
assertThatInvocationDoesNotMatch(InstrumentedMethod.CLASS_GETDECLAREDFIELDS, this.stringGetDeclaredFields);
}

@Test
Expand Down Expand Up @@ -626,9 +603,9 @@ void classGetFieldsShouldNotMatchTypeHint() {
}

@Test
void classGetFieldsShouldMatchFieldHint() throws Exception {
void classGetFieldsShouldNotMatchFieldHint() throws Exception {
hints.reflection().registerField(String.class.getDeclaredField("value"));
assertThatInvocationMatches(InstrumentedMethod.CLASS_GETFIELDS, this.stringGetFields);
assertThatInvocationDoesNotMatch(InstrumentedMethod.CLASS_GETFIELDS, this.stringGetFields);
}

}
Expand Down
23 changes: 0 additions & 23 deletions ...-core/src/main/java/org/springframework/aot/hint/predicate/ReflectionHintsPredicates.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -243,29 +243,6 @@ public Predicate<RuntimeHints> withAnyMemberCategory(MemberCategory... memberCat
.anyMatch(memberCategory -> getTypeHint(hints).getMemberCategories().contains(memberCategory)));
}

/**
* Refine the current predicate to only match if a hint is present for any of its constructors.
* @return the refined {@link RuntimeHints} predicate
*/
public Predicate<RuntimeHints> withAnyConstructor() {
return this.and(hints -> getTypeHint(hints).constructors().findAny().isPresent());
}

/**
* Refine the current predicate to only match if a hint is present for any of its methods.
* @return the refined {@link RuntimeHints} predicate
*/
public Predicate<RuntimeHints> withAnyMethod() {
return this.and(hints -> getTypeHint(hints).methods().findAny().isPresent());
}

/**
* Refine the current predicate to only match if a hint is present for any of its fields.
* @return the refined {@link RuntimeHints} predicate
*/
public Predicate<RuntimeHints> withAnyField() {
return this.and(hints -> getTypeHint(hints).fields().findAny().isPresent());
}
}

public abstract static class ExecutableHintPredicate<T extends Executable> implements Predicate<RuntimeHints> {
Expand Down
36 changes: 0 additions & 36 deletions .../src/test/java/org/springframework/aot/hint/predicate/ReflectionHintsPredicatesTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -296,18 +296,6 @@ void privateConstructorInvocationMatchesInvokeDeclaredConstructors() {
assertPredicateMatches(reflection.onConstructor(privateConstructor).invoke());
}

@Test
void reflectionOnAnyConstructorDoesNotMatchTypeReflection() {
runtimeHints.reflection().registerType(SampleClass.class);
assertPredicateDoesNotMatch(reflection.onType(SampleClass.class).withAnyConstructor());
}

@Test
void reflectionOnAnyConstructorMatchesConstructorReflection() {
runtimeHints.reflection().registerConstructor(publicConstructor, ExecutableMode.INVOKE);
assertPredicateMatches(reflection.onType(SampleClass.class).withAnyConstructor());
}

}

@Nested
Expand Down Expand Up @@ -457,18 +445,6 @@ void privateMethodInvocationMatchesInvokeDeclaredMethods() {
assertPredicateMatches(reflection.onMethod(SampleClass.class, "privateMethod").invoke());
}

@Test
void reflectionOnAnyMethodDoesNotMatchTypeReflection() {
runtimeHints.reflection().registerType(SampleClass.class);
assertPredicateDoesNotMatch(reflection.onType(SampleClass.class).withAnyMethod());
}

@Test
void reflectionOnAnyMethodMatchesMethodReflection() {
runtimeHints.reflection().registerMethod(publicMethod, ExecutableMode.INVOKE);
assertPredicateMatches(reflection.onType(SampleClass.class).withAnyMethod());
}

}

@Nested
Expand Down Expand Up @@ -527,18 +503,6 @@ void privateFieldReflectionMatchesDeclaredFieldsHint() {
assertPredicateMatches(reflection.onField(SampleClass.class, "privateField"));
}

@Test
void reflectionOnAnyFieldDoesNotMatchTypeReflection() {
runtimeHints.reflection().registerType(SampleClass.class);
assertPredicateDoesNotMatch(reflection.onType(SampleClass.class).withAnyField());
}

@Test
void reflectionOnAnyFieldMatchesFieldReflection() {
runtimeHints.reflection().registerField(publicField);
assertPredicateMatches(reflection.onType(SampleClass.class).withAnyField());
}

}

private void assertPredicateMatches(Predicate<RuntimeHints> predicate) {
Expand Down

0 comments on commit 43d39d4

Please sign in to comment.