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

Correct owner chains when mixing byname implicits and inlining #5767

Merged
merged 3 commits into from
Jan 29, 2019

Conversation

milessabin
Copy link
Contributor

When constructing the dictionary for a byname implicit resolution we have to ensure that the owners of any definitions which have been hoisted into the dictionary are correct. Prior to this PR this was only
done fully for implicit expansions which didn't involve inlining. We fix that here by ensuring that all definitions under a given dictionary entry are owned directly or indirectly by that dictionary entry.

Fixes #5766.

When constructing the dictionary for a byname implicit resolution we
have to ensure that the owners of any definitions which have been
hoisted into the dictionary are correct. Prior to this PR this was only
done fully for implicit expansions which didn't involve inlining. We fix
that here by ensuring that all definitions under a given dictionary
entry are owned directly or indirectly by that dictionary entry.

Fixes scala#5766.
@biboudis biboudis requested review from odersky and removed request for nicolasstucki January 23, 2019 15:19
val nonLocalOwners = localOwners.filter(sym => !localOwners.contains(sym.owner)).toList.map(_.owner)
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.

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

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)
}

Copy link
Contributor

@odersky odersky left a comment

Choose a reason for hiding this comment

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

Otherwise LGTM.

val nonLocalOwners = localOwners.filter(sym => !localOwners.contains(sym.owner)).toList.map(_.owner)
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.

@odersky odersky assigned milessabin and unassigned odersky Jan 24, 2019
@milessabin milessabin dismissed odersky’s stale review January 24, 2019 11:58

Incorporated review feedback. I'll merge when green.

@milessabin milessabin merged commit 190f34b into scala:master Jan 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mixing by-name implicits and inlining results in broken owner chains
3 participants