Move TailRec after Erasure. #5112

sjrd · 2018-09-15T18:19:46Z

This used not to be possible because it was too complicated for TailRec to understand trees produced by the pattern matcher. Now that patmat uses Labeled blocks instead of label-defs, it is trivial to do so: the argument of a return from a labeled block lab is in tail position if and only if the labeled block lab is
itself in tail position.

Running TailRec after erasure has two major benefits:

it is much simpler, as it does not have to deal with type parameters, TypeApplys, and a bunch of other stuff.
it supports polymorphic tail-recursive calls by construction (recursive calls whose receiver or method has different type parameters).

It also has one difficulty: it cannot see the call-site @tailrec annotations anymore. This is why we add a mini-phase to record such annotations as tree attachments.

Having TailRec after erasure will also be necessary to later make it use Labeled blocks and loops instead of label-defs itself. Indeed, in that case it will need to declare local vars for its term parameters, which would break path-dependent types within the method if it were done before erasure.

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! ☀️

sjrd · 2018-09-15T18:20:20Z

At this point, I still have bootstrap failures locally, but I'd like to see what the CI says.

smarter · 2018-09-15T18:27:53Z

It also has one difficulty: it cannot see the call-site @tailrec annotations anymore. This is why we add a mini-phase to record such annotations as tree attachments.

We try to avoid using tree attachments unless there's no other choice in Dotty since they're a bit too magic, and using them to communicate across phases seems especially scary. Can we try to find an alternative solution? I suggest simply changing erasure to not drop annotations and check if that breaks anything.

sjrd · 2018-09-15T18:31:29Z

Yeah, I'm not happy with the tree attachment either. I just wanted to make those tests pass rather than moving them to pending (because, you know, one doesn't just remove tests), but it certainly is open for suggestions.

I don't think I have the stomach to try and change erasure right now, though. I suggest we discuss that at the next meeting.

sjrd · 2018-09-18T20:48:58Z

After a discussion with @odersky today, I added a commit that simply removes the call-site @tailrec checks feature altogether. Needs discussion.

sjrd · 2018-09-22T09:27:32Z

Rebased.

smarter · 2018-09-24T14:04:26Z

test performance please

dottybot · 2018-09-24T14:06:23Z

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

dottybot · 2018-09-24T15:15:44Z

Performance test finished successfully:

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

Benchmarks is based on merging with master (a0b429b)

allanrenucci

The doc at the top of the file needs to be updated. It mentions things about pattern matching and type parameters which are no longer true. Maybe add a paragraph about labeled blocks

allanrenucci · 2018-09-24T15:30:16Z

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

            val rhs = transformer.transform(dd.rhs)
            rewrote = transformer.rewrote
            rhs
          }

          if (rewrote) {
-            val dummyDefDef = cpy.DefDef(tree)(rhs = rhsSemiTransformed)
+            assert(dd.tparams.isEmpty, dd)


This is always true. Maybe rewrite the match case as:

tree match { - case dd@DefDef(name, Nil, vparams0 :: Nil, tpt, _) + case dd@DefDef(name, _, vparams0 :: Nil, tpt, _)

allanrenucci · 2018-09-25T09:43:10Z

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

-    val name = TailLabelName.fresh()
+      val enclosingClass = method.enclosingClass.asClass
+      val thisParamType =
+        if (enclosingClass.is(Flags.Module)) enclosingClass.thisType


Why do we have this distinction for ModuleClass?

Because the type of this in a module class is something like TheModule.this.type (not TheModule$, for example) but the type of this in a normal class is TheClass. Using anything else fails to Ycheck. I haven't found a way to produce those subtly different things using one uniform call, although I feel like there should be something in the internal API to do so.

As to why it must be TheModule.this.type and not TheModule$, I don't know, but apparently that's how it is in dotty.

allanrenucci · 2018-09-25T09:46:32Z

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

+                val vrefs = vrefss.head
+                val thisRef = vrefs.head
+                val origMeth = tree.symbol
+                val origVParams = tree.vparamss.flatten map (_.symbol)


Isn't this vparams0.map(_.symbol)? Is the assumption after erasure that there is only one parameter list?

Yes, you're right.

And yes, after erasure all methods have exactly one parameter list (not 0 nor 2+).

allanrenucci · 2018-09-25T12:55:25Z

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

@@ -195,84 +208,45 @@ class TailRec extends MiniPhase with FullParameterization {

    override def transform(tree: Tree)(implicit c: Context): Tree = {
      /* A possibly polymorphic apply to be considered for tail call transformation. */


Update comment? polymorphic after erasure?

allanrenucci · 2018-09-25T13:01:11Z

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

+                    .substThisUnlessStatic(classSym, thisRef.tpe)
+                    .subst(origVParams, vrefs.tail.map(_.tpe)),
+                  treeMap = {
+                    case tree: This if tree.symbol == classSym => thisRef


Why was this not needed before?

It was needed before, but it was handled by fullyParameterizedDef (along with many other complications for type parameters, which are not needed anymore).

allanrenucci · 2018-09-25T13:25:16Z

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

        } else {
-          val receiverIsSuper = (method.name eq sym) && enclosingClass.appliedRef.widen <:< recvWiden
+          val receiverIsSuper = (method.name eq sym) && enclosingClass.appliedRef.widen <:< prefix.tpe.widenDealias


Not your change but this is very suspicious: method.name is a Name and sym is a Symbol. I guess we don't have tests with recursive call to supertype

I agree. It is suspicious. I'd leave out an investigation to a different PR, though, if you think that's reasonable.

Sure! Do you mind opening an issue, once the PR is merged?

allanrenucci · 2018-09-25T13:45:21Z

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

-          case x: This => (x, x, accArgs, accT, x.symbol)
-          case x: Ident if x.symbol eq method => (EmptyTree, x, accArgs, accT, x.symbol)
-          case x => (x, x, accArgs, accT, x.symbol)
+      def rewriteApply(tree: Apply, sym: Symbol): Tree = {


rewriteApply does not need to take the symbol as parameter since the function is called only once and sym really is tree.fun.symbol

allanrenucci · 2018-09-25T13:49:17Z

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

-        val targs = typeArguments.map(noTailTransform)
-        val argumentss = arguments.map(noTailTransforms)
+        def continue =
+          Apply(noTailTransform(call), arguments)


If this used a tree copier and noTailTransforms used mapConserve, I believe we could save some unnecessary tree copies

allanrenucci · 2018-09-25T13:51:01Z

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

+              if (this.method.owner.isClass) receiver :: arguments
+              else arguments
+
+            Apply(method, argumentsWithReceiver)


Should we us a tree copier to properly set the tree position?

tpd.cpy(tree)(method, argumentsWithReceiver)

Otherwise, I (personally) think using our DSL is cleaner:

method.appliedToArgs(argumentsWithReceiver)

allanrenucci · 2018-09-25T13:59:17Z

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

+              if (prefix eq EmptyTree) This(enclosingClass.asClass)
+              else noTailTransform(prefix)
+
+            val method = Ident(label.termRef)


ref(label)?

This used not to be possible because it was too complicated for TailRec to understand trees produced by the pattern matcher. Now that patmat uses `Labeled` blocks instead of label-defs, it is trivial to do so: the argument of a `return` from a labeled block `lab` is in tail position if and only if the labeled block `lab` is itself in tail position. Running TailRec after erasure has two major benefits: * it is much simpler, as it does not have to deal with type parameters, `TypeApply`s, and a bunch of other stuff. * it supports polymorphic tail-recursive calls by construction (recursive calls whose receiver or method has different type parameters). It also has one difficulty: it cannot see the *call-site* `@tailrec` annotations anymore. This is why we add a mini-phase to record such annotations as tree attachments. Having TailRec after erasure will also be necessary to later make it use `Labeled` blocks and loops instead of label-defs itself. Indeed, in that case it will need to declare local `var`s for its term parameters, which would break path-dependent types within the method if it were done before erasure.

This feature does not exist in Scala 2, and was never documented in Scala 3. Its only internal use comes from a swiping commit that added as many `@tailrec` annotations as possible all over the codebase: see be5720c. Supporting that feature became problematic when TailRec was moved after erasure, because we needed an attachment to remember call-site `@tailrec` annotations, which would otherwise be removed by erasure. This commit removes the feature, on the grounds that its cost/benefit is too high.

sjrd

Updated according to your comments :)

sjrd · 2018-09-25T14:58:59Z

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

-    val name = TailLabelName.fresh()
+      val enclosingClass = method.enclosingClass.asClass
+      val thisParamType =
+        if (enclosingClass.is(Flags.Module)) enclosingClass.thisType


Because the type of this in a module class is something like TheModule.this.type (not TheModule$, for example) but the type of this in a normal class is TheClass. Using anything else fails to Ycheck. I haven't found a way to produce those subtly different things using one uniform call, although I feel like there should be something in the internal API to do so.

As to why it must be TheModule.this.type and not TheModule$, I don't know, but apparently that's how it is in dotty.

sjrd · 2018-09-25T15:01:03Z

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

+                val vrefs = vrefss.head
+                val thisRef = vrefs.head
+                val origMeth = tree.symbol
+                val origVParams = tree.vparamss.flatten map (_.symbol)


Yes, you're right.

And yes, after erasure all methods have exactly one parameter list (not 0 nor 2+).

sjrd · 2018-09-25T15:03:13Z

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

+                    .substThisUnlessStatic(classSym, thisRef.tpe)
+                    .subst(origVParams, vrefs.tail.map(_.tpe)),
+                  treeMap = {
+                    case tree: This if tree.symbol == classSym => thisRef


It was needed before, but it was handled by fullyParameterizedDef (along with many other complications for type parameters, which are not needed anymore).

sjrd · 2018-09-25T15:14:46Z

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

        } else {
-          val receiverIsSuper = (method.name eq sym) && enclosingClass.appliedRef.widen <:< recvWiden
+          val receiverIsSuper = (method.name eq sym) && enclosingClass.appliedRef.widen <:< prefix.tpe.widenDealias


I agree. It is suspicious. I'd leave out an investigation to a different PR, though, if you think that's reasonable.

sjrd · 2018-09-25T15:16:19Z

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

-          else tpd.cpy.Select(tree)(noTailTransform(tree.qualifier), tree.name)
-
-        case Apply(fun, args) =>
+        case tree@Apply(fun, args) if !fun.isInstanceOf[TypeApply] =>


Yes, for special methods like isInstanceOf. Those can never be tail-recursive calls since they don't have a definition in trees. It's better not to guard, though. They'll be ignored because isRecursiveCall will be false anyway.

allanrenucci

LGTM 🎉

allanrenucci · 2018-09-25T16:27:32Z

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

@@ -24,7 +24,7 @@ class MixinOps(cls: ClassSymbol, thisPhase: DenotTransformer)(implicit ctx: Cont
      name = member.name.stripScala2LocalSuffix,
      flags = member.flags &~ Deferred,
      info = cls.thisType.memberInfo(member)).enteredAfter(thisPhase).asTerm
-    res.addAnnotations(member.annotations)
+    res.addAnnotations(member.annotations.filter(_.symbol != defn.TailrecAnnot))


Sorry, I missed this. Is this needed if you move TailRec after Mixin within the same group?

Even more so. But right now it's necessary above all for extension methods (for AnyVals, which also use this forwarder() method). In general, when creating a forwarder of any sort, we must remove the @tailrec annotation.

Previously, no forwarder was ever created before TailRec ran. But now that TailRec runs after the creation of extension methods, we must ensure that forwarder drops @tailrec).

dottybot reviewed Sep 15, 2018

View reviewed changes

sjrd force-pushed the tailrec-after-erasure branch from 4eaa795 to 6d8198d Compare September 17, 2018 13:20

sjrd changed the title ~~WiP Move TailRec after Erasure.~~ Move TailRec after Erasure. Sep 17, 2018

sjrd mentioned this pull request Sep 17, 2018

Use Labeled blocks in TailRec, instead of label-defs. #5115

Merged

sjrd force-pushed the tailrec-after-erasure branch from 6d8198d to 5be1d73 Compare September 18, 2018 20:29

sjrd force-pushed the tailrec-after-erasure branch from bbe0a97 to b8be1e7 Compare September 22, 2018 08:38

sjrd mentioned this pull request Sep 22, 2018

Remove label-defs. #5141

Merged

allanrenucci self-requested a review September 24, 2018 14:01

allanrenucci self-assigned this Sep 24, 2018

allanrenucci suggested changes Sep 25, 2018

View reviewed changes

sjrd added 2 commits September 25, 2018 17:38

sjrd force-pushed the tailrec-after-erasure branch from 7b1e8d9 to b1008d8 Compare September 25, 2018 15:40

allanrenucci assigned sjrd and unassigned allanrenucci Sep 25, 2018

sjrd commented Sep 25, 2018

View reviewed changes

sjrd assigned allanrenucci and unassigned sjrd Sep 25, 2018

allanrenucci approved these changes Sep 25, 2018

View reviewed changes

allanrenucci merged commit a40039e into scala:master Sep 26, 2018

allanrenucci deleted the tailrec-after-erasure branch September 26, 2018 09:15

		@@ -195,84 +208,45 @@ class TailRec extends MiniPhase with FullParameterization {

		override def transform(tree: Tree)(implicit c: Context): Tree = {
		/* A possibly polymorphic apply to be considered for tail call transformation. */

Move TailRec after Erasure. #5112

Move TailRec after Erasure. #5112

Uh oh!

Conversation

sjrd commented Sep 15, 2018

Uh oh!

dottybot left a comment

Choose a reason for hiding this comment

Commit Messages

Uh oh!

sjrd commented Sep 15, 2018

Uh oh!

smarter commented Sep 15, 2018

Uh oh!

sjrd commented Sep 15, 2018

Uh oh!

sjrd commented Sep 18, 2018

Uh oh!

sjrd commented Sep 22, 2018

Uh oh!

smarter commented Sep 24, 2018

Uh oh!

dottybot commented Sep 24, 2018

Uh oh!

dottybot commented Sep 24, 2018

Uh oh!

allanrenucci left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sjrd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

allanrenucci left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment