Skip to content

JAVASE-241 FPs when Jakarta / Javax NotNull annotation is used #5990

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
asya-vorobeva wants to merge 5 commits into from
Closed

JAVASE-241 FPs when Jakarta / Javax NotNull annotation is used #5990

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
aaae03c
SONARJAVA-6770: Integrate TestFileClassifier from analyzer-commons
asya-vorobeva Aug 18, 2026
9d1fb5a
Remove Test* filename prefix pattern from JavaFileTypeClassifier
asya-vorobeva Aug 19, 2026
0fb4946
Test sonar.tests config: naming/path heuristic disabled, other signal…
asya-vorobeva Aug 19, 2026
f94524e
Add uppercase IT/ITS Maven integration-test path patterns
asya-vorobeva Aug 19, 2026
43a6fa9
Treat Bean Validation @NotNull as WEAK_NULLABLE to fix FPs (JAVASE-241)
asya-vorobeva Aug 21, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions ...es/default/src/main/java/annotations/nullability/no_default/NullabilityAtMethodLevel.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,19 @@ public Object id2019_type_NO_ANNOTATION_level_PACKAGE(
return new Object();
}

// ============== Bean Validation @NotNull is treated as WEAK_NULLABLE, not NON_NULL ==============
@javax.validation.constraints.NotNull
public Object id2025_type_WEAK_NULLABLE_level_METHOD(
@javax.validation.constraints.NotNull Object id2026_type_WEAK_NULLABLE_level_VARIABLE) {
return new Object();
}

@jakarta.validation.constraints.NotNull
public Object id2027_type_WEAK_NULLABLE_level_METHOD(
@jakarta.validation.constraints.NotNull Object id2028_type_WEAK_NULLABLE_level_VARIABLE) {
return new Object();
}

}

abstract class NullabilityAtMethodLevelParent {
Expand Down
4 changes: 3 additions & 1 deletion .../default/src/main/java/annotations/nullability/no_default/NullabilityAtVariableLevel.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,9 @@ public class NullabilityAtVariableLevel {
@javax.annotation.Nonnull
Object id1032_type_NON_NULL_level_VARIABLE;
@javax.validation.constraints.NotNull
Object id1033_type_NON_NULL_level_VARIABLE;
Object id1033_type_WEAK_NULLABLE_level_VARIABLE;
@jakarta.validation.constraints.NotNull
Object id1090_type_WEAK_NULLABLE_level_VARIABLE;
@lombok.NonNull
Object id1034_type_NON_NULL_level_VARIABLE;
@org.checkerframework.checker.nullness.compatqual.NonNullDecl
Expand Down
5 changes: 1 addition & 4 deletions ...s-test-sources/default/src/main/java/checks/EqualsParametersMarkedNonNullCheckSample.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,11 +43,8 @@ public boolean equals(@Nonnull C c) { // Compliant

static class F {
public boolean equals(
@javax.validation.constraints.NotNull // Noncompliant {{"equals" method parameters should not be marked "@NotNull".}} [[quickfixes=qf2]]
// ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
@javax.validation.constraints.NotNull // Compliant: exceptional annotation
java.lang.Object object) {
// fix@qf2 {{Remove "@NotNull"}}
// edit@qf2 [[sc=7;ec=7;el=+2]] {{}}
return false;
}
}
Expand Down
7 changes: 7 additions & 0 deletions ...checks-test-sources/default/src/main/java/checks/PrimitivesMarkedNullableCheckSample.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package checks;

import jakarta.validation.constraints.NotNull;
import javax.annotation.CheckForNull;
import javax.annotation.Nonnull;
import javax.annotation.Nullable;
Expand Down Expand Up @@ -51,6 +52,12 @@ abstract class PrimitivesMarkedNullableCheckSample {
@Nonnull
public double getDouble2_2() { return 0.0; } // Compliant, Nonnull is useless, but is accepted as it can be added for consistency

@javax.validation.constraints.NotNull
public int getIntWithJavaxNotNull() { return 0; } // Compliant, Bean Validation @NotNull is a runtime constraint, not a nullable annotation

@NotNull
public int getIntWithJakartaNotNull() { return 0; } // Compliant, Bean Validation @NotNull is a runtime constraint, not a nullable annotation

@javax.annotation.Nullable
public Double getDouble3() { return 0.0; }

Expand Down
46 changes: 37 additions & 9 deletions .../java/checks/S2638_ChangeMethodContractCheck/noPackageInfo/ChangeMethodContractCheck.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,35 +187,63 @@ void argAnnotatedDirectlyNullable(@MyNonnullMetaAnnotation Object a) { } // Nonc
}

/**
* Not null with arguments is inconsistently supported. See SONARJAVA-3803.
* Javax and Jakarta validation NotNull annotations are treated as weakly nullable.
*/
class ChangeMethodContractCheck_NonnullWithArguments {
class ChangeMethodContractCheck_JavaxAndJakartaValidation {

class Parent {
@javax.validation.constraints.NotNull(groups = { ChangeMethodContractCheck.class })
String annotatedNotNullWithArg(Object a) { return "null"; }
String annotatedJavaxNotNullWithArg(Object a) { return "null"; }

@javax.validation.constraints.NotNull
String annotatedNotNullWithoutArg(Object a) { return "null"; }
String annotatedJavaxNotNullWithoutArg(Object a) { return "null"; }

@jakarta.validation.constraints.NotNull(groups = { ChangeMethodContractCheck.class })
String annotatedJakartaNotNullWithArg(Object a) { return "null"; }

@jakarta.validation.constraints.NotNull
String annotatedJakartaNotNullWithoutArg(Object a) { return "null"; }

void argAnnotatedNoNullWithArg(@javax.validation.constraints.NotNull(groups = { ChangeMethodContractCheck.class }) Object a) { }
void argAnnotatedNoNullWithoutArg(@javax.validation.constraints.NotNull Object a) { }
void argAnnotatedJavaxNotNull(@javax.validation.constraints.NotNull Object a) { }
void argAnnotatedJakartaNotNull(@jakarta.validation.constraints.NotNull Object a) { }

@javax.validation.constraints.NotNull
String methodNonnullJavaxBvReturn(Object a) { return ""; }
@jakarta.validation.constraints.NotNull
String methodNonnullJakartaBvReturn(Object a) { return ""; }
}

class Child extends Parent {
// Parent is not strictly not null (NotNull with arguments).
// Parent is weakly nullable.
@Override
@javax.annotation.CheckForNull
String annotatedJavaxNotNullWithArg(Object a) { return null; }

@Override
@javax.annotation.CheckForNull
String annotatedJavaxNotNullWithoutArg(Object a) { return null; }

@Override
@javax.annotation.CheckForNull
String annotatedNotNullWithArg(Object a) { return null; }
String annotatedJakartaNotNullWithArg(Object a) { return null; }

@Override
// This one is a TP though.
@javax.annotation.CheckForNull
String annotatedNotNullWithoutArg(Object a) { return null; } // Noncompliant {{Fix the incompatibility of the annotation @CheckForNull to honor @NotNull of the overridden method.}}
String annotatedJakartaNotNullWithoutArg(Object a) { return null; }

// It works correctly for arguments though.
// It works correctly also for arguments.
void argAnnotatedNoNullWithArg(@javax.annotation.CheckForNull Object a) { }
void argAnnotatedNoNullWithoutArg(@javax.annotation.CheckForNull Object a) { }
// Bean Validation @NotNull is a runtime constraint: strengthening to @Nonnull in child is not a contract violation.
void argAnnotatedJavaxNotNull(@javax.annotation.Nonnull Object a) { } // Compliant
void argAnnotatedJakartaNotNull(@javax.annotation.Nonnull Object a) { } // Compliant
// It works correctly also for return values: BV @NotNull to @Nonnull is strengthening.
@javax.annotation.Nonnull
String methodNonnullJavaxBvReturn(Object a) { return ""; } // Compliant
@javax.annotation.Nonnull
String methodNonnullJakartaBvReturn(Object a) { return ""; } // Compliant
}
}

Expand Down
2 changes: 1 addition & 1 deletion ...ces/default/src/main/java/checks/jspecify/RedundantNullabilityAnnotationsCheckSample.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ public void methodNonNullParam(@javax.annotation.Nonnull(when= When.ALWAYS) Obje
// ...
}

@NotNull // Noncompliant {{Remove redundant annotation @NotNull as inside scope annotation @NullMarked at class level.}}
@NotNull // Compliant
public Integer methodJXNonNullReturn(Object o) {
return 0;
}
Expand Down
25 changes: 22 additions & 3 deletions java-checks/src/main/java/org/sonar/java/checks/ChangeMethodContractCheck.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import java.util.Collections;
import java.util.List;
import java.util.Optional;
import java.util.Set;
import org.sonar.check.Rule;
import org.sonar.java.checks.helpers.MethodTreeUtils;
import org.sonar.java.model.JUtils;
Expand All @@ -34,12 +35,21 @@
import org.sonar.plugins.java.api.tree.TypeTree;
import org.sonar.plugins.java.api.tree.VariableTree;

import org.sonarsource.analyzer.commons.collections.SetUtils;

import static org.sonar.java.checks.helpers.NullabilityDataUtils.nullabilityAsString;
import static org.sonar.plugins.java.api.semantic.SymbolMetadata.NullabilityLevel.PACKAGE;

@Rule(key = "S2638")
public class ChangeMethodContractCheck extends IssuableSubscriptionVisitor {

// Bean Validation @NotNull is a runtime constraint, not a static nullability guarantee.
// When the parent parameter is annotated with it, strengthening it to @Nonnull in the child is not a contract violation.
private static final Set<String> BEAN_VALIDATION_ANNOTATIONS = SetUtils.immutableSetOf(
"javax.validation.constraints.NotNull",
"jakarta.validation.constraints.NotNull"
);

@Override
public List<Tree.Kind> nodesToVisit() {
return Collections.singletonList(Tree.Kind.METHOD);
Expand Down Expand Up @@ -83,9 +93,12 @@ private void checkContractChange(MethodTree methodTree, Symbol.MethodSymbol over

private void compareNullability(TypeTree tree, SymbolMetadata upperBound, SymbolMetadata lowerBound, boolean overriddenIsLowerBound) {
// Check current level
if (upperBound.nullabilityData().isNullable(PACKAGE, false, false)
&& lowerBound.nullabilityData().isNonNull(PACKAGE, false, false)) {
reportIssue(tree, lowerBound.nullabilityData(), upperBound.nullabilityData(), overriddenIsLowerBound);
NullabilityData upperData = upperBound.nullabilityData();
NullabilityData lowerData = lowerBound.nullabilityData();
if (!isBeanValidationAnnotation(upperData)
&& upperData.isNullable(PACKAGE, false, false)
&& lowerData.isNonNull(PACKAGE, false, false)) {
reportIssue(tree, lowerData, upperData, overriddenIsLowerBound);
}

// Check type parameters
Expand Down Expand Up @@ -114,6 +127,12 @@ private void checkParameter(VariableTree parameter, SymbolMetadata overrideePara
compareNullability(parameter.type(), overrideeParam, parameter.symbol().metadata(), false);
}

private static boolean isBeanValidationAnnotation(NullabilityData data) {
SymbolMetadata.AnnotationInstance annotation = data.annotation();
return annotation != null
&& BEAN_VALIDATION_ANNOTATIONS.contains(annotation.symbol().type().fullyQualifiedName());
}

private void reportIssue(Tree reportLocation, NullabilityData upperBound, NullabilityData lowerBound, boolean overriddenIsLowerBound) {
NullabilityData otherNullability = overriddenIsLowerBound ? lowerBound : upperBound;
NullabilityData overrideeNullability = overriddenIsLowerBound ? upperBound : lowerBound;
Expand Down
13 changes: 12 additions & 1 deletion java-checks/src/main/java/org/sonar/java/checks/PrimitivesMarkedNullableCheck.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

import java.util.Collections;
import java.util.List;
import java.util.Set;
import org.sonar.check.Rule;
import org.sonar.java.checks.helpers.QuickFixHelper;
import org.sonar.java.reporting.JavaQuickFix;
Expand All @@ -28,13 +29,22 @@
import org.sonar.plugins.java.api.tree.MethodTree;
import org.sonar.plugins.java.api.tree.Tree;
import org.sonar.plugins.java.api.tree.TypeTree;
import org.sonarsource.analyzer.commons.collections.SetUtils;

import static org.sonar.java.reporting.AnalyzerMessage.textSpanBetween;
import static org.sonar.plugins.java.api.semantic.SymbolMetadata.NullabilityLevel.METHOD;

@Rule(key = "S4682")
public final class PrimitivesMarkedNullableCheck extends IssuableSubscriptionVisitor {

// Bean Validation @NotNull is a runtime constraint, not a nullability annotation.
// Primitives can never be null, so this constraint is meaningless on a primitive return type,
// but it is a different concern from what this rule targets (nullable annotations on primitives).
private static final Set<String> CONSTRAINT_ANNOTATIONS_NOT_FLAGGED = SetUtils.immutableSetOf(
"javax.validation.constraints.NotNull",
"jakarta.validation.constraints.NotNull"
);

@Override
public List<Tree.Kind> nodesToVisit() {
return Collections.singletonList(Tree.Kind.METHOD);
Expand All @@ -50,7 +60,8 @@ public void visitNode(Tree tree) {
SymbolMetadata.AnnotationInstance annotation = nullabilityData.annotation();
Tree annotationTree = nullabilityData.declaration();
// Both "annotation" and "declaration" should never be null, as we only target directly annotated methods. We keep the check for defensive programming.
if (annotation != null && annotationTree != null) {
if (annotation != null && annotationTree != null
&& !CONSTRAINT_ANNOTATIONS_NOT_FLAGGED.contains(annotation.symbol().type().fullyQualifiedName())) {
String annotationName = annotation.symbol().name();
QuickFixHelper.newIssue(context)
.forRule(this)
Expand Down
7 changes: 5 additions & 2 deletions java-frontend/src/main/java/org/sonar/java/model/JSymbolMetadataNullabilityHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,11 @@ private JSymbolMetadataNullabilityHelper() {
"io.reactivex.rxjava3.annotations.Nullable",
"javax.annotation.Nullable",
"jakarta.annotation.Nullable",
// Bean Validation @NotNull is a runtime constraint, not a static nullability guarantee.
// It is placed here rather than NONNULL_ANNOTATIONS because it cannot serve as a reliable
// static analysis signal (especially when groups= is used), so it is treated conservatively.
"javax.validation.constraints.NotNull",
"jakarta.validation.constraints.NotNull",
Comment on lines +94 to +98

@gitar-bot gitar-bot Bot Aug 21, 2026 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Bug: @NotNull reclassified globally causes new FP in Spring S6816

Moving javax/jakarta.validation.constraints.NotNull from NONNULL_ANNOTATIONS to WEAK_NULLABLE_ANNOTATIONS is a global change that affects every rule reading nullabilityData(), not just the two rules patched with explicit exclusions. In S6816 (NullableInjectedFieldsHaveDefaultValueCheck), getNullableAnnotation() returns present whenever an annotation exists and isNonNull() is false; a Spring @Value("${x}") @NotNull field (a very common combination) now satisfies that condition and gets flagged "Provide a default null value for this field" — contradictory advice for a field explicitly required to be non-null. This is a new false positive introduced by the reclassification and is not covered by any test in this PR. Consider either keeping @NotNull out of both the NONNULL and NULLABLE sets (so it resolves to NO_ANNOTATION/UNKNOWN and only the intended rules change), or adding a bean-validation guard to S6816 like the ones added to S2638/S4682.

Guard S6816 against Bean Validation @NotNull (mirroring the exclusion sets added to S2638/S4682).:

private static Optional<AnnotationTree> getNullableAnnotation(VariableTree field) {
  SymbolMetadata.NullabilityData nullabilityData = field.symbol().metadata().nullabilityData(SymbolMetadata.NullabilityTarget.FIELD);
  SymbolMetadata.AnnotationInstance instance = nullabilityData.annotation();
  if (instance == null
      || nullabilityData.isNonNull(SymbolMetadata.NullabilityLevel.CLASS, false, false)
      || isBeanValidationConstraint(instance)) {
    return Optional.empty();
  }
  return Optional.ofNullable(field.symbol().metadata().findAnnotationTree(instance));
}

Was this helpful? React with 👍 / 👎

Comment on lines +94 to +98

@gitar-bot gitar-bot Bot Aug 21, 2026 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Bug: Global @NotNull move silently changes S2789/S2447/S4454 behavior

Beyond the two patched rules, the reclassification changes other rules that read nullability with no accompanying tests: S2789 (NullShouldNotBeUsedWithOptionalCheck) will now report an @NotNull Optional<...> method as "should not be @NotNull" (new false positive via isNullable at line 175); S2447 (BooleanMethodReturnCheck) will stop checking @NotNull Boolean methods that return null literals, since !isNullable(...) is now false (new false negative); and S4454 (EqualsParametersMarkedNonNullCheck) stops flagging @NotNull equals parameters (the PR's test change to EqualsParametersMarkedNonNullCheckSample marks this Compliant, so likely intended, but S2789/S2447 appear unintended). Add ruling/test coverage for these rules to confirm the behavior changes are acceptable, or gate the reclassification the same way the two patched rules do.

Avoid globally tagging @NotNull as nullable; scope the special-casing to the intended rules.:

// Prefer not classifying a runtime constraint as a static nullability signal at all:
// keep javax/jakarta validation @NotNull OUT of both WEAK_NULLABLE_ANNOTATIONS and
// NONNULL_ANNOTATIONS so it resolves to NO_ANNOTATION, then only S2638/S4682 need the
// explicit bean-validation handling. This avoids the S2789/S2447/S4454 side effects.

Was this helpful? React with 👍 / 👎

"org.checkerframework.checker.nullness.compatqual.NullableDecl",
"org.checkerframework.checker.nullness.compatqual.NullableType",
"org.checkerframework.checker.nullness.qual.Nullable",
Expand Down Expand Up @@ -119,8 +124,6 @@ private JSymbolMetadataNullabilityHelper() {
"edu.umd.cs.findbugs.annotations.NonNull",
"io.reactivex.annotations.NonNull",
"io.reactivex.rxjava3.annotations.NonNull",
"javax.validation.constraints.NotNull",
"jakarta.validation.constraints.NotNull",
"lombok.NonNull",
"org.checkerframework.checker.nullness.compatqual.NonNullDecl",
"org.checkerframework.checker.nullness.compatqual.NonNullType",
Expand Down
Loading
Loading