Ignore computed getter-only properties in stability inference#180
Open
skydoves wants to merge 1 commit into
Open
Ignore computed getter-only properties in stability inference#180skydoves wants to merge 1 commit into
skydoves wants to merge 1 commit into
Conversation
A class was wrongly inferred RUNTIME/non-skippable when it (or an abstract supertype) exposed a computed getter-only property such as `open val foo: Iface? get() = null`. A computed property has no backing field and stores no state, so the Compose compiler ignores it (its class inference guards on `member.backingField?.let { ... }`). The analyzer was counting it and letting its interface type demote the class.
Filter class properties to those backed by real stored state before inference. Inherited stored `var`/unstable fields are preserved: they appear as IR fake-overrides and resolve through the override chain to a backed declaration. When the filtered list is empty, a bare-Unknown (abstract/open) superclass is dropped rather than propagated, matching Compose, so a concrete subclass with no state of its own stays stable.
Adds an IR-dump regression test covering an inherited computed property, an own computed property, an inherited var (stays unstable), and a computed-only class extending an abstract base.
Fixes #178
📝 WalkthroughWalkthroughStability inference in StabilityAnalyzerTransformer now filters class and value-class property analysis (including ChangesStored-state stability filtering
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@stability-compiler/src/main/kotlin/com/skydoves/compose/stability/compiler/lower/StabilityAnalyzerTransformer.kt`:
- Around line 795-834: The IDEA analyzer still infers stability from every
declared property, so it can disagree with the compiler on getter-only `val`s
and inherited stored properties. Update `StabilityAnalyzer.kt` and
`k2/KtStabilityInferencer.kt` to mirror `IrProperty.representsStoredState()` /
`resolvesToBackedProperty()` by filtering out properties without a backing field
and following override chains for inherited backed properties. Keep the same
stored-state semantics used by `analyzeValueClass()` so class, `@Parcelize`, and
value-class inference match compiler behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f79f7114-c8fc-4f10-b7e5-03516adbcd88
⛔ Files ignored due to path filters (2)
compiler-tests/src/test/data/dump/ir/InheritedComputedPropertyStability.fir.kt.txtis excluded by!compiler-tests/src/test/data/**/*.txtcompiler-tests/src/test/java/com/skydoves/compose/stability/compiler/tests/IrDumpTestGenerated.javais excluded by!compiler-tests/src/test/java/**
📒 Files selected for processing (2)
compiler-tests/src/test/data/dump/ir/InheritedComputedPropertyStability.ktstability-compiler/src/main/kotlin/com/skydoves/compose/stability/compiler/lower/StabilityAnalyzerTransformer.kt
Comment on lines
+795
to
+834
| /** | ||
| * True when this property stores state — it has a backing field, or (for an inherited fake | ||
| * override) resolves through its override chain to a declaration that does. | ||
| * | ||
| * Computed getter-only properties (e.g. `open val foo: Bar? get() = null`) hold no state and | ||
| * must be excluded from stability inference. This mirrors the Compose compiler, whose class | ||
| * inference guards on `member.backingField?.let { ... }` and never inspects the type of a | ||
| * property without a backing field — so a stateless computed property (even of an interface | ||
| * type) never makes a class runtime/unstable. Inherited stored `var`/unstable fields still | ||
| * count, because they resolve through the override chain to a backed declaration (issue #178). | ||
| */ | ||
| private fun org.jetbrains.kotlin.ir.declarations.IrProperty.representsStoredState(): Boolean { | ||
| if (backingField != null) return true | ||
| return resolvesToBackedProperty(this, HashSet()) | ||
| } | ||
|
|
||
| private fun resolvesToBackedProperty( | ||
| property: org.jetbrains.kotlin.ir.declarations.IrProperty, | ||
| seen: MutableSet<org.jetbrains.kotlin.ir.symbols.IrPropertySymbol>, | ||
| ): Boolean { | ||
| if (property.backingField != null) return true | ||
| if (!seen.add(property.symbol)) return false | ||
| return property.overriddenSymbols.any { symbol -> | ||
| val owner = try { | ||
| symbol.owner | ||
| } catch (e: Exception) { | ||
| null | ||
| } | ||
| owner != null && resolvesToBackedProperty(owner, seen) | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Analyzes value class (inline class) stability. | ||
| * Value classes inherit the stability of their underlying type. | ||
| */ | ||
| private fun analyzeValueClass(clazz: IrClass): ParameterStability { | ||
| val properties = clazz.declarations | ||
| .filterIsInstance<org.jetbrains.kotlin.ir.declarations.IrProperty>() | ||
| .filter { it.representsStoredState() } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 1) Does the IDEA analyzer have an equivalent backing-field / stored-state filter?
fd -i 'StabilityAnalyzer|KtStabilityInferencer|StabilityAnalysisConstants' \
| xargs -r rg -nP -C3 '(backingField|representsStoredState|getter\s*only|computed|overriddenSymbols)'
# 2) Confirm how the compiler side previously handled getter-only props (for parity delta)
fd 'StabilityAnalyzerTransformer.kt' \
| xargs -r rg -nP -C3 'filterIsInstance.*IrProperty|isVar|overriddenSymbols|backingField'Repository: skydoves/compose-stability-analyzer
Length of output: 10012
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate IDEA analyzer sources and inspect for any backing-field / stored-state logic.
git ls-files | rg 'compose-stability-analyzer-idea|stability-analyzer-idea|StabilityAnalyzer|KtStabilityInferencer|StabilityAnalysisConstants|analyzer-idea'
# Show concise outlines for candidate files if present.
for f in $(git ls-files | rg 'compose-stability-analyzer-idea|stability-analyzer-idea|StabilityAnalyzer|KtStabilityInferencer|StabilityAnalysisConstants'); do
echo "### $f"
wc -l "$f"
ast-grep outline "$f" --view expanded || true
doneRepository: skydoves/compose-stability-analyzer
Length of output: 50391
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the IDEA K2 inference paths for property/state handling.
sed -n '520,760p' compose-stability-analyzer-idea/src/main/kotlin/com/skydoves/compose/stability/idea/k2/KtStabilityInferencer.kt
echo '---'
sed -n '250,360p' compose-stability-analyzer-idea/src/main/kotlin/com/skydoves/compose/stability/idea/StabilityAnalyzer.ktRepository: skydoves/compose-stability-analyzer
Length of output: 12630
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "### IDEA-side backingField / stored-state references"
rg -n --hidden -S 'backingField|stored state|getter-only|computed getter|representsStoredState|overriddenSymbols' compose-stability-analyzer-idea/src/main/kotlin
echo
echo "### KtStabilityInferencer property-analysis slice"
sed -n '520,760p' compose-stability-analyzer-idea/src/main/kotlin/com/skydoves/compose/stability/idea/k2/KtStabilityInferencer.kt
echo
echo "### StabilityAnalyzer class-analysis slice"
sed -n '260,330p' compose-stability-analyzer-idea/src/main/kotlin/com/skydoves/compose/stability/idea/StabilityAnalyzer.ktRepository: skydoves/compose-stability-analyzer
Length of output: 226
Mirror the stored-state filter in the IDEA analyzer.
representsStoredState()/resolvesToBackedProperty() changes class, @Parcelize, and value-class inference on the compiler side, but compose-stability-analyzer-idea still analyzes all declared properties in StabilityAnalyzer.kt and k2/KtStabilityInferencer.kt with no backing-field guard. That leaves IDE results diverging on getter-only vals and inherited stored properties.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@stability-compiler/src/main/kotlin/com/skydoves/compose/stability/compiler/lower/StabilityAnalyzerTransformer.kt`
around lines 795 - 834, The IDEA analyzer still infers stability from every
declared property, so it can disagree with the compiler on getter-only `val`s
and inherited stored properties. Update `StabilityAnalyzer.kt` and
`k2/KtStabilityInferencer.kt` to mirror `IrProperty.representsStoredState()` /
`resolvesToBackedProperty()` by filtering out properties without a backing field
and following override chains for inherited backed properties. Keep the same
stored-state semantics used by `analyzeValueClass()` so class, `@Parcelize`, and
value-class inference match compiler behavior.
Source: Path instructions
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🎯 Goal
Fixes #178. A
data classwas reported asRUNTIME/ non-skippable bystabilityDump/stabilityCheckwhen it (or an abstract supertype) exposed a computed getter-only property, e.g.:Compose infers
TestClassas stable; the analyzer reportedtestClass: RUNTIME. Regression range matches the report (0.8.0 clean, 0.9.0+ affected — introduced with the 2.4.0 interface/abstract →Unknownhandling).🛠 Implementation details
The Compose compiler's class-stability inference considers only members that store state — it guards on
member.backingField?.let { ... }and never inspects a property that has no backing field (Stability.kt @ v2.4.0). This analyzer instead counted everyIrPropertyinclazz.declarations, including computed getter-only properties (no backing field) and IR fake-overrides of inherited members. So the computedtestInterface: TestInterface?was analyzed and its interface type demoted the class toRUNTIME.Changes in
StabilityAnalyzerTransformer.kt:@Parcelize, value class), via a newrepresentsStoredState()helper. A computed property with no backing field anywhere in its override chain is dropped.var/unstable field appears as a fake-override with no backing field on the subclass, so the helper resolves throughoverriddenSymbolsto the real declaration and keeps it — e.g.data class Child(val x: String) : Base { var count = 0 }stays UNSTABLE.Unknown(abstract/open) superclass instead of propagating it, matching Compose — otherwise a class whose only properties are computed would inherit the abstract base'sUnknown.Verified with the sample app's
debugStabilityDumpacross a matrix (inherited computed → STABLE, own computed → STABLE, inheritedvar→ UNSTABLE, inheritedMutableList→ UNSTABLE, inherited stableval→ STABLE), and cross-checked against the Compose 2.4.0StabilityInferencersource.✍️ Explain examples
New IR-dump regression test
InheritedComputedPropertyStability.ktpins the injectedtrackParameter(..., isStable = ...)verdicts:holder: DataHolder(inherited computed prop) →isStable = truecard: CardData(own computed prop) →isStable = truecounter: CounterData(inheritedvar) →isStable = falseprofile: ProfileCard(computed-only class extending an abstract base) →isStable = trueNote — IDE plugin follow-up (not in this PR)
The IDE plugin's
KtStabilityInferencerhas a separate, pre-existing divergence in this area: it short-circuits abstract/open classes toUnknownwithout analyzing their fields and folds that into concrete subclasses, so any concrete class extending an unannotated abstract/open base is shown non-skippable in the IDE (vs stable in the compiler / Compose). Fixing that properly needs a broader inferencer change with its own tests, so it is intentionally left out to keep this PR focused on the reportedstabilityCheckregression. Happy to follow up with the IDE-side change.Summary by CodeRabbit