Skip to content

fix: remove unnecessary continue in empty while loops #575

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

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

fix: remove unnecessary continue in empty while loops #575

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
14 changes: 10 additions & 4 deletions src/main/java/org/openrewrite/staticanalysis/EmptyBlockVisitor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,9 +48,12 @@ public class EmptyBlockVisitor<P> extends JavaIsoVisitor<P> {
public J.WhileLoop visitWhileLoop(J.WhileLoop whileLoop, P p) {
J.WhileLoop w = super.visitWhileLoop(whileLoop, p);

if (Boolean.TRUE.equals(emptyBlockStyle.getLiteralWhile()) && isEmptyBlock(w.getBody())) {
if (Boolean.TRUE.equals(emptyBlockStyle.getLiteralWhile()) && w.getBody() instanceof J.Block) {
J.Block body = (J.Block) w.getBody();
w = continueStatement.apply(updateCursor(w), body.getCoordinates().lastStatement());
List<Statement> statements = body.getStatements();
if (statements.size() == 1 && statements.get(0) instanceof J.Continue) {
w = w.withBody(body.withStatements(new ArrayList<>()));
}
Comment on lines -51 to +56
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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);
                      }
                  }
                  """
              )
            );
        }
    }

Copy link
Member

Choose a reason for hiding this comment

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

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

}

return w;
Expand All @@ -60,9 +63,12 @@ public J.WhileLoop visitWhileLoop(J.WhileLoop whileLoop, P p) {
public J.DoWhileLoop visitDoWhileLoop(J.DoWhileLoop doWhileLoop, P p) {
J.DoWhileLoop w = super.visitDoWhileLoop(doWhileLoop, p);

if (Boolean.TRUE.equals(emptyBlockStyle.getLiteralWhile()) && isEmptyBlock(w.getBody())) {
if (Boolean.TRUE.equals(emptyBlockStyle.getLiteralWhile()) && w.getBody() instanceof J.Block) {
J.Block body = (J.Block) w.getBody();
w = continueStatement.apply(updateCursor(w), body.getCoordinates().lastStatement());
List<Statement> statements = body.getStatements();
if (statements.size() == 1 && statements.get(0) instanceof J.Continue) {
w = w.withBody(body.withStatements(new ArrayList<>()));
}
}

return w;
Expand Down
4 changes: 2 additions & 2 deletions src/test/java/org/openrewrite/staticanalysis/EmptyBlockTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,8 +223,10 @@ void emptyLoops() {
public class A {
public void foo() {
while(true) {
continue;
}
do {
continue;
} while(true);
}
}
Expand All @@ -233,10 +235,8 @@ public void foo() {
public class A {
public void foo() {
while(true) {
continue;
}
do {
continue;
} while(true);
}
}
Expand Down