Skip to content

Commit

Permalink
Update to Scalafmt 3.8.3 (#1631)
Browse files Browse the repository at this point in the history
* Update to Scalafmt 3.8.3

Ensures that all Scalafmt targets succeed under every supported Scala
version. Part of #1482.

Scala 2.11 remains on Scalafmt 2.7.5, since there's not a more recent
compatible version.

Between 3.0.0 and 3.8.3, Scalafmt's `FileOps` moved packages,
`Config.fromHoconFile` moved to `ScalafmtConfig`, and the methods now
take a `java.nio.file.Path` parameter. As a result, this required
extracting a thin `ScalafmtAdapter` object, with one implementation for
Scala 2.11 and Scalafmt 2.7.5, and one for Scalafmt 3.8.3.

This change also adds the file path to the `Scalafmt.format()` call,
allowing error messages to show the actual file path instead of
`<input>`.

Removed the `verticalMultiline.newlineBeforeImplicitKW = true` and
`unindentTopLevelOperators = false` options from `.scalafmt.conf`.
They don't appear to be available in Scalafmt 3.8.3.

Also removes some `@io_bazel_rules_scala` references to make the
internal implementation less dependendent on that name. This will allow
Bzlmod clients to use `rules_scala` in a `bazel_dep()` without setting
`repo_name = "io_bazel_rules_scala"`.

---

Scalafmt 3.0.0 works with the current default Scala version 2.12.19,
but breaks under Scala 2.13.14:

```txt
$ bazel test --repo_env=SCALA_VERSION=2.13.14 //test/scalafmt/...

ERROR: .../test/scalafmt/BUILD:49:20:
  ScalaFmt test/scalafmt/test/scalafmt/unformatted/unformatted-test.scala.fmt.output
  failed: (Exit 1): scalafmt failed: error executing command
  (from target //test/scalafmt:unformatted-test)
  bazel-out/darwin_arm64-opt-exec-2B5CBBC6/bin/scala/scalafmt/scalafmt
  '--jvm_flag=-Dfile.encoding=UTF-8' ... (remaining 1 argument skipped)

java.util.NoSuchElementException: last of empty IndexedSeq
    at scala.collection.IndexedSeqOps.last(IndexedSeq.scala:110)
    at scala.collection.IndexedSeqOps.last$(IndexedSeq.scala:105)
    at scala.meta.tokens.Tokens.last(Tokens.scala:29)
    at org.scalafmt.internal.Router.$anonfun$getSplitsImpl$90(Router.scala:707)
    at scala.Option.map(Option.scala:242)
    at org.scalafmt.internal.Router.getSplitsImpl(Router.scala:706)
    at org.scalafmt.internal.Router.getSplits(Router.scala:2314)
    at org.scalafmt.internal.BestFirstSearch.$anonfun$routes$1(BestFirstSearch.scala:38)
    [ ...snip... ]
```

This matches:

- scala/community-build#1680

Which mentions apparent fixes in:

- scalameta/scalameta#3235
- scalameta/scalafmt#3581

So the fix was to upgrade the Scalafmt version. That said, I held its
Scalameta dependencies to 4.9.9 per the following link, even though
4.10.2 is out now.

- https://mvnrepository.com/artifact/org.scalameta/scalafmt-core_2.13/3.8.3

---

This change updates Scalameta to version 4.9.9 because between 4.9.9 and
4.10.2, Scalafmt breaks Scala 3 file formatting by unindenting some code
that it shouldn't. Or, our usage of it breaks somehow; I can't find any
open or closed issues in the Scalameta project that matches what happes
in rules_scala. (Perhaps I can file one eventually.)

- https://github.com/scalameta/scalafmt/issues

The solution was to keep the Scalameta dependencies at 4.9.9. FWIW, this
was one of the most time consuming bugs to pinpoint and rectify in the
entire Bzlmodification process.

This was the failing command inside `test_cross_version`:

```txt
$ bazel run //scalafmt:unformatted-test3.format-test

INFO: Analyzed target //scalafmt:unformatted-test3.format-test
  (0 packages loaded, 0 targets configured).
INFO: From ScalaFmt scalafmt/scalafmt/unformatted/unformatted-test3.scala.fmt.output:
```

The `test_cross_version/scalafmt/unformatted/unformatted-test3.scala`
file formatted by this test target looks like this:

```scala
import org.scalatest.flatspec._

class Test extends
  AnyFlatSpec:

        "Test"  should  "be formatted" in  {
     assert(true)
        }
```

I hacked `ScalaWorker.format()` to print the `code` variable, and could
see that  after the first `Scalafmt.format()` pass, the code looks like
this:

```scala
import org.scalatest.flatspec._

class Test extends AnyFlatSpec:

"Test" should "be formatted" in {
  assert(true)
}
```

Since the result doesn't match the original code, it tries to call
`Scalafmt.format()` again on this "formatted" code with the incorrect
indentation. That's when we get the following, which doesn't look
anything like the original file:

```txt
Unable to format file due to bug in scalafmt
scalafmt/unformatted/unformatted-test3.scala:3:
  error: [dialect scala3 [with overrides]] `;` expected but `:` found
class Test extends AnyFlatSpec:
                              ^
```

---

As it turns out, bumping to com.google.protobuf:protobuf-java:4.28.2
alone breaks the bump to Scalafmt 3.8.3. Then bumping to rules_proto
6.0.2, with the separate protobuf v21.7, fixes it, presumably by
recompiling `protoc`.

The in-between breakage happened in the `test_cross_build` project:

```txt
$ bazel build //scalafmt:formatted-binary2

INFO: Analyzed target //scalafmt:formatted-binary2
  (4 packages loaded, 64 targets configured).
ERROR: .../test_cross_build/scalafmt/BUILD:59:22:
  ScalaFmt scalafmt/scalafmt/formatted/formatted-binary2.scala.fmt.output
  failed: Worker process did not return a WorkResponse:

---8<---8<--- Start of log, file at .../bazel-workers/worker-3-ScalaFmt.log ---8<---8<---
Exception in thread "main" java.lang.NoSuchMethodError:
  'void com.google.devtools.build.lib.worker.WorkerProtocol$WorkRequest.makeExtensionsImmutable()'
    at com.google.devtools.build.lib.worker.WorkerProtocol$WorkRequest.<init>(WorkerProtocol.java:1029)
    at com.google.devtools.build.lib.worker.WorkerProtocol$WorkRequest.<init>(WorkerProtocol.java:922)
    at com.google.devtools.build.lib.worker.WorkerProtocol$WorkRequest$1.parsePartialFrom(WorkerProtocol.java:2482)
    at com.google.devtools.build.lib.worker.WorkerProtocol$WorkRequest$1.parsePartialFrom(WorkerProtocol.java:2476)
    at com.google.protobuf.AbstractParser.parsePartialFrom(AbstractParser.java:192)
    at com.google.protobuf.AbstractParser.parsePartialDelimitedFrom(AbstractParser.java:232)
    at com.google.protobuf.AbstractParser.parseDelimitedFrom(AbstractParser.java:244)
    at com.google.protobuf.AbstractParser.parseDelimitedFrom(AbstractParser.java:249)
    at com.google.protobuf.AbstractParser.parseDelimitedFrom(AbstractParser.java:25)
    at com.google.protobuf.GeneratedMessage.parseDelimitedWithIOException(GeneratedMessage.java:367)
    at com.google.devtools.build.lib.worker.WorkerProtocol$WorkRequest.parseDelimitedFrom(WorkerProtocol.java:1438)
    at io.bazel.rulesscala.worker.Worker.persistentWorkerMain(Worker.java:81)
    at io.bazel.rulesscala.worker.Worker.workerMain(Worker.java:49)
    at io.bazel.rules_scala.scalafmt.ScalafmtWorker$.main(ScalafmtWorker.scala:12)
    at io.bazel.rules_scala.scalafmt.ScalafmtWorker.main(ScalafmtWorker.scala)
---8<---8<--- End of log ---8<---8<---
Target //scalafmt:formatted-binary2 failed to build
```

* Add attributes to .scalafmt.conf files

Per suggestions from @WojciechMazur in #1631.

* Bump Scala 2 libs after #1636

Brings all the Scalafmt 3.8.3 updates from #1631 up to date with the
Scala version bumps from #1636.
  • Loading branch information
mbland authored Oct 29, 2024
1 parent ca27760 commit 190fbee
Show file tree
Hide file tree
Showing 19 changed files with 928 additions and 553 deletions.
7 changes: 4 additions & 3 deletions .scalafmt.conf
Original file line number Diff line number Diff line change
@@ -1,16 +1,17 @@
version = "3.8.3"
runner.dialect = scala213
align.openParenCallSite = false
align.openParenDefnSite = false
continuationIndent.defnSite = 2
danglingParentheses.preset = true
docstrings.style = Asterisk
importSelectors = singleLine
maxColumn = 120
verticalMultiline.newlineBeforeImplicitKW = true
rewrite.redundantBraces.stringInterpolation = true
rewrite.rules = [
RedundantParens,
PreferCurlyFors,
SortImports
]
unindentTopLevelOperators = false
lineEndings=preserve
indentOperator.topLevelOnly = true
lineEndings = preserve
24 changes: 15 additions & 9 deletions scala/private/macros/scala_repositories.bzl
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive")
load(
"@io_bazel_rules_scala//scala:scala_cross_version.bzl",
"//scala:scala_cross_version.bzl",
"extract_major_version",
"extract_minor_version",
"version_suffix",
_default_maven_server_urls = "default_maven_server_urls",
)
load("//third_party/repositories:repositories.bzl", "repositories")
load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive")
load("@io_bazel_rules_scala_config//:config.bzl", "SCALA_VERSIONS")

def _dt_patched_compiler_impl(rctx):
Expand Down Expand Up @@ -69,15 +69,19 @@ def _validate_scalac_srcjar(srcjar):
def dt_patched_compiler_setup(scala_version, scala_compiler_srcjar = None):
scala_major_version = extract_major_version(scala_version)
scala_minor_version = extract_minor_version(scala_version)
patch = "@io_bazel_rules_scala//dt_patches:dt_compiler_%s.patch" % scala_major_version
patch = Label("//dt_patches:dt_compiler_%s.patch" % scala_major_version)

minor_version = int(scala_minor_version)

if scala_major_version == "2.12":
if minor_version >= 1 and minor_version <= 7:
patch = "@io_bazel_rules_scala//dt_patches:dt_compiler_%s.1.patch" % scala_major_version
patch = Label(
"//dt_patches:dt_compiler_%s.1.patch" % scala_major_version,
)
elif minor_version <= 11:
patch = "@io_bazel_rules_scala//dt_patches:dt_compiler_%s.8.patch" % scala_major_version
patch = Label(
"//dt_patches:dt_compiler_%s.8.patch" % scala_major_version,
)

build_file_content = "\n".join([
"package(default_visibility = [\"//visibility:public\"])",
Expand Down Expand Up @@ -182,14 +186,16 @@ def _artifact_ids(scala_version):
"io_bazel_rules_scala_scala_parser_combinators",
"org_scalameta_semanticdb_scalac",
] if scala_version.startswith("2") else [
"io_bazel_rules_scala_scala_library",
"io_bazel_rules_scala_scala_asm",
"io_bazel_rules_scala_scala_compiler",
"io_bazel_rules_scala_scala_compiler_2",
"io_bazel_rules_scala_scala_interfaces",
"io_bazel_rules_scala_scala_library",
"io_bazel_rules_scala_scala_library_2",
"io_bazel_rules_scala_scala_parser_combinators",
"io_bazel_rules_scala_scala_reflect_2",
"io_bazel_rules_scala_scala_tasty_core",
"io_bazel_rules_scala_scala_asm",
"io_bazel_rules_scala_scala_xml",
"io_bazel_rules_scala_scala_parser_combinators",
"io_bazel_rules_scala_scala_library_2",
"org_scala_sbt_compiler_interface",
]

Expand Down
15 changes: 8 additions & 7 deletions scala/scalafmt/BUILD
Original file line number Diff line number Diff line change
@@ -1,12 +1,10 @@
load("//scala/scalafmt/toolchain:setup_scalafmt_toolchain.bzl", "setup_scalafmt_toolchains")
load("//scala/scalafmt/toolchain:toolchain.bzl", "export_scalafmt_deps")
load("//scala/scalafmt:phase_scalafmt_ext.bzl", "scalafmt_singleton")
load("//scala:scala.bzl", "scala_binary")
load("//scala:scala_cross_version.bzl", "version_suffix")
load("//scala/scalafmt/toolchain:toolchain.bzl", "export_scalafmt_deps")
load("//scala/scalafmt/toolchain:setup_scalafmt_toolchain.bzl", "setup_scalafmt_toolchains")
load("//scala:scala_cross_version_select.bzl", "select_for_scala_version")
load("@io_bazel_rules_scala_config//:config.bzl", "SCALA_VERSION")
load(
"//scala/scalafmt:phase_scalafmt_ext.bzl",
"scalafmt_singleton",
)

filegroup(
name = "runner",
Expand All @@ -22,7 +20,10 @@ filegroup(

scala_binary(
name = "scalafmt",
srcs = ["scalafmt/ScalafmtWorker.scala"],
srcs = ["scalafmt/ScalafmtWorker.scala"] + select_for_scala_version(
before_2_12_0 = ["scalafmt/ScalafmtAdapter-2_11.scala"],
since_2_12_0 = ["scalafmt/ScalafmtAdapter.scala"],
),
main_class = "io.bazel.rules_scala.scalafmt.ScalafmtWorker",
visibility = ["//visibility:public"],
deps = [
Expand Down
14 changes: 14 additions & 0 deletions scala/scalafmt/scalafmt/ScalafmtAdapter-2_11.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
package io.bazel.rules_scala.scalafmt

import java.io.File
import org.scalafmt.config.Config
import org.scalafmt.config.ScalafmtConfig
import org.scalafmt.util.FileOps

object ScalafmtAdapter {
def readFile(file: File)(implicit codec: scala.io.Codec): String =
FileOps.readFile(file)(codec)

def parseConfigFile(configFile: File): ScalafmtConfig =
Config.fromHoconFile(configFile).get
}
13 changes: 13 additions & 0 deletions scala/scalafmt/scalafmt/ScalafmtAdapter.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
package io.bazel.rules_scala.scalafmt

import java.io.File
import org.scalafmt.config.ScalafmtConfig
import org.scalafmt.sysops.FileOps

object ScalafmtAdapter {
def readFile(file: File)(implicit codec: scala.io.Codec): String =
FileOps.readFile(file.toPath())(codec)

def parseConfigFile(configFile: File): ScalafmtConfig =
ScalafmtConfig.fromHoconFile(configFile.toPath()).get
}
12 changes: 7 additions & 5 deletions scala/scalafmt/scalafmt/ScalafmtWorker.scala
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,6 @@ import io.bazel.rulesscala.worker.Worker
import java.io.File
import java.nio.file.Files
import org.scalafmt.Scalafmt
import org.scalafmt.config.Config
import org.scalafmt.util.FileOps
import scala.annotation.tailrec
import scala.io.Codec

Expand All @@ -18,12 +16,16 @@ object ScalafmtWorker extends Worker.Interface {
val argFile = args.map{x => new File(x)}
val namespace = argName.zip(argFile).toMap

val source = FileOps.readFile(namespace.getOrElse("input", new File("")))(Codec.UTF8)
val sourceFile = namespace.getOrElse("input", new File(""))
val source = ScalafmtAdapter.readFile(sourceFile)(Codec.UTF8)

val configFile = namespace.getOrElse("config", new File(""))
val config = ScalafmtAdapter.parseConfigFile(configFile)

val config = Config.fromHoconFile(namespace.getOrElse("config", new File(""))).get
@tailrec
def format(code: String): String = {
val formatted = Scalafmt.format(code, config).get
val filePath = sourceFile.getPath()
val formatted = Scalafmt.format(code, config, Set.empty, filePath).get
if (code == formatted) code else format(formatted)
}

Expand Down
63 changes: 42 additions & 21 deletions scala/scalafmt/scalafmt_repositories.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -30,46 +30,67 @@ def scalafmt_default_config(path = ".scalafmt.conf", **kwargs):
scalafmt_config(name = "scalafmt_default", path = "//:" + path, **kwargs)

_SCALAFMT_DEPS = [
"com_geirsson_metaconfig_core",
"com_geirsson_metaconfig_typesafe_config",
"com_google_protobuf_protobuf_java",
"com_lihaoyi_fansi",
"com_lihaoyi_fastparse",
"com_lihaoyi_sourcecode",
"com_thesamet_scalapb_lenses",
"com_thesamet_scalapb_scalapb_runtime",
"com_typesafe_config",
"org_scala_lang_modules_scala_collection_compat",
"org_scala_lang_scalap",
"org_scalameta_common",
"org_scalameta_fastparse",
"org_scalameta_fastparse_utils",
"org_scalameta_parsers",
"org_scalameta_scalafmt_core",
"org_scalameta_scalameta",
"org_scalameta_trees",
"org_typelevel_paiges_core",
"com_typesafe_config",
"org_scala_lang_scalap",
"com_thesamet_scalapb_lenses",
"com_thesamet_scalapb_scalapb_runtime",
"com_lihaoyi_fansi",
"com_lihaoyi_fastparse",
"org_scalameta_fastparse_utils",
"org_scala_lang_modules_scala_collection_compat",
]

_SCALAFMT_DEPS_2_11 = [
"com_lihaoyi_pprint",
"com_lihaoyi_sourcecode",
"com_google_protobuf_protobuf_java",
"com_geirsson_metaconfig_core",
"com_geirsson_metaconfig_typesafe_config",
"org_scalameta_fastparse",
"org_scalameta_fastparse_utils",
]

def _artifact_ids(scala_version):
_SCALAFMT_DEPS_2_12 = [
"com_geirsson_metaconfig_pprint",
"org_scalameta_mdoc_parser",
"org_scalameta_scalafmt_config",
"org_scalameta_scalafmt_sysops",
]

def scalafmt_artifact_ids(scala_version):
major_version = extract_major_version(scala_version)
geny = ["com_lihaoyi_geny"] if major_version != "2.11" else []
parallel_collections = ["io_bazel_rules_scala_scala_parallel_collections"] if major_version == "2.13" or major_version.startswith("3") else []
return _SCALAFMT_DEPS + geny + parallel_collections

if major_version == "2.11":
return _SCALAFMT_DEPS + _SCALAFMT_DEPS_2_11

extra_deps = []

if major_version == "2.12":
extra_deps.append("com_github_bigwheel_util_backports")
else:
extra_deps.append("io_bazel_rules_scala_scala_parallel_collections")

return _SCALAFMT_DEPS + _SCALAFMT_DEPS_2_12 + extra_deps

def scalafmt_repositories(
maven_servers = _default_maven_server_urls(),
overriden_artifacts = {}):
overriden_artifacts = {},
bzlmod_enabled = False):
for scala_version in SCALA_VERSIONS:
repositories(
scala_version = scala_version,
for_artifact_ids = _artifact_ids(scala_version),
for_artifact_ids = scalafmt_artifact_ids(scala_version),
maven_servers = maven_servers,
overriden_artifacts = overriden_artifacts,
)
_register_scalafmt_toolchains()

if not bzlmod_enabled:
_register_scalafmt_toolchains()

def _register_scalafmt_toolchains():
for scala_version in SCALA_VERSIONS:
Expand Down
25 changes: 12 additions & 13 deletions scala/scalafmt/toolchain/setup_scalafmt_toolchain.bzl
Original file line number Diff line number Diff line change
@@ -1,18 +1,9 @@
load("//scala/scalafmt/toolchain:toolchain.bzl", "scalafmt_toolchain")
load("//scala/scalafmt:scalafmt_repositories.bzl", "scalafmt_artifact_ids")
load("//scala:providers.bzl", "declare_deps_provider")
load("//scala:scala_cross_version.bzl", "version_suffix")
load("@io_bazel_rules_scala_config//:config.bzl", "SCALA_VERSIONS")

_SCALAFMT_DEPS = [
"@com_geirsson_metaconfig_core",
"@org_scalameta_common",
"@org_scalameta_parsers",
"@org_scalameta_scalafmt_core",
"@org_scalameta_scalameta",
"@org_scalameta_trees",
"@org_scala_lang_modules_scala_collection_compat",
]

def setup_scalafmt_toolchain(
name,
scalafmt_classpath,
Expand All @@ -32,9 +23,14 @@ def setup_scalafmt_toolchain(
)
native.toolchain(
name = name,
target_settings = ["@io_bazel_rules_scala_config//:scala_version" + version_suffix(scala_version)],
target_settings = [
"@io_bazel_rules_scala_config//:scala_version" +
version_suffix(scala_version),
],
toolchain = ":%s_impl" % name,
toolchain_type = "//scala/scalafmt/toolchain:scalafmt_toolchain_type",
toolchain_type = Label(
"//scala/scalafmt/toolchain:scalafmt_toolchain_type",
),
visibility = visibility,
)

Expand All @@ -47,4 +43,7 @@ def setup_scalafmt_toolchains():
)

def _deps(scala_version):
return [dep + version_suffix(scala_version) for dep in _SCALAFMT_DEPS]
return [
"@" + artifact_id + version_suffix(scala_version)
for artifact_id in scalafmt_artifact_ids(scala_version)
]
4 changes: 3 additions & 1 deletion test/scalafmt/.scalafmt.conf
Original file line number Diff line number Diff line change
@@ -1,2 +1,4 @@
version = "3.8.3"
runner.dialect = scala213
maxColumn = 40
lineEndings=preserve
lineEndings = preserve
4 changes: 3 additions & 1 deletion test_cross_build/scalafmt/.scalafmt2.conf
Original file line number Diff line number Diff line change
@@ -1,2 +1,4 @@
version = "3.8.3"
runner.dialect = scala213
maxColumn = 40
lineEndings=preserve
lineEndings = preserve
5 changes: 3 additions & 2 deletions test_cross_build/scalafmt/.scalafmt3.conf
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
runner.dialect=scala3
version = "3.8.3"
runner.dialect = scala3
maxColumn = 40
lineEndings=preserve
lineEndings = preserve
4 changes: 2 additions & 2 deletions test_cross_build/scalafmt/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ scalafmt_scala_binary(
scala_version = "2.12.20",
)

scalafmt_scala_library(
scalafmt_scala_binary(
name = "formatted-binary2",
srcs = ["formatted/formatted-binary2.scala"],
config = ":scala2-conf",
Expand All @@ -74,7 +74,7 @@ scalafmt_scala_binary(
scala_version = "3.2.2",
)

scalafmt_scala_library(
scalafmt_scala_binary(
name = "formatted-binary3",
srcs = ["formatted/formatted-binary3.scala"],
config = ":scala3-conf",
Expand Down
Loading

0 comments on commit 190fbee

Please sign in to comment.