-
Notifications
You must be signed in to change notification settings - Fork 337
Initial subtype checking for wildcards #1520
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from 10 commits
2deb123
136e0c2
4ccd737
7549ba8
5117398
67b1649
856f0bc
13bfcda
0a2a52a
9808d2c
f3de275
adcfe94
2f272e4
90e07cd
e7cc1c5
ffe3ad6
13beea4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -3,6 +3,7 @@ | |||||||||||||
| import static org.junit.Assert.assertTrue; | ||||||||||||||
|
|
||||||||||||||
| import java.io.IOException; | ||||||||||||||
| import org.junit.Ignore; | ||||||||||||||
| import org.junit.Test; | ||||||||||||||
|
|
||||||||||||||
| /** Tests that all our JMH benchmarks compile successfully */ | ||||||||||||||
|
|
@@ -13,6 +14,7 @@ public void testAutodispose() throws IOException { | |||||||||||||
| assertTrue(new AutodisposeCompiler().compile()); | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| @Ignore("Fails after wildcard nullability checking changes; needs benchmark-specific follow-up") | ||||||||||||||
| @Test | ||||||||||||||
|
Comment on lines
+17
to
18
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do not suppress this regression with Skipping Proposed change- `@Ignore`("Fails after wildcard nullability checking changes; needs benchmark-specific follow-up")
`@Test`
public void testCaffeine() throws IOException {
assertTrue(new CaffeineCompiler().compile());
}📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||
| public void testCaffeine() throws IOException { | ||||||||||||||
| assertTrue(new CaffeineCompiler().compile()); | ||||||||||||||
|
|
||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 | ||
| * NullAway's nullability-aware subtype relation. Non-wildcard pairs require matching nullability | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we are implementing JSpecify semantics here, any section of their spec we want to reference in addition to JLS 4.5.1? |
||
| * annotations and recursively matching nested type arguments. Wildcard formals are delegated to | ||
| * {@link #wildcardContains}. | ||
| */ | ||
| private boolean typeArgumentContainedBy(Type lhsTypeArgument, Type rhsTypeArgument) { | ||
| if (lhsTypeArgument.getKind().equals(TypeKind.WILDCARD)) { | ||
| return wildcardContains((Type.WildcardType) lhsTypeArgument, rhsTypeArgument); | ||
| } | ||
| if (rhsTypeArgument.getKind().equals(TypeKind.WILDCARD)) { | ||
| // This branch covers the remaining unsupported case where the formal type argument is not a | ||
| // wildcard but the actual type argument is a wildcard. | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: I'd probablyy track these TODOs in a linked issue, but, again, you are probably the only person actually implementing this now, so as long as you can keep track of what's left todo... 😅 |
||
| return true; | ||
| } | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| 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; | ||
|
msridhar marked this conversation as resolved.
|
||
| } | ||
| Type lhsBound = lhsWildcard.getExtendsBound(); | ||
| if (lhsBound == null) { | ||
| return true; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When is this reachable? Or is it just defensive programming / null checking? (I ask because we filter for non-wildcards in the argument type of the method, and for wildcards without extends in the previous check, so... wonder if I am missing a case. |
||
| } | ||
| 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(); | ||
| if (rhsBound == null) { | ||
| return true; | ||
| } | ||
| 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); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,7 +9,34 @@ | |
|
|
||
| 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<T extends @Nullable Object> {} | ||
| String nullableWildcard(Foo<? extends @Nullable String> foo) { throw new RuntimeException(); } | ||
| String nonnullWildcard(Foo<? extends String> foo) { throw new RuntimeException(); } | ||
| void testNegative(Foo<@Nullable String> f) { | ||
| // this is legal since the wildcard upper bound is @Nullable | ||
| String s = nullableWildcard(f); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This might be already included in further tests below, but, if not, we do want to check a
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Didn't see it below, so I'd suggest maybe having those two extra cases? They don't need to be mirrored on every single follow up test, but it's probably worth having them once in their simplest version. |
||
| s.hashCode(); | ||
| } | ||
| void testPositive(Foo<@Nullable String> f) { | ||
| // 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<? extends String> | ||
| String s = nonnullWildcard(f); | ||
| s.hashCode(); | ||
| } | ||
| } | ||
| """) | ||
| .doTest(); | ||
| } | ||
|
|
||
| @Test | ||
| public void simpleWildcard() { | ||
| makeHelper() | ||
|
|
@@ -29,7 +56,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 | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not a comment on this PR but... uh? How did this test pass before? Unless we were literally emitting an error with "something about how f cannot be passed here" in its message? |
||
| // BUG: Diagnostic contains: incompatible types: Test.Foo<@Nullable String> cannot be converted to Test.Foo<? extends String> | ||
| String s = nonnullWildcard(f); | ||
| s.hashCode(); | ||
| } | ||
| } | ||
| """) | ||
| .doTest(); | ||
| } | ||
|
|
||
| @Test | ||
| public void nestedTypeArgsInWildcardBoundNoInference() { | ||
| makeHelper() | ||
| .addSourceLines( | ||
| "Test.java", | ||
| """ | ||
| import org.jspecify.annotations.*; | ||
| @NullMarked | ||
| class Test { | ||
| class Foo<T extends @Nullable Object> {} | ||
| class Bar<T extends @Nullable Object> {} | ||
| String nullableWildcard(Foo<? extends Bar<@Nullable String>> foo) { | ||
| throw new RuntimeException(); | ||
| } | ||
| String nonnullWildcard(Foo<? extends Bar<String>> foo) { | ||
| throw new RuntimeException(); | ||
| } | ||
| void testNegative(Foo<Bar<@Nullable String>> f) { | ||
| String s = nullableWildcard(f); | ||
| s.hashCode(); | ||
| } | ||
| void testPositive(Foo<Bar<@Nullable String>> f) { | ||
| // BUG: Diagnostic contains: incompatible types: Test.Foo<Test.Bar<@Nullable String>> cannot be converted to Test.Foo<? extends Test.Bar<String>> | ||
| String s = nonnullWildcard(f); | ||
| s.hashCode(); | ||
| } | ||
| } | ||
| """) | ||
| .doTest(); | ||
| } | ||
|
|
||
| @Test | ||
| public void deeplyNestedTypeArgsInWildcardBoundNoInference() { | ||
| makeHelper() | ||
| .addSourceLines( | ||
| "Test.java", | ||
| """ | ||
| import org.jspecify.annotations.*; | ||
| @NullMarked | ||
| class Test { | ||
| class Foo<T extends @Nullable Object> {} | ||
| class Bar<T extends @Nullable Object> {} | ||
| class Baz<T extends @Nullable Object> {} | ||
| String nullableWildcard(Foo<? extends Bar<Baz<@Nullable String>>> foo) { | ||
| throw new RuntimeException(); | ||
| } | ||
| String nonnullWildcard(Foo<? extends Bar<Baz<String>>> foo) { | ||
| throw new RuntimeException(); | ||
| } | ||
| void testNegative(Foo<Bar<Baz<@Nullable String>>> f) { | ||
| String s = nullableWildcard(f); | ||
| s.hashCode(); | ||
| } | ||
| void testPositive(Foo<Bar<Baz<@Nullable String>>> f) { | ||
| // BUG: Diagnostic contains: incompatible types: Test.Foo<Test.Bar<Test.Baz<@Nullable String>>> cannot be converted to Test.Foo<? extends Test.Bar<Test.Baz<String>>> | ||
| String s = nonnullWildcard(f); | ||
| s.hashCode(); | ||
| } | ||
|
|
@@ -38,6 +128,102 @@ void testPositive(Foo<@Nullable String> f) { | |
| .doTest(); | ||
| } | ||
|
|
||
| @Test | ||
| public void intermediateNestedTypeArgsInWildcardBoundNoInference() { | ||
| makeHelper() | ||
| .addSourceLines( | ||
| "Test.java", | ||
| """ | ||
| import org.jspecify.annotations.*; | ||
| @NullMarked | ||
| class Test { | ||
| class Foo<T extends @Nullable Object> {} | ||
| class Bar<T extends @Nullable Object> {} | ||
| class Baz<T extends @Nullable Object> {} | ||
| String nullableWildcard(Foo<? extends @Nullable Bar<Baz<String>>> foo) { | ||
| throw new RuntimeException(); | ||
| } | ||
| String nonnullWildcard(Foo<? extends Bar<Baz<String>>> foo) { | ||
| throw new RuntimeException(); | ||
| } | ||
| void testNegative(Foo<@Nullable Bar<Baz<String>>> f) { | ||
| String s = nullableWildcard(f); | ||
| s.hashCode(); | ||
| } | ||
| void testPositive(Foo<@Nullable Bar<Baz<String>>> f) { | ||
| // BUG: Diagnostic contains: incompatible types: Test.Foo<Test.@Nullable Bar<Test.Baz<String>>> cannot be converted to Test.Foo<? extends Test.Bar<Test.Baz<String>>> | ||
| String s = nonnullWildcard(f); | ||
| s.hashCode(); | ||
| } | ||
| } | ||
| """) | ||
| .doTest(); | ||
| } | ||
|
|
||
| @Test | ||
| public void wildcardActualArgumentNoInference() { | ||
| makeHelper() | ||
| .addSourceLines( | ||
| "Test.java", | ||
| """ | ||
| import org.jspecify.annotations.*; | ||
| @NullMarked | ||
| class Test { | ||
| class Foo<T extends @Nullable Object> {} | ||
| String nullableWildcard(Foo<? extends @Nullable String> foo) { | ||
| throw new RuntimeException(); | ||
| } | ||
| String nonnullWildcard(Foo<? extends String> foo) { | ||
| throw new RuntimeException(); | ||
| } | ||
| void testNegative(Foo<? extends @Nullable String> f) { | ||
| String s = nullableWildcard(f); | ||
| s.hashCode(); | ||
| } | ||
| void testPositive(Foo<? extends @Nullable String> f) { | ||
| // BUG: Diagnostic contains: incompatible types: Test.Foo<? extends @Nullable String> cannot be converted to Test.Foo<? extends String> | ||
| String s = nonnullWildcard(f); | ||
| s.hashCode(); | ||
| } | ||
| } | ||
| """) | ||
| .doTest(); | ||
| } | ||
|
|
||
| @Test | ||
| public void wildcardCheckingForReturnsAndAssignments() { | ||
| makeHelper() | ||
| .addSourceLines( | ||
| "Test.java", | ||
| """ | ||
| import org.jspecify.annotations.*; | ||
| @NullMarked | ||
| class Test { | ||
| class Foo<T extends @Nullable Object> {} | ||
| Foo<? extends String> nonnullField; | ||
| Foo<? extends @Nullable String> nullableField; | ||
| Test(Foo<? extends @Nullable String> f) { | ||
| nullableField = f; | ||
| // BUG: Diagnostic contains: incompatible types: Test.Foo<? extends @Nullable String> cannot be converted to Test.Foo<? extends String> | ||
| nonnullField = f; | ||
| } | ||
| Foo<? extends @Nullable String> nullableReturn(Foo<? extends @Nullable String> f) { | ||
| return f; | ||
| } | ||
| Foo<? extends String> nonnullReturn(Foo<? extends @Nullable String> f) { | ||
| // BUG: Diagnostic contains: incompatible types: Test.Foo<? extends @Nullable String> cannot be converted to Test.Foo<? extends String> | ||
| return f; | ||
| } | ||
| void testLocal(Foo<? extends @Nullable String> f) { | ||
| Foo<? extends @Nullable String> ok = f; | ||
| // BUG: Diagnostic contains: incompatible types: Test.Foo<? extends @Nullable String> cannot be converted to Test.Foo<? extends String> | ||
| Foo<? extends String> bad = f; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How does this interact with something like: right now? Not saying we need to handle it (not sure what kind of handling we have for Java's local type inference at all, actually, lost track of that a while back 😅 ), just curious. |
||
| } | ||
| } | ||
| """) | ||
| .doTest(); | ||
| } | ||
|
|
||
| @Ignore("https://github.com/uber/NullAway/issues/1350") | ||
| @Test | ||
| public void genericMethodLambdaArgWildCard() { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understood. Sorry for the possibly pointless bookkeeping given the number of current active contributors here (😅 ), but could we have an issue tracking this linked in the
@Ignore?