Merge pull request #561 from diffplug/feat/paddedcell-mandatory

Make padded cell mandatory

Author: nedtwigg

CHANGES.md

@@ -12,6 +12,7 @@ We adhere to the [keepachangelog](https://keepachangelog.com/en/1.0.0/) format (
 ## [Unreleased]
 ### Added
 * Support for google-java-format 1.8 (including test infrastructure for Java 11). ([#562](https://github.com/diffplug/spotless/issues/562))
+* 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

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-<FORMAT_NAME>` 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
 
@@ -46,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:
 

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<File> 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

lib/src/main/java/com/diffplug/spotless/PaddedCell.java

@@ -18,8 +18,12 @@
 import static com.diffplug.spotless.LibPreconditions.requireElementsNonNull;
 
 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;
+import java.util.Arrays;
 import java.util.Collections;
 import java.util.Comparator;
 import java.util.List;
@@ -171,4 +175,93 @@ public String userMessage() {
 		}
 		// @formatter:on
 	}
+
+	/**
+	 * 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");
+
+		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 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 new DirtyState(formattedBytes);
+		}
+
+		PaddedCell cell = PaddedCell.check(formatter, file, rawUnix);
+		if (!cell.isResolvable()) {
+			return didNotConverge;
+		}
+
+		// 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 new DirtyState(canonicalBytes);
+		} else {
+			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 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 #writeCanonicalTo()} 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 didNotConverge() {
+			return this == 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());
+		}
+	}
+
+	private static final DirtyState didNotConverge = new DirtyState(null);
+	private static final DirtyState isClean = new DirtyState(null);
 }

lib/src/main/java/com/diffplug/spotless/PaddedCellBulk.java

@@ -22,10 +22,8 @@
 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.Arrays;
 import java.util.Collections;
 import java.util.List;
 import java.util.Locale;
@@ -35,27 +33,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());
 
@@ -72,11 +51,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<File> 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<File> problemFiles, long timeoutMs) {
 		Objects.requireNonNull(formatter, "formatter");
 		Objects.requireNonNull(problemFiles, "problemFiles");
@@ -105,6 +86,7 @@ public static boolean anyMisbehave(Formatter formatter, List<File> 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<File> check(File rootDir, File diagnoseDir, Formatter formatter, List<File> problemFiles) throws IOException {
 		Objects.requireNonNull(rootDir, "rootDir");
 		Objects.requireNonNull(diagnoseDir, "diagnoseDir");
@@ -191,47 +173,20 @@ 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 {
-		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)) {
+		PaddedCell.DirtyState dirtyState = PaddedCell.calculateDirtyState(formatter, file);
+		if (dirtyState.isClean() || dirtyState.didNotConverge()) {
 			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);
-			return true;
 		} else {
-			return false;
+			dirtyState.writeCanonicalTo(file);
+			return true;
 		}
 	}
 

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. ([#561](https://github.com/diffplug/spotless/pull/561))
 
 ## [3.28.1] - 2020-04-02
 ### Added

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')

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 <a href="https://github.com/diffplug/spotless/blob/master/PADDEDCELL.md">Padded cell</a> */
+	@Deprecated
 	public void paddedCell() {
 		paddedCell(true);
 	}
 
 	/** Enables paddedCell mode. @see <a href="https://github.com/diffplug/spotless/blob/master/PADDEDCELL.md">Padded cell</a> */
+	@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) {