From e73c18e463d89edaebccbe9c63688212d5f25a84 Mon Sep 17 00:00:00 2001 From: Abhijit Kulkarni Date: Sun, 15 Oct 2023 12:12:53 -0700 Subject: [PATCH 1/7] Adding test case and logic to handle return types of method references in generics --- .../com/uber/nullaway/GenericsChecks.java | 2 +- .../main/java/com/uber/nullaway/NullAway.java | 19 ++++++++++---- .../NullAwayJSpecifyGenericsTests.java | 25 +++++++++++++++++++ 3 files changed, 40 insertions(+), 6 deletions(-) diff --git a/nullaway/src/main/java/com/uber/nullaway/GenericsChecks.java b/nullaway/src/main/java/com/uber/nullaway/GenericsChecks.java index ce31aebbef..375b7950f8 100644 --- a/nullaway/src/main/java/com/uber/nullaway/GenericsChecks.java +++ b/nullaway/src/main/java/com/uber/nullaway/GenericsChecks.java @@ -735,7 +735,7 @@ private static Type getTypeForSymbol(Symbol symbol, VisitorState state) { } } - private static Nullness getGenericMethodReturnTypeNullness( + static Nullness getGenericMethodReturnTypeNullness( Symbol.MethodSymbol method, Type enclosingType, VisitorState state, Config config) { Type overriddenMethodType = state.getTypes().memberType(enclosingType, method); verify( diff --git a/nullaway/src/main/java/com/uber/nullaway/NullAway.java b/nullaway/src/main/java/com/uber/nullaway/NullAway.java index 79c7c8ca34..677dd476a5 100644 --- a/nullaway/src/main/java/com/uber/nullaway/NullAway.java +++ b/nullaway/src/main/java/com/uber/nullaway/NullAway.java @@ -944,7 +944,8 @@ private Description checkOverriding( // if the super method returns nonnull, overriding method better not return nullable // Note that, for the overriding method, the permissive default is non-null, // but it's nullable for the overridden one. - if (overriddenMethodReturnsNonNull(overriddenMethod, overridingMethod.owner, state) + if (overriddenMethodReturnsNonNull( + overriddenMethod, overridingMethod.owner, memberReferenceTree, state) && getMethodReturnNullness(overridingMethod, state, Nullness.NONNULL) .equals(Nullness.NULLABLE) && (memberReferenceTree == null @@ -983,7 +984,10 @@ && getMethodReturnNullness(overridingMethod, state, Nullness.NONNULL) } private boolean overriddenMethodReturnsNonNull( - Symbol.MethodSymbol overriddenMethod, Symbol enclosingSymbol, VisitorState state) { + Symbol.MethodSymbol overriddenMethod, + Symbol enclosingSymbol, + @Nullable MemberReferenceTree memberReferenceTree, + VisitorState state) { Nullness methodReturnNullness = getMethodReturnNullness(overriddenMethod, state, Nullness.NULLABLE); if (!methodReturnNullness.equals(Nullness.NONNULL)) { @@ -992,9 +996,14 @@ private boolean overriddenMethodReturnsNonNull( // In JSpecify mode, for generic methods, we additionally need to check the return nullness // using the type parameters from the type enclosing the overriding method if (config.isJSpecifyMode()) { - return GenericsChecks.getGenericMethodReturnTypeNullness( - overriddenMethod, enclosingSymbol, state, config) - .equals(Nullness.NONNULL); + + return (memberReferenceTree != null) + ? GenericsChecks.getGenericMethodReturnTypeNullness( + overriddenMethod, ASTHelpers.getType(memberReferenceTree), state, config) + .equals(Nullness.NONNULL) + : GenericsChecks.getGenericMethodReturnTypeNullness( + overriddenMethod, enclosingSymbol, state, config) + .equals(Nullness.NONNULL); } return true; } diff --git a/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyGenericsTests.java b/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyGenericsTests.java index 7f236cacfd..27bb9305a2 100644 --- a/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyGenericsTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyGenericsTests.java @@ -440,6 +440,31 @@ public void testForMethodReferenceInAnAssignment() { .doTest(); } + @Test + public void testForMethodReferenceReturnType() { + makeHelper() + .addSourceLines( + "Test.java", + "package com.uber;", + "import org.jspecify.annotations.Nullable;", + "class Test {", + " interface A {", + " T1 function(Object o);", + " }", + " static @Nullable String foo(Object o) {", + " return o.toString();", + " }", + " static void testPositive() {", + " // BUG: Diagnostic contains: referenced method returns @Nullable", + " A p = Test::foo;", + " }", + " static void testNegative() {", + " A<@Nullable String> p = Test::foo;", + " }", + "}") + .doTest(); + } + @Test public void testForLambdasInAnAssignment() { makeHelper() From fbdd616775512d4b5ab3a165d1031ac9488e6632 Mon Sep 17 00:00:00 2001 From: Abhijit Kulkarni Date: Sun, 15 Oct 2023 12:51:38 -0700 Subject: [PATCH 2/7] adding null check --- .../src/main/java/com/uber/nullaway/GenericsChecks.java | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/nullaway/src/main/java/com/uber/nullaway/GenericsChecks.java b/nullaway/src/main/java/com/uber/nullaway/GenericsChecks.java index 375b7950f8..8edd318a95 100644 --- a/nullaway/src/main/java/com/uber/nullaway/GenericsChecks.java +++ b/nullaway/src/main/java/com/uber/nullaway/GenericsChecks.java @@ -736,7 +736,12 @@ private static Type getTypeForSymbol(Symbol symbol, VisitorState state) { } static Nullness getGenericMethodReturnTypeNullness( - Symbol.MethodSymbol method, Type enclosingType, VisitorState state, Config config) { + Symbol.MethodSymbol method, @Nullable Type enclosingType, VisitorState state, Config config) { + if (enclosingType == null) { + // we have no additional information from generics, so return NONNULL (presence of a @Nullable + // annotation should have been handled by the caller) + return Nullness.NONNULL; + } Type overriddenMethodType = state.getTypes().memberType(enclosingType, method); verify( overriddenMethodType instanceof ExecutableType, From 8c2f5892f9a953d8c98a7ea5785f08fa1a8efb86 Mon Sep 17 00:00:00 2001 From: Abhijit Kulkarni Date: Sun, 15 Oct 2023 13:51:05 -0700 Subject: [PATCH 3/7] removing redundant lines --- nullaway/src/main/java/com/uber/nullaway/GenericsChecks.java | 5 ----- 1 file changed, 5 deletions(-) diff --git a/nullaway/src/main/java/com/uber/nullaway/GenericsChecks.java b/nullaway/src/main/java/com/uber/nullaway/GenericsChecks.java index 8edd318a95..4eb4cdf1d5 100644 --- a/nullaway/src/main/java/com/uber/nullaway/GenericsChecks.java +++ b/nullaway/src/main/java/com/uber/nullaway/GenericsChecks.java @@ -699,11 +699,6 @@ public static void checkTypeParameterNullnessForMethodOverriding( public static Nullness getGenericMethodReturnTypeNullness( Symbol.MethodSymbol method, Symbol enclosingSymbol, VisitorState state, Config config) { Type enclosingType = getTypeForSymbol(enclosingSymbol, state); - if (enclosingType == null) { - // we have no additional information from generics, so return NONNULL (presence of a @Nullable - // annotation should have been handled by the caller) - return Nullness.NONNULL; - } return getGenericMethodReturnTypeNullness(method, enclosingType, state, config); } From d92b71bd209eec3ea66d99ad6106fde2997bfe4f Mon Sep 17 00:00:00 2001 From: Abhijit Kulkarni Date: Mon, 16 Oct 2023 18:58:47 -0700 Subject: [PATCH 4/7] rewriting the ternary operator as if-else and adding comments for better documentation --- .../main/java/com/uber/nullaway/NullAway.java | 20 +++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/nullaway/src/main/java/com/uber/nullaway/NullAway.java b/nullaway/src/main/java/com/uber/nullaway/NullAway.java index 677dd476a5..ac3f71abd9 100644 --- a/nullaway/src/main/java/com/uber/nullaway/NullAway.java +++ b/nullaway/src/main/java/com/uber/nullaway/NullAway.java @@ -996,14 +996,18 @@ private boolean overriddenMethodReturnsNonNull( // In JSpecify mode, for generic methods, we additionally need to check the return nullness // using the type parameters from the type enclosing the overriding method if (config.isJSpecifyMode()) { - - return (memberReferenceTree != null) - ? GenericsChecks.getGenericMethodReturnTypeNullness( - overriddenMethod, ASTHelpers.getType(memberReferenceTree), state, config) - .equals(Nullness.NONNULL) - : GenericsChecks.getGenericMethodReturnTypeNullness( - overriddenMethod, enclosingSymbol, state, config) - .equals(Nullness.NONNULL); + if (memberReferenceTree != null) { + // For a method reference, we get generic type arguments from javac's inferred type for the + // tree which properly preserves type-use annotations + return GenericsChecks.getGenericMethodReturnTypeNullness( + overriddenMethod, ASTHelpers.getType(memberReferenceTree), state, config) + .equals(Nullness.NONNULL); + } else { + // Using the enclosing class of the overriding method to find generic type arguments + return GenericsChecks.getGenericMethodReturnTypeNullness( + overriddenMethod, enclosingSymbol, state, config) + .equals(Nullness.NONNULL); + } } return true; } From dea0d60ddb70de7e06d63ce38597d79569c374a6 Mon Sep 17 00:00:00 2001 From: Abhijit Kulkarni Date: Tue, 17 Oct 2023 22:15:45 -0700 Subject: [PATCH 5/7] dummy test commit --- nullaway/src/main/java/com/uber/nullaway/NullAway.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nullaway/src/main/java/com/uber/nullaway/NullAway.java b/nullaway/src/main/java/com/uber/nullaway/NullAway.java index 07a38b1641..83cb6071f3 100644 --- a/nullaway/src/main/java/com/uber/nullaway/NullAway.java +++ b/nullaway/src/main/java/com/uber/nullaway/NullAway.java @@ -1014,7 +1014,7 @@ private boolean overriddenMethodReturnsNonNull( overriddenMethod, ASTHelpers.getType(memberReferenceTree), state, config) .equals(Nullness.NONNULL); } else { - // Using the enclosing class of the overriding method to find generic type arguments + // Use the enclosing class of the overriding method to find generic type arguments return GenericsChecks.getGenericMethodReturnTypeNullness( overriddenMethod, enclosingSymbol, state, config) .equals(Nullness.NONNULL); From b22da5204e4ae019207ab0875bebf2cfbd933df8 Mon Sep 17 00:00:00 2001 From: Abhijit Kulkarni Date: Wed, 18 Oct 2023 13:52:15 -0700 Subject: [PATCH 6/7] Adding test cases for different scenarios of generic type method references --- .../NullAwayJSpecifyGenericsTests.java | 77 ++++++++++++++++++- 1 file changed, 76 insertions(+), 1 deletion(-) diff --git a/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyGenericsTests.java b/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyGenericsTests.java index 70ab82b65b..dc55a4080b 100644 --- a/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyGenericsTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyGenericsTests.java @@ -440,7 +440,28 @@ public void testForMethodReferenceInAnAssignment() { } @Test - public void testForMethodReferenceReturnType() { + public void testForMethodReferenceForClassFieldAssignment() { + makeHelper() + .addSourceLines( + "Test.java", + "package com.uber;", + "import org.jspecify.annotations.Nullable;", + "class Test {", + " interface A {", + " T1 function(Object o);", + " }", + " static @Nullable String foo(Object o) {", + " return o.toString();", + " }", + " // BUG: Diagnostic contains: referenced method returns @Nullable", + " A positiveField = Test::foo;", + " A<@Nullable String> negativeField = Test::foo;", + "}") + .doTest(); + } + + @Test + public void testForMethodReferenceReturnTypeInAnAssignment() { makeHelper() .addSourceLines( "Test.java", @@ -464,6 +485,60 @@ public void testForMethodReferenceReturnType() { .doTest(); } + @Test + public void testForMethodReferenceWhenReturned() { + makeHelper() + .addSourceLines( + "Test.java", + "package com.uber;", + "import org.jspecify.annotations.Nullable;", + "class Test {", + " interface A {", + " T1 function(Object o);", + " }", + " static @Nullable String foo(Object o) {", + " return o.toString();", + " }", + " static A testPositive() {", + " // BUG: Diagnostic contains: referenced method returns @Nullable", + " return Test::foo;", + " }", + " static A<@Nullable String> testNegative() {", + " return Test::foo;", + " }", + "}") + .doTest(); + } + + @Test + public void testForMethodReferenceAsMethodParameter() { + makeHelper() + .addSourceLines( + "Test.java", + "package com.uber;", + "import org.jspecify.annotations.Nullable;", + "class Test {", + " interface A {", + " T1 function(Object o);", + " }", + " static @Nullable String foo(Object o) {", + " return o.toString();", + " }", + " static void fooPositive(A a) {", + " }", + " static void fooNegative(A<@Nullable String> a) {", + " }", + " static void testPositive() {", + " // BUG: Diagnostic contains: referenced method returns @Nullable", + " fooPositive(Test::foo);", + " }", + " static void testNegative() {", + " fooNegative(Test::foo);", + " }", + "}") + .doTest(); + } + @Test public void testForLambdasInAnAssignment() { makeHelper() From 1096eaa6df04154f9e2c5ca8b355e930bd014fde Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Thu, 19 Oct 2023 09:57:51 -0700 Subject: [PATCH 7/7] comment tweaks --- nullaway/src/main/java/com/uber/nullaway/NullAway.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/nullaway/src/main/java/com/uber/nullaway/NullAway.java b/nullaway/src/main/java/com/uber/nullaway/NullAway.java index 83cb6071f3..69aaee489f 100644 --- a/nullaway/src/main/java/com/uber/nullaway/NullAway.java +++ b/nullaway/src/main/java/com/uber/nullaway/NullAway.java @@ -1005,11 +1005,11 @@ private boolean overriddenMethodReturnsNonNull( return false; } // In JSpecify mode, for generic methods, we additionally need to check the return nullness - // using the type parameters from the type enclosing the overriding method + // using the type arguments from the type enclosing the overriding method if (config.isJSpecifyMode()) { if (memberReferenceTree != null) { // For a method reference, we get generic type arguments from javac's inferred type for the - // tree which properly preserves type-use annotations + // tree, which properly preserves type-use annotations return GenericsChecks.getGenericMethodReturnTypeNullness( overriddenMethod, ASTHelpers.getType(memberReferenceTree), state, config) .equals(Nullness.NONNULL);