Add JSpecify for Micrometer projects and keep it for Spring projects - #1192
Open
martinfrancois wants to merge 3 commits into
Open
Add JSpecify for Micrometer projects and keep it for Spring projects#1192martinfrancois wants to merge 3 commits into
martinfrancois wants to merge 3 commits into
Conversation
MigrateFromMicrometerAnnotations rewrites io.micrometer.core.lang nullness annotations to org.jspecify.annotations, but guarded its AddDependency step on org.springframework.lang.*ull*. A project that used only Micrometer had its source rewritten and never received org.jspecify:jspecify, so the output did not compile. Point the guard at io.micrometer.core.lang.*ull*, the types the recipe migrates. That guard was also the only path adding JSpecify for Spring projects. MigrateFromSpringFrameworkAnnotations is deliberately disabled in MigrateToJSpecify, yet the static analysis recipes in JSpecifyBestPractices still insert org.jspecify.annotations.* into Spring sources. JSpecifyBestPractices therefore gets its own AddDependency step guarded on org.springframework.lang.*ull*. That guard has to match the pre-migration Spring annotations, because AddDependency does not see annotations other recipes insert during the same run. JSpecifyBestPracticesTest covers the Micrometer dependency, an existing JSpecify dependency, two negative controls, and the Spring-only composite case. The shared parser classpath in that class now includes micrometer-core.
4 tasks
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: 3 of 52 (Score: 9)
Review first: #1204
What's changed?
The
AddDependencystep ofMigrateFromMicrometerAnnotationsnow names the package that this recipe actually migrates:The source rewrite is unchanged:
io.micrometer.core.lang.NonNullandNullablebecomeorg.jspecify.annotations.*on main and on this branch alike. The change only corrects the generated build file.JSpecifyBestPracticesalso gains its ownAddDependencystep, guarded on the Spring package, so a Spring project gets the samepom.xmlfromJSpecifyBestPracticesas it gets on main today. That second half is not optional, for the reason given under alternatives.What's your motivation?
Recipe:
org.openrewrite.java.migrate.jakarta.MigrateFromMicrometerToJSpecify.Before
Actual after the recipe
<!-- annotations migrated, but no JSpecify dependency added -->Expected after the recipe
This recipe rewrites
io.micrometer.core.lang.Nullableandio.micrometer.core.lang.NonNulltoorg.jspecify.annotations.*, but on main itsAddDependencystep is guarded ononlyIfUsing: org.springframework.lang.*ull*. That option adds the dependency only to a project whose source uses one of the types the pattern matches, here theorg.springframework.langtypes whose simple name containsull. This recipe never matches, rewrites or imports a type from that package, so for a Micrometer project the guard never fires. Every other migration recipe injspecify.ymlis guarded on the package that the same recipe migrates.The output therefore does not compile: the migrated source imports
org.jspecify.annotations, which the migrated project's own build file does not provide, so javac reportserror: package org.jspecify.annotations does not exist.The wrong guard also produces the opposite error, a dependency added to a project that does not need one. On main this recipe adds
org.jspecify:jspecifyto a Spring-only project, one using theorg.springframework.langnullness annotations and no Micrometer ones, whose sources it never touches.MigrateToJSpecifydeliberately leavesMigrateFromSpringFrameworkAnnotationsout of its recipe list, so such a project run throughMigrateToJSpecifygets the new dependency and not one changed source file. BothMigrateToJSpecifyandJSpecifyBestPracticesrun this recipe, so both are affected. Reproduced on 3.40.0, 3.41.0 and 3.42.0-SNAPSHOT from main at 26f898e.Affected code in real projects
facebook/fbthriftThriftAbstractTimer.java: Thrift's Java runtime annotates its Micrometer timer withio.micrometer.core.lang.Nullable; the project uses noorg.springframework.langtypes and no JSpecify, so the recipe from main rewrites the annotations toorg.jspecify.annotations.*while its misdirectedAddDependencyguard never fires, and the migrated source references a package the build does not provide.rsocket/rsocket-javaCompositeMetadataUtils.java: thersocket-micrometermodule marks return values withio.micrometer.core.lang.Nullableand the project uses neitherorg.springframework.langnor JSpecify, so the recipe from main migrates the annotation but adds noorg.jspecify:jspecifydependency, and the module no longer compiles.CorfuDB/CorfuDBInfluxNamingConvention.java: CorfuDB's metrics naming conventions annotate parameters withio.micrometer.core.lang.Nullable; the repository contains noorg.springframework.langusage and no JSpecify dependency, so the recipe from main rewrites the annotations without adding the artifact they now need.Anything in particular you'd like reviewers to focus on?
No existing test expectation changed. The only deleted line in the whole change is the
onlyIfUsingline shown above, anddefaults(RecipeSpec)gains"micrometer-core"on the shared parser classpath.The new step in
JSpecifyBestPracticesis guarded on the Spring package rather than on JSpecify itself, becauseAddDependencydoes not see annotations that other recipes insert during the same run.Two limits:
JSpecifyBestPracticescan still writeorg.jspecify.annotations.*into a project that uses no nullness library at all. NoAddDependencyguard injspecify.ymlmatches such a project, so no JSpecify dependency is added and the annotations just written do not resolve. That is a separate defect and this change does not address it.Have you considered any alternatives or workarounds?
Change only the
onlyIfUsingline and add nothing toJSpecifyBestPractices. That is a one-line diff, and a regression for Spring users.JSpecifyBestPracticesrunsAnnotateNullableMethods,AnnotateNullableParameters,AnnotateRequiredParametersandNullableOnMethodReturnTypeafterMigrateToJSpecify, and those four writeorg.jspecify.annotations.*into a project whatever nullness library it uses, Spring projects included. On main the wrongly guardedAddDependencyis the only step inJSpecifyBestPracticesthat adds the JSpecify dependency to a Spring project, so correcting the guard and putting nothing in its place would leave Spring projects with those annotations in their source and without the dependency they need.Leave
micrometer-coreoff the test classpath. It was not declared inbuild.gradle.kts, so the tests resolved it transitively throughrewrite-core, at 1.9.17, whilespring-coreandmicronaut-coreare pinned there withtestRuntimeOnly. The version matters, because theio.micrometer.core.langpackage was removed in Micrometer 1.16, so a future bump of that transitive version would silently break these tests. This change pinstestRuntimeOnly("io.micrometer:micrometer-core:1.15.1")alongside the existing two. 1.15.1 rather than 1.9.17, because it is what the new tests' build file fixtures declare, and 1.15.x is the last minor line that still containsio.micrometer.core.lang.Any additional context
This change adds 6 tests to
JSpecifyBestPracticesTest. Without the code change in this pull request, these 3 new tests fail:migrateFromMicrometerAnnotationsToJspecify: the Micrometer Maven project does not get the JSpecify dependency.micrometerProjectBuiltWithGradleAlsoGetsTheJspecifyDependency: the Micrometer Gradle project does not get the JSpecify dependency.micrometerRecipeDoesNotActivateOnSpringAnnotations: the Micrometer recipe changes a Spring-only project.The other 3 new tests pass without the code change.
The Gradle test needs
spec.beforeRecipe(withToolingApi()), and the sources and the build file have to sit in the same project, wrapped inmavenProject(..). Without that wrapper theonlyIfUsingoption never matches, because the Java sources are not attributed to the Gradle project, andAddDependencythen silently does nothing.The other 3 pass either way and pin down behaviour this change keeps as it is:
migrateFromMicrometerAnnotationsWithExistingJspecifyDependency,micrometerRecipeDoesNotActivateWithoutNullnessAnnotationsandaddJspecifyDependencyOnSpringOnlyProject.This change was prepared with AI assistance (Claude Code). I reviewed the code, the tests and this description.
Checklist
./gradlew buildlocally, and committed any resulting changes torecipes.csv