Change order of core.Parser's type parameters to work with -Ypartial-unification

[mberndt123]

Hey there,

One of the more recent developments in the Scala language is the -Ypartial-unification flag, which essentially allows you to treat types with multiple type parameters as if they were curried. You could for instance declare a method like this:def f[F[_], A](fa: F[A]). If you pass an Either[Int, String] to f, it won't compile, because Either is a binary type constructor and F is unary. -Ypartial-unification fixes that. With this flag, scalac will infer that F[X] = Either[Int, X] and A = String. But if you pass a core.Parser[Foo, String, Char] type, it will infer F[X] = Parser[Foo, String, X] and A = Char. That's bad, because usually you'd want that to be F[X] = core.Parser[X, String, Char] and A = Foo, because then F would be an instance of all sorts of type classes. Therefore I suggest to change the order of the type parameters to Parser[Elem, Repr, +T]. It just makes sense given the direction in which the Scala language seems to be evolving.

[olafurpg]

Would this be a binary breaking change? Fastparse is used by a large number of libraries which means binary breaking change to the core API will require a synchronized upgrade for all downstream libraries.

[mberndt123]

I don't think so, because type parameters are implemented via erasure. It would require rewriting the source code, but that can be done in an automated fashion with scalafix.

[mberndt123]

Actually, I have already messed around a bit with scalafix to automate this: https://github.com/mberndt123/fastparse-new-order It mostly works, but it has one small bug: When using parsers in a variadic argument list, it doesn't work. It seems to be a bug in scalameta. Any way, the vast majority of uses should be covered.

Of course, if you decide to go ahead with this change, some other types (e. g. core.Parsed and its subtypes, as well as many subtypes of core.Parser) should be adapted as well for consistency.

[lihaoyi]

w.r.t. binary compatibility, if we're worried someone should just go and run mima on the jar...

[mberndt123]

I'll take a look at running mima on this to find out if there are binary compatibility issues. I might get to it over the weekend.

Fyi, I have already messed around a bit with scalafix to automate the source changes: https://github.com/mberndt123/fastparse-new-order It mostly works, except for a small bug which is caused by a bug in scalameta's semanticdb.

Of course, if you decide to go ahead with this change, some other types (e. g. core.Parsed and its subtypes, as well as many subtypes of core.Parser) should be adapted as well for consistency. And maybe the Scalafix needs to be expanded to handle cases where somebody calls a case class' companion object's apply method while specifying type parameters explicitly, e. g. Either[Foo, Elem, Repr](bar, baz). I can't think of a reason why anybody would do that right now though…

[olafurpg]

I can prioritize fixing the variadic argument bug in scalameta if we go ahead with this. I suspect the change is binary compatible since the type parameters are erased. We should configure MiMa in the build to verify though.

[mberndt123]

I've added the MiMa configuration in my branch (#179) and it doesn't find any binary compatibility issues when running fastparseJVM/mimaReportBinaryIssues. I also tried some bogus changes that break compatibility and they were detected, so I think we can move forward here 😃. It might also be a good idea to add this task to the CI configuration.

[olafurpg]

Great news @mberndt123 This would still be a source breaking change which merits a v2.0 release if we're strict on semver. But v1.0 and v2.0 should be able to live in the same classpath at least. I have no personal opinion on the change itself as I'm not even sure what -Ypartial-unification does 😅

[mberndt123]

OK, I have reworked my branch (#179) to reorder the type parameters everywhere in this way for consistency. It's 24 classes in total. I have also written a scalafix that will automatically rewrite code that needs to be changed to this new order – in fact it's what I used to adapt most of the code in fastparse itself! There are some corner cases that aren't handled correctly, e. g.

val e = fastparse.parsers.Combinators.Either // assign the companion object to a val
e[A, B, C]() // should be rewritten to e[B, C, A]

or

def none[M[_, _, _]]: Option[M[A, B, C]] =
  None
none[fastparse.core.Parser] // should be rewritten to none[({type F[A, B, C] = Parser[B, C, A]})#F]

Handling these might be possible, but it's almost certainly more trouble than it's worth – I'd be surprised if any such code existed in the wild.

I ran MiMa and everything is still binary compatible.

@olafurpg , I explained what -Ypartial-unification does when I first opened the bug. If that explanation is unclear, feel free to ask. On the question of whether a new major version is necessary:

• due to erasure, the ABI hasn't changed
• most people probably use the type aliases in fastparse.Api and hence aren't affected by this change
• the remaining users can do the necessary changes automatically with scalafix

So yeah, it's technically an API break, but I think it's defensible to not increment the major version number in this case.

[mberndt123]

Hey, is there any chance that this will be considered for merging?

[lihaoyi]

Hi,

As mentioned in the gitter, I haven’t had time to look at this much recently. It’ll likely get merged eventually, but if your use case is urgent you could publish it yourself for now

[mberndt123]

Ok, cool, thanks for responding

[mberndt123]

@lihaoyi , given that 4 months have passed, is there any chance to get this change merged?

[lihaoyi]

This is moot now that all the type parameters are gone