Bound function reference to stable class's method with arguments breaks skipping [302680514]

Fixed

Feature Request

Status Update

No update yet.

Description

el...@reaktor.com

created issue #1

Sep 30, 2023 02:18PM

Jetpack Compose version: 1.6.0-alpha06
Jetpack Compose component(s) used: @Composable
Kotlin version: 1.9.10
Compose compiler version: 1.5.3

Steps to Reproduce or Code Sample to Reproduce:

class MainActivity2 : ComponentActivity() {
    override fun onCreate(savedInstanceState: Bundle?) {
        super.onCreate(savedInstanceState)

        val cl = StableClass()
        setContent {
            Root(cl)
        }
    }
}

@Composable
fun Root(stableClass: StableClass) {
    Log.w("TEST", "Recomposing root")
    var internalCounter by remember { mutableIntStateOf(0) }

    Button(onClick = { internalCounter += 1 }) {
        Text("Increment ($internalCounter)")
    }

    internalCounter // recompose this scope when changed
    Skippable(stableClass::method)
}

var previous: Any? = null

class StableClass {
    fun method(arg: Int) {}
}

@Composable
fun Skippable(functionRef: (Int) -> Unit) {
    Log.w("TEST", "Recomposing while equals=${functionRef == previous}")
    previous = functionRef
}

Results in log output:

 W  Recomposing root
 W  Recomposing while equals=false
 W  Recomposing root
 W  Recomposing while equals=true
 W  Recomposing root
 W  Recomposing while equals=true
 W  Recomposing root
 W  Recomposing while equals=true
[...]

The Android Studio debugger claims that the parameter has changed even though equals returns true.

Composable fun Skippable(): Arguments: Different: ["functionRef"]

If the method signature is changed to not have arguments, skipping occurs normally:
fun method() {}
If a new lambda is created instead of passing the function reference, skipping occurs normally:
Skippable { stableClass.method(it) }

Comments

as...@google.com <as...@google.com> #2Oct 3, 2023 12:59PM

Status: Won't Fix (Intended Behavior)

> 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.

js...@google.com <js...@google.com> #3Oct 3, 2023 03:11PM

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.

el...@reaktor.com <el...@reaktor.com> #4Oct 4, 2023 06:55AM

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

as...@google.com <as...@google.com> #5Oct 4, 2023 08:09AM

Assigned to as...@google.com.

I see, this feature request sounds legit to me I think we should be able to remember reference based on receiver types.

el...@reaktor.com <el...@reaktor.com> #6Oct 4, 2023 09:18AM

Regarding #3, #5; based on the compiler source it seems function reference memoization is already implemented in the compiler, it just doesn't work correctly (see #4).

as...@google.com <as...@google.com> #7Oct 4, 2023 09:26AM

It is not quite the case, the line of code you linked prevents memoization in cases if someone does cls::invoke(0)

We'll need to support function references separately, as IR shape for them is different

ap...@google.com <ap...@google.com> #8Nov 13, 2023 05:30PM

Marked as fixed.

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

commit c05d2bbe2b44dfead66315174a4d82951fd56b46
Author: Elias Benkhodja <elias.benkhodja@reaktor.com>
Date: Wed Oct 04 20:57:13 2023

Memoize function references with arguments

`ComposerLambdaMemoization` tried to prematurely prevent potential
syntax addition not breaking function reference optimization by checking
for amount of value parameters.

This inadvertently broke memoization of function references to methods
with more than zero arguments, since `IrFunctionReferenceImpl` currently
reflects the amount of value arguments in the referenced function.

Add a check to disable memoization for function references with context
receivers. Kotlin doesn't support them yet, and behavior might change in
the future.

This change also enables memoizing function references without checking
for stability if strong skipping is enabled.

Test: Added new tests to `LambdaMemoizationTransformTests`:
* `testNonComposableFunctionReferenceWithNoArgumentsMemoization`
* `testNonComposableFunctionReferenceWithArgumentsMemoization`
* `testNonComposableFunctionReferenceWithStableContextReceiverNotMemoized`
* `testUnstableReceiverFunctionReferenceNotMemoized`
* `testUnstableExtensionReceiverFunctionReferenceNotMemoized``

Added new tests to `StrongSkippingModeTransformTests`:
* `testUnstableReceiverFunctionReferenceMemoized`
* `testUnstableExtensionReceiverFunctionReferenceMemoized`

Fixes:

b/302680514

Relnote: Memoize stable function references with arguments
Change-Id: I4d7bf3061e60eaa6fd3af60662d6a7b02c1a83b2

M compose/compiler/compiler-hosted/integration-tests/src/jvmTest/kotlin/androidx/compose/compiler/plugins/kotlin/LambdaMemoizationTransformTests.kt
M compose/compiler/compiler-hosted/integration-tests/src/jvmTest/kotlin/androidx/compose/compiler/plugins/kotlin/StrongSkippingModeTransformTests.kt
A compose/compiler/compiler-hosted/integration-tests/src/test/resources/golden/androidx.compose.compiler.plugins.kotlin.LambdaMemoizationTransformTests/testUnstableExtensionReceiverFunctionReferenceNotMemoized[useFir = false].txt
A compose/compiler/compiler-hosted/integration-tests/src/test/resources/golden/androidx.compose.compiler.plugins.kotlin.LambdaMemoizationTransformTests/testUnstableExtensionReceiverFunctionReferenceNotMemoized[useFir = true].txt
A compose/compiler/compiler-hosted/integration-tests/src/test/resources/golden/androidx.compose.compiler.plugins.kotlin.LambdaMemoizationTransformTests/testUnstableReceiverFunctionReferenceNotMemoized[useFir = false].txt
A compose/compiler/compiler-hosted/integration-tests/src/test/resources/golden/androidx.compose.compiler.plugins.kotlin.LambdaMemoizationTransformTests/testUnstableReceiverFunctionReferenceNotMemoized[useFir = true].txt
A compose/compiler/compiler-hosted/integration-tests/src/test/resources/golden/androidx.compose.compiler.plugins.kotlin.StrongSkippingModeTransformTests/testUnstableExtensionReceiverFunctionReferenceMemoized[useFir = false].txt
A compose/compiler/compiler-hosted/integration-tests/src/test/resources/golden/androidx.compose.compiler.plugins.kotlin.StrongSkippingModeTransformTests/testUnstableExtensionReceiverFunctionReferenceMemoized[useFir = true].txt
A compose/compiler/compiler-hosted/integration-tests/src/test/resources/golden/androidx.compose.compiler.plugins.kotlin.StrongSkippingModeTransformTests/testUnstableReceiverFunctionReferenceMemoized[useFir = false].txt
A compose/compiler/compiler-hosted/integration-tests/src/test/resources/golden/androidx.compose.compiler.plugins.kotlin.StrongSkippingModeTransformTests/testUnstableReceiverFunctionReferenceMemoized[useFir = true].txt
M compose/compiler/compiler-hosted/src/main/java/androidx/compose/compiler/plugins/kotlin/lower/ComposerLambdaMemoization.kt

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

pr...@google.com <pr...@google.com> #9Nov 29, 2023 09:39PM

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

androidx.compose.compiler:compiler-hosted:1.5.5