From 97e1a05a517507c4b684ea90f4ed14db405def65 Mon Sep 17 00:00:00 2001 From: Gergo Magyar Date: Tue, 21 Apr 2026 13:50:08 +0100 Subject: [PATCH] refactor(scope-resolution): remove unused shouldShadow / shouldCreateScope hooks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both LanguageProvider hooks were dead weight: - `shouldShadow` had zero call sites — the interface declared it, Python implemented a trivial always-true no-op, but no consumer ever read it. The shadowing decision lives in pythonMergeBindings and the central merge algorithm, not in a per-scope predicate. - `shouldCreateScope` had one call site in pass1BuildScopes but the only language implementing it (Python) always returned true. No producer ever emits a `@scope.block` for Python, so the hook's "declines to create" branch was unreachable. Other languages didn't implement it at all. Removing both: - Drops the interface declarations in language-provider.ts. - Drops `shouldCreateScope` from ScopeExtractorHooks Pick and from the pass1BuildScopes conditional — the stack-based parent-resolve loop becomes unconditional. - Drops pythonShouldShadow / pythonShouldCreateScope from simple-hooks, the Python index barrel, and the python.ts provider wiring. - Drops the tests that exercised the removed hooks: one block- suppression scenario in scope-extractor.test.ts, one shouldCreateScope test in parse-worker-scope-integration.test.ts, and the pythonShouldShadow / pythonShouldCreateScope always-true assertions in python-hooks.test.ts. pythonBindingScopeFor's delegate-to-default test is preserved in its own describe block. Shadowing itself is unchanged: pythonMergeBindings still runs, LEGB ordering still applies, wildcard transparency is still handled via the merge precedence rules. The hook API just no longer has a vestigial per-scope toggle we decided not to use. Verification: 204/204 test/integration/resolvers/python.test.ts both REGISTRY_PRIMARY_PYTHON=0 and =1. 335/335 scope-resolution + graph unit tests (was 339, net -4 after removing the hook-specific assertions). tsc clean. --- .../src/core/ingestion/language-provider.ts | 26 ------------------- .../src/core/ingestion/languages/python.ts | 4 --- .../core/ingestion/languages/python/index.ts | 2 -- .../languages/python/simple-hooks.ts | 22 ---------------- .../src/core/ingestion/scope-extractor.ts | 20 ++++---------- .../parse-worker-scope-integration.test.ts | 19 +------------- .../python/python-hooks.test.ts | 16 +++--------- .../scope-resolution/scope-extractor.test.ts | 20 -------------- 8 files changed, 9 insertions(+), 120 deletions(-) diff --git a/gitnexus/src/core/ingestion/language-provider.ts b/gitnexus/src/core/ingestion/language-provider.ts index f449aa46b..7ab0d33fc 100644 --- a/gitnexus/src/core/ingestion/language-provider.ts +++ b/gitnexus/src/core/ingestion/language-provider.ts @@ -386,19 +386,6 @@ interface LanguageProviderConfig { */ readonly resolveScopeKind?: (captures: CaptureMatch) => ScopeKind | null; - /** - * Should this scope capture materialize as a real `Scope` node? Return - * `false` to skip scope creation while still emitting declarations that - * would have gone inside (they attach to the enclosing real scope). - * - * Example: Python `if`/`for`/`while` bodies capture as `@scope.block` but - * Python has no block scope — hook returns `false` and child declarations - * lift to the enclosing function/module. - * - * Default: undefined (treated as `true` — always create). - */ - readonly shouldCreateScope?: (captures: CaptureMatch) => boolean; - /** * Override where a declaration's name becomes visible. By default the name * is bound in the innermost enclosing scope; return a different `ScopeId` @@ -510,19 +497,6 @@ interface LanguageProviderConfig { // ── Resolution phase (RFC §4v2) ──────────────────────────────────── - /** - * Does a binding at this scope shadow bindings of the same name in outer - * scopes? Default: any binding shadows (standard lexical scoping). Return - * `false` for transparent-scope edge cases (Python `from x import *` - * contexts, JS `var` hoisting quirks, COBOL PARAGRAPH transparency). - * - * Consulted by `Registry.lookup` Step 1 and by `resolveTypeRef` for - * shadowing decisions during the lexical chain walk. - * - * Default: undefined (treated as `true` — any binding shadows). - */ - readonly shouldShadow?: (scope: Scope, bindings: readonly BindingRef[]) => boolean; - /** * Is this callable definition compatible with the given call-site arity? * Language-specific rules: Python `*args`/`**kwargs`/defaults, JS default diff --git a/gitnexus/src/core/ingestion/languages/python.ts b/gitnexus/src/core/ingestion/languages/python.ts index f669b6dc9..6b8686e33 100644 --- a/gitnexus/src/core/ingestion/languages/python.ts +++ b/gitnexus/src/core/ingestion/languages/python.ts @@ -38,8 +38,6 @@ import { pythonImportOwningScope, pythonMergeBindings, pythonReceiverBinding, - pythonShouldCreateScope, - pythonShouldShadow, resolvePythonImportTarget, } from './python/index.js'; @@ -98,11 +96,9 @@ export const pythonProvider = defineLanguage({ emitScopeCaptures: emitPythonScopeCaptures, interpretImport: interpretPythonImport, interpretTypeBinding: interpretPythonTypeBinding, - shouldCreateScope: pythonShouldCreateScope, bindingScopeFor: pythonBindingScopeFor, importOwningScope: pythonImportOwningScope, mergeBindings: pythonMergeBindings, - shouldShadow: pythonShouldShadow, receiverBinding: pythonReceiverBinding, arityCompatibility: pythonArityCompatibility, resolveImportTarget: resolvePythonImportTarget, diff --git a/gitnexus/src/core/ingestion/languages/python/index.ts b/gitnexus/src/core/ingestion/languages/python/index.ts index 9643e781c..c3e502ec0 100644 --- a/gitnexus/src/core/ingestion/languages/python/index.ts +++ b/gitnexus/src/core/ingestion/languages/python/index.ts @@ -80,9 +80,7 @@ export { pythonMergeBindings } from './merge-bindings.js'; export { pythonArityCompatibility } from './arity.js'; export { resolvePythonImportTarget, type PythonResolveContext } from './import-target.js'; export { - pythonShouldCreateScope, pythonBindingScopeFor, pythonImportOwningScope, - pythonShouldShadow, pythonReceiverBinding, } from './simple-hooks.js'; diff --git a/gitnexus/src/core/ingestion/languages/python/simple-hooks.ts b/gitnexus/src/core/ingestion/languages/python/simple-hooks.ts index 914ae235c..66c4e9ec5 100644 --- a/gitnexus/src/core/ingestion/languages/python/simple-hooks.ts +++ b/gitnexus/src/core/ingestion/languages/python/simple-hooks.ts @@ -7,7 +7,6 @@ */ import type { - BindingRef, CaptureMatch, ParsedImport, Scope, @@ -16,15 +15,6 @@ import type { TypeRef, } from 'gitnexus-shared'; -// ─── shouldCreateScope ──────────────────────────────────────────────────── - -/** We never emit `@scope.block` for Python (no block scope in the - * language), so this hook only ever sees scopes we explicitly want to - * materialize. Always-on. */ -export function pythonShouldCreateScope(_captures: CaptureMatch): boolean { - return true; -} - // ─── bindingScopeFor ────────────────────────────────────────────────────── /** Python has no block scope, so the central extractor's "innermost @@ -55,18 +45,6 @@ export function pythonImportOwningScope( return null; } -// ─── shouldShadow ───────────────────────────────────────────────────────── - -/** Standard Python lexical scoping. The central default (`true` — any - * binding shadows) is correct. Wildcard transparency is handled by - * `pythonMergeBindings`, not by toggling shadowing here. - * - * Implemented as an explicit pass-through so reviewers don't have to - * re-derive the analysis from absence. */ -export function pythonShouldShadow(_scope: Scope, _bindings: readonly BindingRef[]): boolean { - return true; -} - // ─── receiverBinding ────────────────────────────────────────────────────── /** Look up `self` or `cls` in the function scope's type bindings. diff --git a/gitnexus/src/core/ingestion/scope-extractor.ts b/gitnexus/src/core/ingestion/scope-extractor.ts index 071a162d2..ec702e3a2 100644 --- a/gitnexus/src/core/ingestion/scope-extractor.ts +++ b/gitnexus/src/core/ingestion/scope-extractor.ts @@ -25,7 +25,6 @@ * ## The five passes * * 1. **Build scope tree.** Walk `@scope.*` matches. For each, consult - * `provider.shouldCreateScope` (default true) and * `provider.resolveScopeKind` (default: suffix of the capture name). * Derive parent by lexical-range containment. Hand the resulting * `Scope[]` to `buildScopeTree` for validation. @@ -95,7 +94,6 @@ import type { LanguageProvider } from './language-provider.js'; */ export type ScopeExtractorHooks = Pick< LanguageProvider, - | 'shouldCreateScope' | 'resolveScopeKind' | 'bindingScopeFor' | 'interpretImport' @@ -308,9 +306,7 @@ function draftToScope(draft: ScopeDraft): Scope { /** * Convert `@scope.*` matches into `ScopeDraft[]`. Parent relationships * are derived from range containment (outermost scope containing `range` - * becomes the parent). Scopes with `shouldCreateScope === false` are - * silently omitted — their children reparent to the next enclosing - * real scope. + * becomes the parent). */ function pass1BuildScopes( matches: readonly CaptureMatch[], @@ -321,7 +317,6 @@ function pass1BuildScopes( readonly match: CaptureMatch; readonly range: Range; readonly kind: ScopeKind; - readonly create: boolean; readonly id: ScopeId; } @@ -331,9 +326,8 @@ function pass1BuildScopes( if (anchor === undefined) continue; const kind = resolveKindForScopeMatch(match, anchor, provider); if (kind === null) continue; - const create = provider.shouldCreateScope?.(match) ?? true; const id = makeScopeId({ filePath, range: anchor.range, kind }); - candidates.push({ match, range: anchor.range, kind, create, id }); + candidates.push({ match, range: anchor.range, kind, id }); } // Sort by (startLine, startCol) ASC, (endLine, endCol) DESC so outer @@ -354,13 +348,9 @@ function pass1BuildScopes( stack.pop(); } - if (cand.create) { - const parent = stack.length > 0 ? stack[stack.length - 1]!.id : null; - drafts.push(makeDraft(cand.id, parent, cand.kind, cand.range, filePath)); - stack.push(cand); - } - // If `cand.create === false`, we don't push it onto the stack — child - // scopes will reparent to whatever's below it. + const parent = stack.length > 0 ? stack[stack.length - 1]!.id : null; + drafts.push(makeDraft(cand.id, parent, cand.kind, cand.range, filePath)); + stack.push(cand); } return drafts; diff --git a/gitnexus/test/unit/scope-resolution/parse-worker-scope-integration.test.ts b/gitnexus/test/unit/scope-resolution/parse-worker-scope-integration.test.ts index 5d6959298..c824a18b4 100644 --- a/gitnexus/test/unit/scope-resolution/parse-worker-scope-integration.test.ts +++ b/gitnexus/test/unit/scope-resolution/parse-worker-scope-integration.test.ts @@ -42,9 +42,7 @@ const moduleScopeMatch = (): CaptureMatch => ({ * `ScopeExtractorHooks`); the real worker always has a full provider. */ function fakeProvider( - hooks: Partial< - Pick - >, + hooks: Partial>, ): LanguageProvider { return hooks as unknown as LanguageProvider; } @@ -94,21 +92,6 @@ describe('extractParsedFile', () => { expect(seenText).toBe('the real text'); expect(seenPath).toBe('deep/path/file.ts'); }); - - it('honors provider hooks beyond emitScopeCaptures (shouldCreateScope)', () => { - // A Block scope the provider declines to create — the resulting - // ParsedFile should have only the Module scope, not the Block. - const provider = fakeProvider({ - emitScopeCaptures: () => [ - moduleScopeMatch(), - { '@scope.block': cap('@scope.block', 10, 0, 20, 0) }, - ], - shouldCreateScope: (match) => match['@scope.block'] === undefined, - }); - const result = extractParsedFile(provider, 'src', 'a.ts'); - expect(result!.scopes).toHaveLength(1); - expect(result!.scopes[0]!.kind).toBe('Module'); - }); }); describe('error resilience — never breaks legacy parsing', () => { diff --git a/gitnexus/test/unit/scope-resolution/python/python-hooks.test.ts b/gitnexus/test/unit/scope-resolution/python/python-hooks.test.ts index c729abf5d..389a9f448 100644 --- a/gitnexus/test/unit/scope-resolution/python/python-hooks.test.ts +++ b/gitnexus/test/unit/scope-resolution/python/python-hooks.test.ts @@ -23,8 +23,6 @@ import { pythonImportOwningScope, pythonMergeBindings, pythonReceiverBinding, - pythonShouldShadow, - pythonShouldCreateScope, pythonBindingScopeFor, resolvePythonImportTarget, } from '../../../../src/core/ingestion/languages/python/index.js'; @@ -217,18 +215,10 @@ describe('pythonImportOwningScope', () => { }); }); -// ─── shouldShadow / shouldCreateScope / bindingScopeFor — defensive ─────── +// ─── bindingScopeFor — defensive ────────────────────────────────────────── -describe('pythonShouldShadow / pythonShouldCreateScope / pythonBindingScopeFor', () => { - it('shouldShadow always returns true (standard LEGB)', () => { - expect(pythonShouldShadow(fnScope(), [])).toBe(true); - }); - - it('shouldCreateScope always returns true (no @scope.block emitted)', () => { - expect(pythonShouldCreateScope({})).toBe(true); - }); - - it('bindingScopeFor delegates to default for every input', () => { +describe('pythonBindingScopeFor', () => { + it('delegates to default for every input', () => { expect(pythonBindingScopeFor({}, fnScope(), {} as never)).toBeNull(); }); }); diff --git a/gitnexus/test/unit/scope-resolution/scope-extractor.test.ts b/gitnexus/test/unit/scope-resolution/scope-extractor.test.ts index 0a03e95f7..250294592 100644 --- a/gitnexus/test/unit/scope-resolution/scope-extractor.test.ts +++ b/gitnexus/test/unit/scope-resolution/scope-extractor.test.ts @@ -157,26 +157,6 @@ describe('Pass 1: scope tree', () => { for (const fn of fns) expect(fn.parent).toBe(mod.id); }); - it('honors `provider.shouldCreateScope === false` by reparenting children to next real scope', () => { - // Block at [10:0..25:0] is SUPPRESSED; inner function at [12:0..20:0] - // should reparent to the enclosing Module instead of the Block. - const result = extract( - [ - scopeMatch('module', 1, 0, 100, 0), - scopeMatch('block', 10, 0, 25, 0), - scopeMatch('function', 12, 0, 20, 0), - ], - 'a.ts', - mockProvider({ - shouldCreateScope: (match) => match['@scope.block'] === undefined, - }), - ); - const mod = result.scopes.find((s) => s.kind === 'Module')!; - const fn = result.scopes.find((s) => s.kind === 'Function')!; - expect(result.scopes).toHaveLength(2); // block suppressed - expect(fn.parent).toBe(mod.id); - }); - it('uses `provider.resolveScopeKind` to override the default kind from the suffix', () => { // Provider upgrades a `@scope.block` to `Expression` for a comprehension- // style use case.