IssueTracker

Might be related:
https://issuetracker.google.com/issues/120277396

Thank you for the report. R8 is not doing anything unless you tell it to, or fail to tell it.

> The serious issue is that it produces NPE, even though it doesn't make sense.
If DCNetworksResponse is only created reflectively (by GSON or other serialization) you have to explicitly tell R8 to keep it. R8 will try to remove as much dead code as possible, and in case of the mNetworkEntites, it will just throw a NPE instead.

To tell R8 that the fields are used, you can add the following to your proguard-rules.pro:

  -keepclassmembers class * {
      @com.google.gson.annotations.SerializedName <fields>;
    }
    -keep @interface com.google.gson.annotations.SerializedName

Also note, that for some private fields you have to declare them as transient if there are multiple definitions in the type-hierarchy.

> When I remove the function of `getNetworkEntities` and use the field directly, it works fine, which shows how R8 truly messes things up here.
Sure, now your program is creating is explicitly using the value which is what was failing before.

> It took me hours to find the cause for this, because the code is obfuscated on this case, without even proper line numbers .
When you debug, you could use -dontobfuscate to not give new names and -keepattributes InnerClasses,SourceFile,LineNumberTable,EnclosingMethod ect. to keep the information needed. For you to be able to de-obfuscate stacktraces you will need to keep those anyway.

If the above does not work I would love to have a version of your simple program to investigate further.

Actually, the following should also work:

-keepclassmembers,allowobfuscation class * {
      @com.google.gson.annotations.SerializedName <fields>;
    }
-keep,allowobfuscation @interface com.google.gson.annotations.SerializedName

That will minify (obfuscate) the names of the fields and the attribute as well, to further reduce the size of the final APK.

@3 But indeed it is being created and used, and the code is not dead code.
You can see for yourself, that I have a call to get the fields.
And, I've already checked that the instance does exist, so it's not about creation of the class. Before the "getNetworkEntities" , I added a log to show that indeed "response" is not null.
Why would it be the default behavior for apps? Why do we have to find those issues only on release?
The function is a simple getter. It should not be removed by R8 or anything, as it is being used in a normal way, without reflection.
It's also a getter of something that is "final", of an ArrayList that is initialized right away. It should always return a valid value.

If it's gson's fault, which is made by Google (here: https://github.com/google/gson ) , it should have the proper way to deal with R8, having the required Proguard rules if needed.
The library's repository doesn't even mention proguard on its main page. It is written about here though:
https://github.com/google/gson/blob/master/examples/android-proguard-example/proguard.cfg
In any case, this should be a part of the library. Not something we have to look for in the sample.

"When you debug, you could use -dontobfuscate to not give new names and -keepattributes InnerClasses,SourceFile,LineNumberTable,EnclosingMethod ect. to keep the information needed. For you to be able to de-obfuscate stacktraces you will need to keep those anyway. "
We already have:
-keepattributes SourceFile,LineNumberTable,*Annotation*,Signature
-keepattributes SourceFile,LineNumberTable
-keepattributes Signature
-keepattributes InnerClasses

I tried now to add:
-dontobfuscate
-keepattributes InnerClasses,SourceFile,LineNumberTable,EnclosingMethod

Seems to help in this. I suggest adding those (commented of course) to Proguard by default, and explain that it can be used in case of issues on release.

"If the above does not work I would love to have a version of your simple program to investigate further."
It's not mine, it's not a simple program, and it's not open sourced either. It's of the office.

In the end, this was found only recently, due to recent gradle updates. It's not easy to find the cause for it, and we already use the latest version of Gson:
api 'com.google.code.gson:gson:2.8.5'
It breaks Java

So, the instance is not null (checked in runtime, using log), and I try to use a getter that was written manually (and shouldn't be removed because I use it), that should return a non-null value of a final variable.
And I get NPE.
Why would R8 decide to ruin this code?

The sample for Gson actually has a different set of Proguard rules:

##---------------Begin: proguard configuration for Gson  ----------
# Gson uses generic type information stored in a class file when working with fields. Proguard
# removes such information by default, so configure it to keep all of it.
-keepattributes Signature

# For using GSON @Expose annotation
-keepattributes *Annotation*

# Gson specific classes
-dontwarn sun.misc.**
#-keep class com.google.gson.stream.** { *; }

# Application classes that will be serialized/deserialized over Gson
-keep class com.google.gson.examples.android.model.** { <fields>; }

# Prevent proguard from stripping interface information from TypeAdapterFactory,
# JsonSerializer, JsonDeserializer instances (so they can be used in @JsonAdapter)
-keep class * implements com.google.gson.TypeAdapterFactory
-keep class * implements com.google.gson.JsonSerializer
-keep class * implements com.google.gson.JsonDeserializer

# Prevent R8 from leaving Data object members always null
-keepclassmembers,allowobfuscation class * {
  @com.google.gson.annotations.SerializedName <fields>;
}

##---------------End: proguard configuration for Gson  ----------

How come?
What should be used?
Why isn't it a part of the library itself, if it's so important ?

OK, I will try to answer all the questions :)

First:
Did adding the keep rules work or not? It would be nice to know just for iterating going forward and to get you running again.

> Why would R8 decide to ruin this code?
For us to describe clearly what is going maybe you could provide the stacktrace when running with -dontobfuscate. You can also send your generated APK to mkroghj@google.com and I will have a look.

> Why would it be the default behavior for apps? Why do we have to find those issues only on release?
You are not running R8 on debug builds probably, but I would suggest to make some instrumentation tests to test a minified version of your APP.

> It's also a getter of something that is "final", of an ArrayList that is initialized right away. It should always return a valid value.
final values are able to be modified during reflection - which is how Gson updates final values:
https://docs.oracle.com/javase/specs/jls/se7/html/jls-17.html#jls-17.5.3

That is why the keep rule is important.

> Seems to help in this. I suggest adding those (commented of course) to Proguard by default, and explain that it can be used in case of issues on release.

It is already there, the following is pasted from the standard proguard-rules.pro

# Uncomment this to preserve the line number information for
# debugging stack traces.
#-keepattributes SourceFile,LineNumberTable

# If you keep the line number information, uncomment this to
# hide the original source file name.
#-renamesourcefileattribute SourceFile
#-keep,allowobfuscation class com.example.applymapping.actionunits.Minification {
#    void callOnClick();
#    <fields>;
#}

> So, the instance is not null (checked in runtime, using log), and I try to use a getter that was written manually (and shouldn't be removed because I use it), that should return a non-null value of a final variable.

It may not be the same type as you are expecting, but we should be able to see that from the APK if you point to the code where the de-serialization happens.

> The sample for Gson actually has a different set of Proguard rules:
> How come?

This is just an example - the important ones are:
# Application classes that will be serialized/deserialized over Gson
-keep class com.google.gson.examples.android.model.** { <fields>; }

Here you should change it to your package-name and class-names

This is needed for the JsonAdapter:
# Prevent proguard from stripping interface information from TypeAdapterFactory,
# JsonSerializer, JsonDeserializer instances (so they can be used in @JsonAdapter)
-keep class * implements com.google.gson.TypeAdapterFactory
-keep class * implements com.google.gson.JsonSerializer
-keep class * implements com.google.gson.JsonDeserializer

# Prevent R8 from leaving Data object members always null
-keepclassmembers,allowobfuscation class * {
  @com.google.gson.annotations.SerializedName <fields>;
}

this is the same as I suggested above.

> Why isn't it a part of the library itself, if it's so important ?

It may be worth to ask the gson guys to add an entry-point to their user-docs regarding shrinking, to point to their example. That may make it easier to find.

@8
1. I added only what you wrote here:
https://issuetracker.google.com/issues/136600124#comment4
And it seems to fixed those issues.
Is this enough, or should I add more? I also noticed you wrote now about more rules, that are different than what you wrote there.

2. About sending an APK, what do you want to have there? Do you want it without the proguard rules for Gson, release version, and with this:
-dontobfuscate
-keepattributes InnerClasses,SourceFile,LineNumberTable,EnclosingMethod

?

3. About the crash, again, the instance of the class is non-null, and all I do is reach its getter function. Why would it cause NPE ? I already checked that it's not null.
It doesn't make sense that I call a getter function (that isn't annotated) on an instance of a class, and get NPE.
It is even weirder when instead of using the getter function, when I use the field directly, it works fine.
To me it seems R8 decided to remove a simple getter function, causing NPE somehow. But shouldn't it cause a different type of exception instead?

4. About the Gson repository, I meant "Why don't you update the library to include all Proguard rules, so that we won't need to add them manually to projects that use it?" .
At the very least, put a very visible instruction on the main page of the repository, talking about what's needed in Proguard.
The example doesn't tell much, and it's different from what you wrote to me.

1. and 2.
Yes, the APK should be without:
-keepclassmembers,allowobfuscation class * {
  @com.google.gson.annotations.SerializedName <fields>;
}

and with:
-dontobfuscate
-keepattributes InnerClasses,SourceFile,LineNumberTable,EnclosingMethod

And send it to mkroghj@google.com

3.
I can comment further when I have the APK probably and if you provide the NPE stacktrace from the APK running with -dontobfuscate.

4.
It is not our place to say how the developers of Gson's github page should look like. If someone embarks on the journey of minimizing their APK with a shrinker, what we can do is provide support for that. There is a great article here regarding how R8 works here:
https://developer.android.com/studio/build/shrink-code

In that article, there is a section on trouble-shooting:
https://developer.android.com/studio/build/shrink-code#troubleshoot

That has a link to https://r8.googlesource.com/r8/+/refs/heads/master/compatibility-faq.md, which clearly describes what is needed to get Gson working with R8.

I think we provide both sufficient info on how to get things working and we are very helpful here if anyone has problems.

4b.
> The example doesn't tell much, and it's different from what you wrote to me.
How is the example different? I suggested one rule which is also in the example and explained why you may want to include the others in the comment above. Note that, the more you ask to keep, the larger your APK gets.

1. What? Is this enough or not?

2. +3. OK sent you, including instructions.

4. GSON is made by Google, no?
It should be tested with all that Google has to offer, including R8.
Since R8 is enabled by default, this problem will exist.
It's not the first time I've noticed R8 causes issues.
I've recently confirmed that it causes issues with another third party library : Zendesk. Except that I didn't know it, and instead of checking about R8, we added a Proguard rule to exclude all from it.

> 1. What? Is this enough or not?

A) You should definitely add the models that are serialized over Gson (this is written as -keep class com.google.gson.examples.android.model.** { <fields>; } in the example)
-keep class your.models.here.** { <fields>; }

B) Since you are using @SerializedName("xyz") you should add the following
-keepclassmembers class * {
  @com.google.gson.annotations.SerializedName <fields>;
}

C) Maybe the following if your app is not working without it:
This is needed for the JsonAdapter:
# Prevent proguard from stripping interface information from TypeAdapterFactory,
# JsonSerializer, JsonDeserializer instances (so they can be used in @JsonAdapter)
-keep class * implements com.google.gson.TypeAdapterFactory
-keep class * implements com.google.gson.JsonSerializer
-keep class * implements com.google.gson.JsonDeserializer

If it works without, then it works without, otherwise not.

D) Perhaps you would like to reflectively look at generic types, then you have to add:
-keepattributes Signature

E) Are you keeping other annotations or need @Expose?
-keepattributes *Annotation*

This was all of the active rules in the example. You can take what you need and test if it works. If it does not work, add some more.

2) I cannot use .wim files but the stacktrace was very helpful. The error is
java.lang.NullPointerException: throw with null exception
        at com.xxx.yyy.zzz.DCInsertUserResponse.getUserId(Unknown Source:3)

Because DCInsertUserResponse is only instantiated via reflection R8 is rewriting all references to it with throw null to be able to remove the class completely.

I will mark this as expected behavior.

3)
> It should be tested with all that Google has to offer, including R8.
I am sorry you ran into the issue, but we have written about the use of Gson with R8 and I pointed you to two places where you could have found the solution to your problem. Gson's main example has the exact keep rules you needed. The gson package do not include any keep rules, but I am sure you could file a feature request with them to add it to their library.

> It's not the first time I've noticed R8 causes issues.
> I've recently confirmed that it causes issues with another third party library : Zendesk. Except that I didn't know it, and instead of checking about R8, we added a Proguard rule to exclude all from it.
Of course R8 may cause some issues for people, it is a completely new shrinker :) In most cases it is because existing libraries rely on specific behavior/missing optimizations of Proguard and therefore, in order to use R8, one needs to add more keep rules. That is expected and intended behavior. And to add a -keep rule to libraries is perfectly  fine solution.

1. A lot of "maybe add this" . Why isn't it clear what to add, and that's it?

2. Sure you can. Install this (available for all OSs) and extract it:
https://www.7-zip.org/download.html
The only reason I used "wim" is because Gmail blocks APK files and even compressed files that have it. I had to use a special one that it doesn't handle (yet).
Maybe talk to Google to stop block files? :)

The log I sent you is not the original one that I wrote about here. It's the second issue.

"Because DCInsertUserResponse is only instantiated via reflection R8 is rewriting all references to it with throw null to be able to remove the class completely. "
Please explain. Why during runtime, I could check that indeed it's not null, and then when I accessed a getter, it caused NPE ?
The class instance exists, so the class exists, and since the function is used in normal way, R8 shouldn't even remove the class and the function.
There shouldn't be needed any proguard rules here, whether reflection is used to create the instance or not, because there is a function that is used in normal way.

R8 should notice that. It's not related to Gson.

Suppose you don't use Gson. Suppose you use some reflection to instantiate some class. You got not an instance of it, and you call a function.
Why would R8 think that the class should be removed, if it's being used via a function?
It doesn't make sense!
It will clearly always cause trouble.

So if I instantiate this via reflection, and call "foo" function, you mean to tell me that R8 will decide to ruin the class completely, causing NPE :

public class ClassToInstantiate {
    int t = 123;

    public int foo() {
        return t;
    }
}

?
How could this make sense?

3. This should be written about on the main screen of the repo. Not just in some example it has.

I think you are misunderstanding a bit what a shrinker does - there are two conflicting things going in opposite directions.

--> The more we remove the smaller the final APK is
<-- The less you remove the more chance of the output working correctly in all cases

1)
We do not know if you reflectively want to inspect generic type signatures in a program. If you have a specific need for that, then you add a keep rule. Every time one writes a keep-rule, it is a trade-off between size vs correctness. That is why there is no FINAL list, and you have to tailor it to your specific needs. Otherwise the rules will become too general and APK sizes will be larger.

2)
Other people have sent us emails with APKs, and you could have made a drive link and shared with my email. Lots of options. But the stacktrace was sufficient to figure out what was going on.

To answer you specific question here:
Think about it and all the code that R8 strip. If we were to say that all classes, fields and methods could be created and used reflectively, we would not be able to remove anything. The consensus choice is that all code not reached statically from what is determined as entry points into the program can be removed. That is why ALL reflective/runtime use has to be annotated in keep rules. You keep saying ruin the class, but we will remove the class and insert throw null into the use-sites - and it is perfectly sensible, because we cannot determine statically that the class is initiated and will produce anything else than a NPE.

3)
Regarding Gson's github page, if you feel strongly about it, contact them or raise an issue on their github page. Maybe even submit a pull-request and contribute.

I cannot see this conversation progressing and I think we should declare this issue as resolved. Thank you for trying out R8 and feel free to use Proguard if you find it to better suit your needs.

1. Still doesn't make sense in this case.
Again, if R8 sees that I use a class instance via a direct function call, why ruin the class?
Why would I need to tell it for each such case that I don't want it to ruin the class? In which case do you think it's ok to ruin a class that clearly is being used?

2. I have sent you an APK. Gmail blocks APK files, so I wrapped it. I can't do anything about it. You could just request it via email.
Here, I sent you a new APK, via a link. And about the stacktrace, again, it's of a different issue that was caused by R8.
Please check it out now.

"The consensus choice is that all code not reached statically from what is determined as entry points into the program can be removed."
But it IS reached ! It is clearly reached!
There is an instance, and I call it directly via a function that exists in the class. And it's a normal function too.
"You keep saying ruin the class, but we will remove the class and insert throw null into the use-sites"
So, it's ruining the class, because I can't use it anymore. I can't call a function in it. Not only that, but an NPE is clearly the worst exception to choose here. I didn't reach a null instance. The object exists. I just can't call it's function for some reason.
"we cannot determine statically that the class is initiated"
Of course you can. There is a casting to it, and there is a call to its function, so the class has to remain.

3. It's not resolved, and I didn't "try out R8". It's now the default, even though it's not backward compatible with previous behavior, and even though it ruins classes.

So to demonstrate how this is a wrong behavior. Look at this case:

Suppose you have this class:

public class ClassToInstantiate {
    final int t = 123;

    public int foo() {
        return t;
    }
}

And you instantiate it via reflection, in any way you wish, and you call a function of it:

public class Test {
    public static void test() {
        try {
            final ClassToInstantiate classToInstantiate = ClassToInstantiate.class.newInstance();
            Log.d("AppLog", "instance exists?" + (classToInstantiate != null));
            Log.d("AppLog", "test:" + classToInstantiate.foo());
        } catch (Exception e) {
            e.printStackTrace();
        }
    }
}

Isn't it obvious that since you have an instance to the class, and you have a variable of it, and you call a function of it, that you actually need this class to remain?
There is a clease "ClassToInstantiate" variable type here.

Do you really think this is ok to ruin the class in this case, and out of all possible exceptions, have NPE on the second log-line, as if the class instance isn't here?
When would you consider such a behavior ok? Really. On which case? Give an example that such a thing is ok.

I've just checked what you just wrote on one of my real apps. I signed it with R8 enabled and minifyEnabled, called the "test" function.
And guess what? It wasn't removed or ruined.
I still see the logs of it. The app didn't crash. It didn't show any NPE.

I even tried this type of reflection:

            final Class<?> clazz = Class.forName(fullPathToClass);
            final ClassToInstantiate classToInstantiate = (ClassToInstantiate) clazz.newInstance();

Still works fine.
Attached here those 2 classes.

Same should be in the case of DCNetworksResponse  . Why would DCNetworksResponse be ruined, if there is a clear declaration of a variable there, and usage of its function?

Typo: Instead of "call it's function" , I meant "call its function".

As Morten mentioned shrinking is based on whole program static analysis to figure out which parts of a program will actually be live at runtime, and that will always be an approximation. R8 will trace any reachable code from the items matched by the keep rules. These keep rules define all the program entry points. For most reflective operations it is not possible to statically determine which items they lookup, and keep rules are needed. For a simple command line program the main entry point need to have a keep rule as the JVM will reflective look up this method at runtime. No static analysis can determine the main class passed on the command line to invoke the program. Take the following simple one class program:

package mypackage;
class Main {
  public static void hello() {
    System.out.println("Hello, world!");
  }
  public static void main(String[] args) {
    hello();
  }
}

If that is compiled with R8 without any keep rules the result is an empty program without any classes.

Now adding the following keep rule

-keep class mypacakge.Main {
  public static void main(java.lang.String[]);
}

will create an entry point for tracing the code, and that will result in class mypackage.Main with both methods main and hello included in the resulting program.

If the program is changed to use reflection e.g.

package mypackage;
class Main {
  public static void hello() {
    System.out.println("Hello, world!");
  }
  public static void main(String[] args) {
    String methodName = "ahellob".substring(1, 6);
    getClass().getMethod(methodName).invoke(null)
    hello();
  }
}

the hello method will not be part of the output, as the static analysis is not clever enough (*) to figure out that 'getClass().getMethod(methodName).invoke(null)' will invoke method hello, and therefore it is removed. Running this program will result in a NoSuchMethodException. Fixing this requires that the keep rule is extended:

-keep class mypacakge.Main {
  public static void hello();
  public static void main(java.lang.String[]);
}

This is just a simple example, and the analysis finding uninstantiated classes and replacing any access on instances of these classes with plain "throw null" is of course way more complicated. But the rationale is the same if a class is never instantiated, and any access on an instance of such class will cause a NPE, so e.g. "instance.someField" can be replaced with "throw null".

To get some more background on shrinkers you can take a look at this video from Google I/O last year: https://www.youtube.com/watch?v=x9T5EYE-QWQ.

(*) Of course the static analysis can be taught to compute that "ahellob".substring(1, 6) is "hello". However, in general this is not possible, as this method name can be computed in a more complicated way - maybe it is received over the network JSON encoded.

@17 You still ignore the example I've shown.
Sure, some reflection creates a class instance.
But you have a reference to it!
And you have a function call to it!
Why would R8 decide to ruin it?
And why did you choose NPE out of all possible exceptions? The instance does exist!
It doesn't provide any good information of why it occurred this way. When I see NPE, I expect to see some variable that was null, and I try to access it. But it's not the case here at all.
On which case do you think it's ok to have this behavior ? Of ruining such classes usages, and showing NPE ?
And why in my test it didn't do it?

In your example R8 will statically analyze 'ClassToInstantiate.class.newInstance()' and determine that ClassToInstantiate is actually instantiated. For simple reflective use like this the static analysis can determine the class instantiated at compile time. You will need more subtle ways of creating instances. The GSON library will even use unsafe allocations (https://github.com/google/gson/blob/9d44cbc19a73b45971c4ecb33c8d34d673afa210/gson/src/main/java/com/google/gson/internal/UnsafeAllocator.java#L56) if other means fail. The R8 uninstantiated type optimization can be quite effective in pruning code, as when some access (field access or method call) can be replaced with 'throw null', then all the code after will become dead and can be removed.

You might not agree on the aggressiveness of the R8 shrinking algorithms. However, our goal is to shrink applications as much as possible, and that might require additional keep rules to be added when new versions are released. As I understand the additional keep rules that Morten provided you with solved the issue that you had with GSON.

@19 It solves for GSON, but not for Zendesk, which they don't even know how to fix yet.
I didn't even know it's R8 that ruined Zendesk. I thought it was something else, and adding Proguard rule to exclude it entirely is not a good solution either.

This kind of issue will occur for every library out there that wasn't tested with R8 and uses cases that you think that should ruin classes.

As for my example, it really doesn't matter how the class is created.

I think that you should generalize it as such:

            final ClassToInstantiate classToInstantiate = magicFunctionToCreateNewInstanceToRuleTheWorld();
            Log.d("AppLog", "instance exists?" + (classToInstantiate != null));
            Log.d("AppLog", "test:" + classToInstantiate.foo());

Then you shouldn't ruin the class. Ever.
You see that it's being created. You see that there is a casting to it. You see that it's being used via a function that exists in it.

So again, why ruin it?
Why do you want R8 to ruin classes? What's the logic behind ruining classes?

Please make R8 smarter. It doesn't make sense in any way to ruin a class this way.
It's not just aggressive. It's calling for trouble. And I already reached it twice. And there might be even more cases that I just didn't notice yet, because I don't look at code of all libraries I use.
And I don't think you read the code of all libraries you use either.

I'd like to add that we do appreciate the conversation on how and what to keep and there are a lot of choices that can be made to make code smaller vs be more conservative and maintain the same behavior.

The idea of equating a type that is referenced with a type that is allocated/instantiated is once such choice. It is a good idea but in general will lead to much larger code to the detriment of using a shrinker in the first place. A type can be referenced in a lot of contexts, the return type as you have noted, or in formals/arguments, or signatures, or fields, or annotations, etc. Making these imply that a type is also assumed to be allocated leads to pointing to more code which again pulls in even more code. Many developers expect the shrinker to remove unused code so keeping all referenced types would be a problem there too. The simplest notion of an object being allocated is that it is actually allocated somewhere in the program code which is what R8 looks for. If the allocation happens outside the program code (such as JNI or reflectively) the compiler cannot statically determine that and a keep rule is needed.

I hope that substantiates the current choice in R8. We will absolutely keep the trade offs in mind as we continue development. It may be there is a nice way to guess at what might be instantiated better which may avoid this case (as Søren noted, we do identify simple reflective instantiations). The downside of doing that, is that such guesses can and likely will change over time. If you put a keep rule for the exact cases that the code reflectively allocates objects, the program will continue to work even when R8 changes (and you won't pay the extra size cost of R8 guessing incorrectly).

@21 Thing is that I don't know of all classes that are being used in the entire project. And the more libraries the more chance for R8 to ruin classes, as it's not smart in deciding which are safe to be removed.
And in this case I don't see how it could fail so badly.
I use the instance. I call a function.
Please explain why does it matter how I created the instance in this case.
For all I care it was JNI that created this object. I use the instance in a normal way, via Java/Kotlin, after it was created. I even have a variable to use it. So if the class is somehow ruined, I can't use it.
If this logic continues, why would I enable R8 at all? It's never safe to be used. Especially for large projects.

As for other types of usages, it depends. Accessing field is of course the same as accessing the function. It's a part of the class. Annotations are harder because of various ways they are used.

At the very least, put some possible flags for R8 to protect against such cases. It doesn't make sense that we test the app so well on debug mode, and then on release it will fail for users.
Or put a flag that will warn us about possible issues that it has found.
But you can't just ignore those issues and hope that we find it ourselves.
There is a saying "we build on the shoulders of giants" . It's not practical to check what is done by all the libraries.
It never occurred before R8.

We already got issues because R8 decided to ruin both the case of Gson (try-catch, which we found later as users complained) and of Zeplin (users complained they can't use the support, as it crashed for them).
I was sure it's because of Zeplin, but in fact they stayed on the same version that worked before, and it was me who updated stuff of Google.
I told them something is wrong with their Proguard rules, because if I exclude all from them, it works fine, and because if I run the app in debug, it also works fine. But I felt weird saying this, because it used to work fine before, and nothing should have changed to ruin it.

Regarding this discussion, please keep in mind that R8 (and Proguard) are whole program optimizing compilers. The idea is that if the whole program is provided, the compiler can optimize and reduce the program. JNI and reflection break that since the compiler has no general way understand what those parts are doing. That is why keep rules exist: to inform the compiler about the parts of the program it cannot see, such as, the entry points to the program, the types that JNI uses or creates, same for reflection. If those usages are not specified then the program is not known to be correct after compilation. The only way to be correct and not specify any external aspects of the program is to not use the shrinker and stick with D8.

That said, library providers should provide you with the rules and information that are needed for the library to work with shrinking enabled, if not, you can't use them with R8 or Proguard without figuring out their internals unfortunately. Our team has been involved in talking with various teams to help get full and complete rules, such as for Gson. But there are many libraries and understanding what R8 and/or Proguard need is not easy. We are looking at ways to improve this with tooling, maybe a community driven repository of rules. Time will tell.

As it stands, R8 will consider any type that is not instantiated in the project as such, thus the only value that can flow to such a type is null. That is a simple and logical judgment and consistent with the role of a shrinker. It is that judgment you are seeing here. To inform the shrinker that that judgment is not true, you will need to define a keep rule that states that the type is to be kept (which implies it is instantiated) because R8 cannot see that. That is the current resolution of the example listed in the comments above. It is also consistent with the rules that Gson have indicated should be used. I've filed a feature request for evaluating the choice of this here: b/136757434.

Regarding the issues you are having with Zeplin, could you file a new issue with details of the issue (APK, possible sample project, logs, etc.)? We will be happy to diagnose the issue together with you.

Cheers, Ian

@23 Why do you keep thinking about CTOR only?
There are more ways to identify that a class is being used.
You still haven't explained why do you think it's ok to ruin a class if it's clear it's being used. There is an obvious usage of the class.
A CTOR is just one function of a class. Why would using only this one function out of all functions be the only one that matters to you?
Also, even if it is only about CTOR, if I call a function that return an instance of a class, why would you think it's safe to ruin the class?
You already have the declaration of its type.
Why does it matter to you how it was instantiated? JNI or any other way? In the end, what I got is going back to Java. It's not 100% in JNI. Something went back to Java world, with an instance of a class. This means it should not be ruined.

Causing NPE means that the function somehow returned null, which is incorrect, because it didn't.

About the libraries, Zendesk actually has Proguard rules built in, and it worked fine before R8 was added/updated recently.

"That is a simple and logical judgment and consistent with the role of a shrinker"
But it's not. It never caused such damage to classes before. I got those issues only recently, and I always chose to shrink the app.

"because R8 cannot see that. "
That's the problem. R8 should be smarter than that. If it sees that I use the class, obviously it shouldn't ruin it.
It's not just about instantiating it.

About Zendesk, I don't understand what you mean. You want a new APK that crashes when the user tries to get into the support section (meaning it uses Zendesk)?
Sorry, I was confused with Zeplin, which we also use, but is not a part of an app (it's a website for designers and developers to work on, showing sketches of what to implement).

I think we are misunderstanding each other, the comments above are trying to explain the reasoning behind the optimization you are seeing.

I've filed b/136757434 to consider the consequences of your suggestion about references indicating instantiations.

To illustrate, consider your goal is to ensure your program is as small as it can while still meaning the same thing, consider the program:

public class P {
  public static class A {
    public static A a;
    public static A getA() { return a; }
  }
  public static void main(String[] args) {
    System.out.print(getA());
  }
}

The only entry is P.main(String[]) so that is given a -keep rule to ensure the entry. The optimizing compiler will compile this program to the equivalent one:

class P {
  public static void main(String[] args) {
    System.out.print(null);
  }
}

The compiler would deduce that fields are default initialized to null, so the value of A.a is null. The compiler would likely inline the call getA which would be constant null and thus it can remove the entire class A. The compiler is assuming that the program above is the whole program (that is the assumption R8 (and Proguard) is making)). If the program was linked with some JNI code that actually initialized the field P.A.a, or the program classloaded a new definition of P.A at runtime, or any other use of A, the assumption of the compiler is broken. That is why a keep rule would be needed for A if any such use exists. Note that the keep of A depends on the outside usage, not the program, which is self contained.

Now the example above is simple and one could argue that the compiler should just keep A since getA() is used, but then the program will be the same before R8/Proguard and after, so the shrinker would fail to do anything for this program.

References become even more tricky when they are involved in a class hierarchy. Consider a reference to Iterable leading to assuming any class that implements it is allocated. That would lead to keeping huge amounts of unused library code (say Guava).

I hope the above helps clarify our hesitance to deduce that a type is instantiated because it is referenced.

Regarding "It never caused such damage to classes before", I don't believe that is true. Stack overflow is full of content on trying to make Proguard work for various apps and libraries well before R8 entered the picture. It is hard work exactly because of issues such as the one you are hitting here. Also, I'd second Søren's suggestion to check out the video https://www.youtube.com/watch?v=x9T5EYE-QWQ. It illustrates the very real pains of using a shrinker (and predates R8).

Regarding the other issues you are hitting with the new shrinker, we would very much like to find and solve those issues so that you may use R8 for your apps. Do file new issues for each individual issue you encounter and attach concrete data for the issue and we will gladly help out!

@25 You still ignore the example I've shown.
If I have a class that has a field (not a static one) in it, and it's even initialized to a new Array, and I access a function of it that returns the value of the field, why would anything think here to cause me NPE , and that it's considered ok?

Also, at the very least, if the compiler has "real pains" in shrinking, it should identify those special cases and write us a warning about it.
And it is true that "It never caused such damage to classes before" , because that's the first time I access a non-null field via a function, and get NPE. It doesn't make sense in any way to get such a behavior.

You still didn't explain in which case you think it's a good thing to cause NPE on the example I've shown:

public class DCNetworksResponse extends BaseDCResponse {

    @SerializedName("data")
    private final ArrayList<NetworkObject> mNetworkEntites = new ArrayList<>();

    public ArrayList<NetworkObject> getNetworkEntities() {
        return mNetworkEntites;
    }
}

How could calling getNetworkEntities() cause NPE, on an instance of DCNetworksResponse  ?
It's not like on your example, where the static variable "a" is not set anywhere, and therefore it might stay as null value. It's in fact set right away.
In your example, "A" isn't even instantiated. There is no reference to it anywhere. You call a static function, so there is no clue of creation of a new instance of A anywhere. Not a variable, not using a function of its instance.

Not similar to what I've shown.

In the case I've shown, there are multiple clues that should cause the compiler to think twice before doing such a thing:
1. Variable of this type exists, meaning something has made an instance of it.
2. Non-static function of the class is used, showing that indeed there is an instance of it.
3. Field of the class instance is set to be non-null right away, so no matter how you access it, it should stay non-null (unless reflection, but that is unknown).
4. Field is used by a function, meaning it shouldn't return a null value or NPE.

So NPE shouldn't occur here, and if R8 isn't sure about it, it can warn us. Maybe it could even do it while we write the class, suggesting to add it to the rules if it intends to change the class so much that NPE could occur on illogical case.

About other issues, currently as I wrote we have issue only with Zendesk. It's a support-related library, letting users chat with the support right inside the app. Do you want me to send you an APK that uses it and crashes? I don't know why it crashes on their case. Maybe Gson too. In fact in their case it's even worse. After one crash, the app keep crashing on next times I open the app.

And again, this is the first time I'm seeing those impossible issues (getting an NPE on a field that is set to be non-null), and they also admitted about this being first time reported on their case, and they are trying to investigate it. I'm the first to report them about it.

@26 Recently we found a similar issue of not finding a class even on Play Services itself , so I think it might also be related to R8.
I've sent you an email about it, because it's a bit confidential.
Please check it out.

I have forwarded you email to zerny@

@29 Thank you.
I can continue on Gmail if you prefer. Note that this crash happens on a very specific case. I couldn't reproduce it on my device, for example (Pixel 2 with Android Q beta 4). And even on the device it occurred, it happened on a very specific Google account, but not on others.

Liran, can you make a separate issue for the issue you mention in #comment28 and since this is happening in the support library, please write which version of the support library you are using. Then I will take a look at it.

@31 About @28, Sadly on this one I have 2 issues about reporting:
1. I'm not working on this project. It's a project that other people of the company work on.
2. It's still quite confidential.

Due to this, I can communicate via email, showing you a video of the issue and the app that has this issue.
Maybe later you could talk with other people of the company who do work on this project. I can give you their emails (or the opposite).

Plus, I don't think it's a part of the support library. It's about log in to Google. I can give you more information via email .

@31 And, since it's an issue found on a specific, private device of one of the company, it means the email addresses of his account would be shown on the video.
So even more reason to talk only via email.

Liran, I created b/136974945 to track the error you saw. There should not be a need to give any information out regarding users or project name.

Regarding "real pains", sorry for not being clear. I was referring to the pain that developers are faced when using a shrinker, not ours in developing a shrinker.

Regarding your example, thank you for being persistent, as there is absolutely something not right going on. Your example code in @15 is:

public class Test {
    public static void test() {
        try {
            final ClassToInstantiate classToInstantiate = ... // Assume external instantiation, R8 will find this -> ClassToInstantiate.class.newInstance();
            Log.d("AppLog", "instance exists?" + (classToInstantiate != null));
            Log.d("AppLog", "test:" + classToInstantiate.foo());
        } catch (Exception e) {
            e.printStackTrace();
        }
    }
}

Without a keep rule for ClassToInstantiate, R8 will conclude that any value of that type can only be null, however, the compiler appears to only use this information for the invoke, concluding that a.foo() will throw NPE because the receiver 'a' is null. The compiler should also have concluded that the != null check is constant, but failed to do so. I've filed b/136974947 for that.

@35 I don't understand. You mean to tell me that this should also crash with NPE ?
It doesn't, as I've tried it, and I think it shouldn't, at least in the cases I've tried.
About `classToInstantiate != null` , it actually returns true, for both this case and the problematic one with Gson.
The problem is on `classToInstantiate.foo()` which in the Gson example caused NPE, which doesn't make sense because we don't have a null object.
Here somehow all worked fine (meaning I failed to reproduce the issue on my own code).

In any case, please add warnings&errors when we have such cases, that will also have quick-fix to add rules to Proguard.
I would have preferred if it was an error which would fail to build the signed APK (to prevent real crashes by users), till I take action about it.

@35: Yes it should. The class has no keep rule and thus the compiler is allowed to conclude it is never instantiated if nothing in the program explicitly does so. I opened b/136757434 to evaluate that choice and other possible identifications, but that is the indented behavior of the compiler right now. It is a bug that the compiler does not apply the value reasoning consistently and it would be good to enable information about failed value assumptions in the -adddebugconfiguration flag. Filed b/136974947 for that.

Closing this bug again, as the three spin-offs are tracked in there respective issues and the original issue is resolved by adding the keep rule.

@37 It should crash?
How come? On debug mode it doesn't, which is what the developer intended: use the instance of the class by calling `foo` on it.
Why do you think the shrinker can't be smarter and avoid a crash here? On which case is it a good thing to crash here?
I still don't get it. If I have a variable and even a function call to it, why can't it understand that indeed I want to call the function of a real instance?
Why do you think we should add a rule for each such case?

Please allow some protection against this. It's even more crucial in case we have many who work on the code (or when using libraries that miss this part, like Zendesk), as it could slip into the app without anyone noticing.

I understand your frustration here, but the contract when using a shrinker is that the "external" behaviors of the program must be specified using keep rules. If the program does a reflective instantiation of a class, then that class must have a keep rule for the type and for the constructor that is being reflectively used.

@39 Since when a single person work on a project, he can't know what others that worked on it have done, and he can't know what third party libraries that are being used have done, it's very important to know when a shrinker has changed its behavior to be so aggressive that it could cause a crash to the app, and it's also very important to know when it's about to do it on such cases.

Please add something to protect against those cases.
I don't think Gson had the R8 Proguard rules last year, for example, so when R8 decides to mess around with what Gson is doing (More than what was done before R8), and causing a crash, this means there is no backward compatibility, and this means more confusion about crashes.

In my case, I'm not even the one who used Gson (I'm usually working on UI-related tasks), so I was sure that it was my fault when the app started crashing, because the current release of the app was almost entirely made by me.

@39 In other words, even if when I use reflection or use a third party library that uses reflection, and I do add the rules, it doesn't mean others will.
If at least one person forgets to add a rule, it could cause to a lot of confusion of what is going on.