Place @Nullable correctly on qualified nested return types - #996
Draft
martinfrancois wants to merge 1 commit into
Draft
Place @Nullable correctly on qualified nested return types#996martinfrancois wants to merge 1 commit into
@Nullable correctly on qualified nested return types#996martinfrancois wants to merge 1 commit into
Conversation
…urn type moveNullableToNestedTypeSimpleName pins the correct placement of a TYPE_USE annotation on a qualified nested return type: Map.@nullable Entry<K, V>. The recipe currently emits public @nullable Map.Entry<K, V>, which javac rejects for a TYPE_USE-only annotation such as org.jspecify.annotations. Nullable. The limit was disclosed in openrewrite#968. Marked @ExpectedToFail until fixed.
4 tasks
@Nullable correctly on qualified nested return types
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: 30 of 52 (Score: 3.5)
Review first: #970
What's changed?
Adds 1 known-failing test to
NullableOnMethodReturnTypeTestthat reproduces a wrong annotation position: when the return type is a qualified nested type, the recipe places a TYPE_USE@Nullablebefore the whole qualified name instead of before the innermost simple name. No recipe code changes. The test is marked@ExpectedToFailso the suite stays green; removing the mark shows the failure. With the mark in place, the class runs 17 tests, 1 skipped, and all 16 pre-existing tests still pass.What's your motivation?
Recipe: the nullable method-return annotation recipe.
NullableOnMethodReturnTypemoves@Nullablefrom its own line onto the return type. For a qualified nested type it moves the annotation to the wrong spot.Before
Actual after the recipe
Using current main.
This output does not compile.
org.jspecify.annotations.Nullableis TYPE_USE only, and javac rejects the produced form with:Expected after the recipe
The annotation MUST be attached to the innermost simple name. This output compiles and follows JLS 9.7.4 and javac's diagnostic:
This assumes the annotation is TYPE_USE targeted; since #968 merged, that is the only case the recipe acts on. Before the recipe runs, the code compiles. After the recipe runs, the code should still compile, with the annotation attached to the innermost simple name. The mechanism: for non-array return types the recipe wraps the whole type in a
J.AnnotatedType, so the annotation lands beforeMap. The correct placement logic already exists inMoveFieldAnnotationToType(see #379 for the fully qualified handling), whichAnnotateNullableMethodschains after this recipe; the defect only shows whenNullableOnMethodReturnTyperuns on its own.Found while preparing #968, which fixed related defects in this recipe and disclosed this leftover in its description.
Anything in particular you'd like reviewers to focus on?
I think this is a genuine bug, because the recipe turns compiling code into non-compiling code. Note the existing
nestedTypetest (from #579) is not in conflict: it usesorg.openrewrite.internal.lang.Nullable, a declaration annotation, where placement before the qualified name is legal. A fix for this case should still check whether that test's expectationpublic @Nullable B.C bar()needs to change with it. 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?
Running the full
AnnotateNullableMethodschain instead of this recipe alone avoids the broken output, becauseMoveFieldAnnotationToTyperepairs the position afterwards. That only helps users of the composite recipe, not of this recipe by itself.Any additional context
Pre-existing tests changed: None.
The limit was disclosed in the description of my merged #968. 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