From dc5e1ccfb59efe894853dcef3624b2fc7cc6b195 Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Thu, 6 Apr 2023 21:24:36 +0200 Subject: [PATCH 1/3] Have `DirectReturn` check consider `finally` blocks --- .../errorprone/bugpatterns/DirectReturn.java | 50 ++++++++++++++++--- .../bugpatterns/DirectReturnTest.java | 33 ++++++++++++ 2 files changed, 76 insertions(+), 7 deletions(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/DirectReturn.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/DirectReturn.java index ee0091eeeb..a6da45d3d3 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/DirectReturn.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/DirectReturn.java @@ -11,9 +11,11 @@ import static com.google.errorprone.matchers.Matchers.returnStatement; import static com.google.errorprone.matchers.Matchers.staticMethod; import static com.google.errorprone.matchers.Matchers.toType; +import static java.util.Objects.requireNonNull; import static tech.picnic.errorprone.bugpatterns.util.Documentation.BUG_PATTERNS_BASE_URL; import com.google.auto.service.AutoService; +import com.google.common.collect.Streams; import com.google.errorprone.BugPattern; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker; @@ -26,14 +28,18 @@ import com.sun.source.tree.BlockTree; import com.sun.source.tree.ExpressionStatementTree; import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.IdentifierTree; import com.sun.source.tree.MethodInvocationTree; import com.sun.source.tree.ReturnTree; import com.sun.source.tree.StatementTree; import com.sun.source.tree.Tree; +import com.sun.source.tree.TryTree; import com.sun.source.tree.VariableTree; +import com.sun.source.util.TreeScanner; import com.sun.tools.javac.code.Symbol; import java.util.List; import java.util.Optional; +import org.jspecify.annotations.Nullable; import tech.picnic.errorprone.bugpatterns.util.MoreASTHelpers; import tech.picnic.errorprone.bugpatterns.util.SourceCode; @@ -75,7 +81,10 @@ public Description matchBlock(BlockTree tree, VisitorState state) { StatementTree precedingStatement = statements.get(statements.size() - 2); return tryMatchAssignment(variableSymbol, precedingStatement) - .filter(resultExpr -> canInlineToReturnStatement(resultExpr, state)) + .filter( + resultExpr -> + canInlineToReturnStatement(resultExpr, state) + && !isSymbolReferencedInAssociatedFinallyBlock(variableSymbol, state)) .map( resultExpr -> describeMatch( @@ -113,13 +122,13 @@ private static Optional tryMatchAssignment(Symbol targetSymbol, } /** - * Tells whether inlining the given expression to the associated return statement can be done - * safely. + * Tells whether inlining the given expression to the associated return statement can likely be + * done without changing the expression's return type. * - *

Inlining is generally safe, but in rare cases the operation may have a functional impact. - * The sole case considered here is the inlining of a Mockito mock or spy construction without an - * explicit type. In such a case the type created depends on context, such as the method's return - * type. + *

Inlining an expression generally does not change its return type, but in rare cases the + * operation may have a functional impact. The sole case considered here is the inlining of a + * Mockito mock or spy construction without an explicit type. In such a case the type created + * depends on context, such as the method's return type. */ private static boolean canInlineToReturnStatement( ExpressionTree expressionTree, VisitorState state) { @@ -128,4 +137,31 @@ private static boolean canInlineToReturnStatement( .filter(m -> MoreASTHelpers.areSameType(expressionTree, m.getReturnType(), state)) .isPresent(); } + + private static boolean isSymbolReferencedInAssociatedFinallyBlock( + Symbol symbol, VisitorState state) { + BlockTree currentBlock = + requireNonNull(state.findEnclosing(BlockTree.class), "Current path not inside block"); + return Streams.stream(state.getPath()) + .filter(TryTree.class::isInstance) + .map(TryTree.class::cast) + .map(TryTree::getFinallyBlock) + .filter(finallyBlock -> !currentBlock.equals(finallyBlock)) + .anyMatch(finallyBlock -> referencesIdentifierSymbol(symbol, finallyBlock)); + } + + private static boolean referencesIdentifierSymbol(Symbol symbol, BlockTree tree) { + return Boolean.TRUE.equals( + new TreeScanner() { + @Override + public Boolean visitIdentifier(IdentifierTree node, @Nullable Void unused) { + return symbol.equals(ASTHelpers.getSymbol(node)); + } + + @Override + public Boolean reduce(Boolean r1, Boolean r2) { + return Boolean.TRUE.equals(r1) || Boolean.TRUE.equals(r2); + } + }.scan(tree, null)); + } } diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/DirectReturnTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/DirectReturnTest.java index be455b123f..c89de1ff7e 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/DirectReturnTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/DirectReturnTest.java @@ -136,6 +136,39 @@ void identification() { " return variable;", " };", " }", + "", + " String redundantAssignmentInsideTryBlock(AutoCloseable closeable) throws Exception {", + " try (closeable) {", + " // BUG: Diagnostic contains:", + " String variable = toString();", + " return variable;", + " }", + " }", + "", + " String redundantAssignmentsInsideTryAndFinallyBlocks() throws Exception {", + " String variable = toString();", + " try {", + " // BUG: Diagnostic contains:", + " variable = \"foo\";", + " return variable;", + " } finally {", + " String variable2 = toString();", + " // BUG: Diagnostic contains:", + " String variable3 = toString();", + " return variable3;", + " }", + " }", + "", + " String assignmentUsedInsideFinallyBlock() throws Exception {", + " String variable = toString();", + " try {", + " variable = \"foo\";", + " return variable;", + " } finally {", + " String variable2 = toString();", + " return variable + variable2;", + " }", + " }", "}") .doTest(); } From d5139898846dc1838c2f04fac7dad6ab090022f6 Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Fri, 7 Apr 2023 06:58:26 +0200 Subject: [PATCH 2/3] More robust check --- .../errorprone/bugpatterns/DirectReturn.java | 26 +++++++++++------ .../bugpatterns/DirectReturnTest.java | 29 +++++++++++++++---- 2 files changed, 41 insertions(+), 14 deletions(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/DirectReturn.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/DirectReturn.java index a6da45d3d3..6d43afd32b 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/DirectReturn.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/DirectReturn.java @@ -11,7 +11,6 @@ import static com.google.errorprone.matchers.Matchers.returnStatement; import static com.google.errorprone.matchers.Matchers.staticMethod; import static com.google.errorprone.matchers.Matchers.toType; -import static java.util.Objects.requireNonNull; import static tech.picnic.errorprone.bugpatterns.util.Documentation.BUG_PATTERNS_BASE_URL; import com.google.auto.service.AutoService; @@ -138,19 +137,28 @@ private static boolean canInlineToReturnStatement( .isPresent(); } + /** + * Tells whether the given {@link Symbol} is referenced in a {@code finally} block that is + * executed after control flow returns from the {@link VisitorState#getPath() current + * location}. + */ private static boolean isSymbolReferencedInAssociatedFinallyBlock( Symbol symbol, VisitorState state) { - BlockTree currentBlock = - requireNonNull(state.findEnclosing(BlockTree.class), "Current path not inside block"); - return Streams.stream(state.getPath()) - .filter(TryTree.class::isInstance) - .map(TryTree.class::cast) - .map(TryTree::getFinallyBlock) - .filter(finallyBlock -> !currentBlock.equals(finallyBlock)) + return Streams.zip( + Streams.stream(state.getPath()).skip(1), + Streams.stream(state.getPath()), + (tree, child) -> { + if (!(tree instanceof TryTree)) { + return null; + } + + BlockTree finallyBlock = ((TryTree) tree).getFinallyBlock(); + return !child.equals(finallyBlock) ? finallyBlock : null; + }) .anyMatch(finallyBlock -> referencesIdentifierSymbol(symbol, finallyBlock)); } - private static boolean referencesIdentifierSymbol(Symbol symbol, BlockTree tree) { + private static boolean referencesIdentifierSymbol(Symbol symbol, @Nullable BlockTree tree) { return Boolean.TRUE.equals( new TreeScanner() { @Override diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/DirectReturnTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/DirectReturnTest.java index c89de1ff7e..42e1889414 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/DirectReturnTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/DirectReturnTest.java @@ -145,7 +145,7 @@ void identification() { " }", " }", "", - " String redundantAssignmentsInsideTryAndFinallyBlocks() throws Exception {", + " String redundantAssignmentsInsideTryAndFinallyBlocks() {", " String variable = toString();", " try {", " // BUG: Diagnostic contains:", @@ -153,13 +153,16 @@ void identification() { " return variable;", " } finally {", " String variable2 = toString();", - " // BUG: Diagnostic contains:", - " String variable3 = toString();", - " return variable3;", + " if (true) {", + " // BUG: Diagnostic contains:", + " String variable3 = toString();", + " return variable3;", + " }", + " return variable2;", " }", " }", "", - " String assignmentUsedInsideFinallyBlock() throws Exception {", + " String assignmentUsedInsideFinallyBlock() {", " String variable = toString();", " try {", " variable = \"foo\";", @@ -169,6 +172,22 @@ void identification() { " return variable + variable2;", " }", " }", + "", + " String redundantAssignmentToVariableUsedInsideUnexecutedFinallyBlock(AutoCloseable closeable)", + " throws Exception {", + " String variable = toString();", + " try (closeable) {", + " if (true) {", + " // BUG: Diagnostic contains:", + " variable = \"foo\";", + " return variable;", + " }", + " }", + " try {", + " } finally {", + " return variable;", + " }", + " }", "}") .doTest(); } From d29429c5c909e3485b6b67890c4b873792ad2295 Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Fri, 14 Apr 2023 12:40:32 +0200 Subject: [PATCH 3/3] Naming --- .../tech/picnic/errorprone/bugpatterns/DirectReturn.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/DirectReturn.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/DirectReturn.java index 6d43afd32b..212d0b1245 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/DirectReturn.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/DirectReturn.java @@ -83,7 +83,7 @@ public Description matchBlock(BlockTree tree, VisitorState state) { .filter( resultExpr -> canInlineToReturnStatement(resultExpr, state) - && !isSymbolReferencedInAssociatedFinallyBlock(variableSymbol, state)) + && !isIdentifierSymbolReferencedInAssociatedFinallyBlock(variableSymbol, state)) .map( resultExpr -> describeMatch( @@ -138,11 +138,11 @@ private static boolean canInlineToReturnStatement( } /** - * Tells whether the given {@link Symbol} is referenced in a {@code finally} block that is - * executed after control flow returns from the {@link VisitorState#getPath() current + * Tells whether the given identifier {@link Symbol} is referenced in a {@code finally} block that + * is executed after control flow returns from the {@link VisitorState#getPath() current * location}. */ - private static boolean isSymbolReferencedInAssociatedFinallyBlock( + private static boolean isIdentifierSymbolReferencedInAssociatedFinallyBlock( Symbol symbol, VisitorState state) { return Streams.zip( Streams.stream(state.getPath()).skip(1),