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

> The Transition Kotlin extensions target the Framework Transition class which is a bit confusing

All extensions in 'core-ktx' target either 'core' or the framework types. This is working as expected. It sounds like this is a feature request for a transition-ktx with equivalent extensions for the androidx.transition types.

> as most framework classes are being deprecated in favor of their AndroidX counterparts, Fragments for example.

"Most"? This *dramatically* overstates what is actually happening. Fragments (and loaders) were added to the framework in a time where the Android team did not have external libraries for shipping features. Transition was added 5 years later.

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

Pardon my misunderstanding, I was under the belief that where possible, new features that could be developed outside the framework, would be under Androidx. Also it's a common refrain to use the Androidx libraries over the framework libraries where possible, as those ship update independently of platform releases. I wasn't attempting to be dramatic.

If the Transition api is unlikely to change, there is no need for a transition-ktx for androidx.transition types. If developers should use Androidx transitions over framework transitions where possible however, then it would be much appreciated.

I am well aware of the limitations that existed when Fragments and Loaders were created, and when the Transition api was introduced.

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

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

commit ae42e311ecfbac18cedc90e489d1fdde956a4557
Author: Jake Wharton <jakew@google.com>
Date: Fri Nov 22 11:13:30 2019

Introduce transition-ktx for standalone transition library

These extensions are the same as androidx.core.transition for the platform types, just adapted to the standalone library.

Bug: 138870873
Test: gw :transition:transition-ktx:build :transition:transition-ktx:connectedCheck
Change-Id: Ie80022cc60ed1f482b7593db5ef1ac1c5ea33d8d

M buildSrc/src/main/kotlin/androidx/build/PublishDocsRules.kt
M jetifier/jetifier/migration.config
M settings.gradle
A transition/transition-ktx/OWNERS
A transition/transition-ktx/api/1.4.0-alpha01.txt
A transition/transition-ktx/api/current.txt
A transition/transition-ktx/api/public_plus_experimental_1.4.0-alpha01.txt
A transition/transition-ktx/api/public_plus_experimental_current.txt
A transition/transition-ktx/api/res-1.4.0-alpha01.txt
A transition/transition-ktx/api/restricted_1.4.0-alpha01.txt
A transition/transition-ktx/api/restricted_current.txt
A transition/transition-ktx/build.gradle
A transition/transition-ktx/src/androidTest/AndroidManifest.xml
A transition/transition-ktx/src/androidTest/java/androidx/transition/TestActivity.kt
A transition/transition-ktx/src/androidTest/java/androidx/transition/TransitionTest.kt
A transition/transition-ktx/src/androidTest/res/layout/test_activity.xml
A transition/transition-ktx/src/main/AndroidManifest.xml
A transition/transition-ktx/src/main/java/androidx/transition/Transition.kt

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

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