More effectively determine method ownership for RBI generation #896
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
vinistock
reviewed
Apr 12, 2022
lib/tapioca/gem/listeners/methods.rb
Outdated
| # Widen the type of `method` to be nilable | ||
| method = T.let(method, T.nilable(UnboundMethod)) | ||
|
|
||
| loop do |
There was a problem hiding this comment.
Would while method here work and allow us to remove the break?
Morriar
reviewed
Apr 12, 2022
lib/tapioca/gem/listeners/methods.rb
Outdated
| # | ||
| # This method implements a better way of checking whether a constant defines a method. | ||
| # It walks up the ancestor tree via the `super_method` method; if any of the super | ||
| # methods are owned by the constant, it means that the constant declares the method. |
There was a problem hiding this comment.
Could we have this comment on top of the method as documentation rather than inside?
| add_ruby_file("foo.rb", <<~RUBY) | ||
| module Foo | ||
| def bar | ||
| super |
There was a problem hiding this comment.
Can we add an example that does not call super? 🙏
paracycle
reviewed
Apr 13, 2022
lib/tapioca/gem/listeners/methods.rb
Outdated
| @@ -66,7 +66,7 @@ def compile_directly_owned_methods(tree, module_name, mod, for_visibility = [:pu | |||
| end | |||
| def compile_method(tree, symbol_name, constant, method, visibility = RBI::Public.new) | |||
| return unless method | |||
| return unless method.owner == constant | |||
| return unless method_defined_by_constant?(method, constant) | |||
There was a problem hiding this comment.
I know that I had named this method but looking at it again, it feels like method_owned_by_constant? would be a more appropriate name, since we are doing ownerships checks.
egiurleo
force-pushed
the
method-defined-by-constant
branch
from
5b4bf06
to
a0ab5d9
Compare
vinistock
approved these changes
Apr 13, 2022
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
This issue was discovered by @paracycle while investigating #890, but it is technically unrelated to mixin generation.
While generating RBIs for a method, tapioca implements a check to determine which constant "owns" or defines that method. We only want to generate RBIs for a method on the module or class that defines it, not any that just happen to inherit it.
Historically, we have been checking the
method.ownerand verifying that it matches the class/module for which we are generating RBIs. However, this check misses one particular case: when a method is also defined on a prepended module, it will always appear as if it belongs to the prepended module, even when it is also defined on the class that prepends the module.This means that tapioca will not generate RBI for that method on the class that defines it.
Implementation
This code was originally written by @paracycle on commit 80faabc. I've just cleaned it up and moved it to the correct spot.
Instead of just checking the method owner, this implementation walks up the ancestor chain via the
super_methodmethod, checking if any of the super methods are owned by the constant. If that is the case, that means the constant also defines the method, and we should generate RBIs for this method on the constant.Tests
I have added a test which fails without the changes but passes with the changes included.
Tophatting
I tested this change by generating gem and dsl RBIs on Shopify core using this branch of tapioca and verifying that the change did not introduce any new typing issues.