From 558d34ad990e6dd62247ce909f726ceaaefc2f45 Mon Sep 17 00:00:00 2001 From: Abhijit Kulkarni Date: Sat, 28 Oct 2023 21:17:36 -0700 Subject: [PATCH 1/5] adding test case and changes for handling return types in lambda expressions for generic types --- .../main/java/com/uber/nullaway/NullAway.java | 7 +++++++ .../NullAwayJSpecifyGenericsTests.java | 18 ++++++++++++++++++ 2 files changed, 25 insertions(+) diff --git a/nullaway/src/main/java/com/uber/nullaway/NullAway.java b/nullaway/src/main/java/com/uber/nullaway/NullAway.java index d4ce4e827d..6efb7b0604 100644 --- a/nullaway/src/main/java/com/uber/nullaway/NullAway.java +++ b/nullaway/src/main/java/com/uber/nullaway/NullAway.java @@ -873,6 +873,13 @@ private Description checkReturnExpression( retExpr, methodSymbol, this, state); if (getMethodReturnNullness(methodSymbol, state, Nullness.NULLABLE).equals(Nullness.NULLABLE)) { return Description.NO_MATCH; + } else if (config.isJSpecifyMode() + && GenericsChecks.getGenericMethodReturnTypeNullness( + methodSymbol, ASTHelpers.getType(tree), state, config) + .equals(Nullness.NULLABLE)) { + // Get the Nullness if the Annotation is indirectly applied through a generic type if we + // are in JSpecify mode + return Description.NO_MATCH; } if (mayBeNullExpr(state, retExpr)) { return errorBuilder.createErrorDescriptionForNullAssignment( diff --git a/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyGenericsTests.java b/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyGenericsTests.java index 73737b8ca6..d3bb270a53 100644 --- a/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyGenericsTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyGenericsTests.java @@ -650,6 +650,24 @@ public void testForLambdasInAnAssignmentWithoutJSpecifyMode() { .doTest(); } + @Test + public void testForLambdaReturnTypeInAnAssignment() { + makeHelper() + .addSourceLines( + "Test.java", + "package com.uber;", + "import org.jspecify.annotations.Nullable;", + "class Test {", + " interface A {", + " T1 function(Object o);", + " }", + " static void testNegative() {", + " A<@Nullable String> p = x -> null;", + " }", + "}") + .doTest(); + } + @Test public void testForDiamondInAnAssignment() { makeHelper() From 0947ce38e3f4143a7356c6bf4ddcfc363f06cd27 Mon Sep 17 00:00:00 2001 From: Abhijit Kulkarni Date: Sat, 28 Oct 2023 21:21:59 -0700 Subject: [PATCH 2/5] adding positive test scenario for testForLambdaReturnTypeInAnAssignment --- .../java/com/uber/nullaway/NullAwayJSpecifyGenericsTests.java | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyGenericsTests.java b/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyGenericsTests.java index d3bb270a53..f4d70abcf2 100644 --- a/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyGenericsTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyGenericsTests.java @@ -661,6 +661,10 @@ public void testForLambdaReturnTypeInAnAssignment() { " interface A {", " T1 function(Object o);", " }", + " static void testPositive() {", + " // BUG: Diagnostic contains: returning @Nullable expression from method with @NonNull return type", + " A p = x -> null;", + " }", " static void testNegative() {", " A<@Nullable String> p = x -> null;", " }", From 630e6c2c6b6f7766eb7ed1d6aed41cbff9eed6ed Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Sun, 29 Oct 2023 06:40:53 -0700 Subject: [PATCH 3/5] add another test --- .../uber/nullaway/NullAwayJSpecifyGenericsTests.java | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyGenericsTests.java b/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyGenericsTests.java index f4d70abcf2..e7227064f3 100644 --- a/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyGenericsTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyGenericsTests.java @@ -661,13 +661,20 @@ public void testForLambdaReturnTypeInAnAssignment() { " interface A {", " T1 function(Object o);", " }", - " static void testPositive() {", + " static void testPositive1() {", " // BUG: Diagnostic contains: returning @Nullable expression from method with @NonNull return type", " A p = x -> null;", " }", - " static void testNegative() {", + " static void testPositive2() {", + " // BUG: Diagnostic contains: returning @Nullable expression from method with @NonNull return type", + " A p = x -> { return null; };", + " }", + " static void testNegative1() {", " A<@Nullable String> p = x -> null;", " }", + " static void testNegative2() {", + " A<@Nullable String> p = x -> { return null; };", + " }", "}") .doTest(); } From 5588b5d5b0dfa1c2a8ae2469ebb583088424608d Mon Sep 17 00:00:00 2001 From: Abhijit Kulkarni Date: Tue, 31 Oct 2023 15:01:38 -0700 Subject: [PATCH 4/5] updating logic for using checkReturnExpression --- .../main/java/com/uber/nullaway/NullAway.java | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/nullaway/src/main/java/com/uber/nullaway/NullAway.java b/nullaway/src/main/java/com/uber/nullaway/NullAway.java index 6efb7b0604..18ae816765 100644 --- a/nullaway/src/main/java/com/uber/nullaway/NullAway.java +++ b/nullaway/src/main/java/com/uber/nullaway/NullAway.java @@ -388,8 +388,9 @@ public Description matchReturn(ReturnTree tree, VisitorState state) { methodSymbol = NullabilityUtil.getFunctionalInterfaceMethod( (LambdaExpressionTree) leaf, state.getTypes()); + return checkReturnExpression(tree, (LambdaExpressionTree) leaf, retExpr, methodSymbol, state); } - return checkReturnExpression(tree, retExpr, methodSymbol, state); + return checkReturnExpression(tree, null, retExpr, methodSymbol, state); } @Override @@ -854,7 +855,11 @@ private Nullness getMethodReturnNullness( } private Description checkReturnExpression( - Tree tree, ExpressionTree retExpr, Symbol.MethodSymbol methodSymbol, VisitorState state) { + Tree errorTree, + @Nullable LambdaExpressionTree lambdaTree, + ExpressionTree retExpr, + Symbol.MethodSymbol methodSymbol, + VisitorState state) { Type returnType = methodSymbol.getReturnType(); if (returnType.isPrimitive()) { // check for unboxing @@ -874,8 +879,9 @@ private Description checkReturnExpression( if (getMethodReturnNullness(methodSymbol, state, Nullness.NULLABLE).equals(Nullness.NULLABLE)) { return Description.NO_MATCH; } else if (config.isJSpecifyMode() + && null != lambdaTree && GenericsChecks.getGenericMethodReturnTypeNullness( - methodSymbol, ASTHelpers.getType(tree), state, config) + methodSymbol, ASTHelpers.getType(lambdaTree), state, config) .equals(Nullness.NULLABLE)) { // Get the Nullness if the Annotation is indirectly applied through a generic type if we // are in JSpecify mode @@ -887,7 +893,7 @@ private Description checkReturnExpression( MessageTypes.RETURN_NULLABLE, "returning @Nullable expression from method with @NonNull return type"), retExpr, - buildDescription(tree), + buildDescription(errorTree), state, methodSymbol); } @@ -923,7 +929,7 @@ public Description matchLambdaExpression(LambdaExpressionTree tree, VisitorState if (tree.getBodyKind() == LambdaExpressionTree.BodyKind.EXPRESSION && funcInterfaceMethod.getReturnType().getKind() != TypeKind.VOID) { ExpressionTree resExpr = (ExpressionTree) tree.getBody(); - return checkReturnExpression(tree, resExpr, funcInterfaceMethod, state); + return checkReturnExpression(tree, tree, resExpr, funcInterfaceMethod, state); } return Description.NO_MATCH; } From b88266a50885e1507fc8599a5667f8263db208e9 Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Wed, 1 Nov 2023 08:59:08 -0700 Subject: [PATCH 5/5] tweaks and documentation --- .../main/java/com/uber/nullaway/NullAway.java | 44 +++++++++++++------ 1 file changed, 31 insertions(+), 13 deletions(-) diff --git a/nullaway/src/main/java/com/uber/nullaway/NullAway.java b/nullaway/src/main/java/com/uber/nullaway/NullAway.java index 18ae816765..ff4ee80a1b 100644 --- a/nullaway/src/main/java/com/uber/nullaway/NullAway.java +++ b/nullaway/src/main/java/com/uber/nullaway/NullAway.java @@ -380,17 +380,16 @@ public Description matchReturn(ReturnTree tree, VisitorState state) { } Tree leaf = enclosingMethodOrLambda.getLeaf(); Symbol.MethodSymbol methodSymbol; + LambdaExpressionTree lambdaTree = null; if (leaf instanceof MethodTree) { MethodTree enclosingMethod = (MethodTree) leaf; methodSymbol = ASTHelpers.getSymbol(enclosingMethod); } else { // we have a lambda - methodSymbol = - NullabilityUtil.getFunctionalInterfaceMethod( - (LambdaExpressionTree) leaf, state.getTypes()); - return checkReturnExpression(tree, (LambdaExpressionTree) leaf, retExpr, methodSymbol, state); + lambdaTree = (LambdaExpressionTree) leaf; + methodSymbol = NullabilityUtil.getFunctionalInterfaceMethod(lambdaTree, state.getTypes()); } - return checkReturnExpression(tree, null, retExpr, methodSymbol, state); + return checkReturnExpression(retExpr, methodSymbol, lambdaTree, tree, state); } @Override @@ -854,11 +853,25 @@ private Nullness getMethodReturnNullness( methodSymbol, state, isMethodAnnotated, methodReturnNullness); } + /** + * Checks that if a returned expression is {@code @Nullable}, the enclosing method does not have a + * {@code @NonNull} return type. Also performs an unboxing check on the returned expression. + * Finally, in JSpecify mode, also checks that the nullability of generic type arguments of the + * returned expression's type match the method return type. + * + * @param retExpr the expression being returned + * @param methodSymbol symbol for the enclosing method + * @param lambdaTree if return is inside a lambda, the tree for the lambda, otherwise {@code null} + * @param errorTree tree on which to report an error if needed + * @param state the visitor state + * @return {@link Description} of the returning {@code @Nullable} from {@code @NonNull} method + * error if one is to be reported, otherwise {@link Description#NO_MATCH} + */ private Description checkReturnExpression( - Tree errorTree, - @Nullable LambdaExpressionTree lambdaTree, ExpressionTree retExpr, Symbol.MethodSymbol methodSymbol, + @Nullable LambdaExpressionTree lambdaTree, + Tree errorTree, VisitorState state) { Type returnType = methodSymbol.getReturnType(); if (returnType.isPrimitive()) { @@ -872,21 +885,26 @@ private Description checkReturnExpression( // support) return Description.NO_MATCH; } - // Do the generics checks here, since we need to check the type arguments regardless of the - // top-level nullability of the return type + + // Check generic type arguments for returned expression here, since we need to check the type + // arguments regardless of the top-level nullability of the return type GenericsChecks.checkTypeParameterNullnessForFunctionReturnType( retExpr, methodSymbol, this, state); + + // Now, perform the check for returning @Nullable from @NonNull. First, we check if the return + // type is @Nullable, and if so, bail out. if (getMethodReturnNullness(methodSymbol, state, Nullness.NULLABLE).equals(Nullness.NULLABLE)) { return Description.NO_MATCH; } else if (config.isJSpecifyMode() - && null != lambdaTree + && lambdaTree != null && GenericsChecks.getGenericMethodReturnTypeNullness( methodSymbol, ASTHelpers.getType(lambdaTree), state, config) .equals(Nullness.NULLABLE)) { - // Get the Nullness if the Annotation is indirectly applied through a generic type if we - // are in JSpecify mode + // In JSpecify mode, the return type of a lambda may be @Nullable via a type argument return Description.NO_MATCH; } + + // Return type is @NonNull. Check if the expression is @Nullable if (mayBeNullExpr(state, retExpr)) { return errorBuilder.createErrorDescriptionForNullAssignment( new ErrorMessage( @@ -929,7 +947,7 @@ public Description matchLambdaExpression(LambdaExpressionTree tree, VisitorState if (tree.getBodyKind() == LambdaExpressionTree.BodyKind.EXPRESSION && funcInterfaceMethod.getReturnType().getKind() != TypeKind.VOID) { ExpressionTree resExpr = (ExpressionTree) tree.getBody(); - return checkReturnExpression(tree, tree, resExpr, funcInterfaceMethod, state); + return checkReturnExpression(resExpr, funcInterfaceMethod, tree, tree, state); } return Description.NO_MATCH; }