diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 5c62b6298..e6fa6fe02 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -37,6 +37,68 @@ jobs: annotate_only: true job_summary: true + # The tests of `build` on the newer JDKs, in parallel with everything else: nothing waits for these + # jobs and nothing they build is kept. The code is still compiled for Java 21 by JDK 21, as in the + # release; -PjavaRuntime moves only the test JVMs, and with them the javac whose internals the + # formatter parses with. The IntelliJ plugin's tests stay on the runtime of the IDE they start. + # JDK 21 itself is `build`, the check the main branch requires. + jdk: + name: build (JDK ${{ matrix.jdk }}) + runs-on: ubuntu-latest + strategy: + fail-fast: false + matrix: + include: + - jdk: 25 + gradle_args: '' + gradle_args_reason: '' + - jdk: 26 + gradle_args: '' + gradle_args_reason: '' + - jdk: 27 + gradle_args: -x :gradle-open-java-format:test + gradle_args_reason: >- + Gradle 9.7.1 does not run on Java 27 yet, and the Gradle plugin's tests run their TestKit builds on + the test JVM, where they fail with "Unsupported class file major version 71". Those tests are left + out until the wrapper's Gradle supports Java 27. + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + + - name: Install JDK ${{ matrix.jdk }} for the tests + id: test-jdk + uses: actions/setup-java@de7274f081f381c8f8158605e0321c36c376e2e6 # v6.0.1 + with: + distribution: temurin + java-version: ${{ matrix.jdk }} + + # Installed last, so that it is the JAVA_HOME Gradle runs and compiles on. + - name: Install JDK 21 + uses: actions/setup-java@de7274f081f381c8f8158605e0321c36c376e2e6 # v6.0.1 + with: + distribution: temurin + java-version: '21' + + - name: Explain the extra Gradle arguments + if: ${{ matrix.gradle_args != '' }} + env: + GRADLE_ARGS: ${{ matrix.gradle_args }} + REASON: ${{ matrix.gradle_args_reason }} + run: echo "::notice title=JDK ${{ matrix.jdk }} runs with $GRADLE_ARGS::$REASON" + + # Gradle does not look into the runner's tool cache, so it is told where the test JDK is. + - name: Build + run: >- + ./gradlew test -PjavaRuntime=${{ matrix.jdk }} ${{ matrix.gradle_args }} + -Porg.gradle.java.installations.paths=${{ steps.test-jdk.outputs.path }} + + - name: Publish Test Report + uses: mikepenz/action-junit-report@a9170d5795813c01ab4901ffb045b52bab4ab09d # v6.5.0 + if: ${{ !cancelled() }} + with: + report_paths: '**/build/test-results/test/*.xml' + annotate_only: true + job_summary: true + # One explicit job per target, so every binary we ship is traceable to a named run. native: name: native (${{ matrix.platform }}) @@ -87,7 +149,8 @@ jobs: env: JDK21_HOME: ${{ steps.jdk21.outputs.path }} - # The binary itself on a file that needs formatting, a formatted one and one that does not parse. + # The binary itself on a file that needs formatting, a formatted one, one that does not parse and + # one in Java 25 syntax. - name: Smoke-test the binary run: | binary="$PWD/$(ls open-java-format-native/build/native/nativeCompile/open-java-format-* | grep -v '\.txt$')" @@ -100,6 +163,10 @@ jobs: printf 'class B {\n' > B.java set +e; "$binary" B.java; status=$?; set -e test "$status" -eq 2 + # Java 25 syntax: a compact source file with a module import and an unnamed pattern. + printf 'import module java.base;\nrecord Box(int a,int b){}\nvoid main(){Object o=new Box(1,2);if(o instanceof Box(_,_)){IO.println(List.of(1));}}\n' > C.java + "$binary" --replace C.java + printf 'import module java.base;\n\nrecord Box(int a, int b) {}\n\nvoid main() {\n Object o = new Box(1, 2);\n if (o instanceof Box(_, _)) {\n IO.println(List.of(1));\n }\n}\n' | diff - C.java - name: Test the plugins against the image run: ./gradlew -PnativeImage=true :open-java-format-jdk-bootstrap:test :gradle-open-java-format:test diff --git a/README.md b/README.md index c96fb2a74..8ce3d0a03 100644 --- a/README.md +++ b/README.md @@ -119,6 +119,10 @@ mise trust && mise install ./gradlew test # what the CI build job runs ``` +`-PjavaRuntime=25` runs the same tests on JDK 25, as CI's `jdk` jobs do for 25, 26 and 27. The code +is compiled for Java 21 either way. Gradle has to find that JDK: installed with mise, or named with +`-Porg.gradle.java.installations.paths=/path/to/jdk`. + Nothing inside the build downloads a JDK. `gradle.properties` turns toolchain auto-download off and reads the installations from `JDK21_HOME` and `GRAALVM_HOME`, so a missing JDK is an error you can read rather than a silent download. diff --git a/build.gradle b/build.gradle index a73a45132..566cb7961 100644 --- a/build.gradle +++ b/build.gradle @@ -87,6 +87,9 @@ allprojects { tasks.withType(Test).configureEach { jvmArgs(javacInternalExports.collect { "--add-exports=${it}=ALL-UNNAMED".toString() }) + // The short form gave CI logs only "FormatterException at FormatterIntegrationTest.java:76", never what + // the formatter complained about, nor the output of a TestKit build that failed. + testLogging.exceptionFormat = 'full' } tasks.withType(Javadoc).configureEach { @@ -127,7 +130,11 @@ subprojects { } } +// The JDK the tests run on, and with it the javac whose internals the formatter parses with. The code is +// compiled for 21 whatever this says. CI's `jdk` jobs pass -PjavaRuntime=25 and so on. +ext.javaRuntime = providers.gradleProperty('javaRuntime').getOrElse('21') + javaVersions { libraryTarget = 21 - runtime = 21 + runtime = javaRuntime } diff --git a/open-java-format-native/src/main/resources/META-INF/native-image/reachability-metadata.json b/open-java-format-native/src/main/resources/META-INF/native-image/reachability-metadata.json index c40f18794..5fde3bc33 100644 --- a/open-java-format-native/src/main/resources/META-INF/native-image/reachability-metadata.json +++ b/open-java-format-native/src/main/resources/META-INF/native-image/reachability-metadata.json @@ -131,6 +131,15 @@ } ] }, + { + "type": "com.sun.source.tree.ImportTree", + "methods": [ + { + "name": "isModule", + "parameterTypes": [] + } + ] + }, { "type": "com.sun.tools.javac.parser.JavaTokenizer", "fields": [ @@ -151,6 +160,28 @@ } ] }, + { + "type": "com.sun.tools.javac.tree.EndPosTable" + }, + { + "type": "com.sun.tools.javac.tree.JCTree", + "methods": [ + { + "name": "getEndPosition", + "parameterTypes": [ + "com.sun.tools.javac.tree.EndPosTable" + ] + } + ] + }, + { + "type": "com.sun.tools.javac.tree.JCTree$JCCompilationUnit", + "fields": [ + { + "name": "endPositions" + } + ] + }, { "type": "java.io.Serializable" }, diff --git a/open-java-format/build.gradle b/open-java-format/build.gradle index 12eb94e2f..d78260236 100644 --- a/open-java-format/build.gradle +++ b/open-java-format/build.gradle @@ -76,6 +76,9 @@ tasks.named("test") { // Run all classes and tests in parallel // https://junit.org/junit5/docs/current/user-guide/#writing-tests-parallel-execution systemProperty 'junit.jupiter.execution.parallel.mode.default', 'concurrent' + // FormatterVersionTest fails when the tests run on another JDK than the one asked for, so a CI leg + // cannot pass on the wrong JDK and skip the tests that need a newer parser. + systemProperty 'expectedJavaVersion', rootProject.ext.javaRuntime } javaVersion { diff --git a/open-java-format/src/main/java/com/palantir/javaformat/java/Formatter.java b/open-java-format/src/main/java/com/palantir/javaformat/java/Formatter.java index ef8dab408..b6b78bfda 100644 --- a/open-java-format/src/main/java/com/palantir/javaformat/java/Formatter.java +++ b/open-java-format/src/main/java/com/palantir/javaformat/java/Formatter.java @@ -184,8 +184,8 @@ public CharSequence getCharContent(boolean ignoreEncodingErrors) throws IOExcept }; Log.instance(context).useSource(source); ParserFactory parserFactory = ParserFactory.instance(context); - JavacParser parser = parserFactory.newParser( - sourceText, /*keepDocComments=*/ true, /*keepEndPos=*/ true, /*keepLineMap=*/ true); + JavacParser parser = + Trees.newParser(parserFactory, sourceText, /*keepDocComments=*/ true, /*keepLineMap=*/ true); unit = parser.parseCompilationUnit(); unit.sourcefile = source; diff --git a/open-java-format/src/main/java/com/palantir/javaformat/java/ImportOrderer.java b/open-java-format/src/main/java/com/palantir/javaformat/java/ImportOrderer.java index 4a0fdd46d..873b56a01 100644 --- a/open-java-format/src/main/java/com/palantir/javaformat/java/ImportOrderer.java +++ b/open-java-format/src/main/java/com/palantir/javaformat/java/ImportOrderer.java @@ -123,27 +123,42 @@ private String reorderImports() throws FormatterException { /** * A {@link Comparator} that orders {@link Import}s by Google Style, defined at * https://google.github.io/styleguide/javaguide.html#s3.3.3-import-ordering-and-spacing. + * + *

Google Style does not use module imports ({@code import module foo.bar;}, JEP 511); when present, they + * sort between static and non-static type imports, matching google-java-format. */ - private static final Comparator GOOGLE_IMPORT_COMPARATOR = - Comparator.comparing(Import::isStatic, trueFirst()).thenComparing(Import::imported); + private static final Comparator GOOGLE_IMPORT_COMPARATOR = Comparator.comparing( + Import::isStatic, trueFirst()) + .thenComparing(Import::isModule, trueFirst()) + .thenComparing(Import::imported) + // Imports that compare equal collapse into one, so two that are written differently -- one of them + // carrying a comment inside it or on the lines before it, say -- must not compare equal, or that comment + // disappears with it. + .thenComparing(Import::declaration) + .thenComparing(Import::leading); /** * A {@link Comparator} that orders {@link Import}s by AOSP Style, defined at * https://source.android.com/setup/contribute/code-style#order-import-statements and implemented in IntelliJ at * https://android.googlesource.com/platform/development/+/master/ide/intellij/codestyles/AndroidStyle.xml. + * + *

As with {@link #GOOGLE_IMPORT_COMPARATOR}, module imports sort after static imports. */ private static final Comparator AOSP_IMPORT_COMPARATOR = Comparator.comparing(Import::isStatic, trueFirst()) + .thenComparing(Import::isModule, trueFirst()) .thenComparing(Import::isAndroid, trueFirst()) .thenComparing(Import::isThirdParty, trueFirst()) .thenComparing(Import::isJava, trueFirst()) - .thenComparing(Import::imported); + .thenComparing(Import::imported) + .thenComparing(Import::declaration) + .thenComparing(Import::leading); /** * Determines whether to insert a blank line between the {@code prev} and {@code curr} {@link Import}s based on * Google style. */ private static boolean shouldInsertBlankLineGoogle(Import prev, Import curr) { - return prev.isStatic() && !curr.isStatic(); + return prev.isStatic() != curr.isStatic() || prev.isModule() != curr.isModule(); } /** @@ -151,7 +166,7 @@ private static boolean shouldInsertBlankLineGoogle(Import prev, Import curr) { * style. */ private static boolean shouldInsertBlankLineAosp(Import prev, Import curr) { - if (prev.isStatic() && !curr.isStatic()) { + if (prev.isStatic() != curr.isStatic() || prev.isModule() != curr.isModule()) { return true; } // insert blank line between "com.android" from "com.anythingelse" @@ -186,14 +201,32 @@ private ImportOrderer(String text, ImmutableList toks, Style style) { class Import { private final String imported; private final boolean isStatic; + private final boolean isModule; private final String leading; private final String trailing; - - Import(String imported, String leading, String trailing, boolean isStatic) { + private final String declaration; + + Import( + String imported, + String leading, + String trailing, + boolean isStatic, + boolean isModule, + String declaration) { this.imported = imported; this.leading = leading; this.trailing = trailing; this.isStatic = isStatic; + this.isModule = isModule; + this.declaration = declaration; + } + + /** + * The declaration as it will be written: the {@code import} keyword through the semicolon, with whitespace + * normalized and any comments left in the slot the author wrote them in. + */ + String declaration() { + return declaration; } /** The name being imported, for example {@code java.util.List}. */ @@ -206,6 +239,11 @@ boolean isStatic() { return isStatic; } + /** True if this is {@code import module} (JEP 511). */ + boolean isModule() { + return isModule; + } + /** The top-level package of the import. */ String topLevel() { return DOT_SPLITTER.split(imported()).iterator().next(); @@ -257,11 +295,7 @@ public boolean isThirdParty() { public String toString() { StringBuilder sb = new StringBuilder(); sb.append(leading()); - sb.append("import "); - if (isStatic()) { - sb.append("static "); - } - sb.append(imported()).append(';'); + sb.append(declaration()); if (trailing().trim().isEmpty()) { sb.append(lineSeparator); } else { @@ -271,6 +305,42 @@ public String toString() { } } + /** + * Renders one import declaration from the toks it is made of. Whitespace between the toks is normalized, since the + * style guide puts one import on a line of its own, but a comment stays in the slot the author wrote it in rather + * than being moved to wherever the rendered declaration can accommodate it. + */ + private final class Declaration { + private final StringBuilder text = new StringBuilder(); + private boolean atLineStart = false; + + /** Appends one tok: a keyword, an identifier, {@code .}, {@code *}, {@code ;}, or a comment. */ + void append(String piece) { + if (text.length() > 0 && !atLineStart && needsSpaceBefore(piece)) { + text.append(' '); + } + text.append(piece); + atLineStart = false; + if (piece.startsWith("//")) { + // A // comment swallows the rest of its line, so the declaration continues on the next one. + text.append(lineSeparator); + atLineStart = true; + } + } + + private boolean needsSpaceBefore(String piece) { + if (piece.equals(".") || piece.equals(";") || piece.equals("*")) { + return false; + } + return text.charAt(text.length() - 1) != '.'; + } + + @Override + public String toString() { + return text.toString(); + } + } + private String tokString(int start, int end) { StringBuilder sb = new StringBuilder(); for (int i = start; i < end; i++) { @@ -294,9 +364,10 @@ private static class ImportsAndIndex { * * 

{@code
      *  -> ( | )*
-     *  -> ? "import"  ("static" )?
-     *     ("." )* ("." "*")? ? ";"
-     *    ? ( ?)? ? ( )*
+     *  -> ? "import"  (("static" | "module") )?
+     *     ("." )* ("." "*")? ? ";"
+     *    ( | )* ? ( )*
+     *  -> ( |  | )+
      * }
* * The comments before an import are the ones between it and the previous import, so the first import has none: the @@ -314,47 +385,46 @@ private ImportsAndIndex scanImports(int i) throws FormatterException { // 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. while (i < toks.size() && tokenAt(i).equals("import")) { + Declaration declaration = new Declaration(); + declaration.append(tokenAt(i)); i++; - if (isSpaceToken(i)) { + i = skipIgnored(i, declaration); + boolean isModule = isModuleKeyword(i); + if (isModule) { + declaration.append(tokenAt(i)); i++; + i = skipIgnored(i, declaration); } - boolean isStatic = tokenAt(i).equals("static"); + boolean isStatic = !isModule && tokenAt(i).equals("static"); if (isStatic) { + declaration.append(tokenAt(i)); i++; - if (isSpaceToken(i)) { - i++; - } + i = skipIgnored(i, declaration); } if (!isIdentifierToken(i)) { throw new FormatterException("Unexpected token after import: " + tokenAt(i)); } - StringAndIndex imported = scanImported(i); + StringAndIndex imported = scanImported(i, declaration); String importedName = imported.string; i = imported.index; - if (isSpaceToken(i)) { - i++; - } + i = skipIgnored(i, declaration); if (!tokenAt(i).equals(";")) { throw new FormatterException("Expected ; after import"); } + declaration.append(";"); while (tokenAt(i).equals(";")) { // Extra semicolons are not allowed by the JLS but are accepted by javac. i++; } StringBuilder trailing = new StringBuilder(); - if (isSpaceToken(i)) { + // A block comment on the same line as the `;` trails this import and stays with it, as a line comment + // there does; one on a later line goes with whatever follows, so only same-line toks are absorbed here. + // Javadoc is excluded: the formatter moves a javadoc comment onto a line of its own, so it is taken as + // one of the comments below the import from the start. + while (isSpaceToken(i) || isBlockCommentToken(i)) { 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++; @@ -369,7 +439,8 @@ private ImportsAndIndex scanImports(int i) throws FormatterException { i++; } } - imports.add(new Import(importedName, leading, trailing.toString(), isStatic)); + imports.add( + new Import(importedName, leading, trailing.toString(), isStatic, isModule, declaration.toString())); // 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. @@ -423,17 +494,19 @@ private static class StringAndIndex { } /** - * Scans the imported thing, the dot-separated name that comes after import [static] and before the semicolon. We - * don't allow spaces inside the dot-separated name. Wildcard imports are supported: if the input is {@code import - * java.util.*;} then the returned string will be {@code java.util.*}. + * Scans the imported thing, the dot-separated name that comes after import [static] and before the semicolon. + * Whitespace, line terminators and comments may appear between its parts, as they may anywhere else in the + * declaration; the returned name contains none of them. Wildcard imports are supported: if the input is + * {@code import java.util.*;} then the returned string will be {@code java.util.*}. * * @param start the index of the start of the identifier. If the import is {@code import java.util.List;} then this * index points to the token {@code java}. + * @param declaration collects the toks scanned, so a comment between the parts of the name keeps its place * @return the parsed import ({@code java.util.List} in the example) and the index of the first token after the * imported thing ({@code ;} in the example). * @throws FormatterException if the imported name could not be parsed. */ - private StringAndIndex scanImported(int start) throws FormatterException { + private StringAndIndex scanImported(int start, Declaration declaration) throws FormatterException { int i = start; StringBuilder imported = new StringBuilder(); // At the start of each iteration of this loop, i points to an identifier. @@ -441,14 +514,19 @@ private StringAndIndex scanImported(int start) throws FormatterException { while (true) { Preconditions.checkState(isIdentifierToken(i)); imported.append(tokenAt(i)); + declaration.append(tokenAt(i)); i++; + i = skipIgnored(i, declaration); if (!tokenAt(i).equals(".")) { return new StringAndIndex(imported.toString(), i); } imported.append('.'); + declaration.append("."); i++; + i = skipIgnored(i, declaration); if (tokenAt(i).equals("*")) { imported.append('*'); + declaration.append("*"); return new StringAndIndex(imported.toString(), i + 1); } else if (!isIdentifierToken(i)) { throw new FormatterException("Could not parse imported name, at: " + tokenAt(i)); @@ -488,6 +566,41 @@ private String tokenAt(int i) { return toks.get(i).getOriginalText(); } + /** + * Returns true if the token at {@code i} is the {@code module} contextual keyword introducing a module import + * declaration ({@code import module foo.bar;}, JEP 511), as opposed to an ordinary import whose first segment + * happens to be an identifier literally named {@code module} (for example {@code import module.Foo;}). As with + * other contextual keywords ({@code var}, {@code yield}, ...), this is disambiguated by lookahead: {@code + * module} only introduces a module import when the next token is another identifier, rather than {@code .} or + * {@code ;}. Whitespace, line terminators and comments are skipped, as they are by the parsing that follows. + */ + private boolean isModuleKeyword(int i) { + if (!tokenAt(i).equals("module")) { + return false; + } + return isIdentifierToken(skipIgnored(i + 1, new Declaration())); + } + + /** + * Skips whitespace, line terminators and comments starting at {@code i}, appending each comment to + * {@code declaration}, and returns the index of the first token that is none of those. Javadoc comments are not + * skipped: the formatter moves them onto a line of their own, which would separate the imports, so an import + * carrying one is rejected, as it was before module imports were supported. + */ + private int skipIgnored(int i, Declaration declaration) { + while (i < toks.size()) { + if (isSpaceToken(i) || isNewlineToken(i)) { + i++; + } else if (isSlashSlashCommentToken(i) || isBlockCommentToken(i)) { + declaration.append(tokenAt(i).trim()); + i++; + } else { + break; + } + } + return i; + } + private boolean isIdentifierToken(int i) { String s = tokenAt(i); return !s.isEmpty() && Character.isJavaIdentifierStart(s.codePointAt(0)); @@ -506,14 +619,23 @@ private boolean isSlashSlashCommentToken(int i) { return toks.get(i).isSlashSlashComment(); } + /** True if the tok is a {@code /* *}{@code /} comment that is not javadoc. */ private boolean isBlockCommentToken(int i) { - return toks.get(i).isComment() && !toks.get(i).isSlashSlashComment(); + return toks.get(i).isSlashStarComment() && !toks.get(i).isJavadocComment(); } + /** True if the tok is a comment of any kind, javadoc included. */ private boolean isCommentToken(int i) { return toks.get(i).isComment(); } + /** True if {@code text} ends in a line terminator. */ + private static boolean endsInNewline(CharSequence text) { + return text.length() > 0 + && Newlines.isNewline( + text.subSequence(text.length() - 1, text.length()).toString()); + } + private boolean isNewlineToken(int i) { return toks.get(i).isNewline(); } diff --git a/open-java-format/src/main/java/com/palantir/javaformat/java/JavaInputAstVisitor.java b/open-java-format/src/main/java/com/palantir/javaformat/java/JavaInputAstVisitor.java index 3917911ee..0fd3569d8 100644 --- a/open-java-format/src/main/java/com/palantir/javaformat/java/JavaInputAstVisitor.java +++ b/open-java-format/src/main/java/com/palantir/javaformat/java/JavaInputAstVisitor.java @@ -132,6 +132,7 @@ import com.sun.tools.javac.code.Flags; import com.sun.tools.javac.tree.JCTree; import com.sun.tools.javac.tree.TreeScanner; +import java.lang.reflect.Method; import java.util.ArrayDeque; import java.util.ArrayList; import java.util.Collection; @@ -376,8 +377,15 @@ public Void visitCompilationUnit(CompilationUnitTree node, Void unused) { builder.blankLineWanted(BlankLineWanted.YES); } markForPartialFormat(); - scan(type, null); - builder.forcedBreak(); + if (isCompactSourceFile(type)) { + // JEP 512 (Java 25): a "compact source file" has top-level fields/methods and no + // explicit class declaration. javac models this by synthesizing an implicit wrapper + // class; format its member list directly, at column zero, with no enclosing braces. + addBodyDeclarations(((ClassTree) type).getMembers(), BracesOrNot.NO, FirstDeclarationsOrNot.YES, ZERO); + } else { + scan(type, null); + builder.forcedBreak(); + } first = false; dropEmptyDeclarations(); } @@ -1123,12 +1131,19 @@ public Void visitIf(IfTree node, Void unused) { return null; } + // ImportTree#isModule() (JEP 511) exists from JDK 23 on; this module compiles with a JDK 21 + // compiler, so it can't be referenced directly. + private static final Method IMPORT_TREE_IS_MODULE = maybeGetMethod(ImportTree.class, "isModule"); + @Override public Void visitImport(ImportTree node, Void unused) { sync(node); token("import"); builder.space(); - if (node.isStatic()) { + if (IMPORT_TREE_IS_MODULE != null && Boolean.TRUE.equals(invoke(IMPORT_TREE_IS_MODULE, node))) { + token("module"); + builder.space(); + } else if (node.isStatic()) { token("static"); builder.space(); } @@ -1381,6 +1396,16 @@ public Void visitAnnotatedType(AnnotatedTypeTree node, Void unused) { protected static final long RECORD = 1L << 61; + // TODO: use Flags.IMPLICIT_CLASS once this module compiles with JDK 22 or later (JDK 21 calls + // bit 19 UNNAMED_CLASS). javac sets it on the class it synthesizes for a compact source file. + private static final long IMPLICIT_CLASS = 1L << 19; + + /** Is {@code type} the implicit wrapper class javac synthesizes for a compact source file? */ + private static boolean isCompactSourceFile(Tree type) { + return type instanceof JCTree.JCClassDecl + && (((JCTree.JCClassDecl) type).mods.flags & IMPLICIT_CLASS) == IMPLICIT_CLASS; + } + @SuppressWarnings("for-rollout:NullAway") @Override public Void visitMethod(MethodTree node, Void unused) { @@ -3551,15 +3576,17 @@ int declareOne( { builder.open(ZERO); { - if (typeWithDims.isPresent() && typeWithDims.get().node != null) { + // `var` first: from JDK 27 (JDK-8268850) its type is a VarTypeTree rather than null, + // and this visitor, compiled against JDK 21, has no visitVarType to print it. + if (isVar) { + token("var"); + } else if (typeWithDims.isPresent() && typeWithDims.get().node != null) { scan(typeWithDims.get().node, null); int totalDims = dims.size(); builder.open(plusFour); maybeAddDims(dims); builder.close(); baseDims = totalDims - dims.size(); - } else if (isVar) { - token("var"); } else { scan(type, null); } @@ -3741,9 +3768,22 @@ private void declareMany(List fragments, Direction annotationDirec } /** Add a list of declarations. */ - @SuppressWarnings("for-rollout:NullAway") protected void addBodyDeclarations( List bodyDeclarations, BracesOrNot braces, FirstDeclarationsOrNot first0) { + addBodyDeclarations(bodyDeclarations, braces, first0, plusTwo); + } + + /** + * Add a list of declarations, indenting the member list by {@code memberIndent}. Used both for ordinary + * class/interface/enum bodies (indented by {@link #plusTwo}) and for the bare top-level member list of a + * compact source file (JEP 512), which is indented by {@link Indent.Const#ZERO}. + */ + @SuppressWarnings("for-rollout:NullAway") + private void addBodyDeclarations( + List bodyDeclarations, + BracesOrNot braces, + FirstDeclarationsOrNot first0, + Indent memberIndent) { if (bodyDeclarations.isEmpty()) { if (braces.isYes()) { builder.space(); @@ -3759,7 +3799,7 @@ protected void addBodyDeclarations( tokenBreakTrailingComment("{", plusTwo); builder.open(ZERO, BreakBehaviours.breakThisLevel(), LastLevelBreakability.ACCEPT_INLINE_CHAIN); } - builder.open(plusTwo); + builder.open(memberIndent); boolean first = first0.isYes(); boolean lastOneGotBlankLineBefore = false; PeekingIterator it = Iterators.peekingIterator(bodyDeclarations.iterator()); @@ -3958,6 +3998,29 @@ protected void sync(Tree node) { builder.sync(((JCTree) node).getStartPosition()); } + /** + * Returns the public no-argument method {@code name} of {@code c}, or {@code null} if the running JDK does not + * have it. Used with {@link #invoke} to reach AST accessors that are newer than the compiler this module builds + * with, such as {@code ImportTree#isModule()} (JDK 23). + */ + @SuppressWarnings("for-rollout:NullAway") + protected static Method maybeGetMethod(Class c, String name) { + try { + return c.getMethod(name); + } catch (ReflectiveOperationException e) { + return null; + } + } + + /** Invokes {@code m} on {@code target}, wrapping any {@link ReflectiveOperationException} in a runtime one. */ + protected static Object invoke(Method m, Object target) { + try { + return m.invoke(target); + } catch (ReflectiveOperationException e) { + throw new RuntimeException(e.getMessage(), e); + } + } + @Override public String toString() { return MoreObjects.toStringHelper(this).add("builder", builder).toString(); diff --git a/open-java-format/src/main/java/com/palantir/javaformat/java/RemoveUnusedImports.java b/open-java-format/src/main/java/com/palantir/javaformat/java/RemoveUnusedImports.java index be322711f..1d2fae6d5 100644 --- a/open-java-format/src/main/java/com/palantir/javaformat/java/RemoveUnusedImports.java +++ b/open-java-format/src/main/java/com/palantir/javaformat/java/RemoveUnusedImports.java @@ -42,7 +42,6 @@ import com.sun.tools.javac.tree.JCTree.JCCompilationUnit; import com.sun.tools.javac.tree.JCTree.JCFieldAccess; import com.sun.tools.javac.tree.JCTree.JCIdent; -import com.sun.tools.javac.tree.JCTree.JCImport; import com.sun.tools.javac.util.Context; import com.sun.tools.javac.util.Options; import java.lang.reflect.Method; @@ -227,13 +226,16 @@ private static RangeMap buildReplacements( Multimap> usedInJavadoc) { RangeMap replacements = TreeRangeMap.create(); String sep = Newlines.guessLineSeparator(contents); - for (JCImport importTree : unit.getImports()) { + // From JDK 23 on, getImports() also returns JCModuleImport, which is not a JCImport, so iterate + // over their common supertype and use ImportTree, which both implement. + for (JCTree importDecl : unit.getImports()) { + ImportTree importTree = (ImportTree) importDecl; String simpleName = getSimpleName(importTree); if (!isUnused(unit, usedNames, usedInJavadoc, importTree, simpleName)) { continue; } // delete the import - int endPosition = importTree.getEndPosition(unit.endPositions); + int endPosition = Trees.getEndPosition(importDecl, unit); endPosition = Math.max(CharMatcher.isNot(' ').indexIn(contents, endPosition), endPosition); if (endPosition + sep.length() < contents.length() && contents.subSequence(endPosition, endPosition + sep.length()) @@ -243,7 +245,7 @@ private static RangeMap buildReplacements( } // putCoalescing merges adjacent unused imports into one range, so the blank-line cleanup below sees the // whole deleted import block (TreeRangeMap.put does not coalesce). - replacements.putCoalescing(Range.closedOpen(importTree.getStartPosition(), endPosition), ""); + replacements.putCoalescing(Range.closedOpen(importDecl.getStartPosition(), endPosition), ""); } // Removing a whole import block can leave the blank line that preceded it stacked on the blank line that // followed it (package, blank, imports, blank, type). Collapse one of them, so a single formatting pass leaves @@ -293,6 +295,16 @@ private static boolean isBlankLineAfter(String contents, int pos, String sep) { return pos + sep.length() <= contents.length() && contents.regionMatches(pos, sep, 0, sep.length()); } + // ImportTree#isModule() (JEP 511) exists from JDK 23 on; this module compiles with a JDK 21 + // compiler, so it can't be referenced directly. Same idiom as CASE_TREE_GET_LABELS above. + private static final Method IMPORT_TREE_IS_MODULE = + JavaInputAstVisitor.maybeGetMethod(ImportTree.class, "isModule"); + + private static boolean isModuleImport(ImportTree importTree) { + return IMPORT_TREE_IS_MODULE != null + && Boolean.TRUE.equals(JavaInputAstVisitor.invoke(IMPORT_TREE_IS_MODULE, importTree)); + } + private static String getSimpleName(ImportTree importTree) { return importTree.getQualifiedIdentifier() instanceof JCIdent ? ((JCIdent) importTree.getQualifiedIdentifier()).getName().toString() @@ -307,6 +319,11 @@ private static boolean isUnused( Multimap> usedInJavadoc, ImportTree importTree, String simpleName) { + if (isModuleImport(importTree)) { + // A module import binds every exported package of the module, so this scanner can't tell + // whether it's needed - same as the `.*` wildcard imports below. Never remove it. + return false; + } String qualifier = ((JCFieldAccess) importTree.getQualifiedIdentifier()) .getExpression() .toString(); diff --git a/open-java-format/src/main/java/com/palantir/javaformat/java/StringWrapper.java b/open-java-format/src/main/java/com/palantir/javaformat/java/StringWrapper.java index fc9526862..80337879f 100644 --- a/open-java-format/src/main/java/com/palantir/javaformat/java/StringWrapper.java +++ b/open-java-format/src/main/java/com/palantir/javaformat/java/StringWrapper.java @@ -605,7 +605,7 @@ private static boolean noComments(String input, JCTree.JCCompilationUnit unit, T CharMatcher.whitespace().or(CharMatcher.anyOf("\"+")); private static int getEndPosition(JCTree.JCCompilationUnit unit, Tree tree) { - return ((JCTree) tree).getEndPosition(unit.endPositions); + return Trees.getEndPosition(tree, unit); } private static int getStartPosition(Tree tree) { diff --git a/open-java-format/src/main/java/com/palantir/javaformat/java/Trees.java b/open-java-format/src/main/java/com/palantir/javaformat/java/Trees.java index ead97b908..501561fc6 100644 --- a/open-java-format/src/main/java/com/palantir/javaformat/java/Trees.java +++ b/open-java-format/src/main/java/com/palantir/javaformat/java/Trees.java @@ -23,11 +23,19 @@ import com.sun.source.tree.ParenthesizedTree; import com.sun.source.tree.Tree; import com.sun.source.util.TreePath; +import com.sun.tools.javac.parser.JavacParser; +import com.sun.tools.javac.parser.ParserFactory; import com.sun.tools.javac.tree.JCTree; +import com.sun.tools.javac.tree.JCTree.JCCompilationUnit; import com.sun.tools.javac.tree.Pretty; import com.sun.tools.javac.tree.TreeInfo; import java.io.IOException; import java.io.UncheckedIOException; +import java.lang.invoke.MethodHandle; +import java.lang.invoke.MethodHandles; +import java.lang.invoke.MethodType; +import java.lang.invoke.VarHandle; +import javax.annotation.Nullable; import javax.lang.model.element.Name; /** Utilities for working with {@link Tree}s. */ @@ -44,8 +52,73 @@ static int getStartPosition(Tree expression) { /** Returns the source end position of the node. */ static int getEndPosition(Tree expression, TreePath path) { - return ((JCTree) expression) - .getEndPosition(((JCTree.JCCompilationUnit) path.getCompilationUnit()).endPositions); + return getEndPosition(expression, (JCCompilationUnit) path.getCompilationUnit()); + } + + /** Returns the source end position of the node. */ + static int getEndPosition(Tree tree, JCCompilationUnit unit) { + try { + return (int) GET_END_POSITION.invokeExact((JCTree) tree, unit); + } catch (RuntimeException | Error e) { + throw e; + } catch (Throwable e) { + throw new IllegalStateException(e); + } + } + + /** + * Creates a parser that records end positions. JDK-8372948 (JDK 27) dropped the {@code keepEndPos} argument + * from {@link ParserFactory#newParser}, so the remaining four-argument overload now reads + * {@code (input, keepDocComments, keepLineMap, parseModuleInfo)}. + */ + static JavacParser newParser( + ParserFactory parserFactory, CharSequence input, boolean keepDocComments, boolean keepLineMap) { + if (END_POS_TABLE_CLASS != null) { + return parserFactory.newParser(input, keepDocComments, /* keepEndPos= */ true, keepLineMap); + } + // The last argument is parseModuleInfo on these JDKs. + return parserFactory.newParser(input, keepDocComments, keepLineMap, false); + } + + /** + * {@code com.sun.tools.javac.tree.EndPosTable}, or null on JDKs that store end positions directly in the tree + * (JDK-8372948, JDK 27 and later). + */ + @Nullable + private static final Class END_POS_TABLE_CLASS = endPosTableClass(); + + /** {@code (JCTree, JCCompilationUnit) -> int}, bound to whichever end position API this JDK has. */ + private static final MethodHandle GET_END_POSITION = getEndPositionHandle(); + + @Nullable + private static Class endPosTableClass() { + try { + return Class.forName("com.sun.tools.javac.tree.EndPosTable"); + } catch (ClassNotFoundException e) { + return null; + } + } + + private static MethodHandle getEndPositionHandle() { + MethodHandles.Lookup lookup = MethodHandles.lookup(); + try { + if (END_POS_TABLE_CLASS == null) { + // (tree, unit) -> tree.getEndPosition() + return MethodHandles.dropArguments( + lookup.findVirtual(JCTree.class, "getEndPosition", MethodType.methodType(int.class)), + 1, + JCCompilationUnit.class); + } + // (tree, unit) -> tree.getEndPosition(unit.endPositions) + return MethodHandles.filterArguments( + lookup.findVirtual( + JCTree.class, "getEndPosition", MethodType.methodType(int.class, END_POS_TABLE_CLASS)), + 1, + lookup.findVarHandle(JCCompilationUnit.class, "endPositions", END_POS_TABLE_CLASS) + .toMethodHandle(VarHandle.AccessMode.GET)); + } catch (ReflectiveOperationException e) { + throw new IllegalStateException("Unsupported javac end position API", e); + } } /** Returns the source text for the node. */ diff --git a/open-java-format/src/main/java/com/palantir/javaformat/java/java14/Java14InputAstVisitor.java b/open-java-format/src/main/java/com/palantir/javaformat/java/java14/Java14InputAstVisitor.java index 76bb0d578..4566c0fbc 100644 --- a/open-java-format/src/main/java/com/palantir/javaformat/java/java14/Java14InputAstVisitor.java +++ b/open-java-format/src/main/java/com/palantir/javaformat/java/java14/Java14InputAstVisitor.java @@ -113,12 +113,20 @@ public Void visitBindingPattern(BindingPatternTree node, Void unused) { return null; } + /** + * Is {@code type} the {@code var} of a binding pattern? Older JDKs leave such a pattern without a type, JDK 27 + * (JDK-8268850) gives it a VarTypeTree, which does not exist on JDK 21, so it is matched by {@link Tree.Kind} name. + */ + private static boolean isVarType(Tree type) { + return type.getKind().name().equals("VAR_TYPE"); + } + private void visitBindingPattern(ModifiersTree modifiers, Tree type, Name name) { builder.open(plusFour); if (modifiers != null) { builder.addAll(visitModifiers(modifiers, Direction.HORIZONTAL, Optional.empty())); } - if (type == null) { + if (type == null || isVarType(type)) { token("var"); } else { scan(type, null); @@ -351,23 +359,6 @@ public Void visitLambdaExpression(LambdaExpressionTree node, Void unused) { return null; } - @SuppressWarnings("for-rollout:NullAway") - private static Method maybeGetMethod(Class c, String name) { - try { - return c.getMethod(name); - } catch (ReflectiveOperationException e) { - return null; - } - } - - private static Object invoke(Method m, Object target) { - try { - return m.invoke(target); - } catch (ReflectiveOperationException e) { - throw new RuntimeException(e.getMessage(), e); - } - } - @SuppressWarnings({"NullableProblems", "for-rollout:NullAway"}) protected ExpressionTree getGuard(final CaseTree node) { return null; diff --git a/open-java-format/src/main/java/com/palantir/javaformat/java/java21/Java21InputAstVisitor.java b/open-java-format/src/main/java/com/palantir/javaformat/java/java21/Java21InputAstVisitor.java index 5f089c4e6..f6f586ee7 100644 --- a/open-java-format/src/main/java/com/palantir/javaformat/java/java21/Java21InputAstVisitor.java +++ b/open-java-format/src/main/java/com/palantir/javaformat/java/java21/Java21InputAstVisitor.java @@ -25,6 +25,7 @@ import com.sun.source.tree.ExpressionTree; import com.sun.source.tree.PatternCaseLabelTree; import com.sun.source.tree.PatternTree; +import com.sun.source.tree.Tree; import javax.lang.model.element.Name; /** @@ -33,10 +34,25 @@ */ @SuppressWarnings("Since21") public class Java21InputAstVisitor extends Java14InputAstVisitor { + // AnyPatternTree (the unnamed pattern `_`, JEP 456) is still a preview API on JDK 21, which this + // module compiles with, so match it by Tree.Kind name instead of by type. + private static final String ANY_PATTERN_KIND_NAME = "ANY_PATTERN"; + public Java21InputAstVisitor(OpsBuilder builder, int indentMultiplier) { super(builder, indentMultiplier); } + @Override + public Void scan(Tree tree, Void unused) { + if (tree != null && tree.getKind().name().equals(ANY_PATTERN_KIND_NAME)) { + // No sync(tree): javac records the start position one past the `_`, which makes sync() + // think a token was skipped and throw. This branch emits one leaf token and never recurses. + token("_"); + return null; + } + return super.scan(tree, null); + } + @Override protected ExpressionTree getGuard(final CaseTree node) { return node.getGuard(); diff --git a/open-java-format/src/test/java/com/palantir/javaformat/java/AospImportStyleTest.java b/open-java-format/src/test/java/com/palantir/javaformat/java/AospImportStyleTest.java index 9401b1299..f568f8d9e 100644 --- a/open-java-format/src/test/java/com/palantir/javaformat/java/AospImportStyleTest.java +++ b/open-java-format/src/test/java/com/palantir/javaformat/java/AospImportStyleTest.java @@ -277,6 +277,79 @@ public static List parameters() { "public class Blim {}", }, }, + + // Module imports (JEP 511) sort between static imports and the android/third-party/java + // groups, as in google-java-format, each group separated by a blank line. + { + { + "package foo;", + "", + "import java.util.List;", + "import static android.Bar.baz;", + "import module java.desktop;", + "import android.Bar;", + "import module java.base;", + "", + "public class Blim {}", + }, + { + "package foo;", + "", + "import static android.Bar.baz;", + "", + "import module java.base;", + "import module java.desktop;", + "", + "import android.Bar;", + "", + "import java.util.List;", + "", + "public class Blim {}", + }, + }, + + // A module import and a non-module import that share a top level package: the blank line + // comes from the module boundary, not from the top-level-package rule. + { + { + "package foo;", + "", + "import java.util.List;", + "import module java.base;", + "", + "public class Blim {}", + }, + { + "package foo;", + "", + "import module java.base;", + "", + "import java.util.List;", + "", + "public class Blim {}", + }, + }, + + // Module imports sort before third-party imports, which would otherwise come first. + { + { + "package foo;", + "", + "import org.example.Bar;", + "import module java.base;", + "", + "public class Blim {}", + }, + { + "package foo;", + "", + "import module java.base;", + "", + "import org.example.Bar;", + "", + "public class Blim {}", + }, + }, }; ImmutableList.Builder builder = ImmutableList.builder(); Arrays.stream(inputsOutputs).forEach(input -> builder.add(ImportOrdererUtils.createRow(input))); @@ -289,6 +362,10 @@ public void reorder() throws FormatterException { String output = ImportOrderer.reorderImports(input, JavaFormatterOptions.Style.AOSP); assertWithMessage("Expected exception").that(reordered).doesNotMatch("^!!"); assertWithMessage(input).that(output).isEqualTo(reordered); + // Reordering must be a fixed point: a formatted file has to survive being formatted again. + assertWithMessage("not idempotent: %s", output) + .that(ImportOrderer.reorderImports(output, JavaFormatterOptions.Style.AOSP)) + .isEqualTo(output); } catch (FormatterException e) { if (!reordered.startsWith("!!")) { throw e; diff --git a/open-java-format/src/test/java/com/palantir/javaformat/java/FileBasedTests.java b/open-java-format/src/test/java/com/palantir/javaformat/java/FileBasedTests.java index 4edb8fa12..5305d7c47 100644 --- a/open-java-format/src/test/java/com/palantir/javaformat/java/FileBasedTests.java +++ b/open-java-format/src/test/java/com/palantir/javaformat/java/FileBasedTests.java @@ -59,7 +59,12 @@ public final class FileBasedTests { "SwitchUnderscore", "I880", "I1309", - "Unnamed") + "Unnamed", + "UnnamedPattern", + "CompactSource", + "MarkdownDoc", + "FlexibleConstructor") + .putAll(23, "ModuleImport") .build(); private final Class testClass; diff --git a/open-java-format/src/test/java/com/palantir/javaformat/java/FormatterVersionTest.java b/open-java-format/src/test/java/com/palantir/javaformat/java/FormatterVersionTest.java new file mode 100644 index 000000000..b04e99446 --- /dev/null +++ b/open-java-format/src/test/java/com/palantir/javaformat/java/FormatterVersionTest.java @@ -0,0 +1,36 @@ +/* + * (c) Copyright 2026 Palantir Technologies Inc. All rights reserved. + * + * 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.palantir.javaformat.java; + +import static org.assertj.core.api.Assertions.assertThat; + +import org.junit.jupiter.api.Assumptions; +import org.junit.jupiter.api.Test; + +/** + * Guards CI's {@code jdk} legs: without this, a leg that runs on an older JDK than the one it asked for with + * {@code -PjavaRuntime} skips the {@code ModuleImport} golden and the module import tests, and still passes. + */ +public class FormatterVersionTest { + + @Test + public void runsOnTheJdkTheTestTaskAskedFor() { + String expected = System.getProperty("expectedJavaVersion"); + Assumptions.assumeTrue(expected != null, "expectedJavaVersion is set by the Gradle build"); + assertThat(Formatter.getRuntimeVersion()).isEqualTo(Integer.parseInt(expected)); + } +} diff --git a/open-java-format/src/test/java/com/palantir/javaformat/java/GoogleImportStyleTest.java b/open-java-format/src/test/java/com/palantir/javaformat/java/GoogleImportStyleTest.java index 4124ae4c9..44b9b34e5 100644 --- a/open-java-format/src/test/java/com/palantir/javaformat/java/GoogleImportStyleTest.java +++ b/open-java-format/src/test/java/com/palantir/javaformat/java/GoogleImportStyleTest.java @@ -368,7 +368,9 @@ public static List parameters() { "import", }, { - "!!Unexpected token after import: \n", + // The line break after `import` is skipped, so the token we report is the + // zero-width EOF tok rather than the newline. + "!!Unexpected token after import: ", } }, { @@ -419,6 +421,19 @@ public static List parameters() { "import com.foo.Second; /* A block comment after an import stays with it. */", } }, + // Javadoc is the exception: the formatter moves it onto a line of its own, so it goes with the + // import after it, like any comment between imports. + { + { + "import com.foo.Second; /** javadoc after an import */", // + "import com.foo.First;", + }, + { + "/** javadoc after an import */", // + "import com.foo.First;", + "import com.foo.Second;", + } + }, { { "import b.B;", // @@ -496,6 +511,15 @@ public static List parameters() { "class T {}", } }, + { + { + "import /** javadoc inside an import */ com.foo.Second;", // + "import com.foo.First;", + }, + { + "!!Unexpected token after import: /** javadoc inside an import */", + } + }, { { "import com.foo.Second;", @@ -513,13 +537,15 @@ public static List parameters() { "*/", } }, + // Whitespace may appear between the parts of a qualified name; it is normalized away. { { - "import com . foo . Second ;", // syntactically valid, but we don't support it + "import com . foo . Second ;", // "import com.foo.First;", }, { - "!!Expected ; after import", + "import com.foo.First;", // + "import com.foo.Second;", } }, { @@ -608,6 +634,185 @@ public static List parameters() { "class Test {}", } }, + + // Module imports (JEP 511) sort between static and non-static type imports, as in + // google-java-format, each group separated by a blank line. + { + { + "package foo;", + "", + "import java.util.List;", + "import static com.google.truth.Truth.assertThat;", + "import module java.desktop;", + "import module java.base;", + "", + "public class Blim {}", + }, + { + "package foo;", + "", + "import static com.google.truth.Truth.assertThat;", + "", + "import module java.base;", + "import module java.desktop;", + "", + "import java.util.List;", + "", + "public class Blim {}", + }, + }, + + // A module whose name sorts after a non-module import: the module still comes first, and a + // blank line separates the two groups. + { + { + "package foo;", + "", + "import java.util.List;", + "import module org.example.api;", + "", + "public class Blim {}", + }, + { + "package foo;", + "", + "import module org.example.api;", + "", + "import java.util.List;", + "", + "public class Blim {}", + }, + }, + + // Whitespace, line breaks and comments may appear between the tokens of an import. The + // comments are re-emitted after the semicolon so that nothing is dropped. + { + { + "package foo;", + "", + "import module /* the base module */ java.base;", + "import module", + " java.desktop;", + "", + "public class Blim {}", + }, + { + "package foo;", + "", + "import module /* the base module */ java.base;", + "import module java.desktop;", + "", + "public class Blim {}", + }, + }, + + // The same holds for ordinary and static imports, which had this limitation before. + { + { + "package foo;", + "", + "import /* a type */ com.foo.Second;", + "import static /* a member */ com.foo.First.first;", + "import", + " com.foo.Third;", + "", + "public class Blim {}", + }, + { + "package foo;", + "", + "import static /* a member */ com.foo.First.first;", + "", + "import /* a type */ com.foo.Second;", + "import com.foo.Third;", + "", + "public class Blim {}", + }, + }, + + // A comment may also sit between the parts of the name, and stays there. Only the + // whitespace around it is normalized. + { + { + "package foo;", "", "import com.foo./* the second one */Second;", "", "public class Blim {}", + }, + { + "package foo;", "", "import com.foo./* the second one */ Second;", "", "public class Blim {}", + }, + }, + + // Identical declarations collapse into one; ones that differ are both kept, so that a + // comment does not disappear with the copy that goes. + { + { + "package foo;", + "", + "import com.foo.First;", + "import com.foo.First;", + "import /* explanation A */ com.foo.Second;", + "import /* explanation B */ com.foo.Second;", + "", + "public class Blim {}", + }, + { + "package foo;", + "", + "import com.foo.First;", + "import /* explanation A */ com.foo.Second;", + "import /* explanation B */ com.foo.Second;", + "", + "public class Blim {}", + }, + }, + + // The same goes for a comment on the lines before a copy: it moves with that copy, which + // stays. + { + { + "package foo;", + "", + "import com.foo.Second;", + "import com.foo.First;", + "/* why First is here twice */", + "import com.foo.First;", + "", + "public class Blim {}", + }, + { + "package foo;", + "", + "import com.foo.First;", + "/* why First is here twice */", + "import com.foo.First;", + "import com.foo.Second;", + "", + "public class Blim {}", + }, + }, + + // A package literally named `module` is an ordinary import, not a module import: + // `module` only introduces one when another identifier follows it. + { + { + "package foo;", + "", + "import module.Foo;", + "import java.util.List;", + "import module java.base;", + "", + "public class Blim {}", + }, + { + "package foo;", + "", + "import module java.base;", + "", + "import java.util.List;", + "import module.Foo;", + "", + "public class Blim {}", + }, + }, }; ImmutableList.Builder builder = ImmutableList.builder(); @@ -621,6 +826,10 @@ public void reorder() throws FormatterException { String output = ImportOrderer.reorderImports(input, JavaFormatterOptions.Style.GOOGLE); assertWithMessage("Expected exception").that(reordered).doesNotMatch("^!!"); assertWithMessage(input).that(output).isEqualTo(reordered); + // Reordering must be a fixed point: a formatted file has to survive being formatted again. + assertWithMessage("not idempotent: %s", output) + .that(ImportOrderer.reorderImports(output, JavaFormatterOptions.Style.GOOGLE)) + .isEqualTo(output); } catch (FormatterException e) { if (!reordered.startsWith("!!")) { throw e; diff --git a/open-java-format/src/test/java/com/palantir/javaformat/java/ModuleImportTest.java b/open-java-format/src/test/java/com/palantir/javaformat/java/ModuleImportTest.java new file mode 100644 index 000000000..269c98b07 --- /dev/null +++ b/open-java-format/src/test/java/com/palantir/javaformat/java/ModuleImportTest.java @@ -0,0 +1,123 @@ +/* + * (c) Copyright 2026 Palantir Technologies Inc. All rights reserved. + * + * 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.palantir.javaformat.java; + +import static org.assertj.core.api.Assertions.assertThat; + +import java.io.ByteArrayInputStream; +import java.io.PrintWriter; +import java.io.StringWriter; +import java.nio.charset.StandardCharsets; +import org.junit.jupiter.api.Assumptions; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.Test; + +/** + * End-to-end tests for module import declarations (JEP 511) through the pipeline the Gradle plugin and Spotless use, + * which reorders imports before formatting. The {@code ModuleImport} golden only exercises {@code formatSource}. + */ +public class ModuleImportTest { + + @BeforeAll + public static void requiresAParserThatProducesModuleImports() { + // Module imports parse from JDK 23 on, as a preview feature there; the formatter enables preview. + Assumptions.assumeTrue( + Formatter.getRuntimeVersion() >= 23, "import module requires running on JDK 23 or later"); + } + + /** Asserts both entry points that reorder imports, and that their output survives a second pass. */ + private static void assertFormats(String input, String expected) throws FormatterException { + assertThat(Formatter.create().formatSourceAndFixImports(input)).isEqualTo(expected); + assertThat(Formatter.create().fixImports(input)).isEqualTo(expected); + assertThat(Formatter.create().formatSourceAndFixImports(expected)).isEqualTo(expected); + assertThat(Formatter.create().fixImports(expected)).isEqualTo(expected); + } + + @Test + public void formatsAndFixesImports() throws FormatterException { + String input = "import module java.base;\n" + "class Example {}\n"; + String expected = "import module java.base;\n" + "\n" + "class Example {}\n"; + assertFormats(input, expected); + } + + @Test + public void fixesImportsOnlyFromTheCommandLine() throws Exception { + // The flag combination from the #1506 report, which failed with `Expected ; after import`. + String input = "import module java.base;\n" + "class Example {}\n"; + // Reordering puts a blank line after the import block; nothing else changes. + String expected = "import module java.base;\n" + "\n" + "class Example {}\n"; + StringWriter out = new StringWriter(); + StringWriter err = new StringWriter(); + Main main = new Main( + new PrintWriter(out, true), + new PrintWriter(err, true), + new ByteArrayInputStream(input.getBytes(StandardCharsets.UTF_8))); + int exitCode = main.format("-", "--fix-imports-only", "--skip-removing-unused-imports"); + assertThat(err.toString()).isEmpty(); + assertThat(exitCode).isZero(); + assertThat(out.toString()).isEqualTo(expected); + } + + @Test + public void keepsACommentBetweenModuleAndTheModuleName() throws FormatterException { + String input = "import module /* comment */ java.base;\n" + "class Example {}\n"; + String expected = "import module /* comment */ java.base;\n" + "\n" + "class Example {}\n"; + assertFormats(input, expected); + } + + @Test + public void keepsACommentBetweenThePartsOfTheModuleName() throws FormatterException { + String input = "import module java./* comment */base;\n" + "class Example {}\n"; + // Reordering normalizes the whitespace around the comment and leaves it between the parts. + String reordered = "import module java./* comment */ base;\n" + "\n" + "class Example {}\n"; + assertThat(Formatter.create().fixImports(input)).isEqualTo(reordered); + assertThat(Formatter.create().fixImports(reordered)).isEqualTo(reordered); + + // Formatting then breaks the line after the dot, which is where the formatter puts a comment + // in a qualified name; that output is stable too. + String formatted = "import module java.\n" + "/* comment */ base;\n" + "\n" + "class Example {}\n"; + assertThat(Formatter.create().formatSourceAndFixImports(input)).isEqualTo(formatted); + assertThat(Formatter.create().formatSourceAndFixImports(formatted)).isEqualTo(formatted); + } + + @Test + public void normalizesWhitespaceInsideTheModuleName() throws FormatterException { + String input = "import module java . base;\n" + "class Example {}\n"; + String expected = "import module java.base;\n" + "\n" + "class Example {}\n"; + assertFormats(input, expected); + } + + @Test + public void keepsBothCopiesOfADuplicateThatCarriesAComment() throws FormatterException { + // Identical declarations collapse; ones that differ are both kept, so no comment is dropped. + String input = "import module /* explanation A */ java.base;\n" + + "import module /* explanation B */ java.base;\n" + + "class Example {}\n"; + String expected = "import module /* explanation A */ java.base;\n" + + "import module /* explanation B */ java.base;\n" + + "\n" + + "class Example {}\n"; + assertFormats(input, expected); + } + + @Test + public void acceptsALineBreakAfterModule() throws FormatterException { + String input = "import module\n" + " java.base;\n" + "class Example {}\n"; + String expected = "import module java.base;\n" + "\n" + "class Example {}\n"; + assertFormats(input, expected); + } +} diff --git a/open-java-format/src/test/java/com/palantir/javaformat/java/RemoveUnusedImportsTest.java b/open-java-format/src/test/java/com/palantir/javaformat/java/RemoveUnusedImportsTest.java index 0efac4771..4981763f7 100644 --- a/open-java-format/src/test/java/com/palantir/javaformat/java/RemoveUnusedImportsTest.java +++ b/open-java-format/src/test/java/com/palantir/javaformat/java/RemoveUnusedImportsTest.java @@ -21,6 +21,7 @@ import com.google.common.truth.Truth; import com.palantir.javaformat.jupiter.ParameterizedClass; import java.util.List; +import org.junit.jupiter.api.Assumptions; import org.junit.jupiter.api.TestTemplate; import org.junit.jupiter.api.extension.ExtendWith; import org.junit.jupiter.api.parallel.Execution; @@ -254,6 +255,21 @@ public static List parameters() { "interface Test { private static void foo() {} }", }, }, + { + // Module imports (JEP 511, `import module foo.bar;`) parse to JCModuleImport, a + // sibling of JCImport rather than a subtype. They must never be reported as unused + // (see isUnused), and must not crash buildReplacements when mixed with ordinary + // imports, some used and some not. + { + "import module java.base;", + "import java.util.List;", + "import java.util.Map;", + "class T { List xs; }", + }, + { + "import module java.base;", "import java.util.List;", "class T { List xs; }", + }, + }, // An unused import between blank lines takes one of them with it (#37, from // google/google-java-format#1436 and google/google-java-format#1437). { @@ -346,6 +362,10 @@ public RemoveUnusedImportsTest(String input, String expected) { @TestTemplate public void removeUnused() throws FormatterException { + // Module imports (JEP 511) parse from JDK 23 on, as preview there; the formatter enables preview. + Assumptions.assumeTrue( + !input.contains("import module") || Formatter.getRuntimeVersion() >= 23, + "import module requires running on JDK 23 or later"); Truth.assertThat(RemoveUnusedImports.removeUnusedImports(input)).isEqualTo(expected); } } diff --git a/open-java-format/src/test/resources/com/palantir/javaformat/java/testdata/CompactSource.input b/open-java-format/src/test/resources/com/palantir/javaformat/java/testdata/CompactSource.input new file mode 100644 index 000000000..8ab254240 --- /dev/null +++ b/open-java-format/src/test/resources/com/palantir/javaformat/java/testdata/CompactSource.input @@ -0,0 +1,15 @@ +import java.util.List; + +/// A field with a markdown doc comment. +String greeting = "hello"; + +record Pair(String first, String second) {} + +/** Javadoc on a top-level member of a compact source file. */ +List names() { + return List.of(greeting, new Pair(greeting, greeting).second()); +} + +void main() { + System.out.println(names()); +} diff --git a/open-java-format/src/test/resources/com/palantir/javaformat/java/testdata/CompactSource.output b/open-java-format/src/test/resources/com/palantir/javaformat/java/testdata/CompactSource.output new file mode 100644 index 000000000..26ef3e586 --- /dev/null +++ b/open-java-format/src/test/resources/com/palantir/javaformat/java/testdata/CompactSource.output @@ -0,0 +1,15 @@ +import java.util.List; + +/// A field with a markdown doc comment. +String greeting = "hello"; + +record Pair(String first, String second) {} + +/** Javadoc on a top-level member of a compact source file. */ +List names() { + return List.of(greeting, new Pair(greeting, greeting).second()); +} + +void main() { + System.out.println(names()); +} diff --git a/open-java-format/src/test/resources/com/palantir/javaformat/java/testdata/FlexibleConstructor.input b/open-java-format/src/test/resources/com/palantir/javaformat/java/testdata/FlexibleConstructor.input new file mode 100644 index 000000000..e01857a19 --- /dev/null +++ b/open-java-format/src/test/resources/com/palantir/javaformat/java/testdata/FlexibleConstructor.input @@ -0,0 +1,12 @@ +class FlexibleConstructor { + private final int value; + + FlexibleConstructor(int raw) { + if (raw < 0) { + throw new IllegalArgumentException("negative"); + } + var normalized = Math.max(raw, 1); + super(); + this.value = normalized; + } +} diff --git a/open-java-format/src/test/resources/com/palantir/javaformat/java/testdata/FlexibleConstructor.output b/open-java-format/src/test/resources/com/palantir/javaformat/java/testdata/FlexibleConstructor.output new file mode 100644 index 000000000..798bb786c --- /dev/null +++ b/open-java-format/src/test/resources/com/palantir/javaformat/java/testdata/FlexibleConstructor.output @@ -0,0 +1,12 @@ +class FlexibleConstructor { + private final int value; + + FlexibleConstructor(int raw) { + if (raw < 0) { + throw new IllegalArgumentException("negative"); + } + var normalized = Math.max(raw, 1); + super(); + this.value = normalized; + } +} diff --git a/open-java-format/src/test/resources/com/palantir/javaformat/java/testdata/MarkdownDoc.input b/open-java-format/src/test/resources/com/palantir/javaformat/java/testdata/MarkdownDoc.input new file mode 100644 index 000000000..557fd468e --- /dev/null +++ b/open-java-format/src/test/resources/com/palantir/javaformat/java/testdata/MarkdownDoc.input @@ -0,0 +1,10 @@ +/// A greeter. +/// +/// Says hello. See [String]. +class MarkdownDoc { + /// Returns the greeting. + /// @return the text + String greeting() { + return "hi"; + } +} diff --git a/open-java-format/src/test/resources/com/palantir/javaformat/java/testdata/MarkdownDoc.output b/open-java-format/src/test/resources/com/palantir/javaformat/java/testdata/MarkdownDoc.output new file mode 100644 index 000000000..2b5d30db2 --- /dev/null +++ b/open-java-format/src/test/resources/com/palantir/javaformat/java/testdata/MarkdownDoc.output @@ -0,0 +1,10 @@ +/// A greeter. +/// +/// Says hello. See [String]. +class MarkdownDoc { + /// Returns the greeting. + /// @return the text + String greeting() { + return "hi"; + } +} diff --git a/open-java-format/src/test/resources/com/palantir/javaformat/java/testdata/ModuleImport.input b/open-java-format/src/test/resources/com/palantir/javaformat/java/testdata/ModuleImport.input new file mode 100644 index 000000000..63d020f52 --- /dev/null +++ b/open-java-format/src/test/resources/com/palantir/javaformat/java/testdata/ModuleImport.input @@ -0,0 +1,8 @@ +import module java.base; +import module java.sql; + +import java.util.List; + +class ModuleImport { + List xs; +} diff --git a/open-java-format/src/test/resources/com/palantir/javaformat/java/testdata/ModuleImport.output b/open-java-format/src/test/resources/com/palantir/javaformat/java/testdata/ModuleImport.output new file mode 100644 index 000000000..72e62d733 --- /dev/null +++ b/open-java-format/src/test/resources/com/palantir/javaformat/java/testdata/ModuleImport.output @@ -0,0 +1,8 @@ +import module java.base; +import module java.sql; + +import java.util.List; + +class ModuleImport { + List xs; +} diff --git a/open-java-format/src/test/resources/com/palantir/javaformat/java/testdata/UnnamedPattern.input b/open-java-format/src/test/resources/com/palantir/javaformat/java/testdata/UnnamedPattern.input new file mode 100644 index 000000000..79c12d94a --- /dev/null +++ b/open-java-format/src/test/resources/com/palantir/javaformat/java/testdata/UnnamedPattern.input @@ -0,0 +1,34 @@ +class UnnamedPattern { + sealed interface Shape permits Box {} + record Box(Object a, Object b) implements Shape {} + + record Pair(Box first, Object second) {} + + int classify(Shape s) { + return switch (s) { + case Box(Integer i, _) when i > 0 -> i; + case Box(Integer i, _) -> i; + case Box(_, _) -> 0; + }; + } + + int nested(Object o) { + return switch (o) { + case Pair(Box(var _, Integer i), _) -> i; + case Pair(Box(_, _), _) -> 0; + default -> -1; + }; + } + + boolean isBox(Object o) { + return o instanceof Box(Integer _, _); + } + + void swallow(Runnable r) { + try { + r.run(); + } catch (Exception _) { + // ignored + } + } +} diff --git a/open-java-format/src/test/resources/com/palantir/javaformat/java/testdata/UnnamedPattern.output b/open-java-format/src/test/resources/com/palantir/javaformat/java/testdata/UnnamedPattern.output new file mode 100644 index 000000000..21a53185f --- /dev/null +++ b/open-java-format/src/test/resources/com/palantir/javaformat/java/testdata/UnnamedPattern.output @@ -0,0 +1,35 @@ +class UnnamedPattern { + sealed interface Shape permits Box {} + + record Box(Object a, Object b) implements Shape {} + + record Pair(Box first, Object second) {} + + int classify(Shape s) { + return switch (s) { + case Box(Integer i, _) when i > 0 -> i; + case Box(Integer i, _) -> i; + case Box(_, _) -> 0; + }; + } + + int nested(Object o) { + return switch (o) { + case Pair(Box(var _, Integer i), _) -> i; + case Pair(Box(_, _), _) -> 0; + default -> -1; + }; + } + + boolean isBox(Object o) { + return o instanceof Box(Integer _, _); + } + + void swallow(Runnable r) { + try { + r.run(); + } catch (Exception _) { + // ignored + } + } +}