From b64d1c12445792d52b40198ad5ef0d159c021a78 Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Thu, 28 Sep 2023 21:56:52 -0700 Subject: [PATCH] Properly check generic method overriding in explicitly-typed anonymous classes (#808) This completes a task lingering from #755, which did not handle overriding in anonymous classes. This case is slightly tricky as javac does not preserve annotations on type arguments in the types it computes for anonymous classes. To fix, we pass a `Symbol` where we were passing a `Type` in a couple places, as then we can call `Symbol.isAnonymous()` to check for an anonymous type. In such a case, we try to get the type from the enclosing `NewClassTree` of the overriding method. Note that this PR still does not handle anonymous classes that use the diamond operator; we add a test to show the gaps. --- nullaway/build.gradle | 2 + .../com/uber/nullaway/GenericsChecks.java | 82 ++++++++- .../main/java/com/uber/nullaway/NullAway.java | 8 +- .../NullAwayJSpecifyGenericsTests.java | 158 ++++++++++++++++++ 4 files changed, 240 insertions(+), 10 deletions(-) diff --git a/nullaway/build.gradle b/nullaway/build.gradle index 026a89094a..019cb8b5ff 100644 --- a/nullaway/build.gradle +++ b/nullaway/build.gradle @@ -123,6 +123,8 @@ def jdk8Test = tasks.register("testJdk8", Test) { testClassesDirs = testTask.testClassesDirs jvmArgs "-Xbootclasspath/p:${configurations.errorproneJavac.asPath}" filter { + // JDK 8 does not support diamonds on anonymous classes + excludeTestsMatching "com.uber.nullaway.NullAwayJSpecifyGenericsTests.overrideDiamondAnonymousClass" // tests cannot run on JDK 8 since Mockito version no longer supports it excludeTestsMatching "com.uber.nullaway.NullAwaySerializationTest.initializationError" excludeTestsMatching "com.uber.nullaway.handlers.contract.ContractUtilsTest.getEmptyAntecedent" diff --git a/nullaway/src/main/java/com/uber/nullaway/GenericsChecks.java b/nullaway/src/main/java/com/uber/nullaway/GenericsChecks.java index a9f3a6d717..0d814a7bc1 100644 --- a/nullaway/src/main/java/com/uber/nullaway/GenericsChecks.java +++ b/nullaway/src/main/java/com/uber/nullaway/GenericsChecks.java @@ -1,5 +1,6 @@ package com.uber.nullaway; +import static com.google.common.base.Verify.verify; import static com.uber.nullaway.NullabilityUtil.castToNonNull; import static java.util.stream.Collectors.joining; @@ -22,6 +23,7 @@ import com.sun.source.tree.Tree; import com.sun.source.tree.VariableTree; import com.sun.source.util.SimpleTreeVisitor; +import com.sun.source.util.TreePath; import com.sun.tools.javac.code.Attribute; import com.sun.tools.javac.code.BoundKind; import com.sun.tools.javac.code.Symbol; @@ -686,14 +688,53 @@ public static void checkTypeParameterNullnessForMethodOverriding( * String}. * * @param method the generic method - * @param enclosingType the enclosing type in which we want to know {@code method}'s return type - * nullability + * @param enclosingSymbol the enclosing class in which we want to know {@code method}'s return + * type nullability * @param state Visitor state * @param config The analysis config * @return nullability of the return type of {@code method} in the context of {@code * enclosingType} */ 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); + } + + /** + * Get the type for the symbol, accounting for anonymous classes + * + * @param symbol the symbol + * @param state the visitor state + * @return the type for {@code symbol} + */ + @Nullable + private static Type getTypeForSymbol(Symbol symbol, VisitorState state) { + if (symbol.isAnonymous()) { + // For anonymous classes, symbol.type does not contain annotations on generic type parameters. + // So, we get a correct type from the enclosing NewClassTree. + TreePath path = state.getPath(); + NewClassTree newClassTree = ASTHelpers.findEnclosingNode(path, NewClassTree.class); + if (newClassTree == null) { + throw new RuntimeException( + "method should be inside a NewClassTree " + state.getSourceForNode(path.getLeaf())); + } + Type typeFromTree = getTreeType(newClassTree, state); + if (typeFromTree != null) { + verify(state.getTypes().isAssignable(symbol.type, typeFromTree)); + } + return typeFromTree; + } else { + return symbol.type; + } + } + + private static Nullness getGenericMethodReturnTypeNullness( Symbol.MethodSymbol method, Type enclosingType, VisitorState state, Config config) { Type overriddenMethodType = state.getTypes().memberType(enclosingType, method); if (!(overriddenMethodType instanceof Type.MethodType)) { @@ -743,7 +784,7 @@ public static Nullness getGenericReturnNullnessAtInvocation( } Type methodReceiverType = castToNonNull( - ASTHelpers.getType(((MemberSelectTree) tree.getMethodSelect()).getExpression())); + getTreeType(((MemberSelectTree) tree.getMethodSelect()).getExpression(), state)); return getGenericMethodReturnTypeNullness( invokedMethodSymbol, methodReceiverType, state, config); } @@ -791,11 +832,11 @@ public static Nullness getGenericParameterNullnessAtInvocation( if (!(tree.getMethodSelect() instanceof MemberSelectTree)) { return Nullness.NONNULL; } - Type methodReceiverType = + Type enclosingType = castToNonNull( - ASTHelpers.getType(((MemberSelectTree) tree.getMethodSelect()).getExpression())); + getTreeType(((MemberSelectTree) tree.getMethodSelect()).getExpression(), state)); return getGenericMethodParameterNullness( - paramIndex, invokedMethodSymbol, methodReceiverType, state, config); + paramIndex, invokedMethodSymbol, enclosingType, state, config); } /** @@ -822,6 +863,35 @@ public static Nullness getGenericParameterNullnessAtInvocation( * * @param parameterIndex index of the parameter * @param method the generic method + * @param enclosingSymbol the enclosing symbol in which we want to know {@code method}'s parameter + * type nullability + * @param state the visitor state + * @param config the config + * @return nullability of the relevant parameter type of {@code method} in the context of {@code + * enclosingSymbol} + */ + public static Nullness getGenericMethodParameterNullness( + int parameterIndex, + 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 getGenericMethodParameterNullness(parameterIndex, method, enclosingType, state, config); + } + + /** + * Just like {@link #getGenericMethodParameterNullness(int, Symbol.MethodSymbol, Symbol, + * VisitorState, Config)}, but takes the enclosing {@code Type} rather than the enclosing {@code + * Symbol}. + * + * @param parameterIndex index of the parameter + * @param method the generic method * @param enclosingType the enclosing type in which we want to know {@code method}'s parameter * type nullability * @param state the visitor state diff --git a/nullaway/src/main/java/com/uber/nullaway/NullAway.java b/nullaway/src/main/java/com/uber/nullaway/NullAway.java index bfed0af259..79c7c8ca34 100644 --- a/nullaway/src/main/java/com/uber/nullaway/NullAway.java +++ b/nullaway/src/main/java/com/uber/nullaway/NullAway.java @@ -738,7 +738,7 @@ private Description checkParamOverriding( ? GenericsChecks.getGenericMethodParameterNullness( i, overriddenMethod, - overridingParamSymbols.get(i).owner.owner.type, + overridingParamSymbols.get(i).owner.owner, state, config) : Nullness.NONNULL); @@ -944,7 +944,7 @@ 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.type, state) + if (overriddenMethodReturnsNonNull(overriddenMethod, overridingMethod.owner, state) && getMethodReturnNullness(overridingMethod, state, Nullness.NONNULL) .equals(Nullness.NULLABLE) && (memberReferenceTree == null @@ -983,7 +983,7 @@ && getMethodReturnNullness(overridingMethod, state, Nullness.NONNULL) } private boolean overriddenMethodReturnsNonNull( - Symbol.MethodSymbol overriddenMethod, Type overridingMethodType, VisitorState state) { + Symbol.MethodSymbol overriddenMethod, Symbol enclosingSymbol, VisitorState state) { Nullness methodReturnNullness = getMethodReturnNullness(overriddenMethod, state, Nullness.NULLABLE); if (!methodReturnNullness.equals(Nullness.NONNULL)) { @@ -993,7 +993,7 @@ private boolean overriddenMethodReturnsNonNull( // using the type parameters from the type enclosing the overriding method if (config.isJSpecifyMode()) { return GenericsChecks.getGenericMethodReturnTypeNullness( - overriddenMethod, overridingMethodType, state, config) + 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 14eeb16cbc..7f236cacfd 100644 --- a/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyGenericsTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyGenericsTests.java @@ -2,6 +2,7 @@ import com.google.errorprone.CompilationTestHelper; import java.util.Arrays; +import org.junit.Ignore; import org.junit.Test; public class NullAwayJSpecifyGenericsTests extends NullAwayTestsBase { @@ -974,6 +975,163 @@ public void overrideParameterType() { .doTest(); } + @Test + public void overrideExplicitlyTypedAnonymousClass() { + makeHelper() + .addSourceLines( + "Test.java", + "package com.uber;", + "import org.jspecify.annotations.Nullable;", + "class Test {", + " interface Fn

{", + " R apply(P p);", + " }", + " static abstract class FnClass

{", + " abstract R apply(P p);", + " }", + " static void anonymousClasses() {", + " Fn<@Nullable String, String> fn1 = new Fn<@Nullable String, String>() {", + " // BUG: Diagnostic contains: parameter s is @NonNull, but parameter in superclass method", + " public String apply(String s) { return s; }", + " };", + " FnClass fn2 = new FnClass() {", + " // BUG: Diagnostic contains: method returns @Nullable, but superclass method", + " public @Nullable String apply(String s) { return null; }", + " };", + " Fn fn3 = new Fn() {", + " public @Nullable String apply(String s) { return null; }", + " };", + " FnClass<@Nullable String, String> fn4 = new FnClass<@Nullable String, String>() {", + " public String apply(@Nullable String s) { return \"hello\"; }", + " };", + " }", + " static void anonymousClassesFullName() {", + " Test.Fn<@Nullable String, String> fn1 = new Test.Fn<@Nullable String, String>() {", + " // BUG: Diagnostic contains: parameter s is @NonNull, but parameter in superclass method", + " public String apply(String s) { return s; }", + " };", + " Test.FnClass fn2 = new Test.FnClass() {", + " // BUG: Diagnostic contains: method returns @Nullable, but superclass method", + " public @Nullable String apply(String s) { return null; }", + " };", + " Test.Fn fn3 = new Test.Fn() {", + " public @Nullable String apply(String s) { return null; }", + " };", + " Test.FnClass<@Nullable String, String> fn4 = new Test.FnClass<@Nullable String, String>() {", + " public String apply(@Nullable String s) { return \"hello\"; }", + " };", + " }", + "}") + .doTest(); + } + + @Ignore("https://github.com/uber/NullAway/issues/836") + @Test + public void overrideAnonymousNestedClass() { + makeHelper() + .addSourceLines( + "Test.java", + "package com.uber;", + "import org.jspecify.annotations.Nullable;", + "class Test {", + " class Wrapper

{", + " abstract class Fn {", + " abstract R apply(P p);", + " }", + " }", + " void anonymousNestedClasses() {", + " Wrapper<@Nullable String>.Fn fn1 = (this.new Wrapper<@Nullable String>()).new Fn() {", + " // BUG: Diagnostic contains: parameter s is @NonNull, but parameter in superclass method", + " public String apply(String s) { return s; }", + " };", + " }", + "}") + .doTest(); + } + + @Test + public void explicitlyTypedAnonymousClassAsReceiver() { + makeHelper() + .addSourceLines( + "Test.java", + "package com.uber;", + "import org.jspecify.annotations.Nullable;", + "class Test {", + " interface Fn

{", + " R apply(P p);", + " }", + " static abstract class FnClass

{", + " abstract R apply(P p);", + " }", + " static void anonymousClasses() {", + " String s1 = (new Fn() {", + " public @Nullable String apply(String s) { return null; }", + " }).apply(\"hi\");", + " // BUG: Diagnostic contains: dereferenced expression s1", + " s1.hashCode();", + " String s2 = (new FnClass() {", + " public @Nullable String apply(String s) { return null; }", + " }).apply(\"hi\");", + " // BUG: Diagnostic contains: dereferenced expression s2", + " s2.hashCode();", + " (new Fn() {", + " public String apply(String s) { return \"hi\"; }", + " // BUG: Diagnostic contains: passing @Nullable parameter", + " }).apply(null);", + " (new FnClass() {", + " public String apply(String s) { return \"hi\"; }", + " // BUG: Diagnostic contains: passing @Nullable parameter", + " }).apply(null);", + " (new Fn<@Nullable String, String>() {", + " public String apply(@Nullable String s) { return \"hi\"; }", + " }).apply(null);", + " (new FnClass<@Nullable String, String>() {", + " public String apply(@Nullable String s) { return \"hi\"; }", + " }).apply(null);", + " }", + "}") + .doTest(); + } + + /** Diamond anonymous classes are not supported yet; tests are for future reference */ + @Test + public void overrideDiamondAnonymousClass() { + makeHelper() + .addSourceLines( + "Test.java", + "package com.uber;", + "import org.jspecify.annotations.Nullable;", + "class Test {", + " interface Fn

{", + " R apply(P p);", + " }", + " static abstract class FnClass

{", + " abstract R apply(P p);", + " }", + " static void anonymousClasses() {", + " Fn<@Nullable String, String> fn1 = new Fn<>() {", + " // TODO: should report a bug here", + " public String apply(String s) { return s; }", + " };", + " FnClass<@Nullable String, String> fn2 = new FnClass<>() {", + " // TODO: should report a bug here", + " public String apply(String s) { return s; }", + " };", + " Fn fn3 = new Fn<>() {", + " // TODO: this is a false positive", + " // BUG: Diagnostic contains: method returns @Nullable, but superclass method", + " public @Nullable String apply(String s) { return null; }", + " };", + " FnClass fn4 = new FnClass<>() {", + " // TODO: this is a false positive", + " // BUG: Diagnostic contains: method returns @Nullable, but superclass method", + " public @Nullable String apply(String s) { return null; }", + " };", + " }", + "}") + .doTest(); + } + @Test public void nullableGenericTypeVariableReturnType() { makeHelper()