8000 make `GlobalState::*InitForFile` functions only take `FileRef` instead of `Loc` by froydnj · Pull Request #5397 · sorbet/sorbet · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

make GlobalState::*InitForFile functions only take FileRef instead of Loc #5397

8000
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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Mar 3, 2022
Merged

make GlobalState::*InitForFile functions only take FileRef instead of Loc #5397

merged 1 commit into from
Mar 3, 2022

Conversation

froydnj
Copy link
Contributor
@froydnj froydnj commented Mar 2, 2022

Motivation

GlobalState::staticInitForFile and GlobalState::lookupStaticInitForFile take Loc and generally only care about the file attached to that Loc. But because of this, we end up synthesizing a Loc at the callsites to those functions, and we do it in inconsistent ways. This scheme also results in less clear code: one wonders why you have a full Loc for a function that should seemingly only care about the file.

We should standardize: the aforementioned functions should only take FileRef and be responsible for synthesizing full Locs for internal purposes. We can then ensure that things are much more consistent (e.g. the symbols entered by staticInitForFile no longer depend on the first time staticInitForFile is called and the Loc passed to it).

Test plan

See included automated tests.

@froydnj froydnj requested a review from a team as a code owner March 2, 2022 16:39
@froydnj froydnj requested review from aprocter and removed request for a team March 2, 2022 16:39
jez
jez reviewed Mar 2, 2022
View reviewed changes
@jez
Copy link
Collaborator
jez commented Mar 2, 2022

The fact that this PR causes packager stuff to change makes me suspicious that this is as benign a change as the motivation says. I'm curious if my suggestion to only change lookupStaticInitForX (and not staticInitForX) fixes the packager bug.

@froydnj froydnj force-pushed the froydnj-file-static-init branch from 8f4c522 to 38720a1 Compare March 3, 2022 14:53
@froydnj
Copy link
Contributor Author
froydnj commented Mar 3, 2022

The fact that this PR causes packager stuff to change makes me suspicious that this is as benign a change as the motivation says. I'm curious if my suggestion to only change lookupStaticInitForX (and not staticInitForX) fixes the packager bug.

It does...but I have no idea why. 🤷‍♂️

@froydnj froydnj requested a review from jez March 3, 2022 19:30
@froydnj froydnj merged commit c6d23fe into master Mar 3, 2022
@froydnj froydnj deleted the froydnj-file-static-init branch March 3, 2022 19:40
@froydnj froydnj removed the request for review from aprocter March 3, 2022 19:40
@froydnj froydnj self-assigned this Mar 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jez jez jez approved these changes

Assignees

@froydnj froydnj

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@froydnj @jez
0