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

Support by-name implicits with recursive dictionaries #6050

Merged
merged 9 commits into from Jul 10, 2018

Conversation

@milessabin
Copy link
Contributor

commented Aug 29, 2017

This Typelevel Scala collaboration sponsored by Lightbend ...

This is an implementation of byname implicit parameters with recursive dictionaries, intended as a language-level replacement for shapeless's Lazy type. As of this PR, implicit arguments can be marked as byname and at call sites recursive uses of implicit values are permitted if they occur in an implicit byname argument.

There is a SIP describing this language change in greater detail here.

Consider the following example,

trait Foo { def next: Foo }
object Foo {
  implicit def foo(implicit rec: Foo): Foo = new Foo { def next = rec }
}
val foo = implicitly[Foo]
assert(foo eq foo.next)

In current Scala this would diverge due to the recursive implicit argument rec of method foo. Under the scheme implemented in this PR we can mark the recursive implicit parameter as byname,

trait Foo { def next: Foo }
object Foo {
  implicit def foo(implicit rec: => Foo): Foo = new Foo { def next = rec }
                              // ^^
}
val foo = implicitly[Foo]
assert(foo eq foo.next)

and recursive occurrences of this sort are extracted out as val members of a local synthetic object as follows,

val foo: Foo = scala.Predef.implicitly[Foo](
  {
    object LazyDefns$1 {
      val rec$1: Foo = Foo.foo(rec$1)
                       //      ^^^^^
                       // recursive knot tied here
    }
    LazyDefns$1.rec$1
  }
)
assert(foo eq foo.next)

The example now compiles with the assertion successful. Note that the recursive use of rec$1 occurs within the byname argument of foo and is consequently deferred. The desugaring matches what a programmer would do to construct such a recursive value explicitly.

This general pattern is essential to the derivation of type class instances for recursive data types, one of shapeless's most common applications. Byname implicits have a number of benefits over the macro implementation of Lazy in shapeless,

  • the implementation of Lazy in shapeless is extremely delicate, relying on non-portable compiler internals. By contrast there is already an initial implementation of byname implicits in Dotty: lampepfl/dotty#1998.
  • the shapeless implementation is unable to modify divergence checking, so to solve recursive instances it effectively disables divergence checking altogether ... this means that incautious use of Lazy can cause the typechecker to loop indefinitely. The byname implicits implementation is able to both solve recursive occurrences and check for divergence.
  • the implementation of Lazy interferes with the heuristics for solving inductive implicits in #6481 because the latter depends on being able to verify that induction steps strictly reduce the size of the types being solved for; the additional Lazy type constructors make the type appear be non-decreasing in size. Whilst this could be special-cased, doing so would require some knowledge of shapeless to be incorporated into the compiler. Being a language-level feature, byname implicits can be accommodated directly in the induction heuristics.
  • in common cases more implicit arguments would have to be marked as Lazy than would have to be marked as byname under this PR due to restrictions on what the Lazy macro is able to do. Given that there is a runtime cost associated with capturing the thunks required for both Lazy and byname arguments, any reduction in the number is beneficial.
@tpolecat

This comment has been minimized.

Copy link
Contributor

commented Aug 29, 2017

This is great. Thank you!

@tek

This comment has been minimized.

Copy link

commented Aug 29, 2017

yaaaaay awesome!

@andyscott

This comment has been minimized.

Copy link

commented Aug 29, 2017

Yes please!

@fommil

This comment has been minimized.

Copy link
Contributor

commented Aug 30, 2017

One potential problem with this overload/behaviour change of the by-name language feature is that I was planning on using regular byname implicit parameters to avoid constructing the instance when it's not needed. This is necessary to get good memory allocation optimisation for implicits with conditional requirements... caching of implicit def (for runtime performance optimisation) is one such situation I'm currently working on, e.g. https://gitlab.com/fommil/stalactite/issues/35

Is there any requirement for Strict/Cached after this?

@julienrf

This comment has been minimized.

Copy link
Contributor

commented Aug 30, 2017

From what I understand the effective semantics is by-need, not by-name? So, what’s preventing us from replacing the => with a lazy?

@milessabin milessabin referenced this pull request Aug 31, 2017

Closed

Byname implicits parameters with recursive dictionaries #419

4 of 5 tasks complete
@lrytz

This comment has been minimized.

Copy link
Member

commented Sep 4, 2017

@julienrf at least when provided explicitly, it behaves as by-name, not by-need.

Welcome to Scala 2.12.4-20170829-122239-87b912c (Java HotSpot(TM) 64-Bit Server VM, Java 1.8.0_131).

scala> def f(implicit x: => Int) = { x + x }
f: (implicit x: => Int)Int

scala> f { println("hi"); 1 }
hi
hi
res1: Int = 2
@milessabin

This comment has been minimized.

Copy link
Contributor Author

commented Sep 4, 2017

Being implicit doesn't change the by name semantics. I think it would also make sense to have lazy arguments (both implicit and explicit), but that's somewhat orthogonal to this PR. See scala/bug#240 and https://github.com/lampepfl/dotty/issues/3005 for more discussion.

@retronym

This comment has been minimized.

Copy link
Member

commented Sep 11, 2017

This general pattern is essential to the derivation of type class instances for recursive data types, one of shapeless's most common applications.

Could you share a small motivating example that currently diverges? (The example above is distilled too far for me to see the use case)

@fommil

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2017

@retronym https://gitlab.com/fommil/stalactite/tree/master/src/test/scala/xmlformat

Try removing the Lazy, Cached and Strict here in either the generic encoder or decoder and see the recursive tests fail.

I must admit, I'm confused about exactly what this means for my usecase... I'm unable to see the above in my example. Does this only replace Lazy? What about Cached and Strict? What's the implication to performance, because if these things really do become by-name it's going to kill performance when the lazy semantics are not necessary. I have a good usecase for byname implicits, which does help performance, but added across the board this will be terrible.

@milessabin

This comment has been minimized.

Copy link
Contributor Author

commented Sep 11, 2017

@retronym there are motivating examples in the tests. test/files/pos/byname-implicits-9.scala (a Show type class derivation exercised on List) is an example with explicit recursion in the ADT,

List[T] => Either[Nil.type, ::[T]]
::[T] => (T, List[T])

Without the ability to tie the knot here the compiler would diverge on seeing the occurrence of List[T] in ::[T].

@milessabin

This comment has been minimized.

Copy link
Contributor Author

commented Sep 11, 2017

@fommil Cached and Strict will go away too, as will cachedImplicit. There will be an alternative mechanism for handling caching.

@adriaanm adriaanm self-requested a review Sep 21, 2017

@adriaanm

This comment has been minimized.

Copy link
Member

commented Sep 27, 2017

2.13 is a better target for this work. 2.12 is now at a point where we need to focus on stability and bug compatibility.

@adriaanm adriaanm modified the milestones: 2.12.4, 2.13.0-M3 Sep 27, 2017

@fommil

This comment has been minimized.

Copy link
Contributor

commented Oct 19, 2017

I have an alternative mechanism (to shapeless Generic / LabelledGeneric) for typeclass derivation that also hits the recursive types problem. I'll be interested to see if this fixes it. In the meantime I can use Lazy and Cached, hopefully without typeclass authors or end users needing to know about it.

@OlivierBlanvillain

This comment has been minimized.

Copy link
Contributor

commented Oct 20, 2017

I've been playing around with this branch and found some variation between Dotty and this PR. Here is an isolated example (adapted from shapeless-type-class-derivation-2015-demo) that compiles using Dotty, but diverges on this PR.

I suspect the difference comes from the divergence checking.

@milessabin

This comment has been minimized.

Copy link
Contributor Author

commented Nov 18, 2017

@OlivierBlanvillain That's a very useful example. Thanks!

It wouldn't expect the eqH arguments of eqHCons and eqCCons to have to be byname, and if I make them strict then your example compiles with this PR. That said, I think it should also compile if they are byname ... I'll investigate.

You're quite right that it's a difference in the divergence checker which is responsible for the difference here. I've implemented a slightly more sophisticated test than the user specified upper bound that you've gone for in Dotty. I think that it would be better to avoid making the limit a compilation parameter if at all possible.

I'm putting together a SIP for this at the moment and I'll include the details of the divergence criteria along with a proof that implicit search either terminates successfully or fails reporting divergence. I'm fairly confident that you will be able to port it over to the Dotty implementation.

@milessabin milessabin force-pushed the milessabin:topic/byname-implicits branch from 87b912c to 8fe649d Nov 29, 2017

@milessabin milessabin changed the base branch from 2.12.x to 2.13.x Nov 29, 2017

@milessabin milessabin changed the title [WIP] Implementation of byname implicits with recursive dictionaries. Implementation of byname implicits with recursive dictionaries. Nov 29, 2017

@milessabin

This comment has been minimized.

Copy link
Contributor Author

commented Nov 29, 2017

Rebased to 2.13.x.

@OlivierBlanvillain your example now works as is, but also with just the generic instance having a byname argument ... see test/files/pos/byname-implicits-13.scala and test/files/pos/byname-implicits-14.scala.

@OlivierBlanvillain

This comment has been minimized.

Copy link
Contributor

commented Nov 29, 2017

@milessabin I just compiled all the pos & run tests from this PR on dotty master and it seams that both compilers agree as far as bymane implicits are concerned 👍

@milessabin

This comment has been minimized.

Copy link
Contributor Author

commented Nov 29, 2017

@OlivierBlanvillain oh, awesome :-)

@milessabin

This comment was marked as resolved.

Copy link
Contributor Author

commented Nov 29, 2017

Jenkins wedged?

@@ -619,7 +688,7 @@ trait Implicits {
case NullaryMethodType(restpe) => loop(restpe, pt)
case PolyType(_, restpe) => loop(restpe, pt)
case ExistentialType(_, qtpe) => if (fast) loop(qtpe, pt) else normalize(tp) <:< pt // is !fast case needed??
case _ => if (fast) isPlausiblySubType(tp, pt) else tp <:< pt
case _ => if (fast) isPlausiblySubType(tp, if(isView) pt else dropByName(pt)) else tp <:< dropByName(pt)

This comment has been minimized.

Copy link
@retronym

retronym Jul 6, 2018

Member

I'll check this for performance.

@retronym

This comment has been minimized.

Copy link
Member

commented Jul 9, 2018

I'm in the process of reviewing some of the mechanics of this change in https://github.com/retronym/scala/tree/review/6050

@retronym

This comment has been minimized.

Copy link
Member

commented Jul 9, 2018

@milessabin I've finished proposing changes in https://github.com/retronym/scala/tree/review/6050.

The main simplification I found was to do the determine isByName and compute dpt before constructing the ImplicitSearch. Do you see any problems with that approach?

@retronym
Copy link
Member

left a comment

Implementation of byname implicits with recursive dictionaries
This is an implementation of byname implicit parameters with recursive
dictionaries, intended as a language-level replacement for shapeless's
Lazy type. As of this commit, implicit arguments can be marked as byname
and at call sites recursive uses of implicit values are permitted if
they occur in an implicit byname argument.

Consider the following example,

  trait Foo { def next: Foo }
  object Foo {
    implicit def foo(implicit rec: Foo): Foo =
      new Foo { def next = rec }
  }
  val foo = implicitly[Foo]
  assert(foo eq foo.next)

In current Scala this would diverge due to the recursive implicit
argument rec of method foo. Under the scheme implemented in this commit
we can mark the recursive implicit parameter as byname,

  trait Foo { def next: Foo }
  object Foo {
    implicit def foo(implicit rec: => Foo): Foo =
      new Foo { def next = rec }
  }
  val foo = implicitly[Foo]
  assert(foo eq foo.next)

and recursive occurrences of this sort are extracted out as val members
of a local synthetic object as follows,

  val foo: Foo = scala.Predef.implicitly[Foo](
    {
      object LazyDefns$1 {
        val rec$1: Foo = Foo.foo(rec$1)
                         //      ^^^^^
                         // recursive knot tied here
      }
      LazyDefns$1.rec$1
    }
  )
  assert(foo eq foo.next)

and the example compiles with the assertion successful. Note that the
recursive use of rec$1 occurs within the byname argument of foo and is
consequently deferred. The desugaring matches what a programmer would do
to construct such a recursive value explicitly.

This general pattern is essential to the derivation of type class
instances for recursive data types, one of shapeless's most common
applications.

Byname implicits have a number of benefits over the macro implementation
of Lazy in shapeless,

+ the implementation of Lazy in shapeless is extremely delicate, relying
  on non-portable compiler internals. By contrast there is already an
  initial implementation of byname implicits in Dotty:
  lampepfl/dotty#1998.
+ the shapeless implementation is unable to modify divergence checking,
  so to solve recursive instances it effectively disables divergence
  checking altogether ... this means that incautious use of Lazy can cause
  the typechecker to loop indefinitely. The byname implicits
  implementation is able to both solve recursive occurrences and check for
  divergence.
+ the implementation of Lazy interferes with the heuristics for solving
  inductive implicits in #6481 because the latter depends on being able to
  verify that induction steps strictly reduce the size of the types being
  solved for; the additional Lazy type constructors make the type appear
  be non-decreasing in size. Whilst this could be special-cased, doing so
  would require some knowledge of shapeless to be incorporated into the
  compiler. Being a language-level feature, byname implicits can be
  accommodated directly in the induction heuristics.
+ in common cases more implicit arguments would have to be marked as
  Lazy than would have to be marked as byname under this PR due to
  restrictions on what the Lazy macro is able to do. Given that there is a
  runtime cost associated with capturing the thunks required for both Lazy
  and byname arguments, any reduction in the number is beneficial.
@milessabin

This comment has been minimized.

Copy link
Contributor Author

commented Jul 10, 2018

@retronym those changes look fine. I'll rebase and pull them in to this PR.

retronym added some commits Jul 9, 2018

Use finer grained fresh name creators in by-name implicit desugaring
This is the new way to freshen names while keeping compiler output
stable when sources are re-ordered or incrementally recompiled.
Adjust owner of byname implicit RHS
An implicit macro within the by-name implicit dictionary might
return a typechecked tree, so we ought to use changeOwner when
moving that code into the RHS of a synthetic member of `LazyDefns`
@milessabin

This comment has been minimized.

Copy link
Contributor Author

commented Jul 10, 2018

@retronym there are a couple of what look to be unrelated commits in there: retronym@e3cf13e and retronym@67a8665.

I'll leave them out.

@milessabin milessabin force-pushed the milessabin:topic/byname-implicits branch from fdc4261 to 882881b Jul 10, 2018

Incorporated changes suggested by @retronym.

@milessabin

This comment has been minimized.

Copy link
Contributor Author

commented Jul 10, 2018

Rebased and incorporated @retronym's suggestions.

@milessabin

This comment has been minimized.

Copy link
Contributor Author

commented Jul 10, 2018

@adriaanm I think this is ready to merge now :-)

@adriaanm adriaanm merged commit 874bf03 into scala:2.13.x Jul 10, 2018

4 checks passed

cla @milessabin signed the Scala CLA. Thanks!
Details
combined All previous commits successful.
continuous-integration/travis-ci/pr The Travis CI build passed
Details
validate-main [3038] SUCCESS. Took 71 min.
Details
@adriaanm

This comment has been minimized.

Copy link
Member

commented Jul 10, 2018

🍰

@milessabin

This comment has been minimized.

Copy link
Contributor Author

commented Jul 10, 2018

😋

@kubukoz

This comment has been minimized.

Copy link

commented Jul 10, 2018

Amazing work, thank you so much @milessabin and everyone involved! 2.13 is going to be so great!

@SethTisue SethTisue changed the title Implementation of byname implicits with recursive dictionaries. Support by-name implicits with recursive dictionaries Aug 22, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.