Skip to content
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

Infer generics for assignments #1131

Open
wants to merge 64 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 62 commits
Commits
Show all changes
64 commits
Select commit Hold shift + click to select a range
b3eba82
fix and test
msridhar Nov 5, 2024
bc9e5fb
another fix and test
msridhar Nov 5, 2024
174b262
add TODO
msridhar Nov 5, 2024
c94dc28
check explicit type arguments when comparing generic type param nulla…
haewiful Nov 11, 2024
32b87ab
formatting
haewiful Nov 11, 2024
e1dfa8a
add test case for explicit type arguments
haewiful Nov 11, 2024
be9c657
Merge branch 'master' into check-type-param
haewiful Nov 11, 2024
24ca5a1
apply substitution to method type
msridhar Nov 11, 2024
cc45e43
Merge branch 'master' into check-type-param
msridhar Nov 12, 2024
3f3ebf2
make function that substitutes explicit type arguments
haewiful Nov 12, 2024
7004926
test case
haewiful Nov 18, 2024
aaba869
add test for multiple generics
haewiful Dec 7, 2024
37ef4d1
temp structure of inferred_types cache - need to change map key type
haewiful Dec 7, 2024
c6a6244
infer on assignments draft
haewiful Jan 13, 2025
277e2b6
new test case for inference on assignments
haewiful Jan 13, 2025
e7346d3
update test case
haewiful Jan 13, 2025
bca1d7d
simple cleanup
haewiful Jan 15, 2025
453623a
formatting
haewiful Jan 15, 2025
9375170
Merge remote-tracking branch 'upstream/master' into infer-generics
haewiful Jan 15, 2025
db0b6ce
minor changes
haewiful Jan 15, 2025
35b18cf
move cache into GenericsChecks and make related methods nonstatic
haewiful Jan 15, 2025
0308a5b
Merge branch 'master' into infer-generics
msridhar Jan 26, 2025
f5ece94
formatting
msridhar Jan 26, 2025
f89fef0
fix CI Job failure
haewiful Jan 28, 2025
0c577ae
remove unused code
haewiful Jan 28, 2025
61e4f3e
debugged for testJdk23
haewiful Feb 3, 2025
9f6c91b
fetch and merge
haewiful Feb 3, 2025
8a30816
Merge branch 'master' into infer-generics
haewiful Feb 3, 2025
4f4ca89
recreated :caffeine:compileTestJava error
haewiful Feb 4, 2025
d96769a
update test case
haewiful Feb 4, 2025
b3dda68
add scenarios to test cases
haewiful Feb 5, 2025
5f1e08f
add expected errors for testcase
haewiful Feb 6, 2025
1e9699a
add upper bound checks
haewiful Feb 6, 2025
a2489ea
update NullAway.java
haewiful Feb 6, 2025
ddf2440
formatting
msridhar Feb 7, 2025
24241b5
remove unnecessary parameter
msridhar Feb 7, 2025
fbedd9c
Merge branch 'master' into infer-generics
msridhar Feb 7, 2025
e07c020
update test cases
haewiful Feb 10, 2025
e15754a
version passing all tests
haewiful Feb 10, 2025
d5c4e5f
change according to comments
haewiful Feb 10, 2025
6c4430e
mid status
haewiful Feb 10, 2025
e4051cc
make code more simple
haewiful Feb 10, 2025
82628ca
make pass ./gradlew :nullaway:buildWithNullAway
haewiful Feb 10, 2025
430e855
Merge branch 'master' into infer-generics
haewiful Feb 10, 2025
f8c8d07
remove always true condition
haewiful Feb 10, 2025
6bc1d0c
make method for clearing inferredTypes cache
haewiful Feb 10, 2025
099fae7
Merge branch 'master' into infer-generics
msridhar Feb 13, 2025
29255bb
change based on comments
haewiful Feb 18, 2025
575006e
add visitor for infering types on assignments using method calls
haewiful Feb 21, 2025
2ab7a31
javadoc
haewiful Feb 21, 2025
61d6f95
update according to changed method definition
haewiful Feb 21, 2025
d984de2
Merge branch 'master' into infer-generics
haewiful Feb 21, 2025
f7c4a3f
add test case for return types with arrays
haewiful Feb 21, 2025
461c309
add array type inference to visitor
haewiful Feb 21, 2025
48bb542
clean up
haewiful Feb 22, 2025
5573800
change key of inferredTypes to MethodInvokationTree
haewiful Feb 22, 2025
bbae811
change inferredTypes value type to Map<TypeVariable, Type>
haewiful Feb 22, 2025
3c77d70
format
haewiful Feb 22, 2025
ea785ba
remove filter call
msridhar Feb 22, 2025
2aec29b
simplify
msridhar Feb 22, 2025
014fc93
add failing test
msridhar Feb 22, 2025
b45bbad
make it work for assignments that are not declarations
haewiful Feb 22, 2025
50f73de
simplify
msridhar Feb 22, 2025
cd2ec93
another test
msridhar Feb 22, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions nullaway/src/main/java/com/uber/nullaway/NullAway.java
Original file line number Diff line number Diff line change
Expand Up @@ -277,6 +277,8 @@ public Config getConfig() {
*/
private final Map<ExpressionTree, Nullness> computedNullnessMap = new LinkedHashMap<>();

private GenericsChecks genericsChecks = new GenericsChecks();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In a follow up (not for this PR), we should probably make a Config field inside GenericsChecks and then stop passing it as a parameter to a whole bunch of methods. Again not for this PR


/**
* Error Prone requires us to have an empty constructor for each Plugin, in addition to the
* constructor taking an ErrorProneFlags object. This constructor should not be used anywhere
Expand Down Expand Up @@ -500,7 +502,7 @@ public Description matchAssignment(AssignmentTree tree, VisitorState state) {
}
// generics check
if (lhsType != null && config.isJSpecifyMode()) {
GenericsChecks.checkTypeParameterNullnessForAssignability(tree, this, state);
genericsChecks.checkTypeParameterNullnessForAssignability(tree, this, state);
}

if (config.isJSpecifyMode() && tree.getVariable() instanceof ArrayAccessTree) {
Expand Down Expand Up @@ -1494,7 +1496,7 @@ public Description matchVariable(VariableTree tree, VisitorState state) {
}
VarSymbol symbol = ASTHelpers.getSymbol(tree);
if (tree.getInitializer() != null && config.isJSpecifyMode()) {
GenericsChecks.checkTypeParameterNullnessForAssignability(tree, this, state);
genericsChecks.checkTypeParameterNullnessForAssignability(tree, this, state);
}
if (!config.isLegacyAnnotationLocation()) {
checkNullableAnnotationPositionInType(
Expand Down Expand Up @@ -1662,6 +1664,7 @@ public Description matchClass(ClassTree tree, VisitorState state) {
class2Entities.clear();
class2ConstructorUninit.clear();
computedNullnessMap.clear();
genericsChecks.clearCache();
EnclosingEnvironmentNullness.instance(state.context).clear();
} else if (classAnnotationIntroducesPartialMarking(classSymbol)) {
// Handle the case where the top-class is unannotated, but there is a @NullMarked annotation
Expand Down Expand Up @@ -1880,7 +1883,7 @@ private Description handleInvocation(
Nullness.paramHasNullableAnnotation(methodSymbol, i, config)
? Nullness.NULLABLE
: ((config.isJSpecifyMode() && tree instanceof MethodInvocationTree)
? GenericsChecks.getGenericParameterNullnessAtInvocation(
? genericsChecks.getGenericParameterNullnessAtInvocation(
i, methodSymbol, (MethodInvocationTree) tree, state, config)
: Nullness.NONNULL);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.stream.Collectors;
import javax.lang.model.type.ExecutableType;
import javax.lang.model.type.TypeKind;
import javax.lang.model.type.TypeVariable;
Expand All @@ -46,8 +47,12 @@
/** Methods for performing checks related to generic types and nullability. */
public final class GenericsChecks {

/** Do not instantiate; all methods should be static */
private GenericsChecks() {}
/**
* Maps a MethodInvocationTree to a set of type variables that are mapped to their inferred types.
* Any generic type parameter that are not explicitly stated are inferred and cached in this
* field.
*/
private final Map<MethodInvocationTree, Map<TypeVariable, Type>> inferredTypes = new HashMap<>();

/**
* Checks that for an instantiated generic type, {@code @Nullable} types are only used for type
Expand Down Expand Up @@ -413,13 +418,16 @@ private static void reportInvalidOverridingMethodParamTypeError(
* @param analysis the analysis object
* @param state the visitor state
*/
public static void checkTypeParameterNullnessForAssignability(
public void checkTypeParameterNullnessForAssignability(
Tree tree, NullAway analysis, VisitorState state) {
Config config = analysis.getConfig();
if (!config.isJSpecifyMode()) {
return;
}
Type lhsType = getTreeType(tree, config);
if (lhsType == null) {
return;
}
Tree rhsTree;
if (tree instanceof VariableTree) {
VariableTree varTree = (VariableTree) tree;
Expand All @@ -428,14 +436,49 @@ public static void checkTypeParameterNullnessForAssignability(
AssignmentTree assignmentTree = (AssignmentTree) tree;
rhsTree = assignmentTree.getExpression();
}

if (rhsTree instanceof MethodInvocationTree) {
MethodInvocationTree methodInvocationTree = (MethodInvocationTree) rhsTree;
Symbol.MethodSymbol methodSymbol = ASTHelpers.getSymbol(methodInvocationTree);
// update inferredTypes cache for assignments
// generic method call with no explicit generic arguments
if (methodSymbol.type instanceof Type.ForAll
&& methodInvocationTree.getTypeArguments().isEmpty()) {
Type returnType = methodSymbol.getReturnType();
Map<TypeVariable, Type> genericNullness =
returnType.accept(new InferTypeVisitor(config), lhsType);
if (genericNullness != null) {
inferredTypes.put(methodInvocationTree, genericNullness);
}
}
}
// 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 (rhsType != null && rhsTree instanceof MethodInvocationTree) {
// recreate rhsType using inferred types
MethodInvocationTree methodInvocationTree = (MethodInvocationTree) rhsTree;
Symbol.MethodSymbol methodSymbol = ASTHelpers.getSymbol(methodInvocationTree);
if (inferredTypes.containsKey(methodInvocationTree)) {
Map<TypeVariable, Type> genericNullness = inferredTypes.get(methodInvocationTree);
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());
Comment on lines +468 to +474
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

rhsType =
TypeSubstitutionUtils.subst(
state.getTypes(), methodSymbol.getReturnType(), from, to, config);
}
}

if (rhsType != null) {
boolean isAssignmentValid = subtypeParameterNullability(lhsType, rhsType, state, config);
if (!isAssignmentValid) {
reportInvalidAssignmentInstantiationError(tree, lhsType, rhsType, state, analysis);
Expand Down Expand Up @@ -929,7 +972,7 @@ private static Type substituteTypeArgsInGenericMethodType(
* @return Nullness of parameter at {@code paramIndex}, or {@code NONNULL} if the call does not
* invoke an instance method
*/
public static Nullness getGenericParameterNullnessAtInvocation(
public Nullness getGenericParameterNullnessAtInvocation(
int paramIndex,
Symbol.MethodSymbol invokedMethodSymbol,
MethodInvocationTree tree,
Expand All @@ -949,6 +992,21 @@ public static Nullness getGenericParameterNullnessAtInvocation(
getTypeNullness(substitutedParamTypes.get(paramIndex), config), Nullness.NULLABLE)) {
return Nullness.NULLABLE;
}
// check nullness of inferred types
if (inferredTypes.containsKey(tree)) {
Map<TypeVariable, Type> genericNullness = inferredTypes.get(tree);
List<Symbol.VarSymbol> parameters = invokedMethodSymbol.getParameters();
if (genericNullness.containsKey(parameters.get(paramIndex).type)) {
Type genericType = parameters.get(paramIndex).type;
Type inferredGenericType = genericNullness.get(genericType);
if (inferredGenericType != null
&& Objects.equals(getTypeNullness(inferredGenericType, config), Nullness.NULLABLE)) {
return Nullness.NULLABLE;
} else {
return Nullness.NONNULL;
}
}
}
}

if (!(tree.getMethodSelect() instanceof MemberSelectTree) || invokedMethodSymbol.isStatic()) {
Expand Down Expand Up @@ -1157,6 +1215,10 @@ public static boolean passingLambdaOrMethodRefWithGenericReturnToUnmarkedCode(
return callingUnannotated;
}

public void clearCache() {
inferredTypes.clear();
}

public static boolean isNullableAnnotated(Type type, Config config) {
return Nullness.hasNullableAnnotation(type.getAnnotationMirrors().stream(), config);
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
package com.uber.nullaway.generics;

import com.sun.tools.javac.code.Type;
import com.sun.tools.javac.code.Types;
import com.uber.nullaway.Config;
import com.uber.nullaway.Nullness;
import java.util.HashMap;
import java.util.Map;
import javax.lang.model.type.TypeVariable;
import org.jspecify.annotations.Nullable;

/** Visitor that uses two types to infer the type of type variables. */
public class InferTypeVisitor
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

extends Types.DefaultTypeVisitor<@Nullable Map<TypeVariable, Type>, Type> {
private final Config config;

InferTypeVisitor(Config config) {
this.config = config;
}

@Override
public @Nullable Map<TypeVariable, Type> visitClassType(Type.ClassType rhsType, Type lhsType) {
Map<TypeVariable, Type> genericNullness = new HashMap<>();
com.sun.tools.javac.util.List<Type> rhsTypeArguments = rhsType.getTypeArguments();
com.sun.tools.javac.util.List<Type> lhsTypeArguments =
((Type.ClassType) lhsType).getTypeArguments();
// get the inferred type for each type arguments and add them to genericNullness
for (int i = 0; i < rhsTypeArguments.size(); i++) {
Type rhsTypeArg = rhsTypeArguments.get(i);
Type lhsTypeArg = lhsTypeArguments.get(i);
Map<TypeVariable, Type> map = rhsTypeArg.accept(this, lhsTypeArg);
if (map != null) {
genericNullness.putAll(map);
}
}
return genericNullness.isEmpty() ? null : genericNullness;
}

@Override
public @Nullable Map<TypeVariable, Type> visitArrayType(Type.ArrayType rhsType, Type lhsType) {
// unwrap the type of the array and call accept on it
Type rhsComponentType = rhsType.elemtype;
Type lhsComponentType = ((Type.ArrayType) lhsType).elemtype;
Map<TypeVariable, Type> genericNullness = rhsComponentType.accept(this, lhsComponentType);
return genericNullness;
}

@Override
public Map<TypeVariable, Type> visitTypeVar(Type.TypeVar rhsType, Type lhsType) {
Map<TypeVariable, Type> genericNullness = new HashMap<>();
Boolean isLhsNullable =
Nullness.hasNullableAnnotation(lhsType.getAnnotationMirrors().stream(), config);
Type upperBound = rhsType.getUpperBound();
Boolean isRhsNullable =
Nullness.hasNullableAnnotation(upperBound.getAnnotationMirrors().stream(), config);
if (!isLhsNullable) { // lhsType is NonNull, we can just use this
genericNullness.put(rhsType, lhsType);
} else if (isRhsNullable) { // lhsType & rhsType are Nullable, can use lhs for inference
genericNullness.put(rhsType, lhsType);
} else { // rhs can't be nullable, use upperbound
genericNullness.put(rhsType, upperBound);
}
return genericNullness;
}

@Override
public @Nullable Map<TypeVariable, Type> visitType(Type t, Type type) {
return null;

Check warning on line 68 in nullaway/src/main/java/com/uber/nullaway/generics/InferTypeVisitor.java

View check run for this annotation

Codecov / codecov/patch

nullaway/src/main/java/com/uber/nullaway/generics/InferTypeVisitor.java#L68

Added line #L68 was not covered by tests
}
}
Loading
Loading