Skip to content

Commit

Permalink
Revert "Add JSpecify checking for return statements (uber#734)"
Browse files Browse the repository at this point in the history 
This reverts commit 1548c69.
  • Loading branch information
@msridhar
msridhar committed Jul 18, 2023
1 parent 8b2fcee commit ca24615
Show file tree
Hide file tree
Showing 4 changed files with 23 additions and 159 deletions.
1 change: 0 additions & 1 deletion nullaway/src/main/java/com/uber/nullaway/ErrorMessage.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,6 @@ public enum MessageTypes {
WRONG_OVERRIDE_PRECONDITION,
TYPE_PARAMETER_CANNOT_BE_NULLABLE,
ASSIGN_GENERIC_NULLABLE,
RETURN_NULLABLE_GENERIC,
}

public String getMessage() {
Expand Down
108 changes: 22 additions & 86 deletions nullaway/src/main/java/com/uber/nullaway/GenericsChecks.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,11 @@
import com.sun.source.tree.AnnotatedTypeTree;
import com.sun.source.tree.AnnotationTree;
import com.sun.source.tree.AssignmentTree;
import com.sun.source.tree.ExpressionTree;
import com.sun.source.tree.NewClassTree;
import com.sun.source.tree.ParameterizedTypeTree;
import com.sun.source.tree.Tree;
import com.sun.source.tree.VariableTree;
import com.sun.tools.javac.code.Attribute;
import com.sun.tools.javac.code.Symbol;
import com.sun.tools.javac.code.Type;
import com.sun.tools.javac.code.TypeMetadata;
import com.sun.tools.javac.code.Types;
Expand All @@ -25,7 +23,6 @@
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import javax.annotation.Nullable;

/** Methods for performing checks related to generic types and nullability. */
public final class GenericsChecks {
Expand Down Expand Up @@ -133,51 +130,6 @@ private static void reportInvalidAssignmentInstantiationError(
errorMessage, analysis.buildDescription(tree), state, null));
}

private static void reportInvalidReturnTypeError(
Tree tree, Type methodType, Type returnType, VisitorState state, NullAway analysis) {
ErrorBuilder errorBuilder = analysis.getErrorBuilder();
ErrorMessage errorMessage =
new ErrorMessage(
ErrorMessage.MessageTypes.RETURN_NULLABLE_GENERIC,
String.format(
"Cannot return expression of type "
+ returnType
+ " from method with return type "
+ methodType
+ " due to mismatched nullability of type parameters"));
state.reportMatch(
errorBuilder.createErrorDescription(
errorMessage, analysis.buildDescription(tree), state, null));
}

/**
* This method returns the type of the given tree, including any type use annotations.
*
* <p>This method is required because in some cases, the type returned by {@link
* com.google.errorprone.util.ASTHelpers#getType(Tree)} fails to preserve type use annotations,
* particularly when dealing with {@link com.sun.source.tree.NewClassTree} (e.g., {@code new
* Foo<@Nullable A>}).
*
* @param tree A tree for which we need the type with preserved annotations.
* @return Type of the tree with preserved annotations.
*/
@Nullable
private Type getTreeType(Tree tree) {
if (tree instanceof NewClassTree
&& ((NewClassTree) tree).getIdentifier() instanceof ParameterizedTypeTree) {
ParameterizedTypeTree paramTypedTree =
(ParameterizedTypeTree) ((NewClassTree) tree).getIdentifier();
if (paramTypedTree.getTypeArguments().isEmpty()) {
// diamond operator, which we do not yet support; for now, return null
// TODO: support diamond operators
return null;
}
return typeWithPreservedAnnotations(paramTypedTree);
} else {
return ASTHelpers.getType(tree);
}
}

/**
* For a tree representing an assignment, ensures that from the perspective of type parameter
* nullability, the type of the right-hand side is assignable to (a subtype of) the type of the
Expand Down Expand Up @@ -207,39 +159,23 @@ public void checkTypeParameterNullnessForAssignability(Tree tree) {
if (rhsTree == null || rhsTree.getKind().equals(Tree.Kind.NULL_LITERAL)) {
return;
}
Type lhsType = getTreeType(lhsTree);
Type rhsType = getTreeType(rhsTree);

if (lhsType instanceof Type.ClassType && rhsType instanceof Type.ClassType) {
boolean isAssignmentValid =
compareNullabilityAnnotations((Type.ClassType) lhsType, (Type.ClassType) rhsType);
if (!isAssignmentValid) {
reportInvalidAssignmentInstantiationError(tree, lhsType, rhsType, state, analysis);
Type lhsType = ASTHelpers.getType(lhsTree);
Type rhsType = ASTHelpers.getType(rhsTree);
// For NewClassTrees with annotated type parameters, javac does not preserve the annotations in
// its computed type for the expression. As a workaround, we construct a replacement Type
// object with the appropriate annotations.
if (rhsTree instanceof NewClassTree
&& ((NewClassTree) rhsTree).getIdentifier() instanceof ParameterizedTypeTree) {
ParameterizedTypeTree paramTypedTree =
(ParameterizedTypeTree) ((NewClassTree) rhsTree).getIdentifier();
if (paramTypedTree.getTypeArguments().isEmpty()) {
// no explicit type parameters
return;
}
rhsType = typeWithPreservedAnnotations(paramTypedTree);
}
}

public void checkTypeParameterNullnessForFunctionReturnType(
ExpressionTree retExpr, Symbol.MethodSymbol methodSymbol) {
if (!config.isJSpecifyMode()) {
return;
}

Type formalReturnType = methodSymbol.getReturnType();
// check nullability of parameters only for generics
if (formalReturnType.getTypeArguments().isEmpty()) {
return;
}
Type returnExpressionType = getTreeType(retExpr);
if (formalReturnType instanceof Type.ClassType
&& returnExpressionType instanceof Type.ClassType) {
boolean isReturnTypeValid =
compareNullabilityAnnotations(
(Type.ClassType) formalReturnType, (Type.ClassType) returnExpressionType);
if (!isReturnTypeValid) {
reportInvalidReturnTypeError(
retExpr, formalReturnType, returnExpressionType, state, analysis);
}
if (lhsType instanceof Type.ClassType && rhsType instanceof Type.ClassType) {
compareNullabilityAnnotations((Type.ClassType) lhsType, (Type.ClassType) rhsType, tree);
}
}

Expand All @@ -253,8 +189,10 @@ public void checkTypeParameterNullnessForFunctionReturnType(
*
* @param lhsType type for the lhs of the assignment
* @param rhsType type for the rhs of the assignment
* @param tree tree representing the assignment
*/
private boolean compareNullabilityAnnotations(Type.ClassType lhsType, Type.ClassType rhsType) {
private void compareNullabilityAnnotations(
Type.ClassType lhsType, Type.ClassType rhsType, Tree tree) {
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.
Expand Down Expand Up @@ -294,17 +232,15 @@ private boolean compareNullabilityAnnotations(Type.ClassType lhsType, Type.Class
}
}
if (isLHSNullableAnnotated != isRHSNullableAnnotated) {
return false;
reportInvalidAssignmentInstantiationError(tree, lhsType, rhsType, state, analysis);
return;
}
// nested generics
if (lhsTypeArgument.getTypeArguments().length() > 0) {
if (!compareNullabilityAnnotations(
(Type.ClassType) lhsTypeArgument, (Type.ClassType) rhsTypeArgument)) {
return false;
}
compareNullabilityAnnotations(
(Type.ClassType) lhsTypeArgument, (Type.ClassType) rhsTypeArgument, tree);
}
}
return true;
}

/**
Expand Down
2 changes: 0 additions & 2 deletions nullaway/src/main/java/com/uber/nullaway/NullAway.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -834,8 +834,6 @@ private Description checkReturnExpression(
state,
methodSymbol);
}
new GenericsChecks(state, config, this)
.checkTypeParameterNullnessForFunctionReturnType(retExpr, methodSymbol);
return Description.NO_MATCH;
}

Expand Down
71 changes: 1 addition & 70 deletions nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyGenericsTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -155,8 +155,7 @@ public void returnTypeParamInstantiation() {
" static class NullableTypeParam<E extends @Nullable Object> {}",
" // BUG: Diagnostic contains: Generic type parameter",
" static NonNullTypeParam<@Nullable String> testBadNonNull() {",
" // BUG: Diagnostic contains: Generic type parameter",
" return new NonNullTypeParam<@Nullable String>();",
" return new NonNullTypeParam<String>();",
" }",
" static NullableTypeParam<@Nullable String> testOKNull() {",
" return new NullableTypeParam<@Nullable String>();",
Expand Down Expand Up @@ -489,74 +488,6 @@ public void testForDiamondInAnAssignment() {
.doTest();
}

@Test
public void genericFunctionReturnTypeNewClassTree() {
makeHelper()
.addSourceLines(
"Test.java",
"package com.uber;",
"import org.jspecify.annotations.Nullable;",
"class Test {",
" static class A<T extends @Nullable Object> { }",
" static A<String> testPositive1() {",
" // BUG: Diagnostic contains: mismatched nullability of type parameters",
" return new A<@Nullable String>();",
" }",
" static A<@Nullable String> testPositive2() {",
" // BUG: Diagnostic contains: mismatched nullability of type parameters",
" return new A<String>();",
" }",
" static A<@Nullable String> testNegative() {",
" return new A<@Nullable String>();",
" }",
"}")
.doTest();
}

@Test
public void genericFunctionReturnTypeNormalTree() {
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(A<@Nullable String> a) {",
" // BUG: Diagnostic contains: mismatched nullability of type parameters",
" return a;",
" }",
" static A<@Nullable String> testNegative(A<@Nullable String> a) {",
" return a;",
" }",
"}")
.doTest();
}

@Test
public void genericFunctionReturnTypeMultipleReturnStatementsIfElseBlock() {
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(A<@Nullable String> a, int num) {",
" if (num % 2 == 0) {",
" // BUG: Diagnostic contains: mismatched nullability of type parameters",
" return a;",
" } else {",
" return new A<String>();",
" }",
" }",
" static A<String> testNegative(A<String> a, int num) {",
" return a;",
" }",
"}")
.doTest();
}

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

0 comments on commit ca24615

Please sign in to comment.