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

JSpecify mode: initial support for generic methods (with explicit type arguments at calls) #1053

Merged
merged 27 commits into from
Oct 30, 2024
Merged
Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
08f7b6a
couple of tests
msridhar Jun 19, 2024
81fe9cf
Merge branch 'master' into generic-identity-function
msridhar Jun 19, 2024
0775fad
tweak
msridhar Jun 21, 2024
74e7b7b
mid-stage update
haewiful Sep 30, 2024
58a2737
mid status
haewiful Sep 30, 2024
931ee19
passes genericNullAllowingIdentityFunction test
haewiful Oct 4, 2024
b4975ad
a little code cleanup
haewiful Oct 4, 2024
d7379f8
check generic method's parameter's Nullness
haewiful Oct 8, 2024
09b0cb5
Merge branch 'master' into task2
haewiful Oct 8, 2024
b792205
deleted unused import
haewiful Oct 8, 2024
f2c365c
changed formatting for spotlessJavaCheck
haewiful Oct 9, 2024
e1e2af5
fix vararg test failures in JSpecifyVarargsTests.java file
haewiful Oct 14, 2024
85e9e78
formatting
haewiful Oct 14, 2024
c2437a0
Merge branch 'master' into task2
msridhar Oct 14, 2024
f5896df
Merge branch 'master' into task2
msridhar Oct 14, 2024
9a8b8a0
javadoc, run logic for only generic methods with type variables
haewiful Oct 16, 2024
1899ccc
added test for multiple type variables
haewiful Oct 17, 2024
5102342
updated some comments
haewiful Oct 17, 2024
b8fa230
fix nullaway errors
msridhar Oct 17, 2024
da00c99
change some comments & change for coverage
haewiful Oct 18, 2024
97d6b41
Merge branch 'master' into task2
haewiful Oct 21, 2024
8272843
test case for generic instance methods
haewiful Oct 30, 2024
dab6f07
Merge branch 'master' into task2
haewiful Oct 30, 2024
c2747a9
rename and docs
msridhar Oct 30, 2024
7c69532
cleanup logic
msridhar Oct 30, 2024
76623cc
more comments
msridhar Oct 30, 2024
641b9ee
tests for parameters
msridhar Oct 30, 2024
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
4 changes: 4 additions & 0 deletions nullaway/src/main/java/com/uber/nullaway/NullAway.java
Original file line number Diff line number Diff line change
Expand Up @@ -1853,6 +1853,10 @@ private Description handleInvocation(
if (config.isJSpecifyMode()) {
GenericsChecks.compareGenericTypeParameterNullabilityForCall(
formalParams, actualParams, varArgsMethod, this, state);
if (!methodSymbol.getTypeParameters().isEmpty()) {
GenericsChecks.checkInstantiationForGenericMethodCalls(
tree, state, this, config, handler);
}
}
}

Expand Down
152 changes: 151 additions & 1 deletion nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java
Original file line number Diff line number Diff line change
Expand Up @@ -26,16 +26,19 @@
import com.sun.tools.javac.code.Symbol;
import com.sun.tools.javac.code.TargetType;
import com.sun.tools.javac.code.Type;
import com.sun.tools.javac.tree.JCTree;
import com.uber.nullaway.CodeAnnotationInfo;
import com.uber.nullaway.Config;
import com.uber.nullaway.ErrorBuilder;
import com.uber.nullaway.ErrorMessage;
import com.uber.nullaway.NullAway;
import com.uber.nullaway.Nullness;
import com.uber.nullaway.handlers.Handler;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import javax.lang.model.type.ExecutableType;
import javax.lang.model.type.TypeKind;
import javax.lang.model.type.TypeVariable;
Expand Down Expand Up @@ -145,6 +148,81 @@ private static boolean[] getTypeParamsWithNullableUpperBound(
return result;
}

/**
* Checks instantiated generic arguments of generic method calls. {@code @Nullable} types can only
* be used for type variables that have a {@code @Nullable} upper bound.
*
* @param tree the tree representing the instantiated type
* @param state visitor state
* @param analysis the analysis object
* @param config the analysis config
* @param handler the handler instance
*/
public static void checkInstantiationForGenericMethodCalls(
msridhar marked this conversation as resolved.
Show resolved Hide resolved
Tree tree, VisitorState state, NullAway analysis, Config config, Handler handler) {
List<? extends Tree> typeArguments = ((MethodInvocationTree) tree).getTypeArguments();
if (typeArguments.isEmpty()) {
return;
}
// get Nullable annotated type arguments
MethodInvocationTree methodTree = (MethodInvocationTree) tree;
Map<Integer, Tree> nullableTypeArguments = new HashMap<>();
for (int i = 0; i < typeArguments.size(); i++) {
Tree curTypeArg = typeArguments.get(i);
if (curTypeArg instanceof AnnotatedTypeTree) {
AnnotatedTypeTree annotatedType = (AnnotatedTypeTree) curTypeArg;
for (AnnotationTree annotation : annotatedType.getAnnotations()) {
Type annotationType = ASTHelpers.getType(annotation);
if (annotationType != null
&& Nullness.isNullableAnnotation(annotationType.toString(), config)) {
nullableTypeArguments.put(i, curTypeArg);
break;
}
}
}
}
Symbol.MethodSymbol methodSymbol = ASTHelpers.getSymbol(methodTree);

// check if type variables are allowed to be Nullable
Type baseType = methodSymbol.asType();
List<Type> baseTypeVariables = baseType.getTypeArguments();
for (int i = 0; i < baseTypeVariables.size(); i++) {
if (nullableTypeArguments.containsKey(i)) {
Type typeVariable = baseTypeVariables.get(i);
Type upperBound = typeVariable.getUpperBound();
com.sun.tools.javac.util.List<Attribute.TypeCompound> annotationMirrors =
upperBound.getAnnotationMirrors();
boolean hasNullableAnnotation =
Nullness.hasNullableAnnotation(annotationMirrors.stream(), config)
|| handler.onOverrideTypeParameterUpperBound(baseType.tsym.toString(), i);
// if type variable's upper bound does not have @Nullable annotation then the instantiation
// is invalid
if (!hasNullableAnnotation) {
reportInvalidTypeArgumentError(
nullableTypeArguments.get(i), methodSymbol, typeVariable, state, analysis);
}
}
}
}

private static void reportInvalidTypeArgumentError(
Tree tree,
Symbol.MethodSymbol methodSymbol,
Type typeVariable,
VisitorState state,
NullAway analysis) {
ErrorBuilder errorBuilder = analysis.getErrorBuilder();
ErrorMessage errorMessage =
new ErrorMessage(
ErrorMessage.MessageTypes.TYPE_PARAMETER_CANNOT_BE_NULLABLE,
String.format(
"Type argument cannot be @Nullable, as method %s's type variable %s is not @Nullable",
methodSymbol.toString(), typeVariable.tsym.toString()));
state.reportMatch(
errorBuilder.createErrorDescription(
errorMessage, analysis.buildDescription(tree), state, null));
}

private static void reportInvalidInstantiationError(
Tree tree, Type baseType, Type baseTypeVariable, VisitorState state, NullAway analysis) {
ErrorBuilder errorBuilder = analysis.getErrorBuilder();
Expand Down Expand Up @@ -682,7 +760,8 @@ public static Nullness getGenericMethodReturnTypeNullness(
/**
* Computes the nullness of the return of a generic method at an invocation, in the context of the
* declared type of its receiver argument. If the return type is a type variable, its nullness
* depends on the nullability of the corresponding type parameter in the receiver's type.
* depends on the nullability of the corresponding type parameter in the receiver's type or the
* type argument of the method call.
msridhar marked this conversation as resolved.
Show resolved Hide resolved
*
* <p>Consider the following example:
*
Expand Down Expand Up @@ -715,6 +794,33 @@ public static Nullness getGenericReturnNullnessAtInvocation(
MethodInvocationTree tree,
VisitorState state,
Config config) {
// If generic method invocation
if (!invokedMethodSymbol.getTypeParameters().isEmpty()) {
List<? extends Tree> typeArgumentTrees = tree.getTypeArguments();
com.sun.tools.javac.util.List<Type> explicitTypeArgs =
convertTreesToTypes(typeArgumentTrees); // Convert to Type objects

Type methodType = invokedMethodSymbol.type;
Type substitutedReturnType = null;
if (methodType instanceof Type.ForAll) {
Type.ForAll forAllType = (Type.ForAll) methodType;

// Extract the underlying MethodType (the actual signature)
Type.MethodType methodTypeInsideForAll = (Type.MethodType) forAllType.qtype;

// Substitute the argument and return types within the MethodType
substitutedReturnType =
state
.getTypes()
.subst(methodTypeInsideForAll.restype, forAllType.tvars, explicitTypeArgs);
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

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)

if (substitutedReturnType != null
&& Objects.equals(getTypeNullness(substitutedReturnType, config), Nullness.NULLABLE)) {
return Nullness.NULLABLE;
}
}

if (!(tree.getMethodSelect() instanceof MemberSelectTree) || invokedMethodSymbol.isStatic()) {
return Nullness.NONNULL;
}
Expand All @@ -728,6 +834,18 @@ public static Nullness getGenericReturnNullnessAtInvocation(
}
}

private static com.sun.tools.javac.util.List<Type> convertTreesToTypes(
List<? extends Tree> typeArgumentTrees) {
List<Type> types = new ArrayList<>();
for (Tree tree : typeArgumentTrees) {
if (tree instanceof JCTree.JCExpression) {
JCTree.JCExpression expression = (JCTree.JCExpression) tree;
types.add(expression.type); // Retrieve the Type
}
}
return com.sun.tools.javac.util.List.from(types);
}

/**
* Computes the nullness of a formal parameter of a generic method at an invocation, in the
* context of the declared type of its receiver argument. If the formal parameter's type is a type
Expand Down Expand Up @@ -768,6 +886,38 @@ public static Nullness getGenericParameterNullnessAtInvocation(
MethodInvocationTree tree,
VisitorState state,
Config config) {
// If generic method invocation
msridhar marked this conversation as resolved.
Show resolved Hide resolved
if (!invokedMethodSymbol.getTypeParameters().isEmpty()) {
List<? extends Tree> typeArgumentTrees = tree.getTypeArguments();
com.sun.tools.javac.util.List<Type> explicitTypeArgs =
convertTreesToTypes(typeArgumentTrees); // Convert to Type objects

Type methodType = invokedMethodSymbol.type;
List<Type> substitutedParamTypes = null;
if (methodType instanceof Type.ForAll) {
Type.ForAll forAllType = (Type.ForAll) methodType;

// Extract the underlying MethodType (the actual signature)
Type.MethodType methodTypeInsideForAll = (Type.MethodType) forAllType.qtype;

// Substitute the argument and return types within the MethodType
substitutedParamTypes =
state
.getTypes()
.subst(
methodTypeInsideForAll.argtypes,
forAllType.tvars, // The type variables from the ForAll
explicitTypeArgs // The actual type arguments from the method invocation
);
}

if (substitutedParamTypes != null
&& Objects.equals(
getTypeNullness(substitutedParamTypes.get(paramIndex), config), Nullness.NULLABLE)) {
return Nullness.NULLABLE;
}
}

if (!(tree.getMethodSelect() instanceof MemberSelectTree) || invokedMethodSymbol.isStatic()) {
return Nullness.NONNULL;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,74 @@

public class GenericMethodTests extends NullAwayTestsBase {

@Test
public void genericNonNullIdentityFunction() {
makeHelper()
.addSourceLines(
"Test.java",
"package com.uber;",
"import org.jspecify.annotations.Nullable;",
"class Test {",
" static <T> T nonNullIdentity(T t) {",
" return t;",
" }",
" static void test() {",
" // legal",
" nonNullIdentity(new Object()).toString();",
" // BUG: Diagnostic contains: passing @Nullable parameter 'null'",
" nonNullIdentity(null);",
" // BUG: Diagnostic contains: Type argument cannot be @Nullable",
" Test.<@Nullable Object>nonNullIdentity(new Object());",
" }",
"}")
.doTest();
}

@Test
public void genericNullAllowingIdentityFunction() {
makeHelper()
.addSourceLines(
"Test.java",
"package com.uber;",
"import org.jspecify.annotations.Nullable;",
"class Test {",
" static <T extends @Nullable Object> T identity(T t) {",
" return t;",
" }",
" static void test() {",
" // legal",
" identity(new Object()).toString();",
" // also legal",
" Test.<@Nullable Object>identity(null);",
" // BUG: Diagnostic contains: dereferenced expression",
" Test.<@Nullable Object>identity(null).toString();",
" }",
"}")
.doTest();
}

@Test
public void multipleTypeVariablesMethodCall() {
makeHelper()
.addSourceLines(
"Test.java",
"package com.uber;",
"import org.jspecify.annotations.Nullable;",
"import org.jspecify.annotations.NonNull;",
"class Test {",
" public static <T extends @Nullable Object, U> void twoTypeVars(T first, U second) {}",
" static void test() {",
" // legal",
" Test.<@NonNull Object, Object>twoTypeVars(new Object(), new Object());",
" // legal",
" Test.<@Nullable Object, Object>twoTypeVars(null, new Object());",
" // BUG: Diagnostic contains: Type argument cannot be @Nullable",
" Test.<@Nullable Object, @Nullable Object>twoTypeVars(null, null);",
" }",
"}")
.doTest();
}

@Test
@Ignore("requires generic method support")
public void genericMethodAndVoidType() {
Expand Down