Skip to content

Commit f40a9fa

Browse files
msridharmsridhar
authored
Add more modeling for Optional (#1613)
Fixes #1611 We manually write more models for `Optional`, so we can address before fully fixing #950. Note that we do not model `orElseGet` as in JSpecify JDK right now due to false positives; see #1616. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Summary by CodeRabbit * **Bug Fixes** * Improved null-safety handling for `java.util.Optional` operations, including more accurate nullability propagation and diagnostics for `ofNullable`, `map`, `orElse`, and `equals`. * **Tests** * Expanded JUnit test coverage for `Optional` combined with jspecify nullability, adding both positive compilation checks and new negative assertions (with one temporarily skipped scenario). <!-- end of auto-generated comment: release notes by coderabbit.ai -->
1 parent 98096d7 commit f40a9fa

2 files changed

Lines changed: 163 additions & 8 deletions

File tree

‎nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java‎

Lines changed: 44 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@
2828
import static com.uber.nullaway.Nullness.NULLABLE;
2929
import static com.uber.nullaway.librarymodel.NestedAnnotationInfo.TypePathEntry.Kind.ARRAY_ELEMENT;
3030
import static com.uber.nullaway.librarymodel.NestedAnnotationInfo.TypePathEntry.Kind.TYPE_ARGUMENT;
31+
import static com.uber.nullaway.librarymodel.NestedAnnotationInfo.TypePathEntry.Kind.WILDCARD_BOUND;
3132

3233
import com.google.common.base.Preconditions;
3334
import com.google.common.base.Verify;
@@ -750,6 +751,9 @@ private static class DefaultLibraryModels implements LibraryModels {
750751
"android.view.GestureDetector.OnGestureListener",
751752
"onScroll(android.view.MotionEvent,android.view.MotionEvent,float,float)"),
752753
0)
754+
.put(methodRef("java.util.Optional", "<T>ofNullable(T)"), 0)
755+
.put(methodRef("java.util.Optional", "orElse(T)"), 0)
756+
.put(methodRef("java.util.Optional", "equals(java.lang.Object)"), 0)
753757
.build();
754758

755759
private static final ImmutableSetMultimap<MethodRef, Integer> NON_NULL_PARAMETERS =
@@ -980,6 +984,11 @@ private static class DefaultLibraryModels implements LibraryModels {
980984
.add(methodRef("android.view.View", "getHandler()"))
981985
.add(methodRef("android.webkit.WebView", "getUrl()"))
982986
.add(methodRef("android.widget.TextView", "getLayout()"))
987+
.add(methodRef("java.util.Optional", "orElse(T)"))
988+
// https://github.com/uber/NullAway/issues/1616
989+
// .add(
990+
// methodRef(
991+
// "java.util.Optional", "orElseGet(java.util.function.Supplier<? extends T>)"))
983992
.add(methodRef("java.lang.System", "console()"))
984993
.build();
985994

@@ -1055,21 +1064,48 @@ private static class DefaultLibraryModels implements LibraryModels {
10551064
private static final ImmutableMap<
10561065
MethodRef, ImmutableSetMultimap<Integer, NestedAnnotationInfo>>
10571066
NESTED_ANNOTATIONS_FOR_METHODS =
1058-
ImmutableMap.of(
1059-
methodRef(
1060-
"java.util.concurrent.atomic.AtomicReferenceFieldUpdater",
1061-
"<U,W>newUpdater(java.lang.Class<U>,java.lang.Class<W>,java.lang.String)"),
1062-
// turns Class<W> into Class<@NonNull W>
1067+
new ImmutableMap.Builder<
1068+
MethodRef, ImmutableSetMultimap<Integer, NestedAnnotationInfo>>()
1069+
.put(
1070+
methodRef(
1071+
"java.util.concurrent.atomic.AtomicReferenceFieldUpdater",
1072+
"<U,W>newUpdater(java.lang.Class<U>,java.lang.Class<W>,java.lang.String)"),
1073+
// turns Class<W> into Class<@NonNull W>
1074+
ImmutableSetMultimap.of(
1075+
1,
1076+
new NestedAnnotationInfo(
1077+
Annotation.NONNULL,
1078+
ImmutableList.of(
1079+
new NestedAnnotationInfo.TypePathEntry(TYPE_ARGUMENT, 0)))))
1080+
.put(
1081+
methodRef(
1082+
"java.util.Optional",
1083+
"<U>map(java.util.function.Function<? super T,? extends U>)"),
1084+
ImmutableSetMultimap.of(
1085+
0,
1086+
new NestedAnnotationInfo(
1087+
Annotation.NULLABLE,
1088+
ImmutableList.of(
1089+
new NestedAnnotationInfo.TypePathEntry(TYPE_ARGUMENT, 1),
1090+
new NestedAnnotationInfo.TypePathEntry(WILDCARD_BOUND, 0)))))
1091+
// https://github.com/uber/NullAway/issues/1616
1092+
/*.put(
1093+
methodRef(
1094+
"java.util.Optional",
1095+
"orElseGet(java.util.function.Supplier<? extends T>)"),
10631096
ImmutableSetMultimap.of(
1064-
1,
1097+
0,
10651098
new NestedAnnotationInfo(
1066-
Annotation.NONNULL,
1099+
Annotation.NULLABLE,
10671100
ImmutableList.of(
1068-
new NestedAnnotationInfo.TypePathEntry(TYPE_ARGUMENT, 0)))));
1101+
new NestedAnnotationInfo.TypePathEntry(TYPE_ARGUMENT, 0),
1102+
new NestedAnnotationInfo.TypePathEntry(WILDCARD_BOUND, 0)))))*/
1103+
.build();
10691104

10701105
private static final ImmutableSet<String> NULLMARKED_CLASSES =
10711106
new ImmutableSet.Builder<String>()
10721107
.add("java.util.function.Function")
1108+
.add("java.util.Optional")
10731109
.add("java.util.concurrent.atomic.AtomicReference")
10741110
.add("java.util.concurrent.atomic.AtomicReferenceFieldUpdater")
10751111
.build();

‎nullaway/src/test/java/com/uber/nullaway/jspecify/JSpecifyLibraryModelsTests.java‎

Lines changed: 119 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
import com.uber.nullaway.NullAwayTestsBase;
55
import com.uber.nullaway.generics.JSpecifyJavacConfig;
66
import java.util.Arrays;
7+
import org.junit.Ignore;
78
import org.junit.Test;
89

910
public class JSpecifyLibraryModelsTests extends NullAwayTestsBase {
@@ -32,6 +33,124 @@ void testPositive() {
3233
.doTest();
3334
}
3435

36+
@Test
37+
public void optionalMethods() {
38+
makeHelper()
39+
.addSourceLines(
40+
"Test.java",
41+
"""
42+
import java.util.Optional;
43+
import java.util.function.Function;
44+
import org.jspecify.annotations.*;
45+
46+
@NullMarked
47+
class Test {
48+
Optional<String> ofNullableAcceptsNullable(@Nullable String value) {
49+
return Optional.ofNullable(value);
50+
}
51+
52+
Optional<String> mapCanReturnNullable(Optional<String> value) {
53+
Function<String, @Nullable String> mapper = unused -> null;
54+
return value.map(mapper);
55+
}
56+
57+
void equalsAcceptsNullable(Optional<String> value) {
58+
value.equals(null);
59+
}
60+
61+
void ofRejectsNull() {
62+
// BUG: Diagnostic contains: passing @Nullable parameter 'null' where @NonNull is required
63+
Optional.of(null);
64+
}
65+
66+
void orElseReturnsNullable(Optional<String> value) {
67+
// BUG: Diagnostic contains: dereferenced expression 'value.orElse(null)' is @Nullable
68+
value.orElse(null).hashCode();
69+
}
70+
}
71+
""")
72+
.doTest();
73+
}
74+
75+
@Test
76+
public void optionalMapWithLambdaArg() {
77+
makeHelper()
78+
.addSourceLines(
79+
"Test.java",
80+
"""
81+
import java.util.Optional;
82+
import org.jspecify.annotations.*;
83+
84+
@NullMarked
85+
class Test {
86+
Optional<String> mapCanReturnNullable(Optional<String> value) {
87+
return value.map(unused -> null);
88+
}
89+
}
90+
""")
91+
.doTest();
92+
}
93+
94+
@Ignore("https://github.com/uber/NullAway/issues/1616")
95+
@Test
96+
public void optionalOrElseGet() {
97+
makeHelper()
98+
.addSourceLines(
99+
"Test.java",
100+
"""
101+
import java.util.Optional;
102+
import java.util.function.Function;
103+
import org.jspecify.annotations.*;
104+
105+
@NullMarked
106+
class Test {
107+
void orElseGetReturnsNullable(Optional<String> value) {
108+
// BUG: Diagnostic contains: dereferenced expression 'value.orElseGet(() -> null)' is @Nullable
109+
value.orElseGet(() -> null).hashCode();
110+
}
111+
}
112+
""")
113+
.doTest();
114+
}
115+
116+
@Test
117+
public void issue1611() {
118+
makeHelper()
119+
.addSourceLines(
120+
"Test.java",
121+
"""
122+
import java.util.Optional;
123+
import org.jspecify.annotations.*;
124+
125+
@NullMarked
126+
class Test {
127+
static Optional<String> getKey(@Nullable String key) {
128+
return Optional.ofNullable(key).or(() -> Optional.ofNullable(System.getenv("DEFAULT_KEY")));
129+
}
130+
}
131+
""")
132+
.doTest();
133+
}
134+
135+
@Test
136+
public void optionalOfNullableFilterIssue1597() {
137+
makeHelper()
138+
.addSourceLines(
139+
"Repro.java",
140+
"""
141+
import java.util.Optional;
142+
import org.jspecify.annotations.*;
143+
144+
@NullMarked
145+
public record Repro(@Nullable String name) {
146+
public Optional<String> getNameOpt() {
147+
return Optional.ofNullable(name).filter(it -> true);
148+
}
149+
}
150+
""")
151+
.doTest();
152+
}
153+
35154
private CompilationTestHelper makeHelper() {
36155
return makeTestHelperWithArgs(
37156
JSpecifyJavacConfig.withJSpecifyModeArgs(

0 commit comments

Comments
 (0)