-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix validity period of derived SingleDenotations #19983
Conversation
test performance please |
1 similar comment
test performance please |
performance test scheduled: 148 job(s) in queue, 1 running. |
Performance test finished successfully: Visit https://dotty-bench.epfl.ch/19983/ to see the changes. Benchmarks is based on merging with main (a36849f) |
When running: val f: ( => Int) => Int = i => i ; f(1) twice in the REPL, the second time crashed with a ClassCastException. The difference is that in the second run, the denotation for `f.apply` is created from the SymDenotation for `Function1#apply` which already exists and is known to be valid in every phase, but that doesn't mean that the derived denotation for `f.apply` has the same validity: unlike the SymDenotation it needs to be transformed (in particular to run the `ElimByName` transformer). It turns out that there were multiple places in the compiler where we created a new denotation with a validity based on the existing one when this was not legitimate. I've gone through all these places and replaced them by `currentStablePeriod`. Fixes scala#18756.
Performance test finished successfully: Visit https://dotty-bench.epfl.ch/19983/ to see the changes. Benchmarks is based on merging with main (301c977) |
Performance seems to be neutral or better. |
When running:
twice in the REPL, the second time crashed with a ClassCastException.
The difference is that in the second run, the denotation for
f.apply
is created from the SymDenotation forFunction1#apply
which already exists and is known to be valid in every phase, but that doesn't mean that the derived denotation forf.apply
has the same validity: unlike the SymDenotation it needs to be transformed (in particular to run theElimByName
transformer).It turns out that there were multiple places in the compiler where we created a new denotation with a validity based on the existing one when this was not legitimate. I've gone through all these places and replaced them by
currentStablePeriod
.Fixes #18756.