diff --git a/nullaway/src/main/java/com/uber/nullaway/Config.java b/nullaway/src/main/java/com/uber/nullaway/Config.java index 5745666bc7..3efefe47c2 100644 --- a/nullaway/src/main/java/com/uber/nullaway/Config.java +++ b/nullaway/src/main/java/com/uber/nullaway/Config.java @@ -304,6 +304,9 @@ public interface Config { /** Should new checks based on JSpecify (like checks for generic types) be enabled? */ boolean isJSpecifyMode(); + /** Should wildcard-aware generic nullability checks be enabled? */ + boolean handleWildcardGenerics(); + /** * Checks if legacy annotation locations are enabled. * diff --git a/nullaway/src/main/java/com/uber/nullaway/DummyOptionsConfig.java b/nullaway/src/main/java/com/uber/nullaway/DummyOptionsConfig.java index c6908e355a..f564175788 100644 --- a/nullaway/src/main/java/com/uber/nullaway/DummyOptionsConfig.java +++ b/nullaway/src/main/java/com/uber/nullaway/DummyOptionsConfig.java @@ -219,6 +219,11 @@ public boolean isJSpecifyMode() { throw new IllegalStateException(ERROR_MESSAGE); } + @Override + public boolean handleWildcardGenerics() { + throw new IllegalStateException(ERROR_MESSAGE); + } + @Override public boolean isLegacyAnnotationLocation() { throw new IllegalStateException(ERROR_MESSAGE); diff --git a/nullaway/src/main/java/com/uber/nullaway/ErrorProneCLIFlagsConfig.java b/nullaway/src/main/java/com/uber/nullaway/ErrorProneCLIFlagsConfig.java index d7bcb8fd10..73f5119d7a 100644 --- a/nullaway/src/main/java/com/uber/nullaway/ErrorProneCLIFlagsConfig.java +++ b/nullaway/src/main/java/com/uber/nullaway/ErrorProneCLIFlagsConfig.java @@ -104,6 +104,8 @@ final class ErrorProneCLIFlagsConfig implements Config { static final String FL_WARN_ON_GENERIC_INFERENCE_FAILURE = EP_FL_NAMESPACE + ":WarnOnGenericInferenceFailure"; + static final String FL_HANDLE_WILDCARD_GENERICS = EP_FL_NAMESPACE + ":HandleWildcardGenerics"; + static final String ANNOTATED_PACKAGES_ONLY_NULLMARKED_ERROR_MSG = "DO NOT report an issue to Error Prone for this crash! NullAway configuration is " + "incorrect. " @@ -231,6 +233,7 @@ final class ErrorProneCLIFlagsConfig implements Config { private final boolean treatGeneratedAsUnannotated; private final boolean acknowledgeAndroidRecent; private final boolean jspecifyMode; + private final boolean handleWildcardGenerics; private final boolean legacyAnnotationLocation; private final boolean warnOnInferenceFailure; private final ImmutableSet knownInitializers; @@ -303,6 +306,7 @@ final class ErrorProneCLIFlagsConfig implements Config { treatGeneratedAsUnannotated = flags.getBoolean(FL_GENERATED_UNANNOTATED).orElse(false); acknowledgeAndroidRecent = flags.getBoolean(FL_ACKNOWLEDGE_ANDROID_RECENT).orElse(false); jspecifyMode = flags.getBoolean(FL_JSPECIFY_MODE).orElse(false); + handleWildcardGenerics = flags.getBoolean(FL_HANDLE_WILDCARD_GENERICS).orElse(false); assertsEnabled = flags.getBoolean(FL_ASSERTS_ENABLED).orElse(false); fieldAnnotPattern = getPackagePattern( @@ -605,6 +609,11 @@ public boolean isJSpecifyMode() { return jspecifyMode; } + @Override + public boolean handleWildcardGenerics() { + return handleWildcardGenerics; + } + @Override public boolean isLegacyAnnotationLocation() { return legacyAnnotationLocation; 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 bb8f9a604a..7339dca9c8 100644 --- a/nullaway/src/main/java/com/uber/nullaway/generics/CheckIdenticalNullabilityVisitor.java +++ b/nullaway/src/main/java/com/uber/nullaway/generics/CheckIdenticalNullabilityVisitor.java @@ -1,6 +1,7 @@ package com.uber.nullaway.generics; import com.google.errorprone.VisitorState; +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.Types; @@ -63,18 +64,7 @@ public Boolean visitClassType(Type.ClassType lhsType, Type rhsType) { for (int i = 0; i < lhsTypeArguments.size(); i++) { Type lhsTypeArgument = lhsTypeArguments.get(i); Type rhsTypeArgument = rhsTypeArguments.get(i); - if (lhsTypeArgument.getKind().equals(TypeKind.WILDCARD) - || rhsTypeArgument.getKind().equals(TypeKind.WILDCARD)) { - // TODO Handle wildcard types - continue; - } - boolean isLHSNullableAnnotated = genericsChecks.isNullableAnnotated(lhsTypeArgument); - boolean isRHSNullableAnnotated = genericsChecks.isNullableAnnotated(rhsTypeArgument); - if (isLHSNullableAnnotated != isRHSNullableAnnotated) { - return false; - } - // nested generics - if (!lhsTypeArgument.accept(this, rhsTypeArgument)) { + if (!typeArgumentContainedBy(lhsTypeArgument, rhsTypeArgument)) { return false; } } @@ -116,4 +106,84 @@ public Boolean visitArrayType(Type.ArrayType lhsType, Type rhsType) { public Boolean visitType(Type t, Type type) { return true; } + + /** + * Returns whether the actual type argument on the right is contained by the formal type argument + * on the left, following the JLS 4.5.1 notion of type-argument containment but interpreted with + * JSpecify's nullability-aware subtype + * relation. Non-wildcard pairs require matching nullability annotations and recursively + * matching nested type arguments. Wildcard formals are delegated to {@link #wildcardContains}. + */ + private boolean typeArgumentContainedBy(Type lhsTypeArgument, Type rhsTypeArgument) { + if (!config.handleWildcardGenerics() + && (lhsTypeArgument.getKind().equals(TypeKind.WILDCARD) + || rhsTypeArgument.getKind().equals(TypeKind.WILDCARD))) { + // Preserve the pre-flag behavior of skipping wildcard-aware checks entirely. + return true; + } + if (lhsTypeArgument.getKind().equals(TypeKind.WILDCARD)) { + return wildcardContains((Type.WildcardType) lhsTypeArgument, rhsTypeArgument); + } + if (rhsTypeArgument.getKind().equals(TypeKind.WILDCARD)) { + // TODO: Add proper support for the remaining case where the formal type argument is not a + // wildcard but the actual type argument is a wildcard. + return true; + } + boolean isLHSNullableAnnotated = genericsChecks.isNullableAnnotated(lhsTypeArgument); + boolean isRHSNullableAnnotated = genericsChecks.isNullableAnnotated(rhsTypeArgument); + if (isLHSNullableAnnotated != isRHSNullableAnnotated) { + return false; + } + return lhsTypeArgument.accept(this, rhsTypeArgument); + } + + /** + * Handles a narrow slice of the JLS type-argument containment rules from JLS 4.5.1 for wildcard + * type arguments. In particular, for a formal argument {@code ? extends S}, we accept either a + * concrete actual argument {@code T} or a wildcard actual argument {@code ? extends T} whenever + * {@code T <: S}, using NullAway's nullability-aware subtype check in place of plain Java + * subtyping. This covers the JLS cases behind both {@code T <= ? extends S} and {@code ? extends + * T <= ? extends S}. For now, this method intentionally leaves {@code super} wildcards and other + * more complex cases to existing fallback behavior. + */ + private boolean wildcardContains(Type.WildcardType lhsWildcard, Type rhsTypeArgument) { + if (lhsWildcard.kind == BoundKind.UNBOUND) { + // TODO: For unbounded wildcards, we need to find the bound of the corresponding type + // variable rather than accepting outright; see + // https://jspecify.dev/docs/user-guide/#wildcard-bounds + return true; + } + if (lhsWildcard.kind != BoundKind.EXTENDS) { + // Treat non-extends wildcards as accepted here until we add more complete support. + return true; + } + Type lhsBound = lhsWildcard.getExtendsBound(); + if (rhsTypeArgument.getKind().equals(TypeKind.WILDCARD)) { + Type.WildcardType rhsWildcard = (Type.WildcardType) rhsTypeArgument; + if (rhsWildcard.kind != BoundKind.EXTENDS) { + // Treat non-extends wildcard actual arguments as accepted here until we add more complete + // support. + return true; + } + Type rhsBound = rhsWildcard.getExtendsBound(); + return typeArgumentSubtype(lhsBound, rhsBound); + } + return typeArgumentSubtype(lhsBound, rhsTypeArgument); + } + + /** + * Returns whether the actual type argument on the right is a nullability-aware subtype of the + * formal type argument on the left. This check first rejects flows from nullable to non-null at + * the top level of the type argument, then delegates to {@link + * GenericsChecks#subtypeParameterNullability(Type, Type, VisitorState)} for recursive nested + * checks. + */ + private boolean typeArgumentSubtype(Type lhsType, Type rhsType) { + boolean isLHSNullableAnnotated = genericsChecks.isNullableAnnotated(lhsType); + boolean isRHSNullableAnnotated = genericsChecks.isNullableAnnotated(rhsType); + if (isRHSNullableAnnotated && !isLHSNullableAnnotated) { + return false; + } + return genericsChecks.subtypeParameterNullability(lhsType, rhsType, state); + } } 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 8b4d06b202..81d539c4bf 100644 --- a/nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java +++ b/nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java @@ -1450,7 +1450,7 @@ private boolean identicalTypeParameterNullability( * @param rhsType type for the rhs of the assignment * @param state the visitor state */ - private boolean subtypeParameterNullability(Type lhsType, Type rhsType, VisitorState state) { + boolean subtypeParameterNullability(Type lhsType, Type rhsType, VisitorState state) { if (lhsType.isRaw()) { return true; } diff --git a/nullaway/src/main/java/com/uber/nullaway/generics/JSpecifyJavacConfig.java b/nullaway/src/main/java/com/uber/nullaway/generics/JSpecifyJavacConfig.java index f8caf41216..a3a2d1094b 100644 --- a/nullaway/src/main/java/com/uber/nullaway/generics/JSpecifyJavacConfig.java +++ b/nullaway/src/main/java/com/uber/nullaway/generics/JSpecifyJavacConfig.java @@ -11,15 +11,18 @@ * configuring NullAway compilation tests that rely on JSpecify annotations. * *

For tests, this utility ensures that they always pass {@code - * -XepOpt:NullAway:JSpecifyMode=true} and {@code -XDaddTypeAnnotationsToSymbol=true} together. + * -XepOpt:NullAway:JSpecifyMode=true}, {@code -XDaddTypeAnnotationsToSymbol=true}, and the + * wildcard-support flag together. */ public final class JSpecifyJavacConfig { public static final String JSPECIFY_MODE_FLAG = "-XepOpt:NullAway:JSpecifyMode=true"; public static final String ADD_TYPE_ANNOTATIONS_FLAG = "-XDaddTypeAnnotationsToSymbol=true"; + public static final String HANDLE_WILDCARD_GENERICS_FLAG = + "-XepOpt:NullAway:HandleWildcardGenerics=true"; private static final List JSPECIFY_MODE_ARGS = - List.of(JSPECIFY_MODE_FLAG, ADD_TYPE_ANNOTATIONS_FLAG); + List.of(JSPECIFY_MODE_FLAG, ADD_TYPE_ANNOTATIONS_FLAG, HANDLE_WILDCARD_GENERICS_FLAG); private JSpecifyJavacConfig() {} 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 292db95e14..6ce7e7857c 100644 --- a/nullaway/src/test/java/com/uber/nullaway/jspecify/WildcardTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/jspecify/WildcardTests.java @@ -9,7 +9,36 @@ public class WildcardTests extends NullAwayTestsBase { - @Ignore("https://github.com/uber/NullAway/issues/1360") + @Test + public void simpleWildcardNoInference() { + makeHelper() + .addSourceLines( + "Test.java", + """ + import org.jspecify.annotations.*; + @NullMarked + class Test { + class Foo {} + String nullableWildcard(Foo foo) { throw new RuntimeException(); } + String nonnullWildcard(Foo foo) { throw new RuntimeException(); } + void testNegative(Foo<@Nullable String> f, Foo f2) { + // this is legal since the wildcard upper bound is @Nullable + String s = nullableWildcard(f); + // also legal + String s2 = nullableWildcard(f2); + } + void testPositive(Foo<@Nullable String> f, Foo f2) { + // not legal since the wildcard upper bound is non-null + // BUG: Diagnostic contains: incompatible types: Test.Foo<@Nullable String> cannot be converted to Test.Foo + String s = nonnullWildcard(f); + // legal + String s2 = nonnullWildcard(f2); + } + } + """) + .doTest(); + } + @Test public void simpleWildcard() { makeHelper() @@ -29,7 +58,70 @@ void testNegative(Foo<@Nullable String> f) { } void testPositive(Foo<@Nullable String> f) { // not legal since the wildcard upper bound is non-null - // BUG: Diagnostic contains: something about how f cannot be passed here + // BUG: Diagnostic contains: incompatible types: Test.Foo<@Nullable String> cannot be converted to Test.Foo + String s = nonnullWildcard(f); + s.hashCode(); + } + } + """) + .doTest(); + } + + @Test + public void nestedTypeArgsInWildcardBoundNoInference() { + makeHelper() + .addSourceLines( + "Test.java", + """ + import org.jspecify.annotations.*; + @NullMarked + class Test { + class Foo {} + class Bar {} + String nullableWildcard(Foo> foo) { + throw new RuntimeException(); + } + String nonnullWildcard(Foo> foo) { + throw new RuntimeException(); + } + void testNegative(Foo> f) { + String s = nullableWildcard(f); + s.hashCode(); + } + void testPositive(Foo> f) { + // BUG: Diagnostic contains: incompatible types: Test.Foo> cannot be converted to Test.Foo> + String s = nonnullWildcard(f); + s.hashCode(); + } + } + """) + .doTest(); + } + + @Test + public void deeplyNestedTypeArgsInWildcardBoundNoInference() { + makeHelper() + .addSourceLines( + "Test.java", + """ + import org.jspecify.annotations.*; + @NullMarked + class Test { + class Foo {} + class Bar {} + class Baz {} + String nullableWildcard(Foo>> foo) { + throw new RuntimeException(); + } + String nonnullWildcard(Foo>> foo) { + throw new RuntimeException(); + } + void testNegative(Foo>> f) { + String s = nullableWildcard(f); + s.hashCode(); + } + void testPositive(Foo>> f) { + // BUG: Diagnostic contains: incompatible types: Test.Foo>> cannot be converted to Test.Foo>> String s = nonnullWildcard(f); s.hashCode(); } @@ -38,6 +130,134 @@ void testPositive(Foo<@Nullable String> f) { .doTest(); } + @Test + public void intermediateNestedTypeArgsInWildcardBoundNoInference() { + makeHelper() + .addSourceLines( + "Test.java", + """ + import org.jspecify.annotations.*; + @NullMarked + class Test { + class Foo {} + class Bar {} + class Baz {} + String nullableWildcard(Foo>> foo) { + throw new RuntimeException(); + } + String nonnullWildcard(Foo>> foo) { + throw new RuntimeException(); + } + void testNegative(Foo<@Nullable Bar>> f) { + String s = nullableWildcard(f); + s.hashCode(); + } + void testPositive(Foo<@Nullable Bar>> f) { + // BUG: Diagnostic contains: incompatible types: Test.Foo>> cannot be converted to Test.Foo>> + String s = nonnullWildcard(f); + s.hashCode(); + } + } + """) + .doTest(); + } + + @Test + public void wildcardActualArgumentNoInference() { + makeHelper() + .addSourceLines( + "Test.java", + """ + import org.jspecify.annotations.*; + @NullMarked + class Test { + class Foo {} + String nullableWildcard(Foo foo) { + throw new RuntimeException(); + } + String nonnullWildcard(Foo foo) { + throw new RuntimeException(); + } + void testNegative(Foo f) { + String s = nullableWildcard(f); + s.hashCode(); + } + void testPositive(Foo f) { + // BUG: Diagnostic contains: incompatible types: Test.Foo cannot be converted to Test.Foo + String s = nonnullWildcard(f); + s.hashCode(); + } + } + """) + .doTest(); + } + + @Test + public void wildcardCheckingForReturnsAndAssignments() { + makeHelper() + .addSourceLines( + "Test.java", + """ + import org.jspecify.annotations.*; + @NullMarked + class Test { + class Foo {} + Foo nonnullField; + Foo nullableField; + Test(Foo f) { + nullableField = f; + // BUG: Diagnostic contains: incompatible types: Test.Foo cannot be converted to Test.Foo + nonnullField = f; + } + Foo nullableReturn(Foo f) { + return f; + } + Foo nonnullReturn(Foo f) { + // BUG: Diagnostic contains: incompatible types: Test.Foo cannot be converted to Test.Foo + return f; + } + void testLocal(Foo f) { + Foo ok = f; + // BUG: Diagnostic contains: incompatible types: Test.Foo cannot be converted to Test.Foo + Foo bad = f; + var f2 = f; + // BUG: Diagnostic contains: incompatible types: Test.Foo cannot be converted to Test.Foo + Foo bad2 = f2; + } + } + """) + .doTest(); + } + + @Ignore("bad interaction between wildcard support and generic method inference") + @Test + public void wildcardSuperBoundsAndInference() { + makeHelperWithInferenceFailureWarning() + .addSourceLines( + "Test.java", + """ + import org.jspecify.annotations.NullMarked; + import org.jspecify.annotations.Nullable; + @NullMarked + class Test { + public interface BiFunction { + R apply(T t, U u); + } + void test1(BiFunction f) { + BiFunction g = f; + } + static BiFunction id( + BiFunction f) { + return f; + } + void test2(BiFunction f) { + BiFunction g = id(f); + } + } + """) + .doTest(); + } + @Ignore("https://github.com/uber/NullAway/issues/1350") @Test public void genericMethodLambdaArgWildCard() { diff --git a/test-library-models/src/test/java/com/uber/nullaway/CustomLibraryModelsTests.java b/test-library-models/src/test/java/com/uber/nullaway/CustomLibraryModelsTests.java index 1dc7d3a3e1..e00a4f5c06 100644 --- a/test-library-models/src/test/java/com/uber/nullaway/CustomLibraryModelsTests.java +++ b/test-library-models/src/test/java/com/uber/nullaway/CustomLibraryModelsTests.java @@ -455,7 +455,7 @@ public void nestedWildcards() { @NullMarked public class Test { void testUpper(NestedAnnots<@Nullable String> t) { - // TODO report an error here when we support wildcards + // BUG: Diagnostic contains: incompatible types: NestedAnnots<@Nullable String> cannot be converted to NestedAnnots NestedAnnots.wildcardUpper(t); } void testLower(NestedAnnots t) {