-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Expand kind-projector's syntax for polymorphic function values #3193
Expand kind-projector's syntax for polymorphic function values #3193
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Thanks @LukaJCB, hopefully it will work this time. 😄 |
Codecov Report
@@ Coverage Diff @@
## master #3193 +/- ##
=========================================
- Coverage 93.34% 93.1% -0.24%
=========================================
Files 378 377 -1
Lines 7692 7677 -15
Branches 206 206
=========================================
- Hits 7180 7148 -32
- Misses 512 529 +17
Continue to review full report at Codecov.
|
I think you should be able to, see https://github.com/sbt/sbt/blob/e5e462f2c2c659a4700c6fb757ea83d3d8ac0329/internal/util-collection/src/main/scala/sbt/internal/util/INode.scala#L36 where I'm using one of the synthetic type parameter names. |
I'm definitely 👎 on this; I find this syntax to be absolutely indispensable. I get that some people find it confusing though, and Dotty compatibility is a real concern, so I won't stand in the way of its removal, I'm just definitely not in favor. |
Idk, in my view the fact that you can surface and rely on an undocumented implementation detail like that is much worse than not being able to refer to the type at all. |
@djspiewak My goal for this set of PRs is to make Dotty-friendly changes that also improve or at least don't hurt readability on Scala 2 now. If there's disagreement on this one I'm happy to put it on hold. |
At least in my opinion, this hurts readability a lot. Like I said, I present that as a subjective view, and I don't think there's anything wrong with your points that, for those who aren't familiar with it, it may be a bit magical. That's technically true about everything in kind-projector though. Technically speaking, if Dotty is getting polymorphic SAMs (as @smarter mentioned), then the kind-projector lambda syntax is still compatible, since we can just define a |
Just refreshed this branch and made the same changes for cats-free and the tests as I had for the other modules. @djspiewak I don't exactly see how polymorphic SAMs in Dotty will help us have a shared version of the clean syntax here, because of this part:
Even supposing we will have polymorphic SAMs on Dotty, I don't see how we could write a That would also mean that we're forced to wait on significant changes with no specific timeline in both Dotty and kind-projector before we can have Cats cross-building on Dotty in master. I'm inclined not to do that, and to merge this sooner rather than later. We can always undo these expansions if someday we get a nicer solution that works on both Scala 2 and Dotty. |
Fun fact: you can instantly crash the Dotty repl if you enter the following text: def λ[F[a[_], b[_]]] = new {
def apply[A[_], B[_]](f: F[A, B]): F[A, B] = f
} Mostly it appears the Anyway, a bit of fiddling convinced me that you're right, barring any witchcraft in Scala 3 of which I am not currently aware. We really would need something pretty closely approximating polykinds in order to do this. In fact, by using the skolems polykinding trick, I was able to get this to work… in Scala 2.13: trait PolyParent {
type F0[A]
type G0[A]
}
trait FunctionK[F[_], G[_]] {
type F0[A] = F[A]
type G0[A] = G[A]
def apply[A](fa: F[A]): G[A]
}
object Skolem {
type τ
}
def λ[K <: PolyParent](f: K#F0[Skolem.τ] => K#G0[Skolem.τ]): FunctionK[K#F0, K#G0] =
new FunctionK[K#F0, K#G0] {
def apply[A](fa: K#F0[A]): K#G0[A] = f(fa.asInstanceOf).asInstanceOf
} So that's fun. It actually does the right things. But trying to do this in Dotty gives me a At any rate, I am (sadly) convinced that you're probably right and we need to remove the expansions. I withdraw my objection. |
@djspiewak For what it's worth I think Dotty should actually be fine with your first definition—you're probably seeing a bug that affects some synthetic names in the first line you type in the REPL. It's supposed to have been fixed since 0.22.0-RC1, but I've gotten in the habit of typing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Strongly in favour. As @travisbrown, I was never a fan of that piece of syntax.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand what that syntax does, but use it infrequently enough to need to remind myself every time I encounter it. And Dotty support carries the argument for me.
I've always hated this specific piece of kind-projector syntax. It feels very different from everything else in Scala in that it's not even possible to annotate the function argument with its type, because the type parameter is entirely synthetic. I find it confusing whenever I notice it, and I'm familiar with it, so I'm assuming it confuses other people too. Maybe it has a place in application code but I don't personally think it belongs in a library like Cats.
Now that we're thinking about Dotty compatibility (see #3192) I have a good excuse to propose getting rid of it entirely. For this PR I ran this Scalafix rule (which is a slightly modified version of the one here) and then manually fixed a few instances where
A
collided with another type parameter.