SI-10071 Separate compilation for varargs methods #5556

dragos · 2016-11-24T12:06:25Z

Make sure that methods annotated with varargs are properly mixed-in. This commit
splits the transformation into an info transformer (that works on all symbols, whether
they come from source or binary) and a tree transformer.

The gist of this is that the symbol-creation part of the code was moved to the UnCurry
info transformer, while tree operations remained in the tree transformer. The newly
created symbol is attached to the original method so that the tree transformer can still
retrieve the symbol.

A few fall outs:

I removed a local map that was identical to TypeParamsVarargsAttachment
moved the said attachment to StdAttachments so it’s visible between reflect.internal
and nsc.transform
a couple more comments in UnCurry to honour the boy-scout rule

review by @lrytz

retronym · 2016-11-24T12:22:32Z

src/reflect/scala/reflect/internal/transform/UnCurry.scala

@@ -67,8 +74,25 @@ trait UnCurry {
      tp match {
        case ClassInfoType(parents, decls, clazz) =>
          val parents1 = parents mapConserve uncurry
-          if (parents1 eq parents) tp
-          else ClassInfoType(parents1, decls, clazz) // @MAT normalize in decls??
+          val varargOverloads = mutable.ListBuffer.empty[Symbol]


You could skip the new logic here if clazz.isJavaDefined.

In fact, I think we could skip the rest of the info transform in that case. Worth an experiment.

Good idea, I'll give it a go.

retronym · 2016-11-24T12:34:48Z

test/files/jvm/varargs-separate-bytecode/Props_1.flags

@@ -0,0 +1 @@
+-sourcepath test/files/jvm/varargs-separate-bytecode/


I haven't seen this done before in partest. What's the intent? Is there a simpler way?

You're right, it's being used in presentation compiler tests only. What I need is to make sure that AbstractProps is not compiled together with Props.scala. Is there a way to compile first one set of files and afterwards another set of files?

Ok, it looks like appending _1 and _2 to source files makes them compile in separate compiler runs, so my last commit simplifies the test to not use -sourcepath. I can squash the commits later, for the moment I'll keep them as separate, so reviewers can track what's new.

dragos · 2016-11-24T13:49:24Z

src/compiler/scala/tools/nsc/transform/UnCurry.scala

+     *
+     * @note The Java-style varargs method symbol is generated in the Uncurry info transformer. If the
+     *       symbol can't be found this method reports a warning and carries on.
+     * @see  [[scala.reflect.internal.transform.UnCurry]]
     */
    private def addJavaVarargsForwarders(dd: DefDef, flatdd: DefDef): DefDef = {


This method signature is confusing, but I chose to keep things simple and I didn't touch it.

flatdd is not required since almost all the logic is based on Symbols and the two trees always have the same symbol. Additionally, the only legitimate use of a tree is in the call to newTyper(dd,..), though I'm not sure that there's no other way. If you have any suggestions here I'm happy to at least remove the extra parameter.

lrytz · 2016-11-24T14:06:48Z

src/reflect/scala/reflect/internal/transform/UnCurry.scala

@@ -67,8 +74,29 @@ trait UnCurry {
      tp match {
        case ClassInfoType(parents, decls, clazz) =>
          val parents1 = parents mapConserve uncurry


move this declaration into the else branch below?

lrytz · 2016-11-24T14:10:48Z

src/reflect/scala/reflect/internal/transform/UnCurry.scala

+            // is anyway faster and safer
+            for (decl <- decls if decl.annotations.exists(_.symbol == VarargsClass)) {
+              if (mexists(decl.paramss)(sym => definitions.isRepeatedParamType(sym.tpe))) {
+                val forwarderSym = varargForwarderSym(clazz, decl, exitingPhase(phase)(decl.info))


nit, but this local isn't necessary IMO, the code would be clear without

dragos · 2016-11-24T17:35:04Z

Two things still pending:

do we do anything about the parameters of def addJavaVarargsForwarders? I'm fine to leave it as-is, just making sure you've seen my comment
squash, and how much?

Once this is in I will open a PR to backport the fix to 2.11.

lrytz · 2016-11-24T20:23:31Z

src/reflect/scala/reflect/internal/transform/UnCurry.scala

+          for (decl <- decls if decl.annotations.exists(_.symbol == VarargsClass)) {
+            if (mexists(decl.paramss)(sym => definitions.isRepeatedParamType(sym.tpe))) {
+              varargOverloads += varargForwarderSym(clazz, decl, exitingPhase(phase)(decl.info))
+            }


could use exitingUncurry to be more explicit

Unfortunately there is no such thing in scala.reflect.internal, only in the compiler cake.

lrytz · 2016-11-24T20:25:40Z

LGTM, thanks a lot! In terms of squashing I think one commit for the if (clazz.isJavaDefined) tp change (this one could go first), and a second with the rest.

retronym · 2016-11-24T22:05:20Z

src/reflect/scala/reflect/internal/transform/UnCurry.scala

@@ -65,18 +72,90 @@ trait UnCurry {
    def apply(tp0: Type): Type = {
      val tp = expandAlias(tp0)
      tp match {
-        case ClassInfoType(parents, decls, clazz) =>
+        case ClassInfoType(parents, decls, clazz) if !clazz.isJavaDefined =>


This oversight has come up twice in a week, I've opened a ticket to audit the other info transformers for similar opportunities to do less work!

retronym · 2016-11-24T22:33:12Z

src/reflect/scala/reflect/internal/transform/UnCurry.scala

+      val mt = MethodType(ps, resTp)
+      val r = if (tps.isEmpty) mt else PolyType(tps, mt)
+      r.substSym(oldTps, tps)
+    }


This is old code, but it is using very low level means to build up the new method symbol. I believe we can just clone the original symbol to get this for free.

https://github.com/retronym/scala/compare/1a21cc667a6f33a576900ea024c3bb21df84638e...retronym:review/5556?expand=11a21cc667a6f33a576900ea024c3bb21df84638e

Passes at least sbt partest --grep varargs.

@lrytz Do you remember any gotchas in this code that justify the current approach?

no, your patch LGTM. @dragos you want to include it in this PR, maybe as a separate commit?

Ok, I'll add it as another commit and let the bot tell us what's going on

Make sure that methods annotated with varargs are properly mixed-in. This commit splits the transformation into an info transformer (that works on all symbols, whether they come from source or binary) and a tree transformer. The gist of this is that the symbol-creation part of the code was moved to the UnCurry info transformer, while tree operations remained in the tree transformer. The newly created symbol is attached to the original method so that the tree transformer can still retrieve the symbol. A few fall outs: - I removed a local map that was identical to TypeParamsVarargsAttachment - moved the said attachment to StdAttachments so it’s visible between reflect.internal and nsc.transform - a couple more comments in UnCurry to honour the boy-scout rule

Cloning the original symbol in its entirety, rather than cloning its type/value parameters individually. `cloneSymbol` takes care of all the tricky substitutions for us!

dragos · 2016-11-25T16:41:57Z

If this is fine, I'd like to open the backport for 2.11.9. Can you please let me know if there's anything else here, or I can merge?

lrytz · 2016-11-25T17:53:11Z

🚢 it!

dragos · 2016-11-25T18:30:59Z

Wohoo! Thanks for the speedy review!

scala-jenkins added this to the 2.12.2 milestone Nov 24, 2016

retronym reviewed Nov 24, 2016

View reviewed changes

dragos force-pushed the ticket/10071 branch from 530d410 to 4a2a6d2 Compare November 24, 2016 13:39

dragos commented Nov 24, 2016

View reviewed changes

lrytz reviewed Nov 24, 2016

View reviewed changes

dragos force-pushed the ticket/10071 branch from 1176e29 to 1a21cc6 Compare November 24, 2016 20:16

lrytz reviewed Nov 24, 2016

View reviewed changes

scala-jenkins added the reviewed label Nov 24, 2016

retronym mentioned this pull request Nov 24, 2016

Review info transformers to see if we can/should shortcircuit for java defined symbols scala/scala-dev#276

Closed

retronym reviewed Nov 24, 2016

View reviewed changes

dragos and others added 3 commits November 25, 2016 10:50

Don’t run the uncurry info transformer on Java symbols.

6e719af

Simplify creation of varargs forwarder symbol

0fba882

Cloning the original symbol in its entirety, rather than cloning its type/value parameters individually. `cloneSymbol` takes care of all the tricky substitutions for us!

dragos force-pushed the ticket/10071 branch from 1a21cc6 to 0fba882 Compare November 25, 2016 10:15

dragos merged commit 690ba80 into scala:2.12.x Nov 25, 2016

dragos deleted the ticket/10071 branch November 25, 2016 18:31

SethTisue modified the milestones: 2.12.1, 2.12.2 Nov 25, 2016

dragos mentioned this pull request Nov 25, 2016

[backport] SI-10071 and SI-8786 #5557

Closed

adriaanm mentioned this pull request Jan 9, 2017

[backport] SI-10071 SI-8786 varargs methods #5630

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

SI-10071 Separate compilation for varargs methods #5556

SI-10071 Separate compilation for varargs methods #5556

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

		@@ -0,0 +1 @@
		-sourcepath test/files/jvm/varargs-separate-bytecode/

SI-10071 Separate compilation for varargs methods #5556

SI-10071 Separate compilation for varargs methods #5556

Uh oh!

Conversation

Uh oh!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

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!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!