Stop ignoring symbols in payload when discovering subconstants #2203
Conversation
|
Something is wrong with CI, it didn't trigger. |
| @@ -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? | |||
There was a problem hiding this comment.
Didn't we do this to avoid polluting gem RBIs with empty redefinitions of stdlib classes? 🤔
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
If you figure this out and we do need this check, please add a comment explaining why we skip 😂
There was a problem hiding this comment.
I don't see any empty definitions in core. I'm just seeing a 10s(~5%) increase in gem --all time, not too bad.
Yeah, I noticed that too. |
This change makes Tapioca consider subconstants of symbols that may already be defined in the payload, ensuring that all relevant subconstants are discovered and processed correctly. This prevents a case where a new subconstant is defined in a default gem that doesn't get picked up by the payload yet, causing Tapioca to miss it and not generate the corresponding RBI file.
paracycle
force-pushed
the
uk-consider-more-subconstants
branch
from
1aa759f to
21dd01c
Compare
|
@KaanOzkan I rebased on |
|
Ok, the CI is green on this PR both in here and in Core. |
Motivation
This would have prevented the recent problem that folks ran into with
JSON::Ext::ParserConfigsymbol added tojsongem in version 2.10.0Implementation
Since
JSON::Extwas a constant defined in the payload, and because its scope definition was empty, we never never considering its subconstants, so missing constants likeJSON::Ext::ParserConfig.By removing the check, we should be able to generate more accurate RBI files, that add the constants that might be missing from payload definitions.
Tests
No new tests, existing tests should pass. I have no idea how I can test this, since it is so coupled with what's defined in the payload.