Skip to content
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.

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

Better @MonotonicNonNull support #1149

Merged
merged 10 commits into from
Feb 25, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
package com.uber.nullaway.annotations;

import java.lang.annotation.ElementType;
import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;
import java.lang.annotation.Target;

/**
* Indicates that once the field becomes non-null, it never becomes null again. Inspired by the
* identically-named annotation from the Checker Framework. A {@code @MonotonicNonNull} field can
* only be assigned non-null values. The key reason to use this annotation with NullAway is to
* enable reasoning about field non-nullness in nested lambdas / anonymous classes, e.g.:
*
* <pre>
* class Foo {
* {@literal @}MonotonicNonNull Object theField;
* void foo() {
* theField = new Object();
* Runnable r = () -> {
* // No error, NullAway knows theField is non-null after assignment
* theField.toString();
* }
* }
* }
* </pre>
*/
@Retention(RetentionPolicy.CLASS)
@Target(ElementType.FIELD)
public @interface MonotonicNonNull {}
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,6 @@ final class ErrorProneCLIFlagsConfig implements Config {
"jakarta.inject.Inject", // no explicit initialization when there is dependency injection
"javax.inject.Inject", // no explicit initialization when there is dependency injection
"com.google.errorprone.annotations.concurrent.LazyInit",
"org.checkerframework.checker.nullness.qual.MonotonicNonNull",
"org.springframework.beans.factory.annotation.Autowired",
"org.springframework.boot.test.mock.mockito.MockBean",
"org.springframework.boot.test.mock.mockito.SpyBean",
Expand Down Expand Up @@ -483,9 +482,14 @@ public boolean isKnownInitializerMethod(Symbol.MethodSymbol methodSymbol) {
return knownInitializers.contains(classAndName);
}

/**
* NOTE: this checks not only for excluded field annotations according to the config, but also for
* a {@code @Nullable} annotation or a {@code @MonotonicNonNull} annotation.
*/
@Override
public boolean isExcludedFieldAnnotation(String annotationName) {
return Nullness.isNullableAnnotation(annotationName, this)
|| Nullness.isMonotonicNonNullAnnotation(annotationName)
|| (fieldAnnotPattern != null && fieldAnnotPattern.matcher(annotationName).matches());
}

Expand Down
13 changes: 10 additions & 3 deletions nullaway/src/main/java/com/uber/nullaway/NullAway.java
Original file line number Diff line number Diff line change
Expand Up @@ -1509,7 +1509,7 @@ public Description matchVariable(VariableTree tree, VisitorState state) {
}
ExpressionTree initializer = tree.getInitializer();
if (initializer != null) {
if (!symbol.type.isPrimitive() && !skipDueToFieldAnnotation(symbol)) {
if (!symbol.type.isPrimitive() && !skipFieldInitializationCheckingDueToAnnotation(symbol)) {
if (mayBeNullExpr(state, initializer)) {
ErrorMessage errorMessage =
new ErrorMessage(
Expand Down Expand Up @@ -2398,7 +2398,8 @@ private FieldInitEntities collectEntities(ClassTree tree, VisitorState state) {
// field declaration
VariableTree varTree = (VariableTree) memberTree;
Symbol fieldSymbol = ASTHelpers.getSymbol(varTree);
if (fieldSymbol.type.isPrimitive() || skipDueToFieldAnnotation(fieldSymbol)) {
if (fieldSymbol.type.isPrimitive()
|| skipFieldInitializationCheckingDueToAnnotation(fieldSymbol)) {
continue;
}
if (varTree.getInitializer() != null) {
Expand Down Expand Up @@ -2462,7 +2463,13 @@ private boolean isInitializerMethod(VisitorState state, Symbol.MethodSymbol symb
return isInitializerMethod(state, closestOverriddenMethod);
}

private boolean skipDueToFieldAnnotation(Symbol fieldSymbol) {
/**
* Checks if the field has an annotation indicating that we should skip initialization checking
*
* @param fieldSymbol the field symbol
* @return true if the field has an annotation indicating that we should skip initialization
*/
private boolean skipFieldInitializationCheckingDueToAnnotation(Symbol fieldSymbol) {
return NullabilityUtil.getAllAnnotations(fieldSymbol, config)
.map(anno -> anno.getAnnotationType().toString())
.anyMatch(config::isExcludedFieldAnnotation);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -480,7 +480,7 @@ public static boolean mayBeNullFieldFromType(
return !(symbol.getSimpleName().toString().equals("class")
|| symbol.isEnum()
|| codeAnnotationInfo.isSymbolUnannotated(symbol, config, null))
&& Nullness.hasNullableAnnotation(symbol, config);
&& Nullness.hasNullableOrMonotonicNonNullAnnotation(symbol, config);
}

/**
Expand Down
25 changes: 25 additions & 0 deletions nullaway/src/main/java/com/uber/nullaway/Nullness.java
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,31 @@ public enum Nullness implements AbstractValue<Nullness> {
this.displayName = displayName;
}

/**
* Check whether an annotation should be treated as equivalent to <code>@MonotonicNonNull</code>.
* For now checks if the simple name of the annotation is {@code MonotonicNonNull}, from any
* package.
*/
public static boolean isMonotonicNonNullAnnotation(String annotName) {
return annotName.endsWith(".MonotonicNonNull");
}

/**
* Check for either a {@code @Nullable} annotation or a {@code @MonotonicNonNull} annotation on
* {@code symbol}. Used to reason whether a field may be null.
*/
public static boolean hasNullableOrMonotonicNonNullAnnotation(Symbol symbol, Config config) {
return hasNullableOrMonotonicNonNullAnnotation(
NullabilityUtil.getAllAnnotations(symbol, config), config);
}

private static boolean hasNullableOrMonotonicNonNullAnnotation(
Stream<? extends AnnotationMirror> annotations, Config config) {
return annotations
.map(anno -> anno.getAnnotationType().toString())
.anyMatch(anno -> isNullableAnnotation(anno, config) || isMonotonicNonNullAnnotation(anno));
}

// The following leastUpperBound and greatestLowerBound methods were created by handwriting a
// truth table and then encoding the values into these functions. A better approach would be to
// represent the lattice directly and compute these functions from the lattice.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import static com.uber.nullaway.NullabilityUtil.castToNonNull;

import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList;
import com.google.errorprone.VisitorState;
import com.google.errorprone.dataflow.nullnesspropagation.NullnessAnalysis;
import com.sun.source.tree.Tree;
Expand Down Expand Up @@ -211,12 +212,27 @@ public NullnessStore getNullnessInfoBeforeNestedMethodNode(
return store.filterAccessPaths(
(ap) -> {
boolean allAPNonRootElementsAreFinalFields = true;
for (AccessPathElement ape : ap.getElements()) {
ImmutableList<AccessPathElement> elements = ap.getElements();
for (int i = 0; i < elements.size(); i++) {
AccessPathElement ape = elements.get(i);
Element e = ape.getJavaElement();
if (!e.getKind().equals(ElementKind.FIELD)
|| !e.getModifiers().contains(Modifier.FINAL)) {
allAPNonRootElementsAreFinalFields = false;
break;
if (i != elements.size() - 1) { // "inner" elements of the access path
if (!e.getKind().equals(ElementKind.FIELD)
|| !e.getModifiers().contains(Modifier.FINAL)) {
allAPNonRootElementsAreFinalFields = false;
break;
}
} else { // last element
// must be a field that is final or annotated with @MonotonicNonNull
if (!e.getKind().equals(ElementKind.FIELD)
|| (!e.getModifiers().contains(Modifier.FINAL)
&& !e.getAnnotationMirrors().stream()
.anyMatch(
am ->
Nullness.isMonotonicNonNullAnnotation(
am.getAnnotationType().toString())))) {
allAPNonRootElementsAreFinalFields = false;
}
}
}
if (allAPNonRootElementsAreFinalFields) {
Expand Down
186 changes: 186 additions & 0 deletions nullaway/src/test/java/com/uber/nullaway/MonotonicNonNullTests.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,186 @@
package com.uber.nullaway;

import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;

@RunWith(JUnit4.class)
public class MonotonicNonNullTests extends NullAwayTestsBase {

@Test
public void initializerExpression() {
defaultCompilationHelper
.addSourceLines(
"Test.java",
"package com.uber;",
"import com.uber.nullaway.annotations.MonotonicNonNull;",
"class Test {",
" // this is fine; same as implicit initialization",
" @MonotonicNonNull Object f1 = null;",
" @MonotonicNonNull Object f2 = new Object();",
"}")
.doTest();
}

@Test
public void assignments() {
defaultCompilationHelper
.addSourceLines(
"Test.java",
"package com.uber;",
"import com.uber.nullaway.annotations.MonotonicNonNull;",
"class Test {",
" @MonotonicNonNull Object f1;",
" void testPositive() {",
" // BUG: Diagnostic contains: assigning @Nullable expression",
" f1 = null;",
" }",
" void testNegative() {",
" f1 = new Object();",
" }",
"}")
.doTest();
}

@Test
public void lambdas() {
defaultCompilationHelper
.addSourceLines(
"Test.java",
"package com.uber;",
"import com.uber.nullaway.annotations.MonotonicNonNull;",
"class Test {",
" @MonotonicNonNull Object f1;",
" void testPositive() {",
" Runnable r = () -> {",
" // BUG: Diagnostic contains: dereferenced expression f1",
" f1.toString();",
" };",
" }",
" void testNegative() {",
" f1 = new Object();",
" Runnable r = () -> {",
" f1.toString();",
" };",
" }",
"}")
.doTest();
}

@Test
public void anonymousClasses() {
defaultCompilationHelper
.addSourceLines(
"Test.java",
"package com.uber;",
"import com.uber.nullaway.annotations.MonotonicNonNull;",
"class Test {",
" @MonotonicNonNull Object f1;",
" void testPositive() {",
" Runnable r = new Runnable() {",
" @Override",
" public void run() {",
" // BUG: Diagnostic contains: dereferenced expression f1",
" f1.toString();",
" }",
" };",
" }",
" void testNegative() {",
" f1 = new Object();",
" Runnable r = new Runnable() {",
" @Override",
" public void run() {",
" f1.toString();",
" }",
" };",
" }",
"}")
.doTest();
}

@Test
public void nestedObjects() {
defaultCompilationHelper
.addSourceLines(
"Test.java",
"package com.uber;",
"import com.uber.nullaway.annotations.MonotonicNonNull;",
"import org.jspecify.annotations.Nullable;",
"class Test {",
" class Foo {",
" @MonotonicNonNull Object x;",
" }",
" final Foo f1 = new Foo();",
" Foo f2 = new Foo(); // not final",
" @Nullable Foo f3;",
" void testPositive1() {",
" f2.x = new Object();",
" Runnable r = () -> {",
" // report a bug since f2 may be overwritten",
" // BUG: Diagnostic contains: dereferenced expression f2.x",
" f2.x.toString();",
" };",
" }",
" void testPositive2() {",
" f3 = new Foo();",
" f3.x = new Object();",
" Runnable r = () -> {",
" // report a bug since f3 may be overwritten",
" // BUG: Diagnostic contains: dereferenced expression f3.x",
" f3.x.toString();",
" };",
" }",
" void testNegative() {",
" f1.x = new Object();",
" Runnable r = () -> {",
" f1.x.toString();",
" };",
" }",
"}")
.doTest();
}

@Test
public void accessPathsWithMethodCalls() {
defaultCompilationHelper
.addSourceLines(
"Test.java",
"package com.uber;",
"import com.uber.nullaway.annotations.MonotonicNonNull;",
"import org.jspecify.annotations.Nullable;",
"class Test {",
" class Foo {",
" @MonotonicNonNull Object x;",
" }",
" Foo f1 = new Foo();",
" final Foo getF1() {",
" return f1;",
" }",
" final @Nullable Foo getOther() {",
" return null;",
" }",
" void testPositive1() {",
" getF1().x = new Object();",
" Runnable r = () -> {",
" // BUG: Diagnostic contains: dereferenced expression",
" getF1().x.toString();",
" };",
" }",
" void testPositive2() {",
" if (getOther() != null) {",
" getOther().x = new Object();",
" Runnable r1 = () -> {",
" // getOther() should be treated as @Nullable in the lambda",
" // BUG: Diagnostic contains: dereferenced expression",
" getOther().toString();",
" };",
" Runnable r2 = () -> {",
" // BUG: Diagnostic contains: dereferenced expression",
" getOther().x.toString();",
" };",
" }",
" }",
"}")
.doTest();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -326,6 +326,8 @@ static class MonotonicNonNullUsage {

@MonotonicNonNull Object f;

@com.uber.nullaway.annotations.MonotonicNonNull Object g;

MonotonicNonNullUsage() {}
}

Expand Down