Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Start making each FormatterStep roundtrip serializable. #1945

Merged
merged 27 commits into from
Jan 23, 2024
Merged
Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
eb0df83
StepHarness (and WithFile) now have a `supportsRoundTrip` method so w…
nedtwigg Dec 4, 2023
e3843cd
`StepHarnessBase` is now embarked on a methodical journey to make eve…
nedtwigg Dec 4, 2023
cb346a4
Introduce a new `FormatterStep` base class for the serialized roundtr…
nedtwigg Dec 4, 2023
decbd1b
Convert `IndentStep` to be roundtrip serializable.
nedtwigg Dec 4, 2023
317b2ac
Remove the long-deprecated and completely unused `Set<String> mavenCo…
nedtwigg Dec 4, 2023
e4d6c49
Introduce `FileSignature.RoundTrippable` and `JarState.Promised` so t…
nedtwigg Dec 5, 2023
6f7edd3
Make `DiktatStep` round-trippable.
nedtwigg Dec 5, 2023
9fa1da3
Fix spotbugs warnings.
nedtwigg Dec 5, 2023
cce20d4
Rename `FileSignature.RoundTrippable` to `FileSignature.Promised` to …
nedtwigg Dec 5, 2023
56bb67e
More improvements to `StepHarnessWithFile`.
nedtwigg Dec 6, 2023
c56968a
Add a roundtrip-serializable method to `FormatterStep`.
nedtwigg Dec 6, 2023
46635c6
Fixup the `FormatterStep` api.
nedtwigg Dec 6, 2023
f5f6836
Use the new API.
nedtwigg Dec 6, 2023
44b3126
Fixup spotbugs warnings.
nedtwigg Dec 6, 2023
55481fa
`FileSignature.Promised` and `JarState.Promised` should both have `ge…
nedtwigg Dec 6, 2023
ab1452d
Streamline the `FileSignature` api.
nedtwigg Dec 6, 2023
4e54fc2
Update the `CONTRIBUTING.md` for our new roundtrip serializable world.
nedtwigg Dec 6, 2023
5a05beb
Merge branch 'main' into serializable-refactor
nedtwigg Dec 15, 2023
005113e
Remove unneeded `public` declarations.
nedtwigg Dec 15, 2023
ce34f89
Fix various typos found by @jbduncan
nedtwigg Jan 23, 2024
d102709
merge main (DiktatStep conflict)
nedtwigg Jan 23, 2024
3bd4243
Copyright updates.
nedtwigg Jan 23, 2024
19aff77
Merge branch 'main' into serializable-refactor
nedtwigg Jan 23, 2024
4fdb178
Remove unused method.
nedtwigg Jan 23, 2024
14991e4
Update changelog.
nedtwigg Jan 23, 2024
4b15b39
Fix an oversight from the merge conflict earlier.
nedtwigg Jan 23, 2024
93eb099
Merge branch 'main' into serializable-refactor
nedtwigg Jan 23, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 22 additions & 1 deletion CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -95,10 +95,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 decribed in the next section.
nedtwigg marked this conversation as resolved.
Show resolved Hide resolved
- [ ] 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 serialied 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
nedtwigg marked this conversation as resolved.
Show resolved Hide resolved

- `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-2023 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();
}

nedtwigg marked this conversation as resolved.
Show resolved Hide resolved
/**
* 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
34 changes: 33 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-2023 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,35 @@ 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;
nedtwigg marked this conversation as resolved.
Show resolved Hide resolved
@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 static Promised promise(Iterable<File> files) {
return new Promised(MoreIterables.toNullHostileList(files), null);
}
nedtwigg marked this conversation as resolved.
Show resolved Hide resolved

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
2 changes: 2 additions & 0 deletions lib/src/main/java/com/diffplug/spotless/Formatter.java
Original file line number Diff line number Diff line change
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();
nedtwigg marked this conversation as resolved.
Show resolved Hide resolved
}
}
}
Expand Down
64 changes: 55 additions & 9 deletions lib/src/main/java/com/diffplug/spotless/FormatterStep.java
Original file line number Diff line number Diff line change
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.
nedtwigg marked this conversation as resolved.
Show resolved Hide resolved
*/
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.
nedtwigg marked this conversation as resolved.
Show resolved Hide resolved
*/
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-2023 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
Loading