SI-10071 Separate compilation for varargs methods #5556
Next
Next commit
SI-10071 Separate compilation for varargs methods
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
- Loading branch information
8000
commit 7602f2ebc0fbd0e1b51aa8d9d9a9e71607a06dd6
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1278,5 +1278,4 @@ abstract class Erasure extends InfoTransform | |
| } | ||
|
|
||
| private class TypeRefAttachment(val tpe: TypeRef) | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,13 +4,20 @@ package internal | |
| package transform | ||
|
|
||
| import Flags._ | ||
| import scala.collection.mutable | ||
|
|
||
| trait UnCurry { | ||
|
|
||
| val global: SymbolTable | ||
| import global._ | ||
| import definitions._ | ||
|
|
||
| /** | ||
| * The synthetic Java vararg method symbol corresponding to a Scala vararg method | ||
| * annotated with @varargs. | ||
| */ | ||
| case class VarargsSymbolAttachment(varargMethod: Symbol) | ||
|
|
||
| /** Note: changing tp.normalize to tp.dealias in this method leads to a single | ||
| * test failure: run/t5688.scala, where instead of the expected output | ||
| * Vector(ta, tb, tab) | ||
|
|
@@ -67,8 +74,25 @@ trait UnCurry { | |
| tp match { | ||
| case ClassInfoType(parents, decls, clazz) => | ||
| val parents1 = parents mapConserve uncurry | ||
|
There was a problem hiding this comment. move this declaration into the |
||
| val varargOverloads = mutable.ListBuffer.empty[Symbol] | ||
|
There was a problem hiding this comment. You could skip the new logic here if There was a problem hiding this comment. In fact, I think we could skip the rest of the info transform in that case. Worth an experiment. There was a problem hiding this comment. Good idea, I'll give it a go. |
||
|
|
||
| // Not using `hasAnnotation` here because of dreaded cyclic reference errors: | ||
| // it may happen that VarargsClass has not been initialized yet and we get here | ||
| // while processing one of its superclasses (such as java.lang.Object). Since we | ||
| // don't need the more precise `matches` semantics, we only check the symbol, which | ||
| // 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)) | ||
| varargOverloads += forwarderSym | ||
| } | ||
|
There was a problem hiding this comment. could use There was a problem hiding this comment. Unfortunately there is no such thing in |
||
| 10000 | } | |
| if ((parents1 eq parents) && varargOverloads.isEmpty) tp | ||
| else { | ||
| val newDecls = decls.cloneScope | ||
| varargOverloads.foreach(newDecls.enter) | ||
| ClassInfoType(parents1, newDecls, clazz) | ||
| } // @MAT normalize in decls?? | ||
| case PolyType(_, _) => | ||
| mapOver(tp) | ||
| case _ => | ||
|
|
@@ -77,6 +101,60 @@ trait UnCurry { | |
| } | ||
| } | ||
|
|
||
| private def varargForwarderSym(currentClass: Symbol, origSym: Symbol, newInfo: Type): Symbol = { | ||
| val forwSym = currentClass.newMethod(origSym.name.toTermName, origSym.pos, VARARGS | SYNTHETIC | origSym.flags & ~DEFERRED) | ||
|
|
||
| // we are using `origSym.info`, which contains the type *before* the transformation | ||
| // so we still see repeated parameter types (uncurry replaces them with Seq) | ||
| val isRepeated = origSym.info.paramss.flatten.map(sym => definitions.isRepeatedParamType(sym.tpe)) | ||
| val oldPs = newInfo.paramss.head | ||
|
|
||
| val forwTpe = { | ||
| val (oldTps, tps) = newInfo match { | ||
| case PolyType(oldTps, _) => | ||
| val newTps = oldTps.map(_.cloneSymbol(forwSym)) | ||
| (oldTps, newTps) | ||
|
|
||
| case _ => (Nil, Nil) | ||
| } | ||
|
|
||
| def toArrayType(tp: Type, newParam: Symbol): Type = { | ||
| val arg = elementType(SeqClass, tp) | ||
| val elem = if (arg.typeSymbol.isTypeParameterOrSkolem && !(arg <:< AnyRefTpe)) { | ||
| // To prevent generation of an `Object` parameter from `Array[T]` parameter later | ||
| // as this would crash the Java compiler which expects an `Object[]` array for varargs | ||
| // e.g. def foo[T](a: Int, b: T*) | ||
| // becomes def foo[T](a: Int, b: Array[Object]) | ||
| // instead of def foo[T](a: Int, b: Array[T]) ===> def foo[T](a: Int, b: Object) | ||
| // | ||
| // In order for the forwarder method to type check we need to insert a cast: | ||
| // def foo'[T'](a: Int, b: Array[Object]) = foo[T'](a, wrapRefArray(b).asInstanceOf[Seq[T']]) | ||
| // The target element type for that cast (T') is stored in the TypeParamVarargsAttachment | ||
| val originalArg = arg.substSym(oldTps, tps) | ||
| // Store the type parameter that was replaced by Object to emit the correct generic signature | ||
| newParam.updateAttachment(new TypeParamVarargsAttachment(originalArg)) | ||
| ObjectTpe | ||
| } else | ||
| arg | ||
| arrayType(elem) | ||
| } | ||
|
|
||
| val ps = map2(oldPs, isRepeated)((oldParam, isRep) => { | ||
| val newParam = oldParam.cloneSymbol(forwSym) | ||
| val tp = if (isRep) toArrayType(oldParam.tpe, newParam) else oldParam.tpe | ||
| newParam.setInfo(tp) | ||
| }) | ||
|
|
||
| val resTp = newInfo.finalResultType.substSym(oldPs, ps) | ||
| val mt = MethodType(ps, resTp) | ||
| val r = if (tps.isEmpty) mt else PolyType(tps, mt) | ||
| r.substSym(oldTps, tps) | ||
| } | ||
|
There was a problem hiding this comment. 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. Passes at least @lrytz Do you remember any gotchas in this code that justify the current approach? There was a problem hiding this comment. no, your patch LGTM. @dragos you want to include it in this PR, maybe as a separate commit? There was a problem hiding this comment. Ok, I'll add it as another commit and let the bot tell us what's going on |
||
|
|
||
| origSym.updateAttachment(VarargsSymbolAttachment(forwSym)) | ||
| forwSym.setInfo(forwTpe) | ||
| } | ||
|
|
||
| /** - return symbol's transformed type, | ||
| * - if symbol is a def parameter with transformed type T, return () => T | ||
| * | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| Found vararg overload for method create |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| package foo | ||
|
|
||
| import scala.annotation.varargs | ||
|
|
||
| t F438 rait AbstractProps { | ||
| @varargs | ||
| def create(x: String, y: Int*): AbstractProps = null | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| import foo.AbstractProps | ||
|
|
||
| class Props extends AbstractProps |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| import scala.collection.JavaConverters._ | ||
| import scala.tools.asm | ||
| import scala.tools.asm.Opcodes | ||
| import scala.tools.partest.BytecodeTest | ||
|
|
||
| object Test extends BytecodeTest { | ||
| def show: Unit = { | ||
| val classNode = loadClassNode("Props") | ||
| val methods = classNode.methods.iterator().asScala.filter( m => m.name == "create") | ||
|
|
||
| for (m <- methods if (m.access & Opcodes.ACC_VARARGS) > 0) { | ||
| println(s"Found vararg overload for method ${m.name}") | ||
| } | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method signature is confusing, but I chose to keep things simple and I didn't touch it.
flatddis 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 tonewTyper(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.