fastparse icon indicating copy to clipboard operation
fastparse copied to clipboard

Scala 3 support

Open rmgk opened this issue 2 years ago • 12 comments

Hi,

so there was this Scala 3 branch that seemed like it would work when just substituting some macro implementations. This is my experiment what that would entail. And maybe to start a discussion if its desirable to address the remaining issues, and if so, how.

In short, what works

  • All functionality & macros ported to scala 3.
  • all fastparse.jvm[3.1.2].test pass, except for one error message that has a slightly changed parser name, and I am not sure why.

What does not work / is problematic:

  • ~~.rep has a lot of weird overloads that did not directly work on Scala 3 (and everything became ambiguous), so they are removed for now. This is source incompatible in the sense that one has to write .rep() instead of .rep.~~ This works with using instead of implicit.
  • ~~This does not use macros yet. Lots of methods can probably be justed marked inline to get the old runtime behavior, however, for methods that build some matching logic at compile time like charsWhile in and StringsIn, this PR currently does a super literal translation of the prior logic, which is assumed to be incredibly inefficient.~~ All macros are ported, there are some annoyances with the way Scala 3 changed overload resolution, how fastparse uses overloads, and also https://github.com/lampepfl/dotty/issues/15287, which make it seemingly impossible to use an overload for rep(min) while still remaining API compatibility for the rest. But even the non overloaded version should be fast enough for now.
  • I do know basically nothing about mill, and I did not spend time figuring out how to update dependencies to make building for Scala Native with Scala 3 happen.

rmgk avatar May 19 '22 17:05 rmgk

who might be able to review and/or merge this...?

SethTisue avatar Jul 08 '22 21:07 SethTisue

This PR passed under my radar. Thank you for commenting on my old PR @SethTisue. I'm not familiar with the FastParse Scala 2 macros but I can do a first round of review before someone more experienced with them and with Scala 3 macros can help :) This is great!! @rmgk Thank you!

lolgab avatar Jul 08 '22 22:07 lolgab

I think I messed up 🙈 I deleted my PR with its branch not understanding that your PR was targeting my branch, not the master branch.

lolgab avatar Jul 08 '22 23:07 lolgab

Just wondering what the state of this is please @lihaoyi ?

davidrwood avatar Sep 22 '22 14:09 davidrwood

@rmgk @SethTisue @davidrwood @lolgab - Maybe it is time to consider a fork of this repository? It seems abandoned, yet it is critical to a project I'm working on and the lagging Scala 3 support is holding up our Scala 3 migration. I'm sure that is true for many others who use fastparse.

@lihaoyi - would you like to unburden yourself of the maintenance chore of this library?

reid-spencer avatar Sep 24 '22 16:09 reid-spencer

Anyone who wants commit access can have it. I already have given commit access to several others. No need to fork, you can do it right in this repo if you would like. The only question is who wants to do it. There's about a half dozen individuals with commit access already

If someone wants to push Scala 3 support for Fastparse over the finish line, they are welcome to do so. I personally am stuck on Scala 2.12/2.13, so that person is unlikely to be me for the foreseeable future. The question is not whether I want to unburden myself, but if anyone else would like to pick up the burden

lihaoyi avatar Sep 25 '22 00:09 lihaoyi

@lihaoyi Thanks for replying! Not something I can do myself but from what I can tell this exact PR should do the trick and just needs to be reviewed by someone qualified to do so.

Do you have time check it over perhaps? If so that might be all that's needed.🙂

davidrwood avatar Sep 26 '22 09:09 davidrwood

@lihaoyi - Please add me as a committer. I'm highly invested in Fastparse and want to move the ball forward to Scala 3, so I may just take this on. Thanks.

reid-spencer avatar Sep 26 '22 16:09 reid-spencer

Maybe as a meta comment; the largest additions in this PR are just translations of the Scala 2 macros. For the most part, the Scala 2 macros are used to inline code and the only difference between Scala 2 and 3 is the syntax for inlining – some do some code generation (support for literals and some kinda loop unrolling), these also require mostly syntactic translation. The only likely bugs in this part are because I did some copy & paste errors :-)

The API has very minor differences, specifically, it may be necessary to sometimes add/remove empty parameters lists (rant: Scala support for multiple argument lists, default arguments, and overloading are giant hacks which get exposed when using macros/inline). My gut feeling is, that it is not worth it to add too many workarounds to be fully backwards compatible with the current API (as a comparison, most changes in the test suite are because Scala 3 syntax changes independent of fastparse). But this feels like a “project maintainer” decision :-)

I never managed to run the larger test examples as I have no experience with mill and my updates to Scala 3 did break the full project (only the core library worked in my branch). The branch worked for my medium-sized parser, which is the point where I consider it good enough. Thus, I think getting the tests to run and testing on more projects is what I think the PR needs most currently.

Feel free to ask questions if you need help getting this over the finishing line – though no promises that I will find the time to answer :-)

rmgk avatar Sep 26 '22 17:09 rmgk

@reid-spencer I have sent you an invite for write access to this repository

The goal for Scala 3 support should be to get all test suites in this repo passing, including the "integration tests" exercising ScalaParse/PythonParse/CssParse/etc. That would ensure you get all edge cases nailed down.

We should maintain Scala 2 support, so all those existing tests should continue to pass, to avoid regressions.

Ideally we should preserve backwards compatibility as much as possible, but I think it's OK if you find you need to break some stuff in the interest of Scala 3 support. If that becomes necessary, we can always just label the new version FastParse 3.0.0.

Some APIs would look different in Scala 3. IMO the common def parse[_: P] = P(...) can probably be improved to def parse = P(...) by making P(...) a context function. That would be a nice simplification in the FastParse developer experience, and maybe even help encourage people to migrate

lihaoyi avatar Sep 26 '22 23:09 lihaoyi

@reid-spencer I have sent you an invite for write access to this repository

Invitation accepted. Thank you.

The goal for Scala 3 support should be to get all test suites in this repo passing, including the "integration tests" exercising ScalaParse/PythonParse/CssParse/etc. That would ensure you get all edge cases nailed down.

Thanks for making it easy to achieve the goal

We should maintain Scala 2 support, so all those existing tests should continue to pass, to avoid regressions.

Agreed.

Ideally we should preserve backwards compatibility as much as possible, but I think it's OK if you find you need to break some stuff in the interest of Scala 3 support. If that becomes necessary, we can always just label the new version FastParse 3.0.0.

Hopefully that won't be needed, but thanks for your approval in advance.

Some APIs would look different in Scala 3. IMO the common def parse[_: P] = P(...) can probably be improved to def parse = P(...) by making P(...) a context function. That would be a nice simplification in the FastParse developer experience, and maybe even help encourage people to migrate

It would indeed. The simpler we can make it the better. I expect that would require a 3.0.0 version number unless the older syntax can be retained at the same time.

reid-spencer avatar Sep 28 '22 17:09 reid-spencer

@reid-spencer One more point of note: we do not need to upgrade Scalaparse's business logic to parse Scala 3 syntax as part of this effort, we only need to update its source code to be Scala 3 compatible. The existing Scala 2 test input files should be enough to validate Fastparse itself, for people using Fastparse elsewhere. Making Scalaparse able to parse Scala 3 would be a nice-to-have, but is not strictly necessary for the vast majority of use cases

lihaoyi avatar Sep 28 '22 23:09 lihaoyi