diff --git a/gradle/dependencies.gradle b/gradle/dependencies.gradle index 87d8a43d0d..b511bf7d47 100755 --- a/gradle/dependencies.gradle +++ b/gradle/dependencies.gradle @@ -52,6 +52,7 @@ def versions = [ commonscli : "1.4", autoValue : "1.10.2", autoService : "1.1.1", + javaparser : "3.26.2", ] def apt = [ @@ -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", diff --git a/jar-infer/jar-infer-lib/src/main/java/com/uber/nullaway/jarinfer/DefinitelyDerefedParamsDriver.java b/jar-infer/jar-infer-lib/src/main/java/com/uber/nullaway/jarinfer/DefinitelyDerefedParamsDriver.java index d709e09a50..17f1f72a9f 100644 --- a/jar-infer/jar-infer-lib/src/main/java/com/uber/nullaway/jarinfer/DefinitelyDerefedParamsDriver.java +++ b/jar-infer/jar-infer-lib/src/main/java/com/uber/nullaway/jarinfer/DefinitelyDerefedParamsDriver.java @@ -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; @@ -456,6 +457,7 @@ private void writeModel(DataOutputStream out) throws IOException { packageAnnotations, typeAnnotations, methodRecords, + Collections.emptySet(), Collections.emptyMap()); } @@ -527,28 +529,6 @@ private static String getAstubxSignature(IMethod mtd) { * @return String Unqualified type name. */ private static String getSimpleTypeName(TypeReference typ) { - ImmutableMap mapFullTypeName = - ImmutableMap.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()); } } diff --git a/jar-infer/jar-infer-lib/src/test/java/com/uber/nullaway/jarinfer/JarInferTest.java b/jar-infer/jar-infer-lib/src/test/java/com/uber/nullaway/jarinfer/JarInferTest.java index 4188054d4f..e1fe8495d0 100644 --- a/jar-infer/jar-infer-lib/src/test/java/com/uber/nullaway/jarinfer/JarInferTest.java +++ b/jar-infer/jar-infer-lib/src/test/java/com/uber/nullaway/jarinfer/JarInferTest.java @@ -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(); @@ -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) {", @@ -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) {", @@ -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;", @@ -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;", @@ -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;", @@ -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;", @@ -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 {", + " public String foo(T t) {", + " return t.toString();", + " }", + " public static void main(String arg[]) {", + " TestGeneric tg = new TestGeneric();", + " System.out.println(tg.foo(\"generic test\"));", + " }", + "}"); + } + @Test public void toyJARAnnotatingClasses() throws Exception { testAnnotationInJarTemplate( diff --git a/jar-infer/nullaway-integration-test/src/test/java/com/uber/nullaway/jarinfer/JarInferIntegrationTest.java b/jar-infer/nullaway-integration-test/src/test/java/com/uber/nullaway/jarinfer/JarInferIntegrationTest.java index 786e0031d6..f77ae54ae3 100644 --- a/jar-infer/nullaway-integration-test/src/test/java/com/uber/nullaway/jarinfer/JarInferIntegrationTest.java +++ b/jar-infer/nullaway-integration-test/src/test/java/com/uber/nullaway/jarinfer/JarInferIntegrationTest.java @@ -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 g = new Toys.Generic<>();", + " // BUG: Diagnostic contains: passing @Nullable parameter 'null'", + " g.getString(null);", + " }", + "}") + .doTest(); + } + @Test public void jarinferNullableReturnsTest() { compilationHelper diff --git a/jar-infer/test-java-lib-jarinfer/src/main/java/com/uber/nullaway/jarinfer/toys/unannotated/Toys.java b/jar-infer/test-java-lib-jarinfer/src/main/java/com/uber/nullaway/jarinfer/toys/unannotated/Toys.java index 62212693cb..1c781faa52 100644 --- a/jar-infer/test-java-lib-jarinfer/src/main/java/com/uber/nullaway/jarinfer/toys/unannotated/Toys.java +++ b/jar-infer/test-java-lib-jarinfer/src/main/java/com/uber/nullaway/jarinfer/toys/unannotated/Toys.java @@ -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 { + 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"); diff --git a/library-model/library-model-generator-integration-test/src/test/java/com/uber/nullaway/libmodel/LibraryModelIntegrationTest.java b/library-model/library-model-generator-integration-test/src/test/java/com/uber/nullaway/libmodel/LibraryModelIntegrationTest.java index 3a230d0200..89dc76b332 100644 --- a/library-model/library-model-generator-integration-test/src/test/java/com/uber/nullaway/libmodel/LibraryModelIntegrationTest.java +++ b/library-model/library-model-generator-integration-test/src/test/java/com/uber/nullaway/libmodel/LibraryModelIntegrationTest.java @@ -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 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(); + } } diff --git a/library-model/library-model-generator/build.gradle b/library-model/library-model-generator/build.gradle index cf2cb1f2be..654c896ff7 100644 --- a/library-model/library-model-generator/build.gradle +++ b/library-model/library-model-generator/build.gradle @@ -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 diff --git a/library-model/library-model-generator/src/main/java/com/uber/nullaway/libmodel/LibraryModelGenerator.java b/library-model/library-model-generator/src/main/java/com/uber/nullaway/libmodel/LibraryModelGenerator.java index 03546f7c1b..163dc27e25 100644 --- a/library-model/library-model-generator/src/main/java/com/uber/nullaway/libmodel/LibraryModelGenerator.java +++ b/library-model/library-model-generator/src/main/java/com/uber/nullaway/libmodel/LibraryModelGenerator.java @@ -29,11 +29,20 @@ import com.github.javaparser.ast.PackageDeclaration; import com.github.javaparser.ast.body.ClassOrInterfaceDeclaration; import com.github.javaparser.ast.body.MethodDeclaration; +import com.github.javaparser.ast.body.Parameter; import com.github.javaparser.ast.expr.AnnotationExpr; import com.github.javaparser.ast.type.ArrayType; import com.github.javaparser.ast.type.ClassOrInterfaceType; import com.github.javaparser.ast.type.TypeParameter; import com.github.javaparser.ast.visitor.VoidVisitorAdapter; +import com.github.javaparser.resolution.TypeSolver; +import com.github.javaparser.resolution.declarations.ResolvedMethodDeclaration; +import com.github.javaparser.resolution.types.ResolvedPrimitiveType; +import com.github.javaparser.resolution.types.ResolvedType; +import com.github.javaparser.symbolsolver.JavaSymbolSolver; +import com.github.javaparser.symbolsolver.resolution.typesolvers.CombinedTypeSolver; +import com.github.javaparser.symbolsolver.resolution.typesolvers.JavaParserTypeSolver; +import com.github.javaparser.symbolsolver.resolution.typesolvers.ReflectionTypeSolver; import com.github.javaparser.utils.CollectionStrategy; import com.github.javaparser.utils.ParserCollectionStrategy; import com.github.javaparser.utils.ProjectRoot; @@ -47,8 +56,10 @@ import java.nio.file.Path; import java.nio.file.Paths; import java.util.Collections; +import java.util.HashSet; import java.util.LinkedHashMap; import java.util.List; +import java.util.Locale; import java.util.Map; import java.util.Optional; import java.util.Set; @@ -69,12 +80,15 @@ public class LibraryModelGenerator { public static class LibraryModelData { public final Map methodRecords; public final Map> nullableUpperBounds; + public final Set nullMarkedClasses; public LibraryModelData( Map methodRecords, - Map> nullableUpperBounds) { + Map> nullableUpperBounds, + Set nullMarkedClasses) { this.methodRecords = methodRecords; this.nullableUpperBounds = nullableUpperBounds; + this.nullMarkedClasses = nullMarkedClasses; } @Override @@ -84,6 +98,8 @@ public String toString() { + methodRecords + ", nullableUpperBounds=" + nullableUpperBounds + + ", nullMarkedClasses=" + + nullMarkedClasses + '}'; } } @@ -97,14 +113,21 @@ public String toString() { public static LibraryModelData generateAstubxForLibraryModels( String inputSourceDirectory, String outputFile) { Map methodRecords = new LinkedHashMap<>(); + Set nullMarkedClasses = new HashSet<>(); Map> nullableUpperBounds = new LinkedHashMap<>(); Path root = dirnameToPath(inputSourceDirectory); - LibraryModelData modelData = new LibraryModelData(methodRecords, nullableUpperBounds); + LibraryModelData modelData = + new LibraryModelData(methodRecords, nullableUpperBounds, nullMarkedClasses); AnnotationCollectorCallback ac = new AnnotationCollectorCallback(modelData); CollectionStrategy strategy = new ParserCollectionStrategy(); // Required to include directories that contain a module-info.java, which don't parse by // default. + TypeSolver typeSolver = + new CombinedTypeSolver( + new ReflectionTypeSolver(), new JavaParserTypeSolver(Paths.get(inputSourceDirectory))); + JavaSymbolSolver symbolSolver = new JavaSymbolSolver(typeSolver); strategy.getParserConfiguration().setLanguageLevel(LanguageLevel.JAVA_17); + strategy.getParserConfiguration().setSymbolResolver(symbolSolver); ProjectRoot projectRoot = strategy.collect(root); projectRoot @@ -130,7 +153,8 @@ public static LibraryModelData generateAstubxForLibraryModels( private static void writeToAstubx(String outputPath, LibraryModelData modelData) { Map methodRecords = modelData.methodRecords; Map> nullableUpperBounds = modelData.nullableUpperBounds; - if (methodRecords.isEmpty() && nullableUpperBounds.isEmpty()) { + Set nullMarkedClasses = modelData.nullMarkedClasses; + if (methodRecords.isEmpty() && nullableUpperBounds.isEmpty() && nullMarkedClasses.isEmpty()) { return; } ImmutableMap importedAnnotations = @@ -147,6 +171,7 @@ private static void writeToAstubx(String outputPath, LibraryModelData modelData) Collections.emptyMap(), Collections.emptyMap(), methodRecords, + nullMarkedClasses, nullableUpperBounds); } } catch (IOException e) { @@ -189,8 +214,8 @@ private static class AnnotationCollectionVisitor extends VoidVisitorAdapter methodRecords; + private final Set nullMarkedClasses; private final Map> nullableUpperBounds; - private static final String ARRAY_RETURN_TYPE_STRING = "Array"; private static final String NULL_MARKED = "NullMarked"; private static final String NULLABLE = "Nullable"; private static final String JSPECIFY_NULLABLE_IMPORT = "org.jspecify.annotations.Nullable"; @@ -198,6 +223,7 @@ private static class AnnotationCollectionVisitor extends VoidVisitorAdapter { - if (a.getNameAsString().equalsIgnoreCase(NULL_MARKED)) { - this.isNullMarked = true; - } - }); + if (this.isNullMarked) { + // nested class, and enclosing class is null marked + this.nullMarkedClasses.add(parentName); + } else { + cid.getAnnotations() + .forEach( + a -> { + if (a.getNameAsString().equalsIgnoreCase(NULL_MARKED)) { + this.isNullMarked = true; + this.nullMarkedClasses.add(parentName); + } + }); + } if (this.isNullMarked) { ImmutableSet nullableUpperBoundParams = getGenericTypeParameterNullableUpperBounds(cid); @@ -247,10 +279,24 @@ public void visit(ClassOrInterfaceDeclaration cid, Void arg) { @Override public void visit(MethodDeclaration md, Void arg) { - if (this.isNullMarked && hasNullableReturn(md)) { + ImmutableMap> nullableParameterMap = getNullableParameters(md); + boolean isReturnNullable = hasNullableReturn(md); + ImmutableSet nullableReturn = + isReturnNullable ? ImmutableSet.of(NULLABLE) : ImmutableSet.of(); + // We write the method record into the astubx if it contains a Nullable return or any Nullable + // parameter. + ResolvedMethodDeclaration resolved = md.resolve(); + String qualifiedSignature = resolved.getQualifiedSignature(); + String methodSignatureWithQualifiedParameters = + qualifiedSignature.substring(qualifiedSignature.lastIndexOf(md.getNameAsString())); + if (this.isNullMarked && (isReturnNullable || !nullableParameterMap.isEmpty())) { methodRecords.put( - parentName + ":" + getMethodReturnTypeString(md) + " " + md.getSignature().toString(), - MethodAnnotationsRecord.create(ImmutableSet.of("Nullable"), ImmutableMap.of())); + parentName + + ":" + + getMethodReturnTypeString(resolved) + + " " + + methodSignatureWithQualifiedParameters, + MethodAnnotationsRecord.create(nullableReturn, nullableParameterMap)); } super.visit(md, null); } @@ -288,13 +334,19 @@ private boolean hasNullableReturn(MethodDeclaration md) { * @param md The MethodDeclaration instance. * @return The return type string value to be written into the astubx file. */ - private String getMethodReturnTypeString(MethodDeclaration md) { - if (md.getType() instanceof ClassOrInterfaceType) { - return md.getType().getChildNodes().get(0).toString(); - } else if (md.getType() instanceof ArrayType) { - return ARRAY_RETURN_TYPE_STRING; + private String getMethodReturnTypeString(ResolvedMethodDeclaration md) { + ResolvedType returnType = md.getReturnType(); + if (returnType.isReferenceType()) { + return returnType.asReferenceType().getQualifiedName(); + } else if (returnType.isArray()) { + return returnType.asArrayType().getComponentType().asReferenceType().getQualifiedName() + + "[]"; + } else if (returnType.isPrimitive()) { + return ((ResolvedPrimitiveType) returnType).name().toLowerCase(Locale.ROOT); + } else if (returnType.isVoid()) { + return "void"; } else { - return md.getType().toString(); + throw new RuntimeException("Unexpected return type: " + returnType); } } @@ -327,5 +379,33 @@ private ImmutableSet getGenericTypeParameterNullableUpperBounds( } return setBuilder.build(); } + + /** + * Takes a MethodDeclaration instance and provides a map containing the Integer indices for the + * parameters with their Nullable annotation to be written in the astubx format. + * + * @param md MethodDeclaration instance. + * @return Map of Nullable parameters for the method. + */ + private ImmutableMap> getNullableParameters( + MethodDeclaration md) { + ImmutableMap.Builder> mapBuilder = ImmutableMap.builder(); + List parameterList = md.getParameters(); + for (int i = 0; i < parameterList.size(); i++) { + Parameter parameter = parameterList.get(i); + Optional nullableAnnotation; + // For ArrayTypes the annotation is on the type instead of the node when the elements inside + // the Array can be @Nullable for e.g. Object @Nullable [] + if (parameter.getType() instanceof ArrayType) { + nullableAnnotation = ((ArrayType) parameter.getType()).getAnnotationByName(NULLABLE); + } else { + nullableAnnotation = parameter.getAnnotationByName(NULLABLE); + } + if (nullableAnnotation.isPresent() && isAnnotationNullable(nullableAnnotation.get())) { + mapBuilder.put(i, ImmutableSet.of("Nullable")); + } + } + return mapBuilder.build(); + } } } diff --git a/library-model/library-model-generator/src/main/java/com/uber/nullaway/libmodel/StubxWriter.java b/library-model/library-model-generator/src/main/java/com/uber/nullaway/libmodel/StubxWriter.java index a3900534c9..bc18dce2b7 100644 --- a/library-model/library-model-generator/src/main/java/com/uber/nullaway/libmodel/StubxWriter.java +++ b/library-model/library-model-generator/src/main/java/com/uber/nullaway/libmodel/StubxWriter.java @@ -15,10 +15,17 @@ public final class StubxWriter { /** * The file magic number for version 0 .astubx files. It should be the first four bytes of any - * compatible .astubx file. + * compatible .astubx file. Unused field, keeping for reference. */ + @SuppressWarnings("UnusedVariable") private static final int VERSION_0_FILE_MAGIC_NUMBER = 691458791; + /** + * The file magic number for version 1 .astubx files. It should be the first four bytes of any + * compatible .astubx file. + */ + private static final int VERSION_1_FILE_MAGIC_NUMBER = 481874642; + /** * This method writes the provided list of annotations to a DataOutputStream in the astubx format. * @@ -37,10 +44,11 @@ public static void write( Map> packageAnnotations, Map> typeAnnotations, Map methodRecords, + Set nullMarkedClasses, Map> nullableUpperBounds) throws IOException { // File format version/magic number - out.writeInt(VERSION_0_FILE_MAGIC_NUMBER); + out.writeInt(VERSION_1_FILE_MAGIC_NUMBER); // Followed by the number of string dictionary entries int numStringEntries = 0; Map encodingDictionary = new LinkedHashMap<>(); @@ -51,10 +59,13 @@ public static void write( packageAnnotations.keySet(), typeAnnotations.keySet(), methodRecords.keySet(), + nullMarkedClasses, nullableUpperBounds.keySet()); for (Collection keyset : keysets) { for (String key : keyset) { - assert !encodingDictionary.containsKey(key); + if (encodingDictionary.containsKey(key)) { + continue; + } strings.add(key); encodingDictionary.put(key, numStringEntries); ++numStringEntries; @@ -120,6 +131,12 @@ public static void write( } } } + // Followed by the number of NullMarked Classes + out.writeInt(nullMarkedClasses.size()); + // Followed by the null marked class records from the dictionary + for (String entry : nullMarkedClasses) { + out.writeInt(encodingDictionary.get(entry)); + } // Followed by the number of nullable upper bounds records out.writeInt(nullableUpperBounds.size()); for (Map.Entry> entry : nullableUpperBounds.entrySet()) { @@ -127,7 +144,7 @@ public static void write( Set parameters = entry.getValue(); out.writeInt(parameters.size()); for (Integer parameter : parameters) { - // Followed by the nullable upper bound record as a par of integers + // Followed by the nullable upper bound record as a pair of integers out.writeInt(encodingDictionary.get(entry.getKey())); out.writeInt(parameter); } diff --git a/library-model/library-model-generator/src/test/java/com/uber/nullaway/libmodel/LibraryModelGeneratorTest.java b/library-model/library-model-generator/src/test/java/com/uber/nullaway/libmodel/LibraryModelGeneratorTest.java index 25bbbe2091..2990a051a8 100644 --- a/library-model/library-model-generator/src/test/java/com/uber/nullaway/libmodel/LibraryModelGeneratorTest.java +++ b/library-model/library-model-generator/src/test/java/com/uber/nullaway/libmodel/LibraryModelGeneratorTest.java @@ -27,7 +27,8 @@ private void runTest( String sourceFileName, String[] lines, ImmutableMap expectedMethodRecords, - ImmutableMap> expectedNullableUpperBounds) + ImmutableMap> expectedNullableUpperBounds, + ImmutableSet expectedNullMarkedClasses) throws IOException { // write it to a source file in inputSourcesFolder with the right file name Files.write( @@ -40,9 +41,15 @@ private void runTest( LibraryModelGenerator.generateAstubxForLibraryModels( inputSourcesFolder.getRoot().getAbsolutePath(), astubxOutputPath); System.err.println("modelData: " + modelData); - Assert.assertTrue("astubx file was not created", Files.exists(Paths.get(astubxOutputPath))); assertThat(modelData.methodRecords, equalTo(expectedMethodRecords)); assertThat(modelData.nullableUpperBounds, equalTo(expectedNullableUpperBounds)); + assertThat(modelData.nullMarkedClasses, equalTo(expectedNullMarkedClasses)); + Assert.assertTrue( + "astubx file was not created", + Files.exists(Paths.get(astubxOutputPath)) + || (expectedMethodRecords.isEmpty() + && expectedNullableUpperBounds.isEmpty() + && expectedNullMarkedClasses.isEmpty())); } @Test @@ -65,9 +72,14 @@ public void nullableReturn() throws IOException { }; ImmutableMap expectedMethodRecords = ImmutableMap.of( - "AnnotationExample:String makeUpperCase(String)", + "AnnotationExample:java.lang.String makeUpperCase(java.lang.String)", MethodAnnotationsRecord.create(ImmutableSet.of("Nullable"), ImmutableMap.of())); - runTest("AnnotationExample.java", lines, expectedMethodRecords, ImmutableMap.of()); + runTest( + "AnnotationExample.java", + lines, + expectedMethodRecords, + ImmutableMap.of(), + ImmutableSet.of("AnnotationExample")); } @Test @@ -86,6 +98,202 @@ public void nullableUpperBound() throws IOException { }; ImmutableMap> expectedNullableUpperBounds = ImmutableMap.of("NullableUpperBound", ImmutableSet.of(0)); - runTest("NullableUpperBound.java", lines, ImmutableMap.of(), expectedNullableUpperBounds); + runTest( + "NullableUpperBound.java", + lines, + ImmutableMap.of(), + expectedNullableUpperBounds, + ImmutableSet.of("NullableUpperBound")); + } + + @Test + public void nullMarkedClasses() throws IOException { + String[] lines = + new String[] { + "import org.jspecify.annotations.NullMarked;", + "@NullMarked", + "public class NullMarked {", + " public static class Nested {}", + "}", + }; + ImmutableSet expectedNullMarkedClasses = + ImmutableSet.of("NullMarked", "NullMarked.Nested"); + runTest( + "NullMarked.java", lines, ImmutableMap.of(), ImmutableMap.of(), expectedNullMarkedClasses); + } + + @Test + public void noNullMarkedClasses() throws IOException { + String[] lines = + new String[] { + "import org.jspecify.annotations.NullMarked;", + "public class NotNullMarked {", + " public static class Nested {}", + "}", + }; + runTest("NotNullMarked.java", lines, ImmutableMap.of(), ImmutableMap.of(), ImmutableSet.of()); + } + + @Test + public void nullableParameters() throws IOException { + String[] lines = + new String[] { + "import org.jspecify.annotations.NullMarked;", + "import org.jspecify.annotations.Nullable;", + "@NullMarked", + "public class NullableParameters{", + " public static Object getNewObjectIfNull(@Nullable Object object) {", + " if (object == null) {", + " return new Object();", + " } else {", + " return object;", + " }", + " }", + "}" + }; + ImmutableMap expectedMethodRecords = + ImmutableMap.of( + "NullableParameters:java.lang.Object getNewObjectIfNull(java.lang.Object)", + MethodAnnotationsRecord.create( + ImmutableSet.of(), ImmutableMap.of(0, ImmutableSet.of("Nullable")))); + runTest( + "NullableParameters.java", + lines, + expectedMethodRecords, + ImmutableMap.of(), + ImmutableSet.of("NullableParameters")); + } + + @Test + public void nullableParametersInNullUnmarkedClass() throws IOException { + String[] lines = + new String[] { + "import org.jspecify.annotations.NullMarked;", + "import org.jspecify.annotations.Nullable;", + "public class NullUnmarked{", + " public static Object getNewObjectIfNull(@Nullable Object object) {", + " if (object == null) {", + " return new Object();", + " } else {", + " return object;", + " }", + " }", + "}" + }; + runTest("NullUnmarked.java", lines, ImmutableMap.of(), ImmutableMap.of(), ImmutableSet.of()); + } + + @Test + public void nullableArrayTypeParameter() throws IOException { + String[] lines = + new String[] { + "import org.jspecify.annotations.NullMarked;", + "import org.jspecify.annotations.Nullable;", + "@NullMarked", + "public class NullableParameters{", + " public static Object[] getNewObjectArrayIfNull(Object @Nullable [] objectArray) {", + " if (Arrays.stream(objectArray).allMatch(e -> e == null)) {", + " return new Object[]{new Object(),new Object()};", + " } else {", + " return objectArray;", + " }", + " }", + "}" + }; + ImmutableMap expectedMethodRecords = + ImmutableMap.of( + "NullableParameters:java.lang.Object[] getNewObjectArrayIfNull(java.lang.Object[])", + MethodAnnotationsRecord.create( + ImmutableSet.of(), ImmutableMap.of(0, ImmutableSet.of("Nullable")))); + runTest( + "NullableParameters.java", + lines, + expectedMethodRecords, + ImmutableMap.of(), + ImmutableSet.of("NullableParameters")); + } + + @Test + public void genericParameter() throws IOException { + String[] lines = + new String[] { + "import org.jspecify.annotations.NullMarked;", + "import org.jspecify.annotations.Nullable;", + "@NullMarked", + "public class Generic {", + " public String getString(@Nullable T t) {", + " return t.toString();", + " }", + "}" + }; + ImmutableMap expectedMethodRecords = + ImmutableMap.of( + "Generic:java.lang.String getString(T)", + MethodAnnotationsRecord.create( + ImmutableSet.of(), ImmutableMap.of(0, ImmutableSet.of("Nullable")))); + runTest( + "Generic.java", + lines, + expectedMethodRecords, + ImmutableMap.of(), + ImmutableSet.of("Generic")); + } + + @Test + public void primitiveTypeReturn() throws IOException { + String[] lines = + new String[] { + "import org.jspecify.annotations.NullMarked;", + "import org.jspecify.annotations.Nullable;", + "@NullMarked", + "public class PrimitiveType {", + " public int multiply(@Nullable Integer num1, @Nullable Integer num2) {", + " if(num1!=null && num2!=null){", + " return num1*num2;", + " }", + " }", + "}" + }; + ImmutableMap expectedMethodRecords = + ImmutableMap.of( + "PrimitiveType:int multiply(java.lang.Integer, java.lang.Integer)", + MethodAnnotationsRecord.create( + ImmutableSet.of(), + ImmutableMap.of(0, ImmutableSet.of("Nullable"), 1, ImmutableSet.of("Nullable")))); + runTest( + "PrimitiveType.java", + lines, + expectedMethodRecords, + ImmutableMap.of(), + ImmutableSet.of("PrimitiveType")); + } + + @Test + public void voidReturn() throws IOException { + String[] lines = + new String[] { + "import org.jspecify.annotations.NullMarked;", + "import org.jspecify.annotations.Nullable;", + "@NullMarked", + "public class VoidReturn {", + " public void printMultiply(@Nullable Integer num1, @Nullable Integer num2) {", + " if(num1!=null && num2!=null){", + " System.out.println(num1*num2);", + " }", + " }", + "}" + }; + ImmutableMap expectedMethodRecords = + ImmutableMap.of( + "VoidReturn:void printMultiply(java.lang.Integer, java.lang.Integer)", + MethodAnnotationsRecord.create( + ImmutableSet.of(), + ImmutableMap.of(0, ImmutableSet.of("Nullable"), 1, ImmutableSet.of("Nullable")))); + runTest( + "VoidReturn.java", + lines, + expectedMethodRecords, + ImmutableMap.of(), + ImmutableSet.of("VoidReturn")); } } diff --git a/library-model/test-library-model-generator/src/main/java/com/test/ParameterAnnotationExample.java b/library-model/test-library-model-generator/src/main/java/com/test/ParameterAnnotationExample.java new file mode 100644 index 0000000000..8dc1968e50 --- /dev/null +++ b/library-model/test-library-model-generator/src/main/java/com/test/ParameterAnnotationExample.java @@ -0,0 +1,46 @@ +package com.test; + +/** + * This class has the same name as the class under + * resources/sample_annotated/src/com/uber/nullaway/libmodel/AnnotationExample.java because we use + * this as the unannotated version for our test cases to see if we are appropriately processing the + * annotations as an external library model. + */ +public class ParameterAnnotationExample { + + public static Integer add(Integer a, Integer b) { + return a + b; + } + + public static Object getNewObjectIfNull(Object object) { + if (object == null) { + return new Object(); + } else { + return object; + } + } + + public static void printObjectString(Object object) { + System.out.println(object.toString()); + } + + @SuppressWarnings("ArrayToString") + public static void takesNullArray(Object[] objects) { + System.out.println(objects); + } + + @SuppressWarnings("ArrayToString") + public static void takesNonNullArray(Object[] objects) { + String unused = objects.toString(); + } + + public static class Generic { + public String getString(T t) { + return t != null ? t.toString() : ""; + } + + public void printObjectString(T t) { + System.out.println(t.toString()); + } + } +} diff --git a/library-model/test-library-model-generator/src/main/java/com/uber/nullaway/libmodel/AnnotationExample.java b/library-model/test-library-model-generator/src/main/java/com/uber/nullaway/libmodel/AnnotationExample.java index 984b969c35..7b22856025 100644 --- a/library-model/test-library-model-generator/src/main/java/com/uber/nullaway/libmodel/AnnotationExample.java +++ b/library-model/test-library-model-generator/src/main/java/com/uber/nullaway/libmodel/AnnotationExample.java @@ -49,4 +49,16 @@ public T getNullable() { return nullableObject; } } + + public static Integer add(Integer a, Integer b) { + return a + b; + } + + public static Object getNewObjectIfNull(Object object) { + if (object == null) { + return new Object(); + } else { + return object; + } + } } diff --git a/library-model/test-library-model-generator/src/main/resources/sample_annotated/src/com/test/ParameterAnnotationExample.java b/library-model/test-library-model-generator/src/main/resources/sample_annotated/src/com/test/ParameterAnnotationExample.java new file mode 100644 index 0000000000..d7337df9ad --- /dev/null +++ b/library-model/test-library-model-generator/src/main/resources/sample_annotated/src/com/test/ParameterAnnotationExample.java @@ -0,0 +1,44 @@ +package com.test; + +import org.jspecify.annotations.NonNull; +import org.jspecify.annotations.NullMarked; +import org.jspecify.annotations.Nullable; + +@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; + } + } + + public static void printObjectString(@NonNull Object object) { + System.out.println(object.toString()); + } + + public static void takesNullArray(Object @Nullable [] objects) { + System.out.println(objects); + } + + public static void takesNonNullArray(Object[] objects) { + String unused = objects.toString(); + } + + public static class Generic { + + public String getString(@Nullable T t) { + return t != null ? t.toString() : ""; + } + + public void printObjectString(T t) { + System.out.println(t.toString()); + } + } +} diff --git a/library-model/test-library-model-generator/src/main/resources/sample_annotated/src/com/uber/nullaway/libmodel/AnnotationExample.java b/library-model/test-library-model-generator/src/main/resources/sample_annotated/src/com/uber/nullaway/libmodel/AnnotationExample.java index 5fef2e2118..a46ecca152 100644 --- a/library-model/test-library-model-generator/src/main/resources/sample_annotated/src/com/uber/nullaway/libmodel/AnnotationExample.java +++ b/library-model/test-library-model-generator/src/main/resources/sample_annotated/src/com/uber/nullaway/libmodel/AnnotationExample.java @@ -54,4 +54,16 @@ public T getNullable() { return nullableObject; } } + + 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; + } + } } diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/InferredJARModelsHandler.java b/nullaway/src/main/java/com/uber/nullaway/handlers/InferredJARModelsHandler.java index 472c69bbcd..88f474205e 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/InferredJARModelsHandler.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/InferredJARModelsHandler.java @@ -254,9 +254,9 @@ private String getMethodSignature(Symbol.MethodSymbol method) { private String getSimpleTypeName(Type typ) { if (typ.getKind() == TypeKind.TYPEVAR) { - return typ.getUpperBound().tsym.getSimpleName().toString(); + return typ.getUpperBound().tsym.getQualifiedName().toString(); } else { - return typ.tsym.getSimpleName().toString(); + return typ.toString(); } } } diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java b/nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java index 3862b0704b..49aee7889b 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java @@ -1288,19 +1288,20 @@ private NameIndexedMap makeOptimizedLookup( private static class ExternalStubxLibraryModels implements LibraryModels { private final Map>>> argAnnotCache; + private final Set nullMarkedClassesCache; private final Map upperBoundsCache; ExternalStubxLibraryModels() { String libraryModelLogName = "LM"; StubxCacheUtil cacheUtil = new StubxCacheUtil(libraryModelLogName); argAnnotCache = cacheUtil.getArgAnnotCache(); + nullMarkedClassesCache = cacheUtil.getNullMarkedClassesCache(); upperBoundsCache = cacheUtil.getUpperBoundCache(); } @Override public ImmutableSet nullMarkedClasses() { - Set cachedNullMarkedClasses = argAnnotCache.keySet(); - return new ImmutableSet.Builder().addAll(cachedNullMarkedClasses).build(); + return new ImmutableSet.Builder().addAll(nullMarkedClassesCache).build(); } @Override @@ -1320,7 +1321,23 @@ public ImmutableSetMultimap failIfNullParameters() { @Override public ImmutableSetMultimap explicitlyNullableParameters() { - return ImmutableSetMultimap.of(); + ImmutableSetMultimap.Builder mapBuilder = + new ImmutableSetMultimap.Builder<>(); + for (Map.Entry>>> outerEntry : + argAnnotCache.entrySet()) { + String className = outerEntry.getKey(); + for (Map.Entry>> innerEntry : + outerEntry.getValue().entrySet()) { + String methodName = innerEntry.getKey().substring(innerEntry.getKey().indexOf(" ") + 1); + for (Map.Entry> entry : innerEntry.getValue().entrySet()) { + Integer index = entry.getKey(); + if (index >= 0 && entry.getValue().stream().anyMatch(a -> a.contains("Nullable"))) { + mapBuilder.put(methodRef(className, methodName), index); + } + } + } + } + return mapBuilder.build(); } @Override diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/StubxCacheUtil.java b/nullaway/src/main/java/com/uber/nullaway/handlers/StubxCacheUtil.java index e2ac9cb991..8144f6cb49 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/StubxCacheUtil.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/StubxCacheUtil.java @@ -27,6 +27,7 @@ import java.io.IOException; import java.io.InputStream; import java.util.HashMap; +import java.util.HashSet; import java.util.LinkedHashMap; import java.util.LinkedHashSet; import java.util.Map; @@ -43,7 +44,12 @@ */ public class StubxCacheUtil { - private static final int VERSION_0_FILE_MAGIC_NUMBER = 691458791; + /** + * The file magic number for version 1 .astubx files. It should be the first four bytes of any + * compatible .astubx file. + */ + private static final int VERSION_1_FILE_MAGIC_NUMBER = 481874642; + private boolean DEBUG = false; private String logCaller = ""; @@ -59,6 +65,8 @@ private void LOG(boolean cond, String tag, String msg) { private final Map upperBoundCache; + private final Set nullMarkedClassesCache; + /** * Initializes a new {@code StubxCacheUtil} instance. * @@ -70,6 +78,7 @@ private void LOG(boolean cond, String tag, String msg) { public StubxCacheUtil(String logCaller) { argAnnotCache = new LinkedHashMap<>(); upperBoundCache = new HashMap<>(); + nullMarkedClassesCache = new HashSet<>(); this.logCaller = logCaller; loadStubxFiles(); } @@ -78,6 +87,10 @@ public Map getUpperBoundCache() { return upperBoundCache; } + public Set getNullMarkedClassesCache() { + return nullMarkedClassesCache; + } + public Map>>> getArgAnnotCache() { return argAnnotCache; } @@ -109,7 +122,7 @@ public void parseStubStream(InputStream stubxInputStream, String stubxLocation) String[] strings; DataInputStream in = new DataInputStream(stubxInputStream); // Read and check the magic version number - if (in.readInt() != VERSION_0_FILE_MAGIC_NUMBER) { + if (in.readInt() != VERSION_1_FILE_MAGIC_NUMBER) { throw new Error("Invalid file version/magic number for stubx file!" + stubxLocation); } // Read the number of strings in the string dictionary @@ -162,6 +175,11 @@ public void parseStubStream(InputStream stubxInputStream, String stubxLocation) "method: " + methodSig + ", argNum: " + argNum + ", arg annotation: " + annotation); cacheAnnotation(methodSig, argNum, annotation); } + // reading the NullMarked classes + int numNullMarkedClasses = in.readInt(); + for (int i = 0; i < numNullMarkedClasses; i++) { + this.nullMarkedClassesCache.add(strings[in.readInt()]); + } // read the number of nullable upper bound entries int numClassesWithNullableUpperBounds = in.readInt(); for (int i = 0; i < numClassesWithNullableUpperBounds; i++) {