From ee81376aaa0f68c0d1e402e4a6c67626be84eb85 Mon Sep 17 00:00:00 2001 From: Liam Miller-Cushon Date: Mon, 19 Oct 2026 07:08:47 -0700 Subject: [PATCH] Return replacements from RemoveUnusedImports, ModifierOrderer, and ImportOrderer. Update `RemoveUnusedImports`, `ModifierOrderer`, and `ImportOrderer` to return replacements via package-private methods, and implement their existing public APIs on top of them. Move character range validation in `JavaInput` into reusable `validateRange` and `validateRanges` methods. No behavior change. PiperOrigin-RevId: 1002314357 --- .../googlejavaformat/java/ImportOrderer.java | 81 +++++++++++-------- .../googlejavaformat/java/JavaInput.java | 49 +++++++---- .../java/ModifierOrderer.java | 44 +++++----- .../java/RemoveUnusedImports.java | 76 +++++------------ .../java/ImportOrdererTest.java | 29 +++++++ 5 files changed, 155 insertions(+), 124 deletions(-) diff --git a/core/src/main/java/com/google/googlejavaformat/java/ImportOrderer.java b/core/src/main/java/com/google/googlejavaformat/java/ImportOrderer.java index 448ec80b9..52525ed8a 100644 --- a/core/src/main/java/com/google/googlejavaformat/java/ImportOrderer.java +++ b/core/src/main/java/com/google/googlejavaformat/java/ImportOrderer.java @@ -13,7 +13,6 @@ */ package com.google.googlejavaformat.java; -import static com.google.common.collect.Iterables.getLast; import static com.google.common.primitives.Booleans.trueFirst; import com.google.common.base.CharMatcher; @@ -22,13 +21,13 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; import com.google.common.collect.ImmutableSortedSet; +import com.google.common.collect.Range; +import com.google.errorprone.annotations.InlineMe; import com.google.googlejavaformat.Newlines; import com.google.googlejavaformat.java.JavaFormatterOptions.Style; import com.google.googlejavaformat.java.JavaInput.Tok; import com.sun.tools.javac.parser.Tokens.TokenKind; -import java.util.ArrayList; import java.util.Comparator; -import java.util.List; import java.util.Optional; import java.util.function.BiFunction; import java.util.stream.Stream; @@ -43,8 +42,15 @@ public class ImportOrderer { /// /// @throws FormatterException if the input could not be parsed. public static String reorderImports(String text, Style style) throws FormatterException { - ImmutableList toks = JavaInput.buildToks(text, CLASS_START); - return new ImportOrderer(text, toks, style).reorderImports(); + ImmutableList replacements = findReplacements(text, style); + if (replacements.isEmpty()) { + return text; + } + Replacement replacement = replacements.get(0); + Range range = replacement.replaceRange(); + return text.substring(0, range.lowerEndpoint()) + + replacement.replacementString() + + text.substring(range.upperEndpoint()); } /// Reorder the inputs in `text`, a complete Java program, in Google style. On success, another @@ -54,15 +60,30 @@ public static String reorderImports(String text, Style style) throws FormatterEx /// @deprecated Use [#reorderImports(String, Style)] instead /// @throws FormatterException if the input could not be parsed. @Deprecated + @InlineMe( + replacement = "ImportOrderer.reorderImports(text, Style.GOOGLE)", + imports = { + "com.google.googlejavaformat.java.ImportOrderer", + "com.google.googlejavaformat.java.JavaFormatterOptions.Style" + }) public static String reorderImports(String text) throws FormatterException { return reorderImports(text, Style.GOOGLE); } - private String reorderImports() throws FormatterException { + /// Returns a replacement that rewrites the import block, from the start of the first import's + /// line through the whitespace after the last import, or an empty list if the imports are already + /// in order. + static ImmutableList findReplacements(String text, Style style) + throws FormatterException { + ImmutableList toks = JavaInput.buildToks(text, CLASS_START); + return new ImportOrderer(text, toks, style).findReplacements(); + } + + private ImmutableList findReplacements() throws FormatterException { Optional maybeFirstImport = findIdentifier(0, IMPORT_OR_CLASS_START); if (!maybeFirstImport.isPresent() || !tokenAt(maybeFirstImport.get()).equals("import")) { // No imports, so nothing to do. - return text; + return ImmutableList.of(); } int firstImportStart = maybeFirstImport.get(); int unindentedFirstImportStart = unindent(firstImportStart); @@ -76,27 +97,31 @@ private String reorderImports() throws FormatterException { throw new FormatterException("Imports not contiguous (perhaps a comment separates them?)"); } - StringBuilder result = new StringBuilder(); - String prefix = tokString(0, unindentedFirstImportStart); - result.append(prefix); + int start = toks.get(unindentedFirstImportStart).getPosition(); + + StringBuilder replacement = new StringBuilder(); + String prefix = text.substring(0, start); if (!prefix.isEmpty() && Newlines.getLineEnding(prefix) == null) { - result.append(lineSeparator).append(lineSeparator); - } - result.append(reorderedImportsString(imports.imports)); - - List tail = new ArrayList<>(); - tail.add(CharMatcher.whitespace().trimLeadingFrom(tokString(afterLastImport, toks.size()))); - if (!toks.isEmpty()) { - Tok lastTok = getLast(toks); - int tailStart = lastTok.getPosition() + lastTok.length(); - tail.add(text.substring(tailStart)); + replacement.append(lineSeparator).append(lineSeparator); } - if (tail.stream().anyMatch(s -> !s.isEmpty())) { - result.append(lineSeparator); - tail.forEach(result::append); + replacement.append(reorderedImportsString(imports.imports)); + + int afterLastImportPos = + afterLastImport < toks.size() ? toks.get(afterLastImport).getPosition() : text.length(); + String afterText = text.substring(afterLastImportPos); + String trimmedAfterText = CharMatcher.whitespace().trimLeadingFrom(afterText); + int numTrimmed = afterText.length() - trimmedAfterText.length(); + int end = afterLastImportPos + numTrimmed; + + if (end < text.length()) { + replacement.append(lineSeparator); } - return result.toString(); + String replacementString = replacement.toString(); + if (replacementString.equals(text.substring(start, end))) { + return ImmutableList.of(); + } + return ImmutableList.of(Replacement.create(start, end, replacementString)); } /// [TokenKind]s that indicate the start of a type definition. We use this to avoid scanning the @@ -236,14 +261,6 @@ public String toString() { } } - private String tokString(int start, int end) { - StringBuilder sb = new StringBuilder(); - for (int i = start; i < end; i++) { - sb.append(toks.get(i).getOriginalText()); - } - return sb.toString(); - } - private record ImportsAndIndex(ImmutableSortedSet imports, int index) {} /// Scans a sequence of import lines. The parsing uses this approximate grammar: diff --git a/core/src/main/java/com/google/googlejavaformat/java/JavaInput.java b/core/src/main/java/com/google/googlejavaformat/java/JavaInput.java index 4fe3a430f..5dfdbb34a 100644 --- a/core/src/main/java/com/google/googlejavaformat/java/JavaInput.java +++ b/core/src/main/java/com/google/googlejavaformat/java/JavaInput.java @@ -41,7 +41,6 @@ import com.sun.tools.javac.util.Log; import com.sun.tools.javac.util.Log.DeferredDiagnosticHandler; import com.sun.tools.javac.util.Options; -import java.io.IOException; import java.lang.reflect.Constructor; import java.lang.reflect.Method; import java.net.URI; @@ -243,13 +242,10 @@ public String toString() { private final String text; // The input. private int kN; // The number of numbered toks (tokens or comments), excluding the EOF. - /* - * The following lists record the sequential indices of the {@code Tok}s on each input line. (Only - * tokens and comments have sequential indices.) Tokens and {@code //} comments lie on just one - * line; {@code /*} comments can lie on multiple lines. These data structures (along with - * equivalent ones for the formatted output) let us compute correspondences between the input and - * output. - */ + // The following lists record the sequential indices of the `Tok`s on each input line. (Only + // tokens and comments have sequential indices.) Tokens and `//` comments lie on just one line; + // `/*` comments can lie on multiple lines. These data structures (along with equivalent ones for + // the formatted output) let us compute correspondences between the input and output. private final ImmutableMap positionToColumnMap; // Map Tok position to column. private final ImmutableList tokens; // The Tokens for this input. @@ -345,7 +341,7 @@ static ImmutableList buildToks(String text, ImmutableSet stopTok log.useSource( new SimpleJavaFileObject(URI.create("Source.java"), Kind.SOURCE) { @Override - public CharSequence getCharContent(boolean ignoreEncodingErrors) throws IOException { + public String getCharContent(boolean ignoreEncodingErrors) { return text; } }); @@ -353,6 +349,8 @@ public CharSequence getCharContent(boolean ignoreEncodingErrors) throws IOExcept ImmutableList rawToks = JavacTokens.getTokens(text, context, stopTokens); Collection ds; try { + // GET_DIAGNOSTICS is DeferredDiagnosticHandler#getDiagnostics, which returns a + // Collection on all supported JDK versions @SuppressWarnings("unchecked") var extraLocalForSuppression = (Collection) GET_DIAGNOSTICS.invoke(diagnostics); ds = extraLocalForSuppression; @@ -584,22 +582,41 @@ private static boolean isParamComment(Tok tok) { && tok.getText().matches("\\/\\*[A-Za-z0-9\\s_\\-]+=\\s*\\*\\/"); } - /// Convert from a character range to a token range. + /// Checks that `characterRange` ends within a text of length `textLength`. /// - /// @param characterRange the `0`-based [Range] of characters - /// @return the `0`-based [Range] of tokens - /// @throws FormatterException if the upper endpoint of the range is outside the file - private Range characterRangeToTokenRange(Range characterRange) + /// @throws FormatterException if the upper endpoint of the range is outside the text + static void validateRange(Range characterRange, int textLength) throws FormatterException { - if (characterRange.upperEndpoint() > text.length()) { + if (characterRange.upperEndpoint() > textLength) { throw new FormatterException( String.format( "error: invalid offset (%d) or length (%d); offset + length (%d) > file length (%d)", characterRange.lowerEndpoint(), characterRange.upperEndpoint() - characterRange.lowerEndpoint(), characterRange.upperEndpoint(), - text.length())); + textLength)); + } + } + + /// Checks that every range in `characterRanges` ends within a text of length `textLength`. + /// + /// @throws FormatterException if the upper endpoint of any range is outside the text + static void validateRanges(Collection> characterRanges, int textLength) + throws FormatterException { + for (Range characterRange : characterRanges) { + validateRange(characterRange, textLength); } + } + + /// Convert from a character range to a token range. + /// + /// @param characterRange the `0`-based [Range] of characters + /// @return the `0`-based [Range] of tokens + /// @throws FormatterException if the upper endpoint of the range is outside the file + private Range characterRangeToTokenRange(Range characterRange) + throws FormatterException { + validateRange(characterRange, text.length()); + // empty range stands for "format the line under the cursor" Range nonEmptyRange = characterRange.isEmpty() diff --git a/core/src/main/java/com/google/googlejavaformat/java/ModifierOrderer.java b/core/src/main/java/com/google/googlejavaformat/java/ModifierOrderer.java index 251a3e84a..5c5cd8f57 100644 --- a/core/src/main/java/com/google/googlejavaformat/java/ModifierOrderer.java +++ b/core/src/main/java/com/google/googlejavaformat/java/ModifierOrderer.java @@ -24,7 +24,6 @@ import com.google.common.collect.Ordering; import com.google.common.collect.Range; import com.google.common.collect.RangeSet; -import com.google.common.collect.TreeRangeMap; import com.google.googlejavaformat.Input.Tok; import com.google.googlejavaformat.Input.Token; import com.sun.tools.javac.parser.Tokens.TokenKind; @@ -32,20 +31,12 @@ import java.util.Collection; import java.util.Iterator; import java.util.List; -import java.util.Map; -import java.util.Map.Entry; import javax.lang.model.element.Modifier; import org.jspecify.annotations.Nullable; /// Fixes sequences of modifiers to be in JLS order. final class ModifierOrderer { - /// Reorders all modifiers in the given text to be in JLS order. - static JavaInput reorderModifiers(String text) throws FormatterException { - return reorderModifiers( - new JavaInput(text), ImmutableList.of(Range.closedOpen(0, text.length()))); - } - /// A class that contains the tokens corresponding to a modifier. This is usually a single token /// (e.g. for `public`), but may be multiple tokens for modifiers containing `-` (e.g. /// `non-sealed`). @@ -95,6 +86,12 @@ public int compareTo(ModifierTokens o) { } } + /// Reorders all modifiers in the given text to be in JLS order. + static JavaInput reorderModifiers(String text) throws FormatterException { + return reorderModifiers( + new JavaInput(text), ImmutableList.of(Range.closedOpen(0, text.length()))); + } + /// Reorders all modifiers in the given text and within the given character ranges to be in JLS /// order. static JavaInput reorderModifiers(JavaInput javaInput, Collection> characterRanges) @@ -104,9 +101,17 @@ static JavaInput reorderModifiers(JavaInput javaInput, Collection // Errors about invalid input will be reported later after parsing. return javaInput; } + return applyReplacements(javaInput, findModifierReplacements(javaInput, characterRanges)); + } + + static ImmutableList findModifierReplacements( + JavaInput javaInput, Collection> characterRanges) throws FormatterException { + ImmutableList.Builder replacements = ImmutableList.builder(); + if (javaInput.getTokens().isEmpty()) { + return replacements.build(); + } RangeSet tokenRanges = javaInput.characterRangesToTokenRanges(characterRanges); Iterator it = javaInput.getTokens().iterator(); - TreeRangeMap replacements = TreeRangeMap.create(); while (it.hasNext()) { ModifierTokens tokens = getModifierTokens(it); if (tokens.isEmpty() @@ -142,10 +147,10 @@ static JavaInput reorderModifiers(JavaInput javaInput, Collection addTrivia(replacement, modifierTokens.get(i).getToksAfter()); } } - replacements.put(Range.closedOpen(begin, end), replacement.toString()); + replacements.add(Replacement.create(begin, end, replacement.toString())); } } - return applyReplacements(javaInput, replacements); + return replacements.build(); } private static void addTrivia(StringBuilder replacement, ImmutableList toks) { @@ -214,18 +219,15 @@ private static void addTrivia(StringBuilder replacement, ImmutableList replacementMap) throws FormatterException { - // process in descending order so the replacement ranges aren't perturbed if any replacements - // differ in size from the input - Map, String> ranges = replacementMap.asDescendingMapOfRanges(); - if (ranges.isEmpty()) { + private static JavaInput applyReplacements(JavaInput javaInput, List replacements) + throws FormatterException { + if (replacements.isEmpty()) { return javaInput; } StringBuilder sb = new StringBuilder(javaInput.getText()); - for (Entry, String> entry : ranges.entrySet()) { - Range range = entry.getKey(); - sb.replace(range.lowerEndpoint(), range.upperEndpoint(), entry.getValue()); + for (Replacement replacement : replacements.reversed()) { + Range range = replacement.replaceRange(); + sb.replace(range.lowerEndpoint(), range.upperEndpoint(), replacement.replacementString()); } return new JavaInput(sb.toString()); } diff --git a/core/src/main/java/com/google/googlejavaformat/java/RemoveUnusedImports.java b/core/src/main/java/com/google/googlejavaformat/java/RemoveUnusedImports.java index 732f37c67..5a02dc4b8 100644 --- a/core/src/main/java/com/google/googlejavaformat/java/RemoveUnusedImports.java +++ b/core/src/main/java/com/google/googlejavaformat/java/RemoveUnusedImports.java @@ -22,16 +22,12 @@ import com.google.common.base.CharMatcher; import com.google.common.collect.HashMultimap; +import com.google.common.collect.ImmutableList; import com.google.common.collect.Multimap; import com.google.common.collect.Range; -import com.google.common.collect.RangeMap; -import com.google.common.collect.RangeSet; -import com.google.common.collect.TreeRangeMap; -import com.google.common.collect.TreeRangeSet; import com.google.googlejavaformat.Newlines; import com.sun.source.doctree.DocCommentTree; import com.sun.source.doctree.ReferenceTree; -import com.sun.source.tree.CaseTree; import com.sun.source.tree.IdentifierTree; import com.sun.source.tree.ImportTree; import com.sun.source.tree.Tree; @@ -51,7 +47,6 @@ import java.util.ArrayList; import java.util.LinkedHashSet; import java.util.List; -import java.util.Map; import java.util.Set; import javax.tools.Diagnostic; import javax.tools.JavaFileObject; @@ -104,38 +99,13 @@ public Void visitIdentifier(IdentifierTree tree, Void unused) { return null; } - // TODO(cushon): remove this override when pattern matching in switch is no longer a preview - // feature, and TreePathScanner visits CaseTree#getLabels instead of CaseTree#getExpressions - @SuppressWarnings("unchecked") // reflection - @Override - public Void visitCase(CaseTree tree, Void unused) { - if (CASE_TREE_GET_LABELS != null) { - try { - scan((List) CASE_TREE_GET_LABELS.invoke(tree), null); - } catch (ReflectiveOperationException e) { - throw new LinkageError(e.getMessage(), e); - } - } - return super.visitCase(tree, null); - } - - private static final Method CASE_TREE_GET_LABELS = caseTreeGetLabels(); - - private static Method caseTreeGetLabels() { - try { - return CaseTree.class.getMethod("getLabels"); - } catch (NoSuchMethodException e) { - return null; - } - } - @Override public Void scan(Tree tree, Void unused) { if (tree == null) { return null; } scanJavadoc(); - return super.scan(tree, unused); + return super.scan(tree, null); } private void scanJavadoc() { @@ -183,7 +153,7 @@ public Void visitReference(ReferenceTree referenceTree, Void unused) { private class ReferenceScanner extends TreeScanner { private final long basePos; - public ReferenceScanner(long basePos) { + ReferenceScanner(long basePos) { this.basePos = basePos; } @@ -194,7 +164,7 @@ public Void visitIdentifier(IdentifierTree node, Void aVoid) { basePos != -1 ? Range.closedOpen((int) basePos, (int) basePos + node.getName().length()) : null); - return super.visitIdentifier(node, aVoid); + return super.visitIdentifier(node, null); } } } @@ -207,10 +177,14 @@ public static String removeUnusedImports(final String contents) throws Formatter // error handling is done during formatting return contents; } + return applyReplacements(contents, findUnusedImports(context, unit, contents)); + } + + static ImmutableList findUnusedImports( + Context context, JCCompilationUnit unit, String contents) { UnusedImportScanner scanner = new UnusedImportScanner(JavacTrees.instance(context)); scanner.scan(unit, null); - return applyReplacements( - contents, buildReplacements(contents, unit, scanner.usedNames, scanner.usedInJavadoc)); + return buildReplacements(contents, unit, scanner.usedNames, scanner.usedInJavadoc); } private static JCCompilationUnit parse(Context context, String javaInput) @@ -226,12 +200,12 @@ private static JCCompilationUnit parse(Context context, String javaInput) } /// Construct replacements to fix unused imports. - private static RangeMap buildReplacements( + private static ImmutableList buildReplacements( String contents, JCCompilationUnit unit, Set usedNames, Multimap> usedInJavadoc) { - RangeMap replacements = TreeRangeMap.create(); + ImmutableList.Builder replacements = ImmutableList.builder(); for (JCTree importTree : unit.getImports()) { if (isModuleImport(importTree)) { continue; @@ -248,9 +222,9 @@ private static RangeMap buildReplacements( && contents.subSequence(endPosition, endPosition + sep.length()).toString().equals(sep)) { endPosition += sep.length(); } - replacements.put(Range.closedOpen(importTree.getStartPosition(), endPosition), ""); + replacements.add(Replacement.create(importTree.getStartPosition(), endPosition, "")); } - return replacements; + return replacements.build(); } private static String getSimpleName(JCTree importTree) { @@ -325,28 +299,20 @@ private static boolean isModuleImport(JCTree importTree) { } } - /// Applies the replacements to the given source, and re-format any edited javadoc. - private static String applyReplacements(String source, RangeMap replacements) { - // save non-empty fixed ranges for reformatting after fixes are applied - RangeSet fixedRanges = TreeRangeSet.create(); - - // Apply the fixes in increasing order, adjusting ranges to account for - // earlier fixes that change the length of the source. The output ranges are - // needed so we can reformat fixed regions, otherwise the fixes could just - // be applied in descending order without adjusting offsets. + /// Applies the replacements to the given source. + private static String applyReplacements(String source, List replacements) { StringBuilder sb = new StringBuilder(source); int offset = 0; - for (Map.Entry, String> replacement : replacements.asMapOfRanges().entrySet()) { - Range range = replacement.getKey(); - String replaceWith = replacement.getValue(); + for (Replacement replacement : replacements) { + Range range = replacement.replaceRange(); + String replaceWith = replacement.replacementString(); int start = offset + range.lowerEndpoint(); int end = offset + range.upperEndpoint(); sb.replace(start, end, replaceWith); - if (!replaceWith.isEmpty()) { - fixedRanges.add(Range.closedOpen(start, end)); - } offset += replaceWith.length() - (range.upperEndpoint() - range.lowerEndpoint()); } return sb.toString(); } + + private RemoveUnusedImports() {} } diff --git a/core/src/test/java/com/google/googlejavaformat/java/ImportOrdererTest.java b/core/src/test/java/com/google/googlejavaformat/java/ImportOrdererTest.java index d41677add..c281c8276 100644 --- a/core/src/test/java/com/google/googlejavaformat/java/ImportOrdererTest.java +++ b/core/src/test/java/com/google/googlejavaformat/java/ImportOrdererTest.java @@ -878,4 +878,33 @@ private static Object[] createRow(String[][] inputAndOutput) { row[0] = ((String) row[0]).replace("\\\n", ""); return row; } + + /// Tests for [ImportOrderer#findReplacements]. + public static class FindReplacements { + + @Test + public void alreadySorted_noReplacement() throws FormatterException { + String input = + """ + import a.A; + import b.B; + + class C {} + """; + assertThat(ImportOrderer.findReplacements(input, Style.GOOGLE)).isEmpty(); + } + + @Test + public void manyBlankLinesAfterImports() throws FormatterException { + String input = "import b.B;\nimport a.A;" + "\n".repeat(12) + "class C{}\n"; + assertThat(ImportOrderer.reorderImports(input, Style.GOOGLE)) + .isEqualTo( + """ + import a.A; + import b.B; + + class C{} + """); + } + } }