Skip to content

Keep comments between imports with the import after them #44

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
abashev merged 1 commit into from
Sep 23, 2026
Merged

Keep comments between imports with the import after them #44

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 53 additions & 9 deletions open-java-format/src/main/java/com/palantir/javaformat/java/ImportOrderer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,10 +186,12 @@ private ImportOrderer(String text, ImmutableList<Tok> toks, Style style) {
class Import {
private final String imported;
private final boolean isStatic;
private final String leading;
private final String trailing;

Import(String imported, String trailing, boolean isStatic) {
Import(String imported, String leading, String trailing, boolean isStatic) {
this.imported = imported;
this.leading = leading;
this.trailing = trailing;
this.isStatic = isStatic;
}
Expand Down Expand Up @@ -227,9 +229,18 @@ boolean isJava() {
}

/**
* The {@code //} comment lines after the final {@code ;}, up to and including the line terminator of the last
* one. Note: In case two imports were separated by a space (which is disallowed by the style guide), the
* trailing whitespace of the first import does not include a line terminator.
* The comments that stood between the previous import and this one, including the line terminator after the
* last of them, or empty. They move with this import.
*/
String leading() {
return leading;
}

/**
* A block comment on the import's own line and the {@code //} comment lines after the final {@code ;}, up to
* and including the line terminator of the last one. Note: In case two imports were separated by a space
* (which is disallowed by the style guide), the trailing whitespace of the first import does not include a
* line terminator.
*/
String trailing() {
return trailing;
Expand All @@ -240,11 +251,12 @@ public boolean isThirdParty() {
return !(isAndroid() || isJava());
}

// One or multiple lines, the import itself and following comments, including the line
// terminator.
// One or multiple lines, the comments before the import, the import itself and following comments, including
// the line terminator.
@Override
public String toString() {
StringBuilder sb = new StringBuilder();
sb.append(leading());
sb.append("import ");
if (isStatic()) {
sb.append("static ");
Expand Down Expand Up @@ -282,18 +294,22 @@ private static class ImportsAndIndex {
*
* <pre>{@code
* <imports> -> (<end-of-line> | <import>)*
* <import> -> "import" <whitespace> ("static" <whitespace>)?
* <import> -> <comments-and-line-breaks>? "import" <whitespace> ("static" <whitespace>)?
* <identifier> ("." <identifier>)* ("." "*")? <whitespace>? ";"
* <whitespace>? <end-of-line>? (<line-comment> <end-of-line>)*
* <whitespace>? (<block-comment> <whitespace>?)? <end-of-line>? (<line-comment> <end-of-line>)*
* }</pre>
*
* The comments before an import are the ones between it and the previous import, so the first import has none: the
* text before it is left where it is.
*
* @param i the index to start parsing at.
* @return the result of parsing the imports.
* @throws FormatterException if imports could not parsed according to the grammar.
*/
private ImportsAndIndex scanImports(int i) throws FormatterException {
int afterLastImport = i;
ImmutableSortedSet.Builder<Import> imports = ImmutableSortedSet.orderedBy(importComparator);
String leading = "";
// JavaInput.buildToks appends a zero-width EOF token after all tokens. It won't match any
// of our tests here and protects us from running off the end of the toks list. Since it is
// zero-width it doesn't matter if we include it in our string concatenation at the end.
Expand Down Expand Up @@ -330,6 +346,15 @@ private ImportsAndIndex scanImports(int i) throws FormatterException {
trailing.append(tokenAt(i));
i++;
}
// A block comment on the import's own line stays with the import, as a line comment there does.
if (isBlockCommentToken(i)) {
trailing.append(tokenAt(i));
i++;
if (isSpaceToken(i)) {
trailing.append(tokenAt(i));
i++;
}
}
if (isNewlineToken(i)) {
trailing.append(tokenAt(i));
i++;
Expand All @@ -344,14 +369,25 @@ private ImportsAndIndex scanImports(int i) throws FormatterException {
i++;
}
}
imports.add(new Import(importedName, trailing.toString(), isStatic));
imports.add(new Import(importedName, leading, trailing.toString(), isStatic));
// Remember the position just after the import we just saw, before skipping blank lines.
// If the next thing after the blank lines is not another import then we don't want to
// include those blank lines in the text to be replaced.
afterLastImport = i;
while (isNewlineToken(i) || isSpaceToken(i)) {
i++;
}
// Comments between this import and the next one go with the next one, so they move with it when the
// imports are sorted. Comments after the last import belong to whatever follows it.
leading = "";
int next = i;
while (isCommentToken(next) || isNewlineToken(next) || isSpaceToken(next)) {
next++;
}
if (next > i && tokenAt(next).equals("import")) {
leading = CharMatcher.whitespace().trimTrailingFrom(tokString(i, next)) + lineSeparator;
i = next;
}
}
return new ImportsAndIndex(imports.build(), afterLastImport);
}
Expand Down Expand Up @@ -470,6 +506,14 @@ private boolean isSlashSlashCommentToken(int i) {
return toks.get(i).isSlashSlashComment();
}

private boolean isBlockCommentToken(int i) {
return toks.get(i).isComment() && !toks.get(i).isSlashSlashComment();
}

private boolean isCommentToken(int i) {
return toks.get(i).isComment();
}

private boolean isNewlineToken(int i) {
return toks.get(i).isNewline();
}
Expand Down
94 changes: 89 additions & 5 deletions open-java-format/src/test/java/com/palantir/javaformat/java/GoogleImportStyleTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -391,25 +391,109 @@ public static List<Object[]> parameters() {
"!!Could not parse imported name, at: ",
}
},
// A comment between two imports goes with the import after it (#39, from
// google/google-java-format#424); a comment on an import's own line stays with that import.
{
{
"import com.foo.Second;",
"import com.foo.First;",
"/* we don't support block comments",
" between imports either */",
"/* A block comment between imports",
" goes with the import after it. */",
"import com.foo.Third;",
},
{
"!!Imports not contiguous (perhaps a comment separates them?)",
"import com.foo.First;",
"import com.foo.Second;",
"/* A block comment between imports",
" goes with the import after it. */",
"import com.foo.Third;",
}
},
{
{
"import com.foo.Second; /* no block comments after imports */", //
"import com.foo.Second; /* A block comment after an import stays with it. */", //
"import com.foo.First;",
},
{
"!!Imports not contiguous (perhaps a comment separates them?)",
"import com.foo.First;", //
"import com.foo.Second; /* A block comment after an import stays with it. */",
}
},
{
{
"import b.B;", //
"",
"// why we need A",
"import a.A;",
"",
"class T {}",
},
{
"// why we need A", //
"import a.A;",
"import b.B;",
"",
"class T {}",
}
},
{
{
"package foo;",
"",
"import groovy.transform.CompileStatic;",
"",
"/**",
" * Created.",
" */",
"import java.util.ArrayList;",
"",
"/**",
" * Created.",
" */",
"@CompileStatic",
"public class Broken {",
" ArrayList<?> list;",
"}",
},
{
"package foo;",
"",
"import groovy.transform.CompileStatic;",
"/**",
" * Created.",
" */",
"import java.util.ArrayList;",
"",
"/**",
" * Created.",
" */",
"@CompileStatic",
"public class Broken {",
" ArrayList<?> list;",
"}",
}
},
{
{
"import java.lang.reflect.Field;",
"",
"//import org.jline.nativ.JLineLibrary;",
"//import org.jline.nativ.JLineNativeLoader;",
"import org.jline.terminal.Attributes;",
"",
"import static org.jline.terminal.TerminalBuilder.PROP_NON_BLOCKING_READS;",
"",
"class T {}",
},
{
"import static org.jline.terminal.TerminalBuilder.PROP_NON_BLOCKING_READS;",
"",
"import java.lang.reflect.Field;",
"//import org.jline.nativ.JLineLibrary;",
"//import org.jline.nativ.JLineNativeLoader;",
"import org.jline.terminal.Attributes;",
"",
"class T {}",
}
},
{
Expand Down
21 changes: 21 additions & 0 deletions open-java-format/src/test/java/com/palantir/javaformat/java/MainTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -605,6 +605,27 @@ public void noReflowLongStrings() throws Exception {
assertThat(out.toString()).isEqualTo(joiner.join(expected));
}

// A comment between two imports goes with the import after it, where it used to fail the whole file with "Imports
// not contiguous" (#39, from google/google-java-format#424). A second run leaves the result alone.
@Test
public void commentBetweenImportsMovesWithTheImportAfterIt() throws Exception {
String[] input = {
"import b.B;", "", "// why we need A", "import a.A;", "", "class T {", " A a;", " B b;", "}", "",
};
String[] expected = {
"// why we need A", "import a.A;", "import b.B;", "", "class T {", " A a;", " B b;", "}", "",
};
for (String[] source : ImmutableList.of(input, expected)) {
StringWriter out = new StringWriter();
Main main = new Main(
new PrintWriter(out, true),
new PrintWriter(new BufferedWriter(new OutputStreamWriter(System.err, UTF_8)), true),
new ByteArrayInputStream(joiner.join(source).getBytes(UTF_8)));
assertThat(main.format("-")).isEqualTo(0);
assertThat(out.toString()).isEqualTo(joiner.join(expected));
}
}

private static ProcessBuilder formatterMain(String... args) {
return new ProcessBuilder(ImmutableList.<String>builder()
.add(Paths.get(System.getProperty("java.home"))
Expand Down
Loading