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

Skip compiling constants with alias from another gem in namespace #1756

Merged
merged 1 commit into from
Jan 12, 2024

Conversation

egiurleo
Copy link
Contributor

@egiurleo egiurleo commented Jan 10, 2024

Motivation

When compiling a gem, it is possible for the gem to define an alias that includes an alias from another gem in its namespace.

For example, let's say we have gems foo and bar:

# foo.rb
module Foo; end
F = Foo

# bar.rb
module Foo
  module Bar; end
end
F::B = Foo::Bar

Then the RBIs might look something like this:

# foo.rbi
module Foo; end
F = Foo

# bar.rbi
module Foo::Bar; end
F::B = Foo::Bar
Foo::B = Foo::Bar

There are two issues with this RBI:

  1. There will be a Sorbet error because the alias F is defined in the RBI for both gems (once in the line F = Foo and another time in the line F::B = Foo::Bar)
  2. The alias for Foo::Bar is defined twice in bar.rbi

This commit will cause the RBI for gem bar to skip over the line F::B = Foo::Bar, which will prevent the alias F from being defined twice in RBI.

More Context

I found this issue when trying to generate RBI for the eventmachine and em-synchrony gems. eventmachine defines an alias EM = EventMachine, and em-synchrony defines another alias EM::S = EventMachine::Synchrony. The resulting RBI files cause a Sorbet error because EM is defined twice.

Implementation

I implemented this change by adding an additional check to the skip_alias? method that determines whether the namespace of a constant contains an alias and skips compiling that constant if so.

Tests

I added one test that fails without this change and passes with it. All other tests continue to pass. I have tested this in Shopify core and verified no new Sorbet errors.

@egiurleo egiurleo self-assigned this Jan 10, 2024
@egiurleo egiurleo marked this pull request as ready for review January 10, 2024 22:00
@egiurleo egiurleo requested a review from a team as a code owner January 10, 2024 22:00
@egiurleo egiurleo marked this pull request as draft January 10, 2024 22:09
lib/tapioca/runtime/reflection.rb Outdated Show resolved Hide resolved
lib/tapioca/runtime/reflection.rb Outdated Show resolved Hide resolved
lib/tapioca/runtime/reflection.rb Outdated Show resolved Hide resolved
spec/tapioca/cli/gem_spec.rb Outdated Show resolved Hide resolved
@egiurleo egiurleo changed the title Skip compiling constants with alias in namespace Skip compiling constants with alias from another gem in namespace Jan 12, 2024
@egiurleo egiurleo force-pushed the emily/skip-alias-in-namespace branch from 493d965 to e34c655 Compare January 12, 2024 18:47
@egiurleo egiurleo marked this pull request as ready for review January 12, 2024 18:48
@egiurleo egiurleo requested a review from Morriar January 12, 2024 18:48
Comment on lines 267 to 272
constant = constantize(namespace)
next unless Module === constant

name_of(constant) != namespace
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we extract this as a private constant_name_differs_from_actual_namespace method to facilitate reading? Maybe just adding a comment in the block would help.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think of the comment I added?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect 👌

When compiling a gem, it is possible for the gem to define an alias that
includes an alias from another gem in its namespace. For example:

In gem foo:
```
module Foo; end
F = Foo
```

In gem bar:
```
module Foo
  module Bar; end
end

F::B = Foo::Bar
```

Because the alias `F` is not originally defined in the gem `bar`,
tapioca will not skip writing that alias to the RBI file of `bar`, which
will cause a Sorbet error because the alias `F` will be defined in the
RBI files for both `foo` and `bar`.

Now, tapioca will not generate RBI for aliases originally defined in
other gems.
@egiurleo egiurleo force-pushed the emily/skip-alias-in-namespace branch from e34c655 to 9cc3977 Compare January 12, 2024 19:14
@egiurleo egiurleo merged commit aeffcdb into main Jan 12, 2024
33 checks passed
@egiurleo egiurleo deleted the emily/skip-alias-in-namespace branch January 12, 2024 21:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants