Skip to content
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

Gems: Fix gem RBI compilation for aliased methods where the original method has a sig #2051

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Gems: Fix gem RBI compilation for aliased methods where the original method has a sig #2051

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cd6ee9c
Add test for method aliases in gems
marknuzz Oct 18, 2024
e259339
Fix method aliases in gems
marknuzz Oct 18, 2024
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
18 changes: 16 additions & 2 deletions lib/tapioca/gem/listeners/methods.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,15 +73,29 @@ def compile_method(tree, symbol_name, constant, method, visibility = RBI::Public
return unless method_owned_by_constant?(method, constant)
return if @pipeline.symbol_in_payload?(symbol_name) && [email protected]_in_gem?(method)

is_alias = method.name != method.original_name
signature = lookup_signature_of(method)
method = T.let(signature.method, UnboundMethod) if signature
parameters = T.let(nil, T.nilable(T::Array[[Symbol, T.nilable(Symbol)]]))

if signature
method = T.let(signature.method, UnboundMethod)
elsif is_alias && signature.nil? && constant.method_defined?(method.original_name)
alias_source_method = constant.instance_method(method.original_name)
Copy link
Member

@paracycle paracycle Dec 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

In general, this is incorrect since alias binding happens at call site and doesn't get mutated. See: https://bsky.app/profile/fxn.bsky.social/post/3lbehp5rtes2j

class A
  def foo = 42

  alias :bar :foo
end

class B < A
  def foo = 1
end

B.new.foo # => 1
B.new.bar # => 42

That is: The instance method bar of B is not an alias of instance method foo of B, but this line of code would treat them as such.

Copy link
Contributor Author

@marknuzz marknuzz Dec 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

That's a good find, but I think at worst, wouldn't it be the case that we have a signature that is valid for both A and B, where the alternative is no signature at all? We can redefine a method, but not with an incompatible signature.

I imagine there are edge cases where this is done, but I am not sure if it is ever correct to do so. Though there may be other ways to redefine methods that are affected by this.

Since I had not run into a situation in any the gems I ever used in practice, where this PR would affect any of the RBI output for those gems, maybe it is just not worth the effort to get it just right (I can't easily think of how to address this in a simple way, though I'm sure a solution is possible). So I'm fine with closing this as there doesn't seem to be much demand for this to work :).

signature = lookup_signature_of(alias_source_method)

# Skip abstract methods if they are defined this way.
# Aliasing abstract methods is likely to yield unwanted results
signature = nil if signature&.mode == "abstract"
parameters = T.let(signature&.method&.parameters, T.nilable(T::Array[[Symbol, T.nilable(Symbol)]]))
KaanOzkan marked this conversation as resolved.
Show resolved Hide resolved
end

method_name = method.name.to_s
return unless valid_method_name?(method_name)
return if struct_method?(constant, method_name)
return if method_name.start_with?("__t_props_generated_")

parameters = T.let(method.parameters, T::Array[[Symbol, T.nilable(Symbol)]])
method = T.let(signature.method, UnboundMethod) if signature
Copy link
Contributor

@KaanOzkan KaanOzkan Oct 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Why was this moved after the checks on line 92? Does it matter for aliased methods?

I think it'll be a bit cleaner if it was done in the elsif instead.
We could remove the if signature completely and have this assignment after the elsif.

Copy link
Contributor Author

@marknuzz marknuzz Oct 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I agree its a bit complex but wasn't sure if it would be appropriate to refactor it to make it more readable. Would that be appropriate here?

If you move this to line 91, signature could be nil and you get a crash
method = T.let(signature.method, UnboundMethod)

If you leave if signature but move it to line 91, you get duplicate rbi defs for the original method, and the alias method doesn't get the rbi def.

The checks would be done on the original method name and not the alias method name if we replace the method and assign method_name before the checks, because line 97 sets it to the original method. I think the problem here is that the meaning of method is ambiguous between the original method and the alias method - that should at least be fixed if nothing else, but let me know your thoughts

otherwise I think it is correct as is but not as readable as it could be

I'll update the test to ensure that attempting to alias to a name like __t_props_generated_foo will not produce rbi for that method as well

Copy link
Contributor

@KaanOzkan KaanOzkan Oct 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry I wasn't clear in my original message, I wanted to get rid of duplicate method = T.let(signature.method, UnboundMethod) if signature in case of no alias methods (lines 81 and 97), but yes I see the need now.

It might be good enough to just have a comment on line 97 mentioning that it's only for the alias case and that after running checks on the original method we are setting it to the aliased method for generation. But feel free to refactor if there are opportunities.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, will get to this soon

parameters ||= T.let(method.parameters, T::Array[[Symbol, T.nilable(Symbol)]])

sanitized_parameters = parameters.each_with_index.map do |(type, name), index|
fallback_arg_name = "_arg#{index}"
Expand Down
32 changes: 32 additions & 0 deletions spec/tapioca/gem/pipeline_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3181,6 +3181,23 @@ module ClassMethodsWithVariance
end
RUBY

add_ruby_file("method_aliases.rb", <<~RUBY)
class MethodAliases
extend T::Helpers
extend T::Sig

# Foo
sig { params(a: Integer, b: String).returns(Object) }
def foo(a, b); end

alias :bar :foo
alias_method :baz, :bar

def fuzz(x); end
alias_method :buzz, :fuzz
end
RUBY

add_ruby_file("generic.rb", <<~RUBY)
module Generics
class ComplexGenericType
Expand Down Expand Up @@ -3389,6 +3406,21 @@ def something(foo); end

Generics::SimpleGenericType::NullGenericType = T.let(T.unsafe(nil), Generics::SimpleGenericType[::Integer])

class MethodAliases
sig { params(a: ::Integer, b: ::String).returns(::Object) }
def bar(a, b); end

sig { params(a: ::Integer, b: ::String).returns(::Object) }
def baz(a, b); end

def buzz(x); end

sig { params(a: ::Integer, b: ::String).returns(::Object) }
def foo(a, b); end

def fuzz(x); end
end

module Quux
interface!

Expand Down
Loading