Add MethodDefinition#signatures to Ruby API by soutaro · Pull Request #697 · Shopify/rubydex

soutaro · 2026-03-26T05:34:04Z

Summary

Expose method signature information through MethodDefinition#signatures, returning an array of Rubydex::Signature objects
Each Signature holds an array of typed Parameter subclasses (PositionalParameter, KeywordParameter, etc.) with name and location attributes
Add Rust C API (ParameterKind, SignatureEntry, SignatureArray) and C extension glue for conversion

Extracted from #686.

Test plan

Tests cover all 8 parameter kinds (positional, optional, rest, keyword, optional keyword, rest keyword, block, forward)
Tests cover RBS-defined signatures, overloads, untyped parameters, and no-parameter methods

🤖 Generated with Claude Code

ext/rubydex/signature.c

rust/rubydex-sys/src/definition_api.rs

vinistock · 2026-03-26T20:34:13Z

lib/rubydex/signature.rb

+    #: (parameters: Array[Parameter], method_definition: MethodDefinition) -> void
+    def initialize(parameters:, method_definition:)
+      @parameters = parameters
+      @method_definition = method_definition


Do we need this back link? Considering that consumers will interact with the API like this:

declaration.definitions.first.signatures

I think we can remove this.

This is for MethodDeclaration#signatures. It will return an array of Signature, and then we should need the backlink to retrieve docs and something associated to the method definition.

We can drop the backlink and add a new class or use tuple for #signatures method though?

Let's discuss in the team sync, but I think exposing signatures at the declaration level has the potential to be a bit confusing.

Maybe we can handle the alias case in some other way.

Based on what we discussed, let's remove method_definition from the initializer and only accept the parameters. It will also simplify rdxi_signatures_to_ruby a bit.

Sure. Will remove the method_definition.

lib/rubydex/signature.rb

- Rename ParameterKind variants to full names (e.g. Req -> RequiredPositional) - Merge two let-else bindings into one in rdx_definition_signatures - Make collect_method_signatures private - Change Signature#initialize from keyword arguments to positional arguments Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

vinistock · 2026-03-31T15:00:39Z

ext/rubydex/signature.c

+}
+
+VALUE rdxi_signatures_to_ruby(SignatureArray *arr, VALUE graph_obj, VALUE default_method_def) {
+    if (arr == NULL || arr->len == 0) {


How about we raise in rdxr_method_definition_signatures and rdxr_method_alias_definition_signatures if the pointer is NULL?

This should never really happen, so I'm trying to avoid having it fail silently.

Yeah, I think the design is weird. Will take a look at this tonight.

Fixed the implementation. rdx_method_definition_signatures and rdx_method_alias_definition_signatures always return an array. rdxi_signatures_to_ruby can safely skips checking if arr is NULL.

ext/rubydex/signature.c

vinistock · 2026-03-31T15:03:46Z

lib/rubydex/signature.rb

+    #: (parameters: Array[Parameter], method_definition: MethodDefinition) -> void
+    def initialize(parameters:, method_definition:)
+      @parameters = parameters
+      @method_definition = method_definition


Based on what we discussed, let's remove method_definition from the initializer and only accept the parameters. It will also simplify rdxi_signatures_to_ruby a bit.

rust/rubydex-sys/src/definition_api.rs

vinistock · 2026-03-31T15:05:44Z

rust/rubydex-sys/src/definition_api.rs

+        let mut boxed = sig_entries.into_boxed_slice();
+        let len = boxed.len();
+        let items_ptr = boxed.as_mut_ptr();
+        std::mem::forget(boxed);


Do we need to forget here if we're ultimately passing this to Box::into_raw?

If we don't use forget, the contents of the Box will be deallocated by the destructor.
We could use Box::into_raw instead, but I don't think it's worth the rewrite since the mem::forget pattern is already used elsewhere in the codebase.

let items_ptr = Box::into_raw(boxed) as *mut SignatureEntry;

vinistock · 2026-03-31T15:07:22Z

rust/rubydex-sys/src/definition_api.rs

        create_location_for_uri_and_offset(graph, document, name_offset)
    })
 }
+


From this point on, everything is about signatures. What do you think about creating a signature_api.rs file to house these?

rust/rubydex/src/query.rs

soutaro · 2026-04-01T14:28:36Z

rust/rubydex/src/query.rs

+
+/// Result of following a `MethodAliasDefinition` chain to completion.
+#[derive(Debug)]
+pub struct DeepDealiasMethodResult {


I'm not sure if we really need the details of the failure. It keeps every details the implementation has in this layer, and C API layer ignores them.

The implementation would be a bit simpler if we can ignore the error details.

I think differentiating the error message is probably worth it, but I think this should be an enum instead of a struct.

vinistock · 2026-04-01T14:54:17Z

ext/rubydex/signature.c

+            ParameterEntry param_entry = sig_entry.parameters[j];
+
+            VALUE param_class = parameter_class_for_kind(param_entry.kind);
+            VALUE name_sym = ID2SYM(rb_intern(param_entry.name));


Does rb_intern make a copy of the string we pass? I'm asking because we free the signature array object further below. If Ruby interning just keeps a pointer, it could lead to a seg fault.

I believe rb_intern copies the string, but I couldn't find any documentation that explicitly says so. Changed to rb_str_intern with rb_utf8_str_new_cstr, which creates dynamic symbols (subject to GC) from a copied C string.

test/definition_test.rb

rust/rubydex-sys/src/signature_api.rs

vinistock · 2026-04-01T15:11:47Z

rust/rubydex/src/query.rs

+/// # Panics
+///
+/// Panics if a `SelfReceiver` definition cannot be resolved to a namespace with a singleton class.
+pub fn dealias_method(


This seems a bit more complicated than what I would've expected. It's a requirement that the graph must be resolved before performing queries like this one. To dealias a method, I would've expected something like this:

fn dealias(graph: &Graph, alias: &MethodAliasDefinition) { let decl_id = graph.definition_id_to_declaration_id(*def_id).unwrap(); let alias_decl = graph.declarations().get(decl_id).unwrap(); // Get the already resolved alias owner. No need to worry about the receiver // that HAS to have been resolved during resolution already let Some(method_decl_id) = find_member_in_ancestors(graph, alias_decl.owner_id(), *alias.old_name_str_id(), false) else { return Err(DealiasMethodError::MemberNotFound); }; }

I don't think we need the EnclosingNamespace either.

Morriar · 2026-04-02T14:54:10Z

rust/rubydex/src/test_utils/graph_test.rs

+    ///
+    /// Panics if the definition or its document does not exist.
+    #[must_use]
+    pub fn source_at(&self, definition_id: &DefinitionId) -> &str {


I think we can move this to Offset and pass the document as we do with https://github.com/Shopify/rubydex/blob/main/rust/rubydex/src/offset.rs#L90-L103

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

st0012 · 2026-04-08T11:00:57Z

rust/rubydex-sys/src/signature_api.rs

+}
+
+/// Returns a newly allocated array of signatures for the given method definition id.
+/// Returns NULL if the definition is not a method definition.


I think it panics in this case?

st0012 · 2026-04-08T11:01:15Z

rust/skills.json

Should this be included?

st0012 · 2026-04-08T11:02:19Z

test/definition_test.rb

    end
  end

+  def test_method_definition_signatures_with_various_parameter_kinds


Can we include test cases for singleton methods?

soutaro requested a review from a team as a code owner March 26, 2026 05:34

soutaro self-assigned this Mar 26, 2026

soutaro mentioned this pull request Mar 26, 2026

Expose method signature to Ruby #686

Open

vinistock reviewed Mar 26, 2026

View reviewed changes

soutaro commented Mar 27, 2026

View reviewed changes

lib/rubydex/signature.rb Outdated Show resolved Hide resolved

soutaro force-pushed the soutaro-definition-signatures branch from 79877ae to 39084db Compare March 31, 2026 08:58

vinistock reviewed Mar 31, 2026

View reviewed changes

soutaro commented Apr 1, 2026

View reviewed changes

vinistock reviewed Apr 1, 2026

View reviewed changes

Morriar reviewed Apr 2, 2026

View reviewed changes

soutaro and others added 19 commits April 8, 2026 13:33

Add Rubydex::Signature class and Rust C API for method signatures

45dd175

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Add MethodDefinition#signatures to Ruby API

0109b13

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Implement MethodAliasDefinition#signatures

d788ae5

Find inherited methods

8292b33

Delete Signature#method_definition

1440e75

Rename parameter subclasses

bb6b2ee

Extract signature_api.rs

5ae72bd

Refactor rdx_declaration_find_member

380e5cb

Fix dealias_method

dc6e220

Add test for find_member_in_ancestors

8f07027

Add test of deep_dealias_method

a4081f4

👮

752126a

lint

729c2a8

Create *dynamic* symbols, which are garbage collected

e0696d9

Drop … comment

105a237

Add use

4d54346

Get owner_id through declaration

f394ab5

Fixup tests

7680381

dedup deep_dealias_method result

3dfb63f

soutaro added 3 commits April 8, 2026 13:37

cargo fmt

0965c7b

Add GraphTest.source_at_offset

edfb258

Add panics section

e0b3e8a

soutaro force-pushed the soutaro-definition-signatures branch from b33e8c6 to e0b3e8a Compare April 8, 2026 04:48

Move source_at to Offset

eef4e19

st0012 reviewed Apr 8, 2026

View reviewed changes

Conversation

soutaro commented Mar 26, 2026

Summary

Test plan

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

soutaro Apr 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

soutaro Apr 1, 2026 •

edited

Loading