JSpecify generics checks for conditional expressions

nullaway/src/main/java/com/uber/nullaway/GenericsChecks.java

@@ -10,6 +10,7 @@
 import com.sun.source.tree.AnnotatedTypeTree;
 import com.sun.source.tree.AnnotationTree;
 import com.sun.source.tree.AssignmentTree;
+import com.sun.source.tree.ConditionalExpressionTree;
 import com.sun.source.tree.ExpressionTree;
 import com.sun.source.tree.NewClassTree;
 import com.sun.source.tree.ParameterizedTypeTree;
@@ -150,6 +151,23 @@ private static void reportInvalidReturnTypeError(
             errorMessage, analysis.buildDescription(tree), state, null));
   }
 
+  private static void reportMismatchedTypeForTernaryOperator(
+      Tree tree, Type expressionType, Type subPartType, VisitorState state, NullAway analysis) {
+    ErrorBuilder errorBuilder = analysis.getErrorBuilder();
+    ErrorMessage errorMessage =
+        new ErrorMessage(
+            ErrorMessage.MessageTypes.ASSIGN_GENERIC_NULLABLE,
+            String.format(
+                "Conditional expression must have type "
+                    + expressionType
+                    + " but the sub-expression has type "
+                    + subPartType
+                    + ", which has mismatched nullability of type parameters"));
+    state.reportMatch(
+        errorBuilder.createErrorDescription(
+            errorMessage, analysis.buildDescription(tree), state, null));
+  }
+
   /**
    * This method returns the type of the given tree, including any type use annotations.
    *
@@ -366,4 +384,49 @@ private Type.ClassType typeWithPreservedAnnotations(ParameterizedTypeTree tree)
             type.getEnclosingType(), com.sun.tools.javac.util.List.from(newTypeArgs), type.tsym);
     return finalType;
   }
+
+  /**
+   * For a conditional expression <em>c</em>, check whether the type parameter nullability for each
+   * sub-expression of <em>c</em> matches the type parameter nullability of <em>c</em> itself.
+   *
+   * <p>Note that the type parameter nullability for <em>c</em> is computed by javac and reflects
+   * what is required of the surrounding context (an assignment, parameter pass, etc.). It is
+   * possible that both sub-expressions of <em>c</em> will have identical type parameter
+   * nullability, but will still not match the type parameter nullability of <em>c</em> itself, due
+   * to requirements from the surrounding context. In such a case, our error messages may be
+   * somewhat confusing; we may want to improve this in the future.
+   *
+   * @param tree A conditional expression tree to check
+   */
+  public void checkTypeParameterNullnessForConditionalExpression(ConditionalExpressionTree tree) {
+    if (!config.isJSpecifyMode()) {
+      return;
+    }
+
+    Tree truePartTree = tree.getTrueExpression();
+    Tree falsePartTree = tree.getFalseExpression();
+
+    Type condExprType = getTreeType(tree);
+    Type truePartType = getTreeType(truePartTree);
+    Type falsePartType = getTreeType(falsePartTree);
+    // The condExpr type should be the least-upper bound of the true and false part types.  To check
+    // the nullability annotations, we check that the true and false parts are assignable to the
+    // type of the whole expression
+    if (condExprType instanceof Type.ClassType) {
+      if (truePartType instanceof Type.ClassType) {
+        if (!compareNullabilityAnnotations(
+            (Type.ClassType) condExprType, (Type.ClassType) truePartType)) {
+          reportMismatchedTypeForTernaryOperator(
+              truePartTree, condExprType, truePartType, state, analysis);
+        }
+      }
+      if (falsePartType instanceof Type.ClassType) {
+        if (!compareNullabilityAnnotations(
+            (Type.ClassType) condExprType, (Type.ClassType) falsePartType)) {
+          reportMismatchedTypeForTernaryOperator(
+              falsePartTree, condExprType, falsePartType, state, analysis);
+        }
+      }
+    }
+  }
 }

nullaway/src/main/java/com/uber/nullaway/NullAway.java

@@ -1493,6 +1493,8 @@ public Description matchUnary(UnaryTree tree, VisitorState state) {
   public Description matchConditionalExpression(
       ConditionalExpressionTree tree, VisitorState state) {
     if (withinAnnotatedCode(state)) {
+      new GenericsChecks(state, config, this)
+          .checkTypeParameterNullnessForConditionalExpression(tree);
       doUnboxingCheck(state, tree.getCondition());
     }
     return Description.NO_MATCH;

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

@@ -557,6 +557,111 @@ public void genericFunctionReturnTypeMultipleReturnStatementsIfElseBlock() {
         .doTest();
   }
 
+  @Test
+  public void genericsChecksForTernaryOperator() {
+    makeHelper()
+        .addSourceLines(
+            "Test.java",
+            "package com.uber;",
+            "import org.jspecify.annotations.Nullable;",
+            "class Test {",
+            "static class A<T extends @Nullable Object> { }",
+            "  static A<String> testPositive(A<String> a, boolean t) {",
+            "    // BUG: Diagnostic contains: Conditional expression must have type",
+            "    A<@Nullable String> t1 = t ? new A<String>() : new A<@Nullable String>();",
+            "    // BUG: Diagnostic contains: Conditional expression must have type",
+            "    return t ? new A<@Nullable String>() : new A<@Nullable String>();",
+            "  }",
+            "  static void testPositiveTernaryMethodArgument(boolean t) {",
+            "    // BUG: Diagnostic contains: Conditional expression must have type",
+            "    A<String> a = testPositive(t ? new A<String>() : new A<@Nullable String>(), t);",
+            "  }",
+            "  static A<@Nullable String> testNegative(boolean t) {",
+            "    A<@Nullable String> t1 = t ? new A<@Nullable String>() : new A<@Nullable String>();",
+            "    return t ? new A<@Nullable String>() : new A<@Nullable String>();",
+            "  }",
+            "}")
+        .doTest();
+  }
+
+  @Test
+  public void ternaryOperatorComplexSubtyping() {
+    makeHelper()
+        .addSourceLines(
+            "Test.java",
+            "package com.uber;",
+            "import org.jspecify.annotations.Nullable;",
+            "class Test {",
+            "  static class A<T extends @Nullable Object> {}",
+            "  static class B<T extends @Nullable Object> extends A<T> {}",
+            "  static class C<T extends @Nullable Object> extends A<T> {}",
+            "  static void testPositive(boolean t) {",
+            "    // BUG: Diagnostic contains: Conditional expression must have type",
+            "    A<@Nullable String> t1 = t ? new B<@Nullable String>() : new C<String>();",
+            "    // BUG: Diagnostic contains: Conditional expression must have type",
+            "    A<@Nullable String> t2 = t ? new C<String>() : new B<@Nullable String>();",
+            "    // BUG: Diagnostic contains:Conditional expression must have type",
+            "    A<@Nullable String> t3 = t ? new B<String>() : new C<@Nullable String>();",
+            "    // BUG: Diagnostic contains: Conditional expression must have type",
+            "    A<String> t4 = t ? new B<@Nullable String>() : new C<@Nullable String>();",
+            "  }",
+            "  static void testNegative(boolean t) {",
+            "    A<@Nullable String> t1 = t ? new B<@Nullable String>() : new C<@Nullable String>();",
+            "    A<@Nullable String> t2 = t ? new C<@Nullable String>() : new B<@Nullable String>();",
+            "    A<String> t3 = t ? new C<String>() : new B<String>();",
+            "  }",
+            "}")
+        .doTest();
+  }
+
+  @Test
+  public void nestedTernary() {
+    makeHelper()
+        .addSourceLines(
+            "Test.java",
+            "package com.uber;",
+            "import org.jspecify.annotations.Nullable;",
+            "class Test {",
+            "  static class A<T extends @Nullable Object> {}",
+            "  static class B<T extends @Nullable Object> extends A<T> {}",
+            "  static class C<T extends @Nullable Object> extends A<T> {}",
+            "  static void testPositive(boolean t) {",
+            "    A<@Nullable String> t1 = t ? new C<@Nullable String>() :",
+            "        // BUG: Diagnostic contains: Conditional expression must have type",
+            "        (t ? new B<@Nullable String>() : new A<String>());",
+            "  }",
+            "  static void testNegative(boolean t) {",
+            "    A<@Nullable String> t1 = t ? new C<@Nullable String>() :",
+            "        (t ? new B<@Nullable String>() : new A<@Nullable String>());",
+            "  }",
+            "}")
+        .doTest();
+  }
+
+  @Test
+  public void ternaryMismatchedAssignmentContext() {
+    makeHelper()
+        .addSourceLines(
+            "Test.java",
+            "package com.uber;",
+            "import org.jspecify.annotations.Nullable;",
+            "class Test {",
+            "static class A<T extends @Nullable Object> { }",
+            "  static void testPositive(boolean t) {",
+            "    // we get two errors here, one for each sub-expression; perhaps ideally we would report",
+            "    // just one error (that the ternary operator has type A<String> but the assignment LHS",
+            "    // has type A<@Nullable String>), but implementing that check in general is",
+            "    // a bit tricky",
+            "    A<@Nullable String> t1 = t",
+            "        // BUG: Diagnostic contains: Conditional expression must have type",
+            "        ? new A<String>()",
+            "        // BUG: Diagnostic contains: Conditional expression must have type",
+            "        : new A<String>();",
+            "  }",
+            "}")
+        .doTest();
+  }
+
   private CompilationTestHelper makeHelper() {
     return makeTestHelperWithArgs(
         Arrays.asList(