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

vinistock · 2024-07-05T20:44:53Z

lib/ruby_indexer/lib/ruby_indexer/rbs_indexer.rb

-    sig { params(source: T.untyped, pathname: Pathname, declarations: T::Array[RBS::AST::Declarations::Base]).void }
+    sig do
+      params(
+        source: T.any(Symbol, String), # If source is from Ruby Core, it will be the symbol `:core`


It doesn't look like we're using source. Should we remove it?

vinistock · 2024-07-05T20:47:03Z

lib/ruby_indexer/lib/ruby_indexer/rbs_indexer.rb

+      parameters += process_trailing_positionals(function) if function.trailing_positionals
+      parameters += process_rest_positionals(function) if function.rest_positionals
+      parameters += process_required_keywords(function) if function.required_keywords
+      parameters += process_optional_keywords(function) if function.optional_keywords


Looking at our implementation for Ruby, are we missing keyword rest? (e.g.: **kwargs)?

vinistock · 2024-07-05T20:49:10Z

lib/ruby_indexer/lib/ruby_indexer/rbs_indexer.rb

+      function.optional_positionals.map do |param|
+        # Optional positionals may be unnamed, e.g.
+        #  def self.polar: (Numeric, ?Numeric) -> Complex
+        name = param.name || :anon # TODO: what should we call this?


I wouldn't use anon since you cannot actually have an anonymous optional parameter, they must always have some name.

My vote goes for simplicity with arg0, arg1, arg2 and so on using a map with index.

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.

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

lib/ruby_indexer/test/rbs_indexer_test.rb

@@ -74,5 +74,229 @@ def test_location_and_name_location_are_the_same

      assert_same(entry.location, entry.name_location)
    end
+
+    def test_rbs_method_with_required_positionals
+      entries = @index["crypt"] # https://www.rubydoc.info/stdlib/core/String#crypt-instance_method


Let's either remove the link or point to the official docs.

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 👍

vinistock · 2024-07-05T20:52:37Z

lib/ruby_indexer/test/rbs_indexer_test.rb

+      assert_kind_of(Entry::KeywordParameter, parameters[1])
+    end
+
+    def test_parse_simple_rbs


Let's add a test for keyword rest **kwargs.

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 from 44ff15a to bfaca26 Compare July 5, 2024 19:21

Parse RBS overloads into signatures

6916fde

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

wip

4b683e4

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

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

vinistock Jul 5, 2024

vinistock Jul 5, 2024

vinistock Jul 5, 2024

vinistock Jul 5, 2024

vinistock Jul 5, 2024

vinistock Jul 5, 2024


		first_signature = signatures.first

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

Parse RBS overloads into signatures #2243

Are you sure you want to change the base?

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

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