Validate a subclass of a sealed type class #37
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds two new sealed model types, expands the Icon composable signature to accept them, bumps the compose-stability-analyzer version to 0.5.0, and updates analyzer and compiler stability inference to propagate Changes
Sequence DiagramsequenceDiagram
participant Call as Analyzer Entry
participant Class as Class Symbol Analysis
participant Parent as Parent/Sealed Check
participant Props as Property Analysis
rect rgb(245,245,255)
Note over Call: Previous behavior (pre-change)
Call->>Class: analyze abstract class
Class->>Props: short-circuit -> RUNTIME (abstract)
Props-->>Call: return RUNTIME
end
rect rgb(240,255,240)
Note over Call: New behavior (post-change)
Call->>Class: analyze abstract/sealed class
Class->>Parent: is sealed OR annotated with `@Stable/`@Immutable?
alt annotated or sealed+annotated
Parent->>Props: proceed to property analysis (may yield STABLE)
Props-->>Call: return based on properties
else not annotated & abstract non-sealed
Parent-->>Call: return RUNTIME
end
end
rect rgb(255,245,250)
Note over Call: Sealed parent propagation
Call->>Class: analyze subclass
Class->>Parent: parent sealed AND parent annotated?
alt yes
Parent-->>Call: return STABLE (inherit)
else no
Parent->>Props: continue normal analysis
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
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: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/src/main/kotlin/com/skydoves/myapplication/MainActivity.kt(2 hunks)app/src/main/kotlin/com/skydoves/myapplication/models/Models.kt(1 hunks)compose-stability-analyzer-idea/CHANGELOG.md(1 hunks)compose-stability-analyzer-idea/src/main/kotlin/com/skydoves/compose/stability/idea/k2/KtStabilityInferencer.kt(2 hunks)gradle/libs.versions.toml(1 hunks)stability-compiler/src/main/kotlin/com/skydoves/compose/stability/compiler/lower/StabilityAnalyzerTransformer.kt(2 hunks)
🧰 Additional context used
🪛 detekt (1.23.8)
stability-compiler/src/main/kotlin/com/skydoves/compose/stability/compiler/lower/StabilityAnalyzerTransformer.kt
[warning] 404-404: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
[warning] 450-450: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Lint Module Tests
- GitHub Check: Gradle Plugin Tests
- GitHub Check: Runtime Module Tests
- GitHub Check: Compiler Plugin Tests
- GitHub Check: Compiler Tests (FIR/IR Dumps)
- GitHub Check: Build and Tests
- GitHub Check: Build IntelliJ Plugin
- GitHub Check: Spotless check
- GitHub Check: Test IntelliJ Plugin
- GitHub Check: API check
🔇 Additional comments (1)
compose-stability-analyzer-idea/CHANGELOG.md (1)
14-19: Well-documented fix with clear examples. ✓The changelog entry clearly describes the sealed class stability inheritance fix with a concrete before/after example (RUNTIME → STABLE). The formatting is consistent with other Fixed entries, the issue reference is proper, and noting the plugin alignment is helpful context for users.
| val parentHasStabilityAnnotation = classSymbol.superTypes.any { superType -> | ||
| val superClassSymbol = superType.expandedSymbol as? KaClassSymbol | ||
| if (superClassSymbol != null) { | ||
| // Check if superclass is sealed (has sealed subclasses) | ||
| val isSealed = superClassSymbol.modality == KaSymbolModality.SEALED | ||
| // Check if it has @Stable or @Immutable annotation | ||
| val hasAnnotation = superClassSymbol.annotations.any { annotation -> | ||
| val annotationFqName = annotation.classId?.asSingleFqName()?.asString() | ||
| annotationFqName == "androidx.compose.runtime.Stable" || | ||
| annotationFqName == "androidx.compose.runtime.Immutable" | ||
| } | ||
| isSealed && hasAnnotation | ||
| } else { | ||
| false | ||
| } | ||
| } | ||
|
|
||
| if (parentHasStabilityAnnotation) { | ||
| return KtStability.Certain( | ||
| stable = true, | ||
| reason = "Subclass of @Immutable/@Stable sealed class", | ||
| ) | ||
| } |
There was a problem hiding this comment.
Propagate all stable markers, not just @Stable/@Immutable.
Line 435 only matches Compose’s two annotations. Because hasStableAnnotation() already recognizes @StableForAnalysis (and other stable markers), sealed subclasses of those annotations still get downgraded to runtime stability. That breaks the promise we make for custom stable markers. Please reuse superClassSymbol.hasStableAnnotation() (or the same constant set) so every annotation we treat as stable elsewhere also propagates through sealed parents. This keeps IDE inference consistent with the rest of the analyzer.
🤖 Prompt for AI Agents
In
compose-stability-analyzer-idea/src/main/kotlin/com/skydoves/compose/stability/idea/k2/KtStabilityInferencer.kt
around lines 429 to 451, the code only checks for the two Compose annotations
("androidx.compose.runtime.Stable" and "androidx.compose.runtime.Immutable") on
sealed superclasses, but should propagate all stable markers; replace the manual
annotation check with the existing helper (e.g., call
superClassSymbol.hasStableAnnotation() or reuse the same stable-annotation set)
so the condition becomes: detect sealed superclasses AND use
hasStableAnnotation() to determine stability, then return the same
KtStability.Certain when true.
| val parentHasStabilityAnnotation = clazz.superTypes.any { superType -> | ||
| val superClassSymbol = superType.classOrNull | ||
| if (superClassSymbol != null) { | ||
| val superClass = superClassSymbol.owner | ||
| // Check if superclass is sealed AND has stability annotation | ||
| val isSealed = try { | ||
| superClass.sealedSubclasses.isNotEmpty() | ||
| } catch (e: Exception) { | ||
| false | ||
| } | ||
| val hasAnnotation = superClass.hasAnnotation(stableFqName) || | ||
| superClass.hasAnnotation(immutableFqName) | ||
| isSealed && hasAnnotation | ||
| } else { | ||
| false | ||
| } | ||
| } | ||
|
|
||
| if (parentHasStabilityAnnotation) { | ||
| return ParameterStability.STABLE | ||
| } |
There was a problem hiding this comment.
Mirror the IDE fix for all stable annotations.
Here we only check stableFqName/immutableFqName. That leaves sealed parents annotated with @StableForAnalysis (or other supported markers) still reporting runtime in the compiler plugin even though the IDE parks them as stable. Please expand the guard to include every annotation we recognise as stable (e.g. reuse the same helper/constant set) so IDE and compiler stay in lockstep.
🧰 Tools
🪛 detekt (1.23.8)
[warning] 450-450: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
Validate a subclass of a sealed type class. (#31)
Summary by CodeRabbit