Skip to content

Commit

Permalink
Simplify AbstractMustBeClosedChecker with Java 21 features.
Browse files Browse the repository at this point in the history
PiperOrigin-RevId: 695900731
  • Loading branch information
chaoren authored and Error Prone Team committed Nov 14, 2024
1 parent 2124ebf commit 3141d98
Show file tree
Hide file tree
Showing 149 changed files with 892 additions and 505 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@
import com.google.errorprone.matchers.Matcher;
import com.google.errorprone.matchers.UnusedReturnValueMatcher;
import com.google.errorprone.util.ASTHelpers;
import com.sun.source.tree.AssignmentTree;
import com.sun.source.tree.BlockTree;
import com.sun.source.tree.ConditionalExpressionTree;
import com.sun.source.tree.ExpressionStatementTree;
Expand All @@ -56,6 +57,7 @@
import com.sun.source.tree.MethodInvocationTree;
import com.sun.source.tree.MethodTree;
import com.sun.source.tree.NewClassTree;
import com.sun.source.tree.ReturnTree;
import com.sun.source.tree.StatementTree;
import com.sun.source.tree.Tree;
import com.sun.source.tree.TryTree;
Expand Down Expand Up @@ -111,17 +113,14 @@ String uniquifyName(String basename) {
* @param tree must be either MethodInvocationTree or NewClassTree
*/
String suggestName(ExpressionTree tree) {
String symbolName;
switch (tree.getKind()) {
case NEW_CLASS:
symbolName = getSymbol(((NewClassTree) tree).getIdentifier()).getSimpleName().toString();
break;
case METHOD_INVOCATION:
symbolName = getReturnType(tree).asElement().getSimpleName().toString();
break;
default:
throw new AssertionError(tree.getKind());
}
String symbolName =
switch (tree) {
case NewClassTree newClassTree ->
getSymbol(newClassTree.getIdentifier()).getSimpleName().toString();
case MethodInvocationTree methodInvocationTree ->
getReturnType(methodInvocationTree).asElement().getSimpleName().toString();
default -> throw new AssertionError(tree.getKind());
};
return uniquifyName(CaseFormat.UPPER_CAMEL.to(CaseFormat.LOWER_CAMEL, symbolName));
}
}
Expand Down Expand Up @@ -255,80 +254,72 @@ private Optional<Change> checkClosed(
while (true) {
TreePath prev = path;
path = path.getParentPath();
switch (path.getLeaf().getKind()) {
case RETURN:
if (callerMethodTree != null) {
// The invocation occurs within a return statement of a method, instead of a lambda
// expression or anonymous class.
if (HAS_MUST_BE_CLOSED_ANNOTATION.matches(callerMethodTree, state)) {
// Ignore invocations of annotated methods and constructors that occur in the return
// statement of an annotated caller method, since invocations of the caller are
// enforced.
return Optional.empty();
}
// The caller method is not annotated, so the closing of the returned resource is not
// enforced. Suggest fixing this by annotating the caller method.
return Change.of(
SuggestedFix.builder()
.prefixWith(callerMethodTree, "@MustBeClosed\n")
.addImport(MustBeClosed.class.getCanonicalName())
.build());
switch (path.getLeaf()) {
case ReturnTree unused when callerMethodTree != null -> {
// The invocation occurs within a return statement of a method, instead of a lambda
// expression or anonymous class.
if (HAS_MUST_BE_CLOSED_ANNOTATION.matches(callerMethodTree, state)) {
// Ignore invocations of annotated methods and constructors that occur in the return
// statement of an annotated caller method, since invocations of the caller are
// enforced.
return Optional.empty();
}
// The caller method is not annotated, so the closing of the returned resource is not
// enforced. Suggest fixing this by annotating the caller method.
return Change.of(
SuggestedFix.builder()
.prefixWith(callerMethodTree, "@MustBeClosed\n")
.addImport(MustBeClosed.class.getCanonicalName())
.build());
}
case ReturnTree unused -> {
// If enclosingMethod returned null, we must be returning from a statement lambda.
return handleTailPositionInLambda(state);
case LAMBDA_EXPRESSION:
}
case LambdaExpressionTree unused -> {
// The method invocation is the body of an expression lambda.
return handleTailPositionInLambda(state);
case CONDITIONAL_EXPRESSION:
ConditionalExpressionTree conditionalExpressionTree =
(ConditionalExpressionTree) path.getLeaf();
if (conditionalExpressionTree.getTrueExpression().equals(prev.getLeaf())
|| conditionalExpressionTree.getFalseExpression().equals(prev.getLeaf())) {
}
case ConditionalExpressionTree conditionalExpressionTree
when conditionalExpressionTree.getTrueExpression().equals(prev.getLeaf())
|| conditionalExpressionTree.getFalseExpression().equals(prev.getLeaf()) -> {
continue OUTER;
}
case MemberSelectTree memberSelectTree
when memberSelectTree.getExpression().equals(prev.getLeaf()) -> {
Type type = getType(memberSelectTree);
Symbol sym = getSymbol(memberSelectTree);
Type streamType = state.getTypeFromString(Stream.class.getName());
if (isSubtype(sym.enclClass().asType(), streamType, state)
&& isSameType(type.getReturnType(), streamType, state)) {
// skip enclosing method invocation
path = path.getParentPath();
continue OUTER;
}
break;
case MEMBER_SELECT:
MemberSelectTree memberSelectTree = (MemberSelectTree) path.getLeaf();
if (memberSelectTree.getExpression().equals(prev.getLeaf())) {
Type type = getType(memberSelectTree);
Symbol sym = getSymbol(memberSelectTree);
Type streamType = state.getTypeFromString(Stream.class.getName());
if (isSubtype(sym.enclClass().asType(), streamType, state)
&& isSameType(type.getReturnType(), streamType, state)) {
// skip enclosing method invocation
path = path.getParentPath();
continue OUTER;
}
}
break;
case NEW_CLASS:
NewClassTree newClassTree = (NewClassTree) path.getLeaf();
if (isClosingDecorator(newClassTree, prev.getLeaf(), state)) {
if (HAS_MUST_BE_CLOSED_ANNOTATION.matches(newClassTree, state)) {
// if the decorator is also annotated then it would already be enforced
return Optional.empty();
}
// otherwise, enforce that the decorator must be closed
continue OUTER;
}
case NewClassTree newClassTree
when isClosingDecorator(newClassTree, prev.getLeaf(), state) -> {
if (HAS_MUST_BE_CLOSED_ANNOTATION.matches(newClassTree, state)) {
// if the decorator is also annotated then it would already be enforced
return Optional.empty();
}
break;
case VARIABLE:
Symbol sym = getSymbol(path.getLeaf());
if (sym instanceof VarSymbol) {
VarSymbol var = (VarSymbol) sym;
if (var.getKind() == ElementKind.RESOURCE_VARIABLE
|| isClosedInFinallyClause(var, path, state)
|| ASTHelpers.variableIsStaticFinal(var)) {
return Optional.empty();
}
// otherwise, enforce that the decorator must be closed
continue OUTER;
}
case VariableTree variableTree -> {
VarSymbol var = getSymbol(variableTree);
if (var.getKind() == ElementKind.RESOURCE_VARIABLE
|| isClosedInFinallyClause(var, path, state)
|| ASTHelpers.variableIsStaticFinal(var)) {
return Optional.empty();
}
break;
case ASSIGNMENT:
}
case AssignmentTree unused -> {
// We shouldn't suggest a try/finally fix when we know the variable is going to be saved
// for later.
return findingWithNoFix();
default:
break;
}
default -> {}
}
// The constructor or method invocation does not occur within the resource variable
// initializer of a try-with-resources statement.
Expand Down Expand Up @@ -368,8 +359,7 @@ private static Optional<Change> findingWithNoFix() {
private static @Nullable MethodTree enclosingMethod(VisitorState state) {
for (Tree node : state.getPath().getParentPath()) {
switch (node.getKind()) {
case LAMBDA_EXPRESSION:
case NEW_CLASS:
case LAMBDA_EXPRESSION, NEW_CLASS:
return null;
case METHOD:
return (MethodTree) node;
Expand All @@ -384,21 +374,15 @@ private static boolean isClosedInFinallyClause(VarSymbol var, TreePath path, Vis
if (!isConsideredFinal(var)) {
return false;
}
Tree parent = path.getParentPath().getLeaf();
if (parent.getKind() != Tree.Kind.BLOCK) {
if (!(path.getParentPath().getLeaf() instanceof BlockTree block)) {
return false;
}
BlockTree block = (BlockTree) parent;
int idx = block.getStatements().indexOf(path.getLeaf());
if (idx == -1 || idx == block.getStatements().size() - 1) {
return false;
}
StatementTree next = block.getStatements().get(idx + 1);
if (!(next instanceof TryTree)) {
return false;
}
TryTree tryTree = (TryTree) next;
if (tryTree.getFinallyBlock() == null) {
if (!(next instanceof TryTree tryTree) || tryTree.getFinallyBlock() == null) {
return false;
}
boolean[] closed = {false};
Expand All @@ -421,23 +405,20 @@ public Void visitMethodInvocation(MethodInvocationTree tree, Void unused) {

private static Optional<Change> chooseFixType(
ExpressionTree tree, VisitorState state, NameSuggester suggester) {
TreePath path = state.getPath();
Tree parent = path.getParentPath().getLeaf();
if (parent instanceof VariableTree) {
return wrapTryFinallyAroundVariableScope((VariableTree) parent, state);
}
StatementTree stmt = state.findEnclosing(StatementTree.class);
if (stmt == null) {
return Optional.empty();
}
if (!(stmt instanceof VariableTree)) {
return introduceSingleStatementTry(tree, stmt, state, suggester);
if (state.getPath().getParentPath().getLeaf() instanceof VariableTree parent) {
return wrapTryFinallyAroundVariableScope(parent, state);
}
VarSymbol varSym = getSymbol((VariableTree) stmt);
if (varSym.getKind() == ElementKind.RESOURCE_VARIABLE) {
return extractToResourceInCurrentTry(tree, stmt, state, suggester);
}
return splitVariableDeclarationAroundTry(tree, (VariableTree) stmt, state, suggester);
return switch (state.findEnclosing(StatementTree.class)) {
case null -> Optional.empty();
case VariableTree stmt -> {
VarSymbol varSym = getSymbol(stmt);
if (varSym.getKind() == ElementKind.RESOURCE_VARIABLE) {
yield extractToResourceInCurrentTry(tree, stmt, state, suggester);
}
yield splitVariableDeclarationAroundTry(tree, stmt, state, suggester);
}
case StatementTree stmt -> introduceSingleStatementTry(tree, stmt, state, suggester);
};
}

private static Optional<Change> introduceSingleStatementTry(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,8 @@ public void bothMethodCalls() {
System.out.println("arrays are not equal!");
}
}
}""")
}\
""")
.doTest();
}

Expand Down Expand Up @@ -162,7 +163,8 @@ public void objectArray() {
System.out.println("arrays are not equal!");
}
}
}""")
}\
""")
.doTest();
}

Expand Down Expand Up @@ -230,7 +232,8 @@ public void secondArray() {
System.out.println("Objects are not equal!");
}
}
}""")
}\
""")
.doTest();
}

Expand Down Expand Up @@ -282,7 +285,8 @@ public void secondArray() {
System.out.println("arrays are not equal!");
}
}
}""")
}\
""")
.doTest();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,8 @@ public void varargsHashCodeOnMoreThanOneArg() {
// Arrays.deepHashCode(multidimensionalStringArray))
hashCode = Objects.hashCode(obj1, obj2, multidimensionalStringArray);
}
}""")
}\
""")
.doTest();
}

Expand Down Expand Up @@ -147,7 +148,8 @@ public void varagsHashCodeOnObjectOrStringArray() {
hashCode = Objects.hashCode(objArray);
hashCode = Objects.hashCode((Object[]) stringArray);
}
}""")
}\
""")
.doTest();
}

Expand Down Expand Up @@ -193,7 +195,8 @@ public void varagsHashCodeOnObjectOrStringArray() {
hashCode = Objects.hash(objArray);
hashCode = Objects.hash((Object[]) stringArray);
}
}""")
}\
""")
.doTest();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,8 @@ public void arrayOfArrays() {
// BUG: Diagnostic contains: Arrays.deepToString(a)
System.out.println(a);
}
}""")
}\
""")
.doTest();
}

Expand All @@ -129,7 +130,8 @@ public void objectEquals() {
System.out.println("string is not empty!");
}
}
}""")
}\
""")
.doTest();
}

Expand Down Expand Up @@ -305,7 +307,8 @@ public void stringVariableAddsArrayAndAssigns() {
// BUG: Diagnostic contains: += Arrays.toString(a)
b += a;
}
}""")
}\
""")
.doTest();
}

Expand All @@ -332,7 +335,8 @@ public void concatenateCompoundAssign_int() {
String b = " a string ";
b += a;
}
}""")
}\
""")
.doTest();
}

Expand Down Expand Up @@ -362,7 +366,8 @@ public void stringLiteralRightOperandIsArray() {
// BUG: Diagnostic contains: + Arrays.toString(a)
String b = "a string" + a;
}
}""")
}\
""")
.doTest();
}

Expand All @@ -388,7 +393,8 @@ public void notArray_refactored() {
String b = " a string";
String c = a + b;
}
}""")
}\
""")
.doTest();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,8 @@ public void assertTrue() {
public void assertFalseFromCondition() {
assert 0 == 1;
}
}""")
}\
""")
.doTest();
}

Expand All @@ -70,7 +71,8 @@ public void assertFalse() {
// BUG: Diagnostic contains: throw new AssertionError()
assert false;
}
}""")
}\
""")
.doTest();
}
}
Loading

0 comments on commit 3141d98

Please sign in to comment.