diff --git a/AGENTS.md b/AGENTS.md index f83ab322e8..405444e29d 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -24,7 +24,7 @@ Our `CHANGELOG.md` file should be formatted as follows: Do not add top-level `@Nullable` annotations on local variables. NullAway infers the nullability of local variables and ignores these explicit annotations. -Whenever you add a non-trivial method, add Javadoc, even if it's a private method. +Whenever you add a non-trivial method, add Javadoc, even if it's a private method. JUnit test methods do not require Javadoc. You do _not_ need to run `./gradlew spotlessJavaCheck` to check formatting. We have a pre-commit hook that automatically formats code before it is committed. diff --git a/nullaway/src/main/java/com/uber/nullaway/generics/CheckIdenticalNullabilityVisitor.java b/nullaway/src/main/java/com/uber/nullaway/generics/CheckIdenticalNullabilityVisitor.java index 727dd0161b..09b2df0ac4 100644 --- a/nullaway/src/main/java/com/uber/nullaway/generics/CheckIdenticalNullabilityVisitor.java +++ b/nullaway/src/main/java/com/uber/nullaway/generics/CheckIdenticalNullabilityVisitor.java @@ -11,6 +11,7 @@ import com.uber.nullaway.handlers.Handler; import java.util.List; import javax.lang.model.type.NullType; +import javax.lang.model.type.TypeKind; /** * Visitor that checks for identical nullability annotations at all nesting levels within two types. @@ -31,14 +32,36 @@ public class CheckIdenticalNullabilityVisitor extends Types.DefaultTypeVisitorThe RHS is aligned with the LHS's base type before comparing type arguments. When {@link + * Config#handleWildcardGenerics()} is enabled, a direct or captured RHS wildcard is first + * replaced with its effective upper bound. + * + * @param lhsType class type on the left side of the comparison + * @param rhsType type on the right side of the comparison + * @return {@code true} if the types have compatible nested nullability, or if the comparison is + * intentionally skipped + */ @Override public Boolean visitClassType(Type.ClassType lhsType, Type rhsType) { if (rhsType instanceof NullType || rhsType.isPrimitive()) { return true; } - if (GenericsUtils.asWildcard(rhsType) != null) { - // TODO Handle wildcard types - return true; + if (!config.handleWildcardGenerics()) { + // skip checking of wildcards + if (rhsType.getKind().equals(TypeKind.WILDCARD)) { + return true; + } + } else if (GenericsUtils.asWildcard(rhsType) != null) { + Type rhsUpperBound = + GenericsUtils.effectiveWildcardUpperBound(rhsType, state, config, handler); + if (GenericsUtils.asWildcard(rhsUpperBound) != null) { + // Bail out if resolving the upper bound did not produce a usable concrete bound. + return true; + } + rhsType = rhsUpperBound; } if (lhsType.isIntersection()) { return handleIntersectionType((Type.IntersectionClassType) lhsType, rhsType); @@ -98,12 +121,7 @@ public Boolean visitArrayType(Type.ArrayType lhsType, Type rhsType) { return true; } Type rhsComponentType = rhsArrayType.getComponentType(); - boolean isLHSNullableAnnotated = genericsChecks.isNullableAnnotated(lhsComponentType); - boolean isRHSNullableAnnotated = genericsChecks.isNullableAnnotated(rhsComponentType); - if (isRHSNullableAnnotated != isLHSNullableAnnotated) { - return false; - } - return lhsComponentType.accept(this, rhsComponentType); + return haveIdenticalNullability(lhsComponentType, rhsComponentType); } @Override @@ -119,35 +137,47 @@ public Boolean visitType(Type t, Type type) { * matching nested type arguments. Wildcard formals are delegated to {@link #wildcardContains}. */ private boolean typeArgumentContainedBy(Type lhsTypeArgument, Type rhsTypeArgument) { - // Do not use GenericsUtils.asWildcard() for the LHS. A captured LHS is a type variable, not a - // wildcard formal; unwrapping it can repeatedly expand recursive upper bounds (for example, - // F-bounded type parameters) as containment delegates back into subtype checking. See test - // com.uber.nullaway.jspecify.WildcardTests.capturedLhsWithFBoundedTypeParametersDoesNotRecurse - Type.WildcardType lhsWildcard = - lhsTypeArgument instanceof Type.WildcardType wildcardType ? wildcardType : null; - Type.WildcardType rhsWildcard = GenericsUtils.asWildcard(rhsTypeArgument); - if (!config.handleWildcardGenerics() && (lhsWildcard != null || rhsWildcard != null)) { - // Preserve the pre-flag behavior of skipping wildcard-aware checks entirely. - return true; - } - if (lhsWildcard != null) { - return wildcardContains(lhsWildcard, rhsTypeArgument); - } - if (rhsWildcard != null) { - // This case should only arise when generic method invocation inference / capture conversion - // lets a wildcard actual argument flow into a non-wildcard formal type argument, e.g., - // passing Foo to void m(Foo). We do not yet support wildcard inference. - // For non-inference assignment / return / parameter checks, javac rejects these conversions - // before NullAway runs. - // TODO: Add proper support when inference for wildcards is implemented. - return true; + if (!config.handleWildcardGenerics()) { + if (lhsTypeArgument.getKind().equals(TypeKind.WILDCARD) + || rhsTypeArgument.getKind().equals(TypeKind.WILDCARD)) { + // Preserve the pre-flag behavior of skipping wildcard-aware checks entirely. + return true; + } + } else { + // Do not use GenericsUtils.asWildcard() for the LHS. A captured LHS is a type variable, not a + // wildcard formal; unwrapping it can repeatedly expand recursive upper bounds (for example, + // F-bounded type parameters) as containment delegates back into subtype checking. See test + // com.uber.nullaway.jspecify.WildcardTests.capturedLhsWithFBoundedTypeParametersDoesNotRecurse + Type.WildcardType lhsWildcard = + lhsTypeArgument instanceof Type.WildcardType wildcardType ? wildcardType : null; + Type.WildcardType rhsWildcard = GenericsUtils.asWildcard(rhsTypeArgument); + if (lhsWildcard != null) { + return wildcardContains(lhsWildcard, rhsTypeArgument); + } + if (rhsWildcard != null) { + // This case should only arise when generic method invocation inference / capture conversion + // lets a wildcard actual argument flow into a non-wildcard formal type argument, e.g., + // passing Foo to void m(Foo). We do not yet support wildcard inference. + // For non-inference assignment / return / parameter checks, javac rejects these conversions + // before NullAway runs. + // TODO: Add proper support when inference for wildcards is implemented. + return true; + } } - boolean isLHSNullableAnnotated = genericsChecks.isNullableAnnotated(lhsTypeArgument); - boolean isRHSNullableAnnotated = genericsChecks.isNullableAnnotated(rhsTypeArgument); + return haveIdenticalNullability(lhsTypeArgument, rhsTypeArgument); + } + + /** + * Returns whether two types have identical top-level nullability and compatible nested + * nullability. + */ + private boolean haveIdenticalNullability(Type lhsType, Type rhsType) { + boolean isLHSNullableAnnotated = genericsChecks.isNullableAnnotated(lhsType); + boolean isRHSNullableAnnotated = genericsChecks.isNullableAnnotated(rhsType); if (isLHSNullableAnnotated != isRHSNullableAnnotated) { return false; } - return lhsTypeArgument.accept(this, rhsTypeArgument); + return lhsType.accept(this, rhsType); } /** diff --git a/nullaway/src/test/java/com/uber/nullaway/jspecify/WildcardTests.java b/nullaway/src/test/java/com/uber/nullaway/jspecify/WildcardTests.java index 8b4aa1d1bd..951bd9bbd3 100644 --- a/nullaway/src/test/java/com/uber/nullaway/jspecify/WildcardTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/jspecify/WildcardTests.java @@ -4,6 +4,7 @@ import com.uber.nullaway.NullAwayTestsBase; import com.uber.nullaway.generics.JSpecifyJavacConfig; import java.util.Arrays; +import java.util.List; import org.junit.Test; public class WildcardTests extends NullAwayTestsBase { @@ -406,6 +407,31 @@ static void testNonNullSuperBound(Foo f) { .doTest(); } + @Test + public void wildcardCaptureReturnPreservesNestedNullability() { + makeHelper() + .addSourceLines( + "Test.java", + """ + import org.jspecify.annotations.NullMarked; + import org.jspecify.annotations.Nullable; + @NullMarked + class Test { + static class Box {} + static class Holder { + T get() { + throw new RuntimeException(); + } + } + static void test(Holder> holder) { + // BUG: Diagnostic contains: incompatible types + Box box = holder.get(); + } + } + """) + .doTest(); + } + @Test public void wildcardCaptureReturnWithTypeVariableUpperBound() { makeHelper() @@ -979,6 +1005,57 @@ static class Analysis< .doTest(); } + /** ensures we avoid a crash related to wildcards when wildcard handling is disabled */ + @Test + public void methodRefParameterSuperWildcardWithHandlingDisabled() { + makeTestHelperWithArgs( + List.of( + "-XepOpt:NullAway:OnlyNullMarked=true", + JSpecifyJavacConfig.JSPECIFY_MODE_FLAG, + JSpecifyJavacConfig.ADD_TYPE_ANNOTATIONS_FLAG)) + .addSourceLines( + "Test.java", + """ + import java.util.function.Function; + import org.jspecify.annotations.NullMarked; + @NullMarked + final class Test { + static void reproduce() { + getOrThrow(Test::throwAsUncheckedException); + } + private static void getOrThrow(Function exceptionTransformer) { + } + private static RuntimeException throwAsUncheckedException(Throwable throwable) { + return new RuntimeException(throwable); + } + } + """) + .doTest(); + } + + /** reduced from a crasher found when checking junit */ + @Test + public void methodRefReturnUnboundedWildcard() { + makeHelper() + .addSourceLines( + "Test.java", + """ + package repro; + import java.util.concurrent.FutureTask; + import java.util.function.Supplier; + import org.jspecify.annotations.NullMarked; + import org.jspecify.annotations.Nullable; + @NullMarked + final class Test { + private final FutureTask<@Nullable Object> task; + Test(Supplier delegate) { + this.task = new FutureTask<>(delegate::get); + } + } + """) + .doTest(); + } + @Test public void nullableOnWildcard() { makeHelper()