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

Fix #7139: Implement kind-projector compatibility #7775

Merged
merged 1 commit into from Jan 23, 2020

Conversation

@travisbrown
Copy link
Contributor

travisbrown commented Dec 16, 2019

This change adds support for a subset of kind-projector's syntax behind the existing -Ykind-projector flag (closing #7139).

It supports the following kind-projector features:

  • * as a placeholder (Functor[Either[L, *]] is equivalent to Functor[[x] => Either[L, x]]).
  • * as a placeholder in tuple types (Functor[(A, *)] is equivalent to Functor[[x] => (A, x)]).
  • * as a placeholder in function types (both Functor[S => *] and Functor[* => T] work).
  • λ syntax for naming parameters ( Functor[λ[x => (x, x)]] for Functor[[x] => (x, x)]).

There are a few things kind-projector provides that this change doesn't support:

  • ? as a placeholder (since it collides with wildcards).
  • * as a placeholder in infix types.
  • Lambda as an alternative for λ.
  • λ arguments of a kind other than * (e.g. λ[f[_] => Functor[f]] isn't supported).
  • Variance annotations on either * or λ arguments.
  • Polymorphic lambda values (λ[Vector ~> List](_.toList)).

I've prioritized supporting the kind-projector features needed in Cats, and while we do use some of these features there, it's pretty easy to make small changes to avoid them without hurting readability (in most cases thse changes improve readability it, in my view—e.g. getting rid of λ[`-α` => ...]).

The changes here have no effect on parsing if -Ykind-projector isn't enabled. I enabled it generally in the test configuration since I didn't see a way to enable it for specific test files.

Copy link

dotty-bot left a comment

Hello, and thank you for opening this PR! 🎉

All contributors have signed the CLA, thank you! ❤️

Have an awesome day! ☀️

@smarter

This comment has been minimized.

Copy link
Member

smarter commented Dec 16, 2019

I enabled it generally in the test configuration since I didn't see a way to enable it for specific test files.

You'll need to add your test files in tests/pos-special and tests/neg-custom-args, then specify which flags to pass like this:

compileFile("tests/pos-special/indent-colons.scala", defaultOptions.and("-Yindent-colons")),
compileFile("tests/neg-custom-args/noimports.scala", defaultOptions.and("-Yno-imports")),

@travisbrown

This comment has been minimized.

Copy link
Contributor Author

travisbrown commented Dec 16, 2019

@smarter Thanks! Would it be better for me to do that? It seems like there's also value in confirming that the changes behind the flag don't break any existing tests.

@smarter

This comment has been minimized.

Copy link
Member

smarter commented Dec 16, 2019

It seems like there's also value in confirming that the changes behind the flag don't break any existing tests.

Yeah it's a trade-off, but I think it's better to not enable special language features by default in the tests, since that potentially decreases the coverage of the code path that most people will run.

@travisbrown

This comment has been minimized.

Copy link
Contributor Author

travisbrown commented Dec 16, 2019

@smarter Sounds reasonable, thanks. I've just pushed the change.


val newParams = params.map {
case Ident(tpnme.raw.STAR) =>
val name = tpnme.syntheticTypeParamName(tparams.length)

This comment has been minimized.

Copy link
@smarter

smarter Dec 16, 2019

Member

To make sure there's no weird shadowing issues, it's probably better to use fresh names like we do for term parameters:

Suggested change
val name = tpnme.syntheticTypeParamName(tparams.length)
val name = WildcardParamName.fresh().toTypeName
private def replaceKindProjectorPlaceholders(params: List[Tree]): (List[Tree], List[TypeDef]) = {
val tparams = new ListBuffer[TypeDef]

val newParams = params.map {

This comment has been minimized.

Copy link
@smarter

smarter Dec 16, 2019

Member

To reuse the same list if possible:

Suggested change
val newParams = params.map {
val newParams = params.mapConserve {
if (tparams.isEmpty) {
Function(params, t)
} else {
LambdaTypeTree(tparams, Function(newParams, newT))
}
Comment on lines 1330 to 1334

This comment has been minimized.

Copy link
@smarter

smarter Dec 16, 2019

Member

I think this is equivalent to:

lambdaAbstract(tparams, Function(newParams, newT))

(lambdaAbstract could also be used in a couple of other places)

This comment has been minimized.

Copy link
@travisbrown

travisbrown Dec 16, 2019

Author Contributor

I was using LambdaTypeTree directly to avoid using the new params, etc. when they weren't needed, and in the tuple case it lets us avoid instantiating a new Tuple value:

              if (tparams.isEmpty) {
                t
              } else {
                LambdaTypeTree(tparams, Tuple(newParams))
              }

I'm changing the other two now, and am happy to change the tuple one as well if you'd prefer.

val tparams = new ListBuffer[TypeDef]

val newParams = params.map {
case Ident(tpnme.raw.STAR) =>

This comment has been minimized.

Copy link
@smarter

smarter Dec 16, 2019

Member

Name the parameter to be used in another suggestion below:

Suggested change
case Ident(tpnme.raw.STAR) =>
case param @ Ident(tpnme.raw.STAR) =>
val newParams = params.map {
case Ident(tpnme.raw.STAR) =>
val name = tpnme.syntheticTypeParamName(tparams.length)
tparams += TypeDef(name, TypeBoundsTree(EmptyTree, EmptyTree))

This comment has been minimized.

Copy link
@smarter

smarter Dec 16, 2019

Member
  1. set the Param flag to make sure this is handled as a type parameter
  2. set the position ("span") of the constructed tree to match the position of the user-written *, for better error messages and IDE support.
Suggested change
tparams += TypeDef(name, TypeBoundsTree(EmptyTree, EmptyTree))
tparams += TypeDef(name, TypeBoundsTree(EmptyTree, EmptyTree))
.withFlags(Param).withSpan(param.span)

There's another use of TypeDef below which should do something similar (or even better, be refactored so there's only one point where TypeDef is used)

This comment has been minimized.

Copy link
@travisbrown

travisbrown Dec 16, 2019

Author Contributor

I don't see a way to use the * span without ending up with a "child positions in wrong order" error:

java.lang.AssertionError: assertion failed: position error, child positions overlap or in wrong order
parent         = [_$5] =>> Either[Int, _$5]
1st child      = _$5
1st child span = [117..118]
2nd child      = Either[Int, _$5]
2nd child span = <105..115> while compiling tests/pos-special/kind-projector.scala
Fatal compiler crash when compiling: tests/pos-special/kind-projector.scala:
assertion failed: position error, child positions overlap or in wrong order
parent         = [_$5] =>> Either[Int, _$5]
1st child      = _$5
1st child span = [117..118]
2nd child      = Either[Int, _$5]
2nd child span = <105..115>
dotty.DottyPredef$.assertFail(DottyPredef.scala:17)
dotty.tools.dotc.ast.Positioned.check$1(Positioned.scala:183)

It seems like in this case it just might not be possible to point to the user-written *?

@travisbrown

This comment has been minimized.

Copy link
Contributor Author

travisbrown commented Dec 16, 2019

@smarter Thanks for the comments! I've made the changes you suggest except for two things (also noted above):

  • I'm using LambdaTypeTree directly (instead of lambdaAbstract) for the tuple case where that lets us avoid instantiating a new Tuple.
  • I'm only setting the span on the TypeDef in the λ-argument case, since for * it ends up getting positions in the wrong order.
@travisbrown

This comment has been minimized.

Copy link
Contributor Author

travisbrown commented Dec 17, 2019

@smarter Let me know if there's anything else I can do here, and thanks again!

@Katrix

This comment has been minimized.

Copy link

Katrix commented Dec 18, 2019

Isn't Functor[Either[L, *]] being equivalent to [x] => Functor[Either[L, x]] the wrong behavior? Shouldn't it instead be equivalent to Functor[[x] => Either[L, x]]?

It's spelled out pretty explicitly as a gotcha on the docs too. https://github.com/typelevel/kind-projector/blob/master/README.md#type-lambda-gotchas

@travisbrown

This comment has been minimized.

Copy link
Contributor Author

travisbrown commented Dec 18, 2019

@Katrix Sorry, that was me mistyping in the description. The actual behavior is as you describe. For example, if you compile this with -Ykind-projector -Xprint:parser:

trait Functor[F[_]]

object Functor {
  def eitherFunctor[L]: Functor[[x] =>> Either[L, x]] = ???
  def otherEitherFunctor[L]: Functor[Either[L, *]] = ???
}

You get this:

parsed:
package <empty> {
  trait Functor[F[_$1]] {}
  module object Functor {
    def eitherFunctor[L]: Functor[[x] =>> Either[L, x]] = ???
    def otherEitherFunctor[L]: Functor[[_$2] =>> Either[L, _$2]] = ???
  }
}

I've fixed the description. Thanks for catching that.

@travisbrown

This comment has been minimized.

Copy link
Contributor Author

travisbrown commented Jan 16, 2020

@smarter I don't mean to sound impatient, but please let me know if it'd be useful for me to merge master or if there's anything else I can do here. Thanks!

@smarter

This comment has been minimized.

Copy link
Member

smarter commented Jan 16, 2020

Don't worry about merging with master, it's fine as is. Sorry for the wait, I keep getting distracted trying to fix the issues you're reporting :).

Copy link
Member

smarter left a comment

LGTM with one minor nit, sorry for the delay in review!

Longer term, I think we should try handling type placeholders in the same way we handle term placeholders currently (see the var placeholderParams that is used as a stack). This would avoid having to special case applied types, tuples, infix types, etc. This isn't a big deal for now since placeholder params are just a compatibility thing, but in the future we'd like to have them by default with one syntax or another: #5379

@@ -1413,6 +1420,29 @@ object Parsers {
}
}

private def makeKindProjectorTypeDef(name: TypeName, span: Option[Span]): TypeDef = {

This comment has been minimized.

Copy link
@smarter

smarter Jan 22, 2020

Member

I think we should drop the span parameter and just have call-sites that need to set a span do it themselves (makeKindProjectorTypeDef(...).withSpan(...)), it's not much longer and slightly clearer.

This comment has been minimized.

Copy link
@travisbrown

travisbrown Jan 22, 2020

Author Contributor

Agreed—I've just pushed the change.

@smarter

This comment has been minimized.

Copy link
Member

smarter commented Jan 22, 2020

Sorry, you'll need to rebase to pick up some recent CI changes fixing dependency resolution (while you're at it, please squash these commits together since they represent a single change, and maybe reuse the PR description as a commit message since it's more detailed).

This change adds support for a subset of kind-projector's syntax behind
the existing -Ykind-projector flag.

It supports the following kind-projector features:

- * placeholder (Functor[F[L, *]] instead of Functor[[x] => F[L, x]]).
- * in tuple types (Functor[(A, *)] instead of Functor[[x] => (A, x)]).
- * in function types (both Functor[S => *] and Functor[* => T] work).
- λ syntax (Functor[λ[x => (x, x)]] for Functor[[x] => (x, x)]).

There are a few things kind-projector provides that the flag doesn't:

- ? as a placeholder (since it collides with wildcards).
- * as a placeholder in infix types.
- Lambda as an alternative for λ.
- λ arguments of a kind other than * (no λ[f[_] => Functor[f]]).
- Variance annotations on either * or λ arguments.
- Polymorphic lambda values (λ[Vector ~> List](_.toList)).

The changes have no effect on parsing if -Ykind-projector isn't enabled.
@travisbrown travisbrown force-pushed the travisbrown:topic/kind-projector-compat branch from 880f5f4 to b5adfcb Jan 23, 2020
@travisbrown

This comment has been minimized.

Copy link
Contributor Author

travisbrown commented Jan 23, 2020

@smarter Done!

@odersky odersky merged commit 709a3c7 into lampepfl:master Jan 23, 2020
2 checks passed
2 checks passed
CLA User signed CLA
Details
continuous-integration/drone/pr Build is passing
Details
@travisbrown travisbrown mentioned this pull request Jan 24, 2020
4 of 12 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.