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 global init checking crash when using a value defined in by-name closure #22625

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

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?

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.

4 participants