Skip to content

Commit

Permalink
Add an experimental flag to control removing unused imports
Browse files Browse the repository at this point in the history 
Also turn off javadoc-only import removal by default.

MOE_MIGRATED_REVID=126128582
  • Loading branch information
@cushon
cushon committed Jun 29, 2016
1 parent ffdf576 commit c9ec4f7
Show file tree
Hide file tree
Showing 6 changed files with 177 additions and 23 deletions.
17 changes: 16 additions & 1 deletion core/src/main/java/com/google/googlejavaformat/java/CommandLineOptions.java
View file
Expand Up @@ -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();
Expand All @@ -72,7 +81,9 @@ static Builder builder() {
.version(false)
.help(false)
.sortImports(SortImports.NO)
.stdin(false);
.stdin(false)
.fixImportsOnly(false)
.removeJavadocOnlyImports(false);
}

@AutoValue.Builder
Expand All @@ -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();
}
}
6 changes: 6 additions & 0 deletions core/src/main/java/com/google/googlejavaformat/java/CommandLineOptionsParser.java
View file
Expand Up @@ -85,6 +85,12 @@ static CommandLineOptions parse(Iterable<String> 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)));
Expand Down
9 changes: 9 additions & 0 deletions core/src/main/java/com/google/googlejavaformat/java/FormatFileCallable.java
View file
Expand Up @@ -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;

Expand Down Expand Up @@ -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());
}
Expand Down
38 changes: 30 additions & 8 deletions core/src/main/java/com/google/googlejavaformat/java/RemoveUnusedImports.java
View file
Expand Up @@ -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;
Expand Down Expand Up @@ -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.)
Expand Down Expand Up @@ -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) {
Expand All @@ -148,7 +160,11 @@ private static CompilationUnit parse(String source) {

/** Construct replacements to fix unused imports. */
private static RangeMap<Integer, String> buildReplacements(
CompilationUnit unit, Set<String> usedNames, Multimap<String, ASTNode> usedInJavadoc) {
String contents,
CompilationUnit unit,
Set<String> usedNames,
Multimap<String, ASTNode> usedInJavadoc,
JavadocOnlyImports javadocOnlyImports) {
RangeMap<Integer, String> replacements = TreeRangeMap.create();
for (ImportDeclaration importTree : (List<ImportDeclaration>) unit.imports()) {
String simpleName =
Expand All @@ -158,12 +174,18 @@ private static RangeMap<Integer, String> 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()) {
Expand Down
49 changes: 49 additions & 0 deletions core/src/test/java/com/google/googlejavaformat/java/MainTest.java
View file
Expand Up @@ -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<String> names;",
"}",
};
{
String[] expected = {
"import java.util.ArrayList;",
"import java.util.List;",
"class Test {",
" /**",
" * May be an {@link ArrayList}.",
" */",
" public static List<String> 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<String> 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));
}
}
}
81 changes: 67 additions & 14 deletions core/src/test/java/com/google/googlejavaformat/java/RemoveUnusedImportsTest.java
View file
Expand Up @@ -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;
Expand All @@ -36,6 +39,16 @@ public static Collection<Object[]> parameters() {
{
"import java.util.List;",
"import java.util.ArrayList;",
"",
"class Test {",
" /** could be an {@link ArrayList} */",
" List<String> xs;",
"}",
},
{
"import java.util.List;",
"import java.util.ArrayList;",
"",
"class Test {",
" /** could be an {@link ArrayList} */",
" List<String> xs;",
Expand All @@ -52,11 +65,17 @@ public static Collection<Object[]> 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 {}",
}
},
Expand All @@ -72,26 +91,54 @@ public static Collection<Object[]> 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<Object[]> 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);
}
Expand All @@ -100,15 +147,21 @@ public static Collection<Object[]> 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);
}
}

0 comments on commit c9ec4f7

Please sign in to comment.