Support Scalafix #591

hnoson · 2020-06-01T12:21:47Z

The PR adds support for Scalafix which is widely used as well as Scalafmt. In addition to ScalaFixStep, I modified ScalaExtension to add a dependency on scala compilation as Scalafix needs compilation before formatting.

nedtwigg

Wow, thanks very much! This PR looks great. It is the first formatter which relies on the File in a meaningful way - previously the File has only been used for name-based things like "is this a module-info or package-info file". There's some mildly tricky stuff we'll have to take on for this to work, which I've described in my feedback below.

nedtwigg · 2020-06-01T18:30:14Z

lib/src/main/java/com/diffplug/spotless/scala/ScalaCompiler.java

+
+	private final String version;
+	private final Set<String> plugins;
+	private final Set<String> compilerOptions;


Up-to-dateness depends on the serialized state of this class, so it is better to use either TreeSet or LinkedHashSet which will both have predictable serialization.

plugin-gradle/README.md

plugin-gradle/src/main/java/com/diffplug/gradle/spotless/ScalaExtension.java

nedtwigg · 2020-06-01T18:44:32Z

plugin-gradle/src/main/java/com/diffplug/gradle/spotless/ScalaExtension.java

+
+		final PluginManager pluginManager = getProject().getPluginManager();
+		if (!pluginManager.hasPlugin("scala")) {
+			pluginManager.apply("scala");


We might want to throw an error here, rather than apply for the user. The reason I say that is that we compute the destinationDirs/scalaJar eagerly below. If users change them after doing spotless { scala {, then those changes will not be picked up, and it will be confusing to debug. An alternative would be to do all of the ScalaCompile configuration in the setupTask part, which runs in an afterEvaluate context. I have no preference which way we go.

nedtwigg · 2020-06-01T18:50:21Z

plugin-gradle/src/main/java/com/diffplug/gradle/spotless/ScalaExtension.java

+	}
+
+	public ScalaFixConfig scalafix(String version, String scalaVersion) {
+		return new ScalaFixConfig(version, scalaVersion);


Can we slurp the scalaVersion from the gradle build? Having any "default" version of scala smells like a bug to me...

nedtwigg · 2020-06-01T18:54:13Z

plugin-gradle/src/main/java/com/diffplug/gradle/spotless/ScalaExtension.java

+
+		private FormatterStep createStep() {
+			File resolvedConfigFile = configFile == null ? null : getProject().file(configFile);
+			return ScalaFixStep.create(version, scalaVersion, GradleProvisioner.fromProject(getProject()), scalaCompiler, resolvedConfigFile);


If we want, we can use FormatterStep.createLazy which would allow scalaVersion to be computed lazily during task execution when the formatter step is first applied.

nedtwigg · 2020-06-01T18:57:23Z

lib/src/main/java/com/diffplug/spotless/scala/ScalaCompiler.java

+	private final String version;
+	private final Set<String> plugins;
+	private final Set<String> compilerOptions;
+	private final String classpath;


This is a string containing the absolute paths of a bunch of jars, correct? It would be better to use FileSignature here, and then do FileSignature.files() to recreate the string in a public method. Besides being slightly safer, this will allow remote buildcache to work once #566 is implemented.

#566 has been implemented, so using FileSignature here will allow remote build cache to work.

nedtwigg · 2020-06-01T19:21:25Z

lib/src/main/java/com/diffplug/spotless/scala/ScalaFixStep.java

+				}
+
+				@Override
+				public String apply(String input, File source) throws Exception {


Currently, we are ignoring String input. The problem is that there might be other steps, say a license header step, which are called before this step, and this will wipe them out. Also, we have an IDE integration with VSCode and others to come, which can format the dirty state of an editor. So a user might type a bunch of code in a dirty buffer, and when they format this will wipe it out.

In the ideal world, we could pass the file to scalafix over stdin without tying it to the sourcetree on disk, but I assume that is not possible for the kinds of stuff that scalafix is doing.

So what we should do instead is:

read the current state of the File source to a byte[], including its lastmodified timestamp (including handling the possibility that it is null or doesn't exist)

write String input to the file
2a) you need the charset, which you don't have. We can add it as a parameter to FormatterFunc, or we can assume UTF-8. The right thing is probably the parameter, and we can do it in a backwards-compatible way.
2b) before writing it out, I would read the file's content to check if there are any changes, since doing that one read will save you two writes

call the formatter

restore the state of the file to what it was like before (possibly deleting if necessary)

This hunk of code will be useful to other formatters that require to read the file from disk. I would put it in lib, and name it something like DiskBasedHelper.

I see. Thanks for your suggestion. Currently, the formatter depends on compilation of whole project which produces semanticdb for each file, and Scalafix uses the metadata. If we can generate the metadata from a single File source, we can avoid the issue by following your idea. ~~Let find a way to do that.~~ It looks like we can compile a file one by one once we create jar file of the target project.

So long as spotless depends on the compilation task, the IDE hook will still call the compile (or jar, whichever). So it's fine to depend on compilation, so long as we can modify our one "being-formatted" file afterwards. Very excited to have a PR of this sort, we'll definitely work with you to get it merged.

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

nedtwigg · 2020-06-23T22:18:32Z

Hi @hnoson. Just wanted to give you some insight into the merge conflicts. We've maintained support back to Gradle 2.x, and we're finally breaking that and adopting all the latest Gradle features. I think your PR is mostly unaffected, except that pretty soon you could use Gradle 5.4+ as the baseline, which might be helpful. My guesstimate is that we'll have removed all deprecated code, and fully bumped the minimum to Gradle 5.4 in ~2 weeks. Part of that effort will be a big overhaul to the README, so there might be some conflicts there as well.

This is a great PR, and I hope to help it get merged in. I think it would be very doable to merge it in with the current Gradle 2.x baseline, but if that's daunting there will be more stability in a few weeks. Just FYI.

Conflicts: plugin-gradle/src/main/java/com/diffplug/gradle/spotless/ScalaExtension.java plugin-gradle/src/test/java/com/diffplug/gradle/spotless/ScalaExtensionTest.java

nedtwigg · 2020-07-13T03:50:11Z

The brief upheaval in Spotless' core has ended. I fixed the merge conflicts and made some small edits to the PR to adjust for the new APIs in Spotless. All of the feedback above still stands.

One big upside is that when you started this PR, our minimum Gradle was 2.14. It is now 5.4 (and I'm open to going higher), so hopefully that will help in integrating with the scala plugin.

hnoson · 2020-07-19T05:04:35Z

Thanks, and sorry for the delay. I believe I can resume working on this next week.

SimY4 · 2022-02-08T06:00:47Z

Hey hey. Any update on this? I'm happy to provide help if you need any.

mdedetrich · 2022-08-24T14:09:05Z

I am also interested in this feature, is it only merge conflicts with main that is necessary to get this PR through? I also noticed that the implementation is using reflection which may expose similar problems that the scalafmt plugin had until #1283 was merged.

nedtwigg · 2022-08-24T21:35:08Z

is it only merge conflicts with main that is necessary to get this PR through?

I don't think there's a huge amount of work left, but there is more than just merge conflicts.

The biggest problem is that AFAICT, scalafix needs the compiled code in order to operate. Which is fine, but we'll need to figure out a way to make spotlessScala depend on scalaCompile or something like that. Not a blocker, but a bit of work. The open code review comments above are all still valid.

the implementation is using reflection

Yeah, probably a good idea to switch to compile-time deps as you did for scalafmt.

This PR as it stands does a great job with the docs and scaffolding the implementation, and I'd love to merge a PR that takes this all the way.

mdedetrich · 2022-08-25T19:53:54Z

The biggest problem is that AFAICT, scalafix needs the compiled code in order to operate. Which is fine, but we'll need to figure out a way to make spotlessScala depend on scalaCompile or something like that. Not a blocker, but a bit of work. The open code review comments above are all still valid.

There is also an additional problem which is that gradle doesn't have a standard way to handle scala. There is gradle-scala plugin however that has quite a few limitations (i.e. it doesn't really support cross compilation) and because of this there are plugins like https://github.com/ADTRAN/gradle-scala-multiversion-plugin.

This PR as it stands does a great job with the docs and scaffolding the implementation, and I'd love to merge a PR that takes this all the way.

I will have to take a deeper look into it but depending on how many things need to be changed I might make a PR myself and co-author @hnoson .

nedtwigg · 2022-08-25T20:26:50Z

Sounds great. Don't feel a need to support every use-case, just supporting your own use-case in a mostly-future-proof way is plenty to get merged. People can extend to support their own use-cases as needed.

nedtwigg · 2024-02-12T23:08:28Z

Closing due to inactivity.

hnoson added 2 commits June 1, 2020 21:33

Support Scalafix

a69cb7d

Update change log

f805cc8

hnoson force-pushed the add_scalafix branch from c5df610 to f805cc8 Compare June 1, 2020 12:35

nedtwigg requested changes Jun 1, 2020

View reviewed changes

nedtwigg mentioned this pull request Jul 2, 2020

FormatterStep.NeedsFile #637

Merged

nedtwigg added 11 commits July 12, 2020 14:59

Merge branch 'master' into add_scalafix

43f00d6

Merge branch 'main' into add_scalafix

b09e3ef

Conflicts: plugin-gradle/src/main/java/com/diffplug/gradle/spotless/ScalaExtension.java plugin-gradle/src/test/java/com/diffplug/gradle/spotless/ScalaExtensionTest.java

Move scalafix to the right place in the changelog.

dd7edba

Do "spotlessApply".

4ee1408

Fallout from the 5.0 change.

c7a4b0b

Revert the changes in StepHarness back to main.

749df07

Adapt ScalaFixStepTest to use StepHarnessWithFile.

cbf54d0

Reduce boilerplate by using FormatterFunc.NeedsFile.

b930a91

Fix rarely-seen error message in FileSignature.

c03942b

Comment out tests for old versions (with an explanation).

d447f96

Resolve wildcard import.

1bc5d3f

nedtwigg mentioned this pull request Jul 12, 2021

Scala: detect unused imports #892

Closed

nedtwigg mentioned this pull request Dec 7, 2021

Use custom source sets and compile-only dependencies instead of reflection #524

Closed

nedtwigg added the waiting on PR submitter label Jan 5, 2022

nedtwigg force-pushed the main branch from 8b73df8 to a0a35b2 Compare January 12, 2023 18:16

nedtwigg closed this Feb 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Scalafix #591

Support Scalafix #591

hnoson commented Jun 1, 2020

nedtwigg left a comment

nedtwigg Jun 1, 2020

nedtwigg Jun 1, 2020

nedtwigg Jun 1, 2020

nedtwigg Jun 1, 2020

nedtwigg Jun 1, 2020

nedtwigg Jul 13, 2020

nedtwigg Jun 1, 2020

hnoson Jun 2, 2020 •

edited

Loading

nedtwigg Jun 2, 2020

nedtwigg commented Jun 23, 2020

nedtwigg commented Jul 13, 2020 •

edited

Loading

hnoson commented Jul 19, 2020

SimY4 commented Feb 8, 2022

mdedetrich commented Aug 24, 2022 •

edited

Loading

nedtwigg commented Aug 24, 2022

mdedetrich commented Aug 25, 2022

nedtwigg commented Aug 25, 2022

nedtwigg commented Feb 12, 2024

Support Scalafix #591

Support Scalafix #591

Conversation

hnoson commented Jun 1, 2020

nedtwigg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hnoson Jun 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nedtwigg commented Jun 23, 2020

nedtwigg commented Jul 13, 2020 • edited Loading

hnoson commented Jul 19, 2020

SimY4 commented Feb 8, 2022

mdedetrich commented Aug 24, 2022 • edited Loading

nedtwigg commented Aug 24, 2022

mdedetrich commented Aug 25, 2022

nedtwigg commented Aug 25, 2022

nedtwigg commented Feb 12, 2024

hnoson Jun 2, 2020 •

edited

Loading

nedtwigg commented Jul 13, 2020 •

edited

Loading

mdedetrich commented Aug 24, 2022 •

edited

Loading