Skip to content

Backport "Do not bring forward symbols created in transform and backend phases" to 3.3 LTS #143

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

Merged
merged 2 commits into from
Mar 13, 2025

Conversation

tgodzik
Copy link

@tgodzik tgodzik commented Mar 12, 2025

Backports scala#21865 to the 3.3.6.

PR submitted by the release tooling.
[skip ci]

jchyb and others added 2 commits March 12, 2025 14:21
…cala#21865)

In the i21844 issue minimization, first the `Macro.scala` and
`SuperClassWIthLazyVal.scala` compilation units are compiled, then in
the next run `SubClass.scala` is compiled. While compiling
`SuperClassWIthLazyVal.scala`, in the `LazyVals` transform phase, the
lzyINIT initialization fields are created. In the next run, while
compiling `SubClass.scala`, in the `GenBCode` phase, the compiler would
try to access the lzyINIT symbol, leading to a stale symbols crash.
While that symbol was a part of the SuperClass, it by design is not
generated for the Subclass - if we were to completely split those files
into 2 separate compilations, that symbol would be created only for the
classfile, but it would not be included in tasty, so the second
compilation would not try to access it.

This observation inspires the proposed fix, where if the symbol was
first created in or after the first transform phase (so after the
pickler phases), we do not try to bring forward this denotation, instead
returning NoDenotation.

In this PR, we also remove the fix proposed in i21559, as it is no
longer necessary with the newly added condition. There, since one of the
problematic Symbols created in `LazyVals` was moved elsewhere in
`MoveStatics`, we checked if that symbol could be found in a companion
object. I was not able to create any reproduction where a user defined
static would produce this problem, so I believe it’s safe to remove
that.
…cala#21865)

In the i21844 issue minimization, first the `Macro.scala` and
`SuperClassWIthLazyVal.scala` compilation units are compiled, then in
the next run `SubClass.scala` is compiled. While compiling
`SuperClassWIthLazyVal.scala`, in the `LazyVals` transform phase, the
lzyINIT initialization fields are created. In the next run, while
compiling `SubClass.scala`, in the `GenBCode` phase, the compiler would
try to access the lzyINIT symbol, leading to a stale symbols crash.
While that symbol was a part of the SuperClass, it by design is not
generated for the Subclass - if we were to completely split those files
into 2 separate compilations, that symbol would be created only for the
classfile, but it would not be included in tasty, so the second
compilation would not try to access it.

This observation inspires the proposed fix, where if the symbol was
first created in or after the first transform phase (so after the
pickler phases), we do not try to bring forward this denotation, instead
returning NoDenotation.

In this PR, we also remove the fix proposed in i21559, as it is no
longer necessary with the newly added condition. There, since one of the
problematic Symbols created in `LazyVals` was moved elsewhere in
`MoveStatics`, we checked if that symbol could be found in a companion
object. I was not able to create any reproduction where a user defined
static would produce this problem, so I believe it’s safe to remove
that.
[Cherry-picked 7d9771d][modified]
@tgodzik
Copy link
Author

tgodzik commented Mar 13, 2025

No regressions detected in the community build up to backport-lts-3.3-21888.

Reference

@tgodzik tgodzik merged commit 7c169f5 into lts-3.3 Mar 13, 2025
22 checks passed
@tgodzik tgodzik deleted the backport-lts-3.3-21865 branch March 13, 2025 09:37
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.

2 participants