StepHarness now round-trips everything (#2085)

Author: nedtwigg

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

@@ -35,6 +35,8 @@
 
 import javax.annotation.Nullable;
 
+import com.diffplug.spotless.generic.FenceStep;
+
 /** Formatter which performs the full formatting. */
 public final class Formatter implements Serializable, AutoCloseable {
 	private static final long serialVersionUID = 1L;
@@ -302,10 +304,15 @@ public boolean equals(Object obj) {
 	@Override
 	public void close() {
 		for (FormatterStep step : steps) {
+			if (step instanceof DelegateFormatterStep) {
+				step = ((DelegateFormatterStep) step).delegateStep;
+			}
 			if (step instanceof FormatterStepImpl.Standard) {
 				((FormatterStepImpl.Standard) step).cleanupFormatterFunc();
 			} else if (step instanceof FormatterStepEqualityOnStateSerialization) {
 				((FormatterStepEqualityOnStateSerialization) step).cleanupFormatterFunc();
+			} else if (step instanceof FenceStep.BaseStep) {
+				((FenceStep.BaseStep) step).cleanup();
 			}
 		}
 	}

lib/src/main/java/com/diffplug/spotless/generic/FenceStep.java

@@ -25,11 +25,12 @@
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
 
+import javax.annotation.Nullable;
+
 import com.diffplug.spotless.Formatter;
 import com.diffplug.spotless.FormatterFunc;
 import com.diffplug.spotless.FormatterStep;
 import com.diffplug.spotless.LineEnding;
-import com.diffplug.spotless.SerializedFunction;
 
 import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 
@@ -81,10 +82,7 @@ private void assertRegexSet() {
 	/** Returns a step which will apply the given steps but preserve the content selected by the regex / openClose pair. */
 	public FormatterStep preserveWithin(List<FormatterStep> steps) {
 		assertRegexSet();
-		return FormatterStep.createLazy(name,
-				() -> new PreserveWithin(regex, steps),
-				SerializedFunction.identity(),
-				state -> FormatterFunc.Closeable.of(state.buildFormatter(), state));
+		return new PreserveWithin(name, regex, steps);
 	}
 
 	/**
@@ -93,17 +91,14 @@ public FormatterStep preserveWithin(List<FormatterStep> steps) {
 	 */
 	public FormatterStep applyWithin(List<FormatterStep> steps) {
 		assertRegexSet();
-		return FormatterStep.createLazy(name,
-				() -> new ApplyWithin(regex, steps),
-				SerializedFunction.identity(),
-				state -> FormatterFunc.Closeable.of(state.buildFormatter(), state));
+		return new ApplyWithin(name, regex, steps);
 	}
 
-	static class ApplyWithin extends Apply implements FormatterFunc.Closeable.ResourceFuncNeedsFile<Formatter> {
+	static class ApplyWithin extends BaseStep {
 		private static final long serialVersionUID = 17061466531957339L;
 
-		ApplyWithin(Pattern regex, List<FormatterStep> steps) {
-			super(regex, steps);
+		ApplyWithin(String name, Pattern regex, List<FormatterStep> steps) {
+			super(name, regex, steps);
 		}
 
 		@Override
@@ -119,11 +114,11 @@ public String apply(Formatter formatter, String unix, File file) throws Exceptio
 		}
 	}
 
-	static class PreserveWithin extends Apply implements FormatterFunc.Closeable.ResourceFuncNeedsFile<Formatter> {
+	static class PreserveWithin extends BaseStep {
 		private static final long serialVersionUID = -8676786492305178343L;
 
-		PreserveWithin(Pattern regex, List<FormatterStep> steps) {
-			super(regex, steps);
+		PreserveWithin(String name, Pattern regex, List<FormatterStep> steps) {
+			super(name, regex, steps);
 		}
 
 		private void storeGroups(String unix) {
@@ -144,15 +139,17 @@ public String apply(Formatter formatter, String unix, File file) throws Exceptio
 	}
 
 	@SuppressFBWarnings("SE_TRANSIENT_FIELD_NOT_RESTORED")
-	static class Apply implements Serializable {
+	public static abstract class BaseStep implements Serializable, FormatterStep, FormatterFunc.Closeable.ResourceFuncNeedsFile<Formatter> {
+		final String name;
 		private static final long serialVersionUID = -2301848328356559915L;
 		final Pattern regex;
 		final List<FormatterStep> steps;
 
 		transient ArrayList<String> groups = new ArrayList<>();
 		transient StringBuilder builderInternal;
 
-		public Apply(Pattern regex, List<FormatterStep> steps) {
+		public BaseStep(String name, Pattern regex, List<FormatterStep> steps) {
+			this.name = name;
 			this.regex = regex;
 			this.steps = steps;
 		}
@@ -218,5 +215,43 @@ protected String assembleGroups(String unix) {
 				throw new Error("An intermediate step removed a match of " + pattern);
 			}
 		}
+
+		@Override
+		public String getName() {
+			return name;
+		}
+
+		private transient Formatter formatter;
+
+		@Nullable
+		@Override
+		public String format(String rawUnix, File file) throws Exception {
+			if (formatter == null) {
+				formatter = buildFormatter();
+			}
+			return this.apply(formatter, rawUnix, file);
+		}
+
+		@Override
+		public boolean equals(Object o) {
+			if (this == o)
+				return true;
+			if (o == null || getClass() != o.getClass())
+				return false;
+			BaseStep step = (BaseStep) o;
+			return name.equals(step.name) && regex.pattern().equals(step.regex.pattern()) && regex.flags() == step.regex.flags() && steps.equals(step.steps);
+		}
+
+		@Override
+		public int hashCode() {
+			return Objects.hash(name, regex.pattern(), regex.flags(), steps);
+		}
+
+		public void cleanup() {
+			if (formatter != null) {
+				formatter.close();
+				formatter = null;
+			}
+		}
 	}
 }

testlib/src/main/java/com/diffplug/spotless/StepHarness.java

@@ -26,9 +26,9 @@
 import org.assertj.core.api.Assertions;
 
 /** An api for testing a {@code FormatterStep} that doesn't depend on the File path. DO NOT ADD FILE SUPPORT TO THIS, use {@link StepHarnessWithFile} if you need that. */
-public class StepHarness extends StepHarnessBase<StepHarness> {
-	private StepHarness(Formatter formatter) {
-		super(formatter);
+public class StepHarness extends StepHarnessBase {
+	private StepHarness(Formatter formatter, RoundTrip roundTrip) {
+		super(formatter, roundTrip);
 	}
 
 	/** Creates a harness for testing steps which don't depend on the file. */
@@ -49,7 +49,17 @@ public static StepHarness forSteps(FormatterStep... steps) {
 
 	/** Creates a harness for testing a formatter whose steps don't depend on the file. */
 	public static StepHarness forFormatter(Formatter formatter) {
-		return new StepHarness(formatter);
+		return new StepHarness(formatter, RoundTrip.ASSERT_EQUAL);
+	}
+
+	public static StepHarness forStepNoRoundtrip(FormatterStep step) {
+		return new StepHarness(Formatter.builder()
+				.steps(Arrays.asList(step))
+				.lineEndingsPolicy(LineEnding.UNIX.createPolicy())
+				.encoding(StandardCharsets.UTF_8)
+				.rootDir(Paths.get(""))
+				.exceptionPolicy(new FormatExceptionPolicyStrict())
+				.build(), RoundTrip.DONT_ROUNDTRIP);
 	}
 
 	/** Asserts that the given element is transformed as expected, and that the result is idempotent. */

testlib/src/main/java/com/diffplug/spotless/StepHarnessBase.java

@@ -15,67 +15,34 @@
  */
 package com.diffplug.spotless;
 
-import java.util.Locale;
 import java.util.Objects;
-import java.util.Set;
 
 import org.assertj.core.api.Assertions;
 
-class StepHarnessBase<T extends StepHarnessBase<?>> implements AutoCloseable {
+class StepHarnessBase implements AutoCloseable {
+	enum RoundTrip {
+		ASSERT_EQUAL, DONT_ASSERT_EQUAL, DONT_ROUNDTRIP
+	}
+
 	private final Formatter formatter;
-	private Formatter roundTripped;
-	private boolean supportsRoundTrip = false;
 
-	protected StepHarnessBase(Formatter formatter) {
+	protected StepHarnessBase(Formatter formatter, RoundTrip roundTrip) {
 		this.formatter = Objects.requireNonNull(formatter);
-		if (formatter.getSteps().size() == 1) {
-			// our goal is for everything to be roundtrip serializable
-			// the steps to get there are
-			// - make every individual step round-trippable
-			// - make the other machinery (Formatter, LineEnding, etc) round-trippable
-			// - done!
-			//
-			// Right now, we're still trying to get each and every single step to be round-trippable.
-			// You can help by add a test below, make sure that the test for that step fails, and then
-			// make the test pass. `FormatterStepEqualityOnStateSerialization` is a good base class for
-			// easily converting a step to round-trip serialization while maintaining easy and concise
-			// equality code.
-			String onlyStepName = formatter.getSteps().get(0).getName();
-			if (onlyStepName.startsWith("indentWith")) {
-				supportsRoundTrip = true;
-			} else if (onlyStepName.equals("diktat")) {
-				supportsRoundTrip = true;
-			} else if (onlyStepName.toLowerCase(Locale.ROOT).contains("eclipse")) {
-				supportsRoundTrip = true;
-			} else if (onlyStepName.equals("fence")) {
-				supportsRoundTrip = true;
-			} else if (Set.of("black", "buf", "clang", "ktlint", "ktfmt", "scalafmt", "palantir-java-format", "google-java-format",
-					"removeUnusedImports", "cleanthat", "No line break between type annotation and type", "antlr4Formatter",
-					"gson", "jacksonJson", "apply-json-patch", "jsonSimple", "sortPom", "jacksonYaml", "gherkinUtils",
-					"flexmark-java", "freshmark",
-					"importOrder", "Remove unnecessary semicolons").contains(onlyStepName)) {
-				supportsRoundTrip = true;
-			}
+		if (roundTrip == RoundTrip.DONT_ROUNDTRIP) {
+			return;
+		}
+		Formatter roundTripped = SerializableEqualityTester.reserialize(formatter);
+		if (roundTrip == RoundTrip.ASSERT_EQUAL) {
+			Assertions.assertThat(roundTripped).isEqualTo(formatter);
 		}
 	}
 
 	protected Formatter formatter() {
-		if (!supportsRoundTrip) {
-			return formatter;
-		} else {
-			if (roundTripped == null) {
-				roundTripped = SerializableEqualityTester.reserialize(formatter);
-				Assertions.assertThat(roundTripped).isEqualTo(formatter);
-			}
-			return roundTripped;
-		}
+		return formatter;
 	}
 
 	@Override
 	public void close() {
 		formatter.close();
-		if (roundTripped != null) {
-			roundTripped.close();
-		}
 	}
 }

testlib/src/main/java/com/diffplug/spotless/StepHarnessWithFile.java

@@ -26,17 +26,17 @@
 import org.assertj.core.api.Assertions;
 
 /** An api for testing a {@code FormatterStep} that depends on the File path. */
-public class StepHarnessWithFile extends StepHarnessBase<StepHarnessWithFile> {
+public class StepHarnessWithFile extends StepHarnessBase {
 	private final ResourceHarness harness;
 
-	private StepHarnessWithFile(ResourceHarness harness, Formatter formatter) {
-		super(formatter);
+	private StepHarnessWithFile(ResourceHarness harness, Formatter formatter, RoundTrip roundTrip) {
+		super(formatter, roundTrip);
 		this.harness = Objects.requireNonNull(harness);
 	}
 
 	/** Creates a harness for testing steps which do depend on the file. */
 	public static StepHarnessWithFile forStep(ResourceHarness harness, FormatterStep step) {
-		return new StepHarnessWithFile(harness, Formatter.builder()
+		return forFormatter(harness, Formatter.builder()
 				.name(step.getName())
 				.encoding(StandardCharsets.UTF_8)
 				.lineEndingsPolicy(LineEnding.UNIX.createPolicy())
@@ -48,7 +48,7 @@ public static StepHarnessWithFile forStep(ResourceHarness harness, FormatterStep
 
 	/** Creates a harness for testing a formatter whose steps do depend on the file. */
 	public static StepHarnessWithFile forFormatter(ResourceHarness harness, Formatter formatter) {
-		return new StepHarnessWithFile(harness, formatter);
+		return new StepHarnessWithFile(harness, formatter, RoundTrip.ASSERT_EQUAL);
 	}
 
 	/** Asserts that the given element is transformed as expected, and that the result is idempotent. */

testlib/src/test/java/com/diffplug/spotless/generic/FenceStepTest.java

@@ -29,14 +29,13 @@
 import com.diffplug.spotless.FormatterStep;
 import com.diffplug.spotless.ResourceHarness;
 import com.diffplug.spotless.StepHarness;
-import com.diffplug.spotless.StepHarnessWithFile;
 
 class FenceStepTest extends ResourceHarness {
 	@Test
 	void single() {
 		FormatterStep fence = FenceStep.named("fence").openClose("spotless:off", "spotless:on")
 				.preserveWithin(Arrays.asList(createNeverUpToDateSerializable("lowercase", String::toLowerCase)));
-		StepHarness harness = StepHarness.forSteps(fence);
+		StepHarness harness = StepHarness.forStepNoRoundtrip(fence);
 		harness.test(
 				StringPrinter.buildStringFromLines(
 						"A B C",
@@ -56,7 +55,7 @@ void single() {
 	void multiple() {
 		FormatterStep fence = FenceStep.named("fence").openClose("spotless:off", "spotless:on")
 				.preserveWithin(Arrays.asList(createNeverUpToDateSerializable("lowercase", String::toLowerCase)));
-		StepHarness harness = StepHarness.forSteps(fence);
+		StepHarness harness = StepHarness.forStepNoRoundtrip(fence);
 		harness.test(
 				StringPrinter.buildStringFromLines(
 						"A B C",
@@ -90,9 +89,8 @@ void multiple() {
 	void broken() {
 		FormatterStep fence = FenceStep.named("fence").openClose("spotless:off", "spotless:on")
 				.preserveWithin(Arrays.asList(createNeverUpToDateSerializable("uppercase", String::toUpperCase)));
-		StepHarnessWithFile harness = StepHarnessWithFile.forStep(this, fence);
 		// this fails because uppercase turns spotless:off into SPOTLESS:OFF, etc
-		harness.testExceptionMsg(newFile("test"), StringPrinter.buildStringFromLines("A B C",
+		StepHarness.forStepNoRoundtrip(fence).testExceptionMsg(StringPrinter.buildStringFromLines("A B C",
 				"spotless:off",
 				"D E F",
 				"spotless:on",
@@ -103,7 +101,7 @@ void broken() {
 	void andApply() {
 		FormatterStep fence = FenceStep.named("fence").openClose("<lower>", "</lower>")
 				.applyWithin(Arrays.asList(createNeverUpToDateSerializable("lowercase", String::toLowerCase)));
-		StepHarness.forSteps(fence).test(
+		StepHarness.forStepNoRoundtrip(fence).test(
 				StringPrinter.buildStringFromLines(
 						"A B C",
 						"<lower>",
@@ -152,5 +150,6 @@ public String getName() {
 		public String format(String rawUnix, File file) throws Exception {
 			return formatterFunc.apply(rawUnix, file);
 		}
+
 	}
 }

testlib/src/test/java/com/diffplug/spotless/java/GoogleJavaFormatStepTest.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2016-2023 DiffPlug
+ * Copyright 2016-2024 DiffPlug
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -64,7 +64,7 @@ void behavior() throws Exception {
 	@Test
 	void versionBelowMinimumRequiredVersionIsNotAllowed() throws Exception {
 		FormatterStep step = GoogleJavaFormatStep.create("1.2", "AOSP", TestProvisioner.mavenCentral());
-		StepHarness.forStep(step)
+		StepHarness.forStepNoRoundtrip(step)
 				.testResourceExceptionMsg("java/googlejavaformat/JavaCodeWithLicenseUnformatted.test")
 				.contains("you are using 1.2");
 	}
@@ -73,7 +73,7 @@ void versionBelowMinimumRequiredVersionIsNotAllowed() throws Exception {
 	@EnabledForJreRange(min = JAVA_16)
 	void versionBelowOneDotTenIsNotAllowed() throws Exception {
 		FormatterStep step = GoogleJavaFormatStep.create("1.9", "AOSP", TestProvisioner.mavenCentral());
-		StepHarness.forStep(step)
+		StepHarness.forStepNoRoundtrip(step)
 				.testResourceExceptionMsg("java/googlejavaformat/JavaCodeWithLicenseUnformatted.test")
 				.contains("you are using 1.9");
 	}