From 83b029f09e1e01f0632941cdcb3183292463e922 Mon Sep 17 00:00:00 2001 From: Gergo Magyar Date: Fri, 25 Sep 2026 15:26:37 +0000 Subject: [PATCH] fix(ruby): skip only the multi-statement branch in callable alternatives (#3354) rubyValueAlternatives returned `[node]` (opaque) as soon as one branch held more than one statement. Two things went wrong because of that: - At the top level, `run = if c then g = h; 0 else method(:f) end` dropped the single-statement `else`, so `f` got no flow edge. - In an elsif chain, the outer `if` pushed the `elsif` node as a branch. The shared loop called the hook on it again, got `[elsif]` back, and used the whole elsif subtree as one source. So in `if a then method(:run_a) elsif b then g = h; 0 else method(:run_b) end` the `run_b` edge was lost. The hook now walks the elsif chain itself and skips each multi-statement branch, while every single-statement branch still becomes an alternative. This cannot add a wrong edge: each emitted alternative is a value the conditional really evaluates to, and nothing is taken from the skipped branch. Before, that branch did not contribute a resolvable value either. A whole conditional used as a source becomes a qualified-name seed, which resolves to nothing when it has more than one identifier leaf. With a single identifier leaf, it could even seed the condition variable. When no branch is a single statement, the conditional still stays one opaque source, as before. Ruby captures golden: ruby-callable-alternatives/app.rb goes from 33 to 58 capture groups. 23 of them come from the new fixture functions (checked against the old source). The other 2 are the new branch alternatives, `statement_if -> run_sweep` and `elsif_chain -> run_b`. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../core/ingestion/languages/ruby/captures.ts | 34 +++++++++++-------- .../ruby-callable-alternatives/app.rb | 13 ++++++- .../expected-captures.json | 4 +-- .../callable-alternatives-providers.test.ts | 13 ++++++- 4 files changed, 45 insertions(+), 19 deletions(-) diff --git a/gitnexus/src/core/ingestion/languages/ruby/captures.ts b/gitnexus/src/core/ingestion/languages/ruby/captures.ts index eb046f509..8482f788d 100644 --- a/gitnexus/src/core/ingestion/languages/ruby/captures.ts +++ b/gitnexus/src/core/ingestion/languages/ruby/captures.ts @@ -72,26 +72,30 @@ const RUBY_CALLABLE_CAPTURE_OPTIONS = { * branches are `then` / `else` STATEMENT LISTS, so the shared ternary rule * would dig an identifier out of whichever statement it found (`g = h; 0` * flowed `h`, although the branch evaluates to `0`). A branch holding one - * statement is that statement's value; anything longer keeps the whole - * conditional one opaque source. The `c ? a : b` ternary (`conditional`) is - * left to the shared rule. + * statement is that statement's value. A longer branch is skipped: it adds + * no alternative, while the other branches still flow. Every alternative + * emitted is a value the conditional really evaluates to, so skipping cannot + * add an edge. It only drops the skipped branch's own value. The `elsif` + * chain is walked here, so one multi-statement `elsif` no longer hides the + * branches after it. When no branch is a single statement, the whole + * conditional stays one opaque source. The `c ? a : b` ternary + * (`conditional`) is left to the shared rule. */ function rubyValueAlternatives(node: SyntaxNode): readonly SyntaxNode[] | undefined { if (node.type !== 'if' && node.type !== 'unless' && node.type !== 'elsif') return undefined; const branches: SyntaxNode[] = []; - for (const field of ['consequence', 'alternative'] as const) { - const branch = node.childForFieldName(field); - if (branch === null) continue; - if (branch.type === 'elsif') { - branches.push(branch); - continue; + for (let link: SyntaxNode | null = node; link !== null; ) { + const consequence: SyntaxNode | null = link.childForFieldName('consequence'); + const alternative: SyntaxNode | null = link.childForFieldName('alternative'); + link = alternative?.type === 'elsif' ? alternative : null; + for (const branch of [consequence, link === null ? alternative : null]) { + if (branch === null) continue; + const statements = branch.namedChildren.filter( + (child): child is SyntaxNode => child !== null && child.type !== 'comment', + ); + const [statement] = statements; + if (statements.length === 1 && statement !== undefined) branches.push(statement); } - const statements = branch.namedChildren.filter( - (child): child is SyntaxNode => child !== null && child.type !== 'comment', - ); - const [statement] = statements; - if (statements.length !== 1 || statement === undefined) return [node]; - branches.push(statement); } return branches.length > 0 ? branches : [node]; } diff --git a/gitnexus/test/fixtures/lang-resolution/ruby-callable-alternatives/app.rb b/gitnexus/test/fixtures/lang-resolution/ruby-callable-alternatives/app.rb index 205db44f1..f54ee4235 100644 --- a/gitnexus/test/fixtures/lang-resolution/ruby-callable-alternatives/app.rb +++ b/gitnexus/test/fixtures/lang-resolution/ruby-callable-alternatives/app.rb @@ -2,6 +2,9 @@ def run_other; end def run_sweep; end def run_then; end def run_else; end +def run_a; end +def run_b; end +def run_inner; end # Each branch holds one statement, so each branch is the value. def single_statement_if(fast) @@ -14,7 +17,7 @@ def single_statement_if(fast) end # The `then` branch evaluates to 0; `h` is only read by an inner statement, -# so it must not flow into `run`. +# so it must not flow into `run`. The single-statement `else` still does. def statement_if(fast) h = method(:run_other) run = if fast @@ -25,3 +28,11 @@ def statement_if(fast) end run.call end + +# The multi-statement `elsif` branch is skipped; the branches on either side +# of it still flow, and nothing inside it does. +def elsif_chain(a, b) + h = method(:run_inner) + run = if a then method(:run_a) elsif b then g = h; 0 else method(:run_b) end + run.call +end diff --git a/gitnexus/test/fixtures/ruby-captures-golden/expected-captures.json b/gitnexus/test/fixtures/ruby-captures-golden/expected-captures.json index cc2d731e9..ef9d32b63 100644 --- a/gitnexus/test/fixtures/ruby-captures-golden/expected-captures.json +++ b/gitnexus/test/fixtures/ruby-captures-golden/expected-captures.json @@ -40,8 +40,8 @@ "digest": "194c1ca21a7d5d8d85cf9aecfdc4edd881c5d3447de1ad574b56cff5a411bab1" }, "ruby-callable-alternatives/app.rb": { - "captureGroups": 33, - "digest": "d3fe15a75710d65ed0d5116d9fcc7b6891e460ff037a478472fda999161aed49" + "captureGroups": 58, + "digest": "7a7fcac9fd4dae83dfc78eba2864d06b48918eb42713c0429081fdd986b7e8ba" }, "ruby-calls/lib/one_arg.rb": { "captureGroups": 8, diff --git a/gitnexus/test/integration/resolvers/callable-alternatives-providers.test.ts b/gitnexus/test/integration/resolvers/callable-alternatives-providers.test.ts index dbcecde35..dbce87fd9 100644 --- a/gitnexus/test/integration/resolvers/callable-alternatives-providers.test.ts +++ b/gitnexus/test/integration/resolvers/callable-alternatives-providers.test.ts @@ -8,7 +8,7 @@ * hook; without it only the LAST operand flowed and `impact` under-reported * callers while still claiming `epistemic: "exact"`. Ruby's statement-bodied * `if` shares the ternary's field names but its branches are statement lists, - * so its hook only expands single-statement branches. + * so its hook only expands single-statement branches and skips the rest. */ import { describe, it, expect, beforeAll } from 'vitest'; import path from 'path'; @@ -125,4 +125,15 @@ describe('Ruby statement-bodied `if` as a callable source', () => { it('an identifier read inside a multi-statement branch does not flow into the binding', () => { expect(callsOf(result)).not.toContain('statement_if → run_other'); }); + + it('a multi-statement branch is skipped while its sibling branch still flows', () => { + expect(callsOf(result)).toContain('statement_if → run_sweep'); + }); + + it('a multi-statement `elsif` does not hide the branches around it', () => { + expect(callsOf(result)).toEqual( + expect.arrayContaining(['elsif_chain → run_a', 'elsif_chain → run_b']), + ); + expect(callsOf(result)).not.toContain('elsif_chain → run_inner'); + }); });