Compose Compiler uses full file path to generate keys in rememberExpression, breaking build reproducibility [313937484]

Fixed

Bug

Status Update

No update yet.

Description

ig...@jetbrains.com

created issue #1

Nov 30, 2023 03:07PM

Jetpack Compose compiler version: 1.5.4 Android Studio Build: #AI-223.8836.35.2231.11090377, built on November 13, 2023 Kotlin version: 1.9.20

The change introduced by commit 22421ef1f3aab2bb317e164845bf1630606a099c uses currentFile.fileEntry.name field to generate keys to be passed to startReplaceableGroup call. I discovered that this field contains the absolute file path, not just the part relative to the project root.

In practice that results in binary different artifacts being produced for binary equivalent sources stored under different paths. E.g. builds under /tmp/ci-build-42/ will always produce binary equal artifacts for a given source code revision, but these artifacts will be different from the ones produced from the same revision under /tmp/ci-build-43/.

Steps to Reproduce or Code Sample to Reproduce:

Create a new project with "Empty Activity" template
Set the Compose compiler version to 1.5.4 and Kotlin plugin to 1.9.20
Build the application using ./gradlew clean build --no-daemon, store ./app/build/intermediates/compile_app_classes_jar/release/classes.jar
Copy the project root somewhere else
Rebuild the application again with the same command under the new root, store the same file
Compare files from two builds from different directories

ReproducerComposableKt.class will be the only file different between the resulting jars. By applying javap -c and comparing the output for these classes, you'll see the only difference there is a value for a constant used in startReplaceableGroup call.

For your convenience I also provide projects for compiler versions 1.5.4 and 1.5.3 I used to verify my findings (see reproducer.tgz). Also, the fileName value I extracted with a debugger from 1.5.4 project looks like this:

_Users_My_Name_AndroidStudioProjects_ComposeReproIssueReproducer_1_5_4_app_src_main_java_com_example_composereproissuereproducer_ReproducerComposableKt

reproducer.tgz

200 KB

Download

ReproducerComposable.kt

1.6 KB

Download

MainActivity.kt

1.1 KB

Download

Comments

as...@google.com <as...@google.com> Nov 30, 2023 06:15PM

Reassigned to as...@google.com.

ap...@google.com <ap...@google.com> #2Dec 5, 2023 11:16PM

Marked as fixed.

Project: platform/frameworks/support
Branch: androidx-master-dev

commit c60f33e229e31ab328ef6b59dab63b264954831c
Author: Alexandre Elias <aelias@google.com>
Date: Fri Jul 10 16:23:09 2020

Semantics no-op cleanups

Partly in response to lmr's broad code review, I did a pass of
superficial API/implementation cleanup. The main changes are:

- I changed each Boolean SemanticsProperty where false is equivalent to
not being present to take "Unit" instead. This is conceptually
clearer: it avoids questions like "can I cancel out a semantics from a
merged child by setting it to false?" Because "property = Unit" looks
weird, I also changed the style of these to "property()".

- I moved the Semantics id generator closer to where it's used, in
SemanticsModifierCore. I made it internal and an AtomicInt.
(Note that integer ids are heavily used in the Android
AccessibilityNodeInfo APIs so I can't simply remove them entirely.)

- I deleted dead code. Some examples include SemanticsHintOverrides,
a public API not connected to anything, and SemanticsPropertyKey
merge() open method which is never called. (In both cases I have
a different plan in mind for accessibility.)

Fixes: 145951226
Fixes: 145955412
Test: existing tests
Relnote: "Single-value semantics properties now use a calling style.
For example, 'semantics { hidden = true }' is now written as:
'semantics { hidden() }'."

Change-Id: Ic1afd12ea22c926babc9662f1804d80b33aa0cfc

M ui/integration-tests/benchmark/src/androidTest/java/androidx/ui/benchmark/test/LayoutNodeModifierBenchmark.kt
M ui/ui-core/api/0.1.0-dev15.txt
M ui/ui-core/api/current.txt
M ui/ui-core/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-core/api/public_plus_experimental_current.txt
M ui/ui-core/api/restricted_0.1.0-dev15.txt
M ui/ui-core/api/restricted_current.txt
M ui/ui-core/src/androidAndroidTest/kotlin/androidx/ui/graphics/vector/VectorTest.kt
M ui/ui-core/src/androidAndroidTest/kotlin/androidx/ui/semantics/SemanticsTests.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidActuals.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidComposeView.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidComposeViewAccessibilityDelegateCompat.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidPopup.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/Expect.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsConfiguration.kt
D ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsHintOverrides.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsModifier.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsNode.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsOwner.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsWrapper.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/semantics/SemanticsProperties.kt
M ui/ui-foundation/api/0.1.0-dev15.txt
M ui/ui-foundation/api/current.txt
M ui/ui-foundation/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-foundation/api/public_plus_experimental_current.txt
M ui/ui-foundation/api/restricted_0.1.0-dev15.txt
M ui/ui-foundation/api/restricted_current.txt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Clickable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Dialog.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Scroller.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/selection/Selectable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/selection/Toggleable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/semantics/FoundationSemanticsProperties.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ButtonTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/CardTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/CheckboxScreenshotTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/RadioButtonScreenshotTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ScaffoldTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/SnackbarTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/SurfaceTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ripple/RippleIndicationTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/textfield/TextFieldScreenshotTest.kt
M ui/ui-material/src/main/java/androidx/ui/material/AppBar.kt
M ui/ui-material/src/main/java/androidx/ui/material/TextFieldImpl.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/AssertsTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/CallSemanticsActionTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/ErrorMessagesTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/FindersTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/PrintToStringTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/ScrollToTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/TextActionsTest.kt
M ui/ui-test/src/main/java/androidx/ui/test/Actions.kt
M ui/ui-test/src/main/java/androidx/ui/test/Filters.kt
M ui/ui-text/api/0.1.0-dev15.txt
M ui/ui-text/api/current.txt
M ui/ui-text/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-text/api/public_plus_experimental_current.txt
M ui/ui-text/api/restricted_0.1.0-dev15.txt
M ui/ui-text/api/restricted_current.txt
M ui/ui-text/src/commonMain/kotlin/androidx/ui/text/CoreTextField.kt
M ui/ui-text/src/commonMain/kotlin/androidx/ui/text/TextSemanticsProperties.kt

https://android-review.googlesource.com/1360099

am...@gmail.com <am...@gmail.com> #3Dec 14, 2023 05:33AM

Hi. Is this released yet? If so, please ignore this comment.

According to Ian Lake's social media post today about the December 13 AndroidX releases, those were the last release of 2023. It seems this fix did not make it. However this issue has affected lot of apps on F-droid. So it would be great if this fix was released sooner rather than later.

as...@google.com <as...@google.com> #4Dec 14, 2023 01:53PM

Comment has been deleted.

Message last modified on Dec 14, 2023 01:54PM

as...@google.com <as...@google.com> #5Dec 14, 2023 01:56PM

We don't take this path by default in Compose compiler 1.5.6, so it shouldn't affect apps unless intrinsic remember is disabled.

Issue 313937484

Description

Issue summary

Comments

as...@google.com <as...@google.com> Nov 30, 2023 06:15PM

ap...@google.com <ap...@google.com> #2Dec 5, 2023 11:16PM

am...@gmail.com <am...@gmail.com> #3Dec 14, 2023 05:33AM

as...@google.com <as...@google.com> #4Dec 14, 2023 01:53PM

as...@google.com <as...@google.com> #5Dec 14, 2023 01:56PM

Add comment

Issue metadata