Fix -Ytest-pickler with super trees #5516
Conversation
| @@ -329,7 +329,7 @@ trait TypeAssigner { | |||
| errorType("ambiguous parent class qualifier", tree.pos) | |||
| } | |||
| val owntype = | |||
| if (mixinClass.exists) mixinClass.appliedRef | |||
| if (mixinClass.exists) mixinClass.typeRef | |||
There was a problem hiding this comment.
I think neither old nor new version is right. What about this scenario?
trait A[X]
class B extends A[Int] {
super[A] // should have type A[Int], not A, or A[X]
}
There was a problem hiding this comment.
Yes, it's a bit weird, but if we change this then we need to change the pickling/unpickling scheme for super which currently does:
case SUPER =>
val qual = readTerm()
val (mixId, mixTpe) = ifBefore(end)(readQualId(), (untpd.EmptyTypeIdent, NoType))
tpd.Super(qual, mixId, ctx.mode.is(Mode.InSuperCall), mixTpe.typeSymbolSo won't work with mix being an AppliedType.
Alternatively we could change SuperType#underyling to work like SuperType#superType, currently they're different:
abstract case class SuperType(thistpe: Type, supertpe: Type) extends CachedProxyType with SingletonType {
override def underlying(implicit ctx: Context): Type = supertpe
override def superType(implicit ctx: Context): Type =
thistpe.baseType(supertpe.typeSymbol)But note that in practice I haven't found a case where the current definition breaks something, e.g. in findMember we call underlying on SuperType but that ends up working out since findMember keeps around the prefix of the call as pre which contains all the information we need to figure out the type parameters.
There was a problem hiding this comment.
I believe underlying in supertype is fine.
I don't see right now why pickler/unpickler has to change. Should we not instead change the TypeAssigner to do the same as SuperType$superType?
There was a problem hiding this comment.
Coming back to this PR 2 years (!) later, I believe I'm still confused by why underlying and superType are different in SuperType, if I follow your advice of making TypeAssigner use the same logic as superType to set superTpe, then they would always be the same anyway. Would you mind taking another look at this?
There was a problem hiding this comment.
I don't have deep insights here either. underlying does not do baseType invocations, and superType should probably do that.
That said, I now think the change in TypeAssigner makes sense. If everything passes with that change, let's do it.
|
There was no activity on this one for a long while, so let's close it. |
|
I disagree, I asked a question on March 23. |
|
/cc @odersky |
smarter
force-pushed
the
fix-pickling-super
branch
from
08a8fe7 to
acb9519
Compare
in t247.scala, `super[Tree]` was typed as `SuperType(TreeMap, Tree)` but the unpickled type was reconstructed as `SuperType(TreeMap, Tree[KEY, Entry])` leading to a -Ytest-pickler failure. We fix this by tweaking TypeAssigner for Super, and while we're at it we also add some checks related to Super in TreeChecker.
smarter
force-pushed
the
fix-pickling-super
branch
from
acb9519 to
1e00117
Compare
in t247.scala,
super[Tree]was typed asSuperType(TreeMap, Tree)butthe unpickled type was reconstructed as
SuperType(TreeMap, Tree[KEY, Entry])leading to a -Ytest-pickler failure. We fix this by tweakingTypeAssigner for Super, and while we're at it we also add some checks
related to Super in TreeChecker.