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

Migrate calls to Shadow.invokeConstructor and ReflectionHelpers.callConstructor to reflector #8873

Open
hoisie opened this issue Feb 26, 2024 · 4 comments

Comments

@hoisie
Copy link
Contributor

hoisie commented Feb 26, 2024

In Robolectric there are a lot of calls to Shadow.invokeConstructor and ReflectionHelpers.callConstructor in shadow code. Any call to Shadow.invokeConstructor and ReflectionHelpers.callConstructor will end up using reflection.

Ideally all of these should be migrated to reflector. This will still be using reflection but it will be faster. Shadow.invokeConstructor should be migrated to @Direct @Constructor and ReflectionHelpers.callConstructor can be migrated to @Constructor. Note that @Direct @Constructor may not actually be supported at the moment.

@shashankiitbhu
Copy link

@hoisie
For classes instantiated using ReflectionHelpers.callConstructor, we plan to define corresponding Reflector interfaces. Are there any specific guidelines or best practices you recommend following when creating these interfaces, especially for handling classes with multiple constructors or dynamic class loading scenarios?

Also Is there any Similar Migration that has already took place ?

@hoisie
Copy link
Contributor Author

hoisie commented Mar 14, 2024

@shashankiitbhu there are some commits here that migrate ReflectionHelpers (and other things) to reflector.

One example is in here in ShadowLegacyTypeface:

https://github.com/robolectric/robolectric/blob/master/shadows/framework/src/main/java/org/robolectric/shadows/ShadowLegacyTypeface.java#L220-L230

There are calls such as return ReflectionHelpers.callConstructor( Typeface.class, ClassParameter.from(long.class, thisFontId));

This can be migrated to reflector, something like:

interface TypefaceReflector {
  @Constructor
  Typeface newTypeface(long fontId);
}

And then you could just invoke:

return reflector(TypefaceReflector.class).newTypeface(thisFontId);

Anywhere *ReflectionHelpers.callConstructor is used, it can be converted to reflector.

Shadow.invokeConstructor is a little tricker, I'm not sure reflector supports that yet.

@shashankiitbhu
Copy link

shashankiitbhu commented Mar 14, 2024

So Just to make sure I am getting this Right:

This is current approach:

if (getApiLevel() >= LOLLIPOP) {
  return ReflectionHelpers.callConstructor(
      Typeface.class, ClassParameter.from(long.class, thisFontId));
}

After Creating TypefaceReflector above :

protected static Typeface createUnderlyingTypeface(String familyName, int style) {
  long thisFontId = nextFontId.getAndIncrement();
  FONTS.put(thisFontId, new FontDesc(familyName, style));
  TypefaceReflector reflector = Reflector.reflector(TypefaceReflector.class);
  return reflector.newTypeface(thisFontId);
}

This is similar to how I was thinking about approaching this, thanks.

@shashankiitbhu
Copy link

@hoisie Shadow.invokeConstructor is indeed trickier ,

Maybe we can Introduce a New Annotation. For example, @DynamicConstructor, which could be used to annotate a method in the interface passed to Reflector.reflector(...). This annotation would signal that the method is a placeholder for dynamically resolved constructor invocations.

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

No branches or pull requests

2 participants