Repository navigation
feat(lsp): add the Ruby per-file resolver - #1711
benrfairless wants to merge 13 commits into
Conversation
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>
|
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. What that means for this PR, concretely:
Things that will genuinely speed it up whenever review does happen:
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. |
|
Thank you for the Ruby resolver contribution, and sorry that this foundational slice has had only the automated acknowledgement. This is now routed as a Ruby language and parsing enhancement with normal priority. Per-file resolution is the base of the later cross-file slices, so we need time to review its semantic coverage and generated stdlib data carefully rather than infer acceptance from the later stack discussion. The contribution queue is quite full, but we will return with grounded feedback. No changes are requested from you at this triage stage. |
|
Thank you, @benrfairless, and sorry again for the wait. We've made the direction call: a Ruby per-file resolver is welcome, and this PR, as the base of the stack (#1712, #1713), will be reviewed for merge. Because it is a new language resolver, the review follows our checklist for language LSP additions:
You don't need to do anything yet. If we need changes, they will come as one grounded review, not piecemeal. Thank you for the contribution and your patience. |
DeusData
left a comment
There was a problem hiding this comment.
Hi @benrfairless, thank you, sincerely, for this resolver and for your patience. It waited far longer than it should have, and you split #1706 into exactly the reviewable, test-backed slices we asked for, which made this review much easier. As promised, here is the whole review in one go, against our language-LSP checklist.
What's in good shape
- Originality: the resolver reads as original work in the shape of our Perl/PHP resolvers. The stdlib seed is hand-written API surface with no third-party data, so no license or THIRD_PARTY changes are needed. Thanks for adding the ruby-lsp reference to the originality script.
- Per-file complexity: the registry is finalised after registration, nothing is added after finalize, and there is no shared registry to mutate. The linear table scans are bounded by the file, as in Perl. There is no raw allocator growth and no file I/O.
- Fail-closed basics: the
sendfamily, unknown receivers, unknown constants and stdlib targets all emit nothing. The prepend-over-own test pins the winner, which we really appreciate. - It stands alone: no pipeline changes, and it merges cleanly into current
main.
Changes we'd like before merge
- Compact
class A::Bnames. The extractor names these…A::B(the raw name text), but the resolver builds…A.B. Our join needs an exact match on the caller's name, so every row from or to a compact-declared class is silently dropped, and that's the most common Rails style. Please build class names the way the extractor does, and keep the dotted path only for constant lookup. Also, insideclass A::BRuby's lexical nesting is[A::B]only, soA's constants shouldn't be probed. A test with exact name equality would pin it. - Local scope. A
defbody currently inherits the enclosing scope, butdefis a scope gate in Ruby. Parameters and block parameters (|a|) don't shadow outer locals. Reassigning a local to something untyped also keeps the old type:a = Foo.new; a = x; a.barstill resolves toFoo#bar. Please makedefa fresh scope, bind parameters and block parameters as unknown, and rebind to unknown on any untyped reassignment, including multiple assignment and||=. - Instance variables. Any assignment to
@xother thanConst.new(ornil) should latch the conflict, the same way two different constructors already do.@x ||= Foo.newshould count as a constructor assignment. - Blocks that change
self:instance_eval/instance_exec,class_eval/class_exec,module_eval/module_exec,define_method, andClass.new/Module.new. Bare andselfcalls inside these currently dispatch on the enclosing class. Please suppress them there, and add the*_execforms to the dynamic-dispatch list. The existing negative test passes only becauseMetahas nohiddenmethod; giving it one makes the test bite. - Unresolved ancestry, three fail-closed cases:
- an unresolved prepended module should block own-method hits;
- an unresolved included module should stop the walk before the superclass chain;
- a bare call in a class with unresolved ancestors shouldn't fall through to a same-named top-level def.
- Methods defined inside blocks, such as ActiveSupport's
class_methods do … end, shouldn't be registered as instance methods of the enclosing module. - Please use exact-name assertions for the nested and compact-class rows rather than substring matches.
Heads-up on #1712, so it doesn't come as a surprise later: the cross-file slice resolves each Ruby file against a registry rebuilt from the whole project's definitions. With no shared registry and the definition filter bypassed, the cost grows with files × definitions, and ruby_add_class's dedupe scan makes it worse. We'll want a registry built once, read-only and shared, with hashed class lookup; the Go, Python and TypeScript cross registries are the model. Please also add tests/fixtures/complexity/ruby/ so our complexity suite gates it, and handle classes reopened across files (right now the first file wins).
Once these land, we'll run the per-language benchmark on discourse and rails: edge precision on a hand-checked sample, recall, wall time and memory, and a determinism check. Then the three-OS ladder. Thanks again. This is a substantial, careful piece of work, and we're glad Ruby is getting a real resolver.
The structural extractor names a class by appending the declaration's raw name text to the enclosing class QN, so `class Admin::User` yields `<module_qn>.Admin::User` while `module Admin; class User` yields `<module_qn>.Admin.User`. The resolver normalised both to the dotted form, and because the pipeline joins resolved rows to defs by exact caller QN, every row from or to a compact-declared class was silently dropped — the most common Rails style. All three passes now enter a class through one shared frame helper that computes the extractor-shaped QN from the raw name and keeps the dotted path only for constant lookup. The class table dedupes on QN. Nesting is now one raw frame per declaration joined by '.', and the constant probe strips one frame at a time: inside `class A::B` only `A.B.x` and `x` are tried, never `A.x`, matching Ruby's lexical scoping (`class A::B` does not open A). Tests pin exact QN equality on both ends of the row for compact, nested-compact, and the outer-scope-not-opened case. Assisted by OpenCode using anthropic/claude-fable-5-1. Signed-off-by: Ben Fairless <ben@oaf.org.au>
…ignment A `def` body inherited the enclosing scope, so a typed local from the surrounding body (or a preceding sibling method) typed a same-named parameter or local inside it. Parameters and block parameters were never bound, so they could not shadow an outer local either. And an assignment whose right-hand side had no proven type was ignored, so `a = Foo.new; a = x; a.bar` still resolved to Foo#bar. Ruby scoping is now modeled explicitly: `def`, `class` and `module` bodies start from a fresh, unparented scope; blocks and lambdas get a chained child scope; method, block and lambda parameters (including destructured groups and block-local names) bind to UNKNOWN; and any write the resolver cannot type — plain reassignment, multiple assignment, `||=` on an already-typed name, other compound operators, or an assignment inside a block — rebinds the nearest existing binding to UNKNOWN. `a ||= Foo.new` on a fresh name still types as Foo. Assisted by OpenCode using anthropic/claude-fable-5-1. Signed-off-by: Ben Fairless <ben@oaf.org.au>
PASS 1.5 only recorded `@x = Const.new` and ignored every other write, so `@x = compute` (or a parameter, a compound operator, a multiple assignment, or an unresolved constant's .new) left the ivar typed by whichever constructor it saw elsewhere. Any such write now latches the conflict, exactly as two different constructors already did; `nil` is neutral. `@x ||= Const.new` — the memoised-constructor idiom — counts as a constructor assignment. Assisted by OpenCode using anthropic/claude-fable-5-1. Signed-off-by: Ben Fairless <ben@oaf.org.au>
instance_eval/instance_exec, class_eval/class_exec, module_eval/ module_exec, define_method and Class.new/Module.new run their block with a different `self`, but bare and `self.` calls inside them were dispatched on the enclosing class. The eval call itself was already typed unknown; the walk into its block was not. The block of such a call is now walked with self_unknown set: bare calls, `self.` calls and `super` emit nothing there, `self` has no type, and PASS 1.5 does not attribute ivar writes inside it to the enclosing class. Receiver and argument expressions of the same call still resolve, and the suppression ends with the block. The *_exec forms join the dynamic-dispatch list. The existing negative test passed only because Meta had no `hidden`; it now has one, and the calls are written with `()` because tree-sitter-ruby parses a lone identifier as a variable reference, which the extractor never emits as a call. Assisted by OpenCode using anthropic/claude-fable-5-1. Signed-off-by: Ben Fairless <ben@oaf.org.au>
The lookup walks skipped a mixin whose module did not resolve and carried on to the class's own methods and superclass chain, and a bare call whose class ancestry did not resolve fell through to a same-named top-level def. An unseen ancestor may define or shadow the method, so none of those answers is trustworthy. Every lookup now carries a `blocked` flag set the moment the walk meets an unresolved prepend, include, extend or superclass reference, in Ruby's lookup order: an unresolved prepend blocks even an own-method hit; an unresolved include (or superclass) stops the walk before the superclass chain; an unresolved extend blocks the singleton walk. When blocked, nothing is emitted — no Object typing fallback, no top-level fallthrough, no `super` edge. Hits found before the unseen ancestor are still returned (an own method ahead of an unresolved include; a resolved include listed after an unresolved one). Assisted by OpenCode using anthropic/claude-fable-5-1. Signed-off-by: Ben Fairless <ben@oaf.org.au>
PASS 1 recursed through blocks and registered any `def` it met on the enclosing class or module, so ActiveSupport's `class_methods do … end` and `included do … end` bodies became instance methods of the concern and were then found through `include Concern` — edges to defs the structural extractor does not even emit. `include X` written inside such a block was likewise recorded on the concern instead of the includer. PASS 1 now tracks the block depth since the nearest class/module body and, inside a block, registers no methods and collects no mixins. In PASS 2 a class-body block def is skipped outright (no def exists for it to join), and a top-level block def (an RSpec group) is walked with self unknown: typed-receiver calls in its body may still resolve, bare and self. calls emit nothing, and other top-level code cannot reach it. Assisted by OpenCode using anthropic/claude-fable-5-1. Signed-off-by: Ben Fairless <ben@oaf.org.au>
The nested-modules row matched caller and callee by substring, which would have let a wrongly-spelled class path pass; it now pins the full extractor QNs on both ends with strcmp, as the compact-class rows already do. Refresh the file headers to describe the scope gates, fail-closed ancestry and self-rebinding rules the resolver now follows. Assisted by OpenCode using anthropic/claude-fable-5-1. Signed-off-by: Ben Fairless <ben@oaf.org.au>
|
Thank you for the thorough review — every item was a real hole, and the compact-class one in particular would have silently dropped most Rails rows. All seven are addressed, one signed-off commit per item so they can be read against your list:
What changed, per item
Two deliberate trade-offs worth flagging
Verification
On the #1712 heads-up — understood and agreed. The cross-file slice will get a registry built once, read-only and shared, with hashed class lookup on the Go/Python/TypeScript model, plus Assisted by OpenCode using anthropic/claude-fable-5-1. |
|
Thank you, @benrfairless, for the detailed September 30 response and for organizing the changes against all seven review items. We need more time to review the updated resolver and complete the promised validation. Your stated tradeoffs are recorded for that review; this acknowledgment is not a claim that we have independently verified the new head. Please keep the cross-file work separate as discussed. Thanks again for the care and patience. |
Bring the seven review fixes from DeusData#1711 into the cross-file slice. The only conflict was tests/test_ruby_lsp.c, where both sides added tests; both are kept. Assisted-by: Claude Code:claude-sonnet-5-5 Signed-off-by: Ben Fairless <ben@oaf.org.au>
|
Thanks for the detailed response addressing all seven review items and for keeping cross-file work separate. The updated resolver and its stated tradeoffs are still waiting on our re-review and validation. Sorry for the delay—I’ll follow up soon; no further contributor action is requested now. |
What does this PR do?
Slice 3 of 5 splitting #1706, and the one that carries the clean-room boundary. Independent of slices 1 and 2 — see the verification note below.
In-process, type-aware call resolution for single Ruby files, mirroring the
perl_lsp.c/php_lsp.cshape and dispatched fromcbm_extract_file:include/prepend/extendmixins, 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.@x = Const.new; conflicting assignments latch to no type rather than guessing.Const.newtargets the class node, the way Python'slsp_constructordoes), 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 aUser, and relation chains keep the model type. Dynamic dispatch and unresolved receivers emit no edge, and only project-defined targets are emitted.Cross-file resolution is deliberately not here — it is slice 4, so that every line this PR adds is exercised by this PR's own tests.
Provenance and originality
This resolver was authored clean-room from the tree-sitter-ruby grammar and Ruby language semantics. No Ruby language server source was consulted.
Per the review, the originality reference ships with this slice rather than with the docs, so the generated data and the clean-room boundary can be audited together.
scripts/check-lsp-originality.shgains Shopify's ruby-lsp (MIT) as the Ruby reference, and the scan is clean:Verification that this slice is independent
The 20 rows in
tests/test_ruby_lsp.call pass on this branch, which is based directly onmainand contains neither extraction fix from slices 1 and 2. The per-file resolver reads thesuperclassfield straight off the AST rather than going throughbase_classes, so it does not need slice 1; and no per-file row usesA::B.new, so it does not need slice 2. That is why this is not stacked on them.I also ran the Ruby-adjacent suites together (
ruby_lsp extraction extraction_inheritance grammar_regression language lang_contract edge_structural lsp_resolution_probe): 679 passed, 0 failed. Worth checking because the resolver now runs on every Ruby extraction.On size
At ~2.5k lines this is well over the 500-line guidance in CONTRIBUTING. It is the atomic unit the review asked for — resolver, standard-library seed and its focused tests — and splitting it further would separate the generated data from the code that consumes it, which is the opposite of what the originality audit needs.
Where this sits in the series
feat/ruby-lsp-cross-filedocs/ruby-hybrid-lspCI baseline
mainat 4d7d9f1 is red for reasons unrelated to this change (#1181 × #1359 composition; fixed by #1704). Until that merges,ci-okwill be red here for that reason.Refs #1701
Checklist
git commit -s)scripts/test.sh) — see fix(extraction): keep Ruby superclass names bare #1709 for the full-leg numbers; all failures are pre-existing on unmodifiedorigin/main.make -f Makefile.cbm lint-ci)Assisted by OpenCode using anthropic/claude-opus-5.