Intrinsic remember changes behavior related to Java method refs [319810819]

Fixed

Bug

Status Update

No update yet.

Description

yo...@gmail.com

created issue #1

Jan 13, 2024 03:35AM

Jetpack Compose version: bom 2023.10.01

Jetpack Compose component(s) used: remember

Kotlin version: 1.9.22

compose compiler: 1.5.8

The way remember treats Java method refs as keys differs with Intrinsic remember.

The Slider in the code below DOES work if intrinsic remember is disabled. but you can NOT slide the Slider if intrinsic remember is enabled (which is the default in 1.5.8)

The Slider also works with kotlin 1.9.20 and 1.5.5 (before intrinsic remember was enabled by default) .

Also, note that if you use a Kotlin class instead of a Java class here then the Slider works fine with intrinsic remember enabled.

The problem seems to be that remember thinks the methodRef key is changing on every recomposition when intrinsic remember is enabled .

This is a minimal reproducer.

@Preview
@Composable
fun JavaMethodRefPreview() {
    val javaClass = remember {
        JavaClass()
    }

    var sliderState by rememberSliderState(
        methodRef = javaClass::javaMethod,
    )
    Slider(
        modifier = Modifier.padding(horizontal = 32.dp),
        value = sliderState,
        onValueChange = {
            sliderState = it
        })
}


@Composable
private fun rememberSliderState(
    methodRef: (Float) -> Float,
): MutableState<Float> = remember(methodRef) { mutableStateOf(0f) }

public class JavaClass {
    float javaMethod(float x) {
        return x;
    }
}

Note: here is the config to disable intrinsicRemember

tasks.withType<org.jetbrains.kotlin.gradle.tasks.KotlinCompile> {
    compilerOptions.freeCompilerArgs.addAll(
        "-P",
        "plugin:androidx.compose.compiler.plugins.kotlin:intrinsicRemember=false",
    )
}

Comments

yo...@gmail.com <yo...@gmail.com> #2Jan 15, 2024 01:40PM

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

as...@google.com <as...@google.com> Jan 15, 2024 05:13PM

Assigned to as...@google.com.

as...@google.com <as...@google.com> #3Jan 15, 2024 06:13PM

Is the Java class located in the same module? For some reason the repro works for me.

Looking at generated code, I see that javaClass::javaMethod is memoized in my case. Otherwise, the generated function reference uses referential equality now, which wasn't the case before (only skipping code was using referential equality), so maybe that's the reason for behavior change.

as...@google.com <as...@google.com> #4Jan 15, 2024 06:15PM

Nvm, I think the repro works on my side because of strong skipping.

ap...@google.com <ap...@google.com> #5Jan 16, 2024 08:08PM

Marked as fixed.

Project: platform/frameworks/support
Branch: androidx-main

commit 77128e08f24eba21d3a0e416ac238917a235dd8c
Author: Andrei Shikov <ashikov@google.com>
Date: Mon Jan 15 19:29:09 2024

Force .changed for function types with intrinsic remember

Intrinsic remember currently uses the same .changed implementation as skipping. This changes behavior when key is of function type, recreating remembered value more frequently.

Fixes: 319810819
Test: Compiler & runtime tests
Change-Id: I748272758d15a0716cea646d451880daf1384492

M compose/compiler/compiler-hosted/integration-tests/src/jvmTest/kotlin/androidx/compose/compiler/plugins/kotlin/RememberIntrinsicTransformTests.kt
A compose/compiler/compiler-hosted/integration-tests/src/test/resources/golden/androidx.compose.compiler.plugins.kotlin.RememberIntrinsicTransformTestsStrongSkipping/testRememberMethodReference[useFir = false].txt
A compose/compiler/compiler-hosted/integration-tests/src/test/resources/golden/androidx.compose.compiler.plugins.kotlin.RememberIntrinsicTransformTestsStrongSkipping/testRememberMethodReference[useFir = true].txt
M compose/compiler/compiler-hosted/src/main/java/androidx/compose/compiler/plugins/kotlin/lower/AbstractComposeLowering.kt
M compose/compiler/compiler-hosted/src/main/java/androidx/compose/compiler/plugins/kotlin/lower/ComposableFunctionBodyTransformer.kt
M compose/compiler/compiler-hosted/src/main/java/androidx/compose/compiler/plugins/kotlin/lower/ComposerLambdaMemoization.kt
M compose/runtime/runtime/src/nonEmulatorJvmTest/kotlin/androidx/compose/runtime/JvmCompositionTests.kt

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

na...@google.com <na...@google.com> #6Jan 24, 2024 07:52PM

The following release(s) address this bug.It is possible this bug has only been partially addressed:

androidx.wear.protolayout:protolayout-material-core:1.1.0-rc01
androidx.wear.tiles:tiles-tooling:1.3.0-rc01
androidx.wear.tiles:tiles-tooling-preview:1.3.0-rc01

Issue 319810819