Improve AnnotateNullableMethods to avoid duplicate annotations and annotation placement issues#845
Conversation
|
Thanks! Can we resolve the conflicts (revert the changes) to AnnotateNullableParameters? |
|
Good catch. Haven't noticed AnnotateNullableParameter slipped through |
3cfd633 to
33c856a
Compare
| * are assumed to be TYPE_USE and will be moved to the return type position. | ||
| */ | ||
| private static final Set<String> NON_TYPE_USE_FQN = new HashSet<>(Arrays.asList( | ||
| "javax.annotation.CheckForNull" |
There was a problem hiding this comment.
It seems a bit odd to have a set of one; do we see this grow in the future? Might there be some overfit for the test cases that uses the same?
There was a problem hiding this comment.
perhaps better use TYPE_USE_NULLABLE_ANNOTATIONS as in AnnotateNullableParameters
private static final Set<String> TYPE_USE_NULLABLE_ANNOTATIONS = new HashSet<>(Arrays.asList(
"jakarta.annotation.Nullable",
"org.checkerframework.checker.nullness.qual.Nullable",
"org.eclipse.jdt.annotation.Nullable",
"org.jspecify.annotations.Nullable"
));
| private static final Set<String> NULLABLE_ANNOTATION_SIMPLE_NAMES = new HashSet<>(Arrays.asList( | ||
| "Nullable", | ||
| "CheckForNull" | ||
| )); |
There was a problem hiding this comment.
Any reason we're not using the same contains("null") as we did for parameters? it's hard to be exhaustive, and we don't know if folks have their own "@NullAllowed" annotation for instance.
There was a problem hiding this comment.
no reason tbh, I've started updating the recipe yesterday alongside the AnnotateNullableParameters and haven't had time to align with all changes in AnnotateNullableParameter
- Replace NULLABLE_ANNOTATION_SIMPLE_NAMES set with case-insensitive
contains("null") check to handle custom annotations (e.g. @NullAllowed)
- Replace NON_TYPE_USE_FQN negative list with TYPE_USE_NULLABLE_ANNOTATIONS
positive list, matching the approach in AnnotateNullableParameters
- Update tests to expect custom/unknown annotations as declaration-target
Problem
Same issues as #843 (
AnnotateNullableParameters), but for method return types:1. Duplicate annotations when a different nullable annotation is already present
Also fails to detect TYPE_USE annotations already on the return type (e.g.
Outer.@Nullable Inner), causing re-annotation on subsequentRepeat.repeatUntilStablecycles:2. Compilation error with non-TYPE_USE annotations on nested/array return types
Solution
Mirrors the approach from PR #843:
Simple name matching — skip methods already carrying any annotation named
Nullable,CheckForNull, regardless of package.Non-TYPE_USE list — non-type use annotations (such as
CheckForNull) stay as method-level annotations. All others default to TYPE_USE behavior.Requested review
@timtebeek
NOTE: Test case
typescriptCode()fails locally due toRPC process shut down early with exit code 1. I've no context on what that test is meant for. Worth checking it.