-
Notifications
You must be signed in to change notification settings - Fork 27
Fix invalid sig generated for anonymous block param (&) #935
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1 @@ | ||
| 4.0.0 | ||
| 3.4.4 |
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -890,6 +890,45 @@ def rbs_comments_to_sorbet_sigs(ruby_contents, max_line_length: nil, overloads_s | |||||||||
| overloads_strategy: overloads_strategy, | ||||||||||
| ).rewrite | ||||||||||
| end | ||||||||||
|
|
||||||||||
| def test_rbs_comments_to_sorbet_sigs_anonymous_block_param | ||||||||||
| res = Spoom::Sorbet::Translate.rbs_comments_to_sorbet_sigs(<<~RUBY, file: "test.rb") | ||||||||||
| # typed: true | ||||||||||
| class Foo | ||||||||||
| #: (String) ?{ (String) -> void } -> String | ||||||||||
| def bar(request, &); end | ||||||||||
| end | ||||||||||
| RUBY | ||||||||||
|
|
||||||||||
| assert_equal(<<~RUBY, res) | ||||||||||
| # typed: true | ||||||||||
| class Foo | ||||||||||
| sig { params(request: String, block: ::T.nilable(::T.proc.params(arg0: String).void)).returns(String) } | ||||||||||
| def bar(request, &); end | ||||||||||
|
Comment on lines
+906
to
+907
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think this generated code is correct either: https://sorbet.run/#%23%20typed%3A%20true%0Aclass%20Foo%0A%20%20extend%20T%3A%3ASig%0A%0A%20%20sig%20%7B%20params%28request%3A%20String%2C%20block%3A%20%3A%3AT.nilable%28%3A%3AT.proc.params%28arg0%3A%20String%29.void%29%29.void%20%7D%0A%20%20def%20bar%28request%2C%20%26%29%3B%20nil%3B%20end%0Aend I think it should be:
Suggested change
|
||||||||||
| end | ||||||||||
| RUBY | ||||||||||
|
|
||||||||||
| # Must also be valid Ruby | ||||||||||
| assert RubyVM::InstructionSequence.compile(res) | ||||||||||
|
Comment on lines
+910
to
+912
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is overkill, we don't need to waste time compiling it. Perhaps
Suggested change
|
||||||||||
| end | ||||||||||
|
|
||||||||||
| def test_rbs_comments_to_sorbet_sigs_named_block_param_unchanged | ||||||||||
| res = Spoom::Sorbet::Translate.rbs_comments_to_sorbet_sigs(<<~RUBY, file: "test.rb") | ||||||||||
| # typed: true | ||||||||||
| class Foo | ||||||||||
| #: (String) ?{ (String) -> void } -> String | ||||||||||
| def bar(request, &block); end | ||||||||||
| end | ||||||||||
| RUBY | ||||||||||
|
|
||||||||||
| assert_equal(<<~RUBY, res) | ||||||||||
| # typed: true | ||||||||||
| class Foo | ||||||||||
| sig { params(request: String, block: ::T.nilable(::T.proc.params(arg0: String).void)).returns(String) } | ||||||||||
| def bar(request, &block); end | ||||||||||
| end | ||||||||||
| RUBY | ||||||||||
| end | ||||||||||
| end | ||||||||||
| end | ||||||||||
| end | ||||||||||
|
|
||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if this is the correct place to fix this problem. We need to solve it at the source.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah the correct place to fix this is in the RBI gem that Spoom uses for this translation:
https://github.com/Shopify/rbi/blob/4a4f30b9ca743f7723ce451dc06a9b7cf62a42b0/lib/rbi/rbs/method_type_translator.rb#L46