Skip to content

Fix global init checking crash when using a value defined in by-name closure #22625

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 4, 2025

Conversation

q-ata
Copy link
Contributor

@q-ata q-ata commented Feb 19, 2025

This PR fixes a crash that would occur when analyzing a by-name closure that reads a local value.

This was because the value would have an incorrect enclosingMethod field due to the analysis running before by-names are transformed by the elimByName pass.

[test_scala2_library_tasty]

@q-ata q-ata force-pushed the safe-init-global-envs branch from c10505f to 37e7e2a Compare February 19, 2025 04:32
@q-ata q-ata marked this pull request as ready for review February 19, 2025 13:01
@dwijnand dwijnand assigned liufengyun and unassigned dwijnand Feb 19, 2025
@dwijnand dwijnand removed their request for review February 19, 2025 18:41
Copy link
Contributor

@olhotak olhotak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes LGTM, but we should discuss the case where OfClass.outer is a ValueSet in our next meeting.

Copy link
Contributor

@liufengyun liufengyun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you @q-ata !

None
case _ =>
None
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking whether we can simplify resolveEnvByValue to just return the value for the local variable. We'll probably need two methods, one for mutable, one for immutable. But the logic would be simpler and it'll be easier to handle the TODOs.

That leaves us with the case for lazy local variables, for that one, we can use resolveEnvByOwner.

If it makes sense, it can be experimented in another PR.

@liufengyun
Copy link
Contributor

In order to avoid breaking nightly build, I added [test_scala2_library_tasty] to the PR description. @q-ata Could you please touch the last commit to trigger the CI again?

@Gedochao
Copy link
Contributor

@liufengyun @q-ata @olhotak is this ready to be merged?

@liufengyun
Copy link
Contributor

The last commit needs to be remove and then it's ready to go in.

@q-ata q-ata force-pushed the safe-init-global-envs branch from 42d0dfd to f3c4751 Compare March 3, 2025 02:48
@q-ata q-ata requested review from liufengyun and olhotak March 3, 2025 02:49
@olhotak olhotak merged commit df6958b into scala:main Mar 4, 2025
29 checks passed
@olhotak olhotak deleted the safe-init-global-envs branch March 4, 2025 19:14
@WojciechMazur WojciechMazur added this to the 3.7.0 milestone Mar 11, 2025
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.

6 participants