Skip to content

Commit

Permalink
SONARJAVA-4544 Support org.jspecify.annotations @nullable and @nonnull
Browse files Browse the repository at this point in the history
…in existing nullability checks (#4898)
  • Loading branch information
irina-batinic-sonarsource authored Oct 7, 2024
1 parent 8fef1ca commit 09ec082
Show file tree
Hide file tree
Showing 20 changed files with 162 additions and 23 deletions.
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"ruleKey": "S2638",
"hasTruePositives": true,
"falseNegatives": 7,
"falseNegatives": 12,
"falsePositives": 0
}
}
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"ruleKey": "S2789",
"hasTruePositives": true,
"falseNegatives": 11,
"falseNegatives": 17,
"falsePositives": 0
}
}
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"ruleKey": "S4454",
"hasTruePositives": true,
"falseNegatives": 1,
"falseNegatives": 2,
"falsePositives": 0
}
}
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"ruleKey": "S4682",
"hasTruePositives": true,
"falseNegatives": 0,
"falseNegatives": 2,
"falsePositives": 0
}
}
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"ruleKey": "S4738",
"hasTruePositives": false,
"falseNegatives": 44,
"falseNegatives": 46,
"falsePositives": 0
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,4 @@
"hasTruePositives": true,
"falseNegatives": 62,
"falsePositives": 0
}
}
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"ruleKey": "S6816",
"hasTruePositives": false,
"falseNegatives": 11,
"falseNegatives": 12,
"falsePositives": 0
}
}
5 changes: 5 additions & 0 deletions java-checks-test-sources/default/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -981,6 +981,11 @@
<artifactId>vavr</artifactId>
<version>0.10.4</version>
</dependency>
<dependency>
<groupId>org.jspecify</groupId>
<artifactId>jspecify</artifactId>
<version>1.0.0</version>
</dependency>
</dependencies>

<profiles>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,11 @@
import javax.annotation.Nullable;

class BooleanMethodReturnCheckSampleA {

public @org.jspecify.annotations.Nullable Boolean myTest() {
return null; // Compliant
}

public Boolean myMethod() {
return null; // Noncompliant {{Null is returned but a "Boolean" is expected.}}
}
Expand All @@ -27,6 +32,11 @@ public Boolean foo() {
public Boolean bar() {
return null; // Compliant
}

@org.jspecify.annotations.Nullable
public Boolean baz() {
return null; // Compliant
}
}

class BooleanMethodReturnCheckSampleB {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,4 +59,10 @@ public boolean equals(Object object) { // Compliant
}
}

static class J {
public boolean equals(@org.jspecify.annotations.NonNull Object object) { // Noncompliant
return false;
}
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ interface NullShouldNotBeUsedWithOptionalCheck_guava {
//^^^^^^^^^
public Optional<String> getOptionalKo();

@org.jspecify.annotations.Nullable // Noncompliant
Optional<String> getOptional();
}

class NullShouldNotBeUsedWithOptionalCheck_guavaClassA {
Expand Down Expand Up @@ -93,11 +95,19 @@ public void doSomething6(@Nullable Optional<String> arg) { // Noncompliant {{"Op
// ^^^^^^^^^
}

public void doSomething6_Jspecify(@org.jspecify.annotations.Nullable Optional<String> arg) { // Noncompliant
}

public void doSomething7() {
@Nullable // Noncompliant {{"Optional" variables should not be "@Nullable".}}
// ^^^^^^^^^
Optional<String> var;
}

public void doSomething7_jspecify() {
@org.jspecify.annotations.Nullable // Noncompliant
Optional<String> var;
}

public Optional<String> doSomething8(boolean b) {
Object obj = b ? null : new Object();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,9 @@ interface NullShouldNotBeUsedWithOptionalCheck_jdk {
//^^^^^^^^^
public Optional<String> getOptionalKo();


@org.jspecify.annotations.Nullable // Noncompliant
Optional<String> getOptional();
}

class NullShouldNotBeUsedWithOptionalCheck_jdkClassA {
Expand Down Expand Up @@ -101,12 +104,20 @@ public void doSomething6(@Nullable Optional<String> arg) { // Noncompliant {{"Op
// ^^^^^^^^^
}

public void doSomething6_Jspecify(@org.jspecify.annotations.Nullable Optional<String> arg) { // Noncompliant
}

public void doSomething7() {
@Nullable // Noncompliant {{"Optional" variables should not be "@Nullable".}}
// ^^^^^^^^^
Optional<String> var;
}

public void doSomething7_jspecify() {
@org.jspecify.annotations.Nullable // Noncompliant
Optional<String> var;
}

public void NonnullWithArgument1() {
@javax.annotation.Nonnull(when= When.MAYBE) // Noncompliant {{"Optional" variables should not be "@Nonnull(when=MAYBE)".}}
// ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,11 @@ abstract class PrimitivesMarkedNullableCheckSample {
@javax.annotation.Nullable
public double getDouble1() { return 0.0; } // Noncompliant {{"@Nullable" annotation should not be used on primitive types}}

@org.jspecify.annotations.Nullable
public double getDouble1_jspecify() { return 0.0; } // Noncompliant

public @org.jspecify.annotations.Nullable double getDouble2_jspecify() { return 0.0; } // Noncompliant

public double getDouble2() { return 0.0; }

@MyCheckForNull
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,11 @@ public Object bar() {
public int[] fool() {
return null;
}


public int @org.jspecify.annotations.Nullable [] fool_jspecify() {
return null;
}

@CheckForNull
public int[] bark() {
return null;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package checks.S2638_ChangeMethodContractCheck.noPackageInfo;

import javax.annotation.meta.When;
import java.util.List;

/**
* For parameters:
Expand All @@ -21,6 +22,10 @@ class ChangeMethodContractCheck {

void argAnnotatedWeakNullable(@javax.annotation.Nullable Object a) { }
void argAnnotatedStrongNullable(@javax.annotation.CheckForNull Object a) { }
void argAnnotatedNullableJSpecify(@org.jspecify.annotations.Nullable Object a) { }
void typeArgAnnotatedNullableJSpecify(List<@org.jspecify.annotations.Nullable String> a) { }
void argAnnotatedNonNullJSpecify(@org.jspecify.annotations.NonNull Object a) { }
void typeArgAnnotatedNonNullJSpecify(List<@org.jspecify.annotations.NonNull String> a) { }
void argAnnotatedNonNull(@javax.annotation.Nonnull Object a, @javax.annotation.Nonnull Object b) { }

@javax.annotation.Nullable
Expand All @@ -30,6 +35,16 @@ void argAnnotatedNonNull(@javax.annotation.Nonnull Object a, @javax.annotation.N
@javax.annotation.Nonnull
//^^^^^^^^^^^^^^^^^^^^^^^^^>
String annotatedNonNull(Object a) { return ""; }

@org.jspecify.annotations.Nullable
String annotatedNullableJSpecify(Object a) { return "null"; }

@org.jspecify.annotations.NonNull
String annotatedNonNullJSpecify(Object a) { return "null"; }

List<@org.jspecify.annotations.Nullable String> typeAnnotatedNullableJSpecify(Object a) { return List.of(); }

List<@org.jspecify.annotations.NonNull String> typeAnnotatedNonNullJSpecify(Object a) { return List.of(); }
}

class ChangeMethodContractCheck_B extends ChangeMethodContractCheck {
Expand All @@ -39,7 +54,19 @@ void argAnnotatedWeakNullable(@javax.annotation.CheckForNull Object a) { } // Co
@Override
void argAnnotatedStrongNullable(@javax.annotation.Nullable Object a) { } // Compliant: Weak instead of Strong Nullable is accepted.

// For arguments: if you call the the method from the parent but the child is actually used, the caller will be force to give non-null argument
@Override
void argAnnotatedNullableJSpecify(@org.jspecify.annotations.NonNull Object a) { } // Noncompliant

@Override
void typeArgAnnotatedNullableJSpecify(List<@org.jspecify.annotations.NonNull String> a) { } // Noncompliant

@Override
void argAnnotatedNonNullJSpecify(@org.jspecify.annotations.Nullable Object a) { } // Compliant

@Override
void typeArgAnnotatedNonNullJSpecify(List<@org.jspecify.annotations.Nullable String> a) { } // Compliant

// For arguments: if you call the method from the parent but the child is actually used, the caller will be force to give non-null argument
// despite the fact that the implementation would accept null. It is not armful, therefore, NonNull to Strong/Weak Nullable is compliant.
@Override
void argAnnotatedNonNull(@javax.annotation.CheckForNull Object a, @javax.annotation.Nullable Object b) { } // Compliant
Expand All @@ -56,6 +83,18 @@ void argAnnotatedNonNull(@javax.annotation.CheckForNull Object a, @javax.annotat
@javax.annotation.CheckForNull // Compliant: unrelated method.
void unrelatedMethod(Object a) { }

@Override
@org.jspecify.annotations.NonNull
String annotatedNullableJSpecify(Object a) { return "null"; } // Compliant: Nonnull to Nullable is fine.

@Override
@org.jspecify.annotations.Nullable
String annotatedNonNullJSpecify(Object a) { return "null"; } // Noncompliant

List<@org.jspecify.annotations.NonNull String> typeAnnotatedNullableJSpecify(Object a) { return List.of(); } // Compliant

List<@org.jspecify.annotations.Nullable String> typeAnnotatedNonNullJSpecify(Object a) { return List.of(); } // Noncompliant

public boolean equals(Object o) { return false; } // Compliant: no nullable annotation
}

Expand All @@ -80,7 +119,7 @@ void argAnnotatedNonNull(@javax.annotation.Nonnull Object a, @javax.validation.c
String annotatedNonNull(Object a) { return null; } // Noncompliant {{Fix the incompatibility of the annotation @Nullable to honor @Nonnull of the overridden method.}}
//^^^^^^

public boolean equals(@javax.annotation.Nonnull Object o) { return false; } // Compliant, handled by by S4454.
public boolean equals(@javax.annotation.Nonnull Object o) { return false; } // Compliant, handled by S4454.
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ void argAnnotatedNonNullViaPackageAnnotation(@javax.annotation.CheckForNull Obje
//^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^>
class ChangeMethodContractCheckAtClassLevel {
void argAnnotatedNonNullViaClassAnnotation(Object a) { }
Object argAnnotatedNonNullViaClassAnnotation_jspecify(Object a) { return new Object(); }
}

class ChangeMethodContractCheckAtClassLevel_Child extends ChangeMethodContractCheckAtClassLevel {
Expand All @@ -33,4 +34,8 @@ class ChangeMethodContractCheckAtClassLevel_Child extends ChangeMethodContractCh
@Override
void argAnnotatedNonNullViaClassAnnotation(Object a) { } // Noncompliant {{Fix the incompatibility of the annotation @Nullable to honor @NonNullByDefault at class level of the overridden method.}}
//^^^^

@org.jspecify.annotations.Nullable
@Override
Object argAnnotatedNonNullViaClassAnnotation_jspecify(Object a) { return null; } // Noncompliant
}
Original file line number Diff line number Diff line change
Expand Up @@ -114,4 +114,8 @@ private String setNothingWithTwoParameters(@Nullable String a, String b) {
a = b;
return a;
}

@org.jspecify.annotations.Nullable
@Value("${my.property_jspecify}") // Noncompliant {{Provide a default null value for this field.}} [[sc=3;ec=27;secondary=-1]]
private String myProperty_jspecify;
}
18 changes: 12 additions & 6 deletions java-frontend/src/main/java/org/sonar/java/model/JSymbol.java
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
*/
package org.sonar.java.model;

import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.List;
Expand Down Expand Up @@ -371,24 +372,29 @@ private SymbolMetadata convertMetadata() {
return new JSymbolMetadata(
sema,
this,
type == null ? new IAnnotationBinding[0] : type.getTypeAnnotations(),
type == null ? new IAnnotationBinding[0] : getAnnotations(type),
binding.getAnnotations());
case IBinding.METHOD:
ITypeBinding returnType = ((IMethodBinding) binding).getReturnType();
// In rare circumstances, when the semantic information is incomplete, returnType can be null.
if (returnType == null) {
return Symbols.EMPTY_METADATA;
}
return new JSymbolMetadata(
sema,
this,
returnType.getTypeAnnotations(),
binding.getAnnotations());
return new JSymbolMetadata(sema, this, getAnnotations(returnType), binding.getAnnotations());
default:
return new JSymbolMetadata(sema, this, binding.getAnnotations());
}
}

private static IAnnotationBinding[] getAnnotations(ITypeBinding type) {
List<IAnnotationBinding> iAnnotationBindings = new ArrayList<>();
for (ITypeBinding typeArgument : type.getTypeArguments()) {
Collections.addAll(iAnnotationBindings, typeArgument.getTypeAnnotations());
}
Collections.addAll(iAnnotationBindings, type.getTypeAnnotations());
return iAnnotationBindings.toArray(new IAnnotationBinding[0]);
}

/**
* @see #owner()
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,8 @@ private JSymbolMetadataNullabilityHelper() {
// From the documentation (https://wiki.eclipse.org/JDT_Core/Null_Analysis):
// For any variable whose type is annotated with @Nullable [...] It is illegal to dereference such a variable for either field or method access.
"org.eclipse.jdt.annotation.Nullable",
"org.eclipse.jgit.annotations.Nullable");
"org.eclipse.jgit.annotations.Nullable",
"org.jspecify.annotations.Nullable");

/**
* List of "weak" annotations, when something can be null, but it may be fine to not check it.
Expand Down Expand Up @@ -133,7 +134,8 @@ private JSymbolMetadataNullabilityHelper() {
"org.jmlspecs.annotation.NonNull",
"org.netbeans.api.annotations.common.NonNull",
"org.springframework.lang.NonNull",
"reactor.util.annotation.NonNull");
"reactor.util.annotation.NonNull",
"org.jspecify.annotations.NonNull");

/**
* Can have different type depending on the argument "when" value:
Expand Down
Loading

0 comments on commit 09ec082

Please sign in to comment.