Skip to content

feat(pipeline): wire Ruby cross-file LSP resolution - #1712

Open
benrfairless wants to merge 3 commits into
DeusData:mainfrom
benrfairless:feat/ruby-lsp-cross-file
Open

feat(pipeline): wire Ruby cross-file LSP resolution#1712
benrfairless wants to merge 3 commits into
DeusData:mainfrom
benrfairless:feat/ruby-lsp-cross-file

Conversation

@benrfairless

Copy link
Copy Markdown
Contributor

What does this PR do?

Slice 4 of 5 splitting #1706. Stacked on #1711 — see the review note below.

Adds the cross-file tier on top of the per-file resolver.

cbm_run_ruby_lsp_cross builds its class table from the project-wide CBMLSPDef[], deriving each constant path by stripping the def's module QN prefix, and registers methods on both the instance and singleton receiver keys because CBMLSPDef does not carry the def self.m split. Ruby is registered on the fallback cbm_pxc_run_one path, like Java and Kotlin — there is no prebuilt tier-2 registry yet.

The one deliberate design decision here, and the thing most worth arguing with: Ruby joins Rust in bypassing the own+imports module-def filter, for a different reason. Ruby constants live in one global namespace and Rails/Zeitwerk autoloads them with no require statement, so a filter keyed on requires would starve essentially all cross-file resolution in a real Ruby app.

The two new rows drive cbm_run_ruby_lsp_cross with a hand-built CBMLSPDef[] in the shape pass_lsp_cross.c passes it, the second mirroring the Rails-shaped e2e fixture (User < ApplicationRecord < ActiveRecord::Base, a service object, and a caller in a third file).

Reviewing just this slice

A PR from a fork can only target a branch in this repo, so the base here is main and GitHub shows #1711's commit as well as this one. Only the second commit is this slice:

Merge #1711 first and this PR's diff collapses to that single commit.

Note that internal/cbm/lsp/ruby_lsp.{c,h} appear in both PRs. That is the split the review asked for: the cross-file entry point moved out of the resolver slice so each slice's tests exercise everything it adds. I verified the two halves reassemble byte-identically to the original single-commit file.

Where this sits in the series

# PR Depends on
1 #1709
2 #1710
3 #1711
4 this PR 3
5 docs/ruby-hybrid-lsp 4

CI baseline

main at 4d7d9f1 is red for reasons unrelated to this change (#1181 × #1359 composition; fixed by #1704). Until that merges, ci-ok will be red here for that reason.

Refs #1701

Checklist

  • Every commit is signed off (git commit -s)
  • Tests pass locally (scripts/test.sh) — see fix(extraction): keep Ruby superclass names bare #1709 for the full-leg numbers; all failures are pre-existing on unmodified origin/main.
  • Lint passes (make -f Makefile.cbm lint-ci)
  • New behavior is covered by a test

Assisted by OpenCode using anthropic/claude-opus-5.

In-process, type-aware call resolution for single Ruby files, mirroring
the perl_lsp.c / php_lsp.c shape and dispatched from cbm_extract_file:

- PASS 1 collects classes and modules with lexical nesting, superclasses,
  and include / prepend / extend mixins, plus method tables with instance
  and singleton methods classified from the AST. Singleton methods key on
  "<class_qn>.self", which cannot collide with a constant path.
- PASS 1.5 infers instance-variable types from `@x = Const.new`;
  conflicting assignments latch to no type rather than guessing.
- PASS 2 resolves constructors (`Const.new` targets the class node, the
  way Python's lsp_constructor does), implicit and explicit self
  dispatch, singleton methods, superclass chains, mixin lookup in Ruby's
  method-resolution order, super, and ivar / local / chained receivers.

The stdlib seed covers Ruby core plus a curated Rails surface, including
ActiveRecord query typing: `User.find(1)` is a User, and relation chains
keep the model type. Dynamic dispatch (send, method_missing,
define_method) and unresolved receivers emit no edge, and only
project-defined targets are emitted.

Cross-file resolution is a separate tier and is not part of this change;
the 18 rows here drive the resolver through cbm_extract_file exactly as
the extraction pipeline calls it.

PROVENANCE: this resolver was authored clean-room from the
tree-sitter-ruby grammar and Ruby language semantics. No Ruby language
server source was consulted. scripts/check-lsp-originality.sh gains
Shopify's ruby-lsp (MIT) as the Ruby reference so the boundary is
auditable alongside the generated data it guards;
`check-lsp-originality.sh --lang ruby` reports CLEAN — no verbatim
string or comment overlap and no structural clones.

Refs DeusData#1701

Assisted-by: OpenCode:anthropic/claude-opus-5
Signed-off-by: Ben Fairless <ben@oaf.org.au>
Adds the cross-file tier on top of the per-file resolver.

cbm_run_ruby_lsp_cross builds its class table from the project-wide
CBMLSPDef[], deriving each constant path by stripping the def's module QN
prefix, and registers methods on both the instance and singleton receiver
keys because CBMLSPDef does not carry the `def self.m` split. Ruby is
registered on the fallback cbm_pxc_run_one path, like Java and Kotlin —
there is no prebuilt tier-2 registry yet.

Ruby also joins Rust in bypassing the own+imports module-def filter, for
a different reason: Ruby constants live in one global namespace and
Rails/Zeitwerk autoloads them with no require statement, so a filter
keyed on requires would starve essentially all cross-file resolution in a
real Ruby app. That exemption is the one deliberate design decision here.

The two new rows drive cbm_run_ruby_lsp_cross with a hand-built
CBMLSPDef[] in the shape pass_lsp_cross.c passes it, the second mirroring
the Rails-shaped e2e fixture (User < ApplicationRecord <
ActiveRecord::Base, a service object, and a caller in a third file).

Refs DeusData#1701

Assisted-by: OpenCode:anthropic/claude-opus-5
Signed-off-by: Ben Fairless <ben@oaf.org.au>
@github-actions

Copy link
Copy Markdown

Thanks for opening this — it has been seen, and it is queued.

This note is automated, but it is not a brush-off: it exists so you know where your PR stands instead of having to guess from silence.

Current review status: working through a backlog. 0.9.1-rc.1 is out, so the release freeze that held reviews is over — but it left a large queue of open pull requests behind it, and we are reading through them oldest-first. The background is in discussion #1144.

What that means for this PR, concretely:

  • It will not be closed for inactivity. No stale bot touches pull requests here.
  • It may still sit a while before a human reads it. That is on us, not on you.
  • Older PRs are read first, so a recent one is not being skipped — it is behind a queue.

Things that will genuinely speed it up whenever review does happen:

  • Keep it rebased on main — the tree is moving quickly right now, and a conflicting branch cannot be reviewed as the diff you intended.
  • Get CI green, or say which failures you believe are pre-existing.
  • Keep the change to one claim. Bundled features and refactors get split before they get merged, which costs you a round trip.
  • Every commit needs a sign-off (git commit -s) — CI enforces DCO.

If this fixes a bug, a reproduction we can run is worth more than a description of the symptom.

Thanks for contributing, and sorry in advance for the wait.

@DeusData

DeusData commented Sep 1, 2026

Copy link
Copy Markdown
Owner

Reviewed slice 4. The wiring is right and the Zeitwerk argument is correct. There is one consequence of it I want measured before this lands, and you have already named half of it yourself.

The filter exemption is justified. Ruby constants live in one global namespace and Rails/Zeitwerk autoloads them with no require, so an own+imports filter really would starve cross-file resolution. Putting Ruby beside Rust in that branch, with a comment saying why the mechanics differ, is the right way to record it.

The consequence: Ruby has the Rust exemption without the Rust mitigation

Read the branch you edited alongside its own comment:

Rust therefore always resolves against the FULL def universe: the lazily built shared registry when available, else a full per-file build.

Rust survives the exemption because the registry is its normal path and the full per-file build is its fallback. Your PR description says the quiet part plainly: "there is no prebuilt tier-2 registry yet." So for Ruby the fallback is the only path.

Following it through:

  • cbm_pxc_dispatch_file runs per file; with lang != CBM_LANG_RUBY removed from the filter guard, file_defs = all_defs.
  • cbm_pxc_run_one hands that whole array to cbm_run_ruby_lsp_cross.
  • Which opens with for (int i = 0; i < def_count; i++) to build its class table.

So every Ruby file rebuilds a class table over the entire project's defs. That is F × D, and D grows with F — the corpus-proportional shape, and precisely what "a language without a Tier-2 shared registry is automatically quadratic" describes. A Rails app is exactly the kind of repo where F is large.

I want to be honest about the limits of this: I am reasoning from the call structure, not from a measurement. It may be perfectly acceptable at real Ruby project sizes — the constant factor is small and the class table is cheap per def. But that is a claim to check rather than assume, and right now nothing checks it.

What I would like

Either a measurement, or — better, because it is permanent — a Ruby fixture under tests/fixtures/complexity/ruby/.

The complexity suite auto-discovers fixture directories at that path (tests/test_complexity.c:167-175) and gates on counter ratios at k versus 2k replicated copies, so a files×corpus coupling lands at ~4 and goes red. It needs no test edit — just fixture files. That is the mechanism this repo built for exactly this case, and a new language taking the filter exemption is the strongest reason to use it.

If the ratio comes back at ~2, this merges and the fixture stops the next person reintroducing the coupling. If it comes back at ~4, better to know now than after a Rails user reports a slow index.

Smaller notes

The split is clean and I checked the part that is easy to get wrongruby_lsp.{c,h} appearing in both slices is the reviewer-requested move of cbm_run_ruby_lsp_cross out of the resolver slice, and you verified the halves reassemble byte-identically.

Registering methods on both instance and singleton receiver keys because CBMLSPDef carries no def self.m split is a reasonable over-registration, and worth the comment it has.

Clearance: the two flagged files here (Makefile.cbm, scripts/check-lsp-originality.sh) are #1711's commit, already reviewed and cleared on that PR — nothing new in this slice.

Baseline note: the #1181 × #1359 cli failures you documented are fixed on main now; I confirmed both pass in a full-suite run today.

@DeusData DeusData added enhancement New feature or request parsing/quality Graph extraction bugs, false positives, missing edges language-request Request for new language support priority/normal Standard review queue; useful PR with ordinary maintainer urgency. labels Sep 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request language-request Request for new language support parsing/quality Graph extraction bugs, false positives, missing edges priority/normal Standard review queue; useful PR with ordinary maintainer urgency.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants