JSpecify mode: initial support for generic methods (with explicit type arguments at calls) #1053
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1053 +/- ##
============================================
+ Coverage 87.60% 87.65% +0.05%
- Complexity 2204 2222 +18
============================================
Files 85 85
Lines 7195 7269 +74
Branches 1416 1436 +20
============================================
+ Hits 6303 6372 +69
Misses 456 456
- Partials 436 441 +5 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
This is a great start! I have a few comments. Also, can we add some tests for a method like this:
public static <T, U> void twoTypeVars(T first, U second) {
}Maybe write some variants where only one of the type variables has a @Nullable upper bound, and some calls where the wrong type argument is made @Nullable. This is just to check that our indexing logic is matching type arguments to type variables correctly.
nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java
Outdated
Show resolved
Hide resolved
|
|
||
| // base type that is being instantiated | ||
| Type baseType = methodSymbol.asType(); | ||
| List<Type> baseTypeArgs = baseType.getTypeArguments(); |
There was a problem hiding this comment.
Let's rename:
| List<Type> baseTypeArgs = baseType.getTypeArguments(); | |
| List<Type> baseTypeVariables = baseType.getTypeArguments(); |
| @@ -715,6 +785,40 @@ public static Nullness getGenericReturnNullnessAtInvocation( | |||
| MethodInvocationTree tree, | |||
| VisitorState state, | |||
| Config config) { | |||
|
|
|||
There was a problem hiding this comment.
We need to update the Javadoc of getGenericReturnNullnessAtInvocation to indicate it also handles generic methods, in addition to instance methods of generic classes (what it handled before)
| @@ -715,6 +785,40 @@ public static Nullness getGenericReturnNullnessAtInvocation( | |||
| MethodInvocationTree tree, | |||
| VisitorState state, | |||
| Config config) { | |||
|
|
|||
| List<? extends Tree> typeArgumentTrees = tree.getTypeArguments(); | |||
There was a problem hiding this comment.
Does this new logic only apply to generic methods? If so, can we only run it for that case, by checking if the invoked method has any type variables, !invokedMethodSymbol.getTypeParameters().isEmpty()?
| // If it's not a ForAll type, handle it as a normal MethodType | ||
| Type.MethodType methodTypeElse = (Type.MethodType) invokedMethodSymbol.type; | ||
| substitutedReturnType = | ||
| state | ||
| .getTypes() | ||
| .subst( | ||
| methodTypeElse.restype, | ||
| invokedMethodSymbol.type.getTypeArguments(), | ||
| explicitTypeArgs); |
There was a problem hiding this comment.
I feel like if we only apply the new logic to generic methods, we will always get a ForAll type and won't need this else case
| @@ -768,6 +884,51 @@ public static Nullness getGenericParameterNullnessAtInvocation( | |||
| MethodInvocationTree tree, | |||
| VisitorState state, | |||
| Config config) { | |||
| List<? extends Tree> typeArgumentTrees = tree.getTypeArguments(); | |||
There was a problem hiding this comment.
Same comment as above regarding only running the new logic when a generic method is being invoked
| @@ -145,6 +148,83 @@ private static boolean[] getTypeParamsWithNullableUpperBound( | |||
| return result; | |||
| } | |||
|
|
|||
| /** | |||
| * Checks instantiated generic arguments of generic method calls. {@code @Nullable} types are only | |||
There was a problem hiding this comment.
| * Checks instantiated generic arguments of generic method calls. {@code @Nullable} types are only | |
| * Checks instantiated generic arguments of generic method calls. {@code @Nullable} types can only be |
| boolean hasNullableAnnotation = | ||
| Nullness.hasNullableAnnotation(annotationMirrors.stream(), config) | ||
| || handler.onOverrideTypeParameterUpperBound(baseType.tsym.toString(), i); | ||
| // if type argument does not have @Nullable annotation then the instantiation is invalid |
There was a problem hiding this comment.
| // if type argument does not have @Nullable annotation then the instantiation is invalid | |
| // if type variable's upper bound does not have @Nullable annotation then the instantiation is invalid |
| // problem is that it returns nullable from the declaration even though the parameter itself is | ||
| // nonnull |
There was a problem hiding this comment.
do we still want this new comment?
msridhar
changed the title
|
This support is incomplete (even for cases with explicit type arguments), but let's go ahead and land it and do things in pieces. Thanks! |
Thank you for contributing to NullAway!
Please note that once you click "Create Pull Request" you will be asked to sign our Uber Contributor License Agreement via CLA assistant.
Before pressing the "Create Pull Request" button, please provide the following:
Added support for generic identity functions with explicitly-typed nullness.
Related to the issue JSpecify: support generic methods #1035.
void genericNonNullIdentityFunction() and void genericNullAllowingIdentityFunction() unit tests are added to GenericMethodTests.java file.