Fix #8799: Use initial denotation for signatures #8813
Conversation
See scala#8799 for more information. The test currently fails with: Wrong cached signature at phase elimErasedValueType for (Foo.this.value : (): scala.runtime.BoxedUnit). Actual denotation signature: Signature(List(),scala.runtime.BoxedUnit) Cached ref signature: Signature(List(),), The test passes if I tweak NamedType to not cache signatures.
Use the initial denotation to computer the signature of a NamedType
| protected def computeSignature(implicit ctx: Context): Signature = | ||
| lastDenotation match | ||
| case null => symbol.asSeenFrom(prefix).signature | ||
| case sd: SingleDenotation => sd.initial.signature |
There was a problem hiding this comment.
Perhaps Denotation#signature itself should do the work of calling initial? That way we're sure to never use the wrong signature
There was a problem hiding this comment.
Case in point: the code just below for currentSignature calls lastd.signature and lastd might not be the initial denotation. Similarly, symbol.asSeenFrom(prefix).signature above will also be computed at the current phase.
There was a problem hiding this comment.
As long as signatures are defined in denotations, I think they should apply to the owner denotation. If we wanted to always go for initial, then signatures should be defined on symbols.
Agree that currentSignature needs to be fixed as well.
There was a problem hiding this comment.
If we wanted to always go for initial, then signatures should be defined on symbols.
That might make sense, I can see how different prefixes can lead to different signatures for denotations, but I don't think that should ever be needed to disambiguate between overloads ?
There was a problem hiding this comment.
hmm I guess that would probably interfere with how we deal with merging denotations.
There was a problem hiding this comment.
Yes, I think it's better to leave them on denotations.
| protected def computeSignature(implicit ctx: Context): Signature = | ||
| lastDenotation match | ||
| case null => symbol.asSeenFrom(prefix).signature | ||
| case sd: SingleDenotation => sd.initial.signature |
There was a problem hiding this comment.
Case in point: the code just below for currentSignature calls lastd.signature and lastd might not be the initial denotation. Similarly, symbol.asSeenFrom(prefix).signature above will also be computed at the current phase.
|
This change means we need to be very careful anytime we compare signatures: if we compare the signature of two typerefs referring to symbols created at different phases, they might not match anymore. For example this check in TypeComparer is likely not correct anymore: |
Co-Authored-By: Guillaume Martres <[email protected]>
|
I had a look at the TypeComparer test. There it should be OK since the fallBack |
| } | ||
| } | ||
| case sd: SingleDenotation => sd.initial.signature | ||
| case d => d.signature |
There was a problem hiding this comment.
why is the non-SingleDenotation case handled specially without calling initial ?
There was a problem hiding this comment.
We do not have an initial for general denotations
There was a problem hiding this comment.
OK, and it's fine because their signature is always OverloadedSignature right ? In that case maybe we should put a comment in the code or directly return OverloadedSignature to make that obvious
|
I just realized that |
|
In fact the problem is less severe than I thought at first: ExplicitOuter does not change signatures. It delays to erasure to do this. So,
So it's enough to just make sure that all signatures of NamedTypes are computed before erasure. This is what #8819 does. |
Use the initial denotation to compute the signature of a NamedType.