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

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

lib/rubydex/signature.rb

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;

rust/rubydex-sys/src/definition_api.rs

rust/rubydex/src/query.rs

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

rust/rubydex/src/query.rs

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

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?

rust/skills.json

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?

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

st0012 · 2026-04-14T10:38:30Z

rust/rubydex-sys/src/signature_api.rs

+/// C-compatible struct representing a single method signature (a list of parameters).
+#[repr(C)]
+pub struct SignatureEntry {
+    pub definition_id: u64,


Looks like this is never used. Do we keep it for future updates?

If that's the case, please add a comment for that.
If not, let's remove it and simplify related API.

Yeah, I think we can remove this one.

vinistock · 2026-04-14T14:52:25Z

rust/rubydex-sys/src/signature_api.rs

+/// C-compatible struct representing a single method signature (a list of parameters).
+#[repr(C)]
+pub struct SignatureEntry {
+    pub definition_id: u64,


Yeah, I think we can remove this one.

vinistock · 2026-04-14T14:53:56Z

rust/rubydex/src/test_utils/graph_test.rs

+        self.sources.insert(uri.to_string(), source);
+    }
+
+    /// Returns the normalized source for the given URI.
+    ///
+    /// # Panics
+    ///
+    /// Panics if the URI has not been indexed.
+    #[must_use]
+    pub fn source(&self, uri: &str) -> &str {
+        self.sources.get(uri).expect("source not found for URI")
+    }
+
+    /// Returns the source text for a definition, sliced by its offset.
+    ///
+    /// # Panics


Personally, I wouldn't include these changes as part of this PR. It's already pretty large and we can ship test improvements separately.

vinistock · 2026-04-14T14:56:36Z

rust/rubydex/src/query.rs

+    let ancestors: Vec<_> = ns.ancestors().iter().collect();
+
+    let search_start = if only_inherited {
+        let pos = ancestors
+            .iter()
+            .position(|a| matches!(a, Ancestor::Complete(id) if *id == namespace_id))
+            .expect("namespace_id should be present in its own ancestor chain");
+        pos + 1
+    } else {
+        0
+    };
+
+    for ancestor in &ancestors[search_start..] {


This refactor is switching 1 loop for 3. Before, we were iterating on ancestors and performing the checks inline.

Now, we're eagerly collecting ancestors using iter().collect() (which I think is not necessary? ancestors already returns a collection). Then we iterate to find the main namespace position and iterate again to search the ancestors.

I'd vote for us to keep the current implementation. At least for now.

vinistock · 2026-04-14T14:57:52Z

rust/rubydex/src/query.rs

+///
+/// Panics if any alias definition in the chain has no corresponding declaration.
+#[must_use]
+pub fn deep_dealias_method(graph: &Graph, alias_id: DefinitionId) -> Vec<DefinitionId> {


I think part of the test related changes are a result of this method returning definitions.

Should this return a DeclarationId instead? Then we return the declaration that an alias points to and, if desired, the consumer of the API can fetch the related definitions.

amomchilov · 2026-04-14T14:31:34Z

ext/rubydex/signature.c

+    case ParameterKind_RequiredPositional: return cPositionalParameter;
+    case ParameterKind_OptionalPositional: return cOptionalPositionalParameter;
+    case ParameterKind_RestPositional:     return cRestPositionalParameter;
+    case ParameterKind_Post:              return cPostParameter;


Suggested change

case ParameterKind_Post: return cPostParameter;

case ParameterKind_Post: return cPostParameter;

amomchilov · 2026-04-14T14:35:14Z

ext/rubydex/signature.c

+    for (size_t i = 0; i < arr->len; i++) {
+        SignatureEntry sig_entry = arr->items[i];
+
+        VALUE parameters = rb_ary_new_capa((long)sig_entry.parameters_len);


For a follow-up PR, potential performance optimization:

Parameter-less functions are pretty common. We might see some nice memory gains if we reuse the same empty array between them all.

amomchilov · 2026-04-14T14:35:50Z

lib/rubydex/signature.rb

+    attr_reader :parameters
+
+    #: (Array[Parameter]) -> void
+    def initialize(parameters)


No return value?

amomchilov · 2026-04-14T14:37:01Z

lib/rubydex/signature.rb

+# frozen_string_literal: true
+
+module Rubydex
+  class Signature


(Not for this PR, but in a follow-up)

In my experience, having all the parameters in a single array ends up making callers need to do lots of filtering (e.g. to find just positional args, just keyword args, pull out the block param, etc.).

As the need arises, I would encourage adding convenience methods like positional_parameters, keyword_parameters, block, etc.

amomchilov · 2026-04-14T14:40:35Z

rust/rubydex-sys/src/signature_api.rs

+    Block = 8,
+}
+
+fn map_parameter_kind(param: &Parameter) -> ParameterKind {


I think this would be nicer to have in the impl Parameter block, so you can just call like p.kind

amomchilov · 2026-04-14T14:46:01Z

rust/rubydex-sys/src/signature_api.rs

+use std::ptr;
+
+/// C-compatible enum representing the kind of a parameter.
+#[repr(C)]


We can be explicit about the size, too

Suggested change

#[repr(C)]

#[repr(C, u8)]

amomchilov · 2026-04-14T14:47:54Z

rust/rubydex-sys/src/signature_api.rs

+        let len = boxed.len();
+        let items_ptr = boxed.as_mut_ptr();
+        std::mem::forget(boxed);
+
+        Box::into_raw(Box::new(SignatureArray { items: items_ptr, len }))


We're already using Box::into_raw, we can apply it here to remove the need for this std::mem::forget(boxed).

Suggested change

let len = boxed.len();

let items_ptr = boxed.as_mut_ptr();

std::mem::forget(boxed);

Box::into_raw(Box::new(SignatureArray { items: items_ptr, len }))

let len = sig_entries.len();

let items_ptr = Box::into_raw(sig_entries.into_boxed_slice()) as *mut _;

Box::into_raw(Box::new(SignatureArray { items: items_ptr, len }))

(or we can use cast_const(), depending on the convention this repo follows

amomchilov · 2026-04-14T14:50:33Z

rust/rubydex-sys/src/signature_api.rs

+
+        let parameters_len = param_entries.len();
+        let parameters_ptr = param_entries.as_mut_ptr();
+        std::mem::forget(param_entries);


amomchilov · 2026-04-14T14:53:45Z

rust/rubydex-sys/src/signature_api.rs

+
+                for param_entry in &mut param_slice {
+                    if !param_entry.name.is_null() {
+                        let _ = unsafe { CString::from_raw(param_entry.name.cast_mut()) };


Equivalent, but more clear intent IMO

Suggested change

let _ = unsafe { CString::from_raw(param_entry.name.cast_mut()) };

drop(unsafe { CString::from_raw(param_entry.name.cast_mut()) });

amomchilov · 2026-04-14T14:54:06Z

rust/rubydex-sys/src/signature_api.rs

+    // Take ownership of the SignatureArray
+    let arr = unsafe { Box::from_raw(ptr) };
+
+    if !arr.items.is_null() && arr.len > 0 {


Can this be inverted into an early-return, similar to the ptr.is_null() check above?

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

rust/rubydex/src/query.rs Outdated Show resolved Hide resolved

vinistock reviewed Apr 1, 2026

View reviewed changes

Morriar reviewed Apr 2, 2026

View reviewed changes

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

st0012 reviewed Apr 8, 2026

View reviewed changes

soutaro and others added 17 commits April 14, 2026 06:34

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

b32b692

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

Add MethodDefinition#signatures to Ruby API

3203c85

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

Implement MethodAliasDefinition#signatures

8379cc7

Find inherited methods

d7681eb

Delete Signature#method_definition

a8f4c85

Rename parameter subclasses

7d9657a

Extract signature_api.rs

555cbed

Refactor rdx_declaration_find_member

d625d55

Fix dealias_method

a2aedb6

Add test for find_member_in_ancestors

4066306

Add test of deep_dealias_method

783402a

👮

d8520b7

lint

3762ce3

Create *dynamic* symbols, which are garbage collected

b73cb99

Drop … comment

f5e6d8e

Add use

cc54708

Get owner_id through declaration

9fb0de4

soutaro added 10 commits April 14, 2026 06:34

Fixup tests

fe2e52d

dedup deep_dealias_method result

9a0fbad

cargo fmt

7f41cb3

Add GraphTest.source_at_offset

e9942f7

Add panics section

c5c9d07

Move source_at to Offset

4d495ef

Add test for singleton classes

fce9732

It panics instead of returning NULL

536102e

delete skills.json

4494f5c

Simpler deep_dealias_method return value

aeeebb3

soutaro force-pushed the soutaro-definition-signatures branch from eef4e19 to aeeebb3 Compare April 13, 2026 21:34

st0012 reviewed Apr 14, 2026

View reviewed changes

vinistock reviewed Apr 14, 2026

View reviewed changes

amomchilov reviewed Apr 14, 2026

View reviewed changes

	case ParameterKind_Post: return cPostParameter;
	case ParameterKind_Post: return cPostParameter;

	let _ = unsafe { CString::from_raw(param_entry.name.cast_mut()) };
	drop(unsafe { CString::from_raw(param_entry.name.cast_mut()) });

Conversation

soutaro commented Mar 26, 2026

Summary

Test plan

Uh oh!

Uh oh!

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!

Uh oh!

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!

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!

Uh oh!

Uh oh!

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!

st0012 Apr 14, 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!

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!

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!

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!

soutaro Apr 1, 2026 •

edited

Loading

st0012 Apr 14, 2026 •

edited

Loading