Skip to content

Partially migrate go to definition to use Rubydex#4015

Merged
vinistock merged 1 commit intorubydex_adoption_feature_branchfrom
03-03-migrate_go_to_definition_to_use_rubydex
Mar 26, 2026
Merged

Partially migrate go to definition to use Rubydex#4015
vinistock merged 1 commit intorubydex_adoption_feature_branchfrom
03-03-migrate_go_to_definition_to_use_rubydex

Conversation

@vinistock
Copy link
Member

Motivation

This PR partially migrates go to definition to use Rubydex. It ignores constants for now as we need the visibility API to complete it. Everything else is migrated.

Implementation

There's not much that's notable other than changing to use the new APIs, but I'll highlight two things:

  1. I always disliked our Common module. I propose that we instead patch Rubydex's classes with LSP related things so that we can do things like definition.to_lsp_range (see the code). It feels a lot more elegant and, as long as we include lsp in the method names, it won't really conflict with anything
  2. The logic to find the correct graph declaration for go to definition, hover and signature help will probably be super similar. I think we can extract a common implementation for the 3, but I held back for now so that we can take a more complete look into how the requests use the graph

Automated Tests

I migrated our tests. The only thing to call out here is that Rust's ordering does not match the original indexer's ordering (due to how HashMap works).

This makes no functional difference, but tests expect a very specific order to assert things. Instead of paying the price to sort the result, which is unnecessary, I just sorted in the tests.

@vinistock vinistock self-assigned this Mar 18, 2026
@vinistock vinistock requested a review from a team as a code owner March 18, 2026 15:51
@vinistock vinistock added server This pull request should be included in the server gem's release notes other Changes that aren't bugfixes, enhancements or breaking changes labels Mar 18, 2026
@vinistock vinistock requested review from alexcrocha and st0012 March 18, 2026 15:51
@vinistock vinistock force-pushed the 03-18-migrate_type_inferrer branch from 5b864a5 to cd9f1e7 Compare March 18, 2026 17:16
@vinistock vinistock force-pushed the 03-03-migrate_go_to_definition_to_use_rubydex branch 2 times, most recently from a6c13bd to 005200d Compare March 19, 2026 16:44
@vinistock vinistock mentioned this pull request Mar 19, 2026
19 tasks
Base automatically changed from 03-18-migrate_type_inferrer to rubydex_adoption_feature_branch March 20, 2026 19:55
@vinistock vinistock force-pushed the 03-03-migrate_go_to_definition_to_use_rubydex branch from 005200d to f3b0fcc Compare March 20, 2026 20:04
@vinistock vinistock force-pushed the rubydex_adoption_feature_branch branch from 9b81f5a to b0eb37b Compare March 23, 2026 15:17
@vinistock vinistock force-pushed the 03-03-migrate_go_to_definition_to_use_rubydex branch from f3b0fcc to 97436ad Compare March 23, 2026 15:17
Copy link
Contributor

@alexcrocha alexcrocha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I propose that we instead patch Rubydex's classes with LSP related things

This does indeed look nicer!

@st0012
Copy link
Member

st0012 commented Mar 25, 2026

It ignores constants for now as we need the visibility API to complete it. Everything else is migrated.

Is the missing API for replacing entry.private?? If that's the case, can we do the full migration already and skip the test related to that specific check?

@vinistock vinistock force-pushed the rubydex_adoption_feature_branch branch from b0eb37b to 1df056f Compare March 26, 2026 16:55
@vinistock vinistock force-pushed the 03-03-migrate_go_to_definition_to_use_rubydex branch from 97436ad to ffe9281 Compare March 26, 2026 17:27
@vinistock
Copy link
Member Author

@st0012 done! Skipped the visibility test and migrated the request.

@vinistock vinistock merged commit 932b60a into rubydex_adoption_feature_branch Mar 26, 2026
38 checks passed
@vinistock vinistock deleted the 03-03-migrate_go_to_definition_to_use_rubydex branch March 26, 2026 18:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

other Changes that aren't bugfixes, enhancements or breaking changes server This pull request should be included in the server gem's release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants