From cc91371d8128b8f01a7b041e70bc9ad2d2f43457 Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Wed, 12 Oct 2022 10:43:34 -0700 Subject: [PATCH 01/14] Bump Error Prone and EP plugin (#675) Bump Error Prone to 2.16 and the EP Gradle plugin to 3.0.1. Also fix some issues flagged by the new `ASTHelpersSuggestions` check. We should also update our docs to use the latest EP Gradle plugin. But I'd rather do that in a separate PR. --- .github/workflows/continuous-integration.yml | 10 +++++----- build.gradle | 3 +-- gradle/dependencies.gradle | 2 +- .../java/com/uber/nullaway/CodeAnnotationInfo.java | 11 +++++++---- .../main/java/com/uber/nullaway/ErrorBuilder.java | 4 +++- .../src/main/java/com/uber/nullaway/NullAway.java | 12 +++++++++--- .../java/com/uber/nullaway/dataflow/AccessPath.java | 8 +++++--- .../dataflow/AccessPathNullnessPropagation.java | 1 + 8 files changed, 32 insertions(+), 19 deletions(-) diff --git a/.github/workflows/continuous-integration.yml b/.github/workflows/continuous-integration.yml index 9db6348018..51b3931540 100644 --- a/.github/workflows/continuous-integration.yml +++ b/.github/workflows/continuous-integration.yml @@ -21,16 +21,16 @@ jobs: epVersion: 2.4.0 - os: macos-latest java: 11 - epVersion: 2.15.0 + epVersion: 2.16 - os: ubuntu-latest java: 11 - epVersion: 2.15.0 + epVersion: 2.16 - os: windows-latest java: 11 - epVersion: 2.15.0 + epVersion: 2.16 - os: ubuntu-latest java: 17 - epVersion: 2.15.0 + epVersion: 2.16 fail-fast: false runs-on: ${{ matrix.os }} steps: @@ -70,7 +70,7 @@ jobs: with: arguments: coveralls continue-on-error: true - if: runner.os == 'Linux' && matrix.java == '11' && matrix.epVersion == '2.15.0' && github.repository == 'uber/NullAway' + if: runner.os == 'Linux' && matrix.java == '11' && matrix.epVersion == '2.16' && github.repository == 'uber/NullAway' - name: Check that Git tree is clean after build and test run: ./.buildscript/check_git_clean.sh publish_snapshot: diff --git a/build.gradle b/build.gradle index c46fbb94b7..f63d161545 100644 --- a/build.gradle +++ b/build.gradle @@ -36,7 +36,7 @@ buildscript { } plugins { id "com.github.sherter.google-java-format" version "0.9" - id "net.ltgt.errorprone" version "2.0.2" apply false + id "net.ltgt.errorprone" version "3.0.1" apply false id "com.github.johnrengelman.shadow" version "6.1.0" apply false id "com.github.kt3k.coveralls" version "2.12.0" apply false id "me.champeau.jmh" version "0.6.7" apply false @@ -54,7 +54,6 @@ subprojects { project -> project.apply plugin: "net.ltgt.errorprone" project.dependencies { errorprone deps.build.errorProneCore - errorproneJavac deps.build.errorProneJavac } project.tasks.withType(JavaCompile) { dependsOn(installGitHooks) diff --git a/gradle/dependencies.gradle b/gradle/dependencies.gradle index 5d6ffa9c5e..ea70f80391 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.4.0" // Latest released Error Prone version that we've tested with -def latestErrorProneVersion = "2.15.0" +def latestErrorProneVersion = "2.16" // Default to using latest tested Error Prone version, except on Java 8, where 2.10.0 is the last version // that works def defaultErrorProneVersion = JavaVersion.current() >= JavaVersion.VERSION_11 ? latestErrorProneVersion : "2.10.0" diff --git a/nullaway/src/main/java/com/uber/nullaway/CodeAnnotationInfo.java b/nullaway/src/main/java/com/uber/nullaway/CodeAnnotationInfo.java index 55de1b66a5..764b905c5c 100644 --- a/nullaway/src/main/java/com/uber/nullaway/CodeAnnotationInfo.java +++ b/nullaway/src/main/java/com/uber/nullaway/CodeAnnotationInfo.java @@ -78,16 +78,19 @@ public static CodeAnnotationInfo instance(Context context) { private static boolean fromAnnotatedPackage( Symbol.ClassSymbol outermostClassSymbol, Config config) { final String className = outermostClassSymbol.getQualifiedName().toString(); + Symbol.PackageSymbol enclosingPackage = ASTHelpers.enclosingPackage(outermostClassSymbol); if (!config.fromExplicitlyAnnotatedPackage(className) - && !ASTHelpers.hasDirectAnnotationWithSimpleName( - outermostClassSymbol.packge(), NullabilityUtil.NULLMARKED_SIMPLE_NAME)) { + && !(enclosingPackage != null + && ASTHelpers.hasDirectAnnotationWithSimpleName( + enclosingPackage, NullabilityUtil.NULLMARKED_SIMPLE_NAME))) { // By default, unknown code is unannotated unless @NullMarked or configured as annotated by // package name return false; } if (config.fromExplicitlyUnannotatedPackage(className) - || ASTHelpers.hasDirectAnnotationWithSimpleName( - outermostClassSymbol.packge(), NullabilityUtil.NULLUNMARKED_SIMPLE_NAME)) { + || (enclosingPackage != null + && ASTHelpers.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 // explicitly passed to -XepOpt:NullAway::AnnotatedPackages diff --git a/nullaway/src/main/java/com/uber/nullaway/ErrorBuilder.java b/nullaway/src/main/java/com/uber/nullaway/ErrorBuilder.java index b440bc31a2..1c5b0b28ff 100755 --- a/nullaway/src/main/java/com/uber/nullaway/ErrorBuilder.java +++ b/nullaway/src/main/java/com/uber/nullaway/ErrorBuilder.java @@ -468,7 +468,9 @@ void reportInitErrorOnField(Symbol symbol, VisitorState state, Description.Build fieldName = flatName.substring(index) + "." + fieldName; } - if (symbol.isStatic()) { + @SuppressWarnings("ASTHelpersSuggestions") // remove once we require EP 2.16 or greater + boolean isStatic = symbol.isStatic(); + if (isStatic) { state.reportMatch( createErrorDescription( new ErrorMessage( diff --git a/nullaway/src/main/java/com/uber/nullaway/NullAway.java b/nullaway/src/main/java/com/uber/nullaway/NullAway.java index 132733bea7..50f8663ddc 100644 --- a/nullaway/src/main/java/com/uber/nullaway/NullAway.java +++ b/nullaway/src/main/java/com/uber/nullaway/NullAway.java @@ -988,7 +988,9 @@ private Description checkForReadBeforeInit(ExpressionTree tree, VisitorState sta } // for static fields, make sure the enclosing init is a static method or block - if (symbol.isStatic()) { + @SuppressWarnings("ASTHelpersSuggestions") // remove once we require EP 2.16 or greater + boolean isStatic = symbol.isStatic(); + if (isStatic) { Tree enclosing = enclosingBlockPath.getLeaf(); if (enclosing instanceof MethodTree && !ASTHelpers.getSymbol((MethodTree) enclosing).isStatic()) { @@ -1097,7 +1099,9 @@ private boolean fieldAlwaysInitializedBeforeRead( Symbol symbol, TreePath pathToRead, VisitorState state, TreePath enclosingBlockPath) { AccessPathNullnessAnalysis nullnessAnalysis = getNullnessAnalysis(state); Set nonnullFields; - if (symbol.isStatic()) { + @SuppressWarnings("ASTHelpersSuggestions") // remove once we require EP 2.16 or greater + boolean isStatic = symbol.isStatic(); + if (isStatic) { nonnullFields = nullnessAnalysis.getNonnullStaticFieldsBefore(pathToRead, state.context); } else { nonnullFields = new LinkedHashSet<>(); @@ -2063,7 +2067,9 @@ private FieldInitEntities collectEntities(ClassTree tree, VisitorState state) { // matchVariable() continue; } - if (fieldSymbol.isStatic()) { + @SuppressWarnings("ASTHelpersSuggestions") // remove once we require EP 2.16 or greater + boolean fieldIsStatic = fieldSymbol.isStatic(); + if (fieldIsStatic) { nonnullStaticFields.add(fieldSymbol); } else { nonnullInstanceFields.add(fieldSymbol); diff --git a/nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPath.java b/nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPath.java index 1b88865b37..51c7219c68 100644 --- a/nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPath.java +++ b/nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPath.java @@ -365,9 +365,11 @@ public static AccessPath fromFieldElement(VariableElement element) { } private static boolean isBoxingMethod(Symbol.MethodSymbol methodSymbol) { - return methodSymbol.isStatic() - && methodSymbol.getSimpleName().contentEquals("valueOf") - && methodSymbol.enclClass().packge().fullname.contentEquals("java.lang"); + if (methodSymbol.isStatic() && methodSymbol.getSimpleName().contentEquals("valueOf")) { + Symbol.PackageSymbol enclosingPackage = ASTHelpers.enclosingPackage(methodSymbol.enclClass()); + return enclosingPackage != null && enclosingPackage.fullname.contentEquals("java.lang"); + } + return false; } /** 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 19e7f3c2b1..aa003d7df1 100644 --- a/nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPathNullnessPropagation.java +++ b/nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPathNullnessPropagation.java @@ -761,6 +761,7 @@ private CodeAnnotationInfo getCodeAnnotationInfo(VisitorState state) { return codeAnnotationInfo; } + @SuppressWarnings("ASTHelpersSuggestions") // remove once we require EP 2.16 or greater private void setReceiverNonnull( AccessPathNullnessPropagation.ReadableUpdates updates, Node receiver, Symbol symbol) { if (symbol != null && !symbol.isStatic()) { From 89c74f340b5a3a6cde64eadbd1ae795fec96520c Mon Sep 17 00:00:00 2001 From: Carter Kozak Date: Wed, 12 Oct 2022 16:53:29 -0400 Subject: [PATCH 02/14] Fix an NPE in the optional emptiness handler (#678) Fixed by fully implementing the OPTIONAL_CONTENT pseudo-element. Fixes #676 --- .../handlers/OptionalEmptinessHandler.java | 178 +++++++++++------- .../NullAwayOptionalEmptinessTests.java | 28 +++ 2 files changed, 134 insertions(+), 72 deletions(-) diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/OptionalEmptinessHandler.java b/nullaway/src/main/java/com/uber/nullaway/handlers/OptionalEmptinessHandler.java index c07c1da46f..6ff320578c 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/OptionalEmptinessHandler.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/OptionalEmptinessHandler.java @@ -30,9 +30,11 @@ import com.sun.source.tree.Tree; import com.sun.source.util.TreePath; import com.sun.tools.javac.code.Symbol; +import com.sun.tools.javac.code.Symtab; import com.sun.tools.javac.code.Type; import com.sun.tools.javac.code.Types; import com.sun.tools.javac.util.Context; +import com.sun.tools.javac.util.Names; import com.uber.nullaway.Config; import com.uber.nullaway.ErrorMessage; import com.uber.nullaway.NullAway; @@ -41,6 +43,8 @@ import com.uber.nullaway.dataflow.AccessPathNullnessAnalysis; import com.uber.nullaway.dataflow.AccessPathNullnessPropagation; import java.lang.annotation.Annotation; +import java.lang.reflect.Array; +import java.util.Collections; import java.util.List; import java.util.Objects; import java.util.Optional; @@ -69,8 +73,6 @@ public class OptionalEmptinessHandler extends BaseNoOpHandler { private final Config config; private final MethodNameUtil methodNameUtil; - public static final VariableElement OPTIONAL_CONTENT = getOptionalContentElement(); - OptionalEmptinessHandler(Config config, MethodNameUtil methodNameUtil) { this.config = config; this.methodNameUtil = methodNameUtil; @@ -115,12 +117,14 @@ public NullnessHint onDataflowVisitMethodInvocation( Symbol.MethodSymbol symbol = ASTHelpers.getSymbol(node.getTree()); if (optionalIsPresentCall(symbol, types)) { - updateNonNullAPsForOptionalContent(thenUpdates, node.getTarget().getReceiver(), apContext); + updateNonNullAPsForOptionalContent( + context, thenUpdates, node.getTarget().getReceiver(), apContext); } else if (optionalIsEmptyCall(symbol, types)) { - updateNonNullAPsForOptionalContent(elseUpdates, node.getTarget().getReceiver(), apContext); + updateNonNullAPsForOptionalContent( + context, elseUpdates, node.getTarget().getReceiver(), apContext); } else if (config.handleTestAssertionLibraries() && methodNameUtil.isMethodIsTrue(symbol)) { // we check for instance of AssertThat(optionalFoo.isPresent()).isTrue() - updateIfAssertIsPresentTrueOnOptional(node, types, apContext, bothUpdates); + updateIfAssertIsPresentTrueOnOptional(context, node, types, apContext, bothUpdates); } return NullnessHint.UNKNOWN; } @@ -140,9 +144,13 @@ && isOptionalContentNullable(state, baseExpr, analysis.getNullnessAnalysis(state } private boolean isOptionalContentNullable( - VisitorState state, ExpressionTree baseExpr, AccessPathNullnessAnalysis analysis) { - return analysis.getNullnessOfExpressionNamedField( - new TreePath(state.getPath(), baseExpr), state.context, OPTIONAL_CONTENT) + VisitorState state, + ExpressionTree baseExpr, + AccessPathNullnessAnalysis accessPathNullnessAnalysis) { + return accessPathNullnessAnalysis.getNullnessOfExpressionNamedField( + new TreePath(state.getPath(), baseExpr), + state.context, + OptionalContentVariableElement.instance(state.context)) == Nullness.NULLABLE; } @@ -153,13 +161,15 @@ public boolean includeApInfoInSavedContext(AccessPath accessPath, VisitorState s final Element e = accessPath.getRoot(); if (e != null) { return e.getKind().equals(ElementKind.LOCAL_VARIABLE) - && accessPath.getElements().get(0).getJavaElement().equals(OPTIONAL_CONTENT); + && accessPath.getElements().get(0).getJavaElement() + instanceof OptionalContentVariableElement; } } return false; } private void updateIfAssertIsPresentTrueOnOptional( + Context context, MethodInvocationNode node, Types types, AccessPath.AccessPathContext apContext, @@ -181,7 +191,7 @@ private void updateIfAssertIsPresentTrueOnOptional( Symbol.MethodSymbol argSymbol = ASTHelpers.getSymbol(argMethod.getTree()); if (optionalIsPresentCall(argSymbol, types)) { updateNonNullAPsForOptionalContent( - bothUpdates, argMethod.getTarget().getReceiver(), apContext); + context, bothUpdates, argMethod.getTarget().getReceiver(), apContext); } } } @@ -190,10 +200,13 @@ private void updateIfAssertIsPresentTrueOnOptional( } private void updateNonNullAPsForOptionalContent( + Context context, AccessPathNullnessPropagation.Updates updates, Node base, AccessPath.AccessPathContext apContext) { - AccessPath ap = AccessPath.fromBaseAndElement(base, OPTIONAL_CONTENT, apContext); + AccessPath ap = + AccessPath.fromBaseAndElement( + base, OptionalContentVariableElement.instance(context), apContext); if (ap != null && base.getTree() != null) { updates.set(ap, Nullness.NONNULL); } @@ -229,79 +242,100 @@ private boolean optionalIsGetCall(Symbol.MethodSymbol symbol, Types types) { /** * A {@link VariableElement} for a dummy "field" holding the contents of an Optional object, used * in dataflow analysis to track whether the Optional content is present. + * + *

Instances of this type should be accessed using {@link #instance(Context)}, not instantiated + * directly. */ - private static VariableElement getOptionalContentElement() { - return new VariableElement() { - @Override - @Nullable - public Object getConstantValue() { - return null; + private static final class OptionalContentVariableElement implements VariableElement { + public static final Context.Key contextKey = + new Context.Key<>(); + + private static final Set MODIFIERS = ImmutableSet.of(Modifier.PUBLIC, Modifier.FINAL); + private final Name name; + private final TypeMirror asType; + + static synchronized VariableElement instance(Context context) { + OptionalContentVariableElement instance = context.get(contextKey); + if (instance == null) { + instance = + new OptionalContentVariableElement( + Names.instance(context).fromString("value"), Symtab.instance(context).objectType); + context.put(contextKey, instance); } + return instance; + } - @Override - @Nullable - public Name getSimpleName() { - return null; - } + private OptionalContentVariableElement(Name name, TypeMirror asType) { + this.name = name; + this.asType = asType; + } - @Override - @Nullable - public Element getEnclosingElement() { - return null; - } + @Override + @Nullable + public Object getConstantValue() { + return null; + } - @Override - @Nullable - public List getEnclosedElements() { - return null; - } + @Override + public Name getSimpleName() { + return name; + } - @Override - @Nullable - public List getAnnotationMirrors() { - return null; - } + @Override + @Nullable + public Element getEnclosingElement() { + // A field would have an enclosing element, however this method isn't guaranteed to + // return non-null in all cases. It may be beneficial to implement this in a future + // improvement, but that will require tracking an instance per supported optional + // type (e.g. java.util.Optional and guava Optional). + return null; + } - @Override - @Nullable - public A getAnnotation(Class aClass) { - return null; - } + @Override + public List getEnclosedElements() { + return Collections.emptyList(); + } - @Override - @Nullable - public A[] getAnnotationsByType(Class aClass) { - return null; - } + @Override + public List getAnnotationMirrors() { + return Collections.emptyList(); + } - @Override - @Nullable - public R accept(ElementVisitor elementVisitor, P p) { - return null; - } + @Override + @Nullable + public A getAnnotation(Class aClass) { + return null; + } - @Override - @Nullable - public TypeMirror asType() { - return null; - } + @Override + @SuppressWarnings("unchecked") + public A[] getAnnotationsByType(Class aClass) { + return (A[]) Array.newInstance(aClass, 0); + } - @Override - @Nullable - public ElementKind getKind() { - return null; - } + @Override + public R accept(ElementVisitor elementVisitor, P p) { + return elementVisitor.visitVariable(this, p); + } - @Override - @Nullable - public Set getModifiers() { - return null; - } + @Override + public TypeMirror asType() { + return asType; + } - @Override - public String toString() { - return "OPTIONAL_CONTENT"; - } - }; + @Override + public ElementKind getKind() { + return ElementKind.FIELD; + } + + @Override + public Set getModifiers() { + return MODIFIERS; + } + + @Override + public String toString() { + return "OPTIONAL_CONTENT"; + } } } diff --git a/nullaway/src/test/java/com/uber/nullaway/NullAwayOptionalEmptinessTests.java b/nullaway/src/test/java/com/uber/nullaway/NullAwayOptionalEmptinessTests.java index 9dd928d66d..a0cb723a17 100644 --- a/nullaway/src/test/java/com/uber/nullaway/NullAwayOptionalEmptinessTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/NullAwayOptionalEmptinessTests.java @@ -505,4 +505,32 @@ public void optionalEmptinessContextualSuppressionTest() { "}") .doTest(); } + + @Test + public void optionalOfMapResultTest() { + makeTestHelperWithArgs( + Arrays.asList( + "-d", + temporaryFolder.getRoot().getAbsolutePath(), + "-XepOpt:NullAway:AnnotatedPackages=com.uber", + "-XepOpt:NullAway:CheckOptionalEmptiness=true")) + .addSourceLines( + "Test.java", + "package com.uber;", + "import java.util.Map;", + "import java.util.Optional;", + "class Test {", + " private Optional f(Map map1, Map map2) {", + " Optional opt = Optional.ofNullable(map1.get(\"key\"));", + " if (!opt.isPresent()) {", + " return Optional.empty();", + " }", + " return map2.entrySet().stream()", + " .filter(entry -> entry.getValue().equals(opt.get()))", + " .map(Map.Entry::getKey)", + " .findFirst();", + " }", + "}") + .doTest(); + } } From e004e43d1efcc8460daf40360ce66aae95c83318 Mon Sep 17 00:00:00 2001 From: Carter Kozak Date: Wed, 26 Oct 2022 18:34:09 -0400 Subject: [PATCH 03/14] Add support for boolean constraints (about nullness) in Contract annotations (#669) For example, the following method will behave equivalently to guava `Preconditions.checkState` with regards to null checks: ```java @Contract("false -> fail") static void check(boolean pass) { if (!pass) throw new RuntimeException(); } ``` This implementation works in a couple of layers: - Firstly, contracts with a single boolean resultin in failure are handled by inserting a conditional throw node inline following the annotated method. This provides support for complex boolean logic input validateion. - Secondly, simple null equal-to and not-equal-to checks are handled by the existing ContractHandler. These don't support such complex inputs, however they are able to work correctly with boolean outputs rather than exclusively `fail` cases. I've updated the ContractHandler to use more precise language around antecedent nullness, previously null antecedents were described as nullable where cases required null. Please let me know if I've misunderstood the logic, but re-framing the values helped me to understand the logic. --- .../dataflow/cfg/NullAwayCFGBuilder.java | 29 +- .../handlers/contract/ContractHandler.java | 355 ++++++++--- .../handlers/contract/ContractUtils.java | 14 + .../NullAwayContractsBooleanTests.java | 570 ++++++++++++++++++ .../uber/nullaway/NullAwayContractsTests.java | 169 ++++++ 5 files changed, 1040 insertions(+), 97 deletions(-) create mode 100644 nullaway/src/test/java/com/uber/nullaway/NullAwayContractsBooleanTests.java diff --git a/nullaway/src/main/java/com/uber/nullaway/dataflow/cfg/NullAwayCFGBuilder.java b/nullaway/src/main/java/com/uber/nullaway/dataflow/cfg/NullAwayCFGBuilder.java index 0e20e0e429..bf5070866d 100644 --- a/nullaway/src/main/java/com/uber/nullaway/dataflow/cfg/NullAwayCFGBuilder.java +++ b/nullaway/src/main/java/com/uber/nullaway/dataflow/cfg/NullAwayCFGBuilder.java @@ -127,25 +127,44 @@ public TypeMirror classToErrorType(Class klass) { } /** - * Extend the CFG to throw an exception if the passed expression node evaluates to false. + * Extend the CFG to throw an exception if the passed expression node evaluates to {@code + * false}. * * @param booleanExpressionNode a CFG Node representing a boolean expression. * @param errorType the type of the exception to throw if booleanExpressionNode evaluates to - * false. + * {@code false}. */ public void insertThrowOnFalse(Node booleanExpressionNode, TypeMirror errorType) { + insertThrowOn(false, booleanExpressionNode, errorType); + } + + /** + * Extend the CFG to throw an exception if the passed expression node evaluates to {@code true}. + * + * @param booleanExpressionNode a CFG Node representing a boolean expression. + * @param errorType the type of the exception to throw if booleanExpressionNode evaluates to + * {@code true}. + */ + public void insertThrowOnTrue(Node booleanExpressionNode, TypeMirror errorType) { + insertThrowOn(true, booleanExpressionNode, errorType); + } + + private void insertThrowOn(boolean throwOn, Node booleanExpressionNode, TypeMirror errorType) { Tree tree = booleanExpressionNode.getTree(); Preconditions.checkArgument( tree instanceof ExpressionTree, "Argument booleanExpressionNode must represent a boolean expression"); ExpressionTree booleanExpressionTree = (ExpressionTree) booleanExpressionNode.getTree(); Preconditions.checkNotNull(booleanExpressionTree); - Label falsePreconditionEntry = new Label(); + Label preconditionEntry = new Label(); Label endPrecondition = new Label(); this.scan(booleanExpressionTree, (Void) null); - ConditionalJump cjump = new ConditionalJump(endPrecondition, falsePreconditionEntry); + ConditionalJump cjump = + new ConditionalJump( + throwOn ? preconditionEntry : endPrecondition, + throwOn ? endPrecondition : preconditionEntry); this.extendWithExtendedNode(cjump); - this.addLabelForNextNode(falsePreconditionEntry); + this.addLabelForNextNode(preconditionEntry); ExtendedNode exNode = this.extendWithNodeWithException( new ThrowNode( diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/contract/ContractHandler.java b/nullaway/src/main/java/com/uber/nullaway/handlers/contract/ContractHandler.java index 5a9548a97c..386425b384 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/contract/ContractHandler.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/contract/ContractHandler.java @@ -40,9 +40,17 @@ import com.uber.nullaway.Nullness; import com.uber.nullaway.dataflow.AccessPath; import com.uber.nullaway.dataflow.AccessPathNullnessPropagation; +import com.uber.nullaway.dataflow.cfg.NullAwayCFGBuilder; import com.uber.nullaway.handlers.BaseNoOpHandler; +import java.util.Optional; import javax.annotation.Nullable; +import javax.lang.model.type.TypeMirror; +import org.checkerframework.nullaway.dataflow.cfg.node.AbstractNodeVisitor; +import org.checkerframework.nullaway.dataflow.cfg.node.BinaryOperationNode; +import org.checkerframework.nullaway.dataflow.cfg.node.EqualToNode; import org.checkerframework.nullaway.dataflow.cfg.node.MethodInvocationNode; +import org.checkerframework.nullaway.dataflow.cfg.node.Node; +import org.checkerframework.nullaway.dataflow.cfg.node.NotEqualNode; /** * This Handler parses the jetbrains @Contract annotation and honors the nullness spec defined there @@ -82,6 +90,8 @@ public class ContractHandler extends BaseNoOpHandler { private @Nullable NullAway analysis; private @Nullable VisitorState state; + private @Nullable TypeMirror runtimeExceptionType; + public ContractHandler(Config config) { this.config = config; } @@ -93,6 +103,69 @@ public void onMatchTopLevelClass( this.state = state; } + @Override + public MethodInvocationNode onCFGBuildPhase1AfterVisitMethodInvocation( + NullAwayCFGBuilder.NullAwayCFGTranslationPhaseOne phase, + MethodInvocationTree tree, + MethodInvocationNode originalNode) { + Preconditions.checkNotNull(state); + Preconditions.checkNotNull(analysis); + Symbol.MethodSymbol callee = ASTHelpers.getSymbol(tree); + Preconditions.checkNotNull(callee); + for (String clause : ContractUtils.getContractClauses(callee, config)) { + // This method currently handles contracts of the form `(true|false) -> fail`, other + // contracts are handled by 'onDataflowVisitMethodInvocation' which has access to more + // dataflow information. + if (!"fail".equals(getConsequent(clause, tree, analysis, state, callee))) { + continue; + } + String[] antecedent = + getAntecedent(clause, tree, analysis, state, callee, originalNode.getArguments().size()); + // Find a single value constraint that is not already known. If more than one argument with + // unknown nullness affects the method's result, then ignore this clause. + Node arg = null; + // Set to false if the rule is detected to be one we don't yet support + boolean supported = true; + boolean booleanConstraint = false; + + for (int i = 0; i < antecedent.length; ++i) { + String valueConstraint = antecedent[i].trim(); + if ("false".equals(valueConstraint) || "true".equals(valueConstraint)) { + if (arg != null) { + // We don't currently support contracts depending on the boolean value of more than one + // argument using the node-insertion method. + supported = false; + break; + } + booleanConstraint = Boolean.parseBoolean(valueConstraint); + arg = originalNode.getArgument(i); + } else if (!valueConstraint.equals("_")) { + // Found an unsupported type of constraint, only true, false, and '_' (wildcard) are + // supported. + // No need to implement complex handling here, 'onDataflowVisitMethodInvocation' will + // validate the contract. + supported = false; + break; + } + } + if (arg != null && supported) { + if (runtimeExceptionType == null) { + runtimeExceptionType = phase.classToErrorType(RuntimeException.class); + } + // In practice the failure may not be RuntimeException, however the conditional + // throw is inserted after the method invocation where we must assume that + // any invocation is capable of throwing an unchecked throwable. + Preconditions.checkNotNull(runtimeExceptionType); + if (booleanConstraint) { + phase.insertThrowOnTrue(arg, runtimeExceptionType); + } else { + phase.insertThrowOnFalse(arg, runtimeExceptionType); + } + } + } + return originalNode; + } + @Override public NullnessHint onDataflowVisitMethodInvocation( MethodInvocationNode node, @@ -107,110 +180,208 @@ public NullnessHint onDataflowVisitMethodInvocation( Preconditions.checkNotNull(analysis); Symbol.MethodSymbol callee = ASTHelpers.getSymbol(node.getTree()); Preconditions.checkNotNull(callee); - // Check to see if this method has an @Contract annotation - String contractString = ContractUtils.getContractString(callee, config); - if (contractString != null && contractString.trim().length() > 0) { - // Found a contract, lets parse it. - String[] clauses = contractString.split(";"); - for (String clause : clauses) { - - MethodInvocationTree tree = castToNonNull(node.getTree()); - String[] antecedent = - getAntecedent(clause, tree, analysis, state, callee, node.getArguments().size()); - String consequent = getConsequent(clause, tree, analysis, state, callee); - - // Find a single value constraint that is not already known. If more than one arguments with - // unknown - // nullness affect the method's result, then ignore this clause. - int argIdx = -1; - Nullness argAntecedentNullness = null; - boolean supported = - true; // Set to false if the rule is detected to be one we don't yet support - - for (int i = 0; i < antecedent.length; ++i) { - String valueConstraint = antecedent[i].trim(); - if (valueConstraint.equals("_")) { - continue; - } else if (valueConstraint.equals("false") || valueConstraint.equals("true")) { + MethodInvocationTree tree = castToNonNull(node.getTree()); + for (String clause : ContractUtils.getContractClauses(callee, config)) { + + String[] antecedent = + getAntecedent(clause, tree, analysis, state, callee, node.getArguments().size()); + String consequent = getConsequent(clause, tree, analysis, state, callee); + + // Find a single value constraint that is not already known. If more than one argument with + // unknown nullness affects the method's result, then ignore this clause. + Node arg = null; + Nullness argAntecedentNullness = null; + // Set to false if the rule is detected to be one we don't yet support + boolean supported = true; + + for (int i = 0; i < antecedent.length; ++i) { + String valueConstraint = antecedent[i].trim(); + if (valueConstraint.equals("_")) { + continue; + } else if (valueConstraint.equals("false") || valueConstraint.equals("true")) { + // We handle boolean constraints in the case that the boolean argument is the result + // of a null or not-null check. For example, + // '@Contract("true -> true") boolean func(boolean v)' + // called with 'func(obj == null)' + // can be interpreted as equivalent to + // '@Contract("null -> true") boolean func(@Nullable Object v)' + // called with 'func(obj)' + // This path unwraps null reference equality and inequality checks + // to pass the target (in the above example, 'obj') as arg. + Node argument = node.getArgument(i); + // isNullTarget is the variable side of a null check. For example, both 'e == null' + // and 'null == e' would return the node representing 'e'. + Optional isNullTarget = argument.accept(NullEqualityVisitor.IS_NULL, inputs); + // notNullTarget is the variable side of a not-null check. For example, both 'e != null' + // and 'null != e' would return the node representing 'e'. + Optional notNullTarget = argument.accept(NullEqualityVisitor.NOT_NULL, inputs); + // It is possible for at most one of isNullTarget and notNullTarget to be present. + Node nullTestTarget = isNullTarget.orElse(notNullTarget.orElse(null)); + if (nullTestTarget == null) { supported = false; break; - } else if (valueConstraint.equals("!null") - && inputs.valueOfSubNode(node.getArgument(i)).equals(Nullness.NONNULL)) { - // We already know this argument can't be null, so we can treat it as not part of the - // clause - // for the purpose of deciding the non-nullness of the other arguments. + } + // isNullTarget is equivalent to 'null ->' while notNullTarget is equivalent + // to '!null ->'. However, the valueConstraint may reverse the check. + // The following table illustrates expected antecedentNullness based on + // null comparison direction and constraint: + // | (obj == null) | (obj != null) + // Constraint 'true' | NULL | NONNULL + // Constraint 'false' | NONNULL | NULL + boolean booleanConstraintValue = valueConstraint.equals("true"); + Nullness antecedentNullness = + isNullTarget.isPresent() + ? (booleanConstraintValue ? Nullness.NULL : Nullness.NONNULL) + : + // !isNullTarget.isPresent() -> notNullTarget.present() must be true + (booleanConstraintValue ? Nullness.NONNULL : Nullness.NULL); + Nullness targetNullness = inputs.valueOfSubNode(nullTestTarget); + if (antecedentNullness.equals(targetNullness)) { + // We already know this argument is satisfied so we can treat it as part of the + // clause for the purpose of deciding the nullness of the other arguments. continue; - } else if (valueConstraint.equals("null") || valueConstraint.equals("!null")) { - if (argIdx != -1) { - // More than one argument involved in the antecedent, ignore this rule - supported = false; - break; - } - argIdx = i; - argAntecedentNullness = - valueConstraint.equals("null") ? Nullness.NULLABLE : Nullness.NONNULL; - } else { - String errorMessage = - "Invalid @Contract annotation detected for method " - + callee - + ". It contains the following uparseable clause: " - + clause - + " (unknown value constraint: " - + valueConstraint - + ", see https://www.jetbrains.com/help/idea/contract-annotations.html)."; - state.reportMatch( - analysis - .getErrorBuilder() - .createErrorDescription( - new ErrorMessage( - ErrorMessage.MessageTypes.ANNOTATION_VALUE_INVALID, errorMessage), - tree, - analysis.buildDescription(tree), - state, - null)); + } + if (arg != null) { + // More than one argument involved in the antecedent, ignore this rule supported = false; break; } - } - if (!supported) { - // Too many arguments involved, or unsupported @Contract features. On to next clause in - // the - // contract expression + arg = nullTestTarget; + argAntecedentNullness = antecedentNullness; + } else if (valueConstraint.equals("!null") + && inputs.valueOfSubNode(node.getArgument(i)).equals(Nullness.NONNULL)) { + // We already know this argument can't be null, so we can treat it as not part of the + // clause for the purpose of deciding the non-nullness of the other arguments. continue; - } - if (argIdx == -1) { - // The antecedent is unconditionally true. Check for the ... -> !null case and set the - // return nullness accordingly - if (consequent.equals("!null")) { - return NullnessHint.FORCE_NONNULL; + } else if (valueConstraint.equals("null") || valueConstraint.equals("!null")) { + if (arg != null) { + // More than one argument involved in the antecedent, ignore this rule + supported = false; + break; } - continue; + arg = node.getArgument(i); + argAntecedentNullness = valueConstraint.equals("null") ? Nullness.NULL : Nullness.NONNULL; + } else { + String errorMessage = + "Invalid @Contract annotation detected for method " + + callee + + ". It contains the following uparseable clause: " + + clause + + " (unknown value constraint: " + + valueConstraint + + ", see https://www.jetbrains.com/help/idea/contract-annotations.html)."; + state.reportMatch( + analysis + .getErrorBuilder() + .createErrorDescription( + new ErrorMessage( + ErrorMessage.MessageTypes.ANNOTATION_VALUE_INVALID, errorMessage), + tree, + analysis.buildDescription(tree), + state, + null)); + supported = false; + break; } - if (argAntecedentNullness == null) { - throw new IllegalStateException("argAntecedentNullness should have been set"); - } - // The nullness of one argument is all that matters for the antecedent, let's negate the - // consequent to fix the nullness of this argument. - AccessPath accessPath = - AccessPath.getAccessPathForNodeNoMapGet(node.getArgument(argIdx), apContext); - if (accessPath == null) { - continue; - } - if (consequent.equals("false") && argAntecedentNullness.equals(Nullness.NULLABLE)) { - // If argIdx being null implies the return of the method being false, then the return - // being true implies argIdx is not null and we must mark it as such in the then update. - thenUpdates.set(accessPath, Nullness.NONNULL); - } else if (consequent.equals("true") && argAntecedentNullness.equals(Nullness.NULLABLE)) { - // If argIdx being null implies the return of the method being true, then the return being - // false implies argIdx is not null and we must mark it as such in the else update. - elseUpdates.set(accessPath, Nullness.NONNULL); - } else if (consequent.equals("fail") && argAntecedentNullness.equals(Nullness.NULLABLE)) { - // If argIdx being null implies the method throws an exception, then we can mark it as - // non-null on both non-exceptional exits from the method - bothUpdates.set(accessPath, Nullness.NONNULL); + } + if (!supported) { + // Too many arguments involved, or unsupported @Contract features. On to next clause in the + // contract expression + continue; + } + if (arg == null) { + // The antecedent is unconditionally true. Check for the ... -> !null case and set the + // return nullness accordingly + if (consequent.equals("!null")) { + return NullnessHint.FORCE_NONNULL; } + continue; + } + Preconditions.checkState( + argAntecedentNullness != null, "argAntecedentNullness should have been set"); + // The nullness of one argument is all that matters for the antecedent, let's negate the + // consequent to fix the nullness of this argument. + AccessPath accessPath = AccessPath.getAccessPathForNodeNoMapGet(arg, apContext); + if (accessPath == null) { + continue; + } + if (consequent.equals("false") && argAntecedentNullness.equals(Nullness.NULL)) { + // If arg being null implies the return of the method being false, then the return + // being true implies arg is not null and we must mark it as such in the then update. + thenUpdates.set(accessPath, Nullness.NONNULL); + } else if (consequent.equals("true") && argAntecedentNullness.equals(Nullness.NULL)) { + // If arg being null implies the return of the method being true, then the return being + // false implies arg is not null and we must mark it as such in the else update. + elseUpdates.set(accessPath, Nullness.NONNULL); + } else if (consequent.equals("fail") && argAntecedentNullness.equals(Nullness.NULL)) { + // If arg being null implies the method throws an exception, then we can mark it as + // non-null on both non-exceptional exits from the method + bothUpdates.set(accessPath, Nullness.NONNULL); } } return NullnessHint.UNKNOWN; } + + /** + * This visitor returns an {@link Optional} representing the non-null side of a null + * comparison check. + * + *

When the visited node is not an equality check (either {@link EqualToNode} or {@link + * NotEqualNode} based on {@link #equals} being {@code true} or {@code false} respectively) + * against a null value, {@link Optional#empty()} is returned. For example, visiting {@code e == + * null} with {@code new NullEqualityVisitor(true)} would return an {@link Optional} of node + * {@code e}. + */ + private static final class NullEqualityVisitor + extends AbstractNodeVisitor, AccessPathNullnessPropagation.SubNodeValues> { + + private static final NullEqualityVisitor IS_NULL = new NullEqualityVisitor(true); + private static final NullEqualityVisitor NOT_NULL = new NullEqualityVisitor(false); + + private final boolean equals; + + NullEqualityVisitor(boolean equals) { + this.equals = equals; + } + + @Override + public Optional visitNode(Node node, AccessPathNullnessPropagation.SubNodeValues unused) { + return Optional.empty(); + } + + @Override + public Optional visitNotEqual( + NotEqualNode notEqualNode, AccessPathNullnessPropagation.SubNodeValues inputs) { + if (equals) { + return Optional.empty(); + } else { + return visit(notEqualNode, inputs); + } + } + + @Override + public Optional visitEqualTo( + EqualToNode equalToNode, AccessPathNullnessPropagation.SubNodeValues inputs) { + if (equals) { + return visit(equalToNode, inputs); + } else { + return Optional.empty(); + } + } + + private Optional visit( + BinaryOperationNode comparison, AccessPathNullnessPropagation.SubNodeValues inputs) { + Node lhs = comparison.getLeftOperand(); + Node rhs = comparison.getRightOperand(); + Nullness lhsNullness = inputs.valueOfSubNode(lhs); + Nullness rhsNullness = inputs.valueOfSubNode(rhs); + if (Nullness.NULL.equals(lhsNullness)) { + return Optional.of(rhs); + } + if (Nullness.NULL.equals(rhsNullness)) { + return Optional.of(lhs); + } + return Optional.empty(); + } + } } diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/contract/ContractUtils.java b/nullaway/src/main/java/com/uber/nullaway/handlers/contract/ContractUtils.java index 10c62c7579..ac54e11326 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/contract/ContractUtils.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/contract/ContractUtils.java @@ -17,6 +17,8 @@ /** An utility class for {@link ContractHandler} and {@link ContractCheckHandler}. */ public class ContractUtils { + private static final String[] EMPTY_STRING_ARRAY = new String[0]; + /** * Returns a set of field names excluding their receivers (e.g. "this.a" will be "a") * @@ -128,4 +130,16 @@ static String getContractString(Symbol.MethodSymbol methodSymbol, Config config) } return null; } + + static String[] getContractClauses(Symbol.MethodSymbol callee, Config config) { + // Check to see if this method has an @Contract annotation + String contractString = getContractString(callee, config); + if (contractString != null) { + String trimmedContractString = contractString.trim(); + if (!trimmedContractString.isEmpty()) { + return trimmedContractString.split(";"); + } + } + return EMPTY_STRING_ARRAY; + } } diff --git a/nullaway/src/test/java/com/uber/nullaway/NullAwayContractsBooleanTests.java b/nullaway/src/test/java/com/uber/nullaway/NullAwayContractsBooleanTests.java new file mode 100644 index 0000000000..b04189a34e --- /dev/null +++ b/nullaway/src/test/java/com/uber/nullaway/NullAwayContractsBooleanTests.java @@ -0,0 +1,570 @@ +package com.uber.nullaway; + +import com.google.errorprone.CompilationTestHelper; +import java.util.Arrays; +import org.junit.Test; + +public class NullAwayContractsBooleanTests extends NullAwayTestsBase { + + @Test + public void nonNullCheckIsTrueIsNotNullable() { + helper() + .addSourceLines( + "Test.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "import java.util.Map;", + "class Test {", + " String test1(@Nullable Object o1) {", + " Validation.checkTrue(o1 != null);", + " return o1.toString();", + " }", + " String test2(Map map) {", + " Validation.checkTrue(map.get(\"key\") != null);", + " return map.get(\"key\").toString();", + " }", + " interface HasNullableGetter {", + " @Nullable Object get();", + " }", + " String test3(HasNullableGetter obj) {", + " Validation.checkTrue(obj.get() != null);", + " return obj.get().toString();", + " }", + "}") + .doTest(); + } + + @Test + public void nonNullCheckIsTrueIsNotNullableReversed() { + helper() + .addSourceLines( + "Test.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "import java.util.Map;", + "class Test {", + " String test(@Nullable Object o1) {", + " Validation.checkTrue(null != o1);", + " return o1.toString();", + " }", + " String test2(Map map) {", + " Validation.checkTrue(null != map.get(\"key\"));", + " return map.get(\"key\").toString();", + " }", + " interface HasNullableGetter {", + " @Nullable Object get();", + " }", + " String test3(HasNullableGetter obj) {", + " Validation.checkTrue(null != obj.get());", + " return obj.get().toString();", + " }", + "}") + .doTest(); + } + + @Test + public void nullCheckIsFalseIsNotNullable() { + helper() + .addSourceLines( + "Test.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "class Test {", + " String test(@Nullable Object o1) {", + " Validation.checkFalse(o1 == null);", + " return o1.toString();", + " }", + "}") + .doTest(); + } + + @Test + public void nullCheckIsFalseIsNotNullableReversed() { + helper() + .addSourceLines( + "Test.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "class Test {", + " String test(@Nullable Object o1) {", + " Validation.checkFalse(null == o1);", + " return o1.toString();", + " }", + "}") + .doTest(); + } + + @Test + public void nullCheckIsTrueIsNull() { + helper() + .addSourceLines( + "Test.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "class Test {", + " String test(@Nullable Object o1) {", + " Validation.checkTrue(o1 == null);", + " // BUG: Diagnostic contains: dereferenced expression", + " return o1.toString();", + " }", + "}") + .doTest(); + } + + @Test + public void nullCheckIsTrueIsNullReversed() { + helper() + .addSourceLines( + "Test.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "class Test {", + " String test(@Nullable Object o1) {", + " Validation.checkTrue(null == o1);", + " // BUG: Diagnostic contains: dereferenced expression", + " return o1.toString();", + " }", + "}") + .doTest(); + } + + @Test + public void nonNullCheckIsFalseIsNull() { + helper() + .addSourceLines( + "Test.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "class Test {", + " String test(@Nullable Object o1) {", + " Validation.checkFalse(o1 != null);", + " // BUG: Diagnostic contains: dereferenced expression", + " return o1.toString();", + " }", + "}") + .doTest(); + } + + @Test + public void nonNullCheckIsFalseIsNullReversed() { + helper() + .addSourceLines( + "Test.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "class Test {", + " String test(@Nullable Object o1) {", + " Validation.checkFalse(null != o1);", + " // BUG: Diagnostic contains: dereferenced expression", + " return o1.toString();", + " }", + "}") + .doTest(); + } + + @Test + public void compositeNullCheckAndStringEquality() { + helper() + .addSourceLines( + "Test.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "import java.util.Map;", + "class Test {", + " String test1(@Nullable Object o1) {", + " Validation.checkTrue(o1 != null && o1.toString().equals(\"\"));", + " return o1.toString();", + " }", + " String test2(@Nullable Object o1) {", + " Validation.checkTrue(o1 != null ||", + " // BUG: Diagnostic contains: dereferenced expression", + " o1.toString().equals(\"\"));", + " return o1.toString();", + " }", + " String test3(Map map) {", + " Validation.checkTrue(map.get(\"key\") != null && map.get(\"key\").toString().equals(\"\"));", + " return map.get(\"key\").toString();", + " }", + "}") + .doTest(); + } + + @Test + public void compositeNullCheckMultipleNonNull() { + helper() + .addSourceLines( + "Test.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "class Test {", + " String test(@Nullable Object o1, @Nullable Object o2) {", + " Validation.checkTrue(o1 != null && o2 != null);", + " return o1.toString() + o2.toString();", + " }", + "}") + .doTest(); + } + + @Test + public void compositeNullCheckFalseAndStringEquality() { + helper() + .addSourceLines( + "Test.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "class Test {", + " String test(@Nullable Object o1) {", + " Validation.checkFalse(o1 == null || o1.toString().equals(\"\"));", + " return o1.toString();", + " }", + "}") + .doTest(); + } + + @Test + public void compositeNullCheckFalseMultipleNonNull() { + helper() + .addSourceLines( + "Test.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "class Test {", + " String test1(@Nullable Object o1, @Nullable Object o2) {", + " Validation.checkFalse(o1 == null || o2 == null);", + " return o1.toString() + o2.toString();", + " }", + " String test2(@Nullable Object o1, @Nullable Object o2) {", + " Validation.checkFalse(o1 == null && o2 == null);", + " // BUG: Diagnostic contains: dereferenced expression", + " return o1.toString()", + " // BUG: Diagnostic contains: dereferenced expression", + " + o2.toString();", + " }", + "}") + .doTest(); + } + + @Test + public void identityNotNull() { + helper() + .addSourceLines( + "Test.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "class Test {", + " String test(@Nullable Object o1) {", + " if (Validation.identity(null != o1)) {", + " return o1.toString();", + " } else {", + " // BUG: Diagnostic contains: dereferenced expression", + " return o1.toString();", + " }", + " }", + "}") + .doTest(); + } + + @Test + public void complexIdentityNotNull() { + helper() + .addSourceLines( + "Test.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "class Test {", + " String test(@Nullable Object o1, Object o2) {", + " if (Validation.identity(null != o1, o2)) {", + " return o1.toString();", + " } else {", + " // BUG: Diagnostic contains: dereferenced expression", + " return o1.toString();", + " }", + " }", + "}") + .doTest(); + } + + @Test + public void identityIsNull() { + helper() + .addSourceLines( + "Test.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "class Test {", + " String test(@Nullable Object o1) {", + " if (Validation.identity(null == o1)) {", + " // BUG: Diagnostic contains: dereferenced expression", + " return o1.toString();", + " } else {", + " return o1.toString();", + " }", + " }", + "}") + .doTest(); + } + + @Test + public void complexContractIdentityIsNull() { + helper() + .addSourceLines( + "Test.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "class Test {", + " String test(@Nullable Object o1, Object o2) {", + " if (Validation.identity(null == o1, o2)) {", + " // BUG: Diagnostic contains: dereferenced expression", + " return o1.toString();", + " } else {", + " return o1.toString();", + " }", + " }", + "}") + .doTest(); + } + + @Test + public void checkAndReturn() { + helper() + .addSourceLines( + "Test.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "import org.jetbrains.annotations.Contract;", + "class Test {", + " @Contract(\"false -> fail\")", + " static boolean checkAndReturn(boolean value) {", + " if (!value) {", + " throw new RuntimeException();", + " }", + " return true;", + " }", + " String test1(@Nullable Object o1, @Nullable Object o2) {", + " if (checkAndReturn(o1 != null) && o2 != null) {", + " return o1.toString() + o2.toString();", + " } else {", + " return o1.toString() + ", + " // BUG: Diagnostic contains: dereferenced expression", + " o2.toString();", + " }", + " }", + " boolean test2(@Nullable Object o1, @Nullable Object o2) {", + " return checkAndReturn(o1 != null) && o1.toString().isEmpty();", + " }", + " boolean test3(@Nullable Object o1, @Nullable Object o2) {", + " return checkAndReturn(o1 == null) ", + " // BUG: Diagnostic contains: dereferenced expression", + " && o1.toString().isEmpty();", + " }", + "}") + .doTest(); + } + + @Test + public void complexCheckAndReturn() { + helper() + .addSourceLines( + "Test.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "import org.jetbrains.annotations.Contract;", + "class Test {", + " @Contract(\"false, _ -> fail\")", + " static boolean checkAndReturn(boolean value, Object other) {", + " if (!value) {", + " throw new RuntimeException();", + " }", + " return true;", + " }", + " String test1(@Nullable Object o1, @Nullable Object o2, Object other) {", + " if (checkAndReturn(o1 != null, other) && o2 != null) {", + " return o1.toString() + o2.toString();", + " } else {", + " return o1.toString() + ", + " // BUG: Diagnostic contains: dereferenced expression", + " o2.toString();", + " }", + " }", + " boolean test2(@Nullable Object o1, @Nullable Object o2, Object other) {", + " return checkAndReturn(o1 != null, other) && o1.toString().isEmpty();", + " }", + " boolean test3(@Nullable Object o1, @Nullable Object o2, Object other) {", + " return checkAndReturn(o1 == null, other) ", + " // BUG: Diagnostic contains: dereferenced expression", + " && o1.toString().isEmpty();", + " }", + "}") + .doTest(); + } + + @Test + public void contractUnreachablePath() { + helper() + .addSourceLines( + "Test.java", + "package com.uber;", + "class Test {", + " String test(Object required) {", + " return Validation.identity(required == null)", + " ? required.toString()", + " : required.toString();", + " }", + "}") + .doTest(); + } + + @Test + public void complexContractUnreachablePath() { + helper() + .addSourceLines( + "Test.java", + "package com.uber;", + "class Test {", + " String test(Object required, Object other) {", + " return Validation.identity(required == null, other)", + " ? required.toString()", + " : required.toString();", + " }", + "}") + .doTest(); + } + + @Test + public void contractUnreachablePathAfterFailure() { + helper() + .addSourceLines( + "Test.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "class Test {", + " String test(@Nullable Object o) {", + " Validation.checkTrue(o == null);", + " return Validation.identity(o == null)", + " // BUG: Diagnostic contains: dereferenced expression", + " ? o.toString()", + // This path is unreachable because o is guaranteed to be null + // after checkTrue(o == null). No failures should be reported. + // Note that we're not doing general reachability analysis, + // rather ensuring that we don't incorrectly produce errors. + " : o.toString();", + " }", + "}") + .doTest(); + } + + @Test + public void contractNestedBooleanNullness() { + helper() + .addSourceLines( + "Test.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "class Test {", + " String test(@Nullable Object o) {", + " return Validation.identity(o == null)", + " ? (Validation.identity(o != null)", + " ? o.toString()", + " // BUG: Diagnostic contains: dereferenced expression", + " : o.toString())", + " : (Validation.identity(o != null)", + " ? o.toString()", + " : o.toString());", + " }", + "}") + .doTest(); + } + + @Test + public void complexContractNestedBooleanNullness() { + helper() + .addSourceLines( + "Test.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "class Test {", + " String test(@Nullable Object o, Object other) {", + " return Validation.identity(o == null, other)", + " ? (Validation.identity(o != null, other)", + " ? o.toString()", + " // BUG: Diagnostic contains: dereferenced expression", + " : o.toString())", + " : (Validation.identity(o != null, other)", + " ? o.toString()", + " : o.toString());", + " }", + "}") + .doTest(); + } + + @Test + public void nonNullBooleanDoubleContractTest() { + helper() + .addSourceLines( + "Test.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "import org.jetbrains.annotations.Contract;", + "class Test {", + " @Contract(\"false, false -> fail\")", + " static void falseFalseFail(boolean b1, boolean b2) {", + " if (!b1 && !b2) {", + " throw new RuntimeException();", + " }", + " }", + " String test1(@Nullable Object maybe, Object required) {", + // 'required == null' is known to be false, so if we go past this line, + // we know 'maybe != null' evaluates to true, hence both args are @NonNull. + " falseFalseFail(maybe != null, required == null);", + " return maybe.toString() + required.toString();", + " }", + " String test2(@Nullable Object maybe) {", + " String ref = null;", + // 'ref != null' is known to be false, so if we go past this line, + // we know 'maybe != null' evaluates to true. + " falseFalseFail(maybe != null, ref != null);", + " return maybe.toString();", + " }", + " String test3(@Nullable Object maybe) {", + " String ref = \"\";", + " ref = null;", + // 'ref != null' is known to be false, so if we go past this line, + // we know 'maybe != null' evaluates to true. + " falseFalseFail(maybe != null, ref != null);", + " return maybe.toString();", + " }", + "}") + .doTest(); + } + + private CompilationTestHelper helper() { + return makeTestHelperWithArgs( + Arrays.asList( + "-d", + temporaryFolder.getRoot().getAbsolutePath(), + "-XepOpt:NullAway:AnnotatedPackages=com.uber")) + .addSourceLines( + "Validation.java", + "package com.uber;", + "import org.jetbrains.annotations.Contract;", + "import javax.annotation.Nullable;", + "public final class Validation {", + " @Contract(\"false -> fail\")", + " static void checkTrue(boolean value) {", + " if (!value) throw new RuntimeException();", + " }", + " @Contract(\"true -> fail\")", + " static void checkFalse(boolean value) {", + " if (value) throw new RuntimeException();", + " }", + " @Contract(\"true -> true; false -> false\")", + " static boolean identity(boolean value) {", + " return value;", + " }", + " @Contract(\"true, _ -> true; false, _ -> false\")", + " static boolean identity(boolean value, @Nullable Object other) {", + " return value;", + " }", + "}"); + } +} diff --git a/nullaway/src/test/java/com/uber/nullaway/NullAwayContractsTests.java b/nullaway/src/test/java/com/uber/nullaway/NullAwayContractsTests.java index 452676a6b4..235cee8045 100644 --- a/nullaway/src/test/java/com/uber/nullaway/NullAwayContractsTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/NullAwayContractsTests.java @@ -267,4 +267,173 @@ public void customContractAnnotation() { "}") .doTest(); } + + @Test + public void contractDeclaringBothNotNull() { + makeTestHelperWithArgs( + Arrays.asList( + "-d", + temporaryFolder.getRoot().getAbsolutePath(), + "-XepOpt:NullAway:AnnotatedPackages=com.uber")) + .addSourceLines( + "NullnessChecker.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "import org.jetbrains.annotations.Contract;", + "public class NullnessChecker {", + " @Contract(\"null, _ -> false; _, null -> false\")", + " static boolean bothNotNull(@Nullable Object o1, @Nullable Object o2) {", + " // null, _ -> false", + " if (o1 == null) {", + " return false;", + " }", + " // _, null -> false", + " if (o2 == null) {", + " return false;", + " }", + " // Function cannot declare a contract for true", + " return System.currentTimeMillis() % 100 == 0;", + " }", + "}") + .addSourceLines( + "Test.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "class Test {", + " String test1(@Nullable Object o) {", + " return NullnessChecker.bothNotNull(\"\", o)", + " ? o.toString()", + " // BUG: Diagnostic contains: dereferenced expression", + " : o.toString();", + " }", + " String test2(@Nullable Object o) {", + " return NullnessChecker.bothNotNull(o, \"\")", + " ? o.toString()", + " // BUG: Diagnostic contains: dereferenced expression", + " : o.toString();", + " }", + "}") + .doTest(); + } + + @Test + public void contractDeclaringEitherNull() { + makeTestHelperWithArgs( + Arrays.asList( + "-d", + temporaryFolder.getRoot().getAbsolutePath(), + "-XepOpt:NullAway:AnnotatedPackages=com.uber")) + .addSourceLines( + "NullnessChecker.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "import org.jetbrains.annotations.Contract;", + "public class NullnessChecker {", + " @Contract(\"null, _ -> true; _, null -> true\")", + " static boolean eitherIsNullOrRandom(@Nullable Object o1, @Nullable Object o2) {", + " // null, _ -> true", + " if (o1 == null) {", + " return true;", + " }", + " // _, null -> true", + " if (o2 == null) {", + " return true;", + " }", + " // Function cannot declare a contract for false", + " return System.currentTimeMillis() % 100 == 0;", + " }", + "}") + .addSourceLines( + "Test.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "class Test {", + " String test1(@Nullable Object o) {", + " return NullnessChecker.eitherIsNullOrRandom(\"\", o)", + " // BUG: Diagnostic contains: dereferenced expression", + " ? o.toString()", + " : o.toString();", + " }", + " String test2(@Nullable Object o) {", + " return NullnessChecker.eitherIsNullOrRandom(o, \"\")", + " // BUG: Diagnostic contains: dereferenced expression", + " ? o.toString()", + " : o.toString();", + " }", + "}") + .doTest(); + } + + @Test + public void contractDeclaringNullOrRandomFalse() { + makeTestHelperWithArgs( + Arrays.asList( + "-d", + temporaryFolder.getRoot().getAbsolutePath(), + "-XepOpt:NullAway:AnnotatedPackages=com.uber")) + .addSourceLines( + "NullnessChecker.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "import org.jetbrains.annotations.Contract;", + "public class NullnessChecker {", + " @Contract(\"null -> false\")", + " static boolean isHashCodeZero(@Nullable Object o) {", + " // null -> false", + " if (o == null) {", + " return false;", + " }", + " // Function cannot declare a contract for true", + " return o.hashCode() == 0;", + " }", + "}") + .addSourceLines( + "Test.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "class Test {", + " String test1(@Nullable Object o) {", + " return NullnessChecker.isHashCodeZero(o)", + " ? o.toString()", + " // BUG: Diagnostic contains: dereferenced expression", + " : o.toString();", + " }", + "}") + .doTest(); + } + + @Test + public void contractUnreachablePath() { + makeTestHelperWithArgs( + Arrays.asList( + "-d", + temporaryFolder.getRoot().getAbsolutePath(), + "-XepOpt:NullAway:AnnotatedPackages=com.uber")) + .addSourceLines( + "NullnessChecker.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "import org.jetbrains.annotations.Contract;", + "public class NullnessChecker {", + " @Contract(\"!null -> false\")", + " static boolean isNull(@Nullable Object o) {", + " // !null -> false", + " if (o != null) {", + " return false;", + " }", + " return true;", + " }", + "}") + .addSourceLines( + "Test.java", + "package com.uber;", + "class Test {", + " String test(Object required) {", + " return NullnessChecker.isNull(required)", + " ? required.toString()", + " : required.toString();", + " }", + "}") + .doTest(); + } } From 94afe5abd52e9b695f97effd0b1a00813be3ed45 Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Thu, 27 Oct 2022 14:37:02 -0700 Subject: [PATCH 04/14] Handle Guava `Verify` functions (#682) Adds handling of the `verifyNotNull` and `verify` functions in an analogous manner to how we currently handle `checkNotNull`, `checkArgument`, and `checkState`. Fixes #666 --- ...ndler.java => GuavaAssertionsHandler.java} | 22 ++++++++- .../com/uber/nullaway/handlers/Handlers.java | 2 +- .../handlers/LibraryModelsHandler.java | 6 +++ ...java => NullAwayGuavaAssertionsTests.java} | 48 ++++++++++++++++++- 4 files changed, 75 insertions(+), 3 deletions(-) rename nullaway/src/main/java/com/uber/nullaway/handlers/{PreconditionsHandler.java => GuavaAssertionsHandler.java} (68%) rename nullaway/src/test/java/com/uber/nullaway/{NullAwayPreconditionTests.java => NullAwayGuavaAssertionsTests.java} (85%) diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/PreconditionsHandler.java b/nullaway/src/main/java/com/uber/nullaway/handlers/GuavaAssertionsHandler.java similarity index 68% rename from nullaway/src/main/java/com/uber/nullaway/handlers/PreconditionsHandler.java rename to nullaway/src/main/java/com/uber/nullaway/handlers/GuavaAssertionsHandler.java index 2cb276de32..53561715c9 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/PreconditionsHandler.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/GuavaAssertionsHandler.java @@ -10,17 +10,26 @@ import javax.lang.model.type.TypeMirror; import org.checkerframework.nullaway.dataflow.cfg.node.MethodInvocationNode; -public class PreconditionsHandler extends BaseNoOpHandler { +/** + * Handler to expose semantics of Guava routines like {@code checkState}, {@code checkArgument}, and + * {@code verify} that check a boolean condition and fail with an exception if it is false. + */ +public class GuavaAssertionsHandler extends BaseNoOpHandler { private static final String PRECONDITIONS_CLASS_NAME = "com.google.common.base.Preconditions"; private static final String CHECK_ARGUMENT_METHOD_NAME = "checkArgument"; private static final String CHECK_STATE_METHOD_NAME = "checkState"; + private static final String VERIFY_CLASS_NAME = "com.google.common.base.Verify"; + private static final String VERIFY_METHOD_NAME = "verify"; @Nullable private Name preconditionsClass; + @Nullable private Name verifyClass; @Nullable private Name checkArgumentMethod; @Nullable private Name checkStateMethod; + @Nullable private Name verifyMethod; @Nullable TypeMirror preconditionCheckArgumentErrorType; @Nullable TypeMirror preconditionCheckStateErrorType; + @Nullable TypeMirror verifyErrorType; @Override public MethodInvocationNode onCFGBuildPhase1AfterVisitMethodInvocation( @@ -30,13 +39,20 @@ public MethodInvocationNode onCFGBuildPhase1AfterVisitMethodInvocation( Symbol.MethodSymbol callee = ASTHelpers.getSymbol(tree); if (preconditionsClass == null) { preconditionsClass = callee.name.table.fromString(PRECONDITIONS_CLASS_NAME); + verifyClass = callee.name.table.fromString(VERIFY_CLASS_NAME); checkArgumentMethod = callee.name.table.fromString(CHECK_ARGUMENT_METHOD_NAME); checkStateMethod = callee.name.table.fromString(CHECK_STATE_METHOD_NAME); + verifyMethod = callee.name.table.fromString(VERIFY_METHOD_NAME); preconditionCheckArgumentErrorType = phase.classToErrorType(IllegalArgumentException.class); preconditionCheckStateErrorType = phase.classToErrorType(IllegalStateException.class); + // We treat the Verify.* APIs as throwing a RuntimeException to avoid any issues with + // the VerifyException that they actually throw not being in the classpath (this will not + // affect the analysis result) + verifyErrorType = phase.classToErrorType(RuntimeException.class); } Preconditions.checkNotNull(preconditionCheckArgumentErrorType); Preconditions.checkNotNull(preconditionCheckStateErrorType); + Preconditions.checkNotNull(verifyErrorType); if (callee.enclClass().getQualifiedName().equals(preconditionsClass) && !callee.getParameters().isEmpty()) { // Attempt to match Precondition check methods to the expected exception type, providing as @@ -49,6 +65,10 @@ public MethodInvocationNode onCFGBuildPhase1AfterVisitMethodInvocation( } else if (callee.name.equals(checkStateMethod)) { phase.insertThrowOnFalse(originalNode.getArgument(0), preconditionCheckStateErrorType); } + } else if (callee.enclClass().getQualifiedName().equals(verifyClass) + && !callee.getParameters().isEmpty() + && callee.name.equals(verifyMethod)) { + phase.insertThrowOnFalse(originalNode.getArgument(0), verifyErrorType); } return originalNode; } diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/Handlers.java b/nullaway/src/main/java/com/uber/nullaway/handlers/Handlers.java index 7c8c6272bf..ba46360a87 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/Handlers.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/Handlers.java @@ -55,7 +55,7 @@ public static Handler buildDefault(Config config) { if (config.handleTestAssertionLibraries()) { handlerListBuilder.add(new AssertionHandler(methodNameUtil)); } - handlerListBuilder.add(new PreconditionsHandler()); + handlerListBuilder.add(new GuavaAssertionsHandler()); handlerListBuilder.add(new LibraryModelsHandler(config)); handlerListBuilder.add(StreamNullabilityPropagatorFactory.getRxStreamNullabilityPropagator()); handlerListBuilder.add(StreamNullabilityPropagatorFactory.getJavaStreamNullabilityPropagator()); 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 7997851fd9..9670c53022 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java @@ -411,6 +411,12 @@ private static class DefaultLibraryModels implements LibraryModels { "com.google.common.base.Preconditions", "checkNotNull(T,java.lang.String,java.lang.Object,java.lang.Object,java.lang.Object,java.lang.Object)"), 0) + .put(methodRef("com.google.common.base.Verify", "verifyNotNull(T)"), 0) + .put( + methodRef( + "com.google.common.base.Verify", + "verifyNotNull(T,java.lang.String,java.lang.Object...)"), + 0) .put(methodRef("java.util.Objects", "requireNonNull(T)"), 0) .put(methodRef("java.util.Objects", "requireNonNull(T,java.lang.String)"), 0) .put( diff --git a/nullaway/src/test/java/com/uber/nullaway/NullAwayPreconditionTests.java b/nullaway/src/test/java/com/uber/nullaway/NullAwayGuavaAssertionsTests.java similarity index 85% rename from nullaway/src/test/java/com/uber/nullaway/NullAwayPreconditionTests.java rename to nullaway/src/test/java/com/uber/nullaway/NullAwayGuavaAssertionsTests.java index 927aeac863..0fcb39a435 100644 --- a/nullaway/src/test/java/com/uber/nullaway/NullAwayPreconditionTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/NullAwayGuavaAssertionsTests.java @@ -3,7 +3,7 @@ import java.util.Arrays; import org.junit.Test; -public class NullAwayPreconditionTests extends NullAwayTestsBase { +public class NullAwayGuavaAssertionsTests extends NullAwayTestsBase { @Test public void checkNotNullTest() { @@ -26,6 +26,31 @@ public void checkNotNullTest() { .doTest(); } + @Test + public void verifyNotNullTest() { + makeTestHelperWithArgs( + Arrays.asList( + "-d", + temporaryFolder.getRoot().getAbsolutePath(), + "-XepOpt:NullAway:AnnotatedPackages=com.uber")) + .addSourceLines( + "Test.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "import com.google.common.base.Verify;", + "class Test {", + " private void foo(@Nullable Object a) {", + " Verify.verifyNotNull(a);", + " a.toString();", + " }", + " private void bar(@Nullable Object a) {", + " Verify.verifyNotNull(a, \"message\", new Object(), new Object());", + " a.toString();", + " }", + "}") + .doTest(); + } + @Test public void simpleCheckArgumentTest() { makeTestHelperWithArgs( @@ -69,6 +94,27 @@ public void simpleCheckStateTest() { .doTest(); } + @Test + public void simpleVerifyTest() { + makeTestHelperWithArgs( + Arrays.asList( + "-d", + temporaryFolder.getRoot().getAbsolutePath(), + "-XepOpt:NullAway:AnnotatedPackages=com.uber")) + .addSourceLines( + "Test.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "import com.google.common.base.Verify;", + "class Test {", + " private void foo(@Nullable Object a) {", + " Verify.verify(a != null);", + " a.toString();", + " }", + "}") + .doTest(); + } + @Test public void simpleCheckArgumentWithMessageTest() { makeTestHelperWithArgs( From a2c8d18a3803d63292fe3a9e5a133c5b5c6a92ac Mon Sep 17 00:00:00 2001 From: Lazaro Clapp Date: Fri, 28 Oct 2022 14:42:21 -0700 Subject: [PATCH 05/14] Prepare for release 0.10.3. --- CHANGELOG.md | 14 ++++++++++++++ README.md | 4 ++-- gradle.properties | 2 +- 3 files changed, 17 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cee1ab4e96..4f982b08c2 100755 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,19 @@ Changelog ========= +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) +* Support for specific libraries/APIs: + - PreconditionsHandler reflects Guava Preconditions exception types (#668) + - Handle Guava Verify functions (#682) +* Dependency Updates: + - checkerframework 3.26.0 (#671) +* Build / CI tooling for NullAway itself: + - Build and test against Error Prone 2.15.0 (#665) + - Bump Error Prone and EP plugin to 2.16 (#675) + Version 0.10.2 -------------- * Make AbstractConfig collection fields explicity Immutable (#601) diff --git a/README.md b/README.md index cfea33cfcd..af7ddcf282 100644 --- a/README.md +++ b/README.md @@ -23,7 +23,7 @@ plugins { } dependencies { - annotationProcessor "com.uber.nullaway:nullaway:0.10.2" + annotationProcessor "com.uber.nullaway:nullaway:0.10.3" // Optional, some source of nullability annotations. // Not required on Android if you use the support @@ -75,7 +75,7 @@ The configuration for an Android project is very similar to the Java case, with ```gradle dependencies { - annotationProcessor "com.uber.nullaway:nullaway:0.10.2" + annotationProcessor "com.uber.nullaway:nullaway:0.10.3" errorprone "com.google.errorprone:error_prone_core:2.4.0" errorproneJavac "com.google.errorprone:javac:9+181-r4173-1" } diff --git a/gradle.properties b/gradle.properties index 358556f774..b75faae3c9 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.3-SNAPSHOT +VERSION_NAME=0.10.3 POM_DESCRIPTION=A fast annotation-based null checker for Java From b696757aebf22c74a8082c6141a969c419cca671 Mon Sep 17 00:00:00 2001 From: Lazaro Clapp Date: Fri, 28 Oct 2022 14:51:52 -0700 Subject: [PATCH 06/14] Prepare next development version. --- gradle.properties | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gradle.properties b/gradle.properties index b75faae3c9..a468eb4f67 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.3 +VERSION_NAME=0.10.4-SNAPSHOT POM_DESCRIPTION=A fast annotation-based null checker for Java From f27e72b166754704b55b05085e73407e2eb46408 Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Mon, 31 Oct 2022 12:35:29 -0700 Subject: [PATCH 07/14] Bump dependency versions in GitHub Actions config (#683) We get various warnings about use of deprecated features when running CI jobs; these version bumps seem to fix them. --- .github/workflows/continuous-integration.yml | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/.github/workflows/continuous-integration.yml b/.github/workflows/continuous-integration.yml index 51b3931540..cffb1ae88a 100644 --- a/.github/workflows/continuous-integration.yml +++ b/.github/workflows/continuous-integration.yml @@ -35,9 +35,9 @@ jobs: runs-on: ${{ matrix.os }} steps: - name: Check out NullAway sources - uses: actions/checkout@v2 + uses: actions/checkout@v3 - name: 'Set up JDK ${{ matrix.java }}' - uses: actions/setup-java@v2 + uses: actions/setup-java@v3 with: java-version: ${{ matrix.java }} distribution: 'temurin' @@ -80,9 +80,9 @@ jobs: runs-on: ubuntu-latest steps: - name: 'Check out repository' - uses: actions/checkout@v2 + uses: actions/checkout@v3 - name: 'Set up JDK 8' - uses: actions/setup-java@v2 + uses: actions/setup-java@v3 with: java-version: 8 distribution: 'temurin' From d1415f4c342ba66299a33d9206ec76987f32272c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C3=A1zaro=20Clapp?= Date: Wed, 2 Nov 2022 00:44:05 -0400 Subject: [PATCH 08/14] Fix LibraryModels recording of dataflow nullness for Map APs (#685) We had a long-standing bug in our recording of the nullness of access paths inside `LibraryModelsHandler`. Namely, as part of `accessPathsAtIndexes`, we were calling `getAccessPathForNodeNoMapGet`, rather than `getAccessPathForNodeWithMapGet`. Among other things, `accessPathsAtIndexes` is used to calculate which arguments for a method like `Preconditions.checkNotNull([expr])` are marked as being non-null after the method's execution (this extends to all methods in `failIfNullParameters` and similar logic applies to other types of library models). Due to our use of `getAccessPathForNodeNoMapGet`, code like the following: ``` private void foo(Map m) { Preconditions.checkNotNull(m.get(\"foo\")); m.get(\"foo\").toString(); // FP! } } ``` Would result in a false positive, where `m.get(\"foo\").toString()` shows up as a null-dereference, despite the preceding `Preconditions.checkNotNull` call. This despite the following working without issue: ``` private void foo(@Nullable Object o) { Preconditions.checkNotNull(o); o.toString(); // Worked before and still works! } } ``` This PR fixes this bug. The core fix is simply replacing the `getAccessPathForNodeNoMapGet` with `getAccessPathForNodeWithMapGet`. However, to achieve this, we must thread `VisitorState` all the way to the `LibraryModelsHandler.accessPathsAtIndexes(...)` method, which requires changing the signature of the `onDataflowVisitMethodInvocation(...)` handler extension point. This makes the bulk of the code changes in this PR. Finally, we include a few test cases for unboxing, which are an artifact of how we discovered and then triagged this bug. (Edit for posterity: thanks @msridhar for finding the root cause here) --- .../AccessPathNullnessPropagation.java | 9 +-- .../handlers/ApacheThriftIsSetHandler.java | 6 +- .../nullaway/handlers/AssertionHandler.java | 6 +- .../nullaway/handlers/BaseNoOpHandler.java | 3 +- .../nullaway/handlers/CompositeHandler.java | 5 +- .../uber/nullaway/handlers/GrpcHandler.java | 5 +- .../com/uber/nullaway/handlers/Handler.java | 6 +- .../handlers/InferredJARModelsHandler.java | 4 +- .../handlers/LibraryModelsHandler.java | 40 ++++++----- .../handlers/OptionalEmptinessHandler.java | 10 +-- .../RestrictiveAnnotationHandler.java | 5 +- .../handlers/contract/ContractHandler.java | 25 ++++--- .../fieldcontract/EnsuresNonNullHandler.java | 9 +-- .../com/uber/nullaway/NullAwayCoreTests.java | 68 +++++++++++++++++++ .../NullAwayGuavaAssertionsTests.java | 36 ++++++++++ 15 files changed, 164 insertions(+), 73 deletions(-) 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 aa003d7df1..65bd0e5538 100644 --- a/nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPathNullnessPropagation.java +++ b/nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPathNullnessPropagation.java @@ -923,14 +923,7 @@ public TransferResult visitMethodInvocation( node, callee, node.getArguments(), values(input), thenUpdates, bothUpdates); NullnessHint nullnessHint = handler.onDataflowVisitMethodInvocation( - node, - state.getTypes(), - state.context, - apContext, - values(input), - thenUpdates, - elseUpdates, - bothUpdates); + node, state, apContext, values(input), thenUpdates, elseUpdates, bothUpdates); Nullness nullness = returnValueNullness(node, input, nullnessHint); if (booleanReturnType(node)) { ResultingStore thenStore = updateStore(input.getThenStore(), thenUpdates, bothUpdates); diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/ApacheThriftIsSetHandler.java b/nullaway/src/main/java/com/uber/nullaway/handlers/ApacheThriftIsSetHandler.java index 1298cc3a3f..15a47933b3 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/ApacheThriftIsSetHandler.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/ApacheThriftIsSetHandler.java @@ -30,7 +30,6 @@ import com.sun.tools.javac.code.Symbol; import com.sun.tools.javac.code.Type; import com.sun.tools.javac.code.Types; -import com.sun.tools.javac.util.Context; import com.uber.nullaway.NullAway; import com.uber.nullaway.Nullness; import com.uber.nullaway.dataflow.AccessPath; @@ -68,15 +67,14 @@ public void onMatchTopLevelClass( @Override public NullnessHint onDataflowVisitMethodInvocation( MethodInvocationNode node, - Types types, - Context context, + VisitorState state, AccessPath.AccessPathContext apContext, AccessPathNullnessPropagation.SubNodeValues inputs, AccessPathNullnessPropagation.Updates thenUpdates, AccessPathNullnessPropagation.Updates elseUpdates, AccessPathNullnessPropagation.Updates bothUpdates) { Symbol.MethodSymbol symbol = ASTHelpers.getSymbol(node.getTree()); - if (thriftIsSetCall(symbol, types)) { + if (thriftIsSetCall(symbol, state.getTypes())) { String methodName = symbol.getSimpleName().toString(); // remove "isSet" String capPropName = methodName.substring(5); 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 c2b650e827..ddc9572854 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/AssertionHandler.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/AssertionHandler.java @@ -24,10 +24,9 @@ import static com.uber.nullaway.Nullness.NONNULL; +import com.google.errorprone.VisitorState; import com.google.errorprone.util.ASTHelpers; 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.dataflow.AccessPath; import com.uber.nullaway.dataflow.AccessPathNullnessPropagation; import java.util.List; @@ -46,8 +45,7 @@ public class AssertionHandler extends BaseNoOpHandler { @Override public NullnessHint onDataflowVisitMethodInvocation( MethodInvocationNode node, - Types types, - Context context, + VisitorState state, AccessPath.AccessPathContext apContext, AccessPathNullnessPropagation.SubNodeValues inputs, AccessPathNullnessPropagation.Updates thenUpdates, 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 8baeb87caf..8161dcb1bf 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/BaseNoOpHandler.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/BaseNoOpHandler.java @@ -146,8 +146,7 @@ public NullnessStore.Builder onDataflowInitialStore( @Override public NullnessHint onDataflowVisitMethodInvocation( MethodInvocationNode node, - Types types, - Context context, + VisitorState state, AccessPath.AccessPathContext apContext, AccessPathNullnessPropagation.SubNodeValues inputs, AccessPathNullnessPropagation.Updates thenUpdates, 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 c4f8553d7d..7018b46c7c 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/CompositeHandler.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/CompositeHandler.java @@ -173,8 +173,7 @@ public NullnessStore.Builder onDataflowInitialStore( @Override public NullnessHint onDataflowVisitMethodInvocation( MethodInvocationNode node, - Types types, - Context context, + VisitorState state, AccessPath.AccessPathContext apContext, AccessPathNullnessPropagation.SubNodeValues inputs, AccessPathNullnessPropagation.Updates thenUpdates, @@ -184,7 +183,7 @@ public NullnessHint onDataflowVisitMethodInvocation( for (Handler h : handlers) { NullnessHint n = h.onDataflowVisitMethodInvocation( - node, types, context, apContext, inputs, thenUpdates, elseUpdates, bothUpdates); + node, state, apContext, inputs, thenUpdates, elseUpdates, bothUpdates); nullnessHint = nullnessHint.merge(n); } return nullnessHint; diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/GrpcHandler.java b/nullaway/src/main/java/com/uber/nullaway/handlers/GrpcHandler.java index 581ff5159d..629aadfbc9 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/GrpcHandler.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/GrpcHandler.java @@ -34,7 +34,6 @@ import com.sun.tools.javac.code.Symbol; import com.sun.tools.javac.code.Type; import com.sun.tools.javac.code.Types; -import com.sun.tools.javac.util.Context; import com.uber.nullaway.NullAway; import com.uber.nullaway.Nullness; import com.uber.nullaway.dataflow.AccessPath; @@ -80,8 +79,7 @@ public void onMatchTopLevelClass( @Override public NullnessHint onDataflowVisitMethodInvocation( MethodInvocationNode node, - Types types, - Context context, + VisitorState state, AccessPath.AccessPathContext apContext, AccessPathNullnessPropagation.SubNodeValues inputs, AccessPathNullnessPropagation.Updates thenUpdates, @@ -89,6 +87,7 @@ public NullnessHint onDataflowVisitMethodInvocation( AccessPathNullnessPropagation.Updates bothUpdates) { MethodInvocationTree tree = castToNonNull(node.getTree()); Symbol.MethodSymbol symbol = ASTHelpers.getSymbol(tree); + Types types = state.getTypes(); if (grpcIsMetadataContainsKeyCall(symbol, types)) { // On seeing o.containsKey(k), set AP for o.get(k) to @NonNull Element getter = getGetterForMetadataSubtype(symbol.enclClass(), types); 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 bfc255da77..8379a4fae3 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/Handler.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/Handler.java @@ -217,8 +217,7 @@ NullnessStore.Builder onDataflowInitialStore( * Called when the Dataflow analysis visits each method invocation. * * @param node The AST node for the method callsite. - * @param types {@link Types} for the current compilation - * @param context the javac Context object (or Error Prone SubContext) + * @param state The current visitor state. * @param apContext the current access path context information (see {@link * AccessPath.AccessPathContext}). * @param inputs NullnessStore information known before the method invocation. @@ -234,8 +233,7 @@ NullnessStore.Builder onDataflowInitialStore( */ NullnessHint onDataflowVisitMethodInvocation( MethodInvocationNode node, - Types types, - Context context, + VisitorState state, AccessPath.AccessPathContext apContext, AccessPathNullnessPropagation.SubNodeValues inputs, AccessPathNullnessPropagation.Updates thenUpdates, 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 db8a40e2bb..bd364742fd 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/InferredJARModelsHandler.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/InferredJARModelsHandler.java @@ -30,7 +30,6 @@ 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.code.Types; import com.sun.tools.javac.util.Context; import com.uber.nullaway.Config; import com.uber.nullaway.NullAway; @@ -179,8 +178,7 @@ public Nullness[] onOverrideMethodInvocationParametersNullability( @Override public NullnessHint onDataflowVisitMethodInvocation( MethodInvocationNode node, - Types types, - Context context, + VisitorState state, AccessPath.AccessPathContext apContext, AccessPathNullnessPropagation.SubNodeValues inputs, AccessPathNullnessPropagation.Updates thenUpdates, 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 9670c53022..4f1f251328 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java @@ -183,8 +183,7 @@ private CodeAnnotationInfo getCodeAnnotationInfo(Context context) { @Override public NullnessHint onDataflowVisitMethodInvocation( MethodInvocationNode node, - Types types, - Context context, + VisitorState state, AccessPath.AccessPathContext apContext, AccessPathNullnessPropagation.SubNodeValues inputs, AccessPathNullnessPropagation.Updates thenUpdates, @@ -193,12 +192,12 @@ public NullnessHint onDataflowVisitMethodInvocation( Symbol.MethodSymbol callee = ASTHelpers.getSymbol(node.getTree()); Preconditions.checkNotNull(callee); boolean isMethodAnnotated = - !getCodeAnnotationInfo(context).isSymbolUnannotated(callee, this.config); - setUnconditionalArgumentNullness(bothUpdates, node.getArguments(), callee, context, apContext); + !getCodeAnnotationInfo(state.context).isSymbolUnannotated(callee, this.config); + setUnconditionalArgumentNullness(bothUpdates, node.getArguments(), callee, state, apContext); setConditionalArgumentNullness( - thenUpdates, elseUpdates, node.getArguments(), callee, context, apContext); + thenUpdates, elseUpdates, node.getArguments(), callee, state, apContext); ImmutableSet nullImpliesNullIndexes = - getOptLibraryModels(context).nullImpliesNullParameters(callee); + getOptLibraryModels(state.context).nullImpliesNullParameters(callee); if (!nullImpliesNullIndexes.isEmpty()) { // If the method is marked as having argument dependent nullability and any of the // corresponding arguments is null, then the return is nullable. If the method is @@ -212,9 +211,11 @@ public NullnessHint onDataflowVisitMethodInvocation( } return anyNull ? NullnessHint.HINT_NULLABLE : NullnessHint.FORCE_NONNULL; } - if (getOptLibraryModels(context).hasNonNullReturn(callee, types, !isMethodAnnotated)) { + Types types = state.getTypes(); + if (getOptLibraryModels(state.context).hasNonNullReturn(callee, types, !isMethodAnnotated)) { return NullnessHint.FORCE_NONNULL; - } else if (getOptLibraryModels(context).hasNullableReturn(callee, types, !isMethodAnnotated)) { + } else if (getOptLibraryModels(state.context) + .hasNullableReturn(callee, types, !isMethodAnnotated)) { return NullnessHint.HINT_NULLABLE; } else { return NullnessHint.UNKNOWN; @@ -226,30 +227,33 @@ private void setConditionalArgumentNullness( AccessPathNullnessPropagation.Updates elseUpdates, List arguments, Symbol.MethodSymbol callee, - Context context, + VisitorState state, AccessPath.AccessPathContext apContext) { Set nullImpliesTrueParameters = - getOptLibraryModels(context).nullImpliesTrueParameters(callee); + getOptLibraryModels(state.context).nullImpliesTrueParameters(callee); Set nullImpliesFalseParameters = - getOptLibraryModels(context).nullImpliesFalseParameters(callee); + getOptLibraryModels(state.context).nullImpliesFalseParameters(callee); for (AccessPath accessPath : - accessPathsAtIndexes(nullImpliesTrueParameters, arguments, apContext)) { + accessPathsAtIndexes(nullImpliesTrueParameters, arguments, state, apContext)) { elseUpdates.set(accessPath, NONNULL); } for (AccessPath accessPath : - accessPathsAtIndexes(nullImpliesFalseParameters, arguments, apContext)) { + accessPathsAtIndexes(nullImpliesFalseParameters, arguments, state, apContext)) { thenUpdates.set(accessPath, NONNULL); } } private static Iterable accessPathsAtIndexes( - Set indexes, List arguments, AccessPath.AccessPathContext apContext) { + Set indexes, + List arguments, + VisitorState state, + AccessPath.AccessPathContext apContext) { List result = new ArrayList<>(); for (Integer i : indexes) { Preconditions.checkArgument(i >= 0 && i < arguments.size(), "Invalid argument index: " + i); if (i >= 0 && i < arguments.size()) { Node argument = arguments.get(i); - AccessPath ap = AccessPath.getAccessPathForNodeNoMapGet(argument, apContext); + AccessPath ap = AccessPath.getAccessPathForNodeWithMapGet(argument, state, apContext); if (ap != null) { result.add(ap); } @@ -269,12 +273,12 @@ private void setUnconditionalArgumentNullness( AccessPathNullnessPropagation.Updates bothUpdates, List arguments, Symbol.MethodSymbol callee, - Context context, + VisitorState state, AccessPath.AccessPathContext apContext) { Set requiredNonNullParameters = - getOptLibraryModels(context).failIfNullParameters(callee); + getOptLibraryModels(state.context).failIfNullParameters(callee); for (AccessPath accessPath : - accessPathsAtIndexes(requiredNonNullParameters, arguments, apContext)) { + accessPathsAtIndexes(requiredNonNullParameters, arguments, state, apContext)) { bothUpdates.set(accessPath, NONNULL); } } diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/OptionalEmptinessHandler.java b/nullaway/src/main/java/com/uber/nullaway/handlers/OptionalEmptinessHandler.java index 6ff320578c..5de9999249 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/OptionalEmptinessHandler.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/OptionalEmptinessHandler.java @@ -107,8 +107,7 @@ public void onMatchTopLevelClass( @Override public NullnessHint onDataflowVisitMethodInvocation( MethodInvocationNode node, - Types types, - Context context, + VisitorState state, AccessPath.AccessPathContext apContext, AccessPathNullnessPropagation.SubNodeValues inputs, AccessPathNullnessPropagation.Updates thenUpdates, @@ -116,15 +115,16 @@ public NullnessHint onDataflowVisitMethodInvocation( AccessPathNullnessPropagation.Updates bothUpdates) { Symbol.MethodSymbol symbol = ASTHelpers.getSymbol(node.getTree()); + Types types = state.getTypes(); if (optionalIsPresentCall(symbol, types)) { updateNonNullAPsForOptionalContent( - context, thenUpdates, node.getTarget().getReceiver(), apContext); + state.context, thenUpdates, node.getTarget().getReceiver(), apContext); } else if (optionalIsEmptyCall(symbol, types)) { updateNonNullAPsForOptionalContent( - context, elseUpdates, node.getTarget().getReceiver(), apContext); + state.context, elseUpdates, node.getTarget().getReceiver(), apContext); } else if (config.handleTestAssertionLibraries() && methodNameUtil.isMethodIsTrue(symbol)) { // we check for instance of AssertThat(optionalFoo.isPresent()).isTrue() - updateIfAssertIsPresentTrueOnOptional(context, node, types, apContext, bothUpdates); + updateIfAssertIsPresentTrueOnOptional(state.context, node, types, apContext, bothUpdates); } return NullnessHint.UNKNOWN; } 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 b5429e5346..f774626fc2 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/RestrictiveAnnotationHandler.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/RestrictiveAnnotationHandler.java @@ -132,15 +132,14 @@ public Nullness onOverrideMethodInvocationReturnNullability( @Override public NullnessHint onDataflowVisitMethodInvocation( MethodInvocationNode node, - Types types, - Context context, + VisitorState state, AccessPath.AccessPathContext apContext, AccessPathNullnessPropagation.SubNodeValues inputs, AccessPathNullnessPropagation.Updates thenUpdates, AccessPathNullnessPropagation.Updates elseUpdates, AccessPathNullnessPropagation.Updates bothUpdates) { Symbol.MethodSymbol methodSymbol = ASTHelpers.getSymbol(node.getTree()); - return isSymbolRestrictivelyNullable(methodSymbol, context) + return isSymbolRestrictivelyNullable(methodSymbol, state.context) ? NullnessHint.HINT_NULLABLE : NullnessHint.UNKNOWN; } diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/contract/ContractHandler.java b/nullaway/src/main/java/com/uber/nullaway/handlers/contract/ContractHandler.java index 386425b384..7947e908d8 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/contract/ContractHandler.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/contract/ContractHandler.java @@ -32,8 +32,6 @@ import com.sun.source.tree.ClassTree; import com.sun.source.tree.MethodInvocationTree; 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.ErrorMessage; import com.uber.nullaway.NullAway; @@ -88,7 +86,10 @@ public class ContractHandler extends BaseNoOpHandler { private final Config config; private @Nullable NullAway analysis; - private @Nullable VisitorState state; + + // Cached on visiting the top-level class and used for onCFGBuildPhase1AfterVisitMethodInvocation, + // where no VisitorState is otherwise available. + private @Nullable VisitorState storedVisitorState; private @Nullable TypeMirror runtimeExceptionType; @@ -100,7 +101,7 @@ public ContractHandler(Config config) { public void onMatchTopLevelClass( NullAway analysis, ClassTree tree, VisitorState state, Symbol.ClassSymbol classSymbol) { this.analysis = analysis; - this.state = state; + this.storedVisitorState = state; } @Override @@ -108,7 +109,7 @@ public MethodInvocationNode onCFGBuildPhase1AfterVisitMethodInvocation( NullAwayCFGBuilder.NullAwayCFGTranslationPhaseOne phase, MethodInvocationTree tree, MethodInvocationNode originalNode) { - Preconditions.checkNotNull(state); + Preconditions.checkNotNull(storedVisitorState); Preconditions.checkNotNull(analysis); Symbol.MethodSymbol callee = ASTHelpers.getSymbol(tree); Preconditions.checkNotNull(callee); @@ -116,11 +117,17 @@ public MethodInvocationNode onCFGBuildPhase1AfterVisitMethodInvocation( // This method currently handles contracts of the form `(true|false) -> fail`, other // contracts are handled by 'onDataflowVisitMethodInvocation' which has access to more // dataflow information. - if (!"fail".equals(getConsequent(clause, tree, analysis, state, callee))) { + if (!"fail".equals(getConsequent(clause, tree, analysis, storedVisitorState, callee))) { continue; } String[] antecedent = - getAntecedent(clause, tree, analysis, state, callee, originalNode.getArguments().size()); + getAntecedent( + clause, + tree, + analysis, + storedVisitorState, + callee, + originalNode.getArguments().size()); // Find a single value constraint that is not already known. If more than one argument with // unknown nullness affects the method's result, then ignore this clause. Node arg = null; @@ -169,14 +176,12 @@ public MethodInvocationNode onCFGBuildPhase1AfterVisitMethodInvocation( @Override public NullnessHint onDataflowVisitMethodInvocation( MethodInvocationNode node, - Types types, - Context context, + VisitorState state, AccessPath.AccessPathContext apContext, AccessPathNullnessPropagation.SubNodeValues inputs, AccessPathNullnessPropagation.Updates thenUpdates, AccessPathNullnessPropagation.Updates elseUpdates, AccessPathNullnessPropagation.Updates bothUpdates) { - Preconditions.checkNotNull(state); Preconditions.checkNotNull(analysis); Symbol.MethodSymbol callee = ASTHelpers.getSymbol(node.getTree()); Preconditions.checkNotNull(callee); diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/contract/fieldcontract/EnsuresNonNullHandler.java b/nullaway/src/main/java/com/uber/nullaway/handlers/contract/fieldcontract/EnsuresNonNullHandler.java index 122bde79c2..8f238a4c72 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/contract/fieldcontract/EnsuresNonNullHandler.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/contract/fieldcontract/EnsuresNonNullHandler.java @@ -31,8 +31,6 @@ import com.sun.source.tree.MethodTree; import com.sun.source.util.TreePath; 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.ErrorMessage; import com.uber.nullaway.NullAway; import com.uber.nullaway.Nullness; @@ -176,8 +174,7 @@ protected void validateOverridingRules( @Override public NullnessHint onDataflowVisitMethodInvocation( MethodInvocationNode node, - Types types, - Context context, + VisitorState state, AccessPath.AccessPathContext apContext, AccessPathNullnessPropagation.SubNodeValues inputs, AccessPathNullnessPropagation.Updates thenUpdates, @@ -187,7 +184,7 @@ public NullnessHint onDataflowVisitMethodInvocation( // A synthetic node might be inserted by the Checker Framework during CFG construction, it is // safer to do a null check here. return super.onDataflowVisitMethodInvocation( - node, types, context, apContext, inputs, thenUpdates, elseUpdates, bothUpdates); + node, state, apContext, inputs, thenUpdates, elseUpdates, bothUpdates); } Symbol.MethodSymbol methodSymbol = ASTHelpers.getSymbol(node.getTree()); Preconditions.checkNotNull(methodSymbol); @@ -211,6 +208,6 @@ public NullnessHint onDataflowVisitMethodInvocation( } } return super.onDataflowVisitMethodInvocation( - node, types, context, apContext, inputs, thenUpdates, elseUpdates, bothUpdates); + node, state, apContext, inputs, thenUpdates, elseUpdates, bothUpdates); } } diff --git a/nullaway/src/test/java/com/uber/nullaway/NullAwayCoreTests.java b/nullaway/src/test/java/com/uber/nullaway/NullAwayCoreTests.java index 2ee27f6437..b6edbcceb6 100644 --- a/nullaway/src/test/java/com/uber/nullaway/NullAwayCoreTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/NullAwayCoreTests.java @@ -769,4 +769,72 @@ public void primitiveCasts() { "}") .doTest(); } + + @Test + public void primitiveCastsRememberNullChecks() { + defaultCompilationHelper + .addSourceLines( + "Test.java", + "package com.uber;", + "import java.util.Map;", + "import javax.annotation.Nullable;", + "import com.google.common.base.Preconditions;", + "class Test {", + " static void foo(int i) { }", + " static void m1(@Nullable Integer i) {", + " // BUG: Diagnostic contains: unboxing", + " int i1 = (int) i;", + " }", + " static void m2(@Nullable Integer i) {", + " if (i != null) {", + " // this is fine", + " int i2 = (int) i;", + " }", + " }", + " static void m3(@Nullable Integer i) {", + " // BUG: Diagnostic contains: unboxing", + " int i3 = (int) i;", + " }", + " static void m4(@Nullable Integer i) {", + " Preconditions.checkNotNull(i);", + " // this is fine", + " int i4 = (int) i;", + " }", + " static private void consumeInt(int i) { }", + " static void m5(@Nullable Integer i) {", + " // BUG: Diagnostic contains: unboxing", + " consumeInt((int) i);", + " }", + " static void m6(@Nullable Integer i) {", + " Preconditions.checkNotNull(i);", + " // this is fine", + " consumeInt((int) i);", + " }", + " static void m7(@Nullable Object o) {", + " // BUG: Diagnostic contains: unboxing", + " consumeInt((int) o);", + " }", + " static void m8(@Nullable Object o) {", + " Preconditions.checkNotNull(o);", + " // this is fine", + " consumeInt((int) o);", + " }", + " static void m9(Map m) {", + " // BUG: Diagnostic contains: unboxing", + " consumeInt((int) m.get(\"foo\"));", + " }", + " static void m10(Map m) {", + " if(m.get(\"bar\") != null) {", + " // this is fine", + " consumeInt((int) m.get(\"bar\"));", + " }", + " }", + " static void m11(Map m) {", + " Preconditions.checkNotNull(m.get(\"bar\"));", + " // this is fine", + " consumeInt((int) m.get(\"bar\"));", + " }", + "}") + .doTest(); + } } diff --git a/nullaway/src/test/java/com/uber/nullaway/NullAwayGuavaAssertionsTests.java b/nullaway/src/test/java/com/uber/nullaway/NullAwayGuavaAssertionsTests.java index 0fcb39a435..83ee0c94e5 100644 --- a/nullaway/src/test/java/com/uber/nullaway/NullAwayGuavaAssertionsTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/NullAwayGuavaAssertionsTests.java @@ -26,6 +26,42 @@ public void checkNotNullTest() { .doTest(); } + @Test + public void checkNotNullComplexAccessPathsTest() { + makeTestHelperWithArgs( + Arrays.asList( + "-d", + temporaryFolder.getRoot().getAbsolutePath(), + "-XepOpt:NullAway:AnnotatedPackages=com.uber")) + .addSourceLines( + "TestField.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "import com.google.common.base.Preconditions;", + "class TestField {", + " @Nullable private Object f = null;", + " private void foo(@Nullable TestField a) {", + " Preconditions.checkNotNull(a);", + " Preconditions.checkNotNull(a.f);", + " a.f.toString();", + " }", + "}") + .addSourceLines( + "TestMap.java", + "package com.uber;", + "import java.util.Map;", + "import javax.annotation.Nullable;", + "import com.google.common.base.Preconditions;", + "class TestMap {", + " private void foo(@Nullable Map m) {", + " Preconditions.checkNotNull(m);", + " Preconditions.checkNotNull(m.get(\"foo\"));", + " m.get(\"foo\").toString();", + " }", + "}") + .doTest(); + } + @Test public void verifyNotNullTest() { makeTestHelperWithArgs( From a2147f778259f151e9217f1b9424572df772da03 Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Wed, 2 Nov 2022 09:06:39 -0700 Subject: [PATCH 09/14] Proper checking of unboxing in binary trees (#684) Our previous checking of when unboxing may occur for an operand of a binary expression was incorrect. It both missed unboxing cases and also sometimes checked for unboxing where none would occur. --- .../main/java/com/uber/nullaway/NullAway.java | 37 +++++++--- .../com/uber/nullaway/NullAwayCoreTests.java | 67 +++++++++++++++++++ 2 files changed, 93 insertions(+), 11 deletions(-) diff --git a/nullaway/src/main/java/com/uber/nullaway/NullAway.java b/nullaway/src/main/java/com/uber/nullaway/NullAway.java index 50f8663ddc..edc7655100 100644 --- a/nullaway/src/main/java/com/uber/nullaway/NullAway.java +++ b/nullaway/src/main/java/com/uber/nullaway/NullAway.java @@ -1426,20 +1426,35 @@ public Description matchBinary(BinaryTree tree, VisitorState state) { if (!withinAnnotatedCode(state)) { return Description.NO_MATCH; } + // Perform unboxing checks on operands if needed + Type binaryExprType = ASTHelpers.getType(tree); + // If the type of the expression is not primitive, we do not need to do unboxing checks. This + // handles the case of `+` used for string concatenation + if (binaryExprType == null || !binaryExprType.isPrimitive()) { + return Description.NO_MATCH; + } + Tree.Kind kind = tree.getKind(); ExpressionTree leftOperand = tree.getLeftOperand(); ExpressionTree rightOperand = tree.getRightOperand(); - Type leftType = ASTHelpers.getType(leftOperand); - Type rightType = ASTHelpers.getType(rightOperand); - if (leftType == null || rightType == null) { - throw new RuntimeException(); - } - if (leftType.isPrimitive() && !rightType.isPrimitive()) { - return doUnboxingCheck(state, rightOperand); - } - if (rightType.isPrimitive() && !leftType.isPrimitive()) { - return doUnboxingCheck(state, leftOperand); + if (kind.equals(Tree.Kind.EQUAL_TO) || kind.equals(Tree.Kind.NOT_EQUAL_TO)) { + // here we need a check if one operand is of primitive type and the other is not, as that will + // cause unboxing of the non-primitive operand + Type leftType = ASTHelpers.getType(leftOperand); + Type rightType = ASTHelpers.getType(rightOperand); + if (leftType == null || rightType == null) { + return Description.NO_MATCH; + } + if (leftType.isPrimitive() && !rightType.isPrimitive()) { + return doUnboxingCheck(state, rightOperand); + } else if (rightType.isPrimitive() && !leftType.isPrimitive()) { + return doUnboxingCheck(state, leftOperand); + } else { + return Description.NO_MATCH; + } + } else { + // in all other cases, both operands should be checked + return doUnboxingCheck(state, leftOperand, rightOperand); } - return Description.NO_MATCH; } @Override diff --git a/nullaway/src/test/java/com/uber/nullaway/NullAwayCoreTests.java b/nullaway/src/test/java/com/uber/nullaway/NullAwayCoreTests.java index b6edbcceb6..62d9f44267 100644 --- a/nullaway/src/test/java/com/uber/nullaway/NullAwayCoreTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/NullAwayCoreTests.java @@ -770,6 +770,73 @@ public void primitiveCasts() { .doTest(); } + @Test + public void unboxingInBinaryTrees() { + defaultCompilationHelper + .addSourceLines( + "Test.java", + "package com.uber;", + "class Test {", + " static void m1() {", + " Integer i = null;", + " Integer j = null;", + " // BUG: Diagnostic contains: unboxing", + " int i2 = i + j;", + " }", + " static void m2() {", + " Integer i = null;", + " // this is fine", + " String s = i + \"hi\";", + " }", + " static void m3() {", + " Integer i = null;", + " Integer j = null;", + " // BUG: Diagnostic contains: unboxing", + " int i3 = i - j;", + " }", + " static void m4() {", + " Integer i = null;", + " Integer j = null;", + " // BUG: Diagnostic contains: unboxing", + " int i4 = i * j;", + " }", + " static void m5() {", + " Integer i = null;", + " // BUG: Diagnostic contains: unboxing", + " int i5 = i << 2;", + " }", + " static void m6() {", + " Integer i = null;", + " Integer j = null;", + " // BUG: Diagnostic contains: unboxing", + " boolean b1 = i <= j;", + " }", + " static void m7() {", + " Boolean x = null;", + " Boolean y = null;", + " // BUG: Diagnostic contains: unboxing", + " boolean b2 = x && y;", + " }", + " static void m8() {", + " Integer i = null;", + " Integer j = null;", + " // this is fine", + " boolean b = i == j;", + " }", + " static void m9() {", + " Integer i = null;", + " // BUG: Diagnostic contains: unboxing", + " boolean b = i != 0;", + " }", + " static void m10() {", + " Integer i = null;", + " // BUG: Diagnostic contains: unboxing", + " int j = 3 - i;", + " }", + "}") + .doTest(); + } + @Test public void primitiveCastsRememberNullChecks() { defaultCompilationHelper From a4e51004f27301a50e278b04d10ec9530be82aca Mon Sep 17 00:00:00 2001 From: Lazaro Clapp Date: Wed, 2 Nov 2022 12:40:25 -0400 Subject: [PATCH 10/14] Prepare for release 0.10.4. --- CHANGELOG.md | 8 ++++++++ README.md | 4 ++-- gradle.properties | 2 +- 3 files changed, 11 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4f982b08c2..f7cd60c3fe 100755 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,13 @@ Changelog ========= +Version 0.10.4 +-------------- +(Bug fix release) +* Fix LibraryModels recording of dataflow nullness for Map APs (#685) +* Proper checking of unboxing in binary trees (#684) +* Build / CI tooling for NullAway itself: + - Bump dependency versions in GitHub Actions config (#683) + Version 0.10.3 -------------- * Report an error when casting @Nullable expression to primitive type (#663) diff --git a/README.md b/README.md index af7ddcf282..97140bc895 100644 --- a/README.md +++ b/README.md @@ -23,7 +23,7 @@ plugins { } dependencies { - annotationProcessor "com.uber.nullaway:nullaway:0.10.3" + annotationProcessor "com.uber.nullaway:nullaway:0.10.4" // Optional, some source of nullability annotations. // Not required on Android if you use the support @@ -75,7 +75,7 @@ The configuration for an Android project is very similar to the Java case, with ```gradle dependencies { - annotationProcessor "com.uber.nullaway:nullaway:0.10.3" + annotationProcessor "com.uber.nullaway:nullaway:0.10.4" errorprone "com.google.errorprone:error_prone_core:2.4.0" errorproneJavac "com.google.errorprone:javac:9+181-r4173-1" } diff --git a/gradle.properties b/gradle.properties index a468eb4f67..dafa471724 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.4-SNAPSHOT +VERSION_NAME=0.10.4 POM_DESCRIPTION=A fast annotation-based null checker for Java From 01e2c29f06dbfd1a5b4a808df299f912aee499bf Mon Sep 17 00:00:00 2001 From: Lazaro Clapp Date: Wed, 2 Nov 2022 12:48:21 -0400 Subject: [PATCH 11/14] Prepare next development version. --- gradle.properties | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gradle.properties b/gradle.properties index dafa471724..3d1957dfe8 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.4 +VERSION_NAME=0.10.5-SNAPSHOT POM_DESCRIPTION=A fast annotation-based null checker for Java From 93d4e7916411d4d47e2993eea90acd5f9d8d4ed6 Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Fri, 4 Nov 2022 10:55:22 -0700 Subject: [PATCH 12/14] Report more unboxing errors in a single compilation (#686) Previously, if an expression contained multiple `@Nullable` sub-expressions that were being unboxed, we would only report an error for one of them in a single compilation. Now we report errors for all of them in a single compile. In a few other places we may report more errors in a single run as well, e.g., for `arr[i]`, if `arr` and `i` are both `@Nullable`, we will report two errors in one run. Note: this does _not_ change the overall set of unboxing errors that are reported on a piece code, assuming a user fixes errors as they are reported. It just makes it so that the compiler needs to be run fewer times (ideally once in most cases) to see all the errors. --- .../main/java/com/uber/nullaway/NullAway.java | 82 +++++++++---------- .../com/uber/nullaway/NullAwayCoreTests.java | 35 ++++++++ 2 files changed, 72 insertions(+), 45 deletions(-) diff --git a/nullaway/src/main/java/com/uber/nullaway/NullAway.java b/nullaway/src/main/java/com/uber/nullaway/NullAway.java index edc7655100..63aa470879 100644 --- a/nullaway/src/main/java/com/uber/nullaway/NullAway.java +++ b/nullaway/src/main/java/com/uber/nullaway/NullAway.java @@ -459,7 +459,7 @@ public Description matchAssignment(AssignmentTree tree, VisitorState state) { } Type lhsType = ASTHelpers.getType(tree.getVariable()); if (lhsType != null && lhsType.isPrimitive()) { - return doUnboxingCheck(state, tree.getExpression()); + doUnboxingCheck(state, tree.getExpression()); } Symbol assigned = ASTHelpers.getSymbol(tree.getVariable()); if (assigned == null || assigned.getKind() != ElementKind.FIELD) { @@ -494,7 +494,7 @@ public Description matchCompoundAssignment(CompoundAssignmentTree tree, VisitorS Type stringType = Suppliers.STRING_TYPE.get(state); if (lhsType != null && !state.getTypes().isSameType(lhsType, stringType)) { // both LHS and RHS could get unboxed - return doUnboxingCheck(state, tree.getVariable(), tree.getExpression()); + doUnboxingCheck(state, tree.getVariable(), tree.getExpression()); } return Description.NO_MATCH; } @@ -505,11 +505,9 @@ public Description matchArrayAccess(ArrayAccessTree tree, VisitorState state) { return Description.NO_MATCH; } Description description = matchDereference(tree.getExpression(), tree, state); - if (!description.equals(Description.NO_MATCH)) { - return description; - } // also check for unboxing of array index expression - return doUnboxingCheck(state, tree.getIndex()); + doUnboxingCheck(state, tree.getIndex()); + return description; } @Override @@ -658,7 +656,7 @@ public Description matchTypeCast(TypeCastTree tree, VisitorState state) { Type castExprType = ASTHelpers.getType(tree); if (castExprType != null && castExprType.isPrimitive()) { // casting to a primitive type performs unboxing - return doUnboxingCheck(state, tree.getExpression()); + doUnboxingCheck(state, tree.getExpression()); } return Description.NO_MATCH; } @@ -789,9 +787,10 @@ static Trees getTreesInstance(VisitorState state) { private Description checkReturnExpression( Tree tree, ExpressionTree retExpr, Symbol.MethodSymbol methodSymbol, VisitorState state) { Type returnType = methodSymbol.getReturnType(); - if (returnType.isPrimitiveOrVoid()) { + if (returnType.isPrimitive()) { // check for unboxing - return returnType.isPrimitive() ? doUnboxingCheck(state, retExpr) : Description.NO_MATCH; + doUnboxingCheck(state, retExpr); + return Description.NO_MATCH; } if (ASTHelpers.isSameType(returnType, Suppliers.JAVA_LANG_VOID_TYPE.get(state), state)) { // Temporarily treat a Void return type as if it were @Nullable Void. Change this once @@ -1320,7 +1319,7 @@ public Description matchVariable(VariableTree tree, VisitorState state) { } VarSymbol symbol = ASTHelpers.getSymbol(tree); if (symbol.type.isPrimitive() && tree.getInitializer() != null) { - return doUnboxingCheck(state, tree.getInitializer()); + doUnboxingCheck(state, tree.getInitializer()); } if (!symbol.getKind().equals(ElementKind.FIELD)) { return Description.NO_MATCH; @@ -1445,58 +1444,54 @@ public Description matchBinary(BinaryTree tree, VisitorState state) { return Description.NO_MATCH; } if (leftType.isPrimitive() && !rightType.isPrimitive()) { - return doUnboxingCheck(state, rightOperand); + doUnboxingCheck(state, rightOperand); } else if (rightType.isPrimitive() && !leftType.isPrimitive()) { - return doUnboxingCheck(state, leftOperand); - } else { - return Description.NO_MATCH; + doUnboxingCheck(state, leftOperand); } } else { // in all other cases, both operands should be checked - return doUnboxingCheck(state, leftOperand, rightOperand); + doUnboxingCheck(state, leftOperand, rightOperand); } + return Description.NO_MATCH; } @Override public Description matchUnary(UnaryTree tree, VisitorState state) { - if (!withinAnnotatedCode(state)) { - return Description.NO_MATCH; + if (withinAnnotatedCode(state)) { + doUnboxingCheck(state, tree.getExpression()); } - return doUnboxingCheck(state, tree.getExpression()); + return Description.NO_MATCH; } @Override public Description matchConditionalExpression( ConditionalExpressionTree tree, VisitorState state) { - if (!withinAnnotatedCode(state)) { - return Description.NO_MATCH; + if (withinAnnotatedCode(state)) { + doUnboxingCheck(state, tree.getCondition()); } - return doUnboxingCheck(state, tree.getCondition()); + return Description.NO_MATCH; } @Override public Description matchIf(IfTree tree, VisitorState state) { - if (!withinAnnotatedCode(state)) { - return Description.NO_MATCH; + if (withinAnnotatedCode(state)) { + doUnboxingCheck(state, tree.getCondition()); } - return doUnboxingCheck(state, tree.getCondition()); + return Description.NO_MATCH; } @Override public Description matchWhileLoop(WhileLoopTree tree, VisitorState state) { - if (!withinAnnotatedCode(state)) { - return Description.NO_MATCH; + if (withinAnnotatedCode(state)) { + doUnboxingCheck(state, tree.getCondition()); } - return doUnboxingCheck(state, tree.getCondition()); + return Description.NO_MATCH; } @Override public Description matchForLoop(ForLoopTree tree, VisitorState state) { - if (!withinAnnotatedCode(state)) { - return Description.NO_MATCH; - } - if (tree.getCondition() != null) { - return doUnboxingCheck(state, tree.getCondition()); + if (withinAnnotatedCode(state) && tree.getCondition() != null) { + doUnboxingCheck(state, tree.getCondition()); } return Description.NO_MATCH; } @@ -1518,13 +1513,14 @@ public Description matchEnhancedForLoop(EnhancedForLoopTree tree, VisitorState s } /** - * if any expression has non-primitive type, we should check that it can't be null as it is - * getting unboxed + * Checks that all given expressions cannot be null, and for those that are {@code @Nullable}, + * reports an unboxing error. * + * @param state the visitor state, used to report errors via {@link + * VisitorState#reportMatch(Description)} * @param expressions expressions to check - * @return error Description if an error is found, otherwise NO_MATCH */ - private Description doUnboxingCheck(VisitorState state, ExpressionTree... expressions) { + private void doUnboxingCheck(VisitorState state, ExpressionTree... expressions) { for (ExpressionTree tree : expressions) { Type type = ASTHelpers.getType(tree); if (type == null) { @@ -1534,12 +1530,12 @@ private Description doUnboxingCheck(VisitorState state, ExpressionTree... expres if (mayBeNullExpr(state, tree)) { final ErrorMessage errorMessage = new ErrorMessage(MessageTypes.UNBOX_NULLABLE, "unboxing of a @Nullable value"); - return errorBuilder.createErrorDescription( - errorMessage, buildDescription(tree), state, null); + state.reportMatch( + errorBuilder.createErrorDescription( + errorMessage, buildDescription(tree), state, null)); } } } - return Description.NO_MATCH; } /** @@ -1578,12 +1574,8 @@ private Description handleInvocation( for (int i = 0; i < formalParams.size(); i++) { VarSymbol param = formalParams.get(i); if (param.type.isPrimitive()) { - Description unboxingCheck = doUnboxingCheck(state, actualParams.get(i)); - if (unboxingCheck != Description.NO_MATCH) { - return unboxingCheck; - } else { - argumentPositionNullness[i] = Nullness.NONNULL; - } + doUnboxingCheck(state, actualParams.get(i)); + argumentPositionNullness[i] = Nullness.NONNULL; } else if (ASTHelpers.isSameType( param.type, Suppliers.JAVA_LANG_VOID_TYPE.get(state), state)) { // Temporarily treat a Void argument type as if it were @Nullable Void. Handling of Void diff --git a/nullaway/src/test/java/com/uber/nullaway/NullAwayCoreTests.java b/nullaway/src/test/java/com/uber/nullaway/NullAwayCoreTests.java index 62d9f44267..9bd436e1f7 100644 --- a/nullaway/src/test/java/com/uber/nullaway/NullAwayCoreTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/NullAwayCoreTests.java @@ -833,6 +833,41 @@ public void unboxingInBinaryTrees() { " // BUG: Diagnostic contains: unboxing", " int j = 3 - i;", " }", + " static void m11() {", + " Integer i = null;", + " Integer j = null;", + " // BUG: Diagnostic contains: unboxing", + " int i2 = i", + " +", + " // BUG: Diagnostic contains: unboxing", + " j;", + " }", + " static void m12() {", + " Integer i = null;", + " // BUG: Diagnostic contains: unboxing", + " int i2 = i", + " +", + " // no error here, due to previous unbox of i", + " i;", + " }", + " static void m13() {", + " int[] arr = null;", + " Integer i = null;", + " // BUG: Diagnostic contains: dereferenced", + " int i2 = arr[", + " // BUG: Diagnostic contains: unboxing", + " i];", + " }", + " static void primitiveArgs(int x, int y) {}", + " static void m14() {", + " Integer i = null;", + " Integer j = null;", + " primitiveArgs(", + " // BUG: Diagnostic contains: unboxing", + " i,", + " // BUG: Diagnostic contains: unboxing", + " j);", + " }", "}") .doTest(); } From 19bbb915357a323dbb0b449449520f360b237221 Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Fri, 4 Nov 2022 11:06:08 -0700 Subject: [PATCH 13/14] Remove `AccessPath.getAccessPathForNodeNoMapGet` (#687) This should prevent the need for fixes like #685 in the future, and make our results more uniform and easier to understand. We now have a `VisitorState` object already threaded through everywhere we need it, so the previous API is no longer required. --- .../uber/nullaway/dataflow/AccessPath.java | 46 +++++++------------ .../AccessPathNullnessPropagation.java | 16 +++---- .../nullaway/handlers/AssertionHandler.java | 4 +- .../handlers/LibraryModelsHandler.java | 2 +- .../handlers/contract/ContractHandler.java | 2 +- .../nullaway/NullAwayAssertionLibsTests.java | 23 ++++++++++ .../uber/nullaway/NullAwayContractsTests.java | 4 ++ .../com/uber/nullaway/NullAwayCoreTests.java | 21 +++++++++ 8 files changed, 76 insertions(+), 42 deletions(-) diff --git a/nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPath.java b/nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPath.java index 51c7219c68..f324a0befb 100644 --- a/nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPath.java +++ b/nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPath.java @@ -171,9 +171,9 @@ private static AccessPath fromNodeAndContext(Node node, AccessPathContext apCont */ @Nullable static AccessPath fromMethodCall( - MethodInvocationNode node, @Nullable VisitorState state, AccessPathContext apContext) { - if (state != null && isMapGet(ASTHelpers.getSymbol(node.getTree()), state)) { - return fromMapGetCall(node, apContext); + MethodInvocationNode node, VisitorState state, AccessPathContext apContext) { + if (isMapGet(ASTHelpers.getSymbol(node.getTree()), state)) { + return fromMapGetCall(node, state, apContext); } return fromVanillaMethodCall(node, apContext); } @@ -251,14 +251,12 @@ public static AccessPath fromBaseMethodAndConstantArgs( */ @Nullable public static AccessPath getForMapInvocation( - MethodInvocationNode node, AccessPathContext apContext) { + MethodInvocationNode node, VisitorState state, AccessPathContext apContext) { // For the receiver type for get, use the declared type of the receiver of the containsKey() - // call. - // Note that this may differ from the containing class of the resolved containsKey() method, - // which - // can be in a superclass (e.g., LinkedHashMap does not override containsKey()) + // call. Note that this may differ from the containing class of the resolved containsKey() + // method, which can be in a superclass (e.g., LinkedHashMap does not override containsKey()) // assumption: map type will not both override containsKey() and inherit get() - return fromMapGetCall(node, apContext); + return fromMapGetCall(node, state, apContext); } private static Node stripCasts(Node node) { @@ -269,7 +267,8 @@ private static Node stripCasts(Node node) { } @Nullable - private static MapKey argumentToMapKeySpecifier(Node argument, AccessPathContext apContext) { + private static MapKey argumentToMapKeySpecifier( + Node argument, VisitorState state, AccessPathContext apContext) { // Required to have Node type match Tree type in some instances. if (argument instanceof WideningConversionNode) { argument = ((WideningConversionNode) argument).getOperand(); @@ -291,19 +290,20 @@ private static MapKey argumentToMapKeySpecifier(Node argument, AccessPathContext && arguments.size() == 1 && castToNonNull(receiver.getTree()).getKind().equals(Tree.Kind.IDENTIFIER) && (receiver.toString().equals("Integer") || receiver.toString().equals("Long"))) { - return argumentToMapKeySpecifier(arguments.get(0), apContext); + return argumentToMapKeySpecifier(arguments.get(0), state, apContext); } // Fine to fallthrough: default: // Every other type of expression, including variables, field accesses, new A(...), etc. - return getAccessPathForNodeNoMapGet(argument, apContext); // Every AP is a MapKey too + return getAccessPathForNode(argument, state, apContext); // Every AP is a MapKey too } } @Nullable - private static AccessPath fromMapGetCall(MethodInvocationNode node, AccessPathContext apContext) { + private static AccessPath fromMapGetCall( + MethodInvocationNode node, VisitorState state, AccessPathContext apContext) { Node argument = node.getArgument(0); - MapKey mapKey = argumentToMapKeySpecifier(argument, apContext); + MapKey mapKey = argumentToMapKeySpecifier(argument, state, apContext); if (mapKey == null) { return null; } @@ -312,20 +312,6 @@ private static AccessPath fromMapGetCall(MethodInvocationNode node, AccessPathCo return buildAccessPathRecursive(receiver, new ArrayDeque<>(), apContext, mapKey); } - /** - * Gets corresponding AccessPath for node, if it exists. Does not handle calls to - * Map.get() - * - * @param node AST node - * @param apContext the current access path context information (see {@link - * AccessPath.AccessPathContext}). - * @return corresponding AccessPath if it exists; null otherwise - */ - @Nullable - public static AccessPath getAccessPathForNodeNoMapGet(Node node, AccessPathContext apContext) { - return getAccessPathForNodeWithMapGet(node, null, apContext); - } - /** * Gets corresponding AccessPath for node, if it exists. Handles calls to Map.get() * @@ -337,8 +323,8 @@ public static AccessPath getAccessPathForNodeNoMapGet(Node node, AccessPathConte * @return corresponding AccessPath if it exists; null otherwise */ @Nullable - public static AccessPath getAccessPathForNodeWithMapGet( - Node node, @Nullable VisitorState state, AccessPathContext apContext) { + public static AccessPath getAccessPathForNode( + Node node, VisitorState state, AccessPathContext apContext) { if (node instanceof LocalVariableNode) { return fromLocal((LocalVariableNode) node); } else if (node instanceof FieldAccessNode) { 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 65bd0e5538..918fdae3e6 100644 --- a/nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPathNullnessPropagation.java +++ b/nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPathNullnessPropagation.java @@ -456,14 +456,14 @@ private void handleEqualityComparison( Node realLeftNode = unwrapAssignExpr(leftNode); Node realRightNode = unwrapAssignExpr(rightNode); - AccessPath leftAP = AccessPath.getAccessPathForNodeWithMapGet(realLeftNode, state, apContext); + AccessPath leftAP = AccessPath.getAccessPathForNode(realLeftNode, state, apContext); if (leftAP != null) { equalBranchUpdates.set(leftAP, equalBranchValue); notEqualBranchUpdates.set( leftAP, leftVal.greatestLowerBound(rightVal.deducedValueWhenNotEqual())); } - AccessPath rightAP = AccessPath.getAccessPathForNodeWithMapGet(realRightNode, state, apContext); + AccessPath rightAP = AccessPath.getAccessPathForNode(realRightNode, state, apContext); if (rightAP != null) { equalBranchUpdates.set(rightAP, equalBranchValue); notEqualBranchUpdates.set( @@ -665,7 +665,7 @@ private TransferResult updateRegularStore( * the updates */ private void setNonnullIfAnalyzeable(Updates updates, Node node) { - AccessPath ap = AccessPath.getAccessPathForNodeWithMapGet(node, state, apContext); + AccessPath ap = AccessPath.getAccessPathForNode(node, state, apContext); if (ap != null) { updates.set(ap, NONNULL); } @@ -885,8 +885,8 @@ public TransferResult visitAssertionError( } AccessPath accessPath = - AccessPath.getAccessPathForNodeNoMapGet( - ((NotEqualNode) condition).getLeftOperand(), apContext); + AccessPath.getAccessPathForNode( + ((NotEqualNode) condition).getLeftOperand(), state, apContext); if (accessPath == null) { return noStoreChanges(NULLABLE, input); @@ -943,7 +943,7 @@ private void setNullnessForMapCalls( AccessPathNullnessPropagation.Updates bothUpdates) { if (AccessPath.isContainsKey(callee, state)) { // make sure argument is a variable, and get its element - AccessPath getAccessPath = AccessPath.getForMapInvocation(node, apContext); + AccessPath getAccessPath = AccessPath.getForMapInvocation(node, state, apContext); if (getAccessPath != null) { // in the then branch, we want the get() call with the same argument to be non-null // we assume that the declared target of the get() method will be in the same class @@ -951,13 +951,13 @@ private void setNullnessForMapCalls( thenUpdates.set(getAccessPath, NONNULL); } } else if (AccessPath.isMapPut(callee, state)) { - AccessPath getAccessPath = AccessPath.getForMapInvocation(node, apContext); + AccessPath getAccessPath = AccessPath.getForMapInvocation(node, state, apContext); if (getAccessPath != null) { Nullness value = inputs.valueOfSubNode(arguments.get(1)); bothUpdates.set(getAccessPath, value); } } else if (AccessPath.isMapComputeIfAbsent(callee, state)) { - AccessPath getAccessPath = AccessPath.getForMapInvocation(node, apContext); + AccessPath getAccessPath = AccessPath.getForMapInvocation(node, state, apContext); if (getAccessPath != null) { // TODO: For now, Function implies a @NonNull V. We need to revisit this once we // support generics, but we do include a couple defensive tests below. 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 ddc9572854..6f8d48ea00 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/AssertionHandler.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/AssertionHandler.java @@ -69,7 +69,7 @@ public NullnessHint onDataflowVisitMethodInvocation( Symbol.MethodSymbol receiver_symbol = ASTHelpers.getSymbol(receiver_method.getTree()); if (methodNameUtil.isMethodAssertThat(receiver_symbol)) { Node arg = receiver_method.getArgument(0); - AccessPath ap = AccessPath.getAccessPathForNodeNoMapGet(arg, apContext); + AccessPath ap = AccessPath.getAccessPathForNode(arg, state, apContext); if (ap != null) { bothUpdates.set(ap, NONNULL); } @@ -84,7 +84,7 @@ public NullnessHint onDataflowVisitMethodInvocation( || methodNameUtil.isMethodJunitAssertThat(callee)) { List args = node.getArguments(); if (args.size() == 2 && methodNameUtil.isMatcherIsNotNull(args.get(1))) { - AccessPath ap = AccessPath.getAccessPathForNodeNoMapGet(args.get(0), apContext); + AccessPath ap = AccessPath.getAccessPathForNode(args.get(0), state, apContext); if (ap != null) { bothUpdates.set(ap, NONNULL); } 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 4f1f251328..3f12a03475 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java @@ -253,7 +253,7 @@ private static Iterable accessPathsAtIndexes( Preconditions.checkArgument(i >= 0 && i < arguments.size(), "Invalid argument index: " + i); if (i >= 0 && i < arguments.size()) { Node argument = arguments.get(i); - AccessPath ap = AccessPath.getAccessPathForNodeWithMapGet(argument, state, apContext); + AccessPath ap = AccessPath.getAccessPathForNode(argument, state, apContext); if (ap != null) { result.add(ap); } diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/contract/ContractHandler.java b/nullaway/src/main/java/com/uber/nullaway/handlers/contract/ContractHandler.java index 7947e908d8..cb3a06d611 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/contract/ContractHandler.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/contract/ContractHandler.java @@ -306,7 +306,7 @@ public NullnessHint onDataflowVisitMethodInvocation( argAntecedentNullness != null, "argAntecedentNullness should have been set"); // The nullness of one argument is all that matters for the antecedent, let's negate the // consequent to fix the nullness of this argument. - AccessPath accessPath = AccessPath.getAccessPathForNodeNoMapGet(arg, apContext); + AccessPath accessPath = AccessPath.getAccessPathForNode(arg, state, apContext); if (accessPath == null) { continue; } diff --git a/nullaway/src/test/java/com/uber/nullaway/NullAwayAssertionLibsTests.java b/nullaway/src/test/java/com/uber/nullaway/NullAwayAssertionLibsTests.java index f30afc974a..5b61ce7740 100644 --- a/nullaway/src/test/java/com/uber/nullaway/NullAwayAssertionLibsTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/NullAwayAssertionLibsTests.java @@ -52,6 +52,29 @@ public void supportTruthAssertThatIsNotNull_String() { .doTest(); } + @Test + public void supportTruthAssertThatIsNotNull_MapKey() { + makeTestHelperWithArgs( + Arrays.asList( + "-d", + temporaryFolder.getRoot().getAbsolutePath(), + "-XepOpt:NullAway:AnnotatedPackages=com.uber", + "-XepOpt:NullAway:HandleTestAssertionLibraries=true")) + .addSourceLines( + "Test.java", + "package com.uber;", + "import java.util.Map;", + "import javax.annotation.Nullable;", + "import static com.google.common.truth.Truth.assertThat;", + "class Test {", + " private void foo(Map m) {", + " assertThat(m.get(\"foo\")).isNotNull();", + " m.get(\"foo\").toString();", + " }", + "}") + .doTest(); + } + @Test public void doNotSupportTruthAssertThatWhenDisabled() { makeTestHelperWithArgs( diff --git a/nullaway/src/test/java/com/uber/nullaway/NullAwayContractsTests.java b/nullaway/src/test/java/com/uber/nullaway/NullAwayContractsTests.java index 235cee8045..d3849b041d 100644 --- a/nullaway/src/test/java/com/uber/nullaway/NullAwayContractsTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/NullAwayContractsTests.java @@ -64,6 +64,10 @@ public void basicContractAnnotation() { " NullnessChecker.assertNonNull(o);", " return o.toString();", " }", + " String test4(java.util.Map m) {", + " NullnessChecker.assertNonNull(m.get(\"foo\"));", + " return m.get(\"foo\").toString();", + " }", "}") .doTest(); } diff --git a/nullaway/src/test/java/com/uber/nullaway/NullAwayCoreTests.java b/nullaway/src/test/java/com/uber/nullaway/NullAwayCoreTests.java index 9bd436e1f7..8ef7a7fa22 100644 --- a/nullaway/src/test/java/com/uber/nullaway/NullAwayCoreTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/NullAwayCoreTests.java @@ -589,6 +589,27 @@ public void testMapComputeIfAbsent() { .doTest(); } + @Test + public void testMapWithMapGetKey() { + defaultCompilationHelper + .addSourceLines( + "Test.java", + "package com.uber;", + "import java.util.Map;", + "import java.util.function.Function;", + "class Test {", + " String testMapWithMapGetKey(Map m1, Map m2) {", + " if (m1.containsKey(\"s1\")) {", + " if (m2.containsKey(m1.get(\"s1\"))) {", + " return m2.get(m1.get(\"s1\")).toString();", + " }", + " }", + " return \"no\";", + " }", + "}") + .doTest(); + } + @Test public void tryFinallySupport() { defaultCompilationHelper.addSourceFile("NullAwayTryFinallyCases.java").doTest(); From eb62d5711b620f2885bbf5d98c98ab8608330958 Mon Sep 17 00:00:00 2001 From: Nima Karimipour Date: Tue, 8 Nov 2022 12:12:13 -0800 Subject: [PATCH 14/14] Fix Serialization: Split field initialization region into smaller regions (#658) This PR resolves #657 by splitting the field initialization region into smaller regions. We currently only consider the value "null", for all `errors/fixes` reported in this regions. In large classes which they have many number of field, this can easily create a lot builds as they all have the value "null" for enclosing method causing conflicts in regions. This PR generalizes the concept of enclosing method to enclosing member, and breaks a single regions of field initialization regions into a separate region for each field declaration and reduce the conflicts in regions dramatically. The equivalent for enclosing member is described: The enclosing member for `fix` or `error` is: - If it is triggered inside a __method__, the enclosing member will be the __enclosing method__. - If it is enclosed by a __field declaration statement__, the enclosing member will be the __declared field__. - If __none__ of the above, the enclosing member will be `"null"` (used for __static initialization region__). With the current approach the following errors will have the values below: ```java class C { Field foo1 = Field(null); // Passing nullable, enclMethod = "null" Field foo2 = null; // assigning nullable, enclMethod = "null" static Field foo3 3 = null; // assigning nullable, enclMethod = "null" { foo3 = null; // assigning nullable, enclMethod = "null" } void bar(){ this.foo1 = null; // assigning nullable, enclMethod = "bar()" } } ``` From the point of view of Annotator, that is __4__ conflicts in regions as they all have `enclMethod = "null"`. This can be greatly improved with the new approach suggested and changed to below: ```java class C { Field foo1 = Field(null); // Passing nullable, enclMember = "foo1" Field foo2 = null; // assigning nullable, enclMember = "foo2" static Field foo3 = null; // assigning nullable, enclMember = "foo3" { Field3 = null; // assigning nullable, enclMember = "null" } void bar(){ this.foo1 = null; // assigning nullable, enclMember = "bar()" } } ``` None of the error reported above have any conflicts and can be allocated to separate regions. --- ...ethodInfo.java => ClassAndMemberInfo.java} | 80 +++++++++------ .../fixserialization/out/ErrorInfo.java | 18 ++-- .../out/SuggestedNullableFixInfo.java | 16 +-- .../nullaway/NullAwaySerializationTest.java | 99 +++++++++++++++++++ .../com/uber/nullaway/tools/ErrorDisplay.java | 20 ++-- 5 files changed, 177 insertions(+), 56 deletions(-) rename nullaway/src/main/java/com/uber/nullaway/fixserialization/out/{ClassAndMethodInfo.java => ClassAndMemberInfo.java} (50%) diff --git a/nullaway/src/main/java/com/uber/nullaway/fixserialization/out/ClassAndMethodInfo.java b/nullaway/src/main/java/com/uber/nullaway/fixserialization/out/ClassAndMemberInfo.java similarity index 50% rename from nullaway/src/main/java/com/uber/nullaway/fixserialization/out/ClassAndMethodInfo.java rename to nullaway/src/main/java/com/uber/nullaway/fixserialization/out/ClassAndMemberInfo.java index 2f671db65c..ebbd407c5d 100644 --- a/nullaway/src/main/java/com/uber/nullaway/fixserialization/out/ClassAndMethodInfo.java +++ b/nullaway/src/main/java/com/uber/nullaway/fixserialization/out/ClassAndMemberInfo.java @@ -25,32 +25,32 @@ import com.google.errorprone.util.ASTHelpers; import com.sun.source.tree.ClassTree; import com.sun.source.tree.MethodTree; +import com.sun.source.tree.VariableTree; import com.sun.source.util.TreePath; import com.sun.tools.javac.code.Symbol; import javax.annotation.Nullable; -/** Class and method corresponding to a program point at which an error was reported. */ -public class ClassAndMethodInfo { - /** Path to the program point of the reported error */ +/** Class and member corresponding to a program point at which an error / fix was reported. */ +public class ClassAndMemberInfo { + /** Path to the program point of the reported error / fix */ public final TreePath path; - /** - * Finding values for these properties is costly and are not needed by default, hence, they are - * not {@code final} and are only initialized at request. - */ - @Nullable private MethodTree method; + // Finding values for these properties is costly and are not needed by default, hence, they are + // not final and are only initialized at request. + @Nullable private Symbol member; @Nullable private ClassTree clazz; - public ClassAndMethodInfo(TreePath path) { + public ClassAndMemberInfo(TreePath path) { this.path = path; } - /** Finds the class and method where the error is reported according to {@code path}. */ + /** Finds the class and member where the error / fix is reported according to {@code path}. */ public void findValues() { + MethodTree enclosingMethod; // If the error is reported on a method, that method itself is the relevant program point. // Otherwise, use the enclosing method (if present). - method = + enclosingMethod = path.getLeaf() instanceof MethodTree ? (MethodTree) path.getLeaf() : ASTHelpers.findEnclosingNode(path, MethodTree.class); @@ -60,30 +60,52 @@ public void findValues() { path.getLeaf() instanceof ClassTree ? (ClassTree) path.getLeaf() : ASTHelpers.findEnclosingNode(path, ClassTree.class); - if (clazz != null && method != null) { - // It is possible that the computed method is not enclosed by the computed class, e.g., for - // the following case: - // class C { - // void foo() { - // class Local { - // Object f = null; // error - // } - // } - // } - // Here the above code will compute clazz to be Local and method as foo(). In such cases, set - // method to null, we always want the corresponding method to be nested in the corresponding - // class. + if (clazz != null) { Symbol.ClassSymbol classSymbol = ASTHelpers.getSymbol(clazz); - Symbol.MethodSymbol methodSymbol = ASTHelpers.getSymbol(method); - if (!methodSymbol.isEnclosedBy(classSymbol)) { - method = null; + if (enclosingMethod != null) { + // It is possible that the computed method is not enclosed by the computed class, e.g., for + // the following case: + // class C { + // void foo() { + // class Local { + // Object f = null; // error + // } + // } + // } + // Here the above code will compute clazz to be Local and method as foo(). In such cases, + // set method to null, we always want the corresponding method to be nested in the + // corresponding class. + Symbol.MethodSymbol methodSymbol = ASTHelpers.getSymbol(enclosingMethod); + if (!methodSymbol.isEnclosedBy(classSymbol)) { + enclosingMethod = null; + } + } + if (enclosingMethod != null) { + member = ASTHelpers.getSymbol(enclosingMethod); + } else { + // Node is not enclosed by any method, can be a field declaration or enclosed by it. + Symbol sym = ASTHelpers.getSymbol(path.getLeaf()); + Symbol.VarSymbol fieldSymbol = null; + if (sym != null && sym.getKind().isField()) { + // Directly on a field declaration. + fieldSymbol = (Symbol.VarSymbol) sym; + } else { + // Can be enclosed by a field declaration tree. + VariableTree fieldDeclTree = ASTHelpers.findEnclosingNode(path, VariableTree.class); + if (fieldDeclTree != null) { + fieldSymbol = ASTHelpers.getSymbol(fieldDeclTree); + } + } + if (fieldSymbol != null && fieldSymbol.isEnclosedBy(classSymbol)) { + member = fieldSymbol; + } } } } @Nullable - public MethodTree getMethod() { - return method; + public Symbol getMember() { + return member; } @Nullable diff --git a/nullaway/src/main/java/com/uber/nullaway/fixserialization/out/ErrorInfo.java b/nullaway/src/main/java/com/uber/nullaway/fixserialization/out/ErrorInfo.java index 07b00e38c4..7127b82834 100644 --- a/nullaway/src/main/java/com/uber/nullaway/fixserialization/out/ErrorInfo.java +++ b/nullaway/src/main/java/com/uber/nullaway/fixserialization/out/ErrorInfo.java @@ -36,7 +36,7 @@ public class ErrorInfo { private final ErrorMessage errorMessage; - private final ClassAndMethodInfo classAndMethodInfo; + private final ClassAndMemberInfo classAndMemberInfo; /** * if non-null, this error involved a pseudo-assignment of a @Nullable expression into a @NonNull @@ -60,7 +60,7 @@ public class ErrorInfo { '\r', 'r'); public ErrorInfo(TreePath path, ErrorMessage errorMessage, @Nullable Symbol nonnullTarget) { - this.classAndMethodInfo = new ClassAndMethodInfo(path); + this.classAndMemberInfo = new ClassAndMemberInfo(path); this.errorMessage = errorMessage; this.nonnullTarget = nonnullTarget; } @@ -97,20 +97,20 @@ public String tabSeparatedToString() { "\t", errorMessage.getMessageType().toString(), escapeSpecialCharacters(errorMessage.getMessage()), - (classAndMethodInfo.getClazz() != null - ? ASTHelpers.getSymbol(classAndMethodInfo.getClazz()).flatName() + (classAndMemberInfo.getClazz() != null + ? ASTHelpers.getSymbol(classAndMemberInfo.getClazz()).flatName() : "null"), - (classAndMethodInfo.getMethod() != null - ? ASTHelpers.getSymbol(classAndMethodInfo.getMethod()).toString() + (classAndMemberInfo.getMember() != null + ? classAndMemberInfo.getMember().toString() : "null"), (nonnullTarget != null ? SymbolLocation.createLocationFromSymbol(nonnullTarget).tabSeparatedToString() : EMPTY_NONNULL_TARGET_LOCATION_STRING)); } - /** Finds the class and method of program point where the error is reported. */ + /** Finds the class and member of program point where the error is reported. */ public void initEnclosing() { - classAndMethodInfo.findValues(); + classAndMemberInfo.findValues(); } /** @@ -125,7 +125,7 @@ public static String header() { "message_type", "message", "enc_class", - "enc_method", + "enc_member", "target_kind", "target_class", "target_method", diff --git a/nullaway/src/main/java/com/uber/nullaway/fixserialization/out/SuggestedNullableFixInfo.java b/nullaway/src/main/java/com/uber/nullaway/fixserialization/out/SuggestedNullableFixInfo.java index c00a0a751d..8232ced29b 100644 --- a/nullaway/src/main/java/com/uber/nullaway/fixserialization/out/SuggestedNullableFixInfo.java +++ b/nullaway/src/main/java/com/uber/nullaway/fixserialization/out/SuggestedNullableFixInfo.java @@ -36,13 +36,13 @@ public class SuggestedNullableFixInfo { /** Error which will be resolved by this type change. */ private final ErrorMessage errorMessage; - private final ClassAndMethodInfo classAndMethodInfo; + private final ClassAndMemberInfo classAndMemberInfo; public SuggestedNullableFixInfo( TreePath path, SymbolLocation symbolLocation, ErrorMessage errorMessage) { - this.classAndMethodInfo = new ClassAndMethodInfo(path); this.symbolLocation = symbolLocation; this.errorMessage = errorMessage; + this.classAndMemberInfo = new ClassAndMemberInfo(path); } @Override @@ -76,17 +76,17 @@ public String tabSeparatedToString() { symbolLocation.tabSeparatedToString(), errorMessage.getMessageType().toString(), "nullable", - (classAndMethodInfo.getClazz() == null + (classAndMemberInfo.getClazz() == null ? "null" - : ASTHelpers.getSymbol(classAndMethodInfo.getClazz()).flatName()), - (classAndMethodInfo.getMethod() == null + : ASTHelpers.getSymbol(classAndMemberInfo.getClazz()).flatName()), + (classAndMemberInfo.getMember() == null ? "null" - : ASTHelpers.getSymbol(classAndMethodInfo.getMethod()).toString())); + : classAndMemberInfo.getMember().toString())); } - /** Finds the class and method of program point where triggered this type change. */ + /** Finds the class and member of program point where triggered this type change. */ public void initEnclosing() { - classAndMethodInfo.findValues(); + classAndMemberInfo.findValues(); } /** diff --git a/nullaway/src/test/java/com/uber/nullaway/NullAwaySerializationTest.java b/nullaway/src/test/java/com/uber/nullaway/NullAwaySerializationTest.java index 019726c100..2e49f878d9 100644 --- a/nullaway/src/test/java/com/uber/nullaway/NullAwaySerializationTest.java +++ b/nullaway/src/test/java/com/uber/nullaway/NullAwaySerializationTest.java @@ -1369,6 +1369,105 @@ public void errorSerializationTestLocalClassField() { .doTest(); } + @Test + public void errorSerializationTestEnclosedByFieldDeclaration() { + SerializationTestHelper tester = new SerializationTestHelper<>(root); + tester + .setArgs( + Arrays.asList( + "-d", + temporaryFolder.getRoot().getAbsolutePath(), + "-XepOpt:NullAway:AnnotatedPackages=com.uber", + "-XepOpt:NullAway:SerializeFixMetadata=true", + "-XepOpt:NullAway:FixSerializationConfigPath=" + configPath)) + .addSourceLines( + "com/uber/Main.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "public class Main {", + " // BUG: Diagnostic contains: passing @Nullable parameter", + " Foo f = new Foo(null);", // Member should be "f" + " // BUG: Diagnostic contains: assigning @Nullable expression", + " Foo f1 = null;", // Member should be "f1" + " // BUG: Diagnostic contains: assigning @Nullable expression", + " static Foo f2 = null;", // Member should be "f2" + " static {", + " // BUG: Diagnostic contains: assigning @Nullable expression", + " f2 = null;", // Member should be "null" (not field or method) + " }", + " class Inner {", + " // BUG: Diagnostic contains: passing @Nullable parameter", + " Foo f = new Foo(null);", // Member should be "f" but class is Main$Inner + " }", + "}") + .addSourceLines( + "com/uber/Foo.java", + "package com.uber;", + "public class Foo {", + " public Foo(Object foo){", + " }", + "}") + .setExpectedOutputs( + new ErrorDisplay( + "PASS_NULLABLE", + "passing @Nullable parameter", + "com.uber.Main", + "f", + "PARAMETER", + "com.uber.Foo", + "Foo(java.lang.Object)", + "foo", + "0", + "com/uber/Foo.java"), + new ErrorDisplay( + "ASSIGN_FIELD_NULLABLE", + "assigning @Nullable expression to @NonNull field", + "com.uber.Main", + "f1", + "FIELD", + "com.uber.Main", + "null", + "f1", + "null", + "com/uber/Main.java"), + new ErrorDisplay( + "ASSIGN_FIELD_NULLABLE", + "assigning @Nullable expression to @NonNull field", + "com.uber.Main", + "f2", + "FIELD", + "com.uber.Main", + "null", + "f2", + "null", + "com/uber/Main.java"), + new ErrorDisplay( + "ASSIGN_FIELD_NULLABLE", + "assigning @Nullable expression to @NonNull field", + "com.uber.Main", + "null", + "FIELD", + "com.uber.Main", + "null", + "f2", + "null", + "com/uber/Main.java"), + new ErrorDisplay( + "PASS_NULLABLE", + "passing @Nullable parameter", + "com.uber.Main$Inner", + "f", + "PARAMETER", + "com.uber.Foo", + "Foo(java.lang.Object)", + "foo", + "0", + "com/uber/Foo.java")) + .setFactory(errorDisplayFactory) + .setOutputFileNameAndHeader(ERROR_FILE_NAME, ERROR_FILE_HEADER) + .doTest(); + } + @Test public void suggestNullableArgumentOnBytecode() { SerializationTestHelper tester = new SerializationTestHelper<>(root); diff --git a/nullaway/src/test/java/com/uber/nullaway/tools/ErrorDisplay.java b/nullaway/src/test/java/com/uber/nullaway/tools/ErrorDisplay.java index 05317462e8..106482336b 100644 --- a/nullaway/src/test/java/com/uber/nullaway/tools/ErrorDisplay.java +++ b/nullaway/src/test/java/com/uber/nullaway/tools/ErrorDisplay.java @@ -30,7 +30,7 @@ public class ErrorDisplay implements Display { public final String type; public final String message; - public final String encMethod; + public final String encMember; public final String encClass; public final String kind; public final String clazz; @@ -43,7 +43,7 @@ public ErrorDisplay( String type, String message, String encClass, - String encMethod, + String encMember, String kind, String clazz, String method, @@ -52,7 +52,7 @@ public ErrorDisplay( String uri) { this.type = type; this.message = message; - this.encMethod = encMethod; + this.encMember = encMember; this.encClass = encClass; this.kind = kind; this.clazz = clazz; @@ -63,8 +63,8 @@ public ErrorDisplay( this.uri = uri.contains("com/uber/") ? uri.substring(uri.indexOf("com/uber/")) : uri; } - public ErrorDisplay(String type, String message, String encClass, String encMethod) { - this(type, message, encClass, encMethod, "null", "null", "null", "null", "null", "null"); + public ErrorDisplay(String type, String message, String encClass, String encMember) { + this(type, message, encClass, encMember, "null", "null", "null", "null", "null", "null"); } @Override @@ -80,10 +80,10 @@ public boolean equals(Object o) { // To increase readability, a shorter version of the actual message might be present in the // expected output of tests. && (message.contains(that.message) || that.message.contains(message)) - && encMethod.equals(that.encMethod) + && encMember.equals(that.encMember) + && clazz.equals(that.clazz) && encClass.equals(that.encClass) && kind.equals(that.kind) - && clazz.equals(that.clazz) && method.equals(that.method) && variable.equals(that.variable) && index.equals(that.index) @@ -93,7 +93,7 @@ public boolean equals(Object o) { @Override public int hashCode() { return Objects.hash( - type, message, encMethod, encClass, kind, clazz, method, variable, index, uri); + type, message, encMember, encClass, kind, clazz, method, variable, index, uri); } @Override @@ -105,8 +105,8 @@ public String toString() { + "\n\tmessage='" + message + '\'' - + "\n\tencMethod='" - + encMethod + + "\n\tencMember='" + + encMember + '\'' + "\n\tencClass='" + encClass