Skip to content
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.

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

Support customized implementation method name #8841

Conversation

utzcoz
Copy link
Member

@utzcoz utzcoz commented Feb 16, 2024

Sometimes internal methods return different types for different SDKs. It's safe because
these methods are internal/private methods, not public methods. To support different return
types of a method for different SDKs, we often use looseSignature method, although all return
types are common types like bool and int. This field/property can be used to fix this issue by
using different real methods for different SDKs.

@utzcoz utzcoz force-pushed the support-customized-implementation-method-name branch 2 times, most recently from 9317483 to a3d77b2 Compare February 16, 2024 10:57
@utzcoz utzcoz marked this pull request as ready for review February 16, 2024 11:17
@utzcoz
Copy link
Member Author

utzcoz commented Feb 16, 2024

@hoisie @brettchabot It's a prototype, but it's ready for reviewing now.

@utzcoz utzcoz force-pushed the support-customized-implementation-method-name branch from a3d77b2 to 5e72a06 Compare February 16, 2024 11:22
@utzcoz utzcoz force-pushed the support-customized-implementation-method-name branch from 5e72a06 to f8ef8d2 Compare February 24, 2024 10:03
@utzcoz
Copy link
Member Author

utzcoz commented Feb 24, 2024

Friendly ping @hoisie @brettchabot, PTAL.

@hoisie
Copy link
Contributor

hoisie commented Mar 26, 2024

@utzcoz it looks like getDeclaredMethod iterates over all methods of a class until it finds the first match, so I think iterating over all the methods should not be a performance issue:

https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/lang/Class.java#L2905

    @CallerSensitive
    public Method getDeclaredMethod(String name, Class<?>... parameterTypes)
        throws NoSuchMethodException, SecurityException {
        Objects.requireNonNull(name);
        @SuppressWarnings("removal")
        SecurityManager sm = System.getSecurityManager();
        if (sm != null) {
            checkMemberAccess(sm, Member.DECLARED, Reflection.getCallerClass(), true);
        }
        Method method = searchMethods(privateGetDeclaredMethods(false), name, parameterTypes);
        if (method == null) {
            throw new NoSuchMethodException(methodToString(name, parameterTypes));
        }
        return getReflectionFactory().copyMethod(method);
    }

This is searchMethods:

   // This method does not copy the returned Method object!
    private static Method searchMethods(Method[] methods,
                                        String name,
                                        Class<?>[] parameterTypes)
    {
        ReflectionFactory fact = getReflectionFactory();
        Method res = null;
        for (Method m : methods) {
            if (m.getName().equals(name)
                && arrayContentsEq(parameterTypes,
                                   fact.getExecutableSharedParameterTypes(m))
                && (res == null
                    || (res.getReturnType() != m.getReturnType()
                        && res.getReturnType().isAssignableFrom(m.getReturnType()))))
                res = m;
        }
        return res;
    }

@hoisie
Copy link
Contributor

hoisie commented Mar 26, 2024

Looks like the only difference is that calling getDeclaredMethods calls copyMethods.

  @CallerSensitive
    public Method[] getDeclaredMethods() throws SecurityException {
        @SuppressWarnings("removal")
        SecurityManager sm = System.getSecurityManager();
        if (sm != null) {
            checkMemberAccess(sm, Member.DECLARED, Reflection.getCallerClass(), true);
        }
        return copyMethods(privateGetDeclaredMethods(false));
    }

@utzcoz
Copy link
Member Author

utzcoz commented Mar 28, 2024

@hoisie If methodName can support regex or we create a new annotation to support method mapping with regex, we can use it to support Kotlin internal property's shadow. It's can expand shadow mechanism for Kotlin and help developers to use Robolectric shadow their custom code(although I know it's not the main purpose of Robolectric's shadow usage scenario).

copybara-service bot pushed a commit that referenced this pull request Mar 28, 2024
…hods

Previously, in ShadowWrangler, shadow method lookup was performed using
ShadowClass.findDeclaredMethod. It was called once to look for an exact match
of a shadow method, and sometimes called again to check for a looseSignatures
match.

There are plans to add new features and capabilities to the way that shadow
methods are matched. For example:
* looseSignatures being replaced with a more minimal
@classname("internal.type") annotation.
* If the signature of a method changes across SDK levels, we could introduce
different method names that map to the same method name.

However, to search for methods that cannot be matched using
ShadowClass.findDeclaredMethod, it is required to iterate over all candidate
methods using ShadowClass.findDeclaredMethods.

There were some questions about the performance of using
ShadowClass.findDeclaredMethods + iteration. However, after some preliminary
benchmarks, this approach is surprisingly approximately 25% faster than using
ShadowClass.findDeclaredMethod. It is perhaps due to the internal caching of
ShadowClass.findDeclaredMethods.

With this change, it will be possible to perform more advanced filtering and
searching for methods.

For #8841

PiperOrigin-RevId: 619288002
copybara-service bot pushed a commit that referenced this pull request Mar 28, 2024
…hods

Previously, in ShadowWrangler, shadow method lookup was performed using
ShadowClass.findDeclaredMethod. It was called once to look for an exact match
of a shadow method, and sometimes called again to check for a looseSignatures
match.

There are plans to add new features and capabilities to the way that shadow
methods are matched. For example:
* looseSignatures being replaced with a more minimal
@classname("internal.type") annotation.
* If the signature of a method changes across SDK levels, we could introduce
different method names that map to the same method name.

However, to search for methods that cannot be matched using
ShadowClass.findDeclaredMethod, it is required to iterate over all candidate
methods using ShadowClass.findDeclaredMethods.

There were some questions about the performance of using
ShadowClass.findDeclaredMethods + iteration. However, after some preliminary
benchmarks, this approach is surprisingly approximately 25% faster than using
ShadowClass.findDeclaredMethod. It is perhaps due to the internal caching of
ShadowClass.findDeclaredMethods.

With this change, it will be possible to perform more advanced filtering and
searching for methods.

For #8841

PiperOrigin-RevId: 619979740
@utzcoz utzcoz force-pushed the support-customized-implementation-method-name branch 2 times, most recently from d3d3ebe to 57d5b24 Compare March 31, 2024 06:28
@utzcoz
Copy link
Member Author

utzcoz commented Mar 31, 2024

Looks like the only difference is that calling getDeclaredMethods calls copyMethods.

Interesting. I keep to use getDeclaredMethods like your suggested, but create a local variable called methods to only call it once, and multiple iterations use the same methods instance to reduce copy operations.

From my experience, I can't use only one iteration to finish all types of finding jobs, as the exact matching has the first and highest priority than other approaches. So I reused the origin iteration with the exact method matching with highest priority and then looseSignature, and adding a new iteration for methodName with the third priority.

@utzcoz utzcoz force-pushed the support-customized-implementation-method-name branch from 57d5b24 to 719bf7e Compare May 3, 2024 12:32
@utzcoz
Copy link
Member Author

utzcoz commented May 3, 2024

@hoisie Friendly ping for reviewing. I want to land it before other contributors start to code for GSoC.

@utzcoz utzcoz force-pushed the support-customized-implementation-method-name branch from 719bf7e to 039de38 Compare May 4, 2024 15:14
@utzcoz
Copy link
Member Author

utzcoz commented May 20, 2024

@hoisie Maybe we can discuss this one?

1. Expand Implementation with a new method called methodName for mapped
   method name.
2. Expand SdkStore to consider mapped shadow method for validation.
3. Expand shadow method finding logic when running to add mapped shadow
   method finding logic as the third priority after looseSignature.
4. Split `ShadowBluetoothAdapter#setScanMode` into two methods for
   different SDKs to test mapped shadow method.

Signed-off-by: utzcoz <utzcoz@outlook.com>
@utzcoz utzcoz force-pushed the support-customized-implementation-method-name branch from 039de38 to 849e1bc Compare June 2, 2024 09:54
@utzcoz
Copy link
Member Author

utzcoz commented Jun 2, 2024

Friendly ping @hoisie for reviewing.

@utzcoz
Copy link
Member Author

utzcoz commented Jun 4, 2024

we can use it to support Kotlin internal property's shadow

I think developers can use JvmName to set the generated method signature, especially method name for internal Kotlin property's setter and getter.

@utzcoz
Copy link
Member Author

utzcoz commented Jun 4, 2024

@hoisie @MGaetan89 Okay, I decide to land it because long-time no-feedback. If you have other questions, please let me know, I can send new PRs to fix them or @Mia0451 can help to improve it under her GSoC project as methodName is very important to remove looseSignature for classes that have same method name + different signatures for different Android SDKs.

@utzcoz utzcoz merged commit 0f32174 into robolectric:master Jun 4, 2024
22 checks passed
@utzcoz utzcoz deleted the support-customized-implementation-method-name branch June 4, 2024 12:52
copybara-service bot pushed a commit that referenced this pull request Jun 14, 2024
… into google branch.

The original commit was by utzcoz.

google has custom instrumentation that needs special handling of custom method names
that needs to be done before the open source main branch can get merged with google branch.

This dupe commit will be removed during the next `google` -> `master` merge.

See #8841

PiperOrigin-RevId: 643114647
copybara-service bot pushed a commit that referenced this pull request Jun 14, 2024
… into google branch.

The original commit was by utzcoz.

google has custom instrumentation that needs special handling of custom method names
that needs to be done before the open source main branch can get merged with google branch.

This dupe commit will be removed during the next `google` -> `master` merge.

See #8841

PiperOrigin-RevId: 643448518
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants