Infer generics for assignments #1131
Conversation
|
@haewiful thanks for this! It looks like CI fails on the https://github.com/uber/NullAway/actions/runs/12978381939/job/36192901792?pr=1131#step:5:606 I suggest looking at the code it is crashing on and trying to write a unit test that causes the same failure, which will make it easier to debug. |
# Conflicts: # nullaway/src/main/java/com/uber/nullaway/NullAway.java
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1131 +/- ##
============================================
- Coverage 88.16% 88.13% -0.04%
- Complexity 2281 2307 +26
============================================
Files 87 88 +1
Lines 7461 7557 +96
Branches 1491 1513 +22
============================================
+ Hits 6578 6660 +82
- Misses 445 448 +3
- Partials 438 449 +11 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| Tree rhsTree; | ||
| if (tree instanceof VariableTree) { | ||
| VariableTree varTree = (VariableTree) tree; | ||
| rhsTree = varTree.getInitializer(); | ||
|
|
||
| if (rhsTree instanceof MethodInvocationTree) { |
There was a problem hiding this comment.
I think this logic is not in the right place, as it won't work for regular assignments; it only runs for variable declarations. I added a failing test to illustrate.
| import org.jspecify.annotations.Nullable; | ||
|
|
||
| /** Visitor that uses two types to infer the type of type variables. */ | ||
| public class InferTypeVisitor |
There was a problem hiding this comment.
I think that returning Maps from each visit method can work, but it's a bit convoluted and also inefficient (as many short-lived maps may be allocated). Instead, I suggest making the genericNullness map an instance field of InferTypeVisitor, initialized to an empty map. The visit methods could mutate this map. And then, at the end, the caller could call a method like getGenericNullessMap() to get the result. With this approach, this class could extend Types.DefaultTypeVisitor<Void, Type>, and all the visit methods would just return null.
| // rhsTree can be null for a VariableTree. Also, we don't need to do a check | ||
| // if rhsTree is the null literal | ||
| if (rhsTree == null || rhsTree.getKind().equals(Tree.Kind.NULL_LITERAL)) { | ||
| return; | ||
| } | ||
| Type rhsType = getTreeType(rhsTree, config); | ||
|
|
||
| if (lhsType != null && rhsType != null) { | ||
| if (rhsTree instanceof MethodInvocationTree) { |
There was a problem hiding this comment.
Couldn't we move the code under the if at line 440 to also be under this if? We'd still need to store genericNullness inside inferredTypes but then we could just use it immediately here to reconstruct the inferred type.
| List<Type> keyTypeList = | ||
| genericNullness.keySet().stream() | ||
| .map(typeVar -> (Type) typeVar) | ||
| .collect(Collectors.toList()); | ||
| com.sun.tools.javac.util.List<Type> from = com.sun.tools.javac.util.List.from(keyTypeList); | ||
| com.sun.tools.javac.util.List<Type> to = | ||
| com.sun.tools.javac.util.List.from(genericNullness.values()); |
There was a problem hiding this comment.
From this code, it's unclear that the from and to lists will end up in the appropriate order, where the right keys are lined up with the right values. (It might work with LinkedHashMaps but the code doesn't use those or enforce that right now.) Instead, I suggest looping over the entries of genericNullness, and building the lists one key-value pair at a time. You can build up the right list type using a ListBuffer and calling append on it, like here:
| " return new Foo<>(u);", | ||
| " }", | ||
| " static void test(Foo<@Nullable Object> f1, Foo<Object> f2) {", | ||
| " // no error expected", |
There was a problem hiding this comment.
This test currently does not pass. I would expect no error for the first call to create, but an error for the second call to create (since the second argument is Foo<Object>). Do you agree? If so we need to fix this.
# Conflicts: # nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java
| new GenericsChecks() | ||
| .getGenericReturnNullnessAtInvocation( | ||
| ASTHelpers.getSymbol(tree), tree, state, config); |
There was a problem hiding this comment.
Hrm, this seems suspect. If you create a new GenericsChecks object here, then the inferred types will immediately disappear. Do we need to instead somehow pass in the GenericsChecks object from the NullAway instance into this class?
Infer Nullability for assignments of instances with type parameters. ex)
Foo<@Nullable Object> f = Foo.make(null);void genericInferenceOnAssignments()in GenericMethodTests.java