SI-2712 Add support for partial unification of type constructors #5102

Merged
merged 2 commits into from May 27, 2016

Conversation

Projects
None yet
@milessabin
Contributor

milessabin commented Apr 15, 2016

An improvement to type inference for type constructors has been added, enabled by the -Ypartial-unification command line option (also enabled in -Xexperimental mode). This fixes SI-2712 in the way suggested by Paul Chiusano in a comment on the issue. This has many benefits for libraries, such as Cats and Scalaz, which make extensive use of higher-kinded types.

With the feature enabled the following compiles,

def foo[F[_], A](fa: F[A]): String = fa.toString
foo { x: Int => x * 2 }

with the type variable F[t] solved as Int => t and the type variable A solved as Int. Previously the compiler would have rejected this programme because,

  • The type parameter F of foo is a type constructor with a single type parameter, also known as kind * -> *.
  • The provided type, Int => Int, which is a synonym for Function1[Int, Int], has an outer type constructor with two type parameters, or kind * -> * -> *.
  • The type inference algorithm required that the kinds of type parameters and their corresponding type arguments must be the same.

Partial unification solves this problem by treating outer type constructors at call sites as partially applied. In the example above, the compiler does the equivalent of creating a local type alias with the correct kind and using that at the call site,

type Partial[t] = Int => t 
foo[Partial, Int] { x: Int => x * 2 }

Partial has the same kind as the type argument F of foo and so this compiles successfully.

The implementation partially applies type constructors from left to right, leaving the rightmost type parameters free. This works well with binary type constructors with a natural right bias, such as Either, Xor in Cats and Scalaz's disjunction. For example a map operation defined with the signature illustrated below will naturally map over the value of the righthand type, corresponding to validity, whilst leaving the value of the lefthand type untouched,

def map[F[_], A, B](fa: F[A])(f: A => B): F[B] = ...
val right: Either[String, Int] = Right(23)
val left: Either[String, Int] = Left("Invalid")
map(right)(_ + 1) // yields Right(24)
map(left)(_ + 1) // yields Left("Invalid")

An article by Daniel Spiewak expands on the consequences of this strategy.

A major benefit of enabling this feature is that existing workarounds for SI-2712, such as shapeless's Unpack and Cats and Scalaz's Unapply and their U suffixed operations are no longer necessary. This both simplifies code from the library implementor and users points of view, and also potentially reduces compile times by virtue of being implemented as a primitive component of type checking rather than being encoded via type level computation using implicits.

Enabling this feature only affects type inference hence code compiled separately with the feature enabled and disabled is fully binary compatible. There is a compiler plugin which makes this feature available for Scala 2.11.x and 2.10.x.

@scala-jenkins scala-jenkins added this to the 2.12.0-M5 milestone Apr 15, 2016

@non

This comment has been minimized.

Show comment
Hide comment
@non

non Apr 15, 2016

Contributor

+1

I think this would be really great to get in for 2.12 -- given that it's off by default and fixes a longstanding known issue. Thanks for your hard work @milessabin!

Contributor

non commented Apr 15, 2016

+1

I think this would be really great to get in for 2.12 -- given that it's off by default and fixes a longstanding known issue. Thanks for your hard work @milessabin!

@djspiewak

This comment has been minimized.

Show comment
Hide comment
@djspiewak

djspiewak Apr 15, 2016

There are no words to describe the kind of impact this would have on my daily use of Scala. I know it's very late in the 2.12 cycle, but I would love to see this make it.

There are no words to describe the kind of impact this would have on my daily use of Scala. I know it's very late in the 2.12 cycle, but I would love to see this make it.

@fommil

This comment has been minimized.

Show comment
Hide comment
@fommil

fommil Apr 15, 2016

Contributor

Is there any way to know what impact enabling the flag has on existing code, e.g. running all the tests with the flag enabled? Or, indeed, on the community build.

Contributor

fommil commented Apr 15, 2016

Is there any way to know what impact enabling the flag has on existing code, e.g. running all the tests with the flag enabled? Or, indeed, on the community build.

@djspiewak

This comment has been minimized.

Show comment
Hide comment
@djspiewak

djspiewak Apr 15, 2016

@fommil The only problem I've seen it cause thus far was compiling scalaz (found by @runarorama). Specifically, it ran into some problems with type tags. I'm not sure if @milessabin fixed that particular bug or not, but that's literally the only problem I've seen.

@fommil The only problem I've seen it cause thus far was compiling scalaz (found by @runarorama). Specifically, it ran into some problems with type tags. I'm not sure if @milessabin fixed that particular bug or not, but that's literally the only problem I've seen.

@xuwei-k

This comment has been minimized.

Show comment
Hide comment
@xuwei-k

xuwei-k Apr 15, 2016

Contributor

Here is scalaz result. I can remove some Unapply 😃
scalaz/scalaz@scalaz:3a37b10...xuwei-k:03d1fbc

Contributor

xuwei-k commented Apr 15, 2016

Here is scalaz result. I can remove some Unapply 😃
scalaz/scalaz@scalaz:3a37b10...xuwei-k:03d1fbc

+ // A = Int
+ //
+ // A more "natural" unifier might be M[t] = [t][t => t]. There's lots of scope for
+ // experimenting with alternatives here.

This comment has been minimized.

@djspiewak

djspiewak Apr 15, 2016

If this does make it in for 2.12 (or even if it doesn't), the above should be expanded into a blog post somewhere. We need to be clear that this is very literally adding left-to-right curried type constructor semantics to Scala. I tend to think this is a good thing, and it lines up very very nicely with things the community is already doing by default (e.g. right-biased Xor), but it needs to be made clear.

@djspiewak

djspiewak Apr 15, 2016

If this does make it in for 2.12 (or even if it doesn't), the above should be expanded into a blog post somewhere. We need to be clear that this is very literally adding left-to-right curried type constructor semantics to Scala. I tend to think this is a good thing, and it lines up very very nicely with things the community is already doing by default (e.g. right-biased Xor), but it needs to be made clear.

This comment has been minimized.

@Blaisorblade

Blaisorblade Apr 16, 2016

Contributor

it lines up very very nicely with things the community is already doing by default (e.g. right-biased Xor), but it needs to be made clear.

I'm guessing that's an heritage of Haskell's (here, from Either), where type inference works this way since ever. Hence, I conjecture no-Haskell Scalaers might especially need this explained.

@Blaisorblade

Blaisorblade Apr 16, 2016

Contributor

it lines up very very nicely with things the community is already doing by default (e.g. right-biased Xor), but it needs to be made clear.

I'm guessing that's an heritage of Haskell's (here, from Either), where type inference works this way since ever. Hence, I conjecture no-Haskell Scalaers might especially need this explained.

This comment has been minimized.

@larsrh

larsrh Apr 16, 2016

Contributor

I'm guessing that's an heritage of Haskell's (here, from Either), where type inference works this way since ever.

Yes, but on top of that, in Haskell, left-to-right is the only thing you can do.

@larsrh

larsrh Apr 16, 2016

Contributor

I'm guessing that's an heritage of Haskell's (here, from Either), where type inference works this way since ever.

Yes, but on top of that, in Haskell, left-to-right is the only thing you can do.

@raulraja

This comment has been minimized.

Show comment
Hide comment
@raulraja

raulraja Apr 15, 2016

+1 a much awaited fix, this would be great to have in 2.12

+1 a much awaited fix, this would be great to have in 2.12

@milessabin

This comment has been minimized.

Show comment
Hide comment
@milessabin

milessabin Apr 15, 2016

Contributor

@djspiewak the bug that @runarorama found, and a related one that @xuwei-k found are both fixed in this PR.

Contributor

milessabin commented Apr 15, 2016

@djspiewak the bug that @runarorama found, and a related one that @xuwei-k found are both fixed in this PR.

@markus1189

This comment has been minimized.

Show comment
Hide comment
@markus1189

markus1189 Apr 15, 2016

Contributor

I'm unreasonably excited about this! And if there is a chance to get this for 2.12 - oh my! What a start into the weekend. Awesome work @milessabin

Contributor

markus1189 commented Apr 15, 2016

I'm unreasonably excited about this! And if there is a chance to get this for 2.12 - oh my! What a start into the weekend. Awesome work @milessabin

@DavidGregory084

This comment has been minimized.

Show comment
Hide comment
@DavidGregory084

DavidGregory084 Apr 15, 2016

Contributor

This causes lots of pain and leads to lots of weird magic tricks in the source code of Scala libraries that are extremely off-putting to new contributors.
I would be very happy to see this fixed in 2.12 so that I don't ever have to understand how these tricks work or try to explain them to anybody else.

Contributor

DavidGregory084 commented Apr 15, 2016

This causes lots of pain and leads to lots of weird magic tricks in the source code of Scala libraries that are extremely off-putting to new contributors.
I would be very happy to see this fixed in 2.12 so that I don't ever have to understand how these tricks work or try to explain them to anybody else.

@smarter

This comment has been minimized.

Show comment
Hide comment
@smarter

smarter Apr 15, 2016

Contributor

So, I personally like this idea (I implemented something similar in a branch of Dotty a while ago, but didn't pursue it further because of a million more urgent things), but there's a bunch of questions that need to be answered to go down that road, for example:

class X1
class X2
class X3

trait One[A]
trait Two[A, B]

class Foo extends Two[X1, X2] with One[X3]
object Test {
  def test[M[_], A](x: M[A]): M[A] = x

  val foo = new Foo
  test(foo) // M = ?, A = ?
}

With -Yhigher-order-unification off we get M=One, A=X3, but with -Yhigher-order-unification on we get M=[X]=>Two[X1,X], A=X2. Is this good or bad? I have no idea.

However, this seems pretty bad:

class X1
class X2
class X3

trait One[A]
trait Two[A, B]

class Foo extends Two[X1, X2] with One[X3]
object Test {
  def test[M[X] <: One[X], A](x: M[A]): M[A] = x

  val foo = new Foo
  test(foo) // M = ?, A = ?
}

With -Yhigher-order-unification off this works fine, but with -Yhigher-order-unification on, it fails with:

inferred type arguments [[Unify$1]Two[X1,Unify$1],X2] do not conform to method test's type parameter bounds [M[X] <: One[X],A]

in other words, M=[X]=>Two[X1,X], A=X2 is inferred and not rejected, even though this does not conform to the declared bounds.

Contributor

smarter commented Apr 15, 2016

So, I personally like this idea (I implemented something similar in a branch of Dotty a while ago, but didn't pursue it further because of a million more urgent things), but there's a bunch of questions that need to be answered to go down that road, for example:

class X1
class X2
class X3

trait One[A]
trait Two[A, B]

class Foo extends Two[X1, X2] with One[X3]
object Test {
  def test[M[_], A](x: M[A]): M[A] = x

  val foo = new Foo
  test(foo) // M = ?, A = ?
}

With -Yhigher-order-unification off we get M=One, A=X3, but with -Yhigher-order-unification on we get M=[X]=>Two[X1,X], A=X2. Is this good or bad? I have no idea.

However, this seems pretty bad:

class X1
class X2
class X3

trait One[A]
trait Two[A, B]

class Foo extends Two[X1, X2] with One[X3]
object Test {
  def test[M[X] <: One[X], A](x: M[A]): M[A] = x

  val foo = new Foo
  test(foo) // M = ?, A = ?
}

With -Yhigher-order-unification off this works fine, but with -Yhigher-order-unification on, it fails with:

inferred type arguments [[Unify$1]Two[X1,Unify$1],X2] do not conform to method test's type parameter bounds [M[X] <: One[X],A]

in other words, M=[X]=>Two[X1,X], A=X2 is inferred and not rejected, even though this does not conform to the declared bounds.

@non

This comment has been minimized.

Show comment
Hide comment
@non

non Apr 15, 2016

Contributor

@smarter Ouch! I'm assuming that it's trying Two[_, _] first because of the order of the mix-ins? If Foo extends One[X3] with Two[X1, X2] do things work as expected?

Contributor

non commented Apr 15, 2016

@smarter Ouch! I'm assuming that it's trying Two[_, _] first because of the order of the mix-ins? If Foo extends One[X3] with Two[X1, X2] do things work as expected?

@smarter

This comment has been minimized.

Show comment
Hide comment
@smarter

smarter Apr 15, 2016

Contributor

@non: Yup. You can play with it easily using the sbt project at https://github.com/milessabin/si2712fix-demo

Contributor

smarter commented Apr 15, 2016

@non: Yup. You can play with it easily using the sbt project at https://github.com/milessabin/si2712fix-demo

@non

This comment has been minimized.

Show comment
Hide comment
@non

non Apr 15, 2016

Contributor

@smarter It seems like this may just be a generalization of a pre-existing problem in Scala:

class X1
class X3

trait One[A]
trait Three[A]

class Foo31 extends Three[X1] with One[X3]

object Test {
  def test[M[X] <: One[X], A](x: M[A]): M[A] = x

  val foo31 = new Foo31
  test(foo31) // M = ?, A = ?
}

With this I get:

inferred type arguments [si2712.Three,si2712.X1] do not conform to method test's type parameter bounds [M[X] <: si2712.One[X],A]

This doesn't invalidate the point that there are some things that could stop type-checking. But I do think that it's consistent with the kinds of problems these constraints already have.

Contributor

non commented Apr 15, 2016

@smarter It seems like this may just be a generalization of a pre-existing problem in Scala:

class X1
class X3

trait One[A]
trait Three[A]

class Foo31 extends Three[X1] with One[X3]

object Test {
  def test[M[X] <: One[X], A](x: M[A]): M[A] = x

  val foo31 = new Foo31
  test(foo31) // M = ?, A = ?
}

With this I get:

inferred type arguments [si2712.Three,si2712.X1] do not conform to method test's type parameter bounds [M[X] <: si2712.One[X],A]

This doesn't invalidate the point that there are some things that could stop type-checking. But I do think that it's consistent with the kinds of problems these constraints already have.

@runarorama

This comment has been minimized.

Show comment
Hide comment
@runarorama

runarorama Apr 15, 2016

Contributor

Everyone on my team would very much like to see this in 2.12.

Contributor

runarorama commented Apr 15, 2016

Everyone on my team would very much like to see this in 2.12.

@clhodapp clhodapp referenced this pull request in milessabin/shapeless Apr 15, 2016

Merged

unwrap tag (tagged) #538

@ekmett

This comment has been minimized.

Show comment
Hide comment
@ekmett

ekmett Apr 15, 2016

👍 This would greatly simplify a lot of scala code we have

ekmett commented Apr 15, 2016

👍 This would greatly simplify a lot of scala code we have

@wheaties

This comment has been minimized.

Show comment
Hide comment
@wheaties

wheaties Apr 15, 2016

👍 for this but have to agree with @djspiewak on a post explaining how things will work, espcially in light of what @smarter has put up there.

👍 for this but have to agree with @djspiewak on a post explaining how things will work, espcially in light of what @smarter has put up there.

@larsrh

This comment has been minimized.

Show comment
Hide comment
@larsrh

larsrh Apr 15, 2016

Contributor

With -Yhigher-order-unification off we get M=One, A=X3, but with -Yhigher-order-unification on we get M=[X]=>Two[X1,X], A=X2. Is this good or bad? I have no idea.

@smarter The fact that this yields different results already tells us that we're in ambiguous/unspecified territory here. I think changing behaviour is acceptable under these circumstances.

Contributor

larsrh commented Apr 15, 2016

With -Yhigher-order-unification off we get M=One, A=X3, but with -Yhigher-order-unification on we get M=[X]=>Two[X1,X], A=X2. Is this good or bad? I have no idea.

@smarter The fact that this yields different results already tells us that we're in ambiguous/unspecified territory here. I think changing behaviour is acceptable under these circumstances.

@kailuowang kailuowang referenced this pull request in kailuowang/henkan Apr 15, 2016

Closed

Error when the last field is a nested class #15

@etorreborre

This comment has been minimized.

Show comment
Hide comment
@etorreborre

etorreborre Apr 15, 2016

@djspiewak what's the effect on Emm?

@djspiewak what's the effect on Emm?

@smarter

This comment has been minimized.

Show comment
Hide comment
@smarter

smarter Apr 15, 2016

Contributor

@larsrh : changing behaviour is fine, but I don't think this can be done blindly: you need to think carefully about what semantics you want.

Contributor

smarter commented Apr 15, 2016

@larsrh : changing behaviour is fine, but I don't think this can be done blindly: you need to think carefully about what semantics you want.

@djspiewak

This comment has been minimized.

Show comment
Hide comment
@djspiewak

djspiewak Apr 15, 2016

what's the effect on Emm?

I get to delete about 90% of the boilerplate, with an exponential decrease in compile time due to the dramatically smaller search space. I still need the Permute type classes, but all of the rest can be implemented in a very straightforward manner.

what's the effect on Emm?

I get to delete about 90% of the boilerplate, with an exponential decrease in compile time due to the dramatically smaller search space. I still need the Permute type classes, but all of the rest can be implemented in a very straightforward manner.

@djspiewak

This comment has been minimized.

Show comment
Hide comment
@djspiewak

djspiewak Apr 15, 2016

I'm sure that @milessabin would explain things far better than I, but here's a quick write-up of how the fix works and what kind of implications it has for type constructor design: https://gist.github.com/7a81a395c461fd3a09a6941d4cd040f2

djspiewak commented Apr 15, 2016

I'm sure that @milessabin would explain things far better than I, but here's a quick write-up of how the fix works and what kind of implications it has for type constructor design: https://gist.github.com/7a81a395c461fd3a09a6941d4cd040f2

@tpolecat

This comment has been minimized.

Show comment
Hide comment
@tpolecat

tpolecat Apr 15, 2016

Contributor

Big 👍 for inclusion in 2.12 ... this would make my life much happier.

Contributor

tpolecat commented Apr 15, 2016

Big 👍 for inclusion in 2.12 ... this would make my life much happier.

@lambdista

This comment has been minimized.

Show comment
Hide comment
@lambdista

lambdista Apr 15, 2016

Contributor

👍 thank you @milessabin

Contributor

lambdista commented Apr 15, 2016

👍 thank you @milessabin

@adriaanm adriaanm closed this Apr 15, 2016

@adriaanm adriaanm reopened this Apr 15, 2016

@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm Apr 15, 2016

Member

Cool! Happy to have this go into M5 under a switch. I'm on vacation this week (hence the fat fingering on phone), but will take a look when I get back.

Member

adriaanm commented Apr 15, 2016

Cool! Happy to have this go into M5 under a switch. I'm on vacation this week (hence the fat fingering on phone), but will take a look when I get back.

@etorreborre

This comment has been minimized.

Show comment
Hide comment
@etorreborre

etorreborre Apr 15, 2016

This is also allowing me to remove a bunch of implicits for eff-cats.

Since there is a backported version for Scala 2.11.8 (which is the one I tested), could this also make it to a Scala 2.11.9 version?

This is also allowing me to remove a bunch of implicits for eff-cats.

Since there is a backported version for Scala 2.11.8 (which is the one I tested), could this also make it to a Scala 2.11.9 version?

@fommil fommil referenced this pull request in ensime/ensime-server Apr 15, 2016

Closed

DO NOT MERGE - SI-2712 regression test #1419

@fommil

This comment has been minimized.

Show comment
Hide comment
@fommil

fommil Apr 15, 2016

Contributor

another data point for you: ensime-server is green.

Contributor

fommil commented Apr 15, 2016

another data point for you: ensime-server is green.

@glaebhoerl glaebhoerl referenced this pull request in rust-lang/rfcs Apr 16, 2016

Open

Higher kinded polymorphism #324

@adelbertc

This comment has been minimized.

Show comment
Hide comment
@adelbertc

adelbertc Apr 16, 2016

A bunch of others have already said this, but many many 👍 's from me for including in 2.12

A bunch of others have already said this, but many many 👍 's from me for including in 2.12

@channingwalton

This comment has been minimized.

Show comment
Hide comment

2.12 +1

@weihsiu

This comment has been minimized.

Show comment
Hide comment
@weihsiu

weihsiu Apr 16, 2016

+1 on inclusion for 2.12
+1 on backport to 2.11

weihsiu commented Apr 16, 2016

+1 on inclusion for 2.12
+1 on backport to 2.11

@lrytz

This comment has been minimized.

Show comment
Hide comment
@lrytz

lrytz Apr 16, 2016

Member

Started a community build with -Yhigher-order-unification: https://scala-ci.typesafe.com/view/scala-2.12.x/job/scala-2.12.x-integrate-community-build/374/parameters

Note that some failures are expected, check the end of the log of the previous round https://scala-ci.typesafe.com/view/scala-2.12.x/job/scala-2.12.x-integrate-community-build/373/console

Member

lrytz commented Apr 16, 2016

Started a community build with -Yhigher-order-unification: https://scala-ci.typesafe.com/view/scala-2.12.x/job/scala-2.12.x-integrate-community-build/374/parameters

Note that some failures are expected, check the end of the log of the previous round https://scala-ci.typesafe.com/view/scala-2.12.x/job/scala-2.12.x-integrate-community-build/373/console

@milessabin

This comment has been minimized.

Show comment
Hide comment
@milessabin

milessabin Apr 16, 2016

Contributor

@lrytz I expect failures in libraries which have workarounds for SI-2712 (eg. Cats, Kittens, shapeless, Scalaz). The workarounds typically involve introducing additional implicit definitions with differently shaped signatures. With -Yhigher-order-unification enabled these additional implicits will become ambiguous with the primary definitions.

I think a sensible transition strategy for libraries which are affected by SI-2712 would be along the lines of,

  • Move the additional workaround definitions to an optional module.
  • Encourage client code to enable -Yhigher-order-unification and drop the optional dependency.
  • ...
  • Profit!

Note that because this change only affects type inference, code compiled with and without the flag enabled will be binary compatible in both directions.

Contributor

milessabin commented Apr 16, 2016

@lrytz I expect failures in libraries which have workarounds for SI-2712 (eg. Cats, Kittens, shapeless, Scalaz). The workarounds typically involve introducing additional implicit definitions with differently shaped signatures. With -Yhigher-order-unification enabled these additional implicits will become ambiguous with the primary definitions.

I think a sensible transition strategy for libraries which are affected by SI-2712 would be along the lines of,

  • Move the additional workaround definitions to an optional module.
  • Encourage client code to enable -Yhigher-order-unification and drop the optional dependency.
  • ...
  • Profit!

Note that because this change only affects type inference, code compiled with and without the flag enabled will be binary compatible in both directions.

@propensive

This comment has been minimized.

Show comment
Hide comment
@propensive

propensive Apr 16, 2016

Contributor

No issues compiling Rapture and all its hairy types with -Yhigher-order-unification enabled. +1

Contributor

propensive commented Apr 16, 2016

No issues compiling Rapture and all its hairy types with -Yhigher-order-unification enabled. +1

val lhs = if (isLowerBound) tp.typeArgs else typeArgs
val rhs = if (isLowerBound) typeArgs else tp.typeArgs
// This is a higher-kinded type var with same arity as tp.
// If so (see SI-7517), side effect: adds the type constructor itself as a bound.
isSubArgs(lhs, rhs, params, AnyDepth) && { addBound(tp.typeConstructor); true }
- }
+ } else if(settings.YpartialUnification && typeArgs.lengthCompare(0) > 0 && compareLengths(typeArgs, tp.typeArgs) < 0) {

This comment has been minimized.

@adriaanm

adriaanm May 23, 2016

Member

typeArgs.lengthCompare(0) > 0 --> why not typeArgs.nonEmpty?

@adriaanm

adriaanm May 23, 2016

Member

typeArgs.lengthCompare(0) > 0 --> why not typeArgs.nonEmpty?

+
+ val lhs = if (isLowerBound) abstracted else typeArgs
+ val rhs = if (isLowerBound) typeArgs else abstracted
+ // This is a higher-kinded type var with same arity as tp.

This comment has been minimized.

@adriaanm

adriaanm May 23, 2016

Member

drop comment

@adriaanm

adriaanm May 23, 2016

Member

drop comment

+ // This is a higher-kinded type var with same arity as tp.
+ // If so (see SI-7517), side effect: adds the type constructor itself as a bound.
+ isSubArgs(lhs, rhs, params, AnyDepth) && {
+ val absSyms = tpSym.typeParams.drop(numCaptured)

This comment has been minimized.

@adriaanm

adriaanm May 23, 2016

Member

rename to abstractedTypeParams to emphasize symmetry with the val (captured, abstracted ) = ... above?

@adriaanm

adriaanm May 23, 2016

Member

rename to abstractedTypeParams to emphasize symmetry with the val (captured, abstracted ) = ... above?

+ // If so (see SI-7517), side effect: adds the type constructor itself as a bound.
+ isSubArgs(lhs, rhs, params, AnyDepth) && {
+ val absSyms = tpSym.typeParams.drop(numCaptured)
+ val freeSyms = absSyms.map(_.cloneSymbol(tpSym))

This comment has been minimized.

@adriaanm

adriaanm May 23, 2016

Member

I would combine this line with the above one. No need for absSyms, so let's not leave them lying around for someone else to plug them in somewhere they don't belong :-)

@adriaanm

adriaanm May 23, 2016

Member

I would combine this line with the above one. No need for absSyms, so let's not leave them lying around for someone else to plug them in somewhere they don't belong :-)

+ // as constants and solve for the suffix. For the example in the ticket, unifying
+ // M[A] with Int => Int this unifies as,
+ //
+ // M[t] = [t][Int => t]

This comment has been minimized.

@adriaanm

adriaanm May 23, 2016

Member

Tie in to implementation as follows?

            //   M[t] = [t][Int => t]  --> abstract on the right to match the expected arity
            //   A = Int               --> capture the remainder on the left
@adriaanm

adriaanm May 23, 2016

Member

Tie in to implementation as follows?

            //   M[t] = [t][Int => t]  --> abstract on the right to match the expected arity
            //   A = Int               --> capture the remainder on the left
@@ -3131,13 +3131,47 @@ trait Types
*/
def unifyFull(tpe: Type): Boolean = {
def unifySpecific(tp: Type) = {

This comment has been minimized.

@adriaanm

adriaanm May 23, 2016

Member

To summarize my feedback from the individual comments, here's unifySpecific with my suggestions applied:

def unifySpecific(tp: Type) = {
  val tpTypeArgs = tp.typeArgs
  val arityDelta = compareLengths(typeArgs, tpTypeArgs)
  if (arityDelta == 0) {
    val lhs = if (isLowerBound) tpTypeArgs else typeArgs
    val rhs = if (isLowerBound) typeArgs else tpTypeArgs
    // This is a higher-kinded type var with same arity as tp.
    // If so (see SI-7517), side effect: adds the type constructor itself as a bound.
    isSubArgs(lhs, rhs, params, AnyDepth) && {addBound(tp.typeConstructor); true}
  } else if (arityDelta < 0 && typeArgs.nonEmpty) {
    // Simple algorithm as suggested by Paul Chiusano in the comments on SI-2712
    //
    //   https://issues.scala-lang.org/browse/SI-2712?focusedCommentId=61270
    //
    // Treat the type constructor as curried and partially applied, we treat a prefix
    // as constants and solve for the suffix. For the example in the ticket, unifying
    // M[A] with Int => Int this unifies as,
    //
    //   M[t] = [t][Int => t]  --> abstract on the right to match the expected arity
    //   A = Int               --> capture the remainder on the left
    //
    // A more "natural" unifier might be M[t] = [t][t => t]. There's lots of scope for
    // experimenting with alternatives here.
    val numCaptured = tpTypeArgs.length - typeArgs.length
    val (captured, abstractedArgs) = tpTypeArgs.splitAt(numCaptured)

    val (lhs, rhs) =
      if (isLowerBound) (abstractedArgs, typeArgs)
      else (typeArgs, abstractedArgs)

    isSubArgs(lhs, rhs, params, AnyDepth) && {
      val tpSym = tp.typeSymbolDirect
      val abstractedTypeParams = tpSym.typeParams.drop(numCaptured).map(_.cloneSymbol(tpSym))

      addBound(PolyType(abstractedTypeParams, appliedType(tp.typeConstructor, captured ++ abstractedTypeParams.map(_.tpeHK))))
      true
    }
  } else false
}
@adriaanm

adriaanm May 23, 2016

Member

To summarize my feedback from the individual comments, here's unifySpecific with my suggestions applied:

def unifySpecific(tp: Type) = {
  val tpTypeArgs = tp.typeArgs
  val arityDelta = compareLengths(typeArgs, tpTypeArgs)
  if (arityDelta == 0) {
    val lhs = if (isLowerBound) tpTypeArgs else typeArgs
    val rhs = if (isLowerBound) typeArgs else tpTypeArgs
    // This is a higher-kinded type var with same arity as tp.
    // If so (see SI-7517), side effect: adds the type constructor itself as a bound.
    isSubArgs(lhs, rhs, params, AnyDepth) && {addBound(tp.typeConstructor); true}
  } else if (arityDelta < 0 && typeArgs.nonEmpty) {
    // Simple algorithm as suggested by Paul Chiusano in the comments on SI-2712
    //
    //   https://issues.scala-lang.org/browse/SI-2712?focusedCommentId=61270
    //
    // Treat the type constructor as curried and partially applied, we treat a prefix
    // as constants and solve for the suffix. For the example in the ticket, unifying
    // M[A] with Int => Int this unifies as,
    //
    //   M[t] = [t][Int => t]  --> abstract on the right to match the expected arity
    //   A = Int               --> capture the remainder on the left
    //
    // A more "natural" unifier might be M[t] = [t][t => t]. There's lots of scope for
    // experimenting with alternatives here.
    val numCaptured = tpTypeArgs.length - typeArgs.length
    val (captured, abstractedArgs) = tpTypeArgs.splitAt(numCaptured)

    val (lhs, rhs) =
      if (isLowerBound) (abstractedArgs, typeArgs)
      else (typeArgs, abstractedArgs)

    isSubArgs(lhs, rhs, params, AnyDepth) && {
      val tpSym = tp.typeSymbolDirect
      val abstractedTypeParams = tpSym.typeParams.drop(numCaptured).map(_.cloneSymbol(tpSym))

      addBound(PolyType(abstractedTypeParams, appliedType(tp.typeConstructor, captured ++ abstractedTypeParams.map(_.tpeHK))))
      true
    }
  } else false
}
@milessabin

This comment has been minimized.

Show comment
Hide comment
@milessabin

milessabin May 24, 2016

Contributor

Agreed on all counts (though I don't think you intended to remove the check for the presence of the enabling flag ;-) ).

Updated and rebased.

Contributor

milessabin commented May 24, 2016

Agreed on all counts (though I don't think you intended to remove the check for the presence of the enabling flag ;-) ).

Updated and rebased.

@japgolly

This comment has been minimized.

Show comment
Hide comment
@japgolly

japgolly May 24, 2016

Contributor

Getting close!

Dim house lights, cue spotlight, drum roll... 😯

Contributor

japgolly commented May 24, 2016

Getting close!

Dim house lights, cue spotlight, drum roll... 😯

@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm May 24, 2016

Member

As a final consideration, I'd like to revisit @smarter's comment above (#5102 (comment)). My understanding is that it's about the order of places where we look for a type constructor of the expected arity. Currently, there must be one in the base type sequence (transitive closure of the extends clause -- BTS). After this change, the BTS is not consulted and we synthesize a type constructor of the right arity using the new logic.

Is this the order we want? Should we look at the BTS for existing type constructors of the right arity before we start synthesizing new ones?

PS: maybe it is the order we want, but I do think this change is important enough to double check and highlight in the release notes

Member

adriaanm commented May 24, 2016

As a final consideration, I'd like to revisit @smarter's comment above (#5102 (comment)). My understanding is that it's about the order of places where we look for a type constructor of the expected arity. Currently, there must be one in the base type sequence (transitive closure of the extends clause -- BTS). After this change, the BTS is not consulted and we synthesize a type constructor of the right arity using the new logic.

Is this the order we want? Should we look at the BTS for existing type constructors of the right arity before we start synthesizing new ones?

PS: maybe it is the order we want, but I do think this change is important enough to double check and highlight in the release notes

@Blaisorblade

This comment has been minimized.

Show comment
Hide comment
@Blaisorblade

Blaisorblade May 26, 2016

Contributor

Slight OT: TL; DR. Unlike I thought, pattern unification is by far too weak for the interesting scenarios.

So I figured I should really unsuggest it, and apologize a bit for suggesting it in the first place without doing enough homework, in particular to @milessabin and @mandubian.

If anybody is interested in the evidence, see (and comment) this Agda snippet:
https://gist.github.com/Blaisorblade/5284942a6a7bee22a372afe7d86beb98

Contributor

Blaisorblade commented May 26, 2016

Slight OT: TL; DR. Unlike I thought, pattern unification is by far too weak for the interesting scenarios.

So I figured I should really unsuggest it, and apologize a bit for suggesting it in the first place without doing enough homework, in particular to @milessabin and @mandubian.

If anybody is interested in the evidence, see (and comment) this Agda snippet:
https://gist.github.com/Blaisorblade/5284942a6a7bee22a372afe7d86beb98

@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm May 26, 2016

Member

@milessabin I'll let @retronym do the honors, but I think this is ready to merge.

Since this definitely merits inclusion in the release notes, could you update and expand the PR description to make it suitable for that? My last comment is probably one of the things that should be mentioned as a breaking change.

Member

adriaanm commented May 26, 2016

@milessabin I'll let @retronym do the honors, but I think this is ready to merge.

Since this definitely merits inclusion in the release notes, could you update and expand the PR description to make it suitable for that? My last comment is probably one of the things that should be mentioned as a breaking change.

@milessabin

This comment has been minimized.

Show comment
Hide comment
@milessabin

milessabin May 26, 2016

Contributor

@adriaanm will do. I'll flag up @smarter's example as something that warrants further investigation.

Contributor

milessabin commented May 26, 2016

@adriaanm will do. I'll flag up @smarter's example as something that warrants further investigation.

@retronym retronym changed the title from Add support for higher order unification. Fixes SI-2712. to SI-2712 Add support for partial unification of type constructors May 27, 2016

@retronym

This comment has been minimized.

Show comment
Hide comment
@retronym

retronym May 27, 2016

Member

Merging now, I'll let @milessabin update the PR description with a small example of what this means to users

Member

retronym commented May 27, 2016

Merging now, I'll let @milessabin update the PR description with a small example of what this means to users

@retronym retronym merged commit 6b2037a into scala:2.12.x May 27, 2016

6 checks passed

cla @milessabin signed the Scala CLA. Thanks!
Details
combined All previous commits successful.
integrate-ide [2075] SUCCESS. Took 2 s.
Details
validate-main [2351] SUCCESS. Took 84 min.
Details
validate-publish-core [2319] SUCCESS. Took 5 min.
Details
validate-test [2040] SUCCESS. Took 78 min.
Details
@milessabin

This comment has been minimized.

Show comment
Hide comment
@milessabin

milessabin May 27, 2016

Contributor

Awesome! Thanks to everyone who contributed, and especially to @retronym and @adriaanm :-)

Contributor

milessabin commented May 27, 2016

Awesome! Thanks to everyone who contributed, and especially to @retronym and @adriaanm :-)

@TomasMikula

This comment has been minimized.

Show comment
Hide comment
@TomasMikula

TomasMikula Jun 15, 2016

Contributor

Somewhat off-topic, but do you think that partial unification in implicit search could also have a similar solution?

Example:

object ImplicitSearchTest {
  import scala.language.higherKinds

  final case class KPair[F[_], G[_], A](_1: F[A], _2: G[A])

  trait Getter[A, B] {
    def get(a: A): B
  }

  implicit def leftProjection[F[_], G[_], A]: Getter[KPair[F, G, A], F[A]] =
    new Getter[KPair[F, G, A], F[A]] {
      def get(p: KPair[F, G, A]): F[A] = p._1
    }

  implicit def rightProjection[F[_], G[_], A]: Getter[KPair[F, G, A], G[A]] =
    new Getter[KPair[F, G, A], G[A]] {
      def get(p: KPair[F, G, A]): G[A] = p._2
    }

  implicit def composedRightProjection[F[_], G[_], A, B](implicit G: Getter[G[A], B]): Getter[KPair[F, G, A], B] =
    new Getter[KPair[F, G, A], B] {
      def get(p: KPair[F, G, A]): B = G.get(p._2)
    }

  trait Foo[A]
  trait Bar[A]
  trait Baz[A]

//type FooBarBaz[A] = KPair[Foo, KPair[Bar, Baz, ?], A]
  type FooBarBaz[A] = KPair[Foo, ({ type Out[X] = KPair[Bar, Baz, X] })#Out, A]

  implicitly[Getter[FooBarBaz[Int], Foo[Int]]] // error: could not find implicit value
  implicitly[Getter[FooBarBaz[Int], Bar[Int]]] // error: could not find implicit value
  implicitly[Getter[FooBarBaz[Int], Baz[Int]]] // error: could not find implicit value

  // if we create an intermediate alias, implicit search works
  type BarBaz[A] = KPair[Bar, Baz, A]
  type FooBarBaz1[A] = KPair[Foo, BarBaz, A]

  implicitly[Getter[FooBarBaz1[Int], Foo[Int]]] // OK
  implicitly[Getter[FooBarBaz1[Int], Bar[Int]]] // OK
  implicitly[Getter[FooBarBaz1[Int], Baz[Int]]] // OK
}
Contributor

TomasMikula commented Jun 15, 2016

Somewhat off-topic, but do you think that partial unification in implicit search could also have a similar solution?

Example:

object ImplicitSearchTest {
  import scala.language.higherKinds

  final case class KPair[F[_], G[_], A](_1: F[A], _2: G[A])

  trait Getter[A, B] {
    def get(a: A): B
  }

  implicit def leftProjection[F[_], G[_], A]: Getter[KPair[F, G, A], F[A]] =
    new Getter[KPair[F, G, A], F[A]] {
      def get(p: KPair[F, G, A]): F[A] = p._1
    }

  implicit def rightProjection[F[_], G[_], A]: Getter[KPair[F, G, A], G[A]] =
    new Getter[KPair[F, G, A], G[A]] {
      def get(p: KPair[F, G, A]): G[A] = p._2
    }

  implicit def composedRightProjection[F[_], G[_], A, B](implicit G: Getter[G[A], B]): Getter[KPair[F, G, A], B] =
    new Getter[KPair[F, G, A], B] {
      def get(p: KPair[F, G, A]): B = G.get(p._2)
    }

  trait Foo[A]
  trait Bar[A]
  trait Baz[A]

//type FooBarBaz[A] = KPair[Foo, KPair[Bar, Baz, ?], A]
  type FooBarBaz[A] = KPair[Foo, ({ type Out[X] = KPair[Bar, Baz, X] })#Out, A]

  implicitly[Getter[FooBarBaz[Int], Foo[Int]]] // error: could not find implicit value
  implicitly[Getter[FooBarBaz[Int], Bar[Int]]] // error: could not find implicit value
  implicitly[Getter[FooBarBaz[Int], Baz[Int]]] // error: could not find implicit value

  // if we create an intermediate alias, implicit search works
  type BarBaz[A] = KPair[Bar, Baz, A]
  type FooBarBaz1[A] = KPair[Foo, BarBaz, A]

  implicitly[Getter[FooBarBaz1[Int], Foo[Int]]] // OK
  implicitly[Getter[FooBarBaz1[Int], Bar[Int]]] // OK
  implicitly[Getter[FooBarBaz1[Int], Baz[Int]]] // OK
}
@Blaisorblade

This comment has been minimized.

Show comment
Hide comment
@Blaisorblade

Blaisorblade Jun 20, 2016

Contributor

@TomasMikula IIUC, if you expanded implicitly into the needed calls to implicits, you need exactly this PR (and enabling the new behavior with -Ypartial-unification) to have the type arguments to the implicits inferred, right? Can you check if this works now? In principle it should, and e.g. the testcase https://github.com/scala/scala/pull/5102/files#diff-ecb4676a9b808a50a48374436ff39a29R17 suggests it also works in practice.

Contributor

Blaisorblade commented Jun 20, 2016

@TomasMikula IIUC, if you expanded implicitly into the needed calls to implicits, you need exactly this PR (and enabling the new behavior with -Ypartial-unification) to have the type arguments to the implicits inferred, right? Can you check if this works now? In principle it should, and e.g. the testcase https://github.com/scala/scala/pull/5102/files#diff-ecb4676a9b808a50a48374436ff39a29R17 suggests it also works in practice.

@TomasMikula

This comment has been minimized.

Show comment
Hide comment
@TomasMikula

TomasMikula Jun 20, 2016

Contributor

@Blaisorblade huh, the example actually compiles as is. I probably added Miles's si2712fix-plugin to build.sbt and forgot to call reload or something similarly stupid before. Thanks for checking, anyway!

Contributor

TomasMikula commented Jun 20, 2016

@Blaisorblade huh, the example actually compiles as is. I probably added Miles's si2712fix-plugin to build.sbt and forgot to call reload or something similarly stupid before. Thanks for checking, anyway!

@OlivierBlanvillain

This comment has been minimized.

Show comment
Hide comment
@OlivierBlanvillain

OlivierBlanvillain Jul 25, 2016

Contributor

I'm not sure the story is over, here are some examples which I think should compile with Ypartial-unification/si2712fix-plugin but don't.

Contributor

OlivierBlanvillain commented Jul 25, 2016

I'm not sure the story is over, here are some examples which I think should compile with Ypartial-unification/si2712fix-plugin but don't.

@cb372 cb372 referenced this pull request in guardian/apple-news-analytics-lambda Aug 4, 2016

Merged

Poll the Apple endpoint to retrieve a report #1

@philwills philwills referenced this pull request in guardian/riff-raff Oct 7, 2016

Merged

Switch to ValidatedNel / Validated instead of Either #369

@adriaanm adriaanm added 2.12 and removed 2.12 labels Oct 29, 2016

@adriaanm

This comment has been minimized.

Show comment
Hide comment
@djspiewak

This comment has been minimized.

Show comment
Hide comment
@djspiewak

djspiewak Mar 30, 2017

@adriaanm Wow… That definitely looks like a bug to me. Fabricating the Singleton out of nothingness is a neat trick.

@adriaanm Wow… That definitely looks like a bug to me. Fabricating the Singleton out of nothingness is a neat trick.

@paulp

This comment has been minimized.

Show comment
Hide comment
@paulp

paulp Mar 30, 2017

Contributor

Each extraneous factor I eliminate and it gets a little worse. It turns one doesn't need any compiler options at all - using the typelevel compiler is enough. I updated the gist to reflect this.

Contributor

paulp commented Mar 30, 2017

Each extraneous factor I eliminate and it gets a little worse. It turns one doesn't need any compiler options at all - using the typelevel compiler is enough. I updated the gist to reflect this.

@paulp

This comment has been minimized.

Show comment
Hide comment
@paulp

paulp Mar 31, 2017

Contributor

Here's a sliver of a diff after adding logging. The side which mentions Singleton is of course TLC.

Contributor

paulp commented Mar 31, 2017

Here's a sliver of a diff after adding logging. The side which mentions Singleton is of course TLC.

@djspiewak

This comment has been minimized.

Show comment
Hide comment
@djspiewak

djspiewak Mar 31, 2017

A guess would be that a change made to ensure inference doesn't throw away specific singleton types quite so aggressively that was meant to be behind a flag… isn't. It sounds more related to the 42.type implementation than SI-2712.

A guess would be that a change made to ensure inference doesn't throw away specific singleton types quite so aggressively that was meant to be behind a flag… isn't. It sounds more related to the 42.type implementation than SI-2712.

@paulp

This comment has been minimized.

Show comment
Hide comment
@paulp

paulp Mar 31, 2017

Contributor

@djspiewak I think your guess will prove to be correct.

Contributor

paulp commented Mar 31, 2017

@djspiewak I think your guess will prove to be correct.

@paulp

This comment has been minimized.

Show comment
Hide comment
@paulp

paulp Mar 31, 2017

Contributor

It figures to be this, from 8301e1f.

  case argTp@SingletonInstanceCheck(cmpOp, cmpArg) if settings.YliteralTypes =>

I'm supposing the side-effecting unapply is called, and then the settings guard is checked.

Contributor

paulp commented Mar 31, 2017

It figures to be this, from 8301e1f.

  case argTp@SingletonInstanceCheck(cmpOp, cmpArg) if settings.YliteralTypes =>

I'm supposing the side-effecting unapply is called, and then the settings guard is checked.

@milessabin

This comment has been minimized.

Show comment
Hide comment
@milessabin

milessabin Mar 31, 2017

Contributor

Yup, this relates to the literal types PR rather than this one.

Contributor

milessabin commented Mar 31, 2017

Yup, this relates to the literal types PR rather than this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment