Add the JSpecify dependency before generating annotations - #1198
Draft
martinfrancois wants to merge 1 commit into
Draft
Add the JSpecify dependency before generating annotations#1198martinfrancois wants to merge 1 commit into
martinfrancois wants to merge 1 commit into
Conversation
addJspecifyDependencyWithoutPriorNullnessAnnotations pins that best practices annotate a project using no nullness library without adding org.jspecify:jspecify. addDependencyForAnnotationsInsertedInSameRun pins that AddDependency's onlyIfUsing cannot match annotations inserted earlier in the same cycle, as disclosed in openrewrite#1192. Both are marked with @ExpectedToFail.
4 tasks
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
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.
Suggested review order: 32 of 52 (Score: 3)
Review first: openrewrite/rewrite-static-analysis#972
What's changed?
Adds 2 known-failing tests to
JSpecifyBestPracticesTestthat reproduce a gap inorg.openrewrite.java.jspecify.JSpecifyBestPractices: on a project with no nullness library at all, the recipe inserts JSpecify annotations but does not add theorg.jspecify:jspecifydependency. No recipe code changes. The tests are marked@ExpectedToFailso the suite stays green; removing the mark shows the failure. On main the class has 6 tests, all passing. With my additions it has 8; the 2 new ones report as skipped, 0 failures.What's your motivation?
Recipe: the JSpecify migration composites that generate
org.jspecify.annotationsreferences.Before
The project has a plain pom with no dependencies and this source:
Actual after the recipe
Using current main, the Java file gains
import org.jspecify.annotations.Nullable;and the method becomespublic @Nullable String getString(), but the pom stays byte identical to its input:Expected after the recipe
The pom MUST gain
org.jspecify:jspecify:1.0.0so the generatedorg.jspecify.annotations.*references resolve. It does not, in any cycle. Readingjspecify.ymlon main shows why: everyAddDependencyguard usesonlyIfUsingonjavax.annotation,jakarta.annotation,org.jetbrains.annotations,org.springframework.langorio.micronaut.core.annotationpatterns, so none can match a project without a prior nullness library, and the annotator steps such asAnnotateNullableMethodscarry noAddDependencyof their own. TestaddJspecifyDependencyWithoutPriorNullnessAnnotationscovers this.The second test,
addDependencyForAnnotationsInsertedInSameRun, shows the constraint that makes the obvious fix awkward. A composite ofAnnotateNullableMethodsfollowed byAddDependencywithonlyIfUsing: org.jspecify.annotations.*does add the dependency, but one cycle late: theonlyIfUsingscan runs against the original sources and does not see annotations inserted earlier in the same cycle. The test harness fails the run for needing an extra cycle; production runs with their extra cycles do converge. #1192 discloses the same limit, which is why its new step guards on the pre-migration annotation instead.I found this while preparing #1192, which adds a related migration to this recipe. The gap persists on that branch too; the tests here target main.
Anything in particular you'd like reviewers to focus on?
I think the first case is a genuine bug: the recipe writes annotations whose artifact is not on the project's classpath, so the output references a type the build cannot resolve. If you agree this should change, I would gladly prepare the fix. If this behavior is intended, feel free to close this and I know it is settled.
Have you considered any alternatives or workarounds?
Guarding on the pre-migration annotation, as #1192 does for
org.springframework.lang, sidesteps the same-cycle blindness, but there is no pre-migration annotation to guard on when the project starts with no nullness library. Accepting the one-cycle-late addition would work in production runs but fails the single-cycle test expectation.Any additional context
Pre-existing tests changed: None.
The same-cycle blindness itself is framework level, in the scanning phase of openrewrite/rewrite, where no issue exists for it yet. openrewrite/rewrite#6822 is adjacent but distinct: it concerns
onlyIfUsingbeing ignored in declarative recipes, not the visibility of same-run insertions. This reproduction was prepared with AI assistance (Claude Code). I reviewed the tests and this description.The added reproduction tests and the existing suite together cover changed and unchanged behavior. The known-failing tests remain disabled until implementation. The formatter run was calibrated per file; untouched lines were not reformatted.
Checklist
./gradlew buildlocally, and committed any resulting changes torecipes.csv