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([]); });