Skip to content

Commit eb9c553

Browse files
derekxu16derekxu16
committed
Fix StabilityInferencer's handling of internal types
In the past, if an `internal` type `T` was inferred to be stable when it was compiled in a compilation unit `A`, we would infer it to be stable when compiling a compilation unit `B` that uses the type. This caused failures in situations in which `T` was later made unstable by an edit and `A` was incrementally recompiled, but `B` was not. This change eliminates the possibility of such failures. Test: ClassStabilityTransformTests.testInferredStabilityOfInternalTypesFromOtherCompilationUnits Fixes: 427530633
1 parent 843d70a commit eb9c553

File tree

7 files changed

+219
-44
lines changed

7 files changed

+219
-44
lines changed

‎plugins/compose/compiler-hosted/integration-tests/src/jvmTest/kotlin/androidx/compose/compiler/plugins/kotlin/AbstractLiveLiteralTransformTests.kt‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -58,10 +58,7 @@ abstract class AbstractLiveLiteralTransformTests(
5858
pluginContext: IrPluginContext,
5959
) {
6060
val keyVisitor = DurableKeyVisitor(builtKeys)
61-
val stabilityInferencer = StabilityInferencer(
62-
pluginContext.moduleDescriptor,
63-
emptySet()
64-
)
61+
val stabilityInferencer = StabilityInferencer(emptySet())
6562
val featureFlags = FeatureFlags()
6663
val transformer = object : LiveLiteralTransformer(
6764
liveLiteralsEnabled || liveLiteralsV2Enabled,

‎plugins/compose/compiler-hosted/integration-tests/src/jvmTest/kotlin/androidx/compose/compiler/plugins/kotlin/ClassStabilityTransformTests.kt‎

Lines changed: 37 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -193,19 +193,6 @@ class ClassStabilityTransformTests(useFir: Boolean) : AbstractIrTransformTest(us
193193
"Uncertain(Foo)"
194194
)
195195

196-
@Test
197-
fun testInterfacesAreUncertainOnIncrementalCompilation() {
198-
assertStability(
199-
classDefSrc = """
200-
interface Foo
201-
""",
202-
transform = {
203-
it.origin = IrDeclarationOrigin.IR_EXTERNAL_DECLARATION_STUB
204-
},
205-
stability = "Uncertain(Foo)"
206-
)
207-
}
208-
209196
@Test
210197
fun testInterfaceWithStableValAreUncertain() = assertStability(
211198
"interface Foo { val value: Int }",
@@ -223,6 +210,40 @@ class ClassStabilityTransformTests(useFir: Boolean) : AbstractIrTransformTest(us
223210
"Runtime(A),Runtime(B),Runtime(C)"
224211
)
225212

213+
// In the past, if an `internal` type `T` was inferred to be stable when it was compiled in a
214+
// compilation unit `A`, we would infer it to be stable when compiling a compilation unit `B`
215+
// that uses the type. This caused failures in situations in which `T` was later made unstable
216+
// by an edit and `A` was incrementally recompiled, but `B` was not. This test serves as a
217+
// regression test against such failures, e.g. https://issuetracker.google.com/issues/427530633.
218+
@Test
219+
fun testInferredStabilityOfInternalTypesFromOtherCompilationUnits() {
220+
assertStability(
221+
classDefSrc = """
222+
import androidx.compose.runtime.internal.StabilityInferred
223+
224+
@StabilityInferred(parameters = 1)
225+
internal class Foo
226+
""",
227+
transform = {
228+
it.origin = IrDeclarationOrigin.IR_EXTERNAL_DECLARATION_STUB
229+
},
230+
stability = "Runtime(Foo)"
231+
)
232+
233+
assertStability(
234+
classDefSrc = """
235+
import androidx.compose.runtime.internal.StabilityInferred
236+
237+
@StabilityInferred(parameters = 1)
238+
internal interface Bar
239+
""",
240+
transform = {
241+
it.origin = IrDeclarationOrigin.IR_EXTERNAL_DECLARATION_STUB
242+
},
243+
stability = "Runtime(Bar)"
244+
)
245+
}
246+
226247
@Test
227248
fun testStable17() = assertStability(
228249
"""
@@ -1720,7 +1741,7 @@ class ClassStabilityTransformTests(useFir: Boolean) : AbstractIrTransformTest(us
17201741
})
17211742
val irClass = (irModule.files.last().declarations.first() as IrClass).apply(transform)
17221743
val externalTypeMatchers = externalTypes.map { FqNameMatcher(it) }.toSet()
1723-
val stabilityInferencer = StabilityInferencer(irModule.descriptor, externalTypeMatchers)
1744+
val stabilityInferencer = StabilityInferencer(externalTypeMatchers)
17241745
val classStability = stabilityInferencer.stabilityOf(irClass.defaultType as IrType)
17251746

17261747
assertEquals(
@@ -1749,8 +1770,7 @@ class ClassStabilityTransformTests(useFir: Boolean) : AbstractIrTransformTest(us
17491770
val irClass = irModule.files.last().declarations.first() as IrClass
17501771
val externalTypeMatchers = externalTypes.map { FqNameMatcher(it) }.toSet()
17511772
val classStability =
1752-
StabilityInferencer(irModule.descriptor, externalTypeMatchers)
1753-
.stabilityOf(irClass.defaultType as IrType)
1773+
StabilityInferencer(externalTypeMatchers).stabilityOf(irClass.defaultType as IrType)
17541774

17551775
assertEquals(
17561776
stability,
@@ -1893,7 +1913,7 @@ class ClassStabilityTransformTests(useFir: Boolean) : AbstractIrTransformTest(us
18931913
}
18941914
val externalTypeMatchers = externalTypes.map { FqNameMatcher(it) }.toSet()
18951915
val exprStability =
1896-
StabilityInferencer(irModule.descriptor, externalTypeMatchers).stabilityOf(irExpr)
1916+
StabilityInferencer(externalTypeMatchers).stabilityOf(irExpr)
18971917

18981918
assertEquals(
18991919
stability,

‎plugins/compose/compiler-hosted/integration-tests/src/jvmTest/kotlin/androidx/compose/compiler/plugins/kotlin/FunctionBodySkippingTransformTests.kt‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -599,6 +599,23 @@ class FunctionBodySkippingTransformTests(
599599
"""
600600
)
601601

602+
@Test
603+
fun testInternalStableUnstableParams(): Unit = comparisonPropagation(
604+
"""
605+
internal class Foo(var value: Int = 0)
606+
""",
607+
"""
608+
@Composable
609+
internal fun CanSkip(foo: Foo = Foo()) {
610+
used(foo)
611+
}
612+
@Composable
613+
internal fun CannotSkip(foo: Foo) {
614+
used(foo)
615+
}
616+
"""
617+
)
618+
602619
@Test
603620
fun testOptionalUnstableWithStableExtensionReceiver(): Unit = comparisonPropagation(
604621
"""

‎plugins/compose/compiler-hosted/integration-tests/src/jvmTest/resources/golden/androidx.compose.compiler.plugins.kotlin.FunctionBodySkippingTransformTests/testInternalStableUnstableParams[useFir = false].txt‎

Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,81 @@
1+
//
2+
// Source
3+
// ------------------------------------------
4+
5+
import androidx.compose.runtime.Composable
6+
import androidx.compose.runtime.NonRestartableComposable
7+
import androidx.compose.runtime.ReadOnlyComposable
8+
9+
10+
@Composable
11+
internal fun CanSkip(foo: Foo = Foo()) {
12+
used(foo)
13+
}
14+
@Composable
15+
internal fun CannotSkip(foo: Foo) {
16+
used(foo)
17+
}
18+
19+
//
20+
// Transformed IR
21+
// ------------------------------------------
22+
23+
@Composable
24+
internal fun CanSkip(foo: Foo?, %composer: Composer?, %changed: Int, %default: Int) {
25+
%composer = %composer.startRestartGroup(<>)
26+
sourceInformation(%composer, "C(CanSkip):Test.kt")
27+
val %dirty = %changed
28+
if (%changed and 0b0110 == 0) {
29+
%dirty = %dirty or if (%default and 0b0001 == 0 && %composer.changedInstance(foo)) 0b0100 else 0b0010
30+
}
31+
if (%composer.shouldExecute(%dirty and 0b0011 != 0b0010, %dirty and 0b0001)) {
32+
%composer.startDefaults()
33+
if (%changed and 0b0001 == 0 || %composer.defaultsInvalid) {
34+
if (%default and 0b0001 != 0) {
35+
foo = Foo()
36+
%dirty = %dirty and 0b1110.inv()
37+
}
38+
} else {
39+
%composer.skipToGroupEnd()
40+
if (%default and 0b0001 != 0) {
41+
%dirty = %dirty and 0b1110.inv()
42+
}
43+
}
44+
%composer.endDefaults()
45+
if (isTraceInProgress()) {
46+
traceEventStart(<>, %dirty, -1, <>)
47+
}
48+
used(foo as Foo)
49+
if (isTraceInProgress()) {
50+
traceEventEnd()
51+
}
52+
} else {
53+
%composer.skipToGroupEnd()
54+
}
55+
%composer.endRestartGroup()?.updateScope { %composer: Composer?, %force: Int ->
56+
CanSkip(foo, %composer, updateChangedFlags(%changed or 0b0001), %default)
57+
}
58+
}
59+
@Composable
60+
internal fun CannotSkip(foo: Foo, %composer: Composer?, %changed: Int) {
61+
%composer = %composer.startRestartGroup(<>)
62+
sourceInformation(%composer, "C(CannotSkip):Test.kt")
63+
val %dirty = %changed
64+
if (%changed and 0b0110 == 0) {
65+
%dirty = %dirty or if (%composer.changedInstance(foo)) 0b0100 else 0b0010
66+
}
67+
if (%composer.shouldExecute(%dirty and 0b0011 != 0b0010, %dirty and 0b0001)) {
68+
if (isTraceInProgress()) {
69+
traceEventStart(<>, %dirty, -1, <>)
70+
}
71+
used(foo)
72+
if (isTraceInProgress()) {
73+
traceEventEnd()
74+
}
75+
} else {
76+
%composer.skipToGroupEnd()
77+
}
78+
%composer.endRestartGroup()?.updateScope { %composer: Composer?, %force: Int ->
79+
CannotSkip(foo, %composer, updateChangedFlags(%changed or 0b0001))
80+
}
81+
}

‎plugins/compose/compiler-hosted/integration-tests/src/jvmTest/resources/golden/androidx.compose.compiler.plugins.kotlin.FunctionBodySkippingTransformTests/testInternalStableUnstableParams[useFir = true].txt‎

Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,81 @@
1+
//
2+
// Source
3+
// ------------------------------------------
4+
5+
import androidx.compose.runtime.Composable
6+
import androidx.compose.runtime.NonRestartableComposable
7+
import androidx.compose.runtime.ReadOnlyComposable
8+
9+
10+
@Composable
11+
internal fun CanSkip(foo: Foo = Foo()) {
12+
used(foo)
13+
}
14+
@Composable
15+
internal fun CannotSkip(foo: Foo) {
16+
used(foo)
17+
}
18+
19+
//
20+
// Transformed IR
21+
// ------------------------------------------
22+
23+
@Composable
24+
internal fun CanSkip(foo: Foo?, %composer: Composer?, %changed: Int, %default: Int) {
25+
%composer = %composer.startRestartGroup(<>)
26+
sourceInformation(%composer, "C(CanSkip)N(foo):Test.kt")
27+
val %dirty = %changed
28+
if (%changed and 0b0110 == 0) {
29+
%dirty = %dirty or if (%default and 0b0001 == 0 && %composer.changedInstance(foo)) 0b0100 else 0b0010
30+
}
31+
if (%composer.shouldExecute(%dirty and 0b0011 != 0b0010, %dirty and 0b0001)) {
32+
%composer.startDefaults()
33+
if (%changed and 0b0001 == 0 || %composer.defaultsInvalid) {
34+
if (%default and 0b0001 != 0) {
35+
foo = Foo()
36+
%dirty = %dirty and 0b1110.inv()
37+
}
38+
} else {
39+
%composer.skipToGroupEnd()
40+
if (%default and 0b0001 != 0) {
41+
%dirty = %dirty and 0b1110.inv()
42+
}
43+
}
44+
%composer.endDefaults()
45+
if (isTraceInProgress()) {
46+
traceEventStart(<>, %dirty, -1, <>)
47+
}
48+
used(foo as Foo)
49+
if (isTraceInProgress()) {
50+
traceEventEnd()
51+
}
52+
} else {
53+
%composer.skipToGroupEnd()
54+
}
55+
%composer.endRestartGroup()?.updateScope { %composer: Composer?, %force: Int ->
56+
CanSkip(foo, %composer, updateChangedFlags(%changed or 0b0001), %default)
57+
}
58+
}
59+
@Composable
60+
internal fun CannotSkip(foo: Foo, %composer: Composer?, %changed: Int) {
61+
%composer = %composer.startRestartGroup(<>)
62+
sourceInformation(%composer, "C(CannotSkip)N(foo):Test.kt")
63+
val %dirty = %changed
64+
if (%changed and 0b0110 == 0) {
65+
%dirty = %dirty or if (%composer.changedInstance(foo)) 0b0100 else 0b0010
66+
}
67+
if (%composer.shouldExecute(%dirty and 0b0011 != 0b0010, %dirty and 0b0001)) {
68+
if (isTraceInProgress()) {
69+
traceEventStart(<>, %dirty, -1, <>)
70+
}
71+
used(foo)
72+
if (isTraceInProgress()) {
73+
traceEventEnd()
74+
}
75+
} else {
76+
%composer.skipToGroupEnd()
77+
}
78+
%composer.endRestartGroup()?.updateScope { %composer: Composer?, %force: Int ->
79+
CannotSkip(foo, %composer, updateChangedFlags(%changed or 0b0001))
80+
}
81+
}

‎plugins/compose/compiler-hosted/src/main/java/androidx/compose/compiler/plugins/kotlin/ComposeIrGenerationExtension.kt‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -64,10 +64,7 @@ class ComposeIrGenerationExtension(
6464
return
6565
}
6666

67-
val stabilityInferencer = StabilityInferencer(
68-
pluginContext.moduleDescriptor,
69-
stableTypeMatchers,
70-
)
67+
val stabilityInferencer = StabilityInferencer(stableTypeMatchers)
7168

7269
if (useK2) {
7370
moduleFragment.acceptVoid(ComposableLambdaAnnotator(pluginContext))

‎plugins/compose/compiler-hosted/src/main/java/androidx/compose/compiler/plugins/kotlin/analysis/Stability.kt‎

Lines changed: 1 addition & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -22,8 +22,6 @@ import androidx.compose.compiler.plugins.kotlin.ComposeFqNames
2222
import androidx.compose.compiler.plugins.kotlin.lower.annotationClass
2323
import androidx.compose.compiler.plugins.kotlin.lower.isSyntheticComposableFunction
2424
import org.jetbrains.kotlin.backend.jvm.ir.isInlineClassType
25-
import org.jetbrains.kotlin.descriptors.ModuleDescriptor
26-
import org.jetbrains.kotlin.ir.ObsoleteDescriptorBasedAPI
2725
import org.jetbrains.kotlin.ir.declarations.*
2826
import org.jetbrains.kotlin.ir.expressions.*
2927
import org.jetbrains.kotlin.ir.symbols.IrClassifierSymbol
@@ -197,7 +195,6 @@ private data class SymbolForAnalysis(
197195
)
198196

199197
class StabilityInferencer(
200-
private val currentModule: ModuleDescriptor,
201198
externalStableTypeMatchers: Set<FqNameMatcher>,
202199
) {
203200
private val externalTypeMatcherCollection = FqNameMatcherCollection(externalStableTypeMatchers)
@@ -238,25 +235,14 @@ class StabilityInferencer(
238235
mask = externalTypeMatcherCollection
239236
.maskForName(declaration.fqNameWhenAvailable) ?: 0
240237
stability = Stability.Stable
241-
} else if (declaration.isInterface && declaration.isInCurrentModule()) {
242-
// trying to avoid extracting stability bitmask for interfaces in current module
243-
// to support incremental compilation
244-
return Stability.Unknown(declaration)
245238
} else {
246239
val bitmask = declaration.stabilityParamBitmask() ?: return Stability.Unstable
247240

248241
val knownStableMask =
249242
if (typeParameters.size < 32) 0b1 shl typeParameters.size else 0
250-
val isKnownStable = bitmask and knownStableMask != 0
251243
mask = bitmask and knownStableMask.inv()
252244

253-
// supporting incremental compilation, where declaration stubs can be
254-
// in the same module, so we need to use already inferred values
255-
stability = if (isKnownStable && declaration.isInCurrentModule()) {
256-
Stability.Stable
257-
} else {
258-
Stability.Runtime(declaration)
259-
}
245+
stability = Stability.Runtime(declaration)
260246
}
261247
return when {
262248
mask == 0 || typeParameters.isEmpty() -> stability
@@ -305,10 +291,6 @@ class StabilityInferencer(
305291
return stability
306292
}
307293

308-
@OptIn(ObsoleteDescriptorBasedAPI::class)
309-
private fun IrDeclaration.isInCurrentModule() =
310-
module == currentModule
311-
312294
private fun IrClass.isProtobufType(): Boolean {
313295
// Quick exit as all protos are final
314296
if (!isFinalClass) return false

0 commit comments

Comments
 (0)