-
Notifications
You must be signed in to change notification settings - Fork 138
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
Use root/absolute namespace in compiled RBI #780
Use root/absolute namespace in compiled RBI #780
Conversation
272d875
to
8d9ab00
Compare
bd7d429
to
529e1b0
Compare
::String
comes out of RBI as String
)
@paracycle @KaanOzkan - I think this PR fixes the issue that #472 was attempting to fix. Build is green so this should be good for a review. |
529e1b0
to
652e3ea
Compare
918e0fa
to
8c7a7dd
Compare
@paracycle - wanted to give this a friendly bump since tapioca is currently generating invalid RBI for one of our gems and there's no easy workaround besides manually editing the RBI. |
8c7a7dd
to
e6e7e6f
Compare
@@ -140,6 +140,7 @@ def dispatch(event) | |||
sig { params(event: Gem::SymbolFound).void } | |||
def on_symbol(event) | |||
symbol = event.symbol | |||
symbol = T.must(symbol[2..-1]) if symbol.start_with?("::") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The SymbolTableCompiler
was refactored recently but this is intended to be roughly the equivalent change to: fb32984.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use:
symbol = event.symbol.delete_prefix!("::")
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was trying to be consistent with
symbol_name = symbol_name[2..-1] if symbol_name.start_with?("::") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I would like to change that instance later as well. I find delete_prefix
to be more intention revealing and thus more readable, personally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me. Thanks for working on this and rebasing it on latest main
.
e6e7e6f
to
f2d66a8
Compare
@paracycle - Thanks for taking a look! I made the |
f2d66a8
to
69a8fb5
Compare
69a8fb5
to
9e6d36e
Compare
Tested this on Shopify Core and it seems to cause no issues. Thanks for pushing this forward 🙏 |
Motivation
Came across a subtle bug in tapioca RBI generation where absolute namespacing get dropped (e.g.
::String
comes out of RBI asString
). This causes issues when the referenced constant shares a name with both a global constant and one that lives somewhere inside the current namespace. See the test for an example.Implementation
Unsure how to fix, just wanted to send a reproduction.Usequalified_name_of
in the Simple type name patch.Tests
Added new tests and also needed to update existing ones.