Alternative fix for #2099: avoid loading a private member when recomputing a NamedType denot #2106
Conversation
smarter
force-pushed
the
fix/param-forwarder-access-take-2
branch
2 times, most recently
from
78642a8 to
582bcac
Compare
…edType denot ParamForwarding creates the following forwarder in B: private[this] def member: Int = super.member Where the type for `super.member` is `TermRef(SubA, member)` and the symbol is the `val member` in `A`. So far this is correct, but in later phases we might call `loadDenot` on this `TermRef` which will end up calling `asMemberOf`, which before this commit just did: prefix.member(name) This is incorrect in our case because `SubA` also happens to have a private `def member`, which means that our forwarder in B now forwards to a private method in a superclass, this subsequently crashes in `ExpandPrivate`. (Note: in the bytecode, a private method cannot have the same name as an overriden method, but this is already worked around in EnsurePrivate.) The fix is simple: when we recompute a member, we should only look at private members if the previous denotation was private.
This was introduced in the previous commit and caused unpickling failures, we are now more conservative and only check the Private flag on SymDenotations so we don't have to load any other denotation.
smarter
force-pushed
the
fix/param-forwarder-access-take-2
branch
from
582bcac to
1821944
Compare
| // We shouldn't go from a non-private denotation to a private one | ||
| prefix.nonPrivateMember(name) | ||
| case _ => | ||
| prefix.member(name) |
There was a problem hiding this comment.
On second thought, I think we should revert to the previous scheme where allowPrivate was passed as a boolean to asMemberOf. There are 3 calls to asMemberOf. The ones in recomputeDenot and newLikeThis can get the parameter from the symbol. The third one, in loadDenot should unconditionally pass true for
allowPrivate. After all, it could be that a definition was public in run 1, but private in run 2. We need to be able to pick up the new definition in loadDenot.
|
Review addressed. However I'm wondering if it wouldn't make sense to take advantage of the existing |
ParamForwarding creates the following forwarder in B:
Where the type for
super.memberisTermRef(SubA, member)and thesymbol is the
val memberinA.So far this is correct, but in later phases we might call
loadDenotonthis
TermRefwhich will end up callingasMemberOf, which before thiscommit just did:
This is incorrect in our case because
SubAalso happens to have aprivate
def member, which means that our forwarder in B now forwardsto a private method in a superclass, this subsequently crashes in
ExpandPrivate.(Note: in the bytecode, a private method cannot have the same name as an
overriden method, but this is already worked around in EnsurePrivate.)
The fix is simple: when we recompute a member, we should only look at
private members if the previous denotation was private.