Skip to content

SONARJAVA-5803 JSpecify @NullUnmarked should be treated as unknown #5327

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

Merged
merged 3 commits into from
Oct 27, 2025
Merged

SONARJAVA-5803 JSpecify @NullUnmarked should be treated as unknown #5327

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9e3a8ee
SONARJAVA-5803 JSpecify @NullUnmarked should be treated as unknown
andreaguarino Oct 23, 2025
c69c346
Update autoscan expectations
andreaguarino Oct 23, 2025
87a44fb
Fixes after review
andreaguarino Oct 27, 2025
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
2 changes: 1 addition & 1 deletion its/autoscan/src/test/resources/autoscan/diffs/diff_S2638.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"ruleKey": "S2638",
"hasTruePositives": true,
"falseNegatives": 22,
"falseNegatives": 20,
"falsePositives": 0
}
2 changes: 1 addition & 1 deletion its/autoscan/src/test/resources/autoscan/diffs/diff_S2789.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"ruleKey": "S2789",
"hasTruePositives": true,
"falseNegatives": 43,
"falseNegatives": 35,
"falsePositives": 0
}
2 changes: 1 addition & 1 deletion its/autoscan/src/test/resources/autoscan/diffs/diff_S4682.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"ruleKey": "S4682",
"hasTruePositives": true,
"falseNegatives": 10,
"falseNegatives": 2,
"falsePositives": 0
}
2 changes: 1 addition & 1 deletion ...st-sources/default/src/main/java/checks/jspecify/ChangeMethodContractCheckNullMarked.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ class ChangeMethodContractCheck_B extends ChangeMethodContractCheck {

@NullMarked
@Override
String annotatedUnmarked(Object a) { return null; } // Noncompliant {{Fix the incompatibility of the annotation @NullMarked to honor @NullUnmarked of the overridden method.}}
String annotatedUnmarked(Object a) { return null; } // Compliant - NullUnmarked doesn't add any information about nullability

}

10 changes: 4 additions & 6 deletions ...t/src/main/java/checks/jspecify/NullShouldNotBeUsedWithOptionalCheckNullMarked_guava.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,8 @@
@NullMarked
interface NullShouldNotBeUsedWithOptionalCheck_guava {

@NullUnmarked // Noncompliant {{Methods with an "Optional" return type should not be "@NullUnmarked".}}
//^^^^^^^^^^^^^
public Optional<String> getOptionalKo();
@NullUnmarked // Compliant - NullUnmarked doesn't say anything about nullability
public Optional<String> getOptional();

}

Expand All @@ -22,9 +21,8 @@ class NullShouldNotBeUsedWithOptionalCheck_guavaClassA {
public NullShouldNotBeUsedWithOptionalCheck_guavaClassA() {
}

@NullUnmarked // Noncompliant {{Methods with an "Optional" return type should not be "@NullUnmarked".}}
//^^^^^^^^^^^^^
public Optional<String> getOptionalKo() {
@NullUnmarked // Compliant - NullUnmarked doesn't say anything about nullability
public Optional<String> getOptional() {
return null; // Noncompliant {{Methods with an "Optional" return type should never return null.}}
// ^^^^
}
Expand Down
10 changes: 4 additions & 6 deletions ...ult/src/main/java/checks/jspecify/NullShouldNotBeUsedWithOptionalCheckNullMarked_jdk.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,8 @@
@NullMarked
interface NullShouldNotBeUsedWithOptionalCheck_jdk {

@NullUnmarked // Noncompliant {{Methods with an "Optional" return type should not be "@NullUnmarked".}}
//^^^^^^^^^^^^^
public Optional<String> getOptionalKo();
@NullUnmarked // Compliant - NullUnmarked doesn't say anything about nullability
public Optional<String> getOptional();

}

Expand All @@ -23,9 +22,8 @@ class NullShouldNotBeUsedWithOptionalCheck_jdkClassA {
public NullShouldNotBeUsedWithOptionalCheck_jdkClassA() {
}

@NullUnmarked // Noncompliant {{Methods with an "Optional" return type should not be "@NullUnmarked".}}
//^^^^^^^^^^^^^
public Optional<String> getOptionalKo() {
@NullUnmarked // Compliant - NullUnmarked doesn't say anything about nullability
public Optional<String> getOptional() {
return null; // Noncompliant {{Methods with an "Optional" return type should never return null.}}
// ^^^^
}
Expand Down
12 changes: 6 additions & 6 deletions ...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 @@ -262,7 +262,7 @@ class InnerClassTests {
class InnerNullmarked {
@NullMarked // Compliant
class Inner {
@NonNull Object o; // Noncompliant {{Remove redundant annotation @NonNull as inside scope annotation @NullMarked at class level.}}
@NonNull Object o; // Compliant
Copy link
Contributor

@GabrielFleischer GabrielFleischer Oct 24, 2025

Choose a reason for hiding this comment

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

If I am not mistaken, this is indeed noncompliant.

As the class is annotated with @NullMarked, everything is assumed to be nonnull except if it is annotated with @Nullable.

So @NonNull is redundant.

Copy link
Contributor Author

@andreaguarino andreaguarino Oct 24, 2025

Choose a reason for hiding this comment

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

Indeed, thanks for catching this!

@Nullable Object o2; // Compliant
}
}
Expand All @@ -272,7 +272,7 @@ class InnerNullUnmarked {
@NullUnmarked
class Inner {
@NonNull Object o; // Compliant
public void methodNonNullParamTyped(List<@Nullable Object> o) { // Noncompliant {{Remove redundant annotation @Nullable as inside scope annotation @NullUnmarked at class level.}}
public void methodNonNullParamTyped(List<@Nullable Object> o) { // Compliant
// ..
}
}
Expand All @@ -282,7 +282,7 @@ public void methodNonNullParamTyped(List<@Nullable Object> o) { // Noncompliant
class InnerRedundantNullMarked {
@NullMarked // Compliant
class Inner {
@NullMarked // Noncompliant {{Remove redundant annotation @NullMarked at class level as inside scope annotation @NullMarked at class level.}}
@NullMarked // Compliant
Copy link
Contributor

@GabrielFleischer GabrielFleischer Oct 24, 2025

Choose a reason for hiding this comment

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

This is also Noncompliant, the class is transitively NullMarked.

Copy link
Contributor Author

@andreaguarino andreaguarino Oct 24, 2025

Choose a reason for hiding this comment

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

Indeed, thanks!

class InnerInner {}
}
}
Expand All @@ -291,19 +291,19 @@ class InnerInner {}
class InnerRedundantNullUnmarked {
@NullUnmarked // Compliant
class Inner {
@NullUnmarked // Noncompliant {{Remove redundant annotation @NullUnmarked at class level as inside scope annotation @NullUnmarked at class level.}}
@NullUnmarked // Compliant
Copy link
Contributor

@GabrielFleischer GabrielFleischer Oct 24, 2025

Choose a reason for hiding this comment

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

And this is redundant too.

I wonder what caused this to change...

Copy link
Contributor Author

@andreaguarino andreaguarino Oct 24, 2025

Choose a reason for hiding this comment

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

In the implementation, we're only checking for nullabilityData being either "isNonNull" or "isNullable", so we don't check for redundant annotations with unknown nullabilities. I'm not sure if it's worth changing this, as it would change quite a bit the implementation

class InnerInner {}
}
}
}

@NullUnmarked
class InnerClassTestsTwo {
@NullUnmarked // Noncompliant {{Remove redundant annotation @NullUnmarked at class level as inside scope annotation @NullUnmarked at class level.}}
@NullUnmarked // Compliant
class InnerNullmarked {
@NullMarked // Compliant
class Inner {
@NonNull Object o; // Noncompliant {{Remove redundant annotation @NonNull as inside scope annotation @NullMarked at class level.}}
@NonNull Object o; // Compliant
@Nullable Object o2; // Compliant
}
}
Expand Down
2 changes: 1 addition & 1 deletion ...t-sources/default/src/main/java/checks/jspecify/nullmarked/ChangeMethodContractCheck.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ class ChangeMethodContractCheck_B extends ChangeMethodContractCheck {

@NullMarked
@Override
String annotatedUnmarked(Object a) { return null; } // Noncompliant {{Fix the incompatibility of the annotation @NullMarked to honor @NullUnmarked of the overridden method.}}
String annotatedUnmarked(Object a) { return null; } // Compliant - NullUnmarked doesn't add any information about nullability

}

10 changes: 4 additions & 6 deletions .../src/main/java/checks/jspecify/nullmarked/NullShouldNotBeUsedWithOptionalCheck_guava.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,8 @@
// NullMarked at the package level
interface NullShouldNotBeUsedWithOptionalCheck_guava {

@NullUnmarked // Noncompliant {{Methods with an "Optional" return type should not be "@NullUnmarked".}}
//^^^^^^^^^^^^^
public Optional<String> getOptionalKo();
@NullUnmarked // Compliant - NullUnmarked doesn't say anything about nullability
public Optional<String> getOptional();

}

Expand All @@ -21,9 +20,8 @@ class NullShouldNotBeUsedWithOptionalCheck_guavaClassA {
public NullShouldNotBeUsedWithOptionalCheck_guavaClassA() {
}

@NullUnmarked // Noncompliant {{Methods with an "Optional" return type should not be "@NullUnmarked".}}
//^^^^^^^^^^^^^
public Optional<String> getOptionalKo() {
@NullUnmarked // Compliant - NullUnmarked doesn't say anything about nullability
public Optional<String> getOptional() {
return null; // Noncompliant {{Methods with an "Optional" return type should never return null.}}
// ^^^^
}
Expand Down
10 changes: 4 additions & 6 deletions ...lt/src/main/java/checks/jspecify/nullmarked/NullShouldNotBeUsedWithOptionalCheck_jdk.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,8 @@
// NullMarked at the package level
interface NullShouldNotBeUsedWithOptionalCheck_jdk {

@NullUnmarked // Noncompliant {{Methods with an "Optional" return type should not be "@NullUnmarked".}}
//^^^^^^^^^^^^^
public Optional<String> getOptionalKo();
@NullUnmarked // Compliant - NullUnmarked doesn't say anything about nullability
public Optional<String> getOptional();

}

Expand All @@ -22,9 +21,8 @@ class NullShouldNotBeUsedWithOptionalCheck_jdkClassA {
public NullShouldNotBeUsedWithOptionalCheck_jdkClassA() {
}

@NullUnmarked // Noncompliant {{Methods with an "Optional" return type should not be "@NullUnmarked".}}
//^^^^^^^^^^^^^
public Optional<String> getOptionalKo() {
@NullUnmarked // Compliant - NullUnmarked doesn't say anything about nullability
public Optional<String> getOptional() {
return null; // Noncompliant {{Methods with an "Optional" return type should never return null.}}
// ^^^^
}
Expand Down
12 changes: 3 additions & 9 deletions ...urces/default/src/main/java/checks/jspecify/nullmarked/PrimitivesMarkedNullableCheck.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,10 @@ abstract class PrimitivesMarkedNullableCheckSample {
abstract int getInt2();

@NullUnmarked
protected abstract boolean isBool(); // Noncompliant {{"@NullUnmarked" annotation should not be used on primitive types}} [[quickfixes=qf1]]
// ^^^^^^^
// fix@qf1 {{Remove "@NullUnmarked"}}
// edit@qf1 [[sl=-1;el=+0;sc=3;ec=3]] {{}}
protected abstract boolean isBool(); // Compliant - NullUnmarked doesn't add any information about nullability
Comment on lines 21 to +22
Copy link
Contributor

@GabrielFleischer GabrielFleischer Oct 24, 2025

Choose a reason for hiding this comment

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

I think using @NullUnmarked in these tests was a mistake. We should migrate to @Nullable as the test is doesn't prove anything otherwise...

Copy link
Contributor Author

@andreaguarino andreaguarino Oct 27, 2025

Choose a reason for hiding this comment

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

I think the test was there to assert how the check "PrimitivesMarkedNullableCheck" behave in presence of "@NullMarked" / "@NullUnmarked" annotations, so I'm not sure we should replace it with "@nullable" because it's already asserted inside "checks/PrimitivesMarkedNullableCheckSample". So maybe we should just remove this test file

Copy link
Contributor Author

@andreaguarino andreaguarino Oct 27, 2025

Choose a reason for hiding this comment

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

I eventually decided to let the existing testcase and just replace the Noncompliant -> Compliant, to still verify we don't raise FPs


@NullUnmarked
public double getDouble1() { return 0.0; } // Noncompliant {{"@NullUnmarked" annotation should not be used on primitive types}}
public double getDouble1() { return 0.0; } // Compliant - NullUnmarked doesn't add any information about nullability

public double getDouble2() { return 0.0; }

Expand All @@ -49,10 +46,7 @@ abstract class PrimitivesMarkedNullableCheckSample {

public Object getObj2() { return null; }

protected abstract @NullUnmarked boolean isBool2(); // Noncompliant {{"@NullUnmarked" annotation should not be used on primitive types}} [[quickfixes=qf3]]
// ^^^^^^^
// fix@qf3 {{Remove "@NullUnmarked"}}
// edit@qf3 [[sc=22;ec=36]] {{}}
protected abstract @NullUnmarked boolean isBool2(); // Compliant - NullUnmarked doesn't add any information about nullability

@NullUnmarked
Object containsAnonymousClass() {
Expand Down
4 changes: 2 additions & 2 deletions java-checks/src/test/java/org/sonar/java/checks/ChangeMethodContractCheckTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,11 +53,11 @@ void test_jspecify_null_marked() {
CheckVerifier.newVerifier()
.onFile(mainCodeSourcesPath("checks/jspecify/ChangeMethodContractCheckNullMarked.java"))
.withCheck(new ChangeMethodContractCheck())
.verifyIssues();
.verifyNoIssues();
CheckVerifier.newVerifier()
.onFile(mainCodeSourcesPath("checks/jspecify/nullmarked/ChangeMethodContractCheck.java"))
.withCheck(new ChangeMethodContractCheck())
.verifyIssues();
.verifyNoIssues();
}

@Test
Expand Down
4 changes: 2 additions & 2 deletions java-checks/src/test/java/org/sonar/java/checks/PrimitivesMarkedNullableCheckTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,11 +45,11 @@ void test_jspecify_null_marked() {
CheckVerifier.newVerifier()
.onFile(mainCodeSourcesPath("checks/jspecify/PrimitivesMarkedNullableCheckNullMarked.java"))
.withCheck(new PrimitivesMarkedNullableCheck())
.verifyIssues();
.verifyNoIssues();
CheckVerifier.newVerifier()
.onFile(mainCodeSourcesPath("checks/jspecify/nullmarked/PrimitivesMarkedNullableCheck.java"))
.withCheck(new PrimitivesMarkedNullableCheck())
.verifyIssues();
.verifyNoIssues();
}

}
2 changes: 1 addition & 1 deletion 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 @@ -224,7 +224,7 @@ private JSymbolMetadataNullabilityHelper() {
configureAnnotation(ORG_JSPECIFY_ANNOTATIONS_NULL_MARKED, NON_NULL,
Arrays.asList(NullabilityTarget.CLASS, FIELD, METHOD, PARAMETER),
Arrays.asList(NullabilityLevel.METHOD, CLASS, PACKAGE));
configureAnnotation(ORG_JSPECIFY_ANNOTATIONS_NULL_UNMARKED, WEAK_NULLABLE,
configureAnnotation(ORG_JSPECIFY_ANNOTATIONS_NULL_UNMARKED, UNKNOWN,
Arrays.asList(NullabilityTarget.CLASS, FIELD, METHOD, PARAMETER),
Arrays.asList(NullabilityLevel.METHOD, CLASS, PACKAGE));

Expand Down