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

Properly attribute RBIs created by ActiveSupport.on_load #2050

Merged
merged 2 commits into from
Oct 30, 2024

Conversation

egiurleo
Copy link
Contributor

@egiurleo egiurleo commented Oct 17, 2024

Closes #2016
Closes #2048

Motivation

Currently, Tapioca is incorrectly attributing mixins performed by the ActiveSupport.on_load hooks.

A quick summary of how things currently work: whenever a mixin (include, extend, prepend) is performed by a gem for which Tapioca is generating RBIs, we attribute that mixin to a specific location by looking at the backtrace of the call to include/extend/prepend and finding the line that is tagged <top (required)> (or <main> if there is no other tag). This location may be in the gem code, or it may be in the code of another gem entirely.

Then, we can filter those mixins based on where they happen -- only the mixins attributed to a gem get put in that gem's RBI.

This approach does not work when a mixin is performed based on an ActiveSupport.on_load hook because the location that is tagged <top (required)> will be in Rails gem that is being hooked into, rather than in the gem that is performing the on_load hook in the first place.

Implementation

To fix this, we need to change how we choose the location for mixin attribution. By searching for locations tagged with block in <class:..., we can identify locations that trigger a mixin based on a loading hook.

Tests

See automated tests

When flipper/flipper-active_record version 1.3.1 is included
in the Gemfile, Flipper-related code gets added to the
activerecord gemfile.
@egiurleo egiurleo changed the title Emily/flipper bug Properly attribute RBIs created by ActiveSupport.on_load Oct 17, 2024
…hooks

We are mistakenly attributing code that is loaded by ActiveSupport.on_load
to Rails, rather than whatever gem performs the hook.
@egiurleo egiurleo marked this pull request as ready for review October 21, 2024 17:45
@egiurleo egiurleo requested a review from a team as a code owner October 21, 2024 17:45
@amomchilov amomchilov self-assigned this Oct 21, 2024
Copy link
Contributor

@KaanOzkan KaanOzkan left a comment

Choose a reason for hiding this comment

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

Can you update the PR description with the new implementation?

Copy link
Contributor

@amomchilov amomchilov left a comment

Choose a reason for hiding this comment

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

Nice find. Clean solution, LGTM.

Is the next step here for me to try running this against core?

Copy link
Contributor

@lavoiesl lavoiesl left a comment

Choose a reason for hiding this comment

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

🎩 This PR fixed my issue in #2048 👍🏼

@egiurleo
Copy link
Contributor Author

@KaanOzkan updated! Thanks for the reminder.

@amomchilov Yes that would be awesome, thank you!

@KaanOzkan KaanOzkan merged commit 68bfb68 into main Oct 30, 2024
28 checks passed
@KaanOzkan KaanOzkan deleted the emily/flipper-bug branch October 30, 2024 17:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants