diff options
author | Manu Sridharan <msridhar@gmail.com> | 2023-10-18 13:46:18 -0700 |
---|---|---|
committer | GitHub <noreply@github.com> | 2023-10-18 20:46:18 +0000 |
commit | 5355c7cf167ae06cbe9e0ed8b478e37eb50fc36e (patch) | |
tree | f6821b1c289777064b8b509943e898b8c9c744cb | |
parent | e7623f74b667cbeb88fca6a0be7c2af6d111df59 (diff) | |
download | nullaway-5355c7cf167ae06cbe9e0ed8b478e37eb50fc36e.tar.gz |
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`.
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 045890d..cfba752 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 @@ public final class GenericsChecks { 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 @@ public final class GenericsChecks { 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 @@ public final class GenericsChecks { 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 @@ public final class GenericsChecks { 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 @@ public final class GenericsChecks { 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 @@ public final class GenericsChecks { 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 final class GenericsChecks { 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 @@ public final class GenericsChecks { * 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<String, Void> PRETTY_TYPE_VISITOR = - new Types.DefaultTypeVisitor<String, Void>() { - @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<String, Void> { - @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 2da65c8..3a1f328 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 @@ public class NullAway extends BugChecker // 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 @@ public class NullAway extends BugChecker 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 f0e8827..b4d9ff1 100644 --- a/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyGenericsTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyGenericsTests.java @@ -193,6 +193,51 @@ public class NullAwayJSpecifyGenericsTests extends NullAwayTestsBase { } @Test + public void nestedGenericTypes() { + makeHelper() + .addSourceLines( + "Test.java", + "package com.uber;", + "import org.jspecify.annotations.Nullable;", + "class Test {", + " class Wrapper<P extends @Nullable Object> {", + " abstract class Fn<R extends @Nullable Object> {", + " abstract R apply(P p);", + " }", + " }", + " static void param(@Nullable Wrapper<String>.Fn<String> p) {}", + " static void positiveParam() {", + " Wrapper<@Nullable String>.Fn<String> x = null;", + " // BUG: Diagnostic contains: Cannot pass parameter of type Test.Wrapper<@Nullable String>.Fn<String>", + " param(x);", + " }", + " static void positiveAssign() {", + " Wrapper<@Nullable String>.Fn<String> p1 = null;", + " // BUG: Diagnostic contains: Cannot assign from type Test.Wrapper<@Nullable String>.Fn<String> to type Test.Wrapper<String>.Fn<String>", + " Wrapper<String>.Fn<String> p2 = p1;", + " }", + " static @Nullable Wrapper<String>.Fn<String> positiveReturn() {", + " Wrapper<@Nullable String>.Fn<String> p1 = null;", + " // BUG: Diagnostic contains: Cannot return expression of type Test.Wrapper<@Nullable String>.Fn<String>", + " return p1;", + " }", + " static void negativeParam() {", + " Wrapper<String>.Fn<String> x = null;", + " param(x);", + " }", + " static void negativeAssign() {", + " Wrapper<@Nullable String>.Fn<String> p1 = null;", + " Wrapper<@Nullable String>.Fn<String> p2 = p1;", + " }", + " static @Nullable Wrapper<@Nullable String>.Fn<String> negativeReturn() {", + " Wrapper<@Nullable String>.Fn<String> p1 = null;", + " return p1;", + " }", + "}") + .doTest(); + } + + @Test public void downcastInstantiation() { makeHelper() .addSourceLines( @@ -280,7 +325,7 @@ public class NullAwayJSpecifyGenericsTests extends NullAwayTestsBase { " interface Fn<P extends @Nullable Object, R extends @Nullable Object> {}", " 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 class NullAwayJSpecifyGenericsTests extends NullAwayTestsBase { " }", " class TestFunc1 implements Fn<P<@Nullable String, String>, @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<P<@Nullable String, @Nullable String>, @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 class NullAwayJSpecifyGenericsTests extends NullAwayTestsBase { " }", " class TestFunc implements Fn<P<String, String>, String> {", " @Override", - " // BUG: Diagnostic contains: Parameter has type P<@Nullable String, String>, but overridden method has parameter type P<String, String>", + " // BUG: Diagnostic contains: Parameter has type Test.P<@Nullable String, String>, but overridden method has parameter type Test.P<String, String>", " public String apply(P<@Nullable String, String> p, String s) {", " return s;", " }", |