From 362ee2f0842e9b2ce7cbd8f2503955b1566a217a Mon Sep 17 00:00:00 2001 From: Nikita Dinkar Aware Date: Fri, 31 Mar 2023 15:12:53 -0700 Subject: [PATCH] all changes for handling method overrides --- .../com/uber/nullaway/GenericsChecks.java | 202 +++++++++++++- .../main/java/com/uber/nullaway/NullAway.java | 24 +- .../AccessPathNullnessPropagation.java | 19 +- .../NullAwayJSpecifyGenericsTests.java | 256 +++++++++++++++++- 4 files changed, 492 insertions(+), 9 deletions(-) diff --git a/nullaway/src/main/java/com/uber/nullaway/GenericsChecks.java b/nullaway/src/main/java/com/uber/nullaway/GenericsChecks.java index 13ffd147f2..6c20784f60 100644 --- a/nullaway/src/main/java/com/uber/nullaway/GenericsChecks.java +++ b/nullaway/src/main/java/com/uber/nullaway/GenericsChecks.java @@ -12,6 +12,7 @@ import com.sun.source.tree.AssignmentTree; import com.sun.source.tree.ConditionalExpressionTree; import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.MethodTree; import com.sun.source.tree.NewClassTree; import com.sun.source.tree.ParameterizedTypeTree; import com.sun.source.tree.Tree; @@ -21,6 +22,7 @@ import com.sun.tools.javac.code.Type; import com.sun.tools.javac.code.TypeMetadata; import com.sun.tools.javac.code.Types; +import com.sun.tools.javac.tree.JCTree; import java.util.ArrayList; import java.util.Collections; import java.util.HashMap; @@ -188,6 +190,38 @@ private void reportInvalidParametersNullabilityError( errorMessage, analysis.buildDescription(paramExpression), state, null)); } + private void reportInvalidOverridingMethodReturnTypeError( + Tree methodTree, Type typeParameterType, Type methodReturnType) { + ErrorBuilder errorBuilder = analysis.getErrorBuilder(); + ErrorMessage errorMessage = + new ErrorMessage( + ErrorMessage.MessageTypes.PASS_NULLABLE_GENERIC, + "Cannot return type " + + methodReturnType + + ", as corresponding type parameter has type " + + typeParameterType + + ", which has mismatched type parameter nullability"); + state.reportMatch( + errorBuilder.createErrorDescription( + errorMessage, analysis.buildDescription(methodTree), state, null)); + } + + private void reportInvalidOverridingMethodParamTypeError( + Tree methodTree, Type typeParameterType, Type methodParamType) { + ErrorBuilder errorBuilder = analysis.getErrorBuilder(); + ErrorMessage errorMessage = + new ErrorMessage( + ErrorMessage.MessageTypes.PASS_NULLABLE_GENERIC, + "Cannot have method parameter type " + + methodParamType + + ", as corresponding type parameter has type " + + typeParameterType + + ", which has mismatched type parameter nullability"); + state.reportMatch( + errorBuilder.createErrorDescription( + errorMessage, analysis.buildDescription(methodTree), state, null)); + } + /** * This method returns the type of the given tree, including any type use annotations. * @@ -359,7 +393,6 @@ private Type.ClassType typeWithPreservedAnnotations(ParameterizedTypeTree tree) Type nullableType = NULLABLE_TYPE_SUPPLIER.get(state); List typeArguments = tree.getTypeArguments(); List newTypeArgs = new ArrayList<>(); - boolean hasNullableAnnotation = false; for (int i = 0; i < typeArguments.size(); i++) { AnnotatedTypeTree annotatedType = null; Tree curTypeArg = typeArguments.get(i); @@ -373,6 +406,7 @@ private Type.ClassType typeWithPreservedAnnotations(ParameterizedTypeTree tree) } List annotations = annotatedType != null ? annotatedType.getAnnotations() : Collections.emptyList(); + boolean hasNullableAnnotation = false; for (AnnotationTree annotation : annotations) { if (ASTHelpers.isSameType( nullableType, ASTHelpers.getType(annotation.getAnnotationType()), state)) { @@ -405,6 +439,32 @@ private Type.ClassType typeWithPreservedAnnotations(ParameterizedTypeTree tree) return finalType; } + /** + * The Nullness for the overridden method return type that is derived from the type parameter for + * the owner is by default considered as NonNull. This method computes and returns the Nullness of + * the method return type based on the Nullness of the corresponding instantiated type parameter + * for the owner + * + * @param overriddenMethod A symbol of the overridden method + * @param overridingMethodOwnerType An owner of the overriding method + * @param state Visitor state + * @param config The analysis config + * @return Returns the Nullness of the return type of the overridden method + */ + public static Nullness getOverriddenMethodReturnTypeNullness( + Symbol.MethodSymbol overriddenMethod, + Type overridingMethodOwnerType, + VisitorState state, + Config config) { + + Type overriddenMethodType = + state.getTypes().memberType(overridingMethodOwnerType, overriddenMethod); + if (!(overriddenMethodType instanceof Type.MethodType)) { + return Nullness.NONNULL; + } + return getTypeNullness(overriddenMethodType.getReturnType(), config); + } + /** * 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. @@ -504,4 +564,144 @@ public void compareGenericTypeParameterNullabilityForCall( } } } + + /** + * This method gets a method type with return type and method parameters having annotations of the + * corresponding type parameters of the owner and then use it to compare the nullability + * annotations of the return type and the method params with the corresponding type params of the + * owner. + * + * @param tree A method tree to check + * @param overridingMethod A symbol of the overriding method + * @param overriddenMethod A symbol of the overridden method + */ + public void checkTypeParameterNullnessForMethodOverriding( + MethodTree tree, Symbol.MethodSymbol overridingMethod, Symbol.MethodSymbol overriddenMethod) { + if (!config.isJSpecifyMode()) { + return; + } + // A method type with the return type and method parameters having the annotations of the type + // parameters of the + // owner if they are derived from the type parameters + Type methodWithTypeParams = + state.getTypes().memberType(overridingMethod.owner.type, overriddenMethod); + + checkTypeParameterNullnessForOverridingMethodReturnType(tree, methodWithTypeParams); + checkTypeParameterNullnessForOverridingMethodParameterType(tree, methodWithTypeParams); + } + + /** + * The Nullness for the overriding method arguments that are derived from the type parameters for + * the owner are by default considered as NonNull. This method computes and returns the Nullness + * of the method arguments based on the Nullness of the corresponding instantiated type parameters + * for the owner + * + * @param paramIndex An index of the method parameter to get the Nullness + * @param methodSymbol A symbol of the overridden method + * @param tree A method tree to check + * @return Returns Nullness of the parameterIndex th parameter of the overriding method + */ + public Nullness getOverridingMethodParamNullness( + int paramIndex, Symbol.MethodSymbol methodSymbol, Tree tree) { + JCTree.JCMethodInvocation methodInvocationTree = (JCTree.JCMethodInvocation) tree; + // if methodInvocationTree.meth is of type JCTree.JCIdent return Nullness.NONNULL + // if not in JSpecify mode then also the Nullness is set to Nullness.NONNULL, so it won't affect + // the logic outside our scope + if (!(methodInvocationTree.meth instanceof JCTree.JCFieldAccess)) { + return Nullness.NONNULL; + } + Type methodReceiverType = ((JCTree.JCFieldAccess) methodInvocationTree.meth).selected.type; + Type methodType = state.getTypes().memberType(methodReceiverType, methodSymbol); + Type formalParamType = methodType.getParameterTypes().get(paramIndex); + return getTypeNullness(formalParamType, config); + } + + /** + * The Nullness for the overridden method arguments that are derived from the type parameters for + * the owner are by default considered as NonNull. This method computes and returns the Nullness + * of the method arguments based on the Nullness of the corresponding instantiated type parameters + * for the owner + * + * @param parameterIndex An index of the method parameter to get the Nullness + * @param overriddenMethod A symbol of the overridden method + * @param overridingMethodParam A paramIndex th parameter of the overriding method + * @return Returns Nullness of the parameterIndex th parameter of the overridden method + */ + public Nullness getOverriddenMethodArgNullness( + int parameterIndex, + Symbol.MethodSymbol overriddenMethod, + Symbol.VarSymbol overridingMethodParam) { + Type methodType = + state.getTypes().memberType(overridingMethodParam.owner.owner.type, overriddenMethod); + Type paramType = methodType.getParameterTypes().get(parameterIndex); + return getTypeNullness(paramType, config); + } + + /** + * This method compares the type parameter annotations for the overriding method parameters with + * corresponding type parameters for the owner and reports an error if they don't match + * + * @param tree A method tree to check + * @param methodWithTypeParams A method type with the annotations of corresponding type parameters + * of the owner of the overriding method + */ + private void checkTypeParameterNullnessForOverridingMethodParameterType( + MethodTree tree, Type methodWithTypeParams) { + List methodParameters = tree.getParameters(); + List typeParameterTypes = methodWithTypeParams.getParameterTypes(); + for (int i = 0; i < methodParameters.size(); i++) { + Type methodParameterType = ASTHelpers.getType(methodParameters.get(i)); + Type typeParameterType = typeParameterTypes.get(i); + if (typeParameterType instanceof Type.ClassType + && methodParameterType instanceof Type.ClassType) { + // for generic types check if the nullability annotations of the type params match + boolean doTypeParamNullabilityAnnotationsMatch = + compareNullabilityAnnotations( + (Type.ClassType) typeParameterType, (Type.ClassType) methodParameterType); + + if (!doTypeParamNullabilityAnnotationsMatch) { + reportInvalidOverridingMethodParamTypeError( + methodParameters.get(i), typeParameterType, methodParameterType); + } + } + } + } + + /** + * This method compares the type parameter annotations for the overriding method return type with + * corresponding type parameters for the owner and reports an error if they don't match + * + * @param tree A method tree to check + * @param methodWithTypeParams A method type with the annotations of corresponding type parameters + * of the owner of the overriding method + */ + private void checkTypeParameterNullnessForOverridingMethodReturnType( + MethodTree tree, Type methodWithTypeParams) { + Type typeParamType = methodWithTypeParams.getReturnType(); + Type methodReturnType = ASTHelpers.getType(tree.getReturnType()); + if (!(typeParamType instanceof Type.ClassType && methodReturnType instanceof Type.ClassType)) { + return; + } + boolean doNullabilityAnnotationsMatch = + compareNullabilityAnnotations( + (Type.ClassType) typeParamType, (Type.ClassType) methodReturnType); + + if (!doNullabilityAnnotationsMatch) { + reportInvalidOverridingMethodReturnTypeError(tree, typeParamType, methodReturnType); + } + } + + /** + * @param type A type for which we need the Nullness. + * @param config The analysis config + * @return Returns the Nullness of the type based on the Nullability annotation. + */ + private static Nullness getTypeNullness(Type type, Config config) { + boolean hasNullableAnnotation = + Nullness.hasNullableAnnotation(type.getAnnotationMirrors().stream(), config); + if (hasNullableAnnotation) { + return Nullness.NULLABLE; + } + return Nullness.NONNULL; + } } diff --git a/nullaway/src/main/java/com/uber/nullaway/NullAway.java b/nullaway/src/main/java/com/uber/nullaway/NullAway.java index 51f63fcd29..3cda74eaa8 100644 --- a/nullaway/src/main/java/com/uber/nullaway/NullAway.java +++ b/nullaway/src/main/java/com/uber/nullaway/NullAway.java @@ -627,6 +627,11 @@ public Description matchMethod(MethodTree tree, VisitorState state) { Symbol.MethodSymbol closestOverriddenMethod = NullabilityUtil.getClosestOverriddenMethod(methodSymbol, state.getTypes()); if (closestOverriddenMethod != null) { + if (config.isJSpecifyMode()) { + new GenericsChecks(state, config, this) + .checkTypeParameterNullnessForMethodOverriding( + tree, methodSymbol, closestOverriddenMethod); + } return checkOverriding(closestOverriddenMethod, methodSymbol, null, state); } } @@ -722,7 +727,11 @@ private Description checkParamOverriding( overriddenMethodArgNullnessMap[i] = Nullness.paramHasNullableAnnotation(overriddenMethod, i, config) ? Nullness.NULLABLE - : Nullness.NONNULL; + : (config.isJSpecifyMode() + ? new GenericsChecks(state, config, this) + .getOverriddenMethodArgNullness( + i, overriddenMethod, overridingParamSymbols.get(i)) + : Nullness.NONNULL); } } @@ -913,7 +922,13 @@ private Description checkOverriding( Nullness overriddenMethodReturnNullness = Nullness.NULLABLE; // Permissive default for unannotated code. if (isOverriddenMethodAnnotated && !Nullness.hasNullableAnnotation(overriddenMethod, config)) { - overriddenMethodReturnNullness = Nullness.NONNULL; + if (config.isJSpecifyMode()) { + overriddenMethodReturnNullness = + GenericsChecks.getOverriddenMethodReturnTypeNullness( + overriddenMethod, overridingMethod.owner.type, state, config); + } else { + overriddenMethodReturnNullness = Nullness.NONNULL; + } } overriddenMethodReturnNullness = handler.onOverrideMethodInvocationReturnNullability( @@ -1621,7 +1636,10 @@ private Description handleInvocation( argumentPositionNullness[i] = Nullness.paramHasNullableAnnotation(methodSymbol, i, config) ? Nullness.NULLABLE - : Nullness.NONNULL; + : (config.isJSpecifyMode() + ? new GenericsChecks(state, config, this) + .getOverridingMethodParamNullness(i, methodSymbol, tree) + : Nullness.NONNULL); } } new GenericsChecks(state, config, this) diff --git a/nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPathNullnessPropagation.java b/nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPathNullnessPropagation.java index 85fa9ce61c..cecc00f8ee 100644 --- a/nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPathNullnessPropagation.java +++ b/nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPathNullnessPropagation.java @@ -33,6 +33,7 @@ import com.sun.tools.javac.code.TypeTag; import com.uber.nullaway.CodeAnnotationInfo; import com.uber.nullaway.Config; +import com.uber.nullaway.GenericsChecks; import com.uber.nullaway.NullabilityUtil; import com.uber.nullaway.Nullness; import com.uber.nullaway.handlers.Handler; @@ -1008,7 +1009,8 @@ Nullness returnValueNullness( nullness = input.getRegularStore().valueOfMethodCall(node, state, NULLABLE, apContext); } else if (node == null || methodReturnsNonNull.test(node) - || !Nullness.hasNullableAnnotation((Symbol) node.getTarget().getMethod(), config)) { + || (!Nullness.hasNullableAnnotation((Symbol) node.getTarget().getMethod(), config) + && !genericReturnIsNullable(node))) { // definite non-null return nullness = NONNULL; } else { @@ -1018,6 +1020,21 @@ Nullness returnValueNullness( return nullness; } + private boolean genericReturnIsNullable(MethodInvocationNode node) { + if (node != null && config.isJSpecifyMode()) { + if (node.getTarget() != null && node.getTarget().getMethod() != null) { + Nullness nullness = + GenericsChecks.getOverriddenMethodReturnTypeNullness( + (Symbol.MethodSymbol) ASTHelpers.getSymbol(node.getTarget().getTree()), + (Type) node.getTarget().getReceiver().getType(), + state, + config); + return nullness.equals(NULLABLE); + } + } + return false; + } + @Override public TransferResult visitObjectCreation( ObjectCreationNode objectCreationNode, TransferInput input) { diff --git a/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyGenericsTests.java b/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyGenericsTests.java index b16dd52cc7..395a89f24c 100644 --- a/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyGenericsTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyGenericsTests.java @@ -44,10 +44,6 @@ public void constructorTypeParamInstantiation() { " NonNullTypeParam t2 = new NonNullTypeParam<@Nullable String>();", " // BUG: Diagnostic contains: Generic type parameter", " testBadNonNull(new NonNullTypeParam<@Nullable String>());", - " testBadNonNull(", - " new NonNullTypeParam<", - " // BUG: Diagnostic contains: Generic type parameter", - " @Nullable String>());", " }", " static void testOkNullable(NullableTypeParam t1, NullableTypeParam<@Nullable String> t2) {", " NullableTypeParam t3 = new NullableTypeParam();", @@ -718,6 +714,258 @@ public void varargsParameter() { .doTest(); } + @Test + public void overrideReturnTypes() { + makeHelper() + .addSourceLines( + "Test.java", + "package com.uber;", + "import org.jspecify.annotations.Nullable;", + "class Test {", + " interface Fn

{", + " R apply(P p);", + " }", + " static class TestFunc1 implements Fn {", + " @Override", + " public @Nullable String apply(String s) {", + " return s;", + " }", + " }", + " static class TestFunc2 implements Fn {", + " @Override", + " public String apply(String s) {", + " return s;", + " }", + " }", + " static class TestFunc4 implements Fn<@Nullable String, String> {", + " @Override", + " // BUG: Diagnostic contains: method returns @Nullable, but superclass", + " public @Nullable String apply(String s) {", + " return s;", + " }", + " }", + " static class TestFunc3 implements Fn {", + " @Override", + " // BUG: Diagnostic contains: method returns @Nullable, but superclass", + " public @Nullable String apply(String s) {", + " return s;", + " }", + " }", + " static void useTestFunc(String s) {", + " Fn f1 = new TestFunc1();", + " String t1 = f1.apply(s);", + " // BUG: Diagnostic contains: dereferenced expression", + " t1.hashCode();", + " TestFunc2 f2 = new TestFunc2();", + " String t2 = f2.apply(s);", + " // There should not be an error here", + " t2.hashCode();", + " Fn f3 = new TestFunc2();", + " String t3 = f3.apply(s);", + " // BUG: Diagnostic contains: dereferenced expression", + " t3.hashCode();", + " }", + "}") + .doTest(); + } + + @Test + public void overrideWithNullCheck() { + makeHelper() + .addSourceLines( + "Test.java", + "package com.uber;", + "import org.jspecify.annotations.Nullable;", + "class Test {", + " interface Fn

{", + " R apply(P p);", + " }", + " static class TestFunc1 implements Fn {", + " @Override", + " public @Nullable String apply(String s) {", + " return s;", + " }", + " }", + " static void useTestFuncWithCast() {", + " Fn f1 = new TestFunc1();", + " if (f1.apply(\"hello\") != null) {", + " String t1 = f1.apply(\"hello\");", + " // no error here due to null check", + " t1.hashCode();", + " }", + " }", + "}") + .doTest(); + } + + @Test + public void overrideParameterType() { + makeHelper() + .addSourceLines( + "Test.java", + "package com.uber;", + "import org.jspecify.annotations.Nullable;", + "class Test {", + " interface Fn

{", + " R apply(P p);", + " }", + " static class TestFunc1 implements Fn<@Nullable String, String> {", + " @Override", + " // BUG: Diagnostic contains: parameter s is", + " public String apply(String s) {", + " return s;", + " }", + " }", + " static class TestFunc2 implements Fn<@Nullable String, String> {", + " @Override", + " public String apply(@Nullable String s) {", + " return \"hi\";", + " }", + " }", + " static class TestFunc3 implements Fn {", + " @Override", + " public String apply(String s) {", + " return \"hi\";", + " }", + " }", + " static class TestFunc4 implements Fn {", + " // this override is legal, we should get no error", + " @Override", + " public String apply(@Nullable String s) {", + " return \"hi\";", + " }", + " }", + " static void useTestFunc(String s) {", + " Fn<@Nullable String, String> f1 = new TestFunc2();", + " // should get no error here", + " f1.apply(null);", + " Fn f2 = new TestFunc3();", + " // BUG: Diagnostic contains: passing @Nullable parameter", + " f2.apply(null);", + " }", + "}") + .doTest(); + } + + @Test + public void nestedMethodMatch() { + makeHelper() + .addSourceLines( + "Test.java", + "package com.uber;", + "import org.jspecify.annotations.Nullable;", + "class Test {", + " class P{}", + " interface Fn< T extends P, R extends @Nullable Object> {", + " R apply(String s);", + " }", + " static class TestFunc1 implements Fn, @Nullable String> {", + " @Override", + " public String apply(String s) {", + " return s;", + " }", + " }", + " static class TestFunc2 implements Fn, @Nullable String> {", + " @Override", + " public @Nullable String apply(String s) {", + " return s;", + " }", + " }", + " static void useTestFunc(String s) {", + " Fn, @Nullable String> f1 = new TestFunc1();", + " String t1 = f1.apply(s);", + " // BUG: Diagnostic contains: dereferenced expression", + " t1.hashCode();", + " Fn, @Nullable String> f2 = new TestFunc2();", + " String t2 = f2.apply(s);", + " // BUG: Diagnostic contains: dereferenced expression", + " t2.hashCode();", + " }", + "}") + .doTest(); + } + + @Test + public void methodMatchNullableAnnotatedMethod() { + makeHelper() + .addSourceLines( + "Test.java", + "package com.uber;", + "import org.jspecify.annotations.Nullable;", + "class Test {", + " interface Fn

{", + " @Nullable R apply(P p);", + " }", + " static class TestFunc implements Fn {", + " @Override", + " //This override is fine and is handled by the current code", + " public @Nullable String apply(String s) {", + " return s;", + " }", + " }", + " static void useTestFunc(String s) {", + " Fn f = new TestFunc();", + " String t = f.apply(s);", + " // BUG: Diagnostic contains: dereferenced expression", + " t.hashCode();", + " }", + "}") + .doTest(); + } + + @Test + public void nestedMethodReturnTypeMatch() { + makeHelper() + .addSourceLines( + "Test.java", + "package com.uber;", + "import org.jspecify.annotations.Nullable;", + "class Test {", + " class P{}", + " interface Fn, R extends @Nullable Object> {", + " T apply();", + " }", + " class TestFunc1 implements Fn, @Nullable String> {", + " @Override", + " // BUG: Diagnostic contains: Cannot return", + " public P<@Nullable String, @Nullable String> apply() {", + " return new P<@Nullable String, @Nullable String>();", + " }", + " }", + " class TestFunc2 implements Fn, @Nullable String> {", + " @Override", + " // BUG: Diagnostic contains: Cannot return", + " public P<@Nullable String, String> apply() {", + " return new P<@Nullable String, String>();", + " }", + " }", + "}") + .doTest(); + } + + @Test + public void nestedMethodParamTypeMatch() { + makeHelper() + .addSourceLines( + "Test.java", + "package com.uber;", + "import org.jspecify.annotations.Nullable;", + "class Test {", + " class P{}", + " interface Fn, R extends @Nullable Object> {", + " String apply(T t, String s);", + " }", + " class TestFunc implements Fn, String> {", + " @Override", + " // BUG: Diagnostic contains: Cannot have method parameter", + " public String apply(P<@Nullable String, String> p, String s) {", + " return s;", + " }", + " }", + "}") + .doTest(); + } + private CompilationTestHelper makeHelper() { return makeTestHelperWithArgs( Arrays.asList(