Skip to content

Correct owner chains when mixing byname implicits and inlining #5767

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.

Sign up for GitHub

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 3 commits into from
Jan 29, 2019
Merged

Correct owner chains when mixing byname implicits and inlining #5767

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d27adbe
Correct owner chains when mixing byname implicits and inlining
milessabin Jan 21, 2019
cfaf6f0
Don't visit DefTree subtrees in changeNonLocalOwners
milessabin Jan 23, 2019
98a4f14
Simplifications from review
milessabin Jan 24, 2019
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
21 changes: 20 additions & 1 deletion compiler/src/dotty/tools/dotc/ast/tpd.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import core._
import util.Spans._, Types._, Contexts._, Constants._, Names._, Flags._, NameOps._
import Symbols._, StdNames._, Annotations._, Trees._, Symbols._
import Decorators._, DenotTransformers._
import collection.mutable
import collection.{immutable, mutable}
import util.{Property, SourceFile, NoSource}
import NameKinds.{TempResultName, OuterSelectName}
import typer.ConstFold
Expand Down Expand Up @@ -730,6 +730,25 @@ object tpd extends Trees.Instance[Type] with TypedTreeInfo {
if (from == to) tree else loop(from, Nil, to :: Nil)
}

/**
* Set the owner of every definition in this tree which is not itself contained in this
* tree to be `newowner`
*/
def changeNonLocalOwners(newOwner: Symbol)(implicit ctx: Context): Tree = {
val ownerAcc = new TreeAccumulator[immutable.Set[Symbol]] {
def apply(ss: immutable.Set[Symbol], tree: Tree)(implicit ctx: Context) = tree match {
case tree: DefTree =>
if (tree.symbol.exists) ss + tree.symbol.owner
else ss
case _ =>
foldOver(ss, tree)
}
}
val owners = ownerAcc(immutable.Set.empty[Symbol], tree).toList
val newOwners = List.fill(owners.size)(newOwner)
new TreeTypeMap(oldOwners = owners, newOwners = newOwners).apply(tree)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be simplified further?

def changeNonLocalOwners(newOwner: Symbol)(implicit ctx: Context): Tree = {
  val localOwnerAcc = new TreeAccumulator[List[Symbol]] {
    def apply(ss: List[Symbol], tree: Tree)(implicit ctx: Context) = tree match {
      case tree: DefTree =>
        if (tree.symbol.exists) tree.symbol :: ss
        else ss
      case _ =>
        foldOver(ss, tree)
      }
    }
  }
  val nonLocalOwners = localOwnerAcc(Nil, tree).map(_.owner).distinct
  val newOwners = nonLocalOwners.map(_ => newOwner)
  new TreeTypeMap(oldOwners = nonLocalOwners, newOwners = newOwners).apply(tree)
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the idea of using an accumulator is good, but why the detour via lists? Let's use an accumulator over an immutable set instead.

Copy link
Contributor

@allanrenucci allanrenucci Jan 24, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Updated:

def changeNonLocalOwners(newOwner: Symbol)(implicit ctx: Context): Tree = {
  val ownerAcc = new TreeAccumulator[Set[Symbol]] {
    def apply(ss: Set[Symbol], tree: Tree)(implicit ctx: Context) = tree match {
      case tree: DefTree =>
        if (tree.symbol.exists) ss + tree.symbol.owner
        else ss
      case _ =>
        foldOver(ss, tree)
      }
    }
  }
  val owners = ownerAcc(Set.empty, tree)
  val newOwners = List.fill(owners.size)(newOwner)
  new TreeTypeMap(oldOwners = owners.toList, newOwners = newOwners).apply(tree)
}


/** After phase `trans`, set the owner of every definition in this tree that was formerly
* owner by `from` to `to`.
*/
Expand Down
2 changes: 1 addition & 1 deletion compiler/src/dotty/tools/dotc/typer/Implicits.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1577,7 +1577,7 @@ final class SearchRoot extends SearchHistory {
val nrhss = rhss.map(rhsMap(_))

val vdefs = (nsyms zip nrhss) map {
case (nsym, nrhs) => ValDef(nsym.asTerm, nrhs)
case (nsym, nrhs) => ValDef(nsym.asTerm, nrhs.changeNonLocalOwners(nsym))
}

val constr = ctx.newConstructor(classSym, Synthetic, Nil, Nil).entered
Expand Down
32 changes: 32 additions & 0 deletions tests/pos/i5766.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
object Test1 {
trait Foo { def next: Foo }

inline implicit def foo(implicit loop: => Foo): Foo = new Foo { def next = loop }

def summon(implicit f: Foo) = ()
summon
}

object Test2 {
trait Foo { def next: Bar }
trait Bar { def next: Foo }

implicit def foo(implicit loop: => Bar): Foo = new Foo { def next = loop }
inline implicit def bar(implicit loop: Foo): Bar = new Bar { def next = loop }

def summon(implicit f: Foo) = ()
summon
}

object Test3 {
trait Foo { def next: Bar }
trait Bar { def next: Baz }
trait Baz { def next: Foo }

implicit def foo(implicit loop: Bar): Foo = new Foo { def next = loop }
inline implicit def bar(implicit loop: Baz): Bar = new Bar { def next = loop }
inline implicit def baz(implicit loop: => Foo): Baz = new Baz { def next = loop }

def summon(implicit f: Foo) = ()
summon
}