diff options
author | Cole Faust <colefaust@google.com> | 2023-12-01 01:04:56 +0000 |
---|---|---|
committer | Automerger Merge Worker <android-build-automerger-merge-worker@system.gserviceaccount.com> | 2023-12-01 01:04:56 +0000 |
commit | 3b75672bbf467ab6abbbdf5a9fcda9f5620e17a8 (patch) | |
tree | fb113f769bca094b0691db54a9d0f4a354a37b88 | |
parent | 678b3b1539c1dc4621524e914d00866aaa14a91f (diff) | |
parent | 3486bbdf92d21d6b3ae1f7f7eb35ff4e7126a8f0 (diff) | |
download | nullaway-3b75672bbf467ab6abbbdf5a9fcda9f5620e17a8.tar.gz |
Merge commit 'bf74867fbb7b3dd4ea1539060150036635b772a3' into update_for_crash am: 6390a8d8dc am: 3486bbdf92
Original change: https://android-review.googlesource.com/c/platform/external/nullaway/+/2854014
Change-Id: I910225e842c4d9e6bde5222fa4f41209fe23f696
Signed-off-by: Automerger Merge Worker <android-build-automerger-merge-worker@system.gserviceaccount.com>
17 files changed, 449 insertions, 270 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md index c7a8e0c..eda8659 100755 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,16 @@ Changelog ========= +Version 0.10.17 +--------------- +* Fix bug with computing direct type use annotations on parameters (#864) +* Model Apache Flink's RichFunction.open as an @Initializer method (#862) +* Support for JSpecify's 0.3.0 annotation [experimental] + - JSpecify: adding com.google.common to annotated packages in build.gradle (#857) + - JSpecify: handling the return of a diamond operator anonymous object method caller (#858) + - Create com.uber.nullaway.generics package (#855) + - Clarifications and small fixes for checking JSpecify @Nullable annotation (#859) + - Apply minor cleanups suggested by IntelliJ in generics code (#860) + Version 0.10.16 --------------- NOTE: Maven Central signing key rotated for this release following a revocation. diff --git a/gradle.properties b/gradle.properties index 3b5fd0c..fe3f54f 100644 --- a/gradle.properties +++ b/gradle.properties @@ -12,7 +12,7 @@ org.gradle.caching=true org.gradle.jvmargs=-Xmx2g -XX:MaxMetaspaceSize=512m GROUP=com.uber.nullaway -VERSION_NAME=0.10.16 +VERSION_NAME=0.10.18-SNAPSHOT POM_DESCRIPTION=A fast annotation-based null checker for Java diff --git a/gradle/dependencies.gradle b/gradle/dependencies.gradle index 1af2c45..566b5cf 100755 --- a/gradle/dependencies.gradle +++ b/gradle/dependencies.gradle @@ -102,7 +102,7 @@ def test = [ "org.junit.jupiter:junit-jupiter-api:5.0.2", "org.apiguardian:apiguardian-api:1.0.0" ], - jetbrainsAnnotations : "org.jetbrains:annotations:13.0", + jetbrainsAnnotations : "org.jetbrains:annotations:24.1.0", cfQual : "org.checkerframework:checker-qual:${versions.checkerFramework}", // 2.5.5 is the last release to contain this artifact cfCompatQual : "org.checkerframework:checker-compat-qual:2.5.5", diff --git a/nullaway/build.gradle b/nullaway/build.gradle index 8000dbc..90ac062 100644 --- a/nullaway/build.gradle +++ b/nullaway/build.gradle @@ -152,7 +152,7 @@ tasks.register('buildWithNullAway', JavaCompile) { sourceSets.main.compileClasspath) options.errorprone.enabled = true options.errorprone { - option("NullAway:AnnotatedPackages", "com.uber,org.checkerframework.nullaway") + option("NullAway:AnnotatedPackages", "com.uber,org.checkerframework.nullaway,com.google.common") option("NullAway:CastToNonNullMethod", "com.uber.nullaway.NullabilityUtil.castToNonNull") option("NullAway:CheckOptionalEmptiness") option("NullAway:AcknowledgeRestrictiveAnnotations") diff --git a/nullaway/src/main/java/com/uber/nullaway/ErrorBuilder.java b/nullaway/src/main/java/com/uber/nullaway/ErrorBuilder.java index fb7aee6..e60b798 100755 --- a/nullaway/src/main/java/com/uber/nullaway/ErrorBuilder.java +++ b/nullaway/src/main/java/com/uber/nullaway/ErrorBuilder.java @@ -92,7 +92,7 @@ public class ErrorBuilder { * expression into a @NonNull target, and this parameter is the Symbol for that target. * @return the error description */ - Description createErrorDescription( + public Description createErrorDescription( ErrorMessage errorMessage, Description.Builder descriptionBuilder, VisitorState state, diff --git a/nullaway/src/main/java/com/uber/nullaway/ErrorProneCLIFlagsConfig.java b/nullaway/src/main/java/com/uber/nullaway/ErrorProneCLIFlagsConfig.java index 9a189fe..0240d77 100644 --- a/nullaway/src/main/java/com/uber/nullaway/ErrorProneCLIFlagsConfig.java +++ b/nullaway/src/main/java/com/uber/nullaway/ErrorProneCLIFlagsConfig.java @@ -130,7 +130,11 @@ final class ErrorProneCLIFlagsConfig extends AbstractConfig { "androidx.fragment.app.Fragment.onActivityCreated", "androidx.fragment.app.Fragment.onViewCreated", // Multidex app - "android.support.multidex.Application.onCreate"); + "android.support.multidex.Application.onCreate", + // Apache Flink + // See docs: + // https://nightlies.apache.org/flink/flink-docs-master/api/java/org/apache/flink/api/common/functions/RichFunction.html#open-org.apache.flink.api.common.functions.OpenContext- + "org.apache.flink.api.common.functions.RichFunction.open"); static final ImmutableSet<String> DEFAULT_INITIALIZER_ANNOT = ImmutableSet.of( diff --git a/nullaway/src/main/java/com/uber/nullaway/NullAway.java b/nullaway/src/main/java/com/uber/nullaway/NullAway.java index ff4ee80..744bddd 100644 --- a/nullaway/src/main/java/com/uber/nullaway/NullAway.java +++ b/nullaway/src/main/java/com/uber/nullaway/NullAway.java @@ -92,6 +92,7 @@ import com.sun.tools.javac.tree.JCTree; import com.uber.nullaway.ErrorMessage.MessageTypes; import com.uber.nullaway.dataflow.AccessPathNullnessAnalysis; import com.uber.nullaway.dataflow.EnclosingEnvironmentNullness; +import com.uber.nullaway.generics.GenericsChecks; import com.uber.nullaway.handlers.Handler; import com.uber.nullaway.handlers.Handlers; import java.util.ArrayList; diff --git a/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java b/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java index 2a04d46..cca114b 100644 --- a/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java +++ b/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java @@ -175,7 +175,8 @@ public class NullabilityUtil { /** * NOTE: this method does not work for getting all annotations of parameters of methods from class - * files. For that case, use {@link #getAllAnnotationsForParameter(Symbol.MethodSymbol, int)} + * files. For that case, use {@link #getAllAnnotationsForParameter(Symbol.MethodSymbol, int, + * Config)} * * @param symbol the symbol * @return all annotations on the symbol and on the type of the symbol @@ -259,10 +260,11 @@ public class NullabilityUtil { * * @param symbol the method symbol * @param paramInd index of the parameter + * @param config NullAway configuration * @return all declaration and type-use annotations for the parameter */ public static Stream<? extends AnnotationMirror> getAllAnnotationsForParameter( - Symbol.MethodSymbol symbol, int paramInd) { + Symbol.MethodSymbol symbol, int paramInd, Config config) { Symbol.VarSymbol varSymbol = symbol.getParameters().get(paramInd); return Stream.concat( varSymbol.getAnnotationMirrors().stream(), @@ -270,7 +272,8 @@ public class NullabilityUtil { .filter( t -> t.position.type.equals(TargetType.METHOD_FORMAL_PARAMETER) - && t.position.parameter_index == paramInd)); + && t.position.parameter_index == paramInd + && NullabilityUtil.isDirectTypeUseAnnotation(t, config))); } /** diff --git a/nullaway/src/main/java/com/uber/nullaway/Nullness.java b/nullaway/src/main/java/com/uber/nullaway/Nullness.java index 845d547..0ed694b 100644 --- a/nullaway/src/main/java/com/uber/nullaway/Nullness.java +++ b/nullaway/src/main/java/com/uber/nullaway/Nullness.java @@ -218,7 +218,7 @@ public enum Nullness implements AbstractValue<Nullness> { return true; } return hasNullableAnnotation( - NullabilityUtil.getAllAnnotationsForParameter(symbol, paramInd), config); + NullabilityUtil.getAllAnnotationsForParameter(symbol, paramInd, config), config); } private static boolean isRecordEqualsParam(Symbol.MethodSymbol symbol, int paramInd) { @@ -249,6 +249,6 @@ public enum Nullness implements AbstractValue<Nullness> { public static boolean paramHasNonNullAnnotation( Symbol.MethodSymbol symbol, int paramInd, Config config) { return hasNonNullAnnotation( - NullabilityUtil.getAllAnnotationsForParameter(symbol, paramInd), config); + NullabilityUtil.getAllAnnotationsForParameter(symbol, paramInd, config), config); } } 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 36f35e4..b68665a 100644 --- a/nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPathNullnessPropagation.java +++ b/nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPathNullnessPropagation.java @@ -25,6 +25,7 @@ import static javax.lang.model.element.ElementKind.EXCEPTION_PARAMETER; import static org.checkerframework.nullaway.javacutil.TreeUtils.elementFromDeclaration; import com.google.common.base.Preconditions; +import com.google.common.base.VerifyException; import com.google.errorprone.VisitorState; import com.google.errorprone.suppliers.Supplier; import com.google.errorprone.suppliers.Suppliers; @@ -35,9 +36,9 @@ import com.sun.tools.javac.code.Type; 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.generics.GenericsChecks; import com.uber.nullaway.handlers.Handler; import com.uber.nullaway.handlers.Handler.NullnessHint; import java.util.HashMap; @@ -578,7 +579,7 @@ public class AccessPathNullnessPropagation if (mapWithIteratorContentsKey != null) { // put sanity check here to minimize perf impact if (!isCallToMethod(rhsInv, SET_TYPE_SUPPLIER, "iterator")) { - throw new RuntimeException( + throw new VerifyException( "expected call to iterator(), instead saw " + state.getSourceForNode(rhsInv.getTree())); } @@ -603,7 +604,7 @@ public class AccessPathNullnessPropagation if (mapGetPath != null) { // put sanity check here to minimize perf impact if (!isCallToMethod(methodInv, ITERATOR_TYPE_SUPPLIER, "next")) { - throw new RuntimeException( + throw new VerifyException( "expected call to next(), instead saw " + state.getSourceForNode(methodInv.getTree())); } @@ -633,14 +634,32 @@ public class AccessPathNullnessPropagation return null; } + /** + * Checks if an invocation node represents a call to a method on a given type + * + * @param invocationNode the invocation node + * @param containingTypeSupplier supplier for the type containing the method + * @param methodName name of the method + * @return true if the invocation node represents a call to the method on the type + */ private boolean isCallToMethod( MethodInvocationNode invocationNode, Supplier<Type> containingTypeSupplier, String methodName) { - Symbol.MethodSymbol symbol = ASTHelpers.getSymbol(invocationNode.getTree()); - return symbol != null - && symbol.getSimpleName().contentEquals(methodName) - && ASTHelpers.isSubtype(symbol.owner.type, containingTypeSupplier.get(state), state); + MethodInvocationTree invocationTree = invocationNode.getTree(); + Symbol.MethodSymbol symbol = ASTHelpers.getSymbol(invocationTree); + if (symbol != null && symbol.getSimpleName().contentEquals(methodName)) { + // NOTE: previously we checked if symbol.owner.type was a subtype of the containing type. + // However, symbol.owner.type refers to the static type at the call site, in which the target + // class/interface might be a supertype of the containing type with some Java compilers. + // Instead, we now check if the static type of the receiver at the invocation is a subtype of + // the containing type (as this guarantees a method in the containing type or one of its + // subtypes will be invoked, assuming such a method exists). See + // https://github.com/uber/NullAway/issues/866. + return ASTHelpers.isSubtype( + ASTHelpers.getReceiverType(invocationTree), containingTypeSupplier.get(state), state); + } + return false; } /** diff --git a/nullaway/src/main/java/com/uber/nullaway/generics/CompareNullabilityVisitor.java b/nullaway/src/main/java/com/uber/nullaway/generics/CompareNullabilityVisitor.java new file mode 100644 index 0000000..025c3ee --- /dev/null +++ b/nullaway/src/main/java/com/uber/nullaway/generics/CompareNullabilityVisitor.java @@ -0,0 +1,88 @@ +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; +import java.util.List; + +/** + * Visitor that checks equality of nullability annotations for all nested generic type arguments + * within a type. Compares the Type it is called upon, i.e. the LHS type and the Type passed as an + * argument, i.e. The RHS type. + */ +public class CompareNullabilityVisitor extends Types.DefaultTypeVisitor<Boolean, Type> { + private final VisitorState state; + + CompareNullabilityVisitor(VisitorState state) { + this.state = state; + } + + @Override + public Boolean visitClassType(Type.ClassType lhsType, Type rhsType) { + Types types = state.getTypes(); + // The base type of rhsType may be a subtype of lhsType's base type. In such cases, we must + // compare lhsType against the supertype of rhsType with a matching base type. + rhsType = types.asSuper(rhsType, lhsType.tsym); + // This is impossible, considering the fact that standard Java subtyping succeeds before + // running NullAway + if (rhsType == null) { + throw new RuntimeException("Did not find supertype of " + rhsType + " matching " + lhsType); + } + List<Type> lhsTypeArguments = lhsType.getTypeArguments(); + List<Type> rhsTypeArguments = rhsType.getTypeArguments(); + // This is impossible, considering the fact that standard Java subtyping succeeds before + // running NullAway + if (lhsTypeArguments.size() != rhsTypeArguments.size()) { + throw new RuntimeException( + "Number of types arguments in " + rhsType + " does not match " + lhsType); + } + for (int i = 0; i < lhsTypeArguments.size(); i++) { + Type lhsTypeArgument = lhsTypeArguments.get(i); + Type rhsTypeArgument = rhsTypeArguments.get(i); + boolean isLHSNullableAnnotated = false; + List<Attribute.TypeCompound> 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 (ASTHelpers.isSameType( + (Type) annotation.getAnnotationType(), jspecifyNullableType, state)) { + isLHSNullableAnnotated = true; + break; + } + } + boolean isRHSNullableAnnotated = false; + List<Attribute.TypeCompound> rhsAnnotations = rhsTypeArgument.getAnnotationMirrors(); + for (Attribute.TypeCompound annotation : rhsAnnotations) { + if (ASTHelpers.isSameType( + (Type) annotation.getAnnotationType(), jspecifyNullableType, state)) { + isRHSNullableAnnotated = true; + break; + } + } + if (isLHSNullableAnnotated != isRHSNullableAnnotated) { + return false; + } + // nested generics + if (!lhsTypeArgument.accept(this, rhsTypeArgument)) { + return false; + } + } + // 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 + public Boolean visitArrayType(Type.ArrayType lhsType, Type rhsType) { + Type.ArrayType arrRhsType = (Type.ArrayType) rhsType; + return lhsType.getComponentType().accept(this, arrRhsType.getComponentType()); + } + + @Override + public Boolean visitType(Type t, Type type) { + return true; + } +} diff --git a/nullaway/src/main/java/com/uber/nullaway/generics/GenericTypePrettyPrintingVisitor.java b/nullaway/src/main/java/com/uber/nullaway/generics/GenericTypePrettyPrintingVisitor.java new file mode 100644 index 0000000..b6c2f9c --- /dev/null +++ b/nullaway/src/main/java/com/uber/nullaway/generics/GenericTypePrettyPrintingVisitor.java @@ -0,0 +1,75 @@ +package com.uber.nullaway.generics; + +import static java.util.stream.Collectors.joining; + +import com.google.errorprone.VisitorState; +import com.google.errorprone.util.ASTHelpers; +import com.sun.tools.javac.code.Attribute; +import com.sun.tools.javac.code.BoundKind; +import com.sun.tools.javac.code.Type; +import com.sun.tools.javac.code.Types; + +/** + * A visitor that pretty prints a generic type including its type-use nullability annotations, for + * use in error messages. + * + * <p>This code is a modified and extended version of code in {@link + * com.google.errorprone.util.Signatures} + */ +final class GenericTypePrettyPrintingVisitor extends Types.DefaultTypeVisitor<String, Void> { + + private final VisitorState state; + + GenericTypePrettyPrintingVisitor(VisitorState state) { + this.state = state; + } + + @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 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/GenericsChecks.java b/nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java index af79f42..23affc4 100644 --- a/nullaway/src/main/java/com/uber/nullaway/GenericsChecks.java +++ b/nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java @@ -1,8 +1,7 @@ -package com.uber.nullaway; +package com.uber.nullaway.generics; import static com.google.common.base.Verify.verify; import static com.uber.nullaway.NullabilityUtil.castToNonNull; -import static java.util.stream.Collectors.joining; import com.google.common.base.Preconditions; import com.google.errorprone.VisitorState; @@ -11,7 +10,6 @@ import com.google.errorprone.suppliers.Suppliers; import com.google.errorprone.util.ASTHelpers; import com.sun.source.tree.AnnotatedTypeTree; import com.sun.source.tree.AnnotationTree; -import com.sun.source.tree.ArrayTypeTree; import com.sun.source.tree.AssignmentTree; import com.sun.source.tree.ConditionalExpressionTree; import com.sun.source.tree.ExpressionTree; @@ -22,16 +20,15 @@ import com.sun.source.tree.NewClassTree; import com.sun.source.tree.ParameterizedTypeTree; 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; import com.sun.tools.javac.code.Type; -import com.sun.tools.javac.code.TypeMetadata; -import com.sun.tools.javac.code.Types; -import java.util.ArrayList; -import java.util.Collections; +import com.uber.nullaway.Config; +import com.uber.nullaway.ErrorBuilder; +import com.uber.nullaway.ErrorMessage; +import com.uber.nullaway.NullAway; +import com.uber.nullaway.Nullness; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -41,10 +38,13 @@ import javax.lang.model.type.ExecutableType; /** Methods for performing checks related to generic types and nullability. */ public final class GenericsChecks { - private static final String NULLABLE_NAME = "org.jspecify.annotations.Nullable"; - - private static final Supplier<Type> 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.annotations.Nullable} + * annotations and do not apply to other {@code @Nullable} annotations. + */ + static final Supplier<Type> JSPECIFY_NULLABLE_TYPE_SUPPLIER = + Suppliers.typeFromString("org.jspecify.annotations.Nullable"); /** Do not instantiate; all methods should be static */ private GenericsChecks() {} @@ -64,7 +64,7 @@ public final class GenericsChecks { return; } List<? extends Tree> typeArguments = tree.getTypeArguments(); - if (typeArguments.size() == 0) { + if (typeArguments.isEmpty()) { return; } Map<Integer, Tree> nullableTypeArguments = new HashMap<>(); @@ -302,8 +302,7 @@ public final class GenericsChecks { Type rhsType = getTreeType(rhsTree, state); if (lhsType instanceof Type.ClassType && rhsType instanceof Type.ClassType) { - boolean isAssignmentValid = - compareNullabilityAnnotations((Type.ClassType) lhsType, (Type.ClassType) rhsType, state); + boolean isAssignmentValid = compareNullabilityAnnotations(lhsType, rhsType, state); if (!isAssignmentValid) { reportInvalidAssignmentInstantiationError(tree, lhsType, rhsType, state, analysis); } @@ -337,8 +336,7 @@ public final class GenericsChecks { if (formalReturnType instanceof Type.ClassType && returnExpressionType instanceof Type.ClassType) { boolean isReturnTypeValid = - compareNullabilityAnnotations( - (Type.ClassType) formalReturnType, (Type.ClassType) returnExpressionType, state); + compareNullabilityAnnotations(formalReturnType, returnExpressionType, state); if (!isReturnTypeValid) { reportInvalidReturnTypeError( retExpr, formalReturnType, returnExpressionType, state, analysis); @@ -411,15 +409,13 @@ public final class GenericsChecks { // type of the whole expression if (condExprType instanceof Type.ClassType) { if (truePartType instanceof Type.ClassType) { - if (!compareNullabilityAnnotations( - (Type.ClassType) condExprType, (Type.ClassType) truePartType, state)) { + if (!compareNullabilityAnnotations(condExprType, truePartType, state)) { reportMismatchedTypeForTernaryOperator( truePartTree, condExprType, truePartType, state, analysis); } } if (falsePartType instanceof Type.ClassType) { - if (!compareNullabilityAnnotations( - (Type.ClassType) condExprType, (Type.ClassType) falsePartType, state)) { + if (!compareNullabilityAnnotations(condExprType, falsePartType, state)) { reportMismatchedTypeForTernaryOperator( falsePartTree, condExprType, falsePartType, state, analysis); } @@ -458,8 +454,7 @@ public final class GenericsChecks { Type actualParameter = getTreeType(actualParams.get(i), state); if (formalParameter instanceof Type.ClassType && actualParameter instanceof Type.ClassType) { - if (!compareNullabilityAnnotations( - (Type.ClassType) formalParameter, (Type.ClassType) actualParameter, state)) { + if (!compareNullabilityAnnotations(formalParameter, actualParameter, state)) { reportInvalidParametersNullabilityError( formalParameter, actualParameter, actualParams.get(i), state, analysis); } @@ -470,13 +465,12 @@ public final class GenericsChecks { Type.ArrayType varargsArrayType = (Type.ArrayType) formalParams.get(formalParams.size() - 1).type; Type varargsElementType = varargsArrayType.elemtype; - if (varargsElementType.getTypeArguments().size() > 0) { + if (!varargsElementType.getTypeArguments().isEmpty()) { for (int i = formalParams.size() - 1; i < actualParams.size(); i++) { Type actualParameter = getTreeType(actualParams.get(i), state); if (varargsElementType instanceof Type.ClassType && actualParameter instanceof Type.ClassType) { - if (!compareNullabilityAnnotations( - (Type.ClassType) varargsElementType, (Type.ClassType) actualParameter, state)) { + if (!compareNullabilityAnnotations(varargsElementType, actualParameter, state)) { reportInvalidParametersNullabilityError( varargsElementType, actualParameter, actualParams.get(i), state, analysis); } @@ -487,159 +481,6 @@ public final class GenericsChecks { } /** - * Visitor that is called from compareNullabilityAnnotations which recursively compares the - * Nullability annotations for the nested generic type arguments. Compares the Type it is called - * upon, i.e. the LHS type and the Type passed as an argument, i.e. The RHS type. - */ - public static class CompareNullabilityVisitor extends Types.DefaultTypeVisitor<Boolean, Type> { - private final VisitorState state; - - CompareNullabilityVisitor(VisitorState state) { - this.state = state; - } - - @Override - public Boolean visitClassType(Type.ClassType lhsType, Type rhsType) { - Types types = state.getTypes(); - // The base type of rhsType may be a subtype of lhsType's base type. In such cases, we must - // compare lhsType against the supertype of rhsType with a matching base type. - rhsType = (Type.ClassType) types.asSuper(rhsType, lhsType.tsym); - // This is impossible, considering the fact that standard Java subtyping succeeds before - // running NullAway - if (rhsType == null) { - throw new RuntimeException("Did not find supertype of " + rhsType + " matching " + lhsType); - } - List<Type> lhsTypeArguments = lhsType.getTypeArguments(); - List<Type> rhsTypeArguments = rhsType.getTypeArguments(); - // This is impossible, considering the fact that standard Java subtyping succeeds before - // running NullAway - if (lhsTypeArguments.size() != rhsTypeArguments.size()) { - throw new RuntimeException( - "Number of types arguments in " + rhsType + " does not match " + lhsType); - } - for (int i = 0; i < lhsTypeArguments.size(); i++) { - Type lhsTypeArgument = lhsTypeArguments.get(i); - Type rhsTypeArgument = rhsTypeArguments.get(i); - boolean isLHSNullableAnnotated = false; - List<Attribute.TypeCompound> lhsAnnotations = lhsTypeArgument.getAnnotationMirrors(); - // To ensure that we are checking only jspecify nullable annotations - for (Attribute.TypeCompound annotation : lhsAnnotations) { - if (annotation.getAnnotationType().toString().equals(NULLABLE_NAME)) { - isLHSNullableAnnotated = true; - break; - } - } - boolean isRHSNullableAnnotated = false; - List<Attribute.TypeCompound> rhsAnnotations = rhsTypeArgument.getAnnotationMirrors(); - // To ensure that we are checking only jspecify nullable annotations - for (Attribute.TypeCompound annotation : rhsAnnotations) { - if (annotation.getAnnotationType().toString().equals(NULLABLE_NAME)) { - isRHSNullableAnnotated = true; - break; - } - } - if (isLHSNullableAnnotated != isRHSNullableAnnotated) { - return false; - } - // nested generics - if (!lhsTypeArgument.accept(this, rhsTypeArgument)) { - return false; - } - } - // 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 - public Boolean visitArrayType(Type.ArrayType lhsType, Type rhsType) { - Type.ArrayType arrRhsType = (Type.ArrayType) rhsType; - return lhsType.getComponentType().accept(this, arrRhsType.getComponentType()); - } - - @Override - public Boolean visitType(Type t, Type type) { - return true; - } - } - - /** - * Visitor For getting the preserved Annotation Type for the nested generic type arguments within - * the ParameterizedTypeTree originally passed to TypeWithPreservedAnnotations method, since these - * nested arguments may not always be ParameterizedTypeTrees and may be of different types for - * e.g. ArrayTypeTree. - */ - public static class PreservedAnnotationTreeVisitor extends SimpleTreeVisitor<Type, Void> { - - private final VisitorState state; - - PreservedAnnotationTreeVisitor(VisitorState state) { - this.state = state; - } - - @Override - public Type visitArrayType(ArrayTypeTree tree, Void p) { - Type elemType = tree.getType().accept(this, null); - return new Type.ArrayType(elemType, castToNonNull(ASTHelpers.getType(tree)).tsym); - } - - @Override - public Type visitParameterizedType(ParameterizedTypeTree tree, Void p) { - Type.ClassType type = (Type.ClassType) ASTHelpers.getType(tree); - Preconditions.checkNotNull(type); - Type nullableType = NULLABLE_TYPE_SUPPLIER.get(state); - List<? extends Tree> typeArguments = tree.getTypeArguments(); - List<Type> newTypeArgs = new ArrayList<>(); - for (int i = 0; i < typeArguments.size(); i++) { - AnnotatedTypeTree annotatedType = null; - Tree curTypeArg = typeArguments.get(i); - // If the type argument has an annotation, it will either be an AnnotatedTypeTree, or a - // ParameterizedTypeTree in the case of a nested generic type - if (curTypeArg instanceof AnnotatedTypeTree) { - annotatedType = (AnnotatedTypeTree) curTypeArg; - } else if (curTypeArg instanceof ParameterizedTypeTree - && ((ParameterizedTypeTree) curTypeArg).getType() instanceof AnnotatedTypeTree) { - annotatedType = (AnnotatedTypeTree) ((ParameterizedTypeTree) curTypeArg).getType(); - } - List<? extends AnnotationTree> annotations = - annotatedType != null ? annotatedType.getAnnotations() : Collections.emptyList(); - boolean hasNullableAnnotation = false; - for (AnnotationTree annotation : annotations) { - if (ASTHelpers.isSameType( - nullableType, ASTHelpers.getType(annotation.getAnnotationType()), state)) { - hasNullableAnnotation = true; - break; - } - } - // construct a TypeMetadata object containing a nullability annotation if needed - com.sun.tools.javac.util.List<Attribute.TypeCompound> nullableAnnotationCompound = - hasNullableAnnotation - ? com.sun.tools.javac.util.List.from( - Collections.singletonList( - new Attribute.TypeCompound( - nullableType, com.sun.tools.javac.util.List.nil(), null))) - : com.sun.tools.javac.util.List.nil(); - TypeMetadata typeMetadata = - new TypeMetadata(new TypeMetadata.Annotations(nullableAnnotationCompound)); - Type currentTypeArgType = curTypeArg.accept(this, null); - Type newTypeArgType = currentTypeArgType.cloneWithMetadata(typeMetadata); - newTypeArgs.add(newTypeArgType); - } - Type.ClassType finalType = - new Type.ClassType( - type.getEnclosingType(), com.sun.tools.javac.util.List.from(newTypeArgs), type.tsym); - return finalType; - } - - /** By default, just use the type computed by javac */ - @Override - protected Type defaultAction(Tree node, Void unused) { - return castToNonNull(ASTHelpers.getType(node)); - } - } - - /** * Checks that type parameter nullability is consistent between an overriding method and the * corresponding overridden method. * @@ -733,7 +574,7 @@ public final class GenericsChecks { } } - static Nullness getGenericMethodReturnTypeNullness( + public static Nullness getGenericMethodReturnTypeNullness( Symbol.MethodSymbol method, @Nullable Type enclosingType, VisitorState state, Config config) { if (enclosingType == null) { // we have no additional information from generics, so return NONNULL (presence of a @Nullable @@ -784,14 +625,17 @@ public final class GenericsChecks { MethodInvocationTree tree, VisitorState state, Config config) { - if (!(tree.getMethodSelect() instanceof MemberSelectTree)) { + if (!(tree.getMethodSelect() instanceof MemberSelectTree) || invokedMethodSymbol.isStatic()) { return Nullness.NONNULL; } Type methodReceiverType = - castToNonNull( - getTreeType(((MemberSelectTree) tree.getMethodSelect()).getExpression(), state)); - return getGenericMethodReturnTypeNullness( - invokedMethodSymbol, methodReceiverType, state, config); + getTreeType(((MemberSelectTree) tree.getMethodSelect()).getExpression(), state); + if (methodReceiverType == null) { + return Nullness.NONNULL; + } else { + return getGenericMethodReturnTypeNullness( + invokedMethodSymbol, methodReceiverType, state, config); + } } /** @@ -834,7 +678,7 @@ public final class GenericsChecks { MethodInvocationTree tree, VisitorState state, Config config) { - if (!(tree.getMethodSelect() instanceof MemberSelectTree)) { + if (!(tree.getMethodSelect() instanceof MemberSelectTree) || invokedMethodSymbol.isStatic()) { return Nullness.NONNULL; } Type enclosingType = @@ -936,9 +780,7 @@ public final class GenericsChecks { if (overriddenMethodParameterType instanceof Type.ClassType && overridingMethodParameterType instanceof Type.ClassType) { if (!compareNullabilityAnnotations( - (Type.ClassType) overriddenMethodParameterType, - (Type.ClassType) overridingMethodParameterType, - state)) { + overriddenMethodParameterType, overridingMethodParameterType, state)) { reportInvalidOverridingMethodParamTypeError( methodParameters.get(i), overriddenMethodParameterType, @@ -969,9 +811,7 @@ public final class GenericsChecks { } Preconditions.checkArgument(overridingMethodReturnType instanceof Type.ClassType); if (!compareNullabilityAnnotations( - (Type.ClassType) overriddenMethodReturnType, - (Type.ClassType) overridingMethodReturnType, - state)) { + overriddenMethodReturnType, overridingMethodReturnType, state)) { reportInvalidOverridingMethodReturnTypeError( tree, overriddenMethodReturnType, overridingMethodReturnType, analysis, state); } @@ -996,65 +836,6 @@ public final class GenericsChecks { * uses simple names rather than fully-qualified names, and retains all type-use annotations. */ 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 class PrettyTypeVisitor extends Types.DefaultTypeVisitor<String, Void> { - - private final VisitorState state; - - PrettyTypeVisitor(VisitorState state) { - this.state = state; - } - - @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 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(); - } + return type.accept(new GenericTypePrettyPrintingVisitor(state), null); } } diff --git a/nullaway/src/main/java/com/uber/nullaway/generics/PreservedAnnotationTreeVisitor.java b/nullaway/src/main/java/com/uber/nullaway/generics/PreservedAnnotationTreeVisitor.java new file mode 100644 index 0000000..2cde64f --- /dev/null +++ b/nullaway/src/main/java/com/uber/nullaway/generics/PreservedAnnotationTreeVisitor.java @@ -0,0 +1,94 @@ +package com.uber.nullaway.generics; + +import static com.uber.nullaway.NullabilityUtil.castToNonNull; + +import com.google.common.base.Preconditions; +import com.google.errorprone.VisitorState; +import com.google.errorprone.util.ASTHelpers; +import com.sun.source.tree.AnnotatedTypeTree; +import com.sun.source.tree.AnnotationTree; +import com.sun.source.tree.ArrayTypeTree; +import com.sun.source.tree.ParameterizedTypeTree; +import com.sun.source.tree.Tree; +import com.sun.source.util.SimpleTreeVisitor; +import com.sun.tools.javac.code.Attribute; +import com.sun.tools.javac.code.Type; +import com.sun.tools.javac.code.TypeMetadata; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; + +/** + * Visitor For getting the preserved Annotation Types for the nested generic type arguments within a + * ParameterizedTypeTree. This is required primarily since javac does not preserve annotations on + * generic type arguments in its types for NewClassTrees. We need a visitor since the nested + * arguments may appear on different kinds of type trees, e.g., ArrayTypeTrees. + */ +public class PreservedAnnotationTreeVisitor extends SimpleTreeVisitor<Type, Void> { + + private final VisitorState state; + + PreservedAnnotationTreeVisitor(VisitorState state) { + this.state = state; + } + + @Override + public Type visitArrayType(ArrayTypeTree tree, Void p) { + Type elemType = tree.getType().accept(this, null); + return new Type.ArrayType(elemType, castToNonNull(ASTHelpers.getType(tree)).tsym); + } + + @Override + public Type visitParameterizedType(ParameterizedTypeTree tree, Void p) { + Type.ClassType type = (Type.ClassType) ASTHelpers.getType(tree); + Preconditions.checkNotNull(type); + Type nullableType = GenericsChecks.JSPECIFY_NULLABLE_TYPE_SUPPLIER.get(state); + List<? extends Tree> typeArguments = tree.getTypeArguments(); + List<Type> newTypeArgs = new ArrayList<>(); + for (int i = 0; i < typeArguments.size(); i++) { + AnnotatedTypeTree annotatedType = null; + Tree curTypeArg = typeArguments.get(i); + // If the type argument has an annotation, it will either be an AnnotatedTypeTree, or a + // ParameterizedTypeTree in the case of a nested generic type + if (curTypeArg instanceof AnnotatedTypeTree) { + annotatedType = (AnnotatedTypeTree) curTypeArg; + } else if (curTypeArg instanceof ParameterizedTypeTree + && ((ParameterizedTypeTree) curTypeArg).getType() instanceof AnnotatedTypeTree) { + annotatedType = (AnnotatedTypeTree) ((ParameterizedTypeTree) curTypeArg).getType(); + } + List<? extends AnnotationTree> annotations = + annotatedType != null ? annotatedType.getAnnotations() : Collections.emptyList(); + boolean hasNullableAnnotation = false; + for (AnnotationTree annotation : annotations) { + if (ASTHelpers.isSameType( + nullableType, ASTHelpers.getType(annotation.getAnnotationType()), state)) { + hasNullableAnnotation = true; + break; + } + } + // construct a TypeMetadata object containing a nullability annotation if needed + com.sun.tools.javac.util.List<Attribute.TypeCompound> nullableAnnotationCompound = + hasNullableAnnotation + ? com.sun.tools.javac.util.List.from( + Collections.singletonList( + new Attribute.TypeCompound( + nullableType, com.sun.tools.javac.util.List.nil(), null))) + : com.sun.tools.javac.util.List.nil(); + TypeMetadata typeMetadata = + new TypeMetadata(new TypeMetadata.Annotations(nullableAnnotationCompound)); + Type currentTypeArgType = curTypeArg.accept(this, null); + Type newTypeArgType = currentTypeArgType.cloneWithMetadata(typeMetadata); + newTypeArgs.add(newTypeArgType); + } + Type.ClassType finalType = + new Type.ClassType( + type.getEnclosingType(), com.sun.tools.javac.util.List.from(newTypeArgs), type.tsym); + return finalType; + } + + /** By default, just use the type computed by javac */ + @Override + protected Type defaultAction(Tree node, Void unused) { + return castToNonNull(ASTHelpers.getType(node)); + } +} diff --git a/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyGenericsTests.java b/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyGenericsTests.java index e722706..68f1d11 100644 --- a/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyGenericsTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyGenericsTests.java @@ -292,6 +292,26 @@ public class NullAwayJSpecifyGenericsTests extends NullAwayTestsBase { } @Test + public void genericsChecksForAssignmentsWithNonJSpecifyAnnotations() { + makeHelper() + .addSourceLines( + "Test.java", + "package com.uber;", + "import org.checkerframework.checker.nullness.qual.Nullable;", + "class Test {", + " static class NullableTypeParam<E extends @Nullable Object> {}", + " static void testNoWarningForMismatch(NullableTypeParam<@Nullable String> t1) {", + " // no error here since we only do our checks for JSpecify @Nullable annotations", + " NullableTypeParam<String> t2 = t1;", + " }", + " static void testNegative(NullableTypeParam<@Nullable String> t1) {", + " NullableTypeParam<@Nullable String> t2 = t1;", + " }", + "}") + .doTest(); + } + + @Test public void nestedChecksForAssignmentsMultipleArguments() { makeHelper() .addSourceLines( @@ -1494,6 +1514,52 @@ public class NullAwayJSpecifyGenericsTests extends NullAwayTestsBase { .doTest(); } + @Test + public void testForStaticMethodCallAsAParam() { + makeHelper() + .addSourceLines( + "Test.java", + "package com.uber;", + "import org.jspecify.annotations.Nullable;", + "class Test {", + " static class A<T> {", + " public static <T> A<T> returnA(){", + " return new A<T>();", + " }", + " public static <T> A<T> returnAWithParam(Object o){", + " return new A<T>();", + " }", + " }", + " static void func(A<Object> a){", + " }", + " static void testNegative() {", + " func(A.returnA());", + " }", + " static void testNegative2() {", + " func(A.returnAWithParam(new Object()));", + " }", + "}") + .doTest(); + } + + @Test + public void testForDiamondOperatorReturnedAsAMethodCaller() { + makeHelper() + .addSourceLines( + "Test.java", + "package com.uber;", + "import org.jspecify.annotations.Nullable;", + "class Test {", + " static class B<T>{", + " String build(){return \"x\";}", + " }", + " static String testNegative() {", + " return new B<>().build();", + " }", + "}") + .doTest(); + } + private CompilationTestHelper makeHelper() { return makeTestHelperWithArgs( Arrays.asList( diff --git a/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyTests.java b/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyTests.java index ed9acb1..246d7e8 100644 --- a/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyTests.java @@ -988,6 +988,31 @@ public class NullAwayJSpecifyTests extends NullAwayTestsBase { } @Test + public void nullUnmarkedRestrictiveAnnotationsAndGenerics() { + makeTestHelperWithArgs( + Arrays.asList( + "-d", + temporaryFolder.getRoot().getAbsolutePath(), + "-XepOpt:NullAway:AnnotatedPackages=com.uber", + "-XepOpt:NullAway:AcknowledgeRestrictiveAnnotations=true")) + .addSourceLines( + "Test.java", + "package com.uber;", + "import org.jspecify.annotations.NullUnmarked;", + "import org.jetbrains.annotations.Nullable;", + "import org.jetbrains.annotations.NotNull;", + "import java.util.List;", + "public class Test {", + " @NullUnmarked", + " public static void takesNullable(@Nullable List<@NotNull String> l) {}", + " public static void test() {", + " takesNullable(null);", + " }", + "}") + .doTest(); + } + + @Test public void nullMarkedStaticImports() { makeTestHelperWithArgs( Arrays.asList( diff --git a/nullaway/src/test/java/com/uber/nullaway/NullAwayTypeUseAnnotationsTests.java b/nullaway/src/test/java/com/uber/nullaway/NullAwayTypeUseAnnotationsTests.java index 7af8b25..5e13b02 100644 --- a/nullaway/src/test/java/com/uber/nullaway/NullAwayTypeUseAnnotationsTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/NullAwayTypeUseAnnotationsTests.java @@ -38,16 +38,28 @@ public class NullAwayTypeUseAnnotationsTests extends NullAwayTestsBase { "import java.util.List;", "import java.util.ArrayList;", "import org.checkerframework.checker.nullness.qual.Nullable;", + "import org.checkerframework.checker.nullness.qual.NonNull;", "class TypeArgumentAnnotation {", " List<@Nullable String> fSafe = new ArrayList<>();", " @Nullable List<String> fUnsafe = new ArrayList<>();", " void useParamSafe(List<@Nullable String> list) {", " list.hashCode();", " }", + " void unsafeCall() {", + " // BUG: Diagnostic contains: passing @Nullable parameter", + " useParamSafe(null);", + " }", " void useParamUnsafe(@Nullable List<String> list) {", " // BUG: Diagnostic contains: dereferenced", " list.hashCode();", " }", + " void useParamUnsafeNonNullElements(@Nullable List<@NonNull String> list) {", + " // BUG: Diagnostic contains: dereferenced", + " list.hashCode();", + " }", + " void safeCall() {", + " useParamUnsafeNonNullElements(null);", + " }", " void useFieldSafe() {", " fSafe.hashCode();", " }", |