-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Changes from 8 commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
9a998d4
Refactor stopAtStatic
odersky 279c185
Handle opaque types when inlining.
odersky df412c6
Better job of stripping module class names
odersky b9e76d6
Avoid redundant generation of static this proxies
odersky 363c864
Fix completion test
odersky d7e90d5
Update check file
odersky 2414033
Fix canElideThis condition
odersky 3f07fb9
Streamline a collection operation
odersky d4e8f5d
Also use casts for transparent inlines
odersky File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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)) | ||
:: 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 | ||
|
@@ -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 | ||
|
@@ -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)) | ||
|
@@ -843,7 +924,16 @@ class Inliner(call: tpd.Tree, rhsToInline: tpd.Tree)(using Context) { | |
|
||
// Apply inliner to `rhsToInline`, split off any implicit bindings from result, and | ||
// make them part of `bindingsBuf`. The expansion is then the tree that remains. | ||
val expansion = inliner.transform(rhsToInline) | ||
val expansion0 = inliner.transform(rhsToInline) | ||
val expansion = | ||
if opaqueProxies.nonEmpty && !inlinedMethod.is(Transparent) then | ||
odersky marked this conversation as resolved.
Show resolved
Hide resolved
|
||
expansion0.cast(call.tpe)(using ctx.withSource(expansion0.source)) | ||
// the cast makes 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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This branch is never actually reached for |
||
else | ||
expansion0 | ||
|
||
def issueError() = callValueArgss match { | ||
case (msgArg :: Nil) :: Nil => | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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:There was a problem hiding this comment.
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
The weird thing is that with the original condition everything succeeded, even though the expansion was then
This is clearly type-incorrect, since
9
is anInt
andrefined.f
expects arefined.Positive
. Indeed, if I write this expansion manually, it fails.But somehow
-Ycheck
does not test this. I traced Ycheck's behavior and it seems to think thatrefined.f: Int => Int
even though it should berefined.f: Positive => Positive
. The reason for this is that it copies the treerefined.f
as is from the inline method, which is in a scope wherePositive = Int
. Hence,Int => Int
is a legal type forf
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.