Skip to content
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

Merged
merged 5 commits into from
Feb 12, 2022

Conversation

jeffcarbs
Copy link
Contributor

@jeffcarbs jeffcarbs commented Feb 1, 2022

Motivation

Came across a subtle bug in tapioca RBI generation where absolute namespacing get dropped (e.g. ::String comes out of RBI as String). 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. Use qualified_name_of in the Simple type name patch.

Tests

Added new tests and also needed to update existing ones.

@jeffcarbs jeffcarbs force-pushed the bug-drop-absolute-namespace branch from 272d875 to 8d9ab00 Compare February 2, 2022 05:02
@jeffcarbs jeffcarbs force-pushed the bug-drop-absolute-namespace branch from bd7d429 to 529e1b0 Compare February 2, 2022 21:38
@jeffcarbs jeffcarbs changed the title Absolute namespace getting dropped (e.g. ::String comes out of RBI as String) Use root/absolute namespace in compiled RBI Feb 2, 2022
@jeffcarbs
Copy link
Contributor Author

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

@jeffcarbs jeffcarbs force-pushed the bug-drop-absolute-namespace branch from 529e1b0 to 652e3ea Compare February 2, 2022 21:48
@jeffcarbs jeffcarbs force-pushed the bug-drop-absolute-namespace branch 2 times, most recently from 918e0fa to 8c7a7dd Compare February 5, 2022 23:12
@jeffcarbs
Copy link
Contributor Author

jeffcarbs commented Feb 8, 2022

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

@jeffcarbs jeffcarbs force-pushed the bug-drop-absolute-namespace branch from 8c7a7dd to e6e7e6f Compare February 9, 2022 06:12
@@ -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?("::")
Copy link
Contributor Author

@jeffcarbs jeffcarbs Feb 9, 2022

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.

Copy link
Member

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?

Copy link
Contributor Author

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?("::")
but happy to change it. What do you think?

Copy link
Member

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.

Copy link
Member

@paracycle paracycle left a 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.

@paracycle paracycle added the enhancement New feature or request label Feb 11, 2022
@jeffcarbs jeffcarbs force-pushed the bug-drop-absolute-namespace branch from e6e7e6f to f2d66a8 Compare February 11, 2022 21:56
@jeffcarbs
Copy link
Contributor Author

@paracycle - Thanks for taking a look! I made the delete_prefix change you suggested and fixed a new merge conflict that had come up. Build is green so should be good to go :shipit:

@paracycle paracycle force-pushed the bug-drop-absolute-namespace branch from f2d66a8 to 69a8fb5 Compare February 12, 2022 12:40
@paracycle paracycle force-pushed the bug-drop-absolute-namespace branch from 69a8fb5 to 9e6d36e Compare February 12, 2022 12:55
@paracycle paracycle merged commit 3ff5201 into Shopify:main Feb 12, 2022
@paracycle
Copy link
Member

Tested this on Shopify Core and it seems to cause no issues. Thanks for pushing this forward 🙏

@paracycle paracycle mentioned this pull request Feb 12, 2022
@jeffcarbs jeffcarbs deleted the bug-drop-absolute-namespace branch February 14, 2022 03:49
@shopify-shipit shopify-shipit bot temporarily deployed to production February 17, 2022 22:56 Inactive
@shopify-shipit shopify-shipit bot temporarily deployed to 0-7-stable March 29, 2022 19:23 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants