Kotlin / kotlinx.collections.immutable

Immutable persistent collections for Kotlin
Apache License 2.0
1.16k stars 59 forks source link

persistentListOf & compose compiler issue #131

Closed Skyyo closed 10 months ago

Skyyo commented 1 year ago

Not sure if this is Compose Compiler or this collections library issue, so crossposting in both places. Link to google issue tracker - https://issuetracker.google.com/issues/254435410

Problem: ImmutableList is considered stable by the Compose Compiler, but creating a list of integers using persistentListOf(0, 1, 2) will cause recompositions. Creating list using listOf(0, 1, 2).toImmutableList() won't.

Steps to reproduce:

  1. Add "org.jetbrains.kotlinx:kotlinx-collections-immutable:0.3.5" dependency;
  2. Paste code below:

    
    @Composable
    fun TestScreen() {
    var count by remember { mutableStateOf(0) }
    val listViaPersistentListBuilder = remember { persistentListOf(1, 2, 3) }
    val listViaCasting = remember { listOf(1, 2, 3).toImmutableList() }
    
    Column {
        Texts(listViaPersistentListBuilder) // will cause recompositions
        Texts(listViaPersistentListBuilder as ImmutableList<Int>)  // casting fixes the issue
        Texts(listViaCasting) // works as expected
        Button(modifier = Modifier.size(108.dp), onClick = { count++ }) { Text(text = "count++") }
        CounterText(count)
    }
    }

@Composable fun Texts(items: ImmutableList) { for (item in items) { Text(text = "Item $item ") } }

@Composable fun CounterText(count: Int) { Text(text = "Count: $count") }


3. Click on "count++" button to cause recompositions;
4. Use layout inspector tool to track the recompositions. 

or use [this](https://github.com/Skyyo/kotlinx-collections-issue) repository

UPDATE:
google issue tracker got answer:
_We should consider adding PersistentList<T> and friends to this list or considering any interface that derives from any of the above also stable._

Is it possible to consider `immutableListOf()` builder since it matters for Compose?
andrewbailey commented 1 year ago

We've addressed this in the Compose compiler. All of the Persistent and Immutable collections in kotlinx.collections.immutable should be treated as inherently stable by the Compose compiler, including the *Of() builders.

We're maintaining these as a list of classes and functions that are assumed to be stable (rather than relying static analysis of Kotlin's collection types), so if the signatures change or if a new collection is added, we might need to update our list again. But this shouldn't require any changes to the kotlinx artifact.

avidraghav commented 1 year ago

Hi @andrewbailey I'm facing the same issue and I'm using the Compose Compiler Version 1.5.0 which is almost latest Here is my data class

@Stable
data class LaunchesScreenState(
    val launches: ImmutableList<LaunchDetail> = persistentListOf(),
    val isLoading: Boolean = false,
    val errorMessage: String? = null
)

The Output of Compose Compiler Metric report for the above class is

stable class LaunchesScreenState {
  runtime val reminders: ImmutableList<LaunchDetail>
  stable val isLoading: Boolean
  stable val infoMessage: String?
}

The Compose compiler cannot infer that variable reminders is stable even though I have used ImmutableList and the Reminder class is considered stable. Does runtime val means that the stability will be inferred at runtime?

andrewbailey commented 1 year ago

Runtime stability indicates that the stability can't be guaranteed at compile-time because the type isn't marked as @Stable and isn't one of the known-stable types. In other words, the type ImmutableList<LaunchDetail> isn't stable as part of its API contract. The type could still be stable, though, as inferred by the Compose compiler. That can change out from underneath us if the type is in a different compilation unit that changes underneath us without you recompiling your project (e.g. if you're a library and need to know the stability of a class in another library), which is why we wait until runtime to determine stability.

Before this change, ImmutableList would have been treated as unstable, because it's an interface. But type parameters also influence the stability of a type. So ImmutableList<LaunchDetail> can only be stable if both the ImmutableList and LaunchDetail are stable. What's likely happening is that LaunchDetail isn't explicitly marked as Stable, so Compose will wait to resolve its stability until runtime. If that's the case, you can ignore this so long as LaunchDetail is in fact being inferred as stable.

If LaunchDetail is expected to be a known stable type (i.e. it's marked with @Stable), then ImmutableList<LaunchDetail> should be inferred as stable and this is likely an issue either in the reporting or in our stability inferencing. But if LaunchDetail is getting the stability inferencing from the Compose compiler, then this seems to be working as expected.

avidraghav commented 1 year ago

Thanks for the info @andrewbailey Can I say that even if LaunchDetail is being considered Stable in the Compiler Metrics Report but it isn't marked with the @Stable annotation then the variable val launches: ImmutableList<LaunchDetail> = persistentListOf() still wouldn't be considered Stable at compile time?

andrewbailey commented 1 year ago

It depends on how LaunchDetail is defined. A general overview for the stability inferencing rules looks something like this:

It's very likely that this is working as intended and your stability is being inferred correctly. I also don't know 100% for certain how that report output works compared to what the compiler sees, so the stability report may also just be showing an unexpected output.

If you're still unsure, you can consider debugging your program and see if you're observing more recompositions than you're expecting. If you can reproduce a problem where we're inferring stability incorrectly, please file a separate issue.

avidraghav commented 1 year ago

I see, thanks for the detailed explanation. I checked and there were no unnecessary recompositions. The LaunchDetail class is defined in my domain module which is different from module in which LaunchesScreenState resides (app module) but again I enabled the Compose compiler in the domain module (it is a Java/Kotlin library module) for report generation purposes and reverted the changes back before pushing the changes. I'll keep in mind the details you mentioned if I see unnecessary recompositions in the future. Thanks

qurbonzoda commented 10 months ago

Great to see the issue resolved.