Linting - API (#2149)

Author: nedtwigg

CHANGES.md

@@ -11,7 +11,15 @@ We adhere to the [keepachangelog](https://keepachangelog.com/en/1.0.0/) format (
 
 ## [Unreleased]
 ### Added
+* APIs to support linting. (implemented in [#2148](https://github.com/diffplug/spotless/pull/2148) and [#2149](https://github.com/diffplug/spotless/pull/2149))
+  * Spotless is still primarily a formatter, not a linter. But when formatting fails, it's more flexible to model those failures as lints so that the formatting can continue and ideally we can also capture the line numbers causing the failure.
+  * `Lint` models a single change. A `FormatterStep` can create a lint by:
+    * throwing an exception during formatting, ideally `throw Lint.atLine(127, "code", "Well what happened was...")`
+    * or by implementing the `List<Lint> lint(String content, File file)` method to create multiple of them
 * Support for line ending policy `PRESERVE` which just takes the first line ending of every given file as setting (no matter if `\n`, `\r\n` or `\r`) ([#2304](https://github.com/diffplug/spotless/pull/2304))
+### Changes
+* **BREAKING** Moved `PaddedCell.DirtyState` to its own top-level class with new methods. ([#2148](https://github.com/diffplug/spotless/pull/2148))
+  * **BREAKING** Removed `isClean`, `applyTo`, and `applyToAndReturnResultIfDirty` from `Formatter` because users should instead use `DirtyState`.
 ### Fixed
 * `ktlint` steps now read from the `string` instead of the `file` so they don't clobber earlier steps. (fixes [#1599](https://github.com/diffplug/spotless/issues/1599))
 

CONTRIBUTING.md

@@ -93,7 +93,6 @@ Here's a checklist for creating a new step for Spotless:
 - [ ] Class name ends in Step, `SomeNewStep`.
 - [ ] Class has a public static method named `create` that returns a `FormatterStep`.
 - [ ] Has a test class named `SomeNewStepTest` that uses `StepHarness` or `StepHarnessWithFile` to test the step.
-  - [ ] Start with `StepHarness.forStep(myStep).supportsRoundTrip(false)`, and then add round trip support as described in the next section. 
 - [ ] Test class has test methods to verify behavior.
 - [ ] Test class has a test method `equality()` which tests equality using `StepEqualityTester` (see existing methods for examples).
 
@@ -137,6 +136,15 @@ There are many great formatters (prettier, clang-format, black, etc.) which live
 
 Because of Spotless' up-to-date checking and [git ratcheting](https://github.com/diffplug/spotless/tree/main/plugin-gradle#ratchet), Spotless actually doesn't have to call formatters very often, so even an expensive shell call for every single invocation isn't that bad.  Anything that works is better than nothing, and we can always speed things up later if it feels too slow (but it probably won't).
 
+## Lints
+
+Spotless is primarily a formatter, not a linter. But, if something goes wrong during formatting, it's better to model that as a lint with line numbers rather than just a naked exception. There are two ways to go about this:
+
+- at any point during the formatting process, you can throw a `Lint.atLine(int line, ...)` exception. This will be caught and turned into a lint.
+- or you can override the `default List<Lint> lint(String content, File file)` method. This method will only run if the step did not already throw an exception.
+
+Don't go lint crazy! By default, all lints are build failures. Users have to suppress them explicitly if they want to continue.
+
 ## How to add a new plugin for a build system
 
 The gist of it is that you will have to:

gradle.properties

@@ -32,3 +32,4 @@ VER_JGIT=6.10.0.202406032230-r
 VER_JUNIT=5.11.2
 VER_ASSERTJ=3.26.3
 VER_MOCKITO=5.14.2
+VER_SELFIE=2.4.1

gradle/special-tests.gradle

@@ -18,7 +18,14 @@ tasks.withType(Test).configureEach {
 			maxFailures = 10
 		}
 	}
-
+	// selfie https://selfie.dev/jvm/get-started#gradle
+	environment project.properties.subMap([
+		"selfie"
+	]) // optional, see "Overwrite everything" below
+	inputs.files(fileTree("src/test") {
+		// optional, improves up-to-date checking
+		include "**/*.ss"
+	})
 	// https://docs.gradle.org/8.8/userguide/performance.html#execute_tests_in_parallel
 	maxParallelForks = Runtime.runtime.availableProcessors().intdiv(2) ?: 1
 }

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

@@ -46,7 +46,7 @@ public boolean didNotConverge() {
 		return this == didNotConverge;
 	}
 
-	private byte[] canonicalBytes() {
+	byte[] canonicalBytes() {
 		if (canonicalBytes == null) {
 			throw new IllegalStateException("First make sure that {@code !isClean()} and {@code !didNotConverge()}");
 		}
@@ -74,7 +74,17 @@ public static DirtyState of(Formatter formatter, File file) throws IOException {
 	}
 
 	public static DirtyState of(Formatter formatter, File file, byte[] rawBytes) {
-		String raw = new String(rawBytes, formatter.getEncoding());
+		return of(formatter, file, rawBytes, new String(rawBytes, formatter.getEncoding()));
+	}
+
+	public static DirtyState of(Formatter formatter, File file, byte[] rawBytes, String raw) {
+		var valuePerStep = new ValuePerStep<Throwable>(formatter);
+		DirtyState state = of(formatter, file, rawBytes, raw, valuePerStep);
+		LintPolicy.legacyBehavior(formatter, file, valuePerStep);
+		return state;
+	}
+
+	static DirtyState of(Formatter formatter, File file, byte[] rawBytes, String raw, ValuePerStep<Throwable> exceptionPerStep) {
 		// check that all characters were encodable
 		String encodingError = EncodingErrorMsg.msg(raw, rawBytes, formatter.getEncoding());
 		if (encodingError != null) {
@@ -84,7 +94,7 @@ public static DirtyState of(Formatter formatter, File file, byte[] rawBytes) {
 		String rawUnix = LineEnding.toUnix(raw);
 
 		// enforce the format
-		String formattedUnix = formatter.compute(rawUnix, file);
+		String formattedUnix = formatter.computeWithLint(rawUnix, file, exceptionPerStep);
 		// convert the line endings if necessary
 		String formatted = formatter.computeLineEndings(formattedUnix, file);
 
@@ -95,13 +105,13 @@ public static DirtyState of(Formatter formatter, File file, byte[] rawBytes) {
 		}
 
 		// F(input) != input, so we'll do a padded check
-		String doubleFormattedUnix = formatter.compute(formattedUnix, file);
+		String doubleFormattedUnix = formatter.computeWithLint(formattedUnix, file, exceptionPerStep);
 		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);
+		PaddedCell cell = PaddedCell.check(formatter, file, rawUnix, exceptionPerStep);
 		if (!cell.isResolvable()) {
 			return didNotConverge;
 		}

lib/src/main/java/com/diffplug/spotless/FilterByContentPatternFormatterStep.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.
@@ -16,8 +16,8 @@
 package com.diffplug.spotless;
 
 import java.io.File;
+import java.util.List;
 import java.util.Objects;
-import java.util.regex.Matcher;
 import java.util.regex.Pattern;
 
 import javax.annotation.Nullable;
@@ -36,14 +36,24 @@ final class FilterByContentPatternFormatterStep extends DelegateFormatterStep {
 	public @Nullable String format(String raw, File file) throws Exception {
 		Objects.requireNonNull(raw, "raw");
 		Objects.requireNonNull(file, "file");
-		Matcher matcher = contentPattern.matcher(raw);
-		if (matcher.find() == (onMatch == OnMatch.INCLUDE)) {
+		if (contentPattern.matcher(raw).find() == (onMatch == OnMatch.INCLUDE)) {
 			return delegateStep.format(raw, file);
 		} else {
 			return raw;
 		}
 	}
 
+	@Override
+	public List<Lint> lint(String raw, File file) throws Exception {
+		Objects.requireNonNull(raw, "raw");
+		Objects.requireNonNull(file, "file");
+		if (contentPattern.matcher(raw).find() == (onMatch == OnMatch.INCLUDE)) {
+			return delegateStep.lint(raw, file);
+		} else {
+			return List.of();
+		}
+	}
+
 	@Override
 	public boolean equals(Object o) {
 		if (this == o) {

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

@@ -1,5 +1,5 @@
 /*
- * Copyright 2016-2022 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.
@@ -16,6 +16,7 @@
 package com.diffplug.spotless;
 
 import java.io.File;
+import java.util.List;
 import java.util.Objects;
 
 import javax.annotation.Nullable;
@@ -39,6 +40,17 @@ final class FilterByFileFormatterStep extends DelegateFormatterStep {
 		}
 	}
 
+	@Override
+	public List<Lint> lint(String content, File file) throws Exception {
+		Objects.requireNonNull(content, "content");
+		Objects.requireNonNull(file, "file");
+		if (filter.accept(file)) {
+			return delegateStep.lint(content, file);
+		} else {
+			return List.of();
+		}
+	}
+
 	@Override
 	public boolean equals(Object o) {
 		if (this == o) {

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

@@ -1,5 +1,5 @@
 /*
- * Copyright 2016 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.
@@ -23,7 +23,7 @@
  * A policy for handling exceptions in the format.  Any exceptions will
  * halt the build except for a specifically excluded path or step.
  */
-public class FormatExceptionPolicyStrict extends NoLambda.EqualityBasedOnSerialization implements FormatExceptionPolicy {
+public class FormatExceptionPolicyStrict extends NoLambda.EqualityBasedOnSerialization {
 	private static final long serialVersionUID = 1L;
 
 	private final Set<String> excludeSteps = new TreeSet<>();
@@ -39,18 +39,17 @@ public void excludePath(String relativePath) {
 		excludePaths.add(Objects.requireNonNull(relativePath));
 	}
 
-	@Override
 	public void handleError(Throwable e, FormatterStep step, String relativePath) {
 		Objects.requireNonNull(e, "e");
 		Objects.requireNonNull(step, "step");
 		Objects.requireNonNull(relativePath, "relativePath");
 		if (excludeSteps.contains(step.getName())) {
-			FormatExceptionPolicyLegacy.warning(e, step, relativePath);
+			LintPolicy.warning(e, step, relativePath);
 		} else {
 			if (excludePaths.contains(relativePath)) {
-				FormatExceptionPolicyLegacy.warning(e, step, relativePath);
+				LintPolicy.warning(e, step, relativePath);
 			} else {
-				FormatExceptionPolicyLegacy.error(e, step, relativePath);
+				LintPolicy.error(e, step, relativePath);
 				throw ThrowingEx.asRuntimeRethrowError(e);
 			}
 		}

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

@@ -128,28 +128,51 @@ public String computeLineEndings(String unix, File file) {
 	 * is guaranteed to also have unix line endings.
 	 */
 	public String compute(String unix, File file) {
+		ValuePerStep<Throwable> exceptionPerStep = new ValuePerStep<>(this);
+		String result = computeWithLint(unix, file, exceptionPerStep);
+		LintPolicy.legacyBehavior(this, file, exceptionPerStep);
+		return result;
+	}
+
+	/**
+	 * Returns the result of calling all of the FormatterSteps, while also
+	 * tracking any exceptions which are thrown.
+	 * <p>
+	 * The input must have unix line endings, and the output
+	 * is guaranteed to also have unix line endings.
+	 * <p>
+	 * It doesn't matter what is inside `ValuePerStep`, the value at every index will be overwritten
+	 * when the method returns.
+	 */
+	String computeWithLint(String unix, File file, ValuePerStep<Throwable> exceptionPerStep) {
 		Objects.requireNonNull(unix, "unix");
 		Objects.requireNonNull(file, "file");
 
-		for (FormatterStep step : steps) {
+		for (int i = 0; i < steps.size(); ++i) {
+			FormatterStep step = steps.get(i);
+			Throwable storeForStep;
 			try {
 				String formatted = step.format(unix, file);
 				if (formatted == null) {
 					// This probably means it was a step that only checks
 					// for errors and doesn't actually have any fixes.
 					// No exception was thrown so we can just continue.
+					storeForStep = LintState.formatStepCausedNoChange();
 				} else {
 					// Should already be unix-only, but some steps might misbehave.
-					unix = LineEnding.toUnix(formatted);
+					String clean = LineEnding.toUnix(formatted);
+					if (clean.equals(unix)) {
+						storeForStep = LintState.formatStepCausedNoChange();
+					} else {
+						storeForStep = null;
+						unix = LineEnding.toUnix(formatted);
+					}
 				}
 			} catch (Throwable e) {
-				// TODO: this is bad, but it won't matter when add support for linting
-				if (e instanceof RuntimeException) {
-					throw (RuntimeException) e;
-				} else {
-					throw new RuntimeException(e);
-				}
+				// store the exception which was thrown and keep going
+				storeForStep = e;
 			}
+			exceptionPerStep.set(i, storeForStep);
 		}
 		return unix;
 	}