Skip to content

Commit

Permalink
External Library Models: Adding support for @nullable Method paramete…
Browse files Browse the repository at this point in the history 
…rs (#1006)

With these changes we can read annotations from method parameters and
use them with our External Library Models in the following manner.

Externally annotated code example:
```java
@NullMarked
public class ParameterAnnotationExample {

    public static Integer add(Integer a, Integer b) {
        return a + b;
    }

    public static Object getNewObjectIfNull(@nullable Object object) {
        if (object == null) {
            return new Object();
        } else {
            return object;
        }
    }
}
```


```java
class Test {
    static ParameterAnnotationExample annotationExample = new ParameterAnnotationExample();
    static void testPositive() {
      // BUG: Diagnostic contains: passing @nullable parameter 'null' where @nonnull is required
      annotationExample.add(5,null);
    }
    static void testNegative() {
      annotationExample.getNewObjectIfNull(null);
    }
}
```

In order to accomplish this, we had to change the `astubx` format to use
fully-qualified names for argument and return types. This also required
changes to JarInfer. These are backward-incompatible changes, so we have
changed the magic number for `astubx` files.

---------

Co-authored-by: Manu Sridharan <[email protected]>
  • Loading branch information
@akulk022 @msridhar
akulk022 and msridhar authored Sep 23, 2024
1 parent 3418a6c commit f36ab25
Show file tree
Hide file tree
Showing 17 changed files with 713 additions and 68 deletions.
4 changes: 3 additions & 1 deletion gradle/dependencies.gradle
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ def versions = [
commonscli : "1.4",
autoValue : "1.10.2",
autoService : "1.1.1",
javaparser : "3.26.2",
]

def apt = [
Expand All @@ -75,7 +76,8 @@ def build = [
errorProneTestHelpersOld: "com.google.errorprone:error_prone_test_helpers:${oldestErrorProneVersion}",
checkerDataflow : "org.checkerframework:dataflow-nullaway:${versions.checkerFramework}",
guava : "com.google.guava:guava:30.1-jre",
javaparser : "com.github.javaparser:javaparser-core:3.26.0",
javaparser : "com.github.javaparser:javaparser-core:${versions.javaparser}",
javaparserSymbolSolver : "com.github.javaparser:javaparser-symbol-solver-core:${versions.javaparser}",
javaxValidation : "javax.validation:validation-api:2.0.1.Final",
jspecify : "org.jspecify:jspecify:1.0.0",
commonsIO : "commons-io:commons-io:2.11.0",
Expand Down
26 changes: 3 additions & 23 deletions ...jar-infer-lib/src/main/java/com/uber/nullaway/jarinfer/DefinitelyDerefedParamsDriver.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import com.ibm.wala.classLoader.PhantomClass;
import com.ibm.wala.classLoader.ShrikeCTMethod;
import com.ibm.wala.core.util.config.AnalysisScopeReader;
import com.ibm.wala.core.util.strings.StringStuff;
import com.ibm.wala.core.util.warnings.Warnings;
import com.ibm.wala.ipa.callgraph.AnalysisCache;
import com.ibm.wala.ipa.callgraph.AnalysisCacheImpl;
Expand Down Expand Up @@ -456,6 +457,7 @@ private void writeModel(DataOutputStream out) throws IOException {
packageAnnotations,
typeAnnotations,
methodRecords,
Collections.emptySet(),
Collections.emptyMap());
}

Expand Down Expand Up @@ -527,28 +529,6 @@ private static String getAstubxSignature(IMethod mtd) {
* @return String Unqualified type name.
*/
private static String getSimpleTypeName(TypeReference typ) {
ImmutableMap<String, String> mapFullTypeName =
ImmutableMap.<String, String>builder()
.put("B", "byte")
.put("C", "char")
.put("D", "double")
.put("F", "float")
.put("I", "int")
.put("J", "long")
.put("S", "short")
.put("Z", "boolean")
.build();
if (typ.isArrayType()) {
return "Array";
}
String typName = typ.getName().toString();
if (typName.startsWith("L")) {
typName = typName.split("<")[0].substring(1); // handle generics
typName = typName.substring(typName.lastIndexOf('/') + 1); // get unqualified name
typName = typName.substring(typName.lastIndexOf('$') + 1); // handle inner classes
} else {
typName = mapFullTypeName.get(typName);
}
return typName;
return StringStuff.jvmToBinaryName(typ.getName().toString());
}
}
56 changes: 48 additions & 8 deletions jar-infer/jar-infer-lib/src/test/java/com/uber/nullaway/jarinfer/JarInferTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,6 @@

/** Unit tests for {@link com.uber.nullaway.jarinfer}. */
@RunWith(JUnit4.class)
@SuppressWarnings("CheckTestExtendsBaseClass")
public class JarInferTest {

@Rule public TemporaryFolder temporaryFolder = new TemporaryFolder();
Expand Down Expand Up @@ -206,8 +205,8 @@ public void toyStatic() throws Exception {
"toys",
"Test",
ImmutableMap.of(
"toys.Test:void test(String, Foo, Bar)", Sets.newHashSet(0, 2),
"toys.Foo:boolean run(String)", Sets.newHashSet(1)),
"toys.Test:void test(java.lang.String, toys.Foo, toys.Bar)", Sets.newHashSet(0, 2),
"toys.Foo:boolean run(java.lang.String)", Sets.newHashSet(1)),
"class Foo {",
" private String foo;",
" public Foo(String str) {",
Expand Down Expand Up @@ -267,7 +266,8 @@ public void toyNonStatic() throws Exception {
"toyNonStatic",
"toys",
"Foo",
ImmutableMap.of("toys.Foo:void test(String, String)", Sets.newHashSet(1)),
ImmutableMap.of(
"toys.Foo:void test(java.lang.String, java.lang.String)", Sets.newHashSet(1)),
"class Foo {",
" private String foo;",
" public Foo(String str) {",
Expand Down Expand Up @@ -303,7 +303,9 @@ public void toyNullTestAPI() throws Exception {
"toyNullTestAPI",
"toys",
"Foo",
ImmutableMap.of("toys.Foo:void test(String, String, String)", Sets.newHashSet(1, 3)),
ImmutableMap.of(
"toys.Foo:void test(java.lang.String, java.lang.String, java.lang.String)",
Sets.newHashSet(1, 3)),
"import com.google.common.base.Preconditions;",
"import java.util.Objects;",
"import org.junit.Assert;",
Expand Down Expand Up @@ -332,7 +334,9 @@ public void toyConditionalFlow() throws Exception {
"toyNullTestAPI",
"toys",
"Foo",
ImmutableMap.of("toys.Foo:void test(String, String, String)", Sets.newHashSet(1, 2)),
ImmutableMap.of(
"toys.Foo:void test(java.lang.String, java.lang.String, java.lang.String)",
Sets.newHashSet(1, 2)),
"import com.google.common.base.Preconditions;",
"import java.util.Objects;",
"import org.junit.Assert;",
Expand Down Expand Up @@ -362,7 +366,8 @@ public void toyConditionalFlow2() throws Exception {
"toys",
"Foo",
ImmutableMap.of(
"toys.Foo:void test(Object, Object, Object, Object)", Sets.newHashSet(1, 4)),
"toys.Foo:void test(java.lang.Object, java.lang.Object, java.lang.Object, java.lang.Object)",
Sets.newHashSet(1, 4)),
"import com.google.common.base.Preconditions;",
"import java.util.Objects;",
"import org.junit.Assert;",
Expand Down Expand Up @@ -401,7 +406,8 @@ public void toyReassigningTest() throws Exception {
"toyNullTestAPI",
"toys",
"Foo",
ImmutableMap.of("toys.Foo:void test(String, String)", Sets.newHashSet(1)),
ImmutableMap.of(
"toys.Foo:void test(java.lang.String, java.lang.String)", Sets.newHashSet(1)),
"import com.google.common.base.Preconditions;",
"import java.util.Objects;",
"import org.junit.Assert;",
Expand All @@ -421,6 +427,40 @@ public void toyReassigningTest() throws Exception {
"}");
}

@Test
public void testObjectArray() throws Exception {
testTemplate(
"testObjectArray",
"arrays",
"TestArray",
ImmutableMap.of(
"arrays.TestArray:java.lang.String foo(java.lang.Object[])", Sets.newHashSet(0)),
"class TestArray {",
" public static String foo(Object[] o) {",
" return o.toString();",
" }",
"}");
}

@Test
public void testGenericMethod() throws Exception {
testTemplate(
"testGenericMethod",
"generic",
"TestGeneric",
ImmutableMap.of(
"generic.TestGeneric:java.lang.String foo(java.lang.Object)", Sets.newHashSet(1)),
"public class TestGeneric<T> {",
" public String foo(T t) {",
" return t.toString();",
" }",
" public static void main(String arg[]) {",
" TestGeneric<String> tg = new TestGeneric<String>();",
" System.out.println(tg.foo(\"generic test\"));",
" }",
"}");
}

@Test
public void toyJARAnnotatingClasses() throws Exception {
testAnnotationInJarTemplate(
Expand Down
49 changes: 49 additions & 0 deletions ...ay-integration-test/src/test/java/com/uber/nullaway/jarinfer/JarInferIntegrationTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,55 @@ public void jarinferLoadStubsTest() {
.doTest();
}

@Test
public void arrayTest() {
compilationHelper
.setArgs(
Arrays.asList(
"-d",
temporaryFolder.getRoot().getAbsolutePath(),
"-XepOpt:NullAway:AnnotatedPackages=com.uber",
"-XepOpt:NullAway:JarInferEnabled=true",
"-XepOpt:NullAway:UnannotatedSubPackages=com.uber.nullaway.[a-zA-Z0-9.]+.unannotated"))
.addSourceLines(
"Test.java",
"package com.uber;",
"import org.jspecify.annotations.Nullable;",
"import com.uber.nullaway.jarinfer.toys.unannotated.Toys;",
"class Test {",
" void test1(Object @Nullable [] o) {",
" // BUG: Diagnostic contains: passing @Nullable parameter 'o'",
" Toys.testArray(o);",
" }",
"}")
.doTest();
}

@Test
public void genericsTest() {
compilationHelper
.setArgs(
Arrays.asList(
"-d",
temporaryFolder.getRoot().getAbsolutePath(),
"-XepOpt:NullAway:AnnotatedPackages=com.uber",
"-XepOpt:NullAway:JarInferEnabled=true",
"-XepOpt:NullAway:UnannotatedSubPackages=com.uber.nullaway.[a-zA-Z0-9.]+.unannotated"))
.addSourceLines(
"Test.java",
"package com.uber;",
"import org.jspecify.annotations.Nullable;",
"import com.uber.nullaway.jarinfer.toys.unannotated.Toys;",
"class Test {",
" void test1() {",
" Toys.Generic<String> g = new Toys.Generic<>();",
" // BUG: Diagnostic contains: passing @Nullable parameter 'null'",
" g.getString(null);",
" }",
"}")
.doTest();
}

@Test
public void jarinferNullableReturnsTest() {
compilationHelper
Expand Down
11 changes: 11 additions & 0 deletions ...est-java-lib-jarinfer/src/main/java/com/uber/nullaway/jarinfer/toys/unannotated/Toys.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,17 @@ public static void test1(@ExpectNonnull String s, String t, String u) {
}
}

@SuppressWarnings("ArrayHashCode")
public static int testArray(Object[] o) {
return o.hashCode();
}

public static class Generic<T> {
public String getString(T t) {
return t.toString();
}
}

public static void main(String arg[]) throws java.io.IOException {
String s = "test string...";
Foo f = new Foo("let's");
Expand Down
108 changes: 108 additions & 0 deletions ...ntegration-test/src/test/java/com/uber/nullaway/libmodel/LibraryModelIntegrationTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -179,4 +179,112 @@ public void libraryModelNullableUpperBoundsWithoutJarInferTest() {
"}")
.doTest();
}

@Test
public void libraryModelDefaultParameterNullabilityTest() {
compilationHelper
.setArgs(
Arrays.asList(
"-d",
temporaryFolder.getRoot().getAbsolutePath(),
"-XepOpt:NullAway:AnnotatedPackages=com.uber",
"-XepOpt:NullAway:JSpecifyMode=true",
"-XepOpt:NullAway:JarInferEnabled=true",
"-XepOpt:NullAway:JarInferUseReturnAnnotations=true"))
.addSourceLines(
"Test.java",
"package com.uber;",
"import com.test.ParameterAnnotationExample;",
"class Test {",
" static ParameterAnnotationExample annotationExample = new ParameterAnnotationExample();",
" static void testPositive() {",
" // BUG: Diagnostic contains: passing @Nullable parameter 'null' where @NonNull is required",
" annotationExample.add(5,null);",
" }",
"}")
.doTest();
}

@Test
public void libraryModelParameterNullabilityTest() {
compilationHelper
.setArgs(
Arrays.asList(
"-d",
temporaryFolder.getRoot().getAbsolutePath(),
"-XepOpt:NullAway:AnnotatedPackages=com.uber",
"-XepOpt:NullAway:JSpecifyMode=true",
"-XepOpt:NullAway:JarInferEnabled=true",
"-XepOpt:NullAway:JarInferUseReturnAnnotations=true"))
.addSourceLines(
"Test.java",
"package com.uber;",
"import com.test.ParameterAnnotationExample;",
"class Test {",
" static ParameterAnnotationExample annotationExample = new ParameterAnnotationExample();",
" static void testPositive() {",
" // BUG: Diagnostic contains: passing @Nullable parameter 'null' where @NonNull is required",
" annotationExample.printObjectString(null);",
" }",
" static void testNegative() {",
" annotationExample.getNewObjectIfNull(null);",
" }",
"}")
.doTest();
}

@Test
public void nullableArrayTest() {
compilationHelper
.setArgs(
Arrays.asList(
"-d",
temporaryFolder.getRoot().getAbsolutePath(),
"-XepOpt:NullAway:AnnotatedPackages=com.uber",
"-XepOpt:NullAway:JSpecifyMode=true",
"-XepOpt:NullAway:JarInferEnabled=true",
"-XepOpt:NullAway:JarInferUseReturnAnnotations=true"))
.addSourceLines(
"Test.java",
"package com.uber;",
"import com.test.ParameterAnnotationExample;",
"class Test {",
" static void testPositive() {",
" // BUG: Diagnostic contains: passing @Nullable parameter 'null' where @NonNull is required",
" ParameterAnnotationExample.takesNonNullArray(null);",
" }",
" static void testNegative() {",
" ParameterAnnotationExample.takesNullArray(null);",
" }",
"}")
.doTest();
}

@Test
public void genericParameterTest() {
compilationHelper
.setArgs(
Arrays.asList(
"-d",
temporaryFolder.getRoot().getAbsolutePath(),
"-XepOpt:NullAway:AnnotatedPackages=com.uber",
"-XepOpt:NullAway:JSpecifyMode=true",
"-XepOpt:NullAway:JarInferEnabled=true",
"-XepOpt:NullAway:JarInferUseReturnAnnotations=true"))
.addSourceLines(
"Test.java",
"package com.uber;",
"import com.test.ParameterAnnotationExample;",
"class Test {",
" static ParameterAnnotationExample.Generic<String> ex = new ParameterAnnotationExample.Generic<>();",
" static void testPositive() {",
" // BUG: Diagnostic contains: passing @Nullable parameter 'null' where @NonNull is required",
" ex.printObjectString(null);",
" }",
" static void testNegative() {",
" ex.getString(null);",
" }",
"}")
.doTest();
}
}
1 change: 1 addition & 0 deletions library-model/library-model-generator/build.gradle
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ plugins {
dependencies {
implementation deps.build.guava
implementation deps.build.javaparser
implementation deps.build.javaparserSymbolSolver
compileOnly deps.apt.autoValueAnnot
annotationProcessor deps.apt.autoValue

Expand Down
Loading

0 comments on commit f36ab25

Please sign in to comment.