Change how a user disables the Checker Framework - #5
Conversation
mernst
left a comment
mernst
left a comment
There was a problem hiding this comment.
Please enable CI.
Please enable CodeRabbit reviews. I see, in response to my recently-pushed commits, "CodeRabbit — Review skipped".
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThis PR centralizes Checker Framework version resolution and shifts control to a version-driven workflow. It adds a private getCFVersion(cfExtension, project) resolver, removes the DEFAULT_CF_VERSION constant and the public skipCheckerFramework property, and adds support for overriding via a project property (cfVersion). The plugin now treats special version values ("local", "dependencies", "disable") distinctly when configuring dependencies and gating compilation. Tests were updated to use a new TEST_CF_VERSION constant. README and CI workflow were updated (README reorganized; CI triggers no longer filter branches). 🚥 Pre-merge checks | ❌ 1❌ Failed checks (1 warning)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (3)
🧰 Additional context used🪛 detekt (1.23.8)src/main/kotlin/org/checkerframework/plugin/gradle/CheckerFrameworkPlugin.kt[warning] 83-85: This condition is too complex (4). Defined complexity threshold for conditions is set to '4' (detekt.complexity.ComplexCondition) 🪛 LanguageToolREADME.md[style] ~56-~56: Consider changing the order of words to improve your wording. (TO_NOT_VB) ⏰ 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). (20)
🔇 Additional comments (15)
✏️ Tip: You can disable this entire section by setting 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: 6
🤖 Fix all issues with AI agents
In @README.md:
- Around line 53-57: Change the phrasing for the "disable" special value from
"means to not use the Checker Framework" to "means not to use the Checker
Framework" (update the quoted text for "disable"); also standardize the Checker
Framework version used in examples by replacing any occurrences of "3.52.2" with
"3.53.0" so all examples consistently use "3.53.0" (search for the version
strings "3.52.2" and "3.53.0" and update accordingly).
In
@src/functionalTest/kotlin/org/checkerframework/plugin/gradle/CFPluginFunctionalTest.kt:
- Around line 329-332: Remove the dead commented-out assertion or document its
intent: either delete the two commented lines that reference
DEFAULT_CF_VERSION_FOR_TESTING in CFPluginFunctionalTest.kt, or replace them
with a one-line comment explaining why the test intentionally does not assert
the Checker Framework version for local builds; reference the existing test
variable/result output by name (DEFAULT_CF_VERSION_FOR_TESTING and
result.output) so reviewers understand the rationale if you keep it.
- Around line 53-73: There are two identical tests (`test -PcfVersion=disable `
and `test disable with -PcfVersion=disable`) that duplicate setup and
assertions; remove one of them to avoid redundancy (keep the one whose
name/placement fits your naming convention), ensuring any references to the
removed test are deleted and the remaining test still configures
CheckerFrameworkExtension, writes the empty class via
testProjectDir.writeEmptyClass(), runs buildWithArgs("compileJava",
"-PcfVersion=disable") and asserts TaskOutcome.SUCCESS and absence of the
version note.
- Line 325: The Gradle property argument is malformed: change the buildWithArgs
call that currently passes "compileJava", "-cfVersion=local" so the project
property is passed with the -P prefix (i.e., "-PcfVersion=local"); update the
invocation where the variable result is assigned (testProjectDir.buildWithArgs)
to use the corrected argument string.
- Around line 270-275: The configure<CheckerFrameworkExtension> block sets
version twice (version = "$TEST_CF_VERSION" then version = "local"), so remove
the redundant assignment — keep only the intended final value (version =
"local") and delete the earlier version = "$TEST_CF_VERSION" line to avoid
confusion and dead code in the configure<CheckerFrameworkExtension> block that
also defines checkers and extraJavacArgs.
In
@src/main/kotlin/org/checkerframework/plugin/gradle/CheckerFrameworkPlugin.kt:
- Around line 185-194: The method getCFVersion currently does an unsafe cast of
project.properties.get("cfVersion") as String which can throw
ClassCastException; replace this with a safe retrieval such as val cfProp =
project.properties["cfVersion"]; then use when or safe cast (val s = cfProp as?
String ?: cfProp?.toString()) to obtain a String, and if cfProp is null or empty
throw a clear IllegalStateException; update the return to use that String
instead of the unsafe cast in getCFVersion, keeping cfExtension and project
checks intact.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/gradle.ymlREADME.mdbuild.gradle.ktssrc/functionalTest/kotlin/org/checkerframework/plugin/gradle/CFPluginFunctionalTest.ktsrc/main/kotlin/org/checkerframework/plugin/gradle/CheckerFrameworkExtension.ktsrc/main/kotlin/org/checkerframework/plugin/gradle/CheckerFrameworkPlugin.kt
💤 Files with no reviewable changes (2)
- .github/workflows/gradle.yml
- src/main/kotlin/org/checkerframework/plugin/gradle/CheckerFrameworkExtension.kt
🧰 Additional context used
🪛 detekt (1.23.8)
src/main/kotlin/org/checkerframework/plugin/gradle/CheckerFrameworkPlugin.kt
[warning] 83-85: This condition is too complex (4). Defined complexity threshold for conditions is set to '4'
(detekt.complexity.ComplexCondition)
🪛 LanguageTool
README.md
[style] ~56-~56: Consider changing the order of words to improve your wording.
Context: ... The special value "disable" means to not use the Checker Framework. The command...
(TO_NOT_VB)
⏰ 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: test-gradle (8.10.2)
- GitHub Check: test-gradle (8.8)
- GitHub Check: test-jdk (17)
- GitHub Check: build
- GitHub Check: test-jdk (25)
- GitHub Check: build
- GitHub Check: test-jdk (21)
🔇 Additional comments (8)
src/main/kotlin/org/checkerframework/plugin/gradle/CheckerFrameworkPlugin.kt (2)
83-86: Condition logic looks correct.The enablement check correctly prioritizes version-based disabling, then per-task disabling, then test exclusion. The static analysis warning about complexity (threshold 4) appears to be a false positive since this has 3 logical conditions and each is clearly separated.
162-181: Version handling branches are well-structured.The dependency loading logic correctly handles:
"local": Uses$CHECKERFRAMEWORKenvironment variable"dependencies"/"disable": Skips adding default dependencies- Otherwise: Fetches from Maven Central
This unifies the previous separate flags into a single version-based approach.
build.gradle.kts (1)
59-64: Dependency cleanup aligns with version constant relocation.Removing the
implementation(project())dependency is correct sinceDEFAULT_CF_VERSIONwas removed from the plugin. TheassociateWithon line 62 still provides access to internal plugin symbols if needed for testing.src/functionalTest/kotlin/org/checkerframework/plugin/gradle/CFPluginFunctionalTest.kt (2)
9-9: Good practice to define test version as a constant.Centralizing the test version makes it easy to update across all tests.
363-385: Good addition: Test for missing version error.This test validates the new requirement that users must explicitly set a Checker Framework version, ensuring a clear error message is shown.
README.md (3)
24-82: Well-structured version configuration documentation.The new section clearly explains:
- Recommended approach using version catalogs
- Simple inline version specification
- Special values ("local", "disable", "dependencies")
- CLI override with
-PcfVersion=...This effectively documents the new version-driven configuration approach.
115-129: Helpful addition: Checker dependencies guidance.This section clearly explains how to add dependencies for custom checkers or qualifiers, which is important for users of the Subtyping Checker.
166-186: Good documentation for per-task disabling.The example clearly shows how to disable the Checker Framework for specific tasks like generated code compilation.
|
I can't figure out how to enable coderabbit on this pull request. It is working on the other one, but I have to manually request it on this one. |
OK, no problem as long as we get its feedback. |
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
|
@CodeRabbit resume |
✅ Actions performedReviews resumed. |
2 participants
This proposes changes to how the version number is handled.
Currently, the plugin is released every time the Checker Framework is released, just to change
the default version of the Checker Framework that the plugin uses. That is convenient if you use
the plugin without specifying a version number, but such a practice is discouraged.
It is more explicit to have users specify a Checker Framework version number rather than a plugin
number that implies some (unknown) version number. Updating either one is just a version number,
which dependabot or renovate can handle.
If the Checker Framework is skipped, then the version number doesn't matter. Therefore, we can
use the same mechanism for both, reducing the size of the user-visible API. The version number
can be "3.52.1" or "local" or "none". This permits unifying -PcfLocal and -PskipCheckerFramework.
@smillst Let me know what you think, then please take over this PR and change the code as needed. Thanks!