Update to Scalafmt 3.8.3

mbland commented 1 month ago

Description

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".

Motivation

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

$ 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:

https://github.com/scala/community-build/issues/1680

Which mentions apparent fixes in:

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:

$ 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:

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:

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:

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 (#1624) breaks the bump to Scalafmt 3.8.3. Then bumping to rules_proto 6.0.2, with the separate protobuf v21.7 (#1623), fixes it, presumably by recompiling protoc.

The in-between breakage happened in the test_cross_build project:

$ 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

WojciechMazur commented 1 month ago

If you've used the scripts/create_repositories.py to update dependencies then scalameta_version defined there should also be updated. Also, I can see Scala version updates. Consider merging #1636 before so we can separate these two upgrades.

mbland commented 1 month ago

@WojciechMazur I didn't use scripts/create_repositories.py, actually—I didn't see that land in #1607 until after I'd tweaked these files by hand. I'm a little scared to try it on the Scalafmt deps, because as mentioned in the description, I locked it to Scalameta 4.9.9 because 4.10.x breaks Scala 3 formatting, and I don't know exactly why. (If you have a tip, or a workaround, I'm all ears.)

That said, I'll checkout a new branch and give it a try, see what happens. I'm also going to prepare another local test branch rebased on #1636 to have it ready. It does seem like it'd be easier to land #1636, then this one.

mbland commented 1 month ago

Wow, rebasing on #1636 worked perfectly, with all tests passing, as well as bazel test --repo_env=SCALA_VERSION=2.13.15 //test/scalafmt/.... I do need to bump the Scala 2 libs in the scala_3_{1,2,3,4,5}.bzl files to 2.13.15 (they're still 2.13.14 right now), but that's a piece of cake.

Since the rebase was so easy, I definitely think #1636 should go in first.

mbland commented 1 month ago

I've created a new mbland/rules_scala/bzlmod-update-scalafmt-deps-after-scala-versions-update branch with commits representing:

My successful rebase onto WojciechMazur/update/scala-versions
My successful bump of Scala 2 libs on top of that
The successful updates from running scripts/create_repository.py after bumping it to Scalatest 3.2.19 and Scalafmt 3.8.3
The unsuccessful updates from the same run

The commit for the successful create_repository.py run passed the Scalafmt tests from ./test_cross_version.sh. The unsuccessful run crashed with the error described in #1624 before the protobuf-java 4.28.2 update.

So I'll be happy to rebase this PR after #1636 lands, and cherry-pick the second and third commits above into it. But I don't yet know what to suggest regarding the downgrades from the last commit.

simuons commented 4 weeks ago

Hi, @mbland, just want to let you know that https://github.com/bazelbuild/rules_scala/pull/1636 was merged.

mbland commented 4 weeks ago

@simuons Thanks! Just rebased, ran some of the tests, and pushed.

I'll send the successful updates from running scripts/create_repository.py that I mentioned earlier in a separate PR after this one lands.

mbland commented 3 weeks ago

@simuons Thanks for the approval on this and #1632, but neither have merged yet. Pinging in case that wasn't intentional.

mbland commented 3 weeks ago

Thanks, @liucijus!

bazelbuild / rules_scala

Update to Scalafmt 3.8.3 #1631

Description

Motivation