diff --git a/CHANGELOG.md b/CHANGELOG.md index d1fd99c2a8..3c4b0f9173 100755 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,25 @@ Changelog ========= +Version 0.12.2 +--------------- +* Fix reading of JSpecify @nullable annotations from varargs parameter in bytecode (#1089) +* Fix JarInfer handling of generic types (#1078) +* Fix another JSpecify mode crash involving raw types (#1086) +* Fix bugs in handling of valueOf calls for map keys (#1085) +* Suggest correct fix when array component of non-nullable array is made null. (#1087) +* Substitute type arguments when checking type parameter nullability at call site (#1070) +* Fix JarInfer parameter indexes for instance methods (#1071) +* JSpecify mode: initial support for generic methods (with explicit type arguments at calls) (#1053) +* Maintenance + - Update to latest Error Prone and Error Prone Gradle plugin (#1064) + - Refactor serialization adapter retrieval by version (#1066) + - Remove fixes.tsv serialization from NullAway serialization service (#1063) + - Enable javac -parameters flag (#1069) + - Update to Gradle 8.11 (#1073) + - Add test for issue 1035 (#1074) + - remove use of deprecated Gradle API (#1076) + - Update to Error Prone 2.36.0 (#1077) + Version 0.12.1 --------------- * Add library model for Apache Commons CollectionUtils.isNotEmpty (#932) (#1062) diff --git a/gradle.properties b/gradle.properties index fa5e0e1aab..46a1b9761f 100644 --- a/gradle.properties +++ b/gradle.properties @@ -12,7 +12,7 @@ org.gradle.caching=true org.gradle.jvmargs=-Xmx2g -XX:MaxMetaspaceSize=512m GROUP=com.uber.nullaway -VERSION_NAME=0.12.2-SNAPSHOT +VERSION_NAME=0.12.3-SNAPSHOT POM_DESCRIPTION=A fast annotation-based null checker for Java diff --git a/gradle/dependencies.gradle b/gradle/dependencies.gradle index 08bb8bf672..417be1dde9 100755 --- a/gradle/dependencies.gradle +++ b/gradle/dependencies.gradle @@ -48,7 +48,7 @@ def versions = [ // The version of Error Prone that NullAway is compiled and tested against errorProneApi : errorProneVersionToCompileAgainst, support : "27.1.1", - wala : "1.6.8", + wala : "1.6.9", commonscli : "1.4", autoValue : "1.10.2", autoService : "1.1.1", diff --git a/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java b/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java index fc93add428..4df2807189 100644 --- a/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java +++ b/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java @@ -51,6 +51,7 @@ import java.util.List; import java.util.Map; import java.util.Set; +import java.util.function.BiPredicate; import java.util.stream.Collectors; import java.util.stream.Stream; import javax.lang.model.element.AnnotationMirror; @@ -328,13 +329,13 @@ private static Stream getTypeUseAnnotations( return rawTypeAttributes.filter( (t) -> t.position.type.equals(TargetType.METHOD_RETURN) - && isDirectTypeUseAnnotation(t, symbol, config)); + && (!onlyDirect || isDirectTypeUseAnnotation(t, symbol, config))); } else { // filter for annotations directly on the type return rawTypeAttributes.filter( t -> targetTypeMatches(symbol, t.position) - && (!onlyDirect || NullabilityUtil.isDirectTypeUseAnnotation(t, symbol, config))); + && (!onlyDirect || isDirectTypeUseAnnotation(t, symbol, config))); } } @@ -540,22 +541,11 @@ public static T castToNonNull(@Nullable T obj) { * otherwise */ public static boolean isArrayElementNullable(Symbol arraySymbol, Config config) { - for (Attribute.TypeCompound t : arraySymbol.getRawTypeAttributes()) { - for (TypeAnnotationPosition.TypePathEntry entry : t.position.location) { - if (entry.tag == TypeAnnotationPosition.TypePathEntryKind.ARRAY) { - if (Nullness.isNullableAnnotation(t.type.toString(), config)) { - return true; - } - } - } - } - // For varargs symbols we also consider the elements to be @Nullable if there is a @Nullable - // declaration annotation on the parameter - // NOTE this flag check does not work for the varargs parameter of a method defined in bytecodes - if ((arraySymbol.flags() & Flags.VARARGS) != 0) { - return Nullness.hasNullableDeclarationAnnotation(arraySymbol, config); - } - return false; + return checkArrayElementAnnotations( + arraySymbol, + config, + Nullness::isNullableAnnotation, + Nullness::hasNullableDeclarationAnnotation); } /** @@ -582,12 +572,50 @@ public static boolean nullableVarargsElementsForSourceOrBytecode( * otherwise */ public static boolean isArrayElementNonNull(Symbol arraySymbol, Config config) { + return checkArrayElementAnnotations( + arraySymbol, + config, + Nullness::isNonNullAnnotation, + Nullness::hasNonNullDeclarationAnnotation); + } + + /** + * Checks if the given varargs symbol has a {@code @NonNull} annotation for its elements. Works + * for both source and bytecode. + * + * @param varargsSymbol the symbol of the varargs parameter + * @param config NullAway configuration + * @return true if the varargs symbol has a {@code @NonNull} annotation for its elements, false + * otherwise + */ + public static boolean nonnullVarargsElementsForSourceOrBytecode( + Symbol varargsSymbol, Config config) { + return isArrayElementNonNull(varargsSymbol, config) + || Nullness.hasNonNullDeclarationAnnotation(varargsSymbol, config); + } + + /** + * Checks if the annotations on the elements of some array symbol satisfy some predicate. + * + * @param arraySymbol the array symbol + * @param config NullAway configuration + * @param typeUseCheck the predicate to check the type-use annotations + * @param declarationCheck the predicate to check the declaration annotations (applied only to + * varargs symbols) + * @return true if the annotations on the elements of the array symbol satisfy the given + * predicates, false otherwise + */ + private static boolean checkArrayElementAnnotations( + Symbol arraySymbol, + Config config, + BiPredicate typeUseCheck, + BiPredicate declarationCheck) { if (getTypeUseAnnotations(arraySymbol, config, /* onlyDirect= */ false) .anyMatch( t -> { for (TypeAnnotationPosition.TypePathEntry entry : t.position.location) { if (entry.tag == TypeAnnotationPosition.TypePathEntryKind.ARRAY) { - if (Nullness.isNonNullAnnotation(t.type.toString(), config)) { + if (typeUseCheck.test(t.type.toString(), config)) { return true; } } @@ -596,30 +624,14 @@ public static boolean isArrayElementNonNull(Symbol arraySymbol, Config config) { })) { return true; } - // For varargs symbols we also consider the elements to be @NonNull if there is a @NonNull - // declaration annotation on the parameter + // For varargs symbols we also check for declaration annotations on the parameter // NOTE this flag check does not work for the varargs parameter of a method defined in bytecodes if ((arraySymbol.flags() & Flags.VARARGS) != 0) { - return Nullness.hasNonNullDeclarationAnnotation(arraySymbol, config); + return declarationCheck.test(arraySymbol, config); } return false; } - /** - * Checks if the given varargs symbol has a {@code @NonNull} annotation for its elements. Works - * for both source and bytecode. - * - * @param varargsSymbol the symbol of the varargs parameter - * @param config NullAway configuration - * @return true if the varargs symbol has a {@code @NonNull} annotation for its elements, false - * otherwise - */ - public static boolean nonnullVarargsElementsForSourceOrBytecode( - Symbol varargsSymbol, Config config) { - return isArrayElementNonNull(varargsSymbol, config) - || Nullness.hasNonNullDeclarationAnnotation(varargsSymbol, config); - } - /** * Does the given symbol have a JetBrains @NotNull declaration annotation? Useful for workarounds * in light of https://github.com/uber/NullAway/issues/720 diff --git a/nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPath.java b/nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPath.java index a75fd858a9..a7b2d0a4e1 100644 --- a/nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPath.java +++ b/nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPath.java @@ -28,6 +28,8 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; import com.google.errorprone.VisitorState; +import com.google.errorprone.suppliers.Supplier; +import com.google.errorprone.suppliers.Suppliers; import com.google.errorprone.util.ASTHelpers; import com.sun.source.tree.LiteralTree; import com.sun.source.tree.MethodInvocationTree; @@ -75,6 +77,12 @@ */ public final class AccessPath implements MapKey { + private static final Supplier INTEGER_TYPE_SUPPLIER = + Suppliers.typeFromString("java.lang.Integer"); + + private static final Supplier LONG_TYPE_SUPPLIER = + Suppliers.typeFromString("java.lang.Long"); + /** * A prefix added for elements appearing in method invocation APs which represent fields that can * be proven to be class-initialization time constants (i.e. static final fields of a type known @@ -278,14 +286,15 @@ private static Node stripCasts(Node node) { return new NumericMapKey(((LongLiteralNode) argument).getValue()); case METHOD_INVOCATION: MethodAccessNode target = ((MethodInvocationNode) argument).getTarget(); - Node receiver = stripCasts(target.getReceiver()); List arguments = ((MethodInvocationNode) argument).getArguments(); // Check for int/long boxing. if (target.getMethod().getSimpleName().toString().equals("valueOf") - && arguments.size() == 1 - && castToNonNull(receiver.getTree()).getKind().equals(Tree.Kind.IDENTIFIER) - && (receiver.toString().equals("Integer") || receiver.toString().equals("Long"))) { - return argumentToMapKeySpecifier(arguments.get(0), state, apContext); + && arguments.size() == 1) { + Type ownerType = ((Symbol.MethodSymbol) target.getMethod()).owner.type; + if (ASTHelpers.isSameType(ownerType, INTEGER_TYPE_SUPPLIER.get(state), state) + || ASTHelpers.isSameType(ownerType, LONG_TYPE_SUPPLIER.get(state), state)) { + return argumentToMapKeySpecifier(arguments.get(0), state, apContext); + } } // Fine to fallthrough: default: diff --git a/nullaway/src/main/java/com/uber/nullaway/generics/CheckIdenticalNullabilityVisitor.java b/nullaway/src/main/java/com/uber/nullaway/generics/CheckIdenticalNullabilityVisitor.java index 5f1bac2742..d567c2e028 100644 --- a/nullaway/src/main/java/com/uber/nullaway/generics/CheckIdenticalNullabilityVisitor.java +++ b/nullaway/src/main/java/com/uber/nullaway/generics/CheckIdenticalNullabilityVisitor.java @@ -35,10 +35,10 @@ public Boolean visitClassType(Type.ClassType lhsType, Type rhsType) { // The base type of rhsType may be a subtype of lhsType's base type. In such cases, we must // compare lhsType against the supertype of rhsType with a matching base type. Type rhsTypeAsSuper = types.asSuper(rhsType, lhsType.tsym); - // This is impossible, considering the fact that standard Java subtyping succeeds before - // running NullAway if (rhsTypeAsSuper == null) { - throw new RuntimeException("Did not find supertype of " + rhsType + " matching " + lhsType); + // Surprisingly, this can in fact occur, in cases involving raw types. See, e.g., + // GenericsTests#issue1082 and https://github.com/uber/NullAway/pull/1086. Bail out. + return true; } // bail out of checking raw types for now if (rhsTypeAsSuper.isRaw()) { diff --git a/nullaway/src/test/java/com/uber/nullaway/AccessPathsTests.java b/nullaway/src/test/java/com/uber/nullaway/AccessPathsTests.java index 1061e9f7c6..354b00cf35 100644 --- a/nullaway/src/test/java/com/uber/nullaway/AccessPathsTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/AccessPathsTests.java @@ -434,4 +434,58 @@ public void testAccessUsingExplicitThis() { "}") .doTest(); } + + @Test + public void mapKeysFromValueOf() { + defaultCompilationHelper + .addSourceLines( + "Foo.java", + "package com.uber;", + "import java.util.HashMap;", + "import java.util.Map;", + "public class Foo {", + " private final Map map = new HashMap<>();", + " private final Map longMap = new HashMap<>();", + " static Integer valueOf(int i) { return 0; }", + " static Integer valueOf(int i, int j) { return i+j; }", + " public void putThenGetIntegerValueOf() {", + " map.put(Integer.valueOf(10), new Object());", + " map.get(Integer.valueOf(10)).toString();", + " }", + " public void putThenGetLongValueOf() {", + " longMap.put(Long.valueOf(10), new Object());", + " longMap.get(Long.valueOf(10)).toString();", + " }", + " public void putThenGetFooValueOf() {", + " map.put(valueOf(10), new Object());", + " // Unknown valueOf method so we report a warning", + " // BUG: Diagnostic contains: dereferenced expression map.get(valueOf(10)) is @Nullable", + " map.get(valueOf(10)).toString();", + " map.put(valueOf(10,20), new Object());", + " // Unknown valueOf method so we report a warning", + " // BUG: Diagnostic contains: dereferenced expression map.get(valueOf(10,20)) is @Nullable", + " map.get(valueOf(10,20)).toString();", + " }", + "}") + .doTest(); + } + + @Test + public void mapKeyFromIntegerValueOfStaticImport() { + defaultCompilationHelper + .addSourceLines( + "Foo.java", + "package com.uber;", + "import java.util.HashMap;", + "import java.util.Map;", + "import static java.lang.Integer.valueOf;", + "public class Foo {", + " private final Map map = new HashMap<>();", + " public void putThenGet() {", + " map.put(valueOf(10), new Object());", + " map.get(valueOf(10)).toString();", + " }", + "}") + .doTest(); + } } diff --git a/nullaway/src/test/java/com/uber/nullaway/VarargsTests.java b/nullaway/src/test/java/com/uber/nullaway/VarargsTests.java index 3f29e1e082..cfee867589 100644 --- a/nullaway/src/test/java/com/uber/nullaway/VarargsTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/VarargsTests.java @@ -106,6 +106,22 @@ public void nullableTypeUseVarArgsFromBytecode() { .doTest(); } + @Test + public void nullableVarArgsFromBytecodeJSpecify() { + defaultCompilationHelper + .addSourceLines( + "Test.java", + "package com.uber;", + "import com.uber.lib.Varargs;", + "public class Test {", + " public void testTypeUse() {", + " String x = null;", + " Varargs.typeUseNullableElementsJSpecify(x);", + " }", + "}") + .doTest(); + } + @Test public void nullableTypeUseVarargs() { defaultCompilationHelper diff --git a/nullaway/src/test/java/com/uber/nullaway/jspecify/GenericsTests.java b/nullaway/src/test/java/com/uber/nullaway/jspecify/GenericsTests.java index 74a7ef481f..946b6963c4 100644 --- a/nullaway/src/test/java/com/uber/nullaway/jspecify/GenericsTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/jspecify/GenericsTests.java @@ -2002,6 +2002,31 @@ public void issue1019() { .doTest(); } + @Test + public void issue1082() { + makeHelper() + .addSourceLines( + "Main.java", + "package com.uber;", + "import java.util.Optional;", + "public class Main {", + " public interface Factory {", + " T create();", + " }", + " public interface Expiry {}", + " static class Config {", + " Config setFactory(Optional>> factory) {", + " return this;", + " }", + " }", + " static void caller(Config config) {", + " // checking that we don't crash", + " config.setFactory(Optional.empty());", + " }", + "}") + .doTest(); + } + private CompilationTestHelper makeHelper() { return makeTestHelperWithArgs( Arrays.asList( diff --git a/test-java-lib/src/main/java/com/uber/lib/Varargs.java b/test-java-lib/src/main/java/com/uber/lib/Varargs.java index a232d1c14a..81cefb9166 100644 --- a/test-java-lib/src/main/java/com/uber/lib/Varargs.java +++ b/test-java-lib/src/main/java/com/uber/lib/Varargs.java @@ -7,4 +7,7 @@ public class Varargs { public Varargs(@Nullable String... args) {} public static void typeUse(String @org.jspecify.annotations.Nullable ... args) {} + + public static void typeUseNullableElementsJSpecify( + @org.jspecify.annotations.Nullable String... args) {} }