Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Have DirectReturn check consider finally blocks #568

Merged
merged 3 commits into from
Apr 14, 2023
Merged

Have DirectReturn check consider finally blocks #568

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
dc5e1cc
Have `DirectReturn` check consider `finally` blocks
Stephan202 Apr 6, 2023
d513989
More robust check
Stephan202 Apr 7, 2023
d29429c
Naming
Stephan202 Apr 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 51 additions & 7 deletions error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/DirectReturn.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
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;
Expand All @@ -26,14 +27,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;

Expand Down Expand Up @@ -75,7 +80,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)
&& !isIdentifierSymbolReferencedInAssociatedFinallyBlock(variableSymbol, state))
.map(
resultExpr ->
describeMatch(
Expand Down Expand Up @@ -113,13 +121,13 @@ private static Optional<ExpressionTree> 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.
*
* <p>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.
* <p>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(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should now find a better name for this method. Suggestions welcome.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could add Tree or something, but that doesn't add a lot.

In EP they use the term "inlineable". We could do something with that. I cannot think of many suggestions as "inline" is the best verb here IMO.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, my concern here is more that canInline is actually too strong, since now this method only tests are required, but not sufficient set of conditions to be met. I guess we can just merge as-is.

ExpressionTree expressionTree, VisitorState state) {
Expand All @@ -128,4 +136,40 @@ private static boolean canInlineToReturnStatement(
.filter(m -> MoreASTHelpers.areSameType(expressionTree, m.getReturnType(), state))
.isPresent();
}

/**
* Tells whether the given identifier {@link Symbol} is referenced in a {@code finally} block that
* is executed <em>after</em> control flow returns from the {@link VisitorState#getPath() current
* location}.
*/
private static boolean isIdentifierSymbolReferencedInAssociatedFinallyBlock(
Symbol symbol, VisitorState state) {
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, @Nullable BlockTree tree) {
return Boolean.TRUE.equals(
new TreeScanner<Boolean, @Nullable Void>() {
@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));
}
}
52 changes: 52 additions & 0 deletions error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/DirectReturnTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,58 @@ void identification() {
" return variable;",
" };",
" }",
"",
" String redundantAssignmentInsideTryBlock(AutoCloseable closeable) throws Exception {",
" try (closeable) {",
" // BUG: Diagnostic contains:",
" String variable = toString();",
" return variable;",
" }",
" }",
"",
" String redundantAssignmentsInsideTryAndFinallyBlocks() {",
" String variable = toString();",
" try {",
" // BUG: Diagnostic contains:",
" variable = \"foo\";",
" return variable;",
" } finally {",
" String variable2 = toString();",
" if (true) {",
" // BUG: Diagnostic contains:",
" String variable3 = toString();",
" return variable3;",
" }",
" return variable2;",
" }",
" }",
"",
" String assignmentUsedInsideFinallyBlock() {",
" String variable = toString();",
" try {",
" variable = \"foo\";",
" return variable;",
" } finally {",
" String variable2 = toString();",
" 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();
}
Expand Down