Skip to content

Commit

Permalink
Simplify AbstractMustBeClosedChecker with Java 17 features.
Browse files Browse the repository at this point in the history 
Tested:
    TAP for global presubmit queue
    []
PiperOrigin-RevId: 703144708
  • Loading branch information
@chaoren
chaoren authored and Error Prone Team committed Dec 5, 2024
1 parent 53c638d commit b9f4df7
Showing 1 changed file with 45 additions and 56 deletions.
101 changes: 45 additions & 56 deletions core/src/main/java/com/google/errorprone/bugpatterns/AbstractMustBeClosedChecker.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@
import static com.google.errorprone.matchers.Matchers.symbolHasAnnotation;
import static com.google.errorprone.matchers.Matchers.toType;
import static com.google.errorprone.matchers.method.MethodMatchers.staticMethod;
import static com.google.errorprone.util.ASTHelpers.enclosingClass;
import static com.google.errorprone.util.ASTHelpers.findEnclosingNode;
import static com.google.errorprone.util.ASTHelpers.getReceiver;
import static com.google.errorprone.util.ASTHelpers.getReturnType;
import static com.google.errorprone.util.ASTHelpers.getStartPosition;
Expand All @@ -32,8 +34,10 @@
import static com.google.errorprone.util.ASTHelpers.hasAnnotation;
import static com.google.errorprone.util.ASTHelpers.hasExplicitSource;
import static com.google.errorprone.util.ASTHelpers.isConsideredFinal;
import static com.google.errorprone.util.ASTHelpers.isInStaticInitializer;
import static com.google.errorprone.util.ASTHelpers.isSameType;
import static com.google.errorprone.util.ASTHelpers.isSubtype;
import static com.google.errorprone.util.ASTHelpers.variableIsStaticFinal;

import com.google.auto.value.AutoValue;
import com.google.common.base.CaseFormat;
Expand All @@ -46,7 +50,6 @@
import com.google.errorprone.matchers.Description;
import com.google.errorprone.matchers.Matcher;
import com.google.errorprone.matchers.UnusedReturnValueMatcher;
import com.google.errorprone.util.ASTHelpers;
import com.sun.source.tree.BlockTree;
import com.sun.source.tree.ConditionalExpressionTree;
import com.sun.source.tree.ExpressionStatementTree;
Expand All @@ -62,7 +65,6 @@
import com.sun.source.tree.VariableTree;
import com.sun.source.util.TreePath;
import com.sun.source.util.TreeScanner;
import com.sun.tools.javac.code.Symbol;
import com.sun.tools.javac.code.Symbol.MethodSymbol;
import com.sun.tools.javac.code.Symbol.VarSymbol;
import com.sun.tools.javac.code.Type;
Expand Down Expand Up @@ -224,9 +226,9 @@ static Optional<Change> of(SuggestedFix fix) {
* Check that the expression {@code tree} occurs within the resource variable initializer of a
* try-with-resources statement.
*/
private final Optional<Change> matchNewClassOrMethodInvocation(
private Optional<Change> matchNewClassOrMethodInvocation(
ExpressionTree tree, VisitorState state, NameSuggester suggester) {
if (ASTHelpers.isInStaticInitializer(state)) {
if (isInStaticInitializer(state)) {
return Optional.empty();
}
return checkClosed(tree, state, suggester)
Expand All @@ -253,54 +255,53 @@ private Optional<Change> checkClosed(
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());
if (callerMethodTree == null) {
// If enclosingMethod returned null, we must be returning from a statement lambda.
return handleTailPositionInLambda(state);
}
// If enclosingMethod returned null, we must be returning from a statement lambda.
return handleTailPositionInLambda(state);
// 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 LAMBDA_EXPRESSION -> {
// The method invocation is the body of an expression lambda.
return handleTailPositionInLambda(state);
}
case CONDITIONAL_EXPRESSION -> {
ConditionalExpressionTree conditionalExpressionTree =
(ConditionalExpressionTree) path.getLeaf();
var conditionalExpressionTree = (ConditionalExpressionTree) path.getLeaf();
if (conditionalExpressionTree.getTrueExpression().equals(prev.getLeaf())
|| conditionalExpressionTree.getFalseExpression().equals(prev.getLeaf())) {
continue OUTER;
}
}
case MEMBER_SELECT -> {
MemberSelectTree memberSelectTree = (MemberSelectTree) path.getLeaf();
var 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)) {
Type classType = enclosingClass(getSymbol(memberSelectTree)).asType();
Type returnType = getReturnType(memberSelectTree);
if (isSubtype(classType, streamType, state)
&& isSameType(returnType, streamType, state)) {
// skip enclosing method invocation
path = path.getParentPath();
continue OUTER;
}
}
}
case NEW_CLASS -> {
NewClassTree newClassTree = (NewClassTree) path.getLeaf();
var 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
Expand All @@ -311,13 +312,11 @@ && isSameType(type.getReturnType(), streamType, state)) {
}
}
case VARIABLE -> {
Symbol sym = getSymbol(path.getLeaf());
if (sym instanceof VarSymbol var) {
if (var.getKind() == ElementKind.RESOURCE_VARIABLE
|| isClosedInFinallyClause(var, path, state)
|| ASTHelpers.variableIsStaticFinal(var)) {
return Optional.empty();
}
VarSymbol var = getSymbol((VariableTree) path.getLeaf());
if (var.getKind() == ElementKind.RESOURCE_VARIABLE
|| isClosedInFinallyClause(var, path, state)
|| variableIsStaticFinal(var)) {
return Optional.empty();
}
}
case ASSIGNMENT -> {
Expand All @@ -338,8 +337,7 @@ protected Optional<Change> fix(ExpressionTree tree, VisitorState state, NameSugg
}

private static Optional<Change> handleTailPositionInLambda(VisitorState state) {
LambdaExpressionTree lambda =
ASTHelpers.findEnclosingNode(state.getPath(), LambdaExpressionTree.class);
LambdaExpressionTree lambda = findEnclosingNode(state.getPath(), LambdaExpressionTree.class);
if (lambda == null) {
// Apparently we're not inside a lambda?!
return findingWithNoFix();
Expand Down Expand Up @@ -381,21 +379,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 @@ -418,23 +410,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);
if (state.getPath().getParentPath().getLeaf() instanceof VariableTree parent) {
return wrapTryFinallyAroundVariableScope(parent, state);
}
StatementTree stmt = state.findEnclosing(StatementTree.class);
if (stmt == null) {
return Optional.empty();
}
if (!(stmt instanceof VariableTree)) {
if (!(stmt instanceof VariableTree var)) {
return introduceSingleStatementTry(tree, stmt, state, suggester);
}
VarSymbol varSym = getSymbol((VariableTree) stmt);
if (varSym.getKind() == ElementKind.RESOURCE_VARIABLE) {
return extractToResourceInCurrentTry(tree, stmt, state, suggester);
if (getSymbol(var).getKind() == ElementKind.RESOURCE_VARIABLE) {
return extractToResourceInCurrentTry(tree, var, state, suggester);
}
return splitVariableDeclarationAroundTry(tree, (VariableTree) stmt, state, suggester);
return splitVariableDeclarationAroundTry(tree, var, state, suggester);
}

private static Optional<Change> introduceSingleStatementTry(
Expand Down

0 comments on commit b9f4df7

Please sign in to comment.