Skip to content

FallThrough: don't add a break after a guarded early exit #1008

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

Open
Niloyyy wants to merge 1 commit into
base: main
Choose a base branch
from
Open

FallThrough: don't add a break after a guarded early exit #1008

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
23 changes: 23 additions & 0 deletions src/main/java/org/openrewrite/staticanalysis/FallThroughVisitor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,11 +156,34 @@ private static boolean lastLineBreaksOrFallsThrough(List<? extends Statement> tr
return trees.stream()
.reduce((s1, s2) -> s2) // last statement
.map(s -> breaks(s) || // https://github.com/openrewrite/rewrite-static-analysis/issues/173
guardsWithEarlyExit(s) || // https://github.com/openrewrite/rewrite-static-analysis/issues/460
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);
}

/**
* An {@code if} without an {@code else} whose then part completes abruptly guards the statements
* after it, so a fall-through past it is deliberate rather than an omission and adding a
* {@code break} would change behavior. Only blocks and labels are unwrapped; constructs such as
* {@code try} and {@code switch} are deliberately not, because they can complete normally even
* when every {@code if} they contain completes abruptly.
*/
private static boolean guardsWithEarlyExit(Statement s) {
if (s instanceof J.Block) {
List<Statement> statements = ((J.Block) s).getStatements();
return !statements.isEmpty() && guardsWithEarlyExit(statements.get(statements.size() - 1));
}
if (s instanceof J.Label) {
return guardsWithEarlyExit(((J.Label) s).getStatement());
}
if (s instanceof J.If) {
J.If iff = (J.If) s;
return iff.getElsePart() == null && breaks(iff.getThenPart());
}
return false;
}

private static boolean breaks(Statement s) {
if (s instanceof J.Block) {
List<Statement> statements = ((J.Block) s).getStatements();
Expand Down
110 changes: 110 additions & 0 deletions src/test/java/org/openrewrite/staticanalysis/FallThroughTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -452,6 +452,116 @@ public void noCase(int i) {
);
}

@Issue("https://github.com/openrewrite/rewrite-static-analysis/issues/460")
@Test
void doNotAddBreakAfterGuardedEarlyExit() {
// Reduced from quarkus JavadocToAsciidocTransformer.appendEscapedAsciiDoc; the `]` case
// deliberately falls through to the shared escaping branch when not in inline macro mode.
rewriteRun(
//language=java
java(
"""
public class A {
public void escape(String text, boolean inlineMacroMode) {
StringBuilder sb = new StringBuilder();
boolean escaping = false;
for (int i = 0; i < text.length(); i++) {
char ch = text.charAt(i);
switch (ch) {
case ']':
if (inlineMacroMode) {
sb.append("&#93;");
break;
}
case '#':
case '*':
if (!escaping) {
sb.append("++");
escaping = true;
}
sb.append(ch);
break;
default:
sb.append(ch);
}
}
}
}
"""
)
);
}

@Issue("https://github.com/openrewrite/rewrite-static-analysis/issues/460")
@Test
void doNotAddBreakAfterGuardedEarlyExitInsideBlock() {
rewriteRun(
//language=java
java(
"""
public class A {
public void n(int i, boolean b) {
switch (i) {
case 0: {
if (b) {
System.out.println("zero");
break;
}
}
case 1:
System.out.println("one");
break;
}
}
}
"""
)
);
}

@Issue("https://github.com/openrewrite/rewrite-static-analysis/issues/460")
@Test
void addBreakWhenCodeFollowsGuardedEarlyExit() {
rewriteRun(
//language=java
java(
"""
public class A {
public void n(int i, boolean b) {
switch (i) {
case 0:
if (b) {
break;
}
System.out.println("zero");
case 1:
System.out.println("one");
break;
}
}
}
""",
"""
public class A {
public void n(int i, boolean b) {
switch (i) {
case 0:
if (b) {
break;
}
System.out.println("zero");
break;
case 1:
System.out.println("one");
break;
}
}
}
"""
)
);
}

@Test
void addBreaksFallthroughCasesComprehensive() {
rewriteRun(
Expand Down
Loading