diff --git a/nullaway/src/main/java/com/uber/nullaway/GenericsChecks.java b/nullaway/src/main/java/com/uber/nullaway/GenericsChecks.java index 0cb338df44..8905098c83 100644 --- a/nullaway/src/main/java/com/uber/nullaway/GenericsChecks.java +++ b/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 c, check whether the type parameter nullability for each + * sub-expression of c matches the type parameter nullability of c itself. + * + *

Note that the type parameter nullability for c 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 c will have identical type parameter + * nullability, but will still not match the type parameter nullability of c 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); + } + } + } + } } diff --git a/nullaway/src/main/java/com/uber/nullaway/NullAway.java b/nullaway/src/main/java/com/uber/nullaway/NullAway.java index 9bf61ca334..76bb0d3d61 100644 --- a/nullaway/src/main/java/com/uber/nullaway/NullAway.java +++ b/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; diff --git a/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyGenericsTests.java b/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyGenericsTests.java index d78304c18f..e2291d42d3 100644 --- a/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyGenericsTests.java +++ b/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 { }", + " static A testPositive(A a, boolean t) {", + " // BUG: Diagnostic contains: Conditional expression must have type", + " A<@Nullable String> t1 = t ? new A() : 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 a = testPositive(t ? new A() : 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 {}", + " static class B extends A {}", + " static class C extends A {}", + " static void testPositive(boolean t) {", + " // BUG: Diagnostic contains: Conditional expression must have type", + " A<@Nullable String> t1 = t ? new B<@Nullable String>() : new C();", + " // BUG: Diagnostic contains: Conditional expression must have type", + " A<@Nullable String> t2 = t ? new C() : new B<@Nullable String>();", + " // BUG: Diagnostic contains:Conditional expression must have type", + " A<@Nullable String> t3 = t ? new B() : new C<@Nullable String>();", + " // BUG: Diagnostic contains: Conditional expression must have type", + " A 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 t3 = t ? new C() : new B();", + " }", + "}") + .doTest(); + } + + @Test + public void nestedTernary() { + makeHelper() + .addSourceLines( + "Test.java", + "package com.uber;", + "import org.jspecify.annotations.Nullable;", + "class Test {", + " static class A {}", + " static class B extends A {}", + " static class C extends A {}", + " 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());", + " }", + " 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 { }", + " 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 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()", + " // BUG: Diagnostic contains: Conditional expression must have type", + " : new A();", + " }", + "}") + .doTest(); + } + private CompilationTestHelper makeHelper() { return makeTestHelperWithArgs( Arrays.asList(