Ambient reads from default parameters do not cause recompositions [143464846]

Fixed

Bug

Status Update

No update yet.

Description

lp...@google.com

created issue #1

Oct 28, 2019 04:03PM

Assume the following:

```
@Composable
fun FooComponent(number: Int = +ambient(NumberAmbient)) {
...
}

NumberAmbient.Provider(someChangingValue) {
FooComponent()
}
```

When the ambient value changes, because the ambient is being consumed inside the default value it looks like we are not correctly generating the code / observing changes to this ambient, so when we go to compare parameters, (my assumption is that) we see that no parameters have changed, and hence do not recompose.

The workaround is explicitly adding the `+ambient` consumption to the function body, and not in the default value, but then this defeats the point of default values. This currently causes smaller components that contain only comparable parameters, such as ProgressIndicators, to not recompose when the colour in the theme changes.

Comments

ch...@google.com <ch...@google.com> #2Oct 29, 2019 10:45PM

Exactly what Calin is working on right now, assigning to him to track

lp...@google.com <lp...@google.com> #3Oct 29, 2019 11:02PM

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

commit e4894d0b99ea6360cc84bc2bc55d6f602e3b8b63
Author: Calin Tataru <calintat@google.com>
Date: Mon Jul 27 10:53:31 2020

Refactor stateDraggable and rename to swipeable

This CL does the following things:
* Rename Modifier.stateDraggable to Modifier.swipeable.
* Make swipeable public and mark it as ExperimentalMaterialApi.
* Introduce the concept of SwipeableState which holds the current state
of the swipeable as well as the animated float, and provides methods for
updating the state and subscribing to more granular in-movement updates.
* Rename DrawerState to DrawerValue, and introduce a new DrawerState
class which is a subclass of SwipeableState<DrawerValue> and provides
more contextual methods such as isOpen or open(). Same for BottomDrawer.

Bug: 148023068
Test: Ran DrawerTest, ScaffoldTest and SwitchTest
Relnote: "Modifier.stateDraggable was completely reworked and renamed
to Modifier.swipeable. A new SwipeableState class was introduced, and
DrawerState and BottomDrawerState were refactored to inherit from it.
[Modal/Bottom]DrawerLayout no longer take an onStateChange parameter."
Change-Id: I7233229dfc9c04a4615f4c1cc29e604b97edd1df

M compose/compose-runtime/compose-runtime-benchmark/src/androidTest/java/androidx/compose/runtime/HotReloadIntegrationTests.kt
M ui/ui-material/api/current.txt
M ui/ui-material/api/public_plus_experimental_current.txt
M ui/ui-material/api/restricted_current.txt
M ui/ui-material/samples/src/main/java/androidx/compose/material/samples/DrawerSamples.kt
M ui/ui-material/samples/src/main/java/androidx/compose/material/samples/ScaffoldSamples.kt
A ui/ui-material/samples/src/main/java/androidx/compose/material/samples/SwipeableSamples.kt
M ui/ui-material/src/androidAndroidTest/kotlin/androidx/compose/material/DrawerScreenshotTest.kt
M ui/ui-material/src/androidAndroidTest/kotlin/androidx/compose/material/DrawerTest.kt
M ui/ui-material/src/androidAndroidTest/kotlin/androidx/compose/material/ScaffoldTest.kt
M ui/ui-material/src/commonMain/kotlin/androidx/compose/material/Drawer.kt
M ui/ui-material/src/commonMain/kotlin/androidx/compose/material/Scaffold.kt
A ui/ui-material/src/commonMain/kotlin/androidx/compose/material/Swipeable.kt
M ui/ui-material/src/commonMain/kotlin/androidx/compose/material/Switch.kt
D ui/ui-material/src/commonMain/kotlin/androidx/compose/material/internal/StateDraggable.kt

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

an...@gmail.com <an...@gmail.com> #8Oct 31, 2019 09:51AM

Block

le...@google.com <le...@google.com> #9Jan 14, 2020 07:50PM

Reassigned to le...@google.com.

Assigning to me as a reminder that this can be marked as fixed once composer param work is done

le...@google.com <le...@google.com> Apr 22, 2020 08:31PM

Accepted by le...@google.com.

ap...@google.com <ap...@google.com> #10May 13, 2020 08:13PM

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

commit 797bd0cda4fd10a1d9d8b3b06b5954644a200334
Author: Leland Richardson <lelandr@google.com>
Date: Thu May 07 15:16:51 2020

Turn on Function-body-based code generation strategy

Relnote: “
Changes the code generation strategy of Compose’s compiler. Prior to the change, the compose compiler would transform calls to composable functions. With this change, we now transform the body of a composable function and leave the callsite unaltered (mostly).

This means that most of the logic communicating with the compose runtime happens at the start of the function body, instead of at the callsite.

This should be a source-compatible change for all usage of compose. Most users of compose should not have to update any code as a result of this change.

In order to support this work, the JVM signature of all composable functions has changed. A Composable function accepting a single parameter is transformed into a function accepting 3 parameters, the additional parameters are the Composer, a ‘key’ integer. a bitmask integer used to propagate metadata through calls.

Compose now also transforms default arguments to a composable function. It does this without introducing an additional synthetic default overload of the function itself, so this change will result in fewer functions being defined.

Known intentional behavioral changes resulting from this:
1. Some calls will skip where they wouldn’t have previously
2. Composable expressions in default argument expressions are now correctly subscribed to and handled

This work included some optimizations:
1. The result of comparisons of parameters are propagated through the call graph to other composable functions. This will result in fewer comparisons at runtime, reduces the slot table size, as well as more skipping of composable functions that were previously not skipped
2. Paremeters which are determined to be “static” at compile time are no longer compared or stored in the runtime. This reduces the number of comparisons and reduces slot table size.
3. Control flow structure of the body of functions is used to minimize the number of groups that are generated. This reduces slot table size and results in less work for the runtime
4. Unused dispatch and receiver parameters to functions are not included in determining skippability of the function if they are not used inside of the body of the function.

Most breaking changes were for APIs that the compiler targets directly, and typical use of compose will not be affected:
1. Composer::startExpr was removed
2. Composer::endExpr was removed
3. Composer::call was deprecated
4. The non-varargs overloads of `key` have been removed. Use the `vararg` version going forward.
5. The Pivotal annotation was deprecated. Use `key` as a replacement.
6. ScopeUpdateScope::updateScope was changed to expect a Function3 instead of Function1
7. restartableFunction and restartableFunctionN were updated to include additional compile time parameters
“

Bug: 144283149
Bug: 144283416
Bug: 144283245
Bug: 150777987
Bug: 143464846

This effort has been a work in progress for some time, and much of the work has
been done in the following previous commits:

Make ComposableFunctionBodyTransformer work for any number of parameters
Icc265a253c629f5533f1e68e5109db67234311f9

Generate Movable groups with key and handle dynamic groups properly
I5fe7799eb303dadbbdd47d71fe764767407c4ae2

Function Body Skipping and Comparison Propagation
I2ee971458ea466ba070dff25da5a4f60d42ff678

Add Default Parameter IR Transform
If530f61034b284bb3e1a8919e2b0be2511e2f243

Handle Break/Continue in Control Flow Codegen
I2c2c3261886ab8936baeb8a5b5e6b8f5e9e8d409

Allow for early returns in control flow codegen
If6446a90b7364a31ae67816595408c0aa971a39a

Add ControlFlowTransformer WIP and tests
I1f96ab3e11e3f26bf4f9e375e00420d25463510d

Change-Id: I607560574d83b4b6c1e68ff72cc4124c5f8c2602

M compose/compose-compiler-hosted/integration-tests/src/test/java/androidx/compose/plugins/kotlin/AbstractIrTransformTest.kt
M compose/compose-compiler-hosted/integration-tests/src/test/java/androidx/compose/plugins/kotlin/AbstractLoweringTests.kt
M compose/compose-compiler-hosted/integration-tests/src/test/java/androidx/compose/plugins/kotlin/ComposeCallLoweringTests.kt
M compose/compose-compiler-hosted/integration-tests/src/test/java/androidx/compose/plugins/kotlin/ComposeMultiPlatformTests.kt
M compose/compose-compiler-hosted/integration-tests/src/test/java/androidx/compose/plugins/kotlin/ComposerParamSignatureTests.kt
M compose/compose-compiler-hosted/integration-tests/src/test/java/androidx/compose/plugins/kotlin/ComposerParamTransformTests.kt
M compose/compose-compiler-hosted/integration-tests/src/test/java/androidx/compose/plugins/kotlin/ControlFlowTransformTests.kt
M compose/compose-compiler-hosted/integration-tests/src/test/java/androidx/compose/plugins/kotlin/DefaultParamTransformTests.kt
A compose/compose-compiler-hosted/integration-tests/src/test/java/androidx/compose/plugins/kotlin/EmitTransformTests.kt
M compose/compose-compiler-hosted/integration-tests/src/test/java/androidx/compose/plugins/kotlin/FcsCodegenTests.kt
M compose/compose-compiler-hosted/integration-tests/src/test/java/androidx/compose/plugins/kotlin/FcsModelCodeGenTests.kt
M compose/compose-compiler-hosted/integration-tests/src/test/java/androidx/compose/plugins/kotlin/FunctionBodySkippingTransformTests.kt
M compose/compose-compiler-hosted/integration-tests/src/test/java/androidx/compose/plugins/kotlin/KtxCrossModuleTests.kt
M compose/compose-compiler-hosted/integration-tests/src/test/java/androidx/compose/plugins/kotlin/KtxModelCodeGenTests.kt
M compose/compose-compiler-hosted/integration-tests/src/test/java/androidx/compose/plugins/kotlin/KtxTransformationTest.kt
M compose/compose-compiler-hosted/integration-tests/src/test/java/androidx/compose/plugins/kotlin/RobolectricComposeTester.kt
M compose/compose-compiler-hosted/src/main/java/androidx/compose/plugins/kotlin/ComposeEmitResolver.kt
M compose/compose-compiler-hosted/src/main/java/androidx/compose/plugins/kotlin/ComposeFqNames.kt
M compose/compose-compiler-hosted/src/main/java/androidx/compose/plugins/kotlin/ComposeIrGenerationExtension.kt
M compose/compose-compiler-hosted/src/main/java/androidx/compose/plugins/kotlin/KtxNameConventions.kt
M compose/compose-compiler-hosted/src/main/java/androidx/compose/plugins/kotlin/compiler/lower/AbstractComposeLowering.kt
M compose/compose-compiler-hosted/src/main/java/androidx/compose/plugins/kotlin/compiler/lower/ComposableCallTransformer.kt
M compose/compose-compiler-hosted/src/main/java/androidx/compose/plugins/kotlin/compiler/lower/ComposableFunctionBodyTransformer.kt
M compose/compose-compiler-hosted/src/main/java/androidx/compose/plugins/kotlin/compiler/lower/ComposableTypeRemapper.kt
D compose/compose-compiler-hosted/src/main/java/androidx/compose/plugins/kotlin/compiler/lower/ComposeObservePatcher.kt
M compose/compose-compiler-hosted/src/main/java/androidx/compose/plugins/kotlin/compiler/lower/ComposerIntrinsicTransformer.kt
M compose/compose-compiler-hosted/src/main/java/androidx/compose/plugins/kotlin/compiler/lower/ComposerParamTransformer.kt
M compose/compose-compiler-hosted/src/main/java/androidx/compose/plugins/kotlin/compiler/lower/IrSourcePrinter.kt
M compose/compose-runtime/api/0.1.0-dev12.txt
M compose/compose-runtime/api/current.txt
M compose/compose-runtime/api/public_plus_experimental_0.1.0-dev12.txt
M compose/compose-runtime/api/public_plus_experimental_current.txt
M compose/compose-runtime/api/restricted_0.1.0-dev12.txt
M compose/compose-runtime/api/restricted_current.txt
M compose/compose-runtime/compose-runtime-benchmark/src/androidTest/java/androidx/compose/benchmark/siblings/SiblingManagement.kt
M compose/compose-runtime/samples/src/main/java/androidx/compose/samples/PivotalSamples.kt
M compose/compose-runtime/src/androidMain/kotlin/androidx/compose/KeySourceInfo.kt
M compose/compose-runtime/src/commonMain/kotlin/androidx/compose/Composer.kt
M compose/compose-runtime/src/commonMain/kotlin/androidx/compose/Key.kt
M compose/compose-runtime/src/commonMain/kotlin/androidx/compose/Pivotal.kt
M compose/compose-runtime/src/commonMain/kotlin/androidx/compose/SlotTable.kt
M compose/compose-runtime/src/commonMain/kotlin/androidx/compose/internal/RestartableFunction.kt
M compose/compose-runtime/src/jvmMain/kotlin/androidx/compose/internal/RestartableFunctionN.kt
M ui/ui-core/src/androidTest/java/androidx/ui/core/AndroidPointerInputTest.kt
M ui/ui-core/src/main/java/androidx/ui/core/ComposedModifier.kt
M ui/ui-core/src/main/java/androidx/ui/core/focus/FocusModifierImpl.kt
M ui/ui-core/src/main/java/androidx/ui/res/FontResources.kt
M ui/ui-core/src/test/java/androidx/ui/core/ComposedModifierTest.kt
M ui/ui-tooling/src/androidTest/java/androidx/ui/tooling/BoundsTest.kt
M ui/ui-tooling/src/androidTest/java/androidx/ui/tooling/InspectableTests.kt
M ui/ui-tooling/src/androidTest/java/androidx/ui/tooling/ModifierInfoTest.kt
M ui/ui-tooling/src/main/java/androidx/ui/tooling/preview/PreviewUtils.kt

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

le...@google.com <le...@google.com> May 13, 2020 08:34PM

Marked as fixed.

lp...@google.com <lp...@google.com> #11Jun 3, 2020 06:56PM

Status: Assigned (reopened)

Still seeing this in the demo app, switching from light -> dark theme doesn't cause the app bar to change color. But if I manually do:

val color = MaterialTheme.colors.primarySurface
TopAppBar(
    ...
    backgroundColor = color,
    ...
)

(manually writing the default value)

Then this works correctly again.

tc...@google.com <tc...@google.com> #12Jun 3, 2020 08:30PM

The initial change made it into Dev12 but these is an issue that came up that Leland is looking into and will be fixed in Alpha if necessary.

ap...@google.com <ap...@google.com> #13Jun 8, 2020 11:33PM

Marked as fixed.

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

commit 458aaff814870edec3837f3ebe343f197a198697
Author: Leland Richardson <lelandr@google.com>
Date: Mon Jun 08 15:00:32 2020

Prevent non-static default expressions from being treated as static

Fixes: 143464846
Change-Id: I26c8a7f54581c38aede45c5db2f2a2a4ee2e39df

M compose/compose-compiler-hosted/integration-tests/src/test/java/androidx/compose/plugins/kotlin/DefaultParamTransformTests.kt
M compose/compose-compiler-hosted/integration-tests/src/test/java/androidx/compose/plugins/kotlin/FunctionBodySkippingTransformTests.kt
M compose/compose-compiler-hosted/src/main/java/androidx/compose/plugins/kotlin/compiler/lower/ComposableFunctionBodyTransformer.kt

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

Issue 143464846

Description

Issue summary

Comments

ch...@google.com <ch...@google.com> #2Oct 29, 2019 10:45PM

lp...@google.com <lp...@google.com> #3Oct 29, 2019 11:02PM

ch...@google.com <ch...@google.com> #4Oct 29, 2019 11:20PM

lp...@google.com <lp...@google.com> #5Oct 30, 2019 12:43AM

ch...@google.com <ch...@google.com> #6Oct 30, 2019 04:28PM

ap...@google.com <ap...@google.com> #7Oct 30, 2019 08:44PM