Skip to content

Revert to the Scala 2 encoding for trait initialization and super calls #8652

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 24 commits into from
Jul 22, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
90070e2
First pass of reverting to the trait encoding of Scala 2.
sjrd Apr 1, 2020
d495ee0
Move the trait super call encoding logic to the back-end.
sjrd Apr 3, 2020
11de1d3
Do not store to non-existent fields in trait setters.
sjrd Apr 6, 2020
de5df36
Fix trait setter to getter lookup.
sjrd Apr 7, 2020
ce4f7ba
Use a fast path when looking up trait setter getters.
sjrd Apr 7, 2020
3bd5da4
Mark dropped private vars without rhs as indeed dropped.
sjrd Jun 23, 2020
e12f1f9
Fix trait encoding for `private var`s in traits.
sjrd Jun 24, 2020
d682455
Adapt one check file for new emitted static methods.
sjrd Jun 24, 2020
d7bea54
Modules in traits always get an outer pointer.
sjrd Jun 24, 2020
84b230a
Make module classes in traits non-private in Mixin.
sjrd Jun 24, 2020
288fa1d
Adapt the expected result of the jUnitForwarders test.
sjrd Jun 24, 2020
2eb743c
Restore AugmentScala2Traits, but limit it to super accessors.
sjrd Jun 24, 2020
3f99915
Reset the `inline` flag for accessors in traits at Mixin.
sjrd Jun 24, 2020
d990e64
Cleanup, including removing dead code.
sjrd Jun 24, 2020
29a561b
Adapt the expect result in ByteCodeTests.
sjrd Jun 25, 2020
cb84d04
Break cycles in transformSym.
sjrd Jun 25, 2020
80db021
Break more cycles in transformSym.
sjrd Jun 25, 2020
a771574
Ignore bridges when looking for the getter of a trait setter.
sjrd Jun 29, 2020
1128a98
Fix handling of `final val`s after rebase on top of #9261.
sjrd Jul 16, 2020
44de114
Support param accessors that override concrete trait vals.
sjrd Jul 16, 2020
e88c5b7
Avoid round-trip through sourceModule in scalacLinkedClass.
sjrd Jul 17, 2020
64d3b9a
Improve comments for the new trait encoding.
sjrd Jul 19, 2020
a754f24
Do a bit less work in Mixin.transformSym.
sjrd Jul 19, 2020
77e2925
Integrate the remaining job of AugmentScala2Traits into Scala2Unpickler.
sjrd Jul 20, 2020
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
14 changes: 11 additions & 3 deletions compiler/src/dotty/tools/backend/jvm/BCodeBodyBuilder.scala
Original file line number Diff line number Diff line change
Expand Up @@ -780,6 +780,7 @@ trait BCodeBodyBuilder extends BCodeSkelBuilder {
val invokeStyle =
if (sym.isStaticMember) InvokeStyle.Static
else if (sym.is(Private) || sym.isClassConstructor) InvokeStyle.Special
else if (app.hasAttachment(BCodeHelpers.UseInvokeSpecial)) InvokeStyle.Special
else InvokeStyle.Virtual

if (invokeStyle.hasInstance) genLoadQualifier(fun)
Expand Down Expand Up @@ -1165,9 +1166,16 @@ trait BCodeBodyBuilder extends BCodeSkelBuilder {
val isInterface = isEmittedInterface(receiverClass)
import InvokeStyle._
if (style == Super) {
// DOTTY: this differ from how super-calls in traits are handled in the scalac backend,
// this is intentional but could change in the future, see https://github.com/lampepfl/dotty/issues/5928
bc.invokespecial(receiverName, jname, mdescr, isInterface)
if (isInterface && !method.is(JavaDefined)) {
val args = new Array[BType](bmType.argumentTypes.length + 1)
val ownerBType = toTypeKind(method.owner.info)
bmType.argumentTypes.copyToArray(args, 1)
val staticDesc = MethodBType(ownerBType :: bmType.argumentTypes, bmType.returnType).descriptor
val staticName = traitSuperAccessorName(method)
bc.invokestatic(receiverName, staticName, staticDesc, isInterface)
} else {
bc.invokespecial(receiverName, jname, mdescr, isInterface)
}
} else {
val opc = style match {
case Static => Opcodes.INVOKESTATIC
Expand Down
13 changes: 13 additions & 0 deletions compiler/src/dotty/tools/backend/jvm/BCodeHelpers.scala
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,12 @@ trait BCodeHelpers extends BCodeIdiomatic with BytecodeWriters {
}
}

final def traitSuperAccessorName(sym: Symbol): String = {
val nameString = sym.javaSimpleName.toString
if (sym.name == nme.TRAIT_CONSTRUCTOR) nameString
else nameString + "$"
}

// -----------------------------------------------------------------------------------------
// finding the least upper bound in agreement with the bytecode verifier (given two internal names handed by ASM)
// Background:
Expand Down Expand Up @@ -950,4 +956,11 @@ object BCodeHelpers {
val Super = new InvokeStyle(3) // InvokeSpecial (super calls)
}

/** An attachment on Apply nodes indicating that it should be compiled with
* `invokespecial` instead of `invokevirtual`. This is used for static
* forwarders.
Copy link
Member

Choose a reason for hiding this comment

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

I see there was some discussion on this in the PR, but it'd be great if this comment was expanded to explain exactly why this is needed.

Copy link
Member Author

@sjrd sjrd Jul 19, 2020

Choose a reason for hiding this comment

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

I have added a comment on BCodeSkilBuilder.makeStaticForwarder with a summary of the discussion from this PR.

* See BCodeSkelBuilder.makeStaticForwarder for more details.
*/
val UseInvokeSpecial = new dotc.util.Property.Key[Unit]

}
53 changes: 50 additions & 3 deletions compiler/src/dotty/tools/backend/jvm/BCodeSkelBuilder.scala
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,9 @@ import dotty.tools.dotc.CompilationUnit
import dotty.tools.dotc.core.Annotations.Annotation
import dotty.tools.dotc.core.Decorators._
import dotty.tools.dotc.core.Flags._
import dotty.tools.dotc.core.StdNames.str
import dotty.tools.dotc.core.StdNames.{nme, str}
import dotty.tools.dotc.core.Symbols._
import dotty.tools.dotc.core.Types.Type
import dotty.tools.dotc.core.Types.{MethodType, Type}
import dotty.tools.dotc.util.Spans._
import dotty.tools.dotc.report

Expand Down Expand Up @@ -496,7 +496,23 @@ trait BCodeSkelBuilder extends BCodeHelpers {

case ValDef(name, tpt, rhs) => () // fields are added in `genPlainClass()`, via `addClassFields()`

case dd: DefDef => genDefDef(dd)
case dd: DefDef =>
/* First generate a static forwarder if this is a non-private trait
* trait method. This is required for super calls to this method, which
* go through the static forwarder in order to work around limitations
* of the JVM.
* In theory, this would go in a separate MiniPhase, but it would have to
* sit in a MegaPhase of its own between GenSJSIR and GenBCode, so the cost
* is not worth it. We directly do it in this back-end instead, which also
* kind of makes sense because it is JVM-specific.
*/
val sym = dd.symbol
val needsStaticImplMethod =
claszSymbol.isInterface && !dd.rhs.isEmpty && !sym.isPrivate && !sym.isStaticMember
if needsStaticImplMethod then
genStaticForwarderForDefDef(dd)
Copy link
Member

Choose a reason for hiding this comment

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

Could this be done in a mini-phase before the backend?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, because it must not be done for JS (nor Native). So it would have to be alone in its own MegaPhase between GenSJSIR and GenBCode. That would be a huge cost for a few lines of code. I have explained that in a comment.


genDefDef(dd)

case tree: Template =>
val body =
Expand Down Expand Up @@ -537,6 +553,37 @@ trait BCodeSkelBuilder extends BCodeHelpers {

} // end of method initJMethod

private def genStaticForwarderForDefDef(dd: DefDef): Unit =
val forwarderDef = makeStaticForwarder(dd)
genDefDef(forwarderDef)

/* Generates a synthetic static forwarder for a trait method.
* For a method such as
* def foo(...args: Ts): R
* in trait X, we generate the following method:
* static def foo$($this: X, ...args: Ts): R =
* invokespecial $this.X::foo(...args)
* We force an invokespecial with the attachment UseInvokeSpecial. It is
* necessary to make sure that the call will not follow overrides of foo()
* in subtraits and subclasses, since the whole point of this forward is to
* encode super calls.
*/
private def makeStaticForwarder(dd: DefDef): DefDef =
val origSym = dd.symbol.asTerm
val name = traitSuperAccessorName(origSym)
val info = origSym.info match
case mt: MethodType =>
MethodType(nme.SELF :: mt.paramNames, origSym.owner.typeRef :: mt.paramInfos, mt.resType)
val sym = origSym.copy(
name = name.toTermName,
flags = Method | JavaStatic,
info = info
)
tpd.DefDef(sym.asTerm, { paramss =>
val params = paramss.head
tpd.Apply(params.head.select(origSym), params.tail)
.withAttachment(BCodeHelpers.UseInvokeSpecial, ())
})

def genDefDef(dd: DefDef): Unit = {
val rhs = dd.rhs
Expand Down
4 changes: 1 addition & 3 deletions compiler/src/dotty/tools/dotc/Compiler.scala
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,6 @@ class Compiler {
new LiftTry, // Put try expressions that might execute on non-empty stacks into their own methods
new CollectNullableFields, // Collect fields that can be nulled out after use in lazy initialization
new ElimOuterSelect, // Expand outer selections
new AugmentScala2Traits, // Augments Scala2 traits with additional members needed for mixin composition.
new ResolveSuper, // Implement super accessors
new FunctionXXLForwarders, // Add forwarders for FunctionXXL apply method
new ParamForwarding, // Add forwarders for aliases of superclass parameters
Expand All @@ -110,8 +109,7 @@ class Compiler {
// Note: constructors changes decls in transformTemplate, no InfoTransformers should be added after it
new FunctionalInterfaces, // Rewrites closures to implement @specialized types of Functions.
new Instrumentation) :: // Count closure allocations under -Yinstrument-closures
List(new LinkScala2Impls, // Redirect calls to trait methods defined by Scala 2.x, so that they now go to
new LambdaLift, // Lifts out nested functions to class scope, storing free variables in environments
List(new LambdaLift, // Lifts out nested functions to class scope, storing free variables in environments
// Note: in this mini-phase block scopes are incorrect. No phases that rely on scopes should be here
new ElimStaticThis, // Replace `this` references to static objects by global identifiers
new CountOuterAccesses) :: // Identify outer accessors that can be dropped
Expand Down
9 changes: 2 additions & 7 deletions compiler/src/dotty/tools/dotc/core/Flags.scala
Original file line number Diff line number Diff line change
Expand Up @@ -378,13 +378,8 @@ object Flags {
/** Children were queried on this class */
val (_, _, ChildrenQueried @ _) = newFlags(56, "<children-queried>")

/** A module variable (Scala 2.x only)
* /
* A Scala 2.x trait that has been partially augmented.
* This is set in `AugmentScala2Trait` and reset in `LinkScala2Impls`
* when the trait is fully augmented.
*/
val (_, Scala2ModuleVar @ _, Scala2xPartiallyAugmented @ _) = newFlags(57, "<modulevar>", "<scala-2.x-partially-augmented>")
/** A module variable (Scala 2.x only) */
val (_, Scala2ModuleVar @ _, _) = newFlags(57, "<modulevar>")

/** A macro */
val (Macro @ _, _, _) = newFlags(58, "<macro>")
Expand Down
2 changes: 1 addition & 1 deletion compiler/src/dotty/tools/dotc/core/SymDenotations.scala
Original file line number Diff line number Diff line change
Expand Up @@ -1153,7 +1153,7 @@ object SymDenotations {

final def scalacLinkedClass(using Context): Symbol =
if (this.is(ModuleClass)) companionNamed(effectiveName.toTypeName)
else if (this.isClass) companionNamed(effectiveName.moduleClassName).sourceModule.moduleClass
else if (this.isClass) companionNamed(effectiveName.moduleClassName)
else NoSymbol

/** Find companion class symbol with given name, or NoSymbol if none exists.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -454,8 +454,12 @@ class Scala2Unpickler(bytes: Array[Byte], classRoot: ClassDenotation, moduleClas
flags = flags &~ Scala2ExpandedName
}
if (flags.is(Scala2SuperAccessor)) {
name = name.asTermName.unmangle(SuperAccessorName)
flags = flags &~ Scala2SuperAccessor
/* Scala 2 super accessors are pickled as private, but are compiled as public expanded.
* Dotty super accessors, however, are already pickled as public expanded.
* We bridge the gap right now.
*/
name = name.asTermName.unmangle(SuperAccessorName).expandedName(owner)
flags = flags &~ (Scala2SuperAccessor | Private)
}
name = name.mapLast(_.decode)

Expand Down Expand Up @@ -1310,4 +1314,3 @@ class Scala2Unpickler(bytes: Array[Byte], classRoot: ClassDenotation, moduleClas
errorBadSignature("expected an TypeDef (" + other + ")")
}
}

69 changes: 0 additions & 69 deletions compiler/src/dotty/tools/dotc/transform/AugmentScala2Traits.scala

This file was deleted.

29 changes: 28 additions & 1 deletion compiler/src/dotty/tools/dotc/transform/Constructors.scala
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import dotty.tools.dotc.core.StdNames._
import ast._
import Trees._
import Flags._
import NameOps._
import SymUtils._
import Symbols._
import Decorators._
Expand Down Expand Up @@ -204,7 +205,33 @@ class Constructors extends MiniPhase with IdentityDenotTransformer { thisPhase =
initFlags = sym.flags &~ Private,
owner = constr.symbol).installAfter(thisPhase)
constrStats += intoConstr(stat, sym)
}
} else
dropped += sym
case stat @ DefDef(name, _, _, tpt, _)
if stat.symbol.isGetter && stat.symbol.owner.is(Trait) && !stat.symbol.is(Lazy) =>
val sym = stat.symbol
assert(isRetained(sym), sym)
if (!stat.rhs.isEmpty && !isWildcardArg(stat.rhs))
/* !!! Work around #9390
* This should really just be `sym.setter`. However, if we do that, we'll miss
* setters for mixed in `private var`s. Even though the scope clearly contains the
* setter symbol with the correct Name structure (since the `find` finds it),
* `.decl(setterName)` used by `.setter` through `.accessorNamed` will *not* find it.
* Could it be that the hash table of the `Scope` is corrupted?
* We still try `sym.setter` first as an optimization, since it will work for all
* public vars in traits and all (public or private) vars in classes.
*/
val symSetter =
if sym.setter.exists then
sym.setter
else
val setterName = sym.asTerm.name.setterName
sym.owner.info.decls.find(d => d.is(Accessor) && d.name == setterName)
val setter =
if (symSetter.exists) symSetter
else sym.accessorNamed(Mixin.traitSetterName(sym.asTerm))
constrStats += Apply(ref(setter), intoConstr(stat.rhs, sym).withSpan(stat.span) :: Nil)
clsStats += cpy.DefDef(stat)(rhs = EmptyTree)
case DefDef(nme.CONSTRUCTOR, _, ((outerParam @ ValDef(nme.OUTER, _, _)) :: _) :: Nil, _, _) =>
clsStats += mapOuter(outerParam.symbol).transform(stat)
case _: DefTree =>
Expand Down
6 changes: 3 additions & 3 deletions compiler/src/dotty/tools/dotc/transform/ExplicitOuter.scala
Original file line number Diff line number Diff line change
Expand Up @@ -215,9 +215,9 @@ object ExplicitOuter {

/** Class is always instantiated in the compilation unit where it is defined */
private def hasLocalInstantiation(cls: ClassSymbol)(using Context): Boolean =
// scala2x modules always take an outer pointer(as of 2.11)
// dotty modules are always locally instantiated
cls.owner.isTerm || cls.is(Private) || cls.is(Module, butNot = Scala2x)
// Modules are normally locally instantiated, except if they are declared in a trait,
// in which case they will be instantiated in the classes that mix in the trait.
cls.owner.isTerm || cls.is(Private, butNot = Module) || (cls.is(Module) && !cls.owner.is(Trait))

/** The outer parameter accessor of cass `cls` */
private def outerParamAccessor(cls: ClassSymbol)(using Context): TermSymbol =
Expand Down
Loading