From 482d63de744ecdbfea180a1df485477af8d135fa Mon Sep 17 00:00:00 2001 From: Liam Miller-Cushon Date: Thu, 22 Oct 2026 00:45:57 -0700 Subject: [PATCH] Remove redundant semicolons in the cleanup pass. Remove redundant semicolons at top level, in class and enum bodies, and in statements and blocks during the cleanup pass. PiperOrigin-RevId: 1004558693 --- .../googlejavaformat/java/CleanupPass.java | 2 +- .../googlejavaformat/java/CleanupScanner.java | 213 +++++++++++++++++- .../googlejavaformat/java/UsageException.java | 2 +- .../googlejavaformat/java/CleanupsTest.java | 151 +++++++++++++ .../java/FormatterIntegrationTest.java | 10 + 5 files changed, 373 insertions(+), 5 deletions(-) diff --git a/core/src/main/java/com/google/googlejavaformat/java/CleanupPass.java b/core/src/main/java/com/google/googlejavaformat/java/CleanupPass.java index ce390090b..2b585f688 100644 --- a/core/src/main/java/com/google/googlejavaformat/java/CleanupPass.java +++ b/core/src/main/java/com/google/googlejavaformat/java/CleanupPass.java @@ -26,7 +26,7 @@ ImmutableList findReplacements(Cleanups cleanups) throws FormatterE cleanups.context(), cleanups.unit(), cleanups.contents()); } }, - /// AST cleanups: missing control-flow braces. + /// AST cleanups: missing control-flow braces and redundant semicolons. AST_CLEANUPS { @Override ImmutableList findReplacements(Cleanups cleanups) throws FormatterException { diff --git a/core/src/main/java/com/google/googlejavaformat/java/CleanupScanner.java b/core/src/main/java/com/google/googlejavaformat/java/CleanupScanner.java index e381ecca7..2f2e67a01 100644 --- a/core/src/main/java/com/google/googlejavaformat/java/CleanupScanner.java +++ b/core/src/main/java/com/google/googlejavaformat/java/CleanupScanner.java @@ -17,6 +17,10 @@ import static com.google.common.collect.ImmutableList.toImmutableList; import static com.google.googlejavaformat.java.Trees.getEndPosition; import static com.google.googlejavaformat.java.Trees.getStartPosition; +import static com.sun.tools.javac.code.Flags.ENUM; +import static com.sun.tools.javac.code.Flags.GENERATED_MEMBER; +import static java.lang.Math.max; +import static java.lang.Math.min; import static java.util.Comparator.comparingInt; import com.google.common.collect.ImmutableList; @@ -26,16 +30,24 @@ import com.google.common.collect.TreeRangeSet; import com.google.googlejavaformat.java.JavaInput.Tok; import com.sun.source.tree.BlockTree; +import com.sun.source.tree.CaseTree; +import com.sun.source.tree.ClassTree; +import com.sun.source.tree.CompilationUnitTree; import com.sun.source.tree.DoWhileLoopTree; import com.sun.source.tree.EmptyStatementTree; import com.sun.source.tree.EnhancedForLoopTree; import com.sun.source.tree.ForLoopTree; import com.sun.source.tree.IfTree; +import com.sun.source.tree.ImportTree; +import com.sun.source.tree.NewClassTree; import com.sun.source.tree.StatementTree; import com.sun.source.tree.Tree; +import com.sun.source.tree.VariableTree; import com.sun.source.tree.WhileLoopTree; import com.sun.source.util.TreePathScanner; +import com.sun.tools.javac.tree.JCTree; import com.sun.tools.javac.tree.JCTree.JCCompilationUnit; +import com.sun.tools.javac.tree.TreeInfo; import java.util.ArrayList; import java.util.Collection; import java.util.List; @@ -44,10 +56,13 @@ import java.util.TreeMap; import org.jspecify.annotations.Nullable; -/// Scans the compilation unit for non-whitespace cleanup opportunities, inserting missing -/// control-flow braces. +/// Scans the compilation unit for non-whitespace cleanup opportunities: inserting missing +/// control-flow braces and removing redundant semicolons. final class CleanupScanner extends TreePathScanner<@Nullable Void, @Nullable Void> { + // Replace with Flags.IMPLICIT_CLASS once JDK 25 is the minimum supported version + private static final int IMPLICIT_CLASS = 1 << 19; + static ImmutableList scan( JCCompilationUnit unit, String contents, Collection> ranges) throws FormatterException { @@ -55,7 +70,7 @@ static ImmutableList scan( ImmutableList toks = JavaInput.buildToks(contents, ImmutableSet.of()); RangeSet activeDeletions = TreeRangeSet.create(); NavigableMap activeInsertions = new TreeMap<>(); - new CleanupScanner(unit, toks, targetRanges, activeDeletions, activeInsertions) + new CleanupScanner(contents, unit, toks, targetRanges, activeDeletions, activeInsertions) .scan(unit, null); if (activeDeletions.isEmpty() && activeInsertions.isEmpty()) { return ImmutableList.of(); @@ -108,6 +123,7 @@ private static void verifyCommentsPreserved( } } + private final String contents; private final JCCompilationUnit unit; private final ImmutableList toks; private final RangeSet targetRanges; @@ -115,11 +131,13 @@ private static void verifyCommentsPreserved( private final NavigableMap activeInsertions; CleanupScanner( + String contents, JCCompilationUnit unit, ImmutableList toks, RangeSet targetRanges, RangeSet activeDeletions, NavigableMap activeInsertions) { + this.contents = contents; this.unit = unit; this.toks = toks; this.targetRanges = targetRanges; @@ -135,6 +153,154 @@ private void insert(int pos, String text) { activeInsertions.computeIfAbsent(pos, k -> new StringBuilder()).append(text); } + private void deleteRedundantSemicolon(int semiPos) { + int start = semiPos; + while (start > 0 && isHorizontalWhitespace(contents.charAt(start - 1))) { + start--; + } + int lineBegin = start; + while (lineBegin > 0 && !isAtLineStart(contents, lineBegin)) { + lineBegin--; + } + int end = semiPos + 1; + while (end < contents.length() && isHorizontalWhitespace(contents.charAt(end))) { + end++; + } + boolean nothingBeforeOnLine = + start == lineBegin || activeDeletions.encloses(Range.closedOpen(lineBegin, start)); + if (nothingBeforeOnLine && end < contents.length()) { + if (contents.charAt(end) == '\r') { + end++; + if (end < contents.length() && contents.charAt(end) == '\n') { + end++; + } + } else if (contents.charAt(end) == '\n') { + end++; + } + } + activeDeletions.add(Range.closedOpen(start, end)); + } + + private static boolean isHorizontalWhitespace(char c) { + return c == ' ' || c == '\t'; + } + + private static boolean isAtLineStart(String contents, int pos) { + return pos == 0 || contents.charAt(pos - 1) == '\n' || contents.charAt(pos - 1) == '\r'; + } + + @Override + public @Nullable Void visitCompilationUnit(CompilationUnitTree node, @Nullable Void unused) { + checkTopLevelSemicolons(node); + return super.visitCompilationUnit(node, null); + } + + // Top-level semicolons between or after type declarations are represented as EmptyStatementTree + // nodes in CompilationUnitTree.getTypeDecls(), but javac omits top-level semicolons from the AST + // in some cases: + // - In compilation units with no type declarations (e.g. only package and/or import declarations + // followed by extra semicolons), JavacParser drops trailing semicolons at EOF without adding + // them to getTypeDecls(). + // - In compact source files (implicitly declared classes), JavacParser.constructImplicitClass + // strips all JCSkip (EmptyStatementTree) nodes when constructing the synthetic ClassDef. + // Scan token gaps between top-level declarations so all redundant top-level semicolons are + // handled uniformly. + private void checkTopLevelSemicolons(CompilationUnitTree node) { + RangeSet validSpans = TreeRangeSet.create(); + if (node.getPackage() != null) { + addNodeSpan(validSpans, node.getPackage()); + } + if (node.getModule() != null) { + addNodeSpan(validSpans, node.getModule()); + } + for (ImportTree imp : node.getImports()) { + addNodeSpan(validSpans, imp); + } + for (Tree decl : node.getTypeDecls()) { + switch (decl) { + case EmptyStatementTree unused -> {} + case ClassTree classTree + when (TreeInfo.flags((JCTree) classTree) & IMPLICIT_CLASS) == IMPLICIT_CLASS -> { + for (Tree member : classTree.getMembers()) { + if ((TreeInfo.flags((JCTree) member) & GENERATED_MEMBER) == 0) { + addNodeSpan(validSpans, member); + } + } + } + default -> addNodeSpan(validSpans, decl); + } + } + deleteSemicolonsInGaps(0, contents.length(), validSpans); + } + + private void addNodeSpan(RangeSet spans, Tree tree) { + spans.add(Range.closedOpen(getStartPosition(tree), getEndPosition(tree, unit))); + } + + @Override + public @Nullable Void visitClass(ClassTree tree, @Nullable Void unused) { + checkClassSemicolons(tree); + return super.visitClass(tree, null); + } + + private void checkClassSemicolons(ClassTree tree) { + if ((TreeInfo.flags((JCTree) tree) & IMPLICIT_CLASS) == IMPLICIT_CLASS + || tree.getMembers() == null) { + return; + } + int classStart = getStartPosition(tree); + int classEnd = getEndPosition(tree, unit); + RangeSet memberSpans = TreeRangeSet.create(); + // Record the spans of constructor arguments in `new MyClass(arg1, arg2) {...}` and enum + // constants with class bodies `FOO(arg1, arg2) {...}` (where javac sets the anonymous + // ClassTree's start position to the enum constant identifier before the argument list), so + // semicolons inside argument expressions (such as lambdas) do not show up as gaps in the + // class body. + if (getCurrentPath().getParentPath().getLeaf() instanceof NewClassTree newClass) { + for (Tree arg : newClass.getArguments()) { + addNodeSpan(memberSpans, arg); + } + } + int lastEnumConstantEnd = classStart; + int firstNonEnumMemberStart = classEnd; + for (Tree member : tree.getMembers()) { + long flags = TreeInfo.flags((JCTree) member); + if ((flags & GENERATED_MEMBER) != 0) { + continue; + } + int mStart = getStartPosition(member); + int mEnd = getEndPosition(member, unit); + memberSpans.add(Range.closedOpen(mStart, mEnd)); + if (tree.getKind() == Tree.Kind.ENUM) { + if (member instanceof VariableTree && (flags & ENUM) != 0) { + lastEnumConstantEnd = max(lastEnumConstantEnd, mEnd); + } else { + firstNonEnumMemberStart = min(firstNonEnumMemberStart, mStart); + } + } + } + if (tree.getKind() == Tree.Kind.ENUM && !memberSpans.isEmpty()) { + Tok enumSemi = findTokenAtOrAfter(lastEnumConstantEnd, firstNonEnumMemberStart, ";"); + if (enumSemi != null) { + memberSpans.add( + Range.closedOpen(enumSemi.getPosition(), enumSemi.getPosition() + enumSemi.length())); + } + } + deleteSemicolonsInGaps(classStart, classEnd, memberSpans); + } + + private void deleteSemicolonsInGaps(int start, int end, RangeSet validSpans) { + for (Range gap : + validSpans.complement().subRangeSet(Range.closedOpen(start, end)).asRanges()) { + for (Tok semi : semicolonTokensInRange(gap.lowerEndpoint(), gap.upperEndpoint())) { + int pos = semi.getPosition(); + if (inRange(pos, pos + 1)) { + deleteRedundantSemicolon(pos); + } + } + } + } + @Override public @Nullable Void visitIf(IfTree tree, @Nullable Void unused) { super.visitIf(tree, null); @@ -214,6 +380,20 @@ private int closingBraceInsertionPos(StatementTree stmt) { return stmtEnd; } + @Override + public @Nullable Void visitEmptyStatement(EmptyStatementTree tree, @Nullable Void unused) { + // Include CaseTree for empty statements in colon-style switch statement groups + // (e.g. `switch (x) { case FOO: ; case BAR: }`). + Tree parent = getCurrentPath().getParentPath().getLeaf(); + if (parent instanceof BlockTree || parent instanceof CaseTree) { + int pos = getStartPosition(tree); + if (inRange(pos, pos + 1)) { + deleteRedundantSemicolon(pos); + } + } + return null; + } + // Hand-rolled lower-bound binary search to avoid allocating a dummy Tok or wrapper list for // Collections.binarySearch. private int tokIndexAtOrAfter(int pos) { @@ -239,4 +419,31 @@ private int tokIndexAtOrAfter(int pos) { } return null; } + + private @Nullable Tok findTokenAtOrAfter(int startPos, int endPos, String text) { + for (int i = tokIndexAtOrAfter(startPos); i < toks.size(); i++) { + Tok tok = toks.get(i); + if (tok.getPosition() >= endPos) { + break; + } + if (tok.isToken() && tok.getText().equals(text)) { + return tok; + } + } + return null; + } + + private List semicolonTokensInRange(int startPos, int endPos) { + List result = new ArrayList<>(); + for (int i = tokIndexAtOrAfter(startPos); i < toks.size(); i++) { + Tok tok = toks.get(i); + if (tok.getPosition() >= endPos) { + break; + } + if (tok.isToken() && tok.getText().equals(";")) { + result.add(tok); + } + } + return result; + } } diff --git a/core/src/main/java/com/google/googlejavaformat/java/UsageException.java b/core/src/main/java/com/google/googlejavaformat/java/UsageException.java index 89fc5e34b..26738b578 100644 --- a/core/src/main/java/com/google/googlejavaformat/java/UsageException.java +++ b/core/src/main/java/com/google/googlejavaformat/java/UsageException.java @@ -57,7 +57,7 @@ Maximum line length (default is 100). --skip-reordering-modifiers Do not reorder modifiers into the JLS-recommended order. --cleanup[=(true|false)] - Enables non-whitespace cleanups, such as inserting missing braces (default is true). + Enables non-whitespace cleanups, such as inserting missing braces and removing redundant semicolons (default is true). --dry-run, -n Prints the paths of the files whose contents would change if the formatter were run normally. --set-exit-if-changed diff --git a/core/src/test/java/com/google/googlejavaformat/java/CleanupsTest.java b/core/src/test/java/com/google/googlejavaformat/java/CleanupsTest.java index 4f063592b..ab6cbf1c6 100644 --- a/core/src/test/java/com/google/googlejavaformat/java/CleanupsTest.java +++ b/core/src/test/java/com/google/googlejavaformat/java/CleanupsTest.java @@ -383,11 +383,160 @@ void f(boolean a) { assertThat(formatWithCleanups(input)).isEqualTo(expected); } + @Test + public void redundantSemicolons() throws Exception { + String input = + """ + package p; + + import java.util.List;; + + class T { + ; + int x = 1;; + ; + void f(List xs) { + int a = 1;; + ;\s + \t;\t + // line comment + ; int b = 2; + switch (a) { + case 1: + ; + break;; + default: + ; + } + while (xs.isEmpty()); + for (;;); + if (xs.isEmpty()); ; + label: + ; + }; + }; + """; + String expected = + """ + package p; + + import java.util.List; + + class T { + int x = 1; + + void f(List xs) { + int a = 1; + // line comment + int b = 2; + switch (a) { + case 1: + break; + default: + } + while (xs.isEmpty()) {} + for (; ; ) {} + if (xs.isEmpty()) {} + label: + ; + } + } + """; + assertThat(formatWithCleanups(input)).isEqualTo(expected); + assertThat(formatWithCleanups("class T {}\n;")).isEqualTo("class T {}\n"); + assertThat(formatWithCleanups("package p;;\n")).isEqualTo("package p;\n"); + } + + @Test + public void enumSemicolons() throws Exception { + String input = + """ + enum E1 { + A; + } + + enum E2 { + A;; + } + + enum E3 { + A;; + int x;; + } + + enum E4 { + A(() -> { + int x = 1; + }) { + ; + };; + E4(Runnable r) {} + } + + enum E5 { + A; + enum Nested { + B + } + } + + enum E6 { + A; + int x; + enum Nested { + B + } + } + """; + String expected = + """ + enum E1 { + A; + } + + enum E2 { + A; + } + + enum E3 { + A; + int x; + } + + enum E4 { + A(() -> { + int x = 1; + }) {}; + + E4(Runnable r) {} + } + + enum E5 { + A; + + enum Nested { + B + } + } + + enum E6 { + A; + int x; + + enum Nested { + B + } + } + """; + assertThat(formatWithCleanups(input)).isEqualTo(expected); + } + @Test public void aospStyleCleanups() throws Exception { String input = """ class T { + int x;; void f(boolean a) { if (a) doA(); @@ -397,6 +546,8 @@ void f(boolean a) { String expected = """ class T { + int x; + void f(boolean a) { if (a) { doA(); diff --git a/core/src/test/java/com/google/googlejavaformat/java/FormatterIntegrationTest.java b/core/src/test/java/com/google/googlejavaformat/java/FormatterIntegrationTest.java index 0b49ddb1a..ff8a85a86 100644 --- a/core/src/test/java/com/google/googlejavaformat/java/FormatterIntegrationTest.java +++ b/core/src/test/java/com/google/googlejavaformat/java/FormatterIntegrationTest.java @@ -68,6 +68,16 @@ public class FormatterIntegrationTest { ImmutableSet.builder() // Braces around single-statement control flow (b/20569245) .add("B", "B20569245", "C") + // Redundant semicolons (b/22201927) + .add( + "B20128174", + "B21647014", + "B23349153", + "B25811323", + "B28786284", + "E", + "SemicolonInClass", + "i281") .build(); @Parameters(name = "{index}: {0}")