Delete --package-rbi-generation mode
#8662
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
neilparikh
approved these changes
Mar 12, 2025
The `--package-rbi-generation` mode is proving difficult to maintain for
a couple reasons:
- It initializes the packager options differently from the rest of the
pipeline. This is important, because we need the GlobalState to have
the right packager options so that `namer` can decide whether it wants
to treat `__package.rb` files as packages and enter them into the
symbol table, or leave them as normal Ruby files.
- It does weird things with `__package.rb` files. It tries to make it
seem like you're running with `opts.stripePackages` being false, but
then do package-related things (e.g., `findPackages`).
- It's going to complicate our attempts to interleave resolve and
typecheck.
This feature never made it into use at Stripe, and never gained LSP
support. Also, we generally don't think this approach is the approach
we'd use if we were to revisit that: we think that we'd probably have a
"one RBI per package" approach, instead of "one RBI per transitive
package deps" approach, which would get rid a lot of the hacks around
"this symbol doesn't resolve" that have infected resolver right now.
This change deletes all but the code in `packager/rbi_gen.{h,cc}`. The
RBI generation code itself is useful to keep around because it has a
bunch of stuff which could be useful for simply generating RBIs from
`GlobalState`.
The code in `packager/rbi_gen.*` will still be tested indirectly via the `//packager:packager` library. We could split it out, but then we'd have to make a whole job just to test it and there's not that much code in the library in the first place, so I'm not too worried about it being separate.
froydnj
approved these changes
Apr 29, 2025
I don't need to remove it for any of the other packager/namer work, so I had left it so that it would at least keep building. Most of the code is pretty straightforward and could be a useful mechanism to effectively serialize a symbol table to an RBI. If we want it gone too, I'm happy to remove it in a follow up. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
The
--package-rbi-generationmode is proving difficult to maintain for acouple reasons:
It initializes the packager options differently from the rest of the
pipeline. This is important, because we need the GlobalState to have
the right packager options so that
namercan decide whether it wantsto treat
__package.rbfiles as packages and enter them into thesymbol table, or leave them as normal Ruby files.
It does weird things with
__package.rbfiles. It tries to make itseem like you're running with
opts.stripePackagesbeing false, butthen do package-related things (e.g.,
findPackages).It's going to complicate our attempts to interleave resolve and
typecheck.
This feature never made it into use at Stripe, and never gained LSP
support. Also, we generally don't think this approach is the approach we'd
use if we were to revisit that: we think that we'd probably have a
"one RBI per package" approach, instead of "one RBI per transitive package
deps" approach, which would get rid a lot of the hacks around
"this symbol doesn't resolve" that have infected resolver right now.
This change deletes all but the code in
packager/rbi_gen.{h,cc}. The RBIgeneration code itself is useful to keep around because it has a bunch of
stuff which could be useful for simply generating RBIs from
GlobalState.Test plan
not planning on touching
rbi_gen.cccode is still being compiled via//main:sorbetso thosebuild failures will show up in CI still