Skip to content

Fix #3675: don't unnecessarily wrap arrays when eliminating java varargs #3869

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

Merged
merged 5 commits into from
Jan 23, 2018
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions compiler/src/dotty/tools/dotc/core/Definitions.scala
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import scala.collection.{ mutable, immutable }
import PartialFunction._
import collection.mutable
import util.common.alwaysZero
import dotty.tools.dotc.transform.TreeGen

object Definitions {

Expand Down Expand Up @@ -338,6 +339,12 @@ class Definitions {
def Predef_classOf(implicit ctx: Context) = Predef_classOfR.symbol
lazy val Predef_undefinedR = ScalaPredefModule.requiredMethodRef("???")
def Predef_undefined(implicit ctx: Context) = Predef_undefinedR.symbol
// The set of all wrap{X, Ref}Array methods, where X is a value type
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@smarter pre-calculated the set of methods here, as suggested
PTAL

lazy val Predef_wrapArrayR: collection.Set[TermRef] = {
val methodNames = ScalaValueTypes.map(TreeGen.wrapArrayMethodName) + nme.wrapRefArray
methodNames.map(ScalaPredefModule.requiredMethodRef)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would inline this in Predef_wrapArray since it is only used there:

val Predef_wrapArray = new PerRun[collection.Set[Symbol]] { implicit ctx =>
  val methodNames = ScalaValueTypes.map(TreeGen.wrapArrayMethodName) + nme.wrapRefArray
  methodNames.map(name => ScalaPredefModule.requiredMethodRef(name).toSymbol)
}

val Predef_wrapArray = new PerRun[collection.Set[Symbol]](implicit ctx => Predef_wrapArrayR.map(_.symbol))

lazy val ScalaRuntimeModuleRef = ctx.requiredModuleRef("scala.runtime.ScalaRunTime")
def ScalaRuntimeModule(implicit ctx: Context) = ScalaRuntimeModuleRef.symbol
Expand Down
2 changes: 2 additions & 0 deletions compiler/src/dotty/tools/dotc/transform/ElimRepeated.scala
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,8 @@ class ElimRepeated extends MiniPhase with InfoTransformer { thisPhase =>
private def seqToArray(tree: Tree, pt: Type)(implicit ctx: Context): Tree = tree match {
case SeqLiteral(elems, elemtpt) =>
JavaSeqLiteral(elems, elemtpt)
case app@Apply(fun, args) if defn.Predef_wrapArray().contains(fun.symbol) => // rewrite a call to `wrapXArray(arr)` to `arr`
args.head
case _ =>
val elemType = tree.tpe.elemType
var elemClass = elemType.classSymbol
Expand Down
28 changes: 28 additions & 0 deletions compiler/test/dotty/tools/backend/jvm/DottyBytecodeTests.scala
Original file line number Diff line number Diff line change
Expand Up @@ -187,4 +187,32 @@ class TestBCode extends DottyBytecodeTest {
diffInstructions(instructions1, instructions2))
}
}

/** Verifies that arrays are not unnecessarily wrapped when passed to Java varargs methods */
@Test def dontWrapArraysInJavaVarargs = {
val source =
"""
|import java.nio.file._
|class Test {
| def test(xs: Array[String]) = {
| val p4 = Paths.get("Hello", xs: _*)
| }
|}
""".stripMargin

checkBCode(source) { dir =>
val moduleIn = dir.lookupName("Test.class", directory = false)
val moduleNode = loadClassNode(moduleIn.input)
val method = getMethod(moduleNode, "test")

val arrayWrapped = instructionsFromMethod(method).exists { instr: Instruction =>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can do:

val arrayWrapped = instructionsFromMethod(method).exists {
  case inv: Invoke => inv.name.contains("wrapRefArray")
  case _ => false
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

instr match {
case inv: Invoke => inv.name.contains("wrapRefArray")
case _ => false
}
}

assert(!arrayWrapped, "Arrays should not be wrapped when passed to a Java varargs method\n")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I verified that this fails if I comment out the changes to ElimRepeated.

}
}
}