Fix parameter accessor forwarding #2124
Conversation
Making a name shadowed lost the signature before.
Normal references won't work since the referenced accessor has the same name as a private name in the class defining the forwarder. This showed up as pickling failures under separate compilation.
| Super(This(currentClass), tpnme.EMPTY, inConstrCall = false) | ||
| .select(alias) | ||
| val stpe @ TermRef(_, _) = superAcc.tpe | ||
| val superAccShadowed = superAcc.withType(stpe.shadowed) |
There was a problem hiding this comment.
Technically, this is only necessary when the super is not the base class where the paramaccessor is defined, but I guess there's no harm in always using .shadowed. However it would be nice to add a comment explaining when the .shadowed is useful.
| @@ -1782,15 +1782,19 @@ object Types { | |||
| else d.atSignature(sig).checkUnique | |||
| } | |||
|
|
|||
| override def newLikeThis(prefix: Type)(implicit ctx: Context): TermRef = { | |||
| val candidate = TermRef.withSig(prefix, name, sig) | |||
| private def fixDenot(candidate: TermRef, prefix: Type)(implicit ctx: Context): TermRef = | |||
| if (symbol.exists && !candidate.symbol.exists) { // recompute from previous symbol | |||
| val ownSym = symbol | |||
| val newd = asMemberOf(prefix, allowPrivate = ownSym.is(Private)) | |||
There was a problem hiding this comment.
Should we revert the addition of the allowPrivate parameter to asMemberOf since it is no longer useful for ParamForwarder and could mask other bugs that only show up under -Ytest-pickler ?
There was a problem hiding this comment.
I'd leave it in, because the logic makes sense anyway.
|
@smarter We get a difference with actual output and the check file for paramForwarding. But I don't know which one is correct, or whether it is important. Can you check? |
|
Also, I am not sure whether we want to avoid the $$local field for "override val" accessors. What's the point? Specifically, should we add forwarders only to PrivateParamAccessors? |
|
Yes, this difference also happened in my PR. I think updating the check file makes sense, the fact that Y does not get a field is coherent with the fact that C does not get a field, which was already the case before this PR. |
Isn't it always good to avoid adding fields if possible? |
The point is that "override val" seems non-sensical since the val is "overridden" with the same value it has already. But I agree it doe snot matter either way. |
|
Indeed, but override val also allows you to refine the type of the val |
Fixes remaining problems of #2116, from which it takes one change and all tests.