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

Add annotation processor for generating keep rules for types mentioned only inside generics #3588

Closed
piotrros opened this issue Jul 13, 2021 · 24 comments · Fixed by #4067
Closed
Assignees
Labels

Comments

@piotrros
Copy link

piotrros commented Jul 13, 2021

Otherwise they will be removed by R8 since it cannot determine whether we are doing reflection on them.

Old issue below:


Using com.squareup.retrofit2:retrofit:2.9.0

After upgrading gradle plugin from

com.android.tools.build:gradle:4.1.3

to

com.android.tools.build:gradle:4.2.2

I'm getting following error when running with minifyEnabled true:

java.lang.IllegalArgumentException: Method return type must not include a type variable or wildcard

Here's how looks the method, which generates the error:

    @FormUrlEncoded
    @POST("view_ad") //NON-NLS
    Call<GenericDataResponse<EmptyData>> viewAd(
            @Field("session_id") String sessionId, //NON-NLS
            @Field("id") int id //NON-NLS
    );

    public class EmptyData {

        public EmptyData(){}

    }

    public class GenericDataResponse<T> {

        @SerializedName("result") //NON-NLS
        public String result;

        @SerializedName("data") //NON-NLS
        public T data;

        @SerializedName("mess") //NON-NLS
        public String[] messages;
    }

In order to fix this I have to either:

  • downgrade to the previous gradle plugin version
  • add proguard rule: -keep class com.example.app.EmptyData
  • disable proguard/r8

I've read the docs about gradle 4.2 changes and nothing seems to be related to proguard directly.
Can someone explain what's going on?

@piotrros
Copy link
Author

Even after adding rule: -keep class com.example.app.EmptyData I still had strange crashes like NullPointerExceptions so this rule isn't enough. I had to disable proguard for all app classes like this:

-keep class com.example.app.**

@piotrros
Copy link
Author

Anyone :) ? It's pretty important.

@ulmaxy
Copy link

ulmaxy commented Jul 19, 2021

I have a similar issue with gradle plugin 4.2.1. Have you figured out what the issue is?

@piotrros
Copy link
Author

No, I'm still waiting for a response. Downgrading to gradle 4.1.3 is the best option right now.

@ulmaxy
Copy link

ulmaxy commented Jul 19, 2021

It's strange, but simply downgrading to gradle 4.1.3 is not working for me

@ulmaxy
Copy link

ulmaxy commented Jul 19, 2021

In my case it was caused by data structure being removed by proguard, so adding the @keep annotation helped me

@piotrros
Copy link
Author

You used @keep on some class or whole code?

@ulmaxy
Copy link

ulmaxy commented Jul 20, 2021

On a class, the signature I've got is similar to this:
@GET("endpoint") fun getData(): Single<List<SomeClass>>

And adding the @Keep annotation to SomeClass helped

@nvkleban
Copy link

nvkleban commented Jul 23, 2021

I have similar issue with Retrofit 2.9 (checked versions down to 2.6.2 too) and gradle plugin 4.2.2 with minifyEnabled=true
Method return type must not include a type variable or wildcard: io.reactivex.Single<?>
But I have simple object not generic type as a response

    @POST("...")
    fun registerPushToken(
        @Body request: RegisterPushTokenRequest
    ): Single<RegisterPushTokenResponse>
class RegisterPushTokenResponse(
    @field:SerializedName("value") val token: String
)

Downgrading plugin to com.android.tools.build:gradle:4.1.3 fixes issue.

@alexxkovalchuk
Copy link

@piotrros

Updating plugin to
classpath 'com.android.tools.build:gradle:7.0.1'
fixed the issue for me

(Android Studio Arctic Fox | 2020.3.1 Patch 1)

@piotrros
Copy link
Author

@piotrros

Updating plugin to
classpath 'com.android.tools.build:gradle:7.0.1'
fixed the issue for me

(Android Studio Arctic Fox | 2020.3.1 Patch 1)

Thanks, that worked.
It required some more work though like updating java (I was using 1.8 and had to switch to 11).

For that reason I'm not closing this issue yet, because it should be possible using 4.x

@JakeWharton
Copy link
Member

If something changed solely with an AGP version change then it's like a result of a newer version of R8. The easiest way for me to look at the problem would be a minimal, self-contained sample project that exhibits the problem.

@piotrros
Copy link
Author

Though using 7.0.1 I've discovered new issue, but it's not leading to a crash like before.

But I'll be hard to reproduce on your side probably.

I have an api call to receive a list of items (for example articles), inside a model I have an object named PagingInfo - it's a simple class containing information about number of items and if there's more of the to get.

When I'm using 7.0.1 this class becomes null in ALL api calls I have, but only when using R8. If I disable R8 it's working fine. Could this a GSON issue? I fixed the issue like this:

-keep class com.example.PagingInfo

Class looks like this:

public class PagingInfo {

    @Nullable
    @SerializedName("offset") //NON-NLS
    public int offset;

    @Nullable
    @SerializedName("page_size") //NON-NLS
    public int pageSize;

    @SerializedName("total_items") //NON-NLS
    public int totalItems;

    @SerializedName("has_more_items") //NON-NLS
    public boolean hasMoreItems;

    @Nullable
    @SerializedName("last_message_id") //NON-NLS
    public Integer lastMessageId;

    public PagingInfo() {}

}

Inside a JSON response this looks like this:

"paging": {
    "offset": 0,
    "page_size": 10,
    "total_items": 72,
    "has_more_items": true
  },

I have tons of models and api calls - everything but this one works completely fine. But by some reason using 7.0.1 and R8 leads to retrofit (or gson) unable to deserialise this one class. Simply disabling R8, adding an exception in proguard file or rolling back to the older gradle plugin solves the issue.

TWiStErRob added a commit to TWiStErRob/repros that referenced this issue Mar 31, 2022
@TWiStErRob
Copy link

TWiStErRob commented Mar 31, 2022

@JakeWharton here's a repro:

  1. https://github.com/TWiStErRob/repros/tree/master/retrofit/proguard-removed-return-type
  2. Run the app (debug variant)

Expected: Activity starts and reads the header:

I/okhttp.OkHttpClient: <-- 200 OK https://httpstat.us/200 (556ms)
I/okhttp.OkHttpClient: Content-Length: 31
I/okhttp.OkHttpClient: Content-Type: application/json
I/okhttp.OkHttpClient: Server: Kestrel
I/okhttp.OkHttpClient: Request-Context: appId=cid-v1:1e93d241-20e4-4513-bbd7-f452a16a5d69
I/okhttp.OkHttpClient: Strict-Transport-Security: max-age=2592000
I/okhttp.OkHttpClient: Set-Cookie: ARRAffinity=a9cebb550e254ba9270d015acd372767c78fa4f3f79fc353fb793f2912e673b3;Path=/;HttpOnly;Secure;Domain=httpstat.us
I/okhttp.OkHttpClient: Set-Cookie: ARRAffinitySameSite=a9cebb550e254ba9270d015acd372767c78fa4f3f79fc353fb793f2912e673b3;Path=/;HttpOnly;SameSite=None;Secure;Domain=httpstat.us
I/okhttp.OkHttpClient: Date: Thu, 31 Mar 2022 22:43:38 GMT
I/okhttp.OkHttpClient: {"code":200,"description":"OK"}
I/okhttp.OkHttpClient: <-- END HTTP (31-byte body)
I/System.out: 31

Actual:

E/AndroidRuntime: FATAL EXCEPTION: main
    Process: com.example, PID: 9204
    java.lang.RuntimeException: Unable to start activity ComponentInfo{com.example/com.example.MainActivity}: java.lang.IllegalArgumentException: Method return type must not include a type variable or wildcard: b.a.i<g.u<?>>
        for method c.a
        at android.app.ActivityThread.performLaunchActivity(ActivityThread.java:3449)
        at android.app.ActivityThread.handleLaunchActivity(ActivityThread.java:3601)
        at android.app.servertransaction.LaunchActivityItem.execute(LaunchActivityItem.java:85)
        at android.app.servertransaction.TransactionExecutor.executeCallbacks(TransactionExecutor.java:135)
        at android.app.servertransaction.TransactionExecutor.execute(TransactionExecutor.java:95)
        at android.app.ActivityThread$H.handleMessage(ActivityThread.java:2066)
        at android.os.Handler.dispatchMessage(Handler.java:106)
        at android.os.Looper.loop(Looper.java:223)
        at android.app.ActivityThread.main(ActivityThread.java:7656)
        at java.lang.reflect.Method.invoke(Native Method)
        at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:592)
        at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:947)
     Caused by: java.lang.IllegalArgumentException: Method return type must not include a type variable or wildcard: b.a.i<g.u<?>>
        for method c.a
        at g.z.n(:54)
        at g.z.m(:43)
        at g.w.b(:30)
        at g.v.c(:202)
        at g.v$a.invoke(:160)
        at java.lang.reflect.Proxy.invoke(Proxy.java:1006)
        at $Proxy1.a(Unknown Source)
        at com.example.MainActivity.onCreate(:39)
        at android.app.Activity.performCreate(Activity.java:7994)
        at android.app.Activity.performCreate(Activity.java:7978)
        at android.app.Instrumentation.callActivityOnCreate(Instrumentation.java:1309)
        at android.app.ActivityThread.performLaunchActivity(ActivityThread.java:3422)
        	... 11 more

Notable points on what makes it work:

  • Change AGP to 7.1.1
  • Change isMinifyEnabled = false
  • Uncomment println(response.body()!!.description)
  • -keep,allowobfuscation,allowoptimization class com.example.ResponseDTO { }

I think this is a valid scenario in ProGuard/R8, it just removes an unused class. No-on ever accesses the class except that erased generics. Although the fix in future versions might be that generics -> used.

@JakeWharton
Copy link
Member

Thank you for the excellent repro. I don't think there's any way to mitigate this from Retrofit's side with a simple keep rule. On Cash App we actually wrote an annotation processor which generates -keep rules for types mentioned in Retrofit interfaces. I will repurpose this issue for upstreaming that into Retrofit itself.

@JakeWharton JakeWharton changed the title Method return type must not include a type variable or wildcard Add annotation processor for generating keep rules for types mentioned only inside generics Apr 8, 2022
@TWiStErRob
Copy link

@JakeWharton while I'd love to see that for its tech, I'm not sure that would solve the problem.

  • If someone finds this issue, they'll know what to do (keep).
  • If they can't find it, it's not going to help because you can't add annotation processor automatically like you can ProGuard rules, right?

This issue only affects a specific version range (R8 bundled into AGP 4.2.x) in a specific edge case (not using a return value). So the effort in making an annotation processor and advertising it is probably too much?

(Note: the original title was good SEO.)

@TWiStErRob
Copy link

TWiStErRob commented Apr 8, 2022

That said, adjusting the error message might be useful,

because retrace is not helpful here if you have many retrofit interfaces. (click to expand for what I tried)

retrace-ing the stack

%ANDROID_HOME%\cmdline-tools\latest\bin\retrace.bat build/outputs/mapping/debug/mapping.txt stack.txt

gives me:

     Caused by: java.lang.IllegalArgumentException: Method return type must not include a type variable or wildcard: b.a.i<g.u<?>>
        for method c.a
        at retrofit2.Utils.methodError(Utils.java:54)
        at retrofit2.Utils.methodError(Utils.java:43)
        at retrofit2.ServiceMethod.parseAnnotations(ServiceMethod.java:30)
        at retrofit2.Retrofit.loadServiceMethod(Retrofit.java:202)

which doesn't help, because the most relevant parts are still obfuscated.

Faking a stack trace with known info

(b.a.i is a type, g.u is a type, c is a type, and a is a method, ? is key, but unknown)

        at g.u.helpMe()
        at b.a.i.helpMe()
        at c.a()
        at g.z.n(:54)

gives

        at retrofit2.Response.helpMe(Response.java)
        at io.reactivex.Single.helpMe(Single.java)
        at c.a()
        at retrofit2.Utils.methodError(Utils.java:54)

so still not helpful.

Manually looking at mapping file

I searched for -> .*\.c: regex (because the package of c.a is missing), but this could yield so many false positives in a normal project.

com.example.RetrofitService -> com.example.c:
    io.reactivex.Single makeRequest() -> a

so c.a == com.example.RetrofitService.makeRequest()

I would propose changing for method c.a part of the error message to be faking a StackTraceElement:

  at fully.qualified.class.method()

@JakeWharton
Copy link
Member

When running R8 in full mode all generic types are removed unless they are explicitly kept. Retrofit automatically keeps the generic part for users of Call, but if you are using Rx or Kotlin or some other library as your adapter it will not work (and it's wasteful to keep the generic on all usages of those types). The only way is to explicitly keep the body types, hence the annotation processor.

@TWiStErRob
Copy link

Hmm, that's a good point, thanks for clarifying.

I just checked the rx1 and rx3 adapters in this repo, and they have keep rules. Just to understad more, is your proposal to replace those with more explicit ones generated by apt?

@JakeWharton
Copy link
Member

Those rules are painful. It bloats the APK since those types are likely widespread in your application. Not sure we can safely remove them at this point–even with the processor.

@danielesegato
Copy link

danielesegato commented Sep 23, 2022

I've created another bug / question? #3774 but I'm not sure if I should have just wrote here.

Looking at the proguard rules here:
https://github.com/square/retrofit/blob/master/retrofit/src/main/resources/META-INF/proguard/retrofit2.pro

# With R8 full mode, it sees no subtypes of Retrofit interfaces since they are created with a Proxy
# and replaces all potential values with null. Explicitly keeping the interfaces prevents this.
-if interface * { @retrofit2.http.* <methods>; }
-keep,allowobfuscation interface <1>

# Keep generic signature of Call, Response (R8 full mode strips signatures from non-kept items).
-keep,allowobfuscation,allowshrinking interface retrofit2.Call
-keep,allowobfuscation,allowshrinking class retrofit2.Response

# With R8 full mode generic signatures are stripped for classes that are not
# kept. Suspend functions are wrapped in continuations where the type argument
# is used.
-keep,allowobfuscation,allowshrinking class kotlin.coroutines.Continuation

and here:
https://github.com/square/retrofit/blob/master/retrofit-adapters/rxjava2/src/main/resources/META-INF/proguard/retrofit2-rxjava2-adapter.pro

# Keep generic signature of RxJava2 (R8 full mode strips signatures from non-kept items).
-keep,allowobfuscation,allowshrinking class io.reactivex.Flowable
-keep,allowobfuscation,allowshrinking class io.reactivex.Maybe
-keep,allowobfuscation,allowshrinking class io.reactivex.Observable
-keep,allowobfuscation,allowshrinking class io.reactivex.Single

It seems to me the rules for Single and other RX are already there. And I have an androidx.annotation.Keep on my objects (inside Single<Response<MyObject>>)

So what is it missing on my part? Couldn't find any documentation about this.

@tomridder
Copy link

感谢您的出色复制。我认为没有任何方法可以通过简单的保留规则从 Retrofit 方面减轻这种情况。在 Cash App 上,我们实际上编写了一个注释处理器,它-keep为 Retrofit 接口中提到的类型生成规则。我将重新调整此问题的用途,以便将其导入 Retrofit 本身。

hi jake, i have the experience of using APT and java poet to generate files in compile time .here is the PR i wrote for eventbus(greenrobot/EventBus#714)
it would be my pleasure to achieve this feature..
but firstly i need to know is it still neccesary to do this function ,right now .
thanks

@JakeWharton
Copy link
Member

This is fully built internally at Cash App. We've used it for a few years. It only needs moved into open source.

@JakeWharton
Copy link
Member

Doing this next week for 2.10.

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

Successfully merging a pull request may close this issue.

8 participants