Skip to content

Fix Comment handling in NeedsBraces #518

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

Merged
merged 6 commits into from
May 4, 2025
Merged

Fix Comment handling in NeedsBraces #518

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e6f509e
Initial Tests
Jammy-Louie Apr 22, 2025
3209dcd
Simplify buildBlock method and improve handling of comments
Jammy-Louie Apr 22, 2025
2e70cb0
apply code suggestion
Jammy-Louie Apr 22, 2025
7537920
Update src/main/java/org/openrewrite/staticanalysis/NeedBraces.java
timtebeek May 4, 2025
07f6a27
Merge branch 'main' into issue-315-NeedsBraces-fix-comments
timtebeek May 4, 2025
4a04af9
Do not move comments on do while loop
timtebeek May 4, 2025
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
83 changes: 64 additions & 19 deletions src/main/java/org/openrewrite/staticanalysis/NeedBraces.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,10 @@
import org.openrewrite.java.JavaIsoVisitor;
import org.openrewrite.java.style.Checkstyle;
import org.openrewrite.java.style.NeedBracesStyle;
import org.openrewrite.java.tree.*;
import org.openrewrite.java.tree.J;
import org.openrewrite.java.tree.JRightPadded;
import org.openrewrite.java.tree.Space;
import org.openrewrite.java.tree.Statement;
import org.openrewrite.kotlin.tree.K;
import org.openrewrite.marker.Markers;

Expand Down Expand Up @@ -69,20 +72,43 @@ private static class NeedBracesVisitor extends JavaIsoVisitor<ExecutionContext>
* We can use that to our advantage by saying if you aren't a block (e.g. a single {@link Statement}, etc.),
* then we're going to make this into a block. That's how we'll get the code bodies surrounded in braces.
*/
private <T extends Statement> J.Block buildBlock(Statement owner, T element) {
J j = getCursor().getParentTreeCursor().getValue();
private <T extends Statement> J.Block buildBlock(T element) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

remove owner as an argument because it is no longer needed

J rootElement = null;
Space end = Space.EMPTY;
if (j instanceof J.Block) {
J.Block block = (J.Block) j;

Cursor currentCursor = getCursor();
while (
currentCursor != null &&
currentCursor.getParent() != null &&
!(currentCursor.getParent().getValue() instanceof J.Block)
) {
currentCursor = currentCursor.getParent();
}

if (currentCursor != null && currentCursor.getValue() instanceof JRightPadded) {
JRightPadded<J> paddedIf = currentCursor.getValue();
rootElement = paddedIf.getElement();
}
Comment on lines +79 to +91
Copy link
Contributor Author

Choose a reason for hiding this comment

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

need to find the root element because we can run into nested elements such as

if(true) { return ; }
else if (true) { return ; }
else if (true) { return ; }
else return

The else block would be nested at the bottom of the of the If LST and we would need to find the root If statement because that is the Statement in the J.Block


// Move comments
if (rootElement instanceof Statement && !(rootElement instanceof J.DoWhileLoop)) {
Cursor blockParentCursor = currentCursor.getParent();
J.Block block = blockParentCursor.getValue();
List<Statement> statements = block.getStatements();
int i = statements.indexOf(owner);
boolean last = i == statements.size() - 1;
Space trailingSpace = last ? block.getEnd() : statements.get(i + 1).getPrefix();
if (!trailingSpace.getComments().isEmpty() && trailingSpace.getWhitespace().indexOf('\n') == -1) {
end = trailingSpace;
getCursor().getParentTreeCursor().<List<Integer>>computeMessageIfAbsent("replaced", k -> new ArrayList<>()).add(i);
int currentIndex = statements.indexOf(rootElement);
boolean last = currentIndex == statements.size() - 1;
Space trailingComment = last ? block.getEnd() : statements.get(currentIndex + 1).getPrefix();

if (!trailingComment.isEmpty() && !trailingComment.getWhitespace().contains("\n")) {
end = trailingComment;
if (last) {
blockParentCursor.putMessage("removeEndComments", true);
} else {
blockParentCursor.<List<Integer>>computeMessageIfAbsent("replaced", k -> new ArrayList<>()).add(currentIndex);
Comment on lines +104 to +107
Copy link
Contributor Author

Choose a reason for hiding this comment

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

messages can appear at the end of the block for cases such as.

static void method() {
    if (true) return; // if comment
    else return; // else comment
}

for this case the // else comment would be located at the end of the block

}
}
}

return new J.Block(
Tree.randomId(),
Space.EMPTY,
Expand Down Expand Up @@ -111,6 +137,10 @@ private <T extends Statement> J.Block buildBlock(Statement owner, T element) {
@Override
public J.Block visitBlock(J.Block block, ExecutionContext ctx) {
J.Block bl = super.visitBlock(block, ctx);
if (Boolean.TRUE.equals(getCursor().pollMessage("removeEndComments"))) {
bl = bl.withEnd(bl.getEnd().withComments(Collections.emptyList()));
bl = maybeAutoFormat(block, bl, ctx);
}
List<Integer> indexes = getCursor().pollMessage("replaced");
if (indexes != null) {
for (int index : indexes) {
Expand Down Expand Up @@ -140,7 +170,15 @@ public J.If visitIf(J.If iff, ExecutionContext ctx) {
J.If elem = super.visitIf(iff, ctx);
boolean hasAllowableBodyType = elem.getThenPart() instanceof J.Block;
if (!needBracesStyle.getAllowSingleLineStatement() && !hasAllowableBodyType) {
J.Block b = buildBlock(elem, elem.getThenPart());
J.Block b;
if (elem.getElsePart() != null && !elem.getElsePart().getPrefix().getComments().isEmpty()) {
Space end = elem.getElsePart().getPrefix();
elem = elem.withElsePart(elem.getElsePart().withPrefix(Space.EMPTY));
b = buildBlock(elem.getThenPart()).withEnd(end);
} else {
b = buildBlock(elem.getThenPart());
}

Comment on lines +174 to +181
Copy link
Contributor Author

Choose a reason for hiding this comment

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

trailing comments for if statements can appear as a prefix in the else

example:

static void method() {
    if (true) return; // if comment
    else return; // else comment
}

elem = maybeAutoFormat(elem, elem.withThenPart(b), ctx);
}
return elem;
Expand All @@ -155,7 +193,14 @@ public J.If.Else visitElse(J.If.Else else_, ExecutionContext ctx) {
J.If.Else elem = super.visitElse(else_, ctx);
boolean hasAllowableBodyType = elem.getBody() instanceof J.Block || elem.getBody() instanceof J.If;
if (!needBracesStyle.getAllowSingleLineStatement() && !hasAllowableBodyType) {
J.Block b = buildBlock(getCursor().getParentTreeCursor().getValue(), elem.getBody());
Space prefix = elem.getPrefix();
Statement body = elem.getBody();

if (!prefix.getComments().isEmpty() && prefix.getWhitespace().contains("\n")) {
body = body.withPrefix(prefix);
}

J.Block b = buildBlock(body);
elem = maybeAutoFormat(elem, elem.withBody(b), ctx);
}
return elem;
Expand All @@ -168,10 +213,10 @@ public J.WhileLoop visitWhileLoop(J.WhileLoop whileLoop, ExecutionContext ctx) {
elem.getBody() instanceof J.Block || elem.getBody() instanceof J.Empty :
elem.getBody() instanceof J.Block;
if (!needBracesStyle.getAllowEmptyLoopBody() && elem.getBody() instanceof J.Empty) {
J.Block b = buildBlock(elem, elem.getBody());
J.Block b = buildBlock(elem.getBody());
elem = maybeAutoFormat(elem, elem.withBody(b), ctx);
} else if (!needBracesStyle.getAllowSingleLineStatement() && !hasAllowableBodyType) {
J.Block b = buildBlock(elem, elem.getBody());
J.Block b = buildBlock(elem.getBody());
elem = maybeAutoFormat(elem, elem.withBody(b), ctx);
}
return elem;
Expand All @@ -184,10 +229,10 @@ public J.DoWhileLoop visitDoWhileLoop(J.DoWhileLoop doWhileLoop, ExecutionContex
elem.getBody() instanceof J.Block || elem.getBody() instanceof J.Empty :
elem.getBody() instanceof J.Block;
if (!needBracesStyle.getAllowEmptyLoopBody() && elem.getBody() instanceof J.Empty) {
J.Block b = buildBlock(elem, elem.getBody());
J.Block b = buildBlock(elem.getBody());
elem = maybeAutoFormat(elem, elem.withBody(b), ctx);
} else if (!needBracesStyle.getAllowSingleLineStatement() && !hasAllowableBodyType) {
J.Block b = buildBlock(elem, elem.getBody());
J.Block b = buildBlock(elem.getBody());
elem = maybeAutoFormat(elem, elem.withBody(b), ctx);
}
return elem;
Expand All @@ -200,10 +245,10 @@ public J.ForLoop visitForLoop(J.ForLoop forLoop, ExecutionContext ctx) {
elem.getBody() instanceof J.Block || elem.getBody() instanceof J.Empty :
elem.getBody() instanceof J.Block;
if (!needBracesStyle.getAllowEmptyLoopBody() && elem.getBody() instanceof J.Empty) {
J.Block b = buildBlock(elem, elem.getBody());
J.Block b = buildBlock(elem.getBody());
elem = maybeAutoFormat(elem, elem.withBody(b), ctx);
} else if (!needBracesStyle.getAllowSingleLineStatement() && !hasAllowableBodyType) {
J.Block b = buildBlock(elem, elem.getBody());
J.Block b = buildBlock(elem.getBody());
elem = maybeAutoFormat(elem, elem.withBody(b), ctx);
}
return elem;
Expand Down
Loading
Loading