Prevent recursive detection for the complicated type structures #12
Conversation
WalkthroughAdded cycle detection via ThreadLocal tracking sets and StackOverflowError handling to prevent infinite recursion during type stability analysis. The analysis logic is refactored into internal helper methods with proper cleanup in two compiler components to safely process recursive and self-referential types. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant KtStabilityOf
participant AnalyzingTypes as analyzingTypes<br/>(ThreadLocal)
participant KtStabilityOfInternal
participant TypeRenderer
Caller->>KtStabilityOf: ktStabilityOf(type)
rect rgb(220, 240, 255)
Note over KtStabilityOf: Cycle Detection Guard
KtStabilityOf->>AnalyzingTypes: contains(type)?
alt Already Analyzing
AnalyzingTypes-->>KtStabilityOf: true
KtStabilityOf-->>Caller: return RUNTIME<br/>(circular ref)
else First Time
AnalyzingTypes-->>KtStabilityOf: false
end
end
rect rgb(240, 255, 240)
Note over KtStabilityOf: Safe Rendering & Analysis
KtStabilityOf->>AnalyzingTypes: add(type)
KtStabilityOf->>KtStabilityOfInternal: call()
KtStabilityOfInternal->>TypeRenderer: render(type)
alt Render Success
TypeRenderer-->>KtStabilityOfInternal: rendered type string
KtStabilityOfInternal->>KtStabilityOfInternal: analyze stability
KtStabilityOfInternal-->>KtStabilityOf: result
else StackOverflowError
TypeRenderer-->>KtStabilityOfInternal: StackOverflowError
KtStabilityOfInternal->>KtStabilityOfInternal: catch & return RUNTIME
KtStabilityOfInternal-->>KtStabilityOf: RUNTIME (Unknown)
end
end
rect rgb(255, 245, 238)
Note over KtStabilityOf: Cleanup
KtStabilityOf->>AnalyzingTypes: remove(type)
KtStabilityOf-->>Caller: return result
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
compose-stability-analyzer-idea/src/main/kotlin/com/skydoves/compose/stability/idea/k2/KtStabilityInferencer.kt (1)
193-198: Align circular reference handling between cycle detection mechanisms.The inconsistency is confirmed. Two cycle detection mechanisms return different stability types for the same scenario:
- Lines 80-84 (ThreadLocal check): Returns
KtStability.Runtime- Lines 193-198 (Set parameter check): Returns
KtStability.Certain(stable = true)Since circular references mean analysis is incomplete,
Runtimeis the semantically correct choice for both cases (you cannot be certain about stability when a cycle is detected). Update line 193-198 to match the ThreadLocal behavior:if (declaration in currentlyAnalyzing) { - return KtStability.Certain( - stable = true, + return KtStability.Runtime( + className = declaration.name?.asString() ?: "Unknown", reason = StabilityConstants.Messages.CIRCULAR_REFERENCE, ) }
🧹 Nitpick comments (1)
stability-compiler/src/main/kotlin/com/skydoves/compose/stability/compiler/lower/StabilityAnalyzerTransformer.kt (1)
254-264: Consider protecting the fallbacktype.render()call.While the StackOverflowError handling for
classFqNameis good (lines 254-258), thetype.render()fallback on line 260 could also potentially throw StackOverflowError for deeply recursive types.Consider wrapping the typeId initialization:
- val typeId = fqName ?: classSymbol?.owner?.name?.asString() ?: type.render() + val typeId = fqName ?: classSymbol?.owner?.name?.asString() ?: try { + type.render() + } catch (e: StackOverflowError) { + "UnknownType" + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
compose-stability-analyzer-idea/CHANGELOG.md(1 hunks)compose-stability-analyzer-idea/src/main/kotlin/com/skydoves/compose/stability/idea/k2/KtStabilityInferencer.kt(1 hunks)stability-compiler/src/main/kotlin/com/skydoves/compose/stability/compiler/lower/StabilityAnalyzerTransformer.kt(2 hunks)
🧰 Additional context used
🪛 detekt (1.23.8)
compose-stability-analyzer-idea/src/main/kotlin/com/skydoves/compose/stability/idea/k2/KtStabilityInferencer.kt
[warning] 72-72: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
stability-compiler/src/main/kotlin/com/skydoves/compose/stability/compiler/lower/StabilityAnalyzerTransformer.kt
[warning] 256-256: 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). (7)
- GitHub Check: Build and Tests
- GitHub Check: Spotless check
- GitHub Check: API check
- GitHub Check: Build IntelliJ Plugin
- GitHub Check: Runtime Module Tests
- GitHub Check: Test IntelliJ Plugin
- GitHub Check: Compiler Tests (FIR/IR Dumps)
🔇 Additional comments (7)
compose-stability-analyzer-idea/CHANGELOG.md (1)
9-16: LGTM! Clear documentation of the fixes.The changelog entries accurately describe the cycle detection and StackOverflowError handling improvements introduced in this PR.
compose-stability-analyzer-idea/src/main/kotlin/com/skydoves/compose/stability/idea/k2/KtStabilityInferencer.kt (3)
61-62: LGTM! Appropriate use of ThreadLocal for cycle detection.The ThreadLocal tracking set correctly prevents infinite recursion across the call stack. The mutable set is properly managed with add/remove in try/finally blocks to ensure cleanup.
70-93: Excellent cycle detection and error handling.The implementation correctly prevents infinite recursion by:
- Safely handling StackOverflowError during type rendering with an appropriate fallback
- Detecting circular references via ThreadLocal tracking
- Ensuring cleanup in the finally block
Note: The static analysis warning about swallowing StackOverflowError is a false positive in this context—returning RUNTIME stability is the correct defensive behavior for types that cannot be safely rendered.
95-178: LGTM! Clean refactoring with proper separation of concerns.The extraction of the analysis logic into
ktStabilityOfInternalenables proper resource cleanup while maintaining the original analysis flow.stability-compiler/src/main/kotlin/com/skydoves/compose/stability/compiler/lower/StabilityAnalyzerTransformer.kt (3)
58-59: LGTM! Consistent ThreadLocal usage.The ThreadLocal tracking set aligns with the implementation in the IDEA plugin and correctly enables cycle detection.
266-272: LGTM! Proper cycle detection with cleanup.The try/finally pattern ensures the tracking set is cleaned up even if exceptions occur during analysis.
274-368: LGTM! Clean internal implementation.The extracted internal method maintains the original analysis logic while enabling proper resource management in the outer method.
Prevent recursive detection for the complicated type structures. (#11)
Summary by CodeRabbit