Harden erasure of TermRefs #15658
Harden erasure of TermRefs #15658
Conversation
If a getter TermRef has a MethodType, erase it to the erasure of the method result type. Fixes scala#15649 Since the fault in scala#15649 is highly non-deterministic, I can only guess what the reason was. I think what happened was that the TermRef was created very late, when the TermRef was already a getter. I.e. after phase `Getters`, which is itself after `Erasure`. The whole thing was launched from the code that generates static forwarders in the backend. Type erasure is run at erasure phase, but if there is no previous denotation of the `TermRef`, it will keep the first one it read, which would have the `MethodType` as underlying type. The end result was that the `TermRef` was erased to a method type, where it should have been erased to the result type. Consequently, we ended up with a MethodType as the parameter type of another method, which is unexpected. But all that is just a guess. I am not even sure this PR will fix the problem since it comes up so rarely.
|
I think in general it's a bad idea to do static forwarder generation in the backend. It should be a proper miniphase, then we would most likely avoid problems like this one. |
| @@ -687,6 +689,10 @@ class TypeErasure(sourceLanguage: SourceLanguage, semiEraseVCs: Boolean, isConst | |||
| tp | |||
| } | |||
|
|
|||
| private def underlyingOfTermRef(tp: TermRef)(using Context) = tp.widen match | |||
There was a problem hiding this comment.
This probably deserves a comment referencing the issue. Otherwise someone will come here later, figure that this is useless, and remove it.
There was a problem hiding this comment.
I tested this extensively, and was unable to get the reproducer test case to crash with this change in place. The community project from which it was minimized, zio-config, also compiles again with this change.
So at least in that sense, this PR seems to fix the problem.
I was also able to confirm that the getter TermRef has its denotation replaced with a new UniqueRefDenotation using the logic added in e609851, which as far as I can tell is what caused this problem to appear.
I agree with the suggestion to add a code comment referencing the issue.
griggt
added
the
backport:nominated
griggt
added
backport:accepted
Kordyjan
added
backport:done
Backport #15658: Harden erasure of TermRefs
If a getter TermRef has a MethodType, erase it to the erasure of the method
result type.
Fixes #15649
Since the fault in #15649 is highly non-deterministic, I can only guess what the reason
was. I think what happened was that the TermRef was created very late, when the TermRef
was already a getter. I.e. after phase
Getters, which is itself afterErasure. Thewhole thing was launched from the code that generates static forwarders in the backend.
Type erasure is run at erasure phase, but if there is no previous denotation of the
TermRef,it will keep the first one it read, which would have the
MethodTypeas underlying type.The end result was that the
TermRefwas erased to a method type, where it should havebeen erased to the result type. Consequently, we ended up with a MethodType as the
parameter type of another method, which is unexpected.
But all that is just a guess. I am not even sure this PR will fix the problem since it
comes up so rarely.