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.

Sign up for GitHub

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: changes for issue #861 #863

Merged
merged 7 commits into from
Dec 5, 2023
Merged

JSpecify: changes for issue #861 #863

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
8582002
removing class type checks
akulk022 Nov 18, 2023
435b65e
adding null checks and a few class type checks
akulk022 Nov 18, 2023
51e2461
remove more checks, add checks for NullType
msridhar Nov 18, 2023
ade61c7
Merge branch 'master' into generic_types_remove_class_checks_abhijitk
msridhar Dec 1, 2023
290f960
Adding test cases to increase coverage
akulk022 Dec 5, 2023
1aa78b5
Merge branch 'master' into generic_types_remove_class_checks_abhijitk
msridhar Dec 5, 2023
f3b8129
formatting
msridhar Dec 5, 2023
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
15 changes: 11 additions & 4 deletions nullaway/src/main/java/com/uber/nullaway/generics/CompareNullabilityVisitor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import com.sun.tools.javac.code.Type;
import com.sun.tools.javac.code.Types;
import java.util.List;
import javax.lang.model.type.NullType;

/**
* Visitor that checks equality of nullability annotations for all nested generic type arguments
Expand All @@ -21,22 +22,25 @@ public class CompareNullabilityVisitor extends Types.DefaultTypeVisitor<Boolean,

@Override
public Boolean visitClassType(Type.ClassType lhsType, Type rhsType) {
if (rhsType instanceof NullType) {
return true;
}
Types types = state.getTypes();
// The base type of rhsType may be a subtype of lhsType's base type. In such cases, we must
// compare lhsType against the supertype of rhsType with a matching base type.
rhsType = types.asSuper(rhsType, lhsType.tsym);
Type rhsTypeAsSuper = types.asSuper(rhsType, lhsType.tsym);
// This is impossible, considering the fact that standard Java subtyping succeeds before
// running NullAway
if (rhsType == null) {
if (rhsTypeAsSuper == null) {
throw new RuntimeException("Did not find supertype of " + rhsType + " matching " + lhsType);
}
List<Type> lhsTypeArguments = lhsType.getTypeArguments();
List<Type> rhsTypeArguments = rhsType.getTypeArguments();
List<Type> rhsTypeArguments = rhsTypeAsSuper.getTypeArguments();
// This is impossible, considering the fact that standard Java subtyping succeeds before
// running NullAway
if (lhsTypeArguments.size() != rhsTypeArguments.size()) {
throw new RuntimeException(
"Number of types arguments in " + rhsType + " does not match " + lhsType);
"Number of types arguments in " + rhsTypeAsSuper + " does not match " + lhsType);
}
for (int i = 0; i < lhsTypeArguments.size(); i++) {
Type lhsTypeArgument = lhsTypeArguments.get(i);
Expand Down Expand Up @@ -77,6 +81,9 @@ public Boolean visitClassType(Type.ClassType lhsType, Type rhsType) {

@Override
public Boolean visitArrayType(Type.ArrayType lhsType, Type rhsType) {
if (rhsType instanceof NullType) {
msridhar marked this conversation as resolved.
Show resolved Hide resolved
return true;
}
Type.ArrayType arrRhsType = (Type.ArrayType) rhsType;
return lhsType.getComponentType().accept(this, arrRhsType.getComponentType());
}
Expand Down
24 changes: 10 additions & 14 deletions nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,7 +301,7 @@ public static void checkTypeParameterNullnessForAssignability(
Type lhsType = getTreeType(lhsTree, state);
Type rhsType = getTreeType(rhsTree, state);

if (lhsType instanceof Type.ClassType && rhsType instanceof Type.ClassType) {
if (lhsType != null && rhsType != null) {
boolean isAssignmentValid = compareNullabilityAnnotations(lhsType, rhsType, state);
if (!isAssignmentValid) {
reportInvalidAssignmentInstantiationError(tree, lhsType, rhsType, state, analysis);
Expand Down Expand Up @@ -333,8 +333,7 @@ public static void checkTypeParameterNullnessForFunctionReturnType(
return;
}
Type returnExpressionType = getTreeType(retExpr, state);
if (formalReturnType instanceof Type.ClassType
&& returnExpressionType instanceof Type.ClassType) {
if (formalReturnType != null && returnExpressionType != null) {
boolean isReturnTypeValid =
compareNullabilityAnnotations(formalReturnType, returnExpressionType, state);
if (!isReturnTypeValid) {
Expand Down Expand Up @@ -407,14 +406,14 @@ public static void checkTypeParameterNullnessForConditionalExpression(
// The condExpr type should be the least-upper bound of the true and false part types. To check
// the nullability annotations, we check that the true and false parts are assignable to the
// type of the whole expression
if (condExprType instanceof Type.ClassType) {
if (truePartType instanceof Type.ClassType) {
if (condExprType != null) {
if (truePartType != null) {
if (!compareNullabilityAnnotations(condExprType, truePartType, state)) {
reportMismatchedTypeForTernaryOperator(
truePartTree, condExprType, truePartType, state, analysis);
}
}
if (falsePartType instanceof Type.ClassType) {
if (falsePartType != null) {
if (!compareNullabilityAnnotations(condExprType, falsePartType, state)) {
reportMismatchedTypeForTernaryOperator(
falsePartTree, condExprType, falsePartType, state, analysis);
Expand Down Expand Up @@ -452,8 +451,7 @@ public static void compareGenericTypeParameterNullabilityForCall(
Type formalParameter = formalParams.get(i).type;
if (!formalParameter.getTypeArguments().isEmpty()) {
Type actualParameter = getTreeType(actualParams.get(i), state);
if (formalParameter instanceof Type.ClassType
&& actualParameter instanceof Type.ClassType) {
if (actualParameter != null) {
if (!compareNullabilityAnnotations(formalParameter, actualParameter, state)) {
reportInvalidParametersNullabilityError(
formalParameter, actualParameter, actualParams.get(i), state, analysis);
Expand All @@ -468,8 +466,7 @@ public static void compareGenericTypeParameterNullabilityForCall(
if (!varargsElementType.getTypeArguments().isEmpty()) {
for (int i = formalParams.size() - 1; i < actualParams.size(); i++) {
Type actualParameter = getTreeType(actualParams.get(i), state);
if (varargsElementType instanceof Type.ClassType
&& actualParameter instanceof Type.ClassType) {
if (actualParameter != null) {
if (!compareNullabilityAnnotations(varargsElementType, actualParameter, state)) {
reportInvalidParametersNullabilityError(
varargsElementType, actualParameter, actualParams.get(i), state, analysis);
Expand Down Expand Up @@ -777,8 +774,7 @@ private static void checkTypeParameterNullnessForOverridingMethodParameterType(
for (int i = 0; i < methodParameters.size(); i++) {
Type overridingMethodParameterType = ASTHelpers.getType(methodParameters.get(i));
Type overriddenMethodParameterType = overriddenMethodParameterTypes.get(i);
if (overriddenMethodParameterType instanceof Type.ClassType
&& overridingMethodParameterType instanceof Type.ClassType) {
if (overriddenMethodParameterType != null && overridingMethodParameterType != null) {
if (!compareNullabilityAnnotations(
overriddenMethodParameterType, overridingMethodParameterType, state)) {
reportInvalidOverridingMethodParamTypeError(
Expand Down Expand Up @@ -806,10 +802,10 @@ private static void checkTypeParameterNullnessForOverridingMethodReturnType(
MethodTree tree, Type overriddenMethodType, NullAway analysis, VisitorState state) {
Type overriddenMethodReturnType = overriddenMethodType.getReturnType();
Type overridingMethodReturnType = ASTHelpers.getType(tree.getReturnType());
if (!(overriddenMethodReturnType instanceof Type.ClassType)) {
if (overriddenMethodReturnType == null) {
return;
}
Preconditions.checkArgument(overridingMethodReturnType instanceof Type.ClassType);
Preconditions.checkArgument(overridingMethodReturnType != null);
if (!compareNullabilityAnnotations(
overriddenMethodReturnType, overridingMethodReturnType, state)) {
reportInvalidOverridingMethodReturnTypeError(
Expand Down
55 changes: 55 additions & 0 deletions nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyGenericsTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1020,6 +1020,23 @@ public void nestedGenericTypeAssignment() {
.doTest();
}

@Test
public void nestedGenericTypeAssignment2() {
makeHelper()
.addSourceLines(
"Test.java",
"package com.uber;",
"import org.jspecify.annotations.Nullable;",
"class Test {",
" static class A<T extends @Nullable Object> { }",
" static void testPositive() {",
" // BUG: Diagnostic contains: Cannot assign from type",
" A<A<String>[]> var2 = new A<A<@Nullable String>[]>();",
" }",
"}")
.doTest();
}

@Test
public void genericPrimitiveArrayTypeAssignment() {
makeHelper()
Expand Down Expand Up @@ -1560,6 +1577,44 @@ public void testForDiamondOperatorReturnedAsAMethodCaller() {
.doTest();
}

@Test
public void testForNullRhsTypeWhenReturnedForGenericType() {
makeHelper()
.addSourceLines(
"Test.java",
"package com.uber;",
"import org.jspecify.annotations.Nullable;",
"class Test {",
" static class A<T extends @Nullable Object> { }",
" static A<String> testPositive() {",
" // BUG: Diagnostic contains: returning @Nullable expression from method with @NonNull return type",
" return null;",
" }",
" static @Nullable A<String> testNegative() {",
" return null;",
" }",
"}")
.doTest();
}

@Test
public void testForNullTypeRhsTypeForArrayType() {
makeHelper()
.addSourceLines(
"Test.java",
"package com.uber;",
"import org.jspecify.annotations.Nullable;",
"import java.util.List;",
"import java.util.ArrayList;",
"class Test {",
" static void testNegative() {",
" List<String> a = new ArrayList<String>();",
" Object[] o = a != null ? a.toArray() : null;",
" }",
"}")
.doTest();
}

private CompilationTestHelper makeHelper() {
return makeTestHelperWithArgs(
Arrays.asList(
Expand Down