From 8e89a9dfb992be041056f94f4cbd13d1b89d3a35 Mon Sep 17 00:00:00 2001 From: Lazaro Clapp Date: Thu, 30 Nov 2023 14:53:58 -0500 Subject: Prepare for release 0.10.18 --- CHANGELOG.md | 4 ++++ gradle.properties | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index eda8659..09a549f 100755 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,9 @@ Changelog ========= +Version 0.10.18 +--------------- +* Fix assertion check for structure of enhanced-for loop over a Map keySet (#868) + Version 0.10.17 --------------- * Fix bug with computing direct type use annotations on parameters (#864) diff --git a/gradle.properties b/gradle.properties index fe3f54f..15582ed 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.10.18-SNAPSHOT +VERSION_NAME=0.10.18 POM_DESCRIPTION=A fast annotation-based null checker for Java -- cgit v1.2.3 From 1140a8189fcd86aa3c291468dfbe2e7abaa8e47e Mon Sep 17 00:00:00 2001 From: Lazaro Clapp Date: Thu, 30 Nov 2023 15:00:48 -0500 Subject: Prepare next development version. --- gradle.properties | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gradle.properties b/gradle.properties index 15582ed..bbb3fa7 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.10.18 +VERSION_NAME=0.10.19-SNAPSHOT POM_DESCRIPTION=A fast annotation-based null checker for Java -- cgit v1.2.3 From eb2e19c6515cb605293f9bd1c3d5be865a25fe6d Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Mon, 4 Dec 2023 14:47:51 -0800 Subject: Update to Checker Framework 3.41.0 (#873) Just to stay up to date. [Benchmarking results](https://github.com/uber/NullAway/pull/873#issuecomment-1839420450) show no overhead regression, and possibly a very slight improvement. --- gradle/dependencies.gradle | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gradle/dependencies.gradle b/gradle/dependencies.gradle index 566b5cf..26fc151 100755 --- a/gradle/dependencies.gradle +++ b/gradle/dependencies.gradle @@ -40,7 +40,7 @@ if (project.hasProperty("epApiVersion")) { def versions = [ asm : "9.3", - checkerFramework : "3.39.0", + checkerFramework : "3.41.0", // for comparisons in other parts of the build errorProneLatest : latestErrorProneVersion, // The version of Error Prone used to check NullAway's code. -- cgit v1.2.3 From 2f1cf7c8e4990029621ce8b9a73e83db94d7bbf5 Mon Sep 17 00:00:00 2001 From: Abhijit Kulkarni Date: Tue, 5 Dec 2023 13:37:20 -0800 Subject: JSpecify: changes for issue #861 (#863) Removing unnecessary checks performed for ClassType in GenericChecks.java in reference to issue #861. Added some null checks since some of the checks for ClassType were also indirectly checking for nullability and hence removing them entirely cause the test rawTypes to crash. Fixes #861 --------- Co-authored-by: Manu Sridharan --- .../generics/CompareNullabilityVisitor.java | 15 ++++-- .../com/uber/nullaway/generics/GenericsChecks.java | 24 ++++------ .../nullaway/NullAwayJSpecifyGenericsTests.java | 55 ++++++++++++++++++++++ 3 files changed, 76 insertions(+), 18 deletions(-) diff --git a/nullaway/src/main/java/com/uber/nullaway/generics/CompareNullabilityVisitor.java b/nullaway/src/main/java/com/uber/nullaway/generics/CompareNullabilityVisitor.java index 025c3ee..de86547 100644 --- a/nullaway/src/main/java/com/uber/nullaway/generics/CompareNullabilityVisitor.java +++ b/nullaway/src/main/java/com/uber/nullaway/generics/CompareNullabilityVisitor.java @@ -6,6 +6,7 @@ import com.sun.tools.javac.code.Attribute; import com.sun.tools.javac.code.Type; import com.sun.tools.javac.code.Types; import java.util.List; +import javax.lang.model.type.NullType; /** * Visitor that checks equality of nullability annotations for all nested generic type arguments @@ -21,22 +22,25 @@ public class CompareNullabilityVisitor extends Types.DefaultTypeVisitor lhsTypeArguments = lhsType.getTypeArguments(); - List rhsTypeArguments = rhsType.getTypeArguments(); + List rhsTypeArguments = rhsTypeAsSuper.getTypeArguments(); // This is impossible, considering the fact that standard Java subtyping succeeds before // running NullAway if (lhsTypeArguments.size() != rhsTypeArguments.size()) { throw new RuntimeException( - "Number of types arguments in " + rhsType + " does not match " + lhsType); + "Number of types arguments in " + rhsTypeAsSuper + " does not match " + lhsType); } for (int i = 0; i < lhsTypeArguments.size(); i++) { Type lhsTypeArgument = lhsTypeArguments.get(i); @@ -77,6 +81,9 @@ public class CompareNullabilityVisitor extends Types.DefaultTypeVisitor { }", + " static void testPositive() {", + " // BUG: Diagnostic contains: Cannot assign from type", + " A[]> var2 = new A[]>();", + " }", + "}") + .doTest(); + } + @Test public void genericPrimitiveArrayTypeAssignment() { makeHelper() @@ -1560,6 +1577,44 @@ public class NullAwayJSpecifyGenericsTests extends NullAwayTestsBase { .doTest(); } + @Test + public void testForNullRhsTypeWhenReturnedForGenericType() { + makeHelper() + .addSourceLines( + "Test.java", + "package com.uber;", + "import org.jspecify.annotations.Nullable;", + "class Test {", + " static class A { }", + " static A testPositive() {", + " // BUG: Diagnostic contains: returning @Nullable expression from method with @NonNull return type", + " return null;", + " }", + " static @Nullable A testNegative() {", + " return null;", + " }", + "}") + .doTest(); + } + + @Test + public void testForNullTypeRhsTypeForArrayType() { + makeHelper() + .addSourceLines( + "Test.java", + "package com.uber;", + "import org.jspecify.annotations.Nullable;", + "import java.util.List;", + "import java.util.ArrayList;", + "class Test {", + " static void testNegative() {", + " List a = new ArrayList();", + " Object[] o = a != null ? a.toArray() : null;", + " }", + "}") + .doTest(); + } + private CompilationTestHelper makeHelper() { return makeTestHelperWithArgs( Arrays.asList( -- cgit v1.2.3 From 5fbee1ff8682762cd2f291070491fe3e476787cf Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Wed, 6 Dec 2023 05:18:48 -0800 Subject: Model Lombok-generated equals methods as having a @Nullable parameter (#874) See the new test in `UsesDTO`. Without this change, NullAway reports an incorrect warning for this code that passes a `@Nullable` parameter to a Lombok-generated `equals()` method. The real logic change in this PR is very small. Most of the changes are required to make a `VisitorState` object available in a `Handler` method. --- .../src/main/java/com/uber/nullaway/NullAway.java | 7 ++----- .../dataflow/AccessPathNullnessPropagation.java | 2 +- .../dataflow/CoreNullnessStoreInitializer.java | 16 ++++++++-------- .../dataflow/NullnessStoreInitializer.java | 8 +++----- .../uber/nullaway/handlers/BaseNoOpHandler.java | 2 +- .../uber/nullaway/handlers/CompositeHandler.java | 4 ++-- .../java/com/uber/nullaway/handlers/Handler.java | 4 ++-- .../handlers/InferredJARModelsHandler.java | 3 +-- .../nullaway/handlers/LibraryModelsHandler.java | 4 ++-- .../com/uber/nullaway/handlers/LombokHandler.java | 22 ++++++++++++++++++++++ .../handlers/RestrictiveAnnotationHandler.java | 2 +- .../contract/ContractNullnessStoreInitializer.java | 8 +++----- .../src/main/java/com/uber/lombok/UsesDTO.java | 5 +++++ 13 files changed, 53 insertions(+), 34 deletions(-) diff --git a/nullaway/src/main/java/com/uber/nullaway/NullAway.java b/nullaway/src/main/java/com/uber/nullaway/NullAway.java index 744bddd..f7ee780 100644 --- a/nullaway/src/main/java/com/uber/nullaway/NullAway.java +++ b/nullaway/src/main/java/com/uber/nullaway/NullAway.java @@ -760,10 +760,7 @@ public class NullAway extends BugChecker // Check handlers for any further/overriding nullness information overriddenMethodArgNullnessMap = handler.onOverrideMethodInvocationParametersNullability( - state.context, - overriddenMethod, - isOverriddenMethodAnnotated, - overriddenMethodArgNullnessMap); + state, overriddenMethod, isOverriddenMethodAnnotated, overriddenMethodArgNullnessMap); // If we have an unbound method reference, the first parameter of the overridden method must be // @NonNull, as this parameter will be used as a method receiver inside the generated lambda. @@ -1714,7 +1711,7 @@ public class NullAway extends BugChecker // Allow handlers to override the list of non-null argument positions argumentPositionNullness = handler.onOverrideMethodInvocationParametersNullability( - state.context, methodSymbol, isMethodAnnotated, argumentPositionNullness); + state, methodSymbol, isMethodAnnotated, argumentPositionNullness); // now actually check the arguments // NOTE: the case of an invocation on a possibly-null reference diff --git a/nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPathNullnessPropagation.java b/nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPathNullnessPropagation.java index b68665a..fb18368 100644 --- a/nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPathNullnessPropagation.java +++ b/nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPathNullnessPropagation.java @@ -206,7 +206,7 @@ public class AccessPathNullnessPropagation public NullnessStore initialStore( UnderlyingAST underlyingAST, List parameters) { return nullnessStoreInitializer.getInitialStore( - underlyingAST, parameters, handler, state.context, state.getTypes(), config); + underlyingAST, parameters, handler, state, config); } @Override diff --git a/nullaway/src/main/java/com/uber/nullaway/dataflow/CoreNullnessStoreInitializer.java b/nullaway/src/main/java/com/uber/nullaway/dataflow/CoreNullnessStoreInitializer.java index 2dfce9f..170ccb1 100644 --- a/nullaway/src/main/java/com/uber/nullaway/dataflow/CoreNullnessStoreInitializer.java +++ b/nullaway/src/main/java/com/uber/nullaway/dataflow/CoreNullnessStoreInitializer.java @@ -3,6 +3,7 @@ package com.uber.nullaway.dataflow; import static com.uber.nullaway.Nullness.NONNULL; import static com.uber.nullaway.Nullness.NULLABLE; +import com.google.errorprone.VisitorState; import com.google.errorprone.util.ASTHelpers; import com.sun.source.tree.ClassTree; import com.sun.source.tree.LambdaExpressionTree; @@ -30,9 +31,9 @@ class CoreNullnessStoreInitializer extends NullnessStoreInitializer { UnderlyingAST underlyingAST, List parameters, Handler handler, - Context context, - Types types, + VisitorState state, Config config) { + Context context = state.context; if (underlyingAST.getKind().equals(UnderlyingAST.Kind.ARBITRARY_CODE)) { // not a method or a lambda; an initializer expression or block UnderlyingAST.CFGStatement ast = (UnderlyingAST.CFGStatement) underlyingAST; @@ -44,8 +45,7 @@ class CoreNullnessStoreInitializer extends NullnessStoreInitializer { (UnderlyingAST.CFGLambda) underlyingAST, parameters, handler, - context, - types, + state, config, getCodeAnnotationInfo(context)); } else { @@ -77,13 +77,12 @@ class CoreNullnessStoreInitializer extends NullnessStoreInitializer { UnderlyingAST.CFGLambda underlyingAST, List parameters, Handler handler, - Context context, - Types types, + VisitorState state, Config config, CodeAnnotationInfo codeAnnotationInfo) { // include nullness info for locals from enclosing environment EnclosingEnvironmentNullness environmentNullness = - EnclosingEnvironmentNullness.instance(context); + EnclosingEnvironmentNullness.instance(state.context); NullnessStore environmentMapping = Objects.requireNonNull( environmentNullness.getEnvironmentMapping(underlyingAST.getLambdaTree()), @@ -91,6 +90,7 @@ class CoreNullnessStoreInitializer extends NullnessStoreInitializer { NullnessStore.Builder result = environmentMapping.toBuilder(); LambdaExpressionTree code = underlyingAST.getLambdaTree(); // need to check annotation for i'th parameter of functional interface declaration + Types types = state.getTypes(); Symbol.MethodSymbol fiMethodSymbol = NullabilityUtil.getFunctionalInterfaceMethod(code, types); com.sun.tools.javac.util.List fiMethodParameters = fiMethodSymbol.getParameters(); @@ -119,7 +119,7 @@ class CoreNullnessStoreInitializer extends NullnessStoreInitializer { } fiArgumentPositionNullness = handler.onOverrideMethodInvocationParametersNullability( - context, fiMethodSymbol, isFIAnnotated, fiArgumentPositionNullness); + state, fiMethodSymbol, isFIAnnotated, fiArgumentPositionNullness); for (int i = 0; i < parameters.size(); i++) { LocalVariableNode param = parameters.get(i); diff --git a/nullaway/src/main/java/com/uber/nullaway/dataflow/NullnessStoreInitializer.java b/nullaway/src/main/java/com/uber/nullaway/dataflow/NullnessStoreInitializer.java index c654621..37c68d3 100644 --- a/nullaway/src/main/java/com/uber/nullaway/dataflow/NullnessStoreInitializer.java +++ b/nullaway/src/main/java/com/uber/nullaway/dataflow/NullnessStoreInitializer.java @@ -1,10 +1,10 @@ package com.uber.nullaway.dataflow; +import com.google.errorprone.VisitorState; import com.google.errorprone.util.ASTHelpers; import com.sun.source.tree.ClassTree; import com.sun.source.util.Trees; import com.sun.tools.javac.code.Symbol; -import com.sun.tools.javac.code.Types; import com.sun.tools.javac.processing.JavacProcessingEnvironment; import com.sun.tools.javac.util.Context; import com.uber.nullaway.Config; @@ -30,8 +30,7 @@ public abstract class NullnessStoreInitializer { * @param underlyingAST The AST node being matched. * @param parameters list of local variable nodes. * @param handler reference to the handler invoked. - * @param context context. - * @param types types. + * @param state the visitor state. * @param config config for analysis. * @return Initial Nullness store. */ @@ -39,8 +38,7 @@ public abstract class NullnessStoreInitializer { UnderlyingAST underlyingAST, List parameters, Handler handler, - Context context, - Types types, + VisitorState state, Config config); /** diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/BaseNoOpHandler.java b/nullaway/src/main/java/com/uber/nullaway/handlers/BaseNoOpHandler.java index 242a96a..4fc0c0c 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/BaseNoOpHandler.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/BaseNoOpHandler.java @@ -120,7 +120,7 @@ public abstract class BaseNoOpHandler implements Handler { @Override public Nullness[] onOverrideMethodInvocationParametersNullability( - Context context, + VisitorState state, Symbol.MethodSymbol methodSymbol, boolean isAnnotated, Nullness[] argumentPositionNullness) { diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/CompositeHandler.java b/nullaway/src/main/java/com/uber/nullaway/handlers/CompositeHandler.java index 31617da..3602a5f 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/CompositeHandler.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/CompositeHandler.java @@ -137,14 +137,14 @@ class CompositeHandler implements Handler { @Override public Nullness[] onOverrideMethodInvocationParametersNullability( - Context context, + VisitorState state, Symbol.MethodSymbol methodSymbol, boolean isAnnotated, Nullness[] argumentPositionNullness) { for (Handler h : handlers) { argumentPositionNullness = h.onOverrideMethodInvocationParametersNullability( - context, methodSymbol, isAnnotated, argumentPositionNullness); + state, methodSymbol, isAnnotated, argumentPositionNullness); } return argumentPositionNullness; } diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/Handler.java b/nullaway/src/main/java/com/uber/nullaway/handlers/Handler.java index 835c01f..d509d7b 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/Handler.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/Handler.java @@ -182,7 +182,7 @@ public interface Handler { * considered isAnnotated or not. We use a mutable map for performance, but it should not outlive * the chain of handler invocations. * - * @param context The current context. + * @param state The current visitor state. * @param methodSymbol The method symbol for the method in question. * @param isAnnotated A boolean flag indicating whether the called method is considered to be * within isAnnotated or unannotated code, used to avoid querying for this information @@ -195,7 +195,7 @@ public interface Handler { * handler. */ Nullness[] onOverrideMethodInvocationParametersNullability( - Context context, + VisitorState state, Symbol.MethodSymbol methodSymbol, boolean isAnnotated, Nullness[] argumentPositionNullness); 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 bd70059..71dbd92 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/InferredJARModelsHandler.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/InferredJARModelsHandler.java @@ -28,7 +28,6 @@ import com.sun.source.tree.ExpressionTree; import com.sun.source.tree.Tree; import com.sun.tools.javac.code.Symbol; import com.sun.tools.javac.code.Type; -import com.sun.tools.javac.util.Context; import com.uber.nullaway.Config; import com.uber.nullaway.NullAway; import com.uber.nullaway.Nullness; @@ -122,7 +121,7 @@ public class InferredJARModelsHandler extends BaseNoOpHandler { @Override public Nullness[] onOverrideMethodInvocationParametersNullability( - Context context, + VisitorState state, Symbol.MethodSymbol methodSymbol, boolean isAnnotated, Nullness[] argumentPositionNullness) { 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 e000c4f..e5e943c 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java @@ -82,11 +82,11 @@ public class LibraryModelsHandler extends BaseNoOpHandler { @Override public Nullness[] onOverrideMethodInvocationParametersNullability( - Context context, + VisitorState state, Symbol.MethodSymbol methodSymbol, boolean isAnnotated, Nullness[] argumentPositionNullness) { - OptimizedLibraryModels optimizedLibraryModels = getOptLibraryModels(context); + OptimizedLibraryModels optimizedLibraryModels = getOptLibraryModels(state.context); ImmutableSet nullableParamsFromModel = optimizedLibraryModels.explicitlyNullableParameters(methodSymbol); ImmutableSet nonNullParamsFromModel = diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/LombokHandler.java b/nullaway/src/main/java/com/uber/nullaway/handlers/LombokHandler.java index 7069497..7d76611 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/LombokHandler.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/LombokHandler.java @@ -86,4 +86,26 @@ public class LombokHandler extends BaseNoOpHandler { } return returnNullness; } + + /** + * Mark the first argument of Lombok-generated {@code equals} methods as {@code @Nullable}, since + * Lombok does not generate the annotation. + */ + @Override + public Nullness[] onOverrideMethodInvocationParametersNullability( + VisitorState state, + Symbol.MethodSymbol methodSymbol, + boolean isAnnotated, + Nullness[] argumentPositionNullness) { + if (ASTHelpers.hasAnnotation(methodSymbol, LOMBOK_GENERATED_ANNOTATION_NAME, state)) { + // We assume that Lombok-generated equals methods with a single argument override + // Object.equals and are not an overload. + if (methodSymbol.getSimpleName().contentEquals("equals") + && methodSymbol.params().size() == 1) { + // The parameter is not annotated with @Nullable, but it should be. + argumentPositionNullness[0] = Nullness.NULLABLE; + } + } + return argumentPositionNullness; + } } diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/RestrictiveAnnotationHandler.java b/nullaway/src/main/java/com/uber/nullaway/handlers/RestrictiveAnnotationHandler.java index 561ac6e..7c5c9ba 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/RestrictiveAnnotationHandler.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/RestrictiveAnnotationHandler.java @@ -99,7 +99,7 @@ public class RestrictiveAnnotationHandler extends BaseNoOpHandler { @Override public Nullness[] onOverrideMethodInvocationParametersNullability( - Context context, + VisitorState state, Symbol.MethodSymbol methodSymbol, boolean isAnnotated, Nullness[] argumentPositionNullness) { diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/contract/ContractNullnessStoreInitializer.java b/nullaway/src/main/java/com/uber/nullaway/handlers/contract/ContractNullnessStoreInitializer.java index 41b25d5..155bd90 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/contract/ContractNullnessStoreInitializer.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/contract/ContractNullnessStoreInitializer.java @@ -3,12 +3,11 @@ package com.uber.nullaway.handlers.contract; import static com.uber.nullaway.Nullness.NONNULL; import static com.uber.nullaway.Nullness.NULLABLE; +import com.google.errorprone.VisitorState; import com.google.errorprone.util.ASTHelpers; import com.sun.source.tree.ClassTree; import com.sun.source.tree.MethodTree; import com.sun.tools.javac.code.Symbol; -import com.sun.tools.javac.code.Types; -import com.sun.tools.javac.util.Context; import com.uber.nullaway.Config; import com.uber.nullaway.Nullness; import com.uber.nullaway.dataflow.AccessPath; @@ -31,8 +30,7 @@ public class ContractNullnessStoreInitializer extends NullnessStoreInitializer { UnderlyingAST underlyingAST, List parameters, Handler handler, - Context context, - Types types, + VisitorState state, Config config) { assert underlyingAST.getKind() == UnderlyingAST.Kind.METHOD; @@ -49,7 +47,7 @@ public class ContractNullnessStoreInitializer extends NullnessStoreInitializer { String[] parts = clauses[0].split("->"); String[] antecedent = parts[0].split(","); - NullnessStore envStore = getEnvNullnessStoreForClass(classTree, context); + NullnessStore envStore = getEnvNullnessStoreForClass(classTree, state.context); NullnessStore.Builder result = envStore.toBuilder(); for (int i = 0; i < antecedent.length; ++i) { diff --git a/test-java-lib-lombok/src/main/java/com/uber/lombok/UsesDTO.java b/test-java-lib-lombok/src/main/java/com/uber/lombok/UsesDTO.java index f236a65..bef6f78 100644 --- a/test-java-lib-lombok/src/main/java/com/uber/lombok/UsesDTO.java +++ b/test-java-lib-lombok/src/main/java/com/uber/lombok/UsesDTO.java @@ -38,4 +38,9 @@ class UsesDTO { s += (ldto.getNullableField() == null ? "" : ldto.getNullableField().toString()); return s; } + + public static boolean callEquals(LombokDTO ldto, @Nullable Object o) { + // No error should be reported since equals() parameter should be treated as @Nullable + return ldto.equals(o); + } } -- cgit v1.2.3 From 3ee7072af654fbcd1ec9a76509db41635bc521ed Mon Sep 17 00:00:00 2001 From: Nima Karimipour Date: Thu, 14 Dec 2023 20:14:38 -0800 Subject: Extend library models to mark fields as nullable (#878) This PR extends `LibraryModels` to add support for marking fields `@Nullable`. This feature is required to enable [Annotator](https://github.com/ucr-riple/NullAwayAnnotator) index impact of making fields `@Nullable` on downstream dependencies. Please note, since this feature is mostly going to be used while indexing impacts on downstream dependencies, this PR does not focus on the optimality of the implementation. Also the working set in iterations is not expected to be large. We can optimize the implementation in a follow up PR if needed. --------- Co-authored-by: Manu Sridharan --- .../main/java/com/uber/nullaway/LibraryModels.java | 21 +++++++ .../src/main/java/com/uber/nullaway/NullAway.java | 3 +- .../uber/nullaway/handlers/BaseNoOpHandler.java | 6 ++ .../uber/nullaway/handlers/CompositeHandler.java | 12 ++++ .../java/com/uber/nullaway/handlers/Handler.java | 10 ++++ .../nullaway/handlers/LibraryModelsHandler.java | 68 ++++++++++++++++++++++ .../nullaway/NullAwayCustomLibraryModelsTests.java | 37 ++++++++++++ .../uber/modelexample/ExampleLibraryModels.java | 5 ++ .../lib/unannotated/UnannotatedWithModels.java | 4 ++ .../testlibrarymodels/TestLibraryModels.java | 10 ++++ 10 files changed, 175 insertions(+), 1 deletion(-) diff --git a/nullaway/src/main/java/com/uber/nullaway/LibraryModels.java b/nullaway/src/main/java/com/uber/nullaway/LibraryModels.java index a0816b3..1eeb219 100644 --- a/nullaway/src/main/java/com/uber/nullaway/LibraryModels.java +++ b/nullaway/src/main/java/com/uber/nullaway/LibraryModels.java @@ -22,6 +22,7 @@ package com.uber.nullaway; +import com.google.auto.value.AutoValue; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; import com.google.common.collect.ImmutableSetMultimap; @@ -131,6 +132,13 @@ public interface LibraryModels { */ ImmutableSetMultimap castToNonNullMethods(); + /** + * Get the set of library fields that may be {@code null}. + * + * @return set of library fields that may be {@code null}. + */ + ImmutableSet nullableFields(); + /** * Get a list of custom stream library specifications. * @@ -244,4 +252,17 @@ public interface LibraryModels { + '}'; } } + + /** Representation of a field as a qualified class name + a field name */ + @AutoValue + abstract class FieldRef { + + public abstract String getEnclosingClassName(); + + public abstract String getFieldName(); + + public static FieldRef fieldRef(String enclosingClass, String fieldName) { + return new AutoValue_LibraryModels_FieldRef(enclosingClass, fieldName); + } + } } diff --git a/nullaway/src/main/java/com/uber/nullaway/NullAway.java b/nullaway/src/main/java/com/uber/nullaway/NullAway.java index f7ee780..01a43ae 100644 --- a/nullaway/src/main/java/com/uber/nullaway/NullAway.java +++ b/nullaway/src/main/java/com/uber/nullaway/NullAway.java @@ -486,7 +486,8 @@ public class NullAway extends BugChecker return Description.NO_MATCH; } - if (Nullness.hasNullableAnnotation(assigned, config)) { + if (Nullness.hasNullableAnnotation(assigned, config) + || handler.onOverrideFieldNullability(assigned)) { // field already annotated return Description.NO_MATCH; } diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/BaseNoOpHandler.java b/nullaway/src/main/java/com/uber/nullaway/handlers/BaseNoOpHandler.java index 4fc0c0c..9260711 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/BaseNoOpHandler.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/BaseNoOpHandler.java @@ -118,6 +118,12 @@ public abstract class BaseNoOpHandler implements Handler { return returnNullness; } + @Override + public boolean onOverrideFieldNullability(Symbol field) { + // NoOp + return false; + } + @Override public Nullness[] onOverrideMethodInvocationParametersNullability( VisitorState state, diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/CompositeHandler.java b/nullaway/src/main/java/com/uber/nullaway/handlers/CompositeHandler.java index 3602a5f..f857d06 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/CompositeHandler.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/CompositeHandler.java @@ -135,6 +135,18 @@ class CompositeHandler implements Handler { return returnNullness; } + @Override + public boolean onOverrideFieldNullability(Symbol field) { + for (Handler h : handlers) { + if (h.onOverrideFieldNullability(field)) { + // If any handler determines that the field is @Nullable, we should acknowledge that and + // treat it as such. + return true; + } + } + return false; + } + @Override public Nullness[] onOverrideMethodInvocationParametersNullability( VisitorState state, diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/Handler.java b/nullaway/src/main/java/com/uber/nullaway/handlers/Handler.java index d509d7b..7033e40 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/Handler.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/Handler.java @@ -173,6 +173,16 @@ public interface Handler { boolean isAnnotated, Nullness returnNullness); + /** + * Called to potentially override the nullability of a field which is not annotated as @Nullable. + * If the field is decided to be @Nullable by this handler, the field should be treated + * as @Nullable anyway. + * + * @param field The symbol for the field in question. + * @return true if the field should be treated as @Nullable, false otherwise. + */ + boolean onOverrideFieldNullability(Symbol field); + /** * Called after the analysis determines the nullability of a method's arguments, allowing handlers * to override. 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 e5e943c..09f1c41 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java @@ -22,6 +22,7 @@ package com.uber.nullaway.handlers; +import static com.uber.nullaway.LibraryModels.FieldRef.fieldRef; import static com.uber.nullaway.LibraryModels.MethodRef.methodRef; import static com.uber.nullaway.Nullness.NONNULL; import static com.uber.nullaway.Nullness.NULLABLE; @@ -57,6 +58,7 @@ import java.util.ServiceLoader; import java.util.Set; import java.util.function.Function; import javax.annotation.Nullable; +import org.checkerframework.nullaway.dataflow.cfg.node.FieldAccessNode; import org.checkerframework.nullaway.dataflow.cfg.node.MethodInvocationNode; import org.checkerframework.nullaway.dataflow.cfg.node.Node; @@ -80,6 +82,25 @@ public class LibraryModelsHandler extends BaseNoOpHandler { libraryModels = loadLibraryModels(config); } + @Override + public boolean onOverrideFieldNullability(Symbol field) { + return isNullableFieldInLibraryModels(field); + } + + @Override + public NullnessHint onDataflowVisitFieldAccess( + FieldAccessNode node, + Symbol symbol, + Types types, + Context context, + AccessPath.AccessPathContext apContext, + AccessPathNullnessPropagation.SubNodeValues inputs, + AccessPathNullnessPropagation.Updates updates) { + return isNullableFieldInLibraryModels(symbol) + ? NullnessHint.HINT_NULLABLE + : NullnessHint.UNKNOWN; + } + @Override public Nullness[] onOverrideMethodInvocationParametersNullability( VisitorState state, @@ -132,6 +153,9 @@ public class LibraryModelsHandler extends BaseNoOpHandler { @Nullable Symbol exprSymbol, VisitorState state, boolean exprMayBeNull) { + if (isNullableFieldInLibraryModels(exprSymbol)) { + return true; + } if (!(expr.getKind() == Tree.Kind.METHOD_INVOCATION && exprSymbol instanceof Symbol.MethodSymbol)) { return exprMayBeNull; @@ -233,6 +257,32 @@ public class LibraryModelsHandler extends BaseNoOpHandler { } } + /** + * Check if the given symbol is a field that is marked as nullable in any of our library models. + * + * @param symbol The symbol to check. + * @return True if the symbol is a field that is marked as nullable in any of our library models. + */ + private boolean isNullableFieldInLibraryModels(@Nullable Symbol symbol) { + if (libraryModels.nullableFields().isEmpty()) { + // no need to do any work if there are no nullable fields. + return false; + } + if (symbol instanceof Symbol.VarSymbol && symbol.getKind().isField()) { + Symbol.VarSymbol varSymbol = (Symbol.VarSymbol) symbol; + Symbol.ClassSymbol classSymbol = varSymbol.enclClass(); + if (classSymbol == null) { + // e.g. .class expressions + return false; + } + String fieldName = varSymbol.getSimpleName().toString(); + String enclosingClassName = classSymbol.flatName().toString(); + // This check could be optimized further in the future if needed + return libraryModels.nullableFields().contains(fieldRef(enclosingClassName, fieldName)); + } + return false; + } + private void setConditionalArgumentNullness( AccessPathNullnessPropagation.Updates thenUpdates, AccessPathNullnessPropagation.Updates elseUpdates, @@ -824,6 +874,12 @@ public class LibraryModelsHandler extends BaseNoOpHandler { public ImmutableSetMultimap castToNonNullMethods() { return CAST_TO_NONNULL_METHODS; } + + @Override + public ImmutableSet nullableFields() { + // No nullable fields by default. + return ImmutableSet.of(); + } } private static class CombinedLibraryModels implements LibraryModels { @@ -846,6 +902,8 @@ public class LibraryModelsHandler extends BaseNoOpHandler { private final ImmutableSet nonNullReturns; + private final ImmutableSet nullableFields; + private final ImmutableSetMultimap castToNonNullMethods; private final ImmutableList customStreamNullabilitySpecs; @@ -870,6 +928,7 @@ public class LibraryModelsHandler extends BaseNoOpHandler { new ImmutableSetMultimap.Builder<>(); ImmutableList.Builder customStreamNullabilitySpecsBuilder = new ImmutableList.Builder<>(); + ImmutableSet.Builder nullableFieldsBuilder = new ImmutableSet.Builder<>(); for (LibraryModels libraryModels : models) { for (Map.Entry entry : libraryModels.failIfNullParameters().entries()) { if (shouldSkipModel(entry.getKey())) { @@ -932,6 +991,9 @@ public class LibraryModelsHandler extends BaseNoOpHandler { for (StreamTypeRecord streamTypeRecord : libraryModels.customStreamNullabilitySpecs()) { customStreamNullabilitySpecsBuilder.add(streamTypeRecord); } + for (FieldRef fieldRef : libraryModels.nullableFields()) { + nullableFieldsBuilder.add(fieldRef); + } } failIfNullParameters = failIfNullParametersBuilder.build(); explicitlyNullableParameters = explicitlyNullableParametersBuilder.build(); @@ -943,6 +1005,7 @@ public class LibraryModelsHandler extends BaseNoOpHandler { nonNullReturns = nonNullReturnsBuilder.build(); castToNonNullMethods = castToNonNullMethodsBuilder.build(); customStreamNullabilitySpecs = customStreamNullabilitySpecsBuilder.build(); + nullableFields = nullableFieldsBuilder.build(); } private boolean shouldSkipModel(MethodRef key) { @@ -989,6 +1052,11 @@ public class LibraryModelsHandler extends BaseNoOpHandler { return nonNullReturns; } + @Override + public ImmutableSet nullableFields() { + return nullableFields; + } + @Override public ImmutableSetMultimap castToNonNullMethods() { return castToNonNullMethods; diff --git a/nullaway/src/test/java/com/uber/nullaway/NullAwayCustomLibraryModelsTests.java b/nullaway/src/test/java/com/uber/nullaway/NullAwayCustomLibraryModelsTests.java index 937752c..098b98b 100644 --- a/nullaway/src/test/java/com/uber/nullaway/NullAwayCustomLibraryModelsTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/NullAwayCustomLibraryModelsTests.java @@ -227,4 +227,41 @@ public class NullAwayCustomLibraryModelsTests extends NullAwayTestsBase { "}") .doTest(); } + + @Test + public void libraryModelsAndOverridingFieldNullability() { + makeLibraryModelsTestHelperWithArgs( + Arrays.asList( + "-d", + temporaryFolder.getRoot().getAbsolutePath(), + "-XepOpt:NullAway:AnnotatedPackages=com.uber")) + .addSourceLines( + "Test.java", + "package com.uber;", + "import com.uber.lib.unannotated.UnannotatedWithModels;", + "public class Test {", + " UnannotatedWithModels uwm = new UnannotatedWithModels();", + " Object nonnullField = new Object();", + " void assignNullableFromLibraryModelField() {", + " // BUG: Diagnostic contains: assigning @Nullable", + " this.nonnullField = uwm.nullableFieldUnannotated1;", + " // BUG: Diagnostic contains: assigning @Nullable", + " this.nonnullField = uwm.nullableFieldUnannotated2;", + " }", + " void flowTest() {", + " if(uwm.nullableFieldUnannotated1 != null) {", + " // no error here, to check that library models only initialize flow store", + " this.nonnullField = uwm.nullableFieldUnannotated1;", + " }", + " }", + " String dereferenceTest() {", + " // BUG: Diagnostic contains: dereferenced expression uwm.nullableFieldUnannotated1 is @Nullable", + " return uwm.nullableFieldUnannotated1.toString();", + " }", + " void assignmentTest() {", + " uwm.nullableFieldUnannotated1 = null;", + " }", + "}") + .doTest(); + } } diff --git a/sample-library-model/src/main/java/com/uber/modelexample/ExampleLibraryModels.java b/sample-library-model/src/main/java/com/uber/modelexample/ExampleLibraryModels.java index a1aa99d..5c17aca 100644 --- a/sample-library-model/src/main/java/com/uber/modelexample/ExampleLibraryModels.java +++ b/sample-library-model/src/main/java/com/uber/modelexample/ExampleLibraryModels.java @@ -71,4 +71,9 @@ public class ExampleLibraryModels implements LibraryModels { public ImmutableSetMultimap castToNonNullMethods() { return ImmutableSetMultimap.of(); } + + @Override + public ImmutableSet nullableFields() { + return ImmutableSet.of(); + } } diff --git a/test-java-lib/src/main/java/com/uber/lib/unannotated/UnannotatedWithModels.java b/test-java-lib/src/main/java/com/uber/lib/unannotated/UnannotatedWithModels.java index 412042d..a978333 100644 --- a/test-java-lib/src/main/java/com/uber/lib/unannotated/UnannotatedWithModels.java +++ b/test-java-lib/src/main/java/com/uber/lib/unannotated/UnannotatedWithModels.java @@ -2,6 +2,10 @@ package com.uber.lib.unannotated; public class UnannotatedWithModels { + public Object nullableFieldUnannotated1; + + public Object nullableFieldUnannotated2; + public Object returnsNullUnannotated() { return null; } diff --git a/test-library-models/src/main/java/com/uber/nullaway/testlibrarymodels/TestLibraryModels.java b/test-library-models/src/main/java/com/uber/nullaway/testlibrarymodels/TestLibraryModels.java index c94dfef..01304a9 100644 --- a/test-library-models/src/main/java/com/uber/nullaway/testlibrarymodels/TestLibraryModels.java +++ b/test-library-models/src/main/java/com/uber/nullaway/testlibrarymodels/TestLibraryModels.java @@ -15,6 +15,7 @@ */ package com.uber.nullaway.testlibrarymodels; +import static com.uber.nullaway.LibraryModels.FieldRef.fieldRef; import static com.uber.nullaway.LibraryModels.MethodRef.methodRef; import com.google.auto.service.AutoService; @@ -122,4 +123,13 @@ public class TestLibraryModels implements LibraryModels { .withPassthroughMethodFromSignature("distinct()") .end(); } + + @Override + public ImmutableSet nullableFields() { + return ImmutableSet.builder() + .add( + fieldRef("com.uber.lib.unannotated.UnannotatedWithModels", "nullableFieldUnannotated1"), + fieldRef("com.uber.lib.unannotated.UnannotatedWithModels", "nullableFieldUnannotated2")) + .build(); + } } -- cgit v1.2.3 From 10761d9d1fcb1f11605fa081fc3fb1422bbb90d4 Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Wed, 20 Dec 2023 14:23:46 -0800 Subject: Update some dependencies (#883) Update Gradle to 8.5, Versions plugin to 0.50.0, and Spotless to 6.23.3. Just to stay up to date. --- build.gradle | 6 +++--- gradle/wrapper/gradle-wrapper.jar | Bin 63721 -> 43462 bytes gradle/wrapper/gradle-wrapper.properties | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/build.gradle b/build.gradle index 729ffc2..5ccd8b0 100644 --- a/build.gradle +++ b/build.gradle @@ -27,11 +27,11 @@ buildscript { } } plugins { - id "com.diffplug.spotless" version "6.21.0" + id "com.diffplug.spotless" version "6.23.3" id "net.ltgt.errorprone" version "3.1.0" apply false id "com.github.johnrengelman.shadow" version "8.1.1" apply false id "me.champeau.jmh" version "0.7.1" apply false - id "com.github.ben-manes.versions" version "0.48.0" + id "com.github.ben-manes.versions" version "0.50.0" id "com.felipefzdz.gradle.shellcheck" version "1.4.6" } @@ -118,7 +118,7 @@ spotless { } } spotlessPredeclare { - java { googleJavaFormat('1.17.0') } + java { googleJavaFormat('1.18.1') } groovyGradle { greclipse() } diff --git a/gradle/wrapper/gradle-wrapper.jar b/gradle/wrapper/gradle-wrapper.jar index 7f93135..d64cd49 100644 Binary files a/gradle/wrapper/gradle-wrapper.jar and b/gradle/wrapper/gradle-wrapper.jar differ diff --git a/gradle/wrapper/gradle-wrapper.properties b/gradle/wrapper/gradle-wrapper.properties index 46671ac..db8c3ba 100644 --- a/gradle/wrapper/gradle-wrapper.properties +++ b/gradle/wrapper/gradle-wrapper.properties @@ -1,7 +1,7 @@ distributionBase=GRADLE_USER_HOME distributionPath=wrapper/dists -distributionSha256Sum=3e1af3ae886920c3ac87f7a91f816c0c7c436f276a6eefdb3da152100fef72ae -distributionUrl=https\://services.gradle.org/distributions/gradle-8.4-bin.zip +distributionSha256Sum=9d926787066a081739e8200858338b4a69e837c3a821a33aca9db09dd4a41026 +distributionUrl=https\://services.gradle.org/distributions/gradle-8.5-bin.zip networkTimeout=10000 validateDistributionUrl=true zipStoreBase=GRADLE_USER_HOME -- cgit v1.2.3 From ce415992e46e726925b84f66fc728186277d672d Mon Sep 17 00:00:00 2001 From: arteghem <52428902+oliviernotteghem@users.noreply.github.com> Date: Thu, 21 Dec 2023 22:18:47 +0100 Subject: Fix jarinfer cli output determinism (#884) When invoking CLI, jar output sha is not constant. This breaks cacheability in build systems like Buck and Bazel. Binary investigation (see attached screenshot) of these jars show byte 0xa-0xd in PkZip header is what is changes, and corresponds to file modified date/time (see https://users.cs.jmu.edu/buchhofp/forensics/formats/pkzip.html). This PR is setting these times to 0 explicilty, so that jar sha remains constant between invocation. --------- Co-authored-by: Manu Sridharan --- .../uber/nullaway/jarinfer/BytecodeAnnotator.java | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/jar-infer/jar-infer-lib/src/main/java/com/uber/nullaway/jarinfer/BytecodeAnnotator.java b/jar-infer/jar-infer-lib/src/main/java/com/uber/nullaway/jarinfer/BytecodeAnnotator.java index 6931199..b0bc58e 100644 --- a/jar-infer/jar-infer-lib/src/main/java/com/uber/nullaway/jarinfer/BytecodeAnnotator.java +++ b/jar-infer/jar-infer-lib/src/main/java/com/uber/nullaway/jarinfer/BytecodeAnnotator.java @@ -212,6 +212,18 @@ public final class BytecodeAnnotator { annotateBytecode(is, os, nonnullParams, nullableReturns, javaxNullableDesc, javaxNonnullDesc); } + /** + * Create a zip entry with creation time of 0 to ensure that jars always have the same checksum. + * + * @param name of the zip entry. + * @return the zip entry. + */ + private static ZipEntry createZipEntry(String name) { + ZipEntry entry = new ZipEntry(name); + entry.setTime(0); + return entry; + } + private static void copyAndAnnotateJarEntry( JarEntry jarEntry, InputStream is, @@ -224,7 +236,7 @@ public final class BytecodeAnnotator { throws IOException { String entryName = jarEntry.getName(); if (entryName.endsWith(".class")) { - jarOS.putNextEntry(new ZipEntry(jarEntry.getName())); + jarOS.putNextEntry(createZipEntry(jarEntry.getName())); annotateBytecode(is, jarOS, nonnullParams, nullableReturns, nullableDesc, nonnullDesc); } else if (entryName.equals("META-INF/MANIFEST.MF")) { // Read full file @@ -241,7 +253,7 @@ public final class BytecodeAnnotator { if (!manifestText.equals(manifestMinusDigests) && !stripJarSignatures) { throw new SignedJarException(SIGNED_JAR_ERROR_MESSAGE); } - jarOS.putNextEntry(new ZipEntry(jarEntry.getName())); + jarOS.putNextEntry(createZipEntry(jarEntry.getName())); jarOS.write(manifestMinusDigests.getBytes(UTF_8)); } else if (entryName.startsWith("META-INF/") && (entryName.endsWith(".DSA") @@ -251,7 +263,7 @@ public final class BytecodeAnnotator { throw new SignedJarException(SIGNED_JAR_ERROR_MESSAGE); } // the case where stripJarSignatures==true is handled by default by skipping these files } else { - jarOS.putNextEntry(new ZipEntry(jarEntry.getName())); + jarOS.putNextEntry(createZipEntry(jarEntry.getName())); jarOS.write(IOUtils.toByteArray(is)); } jarOS.closeEntry(); @@ -329,7 +341,7 @@ public final class BytecodeAnnotator { while (zipIterator.hasNext()) { ZipEntry zipEntry = zipIterator.next(); InputStream is = inputZip.getInputStream(zipEntry); - zipOS.putNextEntry(new ZipEntry(zipEntry.getName())); + zipOS.putNextEntry(createZipEntry(zipEntry.getName())); if (zipEntry.getName().equals("classes.jar")) { JarInputStream jarIS = new JarInputStream(is); JarEntry inputJarEntry = jarIS.getNextJarEntry(); -- cgit v1.2.3 From c44ab8db09e261baee952b29fa368bdc661615a9 Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Tue, 26 Dec 2023 11:25:07 -0800 Subject: Add support for AssertJ as() and describedAs() in AssertionHandler (#885) Fixes #877 --- .../uber/nullaway/handlers/AssertionHandler.java | 42 ++++++++++---- .../com/uber/nullaway/handlers/MethodNameUtil.java | 21 +++++++ .../uber/nullaway/NullAwayAssertionLibsTests.java | 65 ++++++++++++++++++++++ 3 files changed, 117 insertions(+), 11 deletions(-) diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/AssertionHandler.java b/nullaway/src/main/java/com/uber/nullaway/handlers/AssertionHandler.java index 2a24923..676d3b4 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/AssertionHandler.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/AssertionHandler.java @@ -30,6 +30,7 @@ import com.sun.tools.javac.code.Symbol; import com.uber.nullaway.dataflow.AccessPath; import com.uber.nullaway.dataflow.AccessPathNullnessPropagation; import java.util.List; +import javax.annotation.Nullable; import org.checkerframework.nullaway.dataflow.cfg.node.MethodInvocationNode; import org.checkerframework.nullaway.dataflow.cfg.node.Node; @@ -60,17 +61,9 @@ public class AssertionHandler extends BaseNoOpHandler { // assertThat(A).isInstanceOf(Foo.class) // A will not be NULL after this statement. if (methodNameUtil.isMethodIsNotNull(callee) || methodNameUtil.isMethodIsInstanceOf(callee)) { - Node receiver = node.getTarget().getReceiver(); - if (receiver instanceof MethodInvocationNode) { - MethodInvocationNode receiver_method = (MethodInvocationNode) receiver; - Symbol.MethodSymbol receiver_symbol = ASTHelpers.getSymbol(receiver_method.getTree()); - if (methodNameUtil.isMethodAssertThat(receiver_symbol)) { - Node arg = receiver_method.getArgument(0); - AccessPath ap = AccessPath.getAccessPathForNode(arg, state, apContext); - if (ap != null) { - bothUpdates.set(ap, NONNULL); - } - } + AccessPath ap = getAccessPathForNotNullAssertThatExpr(node, state, apContext); + if (ap != null) { + bothUpdates.set(ap, NONNULL); } } @@ -94,4 +87,31 @@ public class AssertionHandler extends BaseNoOpHandler { return NullnessHint.UNKNOWN; } + + /** + * Returns the AccessPath for the argument of an assertThat() call, if present as a valid nested + * receiver expression of a method invocation + * + * @param node the method invocation node + * @param state the visitor state + * @param apContext the access path context + * @return the AccessPath for the argument of the assertThat() call, if present, otherwise {@code + * null} + */ + private @Nullable AccessPath getAccessPathForNotNullAssertThatExpr( + MethodInvocationNode node, VisitorState state, AccessPath.AccessPathContext apContext) { + Node receiver = node.getTarget().getReceiver(); + if (receiver instanceof MethodInvocationNode) { + MethodInvocationNode receiver_method = (MethodInvocationNode) receiver; + Symbol.MethodSymbol receiver_symbol = ASTHelpers.getSymbol(receiver_method.getTree()); + if (methodNameUtil.isMethodAssertThat(receiver_symbol)) { + Node arg = receiver_method.getArgument(0); + return AccessPath.getAccessPathForNode(arg, state, apContext); + } else if (methodNameUtil.isMethodAssertJDescribedAs(receiver_symbol)) { + // For calls to as() or describedAs(), we recursively search for the assertThat() call + return getAccessPathForNotNullAssertThatExpr(receiver_method, state, apContext); + } + } + return null; + } } diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/MethodNameUtil.java b/nullaway/src/main/java/com/uber/nullaway/handlers/MethodNameUtil.java index 51e9cd9..1a276bb 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/MethodNameUtil.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/MethodNameUtil.java @@ -56,6 +56,9 @@ class MethodNameUtil { private static final String IS_PRESENT_OWNER_ASSERTJ = "org.assertj.core.api.AbstractOptionalAssert"; private static final String ASSERT_THAT_METHOD = "assertThat"; + private static final String AS_METHOD = "as"; + private static final String DESCRIBED_AS_METHOD = "describedAs"; + private static final String ASSERT_THAT_OWNER_TRUTH = "com.google.common.truth.Truth"; private static final String ASSERT_THAT_OWNER_ASSERTJ = "org.assertj.core.api.Assertions"; @@ -101,6 +104,9 @@ class MethodNameUtil { private Name assertThatOwnerTruth; private Name assertThatOwnerAssertJ; + private Name as; + private Name describedAs; + // Names for junit assertion libraries. private Name hamcrestAssertClass; private Name junitAssertClass; @@ -141,6 +147,9 @@ class MethodNameUtil { assertThatOwnerTruth = table.fromString(ASSERT_THAT_OWNER_TRUTH); assertThatOwnerAssertJ = table.fromString(ASSERT_THAT_OWNER_ASSERTJ); + as = table.fromString(AS_METHOD); + describedAs = table.fromString(DESCRIBED_AS_METHOD); + isPresent = table.fromString(IS_PRESENT_METHOD); isNotEmpty = table.fromString(IS_NOT_EMPTY_METHOD); isPresentOwnerAssertJ = table.fromString(IS_PRESENT_OWNER_ASSERTJ); @@ -211,6 +220,18 @@ class MethodNameUtil { || matchesMethod(methodSymbol, assertThat, assertThatOwnerAssertJ); } + /** + * Returns true if the method is describedAs() or as() from AssertJ. Note that this implementation + * does not check the ower, as there are many possible implementations. This method should only be + * used in a caller content where it is clear that the operation is related to use of AssertJ. + * + * @param methodSymbol symbol for the method + * @return {@code true} iff the method is describedAs() or as() from AssertJ + */ + public boolean isMethodAssertJDescribedAs(Symbol.MethodSymbol methodSymbol) { + return methodSymbol.name.equals(as) || methodSymbol.name.equals(describedAs); + } + boolean isMethodHamcrestAssertThat(Symbol.MethodSymbol methodSymbol) { return matchesMethod(methodSymbol, assertThat, hamcrestAssertClass); } diff --git a/nullaway/src/test/java/com/uber/nullaway/NullAwayAssertionLibsTests.java b/nullaway/src/test/java/com/uber/nullaway/NullAwayAssertionLibsTests.java index 3a6838c..2967422 100644 --- a/nullaway/src/test/java/com/uber/nullaway/NullAwayAssertionLibsTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/NullAwayAssertionLibsTests.java @@ -365,6 +365,71 @@ public class NullAwayAssertionLibsTests extends NullAwayTestsBase { .doTest(); } + @Test + public void supportAssertJAssertThatIsNotNullWithDescription_Object() { + makeTestHelperWithArgs( + Arrays.asList( + "-d", + temporaryFolder.getRoot().getAbsolutePath(), + "-XepOpt:NullAway:AnnotatedPackages=com.uber", + "-XepOpt:NullAway:HandleTestAssertionLibraries=true")) + .addSourceLines( + "Test.java", + "package com.uber;", + "import java.lang.Object;", + "import java.util.Objects;", + "import javax.annotation.Nullable;", + "import static org.assertj.core.api.Assertions.assertThat;", + "class Test {", + " private void foo(@Nullable Object o) {", + " assertThat(o).as(\"test\").isNotNull();", + " o.toString();", + " }", + " private void foo2(@Nullable Object o) {", + " assertThat(o).describedAs(\"test\").isNotNull();", + " o.toString();", + " }", + " private void foo3(@Nullable Object o) {", + " assertThat(o).describedAs(\"test1\").as(\"test2\").isNotNull();", + " o.toString();", + " }", + "}") + .doTest(); + } + + @Test + public void assertJAssertThatIsNotNullUnhandled() { + makeTestHelperWithArgs( + Arrays.asList( + "-d", + temporaryFolder.getRoot().getAbsolutePath(), + "-XepOpt:NullAway:AnnotatedPackages=com.uber", + "-XepOpt:NullAway:HandleTestAssertionLibraries=true")) + .addSourceLines( + "Test.java", + "package com.uber;", + "import java.lang.Object;", + "import java.util.Objects;", + "import javax.annotation.Nullable;", + "import static org.assertj.core.api.Assertions.assertThat;", + "class Test {", + " private void foo(@Nullable Object o) {", + " org.assertj.core.api.ObjectAssert t = assertThat(o);", + " t.isNotNull();", + " // False positive", + " // BUG: Diagnostic contains: dereferenced expression", + " o.toString();", + " }", + " private void foo2(@Nullable Object o) {", + " assertThat(o).isEqualToIgnoringNullFields(o).describedAs(\"test\").isNotNull();", + " // False positive", + " // BUG: Diagnostic contains: dereferenced expression", + " o.toString();", + " }", + "}") + .doTest(); + } + @Test public void supportAssertJAssertThatIsNotNull_String() { makeTestHelperWithArgs( -- cgit v1.2.3 From 1a74cd4e2254161e1e117a524fbc743b8e1e1ee4 Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Tue, 26 Dec 2023 19:22:35 -0800 Subject: =?UTF-8?q?Revert=20"Model=20Lombok-generated=20equals=20methods?= =?UTF-8?q?=20as=20having=20a=20@Nullable=20p=E2=80=A6=20(#886)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit …arameter (#874)" This reverts commit 5fbee1ff8682762cd2f291070491fe3e476787cf. It turns out that this change requires a couple of other changes along with it, including #880 and better overall checking of overriding of `equals()` methods. We want to get a release out soon, so temporarily revert this change; we will restore it after cutting the release. --- .../src/main/java/com/uber/nullaway/NullAway.java | 7 +++++-- .../dataflow/AccessPathNullnessPropagation.java | 2 +- .../dataflow/CoreNullnessStoreInitializer.java | 16 ++++++++-------- .../dataflow/NullnessStoreInitializer.java | 8 +++++--- .../uber/nullaway/handlers/BaseNoOpHandler.java | 2 +- .../uber/nullaway/handlers/CompositeHandler.java | 4 ++-- .../java/com/uber/nullaway/handlers/Handler.java | 4 ++-- .../handlers/InferredJARModelsHandler.java | 3 ++- .../nullaway/handlers/LibraryModelsHandler.java | 4 ++-- .../com/uber/nullaway/handlers/LombokHandler.java | 22 ---------------------- .../handlers/RestrictiveAnnotationHandler.java | 2 +- .../contract/ContractNullnessStoreInitializer.java | 8 +++++--- .../src/main/java/com/uber/lombok/UsesDTO.java | 5 ----- 13 files changed, 34 insertions(+), 53 deletions(-) diff --git a/nullaway/src/main/java/com/uber/nullaway/NullAway.java b/nullaway/src/main/java/com/uber/nullaway/NullAway.java index 01a43ae..925fab0 100644 --- a/nullaway/src/main/java/com/uber/nullaway/NullAway.java +++ b/nullaway/src/main/java/com/uber/nullaway/NullAway.java @@ -761,7 +761,10 @@ public class NullAway extends BugChecker // Check handlers for any further/overriding nullness information overriddenMethodArgNullnessMap = handler.onOverrideMethodInvocationParametersNullability( - state, overriddenMethod, isOverriddenMethodAnnotated, overriddenMethodArgNullnessMap); + state.context, + overriddenMethod, + isOverriddenMethodAnnotated, + overriddenMethodArgNullnessMap); // If we have an unbound method reference, the first parameter of the overridden method must be // @NonNull, as this parameter will be used as a method receiver inside the generated lambda. @@ -1712,7 +1715,7 @@ public class NullAway extends BugChecker // Allow handlers to override the list of non-null argument positions argumentPositionNullness = handler.onOverrideMethodInvocationParametersNullability( - state, methodSymbol, isMethodAnnotated, argumentPositionNullness); + state.context, methodSymbol, isMethodAnnotated, argumentPositionNullness); // now actually check the arguments // NOTE: the case of an invocation on a possibly-null reference diff --git a/nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPathNullnessPropagation.java b/nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPathNullnessPropagation.java index fb18368..b68665a 100644 --- a/nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPathNullnessPropagation.java +++ b/nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPathNullnessPropagation.java @@ -206,7 +206,7 @@ public class AccessPathNullnessPropagation public NullnessStore initialStore( UnderlyingAST underlyingAST, List parameters) { return nullnessStoreInitializer.getInitialStore( - underlyingAST, parameters, handler, state, config); + underlyingAST, parameters, handler, state.context, state.getTypes(), config); } @Override diff --git a/nullaway/src/main/java/com/uber/nullaway/dataflow/CoreNullnessStoreInitializer.java b/nullaway/src/main/java/com/uber/nullaway/dataflow/CoreNullnessStoreInitializer.java index 170ccb1..2dfce9f 100644 --- a/nullaway/src/main/java/com/uber/nullaway/dataflow/CoreNullnessStoreInitializer.java +++ b/nullaway/src/main/java/com/uber/nullaway/dataflow/CoreNullnessStoreInitializer.java @@ -3,7 +3,6 @@ package com.uber.nullaway.dataflow; import static com.uber.nullaway.Nullness.NONNULL; import static com.uber.nullaway.Nullness.NULLABLE; -import com.google.errorprone.VisitorState; import com.google.errorprone.util.ASTHelpers; import com.sun.source.tree.ClassTree; import com.sun.source.tree.LambdaExpressionTree; @@ -31,9 +30,9 @@ class CoreNullnessStoreInitializer extends NullnessStoreInitializer { UnderlyingAST underlyingAST, List parameters, Handler handler, - VisitorState state, + Context context, + Types types, Config config) { - Context context = state.context; if (underlyingAST.getKind().equals(UnderlyingAST.Kind.ARBITRARY_CODE)) { // not a method or a lambda; an initializer expression or block UnderlyingAST.CFGStatement ast = (UnderlyingAST.CFGStatement) underlyingAST; @@ -45,7 +44,8 @@ class CoreNullnessStoreInitializer extends NullnessStoreInitializer { (UnderlyingAST.CFGLambda) underlyingAST, parameters, handler, - state, + context, + types, config, getCodeAnnotationInfo(context)); } else { @@ -77,12 +77,13 @@ class CoreNullnessStoreInitializer extends NullnessStoreInitializer { UnderlyingAST.CFGLambda underlyingAST, List parameters, Handler handler, - VisitorState state, + Context context, + Types types, Config config, CodeAnnotationInfo codeAnnotationInfo) { // include nullness info for locals from enclosing environment EnclosingEnvironmentNullness environmentNullness = - EnclosingEnvironmentNullness.instance(state.context); + EnclosingEnvironmentNullness.instance(context); NullnessStore environmentMapping = Objects.requireNonNull( environmentNullness.getEnvironmentMapping(underlyingAST.getLambdaTree()), @@ -90,7 +91,6 @@ class CoreNullnessStoreInitializer extends NullnessStoreInitializer { NullnessStore.Builder result = environmentMapping.toBuilder(); LambdaExpressionTree code = underlyingAST.getLambdaTree(); // need to check annotation for i'th parameter of functional interface declaration - Types types = state.getTypes(); Symbol.MethodSymbol fiMethodSymbol = NullabilityUtil.getFunctionalInterfaceMethod(code, types); com.sun.tools.javac.util.List fiMethodParameters = fiMethodSymbol.getParameters(); @@ -119,7 +119,7 @@ class CoreNullnessStoreInitializer extends NullnessStoreInitializer { } fiArgumentPositionNullness = handler.onOverrideMethodInvocationParametersNullability( - state, fiMethodSymbol, isFIAnnotated, fiArgumentPositionNullness); + context, fiMethodSymbol, isFIAnnotated, fiArgumentPositionNullness); for (int i = 0; i < parameters.size(); i++) { LocalVariableNode param = parameters.get(i); diff --git a/nullaway/src/main/java/com/uber/nullaway/dataflow/NullnessStoreInitializer.java b/nullaway/src/main/java/com/uber/nullaway/dataflow/NullnessStoreInitializer.java index 37c68d3..c654621 100644 --- a/nullaway/src/main/java/com/uber/nullaway/dataflow/NullnessStoreInitializer.java +++ b/nullaway/src/main/java/com/uber/nullaway/dataflow/NullnessStoreInitializer.java @@ -1,10 +1,10 @@ package com.uber.nullaway.dataflow; -import com.google.errorprone.VisitorState; import com.google.errorprone.util.ASTHelpers; import com.sun.source.tree.ClassTree; import com.sun.source.util.Trees; import com.sun.tools.javac.code.Symbol; +import com.sun.tools.javac.code.Types; import com.sun.tools.javac.processing.JavacProcessingEnvironment; import com.sun.tools.javac.util.Context; import com.uber.nullaway.Config; @@ -30,7 +30,8 @@ public abstract class NullnessStoreInitializer { * @param underlyingAST The AST node being matched. * @param parameters list of local variable nodes. * @param handler reference to the handler invoked. - * @param state the visitor state. + * @param context context. + * @param types types. * @param config config for analysis. * @return Initial Nullness store. */ @@ -38,7 +39,8 @@ public abstract class NullnessStoreInitializer { UnderlyingAST underlyingAST, List parameters, Handler handler, - VisitorState state, + Context context, + Types types, Config config); /** diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/BaseNoOpHandler.java b/nullaway/src/main/java/com/uber/nullaway/handlers/BaseNoOpHandler.java index 9260711..815b46d 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/BaseNoOpHandler.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/BaseNoOpHandler.java @@ -126,7 +126,7 @@ public abstract class BaseNoOpHandler implements Handler { @Override public Nullness[] onOverrideMethodInvocationParametersNullability( - VisitorState state, + Context context, Symbol.MethodSymbol methodSymbol, boolean isAnnotated, Nullness[] argumentPositionNullness) { diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/CompositeHandler.java b/nullaway/src/main/java/com/uber/nullaway/handlers/CompositeHandler.java index f857d06..2001269 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/CompositeHandler.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/CompositeHandler.java @@ -149,14 +149,14 @@ class CompositeHandler implements Handler { @Override public Nullness[] onOverrideMethodInvocationParametersNullability( - VisitorState state, + Context context, Symbol.MethodSymbol methodSymbol, boolean isAnnotated, Nullness[] argumentPositionNullness) { for (Handler h : handlers) { argumentPositionNullness = h.onOverrideMethodInvocationParametersNullability( - state, methodSymbol, isAnnotated, argumentPositionNullness); + context, methodSymbol, isAnnotated, argumentPositionNullness); } return argumentPositionNullness; } diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/Handler.java b/nullaway/src/main/java/com/uber/nullaway/handlers/Handler.java index 7033e40..b2018ee 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/Handler.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/Handler.java @@ -192,7 +192,7 @@ public interface Handler { * considered isAnnotated or not. We use a mutable map for performance, but it should not outlive * the chain of handler invocations. * - * @param state The current visitor state. + * @param context The current context. * @param methodSymbol The method symbol for the method in question. * @param isAnnotated A boolean flag indicating whether the called method is considered to be * within isAnnotated or unannotated code, used to avoid querying for this information @@ -205,7 +205,7 @@ public interface Handler { * handler. */ Nullness[] onOverrideMethodInvocationParametersNullability( - VisitorState state, + Context context, Symbol.MethodSymbol methodSymbol, boolean isAnnotated, Nullness[] argumentPositionNullness); 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 71dbd92..bd70059 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/InferredJARModelsHandler.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/InferredJARModelsHandler.java @@ -28,6 +28,7 @@ import com.sun.source.tree.ExpressionTree; import com.sun.source.tree.Tree; import com.sun.tools.javac.code.Symbol; import com.sun.tools.javac.code.Type; +import com.sun.tools.javac.util.Context; import com.uber.nullaway.Config; import com.uber.nullaway.NullAway; import com.uber.nullaway.Nullness; @@ -121,7 +122,7 @@ public class InferredJARModelsHandler extends BaseNoOpHandler { @Override public Nullness[] onOverrideMethodInvocationParametersNullability( - VisitorState state, + Context context, Symbol.MethodSymbol methodSymbol, boolean isAnnotated, Nullness[] argumentPositionNullness) { 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 09f1c41..fe87278 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java @@ -103,11 +103,11 @@ public class LibraryModelsHandler extends BaseNoOpHandler { @Override public Nullness[] onOverrideMethodInvocationParametersNullability( - VisitorState state, + Context context, Symbol.MethodSymbol methodSymbol, boolean isAnnotated, Nullness[] argumentPositionNullness) { - OptimizedLibraryModels optimizedLibraryModels = getOptLibraryModels(state.context); + OptimizedLibraryModels optimizedLibraryModels = getOptLibraryModels(context); ImmutableSet nullableParamsFromModel = optimizedLibraryModels.explicitlyNullableParameters(methodSymbol); ImmutableSet nonNullParamsFromModel = diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/LombokHandler.java b/nullaway/src/main/java/com/uber/nullaway/handlers/LombokHandler.java index 7d76611..7069497 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/LombokHandler.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/LombokHandler.java @@ -86,26 +86,4 @@ public class LombokHandler extends BaseNoOpHandler { } return returnNullness; } - - /** - * Mark the first argument of Lombok-generated {@code equals} methods as {@code @Nullable}, since - * Lombok does not generate the annotation. - */ - @Override - public Nullness[] onOverrideMethodInvocationParametersNullability( - VisitorState state, - Symbol.MethodSymbol methodSymbol, - boolean isAnnotated, - Nullness[] argumentPositionNullness) { - if (ASTHelpers.hasAnnotation(methodSymbol, LOMBOK_GENERATED_ANNOTATION_NAME, state)) { - // We assume that Lombok-generated equals methods with a single argument override - // Object.equals and are not an overload. - if (methodSymbol.getSimpleName().contentEquals("equals") - && methodSymbol.params().size() == 1) { - // The parameter is not annotated with @Nullable, but it should be. - argumentPositionNullness[0] = Nullness.NULLABLE; - } - } - return argumentPositionNullness; - } } diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/RestrictiveAnnotationHandler.java b/nullaway/src/main/java/com/uber/nullaway/handlers/RestrictiveAnnotationHandler.java index 7c5c9ba..561ac6e 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/RestrictiveAnnotationHandler.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/RestrictiveAnnotationHandler.java @@ -99,7 +99,7 @@ public class RestrictiveAnnotationHandler extends BaseNoOpHandler { @Override public Nullness[] onOverrideMethodInvocationParametersNullability( - VisitorState state, + Context context, Symbol.MethodSymbol methodSymbol, boolean isAnnotated, Nullness[] argumentPositionNullness) { diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/contract/ContractNullnessStoreInitializer.java b/nullaway/src/main/java/com/uber/nullaway/handlers/contract/ContractNullnessStoreInitializer.java index 155bd90..41b25d5 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/contract/ContractNullnessStoreInitializer.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/contract/ContractNullnessStoreInitializer.java @@ -3,11 +3,12 @@ package com.uber.nullaway.handlers.contract; import static com.uber.nullaway.Nullness.NONNULL; import static com.uber.nullaway.Nullness.NULLABLE; -import com.google.errorprone.VisitorState; import com.google.errorprone.util.ASTHelpers; import com.sun.source.tree.ClassTree; import com.sun.source.tree.MethodTree; import com.sun.tools.javac.code.Symbol; +import com.sun.tools.javac.code.Types; +import com.sun.tools.javac.util.Context; import com.uber.nullaway.Config; import com.uber.nullaway.Nullness; import com.uber.nullaway.dataflow.AccessPath; @@ -30,7 +31,8 @@ public class ContractNullnessStoreInitializer extends NullnessStoreInitializer { UnderlyingAST underlyingAST, List parameters, Handler handler, - VisitorState state, + Context context, + Types types, Config config) { assert underlyingAST.getKind() == UnderlyingAST.Kind.METHOD; @@ -47,7 +49,7 @@ public class ContractNullnessStoreInitializer extends NullnessStoreInitializer { String[] parts = clauses[0].split("->"); String[] antecedent = parts[0].split(","); - NullnessStore envStore = getEnvNullnessStoreForClass(classTree, state.context); + NullnessStore envStore = getEnvNullnessStoreForClass(classTree, context); NullnessStore.Builder result = envStore.toBuilder(); for (int i = 0; i < antecedent.length; ++i) { diff --git a/test-java-lib-lombok/src/main/java/com/uber/lombok/UsesDTO.java b/test-java-lib-lombok/src/main/java/com/uber/lombok/UsesDTO.java index bef6f78..f236a65 100644 --- a/test-java-lib-lombok/src/main/java/com/uber/lombok/UsesDTO.java +++ b/test-java-lib-lombok/src/main/java/com/uber/lombok/UsesDTO.java @@ -38,9 +38,4 @@ class UsesDTO { s += (ldto.getNullableField() == null ? "" : ldto.getNullableField().toString()); return s; } - - public static boolean callEquals(LombokDTO ldto, @Nullable Object o) { - // No error should be reported since equals() parameter should be treated as @Nullable - return ldto.equals(o); - } } -- cgit v1.2.3 From c6fcc920e5980c3f41e6c71c7352dc78c87e0413 Mon Sep 17 00:00:00 2001 From: Lazaro Clapp Date: Wed, 27 Dec 2023 13:30:07 -0500 Subject: Prepare for release 0.10.19. --- CHANGELOG.md | 11 +++++++++++ gradle.properties | 2 +- 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 09a549f..18033f9 100755 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,16 @@ Changelog ========= +Version 0.10.19 +--------------- +* Update to Checker Framework 3.41.0 (#873) +* Extend library models to mark fields as nullable (#878) + - Main use case is NullAwayAnnotator +* Fix jarinfer cli output determinism (#884) +* Add support for AssertJ as() and describedAs() in AssertionHandler (#885) +* Support for JSpecify's 0.3.0 annotation [experimental] + - JSpecify: In generics code, get rid of checks for ClassType (#863) +* Update some dependencies (#883) + Version 0.10.18 --------------- * Fix assertion check for structure of enhanced-for loop over a Map keySet (#868) diff --git a/gradle.properties b/gradle.properties index bbb3fa7..89a9218 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.10.19-SNAPSHOT +VERSION_NAME=0.10.19 POM_DESCRIPTION=A fast annotation-based null checker for Java -- cgit v1.2.3 From 6da0b8dc43b7f71d0287847f865f39cd3b4b11d4 Mon Sep 17 00:00:00 2001 From: Lazaro Clapp Date: Wed, 27 Dec 2023 15:14:38 -0500 Subject: Prepare next development version. --- gradle.properties | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gradle.properties b/gradle.properties index 89a9218..39843d9 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.10.19 +VERSION_NAME=0.10.20-SNAPSHOT POM_DESCRIPTION=A fast annotation-based null checker for Java -- cgit v1.2.3 From b5cd1f9010f1a09ddb4b9f81dcade81e195362ab Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Wed, 3 Jan 2024 12:57:03 -0500 Subject: Update to WALA 1.6.3 (#887) Fixes #829 WALA 1.6.3 supports running on JDK 21, so JarInfer now runs on JDK 21 and the tests pass. --- gradle/dependencies.gradle | 2 +- jar-infer/jar-infer-lib/build.gradle | 6 ------ 2 files changed, 1 insertion(+), 7 deletions(-) diff --git a/gradle/dependencies.gradle b/gradle/dependencies.gradle index 26fc151..9916951 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.2", + wala : "1.6.3", commonscli : "1.4", autoValue : "1.10.2", autoService : "1.1.1", diff --git a/jar-infer/jar-infer-lib/build.gradle b/jar-infer/jar-infer-lib/build.gradle index 2ae8bea..8ea4f9b 100644 --- a/jar-infer/jar-infer-lib/build.gradle +++ b/jar-infer/jar-infer-lib/build.gradle @@ -51,12 +51,6 @@ test { dependsOn ':jar-infer:test-android-lib-jarinfer:bundleReleaseAar' } -tasks.named('testJdk21', Test).configure { - // Tests fail since WALA does not yet support JDK 21; see https://github.com/uber/NullAway/issues/829 - // So, disable them - onlyIf { false } -} - tasks.withType(JavaCompile).configureEach { options.compilerArgs += "--add-exports=jdk.compiler/com.sun.tools.javac.main=ALL-UNNAMED" } -- cgit v1.2.3 From 257e4bb36f97776ebd4e5a95205186e4f5a4a694 Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Wed, 3 Jan 2024 14:14:42 -0500 Subject: Update to Error Prone 2.24.1 (#888) Just to stay up to date. --- .github/workflows/continuous-integration.yml | 10 +++++----- gradle/dependencies.gradle | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/.github/workflows/continuous-integration.yml b/.github/workflows/continuous-integration.yml index 62adc2f..00c76ab 100644 --- a/.github/workflows/continuous-integration.yml +++ b/.github/workflows/continuous-integration.yml @@ -21,16 +21,16 @@ jobs: epVersion: 2.10.0 - os: macos-latest java: 11 - epVersion: 2.23.0 + epVersion: 2.24.1 - os: ubuntu-latest java: 11 - epVersion: 2.23.0 + epVersion: 2.24.1 - os: windows-latest java: 11 - epVersion: 2.23.0 + epVersion: 2.24.1 - os: ubuntu-latest java: 17 - epVersion: 2.23.0 + epVersion: 2.24.1 fail-fast: false runs-on: ${{ matrix.os }} steps: @@ -63,7 +63,7 @@ jobs: with: arguments: codeCoverageReport continue-on-error: true - if: runner.os == 'Linux' && matrix.java == '11' && matrix.epVersion == '2.23.0' && github.repository == 'uber/NullAway' + if: runner.os == 'Linux' && matrix.java == '11' && matrix.epVersion == '2.24.1' && github.repository == 'uber/NullAway' - name: Upload coverage reports to Codecov uses: codecov/codecov-action@v3 with: diff --git a/gradle/dependencies.gradle b/gradle/dependencies.gradle index 9916951..01bbe70 100755 --- a/gradle/dependencies.gradle +++ b/gradle/dependencies.gradle @@ -19,7 +19,7 @@ import org.gradle.util.VersionNumber // The oldest version of Error Prone that we support running on def oldestErrorProneVersion = "2.10.0" // Latest released Error Prone version that we've tested with -def latestErrorProneVersion = "2.23.0" +def latestErrorProneVersion = "2.24.1" // Default to using latest tested Error Prone version def defaultErrorProneVersion = latestErrorProneVersion def errorProneVersionToCompileAgainst = defaultErrorProneVersion -- cgit v1.2.3 From 97771dc580aec84ae86acae3a1d5952bc1cb4368 Mon Sep 17 00:00:00 2001 From: Abhijit Kulkarni Date: Wed, 10 Jan 2024 08:23:18 -0800 Subject: Fix JSpecify support on JDK 21 (#869) Our JSpecify mode would not run on JDK 21 due some some internal javac API changes. This PR adapts to those changes using code specific to JDK 21. Fixes #827 --------- Co-authored-by: Manu Sridharan --- .../groovy/nullaway.java-test-conventions.gradle | 36 ++--- nullaway/build.gradle | 8 -- .../src/main/java/com/uber/nullaway/NullAway.java | 18 ++- .../generics/PreservedAnnotationTreeVisitor.java | 151 ++++++++++++++++++++- 4 files changed, 180 insertions(+), 33 deletions(-) diff --git a/buildSrc/src/main/groovy/nullaway.java-test-conventions.gradle b/buildSrc/src/main/groovy/nullaway.java-test-conventions.gradle index 07f8f26..6a0e641 100644 --- a/buildSrc/src/main/groovy/nullaway.java-test-conventions.gradle +++ b/buildSrc/src/main/groovy/nullaway.java-test-conventions.gradle @@ -44,22 +44,6 @@ configurations.create('transitiveSourcesElements') { } } -// Share the coverage data to be aggregated for the whole product -configurations.create('coverageDataElements') { - visible = false - canBeResolved = false - canBeConsumed = true - extendsFrom(configurations.implementation) - attributes { - attribute(Usage.USAGE_ATTRIBUTE, objects.named(Usage, Usage.JAVA_RUNTIME)) - attribute(Category.CATEGORY_ATTRIBUTE, objects.named(Category, Category.DOCUMENTATION)) - attribute(DocsType.DOCS_TYPE_ATTRIBUTE, objects.named(DocsType, 'jacoco-coverage-data')) - } - // This will cause the test task to run if the coverage data is requested by the aggregation task - outgoing.artifact(tasks.named("test").map { task -> - task.extensions.getByType(JacocoTaskExtension).destinationFile - }) -} test { maxHeapSize = "1024m" @@ -118,3 +102,23 @@ def testJdk21 = tasks.register("testJdk21", Test) { tasks.named('check').configure { dependsOn testJdk21 } + + +// Share the coverage data to be aggregated for the whole product +configurations.create('coverageDataElements') { + visible = false + canBeResolved = false + canBeConsumed = true + extendsFrom(configurations.implementation) + attributes { + attribute(Usage.USAGE_ATTRIBUTE, objects.named(Usage, Usage.JAVA_RUNTIME)) + attribute(Category.CATEGORY_ATTRIBUTE, objects.named(Category, Category.DOCUMENTATION)) + attribute(DocsType.DOCS_TYPE_ATTRIBUTE, objects.named(DocsType, 'jacoco-coverage-data')) + } + // This will cause the test tasks to run if the coverage data is requested by the aggregation task + tasks.withType(Test).forEach {task -> + // tasks.named(task.name) is a hack to introduce the right task dependence in Gradle. We will fix this + // correctly when addressing https://github.com/uber/NullAway/issues/871 + outgoing.artifact(tasks.named(task.name).map { t -> t.extensions.getByType(JacocoTaskExtension).destinationFile }) + } +} diff --git a/nullaway/build.gradle b/nullaway/build.gradle index 90ac062..829917f 100644 --- a/nullaway/build.gradle +++ b/nullaway/build.gradle @@ -124,14 +124,6 @@ tasks.named('check').configure { dependsOn(jdk8Test) } -tasks.named('testJdk21', Test).configure { - filter { - // JSpecify Generics tests do not yet pass on JDK 21 - // See https://github.com/uber/NullAway/issues/827 - excludeTestsMatching "com.uber.nullaway.NullAwayJSpecifyGenericsTests" - } -} - // Create a task to build NullAway with NullAway checking enabled tasks.register('buildWithNullAway', JavaCompile) { onlyIf { diff --git a/nullaway/src/main/java/com/uber/nullaway/NullAway.java b/nullaway/src/main/java/com/uber/nullaway/NullAway.java index 925fab0..1ab3b12 100644 --- a/nullaway/src/main/java/com/uber/nullaway/NullAway.java +++ b/nullaway/src/main/java/com/uber/nullaway/NullAway.java @@ -476,7 +476,7 @@ public class NullAway extends BugChecker doUnboxingCheck(state, tree.getExpression()); } // generics check - if (lhsType != null && lhsType.getTypeArguments().length() > 0) { + if (lhsType != null && lhsType.getTypeArguments().length() > 0 && config.isJSpecifyMode()) { GenericsChecks.checkTypeParameterNullnessForAssignability(tree, this, state); } @@ -695,7 +695,9 @@ public class NullAway extends BugChecker if (!withinAnnotatedCode(state)) { return Description.NO_MATCH; } - GenericsChecks.checkInstantiationForParameterizedTypedTree(tree, state, this, config); + if (config.isJSpecifyMode()) { + GenericsChecks.checkInstantiationForParameterizedTypedTree(tree, state, this, config); + } return Description.NO_MATCH; } @@ -1424,7 +1426,7 @@ public class NullAway extends BugChecker return Description.NO_MATCH; } VarSymbol symbol = ASTHelpers.getSymbol(tree); - if (tree.getInitializer() != null) { + if (tree.getInitializer() != null && config.isJSpecifyMode()) { GenericsChecks.checkTypeParameterNullnessForAssignability(tree, this, state); } @@ -1577,7 +1579,9 @@ public class NullAway extends BugChecker public Description matchConditionalExpression( ConditionalExpressionTree tree, VisitorState state) { if (withinAnnotatedCode(state)) { - GenericsChecks.checkTypeParameterNullnessForConditionalExpression(tree, this, state); + if (config.isJSpecifyMode()) { + GenericsChecks.checkTypeParameterNullnessForConditionalExpression(tree, this, state); + } doUnboxingCheck(state, tree.getCondition()); } return Description.NO_MATCH; @@ -1708,8 +1712,10 @@ public class NullAway extends BugChecker : Nullness.NONNULL); } } - GenericsChecks.compareGenericTypeParameterNullabilityForCall( - formalParams, actualParams, methodSymbol.isVarArgs(), this, state); + if (config.isJSpecifyMode()) { + GenericsChecks.compareGenericTypeParameterNullabilityForCall( + formalParams, actualParams, methodSymbol.isVarArgs(), this, state); + } } // Allow handlers to override the list of non-null argument positions diff --git a/nullaway/src/main/java/com/uber/nullaway/generics/PreservedAnnotationTreeVisitor.java b/nullaway/src/main/java/com/uber/nullaway/generics/PreservedAnnotationTreeVisitor.java index 2cde64f..822fcf1 100644 --- a/nullaway/src/main/java/com/uber/nullaway/generics/PreservedAnnotationTreeVisitor.java +++ b/nullaway/src/main/java/com/uber/nullaway/generics/PreservedAnnotationTreeVisitor.java @@ -14,6 +14,10 @@ import com.sun.source.util.SimpleTreeVisitor; import com.sun.tools.javac.code.Attribute; import com.sun.tools.javac.code.Type; import com.sun.tools.javac.code.TypeMetadata; +import com.sun.tools.javac.util.ListBuffer; +import java.lang.invoke.MethodHandle; +import java.lang.invoke.MethodHandles; +import java.lang.invoke.MethodType; import java.util.ArrayList; import java.util.Collections; import java.util.List; @@ -74,10 +78,10 @@ public class PreservedAnnotationTreeVisitor extends SimpleTreeVisitor attrs); + + Type cloneTypeWithMetadata(Type typeToBeCloned, TypeMetadata metaData); + } + + /** + * Provides implementations for methods under TypeMetadataBuilder compatible with JDK 17 and + * earlier versions. + */ + private static class JDK17AndEarlierTypeMetadataBuilder implements TypeMetadataBuilder { + + @Override + public TypeMetadata create(com.sun.tools.javac.util.List attrs) { + return new TypeMetadata(new TypeMetadata.Annotations(attrs)); + } + + /** + * Clones the given type with the specified Metadata for getting the right nullability + * annotations. + * + * @param typeToBeCloned The Type we want to clone with the required Nullability Metadata + * @param metadata The required Nullability metadata which is lost from the type + * @return Type after it has been cloned by applying the required Nullability metadata + */ + @Override + public Type cloneTypeWithMetadata(Type typeToBeCloned, TypeMetadata metadata) { + return typeToBeCloned.cloneWithMetadata(metadata); + } + } + + /** + * Provides implementations for methods under TypeMetadataBuilder compatible with the updates made + * to the library methods for Jdk 21. The implementation calls the logic specific to JDK 21 + * indirectly using MethodHandles since we still need the code to compile on earlier versions. + */ + private static class JDK21TypeMetadataBuilder implements TypeMetadataBuilder { + + private static final MethodHandle typeMetadataHandle = createHandle(); + private static final MethodHandle addMetadataHandle = + createVirtualMethodHandle(Type.class, TypeMetadata.class, Type.class, "addMetadata"); + private static final MethodHandle dropMetadataHandle = + createVirtualMethodHandle(Type.class, Class.class, Type.class, "dropMetadata"); + + private static MethodHandle createHandle() { + MethodHandles.Lookup lookup = MethodHandles.lookup(); + MethodType mt = MethodType.methodType(void.class, com.sun.tools.javac.util.ListBuffer.class); + try { + return lookup.findConstructor(TypeMetadata.Annotations.class, mt); + } catch (NoSuchMethodException e) { + throw new RuntimeException(e); + } catch (IllegalAccessException e) { + throw new RuntimeException(e); + } + } + + /** + * Used to get a MethodHandle for a virtual method from the specified class + * + * @param retTypeClass Class to indicate the return type of the desired method + * @param paramTypeClass Class to indicate the parameter type of the desired method + * @param refClass Class within which the desired method is contained + * @param methodName Name of the desired method + * @return The appropriate MethodHandle for the virtual method + */ + private static MethodHandle createVirtualMethodHandle( + Class retTypeClass, Class paramTypeClass, Class refClass, String methodName) { + MethodHandles.Lookup lookup = MethodHandles.lookup(); + MethodType mt = MethodType.methodType(retTypeClass, paramTypeClass); + try { + return lookup.findVirtual(refClass, methodName, mt); + } catch (NoSuchMethodException e) { + throw new RuntimeException(e); + } catch (IllegalAccessException e) { + throw new RuntimeException(e); + } + } + + @Override + public TypeMetadata create(com.sun.tools.javac.util.List attrs) { + ListBuffer b = new ListBuffer<>(); + b.appendList(attrs); + try { + return (TypeMetadata) typeMetadataHandle.invoke(b); + } catch (Throwable e) { + throw new RuntimeException(e); + } + } + + /** + * Calls dropMetadata and addMetadata using MethodHandles for JDK 21, which removed the previous + * cloneWithMetadata method. + * + * @param typeToBeCloned The Type we want to clone with the required Nullability metadata + * @param metadata The required Nullability metadata + * @return Cloned Type with the necessary Nullability metadata + */ + @Override + public Type cloneTypeWithMetadata(Type typeToBeCloned, TypeMetadata metadata) { + try { + // In JDK 21 addMetadata works if there is no metadata associated with the type, so we + // create a copy without the existing metadata first and then add it + Type clonedTypeWithoutMetadata = + (Type) dropMetadataHandle.invoke(typeToBeCloned, metadata.getClass()); + return (Type) addMetadataHandle.invoke(clonedTypeWithoutMetadata, metadata); + } catch (Throwable e) { + throw new RuntimeException(e); + } + } + } + + /** The TypeMetadataBuilder to be used for the current JDK version. */ + private static final TypeMetadataBuilder TYPE_METADATA_BUILDER = + getVersion() >= 21 + ? new JDK21TypeMetadataBuilder() + : new JDK17AndEarlierTypeMetadataBuilder(); + + /** + * Utility method to get the current JDK version, that works on Java 8 and above. + * + *

TODO remove this method once we drop support for Java 8 + * + * @return the current JDK version + */ + private static int getVersion() { + String version = System.getProperty("java.version"); + if (version.startsWith("1.")) { + version = version.substring(2, 3); + } else { + int dot = version.indexOf("."); + if (dot != -1) { + version = version.substring(0, dot); + } + } + return Integer.parseInt(version); + } } -- cgit v1.2.3 From bb81c97e4fdfd541e756b1c1907cb02ae8995a46 Mon Sep 17 00:00:00 2001 From: Yuxin Wang Date: Fri, 12 Jan 2024 11:50:54 -0500 Subject: Prepare for release 0.10.20. --- CHANGELOG.md | 65 ++++++++++++++++++++++++++++++------------------------- gradle.properties | 2 +- 2 files changed, 37 insertions(+), 30 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 18033f9..8ae1571 100755 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,12 @@ Changelog ========= +Version 0.10.20 +--------------- +* Fix JSpecify support on JDK 21 (#869) +* Build / CI tooling upgrades for NullAway itself: + - Update to WALA 1.6.3 (#887) + - Update to Error Prone 2.24.1 (#888) + Version 0.10.19 --------------- * Update to Checker Framework 3.41.0 (#873) @@ -110,7 +117,7 @@ Note: This is the first release built with Java 11. In particular, running - Update to Error Prone 2.20.0 (#772) - Add tasks to run JDK 8 tests on JDK 11+ (#778) - Switch to Spotless for formatting Java code (#780) - - Added GCP JMH Benchmark Workflow (#770) + - Added GCP JMH Benchmark Workflow (#770) - Set concurrency for JMH benchmarking workflow (#784) - Disable daemon when running benchmarks (#786) - Update to Gradle 8.2.1 (#781) @@ -194,7 +201,7 @@ Version 0.10.6 * Fix logic for @Nullable annotation on type parameter (#702) * Preserve nullness checks in final fields when propagating nullness into inner contexts (#703) * NullAwayInfer/Annotator data serialization support [experimental] - - Add source offset and path to reported errors in error serialization. (#704) + - Add source offset and path to reported errors in error serialization. (#704) * Build / CI tooling for NullAway itself: - [Jspecify] Update test dep to final JSpecify 0.3.0 release (#700) = Intermediate PRs: 0.3.0-alpha-3 (#692), 0.3-alpha2 (#691) @@ -226,10 +233,10 @@ Version 0.10.3 -------------- * Report an error when casting @Nullable expression to primitive type (#663) * Fix an NPE in the optional emptiness handler (#678) -* Add support for boolean constraints (about nullness) in Contract annotations (#669) +* Add support for boolean constraints (about nullness) in Contract annotations (#669) * Support for specific libraries/APIs: - PreconditionsHandler reflects Guava Preconditions exception types (#668) - - Handle Guava Verify functions (#682) + - Handle Guava Verify functions (#682) * Dependency Updates: - checkerframework 3.26.0 (#671) * Build / CI tooling for NullAway itself: @@ -238,7 +245,7 @@ Version 0.10.3 Version 0.10.2 -------------- -* Make AbstractConfig collection fields explicity Immutable (#601) +* Make AbstractConfig collection fields explicity Immutable (#601) * NullAwayInfer/Annotator data serialization support [experimental] - Fix crash in fixserialization when ClassSymbol.sourcefile is null (#656) @@ -302,7 +309,7 @@ Version 0.9.8 - Model for com.google.api.client.util.Strings.isNullOrEmpty (#605) * Refactoring: - Cleanups to AccessPath representation and implementation (#603) - - Clean-up: Remove unused fix suggestion code. (#615) + - Clean-up: Remove unused fix suggestion code. (#615) * Dependency Updates: - Update to Checker Framework 3.22.2 (#610) * Build / CI tooling for NullAway itself: @@ -354,7 +361,7 @@ Version 0.9.6 * Build / CI tooling for NullAway itself: - Enable parallel builds (#549) (#555) - Add dependence from coveralls task to codeCoverageReport (#552) - - Switch to temurin on CI (#553) + - Switch to temurin on CI (#553) - Separating NullAwayTests into smaller files (#550) - Require braces for all conditionals and loops (#556) - Enable build cache (#562) @@ -364,7 +371,7 @@ Version 0.9.6 - Update CI jobs (#565) - Set epApiVersion for jacoco coverage reporting (#566) - Compile and test against Error Prone 2.11.0 (#567) - - Fix EP version for jacoco coverage step (#571) + - Fix EP version for jacoco coverage step (#571) - Update to latest Google Java Format (#572) Version 0.9.5 @@ -386,7 +393,7 @@ Version 0.9.3 ------------- IMPORTANT: This version introduces EXPERIMENTAL JDK17 support. There is a known crash on lambdas with switch expressions as body - (see #524). Best current workaround is to + (see #524). Best current workaround is to `@SuppressWarnings("NullAway")` on the enclosing method * Improve reporting of multiple parameter errors on a single method call (#503) * Support compile-time constant field args in method Access Paths (#504) @@ -478,13 +485,13 @@ Version 0.8.0 Version 0.7.10 -------------- -* Add Java 8 streams nullness-propagation support (#371) +* Add Java 8 streams nullness-propagation support (#371) * Give line numbers for uninitialized fields when reporting error on an initializer (#380) -* Include outer$inner class name when reporting field init errors (#375) +* Include outer$inner class name when reporting field init errors (#375) * Update to Gradle 6.1.1 (#381) * Add @MonotonicNonNull as lazy initialization annotation. (#383) * Add default library model for CompilationUnitTree.getPackageName() (#384) -* Improve matching of native Map methods (#390) +* Improve matching of native Map methods (#390) - Fixes an IndexOutOfBoundsException checker crash Version 0.7.9 @@ -494,14 +501,14 @@ Version 0.7.9 - WALA to 1.5.4 (#337) - Checker Dataflow to 3.0.0 (#369) * Added OPTIONAL_CONTENT synthetic field to track Optional emptiness (#364) - - With this, `-XepOpt:NullAway:CheckOptionalEmptiness` should be + - With this, `-XepOpt:NullAway:CheckOptionalEmptiness` should be ready for use. * Handle Nullchk operator (#368) Version 0.7.8 ------------- -* Added NullAway.Optional suppression (#359) -* [JarInfer] Ignore non-public classes when inferring annotations. (#360) +* Added NullAway.Optional suppression (#359) +* [JarInfer] Ignore non-public classes when inferring annotations. (#360) Version 0.7.7 ------------- @@ -537,7 +544,7 @@ Version 0.7.4 * Refactor the driver and annotation summary type in JarInfer (#317) * Minor refactor and cleanup in JarInfer-lib (#319) * Different approach for param analysis (#320) -* Fix @NullableDecl support (#324) +* Fix @NullableDecl support (#324) * Treat methods of final classes as final for initialization. (#325) Version 0.7.3 @@ -551,7 +558,7 @@ Version 0.7.3 Version 0.7.2 ------------- -* Install GJF hook using a gradle task, rather than a gradlew hack (#298). +* Install GJF hook using a gradle task, rather than a gradlew hack (#298). * Nullable switch expression support (#300). * Upgrade to Error Prone 2.3.3 (#295). Update Gradle, Error Prone plugin, and Android Gradle Plugin (#294). @@ -572,7 +579,7 @@ Version 0.7.0 Version 0.6.6 --------------- -This only adds a minor library fix supporting Guava's Preconditions.checkNotNull with an error message +This only adds a minor library fix supporting Guava's Preconditions.checkNotNull with an error message argument (#283) Version 0.6.5 @@ -602,16 +609,16 @@ Version 0.6.2 Version 0.6.1 ------------- * Enable excluded class annotations to (mostly) work on inner classes (#239) -* Assertion of not equal to null updates the access path (#240) +* Assertion of not equal to null updates the access path (#240) * Update Gradle examples in README (#244) * Change how jarinfer finds astubx model jars. (#243) * Update to Error Prone 2.3.2 (#242) * Update net.ltgt.errorprone to 0.6, and build updates ((#248) -* Restrictive annotated method overriding (#249) - Note: This can require significant annotation changes if +* Restrictive annotated method overriding (#249) + Note: This can require significant annotation changes if `-XepOpt:NullAway:AcknowledgeRestrictiveAnnotations=true` is set. Not a new minor version, since that option is false by default. -* Fix error on checking the initTree2PrevFieldInit cache. (#252) +* Fix error on checking the initTree2PrevFieldInit cache. (#252) * Add support for renamed android.support packages in models. (#253) Version 0.6.0 @@ -619,12 +626,12 @@ Version 0.6.0 * Add support for marking library parameters as explicitly @Nullable (#228) * De-genericize NullnessStore (#231) * Bump Checker Framework to 2.5.5 (#233) -* Pass nullability info on enclosing locals into dataflow analysis for +* Pass nullability info on enclosing locals into dataflow analysis for lambdas and anonymous / local classes (#235) Version 0.5.6 ------------- -* Add coverage measurement through coveralls. (#224) +* Add coverage measurement through coveralls. (#224) * Fix empty comment added when AutoFixSuppressionComment is not set. (#225) * Make JarInfer generated jars fully deterministic by removing timestamps. (#227) @@ -657,7 +664,7 @@ android-jarinfer-models-sdk28 artifacts Version 0.5.2 ------------- * Fix NPE in Thrift handler on complex receiver expressions (#195) -* Add ExcludedFieldAnnotations unit tests. (#192) +* Add ExcludedFieldAnnotations unit tests. (#192) * Various crash fixes (#196) * Fix @NonNull argument detection in RestrictiveAnnotationHandler. (#198) @@ -683,7 +690,7 @@ Version 0.4.7 Version 0.4.6 ------------- * Fix a couple of Thrift issues (#164) -* Don't report initialization warnings on fields for @ExternalInit classes with +* Don't report initialization warnings on fields for @ExternalInit classes with no initializer methods (#166) Version 0.4.5 @@ -732,7 +739,7 @@ Version 0.3.6 Version 0.3.5 ------------- -* Support for treating `@Generated`-annotated classes as unannotated (#127) +* Support for treating `@Generated`-annotated classes as unannotated (#127) Version 0.3.4 ------------- @@ -804,8 +811,8 @@ Version 0.1.5 ------------- * Add finer grained suppressions and auto-fixes (#31). You can suppress initialization errors specifically now with - `@SuppressWarnings("NullAway.Init")` -* Fix performance issue with lambdas (#29) + `@SuppressWarnings("NullAway.Init")` +* Fix performance issue with lambdas (#29) * Add lambda support to the RxNullabilityPropagator handler. (#12) Version 0.1.4 diff --git a/gradle.properties b/gradle.properties index 39843d9..64ce2f7 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.10.20-SNAPSHOT +VERSION_NAME=0.10.20 POM_DESCRIPTION=A fast annotation-based null checker for Java -- cgit v1.2.3 From abff73e5aed79bea0967bbfe50cae4463117eea1 Mon Sep 17 00:00:00 2001 From: Yuxin Wang Date: Fri, 12 Jan 2024 13:50:17 -0500 Subject: Prepare next development version. --- gradle.properties | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gradle.properties b/gradle.properties index 64ce2f7..37d5362 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.10.20 +VERSION_NAME=0.10.21-SNAPSHOT POM_DESCRIPTION=A fast annotation-based null checker for Java -- cgit v1.2.3 From 9ff44a7fc9190ae493f58ff516a1f2ec54404aea Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Sat, 13 Jan 2024 18:07:20 -0800 Subject: Fix backwards-incompatible calls to ASTHelpers.hasDirectAnnotationWithSimpleName (#894) Error Prone 2.24.0 introduces new overloads of `ASTHelpers.hasDirectionAnnotationWithSimpleName`; see https://github.com/google/error-prone/commit/aa6e3e7be223e5f8319609281b229d4db4219654. This causes backward compatibility issues when compiling against EP 2.24.0+ and then running on an older version of Error Prone. We introduce a wrapper method to avoid this issue. We also modify our `testJdk8` tasks to properly test the scenario of compiling against the newest supported Error Prone version and then running on the oldest supported version, which would have caught this issue. Ideally we would have our own EP check preventing direct calls to `ASTHelpers.hasDirectionAnnotationWithSimpleName`, but that can be done in a follow-up. --- gradle/dependencies.gradle | 2 ++ nullaway/build.gradle | 22 +++++++++++++------ .../com/uber/nullaway/ASTHelpersBackports.java | 20 ++++++++++++++++- .../java/com/uber/nullaway/CodeAnnotationInfo.java | 25 ++++++++++------------ .../src/main/java/com/uber/nullaway/NullAway.java | 13 +++++------ 5 files changed, 54 insertions(+), 28 deletions(-) diff --git a/gradle/dependencies.gradle b/gradle/dependencies.gradle index 01bbe70..5962a7c 100755 --- a/gradle/dependencies.gradle +++ b/gradle/dependencies.gradle @@ -67,10 +67,12 @@ def build = [ asm : "org.ow2.asm:asm:${versions.asm}", asmTree : "org.ow2.asm:asm-tree:${versions.asm}", errorProneCheckApi : "com.google.errorprone:error_prone_check_api:${versions.errorProneApi}", + errorProneCheckApiOld : "com.google.errorprone:error_prone_check_api:${oldestErrorProneVersion}", errorProneCore : "com.google.errorprone:error_prone_core:${versions.errorProne}", errorProneCoreForApi : "com.google.errorprone:error_prone_core:${versions.errorProneApi}", errorProneJavac : "com.google.errorprone:javac:9+181-r4173-1", errorProneTestHelpers : "com.google.errorprone:error_prone_test_helpers:${versions.errorProneApi}", + errorProneTestHelpersOld: "com.google.errorprone:error_prone_test_helpers:${oldestErrorProneVersion}", checkerDataflow : "org.checkerframework:dataflow-nullaway:${versions.checkerFramework}", guava : "com.google.guava:guava:30.1-jre", javaxValidation : "javax.validation:validation-api:2.0.1.Final", diff --git a/nullaway/build.gradle b/nullaway/build.gradle index 829917f..7d39adc 100644 --- a/nullaway/build.gradle +++ b/nullaway/build.gradle @@ -21,7 +21,8 @@ plugins { } configurations { - nullawayJar + // A configuration holding the jars for the oldest supported version of Error Prone, to use with tests + errorProneOldest } dependencies { @@ -64,6 +65,11 @@ dependencies { testImplementation deps.test.mockito testImplementation deps.test.javaxAnnotationApi testImplementation deps.test.assertJ + + errorProneOldest deps.build.errorProneCheckApiOld + errorProneOldest(deps.build.errorProneTestHelpersOld) { + exclude group: "junit", module: "junit" + } } javadoc { @@ -93,12 +99,10 @@ apply plugin: 'com.vanniktech.maven.publish' // } // Create a task to test on JDK 8 +// NOTE: even when we drop JDK 8 support, we will still need a test task similar to this one for testing building +// against a recent JDK and Error Prone version but then running on the oldest supported JDK and Error Prone version, +// to check for binary compatibility issues. def jdk8Test = tasks.register("testJdk8", Test) { - onlyIf { - // Only if we are using a version of Error Prone compatible with JDK 8 - deps.versions.errorProneApi == "2.10.0" - } - javaLauncher = javaToolchains.launcherFor { languageVersion = JavaLanguageVersion.of(8) } @@ -108,7 +112,11 @@ def jdk8Test = tasks.register("testJdk8", Test) { // Copy inputs from normal Test task. def testTask = tasks.getByName("test") - classpath = testTask.classpath + // A bit of a hack: we add the dependencies of the oldest supported Error Prone version to the _beginning_ of the + // classpath, so that they are used instead of the latest version. This exercises the scenario of building + // NullAway against the latest supported Error Prone version but then running on the oldest supported version. + classpath = configurations.errorProneOldest + testTask.classpath + testClassesDirs = testTask.testClassesDirs jvmArgs "-Xbootclasspath/p:${configurations.errorproneJavac.asPath}" filter { diff --git a/nullaway/src/main/java/com/uber/nullaway/ASTHelpersBackports.java b/nullaway/src/main/java/com/uber/nullaway/ASTHelpersBackports.java index 06a91f9..47dc99f 100644 --- a/nullaway/src/main/java/com/uber/nullaway/ASTHelpersBackports.java +++ b/nullaway/src/main/java/com/uber/nullaway/ASTHelpersBackports.java @@ -1,12 +1,15 @@ package com.uber.nullaway; +import com.google.errorprone.util.ASTHelpers; import com.sun.tools.javac.code.Symbol; import java.util.List; /** * Methods backported from {@link com.google.errorprone.util.ASTHelpers} since we do not yet require * a recent-enough Error Prone version. The methods should be removed once we bump our minimum Error - * Prone version accordingly. + * Prone version accordingly. We also include methods where new overloads have been added in recent + * Error Prone versions, to ensure binary compatibility when compiling against a new version but + * running on an old version. */ public class ASTHelpersBackports { @@ -36,4 +39,19 @@ public class ASTHelpersBackports { public static List getEnclosedElements(Symbol symbol) { return symbol.getEnclosedElements(); } + + /** + * A wrapper for {@link ASTHelpers#hasDirectAnnotationWithSimpleName(Symbol, String)} to avoid + * binary compatibility issues with new overloads in recent Error Prone versions. NullAway code + * should only use this method and not call the corresponding ASTHelpers methods directly. + * + *

TODO: delete this method and switch to ASTHelpers once we can require Error Prone 2.24.0 + * + * @param sym the symbol + * @param simpleName the simple name + * @return {@code true} iff the symbol has a direct annotation with the given simple name + */ + public static boolean hasDirectAnnotationWithSimpleName(Symbol sym, String simpleName) { + return ASTHelpers.hasDirectAnnotationWithSimpleName(sym, simpleName); + } } diff --git a/nullaway/src/main/java/com/uber/nullaway/CodeAnnotationInfo.java b/nullaway/src/main/java/com/uber/nullaway/CodeAnnotationInfo.java index 013e8f4..37fcc03 100644 --- a/nullaway/src/main/java/com/uber/nullaway/CodeAnnotationInfo.java +++ b/nullaway/src/main/java/com/uber/nullaway/CodeAnnotationInfo.java @@ -22,6 +22,7 @@ package com.uber.nullaway; +import static com.uber.nullaway.ASTHelpersBackports.hasDirectAnnotationWithSimpleName; import static com.uber.nullaway.NullabilityUtil.castToNonNull; import com.google.common.base.Preconditions; @@ -81,7 +82,7 @@ public final class CodeAnnotationInfo { Symbol.PackageSymbol enclosingPackage = ASTHelpers.enclosingPackage(outermostClassSymbol); if (!config.fromExplicitlyAnnotatedPackage(className) && !(enclosingPackage != null - && ASTHelpers.hasDirectAnnotationWithSimpleName( + && hasDirectAnnotationWithSimpleName( enclosingPackage, NullabilityUtil.NULLMARKED_SIMPLE_NAME))) { // By default, unknown code is unannotated unless @NullMarked or configured as annotated by // package name @@ -89,7 +90,7 @@ public final class CodeAnnotationInfo { } if (config.fromExplicitlyUnannotatedPackage(className) || (enclosingPackage != null - && ASTHelpers.hasDirectAnnotationWithSimpleName( + && hasDirectAnnotationWithSimpleName( enclosingPackage, NullabilityUtil.NULLUNMARKED_SIMPLE_NAME))) { // Any code explicitly marked as unannotated in our configuration is unannotated, no matter // what. Similarly, any package annotated as @NullUnmarked is unannotated, even if @@ -120,7 +121,7 @@ public final class CodeAnnotationInfo { return false; } Symbol.ClassSymbol outermostClassSymbol = get(classSymbol, config).outermostClassSymbol; - return ASTHelpers.hasDirectAnnotationWithSimpleName(outermostClassSymbol, "Generated"); + return hasDirectAnnotationWithSimpleName(outermostClassSymbol, "Generated"); } /** @@ -216,10 +217,10 @@ public final class CodeAnnotationInfo { if (enclosingMethod != null) { isAnnotated = recordForEnclosing.isMethodNullnessAnnotated(enclosingMethod); } - if (ASTHelpers.hasDirectAnnotationWithSimpleName( + if (hasDirectAnnotationWithSimpleName( classSymbol, NullabilityUtil.NULLUNMARKED_SIMPLE_NAME)) { isAnnotated = false; - } else if (ASTHelpers.hasDirectAnnotationWithSimpleName( + } else if (hasDirectAnnotationWithSimpleName( classSymbol, NullabilityUtil.NULLMARKED_SIMPLE_NAME)) { isAnnotated = true; } @@ -244,7 +245,7 @@ public final class CodeAnnotationInfo { // Generated code is or isn't excluded, depending on configuration // Note: In the future, we might want finer grain controls to distinguish code that is // generated with nullability info and without. - if (ASTHelpers.hasDirectAnnotationWithSimpleName(classSymbol, "Generated")) { + if (hasDirectAnnotationWithSimpleName(classSymbol, "Generated")) { return true; } ImmutableSet generatedCodeAnnotations = config.getGeneratedCodeAnnotations(); @@ -259,13 +260,11 @@ public final class CodeAnnotationInfo { private boolean isAnnotatedTopLevelClass(Symbol.ClassSymbol classSymbol, Config config) { // First, check for an explicitly @NullUnmarked top level class - if (ASTHelpers.hasDirectAnnotationWithSimpleName( - classSymbol, NullabilityUtil.NULLUNMARKED_SIMPLE_NAME)) { + if (hasDirectAnnotationWithSimpleName(classSymbol, NullabilityUtil.NULLUNMARKED_SIMPLE_NAME)) { return false; } // Then, check if the class has a @NullMarked annotation or comes from an annotated package - if ((ASTHelpers.hasDirectAnnotationWithSimpleName( - classSymbol, NullabilityUtil.NULLMARKED_SIMPLE_NAME) + if ((hasDirectAnnotationWithSimpleName(classSymbol, NullabilityUtil.NULLMARKED_SIMPLE_NAME) || fromAnnotatedPackage(classSymbol, config))) { // make sure it's not explicitly configured as unannotated return !shouldTreatAsUnannotated(classSymbol, config); @@ -295,14 +294,12 @@ public final class CodeAnnotationInfo { return methodNullnessCache.computeIfAbsent( methodSymbol, m -> { - if (ASTHelpers.hasDirectAnnotationWithSimpleName( - m, NullabilityUtil.NULLUNMARKED_SIMPLE_NAME)) { + if (hasDirectAnnotationWithSimpleName(m, NullabilityUtil.NULLUNMARKED_SIMPLE_NAME)) { return false; } else if (this.isNullnessAnnotated) { return true; } else { - return ASTHelpers.hasDirectAnnotationWithSimpleName( - m, NullabilityUtil.NULLMARKED_SIMPLE_NAME); + return hasDirectAnnotationWithSimpleName(m, NullabilityUtil.NULLMARKED_SIMPLE_NAME); } }); } diff --git a/nullaway/src/main/java/com/uber/nullaway/NullAway.java b/nullaway/src/main/java/com/uber/nullaway/NullAway.java index 1ab3b12..3bf0a85 100644 --- a/nullaway/src/main/java/com/uber/nullaway/NullAway.java +++ b/nullaway/src/main/java/com/uber/nullaway/NullAway.java @@ -28,6 +28,7 @@ import static com.sun.source.tree.Tree.Kind.IDENTIFIER; import static com.sun.source.tree.Tree.Kind.OTHER; import static com.sun.source.tree.Tree.Kind.PARENTHESIZED; import static com.sun.source.tree.Tree.Kind.TYPE_CAST; +import static com.uber.nullaway.ASTHelpersBackports.hasDirectAnnotationWithSimpleName; import static com.uber.nullaway.ASTHelpersBackports.isStatic; import static com.uber.nullaway.ErrorBuilder.errMsgForInitializer; import static com.uber.nullaway.NullabilityUtil.castToNonNull; @@ -578,20 +579,20 @@ public class NullAway extends BugChecker Symbol.MethodSymbol methodSymbol = ASTHelpers.getSymbol(tree); switch (nullMarkingForTopLevelClass) { case FULLY_MARKED: - if (ASTHelpers.hasDirectAnnotationWithSimpleName( + if (hasDirectAnnotationWithSimpleName( methodSymbol, NullabilityUtil.NULLUNMARKED_SIMPLE_NAME)) { nullMarkingForTopLevelClass = NullMarking.PARTIALLY_MARKED; } break; case FULLY_UNMARKED: - if (ASTHelpers.hasDirectAnnotationWithSimpleName( + if (hasDirectAnnotationWithSimpleName( methodSymbol, NullabilityUtil.NULLMARKED_SIMPLE_NAME)) { nullMarkingForTopLevelClass = NullMarking.PARTIALLY_MARKED; markedMethodInUnmarkedContext = true; } break; case PARTIALLY_MARKED: - if (ASTHelpers.hasDirectAnnotationWithSimpleName( + if (hasDirectAnnotationWithSimpleName( methodSymbol, NullabilityUtil.NULLMARKED_SIMPLE_NAME)) { // We still care here if this is a transition between @NullUnmarked and @NullMarked code, // within partially marked code, see checks below for markedMethodInUnmarkedContext. @@ -1467,10 +1468,10 @@ public class NullAway extends BugChecker */ private boolean classAnnotationIntroducesPartialMarking(Symbol.ClassSymbol classSymbol) { return (nullMarkingForTopLevelClass == NullMarking.FULLY_UNMARKED - && ASTHelpers.hasDirectAnnotationWithSimpleName( + && hasDirectAnnotationWithSimpleName( classSymbol, NullabilityUtil.NULLMARKED_SIMPLE_NAME)) || (nullMarkingForTopLevelClass == NullMarking.FULLY_MARKED - && ASTHelpers.hasDirectAnnotationWithSimpleName( + && hasDirectAnnotationWithSimpleName( classSymbol, NullabilityUtil.NULLUNMARKED_SIMPLE_NAME)); } @@ -2240,7 +2241,7 @@ public class NullAway extends BugChecker } private boolean isInitializerMethod(VisitorState state, Symbol.MethodSymbol symbol) { - if (ASTHelpers.hasDirectAnnotationWithSimpleName(symbol, "Initializer") + if (hasDirectAnnotationWithSimpleName(symbol, "Initializer") || config.isKnownInitializerMethod(symbol)) { return true; } -- cgit v1.2.3 From c7007fd73405683ab089f766179a94c8cf98e692 Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Sat, 13 Jan 2024 18:16:58 -0800 Subject: Downgrade to Checker Framework 3.40.0 (#895) This fixes a regression where we were seeing a crash in CFG construction. Reported upstream as https://github.com/typetools/checker-framework/issues/6396. --- gradle/dependencies.gradle | 2 +- nullaway/build.gradle | 3 +++ .../java/com/uber/nullaway/NullAwayCoreTests.java | 25 ++++++++++++++++++++++ 3 files changed, 29 insertions(+), 1 deletion(-) diff --git a/gradle/dependencies.gradle b/gradle/dependencies.gradle index 5962a7c..bada5ee 100755 --- a/gradle/dependencies.gradle +++ b/gradle/dependencies.gradle @@ -40,7 +40,7 @@ if (project.hasProperty("epApiVersion")) { def versions = [ asm : "9.3", - checkerFramework : "3.41.0", + checkerFramework : "3.40.0", // for comparisons in other parts of the build errorProneLatest : latestErrorProneVersion, // The version of Error Prone used to check NullAway's code. diff --git a/nullaway/build.gradle b/nullaway/build.gradle index 7d39adc..15327ae 100644 --- a/nullaway/build.gradle +++ b/nullaway/build.gradle @@ -65,6 +65,9 @@ dependencies { testImplementation deps.test.mockito testImplementation deps.test.javaxAnnotationApi testImplementation deps.test.assertJ + // This is for a test exposing a CFG construction failure in the Checker Framework. We can probably remove it once + // the issue is fixed upstream and we update. See https://github.com/typetools/checker-framework/issues/6396. + testImplementation 'org.apache.spark:spark-sql_2.12:3.3.2' errorProneOldest deps.build.errorProneCheckApiOld errorProneOldest(deps.build.errorProneTestHelpersOld) { diff --git a/nullaway/src/test/java/com/uber/nullaway/NullAwayCoreTests.java b/nullaway/src/test/java/com/uber/nullaway/NullAwayCoreTests.java index f8f3ed6..dd6e0d7 100644 --- a/nullaway/src/test/java/com/uber/nullaway/NullAwayCoreTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/NullAwayCoreTests.java @@ -960,4 +960,29 @@ public class NullAwayCoreTests extends NullAwayTestsBase { "}") .doTest(); } + + /** + * This test exposes a failure in CFG construction in Checker Framework 3.41.0 and above. Once a + * fix for this issue makes it to a Checker Framework release, we can probably remove this test. + * See https://github.com/typetools/checker-framework/issues/6396. + */ + @Test + public void cfgConstructionSymbolCompletionFailure() { + defaultCompilationHelper + .addSourceLines( + "Test.java", + "package com.uber;", + "import org.apache.spark.sql.SparkSession;", + "class Test {", + " static class X {", + " X(SparkSession session) {}", + " }", + " X run() {", + " try (SparkSession session = SparkSession.builder().getOrCreate()) {", + " return new X(session);", + " }", + " }", + "}") + .doTest(); + } } -- cgit v1.2.3 From 0293b0f2fa18ebdcaac07fa82e759ada2132c514 Mon Sep 17 00:00:00 2001 From: Yuxin Wang Date: Sat, 13 Jan 2024 22:24:32 -0500 Subject: Prepare for release 0.10.21. --- CHANGELOG.md | 9 +++++++++ gradle.properties | 2 +- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8ae1571..6114070 100755 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,14 @@ Changelog ========= +Version 0.10.21 +--------------- +IMPORTANT: This release fixes a crash when running against <2.24.0 release of + Error Prone (see #894) introduced in NullAway v0.10.20 and another crash related to + Checker Framework (see #895) introduced in NullAway v0.10.19. + +* Fix backwards-incompatible calls to ASTHelpers.hasDirectAnnotationWithSimpleName (#894) +* Downgrade to Checker Framework 3.40.0 (#895) + Version 0.10.20 --------------- * Fix JSpecify support on JDK 21 (#869) diff --git a/gradle.properties b/gradle.properties index 37d5362..91ee809 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.10.21-SNAPSHOT +VERSION_NAME=0.10.21 POM_DESCRIPTION=A fast annotation-based null checker for Java -- cgit v1.2.3 From c1eb8cae63cd697e7b124b6bfa43130826c80bb6 Mon Sep 17 00:00:00 2001 From: Yuxin Wang Date: Sat, 13 Jan 2024 22:34:19 -0500 Subject: Prepare next development version. --- gradle.properties | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gradle.properties b/gradle.properties index 91ee809..af0c871 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.10.21 +VERSION_NAME=0.10.22-SNAPSHOT POM_DESCRIPTION=A fast annotation-based null checker for Java -- cgit v1.2.3 From b94597a826d28dd36364f4dff903541910465d28 Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Thu, 25 Jan 2024 09:49:35 -0800 Subject: Fix bug with implicit equals() methods in interfaces (#898) Fixes #897 This fixes a regression in our handling of implicit `equals()` methods in interfaces when building on JDK 21. I see this as an interim fix, until we can fix NullAway to properly always assume / enforce that the parameter of `equals()` is `@Nullable`. But, I think we should fix the regression in the short term. --- .../src/main/java/com/uber/nullaway/NullAway.java | 24 ++++++++++++++++++---- .../java/com/uber/nullaway/NullAwayCoreTests.java | 16 +++++++++++++++ 2 files changed, 36 insertions(+), 4 deletions(-) diff --git a/nullaway/src/main/java/com/uber/nullaway/NullAway.java b/nullaway/src/main/java/com/uber/nullaway/NullAway.java index 3bf0a85..a23438a 100644 --- a/nullaway/src/main/java/com/uber/nullaway/NullAway.java +++ b/nullaway/src/main/java/com/uber/nullaway/NullAway.java @@ -36,6 +36,7 @@ import static com.uber.nullaway.NullabilityUtil.castToNonNull; import com.google.auto.service.AutoService; import com.google.auto.value.AutoValue; import com.google.common.base.Preconditions; +import com.google.common.base.Verify; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMultimap; import com.google.common.collect.ImmutableSet; @@ -399,16 +400,31 @@ public class NullAway extends BugChecker if (!withinAnnotatedCode(state)) { return Description.NO_MATCH; } - final Symbol.MethodSymbol methodSymbol = ASTHelpers.getSymbol(tree); - if (methodSymbol == null) { - throw new RuntimeException("not expecting unresolved method here"); - } + Symbol.MethodSymbol methodSymbol = getSymbolForMethodInvocation(tree, state); handler.onMatchMethodInvocation(this, tree, state, methodSymbol); // assuming this list does not include the receiver List actualParams = tree.getArguments(); return handleInvocation(tree, state, methodSymbol, actualParams); } + private static Symbol.MethodSymbol getSymbolForMethodInvocation( + MethodInvocationTree tree, VisitorState state) { + Symbol.MethodSymbol methodSymbol = ASTHelpers.getSymbol(tree); + Verify.verify(methodSymbol != null, "not expecting unresolved method here"); + // For interface methods, if the method is an implicit method corresponding to a method from + // java.lang.Object, use the symbol for the java.lang.Object method instead. We do this to + // properly treat the method as unannotated, which is particularly important for equals() + // methods. This is an adaptation to a change in JDK 18; see + // https://bugs.openjdk.org/browse/JDK-8272564 + if (methodSymbol.owner.isInterface()) { + Symbol.MethodSymbol baseSymbol = (Symbol.MethodSymbol) methodSymbol.baseSymbol(); + if (baseSymbol != methodSymbol && baseSymbol.owner == state.getSymtab().objectType.tsym) { + methodSymbol = baseSymbol; + } + } + return methodSymbol; + } + @Override public Description matchNewClass(NewClassTree tree, VisitorState state) { if (!withinAnnotatedCode(state)) { diff --git a/nullaway/src/test/java/com/uber/nullaway/NullAwayCoreTests.java b/nullaway/src/test/java/com/uber/nullaway/NullAwayCoreTests.java index dd6e0d7..3649de9 100644 --- a/nullaway/src/test/java/com/uber/nullaway/NullAwayCoreTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/NullAwayCoreTests.java @@ -985,4 +985,20 @@ public class NullAwayCoreTests extends NullAwayTestsBase { "}") .doTest(); } + + @Test + public void testDefaultEqualsInInterfaceTakesNullable() { + defaultCompilationHelper + .addSourceLines( + "Test.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "class Test {", + " public interface AnInterface {}", + " public static boolean foo(AnInterface a, @Nullable AnInterface b) {", + " return a.equals(b);", + " }", + "}") + .doTest(); + } } -- cgit v1.2.3 From 091ac38fb8b410c6483022c03ffbee2bb169bee3 Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Thu, 25 Jan 2024 09:58:48 -0800 Subject: Fix crash with raw types in overrides in JSpecify mode (#899) We should skip checking for errors here instead of crashing --- .../com/uber/nullaway/generics/GenericsChecks.java | 8 +++---- .../nullaway/NullAwayJSpecifyGenericsTests.java | 25 ++++++++++++++++++++++ 2 files changed, 28 insertions(+), 5 deletions(-) diff --git a/nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java b/nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java index 77f185e..4754ed9 100644 --- a/nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java +++ b/nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java @@ -3,7 +3,6 @@ package com.uber.nullaway.generics; import static com.google.common.base.Verify.verify; import static com.uber.nullaway.NullabilityUtil.castToNonNull; -import com.google.common.base.Preconditions; import com.google.errorprone.VisitorState; import com.google.errorprone.suppliers.Supplier; import com.google.errorprone.suppliers.Suppliers; @@ -772,7 +771,7 @@ public final class GenericsChecks { List overriddenMethodParameterTypes = overriddenMethodType.getParameterTypes(); // TODO handle varargs; they are not handled for now for (int i = 0; i < methodParameters.size(); i++) { - Type overridingMethodParameterType = ASTHelpers.getType(methodParameters.get(i)); + Type overridingMethodParameterType = getTreeType(methodParameters.get(i), state); Type overriddenMethodParameterType = overriddenMethodParameterTypes.get(i); if (overriddenMethodParameterType != null && overridingMethodParameterType != null) { if (!compareNullabilityAnnotations( @@ -801,11 +800,10 @@ public final class GenericsChecks { private static void checkTypeParameterNullnessForOverridingMethodReturnType( MethodTree tree, Type overriddenMethodType, NullAway analysis, VisitorState state) { Type overriddenMethodReturnType = overriddenMethodType.getReturnType(); - Type overridingMethodReturnType = ASTHelpers.getType(tree.getReturnType()); - if (overriddenMethodReturnType == null) { + Type overridingMethodReturnType = getTreeType(tree.getReturnType(), state); + if (overriddenMethodReturnType == null || overridingMethodReturnType == null) { return; } - Preconditions.checkArgument(overridingMethodReturnType != null); if (!compareNullabilityAnnotations( overriddenMethodReturnType, overridingMethodReturnType, state)) { reportInvalidOverridingMethodReturnTypeError( diff --git a/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyGenericsTests.java b/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyGenericsTests.java index 8e3452e..d146442 100644 --- a/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyGenericsTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyGenericsTests.java @@ -1615,6 +1615,31 @@ public class NullAwayJSpecifyGenericsTests extends NullAwayTestsBase { .doTest(); } + @Test + public void overrideWithRawType() { + makeHelper() + .addSourceLines( + "Test.java", + "package com.uber;", + "import org.jspecify.annotations.Nullable;", + "class Test {", + " interface Foo {}", + " interface Bar {", + " void add(Foo foo);", + " @Nullable Foo get();", + " }", + " static class Baz implements Bar {", + " @SuppressWarnings(\"rawtypes\")", + " @Override", + " public void add(Foo foo) {}", + " @SuppressWarnings(\"rawtypes\")", + " @Override", + " public @Nullable Foo get() { return null; }", + " }", + "}") + .doTest(); + } + private CompilationTestHelper makeHelper() { return makeTestHelperWithArgs( Arrays.asList( -- cgit v1.2.3 From 115d6837bd96de023d4f3407df8a442ea17c947c Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Thu, 25 Jan 2024 10:07:22 -0800 Subject: Update instructions for Android and our sample app (#900) Fixes #891. It's hard to add a regression test for this, but I manually tested that with the new configuration, NullAway errors are detected in the sample app. After this lands, I'd like to update the readme further to refer to this particular commit, so users can see the changes we needed to make. --- README.md | 10 ++-------- sample-app/build.gradle | 33 ++++++++++++++++++--------------- 2 files changed, 20 insertions(+), 23 deletions(-) diff --git a/README.md b/README.md index 1376997..e968cdd 100644 --- a/README.md +++ b/README.md @@ -58,15 +58,9 @@ Snapshots of the development version are available in [Sonatype's snapshots repo #### Android -The configuration for an Android project is very similar to the Java case, with one key difference: The `com.google.code.findbugs:jsr305:3.0.2` dependency can be removed; you can use the `android.support.annotation.Nullable` annotation from the Android Support library. +Versions 3.0.0 and later of the Gradle Error Prone Plugin [no longer support Android](https://github.com/tbroyer/gradle-errorprone-plugin/releases/tag/v3.0.0). So if you're using a recent version of this plugin, you'll need to add some further configuration to run Error Prone and NullAway. Our [sample app `build.gradle` file](https://github.com/uber/NullAway/blob/master/sample-app/build.gradle) shows one way to do this, but your Android project may require tweaks. Alternately, 2.x versions of the Gradle Error Prone Plugin still support Android and may still work with your project. -```gradle -dependencies { - errorprone "com.uber.nullaway:nullaway:" - errorprone "com.google.errorprone:error_prone_core:" -} -``` -For a more complete example see our [sample app](https://github.com/uber/NullAway/blob/master/sample-app/). (The sample app's [`build.gradle`](https://github.com/uber/NullAway/blob/master/sample-app/) is not suitable for direct copy-pasting, as some configuration is inherited from the top-level `build.gradle`.) +Beyond that, compared to the Java configuration, the `com.google.code.findbugs:jsr305:3.0.2` dependency can be removed; you can use the `android.support.annotation.Nullable` annotation from the Android Support library instead. #### Annotation Processors / Generated Code diff --git a/sample-app/build.gradle b/sample-app/build.gradle index 6c4deb2..0ab1e6f 100644 --- a/sample-app/build.gradle +++ b/sample-app/build.gradle @@ -45,27 +45,30 @@ android { variants.addAll(getUnitTestVariants()) variants.configureEach { variant -> variant.getJavaCompileProvider().configure { - options.errorprone { - check("NullAway", CheckSeverity.ERROR) - option("NullAway:AnnotatedPackages", "com.uber") - } + options.compilerArgs += [ + "-XDcompilePolicy=simple", + "-Xplugin:ErrorProne -XepOpt:NullAway:AnnotatedPackages=com.uber", + ] + options.fork = true + options.forkOptions.jvmArgs = [ + "--add-exports=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED", + "--add-exports=jdk.compiler/com.sun.tools.javac.file=ALL-UNNAMED", + "--add-exports=jdk.compiler/com.sun.tools.javac.main=ALL-UNNAMED", + "--add-exports=jdk.compiler/com.sun.tools.javac.model=ALL-UNNAMED", + "--add-exports=jdk.compiler/com.sun.tools.javac.parser=ALL-UNNAMED", + "--add-exports=jdk.compiler/com.sun.tools.javac.processing=ALL-UNNAMED", + "--add-exports=jdk.compiler/com.sun.tools.javac.tree=ALL-UNNAMED", + "--add-exports=jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED", + "--add-opens=jdk.compiler/com.sun.tools.javac.code=ALL-UNNAMED", + "--add-opens=jdk.compiler/com.sun.tools.javac.comp=ALL-UNNAMED" + ] } } - - // If you want to disable NullAway in just tests, you can do the below - // DomainObjectSet testVariants = getTestVariants() - // testVariants.addAll(getUnitTestVariants()) - // testVariants.configureEach { variant -> - // variant.getJavaCompileProvider().configure { - // options.errorprone { - // check("NullAway", CheckSeverity.OFF) - // } - // } - // } } dependencies { implementation deps.support.appcompat + annotationProcessor deps.build.errorProneCore annotationProcessor project(":nullaway") annotationProcessor project(path: ":sample-library-model") -- cgit v1.2.3 From 986d060b570c629cb00b0b77fd6ec4e019983fbc Mon Sep 17 00:00:00 2001 From: Yuxin Wang Date: Mon, 29 Jan 2024 14:07:39 -0500 Subject: Prepare for release 0.10.22. --- CHANGELOG.md | 9 +++++++++ gradle.properties | 2 +- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6114070..1854e1f 100755 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,14 @@ Changelog ========= +Version 0.10.22 +--------------- +IMPORTANT: The support for JDK 8 is deprecated in this release and will be removed in + an upcoming release. + +* Fix bug with implicit equals() methods in interfaces (#898) +* Fix crash with raw types in overrides in JSpecify mode (#899) +* Docs fix: Update instructions for Android and our sample app (#900) + Version 0.10.21 --------------- IMPORTANT: This release fixes a crash when running against <2.24.0 release of diff --git a/gradle.properties b/gradle.properties index af0c871..3c140cc 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.10.22-SNAPSHOT +VERSION_NAME=0.10.22 POM_DESCRIPTION=A fast annotation-based null checker for Java -- cgit v1.2.3