Scala 3 support #271

lihaoyi · 2023-02-28T16:23:19Z

All tests pass on Scala 3, on all platforms (JVM/JS/Native), and on all old versions of Scala.

Pulls in a bunch of work by @lolgab (#252) @rmgk (#262) and @ajrnz (#266)

Notable Changes:

Moved MIMA checking only to the core fastparse modules; I don't think we can promise the same stability guarantees for the example parsers
Tweaked the source file config in each module to allow per-platform-per-version sources, and extended that ability to test sources.
All the .map(Foo) calls have to become .map(Foo.apply), .map(Foo.tupled) calls have to become (Foo.apply _).tupled
Duplicated package.scala for Scala 2 and 3, but splitting out the shared non-macro logic into SharedPackageDefs.scala
All macros had to be re-implemented in fastparse/src-3/
All implicits had to be given explicit return types
Fixed a bug in aggregateMsgInRep where we were not properly propagating failure strings
then has to be back-ticked
Replaced all scala.Symbols in PythonParse with Strings
Replaced the _ method in ScalaParse with Underscore
Replaced some residual usage of uTests' 'foo - syntax with test("foo") -

Performance

Performance seems to have taken about a 10% hit in Scala 3. Probably missing out on some optimizations that we do in Scala 2. I'm not super familiar with how scala 3 macros work, clawing back the 10% can come in a follow up PR

Scala 2 Bench

------------------ Running Tests perftests.string.ScalaParse ------------------
ScalaParse Benchmark
Max time - 10000 ms. Iterations - 5.
Iteration 1
Benchmark 0. Result: 984
Benchmark 1. Result: 82
Iteration 2
Benchmark 0. Result: 632
Benchmark 1. Result: 88
Iteration 3
Benchmark 0. Result: 618
Benchmark 1. Result: 96
Iteration 4
Benchmark 0. Result: 625
Benchmark 1. Result: 99
Iteration 5
Benchmark 0. Result: 596
Benchmark 1. Result: 99
984 82
632 88
618 96
625 99
596 99

Scala 3 Bench

------------------ Running Tests perftests.string.ScalaParse ------------------
ScalaParse Benchmark
Max time - 10000 ms. Iterations - 5.
Iteration 1
Benchmark 0. Result: 899
Benchmark 1. Result: 62
Iteration 2
Benchmark 0. Result: 535
Benchmark 1. Result: 86
Iteration 3
Benchmark 0. Result: 545
Benchmark 1. Result: 85
Iteration 4
Benchmark 0. Result: 547
Benchmark 1. Result: 86
Iteration 5
Benchmark 0. Result: 538
Benchmark 1. Result: 85
899 62
535 86
545 85
547 86
538 85

This commit does some preparation work to allow the port to Scala 3 The idea is to move all the code using macros to the `src-2` directory, update the dependencies to the ones supporting Scala 3 ( mill, geny, sourcecode, utest ), avoid depending on `scala-reflect` if it's Scala 3 and fixing the compile errors the Scala 3 compiler gives before reaching the missing macros.

Skip scala-compiler dependency for Scala 3

using instead of implicit seems to work out

reid-spencer · 2023-02-28T16:37:32Z

Thank you all for managing to get this to the finish line.

lolgab · 2023-02-28T17:30:30Z

@lihaoyi Thank you for this ❤️

A note about the failing MiMa check:

The MiMa check is failing since there are no artifacts yet for Scala 3.
You can disable the check for scala 3 with:

diff --git a/build.sc b/build.sc
index c8a1041..f6edab1 100644
--- a/build.sc
+++ b/build.sc
@@ -176,6 +176,7 @@ trait CommonCrossModule extends CrossScalaModule with PublishModule with Mima{
       .lastTag
       .getOrElse(throw new Exception("Missing last tag"))
   )
+  def mimaPreviousArtifacts = if(isScala3(crossScalaVersion)) Agg.empty[Dep] else super.mimaPreviousArtifacts()
   def mimaBinaryIssueFilters = super.mimaBinaryIssueFilters() ++ Seq(
     ProblemFilter.exclude[IncompatibleResultTypeProblem]("fastparse.Parsed#Failure.unapply")
   )

This way we can ensure that we are not breaking the binary compatibility for the Scala 2 artifacts.

rmgk · 2023-02-28T18:39:48Z

pythonparse/test/src/pythonparse/UnitTests.scala

@@ -3,7 +3,7 @@ package pythonparse
 import utest._
 import fastparse._
 /**
- * Tests to cover most basic syntactic constructs. It's likely there are
+ * Tests to cover most basic syntactic constructs. ItSymbol("s") likely there are


I feel like this search/replace error was my fault at some point, but someone else fixed it, but now it’s back? 😅

lolgab · 2023-03-01T09:12:16Z

Thank you for taking over @lihaoyi :)
There are some small details that can be fixed in the future, so I'm more than happy to release a new fastparse 2.x version with the long awaited Scala 3 support.
Good call on restricting Mima to the fastparse module only.
The little details I'm thinking about are the usage of package object in Scala 3 (which I think is deprecated) and the .rep which become .rep(), if you read the original PR code and description, there were some overloads that didn't work on Scala 3, but the linked dotty issue is now closed. I'm wondering if they can be reintroduced now so in Scala 3 you can also do .rep.
Anyways, I don't think these should block the release, since we waited already for too long.

rmgk · 2023-03-01T10:32:17Z

The little details I'm thinking about are the usage of package object in Scala 3 (which I think is deprecated) and

The tour of Scala says that:

In Scala 2 top-level method, type and variable definitions had to be wrapped in a package object. These are still usable in Scala 3 for backwards compatibility.

So seems to be fine for the use here.

Edit, the doc does consider them as deprecated. But does not mention any strategy for compatibility.

the .rep which become .rep(), if you read the original PR code and description, there were some overloads that didn't work on Scala 3, but the linked dotty issue is now closed. I'm wondering if they can be reintroduced now so in Scala 3 you can also do .rep. Anyways, I don't think these should block the release, since we waited already for too long.

If I remember correctly, the only remaining issue was that there was an overload for rep(min) that did some performance optimization. I prioritized API compatibility in my original pull request. It seems plausible that the overload could work now that the Scala bug is fixed.

lihaoyi · 2023-03-01T12:07:53Z

Looks like bare .rep and .repX work now. Not sure when that happened, but I reverted the extra ()s we added and things still seem to pass

lolgab

LGTM!
Thank you @lihaoyi for taking this to the finish line! And, of course, thank you to @rmgk for all the work to port the macro code to Scala 3!
Congratulations 🎉
Looking forward to have complete Scala 3 support in Ammonite and Mill after this!

smarter · 2023-03-01T16:46:32Z

fastparse/src-3/fastparse/internal/MacroRepImpls.scala

+      val repeater1   = $repeater
+      var originalCut = ctx1.cut
+      val acc         = repeater1.initial
+      @ _root_.scala.annotation.tailrec


FWIW, you don't need to bother with fully-qualified names in Scala 3 macros, they're hygienic.

reid-spencer · 2023-03-01T17:36:04Z

build.sc

 import de.tobiasroeser.mill.vcs.version.VcsVersion
 import $ivy.`com.github.lolgab::mill-mima::0.0.13`
 import com.github.lolgab.mill.mima._

+val scala31 = "3.1.3"


Can we get to 3.2.2?

reid-spencer

LGTM!

Thanks all who've been involved in making this happen. This is a big deal! Much appreciated!

lihaoyi · 2023-03-02T00:21:08Z

@lolgab shall we tag a new version now that this is merged? We can perform further optimizations and improvements in follow ups

lolgab · 2023-03-02T02:56:11Z

@lolgab shall we tag a new version now that this is merged? We can perform further optimizations and improvements in follow ups

We can.
@lefou Since we bumped geny and other dependencies to a new major, this should also be a new major, right?
In this case I would check if your next PR is backward compatible, to avoid having to release a 4.0.0 right after. To do so locally you can publish the lib locally, change mimaPreviousVersions to the snapshot version, apply your next PR and then run mimaReportBinaryIssues on the Scala 2 modules.

lihaoyi · 2023-03-02T04:05:33Z

If it's a new major, we should try to squeeze in more breaking changes if possible. e.g. the whitespace PR I have open is definitey binary incompatible

… implicit conversions (#272) Fixes #261, which is caused by the whitespace `P[_] => P[Unit]` implicits firing as implicit conversions rather than the implicit parameters they were originally intended to be. Since whitespace is meant to ignore failures, this caused failures caused by the `Fail.! : P[String]` to be ignored when implicitly converted to `P[Unit]` The fix is to create a proper `Whitespace` trait to inherit from. Note that this is a binary incompatible change. It's stacked on top of #271 for convenience, but should probably land separately after. The relevant changes are in this commit d87ab6d if anyone wants to look. Note that I moved the custom whitespace tests to a `scala2.12+` folder to skip them on 2.11. 2.11 does not support SAM conversion, which makes defining custom whitespaces a lot more boilerplatey. Can still be done, but no need to burden everyone with boilerplatey examples just to cater for the 2.11 folks in the test suite. Things are unlikely to break just for 2.11 anyway

lolgab and others added 30 commits March 1, 2022 12:52

Fix testFramework by extending TestModule.Utest

eaa4a0d

Update Scala.js to 1.5.1

4ef9b15

Skip scala-compiler dependency for Scala 3

Use crossScalaVersion instead of scalaVersion

18b6971

Avoid running CI twice on PRs

7a1b4c3

update build dependencies

c7c52c0

copy sources 2

a832aeb

update gitignore

1957b01

replace removed _ identifiers and symbols

9917a64

overload resolution seems to have changed, making plain .rep not work

a274d8c

add non macro implementations for most parsers

6b4027b

remove the last overload, and try with using instead of implicit

3da4b3b

using instead of implicit seems to work out

63f5134

using instead of implicit seems to work out

use same rep impls for scala 2/3

6879cb5

make dynamic decision instead of overloading

4a423ba

fix warning

bd70ff1

adapt pythonparse for scala 3

a57e6cd

port literal string macro

90f51cc

experiment with some inlined methods

3afe0bc

remove unused method

5ffc99a

port string in macro

7971ef4

use inline defs for old inlining macros

db52a6a

rename to better reflect the state of things

c7c91e9

port charsWhileInMacro

b870260

port charInMacro

e09f2fe

minor cleanups

3fc4318

port parsedSequence0 macro

1c895a8

remove some levels of indentation

b822a8d

port rep macro, but not quite sure how to fully use ...

4fccbc0

use plain rep macros, but disable borked overloads

a80ec4c

rmgk reviewed Feb 28, 2023

View reviewed changes

lihaoyi added 6 commits March 1, 2023 07:21

limit mima to fastparse core

4098404

remove symbols in pythonparse

b2bba75

add scala 3 benchmark

2107292

add scala 3 benchmark

624102c

add scala 3 benchmark

46a1b4d

.

350e4ab

lihaoyi mentioned this pull request Mar 1, 2023

Make Whitespace a proper trait rather than function to avoid unwanted implicit conversions #272

Merged

lihaoyi added 2 commits March 1, 2023 20:06

.

e69f150

.

669cdd9

lolgab approved these changes Mar 1, 2023

View reviewed changes

smarter reviewed Mar 1, 2023

View reviewed changes

reid-spencer self-requested a review March 1, 2023 17:35

reid-spencer reviewed Mar 1, 2023

View reviewed changes

reid-spencer approved these changes Mar 1, 2023

View reviewed changes

lihaoyi merged commit ac05071 into master Mar 2, 2023

lefou deleted the scala3 branch March 2, 2023 06:17

lefou added this to the 3.0.0 milestone Mar 2, 2023

This was referenced Mar 2, 2023

SBT version conflict error when fastparse and os-lib are used together #265

Closed

Scala 3 port #246

Closed

scala3parse #250

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Scala 3 support #271

Scala 3 support #271

lihaoyi commented Feb 28, 2023 •

edited

reid-spencer commented Feb 28, 2023

lolgab commented Feb 28, 2023

rmgk Feb 28, 2023 •

edited

lolgab commented Mar 1, 2023 •

edited

rmgk commented Mar 1, 2023 •

edited

lihaoyi commented Mar 1, 2023

lolgab left a comment

smarter Mar 1, 2023

reid-spencer Mar 1, 2023

reid-spencer left a comment

lihaoyi commented Mar 2, 2023

lolgab commented Mar 2, 2023

lihaoyi commented Mar 2, 2023

Scala 3 support #271

Scala 3 support #271

Conversation

lihaoyi commented Feb 28, 2023 • edited

Notable Changes:

Performance

reid-spencer commented Feb 28, 2023

lolgab commented Feb 28, 2023

rmgk Feb 28, 2023 • edited

Choose a reason for hiding this comment

lolgab commented Mar 1, 2023 • edited

rmgk commented Mar 1, 2023 • edited

lihaoyi commented Mar 1, 2023

lolgab left a comment

Choose a reason for hiding this comment

smarter Mar 1, 2023

Choose a reason for hiding this comment

reid-spencer Mar 1, 2023

Choose a reason for hiding this comment

reid-spencer left a comment

Choose a reason for hiding this comment

lihaoyi commented Mar 2, 2023

lolgab commented Mar 2, 2023

lihaoyi commented Mar 2, 2023

lihaoyi commented Feb 28, 2023 •

edited

rmgk Feb 28, 2023 •

edited

lolgab commented Mar 1, 2023 •

edited

rmgk commented Mar 1, 2023 •

edited