-
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
Fix opaque types leaking rhs when inlined and found in type params (and a related stale symbol issue) #22655
base: main
Are you sure you want to change the base?
Conversation
a1755a6
to
4d937b2
Compare
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.
Generally LGTM, couple of requests
@@ -399,7 +399,9 @@ class Inliner(val call: tpd.Tree)(using Context): | |||
* type aliases, add proxy definitions to `opaqueProxies` that expose these aliases. | |||
*/ | |||
private def addOpaqueProxies(tp: Type, span: Span, forThisProxy: Boolean)(using Context): Unit = | |||
tp.foreachPart { | |||
val foreachTpPart = | |||
(p: Type => Unit) => if (forThisProxy) tp.foreachPartWithoutTypeParams(p) else tp.foreachPart(p) |
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.
I wouldn't introduce foreachPartWithoutTypeParams
for one use case. Also, it's a confusing name, because you're skipping applied type arguments, rather than "type parameters".
@@ -712,7 +712,7 @@ class Namer { typer: Typer => | |||
enterSymbol(classConstructorCompanion(classSym.asClass)) | |||
else | |||
for moduleSym <- companionVals do | |||
if moduleSym.is(Module) && !moduleSym.isDefinedInCurrentRun then | |||
if moduleSym.lastKnownDenotation.is(Module) && !moduleSym.isDefinedInCurrentRun then |
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.
Can you add a little detail in a comment about this usage (with any references that would be helpful)
This PR fixes the 2 issues found in #20449, split into 2 commits.
The first commit fixes the stale symbol related issue found if the files from the issue minimization are compiled together. After suspending and retrying compilation, the classDefs that are defined directly in packages previously would sometimes not have companion objects regenerated, instead relying on the stale symbols from the previous run, causing them not to to pass the reallyExists check when looking for a specific ref. Now we make sure to go through lastKnwonDenotation, since the current one may not exists and may not point us to a Module flag when checking if to regenerate it.
The second commit fixes the opaque type alias rhs leaking in a macro. That was caused by building proxies for all parts of the type, including type arguments to opaque types - from the perspective of a type like Object[OpaqueType], the opaque type rhs should not be visible.