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 8e205912db..57caffa1f9 100644 --- a/nullaway/src/main/java/com/uber/nullaway/generics/CheckIdenticalNullabilityVisitor.java +++ b/nullaway/src/main/java/com/uber/nullaway/generics/CheckIdenticalNullabilityVisitor.java @@ -154,7 +154,8 @@ private boolean typeArgumentContainedBy(Type lhsTypeArgument, Type rhsTypeArgume private boolean wildcardContains(Type.WildcardType lhsWildcard, Type rhsTypeArgument) { return switch (lhsWildcard.kind) { case UNBOUND, EXTENDS -> - extendsBoundContains(wildcardUpperBound(lhsWildcard), rhsTypeArgument); + extendsBoundContains( + GenericsUtils.wildcardUpperBound(lhsWildcard, state), rhsTypeArgument); case SUPER -> superWildcardContains(lhsWildcard, rhsTypeArgument); }; } @@ -165,26 +166,14 @@ private boolean wildcardContains(Type.WildcardType lhsWildcard, Type rhsTypeArgu * actuals whose effective upper bound is {@code T}, containment holds when {@code T <: S}. */ private boolean extendsBoundContains(Type lhsBound, Type rhsTypeArgument) { - if (rhsTypeArgument instanceof Type.WildcardType rhsWildcard) { - Type rhsUpperBound = wildcardUpperBound(rhsWildcard); + Type.WildcardType rhsWildcard = GenericsUtils.asWildcard(rhsTypeArgument); + if (rhsWildcard != null) { + Type rhsUpperBound = GenericsUtils.wildcardUpperBound(rhsWildcard, state); return typeArgumentSubtype(lhsBound, rhsUpperBound); } return typeArgumentSubtype(lhsBound, rhsTypeArgument); } - /** - * Returns the effective upper bound of a wildcard, using the corresponding type variable's upper - * bound for unbounded wildcards and {@code super} wildcards. - */ - private Type wildcardUpperBound(Type.WildcardType wildcardType) { - if (wildcardType.kind == BoundKind.EXTENDS) { - return wildcardType.getExtendsBound(); - } - // For ? and ? super L, javac stores the wildcard's corresponding type variable in the `bound` - // field. The upper bound of that type variable is the wildcard's effective upper bound. - return wildcardType.bound.getUpperBound(); - } - /** * Returns whether a formal {@code ? super S} contains the actual type argument on the right. For * concrete actuals {@code T} and wildcard actuals {@code ? super T}, containment holds when diff --git a/nullaway/src/main/java/com/uber/nullaway/generics/ConstraintSolverImpl.java b/nullaway/src/main/java/com/uber/nullaway/generics/ConstraintSolverImpl.java index 51eb67495a..092d4be322 100644 --- a/nullaway/src/main/java/com/uber/nullaway/generics/ConstraintSolverImpl.java +++ b/nullaway/src/main/java/com/uber/nullaway/generics/ConstraintSolverImpl.java @@ -5,10 +5,12 @@ import com.google.common.base.Verify; import com.google.errorprone.VisitorState; 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.Type.ClassType; import com.sun.tools.javac.code.Type.TypeVar; +import com.sun.tools.javac.code.Type.WildcardType; import com.sun.tools.javac.code.Types; import com.uber.nullaway.CodeAnnotationInfo; import com.uber.nullaway.Config; @@ -90,7 +92,15 @@ class AddSubtypeConstraintsVisitor extends Types.DefaultTypeVisitor<@Nullable Vo @Override public @Nullable Void visitType(Type subtype, Type supertype) { - // handle flow into a type variable. the check for !(subtype instanceof TypeVar) is a + if (config.handleWildcardGenerics()) { + WildcardType supertypeWildcard = GenericsUtils.asWildcard(supertype); + if (supertypeWildcard != null) { + Verify.verify(!localVariableType, "A local variable should not have a wildcard type"); + constrainSubtypeToWildcard(subtype, supertypeWildcard); + return null; + } + } + // handle flow into a type variable. The check for !(subtype instanceof TypeVar) is a // small optimization, as that case should be handled in visitTypeVar. if (!localVariableType && (supertype instanceof TypeVar) && !(subtype instanceof TypeVar)) { directlyConstrainTypePair(subtype, supertype); @@ -116,17 +126,13 @@ class AddSubtypeConstraintsVisitor extends Types.DefaultTypeVisitor<@Nullable Vo int numTypeArgs = supertypeTypeArguments.size(); Verify.verify(numTypeArgs == subtypeTypeArguments.size()); for (int i = 0; i < numTypeArgs; i++) { - Type rhsTypeArg = supertypeTypeArguments.get(i); - Type lhsTypeArg = subtypeTypeArguments.get(i); - // constrain in both directions - // TODO should we have a more optimized way to equate two types? this just makes each - // type a subtype of the other - lhsTypeArg.accept(this, rhsTypeArg); - rhsTypeArg.accept(this, lhsTypeArg); + Type supertypeTypeArg = supertypeTypeArguments.get(i); + Type subtypeTypeArg = subtypeTypeArguments.get(i); + constrainTypeArgumentContainment(subtypeTypeArg, supertypeTypeArg); } } // if supertype is not a ClassType, we still call visitType to handle the case where - // supertype is a TypeVar + // supertype is a TypeVar or a wildcard return visitType(subtype, supertype); } @@ -141,7 +147,7 @@ class AddSubtypeConstraintsVisitor extends Types.DefaultTypeVisitor<@Nullable Vo subtypeComponentType.accept(this, superComponentType); } // if supertype is not an ArrayType, we still call visitType to handle the case where - // supertype is a TypeVar + // supertype is a TypeVar or a wildcard return visitType(subtype, supertype); } @@ -152,6 +158,102 @@ class AddSubtypeConstraintsVisitor extends Types.DefaultTypeVisitor<@Nullable Vo } return visitType(subtype, supertype); } + + @Override + public @Nullable Void visitWildcardType(WildcardType subtype, Type supertype) { + if (config.handleWildcardGenerics()) { + Verify.verify(!localVariableType, "A wildcard type cannot be assigned to a local variable"); + constrainWildcardToSupertype(subtype, supertype); + } + return null; + } + + /** + * Adds nullability constraints for containment of one type argument by another during generic + * class/interface subtyping. For non-wildcard arguments, NullAway requires identical + * nullability. When either side is a wildcard, containment is reduced to constraints between + * the wildcard bound and the opposing argument. + */ + private void constrainTypeArgumentContainment(Type subtypeTypeArg, Type supertypeTypeArg) { + if (!config.handleWildcardGenerics()) { + equateTypeArguments(subtypeTypeArg, supertypeTypeArg); + return; + } + WildcardType supertypeWildcard = GenericsUtils.asWildcard(supertypeTypeArg); + if (supertypeWildcard != null) { + constrainContainedByWildcard(subtypeTypeArg, supertypeWildcard); + return; + } + WildcardType subtypeWildcard = GenericsUtils.asWildcard(subtypeTypeArg); + if (subtypeWildcard != null) { + constrainWildcardToSupertype(subtypeWildcard, supertypeTypeArg); + return; + } + equateTypeArguments(subtypeTypeArg, supertypeTypeArg); + } + + private void equateTypeArguments(Type subtypeTypeArg, Type supertypeTypeArg) { + // constrain in both directions + // TODO should we have a more optimized way to equate two types? this just makes each + // type a subtype of the other + subtypeTypeArg.accept(this, supertypeTypeArg); + supertypeTypeArg.accept(this, subtypeTypeArg); + } + + /** + * Adds constraints for type-argument containment where the formal argument is a wildcard. For + * {@code ? extends S} and {@code ?}, containment requires the actual argument's effective upper + * bound to be a subtype of {@code S}. For {@code ? super S}, concrete actual arguments require + * {@code S <: subtypeTypeArg}; {@code ? super T} actual arguments require {@code S <: T}. Other + * actual wildcard forms place no useful nullability constraint. + */ + private void constrainContainedByWildcard(Type subtypeTypeArg, WildcardType supertypeWildcard) { + switch (supertypeWildcard.kind) { + case UNBOUND, EXTENDS -> { + Type subtypeUpperBound = GenericsUtils.effectiveWildcardUpperBound(subtypeTypeArg, state); + subtypeUpperBound.accept( + this, GenericsUtils.wildcardUpperBound(supertypeWildcard, state)); + } + case SUPER -> { + Type supertypeLowerBound = castToNonNull(supertypeWildcard.getSuperBound()); + WildcardType subtypeWildcard = GenericsUtils.asWildcard(subtypeTypeArg); + if (subtypeWildcard != null) { + if (subtypeWildcard.kind == BoundKind.SUPER) { + supertypeLowerBound.accept(this, castToNonNull(subtypeWildcard.getSuperBound())); + } + // the subtype wildcard could have an extends bound, but as far as I know we do not + // need to generate constraints for this case + // TODO revisit if needed + } else { + supertypeLowerBound.accept(this, subtypeTypeArg); + } + } + } + } + + /** + * Adds constraints for a top-level subtype relation {@code subtype <: supertypeWildcard}. For + * {@code ? extends S} and {@code ?}, this reduces to {@code subtype <: S}. A {@code ? super S} + * supertype places no useful nullability constraint on {@code subtype}. + */ + private void constrainSubtypeToWildcard(Type subtype, WildcardType supertypeWildcard) { + if (supertypeWildcard.kind != BoundKind.SUPER) { + subtype.accept(this, GenericsUtils.wildcardUpperBound(supertypeWildcard, state)); + } + } + + /** + * Adds constraints for a top-level subtype relation {@code subtypeWildcard <: supertype}. For + * {@code ? extends S} and {@code ?}, this reduces to {@code S <: supertype}. For {@code ? super + * S}, use the lower bound and reduce to {@code S <: supertype}. + */ + private void constrainWildcardToSupertype(WildcardType subtypeWildcard, Type supertype) { + if (subtypeWildcard.kind == BoundKind.SUPER) { + castToNonNull(subtypeWildcard.getSuperBound()).accept(this, supertype); + } else { + GenericsUtils.wildcardUpperBound(subtypeWildcard, state).accept(this, supertype); + } + } } @Override @@ -272,10 +374,9 @@ private VarState getState(Element typeVarElement) { private boolean isTypeVariable(Type t) { if (t instanceof TypeVar tv) { - // For now ignore capture variables, like "capture#1 of ? extends X". Also, only treat as a - // type variable if it _doesn't_ have an explicit @Nullable or @NonNull annotation. - return !tv.isCaptured() - && !Nullness.hasNullableAnnotation(tv.getAnnotationMirrors().stream(), config) + // Only treat as a type variable if it _doesn't_ have an explicit @Nullable or @NonNull + // annotation. + return !Nullness.hasNullableAnnotation(tv.getAnnotationMirrors().stream(), config) && !Nullness.hasNonNullAnnotation(tv.getAnnotationMirrors().stream(), config); } else { return false; @@ -298,9 +399,9 @@ private boolean upperBoundIsNullable(Element typeVarElement) { } // first, check if library model overrides the upper bound nullability Element enclosingElement = typeVarElement.getEnclosingElement(); - if (enclosingElement instanceof Symbol.MethodSymbol methodSymbol) { - int typeVarIndex = - methodSymbol.getTypeParameters().indexOf((Symbol.TypeVariableSymbol) typeVarElement); + if (enclosingElement instanceof Symbol.MethodSymbol methodSymbol + && typeVarElement instanceof Symbol.TypeVariableSymbol typeVariableSymbol) { + int typeVarIndex = methodSymbol.getTypeParameters().indexOf(typeVariableSymbol); // TODO typeVarIndex is -1 in some cases; see test // com.uber.nullaway.jspecify.GenericMethodTests.instanceGenericMethodWithMethodRefArgument. // Investigate further. @@ -308,9 +409,9 @@ private boolean upperBoundIsNullable(Element typeVarElement) { && handler.onOverrideMethodTypeVariableUpperBound(methodSymbol, typeVarIndex, state)) { return true; } - } else if (enclosingElement instanceof Symbol.ClassSymbol classSymbol) { - int typeVarIndex = - classSymbol.getTypeParameters().indexOf((Symbol.TypeVariableSymbol) typeVarElement); + } else if (enclosingElement instanceof Symbol.ClassSymbol classSymbol + && typeVarElement instanceof Symbol.TypeVariableSymbol typeVariableSymbol) { + int typeVarIndex = classSymbol.getTypeParameters().indexOf(typeVariableSymbol); if (typeVarIndex >= 0 && handler.onOverrideClassTypeVariableUpperBound(classSymbol.toString(), typeVarIndex)) { return true; @@ -324,8 +425,11 @@ private boolean upperBoundIsNullable(Element typeVarElement) { } private boolean fromUnannotatedMethod(Element typeVarElement) { - Symbol enclosingElement = (Symbol) typeVarElement.getEnclosingElement(); - return enclosingElement != null - && codeAnnotationInfo.isSymbolUnannotated(enclosingElement, config, handler); + Element enclosingElement = typeVarElement.getEnclosingElement(); + if (!(enclosingElement instanceof Symbol.MethodSymbol) + && !(enclosingElement instanceof Symbol.ClassSymbol)) { + return false; + } + return codeAnnotationInfo.isSymbolUnannotated((Symbol) enclosingElement, config, handler); } } diff --git a/nullaway/src/main/java/com/uber/nullaway/generics/GenericsUtils.java b/nullaway/src/main/java/com/uber/nullaway/generics/GenericsUtils.java index 968ac6603a..f63f45dac3 100644 --- a/nullaway/src/main/java/com/uber/nullaway/generics/GenericsUtils.java +++ b/nullaway/src/main/java/com/uber/nullaway/generics/GenericsUtils.java @@ -4,12 +4,17 @@ import com.google.errorprone.VisitorState; import com.google.errorprone.util.ASTHelpers; import com.sun.source.tree.MemberReferenceTree; +import com.sun.tools.javac.code.BoundKind; import com.sun.tools.javac.code.Symbol; +import com.sun.tools.javac.code.Symtab; import com.sun.tools.javac.code.Type; +import com.sun.tools.javac.code.Type.CapturedType; +import com.sun.tools.javac.code.Type.WildcardType; import com.sun.tools.javac.code.Types; import com.sun.tools.javac.tree.JCTree; import com.uber.nullaway.NullabilityUtil; import javax.lang.model.type.TypeKind; +import org.jspecify.annotations.Nullable; /** Utility methods for doing generics-related checking */ public class GenericsUtils { @@ -22,6 +27,55 @@ enum MethodRefTypeRelationKind { PARAMETER } + /** + * Returns the effective upper bound of {@code typeArg}. For concrete type arguments, returns the + * type itself. For wildcards and captured wildcards, returns the wildcard's upper bound, + * recursing through nested wildcards and captures produced by javac. + */ + static Type effectiveWildcardUpperBound(Type typeArg, VisitorState state) { + WildcardType wildcardType = asWildcard(typeArg); + return wildcardType == null ? typeArg : wildcardUpperBound(wildcardType, state); + } + + /** + * Returns the effective upper bound of a wildcard, using the corresponding type variable's upper + * bound for unbounded wildcards and {@code super} wildcards. + */ + static Type wildcardUpperBound(WildcardType wildcardType, VisitorState state) { + Type upperBound; + if (wildcardType.kind == BoundKind.EXTENDS) { + upperBound = wildcardType.getExtendsBound(); + } else { + // We have an unbound wildcard or a wildcard with just a lower bound. In such cases, if + // present, we use the upper bound of the formal type variable to which the wildcard is being + // passed (confusingly stored in the `bound` field). E.g., if we have class Foo, and then see Foo, we use @Nullable Object as the upper + // bound. If not present, default to Object. + Type.TypeVar formalTypeVar = wildcardType.bound; + upperBound = + formalTypeVar == null + ? Symtab.instance(state.context).objectType + : formalTypeVar.getUpperBound(); + } + if (upperBound instanceof WildcardType nestedWildcard) { + return wildcardUpperBound(nestedWildcard, state); + } + if (upperBound instanceof CapturedType capturedType && capturedType.wildcard != null) { + return wildcardUpperBound(capturedType.wildcard, state); + } + return upperBound; + } + + static @Nullable WildcardType asWildcard(Type typeArg) { + if (typeArg instanceof WildcardType wildcardType) { + return wildcardType; + } + if (typeArg instanceof CapturedType capturedType) { + return capturedType.wildcard; + } + return null; + } + /** * Handler for method reference type relations, used by {{@link * #processMethodRefTypeRelations(GenericsChecks, Type, MemberReferenceTree, VisitorState, diff --git a/nullaway/src/main/java/com/uber/nullaway/generics/TypeSubstitutionUtils.java b/nullaway/src/main/java/com/uber/nullaway/generics/TypeSubstitutionUtils.java index 575552d3a5..a89d94d400 100644 --- a/nullaway/src/main/java/com/uber/nullaway/generics/TypeSubstitutionUtils.java +++ b/nullaway/src/main/java/com/uber/nullaway/generics/TypeSubstitutionUtils.java @@ -293,13 +293,15 @@ public Type visitMethodType(Type.MethodType t, Type other) { @Override public Type visitClassType(Type.ClassType t, Type other) { - if (other instanceof Type.WildcardType wt && wt.kind == BoundKind.EXTENDS) { - // As a temporary measure, we restore nullability annotations from the upper bound of the - // wildcard. - // TODO revisit this decision when we add fuller support for inference and wildcards. - return visit(t, wt.getExtendsBound()); + if (other instanceof Type.WildcardType wt) { + // When the other type is a wildcard, restore nullability annotations from the bound that + // determines the functional interface type. + if (wt.kind == BoundKind.EXTENDS) { + return visit(t, wt.getExtendsBound()); + } else if (wt.kind == BoundKind.SUPER) { + return visit(t, wt.getSuperBound()); + } } - Type updated = updateDirectNullabilityAnnotationsForType(t, other); if (!(other instanceof Type.ClassType)) { return updated; 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 bcf6ba4037..a7a71ee31b 100644 --- a/nullaway/src/test/java/com/uber/nullaway/jspecify/WildcardTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/jspecify/WildcardTests.java @@ -341,7 +341,6 @@ void testLocals( .doTest(); } - @Ignore("bad interaction between wildcard support and generic method inference") @Test public void wildcardSuperBoundsAndInference() { makeHelperWithInferenceFailureWarning() @@ -370,7 +369,63 @@ void test2(BiFunction {} + static void take(Box box) {} + static T get(Box box) { + throw new RuntimeException(); + } + Object field = new Object(); + void test(Box nullableBox, Box nonNullBox) { + take(nullableBox); + take(nonNullBox); + // TODO we need to report an additional error besides inference failure here + // See https://github.com/uber/NullAway/issues/1551 + // BUG: Diagnostic contains: Failed to infer type argument nullability + field = get(nullableBox); + field = get(nonNullBox); + } + } + """) + .doTest(); + } + + @Test + public void methodRefParameterExtendsWildcardToConcreteParameter() { + makeHelperWithInferenceFailureWarning() + .addSourceLines( + "Test.java", + """ + import org.jspecify.annotations.NullMarked; + import org.jspecify.annotations.Nullable; + @NullMarked + class Test { + interface Consumer { + void accept(T t); + } + static void acceptNullable(@Nullable String s) {} + static void acceptNonNull(String s) {} + static void use(Consumer consumer) {} + static void useNullable(Consumer consumer) {} + void test() { + use(Test::acceptNullable); + // BUG: Diagnostic contains: parameter s of referenced method is @NonNull + useNullable(Test::acceptNonNull); + } + } + """) + .doTest(); + } + @Test public void genericMethodLambdaArgWildCard() { makeHelperWithInferenceFailureWarning() @@ -388,12 +443,38 @@ static void test() { // legal, should infer R -> Object but then the type of the lambda as // Function via wildcard upper bound Object x = invokeWithReturn(t -> null); + x.hashCode(); } } """) .doTest(); } + @Ignore("https://github.com/uber/NullAway/issues/1522") + @Test + public void issue1522() { + makeHelperWithInferenceFailureWarning() + .addSourceLines( + "Test.java", + """ + import org.jspecify.annotations.*; + import java.util.function.Function; + import java.util.Optional; + @NullMarked + class Test { + static class Foo { + public final Foo mapNotNull(Function mapper) { + throw new RuntimeException(); + } + } + static Foo after(Foo> foo) { + return foo.mapNotNull(x -> x.orElse(null)); + } + } + """) + .doTest(); + } + /** * Extracted from Caffeine; exposed some subtle bugs in substitutions involving identity of {@code * Type} objects @@ -431,6 +512,68 @@ void test() { .doTest(); } + @Test + public void mapStreamValuesToNullable() { + makeHelperWithInferenceFailureWarning() + .addSourceLines( + "Test.java", + """ + import org.jspecify.annotations.*; + @NullMarked + class Test { + interface List { + Stream stream(); + } + interface Stream { + Stream map(Function mapper); + void forEach(Consumer action); + } + interface Function { + R apply(T t); + } + interface Consumer { + void accept(T t); + } + static @Nullable String mapToNull(String s) { + return null; + } + static void callHashCode(Object o) { o.hashCode(); } + static void test(List list) { + list.stream().map(Test::mapToNull).forEach(s -> { + // BUG: Diagnostic contains: dereferenced expression s is @Nullable + s.hashCode(); + }); + // TODO we should report an error here (https://github.com/uber/NullAway/issues/1552) + list.stream().map(Test::mapToNull).forEach(Test::callHashCode); + } + }""") + .doTest(); + } + + @Ignore("https://github.com/uber/NullAway/issues/1500") + @Test + public void issue1500() { + makeHelperWithInferenceFailureWarning() + .addSourceLines( + "Test.java", + """ + import org.jspecify.annotations.*; + @NullMarked + class Test { + static class Foo { + public static Foo of(Foo foo) { + return new Foo<>(); + } + + public Foo or(Foo other) { + return this; + } + } + static final Foo<@Nullable Void> FOO = Foo.of(new Foo<@Nullable Void>()).or(new Foo<@Nullable Void>()); + }""") + .doTest(); + } + private CompilationTestHelper makeHelper() { return makeTestHelperWithArgs( JSpecifyJavacConfig.withJSpecifyModeArgs(