-
Notifications
You must be signed in to change notification settings - Fork 275
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
Update to Scalafmt 3.8.3 #1631
base: master
Are you sure you want to change the base?
Update to Scalafmt 3.8.3 #1631
Conversation
rewrite.redundantBraces.stringInterpolation = true | ||
rewrite.rules = [ | ||
RedundantParens, | ||
PreferCurlyFors, | ||
SortImports | ||
] | ||
unindentTopLevelOperators = false | ||
lineEndings=preserve | ||
lineEndings = preserve |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unindentTopLevelOperators
can be probably replaced with indentOperator.topLevelOnly
. AFAIR it worked good enough in one project I was migrating in the past from scalafmt 2.7.5 -> 3.8.1
lineEndings = preserve | |
indentOperator.topLevelOnly = true | |
lineEndings = preserve |
In that case we were having a bit reverse direction: unindentTopLevelOperators = true
into indentOperator.topLevelOnly = false
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, thanks! Done
@@ -1,16 +1,15 @@ | |||
runner.dialect = scala213 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be worthy to add version = "3.8.3"
so it's easier to check which version of scalamft we require.
runner.dialect = scala213 | |
version = "3.8.3" | |
runner.dialect = scala213 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, here and in the other .scalafmt.conf
files.
Heh, I had version = "3.8.3"
in at some point in my bzlmod working branch, because Scalafmt failed without it. Then I took it out here because it didn't seem required. But I do like the explicitness of it.
If you've used the |
Per suggestions from @WojciechMazur in bazelbuild#1631.
@WojciechMazur I didn't use 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. |
Wow, rebasing on #1636 worked perfectly, with all tests passing, as well as Since the rebase was so easy, I definitely think #1636 should go in first. |
Per suggestions from @WojciechMazur in bazelbuild#1631.
Brings all the Scalafmt 3.8.3 updates from bazelbuild#1631 up to date with the Scala version bumps from bazelbuild#1636.
Ran at the suggestion of @WojciechMazur in bazelbuild#1631. After running the script, I selected these specific changes with `git add -p`. I was pleased that the script left the Scalameta deps at version 4.9.9, and it bumped the Scalatest versions appropriately. However, there were a number of downgrades and updates to the Scala 2 `deps` in the Scala 3 files that I didn't accept. I'll include them in the next commit on this branch, but do _not_ merge them.
As mentioned in the previous commit, this was the output I rejected from running `scripts/create_repository.py`. These deps were downgraded: - protobuf-java: 4.28.2 to 3.8.0 or 3.7.0 - scalap: from latest Scala version to an earlier version, including 2.13.14 to 2.13.11 - com.lihaoyi.pprint: from pprint_3:0.9.0 to pprint_2.13:0.6.4 - compiler-interface: 1.10.1 to 1.3.5 or 1.9.6 - com.lihaoyi.geny: from geny_3:1.1.1 to geny_2.13:0.6.5 Plus, all the `@io_bazel_rules_scala_scala_*_2` deps explicitly added to the `scala_3_{1,2,3,4,5}.bzl` files in bazelbuild#1631 for Scalafmt were stripped of the `_2` suffix.
I've created a new mbland/rules_scala/bzlmod-update-scalafmt-deps-after-scala-versions-update branch with commits representing:
The commit for the successful 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. |
Per suggestions from @WojciechMazur in bazelbuild#1631.
Ensures that all Scalafmt targets succeed under every supported Scala version. Part of bazelbuild#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 ```
Per suggestions from @WojciechMazur in bazelbuild#1631.
Brings all the Scalafmt 3.8.3 updates from bazelbuild#1631 up to date with the Scala version bumps from bazelbuild#1636.
baf7ed1
to
9879592
Compare
@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. |
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 toScalafmtConfig
, and the methods now take ajava.nio.file.Path
parameter. As a result, this required extracting a thinScalafmtAdapter
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
andunindentTopLevelOperators = 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 userules_scala
in abazel_dep()
without settingrepo_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:
This matches:
Which mentions apparent fixes in:
extends
scalameta/scalafmt#3581So 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.
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.)
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:I hacked
ScalaWorker.format()
to print thecode
variable, and could see that after the firstScalafmt.format()
pass, the code looks like this: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: