j-roskopf / ComposeGuard

A Gradle plugin for detecting regressions in Jetpack Compose / Compose Multiplatform
MIT License
107 stars 1 forks source link

False negative (bug in compose compiler?) when consuming unstable model classes from another module #62

Open matejdro opened 1 day ago

matejdro commented 1 day ago

I found this weird quirk of the Compose compiler. When declaring an unstable class in one module:

data class UnstableDataClass(var text: String)

(confirmed that it is really unstable:

unstable class UnstableDataClass {
  stable var text: String
  <runtime stability> = Unstable
}

)

and then consuming that class in another module:

@Composable
fun Greeting(name: UnstableDataClass, modifier: Modifier = Modifier) {
    Text(
        text = "Hello ${name.text}!",
        modifier = modifier
    )
}

function will be somehow marked as restartable skippable, even though it uses unstable data class:

restartable skippable scheme("[androidx.compose.ui.UiComposable]") fun Greeting(
  name: UnstableDataClass
  stable modifier: Modifier? = @static Companion
)

and the unstable class does not have any modifier (neither stable nor unstable).

Of course, since function is skippable, ComposeGuard does not report this case.

You can reproduce this by running debugComposeCompilerCheck in ConfigCacheDemo.zip.

Now, from what I checked, this function is actually NOT skippable at the runtime. It appears that Compose properly detects the instability at runtime, it's just the compiler report that is wrong.

Should ComposeGuard also consider parameters with no stability modifier as unstable (maybe with a setting)? The caveat here is that it will report even stable parameters from another modules as unstable, forcing user to add @Stable annotation.

j-roskopf commented 1 day ago

I asked around, and one of the compose engineers responded and stated:

Stability between modules is defined as "runtime" stability, since you can have different class definitions at compile and runtime time. Compose checks stability at runtime and decides whether to skip based on that value

I clarified and asked so parameters that have an absence of stable|unstable in the report are implied to have runtime stability and they responded they believed so.

I don't think it would be safe for compose guard to imply anything about parameters with no stability markers because it could be wrong just as often as it is right unfortunately. As in, I think we'd see the same exact compose compiler report between

data class UnstableDataClass(var text: String) and data class UnstableDataClass(val text: String)