diff --git a/idea-plugin/src/main/java/com/palantir/javaformat/intellij/PalantirJavaFormatFormattingService.java b/idea-plugin/src/main/java/com/palantir/javaformat/intellij/PalantirJavaFormatFormattingService.java index 6c3d8631f..2a02a5a8d 100644 --- a/idea-plugin/src/main/java/com/palantir/javaformat/intellij/PalantirJavaFormatFormattingService.java +++ b/idea-plugin/src/main/java/com/palantir/javaformat/intellij/PalantirJavaFormatFormattingService.java @@ -36,7 +36,6 @@ import java.util.List; import java.util.Optional; import java.util.Set; -import java.util.stream.Collectors; import org.jetbrains.annotations.NotNull; class PalantirJavaFormatFormattingService extends AsyncDocumentFormattingService { @@ -91,6 +90,8 @@ public void run() { return; } + String preFormatText = request.getDocumentText(); + try { if (logger.isDebugEnabled()) { logger.debug(String.format( @@ -98,21 +99,23 @@ public void run() { Optional.ofNullable(request.getIOFile()) .map(file -> file.toPath().toString()) .orElse("null"), - request.getDocumentText().length(), + preFormatText.length(), request.getFormattingRanges())); } - List replacements = - formatterService.get().getFormatReplacements(request.getDocumentText(), toRanges(request)); - if (logger.isDebugEnabled()) { - logger.debug(String.format( - "Applying %s replacements with ranges=%s", - replacements.size(), - replacements.stream() - .map(Replacement::getReplaceRange) - .collect(Collectors.toSet()))); + + // Note: I attempted to implement this using getFormatReplacements and elide an update when + // there were no replacements. There is a bug in the underlying formatter where it _always_ + // returns a change. Thus we must perform a content aware diff / branching. + String formattedText = applyReplacements( + preFormatText, formatterService.get().getFormatReplacements(preFormatText, toRanges(request))); + + // The Javadoc of this API says that you should set it to null when the document is unchanged + // We should not be trying to format a document that is already formatted + if (preFormatText.equals(formattedText)) { + request.onTextReady(null); + } else { + request.onTextReady(formattedText); } - String formattedText = applyReplacements(request.getDocumentText(), replacements); - request.onTextReady(formattedText); } catch (FormatterException e) { logger.error( String.format( diff --git a/palantir-java-format/src/test/java/com/palantir/javaformat/java/FormatterTest.java b/palantir-java-format/src/test/java/com/palantir/javaformat/java/FormatterTest.java index 7bf54dc38..1d7066741 100644 --- a/palantir-java-format/src/test/java/com/palantir/javaformat/java/FormatterTest.java +++ b/palantir-java-format/src/test/java/com/palantir/javaformat/java/FormatterTest.java @@ -21,6 +21,7 @@ import static org.assertj.core.api.Assertions.assertThatThrownBy; import com.google.common.base.Joiner; +import com.google.common.collect.Range; import com.google.common.io.CharStreams; import java.io.ByteArrayInputStream; import java.io.IOException; @@ -31,6 +32,7 @@ import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.nio.file.Path; +import java.util.List; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.io.TempDir; import org.junit.jupiter.api.parallel.Execution; @@ -452,4 +454,36 @@ void canParse_java9_private_interface_methods() { + "}")) .doesNotThrowAnyException(); } + + @Test + void producesFormattingChangesOnAlreadyFormattedFiles() throws FormatterException { + Formatter formatter = Formatter.create(); + String simpleClass = "package com.palantir;\n" + + "\n" + + "public final class Formatted {\n" + + " public static int count(int number) {\n" + + " return number;\n" + + " }\n" + + "}"; + // This is kinda overkill but the point of the test is to show that it + // is for sure formatted and still producing a replacement. + String formattedClass = formatter.formatSourceAndFixImports(simpleClass); + String reformattedClass = formatter.formatSourceAndFixImports(formattedClass); + + // The formatSourceAndFixImports code path flows through getFormatReplacements and + // this shows that despite the replacements happening they are superfluous + assertThat(formattedClass).isEqualTo(reformattedClass); + + assertWithMessage(""" + If this test is failing and you are reading this message it means that an underlying bug + in the formatting service was fixed. Previously, this formatter always produced a "replacement" + even when the document was fully formatted. You can see evidence of this in + https://github.com/palantir/palantir-java-format/pull/1188. To fix this failing test, consider + relying on the new behavior and instead checking that the 'replacements' is empty instead of + full string comparison. + """) + .that(formatter.getFormatReplacements( + formattedClass, List.of(Range.closedOpen(0, formattedClass.length())))) + .isNotEmpty(); + } }