Skip to content

Commit

Permalink
Start making each FormatterStep roundtrip serializable. (#1945)
Browse files Browse the repository at this point in the history
  • Loading branch information
nedtwigg authored Jan 23, 2024
2 parents b3c4893 + 93eb099 commit 26fbd03
Show file tree
Hide file tree
Showing 18 changed files with 498 additions and 125 deletions.
4 changes: 4 additions & 0 deletions CHANGES.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,10 @@ 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]
### Added
* `FileSignature.Promised` and `JarState.Promised` to facilitate round-trip serialization for the Gradle configuration cache. ([#1945](https://github.com/diffplug/spotless/pull/1945))
### Removed
* **BREAKING** Remove `JarState.getMavenCoordinate(String prefix)`. ([#1945](https://github.com/diffplug/spotless/pull/1945))
### Fixed
* Ignore system git config when running tests ([#1990](https://github.com/diffplug/spotless/issues/1990))

Expand Down
23 changes: 22 additions & 1 deletion CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -94,10 +94,31 @@ 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`.
- [ ] 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).

### Serialization roundtrip

In order to support Gradle's configuration cache, all `FormatterStep` must be round-trip serializable. This is a bit tricky because step equality is based on the serialized form of the state, and `transient` is used to take absolute paths out of the equality check. To make this work, roundtrip compatible steps actually have *two* states:

- `RoundtripState` which must be roundtrip serializable but has no equality constraints
- `FileSignature.Promised` for settings files and `JarState.Promised` for the classpath
- `EqualityState` which will never be reserialized and its serialized form is used for equality / hashCode checks
- `FileSignature` for settings files and `JarState` for the classpath

```java
FormatterStep create(String name,
RoundtripState roundTrip,
SerializedFunction<RoundtripState, EqualityState> equalityFunc,
SerializedFunction<EqualityState, FormatterFunc> formatterFunc)
FormatterStep createLazy(String name,
Supplier<RoundtripState> roundTrip,
SerializedFunction<RoundtripState, EqualityState> equalityFunc,
SerializedFunction<EqualityState, FormatterFunc> formatterFunc)
```

### Third-party dependencies via reflection or compile-only source sets

Most formatters are going to use some kind of third-party jar. Spotless integrates with many formatters, some of which have incompatible transitive dependencies. To address this, we resolve third-party dependencies using [`JarState`](https://github.com/diffplug/spotless/blob/b26f0972b185995d7c6a7aefa726c146d24d9a82/lib/src/main/java/com/diffplug/spotless/kotlin/KtfmtStep.java#L118). To call methods on the classes in that `JarState`, you can either use reflection or a compile-only source set. See [#524](https://github.com/diffplug/spotless/issues/524) for examples of both approaches.
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2016-2021 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.
Expand All @@ -24,7 +24,6 @@
import java.util.ArrayList;
import java.util.List;
import java.util.Objects;
import java.util.Optional;
import java.util.Properties;

import com.diffplug.common.base.Errors;
Expand Down Expand Up @@ -187,12 +186,6 @@ public Properties getPreferences() {
return preferences.getProperties();
}

/** Returns first coordinate from sorted set that starts with a given prefix.*/
public Optional<String> getMavenCoordinate(String prefix) {
return jarState.getMavenCoordinates().stream()
.filter(coordinate -> coordinate.startsWith(prefix)).findFirst();
}

/**
* Load class based on the given configuration of JAR provider and Maven coordinates.
* Different class loader instances are provided in the following scenarios:
Expand Down
30 changes: 29 additions & 1 deletion lib/src/main/java/com/diffplug/spotless/FileSignature.java
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2016-2021 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.
Expand Down Expand Up @@ -33,6 +33,7 @@
import java.util.Locale;
import java.util.Map;

import edu.umd.cs.findbugs.annotations.Nullable;
import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;

/** Computes a signature for any needed files. */
Expand All @@ -43,6 +44,8 @@ public final class FileSignature implements Serializable {
* Transient because not needed to uniquely identify a FileSignature instance, and also because
* Gradle only needs this class to be Serializable so it can compare FileSignature instances for
* incremental builds.
*
* We don't want these absolute paths to screw up buildcache keys.
*/
@SuppressFBWarnings("SE_TRANSIENT_FIELD_NOT_RESTORED")
private final transient List<File> files;
Expand Down Expand Up @@ -93,6 +96,31 @@ private FileSignature(final List<File> files) throws IOException {
}
}

/** A view of `FileSignature` which can be safely roundtripped. */
public static class Promised implements Serializable {
private static final long serialVersionUID = 1L;
private final List<File> files;
@SuppressFBWarnings("SE_TRANSIENT_FIELD_NOT_RESTORED")
private transient @Nullable FileSignature cached;

private Promised(List<File> files, @Nullable FileSignature cached) {
this.files = files;
this.cached = cached;
}

public FileSignature get() {
if (cached == null) {
// null when restored via serialization
cached = ThrowingEx.get(() -> new FileSignature(files));
}
return cached;
}
}

public Promised asPromise() {
return new Promised(files, this);
}

/** Returns all of the files in this signature, throwing an exception if there are more or less than 1 file. */
public Collection<File> files() {
return Collections.unmodifiableList(files);
Expand Down
4 changes: 3 additions & 1 deletion lib/src/main/java/com/diffplug/spotless/Formatter.java
Original file line number Diff line number Diff line change
@@ -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.
Expand Down Expand Up @@ -304,6 +304,8 @@ public void close() {
for (FormatterStep step : steps) {
if (step instanceof FormatterStepImpl.Standard) {
((FormatterStepImpl.Standard) step).cleanupFormatterFunc();
} else if (step instanceof FormatterStepEqualityOnStateSerialization) {
((FormatterStepEqualityOnStateSerialization) step).cleanupFormatterFunc();
}
}
}
Expand Down
66 changes: 56 additions & 10 deletions lib/src/main/java/com/diffplug/spotless/FormatterStep.java
Original file line number Diff line number Diff line change
@@ -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.
Expand Down Expand Up @@ -29,7 +29,7 @@
*/
public interface FormatterStep extends Serializable {
/** The name of the step, for debugging purposes. */
public String getName();
String getName();

/**
* Returns a formatted version of the given content.
Expand All @@ -43,7 +43,8 @@ public interface FormatterStep extends Serializable {
* if the formatter step doesn't have any changes to make
* @throws Exception if the formatter step experiences a problem
*/
public @Nullable String format(String rawUnix, File file) throws Exception;
@Nullable
String format(String rawUnix, File file) throws Exception;

/**
* Returns a new FormatterStep which will only apply its changes
Expand All @@ -54,7 +55,7 @@ public interface FormatterStep extends Serializable {
* @return FormatterStep
*/
@Deprecated
public default FormatterStep filterByContentPattern(String contentPattern) {
default FormatterStep filterByContentPattern(String contentPattern) {
return filterByContent(OnMatch.INCLUDE, contentPattern);
}

Expand All @@ -68,7 +69,7 @@ public default FormatterStep filterByContentPattern(String contentPattern) {
* java regular expression used to filter in or out files which content contain pattern
* @return FormatterStep
*/
public default FormatterStep filterByContent(OnMatch onMatch, String contentPattern) {
default FormatterStep filterByContent(OnMatch onMatch, String contentPattern) {
return new FilterByContentPatternFormatterStep(this, onMatch, contentPattern);
}

Expand All @@ -78,7 +79,7 @@ public default FormatterStep filterByContent(OnMatch onMatch, String contentPatt
* <p>
* The provided filter must be serializable.
*/
public default FormatterStep filterByFile(SerializableFileFilter filter) {
default FormatterStep filterByFile(SerializableFileFilter filter) {
return new FilterByFileFormatterStep(this, filter);
}

Expand All @@ -104,6 +105,51 @@ public final String format(String rawUnix, File file) throws Exception {
}
}

/**
* @param name
* The name of the formatter step.
* @param roundtripInit
* If the step has any state, this supplier will calculate it lazily. The supplier doesn't
* have to be serializable, but the result it calculates needs to be serializable.
* @param equalityFunc
* A pure serializable function (method reference recommended) which takes the result of `roundtripInit`,
* and returns a serializable object whose serialized representation will be used for `.equals` and
* `.hashCode` of the FormatterStep.
* @param formatterFunc
* A pure serializable function (method reference recommended) which takes the result of `equalityFunc`,
* and returns a `FormatterFunc` which will be used for the actual formatting.
* @return A FormatterStep which can be losslessly roundtripped through the java serialization machinery.
*/
static <RoundtripState extends Serializable, EqualityState extends Serializable> FormatterStep createLazy(
String name,
ThrowingEx.Supplier<RoundtripState> roundtripInit,
SerializedFunction<RoundtripState, EqualityState> equalityFunc,
SerializedFunction<EqualityState, FormatterFunc> formatterFunc) {
return new FormatterStepSerializationRoundtrip<>(name, roundtripInit, equalityFunc, formatterFunc);
}

/**
* @param name
* The name of the formatter step.
* @param roundTrip
* The roundtrip serializable state of the step.
* @param equalityFunc
* A pure serializable function (method reference recommended) which takes the result of `roundTrip`,
* and returns a serializable object whose serialized representation will be used for `.equals` and
* `.hashCode` of the FormatterStep.
* @param formatterFunc
* A pure serializable function (method reference recommended) which takes the result of `equalityFunc`,
* and returns a `FormatterFunc` which will be used for the actual formatting.
* @return A FormatterStep which can be losslessly roundtripped through the java serialization machinery.
*/
static <RoundtripState extends Serializable, EqualityState extends Serializable> FormatterStep create(
String name,
RoundtripState roundTrip,
SerializedFunction<RoundtripState, EqualityState> equalityFunc,
SerializedFunction<EqualityState, FormatterFunc> formatterFunc) {
return createLazy(name, () -> roundTrip, equalityFunc, formatterFunc);
}

/**
* @param name
* The name of the formatter step
Expand All @@ -115,7 +161,7 @@ public final String format(String rawUnix, File file) throws Exception {
* only the state supplied by state and nowhere else.
* @return A FormatterStep
*/
public static <State extends Serializable> FormatterStep createLazy(
static <State extends Serializable> FormatterStep createLazy(
String name,
ThrowingEx.Supplier<State> stateSupplier,
ThrowingEx.Function<State, FormatterFunc> stateToFormatter) {
Expand All @@ -132,7 +178,7 @@ public static <State extends Serializable> FormatterStep createLazy(
* only the state supplied by state and nowhere else.
* @return A FormatterStep
*/
public static <State extends Serializable> FormatterStep create(
static <State extends Serializable> FormatterStep create(
String name,
State state,
ThrowingEx.Function<State, FormatterFunc> stateToFormatter) {
Expand All @@ -149,7 +195,7 @@ public static <State extends Serializable> FormatterStep create(
* @return A FormatterStep which will never report that it is up-to-date, because
* it is not equal to the serialized representation of itself.
*/
public static FormatterStep createNeverUpToDateLazy(
static FormatterStep createNeverUpToDateLazy(
String name,
ThrowingEx.Supplier<FormatterFunc> functionSupplier) {
return new FormatterStepImpl.NeverUpToDate(name, functionSupplier);
Expand All @@ -163,7 +209,7 @@ public static FormatterStep createNeverUpToDateLazy(
* @return A FormatterStep which will never report that it is up-to-date, because
* it is not equal to the serialized representation of itself.
*/
public static FormatterStep createNeverUpToDate(
static FormatterStep createNeverUpToDate(
String name,
FormatterFunc function) {
Objects.requireNonNull(function, "function");
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
/*
* 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.
* 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.spotless;

import java.io.File;
import java.io.Serializable;
import java.util.Arrays;

import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;

/**
* Standard implementation of FormatterStep which cleanly enforces
* separation of a lazily computed "state" object whose serialized form
* is used as the basis for equality and hashCode, which is separate
* from the serialized form of the step itself, which can include absolute paths
* and such without interfering with buildcache keys.
*/
@SuppressFBWarnings("SE_TRANSIENT_FIELD_NOT_RESTORED")
abstract class FormatterStepEqualityOnStateSerialization<State extends Serializable> implements FormatterStep, Serializable {
private static final long serialVersionUID = 1L;

protected abstract State stateSupplier() throws Exception;

protected abstract FormatterFunc stateToFormatter(State state) throws Exception;

private transient FormatterFunc formatter;
private transient State stateInternal;
private transient byte[] serializedStateInternal;

@Override
public String format(String rawUnix, File file) throws Exception {
if (formatter == null) {
formatter = stateToFormatter(state());
}
return formatter.apply(rawUnix, file);
}

@Override
public boolean equals(Object o) {
if (o == null) {
return false;
} else if (getClass() != o.getClass()) {
return false;
} else {
return Arrays.equals(serializedState(), ((FormatterStepEqualityOnStateSerialization<?>) o).serializedState());
}
}

@Override
public int hashCode() {
return Arrays.hashCode(serializedState());
}

void cleanupFormatterFunc() {
if (formatter instanceof FormatterFunc.Closeable) {
((FormatterFunc.Closeable) formatter).close();
formatter = null;
}
}

private State state() throws Exception {
if (stateInternal == null) {
stateInternal = stateSupplier();
}
return stateInternal;
}

private byte[] serializedState() {
if (serializedStateInternal == null) {
serializedStateInternal = ThrowingEx.get(() -> LazyForwardingEquality.toBytes(state()));
}
return serializedStateInternal;
}
}
Loading

0 comments on commit 26fbd03

Please sign in to comment.