Skip to content

Commit

Permalink
JSpecify: changes for issue #861 (#863)
Browse files Browse the repository at this point in the history 
Removing unnecessary checks performed for ClassType in
GenericChecks.java in reference to issue #861. Added some null checks
since some of the checks for ClassType were also indirectly checking for
nullability and hence removing them entirely cause the test rawTypes to
crash.

Fixes #861

---------

Co-authored-by: Manu Sridharan <[email protected]>
  • Loading branch information
@akulk022 @msridhar
akulk022 and msridhar authored Dec 5, 2023
1 parent eb2e19c commit 2f1cf7c
Show file tree
Hide file tree
Showing 3 changed files with 76 additions and 18 deletions.
15 changes: 11 additions & 4 deletions nullaway/src/main/java/com/uber/nullaway/generics/CompareNullabilityVisitor.java
View file
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) {
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
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
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

0 comments on commit 2f1cf7c

Please sign in to comment.