Skip to content

Commit d427643

Browse files
authored
Fix bug with library-modeled return types, method references, and streams (#1706)
In `LibraryModelsHandler.onOverrideMethodType` we neglected to apply top-level `@Nullable` annotations from library models. This impacted cases like the new `streamMapNullableMapGetMethodReferenceTest`. While investigating this issue, I realized that the test from #1462 now passes, along with a similar test that actually uses the library types, so enable those. Fixes #1462 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved handling of nullable return values and top-level annotations in modeled library methods. * Enhanced nullability analysis for stream transformations and method references. * **Tests** * Added coverage for detecting unsafe dereferences of nullable stream results. * Added coverage for safely filtering nullable map lookups before collection. * Enabled an existing stream nullability test. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
1 parent 6329102 commit d427643

3 files changed

Lines changed: 71 additions & 10 deletions

File tree

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

Lines changed: 20 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -498,10 +498,10 @@ public boolean isSingleArgNullImpliesFalseMethod(
498498
}
499499

500500
/**
501-
* Updates method types based on top-level parameter and nested annotation library models. For
502-
* now, this method is only used in JSpecify mode, primarily for its handling of nested
503-
* annotations. Outside of JSpecify mode, other handler methods are available for reasoning about
504-
* top-level annotations only.
501+
* Updates method types based on top-level parameter and return annotations and nested annotation
502+
* library models. For now, this method is only used in JSpecify mode, primarily for its handling
503+
* of nested annotations. Outside of JSpecify mode, other handler methods are available for
504+
* reasoning about top-level annotations only.
505505
*/
506506
@Override
507507
@SuppressWarnings({"ReferenceEquality", "TypeEquals"})
@@ -516,9 +516,17 @@ public Type.MethodType onOverrideMethodType(
516516
config.isJSpecifyMode()
517517
? optimizedLibraryModels.explicitlyNullableParameters(methodSymbol)
518518
: ImmutableSet.of();
519+
boolean isMethodUnannotated =
520+
getCodeAnnotationInfo(state.context)
521+
.isSymbolUnannotated(methodSymbol, this.config, mainHandler);
522+
boolean modeledNullableReturn =
523+
optimizedLibraryModels.hasNullableReturn(
524+
methodSymbol, state.getTypes(), isMethodUnannotated);
519525
ImmutableSetMultimap<Integer, NestedAnnotationInfo> nestedAnnotations =
520526
optimizedLibraryModels.nestedAnnotationsForMethods(methodSymbol);
521-
if (explicitlyNullableParameters.isEmpty() && nestedAnnotations.isEmpty()) {
527+
if (explicitlyNullableParameters.isEmpty()
528+
&& !modeledNullableReturn
529+
&& nestedAnnotations.isEmpty()) {
522530
return methodType;
523531
}
524532
// update argument types, tracking if anything changed
@@ -546,10 +554,13 @@ public Type.MethodType onOverrideMethodType(
546554
// update return type
547555
Type returnType = methodType.restype;
548556
ImmutableSet<NestedAnnotationInfo> returnAnnotations = nestedAnnotations.get(-1);
549-
Type updatedReturnType =
550-
returnAnnotations.isEmpty()
551-
? returnType
552-
: applyNestedAnnotations(returnType, returnAnnotations, state);
557+
Type updatedReturnType = returnType;
558+
if (modeledNullableReturn) {
559+
updatedReturnType = applyTopLevelNullableAnnotation(updatedReturnType, state);
560+
}
561+
if (!returnAnnotations.isEmpty()) {
562+
updatedReturnType = applyNestedAnnotations(updatedReturnType, returnAnnotations, state);
563+
}
553564
if (updatedReturnType != returnType) {
554565
changed = true;
555566
}

nullaway/src/test/java/com/uber/nullaway/JSpecifyJDKModelsTest.java

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -160,6 +160,57 @@ void test() {
160160
.doTest();
161161
}
162162

163+
@Test
164+
public void streamMapNullableTest() {
165+
makeHelper()
166+
.addSourceLines(
167+
"Test.java",
168+
"""
169+
import org.jspecify.annotations.*;
170+
import java.util.*;
171+
@NullMarked
172+
class Test {
173+
static @Nullable String mapToNull(String s) {
174+
return null;
175+
}
176+
static void test(List<String> list) {
177+
list.stream().map(Test::mapToNull).forEach(s -> {
178+
// BUG: Diagnostic contains: dereferenced expression 's' is @Nullable
179+
s.hashCode();
180+
});
181+
}
182+
}""")
183+
.doTest();
184+
}
185+
186+
@Test
187+
public void streamMapNullableMapGetMethodReferenceTest() {
188+
makeHelper()
189+
.addSourceLines(
190+
"Test.java",
191+
"""
192+
import org.jspecify.annotations.NullMarked;
193+
import java.util.List;
194+
import java.util.Map;
195+
import java.util.Objects;
196+
@NullMarked
197+
class Test {
198+
static void unsafeGetValues(Map<String, String> values, List<String> keys) {
199+
keys.stream().map(values::get).forEach(value -> {
200+
// BUG: Diagnostic contains: dereferenced expression 'value' is @Nullable
201+
value.hashCode();
202+
});
203+
}
204+
static List<String> getValues(Map<String, String> values, List<String> keys) {
205+
return keys.stream()
206+
.map(values::get)
207+
.filter(Objects::nonNull)
208+
.toList();
209+
}
210+
}""")
211+
.doTest();
212+
}
213+
163214
private CompilationTestHelper makeHelper() {
164215
return makeTestHelperWithArgs(
165216
JSpecifyJavacConfig.withJSpecifyModeArgs(List.of("-XepOpt:NullAway:OnlyNullMarked=true")));

nullaway/src/test/java/com/uber/nullaway/jspecify/GenericMethodLambdaOrMethodRefArgTests.java

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1027,7 +1027,6 @@ static void test() {
10271027
.doTest();
10281028
}
10291029

1030-
@Ignore("https://github.com/uber/NullAway/issues/1462")
10311030
@Test
10321031
public void streamMapNullableTest() {
10331032
makeHelper()

0 commit comments

Comments
 (0)