From 4c4fb08c3b444339fa79e092c642a8553d38388a Mon Sep 17 00:00:00 2001 From: asya-vorobeva Date: Fri, 21 Aug 2026 16:51:50 +0200 Subject: [PATCH] Treat Bean Validation @NotNull as WEAK_NULLABLE to fix FPs (JAVASE-241) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit javax.validation.constraints.NotNull and jakarta.validation.constraints.NotNull are runtime constraints, not static nullability guarantees. Their previous NON_NULL classification also caused an inconsistency (SONARJAVA-3803): @NotNull without arguments resolved to NON_NULL while @NotNull(groups=...) resolved to UNKNOWN. Moving both to WEAK_NULLABLE gives consistent, conservative treatment. Rules fixed as a direct consequence: - S4454: @NotNull on equals() parameter no longer fires (WEAK_NULLABLE is not isNonNull()) - S6539: @NotNull inside @NullMarked no longer flagged as redundant Rules requiring explicit guards after reclassification: - S4682: added FQN-based exclusion — BV @NotNull on a primitive return type is a validation constraint, not a nullable annotation - S2638: added isBeanValidationAnnotation() guard in compareNullability() — when the upper-bound annotation is a BV annotation, the comparison is skipped so that BV @NotNull on a parent param or child return does not incorrectly fire Co-Authored-By: Claude Sonnet 4.6 --- .../no_default/NullabilityAtMethodLevel.java | 13 ++++++ .../NullabilityAtVariableLevel.java | 4 +- ...alsParametersMarkedNonNullCheckSample.java | 5 +- .../PrimitivesMarkedNullableCheckSample.java | 7 +++ .../ChangeMethodContractCheck.java | 46 +++++++++++++++---- ...dantNullabilityAnnotationsCheckSample.java | 2 +- .../checks/ChangeMethodContractCheck.java | 25 ++++++++-- .../checks/PrimitivesMarkedNullableCheck.java | 13 +++++- .../JSymbolMetadataNullabilityHelper.java | 7 ++- 9 files changed, 101 insertions(+), 21 deletions(-) diff --git a/java-checks-test-sources/default/src/main/java/annotations/nullability/no_default/NullabilityAtMethodLevel.java b/java-checks-test-sources/default/src/main/java/annotations/nullability/no_default/NullabilityAtMethodLevel.java index 4428585b9b3..1c3ee0a0a40 100644 --- a/java-checks-test-sources/default/src/main/java/annotations/nullability/no_default/NullabilityAtMethodLevel.java +++ b/java-checks-test-sources/default/src/main/java/annotations/nullability/no_default/NullabilityAtMethodLevel.java @@ -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 { diff --git a/java-checks-test-sources/default/src/main/java/annotations/nullability/no_default/NullabilityAtVariableLevel.java b/java-checks-test-sources/default/src/main/java/annotations/nullability/no_default/NullabilityAtVariableLevel.java index 044d5dd6103..ce3906b4910 100644 --- a/java-checks-test-sources/default/src/main/java/annotations/nullability/no_default/NullabilityAtVariableLevel.java +++ b/java-checks-test-sources/default/src/main/java/annotations/nullability/no_default/NullabilityAtVariableLevel.java @@ -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 diff --git a/java-checks-test-sources/default/src/main/java/checks/EqualsParametersMarkedNonNullCheckSample.java b/java-checks-test-sources/default/src/main/java/checks/EqualsParametersMarkedNonNullCheckSample.java index 456fe6c7403..a79d66fc95b 100644 --- a/java-checks-test-sources/default/src/main/java/checks/EqualsParametersMarkedNonNullCheckSample.java +++ b/java-checks-test-sources/default/src/main/java/checks/EqualsParametersMarkedNonNullCheckSample.java @@ -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; } } diff --git a/java-checks-test-sources/default/src/main/java/checks/PrimitivesMarkedNullableCheckSample.java b/java-checks-test-sources/default/src/main/java/checks/PrimitivesMarkedNullableCheckSample.java index 1e362c319d9..0dacf98898c 100644 --- a/java-checks-test-sources/default/src/main/java/checks/PrimitivesMarkedNullableCheckSample.java +++ b/java-checks-test-sources/default/src/main/java/checks/PrimitivesMarkedNullableCheckSample.java @@ -1,5 +1,6 @@ package checks; +import jakarta.validation.constraints.NotNull; import javax.annotation.CheckForNull; import javax.annotation.Nonnull; import javax.annotation.Nullable; @@ -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; } diff --git a/java-checks-test-sources/default/src/main/java/checks/S2638_ChangeMethodContractCheck/noPackageInfo/ChangeMethodContractCheck.java b/java-checks-test-sources/default/src/main/java/checks/S2638_ChangeMethodContractCheck/noPackageInfo/ChangeMethodContractCheck.java index 3b74c31176b..a4c981c3e36 100644 --- a/java-checks-test-sources/default/src/main/java/checks/S2638_ChangeMethodContractCheck/noPackageInfo/ChangeMethodContractCheck.java +++ b/java-checks-test-sources/default/src/main/java/checks/S2638_ChangeMethodContractCheck/noPackageInfo/ChangeMethodContractCheck.java @@ -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 } } diff --git a/java-checks-test-sources/default/src/main/java/checks/jspecify/RedundantNullabilityAnnotationsCheckSample.java b/java-checks-test-sources/default/src/main/java/checks/jspecify/RedundantNullabilityAnnotationsCheckSample.java index 0bf545a82cb..a9b7674f1d7 100644 --- a/java-checks-test-sources/default/src/main/java/checks/jspecify/RedundantNullabilityAnnotationsCheckSample.java +++ b/java-checks-test-sources/default/src/main/java/checks/jspecify/RedundantNullabilityAnnotationsCheckSample.java @@ -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; } diff --git a/java-checks/src/main/java/org/sonar/java/checks/ChangeMethodContractCheck.java b/java-checks/src/main/java/org/sonar/java/checks/ChangeMethodContractCheck.java index 8f5d328cd6a..83749dae56a 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/ChangeMethodContractCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/ChangeMethodContractCheck.java @@ -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; @@ -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 BEAN_VALIDATION_ANNOTATIONS = SetUtils.immutableSetOf( + "javax.validation.constraints.NotNull", + "jakarta.validation.constraints.NotNull" + ); + @Override public List nodesToVisit() { return Collections.singletonList(Tree.Kind.METHOD); @@ -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 @@ -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; diff --git a/java-checks/src/main/java/org/sonar/java/checks/PrimitivesMarkedNullableCheck.java b/java-checks/src/main/java/org/sonar/java/checks/PrimitivesMarkedNullableCheck.java index 651ecc6d6e1..e651e4a8778 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/PrimitivesMarkedNullableCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/PrimitivesMarkedNullableCheck.java @@ -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; @@ -28,6 +29,7 @@ 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; @@ -35,6 +37,14 @@ @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 CONSTRAINT_ANNOTATIONS_NOT_FLAGGED = SetUtils.immutableSetOf( + "javax.validation.constraints.NotNull", + "jakarta.validation.constraints.NotNull" + ); + @Override public List nodesToVisit() { return Collections.singletonList(Tree.Kind.METHOD); @@ -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) diff --git a/java-frontend/src/main/java/org/sonar/java/model/JSymbolMetadataNullabilityHelper.java b/java-frontend/src/main/java/org/sonar/java/model/JSymbolMetadataNullabilityHelper.java index 505fe318d9e..cf5390f7da4 100644 --- a/java-frontend/src/main/java/org/sonar/java/model/JSymbolMetadataNullabilityHelper.java +++ b/java-frontend/src/main/java/org/sonar/java/model/JSymbolMetadataNullabilityHelper.java @@ -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", "org.checkerframework.checker.nullness.compatqual.NullableDecl", "org.checkerframework.checker.nullness.compatqual.NullableType", "org.checkerframework.checker.nullness.qual.Nullable", @@ -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",