Gems: Fix gem RBI compilation for aliased methods where the original method has a sig #2051
Conversation
|
|
||
| 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
ok, will get to this soon
| 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) |
There was a problem hiding this comment.
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 # => 42That 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.
There was a problem hiding this comment.
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 :).
Motivation
I was working on porting a small gem to use Sorbet, with sigs on all of the methods defined within the gem. I noticed that the generated RBI from tapioca gem did not produce the correct output for the methods which were aliased. The gem contained a couple of aliased methods. It seemed worth looking into so I gave it a shot, and was able to fix it.
Implementation
I made sure to play it safe, by confirming that it is an alias method, and that the original method is a valid method that can be referenced. We use the signature for the original method instead of the alias method, and only if the alias method has no signature directly available (which may be true for 100% of alias methods, but I didn't want to assume).
sorbet-runtime does have a way to internally build the signature of an aliased method, but it proved to be too deep in the private namespace and I abandoned that idea. If you actually call the aliased method, not once, but twice, the method will be unwrapped and you can get the signature of the aliased method through sorbet's API. But I couldn't identify any way to trigger that without calling the method, so we just use the signature of the original method if it is available.
If it is an abstract method, we set the aliased signature to nil (as it already would have been before this change), as unintended effects may occur from an aliasing an abstract method. I'm not sure if anyone does it, but in case they do, we don't want to cause a breaking change for them.
Tests
Added unit tests, and tested the gems command (with this fix) on all of the gems I had installed locally. Gems typically don't use signatures in methods, and of those that do, fewer will alias them. So I could only trigger this for my own gem I was working on locally. However, this may have more of a noticeable effect for companies that are using private gems internally.