Support @ClassName in function return type #9268

Mia0451 · 2024-07-07T18:33:13Z

@classname annotation are now also required in function return type of the shadow method. We will verify the shadow method in test compile time to make sure the shadows are correctly defined.

Overview

Proposed Changes

Mia0451

Hello reviewers,
I found in order to support function-return type matching in ShadowWrangler, I might need to add one more return-type in following functions?

ShadowWrangler.pickShadowMethod(Class<?> definingClass, String name, Class<?>[] paramTypes, @Nullable Class<?> rType)
ShadowWrangler.findShadowMethod(..., @Nullable Class<?> rType)
ShadowWrangler.findShadowMethodDeclaredOnClass(..., @Nullable Class<?> rType)

However, would change protected function function signature break backward compatibility?

utzcoz · 2024-07-08T08:08:42Z

I found in order to support function-return type matching in ShadowWrangler, I might need to add one more return-type in following functions?

I prefer to add new functions/methods to support new logic instead of modifying the current one if it was exposed to normal users before.

https://github.com/robolectric/robolectric/pull/8829/files#diff-a4b64f4b2ec3043da6267cff2ffc5adebad864a782a13ac294e3c9ced092be97

Looks like I didn't modify these methods' signature to support return type for ClassName.

annotations/src/main/java/org/robolectric/annotation/ClassName.java

Mia0451 · 2024-07-08T17:37:13Z

I found in order to support function-return type matching in ShadowWrangler, I might need to add one more return-type in following functions?

I prefer to add new functions/methods to support new logic instead of modifying the current one if it was exposed to normal users before.

https://github.com/robolectric/robolectric/pull/8829/files#diff-a4b64f4b2ec3043da6267cff2ffc5adebad864a782a13ac294e3c9ced092be97

Looks like I didn't modify these methods' signature to support return type for ClassName.

Mia0451 · 2024-07-08T17:42:39Z

Hello,

I added one more protected method

protected Method pickShadowMethod(...)

which now will take function return type into consideration when find a matching-method.

and changed the callsite in

public MethodHandle findShadowMethodHandle(...)

to use the new method

However, I think I will break the expectation of anyone who already overridden the old pickShadowMethod(...) method: their overridden will not be used most of the time start from this MR

sandbox/src/main/java/org/robolectric/internal/bytecode/ShadowWrangler.java

hoisie · 2024-07-10T15:48:59Z

annotations/src/main/java/org/robolectric/annotation/ClassName.java

 *
 *    }
 * }
 * </pre>
 */
-@Target(ElementType.PARAMETER)
+@Target({ElementType.PARAMETER, ElementType.TYPE_USE})


I think this should be @Target({ElementType.METHOD, ElementType.PARAMETER}).

TYPE_USE means it can be used for any use, i.e. at the class level, interfaces, casts.

Actually I prefer method first, but after some thoughts, it might be strange to use method for return type. Anyway, I am fine to both of method and type.

done. changed to @Target({ElementType.METHOD, ElementType.PARAMETER})

hoisie · 2024-07-10T15:52:00Z

annotations/src/main/java/org/robolectric/annotation/ClassName.java

 @Retention(RetentionPolicy.RUNTIME)
 public @interface ClassName {

  /**
-   * The class name intended for this parameter.
+   * The class name intended for the parameter, function return.


The class name intended for the parameter or the function return value.

hoisie · 2024-07-10T17:46:38Z

sandbox/src/main/java/org/robolectric/internal/bytecode/ShadowWrangler.java

@@ -181,7 +181,7 @@ public MethodHandle findShadowMethodHandle(
              MethodType actualType = isStatic ? methodType : methodType.dropParameterTypes(0, 1);
              Class<?>[] paramTypes = actualType.parameterArray();

-              Method shadowMethod = pickShadowMethod(definingClass, name, paramTypes);
+              Method shadowMethod = pickShadowMethod(definingClass, name, actualType);


Can we just keep ShadowWrangler as-is for now instead of checking the @ClassName return type annotation?

We don't check the return type when checking any other shadow methods, so I am not seeing a strong reason to special-case the @ClassName return types. It adds a lot of complexity to ShadowWrangler but it does not seem to make a difference functionally. As far as I know, when doing invokedynamic binding, the return type is not considered.

validator/SdkStore.java is what cares about the return type, so we should just add logic to that to check it.

Hi Michael,
I have a side question, may I ask when the SdkStore logic is executed? 1) in the stage of pre-instrument Android-all jars to publish to maven or 2) when in the early stage of unit-test startup during which verify all shadows are correctly defined or 3) in early stage of each test function to verify that shadows used in current test-function are correctly defined? or 4) any other timing?

SdkStore logic is executed whenever the shadows/framework code is compiled. It is invoked by an annotation processor, which occurs during compile-time. So whenever we add/modify a shadow method, it is verified against the Android sdks.

ShadowWrangler is invoked during runtime when an Android method is first called. It looks to check if a shadow method should be called instead of the real Android code.

I think we can write method signature to SdkStore, and ShadowWrangle only do basic validation without caring about too much about ClassName's details.

Maybe like https://github.com/robolectric/robolectric/pull/8829/files?

done.
make SdkStore take care of ClassName annotation in function return type

Hello reviewers,
by running test ./gradlew :shadows:framework:compileJava locally, I found that
1, ClassName must target to {ElementType.TYPE_USE, ElementType.PARAMETER}, otherwise the annotation will not be able to be found in SdkStore logic.
2, sometimes I have to enumerate all annotations for a parameter/return type to find ClassName annotation, sometimes I can use xxx.getAnnotation(ClassName.class) to quickly get a handle to the annotation

Hi @Mia0451 ,

We can start off using TYPE_USE for now, and perhaps as a follow-up we can clean it up.

Thanks!

@classname

@classname annoated function return type is actually NOT used in picking shadow method in test runtime, however we will enforce type matching in earlier stage which is sdk verifying stage.

Mia0451 commented Jul 7, 2024

View reviewed changes

utzcoz reviewed Jul 8, 2024

View reviewed changes

annotations/src/main/java/org/robolectric/annotation/ClassName.java Outdated Show resolved Hide resolved

Mia0451 closed this Jul 8, 2024

Mia0451 reopened this Jul 8, 2024

utzcoz reviewed Jul 9, 2024

View reviewed changes

sandbox/src/main/java/org/robolectric/internal/bytecode/ShadowWrangler.java Outdated Show resolved Hide resolved

Mia0451 force-pushed the feat/replace_loose_signatures_part4 branch from 83c3d6b to 9739682 Compare July 10, 2024 06:03

hoisie reviewed Jul 10, 2024

View reviewed changes

Mia0451 force-pushed the feat/replace_loose_signatures_part4 branch 6 times, most recently from ebc83e9 to 23396d7 Compare July 11, 2024 19:23

Support @classname in function return type

c6293e4

@classname annoated function return type is actually NOT used in picking shadow method in test runtime, however we will enforce type matching in earlier stage which is sdk verifying stage.

Mia0451 force-pushed the feat/replace_loose_signatures_part4 branch from 23396d7 to c6293e4 Compare July 11, 2024 19:26

utzcoz approved these changes Jul 12, 2024

View reviewed changes

hoisie merged commit 7103d72 into robolectric:master Jul 12, 2024

utzcoz mentioned this pull request Jul 26, 2024

Investigate supporting WithType for single input parameter of shadow method #6888

Closed

Mia0451 mentioned this pull request Aug 27, 2024

Provide generic type support in ClassName annotation parsing/validation #9526

Closed

Support @ClassName in function return type #9268

Support @ClassName in function return type #9268

Uh oh!

Conversation

Mia0451 commented Jul 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overview

Proposed Changes

Uh oh!

Mia0451 left a comment

Choose a reason for hiding this comment

Uh oh!

utzcoz commented Jul 8, 2024

Uh oh!

Uh oh!

Mia0451 commented Jul 8, 2024

Uh oh!

Mia0451 commented Jul 8, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mia0451 Jul 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Mia0451 commented Jul 7, 2024 •

edited

Loading

Mia0451 Jul 11, 2024 •

edited

Loading