Skip to content

Commit 585b40e

Browse files
haewifulmsridhar
andauthored
Method name parsing in ExternalStubxLibraryModels class is missing a corner case (uber#1344)
When loading external library models through the `ExternalStubxLibraryModels` class, it previously didn't parse the method name for methods with wildcard types properly. This PR fixes the method parsing so that it will work for those cases. The unit test for this is located in `java/com/uber/nullaway/jdkannotations/JDKIntegrationTest.java` and is `libraryLoadMethodParser()`. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added a new public API method returning a parameterized List (annotated and unannotated variants). * **Tests** * Added an integration test covering library parser behavior for return-value handling. * **Bug Fixes** * Improved library-model method-signature parsing and normalization to handle irregular spacing/formatting. <sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub> <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Manu Sridharan <msridhar@gmail.com>
1 parent feec9d9 commit 585b40e

File tree

4 files changed

+45
-8
lines changed

4 files changed

+45
-8
lines changed

jdk-annotations/jdk-integration-test/src/test/java/com/uber/nullaway/jdkannotations/JDKIntegrationTest.java

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -329,4 +329,28 @@ public void genericParameterTest() {
329329
"}")
330330
.doTest();
331331
}
332+
333+
@Test
334+
public void libraryLoadMethodParserReturn() {
335+
compilationHelper
336+
.setArgs(
337+
Arrays.asList(
338+
"-d",
339+
temporaryFolder.getRoot().getAbsolutePath(),
340+
"-XepOpt:NullAway:AnnotatedPackages=com.uber",
341+
"-XepOpt:NullAway:JarInferEnabled=true"))
342+
.addSourceLines(
343+
"Test.java",
344+
"package com.uber;",
345+
"import org.jspecify.annotations.Nullable;",
346+
"import com.uber.nullaway.jdkannotations.ReturnAnnotation;",
347+
"import java.util.List;",
348+
"class Test<T> {",
349+
" void testCall() {",
350+
" // BUG: Diagnostic contains: dereferenced expression ReturnAnnotation.getList(6, null) is @Nullable",
351+
" ReturnAnnotation.getList(6, null).isEmpty();",
352+
" }",
353+
"}")
354+
.doTest();
355+
}
332356
}

jdk-annotations/test-annotated/src/main/java/com/uber/nullaway/jdkannotations/ReturnAnnotation.java

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
package com.uber.nullaway.jdkannotations;
22

3+
import java.util.List;
34
import java.util.Locale;
45
import org.jspecify.annotations.NullMarked;
56
import org.jspecify.annotations.Nullable;
@@ -76,4 +77,8 @@ public static Object getNewObjectIfNull(@Nullable Object object) {
7677
returnNullableGenericContainingNullable() {
7778
return null;
7879
}
80+
81+
public static @Nullable List<? super String> getList(Integer i, @Nullable Character c) {
82+
return null;
83+
}
7984
}

jdk-annotations/test-unannotated/src/main/java/com/uber/nullaway/jdkannotations/ReturnAnnotation.java

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
package com.uber.nullaway.jdkannotations;
22

3+
import java.util.List;
34
import java.util.Locale;
45

56
public class ReturnAnnotation {
@@ -66,4 +67,8 @@ public static UpperBoundExample<String> returnNonNullGenericContainingNullable()
6667
public static UpperBoundExample<String> returnNullableGenericContainingNullable() {
6768
return null;
6869
}
70+
71+
public static List<? super String> getList(Integer i, Character c) {
72+
return null;
73+
}
6974
}

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

Lines changed: 11 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@
2828
import static com.uber.nullaway.Nullness.NULLABLE;
2929

3030
import com.google.common.base.Preconditions;
31+
import com.google.common.base.Verify;
3132
import com.google.common.collect.ImmutableList;
3233
import com.google.common.collect.ImmutableSet;
3334
import com.google.common.collect.ImmutableSetMultimap;
@@ -1441,8 +1442,7 @@ public ImmutableSetMultimap<MethodRef, Integer> explicitlyNullableParameters() {
14411442
String className = outerEntry.getKey();
14421443
for (Map.Entry<String, Map<Integer, Set<String>>> innerEntry :
14431444
outerEntry.getValue().entrySet()) {
1444-
String methodNameAndSignature =
1445-
innerEntry.getKey().substring(innerEntry.getKey().indexOf(" ") + 1);
1445+
String methodNameAndSignature = getMethodNameAndSignature(innerEntry.getKey());
14461446
for (Map.Entry<Integer, Set<String>> entry : innerEntry.getValue().entrySet()) {
14471447
Integer index = entry.getKey();
14481448
if (index >= 0 && entry.getValue().stream().anyMatch(a -> a.contains("Nullable"))) {
@@ -1463,9 +1463,7 @@ public ImmutableSetMultimap<MethodRef, Integer> nonNullParameters() {
14631463
for (String className : argAnnotCache.keySet()) {
14641464
for (Map.Entry<String, Map<Integer, Set<String>>> methodEntry :
14651465
argAnnotCache.get(className).entrySet()) {
1466-
String methodNameAndSignature =
1467-
methodEntry.getKey().substring(methodEntry.getKey().indexOf(" ") + 1);
1468-
1466+
String methodNameAndSignature = getMethodNameAndSignature(methodEntry.getKey());
14691467
for (Map.Entry<Integer, Set<String>> argEntry : methodEntry.getValue().entrySet()) {
14701468
Integer index = argEntry.getKey();
14711469
if (index >= 0) {
@@ -1491,6 +1489,13 @@ public ImmutableSetMultimap<MethodRef, Integer> nonNullParameters() {
14911489
return mapBuilder.build();
14921490
}
14931491

1492+
private static String getMethodNameAndSignature(String methodInfo) {
1493+
int openParenIndex = methodInfo.indexOf('(');
1494+
Verify.verify(openParenIndex != -1, "Malformed method info: %s", methodInfo);
1495+
int methodNameIndex = methodInfo.lastIndexOf(' ', openParenIndex) + 1;
1496+
return methodInfo.substring(methodNameIndex);
1497+
}
1498+
14941499
@Override
14951500
public ImmutableSetMultimap<MethodRef, Integer> nullImpliesTrueParameters() {
14961501
return ImmutableSetMultimap.of();
@@ -1512,9 +1517,7 @@ public ImmutableSet<MethodRef> nullableReturns() {
15121517
for (String className : argAnnotCache.keySet()) {
15131518
for (Map.Entry<String, Map<Integer, Set<String>>> methodEntry :
15141519
argAnnotCache.get(className).entrySet()) {
1515-
String methodNameAndSignature =
1516-
methodEntry.getKey().substring(methodEntry.getKey().indexOf(" ") + 1);
1517-
1520+
String methodNameAndSignature = getMethodNameAndSignature(methodEntry.getKey());
15181521
for (Map.Entry<Integer, Set<String>> argEntry : methodEntry.getValue().entrySet()) {
15191522
Integer index = argEntry.getKey();
15201523
if (index == -1) {

0 commit comments

Comments
 (0)