Skip to content

Stop ignoring symbols in payload when discovering subconstants #2203

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Feb 14, 2025
Merged

Stop ignoring symbols in payload when discovering subconstants #2203

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ecfb0c7
Stop ignoring symbols in payload when discovering subconstants
paracycle Feb 13, 2025
237189f
Start using a newer version of json with the subconstant problem
paracycle Feb 13, 2025
21dd01c
Bump gem RBI files
paracycle Feb 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Gemfile.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,7 @@ GEM
pp (>= 0.6.0)
rdoc (>= 4.0.0)
reline (>= 0.4.2)
json (2.9.1)
json (2.10.1)
json_api_client (1.23.0)
activemodel (>= 6.0.0)
activesupport (>= 6.0.0)
Expand Down
3 changes: 1 addition & 2 deletions lib/tapioca/gem/listeners/subconstants.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,10 @@ class Subconstants < Base
sig { override.params(event: ScopeNodeAdded).void }
def on_scope(event)
symbol = event.symbol
return if @pipeline.symbol_in_payload?(symbol) && event.node.empty?
Copy link
Collaborator

Choose a reason for hiding this comment

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

Didn't we do this to avoid polluting gem RBIs with empty redefinitions of stdlib classes? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

That's what I was remembering too, but then I realized that this is just the listener that is responsible for discovering subconstants and pushing them into the pipeline. Skipping empty scopes of symbols that are in the payload shouldn't be the responsibility of this step.

I will check this against Core as well, but I think, as we improve the fidelity of the gem attribution of mixins and methods, this will be less and less relevant.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If you figure this out and we do need this check, please add a comment explaining why we skip 😂

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see any empty definitions in core. I'm just seeing a 10s(~5%) increase in gem --all time, not too bad.

constant = event.constant

prefix = symbol == "Object" ? "" : symbol

constant = event.constant
constants_of(constant).sort.uniq.map do |constant_name|
name = "#{prefix}::#{constant_name}"
subconstant = constantize(name)
Expand Down
Loading
Loading