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/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 c3da6ce96..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 @@ -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); } @@ -364,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/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/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/FormatterServiceImplTest.java b/open-java-format/src/test/java/com/palantir/javaformat/java/FormatterServiceImplTest.java new file mode 100644 index 000000000..d1f17f122 --- /dev/null +++ b/open-java-format/src/test/java/com/palantir/javaformat/java/FormatterServiceImplTest.java @@ -0,0 +1,178 @@ +/* + * 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 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; +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(); + 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 = """ + 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, "-")); + } + + @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 + // 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("" 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 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) {