Skip to content

Commit

Permalink
Merge pull request #208 from openrewrite/fallthrough-fix
Browse files Browse the repository at this point in the history 
`FallThrough` must only add `break` on normal termination
  • Loading branch information
@knutwannheden
knutwannheden authored Oct 26, 2023
2 parents a4ea014 + 20aba8c commit 6d9048c
Show file tree
Hide file tree
Showing 2 changed files with 91 additions and 38 deletions.
89 changes: 51 additions & 38 deletions src/main/java/org/openrewrite/staticanalysis/FallThroughVisitor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,7 @@
import org.openrewrite.java.tree.*;
import org.openrewrite.marker.Markers;

import java.util.ArrayList;
import java.util.HashSet;
import java.util.List;
import java.util.Set;
import java.util.*;
import java.util.function.Predicate;
import java.util.regex.Pattern;

Expand Down Expand Up @@ -73,11 +70,11 @@ public AddBreak(J.Case scope) {
public J.Case visitCase(J.Case case_, P p) {
J.Case c = super.visitCase(case_, p);
if (scope.isScope(c) &&
c.getStatements().stream().noneMatch(J.Break.class::isInstance) &&
c.getStatements().stream()
.reduce((s1, s2) -> s2)
.map(s -> !(s instanceof J.Block))
.orElse(true)) {
c.getStatements().stream().noneMatch(J.Break.class::isInstance) &&
c.getStatements().stream()
.reduce((s1, s2) -> s2)
.map(s -> !(s instanceof J.Block))
.orElse(true)) {
List<Statement> statements = new ArrayList<>(c.getStatements());
J.Break breakToAdd = autoFormat(
new J.Break(Tree.randomId(), Space.EMPTY, Markers.EMPTY, null),
Expand All @@ -93,11 +90,11 @@ public J.Case visitCase(J.Case case_, P p) {
public J.Block visitBlock(J.Block block, P p) {
J.Block b = super.visitBlock(block, p);
if (getCursor().isScopeInPath(scope) &&
b.getStatements().stream().noneMatch(J.Break.class::isInstance) &&
b.getStatements().stream()
.reduce((s1, s2) -> s2)
.map(s -> !(s instanceof J.Block))
.orElse(true)) {
b.getStatements().stream().noneMatch(J.Break.class::isInstance) &&
b.getStatements().stream()
.reduce((s1, s2) -> s2)
.map(s -> !(s instanceof J.Block))
.orElse(true)) {
List<Statement> statements = b.getStatements();
J.Break breakToAdd = autoFormat(
new J.Break(Tree.randomId(), Space.EMPTY, Markers.EMPTY, null),
Expand Down Expand Up @@ -132,25 +129,53 @@ private static Set<J> find(J.Switch enclosingSwitch, J.Case scope) {
private static class FindLastLineBreaksOrFallsThroughCommentsVisitor extends JavaIsoVisitor<Set<J>> {
private static final Predicate<Comment> HAS_RELIEF_PATTERN_COMMENT = comment ->
comment instanceof TextComment &&
RELIEF_PATTERN.matcher(((TextComment) comment).getText()).find();
RELIEF_PATTERN.matcher(((TextComment) comment).getText()).find();
private final J.Case scope;

public FindLastLineBreaksOrFallsThroughCommentsVisitor(J.Case scope) {
this.scope = scope;
}

private static boolean lastLineBreaksOrFallsThrough(List<? extends Tree> trees) {
private static boolean lastLineBreaksOrFallsThrough(List<? extends Statement> trees) {
return trees.stream()
.reduce((s1, s2) -> s2) // last statement
.map(s -> s instanceof J.Return ||
s instanceof J.Break ||
s instanceof J.Continue ||
s instanceof J.Throw ||
s instanceof J.Switch || // https://github.com/openrewrite/rewrite-static-analysis/issues/173
((J) s).getComments().stream().anyMatch(HAS_RELIEF_PATTERN_COMMENT)
.map(s -> breaks(s) || // https://github.com/openrewrite/rewrite-static-analysis/issues/173
s.getComments().stream().anyMatch(HAS_RELIEF_PATTERN_COMMENT) ||
s instanceof J.Block && ((J.Block) s).getEnd().getComments().stream().anyMatch(HAS_RELIEF_PATTERN_COMMENT)
).orElse(false);
}

private static boolean breaks(Statement s) {
if (s instanceof J.Block) {
List<Statement> statements = ((J.Block) s).getStatements();
return !statements.isEmpty() && breaks(statements.get(statements.size() - 1));
} else if (s instanceof J.If) {
J.If iff = (J.If) s;
return iff.getElsePart() != null && breaks(iff.getThenPart()) && breaks(iff.getThenPart());
} else if (s instanceof J.Label) {
return breaks(((J.Label) s).getStatement());
} else if (s instanceof J.Try) {
J.Try try_ = (J.Try) s;
if (try_.getFinally() != null && breaks(try_.getFinally())) {
return true;
}
if (!breaks(try_.getBody())) {
return false;
}
for (J.Try.Catch c : try_.getCatches()) {
if (!breaks(c.getBody())) {
return false;
}
}
return true;
}
return s instanceof J.Return ||
s instanceof J.Break ||
s instanceof J.Continue ||
s instanceof J.Throw ||
s instanceof J.Switch;
}

@Override
public J.Switch visitSwitch(J.Switch switch_, Set<J> ctx) {
J.Switch s = super.visitSwitch(switch_, ctx);
Expand Down Expand Up @@ -185,24 +210,12 @@ public J.Switch visitSwitch(J.Switch switch_, Set<J> ctx) {

@Override
public J.Case visitCase(J.Case case_, Set<J> ctx) {
J.Case c = super.visitCase(case_, ctx);
if (c == scope) {
if (c.getStatements().isEmpty() || lastLineBreaksOrFallsThrough(c.getStatements())) {
ctx.add(c);
}
}
return c;
}

@Override
public J.Block visitBlock(J.Block block, Set<J> ctx) {
J.Block b = super.visitBlock(block, ctx);
if (getCursor().isScopeInPath(scope)) {
if (lastLineBreaksOrFallsThrough(b.getStatements()) || b.getEnd().getComments().stream().anyMatch(HAS_RELIEF_PATTERN_COMMENT)) {
ctx.add(b);
if (case_ == scope) {
if (case_.getStatements().isEmpty() || lastLineBreaksOrFallsThrough(case_.getStatements())) {
ctx.add(case_);
}
}
return b;
return case_;
}

}
Expand Down
40 changes: 40 additions & 0 deletions src/test/java/org/openrewrite/staticanalysis/FallThroughTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -269,6 +269,46 @@ public void oneCase(int i) {
);
}

@Test
void abortOnAbruptCompletion() {
rewriteRun(
//language=java
java(
"""
public class A {
public void noCase(int i) {
for (;;) {
switch (i) {
case 0:
if (true)
return;
else
break;
case 1:
if (true)
return;
else {
{
continue;
}
}
case 1:
try {
return;
} catch (Exception e) {
break;
}
default:
System.out.println("default");
}
}
}
}
"""
)
);
}

@Test
void addBreaksFallthroughCasesComprehensive() {
rewriteRun(
Expand Down

0 comments on commit 6d9048c

Please sign in to comment.