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

Substitute type arguments when checking type parameter nullability at call site #1070

Merged
merged 12 commits into from
Nov 13, 2024
2 changes: 1 addition & 1 deletion nullaway/src/main/java/com/uber/nullaway/NullAway.java
Original file line number Diff line number Diff line change
Expand Up @@ -1852,7 +1852,7 @@ private Description handleInvocation(
}
if (config.isJSpecifyMode()) {
GenericsChecks.compareGenericTypeParameterNullabilityForCall(
formalParams, actualParams, varArgsMethod, this, state);
methodSymbol, tree, actualParams, varArgsMethod, this, state);
if (!methodSymbol.getTypeParameters().isEmpty()) {
GenericsChecks.checkGenericMethodCallTypeArguments(tree, state, this, config, handler);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -595,29 +595,58 @@ public static void checkTypeParameterNullnessForConditionalExpression(
* Checks that for each parameter p at a call, the type parameter nullability for p's type matches
* that of the corresponding formal parameter. If a mismatch is found, report an error.
*
* @param formalParams the formal parameters
* @param actualParams the actual parameters
* @param methodSymbol the symbol for the method being called
* @param tree the tree representing the method call
* @param actualParams the actual parameters at the call
* @param isVarArgs true if the call is to a varargs method
* @param analysis the analysis object
* @param state the visitor state
*/
public static void compareGenericTypeParameterNullabilityForCall(
List<Symbol.VarSymbol> formalParams,
Symbol.MethodSymbol methodSymbol,
Tree tree,
List<? extends ExpressionTree> actualParams,
boolean isVarArgs,
NullAway analysis,
VisitorState state) {
if (!analysis.getConfig().isJSpecifyMode()) {
return;
}
int n = formalParams.size();
Type invokedMethodType = methodSymbol.type;
if (!methodSymbol.isStatic() && tree instanceof MethodInvocationTree) {
ExpressionTree methodSelect = ((MethodInvocationTree) tree).getMethodSelect();
Type enclosingType;
if (methodSelect instanceof MemberSelectTree) {
enclosingType = getTreeType(((MemberSelectTree) methodSelect).getExpression(), state);
} else {
// implicit this parameter
enclosingType = methodSymbol.owner.type;
}
if (enclosingType != null) {
invokedMethodType = state.getTypes().memberType(enclosingType, methodSymbol);
}
}
// Handle generic methods
if (tree instanceof MethodInvocationTree && methodSymbol.type instanceof Type.ForAll) {
MethodInvocationTree methodInvocationTree = (MethodInvocationTree) tree;

List<? extends Tree> typeArgumentTrees = methodInvocationTree.getTypeArguments();
com.sun.tools.javac.util.List<Type> explicitTypeArgs = convertTreesToTypes(typeArgumentTrees);

Type.ForAll forAllType = (Type.ForAll) methodSymbol.type;
Type.MethodType underlyingMethodType = (Type.MethodType) forAllType.qtype;
invokedMethodType =
state.getTypes().subst(underlyingMethodType, 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.

This exact logic (or very similar) now appears in multiple places. Can we extract it into a method that returns a method type with the generic type arguments substituted in?

List<Type> formalParamTypes = invokedMethodType.getParameterTypes();
int n = formalParamTypes.size();
if (isVarArgs) {
// If the last argument is var args, don't check it now, it will be checked against
// all remaining actual arguments in the next loop.
n = n - 1;
}
for (int i = 0; i < n; i++) {
Type formalParameter = formalParams.get(i).type;
Type formalParameter = formalParamTypes.get(i);
if (formalParameter.isRaw()) {
// bail out of any checking involving raw types for now
return;
Expand All @@ -630,11 +659,11 @@ public static void compareGenericTypeParameterNullabilityForCall(
}
}
}
if (isVarArgs && !formalParams.isEmpty()) {
if (isVarArgs && !formalParamTypes.isEmpty()) {
Type.ArrayType varargsArrayType =
(Type.ArrayType) formalParams.get(formalParams.size() - 1).type;
(Type.ArrayType) formalParamTypes.get(formalParamTypes.size() - 1);
Type varargsElementType = varargsArrayType.elemtype;
for (int i = formalParams.size() - 1; i < actualParams.size(); i++) {
for (int i = formalParamTypes.size() - 1; i < actualParams.size(); i++) {
Type actualParameterType = getTreeType(actualParams.get(i), state);
// If the actual parameter type is assignable to the varargs array type, then the call site
// is passing the varargs directly in an array, and we should skip our check.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -105,8 +105,36 @@ public void genericInstanceMethods() {
}

@Test
@Ignore("requires generic method support")
public void genericMethodAndVoidType() {
makeHelper()
.addSourceLines(
"Test.java",
"package com.uber;",
"import org.jspecify.annotations.Nullable;",
"class Test {",
" static class Foo {",
" <C extends @Nullable Object> void foo(C c, Visitor<C> visitor) {",
" visitor.visit(this, c);",
" }",
" }",
" static abstract class Visitor<C extends @Nullable Object> {",
" abstract void visit(Foo foo, C c);",
" }",
" static class MyVisitor extends Visitor<@Nullable Void> {",
" @Override",
" void visit(Foo foo, @Nullable Void c) {}",
" }",
" static void test(Foo f) {",
" // this is safe",
" f.<@Nullable Void>foo(null, new MyVisitor());",
" }",
"}")
.doTest();
}

@Test
@Ignore("requires generic method support")
public void genericMethodAndVoidTypeWithInference() {
makeHelper()
.addSourceLines(
"Test.java",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -938,6 +938,28 @@ public void parameterPassing() {
.doTest();
}

@Test
public void parameterPassingInstanceMethods() {
makeHelper()
.addSourceLines(
"Test.java",
"package com.uber;",
"import org.jspecify.annotations.Nullable;",
"class Test {",
" static class A<T extends @Nullable Object> {",
" void foo(A<T> a) {}",
" void bar(A<T> a) { foo(a); this.foo(a); }",
" }",
" static void test(A<@Nullable String> p, A<String> q) {",
" // BUG: Diagnostic contains: Cannot pass parameter of type",
" p.foo(q);",
" // this one is fine",
" p.foo(p);",
" }",
"}")
.doTest();
}

@Test
public void varargsParameter() {
makeHelper()
Expand Down