Test case for new typeclass derivation scheme #6531

odersky · 2019-05-17T16:26:53Z

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 · 2019-05-17T16:49:53Z

For reference, here's the proposal by @milessabin

https://github.com/dotty-staging/dotty/commits/topic/type-class-derivation

To see the code the compiler needs to generate see ADTs_2.scala,

https://github.com/dotty-staging/dotty/blob/07db36d143ac9077ad3aacbc8ddc9891e7ab57b2/tests/typeclass-derivation/typeclass-derivation-4-split/ADTs_2.scala

odersky · 2019-05-17T17:08:29Z

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 · 2019-05-17T17:10:11Z

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?

milessabin · 2019-05-18T11:14:09Z

@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 · 2019-05-18T11:16:42Z

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

odersky · 2019-05-18T13:44:08Z

@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 · 2019-05-18T18:14:31Z

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 · 2019-05-19T07:10:24Z

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

milessabin · 2019-05-20T12:01:31Z

@odersky yes, that would be perfect :-)

OlivierBlanvillain · 2019-05-22T09:25:17Z

library/src/scala/deriving.scala

+  sealed trait Mirror {
+
+    /** The mirrored *-type */
+    type MonoType


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

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

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.

OlivierBlanvillain · 2019-05-22T09:26:02Z

library/src/scala/deriving.scala

+    /** The Mirror for a sum type */
+    trait Sum extends Mirror { self =>
+
+      type ElemTypes <: Tuple


/** The types of the elements */

OlivierBlanvillain · 2019-05-22T09:27:39Z

library/src/scala/deriving.scala

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


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

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.

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

compiler/src/dotty/tools/dotc/transform/SyntheticMembers.scala

OlivierBlanvillain · 2019-05-22T11:53:07Z

compiler/src/dotty/tools/dotc/transform/SyntheticMembers.scala

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


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

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

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

OlivierBlanvillain · 2019-05-22T12:00:50Z

library/src/scala/deriving.scala

+      def fromProduct(p: scala.Product): MonoType
+    }
+
+    trait Singleton extends Product {


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

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.

OlivierBlanvillain · 2019-05-22T12:08:28Z

compiler/src/dotty/tools/dotc/typer/Implicits.scala

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


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

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?

Because there is not always a companion object

sealed traits might come without one

enum cases don't have one either

OlivierBlanvillain · 2019-05-22T12:21:43Z

compiler/src/dotty/tools/dotc/core/TypeComparer.scala

+            tp.info match {
+              case info: MatchAlias =>
+                mapOver(tp)
+                  // TODO: We should follow the alias in this case, but doing so


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?

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

odersky · 2019-05-23T09:50:50Z

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 · 2019-05-23T10:50:12Z

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 · 2019-05-23T11:12:33Z

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 · 2019-05-23T11:22:22Z

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 · 2019-05-23T12:37:01Z

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

odersky · 2019-05-23T13:02:34Z

@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 · 2019-05-23T16:08:43Z

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 · 2019-05-23T16:16:58Z

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

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.

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 that was based on reflect.Generic

milessabin · 2019-05-30T15:41:27Z

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

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.

* 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 · 2019-06-02T22:54:36Z

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

odersky

Some minor changes needed, afterwards this can go in.

compiler/src/dotty/tools/dotc/typer/Implicits.scala

milessabin · 2019-06-04T08:32:09Z

Awesome :-)

I'll finish this up later today ...

milessabin · 2019-06-04T18:45:54Z

I'll merge as soon as this goes green.

nicolasstucki · 2019-06-04T20:00:26Z

library/src/scala/deriving.scala

+      type MirroredMonoType = this.type
+      type MirroredType = this.type
+      type MirroredElemTypes = Unit
+      type MirroredElemLabels = Unit


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

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?

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.

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

It is possible.

PR with a fix here: #6609.

odersky force-pushed the tc-derive branch 4 times, most recently from d5f49b5 to 314567a Compare May 18, 2019 09:53

odersky force-pushed the tc-derive branch from 81cb58c to c77fdcc Compare May 18, 2019 17:45

odersky requested a review from OlivierBlanvillain May 18, 2019 18:06

odersky assigned OlivierBlanvillain May 18, 2019

odersky force-pushed the tc-derive branch from 8487127 to 638b00e Compare May 19, 2019 22:09

odersky force-pushed the tc-derive branch 3 times, most recently from be28902 to 62b3c6a Compare May 22, 2019 11:41

OlivierBlanvillain reviewed May 22, 2019

View reviewed changes

odersky force-pushed the tc-derive branch from efd3473 to 5edbe57 Compare May 22, 2019 21:25

odersky added 11 commits May 30, 2019 16:23

Generate sum mirrors for sealed traits that do not have a companion

275a0a9

Use Label field for sum and product mirrors

e50a032

Synthesize mirrors also for Scala 2 defined classes and objects

eda4232

Polishings

f24e247

Update typeclass-scaling data using old Generic scheme

4784e2f

Drop old deriving infrastructure

56c0f96

Drop old deriving infrastructure that was based on reflect.Generic

Make scheme work also for nested classes and companion objects

01d3ff7

Don't constrain MirroredElemTypes to be a subtype of Tuple

6d3c2f9

Introduce MirroredTypeConstructor

c02861f

milessabin force-pushed the tc-derive branch from e5fe5e5 to c02861f Compare May 30, 2019 15:40

milessabin added 2 commits June 2, 2019 21:15

Avoid widening derived instances too far

930ca64

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.

odersky commented Jun 4, 2019

View reviewed changes

odersky assigned milessabin and unassigned OlivierBlanvillain Jun 4, 2019

milessabin added 3 commits June 4, 2019 19:16

Renamed elemTypes to elemsType

ddb0c4c

MirroredMonoType -> MirroredType in comment

bf67516

Removed redundant case and inlined RHSs

2afa89c

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

nicolasstucki reviewed Jun 4, 2019

View reviewed changes

allanrenucci deleted the tc-derive branch June 5, 2019 16:10

biboudis mentioned this pull request Jun 6, 2019

Add blogpost for 0.16.0-RC3 and 0.15.0 - (Scala Days Release) #6625

Merged

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

bishabosha mentioned this pull request Jan 5, 2021

Mirror.Sum not synthesised for base class without companion #10997

Closed

Test case for new typeclass derivation scheme #6531

Test case for new typeclass derivation scheme #6531

Conversation

odersky commented May 17, 2019

odersky commented May 17, 2019

odersky commented May 17, 2019

odersky commented May 17, 2019

milessabin commented May 18, 2019

milessabin commented May 18, 2019

odersky commented May 18, 2019 • edited Loading

odersky commented May 18, 2019

odersky commented May 19, 2019

milessabin commented May 20, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

odersky commented May 23, 2019 • edited Loading

milessabin commented May 23, 2019

odersky commented May 23, 2019

milessabin commented May 23, 2019

odersky commented May 23, 2019

odersky commented May 23, 2019 • edited Loading

milessabin commented May 23, 2019

milessabin commented May 23, 2019 • edited by allanrenucci Loading

milessabin commented May 30, 2019

milessabin commented Jun 2, 2019

odersky left a comment

Choose a reason for hiding this comment

milessabin commented Jun 4, 2019

milessabin commented Jun 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

odersky commented May 18, 2019 •

edited

Loading

odersky commented May 23, 2019 •

edited

Loading

odersky commented May 23, 2019 •

edited

Loading

milessabin commented May 23, 2019 •

edited by allanrenucci

Loading