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

Interface without any methods not handled by Retrofit consumer keep rules #4295

Open
sgjesse opened this issue Jan 27, 2025 · 5 comments
Open

Comments

@sgjesse
Copy link

sgjesse commented Jan 27, 2025

A question from the R8 team. We recently handled https://issuetracker.google.com/391260908, which turned out to be an interface passed to create in Retrofit, which needed to be kept. The consumer keep rules already have rules to keep Retrofit interfaces, https://github.com/square/retrofit/blob/trunk/retrofit/src/main/resources/META-INF/proguard/retrofit2.pro#L31, says

-if interface * { @retrofit2.http.* <methods>; }
-keep,allowobfuscation interface <1>

However, in the case of the issue the interface had no methods (and then trivially no methods annotated with a retrofit2.http.* annotation).

Is an interface without any methods a relevant use case for Retrofit create?

@JakeWharton
Copy link
Collaborator

This is basically #4134 and https://issuetracker.google.com/issues/327250276, although in that case the interface becomes empty as a result of shrinking, rather than starting empty.

From Retrofit's perspective an empty interface is valid, just a little weird.

That other issue seems solvable, since we have existing methods to match on. This issue seems impossible to fix, unless we do something like introduce a marker interface.

@JakeWharton
Copy link
Collaborator

Unless you all want to do me a solid and give me the Mockito treatment 😁.

@sgjesse
Copy link
Author

sgjesse commented Jan 28, 2025

Thank you Jake. We will consider if we can generalize what we just did for Mockito, to handle classes which are passes as class constants to certain methods.

And you can close this as WAI.

@sgjesse
Copy link
Author

sgjesse commented Jan 29, 2025

Getting back to improving the handling of Retrofit, then something like what we landed (hard-coded) for Mockito could be an option.

How about being able to express something like this:

-if class retrofit2.Retrofit {
  create(java.lang.Class <const-class-interface>);
}
-keep interface <const-class-interface> {
  @retrofit2.http.* <methods>
}

We will not add a keep rule like this, but evaluate how this could be expressed using Keep Annotations, maybe by annotating the Retrofit create method to say how to treat (keep) the interfaces which flows into it as const-class values.

This will only work when R8 knows class of the create argument, so depending on how dynamically Retrofit is used (my experience with Retrofit is limited, so I wouldn't know all the ways this is used).

Would something like this make sense, and which other Retrofit methods (if any) could require the same handling?

@JakeWharton
Copy link
Collaborator

In my experience the class instance that flows into create is almost exclusively a class literal rather than being sourced externally. The Retrofit instance can be passed around or provided by someone else, but usually then the call to create is with a literal.

That being said, I'm still tempted to introduce an optional marker interface and a new embedded rule. This will work in both the literal and value cases, although it has the different trade-off in that you might forget to add it and be subjected to the crash first.

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