Add support for generic tuples #4938

odersky · 2018-08-13T15:52:15Z

Based on #4927.

dottybot

Hello, and thank you for opening this PR! 🎉

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

Commit Messages

We want to keep history, but for that to actually be useful we have
some rules on how to format our commit messages (relevant xkcd).

Please stick to these guidelines for commit messages:

Separate subject from body with a blank line

When fixing an issue, start your commit message with Fix #<ISSUE-NBR>:

Limit the subject line to 72 characters

Capitalize the subject line

Do not end the subject line with a period

Use the imperative mood in the subject line ("Add" instead of "Added")

Wrap the body at 80 characters

Use the body to explain what and why vs. how

adapted from https://chris.beams.io/posts/git-commit

Have an awesome day! ☀️

odersky · 2018-08-13T17:03:01Z

This PR is currently blocked since dotty/library is compiled with Scala-2. I tried several workarounds but there's basically no way to do the kind of typelevel computation we want with Scala-2 not throwing up. So we need to the full bootstrap to be completed first.

smarter · 2018-08-13T17:07:26Z

In what kind of way does Scala 2 explode? Keep in mind that we could have two versions of "Tuple.scala", one compiled by Scala 2 and one compiled by Dotty, sbt makes this fairly easy to do.

odersky · 2018-08-14T09:14:46Z

The blockers right now are

Scala 2 does not understand rewrite modifiers. I can fake it with a rewrite annotation for methods, but would have to do something crazy for rewrite matches and rewrite ifs.
Scala 2 rejects this match:

    @`rewrite` private def _size(xs: Tuple): Int = //rewrite
    xs match {
      case _: Unit => 0
      case _: *:[_, xs1] => _size(erasedValue[xs1]) + 1
    }

because it does not know that Unit is a subtype of Tuple.

Both would be fixed if I could define a Dotty-only Tuple class. How do I do it? Put it in scalaShadowing?

smarter · 2018-08-14T09:16:47Z

It requires some sbt trickery, I can push a commit to this PR with an example and you can check if that works for you.

odersky · 2018-08-14T09:19:54Z

OK, thanks!

smarter · 2018-08-14T10:37:40Z

See the latest commit, you should be able to put any dotty-only files in library/src-scala3

smarter · 2018-08-14T13:37:59Z

library/src/scala/TupleXXL.scala

+  override def toString = elems.mkString("(", ",", ")")
+  override def hashCode = getClass.hashCode * 41 + elems.deep.hashCode
+  override def equals(that: Any) = that match {
+    case that: TupleXXL => this.elems.deep.equals(that.elems.deep)


.deep has been removed from 2.13 (scala/scala#6912), but you can use Arrays.deepEquals instead: https://docs.oracle.com/javase/7/docs/api/java/util/Arrays.html#deepEquals(java.lang.Object[],%20java.lang.Object[])

odersky · 2018-08-14T14:35:38Z

Cool! the src-scala2/src-scala3 seems to have done the trick!

odersky · 2018-08-15T11:46:52Z

@smarter I found an issue: launchIDE does not work anymore. I get:

sbt:dotty> dotty-bootstrapped/clean
[success] Total time: 0 s, completed Aug 15, 2018 1:43:50 PM
sbt:dotty> launchIDE
[info] Cleaning the dotty-sbt-bridge cache
[info] Updating dotty-library-bootstrapped...
[info] Updating dotty-interfaces...
[info] Done updating.
[info] Done updating.
[info] Updating dotty-compiler-bootstrapped...
[info] Updating dotty-compiler...
[info] Done updating.
[info] Updating dotty-sbt-bridge-bootstrapped...
[info] Updating dotty-doc-bootstrapped...
[info] Updating dotty-language-server...
[info] Compiling 7 Java sources to /Users/odersky/workspace/dotty/interfaces/target/classes ...
[info] Done compiling.
[info] Updating dotty-bench-bootstrapped...
[info] Done updating.
[warn] Multiple main classes detected. Run 'show discoveredMainClasses' to see the list
[info] Done updating.
[info] Compiling 59 Scala sources and 136 Java sources to /Users/odersky/workspace/dotty/library/../out/bootstrap/dotty-library-bootstrapped/scala-0.10/classes ...
[error] Bad symbolic reference. A signature
[error] refers to *:/T in package scala which is not available.
[error] It may be completely missing from the current classpath, or the version on
[error] the classpath might be incompatible with the version used when compiling the signature.
[error] Bad symbolic reference. A signature
[error] refers to Tuple/T in package scala which is not available.
[error] It may be completely missing from the current classpath, or the version on
[error] the classpath might be incompatible with the version used when compiling the signature.

smarter · 2018-08-15T14:24:28Z

@odersky I just tried it on the latest commit of this branch and couldn't reproduce the error. Does it happen if you do both clean and dotty-bootstrapped/clean first ? It might be caused by some classpath-related environment variable from your .bashrc.

smarter · 2018-08-15T14:25:39Z

Also try restarting sbt maybe.

odersky · 2018-08-15T16:30:48Z

Now it works for me as well. Sorry for the false alarm!

odersky · 2018-08-15T16:58:50Z

Update on performance:

    object Test extends App {
      val xs0 = (1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16)
      assert(xs0(15) == 16)
        // 2.753s

      val xs1 = xs0 ++ xs0
      assert(xs1(31) == 16)
        // 3.426s

      val xs2 = xs1 ++ xs1
      assert(xs2(63) == 16)
        // 4.082s

      val xs3 = xs2 ++ xs2
      assert(xs3(127) == 16)
        // 4.786s

      val xs4 = xs3 ++ xs3
      assert(xs4(255) == 16)
        // 6.543s

      val xs5 = xs4 ++ xs4
      assert(xs5(511) == 16)
        // 15.041s

    }

Times behind // are (cold) compile times up to this point. The last two (or three?) blocks required a stack size of 10M.

One has has to change -Xmax-inlines accordingly. E.g. for the last block

    java -Xms1g -Xmx3g -Xss10m dotty.tools.dotc.Main tuples2.scala -Xmax-inlines 520

Summary: it would be good if we could improve this.

odersky · 2018-08-15T17:09:24Z

Code size: The Test class above generated 48K, almost all of which were generic signatures involving the non-existent *: type. After the last commit it went down to 5.4K, which looks reasonable.

odersky · 2018-08-15T17:10:55Z

.tasty size is 7.7K btw.

odersky · 2018-08-16T09:21:51Z

What could be done to make the compile times faster? The problem are extensive type-level rewritings. I.e. in the last block we rewrite 256-512 step concat trees 256 times (times some constant factor because the same trees are traversed and rewritten several times in each inlining step). That's a lot of nodes to rewrite and typecheck, and little to show for it, since in the end the result of these rewritings is a small generic tree that works with TupleXXL.

The rewritings are needed to

compute the type of the resulting term
compute the size of that type

One major savings would be if we would compute the type as a type and not as a recursive type tree. So that means lifting term computations to types, analogous to the "TypeOf" approach. If we can do this, we could compute the size also as a (constant) type, and thereby save all typelevel computations on large terms. Typelevel computations on terms up to size 22 are still needed to
generate the right code, but these are less of a problem.

odersky · 2018-08-17T13:09:24Z

@liufengyun We get a wrong exhaustivity warning in run/tuples1.scala. It has to do with TupleXXL matching. Can you take a look?

odersky · 2018-08-17T13:09:45Z

test performance please

dottybot · 2018-08-17T13:11:13Z

performance test scheduled: 7 job(s) in queue, 1 running.

dottybot · 2018-08-17T14:57:20Z

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/4938/ to see the changes.

Benchmarks is based on merging with master (61cba97)

liufengyun · 2018-08-17T15:46:08Z

@odersky OK, I'll investigate.

liufengyun · 2018-08-18T06:24:21Z

There are two problems with TupleXXL.unapplySeq.

object TupleXXL {
  def unapplySeq(x: TupleXXL): Option[Seq[Any]] = Some(x.elems.toSeq)
}

First, we need to change the return type to Some[Seq[Any]], to indicate that is irrefutable.

Second, after typer, we get a pattern match as follows:

    Test.x23 match
      {
        case
          TupleXXL.unapplySeq(x1 @ _, x2 @ _, x3 @ _, x4 @ _, x5 @ _, x6 @ _,
            x7 @ _
          , x8 @ _, x9 @ _, x10 @ _, x11 @ _, x12 @ _, x13 @ _, x14 @ _, x15 @ _
            ,
          x16 @ _, x17 @ _, x18 @ _, x19 @ _, x20 @ _, x21 @ _, x22 @ _, x23 @ _
            )
          : TupleXXL

There is no information from TupleXXL.unapplySeq says that it will return a sequence of exact 23 elements, so the checker produces a warning here.

odersky · 2018-08-19T10:23:05Z

There is no information from TupleXXL.unapplySeq says that it will return a sequence of exact 23 elements, so the checker produces a warning here.

Right. We do not have enough information in TupleXXL to be able to tell whether the warning is real or not. So I believe we have to suppress it. Or we have to special-case TupleXXL in the exhaustivity checker to propagate the type of its argument into the patterns.

nicolasstucki · 2018-09-04T08:57:35Z

tests/run/i4803d/App_2.scala

@@ -10,7 +10,7 @@ object Test {
    println(power2(x3))
  }

-  rewrite def power2(x: Double) = {
+  def power2(x: Double) = {


Given the possible future changes in rewrite we should not do this change in the tests as it does not represent the use case from #4803 anymore. Instead, move it to disabled tests.

nicolasstucki · 2018-09-04T11:20:46Z

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

+  /** Add a `Tuple` as a parent to `Unit`.
+   *  Add the right `*:` instance as a parent to Tuple1..Tuple22
+   */
+  def fixTupleCompleter(cls: ClassSymbol): Unit = cls.infoOrCompleter match {


This logic looks is quite convoluted. Are there more direct approaches that do not require patching the completers?

I looked hard but did not find anything that would work better than patching the completers. The syntheticParent code can be simplified using an utility method that I introduced in the subsequent PR #4964. I'll do the simplification there.

nicolasstucki · 2018-09-04T11:38:17Z

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

@@ -376,7 +379,7 @@ object Erasure {
     *  on selections `e.m`, where `OT` is the type of the owner of `m` and `ET`
     *  is the erased type of the selection's original qualifier expression.
     *
-     *      e.m1 -> e.m2          if `m1` is a member of Any or AnyVal and `m2` is
+     *      e.m1 -> e.m2          if `m1` is a member of a class that erases to object and `m2` is


typo object -> Object

nicolasstucki · 2018-09-04T11:54:26Z

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

+      ctx.error(ex"implementation restriction: nested rewrite methods are not supported", inlined.pos)
+    if (inlined.name == nme.unapply && tupleArgs(body).isEmpty)
+      ctx.warning(
+        em"rewrite unapply method can be rewritten only if its tight hand side is a tuple (e1, ..., eN)",


Typo tight -> right

nicolasstucki · 2018-09-04T11:58:40Z

library/src/scala/TupleXXL.scala

+
+final class TupleXXL private (es: Array[Object]) {
+  override def toString = elems.mkString("(", ",", ")")
+  override def hashCode = getClass.hashCode * 41 + deepHashCode(elems)


Not sure what getClass.hashCode * 41 gives to the hash.

It's just to distinguish TupleXXL hashes from other hashes over elems.

nicolasstucki · 2018-09-04T12:04:37Z

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

+    /** The arity of this tuple type, which can be made up of Unit, TupleX and `*:` pairs,
+     *  or -1 if this is not a tuple type.
+     */
+    def tupleArity(implicit ctx: Context): Int = self match {


Could be tailtec:

def tupleArity(implicit ctx: Context): Int = { def tupleArity(tp: Type, arity: Int): Int = tp match { case AppliedType(tycon, _ :: tl :: Nil) if tycon.isRef(defn.PairClass) => tupleArity(tl, arity + 1) case tp1 => if (tp1.isRef(defn.UnitClass)) arity else if (defn.isTupleClass(tp1.classSymbol)) arity + tp1.dealias.argInfos.length else -1 } tupleArity(self, 0) }

I believe that version behaves differently for *: sequences that do not end in Unit.

Maybe, still there exists a correct tailrec version of the method. Can be revisited later.

These types are injected as parents of Tuple1,...,Tuple22, but are eliminated again at erasure. To support them properly before full dotty bootstrap we needed a @`rewrite` annotation that erases the annotated method (unlike `@forceInline`, which generates code).

And replace Tuple.scala with a Scala 2 implementation and a Scala 3 implementation. The Scala 2 implementaion of Tuple.scala is not strictly necessary but could be useful if non-version-specific library sources end up referencing scala.Tuple.

Erased code does not keep full Inlined trees. The previous version converted the Inlined tree back to the call, but this can expose leaks (since calls are not traversed for references). We now keep the Inlined node, so that its call part can be subsequently simplified.

With the Scala 2/3 split, we don't need it anymore.

Does not rely anymore on `deep`, which is dropped in Scala 2.14.

Was erased to Object before, but this loses precision and breaks binary compatibility with Scala 2.

Currently, this is not exploited but some rewrites might become more effective that way in the future.

There's a difference to be made between projections of instance creations in a preceding val on the one hand and instance creations in a def or directly written creations on the other hand.

Currently only unapplys that return a tuple are supported.

- no nested rewrites - calls to rewrite unapplys only in rewrite code

tuples1.scala is explainable and harmless. The other two (i4758 and t7426) give a stale symbol but in my setup they do that already on master. I wonder how they slipped through the CI.

OlivierBlanvillain · 2018-09-06T17:15:29Z

compiler/test/dotc/pos-test-pickling.blacklist

@@ -33,6 +33,7 @@ i4125.scala
 i4167
 i4586.scala
 i4734
+i4758


They slipped through the CI because before #5059 the CI didn't run -Ytest-pickler on pos and run tests..

dottybot reviewed Aug 13, 2018

View reviewed changes

odersky added the stat:wip label Aug 13, 2018

smarter force-pushed the add-tuples branch from 31c631b to d0b063a Compare August 14, 2018 10:00

smarter reviewed Aug 14, 2018

View reviewed changes

odersky force-pushed the add-tuples branch from 6ec3054 to 488b3d5 Compare August 14, 2018 13:47

odersky force-pushed the add-tuples branch from 6cb9868 to 5029642 Compare August 15, 2018 16:36

odersky mentioned this pull request Aug 16, 2018

Allow rewriting on types #4940

Closed

odersky removed the stat:wip label Aug 17, 2018

nicolasstucki requested changes Sep 4, 2018

View reviewed changes

nicolasstucki assigned odersky and unassigned nicolasstucki Sep 4, 2018

nicolasstucki reviewed Sep 4, 2018

View reviewed changes

odersky and others added 16 commits September 6, 2018 17:34

Add Tuple and *: types

f080a39

These types are injected as parents of Tuple1,...,Tuple22, but are eliminated again at erasure. To support them properly before full dotty bootstrap we needed a @`rewrite` annotation that erases the annotated method (unlike `@forceInline`, which generates code).

Remove rewrite annotation again

6dde9ea

With the Scala 2/3 split, we don't need it anymore.

Add generative operations to tuples

75729c8

Fix TupleXXL equals

0a24170

Does not rely anymore on `deep`, which is dropped in Scala 2.14.

More generative operations on Tuples: tail, apply, ++

b9f3ab7

Erase *: to tuple classes

26f0d75

Was erased to Object before, but this loses precision and breaks binary compatibility with Scala 2.

Allow tuple literals to extend beyond 22

2d1427d

Don't generate *: in generic Java signatures

bc08290

Test pattern matching of tuples >= 23.

0378bb4

Whitelist some Scala-2 classes as pure

79ca51c

Currently, this is not exploited but some rewrites might become more effective that way in the future.

Fix side-effect handling in reduceProjection

0285243

There's a difference to be made between projections of instance creations in a preceding val on the one hand and instance creations in a def or directly written creations on the other hand.

Support a limited form of rewrite unapplys

0996a4a

Currently only unapplys that return a tuple are supported.

Enforce implementation restrictions

3d48ff3

- no nested rewrites - calls to rewrite unapplys only in rewrite code

Address review comments

a160063

odersky force-pushed the add-tuples branch from 98971a6 to a160063 Compare September 6, 2018 15:44

Add failing pickling test to blacklist

42a5a9d

tuples1.scala is explainable and harmless. The other two (i4758 and t7426) give a stale symbol but in my setup they do that already on master. I wonder how they slipped through the CI.

odersky force-pushed the add-tuples branch from ba5e1d3 to 42a5a9d Compare September 6, 2018 16:47

OlivierBlanvillain reviewed Sep 6, 2018

View reviewed changes

odersky assigned nicolasstucki and unassigned odersky Sep 6, 2018

nicolasstucki approved these changes Sep 6, 2018

View reviewed changes

nicolasstucki merged commit 00e396c into scala:master Sep 6, 2018

nicolasstucki deleted the add-tuples branch September 6, 2018 19:12

allanrenucci mentioned this pull request Oct 16, 2018

Redesign Tuples with HList-like structure #2199

Closed

4 tasks

Add support for generic tuples #4938

Add support for generic tuples #4938

Conversation

odersky commented Aug 13, 2018 • edited

dottybot left a comment

Choose a reason for hiding this comment

Commit Messages

odersky commented Aug 13, 2018

smarter commented Aug 13, 2018

odersky commented Aug 14, 2018

smarter commented Aug 14, 2018

odersky commented Aug 14, 2018

smarter commented Aug 14, 2018

Choose a reason for hiding this comment

odersky commented Aug 14, 2018

odersky commented Aug 15, 2018

smarter commented Aug 15, 2018 • edited

smarter commented Aug 15, 2018

odersky commented Aug 15, 2018

odersky commented Aug 15, 2018

odersky commented Aug 15, 2018 • edited

odersky commented Aug 15, 2018

odersky commented Aug 16, 2018 • edited

odersky commented Aug 17, 2018

odersky commented Aug 17, 2018

dottybot commented Aug 17, 2018

dottybot commented Aug 17, 2018

liufengyun commented Aug 17, 2018

liufengyun commented Aug 18, 2018

odersky commented Aug 19, 2018

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 Aug 13, 2018 •

edited

smarter commented Aug 15, 2018 •

edited

odersky commented Aug 15, 2018 •

edited

odersky commented Aug 16, 2018 •

edited