Fix method signatures being overwritten by prepended modules - #2718
DaneHarrison wants to merge 4 commits into
Conversation
|
I have signed the CLA! |
There was a problem hiding this comment.
I tried to do this for DSL generation but it was too complex. I think this is acceptable.
| return unless method | ||
| return unless method_owned_by_constant?(method, constant) | ||
|
|
||
| method = method_defined_by_constant(method, constant) |
There was a problem hiding this comment.
Agent brought up that the method visibility can also be incorrectly attributed to the prepended method, could you add a test and a fix for this?
it "uses the constant's own method visibility instead of the prepended method's visibility" do
add_ruby_file("foo.rb", <<~RUBY)
module Foo
def bar(x); end
end
class Baz
prepend Foo
private
def bar; end
end
RUBY
output = template(<<~RBI)
class Baz
include ::Foo
private
def bar; end
end
module Foo
def bar(x); end
end
RBI
assert_equal(output, compile)
end| it "compiles each method with its own sig when both the class and the prepended module have one" do | ||
| # Sorbet files the class's sig under the prepended module's method, which is also where the module's | ||
| # own sig is filed, so whichever sig is evaluated last overwrites the other one. | ||
| skip "Sorbet keeps only one of the two signatures" |
There was a problem hiding this comment.
I'm in support of removing this test, I don't see it being changed in the future.
| return unless method_owned_by_constant?(reader_method, constant) | ||
|
|
||
| reader_method | ||
| method_defined_by_constant(reader_method, constant) |
There was a problem hiding this comment.
Is this second call necessary?
There was a problem hiding this comment.
I forgot to remove it after changing original_method. Fixed and good catch
| assert_equal(output, compile) | ||
| end | ||
|
|
||
| it "compiles a method using its own signature, not the signature of a module prepended in front of it" do |
There was a problem hiding this comment.
This and the next test shouldn't say "signature" IMO since it's not Sorbet signatures. Maybe method definition?
| assert_equal(output, compile) | ||
| end | ||
|
|
||
| it "compiles a method with a sig using its own signature when the prepended module has different parameters" do |
There was a problem hiding this comment.
Could you add a singleton equivalent for this?
it "compiles a singleton method using its own signature through a prepended module" do
add_ruby_file("foo.rb", <<~RUBY)
module Wrapper
def bar(x, y); end
end
class Baz
extend T::Sig
sig { params(x: Integer).returns(Integer) }
def self.bar(x)
x
end
singleton_class.prepend(Wrapper)
end
RUBY
output = template(<<~RBI)
class Baz
extend ::Wrapper
class << self
sig { params(x: ::Integer).returns(::Integer) }
def bar(x); end
end
end
module Wrapper
def bar(x, y); end
end
RBI
assert_equal(output, compile)
end…pends - Take method visibility from the constant's own method instead of a prepended module's method - Rename tests that compare method definitions, not Sorbet signatures - Add a test for a singleton method behind a prepended module - Remove the skipped test for a class and prepended module both having sigs - Drop a redundant method_defined_by_constant call in attr_reader_for_writer
|
Thank you for the review! I addressed all comments in my last commit. |
Motivation
Fixes #2072 - When a module is prepended onto a class, Tapioca generates the RBI using the prepended module's signature instead of its own.
Implementation
compile_methodreceives methods obtained withModule#instance_method, which follows the ancestor chain. When amodule is prepended to a class and defines the method,
instance_methodreturns the prepended module'sUnboundMethodrather than the class's own, socompile_methodwas compiling the wrong method.method_owned_by_constant?becomesmethod_defined_by_constant. It walkssuper_methodand returns the method the constant itself defines.New
signature_defined_by_constantfinds the sig declared on the constant's own method. Sorbet can file it under a prepended method instead, so it checks those too and only accepts a sig whosesignature.method.ownermatches.The attr_accessor writer inference resolves the constant's own reader first, so it works in either evaluation order.
visibility_defined_by_constantgets the visibility from the constant's own method, so a prepended module's visibility doesn't leak into what we pass tocompile_method.Further Considerations:
signature_ofi.e underdsl, I suspect those will need to be updated as wellTests