From aea0ab06c99839e053001b2f97db954029161abb Mon Sep 17 00:00:00 2001 From: Navid EMAD Date: Tue, 8 Sep 2026 23:47:09 +0200 Subject: [PATCH] fix(review): inspect the module scope in the owner-shadow guard, and check the TSX suffix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `gitnexus-check` pass on `bd6e577e` carried three findings of its own, and I answered the later `1c7c05ff` pass without noticing them. Recording that as a process failure too: bot reviews are per-head, and a later pass does not necessarily repeat an earlier one's findings. R6-1 (Error, valid, REPRODUCED) — the shadow guard stopped one rung short. `isOwnerNameShadowedBySomethingElse` returned `false` on reaching the module scope, justified as "a container declared there IS the binding, and the caller already resolved it". That holds when the owner came from the scope chain and fails when it came from the workspace-wide qualified-name fallback: // Gauge.zig — never imported by Element.zig const Gauge = @This(); pub fn read(self: *Gauge) u8 { … } // Element.zig const Gauge = @import("dom_utils.zig").DEFAULT_NS; // NOT a container pub const level = bridge.accessor(Gauge.read, null, .{}); // → Gauge.zig's read `findClassBindingInScope` steps over the module-scope binding because it is not class-like, answers from `scopes.qualifiedNames`, and the guard waved it through. Worth recording: the first fixture attempt did NOT reproduce. A local `const Gauge: u8 = 3;` also claims the workspace qualified name `Gauge`, leaving two candidates, and the fallback refuses to guess between two — so the shape defeated itself. Binding the name by IMPORT claims no qualified name, the fallback stays unique, and it fires. A negative result on the first shape was not evidence the finding was wrong. Fixed by inspecting the module scope as the last rung instead of skipping it. The identity exemption is what makes that safe where `isNamespaceNameShadowed` cannot do it (#2723: a namespace import writes its own name into the module scope and would read as its own shadow) — the binding that IS the owner exempts itself, and only a binding to something else answers `true`. `lookupBindingsAt` is consulted at that scope and only there, because an imported alias lives in the finalized channel rather than in `scope.bindings`. R6-2 — the hub re-export finding, already fixed in `5d8fe9d8`; the same defect restated on the later head. R6-3 (valid, fixed) — the dispatchability canary omitted the TSX suffix. `getTsScopeQuery` analyzes a `.tsx` file with `TYPESCRIPT_SCOPE_QUERY + TSX_JSX_QUERY_SUFFIX`, and the test read only the base, so a `value-ref` rule added to the suffix would be emitted in TSX analysis with the canary green. The suffix is now exported and concatenated into the check; verified load-bearing by adding an unkeyed `jsx_expression` value-ref rule to it, which fails the TypeScript case. Gates: tsc --noEmit clean, npm run build clean, prettier clean. `test/integration/resolvers` 3,635 passed / 3 skipped (70 files); `test/unit/scope-resolution` 2,015 passed (120 files); `impact-callable-value-references` 7 passed under `lbug-db`. Bench --check: receiver-resolution, zig-cross-file-resolution, scope-capture (15 languages), scope-emission, callable-value-flow all PASS with no baseline edited. --- DECISIONS.md | 60 +++++++++++++++++++ .../ingestion/languages/typescript/query.ts | 8 ++- .../scope-resolution/scope/walkers.ts | 25 ++++++-- .../zig-idioms/src/webapi/Element.zig | 18 ++++++ .../zig-idioms/src/webapi/Gauge.zig | 10 ++++ .../test/integration/resolvers/zig.test.ts | 11 ++++ .../value-ref-dispatchability.test.ts | 10 +++- 7 files changed, 135 insertions(+), 7 deletions(-) create mode 100644 gitnexus/test/fixtures/lang-resolution/zig-idioms/src/webapi/Gauge.zig diff --git a/DECISIONS.md b/DECISIONS.md index 8d3739315..b2853075f 100644 --- a/DECISIONS.md +++ b/DECISIONS.md @@ -692,3 +692,63 @@ and the run's own output was `"changed_lines": 0` — ESLint reported 0 errors (6868 pre-existing warnings) and Prettier reported every file `(unchanged)`, so a successful run would have produced an empty patch anyway. Pushing this commit triggers a fresh run, after which `/autofix` will answer normally. + +--- + +## Review round 6 — the `bd6e577e` bot pass I had skipped + +Three findings. I answered the `1c7c05ff` pass (round 5) without noticing that +the pass on `bd6e577e` carried its own, one of them an Error. Recorded as a +process failure as much as a code one: reviews are per-head and a later pass does +not necessarily repeat an earlier one's findings. + +**R6-1 (Error, valid, REPRODUCED, fixed) — the shadow guard stopped one rung +short.** `isOwnerNameShadowedBySomethingElse` returned `false` on reaching the +module scope, justified as "a container declared there IS the binding, and the +caller already resolved it". That holds when the owner came from the scope chain +and fails when it came from the workspace fallback: + + // Gauge.zig — never imported by Element.zig + const Gauge = @This(); + pub fn read(self: *Gauge) u8 { … } + + // Element.zig + const Gauge = @import("dom_utils.zig").DEFAULT_NS; // NOT a container + pub const level = bridge.accessor(Gauge.read, null, .{}); // → Gauge.zig's read + +`findClassBindingInScope` steps over the module-scope binding (not class-like) +and answers from `scopes.qualifiedNames`; the guard then waved it through. + +The first fixture attempt did NOT reproduce, and the reason is worth keeping: a +local `const Gauge: u8 = 3;` also claims the workspace qualified name `Gauge`, +leaving two candidates, and the fallback refuses to guess between two. Binding +the name by IMPORT claims no qualified name, so the fallback stays unique and +fires. A negative result on the first shape was not evidence the finding was +wrong. + +Fixed by inspecting the module scope as the last rung rather than skipping it. +The identity exemption is what makes that safe where `isNamespaceNameShadowed` +could not do it (#2723: a namespace import writes its own name into the module +scope and would read as its own shadow) — the binding that IS the owner exempts +itself, and only a binding to something else answers `true`. `lookupBindingsAt` +is consulted at that scope and only there, because an imported alias lives in the +finalized channel rather than in `scope.bindings`. + +**R6-2 — the hub finding, already fixed in round 5** (same defect, restated on +the later head). + +**R6-3 (valid, fixed) — the dispatchability canary omitted the TSX suffix.** +`getTsScopeQuery` analyzes a `.tsx` file with `TYPESCRIPT_SCOPE_QUERY + +TSX_JSX_QUERY_SUFFIX`, and the test read only the base, so a `value-ref` rule +added to the suffix would be emitted in TSX analysis with the canary green. The +suffix is now exported and concatenated into the check; verified load-bearing by +adding an unkeyed `jsx_expression` value-ref rule to the suffix, which fails the +TypeScript case. + +### Gates after review round 6 + +- `tsc --noEmit` clean, `npm run build` clean, `prettier --check` clean. +- `test/integration/resolvers` — 3,635 passed / 3 skipped (70 files). +- `test/unit/scope-resolution` — 2,015 passed (120 files). +- `impact-callable-value-references` under `lbug-db` — 7 passed. +- Bench `--check`: all five PASS, no baseline edited. diff --git a/gitnexus/src/core/ingestion/languages/typescript/query.ts b/gitnexus/src/core/ingestion/languages/typescript/query.ts index babdb273b..ce1afcadd 100644 --- a/gitnexus/src/core/ingestion/languages/typescript/query.ts +++ b/gitnexus/src/core/ingestion/languages/typescript/query.ts @@ -1346,7 +1346,13 @@ export const TYPESCRIPT_SCOPE_QUERY = ` * (\`...\`) emit; the closing tag is intentionally NOT captured — * each JSX element should emit exactly one CALLS edge per use site. */ -const TSX_JSX_QUERY_SUFFIX = ` +/** + * Exported alongside `TYPESCRIPT_SCOPE_QUERY` so + * `value-ref-dispatchability.test.ts` checks the whole query a `.tsx` file is + * analyzed with. Checking the base alone would miss a `value-ref` rule added + * here. Not part of the provider surface — use `getTsScopeQuery`. + */ +export const TSX_JSX_QUERY_SUFFIX = ` ;; ((jsx_self_closing_element name: (identifier) @reference.name) @reference.call.free diff --git a/gitnexus/src/core/ingestion/scope-resolution/scope/walkers.ts b/gitnexus/src/core/ingestion/scope-resolution/scope/walkers.ts index f1f03a5e1..dd7afd2b4 100644 --- a/gitnexus/src/core/ingestion/scope-resolution/scope/walkers.ts +++ b/gitnexus/src/core/ingestion/scope-resolution/scope/walkers.ts @@ -368,9 +368,20 @@ export function isNamespaceNameShadowed( * the lexical race, which is the whole point: `findClassBindingInScope` filters * the chain by `isClassLike` and therefore cannot see that it lost it. * - * Same floor and the same fail-closed posture as `isNamespaceNameShadowed`: the - * module scope is not inspected (a container declared there IS the binding, and - * the caller already resolved it), and a missing scope or a parent cycle answers + * The MODULE scope is inspected too, unlike `isNamespaceNameShadowed`, and the + * exemption is what makes that safe. That guard stops one rung short because a + * namespace import writes its own name into the module scope and would read as + * its own shadow (#2723); here the owner is compared by identity, so the binding + * that IS the owner exempts itself and only a binding to something ELSE answers + * `true`. Stopping short would leave the exact hole this walk exists to close: + * `findClassBindingInScope` steps over a module-scope binding that is not + * class-like and then answers from a WORKSPACE-wide qualified-name index, so + * `const Gauge = @import("other.zig").SOME_CONST;` in a file that never imports + * `Gauge.zig` would still resolve `Gauge.read` to that file's container. + * `lookupBindingsAt` is used at that scope and only there, because an imported + * alias lives in the finalized channel rather than in `scope.bindings`. + * + * Fail-closed like its sibling: a missing scope or a parent cycle answers * `true`, because suppressing a resolution costs a missing edge while trusting a * corrupt chain costs a wrong one. */ @@ -387,12 +398,14 @@ export function isOwnerNameShadowedBySomethingElse( visited.add(currentId); const scope = scopes.scopeTree.getScope(currentId); if (scope === undefined) return true; - if (scope.kind === 'Module') return false; if (scope.kind !== 'Object') { + const isModule = scope.kind === 'Module'; + const imported = isModule ? lookupBindingsAt(currentId, name, scopes) : []; const bindsHere = scope.bindings.has(name) || scope.typeBindings.has(name) || scope.lexicalNames?.has(name) === true || + imported.length > 0 || scope.ownedDefs.some((d) => { const qualifiedName = d.qualifiedName; if (qualifiedName === undefined) return false; @@ -402,9 +415,13 @@ export function isOwnerNameShadowedBySomethingElse( if (bindsHere) { if ((scope.bindings.get(name) ?? []).some((b) => b.def.nodeId === def.nodeId)) return false; if (scope.ownedDefs.some((d) => d.nodeId === def.nodeId)) return false; + if (imported.some((b) => b.def.nodeId === def.nodeId)) return false; return true; } } + // The module scope is the last rung, not a rung to skip: nothing above it + // can shadow a name for this file. + if (scope.kind === 'Module') return false; currentId = scope.parent; } return true; diff --git a/gitnexus/test/fixtures/lang-resolution/zig-idioms/src/webapi/Element.zig b/gitnexus/test/fixtures/lang-resolution/zig-idioms/src/webapi/Element.zig index 3e7291fd9..ecc6d929d 100644 --- a/gitnexus/test/fixtures/lang-resolution/zig-idioms/src/webapi/Element.zig +++ b/gitnexus/test/fixtures/lang-resolution/zig-idioms/src/webapi/Element.zig @@ -19,6 +19,21 @@ const dom_utils = @import("dom_utils.zig"); // A hub module that re-exports `dom_utils`' members without declaring any. const hub = @import("hub.zig"); +// A MODULE-LEVEL binding whose name collides with the container `Gauge.zig` +// declares, bound to something that is NOT a container. This file never imports +// `Gauge.zig`. `findClassBindingInScope` filters the scope chain by +// `isClassLike`, so it walks past this binding, and its workspace-wide +// qualified-name fallback answers with the other file's struct — while the +// shadow guard used to permit exactly this, treating the module scope as a floor +// it need not inspect. +// +// The name is bound by IMPORT rather than by a local `const Gauge: u8 = 3`, +// and that detail is the difference between a live case and a self-defeating +// one: a local declaration would also claim the workspace qualified name +// `Gauge`, leaving two candidates, and the fallback refuses to guess between +// two. An imported alias claims nothing, so the fallback stays unique and fires. +const Gauge = @import("dom_utils.zig").DEFAULT_NS; + _namespace: u8 = 0, // ── Registered accessors ──────────────────────────────────────────────────── @@ -113,6 +128,9 @@ pub const JsApi = struct { // …and the callable gate still applies through the hub. pub const hubNs = bridge.accessor(hub.DEFAULT_NS, null, .{}); + + // `Gauge` names this file's `const Gauge: u8`, not `Gauge.zig`'s container. + pub const level = bridge.accessor(Gauge.read, null, .{}); }; // The CALL form of the same hub member, so the two are pinned side by side. diff --git a/gitnexus/test/fixtures/lang-resolution/zig-idioms/src/webapi/Gauge.zig b/gitnexus/test/fixtures/lang-resolution/zig-idioms/src/webapi/Gauge.zig new file mode 100644 index 000000000..5b6071db4 --- /dev/null +++ b/gitnexus/test/fixtures/lang-resolution/zig-idioms/src/webapi/Gauge.zig @@ -0,0 +1,10 @@ +// The unique workspace definition of the name `Gauge`. `Element.zig` never +// imports it, and declares a module-local `const Gauge` of its own — see +// `readsThroughAShadowedContainerName` there. +const Gauge = @This(); + +_level: u8 = 0, + +pub fn read(self: *Gauge) u8 { + return self._level; +} diff --git a/gitnexus/test/integration/resolvers/zig.test.ts b/gitnexus/test/integration/resolvers/zig.test.ts index a7767bc55..00ae59c6d 100644 --- a/gitnexus/test/integration/resolvers/zig.test.ts +++ b/gitnexus/test/integration/resolvers/zig.test.ts @@ -417,6 +417,17 @@ describe.skipIf(!zigAvailable)('Zig idioms (zig-idioms fixture)', () => { expect(valueRefs).toContain('registersALocalContainer → go'); }); + it('declines a container-qualified reference shadowed at MODULE scope', () => { + // `Element.zig` declares `const Gauge: u8 = 3;` at module scope and never + // imports `Gauge.zig`, which declares the container. The class walk filters + // by `isClassLike`, steps over the `const`, and its workspace-wide + // qualified-name fallback answers with the other file's struct. The shadow + // guard has to inspect the MODULE scope to catch it — stopping one rung + // short, as it did, permitted precisely this case. + expect(valueRefs).not.toContain('JsApi → read'); + expect(valueRefTargetIds.filter((id) => id.includes('Gauge'))).toEqual([]); + }); + it('does not mint a value reference for the CALLEE of an ordinary call', () => { // `register(onTick)` must produce ONE value reference (the argument), not // two: without binding the callee to the `function:` field the same rule diff --git a/gitnexus/test/unit/scope-resolution/value-ref-dispatchability.test.ts b/gitnexus/test/unit/scope-resolution/value-ref-dispatchability.test.ts index 2c8df245f..3527587af 100644 --- a/gitnexus/test/unit/scope-resolution/value-ref-dispatchability.test.ts +++ b/gitnexus/test/unit/scope-resolution/value-ref-dispatchability.test.ts @@ -45,7 +45,10 @@ import { describe, it, expect } from 'vitest'; import fs from 'node:fs'; import path from 'node:path'; import { fileURLToPath } from 'node:url'; -import { TYPESCRIPT_SCOPE_QUERY } from '../../../src/core/ingestion/languages/typescript/query.js'; +import { + TSX_JSX_QUERY_SUFFIX, + TYPESCRIPT_SCOPE_QUERY, +} from '../../../src/core/ingestion/languages/typescript/query.js'; import { JAVASCRIPT_SCOPE_QUERY } from '../../../src/core/ingestion/languages/javascript/query.js'; import { ZIG_SCOPE_QUERY } from '../../../src/core/ingestion/languages/zig/query.js'; @@ -124,7 +127,10 @@ describe('value-ref dispatchability partition', () => { }); it('every TypeScript value-ref rule is DISPATCHABLE (carries a property key)', () => { - const rules = valueRefRules(TYPESCRIPT_SCOPE_QUERY); + // The BASE query plus the TSX suffix, because `getTsScopeQuery` concatenates + // them for a `.tsx` file: a value-ref rule added to the suffix alone would + // be emitted in TSX analysis while a base-only check stayed green. + const rules = valueRefRules(TYPESCRIPT_SCOPE_QUERY + TSX_JSX_QUERY_SUFFIX); expect(rules.length).toBeGreaterThan(0); expect(rules.filter((r) => !r.includes(PROPERTY_KEY))).toEqual([]); });