From 9092438609894d96ea69db892e4b69452eeee7a9 Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Tue, 14 Nov 2023 18:29:50 -0800 Subject: [PATCH] Clarifications and small fixes for checking JSpecify @Nullable annotation (#859) Rename a variable and add docs to clarify that in certain places, our JSpecify support specifically checks for `@org.jspecify.annotations.Nullable` annotations and not others. Also, fix a couple of places where we were comparing types by their `String` representation. --- .../generics/CompareNullabilityVisitor.java | 9 ++++++--- .../nullaway/generics/GenericsChecks.java | 10 +++++++--- .../PreservedAnnotationTreeVisitor.java | 2 +- .../NullAwayJSpecifyGenericsTests.java | 20 +++++++++++++++++++ 4 files changed, 34 insertions(+), 7 deletions(-) diff --git a/nullaway/src/main/java/com/uber/nullaway/generics/CompareNullabilityVisitor.java b/nullaway/src/main/java/com/uber/nullaway/generics/CompareNullabilityVisitor.java index bd34553ab7..760d2008ad 100644 --- a/nullaway/src/main/java/com/uber/nullaway/generics/CompareNullabilityVisitor.java +++ b/nullaway/src/main/java/com/uber/nullaway/generics/CompareNullabilityVisitor.java @@ -1,6 +1,7 @@ package com.uber.nullaway.generics; import com.google.errorprone.VisitorState; +import com.google.errorprone.util.ASTHelpers; import com.sun.tools.javac.code.Attribute; import com.sun.tools.javac.code.Type; import com.sun.tools.javac.code.Types; @@ -43,17 +44,19 @@ public Boolean visitClassType(Type.ClassType lhsType, Type rhsType) { boolean isLHSNullableAnnotated = false; List lhsAnnotations = lhsTypeArgument.getAnnotationMirrors(); // To ensure that we are checking only jspecify nullable annotations + Type jspecifyNullableType = GenericsChecks.JSPECIFY_NULLABLE_TYPE_SUPPLIER.get(state); for (Attribute.TypeCompound annotation : lhsAnnotations) { - if (annotation.getAnnotationType().toString().equals(GenericsChecks.NULLABLE_NAME)) { + if (ASTHelpers.isSameType( + (Type) annotation.getAnnotationType(), jspecifyNullableType, state)) { isLHSNullableAnnotated = true; break; } } boolean isRHSNullableAnnotated = false; List rhsAnnotations = rhsTypeArgument.getAnnotationMirrors(); - // To ensure that we are checking only jspecify nullable annotations for (Attribute.TypeCompound annotation : rhsAnnotations) { - if (annotation.getAnnotationType().toString().equals(GenericsChecks.NULLABLE_NAME)) { + if (ASTHelpers.isSameType( + (Type) annotation.getAnnotationType(), jspecifyNullableType, state)) { isRHSNullableAnnotated = true; break; } diff --git a/nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java b/nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java index 37a56f6d3d..247988b3b8 100644 --- a/nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java +++ b/nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java @@ -38,9 +38,13 @@ /** Methods for performing checks related to generic types and nullability. */ public final class GenericsChecks { - static final String NULLABLE_NAME = "org.jspecify.annotations.Nullable"; - - static final Supplier NULLABLE_TYPE_SUPPLIER = Suppliers.typeFromString(NULLABLE_NAME); + /** + * Supplier for the JSpecify {@code @Nullable} annotation. Required since for now, certain checks + * related to generics specifically look for {@code @org.jspecify.ananotations.Nullable} + * annotations and do not apply to other {@code @Nullable} annotations. + */ + static final Supplier JSPECIFY_NULLABLE_TYPE_SUPPLIER = + Suppliers.typeFromString("org.jspecify.annotations.Nullable"); /** Do not instantiate; all methods should be static */ private GenericsChecks() {} diff --git a/nullaway/src/main/java/com/uber/nullaway/generics/PreservedAnnotationTreeVisitor.java b/nullaway/src/main/java/com/uber/nullaway/generics/PreservedAnnotationTreeVisitor.java index e5971d5c3a..2cde64f0ac 100644 --- a/nullaway/src/main/java/com/uber/nullaway/generics/PreservedAnnotationTreeVisitor.java +++ b/nullaway/src/main/java/com/uber/nullaway/generics/PreservedAnnotationTreeVisitor.java @@ -42,7 +42,7 @@ public Type visitArrayType(ArrayTypeTree tree, Void p) { public Type visitParameterizedType(ParameterizedTypeTree tree, Void p) { Type.ClassType type = (Type.ClassType) ASTHelpers.getType(tree); Preconditions.checkNotNull(type); - Type nullableType = GenericsChecks.NULLABLE_TYPE_SUPPLIER.get(state); + Type nullableType = GenericsChecks.JSPECIFY_NULLABLE_TYPE_SUPPLIER.get(state); List typeArguments = tree.getTypeArguments(); List newTypeArgs = new ArrayList<>(); for (int i = 0; i < typeArguments.size(); i++) { diff --git a/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyGenericsTests.java b/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyGenericsTests.java index eb7c02fe65..68f1d11366 100644 --- a/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyGenericsTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyGenericsTests.java @@ -291,6 +291,26 @@ public void genericsChecksForAssignments() { .doTest(); } + @Test + public void genericsChecksForAssignmentsWithNonJSpecifyAnnotations() { + makeHelper() + .addSourceLines( + "Test.java", + "package com.uber;", + "import org.checkerframework.checker.nullness.qual.Nullable;", + "class Test {", + " static class NullableTypeParam {}", + " static void testNoWarningForMismatch(NullableTypeParam<@Nullable String> t1) {", + " // no error here since we only do our checks for JSpecify @Nullable annotations", + " NullableTypeParam t2 = t1;", + " }", + " static void testNegative(NullableTypeParam<@Nullable String> t1) {", + " NullableTypeParam<@Nullable String> t2 = t1;", + " }", + "}") + .doTest(); + } + @Test public void nestedChecksForAssignmentsMultipleArguments() { makeHelper()