Fix #3333: adapt child instantiation #3475
Conversation
|
@AleksanderBG Do you want to review? This may be helpful to your WIP. |
abgruszecki
left a comment
abgruszecki
left a comment
There was a problem hiding this comment.
Thanks for the opportunity to review! Aside from some questions, everything looks OK to me.
| def apply(t: Type): Type = t match { | ||
| case tp @ ThisType(tref) if !tref.symbol.isStaticOwner && !tref.symbol.is(Module) => | ||
| // TODO: stackoverflow here | ||
| // newTypeVar(TypeBounds.upper(mapOver(tp.underlying))) |
There was a problem hiding this comment.
Out of curiosity: what was the original issue here? Was tp present somewhere in tp.underlying? If yes, then wouldn't simply first creating the TVar, memoising it in a map keyed under tp and mapping over tp.underlying be enough? I'm asking, as basing on this code I wrote this and I'm not sure if it is correct.
There was a problem hiding this comment.
I forgot exactly what's the problem, but it's related to ClassInfo#selfType. tp.underlying is supposed to be decreasing (I assume), so I think the case you mention will not occur. For the task you want to deal with, the memoization in your code is correct.
| if (protoTp1 <:< tp2 && isFullyDefined(protoTp1, ForceDegree.noBottom)) protoTp1 | ||
| if (protoTp1 <:< tp2) { | ||
| isFullyDefined(protoTp1, force) | ||
| instUndetMap(protoTp1) |
There was a problem hiding this comment.
If I understand correctly how isFullyDefined behaves, then if it returns true, all TVars in protoTp1 should be already instantiated and instUndetMap will be a no-op. Would it make sense to only use instUndetMap if isFullyDefined returned false?
There was a problem hiding this comment.
Yes, good catch, I've updated the code, thanks a lot.
3 participants
Fix #3333 #3455 #3469: adapt child instantiation
Structural types in #3333 is left open.
ThisTypefor modules.