Fix #8799: Make sure signatures are computed before erasure #8819
Conversation
| val lastd = lastDenotation | ||
| if (lastd != null) lastd.signature | ||
| val isErased = ctx.erasedTypes | ||
| if lastd != null && !isErased then lastd.signature |
There was a problem hiding this comment.
ctx.erasedTypes being false does not mean that lastd itself is before erasure, for that one would have to check last.validFor.firstPhaseId < ctx.erasurePhase.id I think (this might be off by one).
There was a problem hiding this comment.
Yes, good point. I have fixed it now.
| else symbol.asSeenFrom(prefix).signature | ||
| } | ||
|
|
||
| /** The signature of the current denotation if it is known without forcing. | ||
| * Otherwise the signature of the current symbol if it is known without forcing. | ||
| * Otherwise NotAMethod. | ||
| */ | ||
| private def currentSignature(implicit ctx: Context): Signature = |
There was a problem hiding this comment.
currentSignature is passed as an argument to disambiguate which passes it to Denotation#atSignature. If we're after erasure and the signature of the denotation is different from the pre-erasure signature that currentSignature, then atSignature would fail to match anything, wouldn't it? Or is it this OK because overloaded denotations do not appear after erasure at all? If so then currentSignature wouldn't need to do anything special.
There was a problem hiding this comment.
Yes, after erasure all references should be symbolic, so there's no need to do overloading resolution.
|
@smarter I am done with it (meaning both I think it's finished for now and I don't want to do anything further on this). OK with you to merge ? |
|
I'd like to add a test before merging, I'll take care of it. |
|
Just pushed a test which runs into another issue: after the getters phase, vals become defs, which means their signature changes from |
Ah, but this isn't that easy since we use currentSignature to do overload resolution, so it has to match the signatures we see in the current phase which is why the previous PR was problematic: #8813 (comment) |
|
Other possible fix: always return |
Some symbols change their signatures after erasure, e.g. getters with Unit type or constructors taking an outer parameter. Since signatures are assumed to be stable for a complete run, we need to always compute signatures for NamedTypes before ersure.
Getters will replace `val x: Int`, by `def x: Int`, before this commit, this lead to the signature of `x` changing from `Signature.NotAMethod` to `Signature(Nil, scala.Int)` but we need signatures to be stable pre-erasure to disambiguate overloads. This commit fixes this by changing ExprType#signature to always return `NotAMethod`, this should not break anything since we do not allow overloads which only differ in their result type anyway. This commit also adds tests to make sure that the invariants related to signatures are not violated, currently there is one class of failure related to enums which is fixed in the next commit.
CompleteJavaEnums changes some signatures (it adds extra constructor
parameters to classes extending java.lang.Enum), so it is not allowed to
run before Erasure (this is enforced by the check added in TreeChecker
in the previous commit).
Moving CompleteJavaEnums after Erasure required one change, prior to
this commit, constructor parameter were added to enum cases in two
different places:
- `def $new(...) = { new E { ... } }` was handled by transformDefDef
- `val Foo = new E { ... }` was handled by transformTemplate
It turns out that we can just let transformTemplate handle both cases,
this simplifies the code and means that the Mixin phase can run after
CompleteJavaEnums in the same group and see the transformed
constructors.
|
Last commit LGTM |
Some symbols change their signatures after erasure,
e.g. getters with Unit type or constructors taking an outer parameter.
Since signatures are assumed to be stable for a complete run,
we need to always compute signatures for NamedTypes before erasure.