Fix Comment handling in NeedsBraces #518

Jammy-Louie · 2025-04-16T18:54:53Z

What's changed?

Improve comment handling for NeedBraces recipe for if statements. Comments would be placed incorrectly or dropped if they trailed the if statement.

What's your motivation?

fixes NeedsBraces deletes comments, or moves comments around in confusing, inconsistent way #315

Anything in particular you'd like reviewers to focus on?

Anyone you would like to review specifically?

Have you considered any alternatives or workarounds?

Any additional context

Checklist

I've added unit tests to cover both positive and negative cases
I've read and applied the recipe conventions and best practices
I've used the IntelliJ IDEA auto-formatter on affected files

github-actions

Some suggestions could not be made:

src/test/java/org/openrewrite/staticanalysis/RemoveUnusedPrivateMethodsTest.java
- lines 101-102

github-actions

Some suggestions could not be made:

src/test/java/org/openrewrite/staticanalysis/RemoveUnusedPrivateMethodsTest.java
- lines 101-102

github-actions

Some suggestions could not be made:

src/test/java/org/openrewrite/staticanalysis/RemoveUnusedPrivateMethodsTest.java
- lines 101-102

github-actions

Some suggestions could not be made:

src/main/java/org/openrewrite/staticanalysis/ReplaceClassIsInstanceWithInstanceof.java
- lines 36-37

github-actions

Some suggestions could not be made:

src/main/java/org/openrewrite/staticanalysis/ReplaceClassIsInstanceWithInstanceof.java
- lines 36-37

Jammy-Louie · 2025-04-22T14:35:41Z

src/main/java/org/openrewrite/staticanalysis/NeedBraces.java

+            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();
+            }


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

Jammy-Louie · 2025-04-22T14:37:11Z

src/main/java/org/openrewrite/staticanalysis/NeedBraces.java

@@ -69,20 +69,42 @@ 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) {


remove owner as an argument because it is no longer needed

Jammy-Louie · 2025-04-22T14:41:04Z

src/main/java/org/openrewrite/staticanalysis/NeedBraces.java

+                    if (last) {
+                        blockParentCursor.putMessage("removeEndComments", true);
+                    } else {
+                        blockParentCursor.<List<Integer>>computeMessageIfAbsent("replaced", k -> new ArrayList<>()).add(currentIndex);


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

Jammy-Louie · 2025-04-22T14:42:35Z

src/main/java/org/openrewrite/staticanalysis/NeedBraces.java

+            Boolean removeEndComments = getCursor().pollMessage("removeEndComments");
+            if (removeEndComments != null) {
+                bl = bl.withEnd(bl.getEnd().withComments(Collections.emptyList()));
+                bl = maybeAutoFormat(block, bl, ctx);
+            }


remove comments when the trailing comments found at the end of the block

Jammy-Louie · 2025-04-22T14:43:42Z

src/main/java/org/openrewrite/staticanalysis/NeedBraces.java

+                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());
+                }
+


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 }

Jammy-Louie · 2025-04-22T14:45:30Z

src/main/java/org/openrewrite/staticanalysis/NeedBraces.java

+                Space prefix = elem.getPrefix();
+                Statement body = elem.getBody();
+
+                if(!prefix.getComments().isEmpty() && prefix.getWhitespace().contains("\n")) {
+                    body = body.withPrefix(prefix);
+                }


this handles the case where braces appear on the if statement but not the else

if (true) { return; } /* * do something /* else return;

github-actions

Some suggestions could not be made:

src/main/java/org/openrewrite/staticanalysis/ReplaceClassIsInstanceWithInstanceof.java
- lines 36-37

Jammy-Louie · 2025-04-22T15:11:39Z

testing locally on openrewrite

only 1 change suggestion and appears to be correct

Jammy-Louie · 2025-04-22T15:22:13Z

testing locally on apache

Suggested changes appear to be correct for Java Source files.

Sample change:

although there does appear to be issues when running against Javascript files:

Issue appears to be present before my changes as I've tested the recipe from main and still see the same results.

src/main/java/org/openrewrite/staticanalysis/NeedBraces.java

Jenson3210 · 2025-05-04T12:12:25Z

Other than this 1 comment, it looks good to me.
Glad to see this coverage of tests added as prefixes/comments removal could be easily overlooked in a PR generated by a recipe.

Co-authored-by: Jente Sondervorst <[email protected]>

Jenson3210

I see @timtebeek has pushed my proposed change, so other that this LGTM!

github-actions

Some suggestions could not be made:

src/main/resources/META-INF/rewrite/examples.yml
- lines 57-56
- lines 634-633
- lines 679-695
- lines 1228-1247
- lines 1280-1279
- lines 2309-2308
- lines 2324-2336
- lines 3442-3453
- lines 3467-3472

timtebeek

Some optional suggestions, fully realize these might be awkward to make work, and this is already a good improvement, so we need not drag it out. Could be worth briefly exploring before a merge.

src/test/java/org/openrewrite/staticanalysis/NeedBracesTest.java

timtebeek · 2025-05-04T20:43:45Z

src/test/java/org/openrewrite/staticanalysis/NeedBracesTest.java

+                      } else {
+                          /*
+                           * comment should be in else block
+                           */
+                          return;
+                      }


Similarly, I had expected the following to minimize the diff with what was there before. All we ought to be doing is adding braces, where possible.

Suggested change

} else {

/*

* comment should be in else block

*/

return;

}

}

/*

* comment should be in else block

*/

else {

return;

}

github-actions

Some suggestions could not be made:

src/main/resources/META-INF/rewrite/examples.yml
- lines 57-56
- lines 634-633
- lines 679-695
- lines 1228-1247
- lines 1280-1279
- lines 2309-2308
- lines 2324-2336
- lines 3442-3453
- lines 3467-3472

Jammy-Louie self-assigned this Apr 16, 2025

moderne-meeseeks bot added this to OpenRewrite Apr 16, 2025

github-project-automation bot moved this to In Progress in OpenRewrite Apr 16, 2025

Jammy-Louie added the bug Something isn't working label Apr 16, 2025

github-actions bot reviewed Apr 16, 2025

View reviewed changes

Jammy-Louie force-pushed the issue-315-NeedsBraces-fix-comments branch from 01f25d9 to edfda4f Compare April 17, 2025 13:10

github-actions bot reviewed Apr 17, 2025

View reviewed changes

Jammy-Louie force-pushed the issue-315-NeedsBraces-fix-comments branch from edfda4f to a4b7233 Compare April 17, 2025 13:18

github-actions bot reviewed Apr 17, 2025

View reviewed changes

Jammy-Louie force-pushed the issue-315-NeedsBraces-fix-comments branch from 8955931 to 586cf50 Compare April 22, 2025 14:05

Initial Tests

e6f509e

Jammy-Louie force-pushed the issue-315-NeedsBraces-fix-comments branch from 586cf50 to e6f509e Compare April 22, 2025 14:17

github-actions bot reviewed Apr 22, 2025

View reviewed changes

Jammy-Louie commented Apr 22, 2025

View reviewed changes

Simplify buildBlock method and improve handling of comments

3209dcd

Jammy-Louie force-pushed the issue-315-NeedsBraces-fix-comments branch from 1dd2db0 to 3209dcd Compare April 22, 2025 14:48

github-actions bot reviewed Apr 22, 2025

View reviewed changes

apply code suggestion

2e70cb0

Jammy-Louie marked this pull request as ready for review April 22, 2025 15:27

greg-at-moderne reviewed Apr 22, 2025

View reviewed changes

src/main/java/org/openrewrite/staticanalysis/NeedBraces.java Show resolved Hide resolved

Jammy-Louie moved this from In Progress to Ready to Review in OpenRewrite Apr 22, 2025

timtebeek self-requested a review May 3, 2025 11:46

Jenson3210 reviewed May 4, 2025

View reviewed changes

src/main/java/org/openrewrite/staticanalysis/NeedBraces.java Outdated Show resolved Hide resolved

Update src/main/java/org/openrewrite/staticanalysis/NeedBraces.java

7537920

Co-authored-by: Jente Sondervorst <[email protected]>

This comment was marked as off-topic.

Sign in to view

Jenson3210 approved these changes May 4, 2025

View reviewed changes

Merge branch 'main' into issue-315-NeedsBraces-fix-comments

07f6a27

github-actions bot reviewed May 4, 2025

View reviewed changes

timtebeek reviewed May 4, 2025

View reviewed changes

Do not move comments on do while loop

4a04af9

github-actions bot reviewed May 4, 2025

View reviewed changes

timtebeek merged commit 60fb46d into main May 4, 2025
1 of 2 checks passed

timtebeek deleted the issue-315-NeedsBraces-fix-comments branch May 4, 2025 21:04

github-project-automation bot moved this from Ready to Review to Done in OpenRewrite May 4, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Comment handling in NeedsBraces #518

Fix Comment handling in NeedsBraces #518

Jammy-Louie commented Apr 16, 2025 •

edited by timtebeek

Loading

github-actions bot left a comment

github-actions bot left a comment

github-actions bot left a comment

github-actions bot left a comment

github-actions bot left a comment

Jammy-Louie Apr 22, 2025

Jammy-Louie Apr 22, 2025

Jammy-Louie Apr 22, 2025

Jammy-Louie Apr 22, 2025

Jammy-Louie Apr 22, 2025

Jammy-Louie Apr 22, 2025

github-actions bot left a comment

Jammy-Louie commented Apr 22, 2025

Jammy-Louie commented Apr 22, 2025 •

edited

Loading

Jenson3210 commented May 4, 2025

This comment was marked as off-topic.

Jenson3210 left a comment

github-actions bot left a comment

timtebeek left a comment

timtebeek May 4, 2025

github-actions bot left a comment

Fix Comment handling in NeedsBraces #518

Fix Comment handling in NeedsBraces #518

Conversation

Jammy-Louie commented Apr 16, 2025 • edited by timtebeek Loading

What's changed?

What's your motivation?

Anything in particular you'd like reviewers to focus on?

Anyone you would like to review specifically?

Have you considered any alternatives or workarounds?

Any additional context

Checklist

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

Jammy-Louie Apr 22, 2025

Choose a reason for hiding this comment

Jammy-Louie Apr 22, 2025

Choose a reason for hiding this comment

Jammy-Louie Apr 22, 2025

Choose a reason for hiding this comment

Jammy-Louie Apr 22, 2025

Choose a reason for hiding this comment

Jammy-Louie Apr 22, 2025

Choose a reason for hiding this comment

Jammy-Louie Apr 22, 2025

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

Jammy-Louie commented Apr 22, 2025

Jammy-Louie commented Apr 22, 2025 • edited Loading

Jenson3210 commented May 4, 2025

This comment was marked as off-topic.

Jenson3210 left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

timtebeek left a comment

Choose a reason for hiding this comment

timtebeek May 4, 2025

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

Jammy-Louie commented Apr 16, 2025 •

edited by timtebeek

Loading

Jammy-Louie commented Apr 22, 2025 •

edited

Loading