From 5eb4f96dfb78de203303f23b7881221b16a1c96a Mon Sep 17 00:00:00 2001 From: Liam Miller-Cushon Date: Thu, 22 Oct 2026 00:19:14 -0700 Subject: [PATCH] Add a --cleanup option and an AST cleanup pass with brace insertion. Add the `--cleanup[=(true|false)]` command-line flag and `JavaFormatterOptions.Builder.cleanup(boolean)` option (defaulting to true) to gate non-whitespace AST cleanups. Introduce an AST cleanup pass (`CleanupScanner`) that inserts braces around single-statement control flow bodies (`if`, `else`, `for`, `do`, `while`). PiperOrigin-RevId: 1004545653 --- .../googlejavaformat/java/CleanupPass.java | 7 + .../googlejavaformat/java/CleanupScanner.java | 242 +++++++++++++++ .../googlejavaformat/java/Cleanups.java | 5 +- .../java/CommandLineOptions.java | 7 +- .../java/CommandLineOptionsParser.java | 11 + .../googlejavaformat/java/Formatter.java | 6 +- .../java/JavaFormatterOptions.java | 13 +- .../google/googlejavaformat/java/Main.java | 1 + .../googlejavaformat/java/UsageException.java | 2 + .../googlejavaformat/java/CleanupsTest.java | 279 ++++++++++++++++++ .../java/CommandLineOptionsParserTest.java | 30 ++ .../java/FormatterIntegrationTest.java | 55 +++- .../googlejavaformat/java/MainTest.java | 72 +++++ .../java/StringWrapperIntegrationTest.java | 3 +- .../GoogleJavaFormatFormattingService.java | 2 +- 15 files changed, 716 insertions(+), 19 deletions(-) create mode 100644 core/src/main/java/com/google/googlejavaformat/java/CleanupScanner.java 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 69d022a56..ce390090b 100644 --- a/core/src/main/java/com/google/googlejavaformat/java/CleanupPass.java +++ b/core/src/main/java/com/google/googlejavaformat/java/CleanupPass.java @@ -26,6 +26,13 @@ ImmutableList findReplacements(Cleanups cleanups) throws FormatterE cleanups.context(), cleanups.unit(), cleanups.contents()); } }, + /// AST cleanups: missing control-flow braces. + AST_CLEANUPS { + @Override + ImmutableList findReplacements(Cleanups cleanups) throws FormatterException { + return CleanupScanner.scan(cleanups.unit(), cleanups.contents(), cleanups.ranges()); + } + }, /// Reorders modifiers into JLS order. REORDER_MODIFIERS { @Override diff --git a/core/src/main/java/com/google/googlejavaformat/java/CleanupScanner.java b/core/src/main/java/com/google/googlejavaformat/java/CleanupScanner.java new file mode 100644 index 000000000..e381ecca7 --- /dev/null +++ b/core/src/main/java/com/google/googlejavaformat/java/CleanupScanner.java @@ -0,0 +1,242 @@ +/* + * Copyright 2026 Google Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except + * in compliance with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software distributed under the License + * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express + * or implied. See the License for the specific language governing permissions and limitations under + * the License. + */ + +package com.google.googlejavaformat.java; + +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 java.util.Comparator.comparingInt; + +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableSet; +import com.google.common.collect.Range; +import com.google.common.collect.RangeSet; +import com.google.common.collect.TreeRangeSet; +import com.google.googlejavaformat.java.JavaInput.Tok; +import com.sun.source.tree.BlockTree; +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.StatementTree; +import com.sun.source.tree.Tree; +import com.sun.source.tree.WhileLoopTree; +import com.sun.source.util.TreePathScanner; +import com.sun.tools.javac.tree.JCTree.JCCompilationUnit; +import java.util.ArrayList; +import java.util.Collection; +import java.util.List; +import java.util.Map; +import java.util.NavigableMap; +import java.util.TreeMap; +import org.jspecify.annotations.Nullable; + +/// Scans the compilation unit for non-whitespace cleanup opportunities, inserting missing +/// control-flow braces. +final class CleanupScanner extends TreePathScanner<@Nullable Void, @Nullable Void> { + + static ImmutableList scan( + JCCompilationUnit unit, String contents, Collection> ranges) + throws FormatterException { + RangeSet targetRanges = TreeRangeSet.create(ranges); + ImmutableList toks = JavaInput.buildToks(contents, ImmutableSet.of()); + RangeSet activeDeletions = TreeRangeSet.create(); + NavigableMap activeInsertions = new TreeMap<>(); + new CleanupScanner(unit, toks, targetRanges, activeDeletions, activeInsertions) + .scan(unit, null); + if (activeDeletions.isEmpty() && activeInsertions.isEmpty()) { + return ImmutableList.of(); + } + List replacements = new ArrayList<>(); + for (Range del : activeDeletions.asRanges()) { + int dStart = del.lowerEndpoint(); + int dEnd = del.upperEndpoint(); + var subMap = + activeInsertions.subMap(dStart, /* fromInclusive= */ true, dEnd, /* toInclusive= */ true); + String rep = String.join("", subMap.values()); + subMap.clear(); + replacements.add(Replacement.create(dStart, dEnd, rep)); + } + for (Map.Entry entry : activeInsertions.entrySet()) { + int pos = entry.getKey(); + replacements.add(Replacement.create(pos, pos, entry.getValue().toString())); + } + replacements.sort(comparingInt(r -> r.replaceRange().lowerEndpoint())); + verifyCommentsPreserved(toks, contents, replacements); + return ImmutableList.copyOf(replacements); + } + + private static void verifyCommentsPreserved( + ImmutableList oldToks, String contents, List replacements) + throws FormatterException { + StringBuilder sb = new StringBuilder(contents.length()); + int cursor = 0; + for (Replacement replacement : replacements) { + Range range = replacement.replaceRange(); + sb.append(contents, cursor, range.lowerEndpoint()); + sb.append(replacement.replacementString()); + cursor = range.upperEndpoint(); + } + sb.append(contents, cursor, contents.length()); + String newContents = sb.toString(); + + ImmutableList expectedComments = + oldToks.stream() + .filter(Tok::isComment) + .map(Tok::getOriginalText) + .collect(toImmutableList()); + ImmutableList actualComments = + JavaInput.buildToks(newContents, ImmutableSet.of()).stream() + .filter(Tok::isComment) + .map(Tok::getOriginalText) + .collect(toImmutableList()); + if (!expectedComments.equals(actualComments)) { + throw new FormatterException("error: cleanups modified comments"); + } + } + + private final JCCompilationUnit unit; + private final ImmutableList toks; + private final RangeSet targetRanges; + private final RangeSet activeDeletions; + private final NavigableMap activeInsertions; + + CleanupScanner( + JCCompilationUnit unit, + ImmutableList toks, + RangeSet targetRanges, + RangeSet activeDeletions, + NavigableMap activeInsertions) { + this.unit = unit; + this.toks = toks; + this.targetRanges = targetRanges; + this.activeDeletions = activeDeletions; + this.activeInsertions = activeInsertions; + } + + private boolean inRange(int startPos, int endPos) { + return targetRanges.intersects(Range.closedOpen(startPos, endPos)); + } + + private void insert(int pos, String text) { + activeInsertions.computeIfAbsent(pos, k -> new StringBuilder()).append(text); + } + + @Override + public @Nullable Void visitIf(IfTree tree, @Nullable Void unused) { + super.visitIf(tree, null); + checkBraces(tree, tree.getThenStatement()); + if (!(tree.getElseStatement() instanceof IfTree)) { + checkBraces(tree, tree.getElseStatement()); + } + return null; + } + + @Override + public @Nullable Void visitForLoop(ForLoopTree tree, @Nullable Void unused) { + super.visitForLoop(tree, null); + checkBraces(tree, tree.getStatement()); + return null; + } + + @Override + public @Nullable Void visitEnhancedForLoop(EnhancedForLoopTree tree, @Nullable Void unused) { + super.visitEnhancedForLoop(tree, null); + checkBraces(tree, tree.getStatement()); + return null; + } + + @Override + public @Nullable Void visitWhileLoop(WhileLoopTree tree, @Nullable Void unused) { + super.visitWhileLoop(tree, null); + checkBraces(tree, tree.getStatement()); + return null; + } + + @Override + public @Nullable Void visitDoWhileLoop(DoWhileLoopTree tree, @Nullable Void unused) { + super.visitDoWhileLoop(tree, null); + checkBraces(tree, tree.getStatement()); + return null; + } + + private void checkBraces(Tree owner, @Nullable StatementTree body) { + if (body == null || body instanceof BlockTree) { + return; + } + int rangeEnd = + owner instanceof DoWhileLoopTree ? getEndPosition(owner, unit) : getEndPosition(body, unit); + if (!inRange(getStartPosition(owner), rangeEnd)) { + return; + } + Tok prev = prevToken(getStartPosition(body)); + if (prev == null) { + return; + } + insert(prev.getPosition() + prev.length(), "{"); + insert(closingBraceInsertionPos(body), "}"); + if (body instanceof EmptyStatementTree) { + activeDeletions.add(Range.closedOpen(getStartPosition(body), getEndPosition(body, unit))); + } + } + + private int closingBraceInsertionPos(StatementTree stmt) { + int stmtEnd = getEndPosition(stmt, unit); + if (stmt instanceof EmptyStatementTree) { + return stmtEnd; + } + boolean sawComment = false; + for (int i = tokIndexAtOrAfter(stmtEnd); i < toks.size(); i++) { + Tok tok = toks.get(i); + if (tok.isToken()) { + return stmtEnd; + } + if (tok.isNewline()) { + return sawComment ? tok.getPosition() + tok.length() : stmtEnd; + } + if (tok.isComment()) { + sawComment = true; + } + } + return stmtEnd; + } + + // Hand-rolled lower-bound binary search to avoid allocating a dummy Tok or wrapper list for + // Collections.binarySearch. + private int tokIndexAtOrAfter(int pos) { + int lo = 0; + int hi = toks.size(); + while (lo < hi) { + int mid = (lo + hi) >>> 1; + if (toks.get(mid).getPosition() < pos) { + lo = mid + 1; + } else { + hi = mid; + } + } + return lo; + } + + private @Nullable Tok prevToken(int pos) { + for (int i = tokIndexAtOrAfter(pos) - 1; i >= 0; i--) { + Tok tok = toks.get(i); + if (tok.isToken() && !tok.getText().isEmpty()) { + return tok; + } + } + return null; + } +} diff --git a/core/src/main/java/com/google/googlejavaformat/java/Cleanups.java b/core/src/main/java/com/google/googlejavaformat/java/Cleanups.java index e6a420a6e..10e8d4ace 100644 --- a/core/src/main/java/com/google/googlejavaformat/java/Cleanups.java +++ b/core/src/main/java/com/google/googlejavaformat/java/Cleanups.java @@ -104,11 +104,14 @@ private record Parsed(Context context, JCCompilationUnit unit) {} /// Returns the enabled passes for formatting or fixing imports. static ImmutableSet enabledPasses( - boolean removeUnusedImports, boolean reorderModifiers, boolean sortImports) { + boolean cleanup, boolean removeUnusedImports, boolean reorderModifiers, boolean sortImports) { EnumSet passes = EnumSet.noneOf(CleanupPass.class); if (removeUnusedImports) { passes.add(CleanupPass.REMOVE_UNUSED_IMPORTS); } + if (cleanup) { + passes.add(CleanupPass.AST_CLEANUPS); + } if (reorderModifiers) { passes.add(CleanupPass.REORDER_MODIFIERS); } diff --git a/core/src/main/java/com/google/googlejavaformat/java/CommandLineOptions.java b/core/src/main/java/com/google/googlejavaformat/java/CommandLineOptions.java index 54330f65b..49b10fdc0 100644 --- a/core/src/main/java/com/google/googlejavaformat/java/CommandLineOptions.java +++ b/core/src/main/java/com/google/googlejavaformat/java/CommandLineOptions.java @@ -39,6 +39,7 @@ /// @param setExitIfChanged Return exit code 1 if there are any formatting changes. /// @param assumeFilename Return the name to use for diagnostics when formatting standard input. /// @param reorderModifiers Reorder modifiers into the JLS-recommended order. +/// @param cleanup Enables non-whitespace cleanups. record CommandLineOptions( ImmutableList files, boolean inPlace, @@ -57,7 +58,8 @@ record CommandLineOptions( Optional assumeFilename, boolean reflowLongStrings, boolean formatJavadoc, - boolean reorderModifiers) { + boolean reorderModifiers, + boolean cleanup) { boolean aosp() { return style().isAosp(); @@ -80,6 +82,7 @@ static Builder builder() { .reflowLongStrings(true) .formatJavadoc(true) .reorderModifiers(true) + .cleanup(true) .version(false) .help(false) .stdin(false) @@ -140,6 +143,8 @@ default Builder addLength(Integer length) { Builder reorderModifiers(boolean reorderModifiers); + Builder cleanup(boolean cleanup); + CommandLineOptions build(); } } diff --git a/core/src/main/java/com/google/googlejavaformat/java/CommandLineOptionsParser.java b/core/src/main/java/com/google/googlejavaformat/java/CommandLineOptionsParser.java index 73a6366ee..308107e21 100644 --- a/core/src/main/java/com/google/googlejavaformat/java/CommandLineOptionsParser.java +++ b/core/src/main/java/com/google/googlejavaformat/java/CommandLineOptionsParser.java @@ -92,6 +92,17 @@ static CommandLineOptions parse(Iterable options) { case "--skip-reflowing-long-strings" -> optionsBuilder.reflowLongStrings(false); case "--skip-javadoc-formatting" -> optionsBuilder.formatJavadoc(false); case "--skip-reordering-modifiers" -> optionsBuilder.reorderModifiers(false); + case "--cleanup" -> { + if (value == null) { + optionsBuilder.cleanup(true); + } else { + switch (value) { + case "true" -> optionsBuilder.cleanup(true); + case "false" -> optionsBuilder.cleanup(false); + default -> throw new IllegalArgumentException("unexpected --cleanup value: " + value); + } + } + } case "--max-line-length" -> { int length = parseInteger(it, flag, value); checkArgument( diff --git a/core/src/main/java/com/google/googlejavaformat/java/Formatter.java b/core/src/main/java/com/google/googlejavaformat/java/Formatter.java index 4f6f6588f..3849058ae 100644 --- a/core/src/main/java/com/google/googlejavaformat/java/Formatter.java +++ b/core/src/main/java/com/google/googlejavaformat/java/Formatter.java @@ -185,7 +185,8 @@ String formatSource( boolean reflowLongStrings) throws FormatterException { ImmutableSet passes = - Cleanups.enabledPasses(removeUnusedImports, options.reorderModifiers(), sortImports); + Cleanups.enabledPasses( + options.cleanup(), removeUnusedImports, options.reorderModifiers(), sortImports); Cleanups.Result cleaned = Cleanups.apply(input, characterRanges, options.style(), passes); String formatted = JavaOutput.applyReplacements( @@ -231,7 +232,8 @@ public static String fixImports( boolean sortImports) throws FormatterException { ImmutableSet passes = - Cleanups.enabledPasses(removeUnusedImports, /* reorderModifiers= */ false, sortImports); + Cleanups.enabledPasses( + /* cleanup= */ false, removeUnusedImports, /* reorderModifiers= */ false, sortImports); Cleanups.Result result = Cleanups.apply(input, ImmutableList.of(Range.closedOpen(0, input.length())), style, passes); return result.contents(); diff --git a/core/src/main/java/com/google/googlejavaformat/java/JavaFormatterOptions.java b/core/src/main/java/com/google/googlejavaformat/java/JavaFormatterOptions.java index dcbfb28f6..25e1fc644 100644 --- a/core/src/main/java/com/google/googlejavaformat/java/JavaFormatterOptions.java +++ b/core/src/main/java/com/google/googlejavaformat/java/JavaFormatterOptions.java @@ -24,15 +24,18 @@ /// Options for a google-java-format invocation. /// -/// Like gofmt, the google-java-format CLI exposes *no* configuration options (aside from `--aosp`). +/// The google-java-format CLI exposes very few configuration options. /// /// The goal of google-java-format is to provide consistent formatting, and to free developers from /// arguments over style choices. It is an explicit non-goal to support developers' individual /// preferences, and in fact it would work directly against our primary goals. /// +/// @param cleanup Whether to enable non-whitespace cleanups (applies to formatSource* and the CLI +/// only). /// @param style Returns the code style. @Immutable -public record JavaFormatterOptions(boolean formatJavadoc, boolean reorderModifiers, Style style) { +public record JavaFormatterOptions( + boolean formatJavadoc, boolean reorderModifiers, boolean cleanup, Style style) { public JavaFormatterOptions { requireNonNull(style, "style"); } @@ -229,7 +232,8 @@ public static Builder builder() { return new AutoBuilder_JavaFormatterOptions_Builder() .style(Style.GOOGLE) .formatJavadoc(true) - .reorderModifiers(true); + .reorderModifiers(true) + .cleanup(true); } /// A builder for [JavaFormatterOptions]. @@ -242,6 +246,9 @@ public abstract static class Builder { public abstract Builder reorderModifiers(boolean reorderModifiers); + /// Sets whether to enable non-whitespace cleanups (applies to formatSource* and the CLI only). + public abstract Builder cleanup(boolean cleanup); + public abstract JavaFormatterOptions build(); } } diff --git a/core/src/main/java/com/google/googlejavaformat/java/Main.java b/core/src/main/java/com/google/googlejavaformat/java/Main.java index dce08274f..72f44d164 100644 --- a/core/src/main/java/com/google/googlejavaformat/java/Main.java +++ b/core/src/main/java/com/google/googlejavaformat/java/Main.java @@ -112,6 +112,7 @@ public int format(String... args) throws UsageException { .style(parameters.style()) .formatJavadoc(parameters.formatJavadoc()) .reorderModifiers(parameters.reorderModifiers()) + .cleanup(parameters.cleanup()) .build(); if (parameters.stdin()) { 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 354f1fcd3..89fc5e34b 100644 --- a/core/src/main/java/com/google/googlejavaformat/java/UsageException.java +++ b/core/src/main/java/com/google/googlejavaformat/java/UsageException.java @@ -56,6 +56,8 @@ Maximum line length (default is 100). Do not reformat javadoc. --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). --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 8a6f4fddf..4f063592b 100644 --- a/core/src/test/java/com/google/googlejavaformat/java/CleanupsTest.java +++ b/core/src/test/java/com/google/googlejavaformat/java/CleanupsTest.java @@ -15,16 +15,53 @@ package com.google.googlejavaformat.java; import static com.google.common.truth.Truth.assertThat; +import static java.nio.charset.StandardCharsets.UTF_8; import static org.junit.Assert.assertThrows; import com.google.common.collect.ImmutableList; import com.google.common.collect.Range; import com.google.googlejavaformat.java.JavaFormatterOptions.Style; +import java.io.BufferedWriter; +import java.io.ByteArrayInputStream; +import java.io.InputStream; +import java.io.OutputStreamWriter; +import java.io.PrintWriter; +import java.io.StringWriter; +import java.util.Arrays; import org.junit.Test; /// Tests for [Cleanups]. public class CleanupsTest { + private static String formatWithCleanups(String input) throws Exception { + Formatter formatter = new Formatter(JavaFormatterOptions.builder().cleanup(true).build()); + String output = formatter.formatSourceAndFixImports(input); + assertThat(formatter.formatSourceAndFixImports(output)).isEqualTo(output); + return output; + } + + private static String formatWithCleanupsAosp(String input) throws Exception { + Formatter formatter = + new Formatter(JavaFormatterOptions.builder().style(Style.AOSP).cleanup(true).build()); + String output = formatter.formatSourceAndFixImports(input); + assertThat(formatter.formatSourceAndFixImports(output)).isEqualTo(output); + return output; + } + + private static String runCli(String input, String... args) throws Exception { + InputStream in = new ByteArrayInputStream(input.getBytes(UTF_8)); + StringWriter out = new StringWriter(); + Main main = + new Main( + new PrintWriter(out, true), + new PrintWriter(new BufferedWriter(new OutputStreamWriter(System.err, UTF_8)), true), + in); + String[] allArgs = Arrays.copyOf(args, args.length + 1); + allArgs[args.length] = "-"; + assertThat(main.format(allArgs)).isEqualTo(0); + return out.toString(); + } + @Test public void formatSource_rangePastEndOfInput_throwsFormatterException() { String input = @@ -197,6 +234,214 @@ class Test { assertThat(applied).contains("final static public int X = 1;"); } + @Test + public void cleanupFlagAndOptions() throws Exception { + String input = + """ + class T { + void f(boolean cond) { + if (cond) doA(); + } + } + """; + String uncleaned = + """ + class T { + void f(boolean cond) { + if (cond) doA(); + } + } + """; + String cleaned = + """ + class T { + void f(boolean cond) { + if (cond) { + doA(); + } + } + } + """; + assertThat(new Formatter().formatSourceAndFixImports(input)).isEqualTo(cleaned); + assertThat(runCli(input)).isEqualTo(cleaned); + assertThat(runCli(input, "--cleanup")).isEqualTo(cleaned); + assertThat(runCli(input, "--cleanup=true")).isEqualTo(cleaned); + assertThat( + new Formatter(JavaFormatterOptions.builder().cleanup(false).build()) + .formatSourceAndFixImports(input)) + .isEqualTo(uncleaned); + assertThat(runCli(input, "--cleanup=false")).isEqualTo(uncleaned); + } + + @Test + public void controlFlowBraces() throws Exception { + String input = + """ + class T { + void f(boolean a, boolean b, Iterable xs) { + if (a) + doA(); + else if (b) + doB(); + else + doC(); + for (int i = 0; i < 10; i++) + doA(); + for (String x : xs) + doA(); + while (a) + doA(); + while (a); + do + doA(); + while (a); + } + } + """; + String expected = + """ + class T { + void f(boolean a, boolean b, Iterable xs) { + if (a) { + doA(); + } else if (b) { + doB(); + } else { + doC(); + } + for (int i = 0; i < 10; i++) { + doA(); + } + for (String x : xs) { + doA(); + } + while (a) { + doA(); + } + while (a) {} + do { + doA(); + } while (a); + } + } + """; + assertThat(formatWithCleanups(input)).isEqualTo(expected); + } + + @Test + public void nestedIfElseBraces() throws Exception { + String input = + """ + class T { + void f(boolean a, boolean b) { + if (a) + if (b) + doA(); + else + doB(); + } + } + """; + String expected = + """ + class T { + void f(boolean a, boolean b) { + if (a) { + if (b) { + doA(); + } else { + doB(); + } + } + } + } + """; + assertThat(formatWithCleanups(input)).isEqualTo(expected); + } + + @Test + public void controlFlowBracesWithComments() throws Exception { + String input = + """ + class T { + void f(boolean a) { + if (a) // trailing comment + doA(); // body comment + } + } + """; + String expected = + """ + class T { + void f(boolean a) { + if (a) { // trailing comment + doA(); // body comment + } + } + } + """; + assertThat(formatWithCleanups(input)).isEqualTo(expected); + } + + @Test + public void aospStyleCleanups() throws Exception { + String input = + """ + class T { + void f(boolean a) { + if (a) + doA(); + } + } + """; + String expected = + """ + class T { + void f(boolean a) { + if (a) { + doA(); + } + } + } + """; + assertThat(formatWithCleanupsAosp(input)).isEqualTo(expected); + } + + @Test + public void braceInsertionWhenRangeTouchesDeepInsideBracelessIf() throws Exception { + String input = + """ + class T { + void f(boolean a, boolean b) { + if (a) + if (b) + doSomething(); + } + } + """; + int offset = input.indexOf("doSomething();"); + String result = + runCli( + input, + "--offset", + Integer.toString(offset), + "--length", + Integer.toString("doSomething();".length())); + String expected = + """ + class T { + void f(boolean a, boolean b) { + if (a) { + if (b) { + doSomething(); + } + } + } + } + """; + assertThat(result).isEqualTo(expected); + } + @Test public void parseReuseWhenNoEdits() throws Exception { String input = @@ -211,6 +456,7 @@ class Test { ImmutableList.of(Range.closedOpen(0, input.length())), Style.GOOGLE, Cleanups.enabledPasses( + /* cleanup= */ false, /* removeUnusedImports= */ true, /* reorderModifiers= */ false, /* sortImports= */ false)); @@ -232,6 +478,7 @@ class Test { ImmutableList.of(Range.closedOpen(0, input.length())), Style.GOOGLE, Cleanups.enabledPasses( + /* cleanup= */ false, /* removeUnusedImports= */ true, /* reorderModifiers= */ true, /* sortImports= */ false)); @@ -260,6 +507,7 @@ class Test { ImmutableList.of(Range.closedOpen(start, end)), Style.GOOGLE, Cleanups.enabledPasses( + /* cleanup= */ false, /* removeUnusedImports= */ true, /* reorderModifiers= */ true, /* sortImports= */ true)); @@ -270,4 +518,35 @@ class Test { int expectedEnd = result.contents().indexOf(";", expectedStart) + 1; assertThat(result.characterRanges()).contains(Range.closedOpen(expectedStart, expectedEnd)); } + + @Test + public void parseReusedWhenLaterPassParsesAfterEarlierEdits() throws Exception { + String input = + """ + package test; + + import java.util.ArrayList; + + class Test { + void f(boolean a) { + if (a) { + f(false); + } + } + } + """; + Cleanups.Result result = + Cleanups.apply( + input, + ImmutableList.of(Range.closedOpen(0, input.length())), + Style.GOOGLE, + Cleanups.enabledPasses( + /* cleanup= */ true, + /* removeUnusedImports= */ true, + /* reorderModifiers= */ false, + /* sortImports= */ false)); + assertThat(result.unit()).isNotNull(); + assertThat(result.contents()).doesNotContain("ArrayList"); + assertThat(result.contents()).contains("if (a) {"); + } } diff --git a/core/src/test/java/com/google/googlejavaformat/java/CommandLineOptionsParserTest.java b/core/src/test/java/com/google/googlejavaformat/java/CommandLineOptionsParserTest.java index 51a4d7fac..1e8887c89 100644 --- a/core/src/test/java/com/google/googlejavaformat/java/CommandLineOptionsParserTest.java +++ b/core/src/test/java/com/google/googlejavaformat/java/CommandLineOptionsParserTest.java @@ -55,6 +55,7 @@ public void defaults() { assertThat(options.reflowLongStrings()).isTrue(); assertThat(options.formatJavadoc()).isTrue(); assertThat(options.reorderModifiers()).isTrue(); + assertThat(options.cleanup()).isTrue(); assertThat(options.maxLineLength()).isEqualTo(100); } @@ -237,6 +238,35 @@ public void skipReorderingModifiers() { .isFalse(); } + @Test + public void cleanup() { + assertThat(CommandLineOptionsParser.parse(Arrays.asList("--cleanup")).cleanup()).isTrue(); + assertThat(CommandLineOptionsParser.parse(Arrays.asList("--cleanup=true")).cleanup()).isTrue(); + assertThat(CommandLineOptionsParser.parse(Arrays.asList("--cleanup=false")).cleanup()) + .isFalse(); + } + + @Test + public void cleanupAndSkipFlagsOrderIndependent() { + CommandLineOptions opt1 = + CommandLineOptionsParser.parse(Arrays.asList("--skip-reordering-modifiers", "--cleanup")); + CommandLineOptions opt2 = + CommandLineOptionsParser.parse(Arrays.asList("--cleanup", "--skip-reordering-modifiers")); + assertThat(opt1.reorderModifiers()).isFalse(); + assertThat(opt1.cleanup()).isTrue(); + assertThat(opt2.reorderModifiers()).isFalse(); + assertThat(opt2.cleanup()).isTrue(); + + CommandLineOptions opt3 = + CommandLineOptionsParser.parse(Arrays.asList("--skip-javadoc-formatting", "--cleanup")); + CommandLineOptions opt4 = + CommandLineOptionsParser.parse(Arrays.asList("--cleanup", "--skip-javadoc-formatting")); + assertThat(opt3.formatJavadoc()).isFalse(); + assertThat(opt3.cleanup()).isTrue(); + assertThat(opt4.formatJavadoc()).isFalse(); + assertThat(opt4.cleanup()).isTrue(); + } + @Test public void maxLineLength() { assertThat( 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 bf09a17b1..0b49ddb1a 100644 --- a/core/src/test/java/com/google/googlejavaformat/java/FormatterIntegrationTest.java +++ b/core/src/test/java/com/google/googlejavaformat/java/FormatterIntegrationTest.java @@ -19,10 +19,12 @@ import static com.google.common.io.Files.getNameWithoutExtension; import static java.nio.charset.StandardCharsets.UTF_8; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotEquals; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; import com.google.common.collect.ImmutableMultimap; +import com.google.common.collect.ImmutableSet; import com.google.common.io.CharStreams; import com.google.common.reflect.ClassPath; import com.google.common.reflect.ClassPath.ResourceInfo; @@ -62,6 +64,12 @@ public class FormatterIntegrationTest { .putAll(25, "ModuleImport", "InstanceMain") .build(); + private static final ImmutableSet GOLDENS_CHANGED_BY_CLEANUP = + ImmutableSet.builder() + // Braces around single-statement control flow (b/20569245) + .add("B", "B20569245", "C") + .build(); + @Parameters(name = "{index}: {0}") public static Iterable data() throws IOException { Path testDataPath = Paths.get("com/google/googlejavaformat/java/testdata"); @@ -116,25 +124,52 @@ public FormatterIntegrationTest(String name, String input, String expected) { this.separator = Newlines.getLineEnding(expected); } + private static final Formatter FORMATTER = + new Formatter(JavaFormatterOptions.builder().cleanup(false).build()); + @Test public void format() { try { - Formatter formatter = new Formatter(); - String output = formatter.formatSource(input); - output = StringWrapper.wrap(output, formatter); + String output = FORMATTER.formatSource(input); + output = StringWrapper.wrap(output, FORMATTER); assertEquals("bad output for " + name, expected, output); } catch (FormatterException e) { fail(String.format("Formatter crashed on %s: %s", name, e.getMessage())); } } + @Test + public void cleanupOutputIsFixedPoint() { + try { + Formatter defaultFormatter = new Formatter(); + String output = defaultFormatter.formatSource(expected); + if (GOLDENS_CHANGED_BY_CLEANUP.contains(name)) { + assertNotEquals( + "Golden " + + name + + " is expected to change under cleanup=true, but remained identical. " + + "Remove it from GOLDENS_CHANGED_BY_CLEANUP.", + expected, + output); + String cleanedFixedPoint = defaultFormatter.formatSource(output); + assertEquals( + "Cleaned output for " + name + " should be a fixed point under cleanup", + output, + cleanedFixedPoint); + } else { + assertEquals("fixed point under cleanup for " + name, expected, output); + } + } catch (FormatterException e) { + fail(String.format("Formatter crashed on %s: %s", name, e.getMessage())); + } + } + @Test public void idempotent() { try { - Formatter formatter = new Formatter(); - String formatted = formatter.formatSource(input); - formatted = StringWrapper.wrap(formatted, formatter); - String reformatted = formatter.formatSource(formatted); + String formatted = FORMATTER.formatSource(input); + formatted = StringWrapper.wrap(formatted, FORMATTER); + String reformatted = FORMATTER.formatSource(formatted); assertEquals("bad output for " + name, formatted, reformatted); } catch (FormatterException e) { fail(String.format("Formatter crashed on %s: %s", name, e.getMessage())); @@ -145,7 +180,7 @@ public void idempotent() { public void idempotentLF() { try { String mangled = expected.replace(separator, "\n"); - String output = new Formatter().formatSource(mangled); + String output = FORMATTER.formatSource(mangled); assertEquals("bad output for " + name, mangled, output); } catch (FormatterException e) { fail(String.format("Formatter crashed on %s: %s", name, e.getMessage())); @@ -156,7 +191,7 @@ public void idempotentLF() { public void idempotentCR() throws IOException { try { String mangled = expected.replace(separator, "\r"); - String output = new Formatter().formatSource(mangled); + String output = FORMATTER.formatSource(mangled); assertEquals("bad output for " + name, mangled, output); } catch (FormatterException e) { fail(String.format("Formatter crashed on %s: %s", name, e.getMessage())); @@ -167,7 +202,7 @@ public void idempotentCR() throws IOException { public void idempotentCRLF() { try { String mangled = expected.replace(separator, "\r\n"); - String output = new Formatter().formatSource(mangled); + String output = FORMATTER.formatSource(mangled); assertEquals("bad output for " + name, mangled, output); } catch (FormatterException e) { fail(String.format("Formatter crashed on %s: %s", name, e.getMessage())); diff --git a/core/src/test/java/com/google/googlejavaformat/java/MainTest.java b/core/src/test/java/com/google/googlejavaformat/java/MainTest.java index c87277175..7a717af1a 100644 --- a/core/src/test/java/com/google/googlejavaformat/java/MainTest.java +++ b/core/src/test/java/com/google/googlejavaformat/java/MainTest.java @@ -657,6 +657,78 @@ static public void main(String... args) {} assertThat(out.toString()).isEqualTo(input); } + @Test + public void cleanupsOptionTest() throws Exception { + String input = + """ + class Test { + void f(boolean a) { + if (a) f(false); + } + } + """; + String cleaned = + """ + class Test { + void f(boolean a) { + if (a) { + f(false); + } + } + } + """; + assertThat(new Formatter(JavaFormatterOptions.builder().build()).formatSource(input)) + .isEqualTo(cleaned); + assertThat( + new Formatter(JavaFormatterOptions.builder().cleanup(false).build()) + .formatSource(input)) + .isEqualTo(input); + } + + @Test + public void cleanupFlag() throws Exception { + String input = + """ + class Test { + void f(boolean a) { + if (a) f(false); + } + } + """; + String cleaned = + """ + class Test { + void f(boolean a) { + if (a) { + f(false); + } + } + } + """; + { + InputStream in = new ByteArrayInputStream(input.getBytes(UTF_8)); + StringWriter out = new StringWriter(); + Main main = + new Main( + new PrintWriter(out, true), + new PrintWriter(new BufferedWriter(new OutputStreamWriter(System.err, UTF_8)), true), + in); + assertThat(main.format("-")).isEqualTo(0); + assertThat(out.toString()).isEqualTo(cleaned); + } + { + InputStream in = new ByteArrayInputStream(input.getBytes(UTF_8)); + StringWriter out = new StringWriter(); + Main main = + new Main( + new PrintWriter(out, true), + new PrintWriter(new BufferedWriter(new OutputStreamWriter(System.err, UTF_8)), true), + in); + assertThat(main.format("--cleanup=false", "-")).isEqualTo(0); + assertThat(out.toString()).isEqualTo(input); + } + } + @Test public void syntaxError() throws Exception { Path path = testFolder.newFile("Test.java").toPath(); diff --git a/core/src/test/java/com/google/googlejavaformat/java/StringWrapperIntegrationTest.java b/core/src/test/java/com/google/googlejavaformat/java/StringWrapperIntegrationTest.java index 4ee700ac8..6e5dedd38 100644 --- a/core/src/test/java/com/google/googlejavaformat/java/StringWrapperIntegrationTest.java +++ b/core/src/test/java/com/google/googlejavaformat/java/StringWrapperIntegrationTest.java @@ -426,7 +426,8 @@ class T { .collect(toImmutableList()); } - private final Formatter formatter = new Formatter(); + private final Formatter formatter = + new Formatter(JavaFormatterOptions.builder().cleanup(false).build()); private final String input; private final String output; diff --git a/idea_plugin/src/main/java/com/google/googlejavaformat/intellij/GoogleJavaFormatFormattingService.java b/idea_plugin/src/main/java/com/google/googlejavaformat/intellij/GoogleJavaFormatFormattingService.java index 9d2d7a595..442cfa079 100644 --- a/idea_plugin/src/main/java/com/google/googlejavaformat/intellij/GoogleJavaFormatFormattingService.java +++ b/idea_plugin/src/main/java/com/google/googlejavaformat/intellij/GoogleJavaFormatFormattingService.java @@ -67,7 +67,7 @@ protected String getName() { private static Formatter createFormatter(Style style, boolean canChangeWhiteSpaceOnly) { JavaFormatterOptions.Builder optBuilder = JavaFormatterOptions.builder().style(style); if (canChangeWhiteSpaceOnly) { - optBuilder.formatJavadoc(false).reorderModifiers(false); + optBuilder.formatJavadoc(false).reorderModifiers(false).cleanup(false); } return new Formatter(optBuilder.build()); }