From c9ec4f7574ec578b43e82c6afc17ffc2d3ed3851 Mon Sep 17 00:00:00 2001 From: cushon Date: Tue, 28 Jun 2016 15:47:14 -0700 Subject: [PATCH] Add an experimental flag to control removing unused imports Also turn off javadoc-only import removal by default. MOE_MIGRATED_REVID=126128582 --- .../java/CommandLineOptions.java | 17 +++- .../java/CommandLineOptionsParser.java | 6 ++ .../java/FormatFileCallable.java | 9 +++ .../java/RemoveUnusedImports.java | 38 +++++++-- .../googlejavaformat/java/MainTest.java | 49 +++++++++++ .../java/RemoveUnusedImportsTest.java | 81 +++++++++++++++---- 6 files changed, 177 insertions(+), 23 deletions(-) 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 13b1afffb..6de7970b4 100644 --- a/core/src/main/java/com/google/googlejavaformat/java/CommandLineOptions.java +++ b/core/src/main/java/com/google/googlejavaformat/java/CommandLineOptions.java @@ -60,6 +60,15 @@ enum SortImports { /** Format input from stdin. */ abstract boolean stdin(); + /** Fix imports, but do no formatting. */ + abstract boolean fixImportsOnly(); + + /** + * When fixing imports, remove imports that are used only in javadoc and fully-qualify any + * {@code @link} tags referring to the imported types. + */ + abstract boolean removeJavadocOnlyImports(); + /** Returns true if partial formatting was selected. */ boolean isSelection() { return !lines().isEmpty() || !offsets().isEmpty() || !lengths().isEmpty(); @@ -72,7 +81,9 @@ static Builder builder() { .version(false) .help(false) .sortImports(SortImports.NO) - .stdin(false); + .stdin(false) + .fixImportsOnly(false) + .removeJavadocOnlyImports(false); } @AutoValue.Builder @@ -97,6 +108,10 @@ abstract static class Builder { abstract Builder stdin(boolean stdin); + abstract Builder fixImportsOnly(boolean fixImportsOnly); + + abstract Builder removeJavadocOnlyImports(boolean removeJavadocOnlyImports); + abstract 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 afcf549d7..3b6c7abfc 100644 --- a/core/src/main/java/com/google/googlejavaformat/java/CommandLineOptionsParser.java +++ b/core/src/main/java/com/google/googlejavaformat/java/CommandLineOptionsParser.java @@ -85,6 +85,12 @@ static CommandLineOptions parse(Iterable options) { case "-h": optionsBuilder.help(true); break; + case "--fix-imports-only": + optionsBuilder.fixImportsOnly(true); + break; + case "--experimental-remove-javadoc-only-imports": + optionsBuilder.removeJavadocOnlyImports(true); + break; case "--sort-imports": case "-sort-imports": optionsBuilder.sortImports(parseSortImports(flag, getValue(flag, it, value))); diff --git a/core/src/main/java/com/google/googlejavaformat/java/FormatFileCallable.java b/core/src/main/java/com/google/googlejavaformat/java/FormatFileCallable.java index 97778c7c0..b402e7c87 100644 --- a/core/src/main/java/com/google/googlejavaformat/java/FormatFileCallable.java +++ b/core/src/main/java/com/google/googlejavaformat/java/FormatFileCallable.java @@ -18,6 +18,7 @@ import com.google.common.collect.RangeSet; import com.google.common.collect.TreeRangeSet; import com.google.googlejavaformat.java.CommandLineOptions.SortImports; +import com.google.googlejavaformat.java.RemoveUnusedImports.JavadocOnlyImports; import java.util.concurrent.Callable; @@ -49,6 +50,14 @@ public String call() throws FormatterException { } } + if (parameters.fixImportsOnly()) { + return RemoveUnusedImports.removeUnusedImports( + inputString, + parameters.removeJavadocOnlyImports() + ? JavadocOnlyImports.REMOVE + : JavadocOnlyImports.KEEP); + } + return new Formatter(options) .formatSource(inputString, characterRanges(inputString).asRanges()); } 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 b54b1bb5c..c60eabab4 100644 --- a/core/src/main/java/com/google/googlejavaformat/java/RemoveUnusedImports.java +++ b/core/src/main/java/com/google/googlejavaformat/java/RemoveUnusedImports.java @@ -16,6 +16,7 @@ package com.google.googlejavaformat.java; +import com.google.common.base.CharMatcher; import com.google.common.collect.HashMultimap; import com.google.common.collect.Iterables; import com.google.common.collect.Multimap; @@ -51,6 +52,14 @@ */ public class RemoveUnusedImports { + /** Configuration for javadoc-only imports. */ + public enum JavadocOnlyImports { + /** Remove imports that are only used in javadoc, and fully qualify any {@code @link} tags. */ + REMOVE, + /** Keep imports that are only used in javadoc. */ + KEEP + } + // Visits an AST, recording all simple names that could refer to imported // types and also any javadoc references that could refer to imported // types (`@link`, `@see`, `@throws`, etc.) @@ -125,12 +134,15 @@ public boolean visit(SimpleName node) { } } - public static String removeUnusedImports(final String contents) { + public static String removeUnusedImports( + final String contents, JavadocOnlyImports javadocOnlyImports) { CompilationUnit unit = parse(contents); UnusedImportScanner scanner = new UnusedImportScanner(); unit.accept(scanner); return applyReplacements( - contents, buildReplacements(unit, scanner.usedNames, scanner.usedInJavadoc)); + contents, + buildReplacements( + contents, unit, scanner.usedNames, scanner.usedInJavadoc, javadocOnlyImports)); } private static CompilationUnit parse(String source) { @@ -148,7 +160,11 @@ private static CompilationUnit parse(String source) { /** Construct replacements to fix unused imports. */ private static RangeMap buildReplacements( - CompilationUnit unit, Set usedNames, Multimap usedInJavadoc) { + String contents, + CompilationUnit unit, + Set usedNames, + Multimap usedInJavadoc, + JavadocOnlyImports javadocOnlyImports) { RangeMap replacements = TreeRangeMap.create(); for (ImportDeclaration importTree : (List) unit.imports()) { String simpleName = @@ -158,12 +174,18 @@ private static RangeMap buildReplacements( if (usedNames.contains(simpleName)) { continue; } + if (usedInJavadoc.containsKey(simpleName) && javadocOnlyImports == JavadocOnlyImports.KEEP) { + continue; + } // delete the import - replacements.put( - Range.closedOpen( - importTree.getStartPosition(), - importTree.getStartPosition() + importTree.getLength()), - ""); + int endPosition = importTree.getStartPosition() + importTree.getLength(); + endPosition = Math.max(CharMatcher.isNot(' ').indexIn(contents, endPosition), endPosition); + String sep = System.lineSeparator(); + if (endPosition + sep.length() < contents.length() + && contents.subSequence(endPosition, endPosition + sep.length()).equals(sep)) { + endPosition += sep.length(); + } + replacements.put(Range.closedOpen(importTree.getStartPosition(), endPosition), ""); // fully qualify any javadoc references with the same simple name as a deleted // non-static import if (!importTree.isStatic()) { 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 31d996832..791ae07c5 100644 --- a/core/src/test/java/com/google/googlejavaformat/java/MainTest.java +++ b/core/src/test/java/com/google/googlejavaformat/java/MainTest.java @@ -166,4 +166,53 @@ public void javadoc() throws Exception { assertThat(main.format("-")).isEqualTo(0); assertThat(out.toString()).isEqualTo(Joiner.on('\n').join(expected)); } + + // end to end import fixing test + @Test + public void imports() throws Exception { + String[] input = { + "import java.util.ArrayList;", + "import java.util.LinkedList;", + "import java.util.List;", + "class Test {", + " /**", + " * May be an {@link ArrayList}.", + " */", + " public static List names;", + "}", + }; + { + String[] expected = { + "import java.util.ArrayList;", + "import java.util.List;", + "class Test {", + " /**", + " * May be an {@link ArrayList}.", + " */", + " public static List names;", + "}", + }; + InputStream in = new ByteArrayInputStream(Joiner.on('\n').join(input).getBytes(UTF_8)); + StringWriter out = new StringWriter(); + Main main = new Main(new PrintWriter(out, true), new PrintWriter(System.err, true), in); + assertThat(main.format("-", "--fix-imports-only")).isEqualTo(0); + assertThat(out.toString()).isEqualTo(Joiner.on('\n').join(expected)); + } + { + String[] expected = { + "import java.util.List;", + "class Test {", + " /** May be an {@link java.util.ArrayList}. */", + " public static List names;", + "}", + }; + InputStream in = new ByteArrayInputStream(Joiner.on('\n').join(input).getBytes(UTF_8)); + StringWriter out = new StringWriter(); + Main main = new Main(new PrintWriter(out, true), new PrintWriter(System.err, true), in); + assertThat( + main.format("-", "--fix-imports-only", "--experimental-remove-javadoc-only-imports")) + .isEqualTo(0); + assertThat(out.toString()).isEqualTo(Joiner.on('\n').join(expected)); + } + } } diff --git a/core/src/test/java/com/google/googlejavaformat/java/RemoveUnusedImportsTest.java b/core/src/test/java/com/google/googlejavaformat/java/RemoveUnusedImportsTest.java index d7ee6e059..655f79e7a 100644 --- a/core/src/test/java/com/google/googlejavaformat/java/RemoveUnusedImportsTest.java +++ b/core/src/test/java/com/google/googlejavaformat/java/RemoveUnusedImportsTest.java @@ -15,6 +15,9 @@ package com.google.googlejavaformat.java; import static com.google.common.truth.Truth.assertThat; +import static com.google.googlejavaformat.java.RemoveUnusedImports.JavadocOnlyImports.KEEP; +import static com.google.googlejavaformat.java.RemoveUnusedImports.JavadocOnlyImports.REMOVE; +import static com.google.googlejavaformat.java.RemoveUnusedImports.removeUnusedImports; import com.google.common.base.Joiner; import com.google.common.collect.ImmutableList; @@ -36,6 +39,16 @@ public static Collection parameters() { { "import java.util.List;", "import java.util.ArrayList;", + "", + "class Test {", + " /** could be an {@link ArrayList} */", + " List xs;", + "}", + }, + { + "import java.util.List;", + "import java.util.ArrayList;", + "", "class Test {", " /** could be an {@link ArrayList} */", " List xs;", @@ -52,11 +65,17 @@ public static Collection parameters() { }, { { - "import java.util.ArrayList;", "/** {@link ArrayList#add} */", "class Test {}", + "import java.util.ArrayList;", // + "/** {@link ArrayList#add} */", + "class Test {}", }, { - "", // - "/** {@link java.util.ArrayList#add} */", + "import java.util.ArrayList;", // + "/** {@link ArrayList#add} */", + "class Test {}", + }, + { + "/** {@link java.util.ArrayList#add} */", // "class Test {}", } }, @@ -72,26 +91,54 @@ public static Collection parameters() { "}", }, { - "", // - "", - "", + "import a.A;", // "class Test {", + " /** a", + " * {@link A} */", + " void f() {}", + "}", + }, + { + "class Test {", // " /** a {@link a.A} */", " void f() {}", "}", } }, + { + { + "import a.A;import a.B;", // + "import a.C; // hello", + "class Test {", + " B b;", + "}", + }, + { + "import a.B;", // + "// hello", + "class Test {", + " B b;", + "}", + }, + { + "import a.B;", // + "// hello", + "class Test {", + " B b;", + "}", + } + }, }; ImmutableList.Builder builder = ImmutableList.builder(); for (String[][] inputAndOutput : inputsOutputs) { - assertThat(inputAndOutput.length).isEqualTo(2); + assertThat(inputAndOutput.length).isEqualTo(3); String[] input = inputAndOutput[0]; String[] output = inputAndOutput[1]; - if (output.length == 0) { - output = input; - } + String[] outputFixJavadoc = inputAndOutput[2]; String[] parameters = { - Joiner.on('\n').join(input) + '\n', Joiner.on('\n').join(output) + '\n', + Joiner.on('\n').join(input) + '\n', + Joiner.on('\n').join(output) + '\n', + Joiner.on('\n').join(outputFixJavadoc) + '\n', }; builder.add(parameters); } @@ -100,15 +147,21 @@ public static Collection parameters() { private final String input; private final String expected; + private final String expectedFixJavadoc; - public RemoveUnusedImportsTest(String input, String expected) { + public RemoveUnusedImportsTest(String input, String expected, String expectedFixJavadoc) { this.input = input; this.expected = expected; + this.expectedFixJavadoc = expectedFixJavadoc; } @Test public void removeUnused() throws FormatterException { - String actual = RemoveUnusedImports.removeUnusedImports(input); - assertThat(actual).isEqualTo(expected); + assertThat(removeUnusedImports(input, KEEP)).isEqualTo(expected); + } + + @Test + public void fixJavadoc() throws FormatterException { + assertThat(removeUnusedImports(input, REMOVE)).isEqualTo(expectedFixJavadoc); } }