Skip to content
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

Merged
merged 1 commit into from
Mar 29, 2024

Conversation

smarter
Copy link
Member

@smarter smarter commented Mar 19, 2024

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 #18756.

@smarter smarter added the backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it. label Mar 19, 2024
@smarter
Copy link
Member Author

smarter commented Mar 19, 2024

test performance please

1 similar comment
@mbovel
Copy link
Member

mbovel commented Mar 21, 2024

test performance please

@dottybot
Copy link
Member

performance test scheduled: 148 job(s) in queue, 1 running.

@dottybot
Copy link
Member

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.
@smarter
Copy link
Member Author

smarter commented Mar 26, 2024

test performance please: ff751dd 49571fa

@dottybot
Copy link
Member

performance test scheduled for ff751dd 49571fa: 103 job(s) in queue, 1 running.

@dottybot
Copy link
Member

Performance test finished successfully:

Visit https://dotty-bench.epfl.ch/19983/ to see the changes.

Benchmarks is based on merging with main (301c977)

@smarter
Copy link
Member Author

smarter commented Mar 28, 2024

Performance seems to be neutral or better.

@odersky odersky merged commit fecfbda into scala:main Mar 29, 2024
19 checks passed
@odersky odersky deleted the i18576 branch March 29, 2024 09:38
@Kordyjan Kordyjan added this to the 3.5.0 milestone May 10, 2024
@WojciechMazur WojciechMazur removed the backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it. label Jul 1, 2024
WojciechMazur added a commit that referenced this pull request Jul 5, 2024
…21026)

Backports #19983 to the LTS branch.

PR submitted by the release tooling.
[skip ci]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Evaluating function value can lead to ClassCastException, when function type has by-name parameter
6 participants