Skip to content

Commit 8e525e6

Browse files
msridharmsridhar
authored
Better @MonotonicNonNull support (#1149)
Fixes #1148 We add explicit support for any annotation named `@MonotonicNonNull` and add our own version of the annotation to our annotations package. The main additional support is that we now reason that once assigned a non-null value, `@MonotonicNull` fields remain non-null when accessed from subsequent lambdas, even if the lambdas are invoked asynchronously.
1 parent 15c817a commit 8e525e6

File tree

8 files changed

+279
-10
lines changed

8 files changed

+279
-10
lines changed

‎annotations/src/main/java/com/uber/nullaway/annotations/MonotonicNonNull.java‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
package com.uber.nullaway.annotations;
2+
3+
import java.lang.annotation.ElementType;
4+
import java.lang.annotation.Retention;
5+
import java.lang.annotation.RetentionPolicy;
6+
import java.lang.annotation.Target;
7+
8+
/**
9+
* Indicates that once the field becomes non-null, it never becomes null again. Inspired by the
10+
* identically-named annotation from the Checker Framework. A {@code @MonotonicNonNull} field can
11+
* only be assigned non-null values. The key reason to use this annotation with NullAway is to
12+
* enable reasoning about field non-nullness in nested lambdas / anonymous classes, e.g.:
13+
*
14+
* <pre>
15+
* class Foo {
16+
* {@literal @}MonotonicNonNull Object theField;
17+
* void foo() {
18+
* theField = new Object();
19+
* Runnable r = () -> {
20+
* // No error, NullAway knows theField is non-null after assignment
21+
* theField.toString();
22+
* }
23+
* }
24+
* }
25+
* </pre>
26+
*/
27+
@Retention(RetentionPolicy.CLASS)
28+
@Target(ElementType.FIELD)
29+
public @interface MonotonicNonNull {}

‎nullaway/src/main/java/com/uber/nullaway/ErrorProneCLIFlagsConfig.java‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -175,7 +175,6 @@ final class ErrorProneCLIFlagsConfig implements Config {
175175
"jakarta.inject.Inject", // no explicit initialization when there is dependency injection
176176
"javax.inject.Inject", // no explicit initialization when there is dependency injection
177177
"com.google.errorprone.annotations.concurrent.LazyInit",
178-
"org.checkerframework.checker.nullness.qual.MonotonicNonNull",
179178
"org.springframework.beans.factory.annotation.Autowired",
180179
"org.springframework.boot.test.mock.mockito.MockBean",
181180
"org.springframework.boot.test.mock.mockito.SpyBean",
@@ -483,9 +482,14 @@ public boolean isKnownInitializerMethod(Symbol.MethodSymbol methodSymbol) {
483482
return knownInitializers.contains(classAndName);
484483
}
485484

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

‎nullaway/src/main/java/com/uber/nullaway/NullAway.java‎

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1509,7 +1509,7 @@ public Description matchVariable(VariableTree tree, VisitorState state) {
15091509
}
15101510
ExpressionTree initializer = tree.getInitializer();
15111511
if (initializer != null) {
1512-
if (!symbol.type.isPrimitive() && !skipDueToFieldAnnotation(symbol)) {
1512+
if (!symbol.type.isPrimitive() && !skipFieldInitializationCheckingDueToAnnotation(symbol)) {
15131513
if (mayBeNullExpr(state, initializer)) {
15141514
ErrorMessage errorMessage =
15151515
new ErrorMessage(
@@ -2398,7 +2398,8 @@ private FieldInitEntities collectEntities(ClassTree tree, VisitorState state) {
23982398
// field declaration
23992399
VariableTree varTree = (VariableTree) memberTree;
24002400
Symbol fieldSymbol = ASTHelpers.getSymbol(varTree);
2401-
if (fieldSymbol.type.isPrimitive() || skipDueToFieldAnnotation(fieldSymbol)) {
2401+
if (fieldSymbol.type.isPrimitive()
2402+
|| skipFieldInitializationCheckingDueToAnnotation(fieldSymbol)) {
24022403
continue;
24032404
}
24042405
if (varTree.getInitializer() != null) {
@@ -2462,7 +2463,13 @@ private boolean isInitializerMethod(VisitorState state, Symbol.MethodSymbol symb
24622463
return isInitializerMethod(state, closestOverriddenMethod);
24632464
}
24642465

2465-
private boolean skipDueToFieldAnnotation(Symbol fieldSymbol) {
2466+
/**
2467+
* Checks if the field has an annotation indicating that we should skip initialization checking
2468+
*
2469+
* @param fieldSymbol the field symbol
2470+
* @return true if the field has an annotation indicating that we should skip initialization
2471+
*/
2472+
private boolean skipFieldInitializationCheckingDueToAnnotation(Symbol fieldSymbol) {
24662473
return NullabilityUtil.getAllAnnotations(fieldSymbol, config)
24672474
.map(anno -> anno.getAnnotationType().toString())
24682475
.anyMatch(config::isExcludedFieldAnnotation);

‎nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -480,7 +480,7 @@ public static boolean mayBeNullFieldFromType(
480480
return !(symbol.getSimpleName().toString().equals("class")
481481
|| symbol.isEnum()
482482
|| codeAnnotationInfo.isSymbolUnannotated(symbol, config, null))
483-
&& Nullness.hasNullableAnnotation(symbol, config);
483+
&& Nullness.hasNullableOrMonotonicNonNullAnnotation(symbol, config);
484484
}
485485

486486
/**

‎nullaway/src/main/java/com/uber/nullaway/Nullness.java‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,31 @@ public enum Nullness implements AbstractValue<Nullness> {
5454
this.displayName = displayName;
5555
}
5656

57+
/**
58+
* Check whether an annotation should be treated as equivalent to <code>@MonotonicNonNull</code>.
59+
* For now checks if the simple name of the annotation is {@code MonotonicNonNull}, from any
60+
* package.
61+
*/
62+
public static boolean isMonotonicNonNullAnnotation(String annotName) {
63+
return annotName.endsWith(".MonotonicNonNull");
64+
}
65+
66+
/**
67+
* Check for either a {@code @Nullable} annotation or a {@code @MonotonicNonNull} annotation on
68+
* {@code symbol}. Used to reason whether a field may be null.
69+
*/
70+
public static boolean hasNullableOrMonotonicNonNullAnnotation(Symbol symbol, Config config) {
71+
return hasNullableOrMonotonicNonNullAnnotation(
72+
NullabilityUtil.getAllAnnotations(symbol, config), config);
73+
}
74+
75+
private static boolean hasNullableOrMonotonicNonNullAnnotation(
76+
Stream<? extends AnnotationMirror> annotations, Config config) {
77+
return annotations
78+
.map(anno -> anno.getAnnotationType().toString())
79+
.anyMatch(anno -> isNullableAnnotation(anno, config) || isMonotonicNonNullAnnotation(anno));
80+
}
81+
5782
// The following leastUpperBound and greatestLowerBound methods were created by handwriting a
5883
// truth table and then encoding the values into these functions. A better approach would be to
5984
// represent the lattice directly and compute these functions from the lattice.

‎nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPathNullnessAnalysis.java‎

Lines changed: 21 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
import static com.uber.nullaway.NullabilityUtil.castToNonNull;
2222

2323
import com.google.common.base.Preconditions;
24+
import com.google.common.collect.ImmutableList;
2425
import com.google.errorprone.VisitorState;
2526
import com.google.errorprone.dataflow.nullnesspropagation.NullnessAnalysis;
2627
import com.sun.source.tree.Tree;
@@ -211,12 +212,27 @@ public NullnessStore getNullnessInfoBeforeNestedMethodNode(
211212
return store.filterAccessPaths(
212213
(ap) -> {
213214
boolean allAPNonRootElementsAreFinalFields = true;
214-
for (AccessPathElement ape : ap.getElements()) {
215+
ImmutableList<AccessPathElement> elements = ap.getElements();
216+
for (int i = 0; i < elements.size(); i++) {
217+
AccessPathElement ape = elements.get(i);
215218
Element e = ape.getJavaElement();
216-
if (!e.getKind().equals(ElementKind.FIELD)
217-
|| !e.getModifiers().contains(Modifier.FINAL)) {
218-
allAPNonRootElementsAreFinalFields = false;
219-
break;
219+
if (i != elements.size() - 1) { // "inner" elements of the access path
220+
if (!e.getKind().equals(ElementKind.FIELD)
221+
|| !e.getModifiers().contains(Modifier.FINAL)) {
222+
allAPNonRootElementsAreFinalFields = false;
223+
break;
224+
}
225+
} else { // last element
226+
// must be a field that is final or annotated with @MonotonicNonNull
227+
if (!e.getKind().equals(ElementKind.FIELD)
228+
|| (!e.getModifiers().contains(Modifier.FINAL)
229+
&& !e.getAnnotationMirrors().stream()
230+
.anyMatch(
231+
am ->
232+
Nullness.isMonotonicNonNullAnnotation(
233+
am.getAnnotationType().toString())))) {
234+
allAPNonRootElementsAreFinalFields = false;
235+
}
220236
}
221237
}
222238
if (allAPNonRootElementsAreFinalFields) {

‎nullaway/src/test/java/com/uber/nullaway/MonotonicNonNullTests.java‎

Lines changed: 186 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,186 @@
1+
package com.uber.nullaway;
2+
3+
import org.junit.Test;
4+
import org.junit.runner.RunWith;
5+
import org.junit.runners.JUnit4;
6+
7+
@RunWith(JUnit4.class)
8+
public class MonotonicNonNullTests extends NullAwayTestsBase {
9+
10+
@Test
11+
public void initializerExpression() {
12+
defaultCompilationHelper
13+
.addSourceLines(
14+
"Test.java",
15+
"package com.uber;",
16+
"import com.uber.nullaway.annotations.MonotonicNonNull;",
17+
"class Test {",
18+
" // this is fine; same as implicit initialization",
19+
" @MonotonicNonNull Object f1 = null;",
20+
" @MonotonicNonNull Object f2 = new Object();",
21+
"}")
22+
.doTest();
23+
}
24+
25+
@Test
26+
public void assignments() {
27+
defaultCompilationHelper
28+
.addSourceLines(
29+
"Test.java",
30+
"package com.uber;",
31+
"import com.uber.nullaway.annotations.MonotonicNonNull;",
32+
"class Test {",
33+
" @MonotonicNonNull Object f1;",
34+
" void testPositive() {",
35+
" // BUG: Diagnostic contains: assigning @Nullable expression",
36+
" f1 = null;",
37+
" }",
38+
" void testNegative() {",
39+
" f1 = new Object();",
40+
" }",
41+
"}")
42+
.doTest();
43+
}
44+
45+
@Test
46+
public void lambdas() {
47+
defaultCompilationHelper
48+
.addSourceLines(
49+
"Test.java",
50+
"package com.uber;",
51+
"import com.uber.nullaway.annotations.MonotonicNonNull;",
52+
"class Test {",
53+
" @MonotonicNonNull Object f1;",
54+
" void testPositive() {",
55+
" Runnable r = () -> {",
56+
" // BUG: Diagnostic contains: dereferenced expression f1",
57+
" f1.toString();",
58+
" };",
59+
" }",
60+
" void testNegative() {",
61+
" f1 = new Object();",
62+
" Runnable r = () -> {",
63+
" f1.toString();",
64+
" };",
65+
" }",
66+
"}")
67+
.doTest();
68+
}
69+
70+
@Test
71+
public void anonymousClasses() {
72+
defaultCompilationHelper
73+
.addSourceLines(
74+
"Test.java",
75+
"package com.uber;",
76+
"import com.uber.nullaway.annotations.MonotonicNonNull;",
77+
"class Test {",
78+
" @MonotonicNonNull Object f1;",
79+
" void testPositive() {",
80+
" Runnable r = new Runnable() {",
81+
" @Override",
82+
" public void run() {",
83+
" // BUG: Diagnostic contains: dereferenced expression f1",
84+
" f1.toString();",
85+
" }",
86+
" };",
87+
" }",
88+
" void testNegative() {",
89+
" f1 = new Object();",
90+
" Runnable r = new Runnable() {",
91+
" @Override",
92+
" public void run() {",
93+
" f1.toString();",
94+
" }",
95+
" };",
96+
" }",
97+
"}")
98+
.doTest();
99+
}
100+
101+
@Test
102+
public void nestedObjects() {
103+
defaultCompilationHelper
104+
.addSourceLines(
105+
"Test.java",
106+
"package com.uber;",
107+
"import com.uber.nullaway.annotations.MonotonicNonNull;",
108+
"import org.jspecify.annotations.Nullable;",
109+
"class Test {",
110+
" class Foo {",
111+
" @MonotonicNonNull Object x;",
112+
" }",
113+
" final Foo f1 = new Foo();",
114+
" Foo f2 = new Foo(); // not final",
115+
" @Nullable Foo f3;",
116+
" void testPositive1() {",
117+
" f2.x = new Object();",
118+
" Runnable r = () -> {",
119+
" // report a bug since f2 may be overwritten",
120+
" // BUG: Diagnostic contains: dereferenced expression f2.x",
121+
" f2.x.toString();",
122+
" };",
123+
" }",
124+
" void testPositive2() {",
125+
" f3 = new Foo();",
126+
" f3.x = new Object();",
127+
" Runnable r = () -> {",
128+
" // report a bug since f3 may be overwritten",
129+
" // BUG: Diagnostic contains: dereferenced expression f3.x",
130+
" f3.x.toString();",
131+
" };",
132+
" }",
133+
" void testNegative() {",
134+
" f1.x = new Object();",
135+
" Runnable r = () -> {",
136+
" f1.x.toString();",
137+
" };",
138+
" }",
139+
"}")
140+
.doTest();
141+
}
142+
143+
@Test
144+
public void accessPathsWithMethodCalls() {
145+
defaultCompilationHelper
146+
.addSourceLines(
147+
"Test.java",
148+
"package com.uber;",
149+
"import com.uber.nullaway.annotations.MonotonicNonNull;",
150+
"import org.jspecify.annotations.Nullable;",
151+
"class Test {",
152+
" class Foo {",
153+
" @MonotonicNonNull Object x;",
154+
" }",
155+
" Foo f1 = new Foo();",
156+
" final Foo getF1() {",
157+
" return f1;",
158+
" }",
159+
" final @Nullable Foo getOther() {",
160+
" return null;",
161+
" }",
162+
" void testPositive1() {",
163+
" getF1().x = new Object();",
164+
" Runnable r = () -> {",
165+
" // BUG: Diagnostic contains: dereferenced expression",
166+
" getF1().x.toString();",
167+
" };",
168+
" }",
169+
" void testPositive2() {",
170+
" if (getOther() != null) {",
171+
" getOther().x = new Object();",
172+
" Runnable r1 = () -> {",
173+
" // getOther() should be treated as @Nullable in the lambda",
174+
" // BUG: Diagnostic contains: dereferenced expression",
175+
" getOther().toString();",
176+
" };",
177+
" Runnable r2 = () -> {",
178+
" // BUG: Diagnostic contains: dereferenced expression",
179+
" getOther().x.toString();",
180+
" };",
181+
" }",
182+
" }",
183+
"}")
184+
.doTest();
185+
}
186+
}

‎nullaway/src/test/resources/com/uber/nullaway/testdata/CheckFieldInitNegativeCases.java‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -326,6 +326,8 @@ static class MonotonicNonNullUsage {
326326

327327
@MonotonicNonNull Object f;
328328

329+
@com.uber.nullaway.annotations.MonotonicNonNull Object g;
330+
329331
MonotonicNonNullUsage() {}
330332
}
331333

0 commit comments

Comments
 (0)