8000 Store the package name mapping in the PackageDB instead of the file table by elliottt · Pull Request #5724 · sorbet/sorbet · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Store the package name mapping in the PackageDB instead of the file table #5724

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 2 commits into from
Apr 29, 2022
Merged

Store the package name mapping in the PackageDB instead of the file table #5724

merged 2 commits into from
Apr 29, 2022

Conversation

elliottt
Copy link
Collaborator

Storing a NameRef in the file table is problematic, as multiple global states can share file tables, but have differing name tables. This PR moves the mapping from file to package into the PackageDB instead, avoiding the situation where we might have incompatible NameRef values stored in the file table.

Motivation

Stable package to file mapping.

Test plan

See included automated tests.

@elliottt elliottt requested a review from a team as a code owner April 29, 2022 18:31
@elliottt elliottt requested review from froydnj and removed request for a team April 29, 2022 18:31
@elliottt elliottt mentioned this pull request Apr 29, 2022
Closed
@elliottt elliottt requested review from andrejewski-stripe and jez and removed request for andrejewski-stripe April 29, 2022 18:33
@froydnj
Copy link
Contributor
froydnj commented Apr 29, 2022

I think this is fine as far as it goes, but doesn't this sort of have the same problem? Or do we always have a 1:1 mapping of GlobalState to PackageDB, and therefore the NameRef is unambiguous?

@elliottt
Copy link
Collaborator Author

I think this is fine as far as it goes, but doesn't this sort of have the same problem? Or do we always have a 1:1 mapping of GlobalState to PackageDB, and therefore the NameRef is unambiguous?

This is a little different in that the PackageDB values aren't shared between GlobalState instances, while the File values are explicitly shared with a shared_ptr. This way the mapping is unique for each GlobalState, rather than accidentally leaking NameRef values between them.

@elliottt elliottt force-pushed the trevor/move-file-package-to-package-db branch from d238864 to 1157bda Compare April 29, 2022 20:25
@elliottt
Copy link
Collaborator Author

We have a policy of testing changes to Sorbet against Stripe's codebase before
merging them. I've kicked off a test run for the current PR. When the build
finishes, I'll share with you whether or how it failed. Thanks!

Stripe employees can see the build results here:

→ https://go/builds/bui_LbCnb4hzeVmsUC
→ https://go/builds/bui_LbCnLINOW7yl08

@elliottt
Copy link
Collaborator Author

I think this is fine as far as it goes, but doesn't this sort of have the same problem? Or do we always have a 1:1 mapping of GlobalState to PackageDB, and therefore the NameRef is unambiguous?

This is a little different in that the PackageDB values aren't shared between GlobalState instances, while the File values are explicitly shared with a shared_ptr. This way the mapping is unique for each GlobalState, rather than accidentally leaking NameRef values between them.

I think my answer ended up more complicated than it needed to be: you're right about the 1:1 mapping of GlobalState to PackageDB, and that's what this PR relies on.

@elliottt elliottt merged commit 4e47d0a into master Apr 29, 2022
@elliottt elliottt deleted the trevor/move-file-package-to-package-db branch April 29, 2022 21:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
< 65C0 form class="js-issue-sidebar-form" aria-label="Select reviewers" data-reviewers-team-size-check-url="/sorbet/sorbet/pull/5724/review-requests/team-size-check" data-turbo="false" action="/sorbet/sorbet/pull/5724/review-requests" accept-charset="UTF-8" method="post">
Reviewers

@jez jez jez approved these changes

@froydnj froydnj Awaiting requested review from froydnj froydnj is a code owner automatically assigned from sorbet/core-reviewers

Assignees

@elliottt elliottt

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

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