mirror of
https://github.com/abhigyanpatwari/GitNexus.git
synced 2026-08-28 05:25:25 +00:00
fix(typescript): address review findings — formatting + tighter test assertions
Addresses the automated review findings on PR #1175: - prettier --write the 3 files flagged by `quality / format` CI check (query.ts, typescript-hof-callbacks.test.ts, typescript-jsx-as-call.test.ts). - [medium] typescript-jsx-as-call.test.ts: tighten the combined HOF+JSX assertion from `toBeGreaterThan(0)` to `toHaveLength(1)`. A single `<Foo />` is one logical invocation; the bounds-only assertion would have masked a duplicate-CALLS-edge regression (e.g. if both `jsx_self_closing_element` and a generic call pattern matched the same site). - [medium] typescript-hof-callbacks.test.ts: replace the vacuously-true `for (c of calls) expect(...)` Zustand assertion with a structural one. Old form passed unconditionally when `calls` was empty (any change that silenced ALL CALLS edges from store.ts would have slipped through). New form asserts both: (a) at least one File-rooted edge exists (proving the `isCallerAnchorLabel` fallback fires), and (b) no edge sources from anything else (proving the fallback fires exclusively). - [low] finalize-algorithm.ts (`findExportByName`): rephrase the comment to make the language-agnostic nature of the tie-break rule explicit. The implementation was already correct for all migrated languages; only the comment overplayed the TypeScript specificity. - [low] captures.ts (arity synthesis): add a comment explaining why JSX call anchors (`jsx_self_closing_element` / `jsx_opening_element`) intentionally don't synthesize `@reference.arity`. Name-only resolution is correct for React (components aren't overloaded in the current graph model); a JSX-aware synthesizer counting jsx_attribute children would be needed if that ever changes. No production behavior change. All 8/8 HOF + 7/7 JSX + 236/236 typescript + 11/11 api-deep-flow integration tests still pass. gitnexus and gitnexus-shared typechecks clean. Made-with: Cursor
This commit is contained in:
parent
7be595d317
commit
851d2ab749
5 changed files with 71 additions and 37 deletions
|
|
@ -740,18 +740,30 @@ function findExportByName(
|
|||
defs: readonly SymbolDefinition[],
|
||||
name: string,
|
||||
): SymbolDefinition | undefined {
|
||||
// Languages like TypeScript can emit MULTIPLE `SymbolDefinition`s with
|
||||
// the same simple name from a single declaration: `const fn = () =>
|
||||
// {}` produces both a `Function` def (from `@declaration.function` on
|
||||
// the inner arrow) AND a `Variable` def (from the generic
|
||||
// `@declaration.variable` pattern matching the wrapping
|
||||
// `lexical_declaration`). Both end up in `localDefs`. The CALLER who
|
||||
// writes `import { fn }` wants the callable, not the variable
|
||||
// shadow — without a preference rule, capture order silently decides
|
||||
// which def the import binds to, which broke cross-file CALLS edges
|
||||
// for arrow-typed exports (see `typescript-hof-callbacks.test.ts`).
|
||||
// GENERIC RULE (applies to every language using this finalize
|
||||
// algorithm): when MULTIPLE `SymbolDefinition`s share the same simple
|
||||
// name in `localDefs`, prefer callable / type-like defs over plain
|
||||
// value defs (`Variable`, `Property`, …). The CALLER side of an
|
||||
// import almost always wants the callable, not a value shadow that
|
||||
// happens to share the name — and without a deterministic
|
||||
// preference, capture order silently decides which def the import
|
||||
// binds to.
|
||||
//
|
||||
// Prefer callable / class-like defs; fall back to first-name-match.
|
||||
// The single-def case is unchanged: when only one def has the name,
|
||||
// it's returned regardless of its type (the `fallback` path below).
|
||||
//
|
||||
// TypeScript is the first known language where this matters in
|
||||
// practice: `const fn = () => {}` emits BOTH a `Function` def (from
|
||||
// `@declaration.function` on the inner arrow) AND a `Variable` def
|
||||
// (from the generic `@declaration.variable` pattern matching the
|
||||
// wrapping `lexical_declaration`), and consumers of `import { fn }`
|
||||
// need to bind to the callable. Other migrated languages don't
|
||||
// currently produce dual emits of this shape, so the rule is a no-op
|
||||
// for them today; future languages get the same correctness
|
||||
// guarantee for free if they ever do.
|
||||
//
|
||||
// See `gitnexus/test/integration/resolvers/typescript-hof-callbacks.test.ts`
|
||||
// for the cross-file regression this rule prevents.
|
||||
let fallback: SymbolDefinition | undefined;
|
||||
for (const d of defs) {
|
||||
if (deriveSimpleName(d) !== name) continue;
|
||||
|
|
|
|||
|
|
@ -240,6 +240,20 @@ export function emitTsScopeCaptures(
|
|||
// arity filter can narrow overloads. Count the `argument` named
|
||||
// children of the backing `arguments` node. TypeScript constructor
|
||||
// calls use `new_expression`; regular calls use `call_expression`.
|
||||
//
|
||||
// JSX call anchors (`jsx_self_closing_element` / `jsx_opening_element`
|
||||
// captured by the TSX-only suffix in `query.ts`) intentionally do
|
||||
// NOT carry arity metadata. The lookup below would resolve `callNode`
|
||||
// to `null` for a JSX anchor (the anchor is neither a call_expression
|
||||
// nor a new_expression), so the synthesis branch silently no-ops and
|
||||
// the JSX call enters the registry with name-only resolution. This
|
||||
// is acceptable for React: components are virtually never
|
||||
// overloaded in the current GitNexus graph model, so name-only
|
||||
// dispatch matches the single component definition. If a future
|
||||
// codebase introduces overloaded React components AND needs JSX
|
||||
// calls to disambiguate by props-arity, a JSX-aware arity
|
||||
// synthesizer would need to count `jsx_attribute` children of the
|
||||
// opening tag instead of `arguments`.
|
||||
const callAnchor = pickFirstDefined(grouped, CALL_TAGS);
|
||||
if (callAnchor !== undefined && grouped['@reference.arity'] === undefined) {
|
||||
const callNode =
|
||||
|
|
|
|||
|
|
@ -823,10 +823,7 @@ export function getTsParser(filePath?: string): Parser {
|
|||
export function getTsScopeQuery(filePath?: string): Parser.Query {
|
||||
if (filePath !== undefined && isTsxFile(filePath)) {
|
||||
if (_tsxQuery === null) {
|
||||
_tsxQuery = new Parser.Query(
|
||||
TSX_GRAMMAR,
|
||||
TYPESCRIPT_SCOPE_QUERY + TSX_JSX_QUERY_SUFFIX,
|
||||
);
|
||||
_tsxQuery = new Parser.Query(TSX_GRAMMAR, TYPESCRIPT_SCOPE_QUERY + TSX_JSX_QUERY_SUFFIX);
|
||||
}
|
||||
return _tsxQuery;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -48,10 +48,7 @@ describe('TypeScript HOF-callback CALLS edges', () => {
|
|||
let result: PipelineResult;
|
||||
|
||||
beforeAll(async () => {
|
||||
result = await runPipelineFromRepo(
|
||||
path.join(FIXTURES, 'typescript-hof-callbacks'),
|
||||
() => {},
|
||||
);
|
||||
result = await runPipelineFromRepo(path.join(FIXTURES, 'typescript-hof-callbacks'), () => {});
|
||||
}, 60000);
|
||||
|
||||
it('control: direct (x) => transform(x) emits direct → transform', () => {
|
||||
|
|
@ -108,21 +105,30 @@ describe('TypeScript HOF-callback CALLS edges', () => {
|
|||
expect(fromCreate, 'create() must not be a phantom caller').toEqual([]);
|
||||
});
|
||||
|
||||
it('Zustand call expressions are attributable (to File or absent — never to a wrong sibling)', () => {
|
||||
// The complement check: if any CALLS edge is emitted for the
|
||||
// module-scope calls in store.ts, its source must be either
|
||||
// `store.ts` (the File fallback) or undefined. We accept zero
|
||||
// edges here as a valid outcome; the strict assertion is the
|
||||
// anti-self-loop one above.
|
||||
it('Zustand module-level calls source from the File node (not a sibling Function)', () => {
|
||||
// The positive complement to the anti-self-loop assertion above:
|
||||
// module-level calls in `store.ts` (`create()`, `devtools(...)`,
|
||||
// `persist(...)`) MUST attribute to the `File` node — that's the
|
||||
// entire point of `isCallerAnchorLabel` excluding `Variable` from
|
||||
// the caller-walk fallback. If the fix regresses (Variable defs
|
||||
// re-enter the fallback, or the walk-up grabs a sibling Function),
|
||||
// the source would change away from `File:store.ts`.
|
||||
//
|
||||
// Earlier formulation iterated `for (c of calls)` and asserted each
|
||||
// edge sourced from File. That passed VACUOUSLY when `calls` was
|
||||
// empty — any change that silenced ALL CALLS edges from `store.ts`
|
||||
// would have slipped through. The structural assertion below is
|
||||
// explicit: at least one File-rooted edge must exist (proving the
|
||||
// fallback fired), and no edge may source from anything else
|
||||
// (proving the fallback fired EXCLUSIVELY, not as one option
|
||||
// alongside a buggy sibling-Function attribution).
|
||||
const calls = getRelationships(result, 'CALLS').filter(
|
||||
(c) => c.sourceFilePath === 'src/store.ts',
|
||||
);
|
||||
for (const c of calls) {
|
||||
// Source must NOT be a sibling local Function. The only
|
||||
// acceptable source for module-level calls in store.ts is the
|
||||
// File node itself (label 'File', name 'store.ts').
|
||||
expect([c.sourceLabel, c.source]).toEqual(['File', 'store.ts']);
|
||||
}
|
||||
const fromFile = calls.filter((c) => c.sourceLabel === 'File' && c.source === 'store.ts');
|
||||
const fromOther = calls.filter((c) => !(c.sourceLabel === 'File' && c.source === 'store.ts'));
|
||||
expect(fromOther, 'no module-level call may attribute to a non-File source').toEqual([]);
|
||||
expect(fromFile.length, 'at least one File-rooted call edge must exist').toBeGreaterThan(0);
|
||||
});
|
||||
|
||||
it('transform is reachable from at least 3 of {direct, fanOut, wrap}', () => {
|
||||
|
|
|
|||
|
|
@ -35,10 +35,7 @@ describe('TypeScript JSX-as-call CALLS edges', () => {
|
|||
let result: PipelineResult;
|
||||
|
||||
beforeAll(async () => {
|
||||
result = await runPipelineFromRepo(
|
||||
path.join(FIXTURES, 'typescript-jsx-as-call'),
|
||||
() => {},
|
||||
);
|
||||
result = await runPipelineFromRepo(path.join(FIXTURES, 'typescript-jsx-as-call'), () => {});
|
||||
}, 60000);
|
||||
|
||||
it('self-closing <Foo /> emits useFoo → Foo', () => {
|
||||
|
|
@ -110,14 +107,22 @@ describe('TypeScript JSX-as-call CALLS edges', () => {
|
|||
expect(accesses).toEqual([]);
|
||||
});
|
||||
|
||||
it('combined HOF + JSX: const Wrapped = () => <Foo /> emits Wrapped → Foo', () => {
|
||||
it('combined HOF + JSX: const Wrapped = () => <Foo /> emits exactly one Wrapped → Foo', () => {
|
||||
// Probes the interaction between the HOF-callback caller-attribution
|
||||
// fix and the JSX-as-call fix. Pre-this-PR: caller mis-attribution
|
||||
// (HOF bug) plus invisible JSX (this fix's bug) both broke this
|
||||
// case. Post-PR: both are fixed and the edge lands.
|
||||
//
|
||||
// Asserts EXACTLY ONE edge: a single self-closing `<Foo />` is one
|
||||
// logical invocation. If the JSX query suffix ever accidentally
|
||||
// double-matched the same site (e.g. both
|
||||
// `jsx_self_closing_element` and a generic call pattern firing, or
|
||||
// both an opening-tag and a closing-tag capture), this would catch
|
||||
// the regression — duplicate CALLS edges silently inflate
|
||||
// blast-radius counts in `gitnexus_impact`.
|
||||
const calls = getRelationships(result, 'CALLS').filter(
|
||||
(c) => c.source === 'Wrapped' && c.target === 'Foo',
|
||||
);
|
||||
expect(calls.length).toBeGreaterThan(0);
|
||||
expect(calls).toHaveLength(1);
|
||||
});
|
||||
});
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue