From 42913c90fa414d03ea62488a1d23b4c6b4b2c5f5 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Sat, 9 Jun 2018 16:15:53 +0200 Subject: [PATCH] Fix #4590: Reintroduce old inline accessor scheme as a fallback Turns out things are not so simple. Some inline accessors cannot be handled in the same way as protected accessors because we cannot always place an accessor method next to the accessed symbol. We solve this by using an adapted version of the old inline accessor scheme as a fallback. --- .../src/dotty/tools/dotc/core/NameKinds.scala | 1 + .../tools/dotc/transform/AccessProxies.scala | 49 ++++-- .../src/dotty/tools/dotc/typer/Inliner.scala | 150 ++++++++++++++++-- tests/pos/i4590.scala | 19 +++ tests/run/inline.check | 2 + tests/run/inline/Test_2.scala | 6 +- tests/run/inline/inlines_1.scala | 18 +++ 7 files changed, 214 insertions(+), 31 deletions(-) create mode 100644 tests/pos/i4590.scala diff --git a/compiler/src/dotty/tools/dotc/core/NameKinds.scala b/compiler/src/dotty/tools/dotc/core/NameKinds.scala index 6474a1e084f2..a9f841c33697 100644 --- a/compiler/src/dotty/tools/dotc/core/NameKinds.scala +++ b/compiler/src/dotty/tools/dotc/core/NameKinds.scala @@ -291,6 +291,7 @@ object NameKinds { val LiftedTreeName = new UniqueNameKind("liftedTree") val SuperArgName = new UniqueNameKind("$superArg$") val DocArtifactName = new UniqueNameKind("$doc") + val UniqueInlineName = new UniqueNameKind("$i") /** A kind of unique extension methods; Unlike other unique names, these can be * unmangled. diff --git a/compiler/src/dotty/tools/dotc/transform/AccessProxies.scala b/compiler/src/dotty/tools/dotc/transform/AccessProxies.scala index 6824df459bde..0de32b5bb6d1 100644 --- a/compiler/src/dotty/tools/dotc/transform/AccessProxies.scala +++ b/compiler/src/dotty/tools/dotc/transform/AccessProxies.scala @@ -27,6 +27,11 @@ abstract class AccessProxies { /** accessor -> accessed */ private val accessedBy = newMutableSymbolMap[Symbol] + /** Given the name of an accessor, is the receiver of the call to accessed obtained + * as a parameterer? + */ + protected def passReceiverAsArg(accessorName: Name)(implicit ctx: Context) = false + /** The accessor definitions that need to be added to class `cls` * As a side-effect, this method removes entries from the `accessedBy` map. * So a second call of the same method will yield the empty list. @@ -34,13 +39,21 @@ abstract class AccessProxies { private def accessorDefs(cls: Symbol)(implicit ctx: Context): Iterator[DefDef] = for (accessor <- cls.info.decls.iterator; accessed <- accessedBy.remove(accessor)) yield polyDefDef(accessor.asTerm, tps => argss => { - val accessRef = ref(TermRef(cls.thisType, accessed)) + def numTypeParams = accessed.info match { + case info: PolyType => info.paramNames.length + case _ => 0 + } + val (accessRef, forwardedTypes, forwardedArgss) = + if (passReceiverAsArg(accessor.name)) + (argss.head.head.select(accessed), tps.takeRight(numTypeParams), argss.tail) + else + (ref(TermRef(cls.thisType, accessed)), tps, argss) val rhs = if (accessor.name.isSetterName && - argss.nonEmpty && argss.head.nonEmpty) // defensive conditions - accessRef.becomes(argss.head.head) + forwardedArgss.nonEmpty && forwardedArgss.head.nonEmpty) // defensive conditions + accessRef.becomes(forwardedArgss.head.head) else - accessRef.appliedToTypes(tps).appliedToArgss(argss) + accessRef.appliedToTypes(forwardedTypes).appliedToArgss(forwardedArgss) rhs.withPos(accessed.pos) }) @@ -65,7 +78,7 @@ abstract class AccessProxies { } /** An accessor symbol, create a fresh one unless one exists already */ - private def accessorSymbol(owner: Symbol, accessorName: TermName, accessorInfo: Type, accessed: Symbol)(implicit ctx: Context) = { + protected def accessorSymbol(owner: Symbol, accessorName: TermName, accessorInfo: Type, accessed: Symbol)(implicit ctx: Context) = { def refersToAccessed(sym: Symbol) = accessedBy.get(sym).contains(accessed) owner.info.decl(accessorName).suchThat(refersToAccessed).symbol.orElse { val acc = newAccessorSymbol(owner, accessorName, accessorInfo, accessed.pos) @@ -83,14 +96,24 @@ abstract class AccessProxies { }.withPos(reference.pos) /** Given a reference to a getter accessor, the corresponding setter reference */ - def useSetter(getterRef: RefTree)(implicit ctx: Context): Tree = { - val getter = getterRef.symbol - val accessed = accessedBy(getter) - val accessedName = accessed.name.asTermName - val setterName = accessorNameKind(accessedName.setterName) - val setterInfo = MethodType(getter.info.widenExpr :: Nil, defn.UnitType) - val setter = accessorSymbol(getter.owner, setterName, setterInfo, accessed) - rewire(getterRef, setter) + def useSetter(getterRef: Tree)(implicit ctx: Context): Tree = getterRef match { + case getterRef: RefTree => + val getter = getterRef.symbol.asTerm + val accessed = accessedBy(getter) + val setterName = getter.name.setterName + def toSetterInfo(getterInfo: Type): Type = getterInfo match { + case getterInfo: LambdaType => + getterInfo.derivedLambdaType(resType = toSetterInfo(getterInfo.resType)) + case _ => + MethodType(getterInfo :: Nil, defn.UnitType) + } + val setterInfo = toSetterInfo(getter.info.widenExpr) + val setter = accessorSymbol(getter.owner, setterName, setterInfo, accessed) + rewire(getterRef, setter) + case Apply(fn, args) => + cpy.Apply(getterRef)(useSetter(fn), args) + case TypeApply(fn, args) => + cpy.TypeApply(getterRef)(useSetter(fn), args) } /** Create an accessor unless one exists already, and replace the original diff --git a/compiler/src/dotty/tools/dotc/typer/Inliner.scala b/compiler/src/dotty/tools/dotc/typer/Inliner.scala index 59b8c6a32f6e..52464bedc7d7 100644 --- a/compiler/src/dotty/tools/dotc/typer/Inliner.scala +++ b/compiler/src/dotty/tools/dotc/typer/Inliner.scala @@ -15,7 +15,7 @@ import StdNames.nme import Contexts.Context import Names.{Name, TermName, EmptyTermName} import NameOps._ -import NameKinds.{ClassifiedNameKind, InlineAccessorName} +import NameKinds.{ClassifiedNameKind, InlineAccessorName, UniqueInlineName} import ProtoTypes.selectionProto import SymDenotations.SymDenotation import Annotations._ @@ -33,10 +33,18 @@ object Inliner { class InlineAccessors extends AccessProxies { - /** A tree map which inserts accessors for all non-public term members accessed - * from inlined code. Accessors are collected in the `accessors` buffer. - */ - class MakeInlineable(inlineSym: Symbol) extends TreeMap with Insert { + /** If an inline accessor name wraps a unique inline name, this is taken as indication + * that the inline accessor takes its receiver as first parameter. Such accessors + * are created by MakeInlineablePassing. + */ + override def passReceiverAsArg(name: Name)(implicit ctx: Context) = name match { + case InlineAccessorName(UniqueInlineName(_, _)) => true + case _ => false + } + + /** A tree map which inserts accessors for non-public term members accessed from inlined code. + */ + abstract class MakeInlineableMap(val inlineSym: Symbol) extends TreeMap with Insert { def accessorNameKind = InlineAccessorName /** A definition needs an accessor if it is private, protected, or qualified private @@ -48,18 +56,126 @@ object Inliner { (sym.is(AccessFlags) || sym.privateWithin.exists) && !sym.isContainedIn(inlineSym) + def preTransform(tree: Tree)(implicit ctx: Context): Tree + + def postTransform(tree: Tree)(implicit ctx: Context) = tree match { + case Assign(lhs, rhs) if lhs.symbol.name.is(InlineAccessorName) => + cpy.Apply(tree)(useSetter(lhs), rhs :: Nil) + case _ => + tree + } - // TODO: Also handle references to non-public types. - // This is quite tricky, as such types can appear anywhere, including as parts - // of types of other things. For the moment we do nothing and complain - // at the implicit expansion site if there's a reference to an inaccessible type. override def transform(tree: Tree)(implicit ctx: Context): Tree = - super.transform(accessorIfNeeded(tree)) match { - case tree1 @ Assign(lhs: RefTree, rhs) if lhs.symbol.name.is(InlineAccessorName) => - cpy.Apply(tree1)(useSetter(lhs), rhs :: Nil) - case tree1 => - tree1 - } + postTransform(super.transform(preTransform(tree))) + } + + /** Direct approach: place the accessor with the accessed symbol. This has the + * advantage that we can re-use the receiver as is. But it is only + * possible if the receiver is essentially this or an outer this, which is indicated + * by the test that we can find a host for the accessor. + */ + class MakeInlineableDirect(inlineSym: Symbol) extends MakeInlineableMap(inlineSym) { + def preTransform(tree: Tree)(implicit ctx: Context): Tree = tree match { + case tree: RefTree if needsAccessor(tree.symbol) => + if (tree.symbol.isConstructor) { + ctx.error("Implementation restriction: cannot use private constructors in inline methods", tree.pos) + tree // TODO: create a proper accessor for the private constructor + } + else if (AccessProxies.hostForAccessorOf(tree.symbol).exists) useAccessor(tree) + else tree + case _ => + tree + } + } + + /** Fallback approach if the direct approach does not work: Place the accessor method + * in the same class as the inlined method, and let it take the receiver as parameter. + * This is tricky, since we have to find a suitable type for the parameter, which might + * require additional type parameters for the inline accessor. An example is in the + * `TestPassing` class in test `run/inline/inlines_1`: + * + * class C[T](x: T) { + * private[inlines] def next[U](y: U): (T, U) = (x, y) + * } + * class TestPassing { + * inline def foo[A](x: A): (A, Int) = { + * val c = new C[A](x) + * c.next(1) + * } + * inline def bar[A](x: A): (A, String) = { + * val c = new C[A](x) + * c.next("") + * } + * + * `C` could be compiled separately, so we cannot place the inline accessor in it. + * Instead, the inline accessor goes into `TestPassing` and takes the actual receiver + * type as argument: + * + * def inline$next$i1[A, U](x$0: C[A])(y: U): (A, U) = + * x$0.next[U](y) + * + * Since different calls might have different receiver types, we need to generate one + * such accessor per call, so they need to have unique names. + */ + class MakeInlineablePassing(inlineSym: Symbol) extends MakeInlineableMap(inlineSym) { + + def preTransform(tree: Tree)(implicit ctx: Context): Tree = tree match { + case _: Apply | _: TypeApply | _: RefTree + if needsAccessor(tree.symbol) && tree.isTerm && !tree.symbol.isConstructor => + val (refPart, targs, argss) = decomposeCall(tree) + val qual = qualifier(refPart) + inlining.println(i"adding receiver passing inline accessor for $tree -> (${qual.tpe}, $refPart: ${refPart.getClass}, [$targs%, %], ($argss%, %))") + + // Need to dealias in order to cagtch all possible references to abstracted over types in + // substitutions + val dealiasMap = new TypeMap { + def apply(t: Type) = mapOver(t.dealias) + } + val qualType = dealiasMap(qual.tpe.widen) + + // The types that are local to the inlined method, and that therefore have + // to be abstracted out in the accessor, which is external to the inlined method + val localRefs = qualType.namedPartsWith(ref => + ref.isType && ref.symbol.isContainedIn(inlineSym)).toList + + // Add qualifier type as leading method argument to argument `tp` + def addQualType(tp: Type): Type = tp match { + case tp: PolyType => tp.derivedLambdaType(tp.paramNames, tp.paramInfos, addQualType(tp.resultType)) + case tp: ExprType => addQualType(tp.resultType) + case tp => MethodType(qualType :: Nil, tp) + } + + // Abstract accessed type over local refs + def abstractQualType(mtpe: Type): Type = + if (localRefs.isEmpty) mtpe + else PolyType.fromParams(localRefs.map(_.symbol.asType), mtpe) + .asInstanceOf[PolyType].flatten + + val accessed = refPart.symbol.asTerm + val accessedType = refPart.tpe.widen + val accessor = accessorSymbol( + owner = inlineSym.owner, + accessorName = InlineAccessorName(UniqueInlineName.fresh(accessed.name)), + accessorInfo = abstractQualType(addQualType(dealiasMap(accessedType))), + accessed = accessed) + + ref(accessor) + .appliedToTypeTrees(localRefs.map(TypeTree(_)) ++ targs) + .appliedToArgss((qual :: Nil) :: argss) + .withPos(tree.pos) + + // TODO: Handle references to non-public types. + // This is quite tricky, as such types can appear anywhere, including as parts + // of types of other things. For the moment we do nothing and complain + // at the implicit expansion site if there's a reference to an inaccessible type. + // Draft code (incomplete): + // + // val accessor = accessorSymbol(tree, TypeAlias(tree.tpe)).asType + // myAccessors += TypeDef(accessor).withPos(tree.pos.focus) + // ref(accessor).withPos(tree.pos) + // + case _ => tree + } } /** Adds accessors for all non-public term members accessed @@ -76,7 +192,9 @@ object Inliner { // Inline methods in local scopes can only be called in the scope they are defined, // so no accessors are needed for them. tree - else new MakeInlineable(inlineSym).transform(tree) + else + new MakeInlineablePassing(inlineSym).transform( + new MakeInlineableDirect(inlineSym).transform(tree)) } } diff --git a/tests/pos/i4590.scala b/tests/pos/i4590.scala new file mode 100644 index 000000000000..5ca26e197f2d --- /dev/null +++ b/tests/pos/i4590.scala @@ -0,0 +1,19 @@ +package test + +class ArrayDeque { + inline def isResizeNecessary(len: Int) = len > ArrayDeque.StableSize +} + +object ArrayDeque { + private val StableSize = 256 +} + +class List { + inline def foo(x: List.Cons): Unit = { + x.next = this + } +} +object List { + case class Cons(head: Int, private[List] var next: List) +} + diff --git a/tests/run/inline.check b/tests/run/inline.check index 5f711274b935..779315ff2880 100644 --- a/tests/run/inline.check +++ b/tests/run/inline.check @@ -7,3 +7,5 @@ Outer.f Inner Inner Outer.f Outer.f Inner +(hi,1) +(true,) diff --git a/tests/run/inline/Test_2.scala b/tests/run/inline/Test_2.scala index 605868c80164..431951300eb6 100644 --- a/tests/run/inline/Test_2.scala +++ b/tests/run/inline/Test_2.scala @@ -10,12 +10,14 @@ object Test { val o = new Outer val i = new o.Inner + val p = new TestPassing println(i.m) println(i.g) println(i.h) println(o.inner.m) println(o.inner.g) println(o.inner.h) + println(p.foo("hi")) + println(p.bar(true)) } - -} +} \ No newline at end of file diff --git a/tests/run/inline/inlines_1.scala b/tests/run/inline/inlines_1.scala index 24f1c78fe237..825d84913013 100644 --- a/tests/run/inline/inlines_1.scala +++ b/tests/run/inline/inlines_1.scala @@ -38,4 +38,22 @@ object inlines { } val inner = new Inner } + + class C[T](private[inlines] val x: T) { + private[inlines] def next[U](y: U): (T, U) = (xx, y) + private[inlines] var xx: T = _ + } + + class TestPassing { + inline def foo[A](x: A): (A, Int) = { + val c = new C[A](x) + c.xx = c.x + c.next(1) + } + inline def bar[A](x: A): (A, String) = { + val c = new C[A](x) + c.xx = c.x + c.next("") + } + } }