Merge remote-tracking branch 'origin/library_model_stub_abhijitk' int…

…o library_model_stub_abhijitk

code-coverage-report/build.gradle

@@ -80,4 +80,6 @@ dependencies {
     implementation project(':jar-infer:nullaway-integration-test')
     implementation project(':guava-recent-unit-tests')
     implementation project(':jdk-recent-unit-tests')
+    implementation project(':library-model:library-model-generator')
+    implementation project(':library-model:library-model-generator-integration-test')
 }

jdk-recent-unit-tests/src/test/java/com/uber/nullaway/jdk17/NullAwaySwitchTests.java

@@ -206,4 +206,42 @@ public void testSwitchExprNullCase() {
             "}")
         .doTest();
   }
+
+  @Test
+  public void testSwitchExprNullCaseDataflow() {
+    defaultCompilationHelper
+        .addSourceLines(
+            "SwitchNullCase.java",
+            "package com.uber;",
+            "import javax.annotation.Nullable;",
+            "class SwitchNullCase {",
+            "  public enum NullableEnum {",
+            "    A,",
+            "    B,",
+            "  }",
+            "  static Object testPositive1(@Nullable NullableEnum nullableEnum) {",
+            "    return switch (nullableEnum) {",
+            "      case A -> new Object();",
+            "      // BUG: Diagnostic contains: dereferenced expression nullableEnum is @Nullable",
+            "      case null -> nullableEnum.hashCode();",
+            "      default -> nullableEnum.toString();",
+            "    };",
+            "  }",
+            "  static Object testPositive2(@Nullable NullableEnum nullableEnum) {",
+            "    return switch (nullableEnum) {",
+            "      case A -> new Object();",
+            "      // BUG: Diagnostic contains: dereferenced expression nullableEnum is @Nullable",
+            "      case null, default -> nullableEnum.toString();",
+            "    };",
+            "  }",
+            "  static Object testNegative(@Nullable NullableEnum nullableEnum) {",
+            "    return switch (nullableEnum) {",
+            "      case A, B -> nullableEnum.hashCode();",
+            "      case null -> new Object();",
+            "      default -> nullableEnum.toString();",
+            "    };",
+            "  }",
+            "}")
+        .doTest();
+  }
 }

library-model/library-model-generator/build.gradle

@@ -14,7 +14,8 @@
  * limitations under the License.
  */
 plugins {
-    id "java-library"
+    id 'java-library'
+    id 'nullaway.java-test-conventions'
 }
 
 dependencies {

nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPath.java

@@ -46,6 +46,7 @@
 import javax.lang.model.element.ElementKind;
 import javax.lang.model.element.Modifier;
 import javax.lang.model.element.VariableElement;
+import org.checkerframework.nullaway.dataflow.cfg.node.ClassNameNode;
 import org.checkerframework.nullaway.dataflow.cfg.node.FieldAccessNode;
 import org.checkerframework.nullaway.dataflow.cfg.node.IntegerLiteralNode;
 import org.checkerframework.nullaway.dataflow.cfg.node.LocalVariableNode;
@@ -477,6 +478,19 @@ && isBoxingMethod(ASTHelpers.getSymbol(methodInvocationTree))) {
       result =
           new AccessPath(
               ((LocalVariableNode) node).getElement(), ImmutableList.copyOf(elements), mapKey);
+    } else if (node instanceof ClassNameNode) {
+      // It is useful to make an access path if elements.size() > 1 and elements.getFirst() is
+      // "this".  In this case, we may have an access of a field of an enclosing class from a nested
+      // class.  Tracking an access path for "Foo.this" alone is not useful, since that can never be
+      // null.  Also, we do not attempt to canonicalize cases where "Foo.this" is used to refer to
+      // the receiver of the current method instead of "this".
+      if (elements.size() > 1
+          && elements.getFirst().getJavaElement().getSimpleName().contentEquals("this")) {
+        Element rootElement = elements.pop().getJavaElement();
+        result = new AccessPath(rootElement, ImmutableList.copyOf(elements), mapKey);
+      } else {
+        result = null;
+      }
     } else if (node instanceof ThisNode || node instanceof SuperNode) {
       result = new AccessPath(null, ImmutableList.copyOf(elements), mapKey);
     } else {

nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPathNullnessPropagation.java

@@ -405,54 +405,43 @@ public TransferResult<Nullness, NullnessStore> visitGreaterThanOrEqual(
   @Override
   public TransferResult<Nullness, NullnessStore> visitEqualTo(
       EqualToNode equalToNode, TransferInput<Nullness, NullnessStore> input) {
-    ReadableUpdates thenUpdates = new ReadableUpdates();
-    ReadableUpdates elseUpdates = new ReadableUpdates();
-    handleEqualityComparison(
-        true,
-        equalToNode.getLeftOperand(),
-        equalToNode.getRightOperand(),
-        values(input),
-        thenUpdates,
-        elseUpdates);
-    ResultingStore thenStore = updateStore(input.getThenStore(), thenUpdates);
-    ResultingStore elseStore = updateStore(input.getElseStore(), elseUpdates);
-    return conditionalResult(
-        thenStore.store, elseStore.store, thenStore.storeChanged || elseStore.storeChanged);
+    return handleEqualityComparison(
+        input, equalToNode.getLeftOperand(), equalToNode.getRightOperand(), true);
   }
 
   @Override
   public TransferResult<Nullness, NullnessStore> visitNotEqual(
       NotEqualNode notEqualNode, TransferInput<Nullness, NullnessStore> input) {
-    ReadableUpdates thenUpdates = new ReadableUpdates();
-    ReadableUpdates elseUpdates = new ReadableUpdates();
-    handleEqualityComparison(
-        false,
-        notEqualNode.getLeftOperand(),
-        notEqualNode.getRightOperand(),
-        values(input),
-        thenUpdates,
-        elseUpdates);
-    ResultingStore thenStore = updateStore(input.getThenStore(), thenUpdates);
-    ResultingStore elseStore = updateStore(input.getElseStore(), elseUpdates);
-    return conditionalResult(
-        thenStore.store, elseStore.store, thenStore.storeChanged || elseStore.storeChanged);
+    return handleEqualityComparison(
+        input, notEqualNode.getLeftOperand(), notEqualNode.getRightOperand(), false);
   }
 
-  private void handleEqualityComparison(
-      boolean equalTo,
-      Node leftNode,
-      Node rightNode,
-      SubNodeValues inputs,
-      Updates thenUpdates,
-      Updates elseUpdates) {
-    Nullness leftVal = inputs.valueOfSubNode(leftNode);
-    Nullness rightVal = inputs.valueOfSubNode(rightNode);
+  /**
+   * Handle nullability refinements from an equality comparison.
+   *
+   * @param input transfer input for the operation
+   * @param leftOperand left operand of the comparison
+   * @param rightOperand right operand of the comparison
+   * @param equalTo if {@code true}, the comparison is an equality comparison, otherwise it is a
+   *     dis-equality ({@code !=}) comparison
+   * @return a TransferResult reflecting any updates from the comparison
+   */
+  private TransferResult<Nullness, NullnessStore> handleEqualityComparison(
+      TransferInput<Nullness, NullnessStore> input,
+      Node leftOperand,
+      Node rightOperand,
+      boolean equalTo) {
+    ReadableUpdates thenUpdates = new ReadableUpdates();
+    ReadableUpdates elseUpdates = new ReadableUpdates();
+    SubNodeValues inputs = values(input);
+    Nullness leftVal = inputs.valueOfSubNode(leftOperand);
+    Nullness rightVal = inputs.valueOfSubNode(rightOperand);
     Nullness equalBranchValue = leftVal.greatestLowerBound(rightVal);
     Updates equalBranchUpdates = equalTo ? thenUpdates : elseUpdates;
     Updates notEqualBranchUpdates = equalTo ? elseUpdates : thenUpdates;
 
-    Node realLeftNode = unwrapAssignExpr(leftNode);
-    Node realRightNode = unwrapAssignExpr(rightNode);
+    Node realLeftNode = unwrapAssignExpr(leftOperand);
+    Node realRightNode = unwrapAssignExpr(rightOperand);
 
     AccessPath leftAP = AccessPath.getAccessPathForNode(realLeftNode, state, apContext);
     if (leftAP != null) {
@@ -467,6 +456,10 @@ private void handleEqualityComparison(
       notEqualBranchUpdates.set(
           rightAP, rightVal.greatestLowerBound(leftVal.deducedValueWhenNotEqual()));
     }
+    ResultingStore thenStore = updateStore(input.getThenStore(), thenUpdates);
+    ResultingStore elseStore = updateStore(input.getElseStore(), elseUpdates);
+    return conditionalResult(
+        thenStore.store, elseStore.store, thenStore.storeChanged || elseStore.storeChanged);
   }
 
   @Override
@@ -932,7 +925,18 @@ public TransferResult<Nullness, NullnessStore> visitThrow(
   @Override
   public TransferResult<Nullness, NullnessStore> visitCase(
       CaseNode caseNode, TransferInput<Nullness, NullnessStore> input) {
-    return noStoreChanges(NULLABLE, input);
+    List<Node> caseOperands = caseNode.getCaseOperands();
+    if (caseOperands.isEmpty()) {
+      return noStoreChanges(NULLABLE, input);
+    } else {
+      // `null` can only appear on its own as a case operand, or together with the default case
+      // (i.e., `case null, default:`).  So, it is safe to only look at the first case operand, and
+      // update the stores based on that.  We treat the case operation as an equality comparison
+      // between the switch expression and the case operand.
+      Node switchOperand = caseNode.getSwitchOperand().getExpression();
+      Node caseOperand = caseOperands.get(0);
+      return handleEqualityComparison(input, switchOperand, caseOperand, true);
+    }
   }
 
   @Override

nullaway/src/test/java/com/uber/nullaway/NullAwayAccessPathsTests.java

@@ -379,4 +379,59 @@ public void testFinalFieldNullabilityPropagatesToInnerContexts() {
             "}")
         .doTest();
   }
+
+  @Test
+  public void testAccessUsingExplicitThis() {
+    defaultCompilationHelper
+        .addSourceLines(
+            "Foo.java",
+            "package com.uber;",
+            "import javax.annotation.Nullable;",
+            "public class Foo {",
+            "  @Nullable public Object bar;",
+            "  public class Nested {",
+            "    @Nullable public Object bar;",
+            "    public void testNegative1() {",
+            "      if (Foo.this.bar != null) {",
+            "        Foo.this.bar.toString();",
+            "      }",
+            "    }",
+            "    public void testNegative2() {",
+            "      // Foo.this can never be null",
+            "      Foo.this.toString();",
+            "    }",
+            "    public void testPositive() {",
+            "      if (Foo.this.bar != null) {",
+            "        // BUG: Diagnostic contains: dereferenced expression this.bar is @Nullable",
+            "        this.bar.toString();",
+            "      }",
+            "      if (this.bar != null) {",
+            "        // BUG: Diagnostic contains: dereferenced expression Foo.this.bar is @Nullable",
+            "        Foo.this.bar.toString();",
+            "      }",
+            "    }",
+            "  }",
+            "  public void testUnhandled1() {",
+            "    if (bar != null) {",
+            "      // This is safe but we don't currently handle it",
+            "      // BUG: Diagnostic contains: dereferenced expression Foo.this.bar is @Nullable",
+            "      Foo.this.bar.toString();",
+            "    }",
+            "  }",
+            "  public void testUnhandled2() {",
+            "    if (Foo.this.bar != null) {",
+            "      // This is safe but we don't currently handle it",
+            "      // BUG: Diagnostic contains: dereferenced expression bar is @Nullable",
+            "      bar.toString();",
+            "    }",
+            "  }",
+            "  public void testNegative1() {",
+            "    if (bar != null) {",
+            "      // This is safe and handled",
+            "      this.bar.toString();",
+            "    }",
+            "  }",
+            "}")
+        .doTest();
+  }
 }