From 5355c7cf167ae06cbe9e0ed8b478e37eb50fc36e Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Wed, 18 Oct 2023 13:46:18 -0700 Subject: [PATCH] JSpecify: initial handling of generic enclosing types for inner classes (#837) This is a step towards fixing #836. We now properly handle the case of a generic enclosing type for an inner class type, both in checking matching of type argument nullability and in pretty-printing for error messages. We still don't handle `NewClassTree` types correctly which is why #836 is not yet fixed. Also fixes a bug in where generics checks were performed for `return` statements. Previously we would only check generic type arguments if the top-level nullability of the return type was `@NonNull`. --- .../com/uber/nullaway/GenericsChecks.java | 130 ++++++++++-------- .../main/java/com/uber/nullaway/NullAway.java | 6 +- .../NullAwayJSpecifyGenericsTests.java | 53 ++++++- 3 files changed, 124 insertions(+), 65 deletions(-) diff --git a/nullaway/src/main/java/com/uber/nullaway/GenericsChecks.java b/nullaway/src/main/java/com/uber/nullaway/GenericsChecks.java index 045890d7d2..cfba752fb2 100644 --- a/nullaway/src/main/java/com/uber/nullaway/GenericsChecks.java +++ b/nullaway/src/main/java/com/uber/nullaway/GenericsChecks.java @@ -129,9 +129,9 @@ private static void reportInvalidAssignmentInstantiationError( ErrorMessage.MessageTypes.ASSIGN_GENERIC_NULLABLE, String.format( "Cannot assign from type " - + prettyTypeForError(rhsType) + + prettyTypeForError(rhsType, state) + " to type " - + prettyTypeForError(lhsType) + + prettyTypeForError(lhsType, state) + " due to mismatched nullability of type parameters")); state.reportMatch( errorBuilder.createErrorDescription( @@ -146,9 +146,9 @@ private static void reportInvalidReturnTypeError( ErrorMessage.MessageTypes.RETURN_NULLABLE_GENERIC, String.format( "Cannot return expression of type " - + prettyTypeForError(returnType) + + prettyTypeForError(returnType, state) + " from method with return type " - + prettyTypeForError(methodType) + + prettyTypeForError(methodType, state) + " due to mismatched nullability of type parameters")); state.reportMatch( errorBuilder.createErrorDescription( @@ -163,9 +163,9 @@ private static void reportMismatchedTypeForTernaryOperator( ErrorMessage.MessageTypes.ASSIGN_GENERIC_NULLABLE, String.format( "Conditional expression must have type " - + prettyTypeForError(expressionType) + + prettyTypeForError(expressionType, state) + " but the sub-expression has type " - + prettyTypeForError(subPartType) + + prettyTypeForError(subPartType, state) + ", which has mismatched nullability of type parameters")); state.reportMatch( errorBuilder.createErrorDescription( @@ -183,9 +183,9 @@ private static void reportInvalidParametersNullabilityError( new ErrorMessage( ErrorMessage.MessageTypes.PASS_NULLABLE_GENERIC, "Cannot pass parameter of type " - + prettyTypeForError(actualParameterType) + + prettyTypeForError(actualParameterType, state) + ", as formal parameter has type " - + prettyTypeForError(formalParameterType) + + prettyTypeForError(formalParameterType, state) + ", which has mismatched type parameter nullability"); state.reportMatch( errorBuilder.createErrorDescription( @@ -203,9 +203,9 @@ private static void reportInvalidOverridingMethodReturnTypeError( new ErrorMessage( ErrorMessage.MessageTypes.WRONG_OVERRIDE_RETURN_GENERIC, "Method returns " - + prettyTypeForError(overridingMethodReturnType) + + prettyTypeForError(overridingMethodReturnType, state) + ", but overridden method returns " - + prettyTypeForError(overriddenMethodReturnType) + + prettyTypeForError(overriddenMethodReturnType, state) + ", which has mismatched type parameter nullability"); state.reportMatch( errorBuilder.createErrorDescription( @@ -223,9 +223,9 @@ private static void reportInvalidOverridingMethodParamTypeError( new ErrorMessage( ErrorMessage.MessageTypes.WRONG_OVERRIDE_PARAM_GENERIC, "Parameter has type " - + prettyTypeForError(methodParamType) + + prettyTypeForError(methodParamType, state) + ", but overridden method has parameter type " - + prettyTypeForError(typeParameterType) + + prettyTypeForError(typeParameterType, state) + ", which has mismatched type parameter nullability"); state.reportMatch( errorBuilder.createErrorDescription( @@ -546,7 +546,10 @@ public Boolean visitClassType(Type.ClassType lhsType, Type rhsType) { return false; } } - return true; + // If there is an enclosing type (for non-static inner classes), its type argument nullability + // should also match. When there is no enclosing type, getEnclosingType() returns a NoType + // object, which gets handled by the fallback visitType() method + return lhsType.getEnclosingType().accept(this, rhsType.getEnclosingType()); } @Override @@ -992,57 +995,66 @@ private static Nullness getTypeNullness(Type type, Config config) { * Returns a pretty-printed representation of type suitable for error messages. The representation * uses simple names rather than fully-qualified names, and retains all type-use annotations. */ - public static String prettyTypeForError(Type type) { - return type.accept(PRETTY_TYPE_VISITOR, null); + public static String prettyTypeForError(Type type, VisitorState state) { + return type.accept(new PrettyTypeVisitor(state), null); } /** This code is a modified version of code in {@link com.google.errorprone.util.Signatures} */ - private static final Type.Visitor PRETTY_TYPE_VISITOR = - new Types.DefaultTypeVisitor() { - @Override - public String visitWildcardType(Type.WildcardType t, Void unused) { - StringBuilder sb = new StringBuilder(); - sb.append(t.kind); - if (t.kind != BoundKind.UNBOUND) { - sb.append(t.type.accept(this, null)); - } - return sb.toString(); - } + private static final class PrettyTypeVisitor extends Types.DefaultTypeVisitor { - @Override - public String visitClassType(Type.ClassType t, Void s) { - StringBuilder sb = new StringBuilder(); - for (Attribute.TypeCompound compound : t.getAnnotationMirrors()) { - sb.append('@'); - sb.append(compound.type.accept(this, null)); - sb.append(' '); - } - sb.append(t.tsym.getSimpleName()); - if (t.getTypeArguments().nonEmpty()) { - sb.append('<'); - sb.append( - t.getTypeArguments().stream() - .map(a -> a.accept(this, null)) - .collect(joining(", "))); - sb.append(">"); - } - return sb.toString(); - } + private final VisitorState state; - @Override - public String visitCapturedType(Type.CapturedType t, Void s) { - return t.wildcard.accept(this, null); - } + PrettyTypeVisitor(VisitorState state) { + this.state = state; + } - @Override - public String visitArrayType(Type.ArrayType t, Void unused) { - // TODO properly print cases like int @Nullable[] - return t.elemtype.accept(this, null) + "[]"; - } + @Override + public String visitWildcardType(Type.WildcardType t, Void unused) { + // NOTE: we have not tested this code yet as we do not yet support wildcard types + StringBuilder sb = new StringBuilder(); + sb.append(t.kind); + if (t.kind != BoundKind.UNBOUND) { + sb.append(t.type.accept(this, null)); + } + return sb.toString(); + } - @Override - public String visitType(Type t, Void s) { - return t.toString(); - } - }; + @Override + public String visitClassType(Type.ClassType t, Void s) { + StringBuilder sb = new StringBuilder(); + Type enclosingType = t.getEnclosingType(); + if (!ASTHelpers.isSameType(enclosingType, Type.noType, state)) { + sb.append(enclosingType.accept(this, null)).append('.'); + } + for (Attribute.TypeCompound compound : t.getAnnotationMirrors()) { + sb.append('@'); + sb.append(compound.type.accept(this, null)); + sb.append(' '); + } + sb.append(t.tsym.getSimpleName()); + if (t.getTypeArguments().nonEmpty()) { + sb.append('<'); + sb.append( + t.getTypeArguments().stream().map(a -> a.accept(this, null)).collect(joining(", "))); + sb.append(">"); + } + return sb.toString(); + } + + @Override + public String visitCapturedType(Type.CapturedType t, Void s) { + return t.wildcard.accept(this, null); + } + + @Override + public String visitArrayType(Type.ArrayType t, Void unused) { + // TODO properly print cases like int @Nullable[] + return t.elemtype.accept(this, null) + "[]"; + } + + @Override + public String visitType(Type t, Void s) { + return t.toString(); + } + } } diff --git a/nullaway/src/main/java/com/uber/nullaway/NullAway.java b/nullaway/src/main/java/com/uber/nullaway/NullAway.java index 2da65c8eeb..3a1f328cd5 100644 --- a/nullaway/src/main/java/com/uber/nullaway/NullAway.java +++ b/nullaway/src/main/java/com/uber/nullaway/NullAway.java @@ -867,6 +867,10 @@ 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 + GenericsChecks.checkTypeParameterNullnessForFunctionReturnType( + retExpr, methodSymbol, this, state); if (getMethodReturnNullness(methodSymbol, state, Nullness.NULLABLE).equals(Nullness.NULLABLE)) { return Description.NO_MATCH; } @@ -880,8 +884,6 @@ private Description checkReturnExpression( state, methodSymbol); } - GenericsChecks.checkTypeParameterNullnessForFunctionReturnType( - retExpr, methodSymbol, this, state); 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 f0e8827041..b4d9ff1137 100644 --- a/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyGenericsTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyGenericsTests.java @@ -192,6 +192,51 @@ public void testOKNewClassInstantiationForOtherAnnotations() { .doTest(); } + @Test + public void nestedGenericTypes() { + makeHelper() + .addSourceLines( + "Test.java", + "package com.uber;", + "import org.jspecify.annotations.Nullable;", + "class Test {", + " class Wrapper

{", + " abstract class Fn {", + " abstract R apply(P p);", + " }", + " }", + " static void param(@Nullable Wrapper.Fn p) {}", + " static void positiveParam() {", + " Wrapper<@Nullable String>.Fn x = null;", + " // BUG: Diagnostic contains: Cannot pass parameter of type Test.Wrapper<@Nullable String>.Fn", + " param(x);", + " }", + " static void positiveAssign() {", + " Wrapper<@Nullable String>.Fn p1 = null;", + " // BUG: Diagnostic contains: Cannot assign from type Test.Wrapper<@Nullable String>.Fn to type Test.Wrapper.Fn", + " Wrapper.Fn p2 = p1;", + " }", + " static @Nullable Wrapper.Fn positiveReturn() {", + " Wrapper<@Nullable String>.Fn p1 = null;", + " // BUG: Diagnostic contains: Cannot return expression of type Test.Wrapper<@Nullable String>.Fn", + " return p1;", + " }", + " static void negativeParam() {", + " Wrapper.Fn x = null;", + " param(x);", + " }", + " static void negativeAssign() {", + " Wrapper<@Nullable String>.Fn p1 = null;", + " Wrapper<@Nullable String>.Fn p2 = p1;", + " }", + " static @Nullable Wrapper<@Nullable String>.Fn negativeReturn() {", + " Wrapper<@Nullable String>.Fn p1 = null;", + " return p1;", + " }", + "}") + .doTest(); + } + @Test public void downcastInstantiation() { makeHelper() @@ -280,7 +325,7 @@ public void superTypeAssignmentChecksSingleInterface() { " interface Fn

{}", " class FnImpl implements Fn<@Nullable String, @Nullable String> {}", " void testPositive() {", - " // BUG: Diagnostic contains: Cannot assign from type FnImpl", + " // BUG: Diagnostic contains: Cannot assign from type Test.FnImpl", " Fn<@Nullable String, String> f = new FnImpl();", " }", " void testNegative() {", @@ -1173,14 +1218,14 @@ public void overrideWithNestedTypeParametersInReturnType() { " }", " class TestFunc1 implements Fn, @Nullable String> {", " @Override", - " // BUG: Diagnostic contains: Method returns P<@Nullable String, @Nullable String>, but overridden method", + " // BUG: Diagnostic contains: Method returns Test.P<@Nullable String, @Nullable String>, but overridden method", " public P<@Nullable String, @Nullable String> apply() {", " return new P<@Nullable String, @Nullable String>();", " }", " }", " class TestFunc2 implements Fn, @Nullable String> {", " @Override", - " // BUG: Diagnostic contains: Method returns P<@Nullable String, String>, but overridden method returns", + " // BUG: Diagnostic contains: Method returns Test.P<@Nullable String, String>, but overridden method returns", " public P<@Nullable String, String> apply() {", " return new P<@Nullable String, String>();", " }", @@ -1209,7 +1254,7 @@ public void overrideWithNestedTypeParametersInParameterType() { " }", " class TestFunc implements Fn, String> {", " @Override", - " // BUG: Diagnostic contains: Parameter has type P<@Nullable String, String>, but overridden method has parameter type P", + " // BUG: Diagnostic contains: Parameter has type Test.P<@Nullable String, String>, but overridden method has parameter type Test.P", " public String apply(P<@Nullable String, String> p, String s) {", " return s;", " }",