fix: remove unnecessary continue in empty while loops

[juancaCeb]

What's changed?

Changed EmptyBlockVisitor "visitWhileLoop" and "visitDoWhileLoop" methods so that they do not add any "continue" statement. Instead, If there is a continue statement, it should remove them.

What's your motivation?

• Closes EmptyBlock adds unnecessary continue in empty while loops #570

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

I would like to check if it's okay to put the logic of removing the continue statement on the same class. Or if not, where should it be?

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/main/java/org/openrewrite/staticanalysis/CustomImportOrder.java
  • lines 24-24

[greg-at-moderne]

When reading the code, it seems like it handles the case of removing an existing continue statement in while loop if it's the only statement. But does the code handle empty loops after the change? I suggest you add an explicit test case for this.

[greg-at-moderne]

In other words, there are at least two scenarios on input:

• a while-loop which is empty
• a while-loop which just has a continue statement and nothing else.

I think before the changes the recipe handled the first case. Your code seems to change it to handling the second case. I suggest to keep the support for case no. 1, and if you prefer we can add support for case no. 2 on top of that.

[juancaCeb]

Thanks for the clarifications! I do have one question though:
Originally, the behavior only added a continue statement to empty while loops. That's why I did the opposite which is to remove the continue statement. However, keeping both behaviors would introduce a conflict between them. Was the original behavior intended to do something else?

[greg-at-moderne]

OK. Sorry, my input might have been confusing. I've just taken a broader look and the things are more complex than I thought. Still I think there are things to fix and your contribution is welcome.

• When looking at EmptyBlock, its description and name, it clearly indicates empty blocks should be removed. They are not. Instead some continue statements are added.
• At the same time, there's a thing called EmptyBlockStyle which is a very detailed way of modelling what should happen with empty blocks.
• As a consequence I think we need multiple variants of the EmptyBlock logic. As we need to cater for multiple behaviors.
• BlockPolicy seems to have two values: TEXT, STATEMENT. None of them calls for removing the blocks.

My personal preference would be for the EmptyBlock recipe to remove blocks by default. And optionally do something else if instructed.

Thus I suggest:

• keep the BlockPolicy and EmptyBlockStyle as they are,
• change the default behavior of EmptyBlock - the default should remove the empty blocks
• if a valid EmptyBlockStyle is passed, it should be honored accordingly

@timtebeek Can you please review the suggestion?

[timtebeek]

Also my first time looking in detail here; I propose we implement the following:

    @Nested
    class WhileLoop {
        @Test
        void emptyWhileDefault() { // Checkstyle.emptyBlock() uses `BlockPolicy` `TEXT`
            rewriteRun(
              //language=java
              java(
                """
                  public class A {
                      public void foo() {
                          while(true) {
                          }
                          do {
                          } while(true);
                      }
                  }
                  """,
                """
                  public class A {
                      public void foo() {
                          while(true) {
                              // do nothing
                          }
                          do {
                              // do nothing
                          } while(true);
                      }
                  }
                  """
              )
            );
        }

        @Test
        void whileWithContinueDefault() {
            rewriteRun(
              //language=java
              java( // No change expected
                """
                  public class A {
                      public void foo() {
                          while(true) {
                              continue;
                          }
                          do {
                              continue;
                          } while(true);
                      }
                  }
                  """
              )
            );
        }

        @Test
        void emptyWhileStyleStatement() {
            rewriteRun(
              spec -> {
                  spec.parser(JavaParser.fromJavaVersion().styles(
                    singleton(Checkstyle.defaults()
                      .withStyles(singleton(Checkstyle.emptyBlock()
                        .withBlockPolicy(EmptyBlockStyle.BlockPolicy.STATEMENT))))
                  ));
              },
              //language=java
              java(
                """
                  public class A {
                      public void foo() {
                          while(true) {
                          }
                          do {
                          } while(true);
                      }
                  }
                  """,
                """
                  public class A {
                      public void foo() {
                          while(true) {
                              continue;
                          }
                          do {
                              continue;
                          } while(true);
                      }
                  }
                  """
              )
            );
        }
    }

[timtebeek]

I do not think we can safely remove empty while blocks; I'm somewhat surprised a recipe aimed at removing empty blocks is even touching these at all, or changing their body, but the above strikes the right balance I think.

The reason I think we can not remove empty while blocks is things like the following, which are valid & should not be removed

while (input.read() != null) {} // fully consume input

[timtebeek]

Wondering a bit about if, and which changes we should be making here; as pointed out in

• https://github.com/openrewrite/rewrite-static-analysis/pull/575/files#r2132004866