Parse RBS overloads into signatures #2243

andyw8 · 2024-07-02T19:28:30Z

Motivation

In this PR we are beginning fuller indexing of RBS, by handline multiple signatures per method.

Implementation

Previously, arguments was a field of Entry::Method. Now an Entry::Method has a signatures field (of type array), and each signature has the array of arguments.

Not covered yet:

Return types
Individual block arguments

Automated Tests

The tests are mostly using a selection of methods from Ruby core to verify the various forms of signatures in use.

Manual Tests

There is no integration with the LSP features yet so this cannot be easily manually tested.

andyw8 · 2024-07-04T14:23:42Z

(have moved the Ruby LSP changes out to andyw8/support-multiple-signatures)

andyw8 · 2024-07-04T17:12:09Z

@soutaro this is still WIP, but thought you may be interested to see our approach.

andyw8 · 2024-07-05T19:35:40Z

lib/ruby_indexer/test/rbs_indexer_test.rb

+      # There are no methods in Core that have required keyword arguments,
+      # so we test against RBS directly
+
+      # TODO: why does removing the block break this?


Need to figure out why

def foo: (::Numeric a, ::Numeric b) -> self

is failing with:

Minitest::Assertion: Expected #<RubyIndexer::Entry::RequiredParameter:0x0000000129a54158 @name=:a> to be a kind of RubyIndexer::Entry::KeywordParameter, not RubyIndexer::Entry::RequiredParameter.

andyw8 · 2024-07-05T19:41:02Z

lib/ruby_indexer/test/method_test.rb

-      assert_equal(1, entry.parameters.length)
-      parameter = entry.parameters.first
+      parameters = entry.signatures.first.parameters
+      assert_equal(1, parameters.length)


These kind of length assertions were useful during development, but we could remove them to make the tests a little shorter.

andyw8 · 2024-07-05T19:41:42Z

lib/ruby_indexer/test/rbs_indexer_test.rb

+
+      first_signature = signatures.first
+
+      # (::string salt_str) -> ::String


I copied in the RBS definitions for easy reference, but we could remove them.

I like those, it's easier to understand 👍

andyw8 · 2024-07-05T19:42:43Z

lib/ruby_indexer/test/rbs_indexer_test.rb

+    end
+
+    def test_rbs_method_with_optional_keywords
+      entries = @index["step"] # https://www.rubydoc.info/stdlib/core/Numeric#step-instance_method


step is an interesting one since it can called with either positional or keyword args.

andyw8 · 2024-07-05T19:43:53Z

There's one thing I need to fix but I'd like to start getting reviews on this.

vinistock

This is great

lib/ruby_indexer/lib/ruby_indexer/rbs_indexer.rb

vinistock · 2024-07-05T20:50:02Z

lib/ruby_indexer/lib/ruby_indexer/rbs_indexer.rb

+    end
+
+    sig { params(function: RBS::Types::Function).returns(T::Array[Entry::OptionalParameter]) }
+    def process_trailing_positionals(function)


I'll defer to you if you prefer this style, but I'm not a fan of this tiny methods that just map through one parameter type. I would just inline them all.

I do generally prefer this way, I picked it from the guidance in Clean Code that methods should have only one level of abstraction.

lib/ruby_indexer/test/rbs_indexer_test.rb

vinistock · 2024-07-05T20:51:07Z

lib/ruby_indexer/test/rbs_indexer_test.rb

+
+      first_signature = signatures.first
+
+      # (::string salt_str) -> ::String


I like those, it's easier to understand 👍

lib/ruby_indexer/test/rbs_indexer_test.rb

vinistock · 2024-07-08T20:21:47Z

lib/ruby_indexer/lib/ruby_indexer/rbs_indexer.rb

+      function.required_positionals.map do |param|
+        # Some parameters don't have names, e.g.
+        #   def self.try_convert: [U] (untyped) -> ::Array[U]?
+        name = param.name || :object


Let's also use arg{index} here instead of just object.

vinistock · 2024-07-08T20:22:49Z

lib/ruby_indexer/lib/ruby_indexer/rbs_indexer.rb

+
+      name = param.name || Entry::KeywordRestParameter::DEFAULT_NAME
+
+      [Entry::KeywordRestParameter.new(name: name)]


I'm disliking this pattern of tiny methods more and more. Can we at least redesign this so that we are not forced to create arrays just to concatenate in the call site?

vinistock · 2024-07-08T20:23:19Z

lib/ruby_indexer/lib/ruby_indexer/rbs_indexer.rb

+    sig { params(function: RBS::Types::Function).returns(T::Array[Entry::Parameter]) }
+    def parse_arguments(function)
+      parameters = []
+      parameters += process_required_positionals(function) if function.required_positionals


All of these += will generate intermediate arrays. Let's switch them all for concat.

andyw8 · 2024-07-09T14:03:59Z

lib/ruby_indexer/lib/ruby_indexer/rbs_indexer.rb

+    end
+
+    sig { params(function: RBS::Types::Function).returns(T::Array[Entry::RequiredParameter]) }
+    def process_required_and_optional_positionals(function)


This was prevously two methods, I combined them because we need a way to keep track of an index to use for arg0 etc, and this seemed the simplest way.

andyw8 · 2024-07-09T14:04:46Z

lib/ruby_indexer/lib/ruby_indexer/rbs_indexer.rb

+    def process_required_and_optional_positionals(function)
+      argument_offset = 0
+
+      required = function.required_positionals.map.with_index(argument_offset) do |param, i|


argument_offset would default to 0 here, but I added the argument for symmetry with the other loop.

andyw8 mentioned this pull request Jul 2, 2024

WIP: Index method arguments using RBS #2200

Closed

1 task

andyw8 force-pushed the andyw8/parse-rbs-overloads-into-signatures branch 2 times, most recently from ea7c625 to c9a3dca Compare July 3, 2024 14:53

andyw8 added enhancement New feature or request server This pull request should be included in the server gem's release notes labels Jul 3, 2024

andyw8 mentioned this pull request Jul 3, 2024

Fix display of label details for completion requests #2253

Merged

andyw8 force-pushed the andyw8/parse-rbs-overloads-into-signatures branch from 2f4c24e to 640f9fc Compare July 4, 2024 14:23

andyw8 force-pushed the andyw8/parse-rbs-overloads-into-signatures branch from 640f9fc to cd5f555 Compare July 4, 2024 14:26

andyw8 force-pushed the andyw8/parse-rbs-overloads-into-signatures branch 2 times, most recently from bfaca26 to 4a2b446 Compare July 5, 2024 19:32

andyw8 commented Jul 5, 2024

View reviewed changes

andyw8 changed the title ~~WIP: Parse RBS overloads into signatures~~ Parse RBS overloads into signatures Jul 5, 2024

andyw8 force-pushed the andyw8/parse-rbs-overloads-into-signatures branch from 4a2b446 to 4b683e4 Compare July 5, 2024 19:36

andyw8 commented Jul 5, 2024

View reviewed changes

andyw8 marked this pull request as ready for review July 5, 2024 19:44

andyw8 requested a review from a team as a code owner July 5, 2024 19:44

andyw8 requested review from st0012 and vinistock July 5, 2024 19:44

vinistock reviewed Jul 5, 2024

View reviewed changes

vinistock reviewed Jul 8, 2024

View reviewed changes

andyw8 commented Jul 9, 2024

View reviewed changes

vinistock approved these changes Jul 9, 2024

View reviewed changes

Parse RBS overloads into signatures

c272cf4

andyw8 force-pushed the andyw8/parse-rbs-overloads-into-signatures branch from 2a377fa to c272cf4 Compare July 9, 2024 19:51

andyw8 enabled auto-merge (squash) July 9, 2024 19:52

andyw8 merged commit e27dec2 into main Jul 9, 2024
33 checks passed

andyw8 deleted the andyw8/parse-rbs-overloads-into-signatures branch July 9, 2024 20:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parse RBS overloads into signatures #2243

Parse RBS overloads into signatures #2243

andyw8 commented Jul 2, 2024 •

edited

Loading

andyw8 commented Jul 4, 2024

andyw8 commented Jul 4, 2024

andyw8 Jul 5, 2024

andyw8 Jul 5, 2024 •

edited

Loading

andyw8 Jul 5, 2024

vinistock Jul 5, 2024

andyw8 Jul 5, 2024

andyw8 commented Jul 5, 2024

vinistock left a comment

vinistock Jul 5, 2024

andyw8 Jul 9, 2024 •

edited

Loading

vinistock Jul 5, 2024

vinistock Jul 8, 2024

vinistock Jul 8, 2024

vinistock Jul 8, 2024

andyw8 Jul 9, 2024

andyw8 Jul 9, 2024


		first_signature = signatures.first

		# (::string salt_str) -> ::String


		name = param.name \|\| Entry::KeywordRestParameter::DEFAULT_NAME

		[Entry::KeywordRestParameter.new(name: name)]

Parse RBS overloads into signatures #2243

Parse RBS overloads into signatures #2243

Conversation

andyw8 commented Jul 2, 2024 • edited Loading

Motivation

Implementation

Automated Tests

Manual Tests

andyw8 commented Jul 4, 2024

andyw8 commented Jul 4, 2024

Choose a reason for hiding this comment

andyw8 Jul 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyw8 commented Jul 5, 2024

vinistock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyw8 Jul 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyw8 commented Jul 2, 2024 •

edited

Loading

andyw8 Jul 5, 2024 •

edited

Loading

andyw8 Jul 9, 2024 •

edited

Loading