Allow to compile with Scala Native #4325

jchyb · 2024-09-24T16:32:54Z

Apologies for a big PR once again.

Vast majority of the additions here consist of the rewritten interfaces to Scala, and it's difficult to test them without the rest of the scala native implementation, so instead of a different PR I decided to put them into a different commit. My only other idea was to split the TermUtils, CliUtils, etc. version dependent implementations to another PR, but I think in those cases it's helpful to see what I was trying to accomplish by having both native and jvm implementations side by side (and they are all very small).

Part of the Scala Native cross compilation effort (#4279)

jchyb · 2024-09-24T16:35:52Z

build.sbt

 // .jsSettings(
 // libraryDependencies ++= List(
 // scalatest.value % Test // must be here for coreJS/test to run anything
 // )
 // )
+ .nativeSettings(libraryDependencies += "com.lihaoyi" %%% "fastparse" % "3.1.0")


This is probably the weirdest part. scalameta-trees seems to use fastparse in its implementation, however I think it does not point to it in the jar metadata? (I cannot find any mention of fastparse there on scaladex either). Either way, without this we get errors about missing symbols when linking.

The annoying part here is keeping the version set to the same one scalameta uses

it's shaded... why is it used by scalafmt?

https://github.com/scalameta/scalameta/blob/main/build.sbt#L783
https://github.com/scalameta/scalameta/blob/main/project/ShadedDependency.scala

also, the version is 3.1.1, i think.

jchyb · 2024-09-24T16:40:22Z

scalafmt-core/native/src/main/scala/org/scalafmt/internal/RegexCompat.scala

+/* Before text matching (?=re), after text matching (?<=re)
+ * and more are incompatible in Scala Native, so custom functions
+ * have to be used.
+ * Scala Native uses an implementation of:
+ * https://github.com/google/re2/wiki/Syntax
+ */
+
+object RegexCompat {
+
+ /* Replaces '\\h', which is incompatible in Scala Native.
+ * Does not check correctness of the input regex string.
+ */
+ private def fixHorizontalSpaceInRegex(reg: String) = {
+
+ @inline
+ val replacingInsideClass =
+ "\t \u00A0\u1680\u180E\u2000-\u200A\u202F\u205F\u3000"
+
+ @inline
+ val replacingOutsideClass = s"[$replacingInsideClass]"
+
+ val sb = new StringBuilder()
+ var isInClass = false
+ var isEscaped = false
+
+ for (char <- reg) char match {
+ case '\\' if !isEscaped => isEscaped = true
+ case 'h' if isEscaped =>
+ sb.append(if (isInClass) replacingInsideClass else replacingOutsideClass)
+ isEscaped = false
+ case '[' if !isEscaped =>
+ sb.append('[')
+ isInClass = true
+ case ']' if !isEscaped =>
+ sb.append(']')
+ isInClass = false
+ case other =>
+ if (isEscaped) {
+ isEscaped = false
+ sb.append('\\')
+ }
+ sb.append(other)
+ }
+ sb.toString()
+ }


This is mostly a reply to a related comment from a previous PR. This method was written mostly because I found \\h replacements specifically very annoying, unintuitive and ugly. But the regexes here have many more differences unfortunately, so just supplying this method to JVM style regexes would not work unfortunately.

kitbellew · 2024-09-24T17:00:07Z

build.sbt

 // .jsSettings(
 // libraryDependencies ++= List(
 // scalatest.value % Test // must be here for coreJS/test to run anything
 // )
 // )
+ .nativeSettings(libraryDependencies += "com.lihaoyi" %%% "fastparse" % "3.1.0")


it's shaded... why is it used by scalafmt?

https://github.com/scalameta/scalameta/blob/main/build.sbt#L783
https://github.com/scalameta/scalameta/blob/main/project/ShadedDependency.scala

also, the version is 3.1.1, i think.

kitbellew · 2024-09-24T17:03:27Z

build.sbt

@@ -284,3 +298,7 @@ lazy val buildInfoSettings: Seq[Def.Setting[_]] = Seq(
 buildInfoPackage := "org.scalafmt",
 buildInfoObject := "Versions",
 )
+
+lazy val scalaNativeNativeConfig = nativeConfig ~= {


why "native" twice?

It's a nativeConfig setting for Scala Native... but I guess singular one still makes more sense, I'll change it to that

kitbellew · 2024-09-24T17:04:33Z

project/plugins.sbt

@@ -15,3 +15,5 @@ addSbtPlugin("org.scala-js" % "sbt-scalajs" % "1.16.0")
 addSbtPlugin("org.portable-scala" % "sbt-scalajs-crossproject" % "1.3.2")
 addSbtPlugin("com.typesafe" % "sbt-mima-plugin" % "1.1.4")
 addSbtPlugin("com.github.sbt" % "sbt-native-packager" % "1.10.4")
+addSbtPlugin("org.portable-scala" % "sbt-scala-native-crossproject" % "1.3.2")


@should we put this a few lines above, next to scalajs variant? and, perhaps, add a variable for "1.3.2".

kitbellew · 2024-09-24T17:07:50Z

scalafmt-cli/native/src/main/scala/org/scalafmt/cli/CliUtils.scala

+ protected val isNative: Boolean = true
+
+ protected def returnDynamicRunner(): Either[String, ScalafmtRunner] = {
+ assert(false, "Code path should be unreachable.")


isn't it redundant, given ??? below?

kitbellew · 2024-09-24T17:10:13Z

scalafmt-cli/shared/src/main/scala/org/scalafmt/cli/Cli.scala

@@ -125,7 +100,7 @@ object Cli {
 case Right(`stableVersion`) =>
 options.common.debug.println(s"Using core runner [$stableVersion]")
 Right(ScalafmtCoreRunner)
- case Right(v) if isNativeImage =>
+ case Right(v) if isNativeImage || isNative =>


why not move isNative directly into isNativeImage? line 64 above:

private val isNativeImage: Boolean = isNative || ...

scalafmt-core/native/src/main/scala/org/scalafmt/internal/RegexCompat.scala

scalafmt-interfaces/native/src/main/java/org/scalafmt/interfaces/Scalafmt.scala

kitbellew · 2024-09-24T17:57:27Z

scalafmt-sysops/native/src/main/scala-2.12/org/scalafmt/CompatCollections.scala

+ // Scala Native does not include ParallelConverters
+ object ParConverters {
+ implicit class XIterable[T](val col: Iterable[T]) extends AnyVal {
+ def compatPar = col


hmm... this will make native version of scalafmt substantially slower than jvm (since it's no longer in parallel), which might generally defeat the purpose....

So back in the previous iteration 3 years ago SN did not have multithreading yet, but even then the single threaded scala native version was usually faster (I imagine this would change at scale). However this comment made me retest performance and I also realized that parallel collections were built into stdlib in scala 2.12, and indeed, they work on Scala Native now. So now, after that change, that would make scala 2.12 on Scala Native the best. Example test:

vl-d-1450:main jchyb$ time java --class-path scalafmt.jar org.scalafmt.cli.Cli Reformatting... 100,0% [##########] 700 source files formatted real 0m2,787s user 0m17,050s sys 0m0,961s vl-d-1450:main jchyb$ time ./scalafmt-cli2 Reformatting... 100.0% [##########] 700 source files formatted real 0m1,024s user 0m2,016s sys 0m0,365s vl-d-1450:main jchyb$ time ./scalafmt-cli3 Reformatting... 100.0% [##########] 700 source files formatted real 0m2,056s user 0m1,611s sys 0m0,207s

So that makes even the Scala Native version without parallelization better (again, I imagine this would change with a bigger project). Since the 2.12 built-in version works, I can't imagine porting scala-parallel-collections being difficult, so once that is done, the CollectionCompat implementations could be replaced and merged (I added a related comment).

very interesting, thank you for running this experiment. i have added a CommunitySuiteb test which is going to do a lot and is a bit slow, any way you could suggest a method to run it in scala native instead, as part of the GitHub build?

kitbellew · 2024-09-24T18:00:01Z

scalafmt-sysops/shared/src/main/scala/org/scalafmt/sysops/FileOps.scala

- Try(new URL(filename)) match {
+ if (PlatformCompat.isNative) readFile(getFile(filename))
+ else Try(new URL(filename)) match {
 case Success(url) => readFile(url)
 case _ => readFile(getFile(filename))
 }


how's this:

val urlOpt = if (PlatformCompat.isNative) None else Try(new URL(filename)).toOption urlOpt.fold(readFile(getFile(filename))(readFile)

kitbellew · 2024-09-24T18:00:53Z

scalafmt-tests/jvm/src/test/scala/org/scalafmt/ScalafmtProps.scala

@@ -19,12 +19,12 @@ class ScalafmtProps extends FunSuite with FormatAssertions {
 def getBugs(
 config: ScalafmtConfig = ScalafmtConfig.default,
 count: Int = Int.MaxValue,
- ): mutable.Seq[(CorpusFile, Observation[Bug])] = {
+ ): mutable.Seq[(scala.meta.testkit.CorpusFile, Observation[Bug])] = {


why is this package qualification necessary?

Ah, sorry, leftover I have missed, removing now

kitbellew

I'm probably reviewing too early since it's marked a draft.

kitbellew · 2024-09-25T12:34:25Z

scalafmt-cli/shared/src/main/scala/org/scalafmt/cli/Cli.scala

- System.getProperty("scalafmt.native-image", "false")
+ private val isNative: Boolean =
+ ("true" == System.getProperty("scalafmt.native-image", "false")) ||
+ isScalaNative


nitpick: isScalaNative seems cheaper to compute, could put it first 🙂

kitbellew · 2024-09-25T12:39:52Z

scalafmt-cli/shared/src/main/scala/org/scalafmt/cli/CliOptions.scala

- if (parsed.noStdErr || !usesOut) parsed.common.out
- else parsed.common.err,
- )
+ val writerMaybe = if (usesOut) getConsoleWriter() else None


please change to consWriter or consWriterOpt, or, if you really prefer, maybeCons[ole]Writer. other than a few standard geeky suffixes like Opt a variable or class name should be a correct English noun phrase (and methods a verb phrase).

kitbellew · 2024-09-25T12:44:11Z

scalafmt-core/native/src/main/scala/org/scalafmt/internal/RegexCompat.scala

+ val start = matcher.start()
+ val end = matcher.end()
+
+ sb.append(baseText.substring(currPosition, start))


if you had used a java StringBuilder, you could have used the append method which takes offset and length so doesn't need to create a copy of the substring.

scalafmt-interfaces/native/src/main/java/org/scalafmt/interfaces/Scalafmt.scala

kitbellew · 2024-09-25T12:47:57Z

scalafmt-sysops/jvm/src/main/scala-2.12/org/scalafmt/CompatCollections.scala

+private[scalafmt] object CompatCollections {
+ val JavaConverters = scala.collection.JavaConverters
+ object ParConverters {
+ implicit class XIterable[T](val col: Iterable[T]) extends AnyVal {


i think in scalameta code there's a convention of what to call these implicit extension classes. i forgot but you can easily find existing examples. I thought it was a bit longer than just X, with all due respect to Shmelon Shmusk.

kitbellew · 2024-09-25T14:08:37Z

@jchyb windows native git test fails, looks like argument "dir 1" isn't properly passed in sys.process.Process called in GitOps.tryExec (ends up being two separate arguments).

kitbellew · 2024-09-25T16:26:26Z

@jchyb what does this mean:

[info] Discovered 9996 classes and 65197 methods after classloading
[info] Loaded 0 service provider(s) for 1 referenced service(s):
[info] |------------------------------------------------------------------|
[info] | Service                              | Service Provider | Status |
[info] |------------------------------------------------------------------|
[info] | java.nio.charset.spi.CharsetProvider | ---              | NoProviders |
[info] |------------------------------------------------------------------|

(https://github.com/scalameta/scalafmt/actions/runs/11036880214/job/30656386467?pr=4325)

jchyb · 2024-09-25T17:44:03Z

This relates to how Scala Native implements java.util.ServiceLoader service providers, where it requires them to be preconfigured (in the META-INF files, or nativeLink settings). As I understand it, during linking it checks for existence of ServiceLoader.load(classOf[SomeService]) and tries to find all of the related providers, which have to be specified separately. Since there is a ServiceLoader.load[Charset] in the java library reimplementation code, we get this message here about what it managed to find. Not something we should be worried about, since basic charsets like UTF_8, etc. are already built-in and do not require a provider.

kitbellew · 2024-09-25T20:49:00Z

scalafmt-tests/shared/src/test/scala/org/scalafmt/config/ScalafmtConfigTest.scala

+ |newlines.topLevelBodyIfMinStatements = [before,after]
+ |fileOverride {
+ | "glob:${PlatformCompat
+ .fixPathOnNativeWindows("**/src/test/scala/**")}" {


can users still specify paths using forward slash? the config file is shared between platforms, and they can't have one for windows and another for unix.

Turns out the problem I was trying to fix here was found elsewhere. I had to remove 2 uses of .asFilename compatibility tool (one used on fileOverride, one for include/excludePaths), as it messed with scala native's glob implementation. That asFilename method replaced unix file separator with Windows one, but it seems to work even on JVM just as well without it

kitbellew · 2024-09-25T20:49:25Z

scalafmt-tests/shared/src/test/scala/org/scalafmt/cli/CliTest.scala

@@ -859,14 +868,16 @@ class CliTest extends AbstractCliTest with CliTestBehavior {
 |/.scalafmt.conf
 |version = $stableVersion
 |maxColumn = 2
- |project { includePaths = ["glob:**/bar.scala"] }
+ |project { includePaths = ["glob:${PlatformCompat


same concern about using slashes.

kitbellew · 2024-09-25T20:50:19Z

scalafmt-tests/shared/src/test/scala/org/scalafmt/cli/CliTest.scala

@@ -295,8 +298,14 @@ trait CliTestBehavior {
 "--config-str",
 s"""{version="$version",style=IntelliJ}""",
 )
- intercept[IOException] {
- Cli.exceptionThrowingMainWithOptions(args, baseCliOptions)
+ if(PlatformCompat.isNativeOnWindows()) {


formatting issue (needs a space). you should run ./scalafmt in your local checkout directory before amending the commit.

kitbellew · 2024-09-25T20:52:56Z

scalafmt-cli/shared/src/main/scala/org/scalafmt/cli/InputMethod.scala

@@ -76,8 +76,8 @@ object InputMethod {
 file.writeFile(text)(options.encoding)

 override protected def list(options: CliOptions): Unit = {
- val cwd = options.cwd.toUri
- options.common.out.println(cwd.relativize(file.toUri))
+ val cwd = options.cwd.jfile.toPath


jfile.toPath is path.toFile.toPath. why not just use path?

scalafmt-core/shared/src/main/scala/org/scalafmt/config/ProjectFiles.scala

Adds Regexes for scala native, splits up uses of certain Collections, System.console, and metaconfig. Also moves some tests to a JVM only directory.

jchyb · 2024-09-27T16:36:39Z

@kitbellew Thank you so much for all of the reviews! I'll try to look into running communitytest stuff on native next week

kitbellew · 2024-09-27T17:15:13Z

@jchyb and thank you both for your submission and for the patience.

kitbellew · 2024-09-27T17:17:35Z

@jchyb as to community code, i am not sure you have to. it is more to test that formatting rules don't break on actual code, as the existing unit tests are but a small subset. i kept only one jvm version, and i am not even sure we should keep the windows run.

@tgodzik wdyt?

tgodzik · 2024-09-27T21:17:47Z

Unless we find a specific issue in the future that we wouldn't discover otherwise I think is good keeping as is.

Great work @jchyb !

jchyb commented Sep 24, 2024

View reviewed changes

jchyb force-pushed the scala-native/cross-compile branch from 1f9775e to b2864f4 Compare September 24, 2024 16:47

kitbellew requested changes Sep 24, 2024

View reviewed changes

jchyb force-pushed the scala-native/cross-compile branch 2 times, most recently from 26b5729 to 247e582 Compare September 25, 2024 12:38

kitbellew requested changes Sep 25, 2024

View reviewed changes

jchyb force-pushed the scala-native/cross-compile branch from 247e582 to 1e10509 Compare September 25, 2024 13:38

jchyb force-pushed the scala-native/cross-compile branch 2 times, most recently from 9123873 to ae6514e Compare September 25, 2024 16:06

jchyb force-pushed the scala-native/cross-compile branch from ae6514e to d294347 Compare September 25, 2024 19:01

kitbellew requested changes Sep 25, 2024

View reviewed changes

jchyb force-pushed the scala-native/cross-compile branch 8 times, most recently from b87a491 to 9b9699a Compare September 26, 2024 13:33

jchyb marked this pull request as ready for review September 26, 2024 14:06

kitbellew reviewed Sep 26, 2024

View reviewed changes

scalafmt-core/shared/src/main/scala/org/scalafmt/config/ProjectFiles.scala Show resolved Hide resolved

jchyb force-pushed the scala-native/cross-compile branch from 9b9699a to c624520 Compare September 27, 2024 11:38

Rewrite interfaces to scala for Scala Native

5c88293

jchyb force-pushed the scala-native/cross-compile branch from c624520 to 1b488f1 Compare September 27, 2024 11:49

Allow to compile with Scala Native

28fb625

Adds Regexes for scala native, splits up uses of certain Collections, System.console, and metaconfig. Also moves some tests to a JVM only directory.

jchyb force-pushed the scala-native/cross-compile branch from 1b488f1 to 28fb625 Compare September 27, 2024 13:54

kitbellew approved these changes Sep 27, 2024

View reviewed changes

kitbellew merged commit f0724f3 into scalameta:main Sep 27, 2024
17 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow to compile with Scala Native #4325

Allow to compile with Scala Native #4325

jchyb commented Sep 24, 2024

jchyb Sep 24, 2024

jchyb Sep 24, 2024

kitbellew Sep 24, 2024

jchyb Sep 24, 2024

kitbellew Sep 24, 2024

kitbellew Sep 24, 2024

jchyb Sep 25, 2024

kitbellew Sep 24, 2024

kitbellew Sep 24, 2024

kitbellew Sep 24, 2024

kitbellew Sep 24, 2024

jchyb Sep 25, 2024

kitbellew Sep 25, 2024

kitbellew Sep 24, 2024

kitbellew Sep 24, 2024

jchyb Sep 25, 2024

kitbellew left a comment

kitbellew Sep 25, 2024

kitbellew Sep 25, 2024

kitbellew Sep 25, 2024

kitbellew Sep 25, 2024

kitbellew commented Sep 25, 2024

kitbellew commented Sep 25, 2024

jchyb commented Sep 25, 2024

kitbellew Sep 25, 2024

jchyb Sep 26, 2024

kitbellew Sep 25, 2024

kitbellew Sep 25, 2024

kitbellew Sep 25, 2024

jchyb commented Sep 27, 2024

kitbellew commented Sep 27, 2024

kitbellew commented Sep 27, 2024 •

edited

Loading

tgodzik commented Sep 27, 2024

Allow to compile with Scala Native #4325

Allow to compile with Scala Native #4325

Conversation

jchyb commented Sep 24, 2024

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

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

kitbellew 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

kitbellew commented Sep 25, 2024

kitbellew commented Sep 25, 2024

jchyb commented Sep 25, 2024

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

jchyb commented Sep 27, 2024

kitbellew commented Sep 27, 2024

kitbellew commented Sep 27, 2024 • edited Loading

tgodzik commented Sep 27, 2024

kitbellew commented Sep 27, 2024 •

edited

Loading