SONARJAVA-5803 JSpecify @NullUnmarked should be treated as unknown #5327
Conversation
| @NullMarked // Compliant | ||
| class Inner { | ||
| @NonNull Object o; // Noncompliant {{Remove redundant annotation @NonNull as inside scope annotation @NullMarked at class level.}} | ||
| @NonNull Object o; // Compliant |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Indeed, thanks for catching this!
| @NullMarked // Compliant | ||
| class Inner { | ||
| @NullMarked // Noncompliant {{Remove redundant annotation @NullMarked at class level as inside scope annotation @NullMarked at class level.}} | ||
| @NullMarked // Compliant |
There was a problem hiding this comment.
This is also Noncompliant, the class is transitively NullMarked.
There was a problem hiding this comment.
Indeed, thanks!
| @NullUnmarked // Compliant | ||
| class Inner { | ||
| @NullUnmarked // Noncompliant {{Remove redundant annotation @NullUnmarked at class level as inside scope annotation @NullUnmarked at class level.}} | ||
| @NullUnmarked // Compliant |
There was a problem hiding this comment.
And this is redundant too.
I wonder what caused this to change...
There was a problem hiding this comment.
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
| @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 |
There was a problem hiding this comment.
I think using @NullUnmarked in these tests was a mistake. We should migrate to @Nullable as the test is doesn't prove anything otherwise...
There was a problem hiding this comment.
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
There was a problem hiding this comment.
I eventually decided to let the existing testcase and just replace the Noncompliant -> Compliant, to still verify we don't raise FPs
andreaguarino
force-pushed
the
SONARJAVA-5803
branch
from
55ccdf2 to
87a44fb
Compare
|
2 participants
SONARJAVA-5803
Part of