NeedsBraces deletes comments, or moves comments around in confusing, inconsistent way

[valters]

What version of OpenRewrite are you using?

I am using

• Maven plugin 5.35.0
• rewrite-static-analysis 1.11.0

How are you running OpenRewrite?

Maven plugin.
Minimal project to repro the issue: https://github.com/valters/test-openrewrite
Please see steps in readme.

What is the smallest, simplest way to reproduce the problem?

I am trying to rewrite following code:

import java.io.File;

public class WithCommentPlacingInBraces {

  public void shouldKeepTrackOfComments(boolean success) {

    // keep track of comments corresponding to lines
    if (success)
      System.out.println("success comment belongs here: but somehow failure comment ends up here"); // success!
    else
      System.out.println("failure comment belongs here: but somehow comment at end of line gets moved before else?"); // failure! this should stay here, firmly inside `else` block
    // this line should stay in place. the comments at end of line, above, should not get mixed up! but somehow this gets moved into `if {}` block?
  }

  public int shouldKeepTrackOfBlockComment(File f) {

    if (f.length() > 0) {
      System.out.println("file is not empty");
    }
    /*
     * this comment should not be lost.
     * it would be great if it went inside the `else {}` block.
     */
    else
      System.out.println("where did the comment go?");
    return 0;
  }

  public int shouldKeepTrackOfComments(String s) {
    if ("ok".equals(s))
      return 1;
      // this comment should not be lost.
      // it would be great if it went inside the `if {}` block.
    else {
      System.out.println("where did the comment go?");
      // nothing
    }
    return 0;
  }
}

What did you expect to see?

Comments should be preserved.
If comments are moved, they should move into block that they logically belong to.

What did you see instead?

diff --git a/source-problems/src/main/java/ch/vingolds/testopenrewrite/WithCommentPlacingInBraces.java b/source-problems/src/main/java/ch/vingolds/testopenrewrite/WithCommentPlacingInBraces.java
index 8dba36a..485fdb2 100644
--- a/source-problems/src/main/java/ch/vingolds/testopenrewrite/WithCommentPlacingInBraces.java
+++ b/source-problems/src/main/java/ch/vingolds/testopenrewrite/WithCommentPlacingInBraces.java
@@ -7,33 +7,29 @@ public class WithCommentPlacingInBraces {
   public void shouldKeepTrackOfComments(boolean success) {
 
     // keep track of comments corresponding to lines
-    if (success)
-      System.out.println("success comment belongs here: but somehow failure comment ends up here"); // success!
-    else
-      System.out.println("failure comment belongs here: but somehow comment at end of line gets moved before else?"); // failure! this should stay here, firmly inside `else` block
-    // this line should stay in place. the comments at end of line, above, should not get mixed up! but somehow this gets moved into `if {}` block?
+    if (success) {
+      System.out.println("success comment belongs here: but somehow failure comment ends up here"); // failure! this should stay here, firmly inside `else` block
+      // this line should stay in place. the comments at end of line, above, should not get mixed up! but somehow this gets moved into `if {}` block?
+    } // success!
+    else {
+      System.out.println("failure comment belongs here: but somehow comment at end of line gets moved before else?");
+    }
   }
 
   public int shouldKeepTrackOfBlockComment(File f) {
 
     if (f.length() > 0) {
       System.out.println("file is not empty");
-    }
-    /*
-     * this comment should not be lost.
-     * it would be great if it went inside the `else {}` block.
-     */
-    else
+    } else {
       System.out.println("where did the comment go?");
+    }
     return 0;
   }
 
   public int shouldKeepTrackOfComments(String s) {
-    if ("ok".equals(s))
+    if ("ok".equals(s)) {
       return 1;
-      // this comment should not be lost.
-      // it would be great if it went inside the `if {}` block.
-    else {
+    } else {
       System.out.println("where did the comment go?");
       // nothing
     }

What is the full stack trace of any errors you encountered?

No errors, just incorrect output.

[timtebeek]

Thanks for the very clear example. I especially like how you labeled the comments to highlight what happens right now, and what you'd like to happen instead.

We have a unit test for comment handling already, but it appears that only looks at the if clause, not checking for any else clause handling.

rewrite-static-analysis/src/test/java/org/openrewrite/staticanalysis/NeedBracesTest.java

Lines 301 to 330 in 48c94b5

	@Test
	void trailingComment() {
	rewriteRun(
	//language=java
	java(
	"""
	class Test {
	static void method() {
	if (true) return; // comment
	if (true) return; // comment 2
	return;
	}
	}
	""",
	"""
	class Test {
	static void method() {
	if (true) {
	return; // comment
	}
	if (true) {
	return; // comment 2
	}
	return;
	}
	}
	"""
	)
	);
	}

Your example would make a great additional unit test to harden the handling here.