From a86e56c08a17b4bffbad31c58244d80b99cccfba Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Thu, 30 Apr 2020 14:21:08 -0700 Subject: [PATCH 01/18] Remove `paddedCell` option throughout Spotless, and deprecate its getters and setters. --- .../extra/integration/DiffMessageFormatter.java | 10 +++------- .../diffplug/gradle/spotless/FormatExtension.java | 7 +++---- .../com/diffplug/gradle/spotless/SpotlessTask.java | 12 +++++------- 3 files changed, 11 insertions(+), 18 deletions(-) diff --git a/lib-extra/src/main/java/com/diffplug/spotless/extra/integration/DiffMessageFormatter.java b/lib-extra/src/main/java/com/diffplug/spotless/extra/integration/DiffMessageFormatter.java index 398ccca1ad..d2d86ff7db 100644 --- a/lib-extra/src/main/java/com/diffplug/spotless/extra/integration/DiffMessageFormatter.java +++ b/lib-extra/src/main/java/com/diffplug/spotless/extra/integration/DiffMessageFormatter.java @@ -48,7 +48,6 @@ public static class Builder { private Builder() {} private String runToFix; - private boolean isPaddedCell; private Formatter formatter; private List problemFiles; @@ -58,8 +57,9 @@ public Builder runToFix(String runToFix) { return this; } + @Deprecated public Builder isPaddedCell(boolean isPaddedCell) { - this.isPaddedCell = isPaddedCell; + System.err.println("PaddedCell is now always on, and cannot be turned off."); return this; } @@ -171,11 +171,7 @@ private static String diff(Builder builder, File file) throws IOException { String raw = new String(Files.readAllBytes(file.toPath()), builder.formatter.getEncoding()); String rawUnix = LineEnding.toUnix(raw); String formattedUnix; - if (builder.isPaddedCell) { - formattedUnix = PaddedCell.check(builder.formatter, file, rawUnix).canonical(); - } else { - formattedUnix = builder.formatter.compute(rawUnix, file); - } + formattedUnix = PaddedCell.check(builder.formatter, file, rawUnix).canonical(); if (rawUnix.equals(formattedUnix)) { // the formatting is fine, so it's a line-ending issue diff --git a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/FormatExtension.java b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/FormatExtension.java index b771c78e58..c193fbb8ca 100644 --- a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/FormatExtension.java +++ b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/FormatExtension.java @@ -67,16 +67,16 @@ private String formatName() { throw new IllegalStateException("This format is not contained by any SpotlessExtension."); } - boolean paddedCell = false; - /** Enables paddedCell mode. @see Padded cell */ + @Deprecated public void paddedCell() { paddedCell(true); } /** Enables paddedCell mode. @see Padded cell */ + @Deprecated public void paddedCell(boolean paddedCell) { - this.paddedCell = paddedCell; + root.project.getLogger().warn("PaddedCell is now always on, and cannot be turned off."); } LineEnding lineEndings; @@ -593,7 +593,6 @@ public EclipseWtpConfig eclipseWtp(EclipseWtpFormatterStep type, String version) /** Sets up a format task according to the values in this extension. */ protected void setupTask(SpotlessTask task) { - task.setPaddedCell(paddedCell); task.setEncoding(getEncoding().name()); task.setExceptionPolicy(exceptionPolicy); if (targetExclude == null) { diff --git a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTask.java b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTask.java index 1cca30ec5e..86d79c1e02 100644 --- a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTask.java +++ b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTask.java @@ -74,16 +74,15 @@ public void setLineEndingsPolicy(LineEnding.Policy lineEndingsPolicy) { this.lineEndingsPolicy = Objects.requireNonNull(lineEndingsPolicy); } - // set by FormatExtension - protected boolean paddedCell = false; - - @Input + @Deprecated + @Internal public boolean isPaddedCell() { - return paddedCell; + return true; } + @Deprecated public void setPaddedCell(boolean paddedCell) { - this.paddedCell = paddedCell; + getLogger().warn("PaddedCell is now always on, and cannot be turned off."); } protected String filePatterns = ""; @@ -330,7 +329,6 @@ private void check(Formatter formatter, List outOfDate) throws Exception { GradleException formatViolationsFor(Formatter formatter, List problemFiles) { return new GradleException(DiffMessageFormatter.builder() .runToFix("Run 'gradlew spotlessApply' to fix these violations.") - .isPaddedCell(paddedCell) .formatter(formatter) .problemFiles(problemFiles) .getMessage()); From eeb85ba02b8ec97f4872055a1f0de997a4f803fd Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Thu, 30 Apr 2020 15:09:02 -0700 Subject: [PATCH 02/18] Update documentation for the always-paddedcell future. --- PADDEDCELL.md | 17 +---------------- plugin-gradle/CHANGES.md | 2 ++ plugin-gradle/README.md | 3 --- 3 files changed, 3 insertions(+), 19 deletions(-) diff --git a/PADDEDCELL.md b/PADDEDCELL.md index 54bb70b321..cd4476d46c 100644 --- a/PADDEDCELL.md +++ b/PADDEDCELL.md @@ -1,19 +1,4 @@ -# You have a misbehaving rule that needs a `paddedCell()` - -`spotlessCheck` has detected that one of your rules is misbehaving. This will cause `spotlessCheck` to fail even after you have called `spotlessApply`. To bandaid over this problem, add `paddedCell()` to your `build.gradle`, as such: - -```gradle -spotless { - java { - ... - paddedCell() - } -} -``` - -This is not a bug in Spotless itself, but in one of the third-party formatters, such as the [eclipse formatter](https://bugs.eclipse.org/bugs/show_bug.cgi?id=310642), [google-java-format](https://github.com/google/google-java-format/issues), or some custom rule. - -`paddedCell()` will ensure that your code continues to be formatted, although it will be a little slower. Now when you run `spotlessCheck`, it will generate helpful bug report files in the `build/spotless-diagnose-` folder which will contain the states that your rules are fighting over. These files are very helpful to the developers of the code formatter you are using. +# PaddedCell ## How spotless works diff --git a/plugin-gradle/CHANGES.md b/plugin-gradle/CHANGES.md index cce7527784..09f2f6b840 100644 --- a/plugin-gradle/CHANGES.md +++ b/plugin-gradle/CHANGES.md @@ -3,6 +3,8 @@ We adhere to the [keepachangelog](https://keepachangelog.com/en/1.0.0/) format (starting after version `3.27.0`). ## [Unreleased] +### Changed +* PaddedCell is now always enabled. It is strictly better than non-padded cell, and there is no performance penalty. [See here](https://github.com/diffplug/spotless/pull/560#issuecomment-621752798) for detailed explanation. ## [3.28.1] - 2020-04-02 ### Fixed diff --git a/plugin-gradle/README.md b/plugin-gradle/README.md index 9517fc4623..ebe9e958d8 100644 --- a/plugin-gradle/README.md +++ b/plugin-gradle/README.md @@ -168,8 +168,6 @@ Configuration for Groovy is similar to [Java](#java). Most java steps, like `li The groovy formatter's default behavior is to format all `.groovy` and `.java` files found in the Groovy source directories. If you would like to exclude the `.java` files, set the parameter `excludeJava`, or you can set the `target` parameter as described in the [Custom rules](#custom) section. -Due to cyclic ambiguities of groovy formatter results, e.g. for nested closures, the use of [paddedCell()](../PADDEDCELL.md) and/or [Custom rules](#custom) is recommended to bandaid over this third-party formatter problem. - ```gradle apply plugin: 'groovy' ... @@ -182,7 +180,6 @@ spotless { groovy { licenseHeaderFile 'spotless.license.java' excludeJava() // excludes all Java sources within the Groovy source dirs from formatting - paddedCell() // Avoid cyclic ambiguities // the Groovy Eclipse formatter extends the Java Eclipse formatter, // so it formats Java files by default (unless `excludeJava` is used). greclipse().configFile('greclipse.properties') From ed81aad306d0347a72714fb477b5a85da6a3749f Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Thu, 30 Apr 2020 15:13:45 -0700 Subject: [PATCH 03/18] Fix compile-errors in the "remove paddedcell" adventure. --- .../gradle/spotless/PaddedCellGradle.java | 27 --------- .../gradle/spotless/SpotlessTask.java | 56 ++----------------- 2 files changed, 5 insertions(+), 78 deletions(-) diff --git a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/PaddedCellGradle.java b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/PaddedCellGradle.java index 6c69487054..409cda7f74 100644 --- a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/PaddedCellGradle.java +++ b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/PaddedCellGradle.java @@ -17,11 +17,8 @@ import java.io.File; import java.io.IOException; -import java.nio.file.Path; import java.util.List; -import org.gradle.api.GradleException; - import com.diffplug.common.base.StringPrinter; import com.diffplug.spotless.Formatter; import com.diffplug.spotless.PaddedCellBulk; @@ -55,30 +52,6 @@ class PaddedCellGradle { /** URL to a page which describes the padded cell thing. */ private static final String URL = "https://github.com/diffplug/spotless/blob/master/PADDEDCELL.md"; - static GradleException youShouldTurnOnPaddedCell(SpotlessTask task) { - Path rootPath = task.getProject().getRootDir().toPath(); - return new GradleException(StringPrinter.buildStringFromLines( - "You have a misbehaving rule which can't make up its mind.", - "This means that spotlessCheck will fail even after spotlessApply has run.", - "", - "This is a bug in a formatting rule, not Spotless itself, but Spotless can", - "work around this bug and generate helpful bug reports for the broken rule", - "if you add 'paddedCell()' to your build.gradle as such: ", - "", - " spotless {", - " format 'someFormat', {", - " ...", - " paddedCell()", - " }", - " }", - "", - "The next time you run spotlessCheck, it will put helpful bug reports into", - "'" + rootPath.relativize(diagnoseDir(task).toPath()) + "', and spotlessApply", - "and spotlessCheck will be self-consistent from here on out.", - "", - "For details see " + URL)); - } - private static File diagnoseDir(SpotlessTask task) { return new File(task.getProject().getBuildDir(), "spotless-diagnose-" + task.formatName()); } diff --git a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTask.java b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTask.java index 86d79c1e02..ca471ababf 100644 --- a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTask.java +++ b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTask.java @@ -18,8 +18,6 @@ import java.io.File; import java.io.Serializable; import java.nio.charset.Charset; -import java.nio.file.Files; -import java.nio.file.StandardOpenOption; import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; @@ -46,7 +44,6 @@ import com.diffplug.spotless.Formatter; import com.diffplug.spotless.FormatterStep; import com.diffplug.spotless.LineEnding; -import com.diffplug.spotless.PaddedCell; import com.diffplug.spotless.PaddedCellBulk; import com.diffplug.spotless.extra.integration.DiffMessageFormatter; @@ -262,42 +259,10 @@ static class LastApply implements Serializable { private List applyAnyChanged(Formatter formatter, List outOfDate) throws Exception { List changed = new ArrayList<>(outOfDate.size()); - if (isPaddedCell()) { - for (File file : outOfDate) { - getLogger().debug("Applying format to " + file); - if (PaddedCellBulk.applyAnyChanged(formatter, file)) { - changed.add(file); - } - } - } else { - boolean anyMisbehave = false; - for (File file : outOfDate) { - getLogger().debug("Applying format to " + file); - String unixResultIfDirty = formatter.applyToAndReturnResultIfDirty(file); - if (unixResultIfDirty != null) { - changed.add(file); - } - // because apply will count as up-to-date, it's important - // that every call to apply will get a PaddedCell check - if (!anyMisbehave && unixResultIfDirty != null) { - String onceMore = formatter.compute(unixResultIfDirty, file); - // f(f(input) == f(input) for an idempotent function - if (!onceMore.equals(unixResultIfDirty)) { - // it's not idempotent. but, if it converges, then it's likely a glitch that won't reoccur, - // so there's no need to make a bunch of noise for the user - PaddedCell result = PaddedCell.check(formatter, file, onceMore); - if (result.type() == PaddedCell.Type.CONVERGE) { - String finalResult = formatter.computeLineEndings(result.canonical(), file); - Files.write(file.toPath(), finalResult.getBytes(formatter.getEncoding()), StandardOpenOption.TRUNCATE_EXISTING); - } else { - // it didn't converge, so the user is going to need padded cell mode - anyMisbehave = true; - } - } - } - } - if (anyMisbehave) { - throw PaddedCellGradle.youShouldTurnOnPaddedCell(this); + for (File file : outOfDate) { + getLogger().debug("Applying format to " + file); + if (PaddedCellBulk.applyAnyChanged(formatter, file)) { + changed.add(file); } } return changed; @@ -311,18 +276,7 @@ private void check(Formatter formatter, List outOfDate) throws Exception { problemFiles.add(file); } } - if (paddedCell) { - PaddedCellGradle.check(this, formatter, problemFiles); - } else { - if (!problemFiles.isEmpty()) { - // if we're not in paddedCell mode, we'll check if maybe we should be - if (PaddedCellBulk.anyMisbehave(formatter, problemFiles)) { - throw PaddedCellGradle.youShouldTurnOnPaddedCell(this); - } else { - throw formatViolationsFor(formatter, problemFiles); - } - } - } + PaddedCellGradle.check(this, formatter, problemFiles); } /** Returns an exception which indicates problem files nicely. */ From f8ab7ee1d8c373278d9945b19fddbce554fd914a Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Thu, 30 Apr 2020 15:47:19 -0700 Subject: [PATCH 04/18] Update spotbugs to latest. --- build.gradle | 4 ++-- gradle.properties | 2 +- gradle/java-setup.gradle | 13 +++---------- 3 files changed, 6 insertions(+), 13 deletions(-) diff --git a/build.gradle b/build.gradle index 48c431b337..a42e9deb6f 100644 --- a/build.gradle +++ b/build.gradle @@ -8,7 +8,7 @@ plugins { // https://github.com/mnlipp/jdrupes-mdoclet/releases id 'org.jdrupes.mdoclet' version '1.0.9' apply false // https://github.com/spotbugs/spotbugs/releases - id "com.github.spotbugs" version "2.0.0" apply false + id "com.github.spotbugs" version "4.0.8" apply false //https://github.com/diffplug/goomph id "com.diffplug.p2.asmaven" version "3.21.0" apply false // https://github.com/diffplug/spotless-changelog @@ -47,5 +47,5 @@ eclipseResourceFilters { } static Class spotBugsTaskType() { - return com.github.spotbugs.SpotBugsTask + return com.github.spotbugs.snom.SpotBugsTask } diff --git a/gradle.properties b/gradle.properties index ea5d528a40..165a1403d7 100644 --- a/gradle.properties +++ b/gradle.properties @@ -15,7 +15,7 @@ artifactIdGradle=spotless-plugin-gradle # Build requirements VER_JAVA=1.8 -VER_SPOTBUGS=3.1.6 +VER_SPOTBUGS=4.0.2 # Dependencies provided by Spotless plugin VER_SLF4J=[1.6,2.0[ diff --git a/gradle/java-setup.gradle b/gradle/java-setup.gradle index f0488ec346..9a1220c1f3 100644 --- a/gradle/java-setup.gradle +++ b/gradle/java-setup.gradle @@ -34,23 +34,16 @@ eclipseResourceFilters { apply plugin: 'com.github.spotbugs' spotbugs { toolVersion = VER_SPOTBUGS - sourceSets = [ - // don't check the test code - sourceSets.main - ] ignoreFailures = false // bug free or it doesn't ship! reportsDir = file('build/spotbugs') effort = 'max' // min|default|max reportLevel = 'medium' // low|medium|high (low = sensitive to even minor mistakes) omitVisitors = [] // bugs that we want to ignore } -// HTML instead of XML -tasks.withType(spotBugsTaskType()) { - reports { - xml.enabled = false - html.enabled = true - } +tasks.named('spotbugsTest') { + enabled = false } + dependencies { compileOnly 'net.jcip:jcip-annotations:1.0' compileOnly 'com.github.spotbugs:spotbugs-annotations:3.1.6' From d4af11faba6620d41cf4ffe34419b91260b29cb6 Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Thu, 30 Apr 2020 15:47:57 -0700 Subject: [PATCH 05/18] Fix tests now that PaddedCell isn't present. --- .../gradle/spotless/PaddedCellTaskTest.java | 35 ++++++------------- 1 file changed, 10 insertions(+), 25 deletions(-) diff --git a/plugin-gradle/src/test/java/com/diffplug/gradle/spotless/PaddedCellTaskTest.java b/plugin-gradle/src/test/java/com/diffplug/gradle/spotless/PaddedCellTaskTest.java index 1d05b4419c..8d4002275b 100644 --- a/plugin-gradle/src/test/java/com/diffplug/gradle/spotless/PaddedCellTaskTest.java +++ b/plugin-gradle/src/test/java/com/diffplug/gradle/spotless/PaddedCellTaskTest.java @@ -23,7 +23,6 @@ import java.util.Collections; import java.util.Locale; -import org.assertj.core.api.Assertions; import org.gradle.api.Project; import org.junit.Assert; import org.junit.Test; @@ -76,12 +75,6 @@ private SpotlessTask createApplyTask(String name, FormatterStep step) { return task; } - private Bundle paddedCell() { - check.setPaddedCell(true); - apply.setPaddedCell(true); - return this; - } - private String checkFailureMsg() { try { execute(check); @@ -108,20 +101,12 @@ private Bundle diverge() throws IOException { return new Bundle("diverge", x -> x + " "); } - @Test - public void failsWithoutPaddedCell() throws IOException { - Assertions.assertThat(wellbehaved().checkFailureMsg()).startsWith("The following files had format violations"); - Assertions.assertThat(cycle().checkFailureMsg()).startsWith("You have a misbehaving rule"); - Assertions.assertThat(converge().checkFailureMsg()).startsWith("You have a misbehaving rule"); - Assertions.assertThat(diverge().checkFailureMsg()).startsWith("You have a misbehaving rule"); - } - @Test public void paddedCellApply() throws Exception { - Bundle wellbehaved = wellbehaved().paddedCell(); - Bundle cycle = cycle().paddedCell(); - Bundle converge = converge().paddedCell(); - Bundle diverge = diverge().paddedCell(); + Bundle wellbehaved = wellbehaved(); + Bundle cycle = cycle(); + Bundle converge = converge(); + Bundle diverge = diverge(); execute(wellbehaved.apply); execute(cycle.apply); @@ -141,10 +126,10 @@ public void paddedCellApply() throws Exception { @Test public void paddedCellCheckFailureFiles() throws Exception { - wellbehaved().paddedCell().checkFailureMsg(); - cycle().paddedCell().checkFailureMsg(); - converge().paddedCell().checkFailureMsg(); - execute(diverge().paddedCell().check); + wellbehaved().checkFailureMsg(); + cycle().checkFailureMsg(); + converge().checkFailureMsg(); + execute(diverge().check); assertFolderContents("build", "spotless-diagnose-converge", @@ -179,7 +164,7 @@ private void assertFolderContents(String subfolderName, String... files) throws @Test public void paddedCellCheckCycleFailureMsg() throws IOException { - assertFailureMessage(cycle().paddedCell(), + assertFailureMessage(cycle(), "The following files had format violations:", slashify(" src/test.cycle"), " @@ -1 +1 @@", @@ -190,7 +175,7 @@ public void paddedCellCheckCycleFailureMsg() throws IOException { @Test public void paddedCellCheckConvergeFailureMsg() throws IOException { - assertFailureMessage(converge().paddedCell(), + assertFailureMessage(converge(), "The following files had format violations:", slashify(" src/test.converge"), " @@ -1 +0,0 @@", From f47d6bb9c1523e50b0478765e272d9346c8c207f Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Thu, 30 Apr 2020 15:59:05 -0700 Subject: [PATCH 06/18] Refactor PaddedCellBulk's important API into PaddedCell.canonicalIfDirty --- .../com/diffplug/spotless/PaddedCell.java | 43 +++++++++++++++++++ .../com/diffplug/spotless/PaddedCellBulk.java | 36 ++-------------- 2 files changed, 46 insertions(+), 33 deletions(-) diff --git a/lib/src/main/java/com/diffplug/spotless/PaddedCell.java b/lib/src/main/java/com/diffplug/spotless/PaddedCell.java index d8fd282051..edc5580ea1 100644 --- a/lib/src/main/java/com/diffplug/spotless/PaddedCell.java +++ b/lib/src/main/java/com/diffplug/spotless/PaddedCell.java @@ -18,14 +18,18 @@ import static com.diffplug.spotless.LibPreconditions.requireElementsNonNull; import java.io.File; +import java.io.IOException; import java.nio.file.Files; import java.util.ArrayList; +import java.util.Arrays; import java.util.Collections; import java.util.Comparator; import java.util.List; import java.util.Objects; import java.util.function.Function; +import javax.annotation.Nullable; + /** * Models the result of applying a {@link Formatter} on a given {@link File} * while characterizing various failure modes (slow convergence, cycles, and divergence). @@ -171,4 +175,43 @@ public String userMessage() { } // @formatter:on } + + /** Returns the canonical content of this file if it is dirty, or null if it is clean or the formatter is non-idempotent. */ + public static @Nullable byte[] canonicalIfDirty(Formatter formatter, File file) throws IOException { + Objects.requireNonNull(formatter, "formatter"); + Objects.requireNonNull(file, "file"); + + byte[] rawBytes = Files.readAllBytes(file.toPath()); + String raw = new String(rawBytes, formatter.getEncoding()); + String rawUnix = LineEnding.toUnix(raw); + + // enforce the format + String formattedUnix = formatter.compute(rawUnix, file); + // convert the line endings if necessary + String formatted = formatter.computeLineEndings(formattedUnix, file); + + // if F(input) == input, then the formatter is well-behaving and the input is clean + byte[] formattedBytes = formatted.getBytes(formatter.getEncoding()); + if (Arrays.equals(rawBytes, formattedBytes)) { + return null; + } + + // F(input) != input, so we'll do a padded check + PaddedCell cell = PaddedCell.check(formatter, file, rawUnix); + if (!cell.isResolvable()) { + // nothing we can do, but check will warn and dump out the divergence path + return null; + } + + // get the canonical bytes + String canonicalUnix = cell.canonical(); + String canonical = formatter.computeLineEndings(canonicalUnix, file); + byte[] canonicalBytes = canonical.getBytes(formatter.getEncoding()); + if (!Arrays.equals(rawBytes, canonicalBytes)) { + // and write them to disk if needed + return canonicalBytes; + } else { + return null; + } + } } diff --git a/lib/src/main/java/com/diffplug/spotless/PaddedCellBulk.java b/lib/src/main/java/com/diffplug/spotless/PaddedCellBulk.java index 1d37ea72dc..8434f8520d 100644 --- a/lib/src/main/java/com/diffplug/spotless/PaddedCellBulk.java +++ b/lib/src/main/java/com/diffplug/spotless/PaddedCellBulk.java @@ -25,7 +25,6 @@ import java.nio.file.StandardOpenOption; import java.nio.file.attribute.BasicFileAttributes; import java.util.ArrayList; -import java.util.Arrays; import java.util.Collections; import java.util.List; import java.util.Locale; @@ -197,38 +196,9 @@ public static void apply(Formatter formatter, File file) throws IOException { /** Performs the typical spotlessApply, but with PaddedCell handling of misbehaving FormatterSteps. */ public static boolean applyAnyChanged(Formatter formatter, File file) throws IOException { - Objects.requireNonNull(formatter, "formatter"); - Objects.requireNonNull(file, "file"); - - byte[] rawBytes = Files.readAllBytes(file.toPath()); - String raw = new String(rawBytes, formatter.getEncoding()); - String rawUnix = LineEnding.toUnix(raw); - - // enforce the format - String formattedUnix = formatter.compute(rawUnix, file); - // convert the line endings if necessary - String formatted = formatter.computeLineEndings(formattedUnix, file); - - // if F(input) == input, then the formatter is well-behaving and the input is clean - byte[] formattedBytes = formatted.getBytes(formatter.getEncoding()); - if (Arrays.equals(rawBytes, formattedBytes)) { - return false; - } - - // F(input) != input, so we'll do a padded check - PaddedCell cell = PaddedCell.check(formatter, file, rawUnix); - if (!cell.isResolvable()) { - // nothing we can do, but check will warn and dump out the divergence path - return false; - } - - // get the canonical bytes - String canonicalUnix = cell.canonical(); - String canonical = formatter.computeLineEndings(canonicalUnix, file); - byte[] canonicalBytes = canonical.getBytes(formatter.getEncoding()); - if (!Arrays.equals(rawBytes, canonicalBytes)) { - // and write them to disk if needed - Files.write(file.toPath(), canonicalBytes, StandardOpenOption.TRUNCATE_EXISTING); + byte[] canonical = PaddedCell.canonicalIfDirty(formatter, file); + if (canonical != null) { + Files.write(file.toPath(), canonical, StandardOpenOption.TRUNCATE_EXISTING); return true; } else { return false; From 345c519ceda3165f80c6d5d046224f8dba4742b7 Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Thu, 30 Apr 2020 16:00:52 -0700 Subject: [PATCH 07/18] Improve performance for the common case where PaddedCell is not needed. --- lib/src/main/java/com/diffplug/spotless/PaddedCell.java | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/lib/src/main/java/com/diffplug/spotless/PaddedCell.java b/lib/src/main/java/com/diffplug/spotless/PaddedCell.java index edc5580ea1..956838e4c9 100644 --- a/lib/src/main/java/com/diffplug/spotless/PaddedCell.java +++ b/lib/src/main/java/com/diffplug/spotless/PaddedCell.java @@ -197,9 +197,16 @@ public String userMessage() { } // F(input) != input, so we'll do a padded check + String doubleFormattedUnix = formatter.compute(formattedUnix, file); + if (doubleFormattedUnix.equals(formattedUnix)) { + // most dirty files are idempotent-dirty, so this is a quick-short circuit for that common case + return formattedBytes; + } + PaddedCell cell = PaddedCell.check(formatter, file, rawUnix); if (!cell.isResolvable()) { // nothing we can do, but check will warn and dump out the divergence path + // TODO: where to put warnings? return null; } From 18719fd2ad10056ee412dd756383e180e5d50808 Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Thu, 30 Apr 2020 16:31:41 -0700 Subject: [PATCH 08/18] Deprecated all of PaddedCellBulk, deleted PaddedCellGradle. --- .../com/diffplug/spotless/PaddedCellBulk.java | 28 ++----- .../gradle/spotless/PaddedCellGradle.java | 76 ------------------- .../gradle/spotless/SpotlessTask.java | 20 ++++- 3 files changed, 23 insertions(+), 101 deletions(-) delete mode 100644 plugin-gradle/src/main/java/com/diffplug/gradle/spotless/PaddedCellGradle.java diff --git a/lib/src/main/java/com/diffplug/spotless/PaddedCellBulk.java b/lib/src/main/java/com/diffplug/spotless/PaddedCellBulk.java index 8434f8520d..ba41e9b892 100644 --- a/lib/src/main/java/com/diffplug/spotless/PaddedCellBulk.java +++ b/lib/src/main/java/com/diffplug/spotless/PaddedCellBulk.java @@ -34,27 +34,8 @@ import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; -/** - * Incorporates the PaddedCell machinery into broader apply / check usage. - * - * Here's the general workflow: - * - * ### Identify that paddedCell is needed - * - * Initially, paddedCell is off. That's the default, and there's no need for users to know about it. - * - * If they encounter a scenario where `spotlessCheck` fails after calling `spotlessApply`, then they would - * justifiably be frustrated. Luckily, every time `spotlessCheck` fails, it passes the failed files to - * {@link #anyMisbehave(Formatter, List)}, which checks to see if any of the rules are causing a cycle - * or some other kind of mischief. If they are, it can give the user a special error message instructing - * them to turn on paddedCell. - * - * ### spotlessCheck with paddedCell on - * - * Spotless check behaves as normal, finding a list of problem files, but then passes that list - * to {@link #check(File, File, Formatter, List)}. If there were no problem files, then `paddedCell` - * is no longer necessary, so users might as well turn it off, so we give that info as a warning. - */ +/** COMPLETELY DEPRECATED, use {@link PaddedCell#canonicalIfDirty(Formatter, File)} instead. */ +@Deprecated public final class PaddedCellBulk { private static final Logger logger = Logger.getLogger(PaddedCellBulk.class.getName()); @@ -71,11 +52,13 @@ public final class PaddedCellBulk { * tell the user about a misbehaving rule and alert her to how to enable * paddedCell mode, with minimal effort. */ + @Deprecated public static boolean anyMisbehave(Formatter formatter, List problemFiles) { return anyMisbehave(formatter, problemFiles, 500); } /** Same as {@link #anyMisbehave(Formatter, List)} but with a customizable timeout. */ + @Deprecated public static boolean anyMisbehave(Formatter formatter, List problemFiles, long timeoutMs) { Objects.requireNonNull(formatter, "formatter"); Objects.requireNonNull(problemFiles, "problemFiles"); @@ -104,6 +87,7 @@ public static boolean anyMisbehave(Formatter formatter, List problemFiles, * @return A list of files which are failing because of paddedCell problems, but could be fixed. (specifically, the files for which spotlessApply would be effective) */ @SuppressFBWarnings("NP_NULL_ON_SOME_PATH_FROM_RETURN_VALUE") + @Deprecated public static List check(File rootDir, File diagnoseDir, Formatter formatter, List problemFiles) throws IOException { Objects.requireNonNull(rootDir, "rootDir"); Objects.requireNonNull(diagnoseDir, "diagnoseDir"); @@ -190,11 +174,13 @@ public String getName() { } /** Performs the typical spotlessApply, but with PaddedCell handling of misbehaving FormatterSteps. */ + @Deprecated public static void apply(Formatter formatter, File file) throws IOException { applyAnyChanged(formatter, file); } /** Performs the typical spotlessApply, but with PaddedCell handling of misbehaving FormatterSteps. */ + @Deprecated public static boolean applyAnyChanged(Formatter formatter, File file) throws IOException { byte[] canonical = PaddedCell.canonicalIfDirty(formatter, file); if (canonical != null) { diff --git a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/PaddedCellGradle.java b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/PaddedCellGradle.java deleted file mode 100644 index 409cda7f74..0000000000 --- a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/PaddedCellGradle.java +++ /dev/null @@ -1,76 +0,0 @@ -/* - * Copyright 2016 DiffPlug - * - * 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.diffplug.gradle.spotless; - -import java.io.File; -import java.io.IOException; -import java.util.List; - -import com.diffplug.common.base.StringPrinter; -import com.diffplug.spotless.Formatter; -import com.diffplug.spotless.PaddedCellBulk; - -import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; - -/** - * Incorporates the PaddedCell machinery into SpotlessTask.apply() and SpotlessTask.check(). - * - * Here's the general workflow: - * - * ### Identify that paddedCell is needed - * - * Initially, paddedCell is off. That's the default, and there's no need for users to know about it. - * - * If they encounter a scenario where `spotlessCheck` fails after calling `spotlessApply`, then they would - * justifiably be frustrated. Luckily, every time `spotlessCheck` fails, it passes the failed files to - * {@link #anyMisbehave(Formatter, List)}, which checks to see if any of the rules are causing a cycle - * or some other kind of mischief. If they are, it throws a special error message, - * {@link #youShouldTurnOnPaddedCell(SpotlessTask)} which tells them to turn on paddedCell. - * - * ### spotlessCheck with paddedCell on - * - * Spotless check behaves as normal, finding a list of problem files, but then passes that list - * to {@link #check(SpotlessTask, Formatter, List)}. If there were no problem files, then `paddedCell` - * is no longer necessary, so users might as well turn it off, so we give that info as a warning. - */ -// TODO: Cleanup this javadoc - it's a copy of the javadoc of PaddedCellBulk, so some info -// is out-of-date (like the link to #anyMisbehave(Formatter, List)) -class PaddedCellGradle { - /** URL to a page which describes the padded cell thing. */ - private static final String URL = "https://github.com/diffplug/spotless/blob/master/PADDEDCELL.md"; - - private static File diagnoseDir(SpotlessTask task) { - return new File(task.getProject().getBuildDir(), "spotless-diagnose-" + task.formatName()); - } - - @SuppressFBWarnings("NP_NULL_ON_SOME_PATH_FROM_RETURN_VALUE") - static void check(SpotlessTask task, Formatter formatter, List problemFiles) throws IOException { - if (problemFiles.isEmpty()) { - // if the first pass was successful, then paddedCell() mode is unnecessary - task.getLogger().info(StringPrinter.buildStringFromLines( - task.getName() + " is in paddedCell() mode, but it doesn't need to be.", - "If you remove that option, spotless will run ~2x faster.", - "For details see " + URL)); - } - - File diagnoseDir = diagnoseDir(task); - File rootDir = task.getProject().getRootDir(); - List stillFailing = PaddedCellBulk.check(rootDir, diagnoseDir, formatter, problemFiles); - if (!stillFailing.isEmpty()) { - throw task.formatViolationsFor(formatter, problemFiles); - } - } -} diff --git a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTask.java b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTask.java index ca471ababf..eaab265a27 100644 --- a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTask.java +++ b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTask.java @@ -18,6 +18,8 @@ import java.io.File; import java.io.Serializable; import java.nio.charset.Charset; +import java.nio.file.Files; +import java.nio.file.StandardOpenOption; import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; @@ -44,7 +46,7 @@ import com.diffplug.spotless.Formatter; import com.diffplug.spotless.FormatterStep; import com.diffplug.spotless.LineEnding; -import com.diffplug.spotless.PaddedCellBulk; +import com.diffplug.spotless.PaddedCell; import com.diffplug.spotless.extra.integration.DiffMessageFormatter; public class SpotlessTask extends DefaultTask { @@ -261,7 +263,9 @@ private List applyAnyChanged(Formatter formatter, List outOfDate) th List changed = new ArrayList<>(outOfDate.size()); for (File file : outOfDate) { getLogger().debug("Applying format to " + file); - if (PaddedCellBulk.applyAnyChanged(formatter, file)) { + byte[] canonical = PaddedCell.canonicalIfDirty(formatter, file); + if (canonical != null) { + Files.write(file.toPath(), canonical, StandardOpenOption.TRUNCATE_EXISTING); changed.add(file); } } @@ -272,11 +276,19 @@ private void check(Formatter formatter, List outOfDate) throws Exception { List problemFiles = new ArrayList<>(); for (File file : outOfDate) { getLogger().debug("Checking format on " + file); - if (!formatter.isClean(file)) { + byte[] canonical = PaddedCell.canonicalIfDirty(formatter, file); + if (canonical != null) { problemFiles.add(file); } } - PaddedCellGradle.check(this, formatter, problemFiles); + if (!problemFiles.isEmpty()) { + throw formatViolationsFor(formatter, problemFiles); + } + } + + // TODO: store paddedcell diagnosis files somewhere + private static File diagnoseDir(SpotlessTask task) { + return new File(task.getProject().getBuildDir(), "spotless-diagnose-" + task.formatName()); } /** Returns an exception which indicates problem files nicely. */ From 8f54aa15c29e8acaf1f68fa2bf185da588b65593 Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Thu, 30 Apr 2020 21:55:26 -0700 Subject: [PATCH 09/18] Renamed PaddedCell.canonicalIfDirty to PaddedCell.calculateDirtyState(), which now includes info about non-converging steps. --- .../com/diffplug/spotless/PaddedCell.java | 52 +++++++++++++++---- .../com/diffplug/spotless/PaddedCellBulk.java | 6 +-- .../gradle/spotless/SpotlessTask.java | 42 ++++++++------- 3 files changed, 68 insertions(+), 32 deletions(-) diff --git a/lib/src/main/java/com/diffplug/spotless/PaddedCell.java b/lib/src/main/java/com/diffplug/spotless/PaddedCell.java index 956838e4c9..1f13188589 100644 --- a/lib/src/main/java/com/diffplug/spotless/PaddedCell.java +++ b/lib/src/main/java/com/diffplug/spotless/PaddedCell.java @@ -28,8 +28,6 @@ import java.util.Objects; import java.util.function.Function; -import javax.annotation.Nullable; - /** * Models the result of applying a {@link Formatter} on a given {@link File} * while characterizing various failure modes (slow convergence, cycles, and divergence). @@ -176,8 +174,12 @@ public String userMessage() { // @formatter:on } - /** Returns the canonical content of this file if it is dirty, or null if it is clean or the formatter is non-idempotent. */ - public static @Nullable byte[] canonicalIfDirty(Formatter formatter, File file) throws IOException { + /** + * Calculates whether the given file is dirty according to a PaddedCell invocation of the given formatter. + * DirtyState includes the clean state of the file, as well as a warning if we were not able to apply the formatter + * due to diverging idempotence. + */ + public static DirtyState calculateDirtyState(Formatter formatter, File file) throws IOException { Objects.requireNonNull(formatter, "formatter"); Objects.requireNonNull(file, "file"); @@ -193,21 +195,19 @@ public String userMessage() { // if F(input) == input, then the formatter is well-behaving and the input is clean byte[] formattedBytes = formatted.getBytes(formatter.getEncoding()); if (Arrays.equals(rawBytes, formattedBytes)) { - return null; + return isClean; } // F(input) != input, so we'll do a padded check String doubleFormattedUnix = formatter.compute(formattedUnix, file); if (doubleFormattedUnix.equals(formattedUnix)) { // most dirty files are idempotent-dirty, so this is a quick-short circuit for that common case - return formattedBytes; + return new DirtyState(formattedBytes); } PaddedCell cell = PaddedCell.check(formatter, file, rawUnix); if (!cell.isResolvable()) { - // nothing we can do, but check will warn and dump out the divergence path - // TODO: where to put warnings? - return null; + return formatterDoesntConverge; } // get the canonical bytes @@ -216,9 +216,39 @@ public String userMessage() { byte[] canonicalBytes = canonical.getBytes(formatter.getEncoding()); if (!Arrays.equals(rawBytes, canonicalBytes)) { // and write them to disk if needed - return canonicalBytes; + return new DirtyState(canonicalBytes); } else { - return null; + return isClean; + } + } + + /** + * The clean/dirty state of a single file. Intended use: + * - {@link #isClean()} means that the file is is clean, and there's nothing else to say + * - {@link #isConverged()} means that we can't tell what the clean state of the file would be + * - once you've tested the above conditions and you know that it's a dirty file with a converged state, + * then you can call {@link #canonicalBytes()} to get the canonical form of the given file. + */ + public static class DirtyState { + private final byte[] canonicalBytes; + + private DirtyState(byte[] canonicalBytes) { + this.canonicalBytes = canonicalBytes; + } + + public boolean isClean() { + return this == isClean; + } + + public boolean isConverged() { + return this != formatterDoesntConverge; + } + + public byte[] canonicalBytes() { + return Objects.requireNonNull(canonicalBytes, "First make sure that `!isClean()` and that `formatterConverges()`."); } } + + private static final DirtyState formatterDoesntConverge = new DirtyState(null); + private static final DirtyState isClean = new DirtyState(null); } diff --git a/lib/src/main/java/com/diffplug/spotless/PaddedCellBulk.java b/lib/src/main/java/com/diffplug/spotless/PaddedCellBulk.java index ba41e9b892..bec2f1d7e5 100644 --- a/lib/src/main/java/com/diffplug/spotless/PaddedCellBulk.java +++ b/lib/src/main/java/com/diffplug/spotless/PaddedCellBulk.java @@ -182,9 +182,9 @@ public static void apply(Formatter formatter, File file) throws IOException { /** Performs the typical spotlessApply, but with PaddedCell handling of misbehaving FormatterSteps. */ @Deprecated public static boolean applyAnyChanged(Formatter formatter, File file) throws IOException { - byte[] canonical = PaddedCell.canonicalIfDirty(formatter, file); - if (canonical != null) { - Files.write(file.toPath(), canonical, StandardOpenOption.TRUNCATE_EXISTING); + PaddedCell.DirtyState dirtyState = PaddedCell.calculateDirtyState(formatter, file); + if (!dirtyState.isClean() && dirtyState.isConverged()) { + Files.write(file.toPath(), dirtyState.canonicalBytes(), StandardOpenOption.TRUNCATE_EXISTING); return true; } else { return false; diff --git a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTask.java b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTask.java index eaab265a27..5ceb6a49d7 100644 --- a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTask.java +++ b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTask.java @@ -226,14 +226,7 @@ public void performAction(IncrementalTaskInputs inputs) throws Exception { } // create the formatter - try (Formatter formatter = Formatter.builder() - .lineEndingsPolicy(lineEndingsPolicy) - .encoding(Charset.forName(encoding)) - .rootDir(getProject().getRootDir().toPath()) - .steps(steps) - .exceptionPolicy(exceptionPolicy) - .build()) { - + try (Formatter formatter = buildFormatter()) { if (apply) { List changedFiles = applyAnyChanged(formatter, outOfDate); if (!changedFiles.isEmpty()) { @@ -252,6 +245,16 @@ public void performAction(IncrementalTaskInputs inputs) throws Exception { } } + Formatter buildFormatter() { + return Formatter.builder() + .lineEndingsPolicy(lineEndingsPolicy) + .encoding(Charset.forName(encoding)) + .rootDir(getProject().getRootDir().toPath()) + .steps(steps) + .exceptionPolicy(exceptionPolicy) + .build(); + } + static class LastApply implements Serializable { private static final long serialVersionUID = 6245070824310295090L; @@ -263,9 +266,13 @@ private List applyAnyChanged(Formatter formatter, List outOfDate) th List changed = new ArrayList<>(outOfDate.size()); for (File file : outOfDate) { getLogger().debug("Applying format to " + file); - byte[] canonical = PaddedCell.canonicalIfDirty(formatter, file); - if (canonical != null) { - Files.write(file.toPath(), canonical, StandardOpenOption.TRUNCATE_EXISTING); + PaddedCell.DirtyState dirtyState = PaddedCell.calculateDirtyState(formatter, file); + if (dirtyState.isClean()) { + // do nothing + } else if (!dirtyState.isConverged()) { + getLogger().warn("Skipping '" + file + "' because it does not converge. Run `spotlessDiagnose` to understand why"); + } else { + Files.write(file.toPath(), dirtyState.canonicalBytes(), StandardOpenOption.TRUNCATE_EXISTING); changed.add(file); } } @@ -276,8 +283,12 @@ private void check(Formatter formatter, List outOfDate) throws Exception { List problemFiles = new ArrayList<>(); for (File file : outOfDate) { getLogger().debug("Checking format on " + file); - byte[] canonical = PaddedCell.canonicalIfDirty(formatter, file); - if (canonical != null) { + PaddedCell.DirtyState dirtyState = PaddedCell.calculateDirtyState(formatter, file); + if (dirtyState.isClean()) { + // do nothing + } else if (!dirtyState.isConverged()) { + getLogger().warn("Skipping '" + file + "' because it does not converge. Run `spotlessDiagnose` to understand why"); + } else { problemFiles.add(file); } } @@ -286,11 +297,6 @@ private void check(Formatter formatter, List outOfDate) throws Exception { } } - // TODO: store paddedcell diagnosis files somewhere - private static File diagnoseDir(SpotlessTask task) { - return new File(task.getProject().getBuildDir(), "spotless-diagnose-" + task.formatName()); - } - /** Returns an exception which indicates problem files nicely. */ GradleException formatViolationsFor(Formatter formatter, List problemFiles) { return new GradleException(DiffMessageFormatter.builder() From 1684d65a7e3971ea9f93364a27da04e6a79bf588 Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Thu, 30 Apr 2020 22:32:23 -0700 Subject: [PATCH 10/18] Added the spotlessDiagnose task, which does what used to be baked-in to spotlessCheck/Apply. --- .../gradle/spotless/SpotlessDiagnoseTask.java | 72 +++++++++++++++++++ .../gradle/spotless/SpotlessExtension.java | 11 ++- .../gradle/spotless/PaddedCellTaskTest.java | 18 +++-- 3 files changed, 95 insertions(+), 6 deletions(-) create mode 100644 plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessDiagnoseTask.java diff --git a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessDiagnoseTask.java b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessDiagnoseTask.java new file mode 100644 index 0000000000..9d458e6ed6 --- /dev/null +++ b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessDiagnoseTask.java @@ -0,0 +1,72 @@ +/* + * Copyright 2016 DiffPlug + * + * 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.diffplug.gradle.spotless; + +import java.io.File; +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.util.Locale; + +import org.gradle.api.DefaultTask; +import org.gradle.api.tasks.Internal; +import org.gradle.api.tasks.TaskAction; + +import com.diffplug.spotless.Formatter; +import com.diffplug.spotless.PaddedCell; + +public class SpotlessDiagnoseTask extends DefaultTask { + SpotlessTask source; + + @Internal + public SpotlessTask getSource() { + return source; + } + + @TaskAction + public void performAction() throws IOException { + Path srcRoot = getProject().getProjectDir().toPath(); + Path diagnoseRoot = getProject().getBuildDir().toPath().resolve("spotless-diagnose-" + source.formatName()); + getProject().delete(diagnoseRoot.toFile()); + try (Formatter formatter = source.buildFormatter()) { + for (File file : source.target) { + getLogger().debug("Running padded cell check on " + file); + PaddedCell padded = PaddedCell.check(formatter, file); + if (!padded.misbehaved()) { + getLogger().debug(" well-behaved."); + } else { + // the file is misbehaved, so we'll write all its steps to DIAGNOSE_DIR + Path relative = srcRoot.relativize(file.toPath()); + Path diagnoseFile = diagnoseRoot.resolve(relative); + for (int i = 0; i < padded.steps().size(); ++i) { + Path path = Paths.get(diagnoseFile + "." + padded.type().name().toLowerCase(Locale.ROOT) + i); + Files.createDirectories(path.getParent()); + String version = padded.steps().get(i); + Files.write(path, version.getBytes(formatter.getEncoding())); + } + // dump the type of the misbehavior to console + getLogger().lifecycle(" " + relative + " " + padded.userMessage()); + } + } + } + if (Files.exists(diagnoseRoot)) { + getLogger().lifecycle("Some formatters are misbehaving, you can see details at " + diagnoseRoot); + } else { + getLogger().lifecycle("All formatters are well behaved for all files."); + } + } +} diff --git a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessExtension.java b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessExtension.java index 4503317d97..7ebb62bab4 100644 --- a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessExtension.java +++ b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessExtension.java @@ -38,12 +38,13 @@ public class SpotlessExtension { final Project project; - final Task rootCheckTask, rootApplyTask; + final Task rootCheckTask, rootApplyTask, rootDiagnoseTask; final RegisterDependenciesTask registerDependenciesTask; static final String EXTENSION = "spotless"; static final String CHECK = "Check"; static final String APPLY = "Apply"; + static final String DIAGNOSE = "Diagnose"; private static final String TASK_GROUP = "Verification"; private static final String CHECK_DESCRIPTION = "Checks that sourcecode satisfies formatting steps."; @@ -59,6 +60,8 @@ public SpotlessExtension(Project project) { rootApplyTask = project.task(EXTENSION + APPLY); rootApplyTask.setGroup(TASK_GROUP); rootApplyTask.setDescription(APPLY_DESCRIPTION); + rootDiagnoseTask = project.task(EXTENSION + DIAGNOSE); + rootDiagnoseTask.setGroup(TASK_GROUP); // no description on purpose RegisterDependenciesTask registerDependenciesTask = (RegisterDependenciesTask) project.getRootProject().getTasks().findByName(RegisterDependenciesTask.TASK_NAME); if (registerDependenciesTask == null) { @@ -293,5 +296,11 @@ public Object doCall(TaskExecutionGraph graph) { // the root tasks depend on the control tasks rootCheckTask.dependsOn(checkTask); rootApplyTask.dependsOn(applyTask); + + // create the diagnose task + SpotlessDiagnoseTask diagnoseTask = project.getTasks().create(taskName + DIAGNOSE, SpotlessDiagnoseTask.class); + diagnoseTask.source = spotlessTask; + rootDiagnoseTask.dependsOn(diagnoseTask); + diagnoseTask.mustRunAfter(clean); } } diff --git a/plugin-gradle/src/test/java/com/diffplug/gradle/spotless/PaddedCellTaskTest.java b/plugin-gradle/src/test/java/com/diffplug/gradle/spotless/PaddedCellTaskTest.java index 8d4002275b..1bd60f56f8 100644 --- a/plugin-gradle/src/test/java/com/diffplug/gradle/spotless/PaddedCellTaskTest.java +++ b/plugin-gradle/src/test/java/com/diffplug/gradle/spotless/PaddedCellTaskTest.java @@ -43,12 +43,14 @@ private static String slashify(String input) { } private class Bundle { + String name; Project project = TestProvisioner.gradleProject(rootFolder()); File file; SpotlessTask check; SpotlessTask apply; Bundle(String name, FormatterFunc function) throws IOException { + this.name = name; file = setFile("src/test." + name).toContent("CCC"); FormatterStep step = FormatterStep.createNeverUpToDate(name, function); check = createCheckTask(name, step); @@ -83,6 +85,12 @@ private String checkFailureMsg() { return e.getMessage(); } } + + private void diagnose() throws IOException { + SpotlessDiagnoseTask diagnose = project.getTasks().create("spotless" + SpotlessPlugin.capitalize(name) + "Diagnose", SpotlessDiagnoseTask.class); + diagnose.source = check; + diagnose.performAction(); + } } private Bundle wellbehaved() throws IOException { @@ -125,11 +133,11 @@ public void paddedCellApply() throws Exception { } @Test - public void paddedCellCheckFailureFiles() throws Exception { - wellbehaved().checkFailureMsg(); - cycle().checkFailureMsg(); - converge().checkFailureMsg(); - execute(diverge().check); + public void diagnose() throws Exception { + wellbehaved().diagnose(); + cycle().diagnose(); + converge().diagnose(); + diverge().diagnose(); assertFolderContents("build", "spotless-diagnose-converge", From 7b81436e83d6e2c2835670df677ac6a4c7c90bd1 Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Thu, 30 Apr 2020 22:50:57 -0700 Subject: [PATCH 11/18] Fix spotbugs warning. --- gradle/java-setup.gradle | 7 +++++++ .../com/diffplug/gradle/spotless/SpotlessDiagnoseTask.java | 3 +++ 2 files changed, 10 insertions(+) diff --git a/gradle/java-setup.gradle b/gradle/java-setup.gradle index 9a1220c1f3..f63183b000 100644 --- a/gradle/java-setup.gradle +++ b/gradle/java-setup.gradle @@ -43,6 +43,13 @@ spotbugs { tasks.named('spotbugsTest') { enabled = false } +spotbugsMain { + reports { + html { + enabled = true + } + } +} dependencies { compileOnly 'net.jcip:jcip-annotations:1.0' diff --git a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessDiagnoseTask.java b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessDiagnoseTask.java index 9d458e6ed6..f20957e650 100644 --- a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessDiagnoseTask.java +++ b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessDiagnoseTask.java @@ -29,6 +29,8 @@ import com.diffplug.spotless.Formatter; import com.diffplug.spotless.PaddedCell; +import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; + public class SpotlessDiagnoseTask extends DefaultTask { SpotlessTask source; @@ -38,6 +40,7 @@ public SpotlessTask getSource() { } @TaskAction + @SuppressFBWarnings("NP_NULL_ON_SOME_PATH_FROM_RETURN_VALUE") public void performAction() throws IOException { Path srcRoot = getProject().getProjectDir().toPath(); Path diagnoseRoot = getProject().getBuildDir().toPath().resolve("spotless-diagnose-" + source.formatName()); From df30fdb544787ea443ae1f0394bbef620fba08b8 Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Thu, 30 Apr 2020 22:57:09 -0700 Subject: [PATCH 12/18] Small change to the PaddedCell.DirtyState API. --- .../main/java/com/diffplug/spotless/PaddedCell.java | 12 ++++++------ .../java/com/diffplug/spotless/PaddedCellBulk.java | 6 +++--- .../com/diffplug/gradle/spotless/SpotlessTask.java | 4 ++-- 3 files changed, 11 insertions(+), 11 deletions(-) diff --git a/lib/src/main/java/com/diffplug/spotless/PaddedCell.java b/lib/src/main/java/com/diffplug/spotless/PaddedCell.java index 1f13188589..2841f18118 100644 --- a/lib/src/main/java/com/diffplug/spotless/PaddedCell.java +++ b/lib/src/main/java/com/diffplug/spotless/PaddedCell.java @@ -207,7 +207,7 @@ public static DirtyState calculateDirtyState(Formatter formatter, File file) thr PaddedCell cell = PaddedCell.check(formatter, file, rawUnix); if (!cell.isResolvable()) { - return formatterDoesntConverge; + return didNotConverge; } // get the canonical bytes @@ -225,7 +225,7 @@ public static DirtyState calculateDirtyState(Formatter formatter, File file) thr /** * The clean/dirty state of a single file. Intended use: * - {@link #isClean()} means that the file is is clean, and there's nothing else to say - * - {@link #isConverged()} means that we can't tell what the clean state of the file would be + * - {@link #isConverged()} means that we were able to determine a clean state * - once you've tested the above conditions and you know that it's a dirty file with a converged state, * then you can call {@link #canonicalBytes()} to get the canonical form of the given file. */ @@ -240,15 +240,15 @@ public boolean isClean() { return this == isClean; } - public boolean isConverged() { - return this != formatterDoesntConverge; + public boolean didNotConverge() { + return this == didNotConverge; } public byte[] canonicalBytes() { - return Objects.requireNonNull(canonicalBytes, "First make sure that `!isClean()` and that `formatterConverges()`."); + return Objects.requireNonNull(canonicalBytes, "First make sure that `!isClean()` and `!didNotConverge()`"); } } - private static final DirtyState formatterDoesntConverge = new DirtyState(null); + private static final DirtyState didNotConverge = new DirtyState(null); private static final DirtyState isClean = new DirtyState(null); } diff --git a/lib/src/main/java/com/diffplug/spotless/PaddedCellBulk.java b/lib/src/main/java/com/diffplug/spotless/PaddedCellBulk.java index bec2f1d7e5..992d18a9c2 100644 --- a/lib/src/main/java/com/diffplug/spotless/PaddedCellBulk.java +++ b/lib/src/main/java/com/diffplug/spotless/PaddedCellBulk.java @@ -183,11 +183,11 @@ public static void apply(Formatter formatter, File file) throws IOException { @Deprecated public static boolean applyAnyChanged(Formatter formatter, File file) throws IOException { PaddedCell.DirtyState dirtyState = PaddedCell.calculateDirtyState(formatter, file); - if (!dirtyState.isClean() && dirtyState.isConverged()) { + if (dirtyState.isClean() || dirtyState.didNotConverge()) { + return false; + } else { Files.write(file.toPath(), dirtyState.canonicalBytes(), StandardOpenOption.TRUNCATE_EXISTING); return true; - } else { - return false; } } diff --git a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTask.java b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTask.java index 5ceb6a49d7..83c9a8f181 100644 --- a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTask.java +++ b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTask.java @@ -269,7 +269,7 @@ private List applyAnyChanged(Formatter formatter, List outOfDate) th PaddedCell.DirtyState dirtyState = PaddedCell.calculateDirtyState(formatter, file); if (dirtyState.isClean()) { // do nothing - } else if (!dirtyState.isConverged()) { + } else if (dirtyState.didNotConverge()) { getLogger().warn("Skipping '" + file + "' because it does not converge. Run `spotlessDiagnose` to understand why"); } else { Files.write(file.toPath(), dirtyState.canonicalBytes(), StandardOpenOption.TRUNCATE_EXISTING); @@ -286,7 +286,7 @@ private void check(Formatter formatter, List outOfDate) throws Exception { PaddedCell.DirtyState dirtyState = PaddedCell.calculateDirtyState(formatter, file); if (dirtyState.isClean()) { // do nothing - } else if (!dirtyState.isConverged()) { + } else if (dirtyState.didNotConverge()) { getLogger().warn("Skipping '" + file + "' because it does not converge. Run `spotlessDiagnose` to understand why"); } else { problemFiles.add(file); From d8db03f08d9ac365d057b45109c0db2f880ff528 Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Thu, 30 Apr 2020 22:57:17 -0700 Subject: [PATCH 13/18] Minor changelog improvement. --- plugin-gradle/CHANGES.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugin-gradle/CHANGES.md b/plugin-gradle/CHANGES.md index 09f2f6b840..29f4e9d361 100644 --- a/plugin-gradle/CHANGES.md +++ b/plugin-gradle/CHANGES.md @@ -4,7 +4,7 @@ We adhere to the [keepachangelog](https://keepachangelog.com/en/1.0.0/) format ( ## [Unreleased] ### Changed -* PaddedCell is now always enabled. It is strictly better than non-padded cell, and there is no performance penalty. [See here](https://github.com/diffplug/spotless/pull/560#issuecomment-621752798) for detailed explanation. +* PaddedCell is now always enabled. It is strictly better than non-padded cell, and there is no performance penalty. [See here](https://github.com/diffplug/spotless/pull/560#issuecomment-621752798) for detailed explanation. ([#561](https://github.com/diffplug/spotless/pull/561)) ## [3.28.1] - 2020-04-02 ### Fixed From 9bfdc9023a7edbe4a1e2b1b8b4fae134db5405da Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Thu, 30 Apr 2020 23:04:46 -0700 Subject: [PATCH 14/18] Update the lib changelog. --- CHANGES.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGES.md b/CHANGES.md index 581cab23ae..50e67a3b05 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -10,6 +10,7 @@ This document is intended for Spotless developers. We adhere to the [keepachangelog](https://keepachangelog.com/en/1.0.0/) format (starting after version `1.27.0`). ## [Unreleased] +* Improved PaddedCell such that it is as performant as non-padded cell - no reason not to have it always enabled. Deprecated all of `PaddedCellBulk`. ([#561](https://github.com/diffplug/spotless/pull/561)) ## [1.28.1] - 2020-04-02 ### Fixed From b9ac203e16852c1cb39b5202c6b797894c8b8fe4 Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Sat, 2 May 2020 14:03:11 -0700 Subject: [PATCH 15/18] Improve the padded cell documentation. --- PADDEDCELL.md | 21 +-------------------- 1 file changed, 1 insertion(+), 20 deletions(-) diff --git a/PADDEDCELL.md b/PADDEDCELL.md index cd4476d46c..41bc13dc7e 100644 --- a/PADDEDCELL.md +++ b/PADDEDCELL.md @@ -31,26 +31,7 @@ The rule we wrote above is obviously a bad idea. But complex code formatters ca Formally, a correct formatter `F` must satisfy `F(F(input)) == F(input)` for all values of input. Any formatter which doesn't meet this rule is misbehaving. -## How does `paddedCell()` work? - -Spotless now has a special `paddedCell()` mode. If you add it to your format as such: - -```gradle -spotless { - format 'cpp', { - ... - paddedCell() - } -} -``` - -then it will run in the following way: - -- When you call `spotlessApply`, it will automatically check for a ping-pong condition. -- If there is a ping-pong condition, it will resolve the ambiguity arbitrarily, but consistently -- It will also warn that `filename such-and-such cycles between 2 steps`. - -## How is the ambiguity resolved? +## How spotless fixes this automatically This is easiest to show in an example: From 8c1a0063e8eed856c9aaded743732a3cd102e729 Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Sat, 2 May 2020 14:27:29 -0700 Subject: [PATCH 16/18] Protect `PaddedCell.DirtyState` byte[] from user corruption - byte[] canonicalBytes() is now private, and IllegalStateException if called incorrectly - access to canonicalBytes() is handled by `void writeCanonicalTo(File file)` and `void writeCanonicalTo(OutputStream out)` --- .../com/diffplug/spotless/PaddedCell.java | 20 ++++++++++++++++--- .../com/diffplug/spotless/PaddedCellBulk.java | 3 +-- .../gradle/spotless/SpotlessTask.java | 4 +--- 3 files changed, 19 insertions(+), 8 deletions(-) diff --git a/lib/src/main/java/com/diffplug/spotless/PaddedCell.java b/lib/src/main/java/com/diffplug/spotless/PaddedCell.java index 2841f18118..a6694c44d0 100644 --- a/lib/src/main/java/com/diffplug/spotless/PaddedCell.java +++ b/lib/src/main/java/com/diffplug/spotless/PaddedCell.java @@ -20,6 +20,7 @@ import java.io.File; import java.io.IOException; import java.nio.file.Files; +import java.nio.file.StandardOpenOption; import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; @@ -28,6 +29,8 @@ import java.util.Objects; import java.util.function.Function; +import org.omg.CORBA.portable.OutputStream; + /** * Models the result of applying a {@link Formatter} on a given {@link File} * while characterizing various failure modes (slow convergence, cycles, and divergence). @@ -227,7 +230,7 @@ public static DirtyState calculateDirtyState(Formatter formatter, File file) thr * - {@link #isClean()} means that the file is is clean, and there's nothing else to say * - {@link #isConverged()} means that we were able to determine a clean state * - once you've tested the above conditions and you know that it's a dirty file with a converged state, - * then you can call {@link #canonicalBytes()} to get the canonical form of the given file. + * then you can call {@link #writeCanonicalTo()} to get the canonical form of the given file. */ public static class DirtyState { private final byte[] canonicalBytes; @@ -244,8 +247,19 @@ public boolean didNotConverge() { return this == didNotConverge; } - public byte[] canonicalBytes() { - return Objects.requireNonNull(canonicalBytes, "First make sure that `!isClean()` and `!didNotConverge()`"); + private byte[] canonicalBytes() { + if (canonicalBytes == null) { + throw new IllegalStateException("First make sure that `!isClean()` and `!didNotConverge()`"); + } + return canonicalBytes; + } + + public void writeCanonicalTo(File file) throws IOException { + Files.write(file.toPath(), canonicalBytes(), StandardOpenOption.TRUNCATE_EXISTING); + } + + public void writeCanonicalTo(OutputStream out) throws IOException { + out.write(canonicalBytes()); } } diff --git a/lib/src/main/java/com/diffplug/spotless/PaddedCellBulk.java b/lib/src/main/java/com/diffplug/spotless/PaddedCellBulk.java index 992d18a9c2..e249ba1ae1 100644 --- a/lib/src/main/java/com/diffplug/spotless/PaddedCellBulk.java +++ b/lib/src/main/java/com/diffplug/spotless/PaddedCellBulk.java @@ -22,7 +22,6 @@ import java.nio.file.Path; import java.nio.file.Paths; import java.nio.file.SimpleFileVisitor; -import java.nio.file.StandardOpenOption; import java.nio.file.attribute.BasicFileAttributes; import java.util.ArrayList; import java.util.Collections; @@ -186,7 +185,7 @@ public static boolean applyAnyChanged(Formatter formatter, File file) throws IOE if (dirtyState.isClean() || dirtyState.didNotConverge()) { return false; } else { - Files.write(file.toPath(), dirtyState.canonicalBytes(), StandardOpenOption.TRUNCATE_EXISTING); + dirtyState.writeCanonicalTo(file); return true; } } diff --git a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTask.java b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTask.java index 83c9a8f181..0a40d8b3ac 100644 --- a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTask.java +++ b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTask.java @@ -18,8 +18,6 @@ import java.io.File; import java.io.Serializable; import java.nio.charset.Charset; -import java.nio.file.Files; -import java.nio.file.StandardOpenOption; import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; @@ -272,7 +270,7 @@ private List applyAnyChanged(Formatter formatter, List outOfDate) th } else if (dirtyState.didNotConverge()) { getLogger().warn("Skipping '" + file + "' because it does not converge. Run `spotlessDiagnose` to understand why"); } else { - Files.write(file.toPath(), dirtyState.canonicalBytes(), StandardOpenOption.TRUNCATE_EXISTING); + dirtyState.writeCanonicalTo(file); changed.add(file); } } From 5fbf5bb1c8ea91b61afba8fe0345a48bed57b4e5 Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Sun, 3 May 2020 08:59:42 -0700 Subject: [PATCH 17/18] Oops! Meant to get the plain OutputStream, not the CORBA one. --- lib/src/main/java/com/diffplug/spotless/PaddedCell.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/src/main/java/com/diffplug/spotless/PaddedCell.java b/lib/src/main/java/com/diffplug/spotless/PaddedCell.java index a6694c44d0..0888e1646a 100644 --- a/lib/src/main/java/com/diffplug/spotless/PaddedCell.java +++ b/lib/src/main/java/com/diffplug/spotless/PaddedCell.java @@ -19,6 +19,7 @@ import java.io.File; import java.io.IOException; +import java.io.OutputStream; import java.nio.file.Files; import java.nio.file.StandardOpenOption; import java.util.ArrayList; @@ -29,8 +30,6 @@ import java.util.Objects; import java.util.function.Function; -import org.omg.CORBA.portable.OutputStream; - /** * Models the result of applying a {@link Formatter} on a given {@link File} * while characterizing various failure modes (slow convergence, cycles, and divergence). From 36a8f97cc3fdf288f72c71d422fff8216da10bab Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Sun, 3 May 2020 09:04:05 -0700 Subject: [PATCH 18/18] Revert the spotbugs upgrade. --- build.gradle | 4 ++-- gradle.properties | 2 +- gradle/java-setup.gradle | 10 ++++++---- 3 files changed, 9 insertions(+), 7 deletions(-) diff --git a/build.gradle b/build.gradle index a42e9deb6f..48c431b337 100644 --- a/build.gradle +++ b/build.gradle @@ -8,7 +8,7 @@ plugins { // https://github.com/mnlipp/jdrupes-mdoclet/releases id 'org.jdrupes.mdoclet' version '1.0.9' apply false // https://github.com/spotbugs/spotbugs/releases - id "com.github.spotbugs" version "4.0.8" apply false + id "com.github.spotbugs" version "2.0.0" apply false //https://github.com/diffplug/goomph id "com.diffplug.p2.asmaven" version "3.21.0" apply false // https://github.com/diffplug/spotless-changelog @@ -47,5 +47,5 @@ eclipseResourceFilters { } static Class spotBugsTaskType() { - return com.github.spotbugs.snom.SpotBugsTask + return com.github.spotbugs.SpotBugsTask } diff --git a/gradle.properties b/gradle.properties index 165a1403d7..ea5d528a40 100644 --- a/gradle.properties +++ b/gradle.properties @@ -15,7 +15,7 @@ artifactIdGradle=spotless-plugin-gradle # Build requirements VER_JAVA=1.8 -VER_SPOTBUGS=4.0.2 +VER_SPOTBUGS=3.1.6 # Dependencies provided by Spotless plugin VER_SLF4J=[1.6,2.0[ diff --git a/gradle/java-setup.gradle b/gradle/java-setup.gradle index 615a631e05..c3f3c167eb 100644 --- a/gradle/java-setup.gradle +++ b/gradle/java-setup.gradle @@ -34,6 +34,10 @@ eclipseResourceFilters { apply plugin: 'com.github.spotbugs' spotbugs { toolVersion = VER_SPOTBUGS + sourceSets = [ + // don't check the test code + sourceSets.main + ] ignoreFailures = false // bug free or it doesn't ship! reportsDir = file('build/spotbugs') effort = 'max' // min|default|max @@ -45,12 +49,10 @@ tasks.withType(spotBugsTaskType()) { // only run on Java 8 (no benefit to running twice) enabled = org.gradle.api.JavaVersion.current() == org.gradle.api.JavaVersion.VERSION_1_8 reports { - html { - enabled = true - } + xml.enabled = false + html.enabled = true } } - dependencies { compileOnly 'net.jcip:jcip-annotations:1.0' compileOnly 'com.github.spotbugs:spotbugs-annotations:3.1.6'