From 502c7c9fd700ad449cf8383cf75153999ee90ba9 Mon Sep 17 00:00:00 2001 From: Guillaume Martres Date: Tue, 9 Apr 2019 18:21:50 +0200 Subject: [PATCH] Fix #6199: Use a skolemized prefix in asSeenFrom when needed Prior to https://github.com/lampepfl/dotty/commit/91ee02fcca4881b34e35d3e2721e1b00e98fd512, unstable prefixes appearing in selection types were handled in a two-pass approach which can be summarized as: 1. Type the selection with the unstable prefix, if this prefix appears in a position where it cannot be widened away, mark it with a special annotation. 2. If the result of the selection contains this annotation, retype it after having wrapped its prefix in a skolem. This got replaced by the use of an `ApproximatingTypeMap` which can always construct a valid (but potentially approximated) type for the selection. Most of the time this is all we need but there are some cases where skolemizing the prefix is still useful as witnessed by the tests added in this commit. They are now handled by unconditionally skolemizing unstable prefixes. To avoid any potential performance impact from this, we teach `asSeenFrom` to always widen these prefixes first and only make use of the skolem to avoid returning an approximation. Since this almost never occurs (it happens only once when compiling Dotty) this means we incur almost no extra cost (the skolem itself still needs to be allocated unconditionally in advance, this cannot be delegated to `asSeenFrom` because it's supposed to be an idempotent operation and each skolem is unique). --- .../src/dotty/tools/dotc/core/TypeOps.scala | 47 +++++++++++++++++-- .../src/dotty/tools/dotc/core/Types.scala | 26 ++++++++-- .../tools/dotc/core/tasty/TreeUnpickler.scala | 19 ++++---- .../dotty/tools/dotc/typer/TypeAssigner.scala | 15 +++++- tests/pos/i6199a.scala | 8 ++++ tests/pos/i6199b.scala | 11 +++++ tests/pos/i6199c.scala | 8 ++++ 7 files changed, 116 insertions(+), 18 deletions(-) create mode 100644 tests/pos/i6199a.scala create mode 100644 tests/pos/i6199b.scala create mode 100644 tests/pos/i6199c.scala diff --git a/compiler/src/dotty/tools/dotc/core/TypeOps.scala b/compiler/src/dotty/tools/dotc/core/TypeOps.scala index f41a710dcde5..42be245f1e91 100644 --- a/compiler/src/dotty/tools/dotc/core/TypeOps.scala +++ b/compiler/src/dotty/tools/dotc/core/TypeOps.scala @@ -5,6 +5,7 @@ package core import Contexts._, Types._, Symbols._, Names._, Flags._ import SymDenotations._ import util.Spans._ +import util.Stats import util.SourcePosition import NameKinds.DepParamName import Decorators._ @@ -26,11 +27,34 @@ trait TypeOps { this: Context => // TODO: Make standalone object. /** The type `tp` as seen from prefix `pre` and owner `cls`. See the spec * for what this means. */ - final def asSeenFrom(tp: Type, pre: Type, cls: Symbol): Type = + final def asSeenFrom(tp: Type, pre: Type, cls: Symbol): Type = { + pre match { + case pre: QualSkolemType => + // When a selection has an unstable qualifier, the qualifier type gets + // wrapped in a `QualSkolemType` so that it may appear soundly as the + // prefix of a path in the selection type. + // However, we'd like to avoid referring to skolems when possible since + // they're an extra level of indirection we usually don't need, so we + // compute the type as seen from the widened prefix, and in the rare + // cases where this leads to an approximated type we recompute it with + // the skolemized prefix. See the i6199* tests for usecases. + val widenedAsf = new AsSeenFromMap(pre.info, cls) + val ret = widenedAsf.apply(tp) + + if (!widenedAsf.approximated) + return ret + + Stats.record("asSeenFrom skolem prefix required") + case _ => + } + new AsSeenFromMap(pre, cls).apply(tp) + } /** The TypeMap handling the asSeenFrom */ class AsSeenFromMap(pre: Type, cls: Symbol) extends ApproximatingTypeMap { + /** Set to true when the result of `apply` was approximated to avoid an unstable prefix. */ + var approximated: Boolean = false def apply(tp: Type): Type = { @@ -44,7 +68,19 @@ trait TypeOps { this: Context => // TODO: Make standalone object. case pre: SuperType => toPrefix(pre.thistpe, cls, thiscls) case _ => if (thiscls.derivesFrom(cls) && pre.baseType(thiscls).exists) - if (variance <= 0 && !isLegalPrefix(pre)) range(defn.NothingType, pre) + if (variance <= 0 && !isLegalPrefix(pre)) { + if (variance < 0) { + approximated = true + defn.NothingType + } + else + // Don't set the `approximated` flag yet: if this is a prefix + // of a path, we might be able to dealias the path instead + // (this is handled in `ApproximatingTypeMap`). If dealiasing + // is not possible, then `expandBounds` will end up being + // called which we override to set the `approximated` flag. + range(defn.NothingType, pre) + } else pre else if ((pre.termSymbol is Package) && !(thiscls is Package)) toPrefix(pre.select(nme.PACKAGE), cls, thiscls) @@ -74,9 +110,14 @@ trait TypeOps { this: Context => // TODO: Make standalone object. override def reapply(tp: Type): Type = // derived infos have already been subjected to asSeenFrom, hence to need to apply the map again. tp + + override protected def expandBounds(tp: TypeBounds): Type = { + approximated = true + super.expandBounds(tp) + } } - private def isLegalPrefix(pre: Type)(implicit ctx: Context) = + def isLegalPrefix(pre: Type)(implicit ctx: Context): Boolean = pre.isStable || !ctx.phase.isTyper /** Implementation of Types#simplified */ diff --git a/compiler/src/dotty/tools/dotc/core/Types.scala b/compiler/src/dotty/tools/dotc/core/Types.scala index 49908bacdcd6..660d2cbbebd9 100644 --- a/compiler/src/dotty/tools/dotc/core/Types.scala +++ b/compiler/src/dotty/tools/dotc/core/Types.scala @@ -549,7 +549,7 @@ object Types { * flags and no `excluded` flag and produce a denotation that contains * the type of the member as seen from given prefix `pre`. */ - final def findMember(name: Name, pre: Type, required: FlagConjunction, excluded: FlagSet)(implicit ctx: Context): Denotation = { + final def findMember(name: Name, pre: Type, required: FlagConjunction = EmptyFlagConjunction, excluded: FlagSet = EmptyFlags)(implicit ctx: Context): Denotation = { @tailrec def go(tp: Type): Denotation = tp match { case tp: TermRef => go (tp.underlying match { @@ -3723,6 +3723,21 @@ object Types { override def toString: String = s"Skolem($hashCode)" } + /** A skolem type used to wrap the type of the qualifier of a selection. + * + * When typing a selection `e.f`, if `e` is unstable then we unconditionally + * skolemize it. We use a subclass of `SkolemType` for this so that + * [[TypeOps#asSeenFrom]] may treat it specially for optimization purposes, + * see its implementation for more details. + */ + class QualSkolemType(info: Type) extends SkolemType(info) { + override def derivedSkolemType(info: Type)(implicit ctx: Context): SkolemType = + if (info eq this.info) this else QualSkolemType(info) + } + object QualSkolemType { + def apply(info: Type): QualSkolemType = new QualSkolemType(info) + } + // ------------ Type variables ---------------------------------------- /** In a TypeApply tree, a TypeVar is created for each argument type to be inferred. @@ -4646,6 +4661,9 @@ object Types { case _ => tp } + protected def expandBounds(tp: TypeBounds): Type = + range(atVariance(-variance)(reapply(tp.lo)), reapply(tp.hi)) + /** Try to widen a named type to its info relative to given prefix `pre`, where possible. * The possible cases are listed inline in the code. */ @@ -4657,10 +4675,10 @@ object Types { // if H#T = U, then for any x in L..H, x.T =:= U, // hence we can replace with U under all variances reapply(alias.rewrapAnnots(tp1)) - case TypeBounds(lo, hi) => + case tp: TypeBounds => // If H#T = _ >: S <: U, then for any x in L..H, S <: x.T <: U, // hence we can replace with S..U under all variances - range(atVariance(-variance)(reapply(lo)), reapply(hi)) + expandBounds(tp) case info: SingletonType => // if H#x: y.type, then for any x in L..H, x.type =:= y.type, // hence we can replace with y.type under all variances @@ -4676,8 +4694,6 @@ object Types { * underlying bounds to a range, otherwise return the expansion. */ def expandParam(tp: NamedType, pre: Type): Type = { - def expandBounds(tp: TypeBounds) = - range(atVariance(-variance)(reapply(tp.lo)), reapply(tp.hi)) tp.argForParam(pre) match { case arg @ TypeRef(pre, _) if pre.isArgPrefixOf(arg.symbol) => arg.info match { diff --git a/compiler/src/dotty/tools/dotc/core/tasty/TreeUnpickler.scala b/compiler/src/dotty/tools/dotc/core/tasty/TreeUnpickler.scala index 7184b368af62..71a14e25b6d9 100644 --- a/compiler/src/dotty/tools/dotc/core/tasty/TreeUnpickler.scala +++ b/compiler/src/dotty/tools/dotc/core/tasty/TreeUnpickler.scala @@ -1016,14 +1016,14 @@ class TreeUnpickler(reader: TastyReader, val localCtx = if (name == nme.CONSTRUCTOR) ctx.addMode(Mode.InSuperCall) else ctx val qual = readTerm()(localCtx) - var pre = qual.tpe.widenIfUnstable - val denot = accessibleDenot(pre, name, sig) + var qualType = qual.tpe.widenIfUnstable + val denot = accessibleDenot(qualType, name, sig) val owner = denot.symbol.maybeOwner - if (owner.isPackageObject && pre.termSymbol.is(Package)) - pre = pre.select(owner.sourceModule) + if (owner.isPackageObject && qualType.termSymbol.is(Package)) + qualType = qualType.select(owner.sourceModule) val tpe = name match { - case name: TypeName => TypeRef(pre, name, denot) - case name: TermName => TermRef(pre, name, denot) + case name: TypeName => TypeRef(qualType, name, denot) + case name: TermName => TermRef(qualType, name, denot) } ConstFold(untpd.Select(qual, name).withType(tpe)) } @@ -1033,10 +1033,11 @@ class TreeUnpickler(reader: TastyReader, (untpd.Ident(qual.name).withSpan(qual.span), qual.tpe.asInstanceOf[TypeRef]) } - def accessibleDenot(pre: Type, name: Name, sig: Signature) = { - val d = pre.member(name).atSignature(sig) + def accessibleDenot(qualType: Type, name: Name, sig: Signature) = { + val pre = ctx.typeAssigner.maybeSkolemizePrefix(qualType, name) + val d = qualType.findMember(name, pre).atSignature(sig) if (!d.symbol.exists || d.symbol.isAccessibleFrom(pre)) d - else pre.nonPrivateMember(name).atSignature(sig) + else qualType.findMember(name, pre, excluded = Private).atSignature(sig) } def readSimpleTerm(): Tree = tag match { diff --git a/compiler/src/dotty/tools/dotc/typer/TypeAssigner.scala b/compiler/src/dotty/tools/dotc/typer/TypeAssigner.scala index 9886fdf04017..5e80c4b2be72 100644 --- a/compiler/src/dotty/tools/dotc/typer/TypeAssigner.scala +++ b/compiler/src/dotty/tools/dotc/typer/TypeAssigner.scala @@ -226,6 +226,17 @@ trait TypeAssigner { test(tpe, true) } + /** Return a potentially skolemized version of `qualTpe` to be used + * as a prefix when selecting `name`. + * + * @see QualSkolemType, TypeOps#asSeenFrom + */ + def maybeSkolemizePrefix(qualType: Type, name: Name)(implicit ctx: Context): Type = + if (name.isTermName && !ctx.isLegalPrefix(qualType)) + QualSkolemType(qualType) + else + qualType + /** The type of the selection `tree`, where `qual1` is the typed qualifier part. */ def selectionType(tree: untpd.RefTree, qual1: Tree)(implicit ctx: Context): Type = { var qualType = qual1.tpe.widenIfUnstable @@ -234,8 +245,10 @@ trait TypeAssigner { qualType = errorType(em"$qualType takes type parameters", qual1.sourcePos) else if (!qualType.isInstanceOf[TermType]) qualType = errorType(em"$qualType is illegal as a selection prefix", qual1.sourcePos) + val name = tree.name - val mbr = qualType.member(name) + val pre = maybeSkolemizePrefix(qualType, name) + val mbr = qualType.findMember(name, pre) if (reallyExists(mbr)) qualType.select(name, mbr) else if (qualType.derivesFrom(defn.DynamicClass) && name.isTermName && !Dynamic.isDynamicMethod(name)) diff --git a/tests/pos/i6199a.scala b/tests/pos/i6199a.scala new file mode 100644 index 000000000000..5bd31912a8c9 --- /dev/null +++ b/tests/pos/i6199a.scala @@ -0,0 +1,8 @@ +class Encoder[T] { def encode(v: T): String = v.toString } +case class ValueWithEncoder[T](value: T, encoder: Encoder[T]) + +object Test { + val a: Seq[ValueWithEncoder[_]] = Seq.empty + val b = a.map((value, encoder) => encoder.encode(value)) + val c: Seq[String] = b +} diff --git a/tests/pos/i6199b.scala b/tests/pos/i6199b.scala new file mode 100644 index 000000000000..3a9dcc7f235e --- /dev/null +++ b/tests/pos/i6199b.scala @@ -0,0 +1,11 @@ +trait Foo { + type State + def bar(f: Bar[State] => Bar[State]): Foo = this +} +object Foo { + def unit: Foo = new Foo { + type State = Any + } + def doBar: Foo = unit.bar(bar => bar) +} +class Bar[+A] diff --git a/tests/pos/i6199c.scala b/tests/pos/i6199c.scala new file mode 100644 index 000000000000..85bcb0f04e7e --- /dev/null +++ b/tests/pos/i6199c.scala @@ -0,0 +1,8 @@ +trait Foo[State] { + def bar(f: Bar[State] => Bar[State]): Foo[_] = this +} +object Foo { + def unit: Foo[_] = new Foo[Any] {} + def doBar: Foo[_] = unit.bar(bar => bar) +} +class Bar[+A]