Add Scalafix integration

[DavidGregory084]

Currently includes only liancheng/scalafix-organize-imports, could be used later to roll out typelevel-scalafix.

Unfortunately it's not totally straightforward to roll this out as a CI step - e.g. if we consider cats, there are currently some bincompat tricks in place that conflict with the simulacrum-scalafix generated code, so it's currently (deliberately) not in sync with its Scalafix migrations.
Unless we implemented some kind of "redirect" or "migrate" annotation to handle those cases in the simulacrum code by dispatching a deprecated ops method to its replacement, I don't think Github Actions part of this could be enabled in cats by default as it would always produce a diff.

Another concern is that enabling Scalafix in check mode could lead to build failures whenever a Scalafix rule produces code that is not correctly formatted according to scalafmtCheckAll. I have asked on the Scalameta Discord about whether it's possible to run Scalafmt on Scalafix output before running checks.

It's probably easiest to review the first commit here as the second contains the result of running Scalafix import sorting on this repo!

[armanbilge]

So actually after the discussion in #247 (comment) I had an idea to do these differently.

Basically, I think the TypelevelCiPlugin should expose boolean settings for enabling header, fmt, scalafix, and mima checks in CI and then generate the CI steps based on those. Then, builds and/or other plugins can enable those settings (or if we are really clever, they can introspect plugins available on the build and set themselves).

This will also make it easier for an individual build to decide whether to opt-in or opt-out of any of these particular checks.

[armanbilge]

Oh, another thing to note here, is that scalafixAll --check I believe needs a full compile of the project, which is why it's usually called as a later step in CI.

[DavidGregory084]

Ah of course, the semanticdb doesn't exist until then anyway! I guess it ought to be a build step then.

[armanbilge]

I'm going to work on what I described in #274 (comment), been putting it off long enough, and then we can try and re-integrate this PR against those changes. Thanks, and sorry!

[DavidGregory084]

I had a quick look at this and this is where I got to:

diff --git a/ci/src/main/scala/org/typelevel/sbt/TypelevelCiPlugin.scala b/ci/src/main/scala/org/typelevel/sbt/TypelevelCiPlugin.scala
index 335f99a..53385fa 100644
--- a/ci/src/main/scala/org/typelevel/sbt/TypelevelCiPlugin.scala
+++ b/ci/src/main/scala/org/typelevel/sbt/TypelevelCiPlugin.scala
@@ -20,33 +20,87 @@ import sbt._
 import org.typelevel.sbt.gha.GenerativePlugin
 import org.typelevel.sbt.gha.GitHubActionsPlugin
 import org.typelevel.sbt.gha.GenerativePlugin.autoImport._
+import org.typelevel.sbt.TypelevelScalafixPlugin.autoImport._
 import com.typesafe.tools.mima.plugin.MimaPlugin
 import scala.language.experimental.macros
 
 object TypelevelCiPlugin extends AutoPlugin {
 
-  override def requires = GitHubActionsPlugin && GenerativePlugin && MimaPlugin
+  override def requires =
+    GitHubActionsPlugin && GenerativePlugin && MimaPlugin && TypelevelScalafixPlugin
+
   override def trigger = allRequirements
 
   object autoImport {
     def tlCrossRootProject: CrossRootProject = macro CrossRootProjectMacros.crossRootProjectImpl
+
+    val tlCheckHeaders =
+      settingKey[Boolean]("Check for presence of copyright headers in CI (default: true)")
+    val tlCheckFormatting = settingKey[Boolean](
+      "Check code is formatted according to the project's Scalafmt config in CI (default: true)")
+    val tlCheckMigrations = settingKey[Boolean](
+      "Check configured Scalafix migrations have been applied in CI (default: true)")
   }
 
+  import autoImport._
+
   override def buildSettings = Seq(
+    tlCheckHeaders := true,
+    tlCheckFormatting := true,
+    tlCheckMigrations := true,
     githubWorkflowPublishTargetBranches := Seq(),
-    githubWorkflowBuild := Seq(
-      WorkflowStep.Sbt(List("test"), name = Some("Test")),
-      WorkflowStep.Sbt(
-        List("mimaReportBinaryIssues"),
-        name = Some("Check binary compatibility"),
-        cond = Some(primaryJavaCond.value)
-      ),
-      WorkflowStep.Sbt(
-        List("doc"),
-        name = Some("Generate API documentation"),
-        cond = Some(primaryJavaCond.value)
+    githubWorkflowBuild := {
+      val checkHeadersCommands =
+        if (tlCheckHeaders.value) List("headerCheckAll") else Nil
+
+      val checkFormattingCommands =
+        if (tlCheckFormatting.value)
+          List("scalafmtCheckAll", "project /", "scalafmtSbtCheck")
+        else
+          Nil
+
+      val checkMigrationsStep =
+        if (tlCheckMigrations.value)
+          Seq(
+            WorkflowStep.Sbt(
+              List("scalafixAll --check"),
+              name = Some("Check Scalafix migrations"),
+              cond = Some(primaryJavaCond.value)
+            )
+          )
+        else Nil
+
+      val preBuildSteps = Seq(
+        WorkflowStep.Sbt(
+          checkHeadersCommands ++ checkFormattingCommands,
+          name = Some("Check headers and formatting"),
+          cond = Some(primaryJavaCond.value)
+        )
       )
-    ),
+
+      val buildStep = Seq(
+        WorkflowStep.Sbt(List("test"), name = Some("Test"))
+      )
+
+      val postBuildSteps = checkMigrationsStep ++ Seq(
+        WorkflowStep.Sbt(
+          List("mimaReportBinaryIssues"),
+          name = Some("Check binary compatibility"),
+          cond = Some(primaryJavaCond.value)
+        ),
+        WorkflowStep.Sbt(
+          List("doc"),
+          name = Some("Generate API documentation"),
+          cond = Some(primaryJavaCond.value)
+        )
+      )
+
+      Seq(
+        preBuildSteps,
+        buildStep,
+        postBuildSteps
+      ).flatten
+    },
     githubWorkflowJavaVersions := Seq(JavaSpec.temurin("8"))
   )
 
@@ -54,5 +108,4 @@ object TypelevelCiPlugin extends AutoPlugin {
     val java = githubWorkflowJavaVersions.value.head
     s"matrix.java == '${java.render}'"
   }
-
 }

I need to take a break now though and I seem to have lost the artifact upload steps, so I think I will wait to see what your changes look like instead 😆

[armanbilge]

if we consider cats, there are currently some bincompat tricks in place that conflict with the simulacrum-scalafix generated code, so it's currently (deliberately) not in sync with its Scalafix migrations.

IIUC it seems the problem is that some of the configured scalafix rules are not actually ones that we want to run/test in CI.
https://github.com/typelevel/cats/blob/3cb763920f177f045e2253be51e52ea2554770ad/.scalafix.conf

It seems to me we may want to have two separate scalafix configs. We do this in http4s for example so we can relax some of the linting rules for test sources.
https://github.com/http4s/http4s/blob/9ac020a92409ce86ad8d67c08ba4dcd31bb6438b/.scalafix.test.conf

[armanbilge]

Is there a reason we need our own tl setting for this one? It seems reasonable to just append to the existing setting.

[DavidGregory084]

No reason, that just seemed to be the convention in this plugin e.g. tlMimaPreviousVersions 😛 I will remove it

[armanbilge]

Ah, that's because there is no such equivalent setting in MiMa itself :) see #176 (comment).

[armanbilge]

Summoning @rossabaker to tell us how to organize our imports then @valencik can put it into typelevel.g8.

Looking at http4s I guess we are just using the default configuration, since that is best for diffs and merges?
https://github.com/http4s/http4s/blob/9ac020a92409ce86ad8d67c08ba4dcd31bb6438b/.scalafix.conf

[DavidGregory084]

I chose this configuration because although LSP users can Organize Imports straight from their editor, there will always be folks using IntelliJ which has its own way of handling import organisation, so this seems like a good choice to minimise friction for contributors.

[rossabaker]

I might otherwise agree, but the IntelliJ preset's correctness issue concerns me. And since most projects I maintain end up with maintenance branches, I appreciate the care the defaults put into diff reduction.

I won't die on this hill, though.