Skip to content

Commit

Permalink
Recognize @NullUnmarked.
Browse files Browse the repository at this point in the history
This is slightly more important now that Guava uses package-level `@NullMarked` annotations (which I don't normally recommend but which we use to support Java 8 users who do unusual things). But, aside from some Google-internal classes, the _public_ types in the Guava packages are in fact null-marked. So this change is (for now) largely about avoiding `NullArgumentForNonNullParameter` findings in Guava itself.

PiperOrigin-RevId: 708420114
  • Loading branch information
cpovirk authored and Error Prone Team committed Jan 15, 2025
1 parent e565405 commit 665073d
Show file tree
Hide file tree
Showing 4 changed files with 100 additions and 3 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,8 @@ public final class NullArgumentForNonNullParameter extends BugChecker
memoize(state -> state.getName("com.google.protobuf.Internal$ProtoNonnullApi"));
private static final Supplier<Name> NULL_MARKED_NAME =
memoize(state -> state.getName("org.jspecify.annotations.NullMarked"));
private static final Supplier<Name> NULL_UNMARKED_NAME =
memoize(state -> state.getName("org.jspecify.annotations.NullUnmarked"));
private static final Supplier<ImmutableSet<Name>> NULL_MARKED_PACKAGES_WE_TRUST =
memoize(
state ->
Expand Down Expand Up @@ -245,16 +247,27 @@ private static boolean isParameterOfMethodOnTypeStartingWith(
return enclosingClass(sym).fullname.startsWith(nameSupplier.get(state));
}

/*
* TODO(cpovirk): Unify this with NullnessUtils.isInNullMarkedScope, noting that this method also
* recognizes @ProtoNonnullApi (though perhaps we will successfully migrate protobuf to
* @NullMarked first) and also that this method trusts @NullMarked only on certain packages.
*/
private boolean enclosingAnnotationDefaultsNonTypeVariablesToNonNull(
Symbol sym, VisitorState state) {
for (; sym != null; sym = sym.getEnclosingElement()) {
if (hasDirectAnnotation(sym, PROTO_NONNULL_API_NAME.get(state))) {
return true;
}
if (hasDirectAnnotation(sym, NULL_MARKED_NAME.get(state))
&& weTrustNullMarkedOn(sym, state)) {
// https://jspecify.dev/docs/spec/#null-marked-scope
// TODO(cpovirk): Including handling of @kotlin.Metadata.
boolean marked = hasDirectAnnotation(sym, NULL_MARKED_NAME.get(state));
boolean unmarked = hasDirectAnnotation(sym, NULL_UNMARKED_NAME.get(state));
if (marked && !unmarked && weTrustNullMarkedOn(sym, state)) {
return true;
}
if (unmarked && !marked) {
return false;
}
}
return false;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -128,11 +128,23 @@ static boolean nullnessChecksShouldBeConservative(ErrorProneFlags flags) {
* such a Symbol.
*/

/*
* TODO(cpovirk): Unify this with
* NullArgumentForNonnullParameter.enclosingAnnotationDefaultsNonTypeVariablesToNonNull, but note
* the differences documented on that method.
*/
static boolean isInNullMarkedScope(Symbol sym, VisitorState state) {
for (; sym != null; sym = sym.getEnclosingElement()) {
if (hasAnnotation(sym, "org.jspecify.annotations.NullMarked", state)) {
// https://jspecify.dev/docs/spec/#null-marked-scope
// TODO(cpovirk): Including handling of @kotlin.Metadata.
boolean marked = hasAnnotation(sym, "org.jspecify.annotations.NullMarked", state);
boolean unmarked = hasAnnotation(sym, "org.jspecify.annotations.NullUnmarked", state);
if (marked && !unmarked) {
return true;
}
if (unmarked && !marked) {
return false;
}
}
return false;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,24 @@ abstract class Foo {
.doTest();
}

@Test
public void negativeConservativeNullMarkedCountermanded() {
conservativeHelper
.addSourceLines(
"Foo.java",
"""
import org.jspecify.annotations.NullMarked;
import org.jspecify.annotations.NullUnmarked;
@NullMarked
abstract class Foo {
@NullUnmarked
public abstract boolean equals(Object o);
}
""")
.doTest();
}

@Test
public void negativeConservativeNotNullMarked() {
conservativeHelper
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,60 @@ void foo() {
.doTest();
}

@Test
public void positiveNullMarkedPackageInfo() {
aggressiveHelper
.addSourceLines(
"p/package-info.java",
"""
@org.jspecify.annotations.NullMarked
package p;
""")
.addSourceLines(
"p/Foo.java",
"""
package p;
class Foo {
void consume(String s) {}
void foo() {
// BUG: Diagnostic contains:
consume(null);
}
}
""")
.doTest();
}

@Test
public void negativeNullMarkedPackageInfoCountermanded() {
aggressiveHelper
.addSourceLines(
"p/package-info.java",
"""
@org.jspecify.annotations.NullMarked
package p;
""")
.addSourceLines(
"p/Foo.java",
"""
package p;
import org.jspecify.annotations.NullUnmarked;
@NullUnmarked
class Foo {
void consume(String s) {}
void foo() {
consume(null);
}
}
""")
.doTest();
}

@Test
public void negativeNullMarkedNonComGoogleCommonPackageConservative() {
conservativeHelper
Expand Down

0 comments on commit 665073d

Please sign in to comment.