Skip to content
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

Drop "no inlines with opaques" implementation restriction #12815

Merged
merged 9 commits into from
Jun 17, 2021
Merged
Show file tree
Hide file tree
Changes from all 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
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ trait ConstraintHandling {
val bound = dropWildcards(rawBound)
val oldBounds @ TypeBounds(lo, hi) = constraint.nonParamBounds(param)
val equalBounds = (if isUpper then lo else hi) eq bound
if equalBounds && !bound.existsPart(_ eq param, stopAtStatic = true) then
if equalBounds && !bound.existsPart(_ eq param, StopAt.Static) then
// The narrowed bounds are equal and not recursive,
// so we can remove `param` from the constraint.
constraint = constraint.replace(param, bound)
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 @@ -2546,7 +2546,7 @@ object SymDenotations {
}

private[SymDenotations] def stillValidInOwner(denot: SymDenotation)(using Context): Boolean = try
val owner = denot.owner.denot
val owner = denot.maybeOwner.denot
stillValid(owner)
&& (
!owner.isClass
Expand Down
2 changes: 1 addition & 1 deletion compiler/src/dotty/tools/dotc/core/TypeComparer.scala
Original file line number Diff line number Diff line change
Expand Up @@ -1391,7 +1391,7 @@ class TypeComparer(@constructorOnly initctx: Context) extends ConstraintHandling
*/
def canCompare(ts: Set[Type]) =
ctx.phase.isTyper
|| !ts.exists(_.existsPart(_.isInstanceOf[SkolemType], stopAtStatic = true))
|| !ts.exists(_.existsPart(_.isInstanceOf[SkolemType], StopAt.Static))

def verified(result: Boolean): Boolean =
if Config.checkAtomsComparisons then
Expand Down
30 changes: 20 additions & 10 deletions compiler/src/dotty/tools/dotc/core/Types.scala
Original file line number Diff line number Diff line change
Expand Up @@ -439,23 +439,23 @@ object Types {

/** Does this type contain wildcard types? */
final def containsWildcardTypes(using Context) =
existsPart(_.isInstanceOf[WildcardType], stopAtStatic = true, forceLazy = false)
existsPart(_.isInstanceOf[WildcardType], StopAt.Static, forceLazy = false)

// ----- Higher-order combinators -----------------------------------

/** Returns true if there is a part of this type that satisfies predicate `p`.
*/
final def existsPart(p: Type => Boolean, stopAtStatic: Boolean = false, forceLazy: Boolean = true)(using Context): Boolean =
new ExistsAccumulator(p, stopAtStatic, forceLazy).apply(false, this)
final def existsPart(p: Type => Boolean, stopAt: StopAt = StopAt.None, forceLazy: Boolean = true)(using Context): Boolean =
new ExistsAccumulator(p, stopAt, forceLazy).apply(false, this)

/** Returns true if all parts of this type satisfy predicate `p`.
*/
final def forallParts(p: Type => Boolean)(using Context): Boolean =
!existsPart(!p(_))

/** Performs operation on all parts of this type */
final def foreachPart(p: Type => Unit, stopAtStatic: Boolean = false)(using Context): Unit =
new ForeachAccumulator(p, stopAtStatic).apply((), this)
final def foreachPart(p: Type => Unit, stopAt: StopAt = StopAt.None)(using Context): Unit =
new ForeachAccumulator(p, stopAt).apply((), this)

/** The parts of this type which are type or term refs and which
* satisfy predicate `p`.
Expand Down Expand Up @@ -5199,6 +5199,12 @@ object Types {

// ----- TypeMaps --------------------------------------------------------------------

/** Where a traversal should stop */
enum StopAt:
case None // traverse everything
case Package // stop at package references
case Static // stop at static references

/** Common base class of TypeMap and TypeAccumulator */
abstract class VariantTraversal:
protected[core] var variance: Int = 1
Expand All @@ -5211,7 +5217,7 @@ object Types {
res
}

protected def stopAtStatic: Boolean = true
protected def stopAt: StopAt = StopAt.Static

/** Can the prefix of this static reference be omitted if the reference
* itself can be omitted? Overridden in TypeOps#avoid.
Expand All @@ -5220,7 +5226,11 @@ object Types {

protected def stopBecauseStaticOrLocal(tp: NamedType)(using Context): Boolean =
(tp.prefix eq NoPrefix)
|| stopAtStatic && tp.currentSymbol.isStatic && isStaticPrefix(tp.prefix)
|| {
val stop = stopAt
stop == StopAt.Static && tp.currentSymbol.isStatic && isStaticPrefix(tp.prefix)
|| stop == StopAt.Package && tp.currentSymbol.is(Package)
}
end VariantTraversal

abstract class TypeMap(implicit protected var mapCtx: Context)
Expand Down Expand Up @@ -5409,7 +5419,7 @@ object Types {
derivedClassInfo(tp, this(tp.prefix))

def andThen(f: Type => Type): TypeMap = new TypeMap {
override def stopAtStatic = thisMap.stopAtStatic
override def stopAt = thisMap.stopAt
def apply(tp: Type) = f(thisMap(tp))
}
}
Expand Down Expand Up @@ -5831,12 +5841,12 @@ object Types {

class ExistsAccumulator(
p: Type => Boolean,
override val stopAtStatic: Boolean,
override val stopAt: StopAt,
forceLazy: Boolean)(using Context) extends TypeAccumulator[Boolean]:
def apply(x: Boolean, tp: Type): Boolean =
x || p(tp) || (forceLazy || !tp.isInstanceOf[LazyRef]) && foldOver(x, tp)

class ForeachAccumulator(p: Type => Unit, override val stopAtStatic: Boolean)(using Context) extends TypeAccumulator[Unit] {
class ForeachAccumulator(p: Type => Unit, override val stopAt: StopAt)(using Context) extends TypeAccumulator[Unit] {
def apply(x: Unit, tp: Type): Unit = foldOver(p(tp), tp)
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -700,7 +700,7 @@ class Scala2Unpickler(bytes: Array[Byte], classRoot: ClassDenotation, moduleClas
}
// Cannot use standard `existsPart` method because it calls `lookupRefined`
// which can cause CyclicReference errors.
val isBoundAccumulator = new ExistsAccumulator(isBound, stopAtStatic = true, forceLazy = true):
val isBoundAccumulator = new ExistsAccumulator(isBound, StopAt.Static, forceLazy = true):
override def foldOver(x: Boolean, tp: Type): Boolean = tp match
case tp: TypeRef => applyToPrefix(x, tp)
case _ => super.foldOver(x, tp)
Expand Down
5 changes: 3 additions & 2 deletions compiler/src/dotty/tools/dotc/printing/RefinedPrinter.scala
Original file line number Diff line number Diff line change
Expand Up @@ -78,9 +78,10 @@ class RefinedPrinter(_ctx: Context) extends PlainPrinter(_ctx) {
}

override def nameString(name: Name): String =
if ctx.settings.YdebugNames.value then name.debugString
def strippedName = if printDebug then name else name.stripModuleClassSuffix
if ctx.settings.YdebugNames.value then strippedName.debugString
else if name.isTypeName && name.is(WildcardParamName) && !printDebug then "_"
else super.nameString(name)
else super.nameString(strippedName)

override protected def simpleNameString(sym: Symbol): String =
nameString(if (ctx.property(XprintMode).isEmpty) sym.initial.name else sym.name)
Expand Down
2 changes: 1 addition & 1 deletion compiler/src/dotty/tools/dotc/typer/Checking.scala
Original file line number Diff line number Diff line change
Expand Up @@ -1088,7 +1088,7 @@ trait Checking {
}
case _ =>
}
tp.foreachPart(check, stopAtStatic = true)
tp.foreachPart(check, StopAt.Static)
if (ok) tp else UnspecifiedErrorType
}

Expand Down
2 changes: 1 addition & 1 deletion compiler/src/dotty/tools/dotc/typer/Implicits.scala
Original file line number Diff line number Diff line change
Expand Up @@ -732,7 +732,7 @@ trait ImplicitRunInfo:
case null =>
record(i"implicitScope")
val liftToAnchors = new TypeMap:
override def stopAtStatic = true
override def stopAt = StopAt.Static
private val seen = util.HashSet[Type]()

def applyToUnderlying(t: TypeProxy) =
Expand Down
109 changes: 100 additions & 9 deletions compiler/src/dotty/tools/dotc/typer/Inliner.scala
Original file line number Diff line number Diff line change
Expand Up @@ -556,18 +556,93 @@ class Inliner(call: tpd.Tree, rhsToInline: tpd.Tree)(using Context) {
ref(lastSelf).outerSelect(lastLevel - level, selfSym.info)
else
inlineCallPrefix
val binding = ValDef(selfSym.asTerm, QuoteUtils.changeOwnerOfTree(rhs, selfSym)).withSpan(selfSym.span)
val binding = accountForOpaques(
ValDef(selfSym.asTerm, QuoteUtils.changeOwnerOfTree(rhs, selfSym)).withSpan(selfSym.span))
bindingsBuf += binding
inlining.println(i"proxy at $level: $selfSym = ${bindingsBuf.last}")
lastSelf = selfSym
lastLevel = level
}
}

/** A list of pairs between TermRefs appearing in thisProxy bindings that
* refer to objects with opaque type aliases and local proxy symbols
* that contain refined versions of these TermRefs where the aliases
* are exposed.
*/
private val opaqueProxies = new mutable.ListBuffer[(TermRef, TermRef)]

/** Map first halfs of opaqueProxies pairs to second halfs, using =:= as equality */
def mapRef(ref: TermRef): Option[TermRef] =
opaqueProxies.collectFirst {
case (from, to) if from.symbol == ref.symbol && from =:= ref => to
}

/** If `binding` contains TermRefs that refer to objects with opaque
* type aliases, add proxy definitions that expose these aliases
* and substitute such TermRefs with theproxies. Example from pos/opaque-inline1.scala:
*
* object refined:
* opaque type Positive = Int
* inline def Positive(value: Int): Positive = f(value)
* def f(x: Positive): Positive = x
* def run: Unit = { val x = 9; val nine = refined.Positive(x) }
*
* This generates the following proxies:
*
* val $proxy1: refined.type{type Positive = Int} =
* refined.$asInstanceOf$[refined.type{type Positive = Int}]
* val refined$_this: ($proxy1 : refined.type{Positive = Int}) =
* $proxy1
*
* and every reference to `refined` in the inlined expression is replaced by
* `refined_$this`.
*/
def accountForOpaques(binding: ValDef)(using Context): ValDef =
binding.symbol.info.foreachPart {
case ref: TermRef =>
for cls <- ref.widen.classSymbols do
if cls.containsOpaques && mapRef(ref).isEmpty then
def openOpaqueAliases(selfType: Type): List[(Name, Type)] = selfType match
case RefinedType(parent, rname, TypeAlias(alias)) =>
val opaq = cls.info.member(rname).symbol
if opaq.isOpaqueAlias then
(rname, alias.stripLazyRef.asSeenFrom(ref, cls))
Copy link
Member

Choose a reason for hiding this comment

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

Minor point: we could check the accessibility of alias at this point to generate a slightly better error message if it isn't accessible, but the current one isn't too bad:

object refined:
  private class Internal(x: Int)
  opaque type Positive = Internal
  inline def Positive(value: Int): Positive = new Internal(value)

object test:
  def run: Unit =
    val x = 9
    val nine = refined.Positive(x)
-- Error: tests/pos/opaque-inline1.scala:10:31 ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
10 |    val nine = refined.Positive(x)
   |               ^^^^^^^^^^^^^^^^^^^
   |               class Internal cannot be accessed as a member of (refined : refined.type{Positive = refined.Internal}) from module class test$.
   | This location contains code that was inlined from opaque-inline1.scala:4

Copy link
Contributor Author

@odersky odersky Jun 16, 2021

Choose a reason for hiding this comment

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

There was a wrong condition for eliding the proxies, which is fixed in 2414033. Now pos/opaque-inline1 does not compile anymore if I remove the cast. It fails in -Ycheck with

java.lang.AssertionError: assertion failed: Found:    Int
Required: refined.Positive
found: class Int in package scala with class Int, flags = final abstract <scala-2.x> <touched>, underlying = Int, AnyVal {...}
expected: type Positive in object refined with type Positive, flags = <deferred> opaque <touched>, underlying = refined.Positive, , Any, {...}
tree = {
  val $proxy1: refined.type{Positive = Int} = 
    refined.$asInstanceOf[refined.type{Positive = Int}]
  val refined$_this: ($proxy1 : refined.type{Positive = Int}) = $proxy1
  refined$_this.f(x):refined$_this.Positive
} while compiling opaque-inline1.scala

The weird thing is that with the original condition everything succeeded, even though the expansion was then

  val x = 9
  val nine = refined.f(x): refined.Positive

This is clearly type-incorrect, since 9 is an Int and refined.f expects a refined.Positive. Indeed, if I write this expansion manually, it fails.

-- [E007] Type Mismatch Error: opaque-inline1.scala:12:25 ----------------------
12 |    val nine = refined.f(x): refined.Positive
   |                         ^
   |                         Found:    (x : Int)
   |                         Required: refined.Positive

But somehow -Ycheck does not test this. I traced Ycheck's behavior and it seems to think that refined.f: Int => Int even though it should be refined.f: Positive => Positive. The reason for this is that it copies the tree refined.f as is from the inline method, which is in a scope where Positive = Int. Hence, Int => Int is a legal type for f there. It's the copying that is wrong, and unfortunately Ycheck does not catch it.

But the case won't arise anymore with this PR.

:: openOpaqueAliases(parent)
else Nil
case _ =>
Nil
val refinements = openOpaqueAliases(cls.givenSelfType)
val refinedType = refinements.foldLeft(ref: Type) ((parent, refinement) =>
RefinedType(parent, refinement._1, TypeAlias(refinement._2))
)
val refiningSym = newSym(InlineBinderName.fresh(), Synthetic, refinedType).asTerm
val refiningDef = ValDef(refiningSym, tpd.ref(ref).cast(refinedType)).withSpan(binding.span)
inlining.println(i"add opaque alias proxy $refiningDef")
bindingsBuf += refiningDef
opaqueProxies += ((ref, refiningSym.termRef))
case _ =>
}
if opaqueProxies.isEmpty then binding
else
val mapType = new TypeMap:
override def stopAt = StopAt.Package
def apply(t: Type) = mapOver {
t match
case ref: TermRef => mapRef(ref).getOrElse(ref)
case _ => t
}
binding.symbol.info = mapType(binding.symbol.info)
val mapTree = TreeTypeMap(typeMap = mapType)
mapTree.transform(binding).asInstanceOf[ValDef]
.showing(i"transformed this binding exposing opaque aliases: $result", inlining)
end accountForOpaques

private def canElideThis(tpe: ThisType): Boolean =
inlineCallPrefix.tpe == tpe && ctx.owner.isContainedIn(tpe.cls) ||
tpe.cls.isContainedIn(inlinedMethod) ||
tpe.cls.is(Package)
inlineCallPrefix.tpe == tpe && ctx.owner.isContainedIn(tpe.cls)
|| tpe.cls.isContainedIn(inlinedMethod)
|| tpe.cls.is(Package)
|| tpe.cls.isStaticOwner && !(tpe.cls.seesOpaques && inlinedMethod.isContainedIn(tpe.cls))

/** Very similar to TreeInfo.isPureExpr, but with the following inliner-only exceptions:
* - synthetic case class apply methods, when the case class constructor is empty, are
Expand Down Expand Up @@ -666,12 +741,16 @@ class Inliner(call: tpd.Tree, rhsToInline: tpd.Tree)(using Context) {
case _ =>
}

private val registerTypes = new TypeTraverser:
override def stopAt = StopAt.Package
override def traverse(t: Type) =
registerType(t)
traverseChildren(t)

/** Register type of leaf node */
private def registerLeaf(tree: Tree): Unit = tree match {
case _: This | _: Ident | _: TypeTree =>
tree.typeOpt.foreachPart(registerType, stopAtStatic = true)
private def registerLeaf(tree: Tree): Unit = tree match
case _: This | _: Ident | _: TypeTree => registerTypes.traverse(tree.typeOpt)
case _ =>
}

/** Make `tree` part of inlined expansion. This means its owner has to be changed
* from its `originalOwner`, and, if it comes from outside the inlined method
Expand Down Expand Up @@ -797,6 +876,8 @@ class Inliner(call: tpd.Tree, rhsToInline: tpd.Tree)(using Context) {
val inliner = new InlinerMap(
typeMap =
new DeepTypeMap {
override def stopAt =
if opaqueProxies.isEmpty then StopAt.Static else StopAt.Package
def apply(t: Type) = t match {
case t: ThisType => thisProxy.getOrElse(t.cls, t)
case t: TypeRef => paramProxy.getOrElse(t, mapOver(t))
Expand Down Expand Up @@ -915,7 +996,17 @@ class Inliner(call: tpd.Tree, rhsToInline: tpd.Tree)(using Context) {

// Take care that only argument bindings go into `bindings`, since positions are
// different for bindings from arguments and bindings from body.
tpd.Inlined(call, finalBindings, finalExpansion)
val res = tpd.Inlined(call, finalBindings, finalExpansion)
if opaqueProxies.isEmpty then res
else
val target =
if inlinedMethod.is(Transparent) then call.tpe & res.tpe
else call.tpe
res.ensureConforms(target)
// Make sure that the sealing with the declared type
// is type correct. Without it we might get problems since the
// expression's type is the opaque alias but the call's type is
// the opaque type itself. An example is in pos/opaque-inline1.scala.
}
}

Expand Down
2 changes: 1 addition & 1 deletion compiler/src/dotty/tools/dotc/typer/Namer.scala
Original file line number Diff line number Diff line change
Expand Up @@ -1519,7 +1519,7 @@ class Namer { typer: Typer =>
approxTp.stripPoly match
case atp @ defn.ContextFunctionType(_, resType, _)
if !defn.isNonRefinedFunction(atp) // in this case `resType` is lying, gives us only the non-dependent upper bound
|| resType.existsPart(_.isInstanceOf[WildcardType], stopAtStatic = true, forceLazy = false) =>
|| resType.existsPart(_.isInstanceOf[WildcardType], StopAt.Static, forceLazy = false) =>
originalTp
case _ =>
approxTp
Expand Down
2 changes: 0 additions & 2 deletions compiler/src/dotty/tools/dotc/typer/PrepareInlineable.scala
Original file line number Diff line number Diff line change
Expand Up @@ -259,8 +259,6 @@ object PrepareInlineable {
}

private def checkInlineMethod(inlined: Symbol, body: Tree)(using Context): body.type = {
if (inlined.owner.isClass && inlined.owner.seesOpaques)
report.error(em"Implementation restriction: No inline methods allowed where opaque type aliases are in scope", inlined.srcPos)
if Inliner.inInlineMethod(using ctx.outer) then
report.error(ex"Implementation restriction: nested inline methods are not supported", inlined.srcPos)

Expand Down
2 changes: 1 addition & 1 deletion compiler/src/dotty/tools/dotc/typer/Typer.scala
Original file line number Diff line number Diff line change
Expand Up @@ -2803,7 +2803,7 @@ class Typer extends Namer
// see tests/pos/i7778b.scala

val paramTypes = {
val hasWildcard = formals.exists(_.existsPart(_.isInstanceOf[WildcardType], stopAtStatic = true))
val hasWildcard = formals.exists(_.existsPart(_.isInstanceOf[WildcardType], StopAt.Static))
if hasWildcard then formals.map(_ => untpd.TypeTree())
else formals.map(untpd.TypeTree)
}
Expand Down
Loading