From edce6f5f120c868a33dfeaa09b788383bb32d97b Mon Sep 17 00:00:00 2001 From: Alex Abashev Date: Thu, 12 Nov 2026 23:47:31 +0300 Subject: [PATCH 1/4] Fix the imports after formatting, as the command line does formatSourceAndFixImports, which the Maven plugin and the Spotless step run, ordered the imports and removed the unused ones before formatting. Main formats first and fixes the imports after, because the ranges of --lines and --offset are offsets into the input. The order decides where the comment of a removed import ends up: ImportOrderer moves a comment along with its import, and RemoveUnusedImports leaves it behind. Fixing them first also dropped an extraneous semicolon between imports, where the command line fails the file (#219). formatSourceAndFixImports now fixes the imports after formatting. FormatterServiceImplTest holds the in-process service to what Main prints for whole files. --- .../palantir/javaformat/java/Formatter.java | 10 +- .../java/FormatterServiceImplTest.java | 120 ++++++++++++++++++ .../javaformat/java/FormatterTest.java | 21 +++ 3 files changed, 147 insertions(+), 4 deletions(-) create mode 100644 open-java-format/src/test/java/com/palantir/javaformat/java/FormatterServiceImplTest.java 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 c3da6ce96..712ff2ed8 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 @@ -296,7 +296,8 @@ public String formatSource(String input) throws FormatterException { /** * Formats an input string (a Java compilation unit) and fixes imports. * - *

Fixing imports includes ordering, spacing, and removal of unused import statements. + *

Fixing imports includes ordering, spacing, and removal of unused import statements. Long strings are reflowed + * too, and the result is what {@code open-java-format -} prints with no flags. * * @param input the input string * @return the output string @@ -305,9 +306,10 @@ public String formatSource(String input) throws FormatterException { * Style Guide - 3.3.3 Import ordering and spacing */ public String formatSourceAndFixImports(String input) throws FormatterException { - input = ImportOrderer.reorderImports(input, options.style()); - input = RemoveUnusedImports.removeUnusedImports(input); - String formatted = formatSource(input); + // The imports are fixed after formatting, as the command line fixes them: it has to format first, because the + // ranges it formats are offsets into the input. In the other order the comment of a removed import ends up + // elsewhere, and an extraneous semicolon between imports is dropped where the command line fails the file. + String formatted = fixImports(formatSource(input)); return StringWrapper.wrap(options.maxLineLength(), formatted, this); } diff --git a/open-java-format/src/test/java/com/palantir/javaformat/java/FormatterServiceImplTest.java b/open-java-format/src/test/java/com/palantir/javaformat/java/FormatterServiceImplTest.java new file mode 100644 index 000000000..9dcdfa509 --- /dev/null +++ b/open-java-format/src/test/java/com/palantir/javaformat/java/FormatterServiceImplTest.java @@ -0,0 +1,120 @@ +/* + * 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 java.nio.charset.StandardCharsets.UTF_8; +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +import java.io.ByteArrayInputStream; +import java.io.PrintWriter; +import java.io.StringWriter; +import java.util.stream.Stream; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; + +/** + * The service that formats in the build's own JVM, for {@code formatDiff} without the native binary, the Spotless step + * and the Maven plugin, against the command line, which the bootstrapping and native services run for IntelliJ IDEA + * and {@code formatDiff}, and the GitHub Action runs to check. + */ +final class FormatterServiceImplTest { + private static final FormatterService SERVICE = new FormatterServiceImpl(); + + // Two long strings, as in a file where only the line of the second one has changed. + private static final String LONG_STRINGS = """ + class S { + String outside = "Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat sed."; + + String changed = "Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna."; + } + """; + + static Stream sources() { + return Stream.of( + Arguments.of("long strings", LONG_STRINGS), + Arguments.of("joinable literals", """ + class J { + String joined = "one " + "two"; + String wrapped = "Lorem ipsum dolor sit amet, " + "consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore"; + } + """), + // The comment stays behind when its import is removed, and the order of fixing the imports and + // formatting decides where it ends up. + Arguments.of("trailing comment of an unused import", """ + package p; + + // group one + import java.util.Map; // used + import java.io.File; // unused, trailing comment + /* block between */ + import java.util.List; + + class C { List l; Map m; } + """), + Arguments.of("comment under an unused import", """ + package p; + + import java.io.File; + // comment about the class + class C {} + """)); + } + + @ParameterizedTest(name = "{0}") + @MethodSource("sources") + void formatsAFileAsTheCommandLineDoes(String name, String source) throws Exception { + assertThat(SERVICE.formatSourceReflowStringsAndFixImports(source)).isEqualTo(commandLine(source, "-")); + } + + @Test + void failsOnAnExtraneousSemicolonAsTheCommandLineDoes() throws Exception { + // Fixing the imports first used to drop the semicolon, which javac 21 rejects between imports, and format + // the file that the command line refuses. + String source = """ + package p; + + import java.util.List;; + import java.io.File; + + class C { List l; } + """; + StringWriter err = new StringWriter(); + Main main = new Main( + new PrintWriter(new StringWriter(), true), + new PrintWriter(err, true), + new ByteArrayInputStream(source.getBytes(UTF_8))); + + assertThat(main.format("-")).isNotZero(); + assertThat(err.toString()).contains("extraneous semicolon"); + assertThatThrownBy(() -> SERVICE.formatSourceReflowStringsAndFixImports(source)) + .isInstanceOf(FormatterException.class) + .hasMessageContaining("extraneous semicolon"); + } + + /** What {@code open-java-format} prints for the source on standard input. */ + private static String commandLine(String source, String... args) throws UsageException { + StringWriter out = new StringWriter(); + StringWriter err = new StringWriter(); + Main main = new Main( + new PrintWriter(out, true), + new PrintWriter(err, true), + new ByteArrayInputStream(source.getBytes(UTF_8))); + assertThat(main.format(args)).as(err::toString).isZero(); + return out.toString(); + } +} diff --git a/open-java-format/src/test/java/com/palantir/javaformat/java/FormatterTest.java b/open-java-format/src/test/java/com/palantir/javaformat/java/FormatterTest.java index 973a1a3f6..c0bb55eaa 100644 --- a/open-java-format/src/test/java/com/palantir/javaformat/java/FormatterTest.java +++ b/open-java-format/src/test/java/com/palantir/javaformat/java/FormatterTest.java @@ -276,6 +276,27 @@ class T { assertThat(ImportOrderer.reorderImports(withoutBlank, Style.OJF)).isEqualTo(withoutBlank); } + @Test + void commentUnderARemovedImportStaysOnTheClass() throws FormatterException { + String input = """ + package p; + + import java.io.File; + // comment about the class + class C {} + """; + String output = """ + package p; + + // comment about the class + class C {} + """; + + // The imports are fixed after formatting, as the command line fixes them. Ordering them first left a blank + // line after the import, which stayed between the comment and the class once the import was gone. + assertThat(FORMATTER.formatSourceAndFixImports(input)).isEqualTo(output); + } + @Test void canParse_java9_private_interface_methods() { assertThatCode(() -> FORMATTER.formatSourceAndFixImports("" From 68e5bdf1b4ee2191ddf712b7a7142a4d8f60d1df Mon Sep 17 00:00:00 2001 From: Alex Abashev Date: Thu, 12 Nov 2026 23:47:40 +0300 Subject: [PATCH 2/4] Reflow only the strings in the code that formatting the ranges rewrites --output-replacements, which IntelliJ IDEA and formatDiff with the native binary run, reflowed the whole file once a line in the ranges needed it, and long strings on lines nobody touched changed with it (#219). StringWrapper.wrap takes the ranges and reflows only the literals and text blocks that overlap them. Its passes format and replace text, so the ranges move along with each of them, growing to take in what a replacement rewrites. The ranges it gets are those of the code that formatting the given ranges rewrote, and they also decide whether to reflow at all; that check used to hold the ranges of the input against the lines of the formatted text. Without ranges nothing changes: the whole file is reflowed as before. The method that formats the ranges and reflows them moves to Formatter, for the in-process service to use next. --- .../javaformat/java/FormatFileCallable.java | 22 +--- .../palantir/javaformat/java/Formatter.java | 23 ++++ .../javaformat/java/StringWrapper.java | 124 +++++++++++++++--- .../javaformat/java/StringWrapperTest.java | 55 ++++++++ 4 files changed, 190 insertions(+), 34 deletions(-) diff --git a/open-java-format/src/main/java/com/palantir/javaformat/java/FormatFileCallable.java b/open-java-format/src/main/java/com/palantir/javaformat/java/FormatFileCallable.java index 813e94310..1faeedf1a 100644 --- a/open-java-format/src/main/java/com/palantir/javaformat/java/FormatFileCallable.java +++ b/open-java-format/src/main/java/com/palantir/javaformat/java/FormatFileCallable.java @@ -24,7 +24,6 @@ import com.palantir.javaformat.Utils; import java.io.UncheckedIOException; import java.util.List; -import java.util.Set; import java.util.concurrent.Callable; /** Encapsulates information about a file to be formatted, including which parts of the file to format. */ @@ -51,23 +50,10 @@ public String call() throws FormatterException { Formatter formatter = Formatter.createFormatter(options); if (parameters.outputReplacements()) { RangeSet characterRanges = characterRanges(input); - Set> rangesToChange = characterRanges.asRanges(); - List replacements = formatter.getFormatReplacements(input, rangesToChange); - if (parameters.reflowLongStrings()) { - String formattedText = Utils.applyReplacements(input, replacements); - // avoid trying to wrap the input unless the lines needing wrapping are part of the "characterRanges" - if (!StringWrapper.linesNeedWrapping(options.maxLineLength(), formattedText, characterRanges)) { - return writeFormatReplacements(replacements); - } - String wrappedFormattedText = StringWrapper.wrap(options.maxLineLength(), formattedText, formatter); - // if no wrapping change happened, return the initial replacements - if (wrappedFormattedText.equals(formattedText)) { - return writeFormatReplacements(replacements); - } - return writeFormatReplacements(List.of(Replacement.create(0, input.length(), wrappedFormattedText))); - } else { - return writeFormatReplacements(replacements); - } + return writeFormatReplacements( + parameters.reflowLongStrings() + ? formatter.getFormatAndReflowReplacements(input, characterRanges) + : formatter.getFormatReplacements(input, characterRanges.asRanges())); } return formatFile(formatter); } 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 712ff2ed8..0ea8f0212 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 @@ -366,4 +366,27 @@ public List getFormatReplacements(String input, Collection tokenRangeSet = javaInput.characterRangesToTokenRanges(characterRanges); return javaOutput.getFormatReplacements(tokenRangeSet); } + + /** + * Emit the {@link Replacement}s that format the given character ranges and reflow the long strings in the code + * that formatting them rewrites; the imports, and the strings elsewhere, stay as they are. The command line prints + * these for {@code --output-replacements}, and {@link FormatterServiceImpl#getFormatReplacements} returns them, + * so that {@code formatDiff} and IntelliJ IDEA format the same on the JVM and with the native binary. + */ + List getFormatAndReflowReplacements(String input, RangeSet characterRanges) + throws FormatterException { + List replacements = getFormatReplacements(input, characterRanges.asRanges()); + String formatted = Utils.applyReplacements(input, replacements); + RangeSet formattedRanges = StringWrapper.moveRanges(characterRanges, replacements); + // avoid trying to wrap the input unless the lines needing wrapping are part of the ranges + if (!StringWrapper.linesNeedWrapping(options.maxLineLength(), formatted, formattedRanges)) { + return replacements; + } + String wrapped = StringWrapper.wrap(options.maxLineLength(), formatted, formattedRanges, this); + // if no wrapping change happened, return the initial replacements + if (wrapped.equals(formatted)) { + return replacements; + } + return List.of(Replacement.create(0, input.length(), wrapped)); + } } 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 c6a54eb3a..110a7067f 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 @@ -19,6 +19,7 @@ import com.google.common.base.CharMatcher; import com.google.common.base.Verify; +import com.google.common.collect.DiscreteDomain; import com.google.common.collect.Range; import com.google.common.collect.RangeSet; import com.google.common.collect.TreeRangeMap; @@ -41,6 +42,7 @@ import com.sun.tools.javac.util.Position.LineMap; import java.util.ArrayDeque; import java.util.ArrayList; +import java.util.Collection; import java.util.Collections; import java.util.Comparator; import java.util.Deque; @@ -72,16 +74,30 @@ public final class StringWrapper { * adjacent string literals that share a line. */ static String wrap(final int columnLimit, String input, Formatter formatter) throws FormatterException { - String result = wrapOnce(columnLimit, input, formatter); - for (int round = 1; round < MAX_ROUNDS && !result.equals(input); round++) { + return wrap(columnLimit, new Source(input, null), formatter).text(); + } + + /** + * Reflows, as {@link #wrap(int, String, Formatter)} does, only the string literals and text blocks that overlap + * the given character ranges: when part of a file is formatted, the long strings in the rest of it stay as they + * are. + */ + static String wrap(final int columnLimit, String input, RangeSet characterRanges, Formatter formatter) + throws FormatterException { + return wrap(columnLimit, new Source(input, characterRanges), formatter).text(); + } + + private static Source wrap(final int columnLimit, Source input, Formatter formatter) throws FormatterException { + Source result = wrapOnce(columnLimit, input, formatter); + for (int round = 1; round < MAX_ROUNDS && !result.text().equals(input.text()); round++) { input = result; result = wrapOnce(columnLimit, input, formatter); } return result; } - private static String wrapOnce(final int columnLimit, String input, Formatter formatter) throws FormatterException { - if (!needWrapping(columnLimit, input)) { + private static Source wrapOnce(final int columnLimit, Source input, Formatter formatter) throws FormatterException { + if (!needWrapping(columnLimit, input.text())) { // fast path return input; } @@ -89,36 +105,36 @@ private static String wrapOnce(final int columnLimit, String input, Formatter fo // The pieces continue the way the formatter continues a chain of +: four columns times the multiplier of its // style, eight in OJF. var continuationIndent = 4 * formatter.options().indentationMultiplier(); - TreeRangeMap replacements = getReflowReplacements(columnLimit, continuationIndent, input); + TreeRangeMap replacements = input.reflowReplacements(columnLimit, continuationIndent); if (replacements.asMapOfRanges().isEmpty()) { // needWrapping is an over-approximation, so there may be nothing to do after all. return input; } - String firstPass = - formatter.formatSource(input, replacements.asMapOfRanges().keySet()); + Source firstPass = input.format(formatter, replacements.asMapOfRanges().keySet()); - if (!firstPass.equals(input)) { + if (!firstPass.text().equals(input.text())) { // If formatting the replacement ranges resulted in a change, recalculate the replacements on // the updated input. input = firstPass; - replacements = getReflowReplacements(columnLimit, continuationIndent, input); + replacements = input.reflowReplacements(columnLimit, continuationIndent); } - String result = applyReplacements(input, replacements); + Source result = input.apply(replacements); // Format again, because broken strings might now fit on the first line in case of assignments - String secondPass = formatter.formatSource(result, rangesAfterAppliedReplacements(replacements)); + Source secondPass = result.format(formatter, rangesAfterAppliedReplacements(replacements)); - if (!secondPass.equals(result)) { - replacements = getReflowReplacements(columnLimit, continuationIndent, secondPass); - result = applyReplacements(secondPass, replacements); + if (!secondPass.text().equals(result.text())) { + replacements = secondPass.reflowReplacements(columnLimit, continuationIndent); + result = secondPass.apply(replacements); } { // We really don't want bugs in this pass to change the behaviour of programs we're // formatting, so check that the pretty-printed AST is the same before and after reformatting. - String expected = parse(input, /* allowStringFolding= */ true).toString(); - String actual = parse(result, /* allowStringFolding= */ true).toString(); + String expected = + parse(input.text(), /* allowStringFolding= */ true).toString(); + String actual = parse(result.text(), /* allowStringFolding= */ true).toString(); if (!expected.equals(actual)) { throw new FormatterException(String.format( "Something has gone terribly wrong. Please file a bug: " @@ -131,6 +147,82 @@ private static String wrapOnce(final int columnLimit, String input, Formatter fo return result; } + /** + * The source between two passes of {@link #wrapOnce}, with the character ranges of it that the passes may change: + * the ranges being formatted, moved along with every change, or {@code null} when the whole file is. + */ + private record Source(String text, @Nullable RangeSet ranges) { + + /** The replacements of {@link #getReflowReplacements}, less those for literals outside the ranges. */ + TreeRangeMap reflowReplacements(int columnLimit, int continuationIndent) + throws FormatterException { + TreeRangeMap replacements = getReflowReplacements(columnLimit, continuationIndent, text); + RangeSet within = ranges; + if (within == null) { + return replacements; + } + TreeRangeMap inRanges = TreeRangeMap.create(); + replacements.asMapOfRanges().forEach((literal, replacement) -> { + if (within.intersects(literal)) { + inRanges.put(literal, replacement); + } + }); + return inRanges; + } + + /** Formats the given ranges, as {@link Formatter#formatSource(String, Collection)} does. */ + Source format(Formatter formatter, Collection> rangesToFormat) throws FormatterException { + List replacements = formatter.getFormatReplacements(text, rangesToFormat); + return new Source(Utils.applyReplacements(text, replacements), moved(replacements)); + } + + Source apply(TreeRangeMap replacements) { + List applied = replacements.asMapOfRanges().entrySet().stream() + .map(entry -> Replacement.create( + entry.getKey().lowerEndpoint(), entry.getKey().upperEndpoint(), entry.getValue())) + .toList(); + return new Source(applyReplacements(text, replacements), moved(applied)); + } + + private @Nullable RangeSet moved(List replacements) { + return ranges == null ? null : moveRanges(ranges, replacements); + } + } + + /** + * Where the given character ranges are once the replacements, sorted and without overlaps, have been applied. A + * range grows to take in all of a replacement that overlaps it, or that inserts text at its start. + */ + static RangeSet moveRanges(RangeSet ranges, List replacements) { + RangeSet moved = TreeRangeSet.create(); + for (Range range : ranges.asRanges()) { + Range canonical = range.canonical(DiscreteDomain.integers()); + moved.add(Range.closedOpen( + movePosition(canonical.lowerEndpoint(), replacements, /* start= */ true), + movePosition(canonical.upperEndpoint(), replacements, /* start= */ false))); + } + return moved; + } + + private static int movePosition(int position, List replacements, boolean start) { + int shift = 0; + for (Replacement replacement : replacements) { + int lower = replacement.getReplaceRange().lowerEndpoint(); + int upper = replacement.getReplaceRange().upperEndpoint(); + int length = replacement.getReplacementString().length(); + if (upper < position || (upper == position && lower < position)) { + // Before the position. + shift += length - (upper - lower); + } else if (lower < position) { + // Around the position, which moves to the replacement's start or end. + return lower + shift + (start ? 0 : length); + } else { + break; + } + } + return position + shift; + } + private static Set> rangesAfterAppliedReplacements(TreeRangeMap replacements) { var outputRanges = new LinkedHashSet>(); int offset = 0; diff --git a/open-java-format/src/test/java/com/palantir/javaformat/java/StringWrapperTest.java b/open-java-format/src/test/java/com/palantir/javaformat/java/StringWrapperTest.java index 1379bd871..34e466178 100644 --- a/open-java-format/src/test/java/com/palantir/javaformat/java/StringWrapperTest.java +++ b/open-java-format/src/test/java/com/palantir/javaformat/java/StringWrapperTest.java @@ -19,6 +19,9 @@ import com.google.common.base.Joiner; import com.google.common.collect.Iterables; +import com.google.common.collect.Range; +import com.google.common.collect.RangeSet; +import com.google.common.collect.TreeRangeSet; import com.sun.source.tree.BinaryTree; import com.sun.source.tree.ClassTree; import com.sun.source.tree.CompilationUnitTree; @@ -29,6 +32,7 @@ import java.io.IOException; import java.net.URI; import java.util.List; +import java.util.Set; import javax.tools.JavaFileObject; import javax.tools.SimpleJavaFileObject; import javax.tools.ToolProvider; @@ -94,6 +98,57 @@ void wrappingKeepsTheValueOfAStringWithEscapes() throws Exception { assertThat(stringValue(output)).isEqualTo(stringValue(input)); } + @Test + void wrapsOnlyTheStringsInTheRanges() throws Exception { + // As when one line of the file is formatted: the long string on the other line stays as it is. + String input = """ + class T { + String outside = "one long incredibly unbroken sentence moving from topic to topic so that no one had a chance"; + String inside = "one long incredibly unbroken sentence moving from topic to topic so that no one had a chance"; + } + """; + String output = """ + class T { + String outside = "one long incredibly unbroken sentence moving from topic to topic so that no one had a chance"; + String inside = + "one long incredibly unbroken sentence moving from topic to topic so that no one had a" + + " chance"; + } + """; + int inside = input.indexOf(" String inside"); + RangeSet insideLine = + TreeRangeSet.create(List.of(Range.closedOpen(inside, input.indexOf('\n', inside)))); + RangeSet wholeFile = TreeRangeSet.create(List.of(Range.closedOpen(0, input.length()))); + + assertThat(StringWrapper.wrap(100, input, insideLine, Formatter.create())) + .isEqualTo(output); + assertThat(StringWrapper.wrap(100, input, wholeFile, Formatter.create())) + .isEqualTo(StringWrapper.wrap(100, input, Formatter.create())); + } + + @Test + void movesRangesAlongWithTheReplacements() { + // Characters 4 to 8 become two, and two are inserted at 12. + List replacements = List.of(Replacement.create(4, 8, "ab"), Replacement.create(12, 12, "cd")); + + // Before the replacements, and after the first one. + assertThat(moved(Range.closedOpen(0, 2), replacements)).containsExactly(Range.closedOpen(0, 2)); + assertThat(moved(Range.closedOpen(9, 11), replacements)).containsExactly(Range.closedOpen(7, 9)); + // Over the start or the end of a replacement: the range takes in all of it. + assertThat(moved(Range.closedOpen(2, 5), replacements)).containsExactly(Range.closedOpen(2, 6)); + assertThat(moved(Range.closedOpen(6, 10), replacements)).containsExactly(Range.closedOpen(4, 8)); + // Text inserted at the start of a range goes in, at its end it does not. + assertThat(moved(Range.closedOpen(12, 14), replacements)).containsExactly(Range.closedOpen(10, 14)); + assertThat(moved(Range.closedOpen(10, 12), replacements)).containsExactly(Range.closedOpen(8, 10)); + // A closed range moves as the half-open one with the same characters. + assertThat(moved(Range.closed(9, 10), replacements)).containsExactly(Range.closedOpen(7, 9)); + } + + private static Set> moved(Range range, List replacements) { + return StringWrapper.moveRanges(TreeRangeSet.create(List.of(range)), replacements) + .asRanges(); + } + /** The value javac gives the initializer of the only field: a string literal, or literals joined with +. */ private static String stringValue(String source) throws IOException { JavaFileObject file = new SimpleJavaFileObject(URI.create("string:///T.java"), JavaFileObject.Kind.SOURCE) { From b445927d718798484a4eb404a127322ef8e3ba83 Mon Sep 17 00:00:00 2001 From: Alex Abashev Date: Thu, 12 Nov 2026 23:47:52 +0300 Subject: [PATCH 3/4] Reflow long strings in formatDiff on the JVM, as with the native binary The Gradle plugin loads FormatterServiceImpl into its own JVM, and its getFormatReplacements never reflowed strings, while the bootstrapping and native services run Main --output-replacements, which does. formatDiff without the native binary left long strings on one line, and the GitHub Action failed on the result (#219). FormatterServiceImpl.getFormatReplacements now returns what --output-replacements prints. FormatterServiceImplTest compares the two for each line of its sources, and FormatterServicesTest and FormatDiffTest run the in-process formatter next to the bootstrapping and native ones, which they never did. --- .../javaformat/gradle/FormatDiffTest.java | 55 ++++++++++++++++-- .../bootstrap/FormatterServicesTest.java | 35 +++++++++++ .../javaformat/java/FormatterService.java | 3 + .../javaformat/java/FormatterServiceImpl.java | 4 +- .../java/FormatterServiceImplTest.java | 58 +++++++++++++++++++ 5 files changed, 149 insertions(+), 6 deletions(-) diff --git a/gradle-open-java-format/src/test/java/com/palantir/javaformat/gradle/FormatDiffTest.java b/gradle-open-java-format/src/test/java/com/palantir/javaformat/gradle/FormatDiffTest.java index a0ebb03d2..e90808a37 100644 --- a/gradle-open-java-format/src/test/java/com/palantir/javaformat/gradle/FormatDiffTest.java +++ b/gradle-open-java-format/src/test/java/com/palantir/javaformat/gradle/FormatDiffTest.java @@ -69,11 +69,7 @@ void parsing_git_diff_output_works() throws IOException { @MethodSource("getFormatters") void reformat_a_subpath_of_a_git_directory_for_only_changed_lines(FormatterService formatterService) throws IOException, InterruptedException { - runCommandInRepo("git", "init"); - runCommandInRepo("git", "config", "user.name", "Test User"); - runCommandInRepo("git", "config", "user.email", "test-user@palantir.com"); - runCommandInRepo("git", "config", "commit.gpgsign", "false"); - runCommandInRepo("git", "commit", "--allow-empty", "-m", "Init"); + initRepo(); Path subdir = repo.resolve("subdir"); Files.createDirectories(subdir); @@ -92,6 +88,52 @@ void reformat_a_subpath_of_a_git_directory_for_only_changed_lines(FormatterServi assertThat(dontTouchMe).hasContent(" class DontTouchMe {}"); } + @ParameterizedTest + @MethodSource("getFormatters") + void reflows_the_long_strings_of_changed_lines_only(FormatterService formatterService) + throws IOException, InterruptedException { + initRepo(); + String outside = " String outside = \"Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris" + + " nisi ut aliquip ex ea commodo consequat sed.\";"; + Path longStrings = repo.resolve("LongStrings.java"); + Files.write(longStrings, List.of("class LongStrings {", outside, "}"), UTF_8); + runCommandInRepo("git", "add", "."); + runCommandInRepo("git", "commit", "-m", "A long string"); + + Files.write( + longStrings, + List.of( + "class LongStrings {", + outside, + "", + " String changed = \"Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod" + + " tempor incididunt ut labore et dolore magna.\";", + "}"), + UTF_8); + + FormatDiff.formatDiff(repo, formatterService); + + assertThat(longStrings) + .hasContent(String.join( + "\n", + "class LongStrings {", + outside, + "", + " String changed =", + " \"Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor" + + " incididunt ut labore et\"", + " + \" dolore magna.\";", + "}")); + } + + private void initRepo() throws IOException, InterruptedException { + runCommandInRepo("git", "init"); + runCommandInRepo("git", "config", "user.name", "Test User"); + runCommandInRepo("git", "config", "user.email", "test-user@palantir.com"); + runCommandInRepo("git", "config", "commit.gpgsign", "false"); + runCommandInRepo("git", "commit", "--allow-empty", "-m", "Init"); + } + private void runCommandInRepo(String... args) throws IOException, InterruptedException { Process process = new ProcessBuilder().command(args).directory(repo.toFile()).start(); @@ -107,6 +149,9 @@ private void runCommandInRepo(String... args) throws IOException, InterruptedExc private static Stream getFormatters() throws IOException { return Stream.of( + // In this JVM, as formatDiff runs it without the native binary. + FormatterServiceLoader.load( + getClasspath().stream().map(Path::toFile).toList()), new BootstrappingFormatterService( javaBinPath(), Runtime.version().feature(), getClasspath()), new NativeImageFormatterService( diff --git a/open-java-format-jdk-bootstrap/src/test/java/com/palantir/javaformat/bootstrap/FormatterServicesTest.java b/open-java-format-jdk-bootstrap/src/test/java/com/palantir/javaformat/bootstrap/FormatterServicesTest.java index 5821fade2..1ed2b5a2c 100644 --- a/open-java-format-jdk-bootstrap/src/test/java/com/palantir/javaformat/bootstrap/FormatterServicesTest.java +++ b/open-java-format-jdk-bootstrap/src/test/java/com/palantir/javaformat/bootstrap/FormatterServicesTest.java @@ -24,6 +24,7 @@ import com.palantir.javaformat.Utils; import com.palantir.javaformat.java.FormatterException; import com.palantir.javaformat.java.FormatterService; +import com.palantir.javaformat.java.FormatterServiceImpl; import com.palantir.javaformat.java.Replacement; import java.io.File; import java.net.URI; @@ -96,6 +97,34 @@ public void main( String aaaa ) { .isEmpty(); } + @ParameterizedTest + @MethodSource("getFormattersAndTheInProcessOne") + void reflows_the_long_strings_of_the_ranges_only(FormatterService formatterService) throws FormatterException { + String input = """ + class S { + String outside = "Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat sed."; + + String changed = "Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna."; + } + """; + String expectedOutput = """ + class S { + String outside = "Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat sed."; + + String changed = + "Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et" + + " dolore magna."; + } + """; + int start = input.indexOf(" String changed"); + int end = input.indexOf('\n', start); + + List replacements = + formatterService.getFormatReplacements(input, List.of(Range.closedOpen(start, end))); + + assertThat(Utils.applyReplacements(input, replacements)).isEqualTo(expectedOutput); + } + private static Stream getFormatters() { return Stream.of( new BootstrappingFormatterService( @@ -103,6 +132,12 @@ private static Stream getFormatters() { new NativeImageFormatterService(Path.of(System.getenv("NATIVE_IMAGE_CLASSPATH")))); } + // With the one that formats in this JVM, as formatDiff does without the native binary. On input that does not parse + // it throws, where the other two return no replacements, so it is not among getFormatters. + private static Stream getFormattersAndTheInProcessOne() { + return Stream.concat(Stream.of(new FormatterServiceImpl()), getFormatters()); + } + private String getTestResourceContent(String resourceName) { try { URI resource = getClass().getClassLoader().getResource(resourceName).toURI(); diff --git a/open-java-format-spi/src/main/java/com/palantir/javaformat/java/FormatterService.java b/open-java-format-spi/src/main/java/com/palantir/javaformat/java/FormatterService.java index 292549467..a314ac57a 100644 --- a/open-java-format-spi/src/main/java/com/palantir/javaformat/java/FormatterService.java +++ b/open-java-format-spi/src/main/java/com/palantir/javaformat/java/FormatterService.java @@ -28,6 +28,9 @@ public interface FormatterService { /** * Emit a list of {@link Replacement}s to convert from input to formatted output. * + *

The ranges are formatted, and the long strings in the code that formatting them rewrites are reflowed; the + * imports, and the strings elsewhere, stay as they are. + * * @param input the input compilation unit * @param ranges the character ranges to reformat * @return a list of {@link Replacement}s, sorted from low index to high index, without overlaps diff --git a/open-java-format/src/main/java/com/palantir/javaformat/java/FormatterServiceImpl.java b/open-java-format/src/main/java/com/palantir/javaformat/java/FormatterServiceImpl.java index 08681da2e..41644d0f1 100644 --- a/open-java-format/src/main/java/com/palantir/javaformat/java/FormatterServiceImpl.java +++ b/open-java-format/src/main/java/com/palantir/javaformat/java/FormatterServiceImpl.java @@ -18,6 +18,7 @@ import com.google.auto.service.AutoService; import com.google.common.collect.ImmutableList; import com.google.common.collect.Range; +import com.google.common.collect.TreeRangeSet; import com.palantir.javaformat.java.JavaFormatterOptions.Style; import java.util.Collection; @@ -36,7 +37,8 @@ public FormatterServiceImpl() { @SuppressForbidden(reason = "FormatterService, kept in the SPI for compatibility, returns ImmutableList") public ImmutableList getFormatReplacements(String text, Collection> toRanges) throws FormatterException { - return ImmutableList.copyOf(formatter.getFormatReplacements(text, toRanges)); + // What the command line prints for --output-replacements, which the bootstrapping and native services run. + return ImmutableList.copyOf(formatter.getFormatAndReflowReplacements(text, TreeRangeSet.create(toRanges))); } @Override diff --git a/open-java-format/src/test/java/com/palantir/javaformat/java/FormatterServiceImplTest.java b/open-java-format/src/test/java/com/palantir/javaformat/java/FormatterServiceImplTest.java index 9dcdfa509..d1f17f122 100644 --- a/open-java-format/src/test/java/com/palantir/javaformat/java/FormatterServiceImplTest.java +++ b/open-java-format/src/test/java/com/palantir/javaformat/java/FormatterServiceImplTest.java @@ -18,9 +18,16 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; +import com.fasterxml.jackson.core.type.TypeReference; +import com.fasterxml.jackson.databind.ObjectMapper; +import com.fasterxml.jackson.databind.json.JsonMapper; +import com.fasterxml.jackson.datatype.guava.GuavaModule; +import com.google.common.collect.Range; +import com.palantir.javaformat.Utils; import java.io.ByteArrayInputStream; import java.io.PrintWriter; import java.io.StringWriter; +import java.util.List; import java.util.stream.Stream; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; @@ -34,6 +41,8 @@ */ final class FormatterServiceImplTest { private static final FormatterService SERVICE = new FormatterServiceImpl(); + private static final ObjectMapper MAPPER = + JsonMapper.builder().addModule(new GuavaModule()).build(); // Two long strings, as in a file where only the line of the second one has changed. private static final String LONG_STRINGS = """ @@ -81,6 +90,55 @@ void formatsAFileAsTheCommandLineDoes(String name, String source) throws Excepti assertThat(SERVICE.formatSourceReflowStringsAndFixImports(source)).isEqualTo(commandLine(source, "-")); } + @ParameterizedTest(name = "{0}") + @MethodSource("sources") + void replacesEachLineAsTheCommandLineDoes(String name, String source) throws Exception { + int start = 0; + for (String line : source.split("\n", -1)) { + int end = start + line.length(); + if (!line.isBlank()) { + List printed = MAPPER.readValue( + commandLine( + source, "--character-ranges", start + ":" + (end - 1), "--output-replacements", "-"), + new TypeReference<>() {}); + assertThat(SERVICE.getFormatReplacements(source, List.of(Range.closedOpen(start, end)))) + .as(line) + .isEqualTo(printed); + } + start = end + 1; + } + } + + @Test + void reflowsTheLongStringOfAChangedLineOnly() throws Exception { + String changed = LONG_STRINGS + .lines() + .filter(line -> line.contains("changed")) + .findFirst() + .orElseThrow(); + int start = LONG_STRINGS.indexOf(changed); + int end = start + changed.length(); + + List replacements = + SERVICE.getFormatReplacements(LONG_STRINGS, List.of(Range.closedOpen(start, end))); + // The command line used to reflow every long string in the file once one in the ranges needed it. + List printed = MAPPER.readValue( + commandLine(LONG_STRINGS, "--character-ranges", start + ":" + (end - 1), "--output-replacements", "-"), + new TypeReference<>() {}); + + assertThat(Utils.applyReplacements(LONG_STRINGS, printed)) + .isEqualTo(Utils.applyReplacements(LONG_STRINGS, replacements)); + assertThat(Utils.applyReplacements(LONG_STRINGS, replacements)).isEqualTo(""" + class S { + String outside = "Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat sed."; + + String changed = + "Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et" + + " dolore magna."; + } + """); + } + @Test void failsOnAnExtraneousSemicolonAsTheCommandLineDoes() throws Exception { // Fixing the imports first used to drop the semicolon, which javac 21 rejects between imports, and format From b06ccee5a4656924b6298f06a2f64e283c8661a6 Mon Sep 17 00:00:00 2001 From: Alex Abashev Date: Fri, 13 Nov 2026 00:27:30 +0300 Subject: [PATCH 4/4] Expect what the command line prints for a comment in a module name ModuleImportTest held formatSourceAndFixImports to what it wrote when it fixed the imports before formatting: the formatter broke "import module java./* comment */ base;" after the dot, and the line stayed broken. The command line fixes the imports after formatting, where ImportOrderer joins the line again, and formatSourceAndFixImports now does the same (#219). The test runs on JDK 25 and later only, where import module parses, so the JDK 21 build passed without this and the JDK 25 to 28 builds failed. --- .../palantir/javaformat/java/ModuleImportTest.java | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) 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 index 96ebbede8..6a390be4b 100644 --- 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 @@ -115,16 +115,10 @@ class Example {} 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. - /* comment */ base; - - class Example {} - """; - assertThat(Formatter.create().formatSourceAndFixImports(input)).isEqualTo(formatted); - assertThat(Formatter.create().formatSourceAndFixImports(formatted)).isEqualTo(formatted); + // Formatting breaks the line after the dot, which is where the formatter puts a comment in a + // qualified name, and reordering, which comes after it as on the command line, joins it again. + assertThat(Formatter.create().formatSourceAndFixImports(input)).isEqualTo(reordered); + assertThat(Formatter.create().formatSourceAndFixImports(reordered)).isEqualTo(reordered); } @Test