com-lihaoyi / fastparse

Writing Fast Parsers Fast in Scala
https://com-lihaoyi.github.io/fastparse
MIT License
1.09k stars 164 forks source link

Scala 3 support #271

Closed lihaoyi closed 1 year ago

lihaoyi commented 1 year ago

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 (https://github.com/com-lihaoyi/fastparse/pull/252) @rmgk (https://github.com/com-lihaoyi/fastparse/pull/262) and @ajrnz (https://github.com/com-lihaoyi/fastparse/pull/266)

Notable Changes:

  1. Moved MIMA checking only to the core fastparse modules; I don't think we can promise the same stability guarantees for the example parsers
  2. Tweaked the source file config in each module to allow per-platform-per-version sources, and extended that ability to test sources.
  3. All the .map(Foo) calls have to become .map(Foo.apply), .map(Foo.tupled) calls have to become (Foo.apply _).tupled
  4. Duplicated package.scala for Scala 2 and 3, but splitting out the shared non-macro logic into SharedPackageDefs.scala
  5. All macros had to be re-implemented in fastparse/src-3/
  6. All implicits had to be given explicit return types
  7. Fixed a bug in aggregateMsgInRep where we were not properly propagating failure strings
  8. then has to be back-ticked
  9. Replaced all scala.Symbols in PythonParse with Strings
  10. Replaced the _ method in ScalaParse with Underscore
  11. 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
lihaoyi commented 1 year ago

@lolgab looks like it's been exactly a year to the day you started this: https://github.com/com-lihaoyi/fastparse/pull/271/commits/624c8131fabd8112180675281b21b89a84d5df01 landed 1st March 2022, and here we are 1st March 2023. Now just need to do final review and cleanups before we merge/release this

reid-spencer commented 1 year ago

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

lolgab commented 1 year ago

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

lolgab commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

lihaoyi commented 1 year ago

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

lolgab commented 1 year ago

@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 commented 1 year ago

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