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

Test case for new typeclass derivation scheme #6531

Merged
merged 35 commits into from Jun 4, 2019

Conversation

Projects
None yet
5 participants
@odersky
Copy link
Contributor

commented May 17, 2019

A simplified test case which adopts several improvements from Miles Sabin's branch.
Compared to typeclass-derivation2c.scala:

  • The alternatives and numberOfCases methods are gone
  • All compiler-generated info is in the companion objects;
    no additional classes are generated.
  • The derivation code largely uses the type traversals of
    the previous schemes instead of simple counting, as was
    the case in typeclass-derivation2c.scala.
  • Generic has been renamed to Mirror (but now all mirrors are static)

The key insight is that we do not need a mapping from the sum type
to its alternatives except knowing what the alternative types are.
Once we have an alternative type, we can use an implicit match
to retrieve the mirror for that alternative.

@odersky

This comment has been minimized.

@odersky

This comment has been minimized.

Copy link
Contributor Author

commented May 17, 2019

I noted the failures come from #6319. It worked fine on my local branch where I have not merged #6319 yet. Need to investigate further what goes wrong here /cc @OlivierBlanvillain

@odersky

This comment has been minimized.

Copy link
Contributor Author

commented May 17, 2019

Regarding c1fbed9 it's not clear to me that we want to keep MirroredType as a type member. Making it a type parameter seems simpler. @milessabin WDYT?

@odersky odersky force-pushed the dotty-staging:tc-derive branch 4 times, most recently from d5f49b5 to 314567a May 18, 2019

@milessabin

This comment has been minimized.

Copy link
Contributor

commented May 18, 2019

@odersky I've just confirmed on my branch that it's safe to make MonoType a type parameter of Mirror.

I've also seen the failures due to #6319. I'll try and boil these down to a couple of standalone reproductions.

The branch itself contains a few fairly clunky workarounds for match type issues and I'd rather make the code that I really want to work, work, rather than making the workarounds work.

@milessabin

This comment has been minimized.

Copy link
Contributor

commented May 18, 2019

MonoType member -> parameter change here: dotty-staging@5930938.

@odersky

This comment has been minimized.

Copy link
Contributor Author

commented May 18, 2019

@milessabin I went back to the type member scheme, since it allows a nicer treatment of singleton mirrors. The problem is that you can't write:

 class EnumValue extends Mirror.Singleton[this.type] { ... }

the this.type resolves to the object containing EnumValue in this case.

@odersky odersky force-pushed the dotty-staging:tc-derive branch from 81cb58c to c77fdcc May 18, 2019

@odersky odersky requested a review from OlivierBlanvillain May 18, 2019

@odersky

This comment has been minimized.

Copy link
Contributor Author

commented May 18, 2019

What needs to be generated ahead of time:

  • For a sum type: one method (ordinal) and one type (MonoType)
  • For a product type: one method (fromProduct) and one type (MonoType)
  • For a singleton: just an additional parent in the extends clause

I believe we have found the optimum now.

@odersky

This comment has been minimized.

Copy link
Contributor Author

commented May 19, 2019

@milessabin How should we proceed on this? I can change Deriving.scala to use the new code generation scheme and do a rudimentary synthesis of Mirror implicits that I leave to you to flesh out. Would that work for you?

@odersky odersky force-pushed the dotty-staging:tc-derive branch from 8487127 to 638b00e May 19, 2019

@milessabin

This comment has been minimized.

Copy link
Contributor

commented May 20, 2019

@odersky yes, that would be perfect :-)

@odersky odersky force-pushed the dotty-staging:tc-derive branch 3 times, most recently from be28902 to 62b3c6a May 20, 2019

sealed trait Mirror {

/** The mirrored *-type */
type MonoType

This comment has been minimized.

Copy link
@OlivierBlanvillain

OlivierBlanvillain May 22, 2019

Contributor

What's the idea behind the MonoType name? It there a PolyType planned for later? My first intuition would be to call it something like MirroredType or SelfType

This comment has been minimized.

Copy link
@odersky

odersky May 22, 2019

Author Contributor

I let Miles answer that. The name comes from him.

This comment has been minimized.

Copy link
@milessabin

milessabin May 22, 2019

Contributor

See my branch. MirroredType is used there as the type with the same kind as the type being mirrored. MonoType is MirroredType existentially quantified. I'll add MirroredType later along with some differently kinded examples.

/** The Mirror for a sum type */
trait Sum extends Mirror { self =>

type ElemTypes <: Tuple

This comment has been minimized.

Copy link
@OlivierBlanvillain

OlivierBlanvillain May 22, 2019

Contributor

/** The types of the elements */


/** The ordinal number of the case class of `x`. For enums, `ordinal(x) == x.ordinal` */
def ordinal(x: MonoType): Int
}

This comment has been minimized.

Copy link
@OlivierBlanvillain

OlivierBlanvillain May 22, 2019

Contributor

Don't we also need the counterpart to type CaseLabel <: String for sums?

This comment has been minimized.

Copy link
@milessabin

milessabin May 22, 2019

Contributor

I've argued that we do. I think @odersky agreed that it could be added later. I'm happy to add that if he still agrees.

This comment has been minimized.

Copy link
@odersky

odersky May 22, 2019

Author Contributor

I have changed now to Label for both sums and products.

Show resolved Hide resolved compiler/src/dotty/tools/dotc/transform/SyntheticMembers.scala

/** For an enum T:
*
* def ordinal(x: MonoType) = x.enumTag

This comment has been minimized.

Copy link
@OlivierBlanvillain

OlivierBlanvillain May 22, 2019

Contributor

I'm skeptical about having to special case any of this for enum. I would expect the generic derivation framework to simply fall apart in the case where enums a desugared to vals since there is nothing to populate Mirror.Sum.ElemTypes...

This comment has been minimized.

Copy link
@odersky

odersky May 22, 2019

Author Contributor

Oh, but there is: The singleton types of the enum values.

This comment has been minimized.

Copy link
@milessabin

milessabin May 22, 2019

Contributor

Also see my branch ... all the Nil-like types fall into the same category.

def fromProduct(p: scala.Product): MonoType
}

trait Singleton extends Product {

This comment has been minimized.

Copy link
@OlivierBlanvillain

OlivierBlanvillain May 22, 2019

Contributor

How is this different from an empty product? Why do we need such distinction?

This comment has been minimized.

Copy link
@odersky

odersky May 22, 2019

Author Contributor

First, it's an optimization since we do not need to spell out ElemTypes and CaseLabels.
Second, it's important for printing. A case class case class C() should print as C(), but
a case object case object C should print as C. We can tell the difference since the mirror
of the case object is a Mirror.Singleton.

.refinedWith(tpnme.ElemLabels, TypeAlias(TypeOps.nestedPairs(elemLabels)))
val modul = cls.linkedClass.sourceModule
assert(modul.is(Module))
ref(modul).withSpan(span).cast(mirrorType)

This comment has been minimized.

Copy link
@OlivierBlanvillain

OlivierBlanvillain May 22, 2019

Contributor

What's preventing us from simply using actual type of modul here?

This comment has been minimized.

Copy link
@OlivierBlanvillain

OlivierBlanvillain May 22, 2019

Contributor

I a bit confused about these implicits. If we need them, why synthesize them as opposed to adding an implicit val mirror: ... = this in companion objects?

This comment has been minimized.

Copy link
@odersky

odersky May 22, 2019

Author Contributor

Because there is not always a companion object

  • sealed traits might come without one
  • enum cases don't have one either
tp.info match {
case info: MatchAlias =>
mapOver(tp)
// TODO: We should follow the alias in this case, but doing so

This comment has been minimized.

Copy link
@OlivierBlanvillain

OlivierBlanvillain May 22, 2019

Contributor

Is this just about error reporting (proper cyclic reference instead of a stack overflow), or do you actually have a type correct program that makes the compiler loop here?

This comment has been minimized.

Copy link
@odersky

odersky May 22, 2019

Author Contributor

The second: if we would follow the alias here instead of just doing a mapOver every recursive match type would loop.

@odersky odersky force-pushed the dotty-staging:tc-derive branch from efd3473 to 5edbe57 May 22, 2019

@odersky

This comment has been minimized.

Copy link
Contributor Author

commented May 23, 2019

Summary of measured differences with the old scheme:

  • About 100 lines more compiler code - the rest of the lines changed diff is tests.
  • About 13-15% more code generated for typeclass instances (as measured using the typeclass-scaling.scala test, see 63b7ae6)
  • About 3-4% slower to compile typeclass instances (same test)

Advantages of new scheme:

  • Fewer allocations, since mirrors are usually shared instead of being allocated at runtime.
  • It works well even if there are no derives clauses. The old scheme would generate more code in that case.
  • Complete decoupling between derives clauses and mirror generation.
@milessabin

This comment has been minimized.

Copy link
Contributor

commented May 23, 2019

The type class code gen size and compile time results you're reporting are what you'd expect from a scheme based on aggressive inlining, as you've done here. The erased scheme in my branch should improve significantly on both :-)

@odersky

This comment has been minimized.

Copy link
Contributor Author

commented May 23, 2019

The type class code gen size and compile time results you're reporting are what you'd expect from a scheme based on aggressive inlining, as you've done here. The erased scheme in my branch should improve significantly on both :-)

Cool. We should run the same tests with the new scheme as well once it is ready. It's

pos-special/typeclass-scaling.scala.

Instructions are in the test.

@milessabin

This comment has been minimized.

Copy link
Contributor

commented May 23, 2019

Playing with it now ... looks good :-)

From my PoV the only thing missing is the MirroredType with the kind matching that of the type being mirrored. I'm happy to add that unless you'd rather.

@odersky

This comment has been minimized.

Copy link
Contributor Author

commented May 23, 2019

I can give it a shot since I am still on the branch.

@odersky

This comment has been minimized.

Copy link
Contributor Author

commented May 23, 2019

@milessabin Actually, I have a question about MirroredType.

I know that, if MonoType = List[_] then MirroredType = [t] => List[t].

But what if MonoType = List[Int]? Is MirroredType then also List[Int] or again [t] => List[t]?

EDIT: Looking at your examples, I conclude it should be List[Int]. Correct?

@milessabin

This comment has been minimized.

Copy link
Contributor

commented May 23, 2019

But what if MonoType = List[Int]? Is MirroredType then also List[Int] or again [t] => List[t]?

EDIT: Looking at your examples, I conclude it should be List[Int]. Correct?

Yes, that's correct.

@milessabin

This comment has been minimized.

Copy link
Contributor

commented May 23, 2019

While experimenting with the current state of this branch I had to make the following change,

--- library/src/scala/deriving.scala
+++ library/src/scala/deriving.scala
@@ -19,7 +19,7 @@ object deriving {
     trait Sum extends Mirror { self =>
 
       /** The types of the alternatives */
-      type MirroredElemTypes <: Tuple
+      //type MirroredElemTypes <: Tuple
 
       /** The ordinal number of the case class of `x`. For enums, `ordinal(x) == x.ordinal` */
       def ordinal(x: MirroredMonoType): Int
@@ -29,7 +29,7 @@ object deriving {
     trait Product extends Mirror {
 
       /** The types of the product elements */
-      type MirroredElemTypes <: Tuple
+      //type MirroredElemTypes <: Tuple
 
       /** The names of the product elements */
       type MirroredElemLabels <: Tuple
@@ -53,9 +53,9 @@ object deriving {
       def fromProduct(p: scala.Product) = value
     }
 
-    type Of[T]        = Mirror { type MirroredMonoType = T }
-    type ProductOf[T] = Mirror.Product { type MirroredMonoType = T }
-    type SumOf[T]     = Mirror.Sum { type MirroredMonoType = T }
+    type Of[T]        = Mirror { type MirroredMonoType = T ; type MirroredElemTypes <: Tuple }
+    type ProductOf[T] = Mirror.Product { type MirroredMonoType = T ; type MirroredElemTypes <: Tuple }
+    type SumOf[T]     = Mirror.Sum { type MirroredMonoType = T ; type MirroredElemTypes <: Tuple }
   }

The issue that this addresses is that the declaration,

trait Product/Sum extends Mirror {
  ...
  type MirroredElemTypes <: Tuple
  ...
}

commits us to MirroredElemTypes being of kind *. We can move that refinement to the definitions of Of, ProductOf and SumOf which are already committed to being of kind * without affecting the client code in any way, but it leaves MirroredElemTypes free to be refined to whatever kind is appropriate.

You can see this on my branch in Library_3.scala you'll see that there's an equivalent of Of, ProductOf, SumOf defined for each of the kinds which are directly supported (in objects K0, K1, K11 and K2).

odersky added some commits May 22, 2019

Avoid name clashes by prefixing all type members with Mirrored
We got a community build failure since a companion object acquired
a `Label` type member by auto-generating a `Mirror` parent. To avoid
name clashes (or at least make thme very unlikely), we now systematically
prefix all type fields with  `Mirrored`.

We probably should do something similar to the auto-generated `ordinal`
and `fromProduct` methods.
Measure typeclass scaling using new scheme
Summary:

 - Code size increases by 13% (counting characters) to 15% (counting words)
 - Compile time increases by 3% (wallclock) to 4% (user)

Runtime should be somewhat better for new scheme since there are fewer allocations.

(Btw, the -Yshow-no-inlines option in the old measurements should be ignored; it is
no linger valid and was not included in the tests)
Drop old deriving infrastructure
Drop old deriving infrastructure that was based on reflect.Generic

@milessabin milessabin force-pushed the dotty-staging:tc-derive branch from e5fe5e5 to c02861f May 30, 2019

@milessabin

This comment has been minimized.

Copy link
Contributor

commented May 30, 2019

Rebased. There's more to come, but I wanted to get this on top of the poly functions stuff now that it's merged.

milessabin added some commits Jun 2, 2019

Avoid widening derived instances too far
If derived instances are created in companions with the Synthetic flag
set then widenImplied will widen too far, and the derived instances will
have too low a priority to be selected over a freshly derived instances
at the summoning site.
Generalized type class derivation for higher kinded type classes
* Mirrors of data types of kinds other than * are now generated. The
Mirror type member MirroredTypeConstructor has been renamed to
MirroredType: this is the type which is used to select the "naturally"
kinded Mirror (ie. the Mirror with the kind that matches the kind of the
data type).

* Data types can now have derives clauses for type class which are
indexed by types of the same kind, for all kinds. Data types continue to
support derives clauses for type classes indexed by types of lower kinds
than the data type via polymorphic derived members.
@milessabin

This comment has been minimized.

Copy link
Contributor

commented Jun 2, 2019

I've updated my branch relative to this: dotty-staging@4e434e0.

@odersky
Copy link
Contributor Author

left a comment

Some minor changes needed, afterwards this can go in.

@milessabin

This comment has been minimized.

Copy link
Contributor

commented Jun 4, 2019

Awesome :-)

I'll finish this up later today ...

@milessabin

This comment has been minimized.

Copy link
Contributor

commented Jun 4, 2019

I'll merge as soon as this goes green.

@milessabin milessabin merged commit 8144f98 into lampepfl:master Jun 4, 2019

1 check passed

continuous-integration/drone/pr the build was successful
Details
type MirroredMonoType = this.type
type MirroredType = this.type
type MirroredElemTypes = Unit
type MirroredElemLabels = Unit

This comment has been minimized.

Copy link
@nicolasstucki

nicolasstucki Jun 4, 2019

Contributor

This fails when compiling from scala 2 as Unit is not a subtype of Tuple

This comment has been minimized.

Copy link
@milessabin

milessabin Jun 5, 2019

Contributor

What's the best way to fix this? We can remove the bound in Mirror (if we do we need to add an & Tuple at one use-site in tests/run/typeclass-derivation3.scala)? Or we could move this file to src-3.x?

This comment has been minimized.

Copy link
@nicolasstucki

nicolasstucki Jun 5, 2019

Contributor

Yes, we should move it to src-3.x. We may also need a dummy version of it in src-2.x the only defines classes, methods and types that we refere to in Definitions.

This comment has been minimized.

Copy link
@milessabin

milessabin Jun 5, 2019

Contributor

Could the dummy version be identical, but without the Tuple bound?

This comment has been minimized.

Copy link
@nicolasstucki

nicolasstucki Jun 5, 2019

Contributor

It is possible.

This comment has been minimized.

Copy link
@milessabin

milessabin Jun 5, 2019

Contributor

PR with a fix here: #6609.

@allanrenucci allanrenucci deleted the dotty-staging:tc-derive branch Jun 5, 2019

@biboudis biboudis added this to the 0.16 Tech Preview milestone Jun 7, 2019

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.