Stability configuration files in compiler plugin#181
Open
mlewandowskiphysitrack wants to merge 12 commits into
Open
Stability configuration files in compiler plugin#181mlewandowskiphysitrack wants to merge 12 commits into
mlewandowskiphysitrack wants to merge 12 commits into
Conversation
To be used in StabilityAnalyzerIrGenerationExtension
… to compiler plugin and pass it down to IrGenerationExtension
…rs in analyzer to mark classes specified in stability configuration files as stable
… pass the value to compiler plugin
…nfig sub-extension; stabilityConfigurationFiles in StabilityAnalyzerExtension should be used instead, as it wires directly into compiler plugin.
…tion block, point to and describe stabilityConfigurationFiles in composeStabilityAnalyzer block (that applies directly to compiler plugin)
… into account Using IR test with ENABLE_TRACE_ALL for this as it's the only way to get a verdict on stability in existing tests setup.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds stability configuration file support through parser, compiler/Gradle wiring, transformer classification, and validation fixtures/docs. ChangesStability configuration file feature
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Comment on lines
-472
to
+474
| * 4. Known stable types | ||
| * 4. Known stable types and stability configuration files types |
There was a problem hiding this comment.
Extending the existing point since there's a mismatch in the numbering; and to not spoil the diff with numbering changes.
| valueDescription = "<path>", | ||
| description = "Stability configuration file to take into account when analyzing stability", | ||
| required = false, | ||
| allowMultipleOccurrences = true, |
There was a problem hiding this comment.
This is needed to pass multiple files
| """.trimIndent() | ||
| } | ||
|
|
||
| public class FqNameMatcher(private val pattern: String) { |
There was a problem hiding this comment.
The class needs to be public since it's used in public constructor of public StabilityAnalyzerIrGenerationExtension
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 `@compiler-tests/src/test/data/dump/ir/StabilityConfigurationFile.kt`:
- Around line 1-17: Add directive-driven coverage around
StabilityConfigurationFile to exercise the skip/exclusion matrix for the new
stability config behavior. Keep the existing positive case in
ComposableToBeSkippable, and add a companion test using the same IR dump pattern
(or a nearby test in the same suite) that shows a type not listed in
stability_config.conf remains unstable, plus a baseline case without
STABILITY_CONFIGURATION_FILES to confirm default behavior is unchanged.
Reference the existing markers in StabilityConfigurationFile and the
ComposableToBeSkippable/Wrapper/ToBeStable setup so the new test clearly
distinguishes configured vs non-configured stability.
- Line 3: The current StabilityConfigurationFile fixture only exercises the
configured stable path, so add a companion negative test that either removes
STABILITY_CONFIGURATION_FILES or points to a config that does not include
ToBeStable. Update the relevant IR dump test setup alongside
StabilityConfigurationFile so the skip/unstable branch is covered and the
behavior is verified with a missing or mismatched stability config.
In
`@stability-compiler/src/main/kotlin/com/skydoves/compose/stability/compiler/StabilityAnalyzerIrGenerationExtension.kt`:
- Around line 60-64: Wrap the stability config parsing in
StabilityAnalyzerIrGenerationExtension.generate the same way as the dependency
scan so a bad config file does not abort compilation. Update the
stabilityConfigurationFiles processing around
StabilityConfigParser.fromFile(...) to catch parsing/read errors per file, skip
only the failing file, and continue building stabilityConfigurationMatchers from
the remaining valid configs.
In
`@stability-compiler/src/main/kotlin/com/skydoves/compose/stability/compiler/StabilityConfigParser.kt`:
- Around line 50-99: StabilityConfigParser.fromFile() still lets read/parse
exceptions escape from StabilityConfigParserImpl and abort compilation; make it
fail open by catching file read and parser construction errors in fromFile and
returning StabilityConfigParserImpl(emptyList()) instead. Keep the existing
parsing logic in StabilityConfigParserImpl, but ensure any error while reading
the file or parsing lines falls back to no stableTypeMatchers so
StabilityAnalyzerIrGenerationExtension.generate() can skip instrumentation
instead of failing.
- Around line 172-202: The generic mask is parsed in StabilityConfigParser but
never applied during matching, so patterns like foo.Bar<*,_> behave the same as
foo.Bar. Update FqNameMatcher to use the parsed mask when evaluating matches,
and ensure the logic in matches(name) validates generic arguments against mask
instead of only comparing the raw key/regex. Apply the same fix in the mirrored
stability-gradle parser copy so both implementations honor generic-mask syntax
consistently.
In
`@stability-gradle/src/main/kotlin/com/skydoves/compose/stability/gradle/StabilityAnalyzerExtension.kt`:
- Around line 282-288: The deprecation message in
StabilityAnalyzerExtension.stabilityConfigurationFiles is missing a space
because the two string literals are concatenated directly. Update the
`@Deprecated` message to read naturally by adding the missing space between
“option” and “in top-level StabilityAnalyzerExtension” so the generated
deprecation text is correct.
🪄 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: 2319d9a7-efa0-4731-b1fe-af20e9028872
⛔ Files ignored due to path filters (4)
compiler-tests/src/test/data/dump/ir/StabilityConfigurationFile.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/**stability-compiler/api/stability-compiler.apiis excluded by!**/api/*.apistability-gradle/api/stability-gradle.apiis excluded by!**/api/*.api
📒 Files selected for processing (14)
compiler-tests/src/test/data/dump/ir/StabilityConfigurationFile.ktcompiler-tests/src/test/data/dump/ir/stability_config.confcompiler-tests/src/test/kotlin/com/skydoves/compose/stability/compiler/tests/StabilityTestConfigurator.ktcompiler-tests/src/test/kotlin/com/skydoves/compose/stability/compiler/tests/StabilityTestDirectives.ktdocs/gradle-plugin/stability-configuration-files.mddocs/gradle-plugin/stability-validation.mdmkdocs.ymlstability-compiler/src/main/kotlin/com/skydoves/compose/stability/compiler/StabilityAnalyzerCommandLineProcessor.ktstability-compiler/src/main/kotlin/com/skydoves/compose/stability/compiler/StabilityAnalyzerIrGenerationExtension.ktstability-compiler/src/main/kotlin/com/skydoves/compose/stability/compiler/StabilityAnalyzerPluginRegistrar.ktstability-compiler/src/main/kotlin/com/skydoves/compose/stability/compiler/StabilityConfigParser.ktstability-compiler/src/main/kotlin/com/skydoves/compose/stability/compiler/lower/StabilityAnalyzerTransformer.ktstability-gradle/src/main/kotlin/com/skydoves/compose/stability/gradle/StabilityAnalyzerExtension.ktstability-gradle/src/main/kotlin/com/skydoves/compose/stability/gradle/StabilityAnalyzerGradlePlugin.kt
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
The main goal is to fix #176. To achieve this, stability configuration files are passed directly to compiler plugin and their contents are taken into account there when checking stability of classes used as parameters in Composables.
🛠 Implementation details
To achieve this, new option for passing stability configuration files is added to compiler plugin. Compiler plugin parses stability configuration files (logic copied from existing, buggy stability configuration files handling in Gradle plugin) and
StabilityAnalyzerTransformerreturnsParameterStability.STABLEif class fully-qualified name matches entry in stability configuration file. There is a parity between this and checking user settings (ignored types, custom stable types) inKtStabilityInferencer.In Gradle plugin, there is added new option
stabilityConfigurationFileson top-level extension. This is done for backwards compatibility, but also to indicate that the passed files are not wired into stability validation functionality in Gradle plugin, but go directly to the compiler plugin (similarly to e.g.traceAlloption) and are used by all features that use compiler plugin work (e.g. mentioned tracing). The old option instabilityValidationblock is deprecated and a subject to removal in some future release. It is not removed to not suddenly break compilation when someone bumps the plugin from 0.10.0 to 0.11.0. The changes are reflected in the docs.The change is tested by IR compiler test - see commit message and comment in the test that explain why this was the only reasonable way to test if the feature behaves correctly.
✍️ Explain examples
As explained in #176, when attaching
stability_config.confviastabilityValidationblock option, when runningstabilityChecktheComposableToBeSkippablewould not be marked as skippable, andWrapperwould be marked asUNSTABLE. When attaching via new option on top-level extension, the correct value of skippable/stable will be returned.Migration path
Before:
After:
Summary by CodeRabbit
New Features
*and**patterns).Documentation
Bug Fixes
Tests