Fix Reflection#inherited_ancestors_of method for modules
#1530
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We used to return the ancestors of the Module class itself, as inherited ancestors for modules, but that was wrong. We should return the ancestors of an empty module (i.e. `Module.new`) instead.
bitwise-aiden
approved these changes
Jun 12, 2023
Morriar
pushed a commit
that referenced
this pull request
Jul 12, 2023
Since #1530, inclusion of Kernel is properly generated by Tapioca. When generating RBIs, this creates an issue for Isolation modules where the `requires_ancestor { Kernel }` on `DslCompiler` is reported as useless. we do not have this error locally in Tapioca because the include is conditional so Sorbet doesn't see it. But we get it with RBIs in other projects because Tapioca finds the include through reflection. The Isolation sub-modules do not need to include Kernel. It seems required only to call fork and exit so we can replace them by `requires_ancestor { Kernel }`. Closes #1563. Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
As reported by @searls on the Sorbet Slack, Tapioca would remove
include Kernellines from a module when generating the RBI file for the gem.Implementation
It turns out the problem stems from the implementation of
Reflection#inherited_ancestors_ofmethod doing the wrong thing for modules. For classes we look at the ancestors of the class' superclass (orObjectif no explicit superclass is set), but for modules we used to look at the ancestors ofModule. However, a module is an instance ofModule, so the ancestors ofModulewould not be in the original module's ancestor list anyway. What we needed to check was the ancestors of a newly createdModuleinstance, and return those.This ends up affecting only
Kernelmixed into a module, sinceModule's ancestors only hasKernelas a mixin, all the other ancestors are classes likeObjectandBasicObject. That's probably why this behaviour was not caught earlier.Tests
Added a test for module mixins that exhibits the wrong behaviour and passes after the change.