--stripe-packages: Hide PackageSpecs from top-level namespace #5644
Conversation
| BBB | ||
| # ^^^ error: Unable to resolve constant `BBB` |
There was a problem hiding this comment.
Prior to this change, this would have resolved. That's the whole point of this change.
| module ::Project | ||
| end | ||
|
|
||
| module ::Project::Foo | ||
| end | ||
|
|
There was a problem hiding this comment.
I don't have a ton of context on the rbi gen stuf going on here, but this seems like effectively a noop. cc @aadi-stripe @jez
There was a problem hiding this comment.
Doesn't this just mean that because these symbols are not available at the top-level by default anymore (due to the re-namespacing), we end up generating the shims for them?
There was a problem hiding this comment.
seems fine to me, we can figure it out later if it's a problem
There was a problem hiding this comment.
Yep, that's my understanding.
|
We have a policy of testing changes to Sorbet against Stripe's codebase before Stripe employees can see the build results here: → https://go/builds/bui_LX17eQnRsMp3z6 |
6c27594 to
1b489c6
Compare
|
I got this passing on Stripe's codebase with some minor changes: |
st //test:test_{Packager,Pos}Tests/testdata/packager/namespace
st is aliased to `bazel test --config=dbg --test_output=errors'
this is more accurate because dep does not exist anymore
1b489c6 to
adcc35a
Compare
|
(Rebased) |
packager/packager.cc
Outdated
| @@ -1191,6 +1214,10 @@ struct PackageInfoFinder { | |||
|
|
|||
| info->loc = ctx.locAt(classDef.loc); | |||
| info->declLoc_ = ctx.locAt(classDef.declLoc); | |||
|
|
|||
| // `class Foo < PackageSpace` -> `class <PackageSpecRegistry>::Foo < PackageSpec` | |||
There was a problem hiding this comment.
| // `class Foo < PackageSpace` -> `class <PackageSpecRegistry>::Foo < PackageSpec` | |
| // `class Foo < PackageSpec` -> `class <PackageSpecRegistry>::Foo < PackageSpec` |
packager/packager.cc
Outdated
| auto *lastConstLit = &ast::cast_tree_nonnull<ast::UnresolvedConstantLit>(scope); | ||
| while (auto constLit = ast::cast_tree<ast::UnresolvedConstantLit>(lastConstLit->scope)) { |
There was a problem hiding this comment.
Hm, why not just use pointers?
| auto *lastConstLit = &ast::cast_tree_nonnull<ast::UnresolvedConstantLit>(scope); | |
| while (auto constLit = ast::cast_tree<ast::UnresolvedConstantLit>(lastConstLit->scope)) { | |
| auto lastConstLit = ast::cast_tree<ast::UnresolvedConstantLit>(scope); | |
| ENFORCE(lastContLit != nullptr); | |
| while (auto constLit = ast::cast_tree<ast::UnresolvedConstantLit>(lastConstLit->scope)) { |
(i try to avoid the address-of & operator whenever possible)
Wrap all package spec declarations so that they don't resolve at the top level. This is done by prefixing them with a symbol
<PackageSpecRegistry>during packaging.Motivation
Referencing unimported packages should result in a constant resolution error. However, in cases, especially where the package had behavior at the top level, you could get confusing errors or no error at all. For example:
If you were to
include Unimported::Packagethis would be a no-op and not result in any error.Test plan
Added tests.
See included automated tests.