Skip to content
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

Conversation

travisbrown
Copy link
Contributor

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.

LukaJCB
LukaJCB previously approved these changes Dec 4, 2019
Copy link
Member

@LukaJCB LukaJCB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

LukaJCB
LukaJCB previously approved these changes Dec 4, 2019
@travisbrown
Copy link
Contributor Author

Thanks @LukaJCB, hopefully it will work this time. 😄

@codecov-io
Copy link

codecov-io commented Dec 4, 2019

Codecov Report

Merging #3193 into master will decrease coverage by 0.23%.
The diff coverage is 100%.

Impacted file tree graph

@@            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
Flag Coverage Δ
#scala_version_212 ?
#scala_version_213 93.1% <100%> (-0.02%) ⬇️
Impacted Files Coverage Δ
core/src/main/scala/cats/data/NonEmptyList.scala 98.72% <100%> (-0.01%) ⬇️
...src/main/scala-2.13+/cats/instances/lazyList.scala 100% <100%> (ø) ⬆️
...ree/src/main/scala/cats/free/FreeApplicative.scala 98.64% <100%> (ø) ⬆️
...in/scala/cats/data/IndexedReaderWriterStateT.scala 95.23% <100%> (+0.04%) ⬆️
.../main/scala-2.13+/cats/data/NonEmptyLazyList.scala 78.37% <100%> (-0.72%) ⬇️
core/src/main/scala/cats/instances/vector.scala 100% <100%> (ø) ⬆️
core/src/main/scala/cats/instances/list.scala 100% <100%> (ø) ⬆️
core/src/main/scala/cats/instances/either.scala 100% <100%> (ø) ⬆️
core/src/main/scala/cats/data/WriterT.scala 91.4% <100%> (+0.13%) ⬆️
core/src/main/scala/cats/data/OptionT.scala 95.96% <100%> (+0.06%) ⬆️
... and 37 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4351ea3...5003b57. Read the comment docs.

@dwijnand
Copy link
Contributor

dwijnand commented Dec 4, 2019

it's not even possible to annotate the function argument with its type, because the type parameter is entirely synthetic

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.

@djspiewak
Copy link
Member

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.

@travisbrown
Copy link
Contributor Author

@dwijnand

I think you should be able to

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.

@travisbrown
Copy link
Contributor Author

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

@djspiewak
Copy link
Member

djspiewak commented Dec 5, 2019

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 def λ[F[_], G[_]](f: FunctionK[F, G]): FunctionK[F, G] = f helper (not exactly this since it would need to be partially-applied to keep the correct parameter shape, but you get the idea) which allows code to cross-compile. It wouldn't even need to be compiled specifically for Dotty and not for Scala 2, since kind-projector would shadow it.

@travisbrown
Copy link
Contributor Author

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:

not exactly this since it would need to be partially-applied to keep the correct parameter shape, but you get the idea

Even supposing we will have polymorphic SAMs on Dotty, I don't see how we could write a λ there such that λ[K] where K is a FunctionK[F, G] would work. It would be different if kind-projector supported λ[FunctionK, F, G](fa => ???) as a way to construct FunctionK[F, G] values, but that would require changes in kind-projector that I'm not sure anyone will be particularly interested in making (I definitely am not 😄).

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.

@djspiewak
Copy link
Member

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 F[a[_], b[_]] is the culprit, since that shape messes things up in other types as well. Also that shape isn't exactly what I want anyway, but I thought it was a fun discovery.

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 NullPointerException (also it shouldn't work in Dotty due to the unrestricted type projection).

At any rate, I am (sadly) convinced that you're probably right and we need to remove the expansions. I withdraw my objection.

@travisbrown
Copy link
Contributor Author

@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 val x = 1 every time I open a Dotty REPL and it's going to be hard to shake 😄.

Copy link
Contributor

@larsrh larsrh left a 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.

Copy link
Member

@rossabaker rossabaker left a 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.

@travisbrown travisbrown merged commit eba3fcb into typelevel:master Mar 23, 2020
@travisbrown travisbrown added this to the 2.2.0-M1 milestone Mar 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants