Status Update
Comments
iv...@gradle.com <iv...@gradle.com> #2
I can reproduce this (thanks for the repro project!)
It looks like the problem is that the desugared api list from r8 contains this entry:
java/util/Collection#removeIf(Ljava/util/function/Predicate;)Z
but the bytecode here doesn't match -- it's java/util/ArrayList. Collection isn't a directly implemented interface or a direct super class, it's an interface on the super super class. The most efficient thing runtime wise would be for the signature list to inline this method on all implemented subclasses. But I should probably at least for now go and make the desugared API lookup do something similar to what it does for API lookup -- search through all super classes and interfaces as well. This isn't a new problem, so I'm very surprised this hasn't come up before (or it has, and I've forgotten).
xa...@google.com <xa...@google.com>
hu...@google.com <hu...@google.com> #3
(I have a pending CL that was working to improve the handling of fields now that r8 handles desugaring fields, I'll try to dust that off and combine the fix in there.)
hu...@google.com <hu...@google.com> #4
I went to implement this, and hooked up inheritance search when analyzing the source file containing the call.
However, lint also handles the case where the library being analyzed is not using core library desugaring (for example, it may be a plain Java library). But when that library is consumed in a downstream app module, where library desugaring is turned on, lint then processes the partial results from the library and filters each violation through the desugaring allowlist.
At this point, it's tricky to do the inheritance search -- this happens when we no longer have a compilation environment and can do class inheritance lookups. So there are three possible solutions.
First, we pay the cost up front -- even when you're not using core library desugaring, we record whether the method is potentially library desugared if turned on. (This is also tricky because at this point we don't know which exact desugaring library version is used, which determines the exact list of APIs).
Or, more expensively, for every API violation of this type we store all the potential super class and interface names for each result...
Or, we handle this in the code which generates the desugaring API list, inlining all subclasses affected. This could be quite a long list, but on the other hand this list is really only intended to be machine readable.
ri...@google.com <ri...@google.com> #5
This is partially fixed now; it's fully fixed for the sample project, but in the scenario I described in comment 4, it works if you also configure library desugaring to be on in the library.
ri...@google.com <ri...@google.com> #6
likely related:
Objects.requireNonNullElse
Objects.requireNonNullElseGet
are now also showing this same false positive warning.
Android Studio Koala | 2023.3.2 Canary 2 gradle 8.6 plugin: 8.3.1
ri...@google.com <ri...@google.com> #7
This seems unrelated. Can you file a new bug?
Specifically, for Objects.requireNonNullElse
I am not seeing a lint API error, but I am seeing a warning from IntelliJ's built-in Java inspections. These should probably not show up in Android modules. I swear we've fixed that before but I'm assuming something changed to break that.
For Objects.requireNonNullElseGet
I do see a lint API warning for that. But, that's not a recent change, right? I looked at R8's desugaring list and that method is not there. So that method does crash at runtime, right?
ri...@google.com <ri...@google.com> #8
Can you file a new bug?
fair enough - will do.
I swear we've fixed that before but I'm assuming something changed to break that.
I'm not 100% sure whether it first occurred when I moved to 8.3.1, and/or to Koala Canary 2
So that method does crash at runtime, right?
funnily enough no, even when I run on Android 8.0 (in the emulator); In fact I seem to have been using that method for 2 years now and the first of those 2 years I ran it personally on a physical device (Samsung A5 2017 with Android 8.0 / other users on?) ... and yes, I am flabbergasted as well :)
I'll try and dig a little regarding the requireNonNullElseGet first as indeed I realise it should crash, and then will file a new bug (as requireNonNullElse should work regardless)
I'll add a reference here for your enjoyment :)
ap...@google.com <ap...@google.com> #9
Actually it looks like D8 is really backporting it; I wrote this test class:
public class JavaTest {
public void test(String s) {
Objects.requireNonNullElse(s, "test");
Objects.requireNonNullElseGet(s, new Supplier<String>() {
@Override
public String get() {
return null;
}
});
}
}
Built the APK and opened the APK analyzer, drilled to this method and invoked Show Bytecode and here's what I get:
# virtual methods
.method public test(Ljava/lang/String;)V
.registers 3
.param p1, "s" # Ljava/lang/String;
.line 8
const-string v0, "test"
invoke-static {p1, v0}, Lcom/example/myapplication/JavaTest$$ExternalSyntheticBackport0;->m(Ljava/lang/Object;Ljava/lang/Object;)Ljava/lang/Object;
.line 9
new-instance v0, Lcom/example/myapplication/JavaTest$1;
invoke-direct {v0, p0}, Lcom/example/myapplication/JavaTest$1;-><init>(Lcom/example/myapplication/JavaTest;)V
invoke-static {p1, v0}, Lcom/example/myapplication/JavaTest$$ExternalSyntheticBackport1;->m(Ljava/lang/Object;Ljava/util/function/Supplier;)Ljava/lang/Object;
.line 15
return-void
.end method
So both methods are desugared.
Clement/Søren, I don't see this in the backport list:
$ java -cp $ANDROID_HOME/cmdline-tools/latest/lib/r8.jar com.android.tools.r8.BackportedMethodList --min-api 15 | grep requireNonNullElse
java/util/Objects#requireNonNullElse(Ljava/lang/Object;Ljava/lang/Object;)Ljava/lang/Object;
(requireNonNullElse
is there, requireNonNullElseGet
is not.)
xa...@google.com <xa...@google.com> #10
aha - so requireNonNullElseGet is not magical after all :)
Please let me know if I should still file a new bug.
tx
ri...@google.com <ri...@google.com> #11
The method Objects.requireNonNullElseGet
is a bit special in terms of backporting, as it takes an argument of type Supplier
, which was added in API level 24. For pure backporting (no desugared library) this method can only be backported from API level 24:
$ java -cp $ANDROID_HOME/cmdline-tools/latest/lib/r8.jar com.android.tools.r8.BackportedMethodList --min-api 24 | grep requireNonNullElse
java/util/Objects#requireNonNullElse(Ljava/lang/Object;Ljava/lang/Object;)Ljava/lang/Object;
java/util/Objects#requireNonNullElseGet(Ljava/lang/Object;Ljava/util/function/Supplier;)Ljava/lang/Object;
Setting minSdk
below 24 (without using desugared library) this is the DEX for the test code.
.method public test(Ljava/lang/String;)V
.locals 1
.param p1, "s" # Ljava/lang/String;
.line 8
const-string v0, "test"
invoke-static {p1, v0}, Ldk/gjesse/jdk11desugaredlibrary/ObjectsTest$$ExternalSyntheticBackport0;->m(Ljava/lang/Object;Ljava/lang/Object;)Ljava/lang/Object;
.line 9
new-instance v0, Ldk/gjesse/jdk11desugaredlibrary/ObjectsTest$1;
invoke-direct {v0, p0}, Ldk/gjesse/jdk11desugaredlibrary/ObjectsTest$1;-><init>(Ldk/gjesse/jdk11desugaredlibrary/ObjectsTest;)V
invoke-static {p1, v0}, Ljava/util/Objects;->requireNonNullElseGet(Ljava/lang/Object;Ljava/util/function/Supplier;)Ljava/lang/Object;
.line 15
return-void
.end method
with only Objects.requireNonNullElse
getting backported.
With desugared library enabled this change, due to two things
- desugared library adds support for
Supplier
Objects
is no longer backported, but implemented by desugared library
The "tool" com.android.tools.r8.ir.desugar.desugaredlibrary.lint.DesugaredMethodsList
is used for desugared library:
java -cp $ANDROID_HOME/cmdline-tools/latest/lib/r8.jar com.android.tools.r8.ir.desugar.desugaredlibrary.lint.DesugaredMethodsList --min-api 15 --desugared-lib META-INF/desugar/d8/desugar.json --lib $ANDROID_HOME/platforms/android-34/android.jar --desugared-lib-jar desugar_jdk_libs-2.0.4.jar | grep Objects
java/util/Objects
(META-INF/desugar/d8/desugar.json
is from desugar_jdk_libs-2.0.4.jar
is
The line java/util/Objects
indicate that all methods of Objects
are supported.
With desugared library (for any minSdk
) this is the DEX for the test code:
# virtual methods
.method public test(Ljava/lang/String;)V
.locals 1
.param p1, "s" # Ljava/lang/String;
.line 8
const-string v0, "test"
invoke-static {p1, v0}, Lj$/util/Objects;->requireNonNullElse(Ljava/lang/Object;Ljava/lang/Object;)Ljava/lang/Object;
.line 9
new-instance v0, Ldk/gjesse/jdk11desugaredlibrary/ObjectsTest$1;
invoke-direct {v0, p0}, Ldk/gjesse/jdk11desugaredlibrary/ObjectsTest$1;-><init>(Ldk/gjesse/jdk11desugaredlibrary/ObjectsTest;)V
invoke-static {p1, v0}, Lj$/util/Objects;->requireNonNullElseGet(Ljava/lang/Object;Ljava/util/function/Supplier;)Ljava/lang/Object;
.line 15
return-void
.end method
ap...@google.com <ap...@google.com> #12
Thanks for that explanation Søren.
So, when I tried this with the latest Koala canary, it looks like things are working correctly. For the following code:
Objects.requireNonNullElse
Objects.requireNonNullElseGet
Both AGP and Studio correctly will not flag the first call.
The second call is flagged as an API error. But if I turn on core library desugaring, then that warning also goes away, both in AGP and in Studio.
There is still the issue of the builtin Java 9 APIs inspection in IntelliJ -- again, not a lint bug, but it is a Studio bug. Original submitter, can you file that one? And can you confirm (if you're not seeing warnings for these) that you are in fact using library desugaring and a recent Studio?
ap...@google.com <ap...@google.com> #13
I've filed
ap...@google.com <ap...@google.com> #14
thanks for filing the bug!
using library desugaring and a recent Studio?
yes I am, I have desugaring in all modules + using Koala Canary 2 now.
I will have to switch back to Jelly fish anyhow due to Koala being (to put it mildly) a runaway memory and cpu lover... which makes it nearly unusable (but that's another issue) so I might not be able to follow up on the new bug in the short term.
ap...@google.com <ap...@google.com> #15
On studio-main I can still repro the following false positive from
- Create a new project
- Enable desugaring, run Gradle sync
- Add a usage of
java.util.Objects#requireNonNullElse
Lint warning:
Call requires API level 30 (current min is 26): java.util.Objects#requireNonNullElse
ap...@google.com <ap...@google.com> #16
Just tested this with Koala.1 Beta 1, and something seems to not be right with lint (see results below). Not exactly sure where the integration is not working.
Using the test from git clone sso://user/sgjesse/B327670482
minApi | desugared library | requireNonNullElse | equireNonNullElseGet |
---|---|---|---|
21 | No | No lint message | Call requires API level 30 (and Call requires API level 24 on Supplier ) |
24 | No | No lint message | Call requires API level 30 |
21 | Yes | Call requires API level 30 | Call requires API level 30 |
The expected messages should be:
minApi | desugared library | requireNonNullElse | equireNonNullElseGet |
---|---|---|---|
21 | No | No lint message | Call requires API level 30 (and Call requires API level 24 on Supplier ) |
24 | No | No lint message | No lint message |
21 | Yes | No lint message | No lint message |
Looking at the DEX after compiling the code it is desugared as expected:
minApi | desugared library | requireNonNullElse | equireNonNullElseGet |
---|---|---|---|
21 | No | Desugared (backport) | Call to java.util.Objects through an D8 outline |
24 | No | Desugared (backport) | Desugared (backport) |
21 | Yes | Desugared (to target j$.util.Objects ) |
Desugared (to target j$.util.Objects ) |
Running the command line version of the D8 lint info generation tool the output looks correct.
As far as I can see the BackportedMethodList
D8 tool generated the correct desugared methods:
minApi
21 no library desugaring:
java -cp build/libs/r8.jar com.android.tools.r8.BackportedMethodList --min-api 21 --lib third_party/android_jar/lib-v34/android.jar | grep Objects
java/util/Objects#checkFromIndexSize(III)I
java/util/Objects#checkFromIndexSize(JJJ)J
java/util/Objects#checkFromToIndex(III)I
java/util/Objects#checkFromToIndex(JJJ)J
java/util/Objects#checkIndex(II)I
java/util/Objects#checkIndex(JJ)J
java/util/Objects#isNull(Ljava/lang/Object;)Z
java/util/Objects#nonNull(Ljava/lang/Object;)Z
java/util/Objects#requireNonNullElse(Ljava/lang/Object;Ljava/lang/Object;)Ljava/lang/Object;
requireNonNullElse
is desugared.
minApi
24 no library desugaring:
java -cp build/libs/r8.jar com.android.tools.r8.BackportedMethodList --min-api 24 --lib third_party/android_jar/lib-v34/android.jar | grep Objects
java/util/Objects#checkFromIndexSize(III)I
java/util/Objects#checkFromIndexSize(JJJ)J
java/util/Objects#checkFromToIndex(III)I
java/util/Objects#checkFromToIndex(JJJ)J
java/util/Objects#checkIndex(II)I
java/util/Objects#checkIndex(JJ)J
java/util/Objects#requireNonNullElse(Ljava/lang/Object;Ljava/lang/Object;)Ljava/lang/Object;
java/util/Objects#requireNonNullElseGet(Ljava/lang/Object;Ljava/util/function/Supplier;)Ljava/lang/Object;
requireNonNullElse
and requireNonNullElseGet
are desugared.
minApi
21 with library desugaring:
java -cp build/libs/r8.jar com.android.tools.r8.BackportedMethodList --min-api 21 --lib third_party/android_jar/lib-v34/android.jar --desugared-lib src/library_desugar/jdk11/desugar_jdk_libs.json | grep Objects
java/util/Objects#checkFromIndexSize(JJJ)J
java/util/Objects#checkFromToIndex(JJJ)J
java/util/Objects#checkIndex(JJ)J```
Neither requireNonNullElse
nor requireNonNullElseGet
is mentioned, but META-INF/desugar/d8/lint/compile_api_level_30/desugared_apis_30_1.txt
from the desugared library configuration artifact contains:
java/util/Objects
So all of Objects
is desugared.
We need to track down where the information is lost.
sg...@google.com <sg...@google.com>
an...@google.com <an...@google.com> #17
Shouldn't this be a P1 and Koala.1 RC blocker?
Description
Gradle 8.9 introduces an internal property that allows disabling artifact transform caching during build execution( disabling cache transforms and cache transform enabled ) with the following behavior in an Android project:
-Dorg.gradle.internal.transform-caching-disabled
). We measured the impact of disabling these transforms in a performance scenario where the same build was executed multiple times on a GitHub runner. We observed a significant regression(Disabling the cache transforms affects the task input build scans , the input differences in the task are found in the classes.dex files of the input artifacts. After analyzing the dex files from two different builds(attached), we observed the following differences in the strings within the dex file:
dexDir
of theDexMergingTask
, resulting in non-deterministic output for each iteration. When comparing twoThe diff report was generated by diffuse the following output:
However, if we exclude
DexingWithClasspathTransform
instead of disabling the cache for all transforms, the inputdexDir
of theDexMergingTask
remains consistent, and the task hits the cache.Attached the transform outputs (classes.dex) for
androidx.compose.ui:ui-graphics-android:1.7.0-beta01 (instrumented_ui-graphics-release-runtime.jar)
of two builds without caching forDexingWithClasspathTransform
Steps to reproduce:
reproducing_issue_dex
./gradlew assembleDebug -Dorg.gradle.internal.transform-caching-disabled
publishing build scans from two different runners/agentsGradle 8.9
AGP 8.5.2