mirror of
https://github.com/abhigyanpatwari/GitNexus.git
synced 2026-09-10 22:43:40 +00:00
fix(review): inspect the module scope in the owner-shadow guard, and check the TSX suffix
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.
This commit is contained in:
parent
daceee15a3
commit
aea0ab06c9
7 changed files with 135 additions and 7 deletions
60
DECISIONS.md
60
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.
|
||||
|
|
|
|||
|
|
@ -1346,7 +1346,13 @@ export const TYPESCRIPT_SCOPE_QUERY = `
|
|||
* (\`<Foo>...</Foo>\`) 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 = `
|
||||
;; <Foo />
|
||||
((jsx_self_closing_element
|
||||
name: (identifier) @reference.name) @reference.call.free
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
|
|
|
|||
|
|
@ -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.
|
||||
|
|
|
|||
10
gitnexus/test/fixtures/lang-resolution/zig-idioms/src/webapi/Gauge.zig
vendored
Normal file
10
gitnexus/test/fixtures/lang-resolution/zig-idioms/src/webapi/Gauge.zig
vendored
Normal file
|
|
@ -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;
|
||||
}
|
||||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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([]);
|
||||
});
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue