From 29275868120a15c7225c39bbe75915944787aacf Mon Sep 17 00:00:00 2001 From: ReidenXerx Date: Fri, 7 Aug 2026 05:13:48 +0300 Subject: [PATCH] Revert "return-shape anchoring" (R3-4/R3-5): it degrades query MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reverts af5eec5c, c764847a and 4f93f32e. The capability was real and measured — all six fields round 3 verified OUT-OF-SAMPLE went from 0 backend readers to 7/11/10/7/6/14, 0/6 to 6/6, via 1,410 precise return-shape edges. It is reverted anyway, because it costs more than it buys in its current form. `cli-limit-e2e` caught it. Bisected to af5eec5c: on the mini-repo fixture, `query('message')` returned two processes before and NONE after. The mechanism is not window displacement — that hypothesis was tested with a partition that kept function-local property keys from taking window slots, and it changed nothing. Indexing the keys of every returned literal adds many nodes whose names are ordinary words, which moves the BM25 CORPUS statistics: "message" gets less discriminating, and `createLogEntry` — the callable that actually carries the processes — stops ranking at all. A corpus-level effect is not repairable by a tie-break. Trading a regression in `query`, one of the core tools, for coverage in `context` is the wrong trade, and shipping it because the number was good would be the same mistake this PR spent three rounds removing: a confident answer that is worse than the honest one. What the work established, and what re-landing needs: - The mechanism is right. Joining the existing call-result type binding to a named return shape resolves `alert.wickRatio` by EVIDENCE, which is why it succeeds exactly where name inference must refuse. - The cost is search dilution, and it needs to be measured on BM25 ranking BEFORE the capture lands — not discovered by a downstream e2e test. - The likely shape of the fix is keeping return-shape keys out of the text search corpus while keeping them in the graph, which needs persisted provenance rather than the in-memory flag used here. Kept: everything through 8972d223, which is verified green. Co-Authored-By: Claude Opus 5 (1M context) --- .../passes/return-shape-members.ts | 151 ------------------ .../passes/unique-name-properties.ts | 55 +------ .../scope-resolution/pipeline/run.ts | 21 +-- .../src/core/ingestion/tree-sitter-queries.ts | 74 --------- .../src/core/ingestion/utils/ast-helpers.ts | 85 ---------- .../core/ingestion/workers/parse-worker.ts | 16 +- gitnexus/src/storage/parse-cache.ts | 17 +- .../fixture-shapes.test.js | 9 -- .../production-reader.js | 8 - .../return-shape-typed.js | 32 ---- .../return-shape.js | 55 ------- .../typescript-alias-fields/contracts.ts | 9 +- .../mini-repo/expected-graph.json | 10 +- .../javascript-object-properties.test.ts | 132 +-------------- .../resolvers/typescript-alias-fields.test.ts | 5 +- .../test/unit/incremental-parse-cache.test.ts | 4 +- 16 files changed, 23 insertions(+), 660 deletions(-) delete mode 100644 gitnexus/src/core/ingestion/scope-resolution/passes/return-shape-members.ts delete mode 100644 gitnexus/test/fixtures/lang-resolution/javascript-object-properties/fixture-shapes.test.js delete mode 100644 gitnexus/test/fixtures/lang-resolution/javascript-object-properties/production-reader.js delete mode 100644 gitnexus/test/fixtures/lang-resolution/javascript-object-properties/return-shape-typed.js delete mode 100644 gitnexus/test/fixtures/lang-resolution/javascript-object-properties/return-shape.js diff --git a/gitnexus/src/core/ingestion/scope-resolution/passes/return-shape-members.ts b/gitnexus/src/core/ingestion/scope-resolution/passes/return-shape-members.ts deleted file mode 100644 index 567387fff..000000000 --- a/gitnexus/src/core/ingestion/scope-resolution/passes/return-shape-members.ts +++ /dev/null @@ -1,151 +0,0 @@ -/** - * PRECISE member resolution through a call result's RETURN SHAPE (R3-5). - * - * The last unanswered question from three rounds of blind-spot reports was - * "who reads `wickRatio`?", where the field is produced by several functions - * that each return an anonymous object containing it. Name inference must - * refuse that — a read of `spike.wickRatio` could mean any producer, and a - * wrong edge in the pre-edit safety gate is worse than a missing one — so no - * amount of narrowing gets there. It needs EVIDENCE instead of inference. - * - * The evidence already exists in two halves that had never been joined: - * - * 1. The call-result type binding. `const alert = formatSpikeAlert(row)` - * binds `alert` to a `TypeRef` whose `rawName` is the callee. That - * machinery predates this work; it simply had nothing to resolve to when - * the callee returned an anonymous literal, because an anonymous literal - * named nothing. - * 2. R3-4 gave it a name. A returned literal's keys are now owned by the - * producing function, so `formatSpikeAlert.wickRatio` is a real symbol. - * - * Joining them turns a refusal into a precise answer: - * - * const alert = formatSpikeAlert(row); - * alert.wickRatio → Property:…:formatSpikeAlert.wickRatio - * - * and it works for exactly the case narrowing cannot: several producers sharing - * a field name are no longer competitors, because the receiver says WHICH one. - * That is why this runs before the unique-name fallback and registers its sites - * as handled — a precise answer must never be second-guessed by a name match. - * - * BOUND, deliberately. This only fires where the value is BOUND to a name the - * type binding could attach to. A field read off a bare parameter - * (`function f(spike) { return spike.wickRatio }`) still has no receiver type - * here, because typing it requires the CALLER's type to flow in — that is - * inter-procedural and genuinely larger. Those reads keep falling through to - * name inference, and keep being reported when it declines. - */ - -import type { ParsedFile } from 'gitnexus-shared'; -import type { KnowledgeGraph } from '../../../graph/types.js'; -import type { ScopeResolutionIndexes } from '../../model/scope-resolution-indexes.js'; -import type { GraphNodeLookup } from '../graph-bridge/node-lookup.js'; -import { resolveCallerGraphId } from '../graph-bridge/ids.js'; -import { findReceiverTypeBinding } from '../scope/walkers.js'; -import { callableFlowSiteKey } from './callable-value-flow.js'; -import type { PropertyNameIndex } from './unique-name-properties.js'; - -/** - * Confidence for a return-shape member. This is a PRECISE resolution — the - * receiver's binding names the producing function and the member is owned by - * it — so it carries the ordinary emission confidence, not the reduced tier - * name inference uses. Nothing here is guessed. - */ -const RETURN_SHAPE_CONFIDENCE = 0.9; - -const EDGE_REASON = 'scope-resolution: return-shape member'; - -export interface ReturnShapeMemberStats { - /** ACCESSES edges resolved through a call result's return shape. */ - readonly emitted: number; - /** - * Sites where the receiver WAS typed to a producer but that producer owns no - * member of this name. Reported rather than dropped: it means the read and - * the shape disagree, which is either a stale field name or a producer this - * pass mis-attributed, and both are worth seeing. - */ - readonly memberNotOnShape: number; -} - -/** - * Does this Property node id name `.`? - * - * Ids carry an optional position suffix for function-local symbols - * (`…:buildFlat.field@33:4`), so the owner segment is matched up to a `@` or - * the end rather than by equality. - */ -function idNamesMember(id: string, owner: string, member: string): boolean { - const needle = `:${owner}.${member}`; - const at = id.indexOf(needle); - if (at === -1) return false; - const after = id.slice(at + needle.length); - return after.length === 0 || after.startsWith('@'); -} - -export function emitReturnShapeMemberAccesses( - graph: KnowledgeGraph, - indexes: ScopeResolutionIndexes, - parsedFiles: readonly ParsedFile[], - nodeLookup: GraphNodeLookup, - /** Sites a precise pass already owns — never re-resolved here. */ - skipSites: ReadonlySet, - propertyNameIndex: PropertyNameIndex, - /** Sites this pass resolves, so the name fallback leaves them alone. */ - handledSink: Set, -): ReturnShapeMemberStats { - let emitted = 0; - let memberNotOnShape = 0; - const seen = new Set(); - - for (const parsed of parsedFiles) { - for (const site of parsed.referenceSites) { - if (site.kind !== 'read' && site.kind !== 'write') continue; - const receiver = site.explicitReceiver?.name; - if (receiver === undefined || receiver.length === 0) continue; - const siteKey = callableFlowSiteKey(parsed.filePath, site.atRange); - if (skipSites.has(siteKey)) continue; - - // The receiver's binding names the PRODUCER, not a class. That is the - // whole point: `formatSpikeAlert` is a function, and before R3-4 there - // was nothing named after it to look a member up on. - const typeRef = findReceiverTypeBinding(site.inScope, receiver, indexes); - const producer = typeRef?.rawName; - if (producer === undefined || producer.length === 0) continue; - - const candidates = propertyNameIndex.get(site.name); - if (candidates === undefined) continue; - const owned = candidates.filter((c) => idNamesMember(c.id, producer, site.name)); - // Exactly one, or nothing. Two nodes claiming `.` would - // mean the id qualifier failed to separate them, and picking between them - // would be the guess this pass exists to avoid. - if (owned.length !== 1) { - if (owned.length === 0) memberNotOnShape++; - continue; - } - const target = owned[0]!; - - const callerGraphId = resolveCallerGraphId(site.inScope, indexes, nodeLookup, site.atRange); - if (callerGraphId === undefined) continue; - if (callerGraphId === target.id) continue; - - const dedupKey = `ACCESSES:${callerGraphId}->${target.id}:${site.atRange.startLine}:${site.atRange.startCol}`; - if (seen.has(dedupKey)) continue; - seen.add(dedupKey); - - graph.addRelationship({ - id: `rel:${dedupKey}`, - sourceId: callerGraphId, - targetId: target.id, - type: 'ACCESSES', - confidence: RETURN_SHAPE_CONFIDENCE, - reason: `${EDGE_REASON}: ${site.kind}`, - evidence: [], - }); - // Claim the site so the name fallback cannot re-answer it differently. - handledSink.add(siteKey); - emitted++; - } - } - - return { emitted, memberNotOnShape }; -} diff --git a/gitnexus/src/core/ingestion/scope-resolution/passes/unique-name-properties.ts b/gitnexus/src/core/ingestion/scope-resolution/passes/unique-name-properties.ts index 2deac2a96..45d2a9cd9 100644 --- a/gitnexus/src/core/ingestion/scope-resolution/passes/unique-name-properties.ts +++ b/gitnexus/src/core/ingestion/scope-resolution/passes/unique-name-properties.ts @@ -69,7 +69,6 @@ import type { ScopeResolutionIndexes } from '../../model/scope-resolution-indexe import type { GraphNodeLookup } from '../graph-bridge/node-lookup.js'; import { resolveCallerGraphId } from '../graph-bridge/ids.js'; import { callableFlowSiteKey } from './callable-value-flow.js'; -import { isTestFile } from '../../entry-point-scoring.js'; import { getLanguageFromFilename } from 'gitnexus-shared'; /** Language a definition lives in, for reporting which anchor a reader cannot reach. */ @@ -102,19 +101,6 @@ const OVERSATURATED = null; interface PropertyCandidate { readonly id: string; readonly filePath: string; - /** - * True when this definition is the RETURN SHAPE of a function (R3-4) rather - * than a declared surface — a named object literal, a class field, an - * interface or alias member. - * - * Return shapes are the weaker anchor and are ranked below declared ones, so - * adding them cannot change an answer that already resolved. That is what - * reconciles this with R2-1b, which deliberately modelled returned keys as - * WRITES to avoid adding same-named competitors to narrowing: they are - * definitions now, but they never outrank a real declaration, so the - * competitor problem it was avoiding does not come back. - */ - readonly fromReturnShape: boolean; } /** @@ -200,18 +186,13 @@ export function buildPropertyNameIndex(graph: KnowledgeGraph): PropertyNameIndex if (typeof name !== 'string' || name.length === 0) continue; const filePath = node.properties.filePath; if (typeof filePath !== 'string') continue; - const candidate: PropertyCandidate = { - id: node.id, - filePath, - fromReturnShape: node.properties.fromReturnShape === true, - }; const existing = byName.get(name); if (existing === undefined) { - byName.set(name, [candidate]); + byName.set(name, [{ id: node.id, filePath }]); continue; } if (existing.some((c) => c.id === node.id)) continue; - existing.push(candidate); + existing.push({ id: node.id, filePath }); } return byName; } @@ -300,40 +281,14 @@ function buildDirectImportMap( * imported third would answer a question the reader's own file contradicts. */ function narrowToSingleCandidate( - candidatesIn: readonly PropertyCandidate[], + candidates: readonly PropertyCandidate[], readingFile: string, importedFiles: ReadonlySet | undefined, ): { readonly id: string; readonly tier: string } | null { - // DECLARED ANCHORS FIRST. A return shape is a real definition but a weaker - // one: it says "some function builds an object with this key", where a named - // literal or a class/interface member says "this IS the field". Whenever both - // exist, the declared one is what a reader means — and ranking it first is - // what guarantees R3-4 cannot change an answer that already resolved before - // return shapes were indexed at all. - let candidates = candidatesIn; - - // PRODUCTION CODE FIRST. A test constructs throwaway shapes with the same - // field names as the thing it exercises — measured on the reporting repo, - // four of the seven JavaScript anchors for `wickRatio` are in `tests/` — and - // a read in production code cannot mean any of them. Applied before the - // declared/return-shape split because "is this the shipped program" is the - // stronger signal: a declaration in a test fixture is still a test fixture. - // - // Only when the READER is production. A read inside a test legitimately means - // the test's own shape, so this must not fire there. - if (!isTestFile(readingFile)) { - const production = candidates.filter((c) => !isTestFile(c.filePath)); - if (production.length > 0) candidates = production; + if (candidates.length === 1) { + return { id: candidates[0]!.id, tier: 'workspace-unique' }; } - const declared = candidates.filter((c) => !c.fromReturnShape); - const ranked = declared.length > 0 ? declared : candidates; - - if (ranked.length === 1) { - return { id: ranked[0]!.id, tier: 'workspace-unique' }; - } - candidates = ranked; - const sameFile = candidates.filter((c) => c.filePath === readingFile); if (sameFile.length > 0) { return sameFile.length === 1 ? { id: sameFile[0]!.id, tier: 'same-file' } : null; diff --git a/gitnexus/src/core/ingestion/scope-resolution/pipeline/run.ts b/gitnexus/src/core/ingestion/scope-resolution/pipeline/run.ts index 35b07a157..f13e43505 100644 --- a/gitnexus/src/core/ingestion/scope-resolution/pipeline/run.ts +++ b/gitnexus/src/core/ingestion/scope-resolution/pipeline/run.ts @@ -77,11 +77,9 @@ import { } from '../passes/property-dispatch.js'; import { emitReferencesViaLookup } from '../graph-bridge/references-to-edges.js'; import { - buildPropertyNameIndex, emitUniqueNamePropertyAccesses, type PropertyNameIndex, } from '../passes/unique-name-properties.js'; -import { emitReturnShapeMemberAccesses } from '../passes/return-shape-members.js'; import { emitImportedValueReferences } from '../passes/imported-value-refs.js'; import { createCalleeIdAccumulator, @@ -1055,23 +1053,6 @@ export function runScopeResolution( // TS-anchor case R3-1 was filed about — the identical defect, mirrored. // `reportOnly` counts without emitting: no edge, no inference, no change to // what the opt-out protects. - // PRECISE first (R3-5). A call result's return shape names WHICH producer a - // receiver holds, so it answers exactly the case name inference must refuse: - // several functions returning the same field name. Sites it resolves are - // added to the skip set, so the fallback below never second-guesses them. - const sharedPropertyIndex = input.prebuiltPropertyNameIndex ?? buildPropertyNameIndex(graph); - const returnShapeMembers = callableFlowOnly - ? { emitted: 0, memberNotOnShape: 0 } - : emitReturnShapeMemberAccesses( - graph, - indexes, - emitParsedFiles, - postHeritageNodeLookup, - uniqueNameSkipSites, - sharedPropertyIndex, - uniqueNameSkipSites, - ); - const nameFallbackDisabled = provider.fieldFallbackOnMethodLookup === false; const uniqueNameProperties = callableFlowOnly ? { @@ -1089,7 +1070,7 @@ export function runScopeResolution( postHeritageNodeLookup, uniqueNameSkipSites, finalized, - sharedPropertyIndex, + input.prebuiltPropertyNameIndex, nameFallbackDisabled, ); diff --git a/gitnexus/src/core/ingestion/tree-sitter-queries.ts b/gitnexus/src/core/ingestion/tree-sitter-queries.ts index 7a6dee3df..f2655c4a8 100644 --- a/gitnexus/src/core/ingestion/tree-sitter-queries.ts +++ b/gitnexus/src/core/ingestion/tree-sitter-queries.ts @@ -424,43 +424,6 @@ export const TYPESCRIPT_QUERIES = ` (pair key: (property_identifier) @name) @definition.property)) -; Keys of an ANONYMOUS object literal in RETURN position (R3-4). The dominant -; shape in idiomatic JS: 437 sites in one backend directory of the reporting -; repo, including the ~25-field payload of its whole signal pipeline, none of -; which could be named because the literal binds to nothing. -; -; The enclosing function is the owner -- the literal is that function's return -; shape, a contract its callers consume -- so the key qualifies as -; . and two functions returning the same key stay distinct. -; -; DEFINITIONS, unlike the record-construction writes of R2-1b, and the -; difference is deliberate: there a definition already existed elsewhere and a -; construction site was a USE of it, while here nothing else names the field at -; all. To keep that from regressing R2-1b's case, narrowing ranks declared -; anchors ABOVE return shapes, so a name that already resolves keeps resolving -; to what it resolved to before. -(return_statement - (object - (pair - key: (property_identifier) @name) @definition.property)) - -; SHORTHAND keys of the same literal. "return { symbol, interval, score }" is -; the commonest spelling of all -- the reporting repo's own alert payload is -; mostly shorthand -- and (pair) does not match it: tree-sitter models it as -; shorthand_property_identifier, where the key IS the value. Found by dumping -; the golden fixture and noticing that a literal returning -; { level, message, timestamp: Date.now() } had indexed only timestamp. -(return_statement - (object - (shorthand_property_identifier) @name @definition.property)) - -; Shorthand keys of a named object literal -- same gap, same reason as the -; return-position rule above. -(variable_declarator - name: (identifier) - value: (object - (shorthand_property_identifier) @name @definition.property)) - (variable_declarator name: (identifier) value: (call_expression @@ -969,43 +932,6 @@ export const JAVASCRIPT_QUERIES = ` (pair key: (property_identifier) @name) @definition.property)) -; Keys of an ANONYMOUS object literal in RETURN position (R3-4). The dominant -; shape in idiomatic JS: 437 sites in one backend directory of the reporting -; repo, including the ~25-field payload of its whole signal pipeline, none of -; which could be named because the literal binds to nothing. -; -; The enclosing function is the owner -- the literal is that function's return -; shape, a contract its callers consume -- so the key qualifies as -; . and two functions returning the same key stay distinct. -; -; DEFINITIONS, unlike the record-construction writes of R2-1b, and the -; difference is deliberate: there a definition already existed elsewhere and a -; construction site was a USE of it, while here nothing else names the field at -; all. To keep that from regressing R2-1b's case, narrowing ranks declared -; anchors ABOVE return shapes, so a name that already resolves keeps resolving -; to what it resolved to before. -(return_statement - (object - (pair - key: (property_identifier) @name) @definition.property)) - -; SHORTHAND keys of the same literal. "return { symbol, interval, score }" is -; the commonest spelling of all -- the reporting repo's own alert payload is -; mostly shorthand -- and (pair) does not match it: tree-sitter models it as -; shorthand_property_identifier, where the key IS the value. Found by dumping -; the golden fixture and noticing that a literal returning -; { level, message, timestamp: Date.now() } had indexed only timestamp. -(return_statement - (object - (shorthand_property_identifier) @name @definition.property)) - -; Shorthand keys of a named object literal -- same gap, same reason as the -; return-position rule above. -(variable_declarator - name: (identifier) - value: (object - (shorthand_property_identifier) @name @definition.property)) - ; Same named shape, behind an IDENTITY-PRESERVING wrapper (R2-1a): ; ; export const INERT_EXIT_CONTRACT = Object.freeze({ exitModel: 'bracket', ... }); diff --git a/gitnexus/src/core/ingestion/utils/ast-helpers.ts b/gitnexus/src/core/ingestion/utils/ast-helpers.ts index 65f06bd31..f4ec2dd2f 100644 --- a/gitnexus/src/core/ingestion/utils/ast-helpers.ts +++ b/gitnexus/src/core/ingestion/utils/ast-helpers.ts @@ -1177,91 +1177,6 @@ const BLOCK_SCOPE_BOUNDARY_TYPES = new Set([ * ancestor also returns null (catches block-scoped declarations inside * top-level `if`/`for`/`try`/etc., which cannot be imported). */ -/** - * Owner for the keys of an ANONYMOUS object literal in return position (R3-4). - * - * `return { symbol, score, wickRatio, … }` binds to nothing, so its keys had no - * anchor and could not be qualified — which on the reporting repo left the - * central payload of the signal pipeline, ~25 fields, entirely unqueryable. - * There are 437 such sites in one backend directory, so this is the dominant - * shape, not an edge case. - * - * The enclosing FUNCTION is the honest owner: the literal is that function's - * return shape, which is a contract its callers consume. Qualifying by it keeps - * two functions returning the same key name as two distinct nodes, exactly as - * `ownerName` does for variable-bound literals. - * - * Returns null when the literal is not DIRECTLY returned (a nested literal, or - * one inside a callback several frames down), because then the enclosing - * function is not what the object describes. - */ -/** - * True when this definition node is a key of a literal in RETURN position. - * - * Deliberately independent of whether an OWNER NAME could be derived. The two - * are different questions, and conflating them mislabels the anonymous case: - * `[function (row) { return { k: row.x }; }]` yields no name to qualify by, so - * the owner lookup returns null — but the key is still a return shape, and - * flagging it by owner-presence would leave it looking like a DECLARED anchor - * and let it outrank a real declaration during narrowing. - */ -export const isReturnShapeProperty = (node: SyntaxNode): boolean => { - let current: SyntaxNode | null = node; - let objectDepth = 0; - while (current && objectDepth === 0) { - if (current.type === 'object') objectDepth = 1; - else if (FUNCTION_NODE_TYPES.has(current.type)) return false; - else current = current.parent; - } - return current?.parent?.type === 'return_statement'; -}; - -export const findReturnShapeOwnerInfo = ( - node: SyntaxNode, - filePath: string, - // NO `ownerId`, deliberately, and the union's optional field is what says so. - // An owner id would emit `HAS_PROPERTY` from the FUNCTION, a `Function|Property` - // relation pair that the schema does not declare — and an undeclared pair does - // not degrade, it throws `UndeclaredRelationPairError` and kills the entire - // analyze. That already shipped once in this PR. The qualifier alone is what - // this needs: it makes the key nameable and keeps two functions' same-named - // keys distinct, without asserting a containment edge nothing consumes. -): { readonly ownerId?: string; readonly ownerName: string } | null => { - // Walk to the literal this key belongs to; bail if it is nested inside - // another object, whose shape it describes instead. - let current: SyntaxNode | null = node; - let objectDepth = 0; - while (current && objectDepth === 0) { - if (current.type === 'object') objectDepth = 1; - else if (FUNCTION_NODE_TYPES.has(current.type)) return null; - else current = current.parent; - } - if (!current) return null; - const literal = current; - if (literal.parent?.type !== 'return_statement') return null; - - // The nearest enclosing function-like, and its name. An anonymous function - // (a callback, an IIFE) gives nothing to qualify by, so those stay - // unanchored rather than colliding on a shared empty owner. - let fn: SyntaxNode | null = literal.parent.parent; - while (fn && !FUNCTION_NODE_TYPES.has(fn.type)) fn = fn.parent; - if (!fn) return null; - - const nameNode = fn.childForFieldName?.('name'); - if (nameNode?.type === 'identifier' || nameNode?.type === 'property_identifier') { - return { ownerName: nameNode.text }; - } - // `const formatAlert = (…) => ({ … })` and `const f = function () {}`: the - // name is on the declarator, not the function. - const declarator = fn.parent; - if (declarator?.type === 'variable_declarator') { - const declName = declarator.childForFieldName?.('name'); - if (declName?.type === 'identifier') return { ownerName: declName.text }; - } - void filePath; - return null; -}; - export const findObjectLiteralBindingInfo = ( node: SyntaxNode, filePath: string, diff --git a/gitnexus/src/core/ingestion/workers/parse-worker.ts b/gitnexus/src/core/ingestion/workers/parse-worker.ts index 2158cffd4..029ffab83 100644 --- a/gitnexus/src/core/ingestion/workers/parse-worker.ts +++ b/gitnexus/src/core/ingestion/workers/parse-worker.ts @@ -91,8 +91,6 @@ import { getDefinitionNodeFromCaptures, findEnclosingClassInfo, findObjectLiteralBindingInfo, - findReturnShapeOwnerInfo, - isReturnShapeProperty, findMemberAssignmentOwnerInfo, isCjsDefaultExportAssignment, type EnclosingClassInfo, @@ -2363,19 +2361,8 @@ const processFileGroup = ( // byte-identical or every object-literal method in every indexed // repo changes id. includeOwnerName: nodeLabel === 'Property', - }) ?? - // R3-4: an anonymous literal in return position is owned by the - // function whose shape it is. Last in the chain so a variable-bound - // literal keeps its existing owner and its existing id. - (nodeLabel === 'Property' ? findReturnShapeOwnerInfo(definitionNode, file.path) : null)) + })) : null; - // Provenance for narrowing (R3-4). A return shape is a real definition but - // the weaker one, and the unique-name pass ranks declared anchors above it - // so indexing these cannot change an answer that already resolved. - const returnShapeProperty = - nodeLabel === 'Property' && definitionNode !== undefined && definitionNode !== null - ? isReturnShapeProperty(definitionNode) - : false; // #1978: hoisted ABOVE qualifiedName/node-id (load-bearing order) so a // class-like node can key its id by its fully-qualified path. Derived from @@ -2822,7 +2809,6 @@ const processFileGroup = ( ...(description !== undefined ? { description } : {}), ...methodProps, ...(declaredType !== undefined ? { declaredType } : {}), - ...(returnShapeProperty ? { fromReturnShape: true } : {}), }, }); diff --git a/gitnexus/src/storage/parse-cache.ts b/gitnexus/src/storage/parse-cache.ts index f0a3f0036..c798f97e5 100644 --- a/gitnexus/src/storage/parse-cache.ts +++ b/gitnexus/src/storage/parse-cache.ts @@ -289,22 +289,7 @@ import type { ParseWorkerResult } from '../core/ingestion/workers/parse-worker.j // clashes. It is NOT on this branch, so until that one merges the re-check // below is still manual. // RE-CHECK AGAINST origin/main IMMEDIATELY BEFORE MERGING. -// 48 -> 49 for the return-shape and shorthand captures (R3-4): keys of an -// anonymous literal in return position, and shorthand keys in both that and the -// variable-bound form. Parse-time again. -// -// The v34 hazard, and this branch has already tripped it: a build stamped 48 -// was installed and used to analyze two repos BEFORE these captures existed, so -// caches stamped 48 exist that carry none of them. Within one PR the version -// only has to differ from main's, but an INTERMEDIATE build of the same series -// is a different capture set wearing the same number — which is exactly what -// the note above records for 33/34. -// RE-CHECK AGAINST origin/main IMMEDIATELY BEFORE MERGING. -// 49 -> 50 is NOT needed for R3-5: that pass is scope-resolution, not -// parse-time capture, so a warm cache replays ParsedFiles that already carry -// everything it reads. Recorded because the reflex on this branch has been to -// bump, and a bump nobody needs still forces every user a full re-parse. -const SCHEMA_BUMP = 49; +const SCHEMA_BUMP = 48; const GITNEXUS_PKG_VERSION = (() => { try { // package.json sits at gitnexus/package.json — two levels up from diff --git a/gitnexus/test/fixtures/lang-resolution/javascript-object-properties/fixture-shapes.test.js b/gitnexus/test/fixtures/lang-resolution/javascript-object-properties/fixture-shapes.test.js deleted file mode 100644 index ebf8a73fe..000000000 --- a/gitnexus/test/fixtures/lang-resolution/javascript-object-properties/fixture-shapes.test.js +++ /dev/null @@ -1,9 +0,0 @@ -// A TEST file that constructs a throwaway shape with a production field name. -// Measured on the reporting repo: four of seven JavaScript anchors for one -// field lived in `tests/`, competing with the three real ones and making every -// production read ambiguous. A read in production cannot mean any of these. -export function buildTestFixture() { - return { - productionAndTestField: 'fixture', - }; -} diff --git a/gitnexus/test/fixtures/lang-resolution/javascript-object-properties/production-reader.js b/gitnexus/test/fixtures/lang-resolution/javascript-object-properties/production-reader.js deleted file mode 100644 index 6c7638059..000000000 --- a/gitnexus/test/fixtures/lang-resolution/javascript-object-properties/production-reader.js +++ /dev/null @@ -1,8 +0,0 @@ -// The reader lives in its OWN file and imports neither anchor, so no same-file -// or direct-import tier can decide this. What is left is production-vs-test, -// which is exactly the tier under test — with a reader beside the production -// anchor, the same-file tier resolves it either way and the assertion proves -// nothing. -export function readsProductionShape(bag) { - return bag.productionAndTestField; -} diff --git a/gitnexus/test/fixtures/lang-resolution/javascript-object-properties/return-shape-typed.js b/gitnexus/test/fixtures/lang-resolution/javascript-object-properties/return-shape-typed.js deleted file mode 100644 index 67cdae6b9..000000000 --- a/gitnexus/test/fixtures/lang-resolution/javascript-object-properties/return-shape-typed.js +++ /dev/null @@ -1,32 +0,0 @@ -// R3-5: TWO producers returning the same field name — the case name inference -// must refuse, because `x.ambiguousProducedField` alone cannot say which shape -// is meant. The receiver's binding says which, so this resolves precisely. -export function producerAlpha(row) { - return { - ambiguousProducedField: row.a, - }; -} - -export function producerBeta(row) { - return { - ambiguousProducedField: row.b, - }; -} - -// BOUND to the call result, so the type binding attaches. -export function readsAlpha(row) { - const shaped = producerAlpha(row); - return shaped.ambiguousProducedField; -} - -export function readsBeta(row) { - const shaped = producerBeta(row); - return shaped.ambiguousProducedField; -} - -// THE BOUND of the mechanism: a bare parameter has no binding here, because -// typing it needs the CALLER's type to flow in. This must stay unresolved and -// fall through to name inference, which will refuse it (two producers). -export function readsUnbound(shaped) { - return shaped.ambiguousProducedField; -} diff --git a/gitnexus/test/fixtures/lang-resolution/javascript-object-properties/return-shape.js b/gitnexus/test/fixtures/lang-resolution/javascript-object-properties/return-shape.js deleted file mode 100644 index cc7e49877..000000000 --- a/gitnexus/test/fixtures/lang-resolution/javascript-object-properties/return-shape.js +++ /dev/null @@ -1,55 +0,0 @@ -// R3-4: an anonymous literal in return position — the dominant shape in -// idiomatic JS (437 sites in one backend directory of the reporting repo), -// including the ~25-field payload of its entire signal pipeline. It binds to -// nothing, so its keys had no anchor and could not be named at all. -export function formatAlert(row) { - const shorthandOnlyField = row.shorthand; - return { - returnShapeOnlyField: row.raw, - sharedWithDeclared: row.other, - // SHORTHAND — the commonest spelling, and the one `(pair)` cannot match. - // The reporting repo's own alert payload is mostly this form. - shorthandOnlyField, - }; -} - -// A SECOND function returning a same-named key. Two distinct shapes, so two -// distinct nodes — qualifying by the owning function is what keeps them apart. -export function formatSummary(row) { - return { - summaryOnlyField: row.summary, - }; -} - -// The reader. Untyped receiver, so this is the name-inference path. -export function readsReturnShape(alert) { - return alert.returnShapeOnlyField; -} - -// The R2-1b GUARANTEE, as a fixture: a DECLARED anchor for the same name. -// `sharedWithDeclared` is both a named-object key and a return-shape key, and a -// read of it must keep resolving to the DECLARED one — otherwise indexing -// return shapes would silently move existing answers. -export const declaredHome = { - sharedWithDeclared: 1, -}; - -export function readsShared(bag) { - return bag.sharedWithDeclared; -} - -// Anonymous functions give nothing to qualify by, so their return shapes stay -// unanchored rather than colliding on a shared empty owner. -export const anonHolder = [ - function (row) { - return { anonReturnKey: row.x }; - }, -]; - -// The production anchor for a name a test fixture also constructs. A read here -// must resolve to THIS one, not to the fixture's. -export function buildProductionShape(row) { - return { - productionAndTestField: row.real, - }; -} diff --git a/gitnexus/test/fixtures/lang-resolution/typescript-alias-fields/contracts.ts b/gitnexus/test/fixtures/lang-resolution/typescript-alias-fields/contracts.ts index 56657ac0e..a7689757d 100644 --- a/gitnexus/test/fixtures/lang-resolution/typescript-alias-fields/contracts.ts +++ b/gitnexus/test/fixtures/lang-resolution/typescript-alias-fields/contracts.ts @@ -44,11 +44,6 @@ export type NestedConfig = { }; // Inline RETURN type — the third position the unanchored rule reached. -// The TYPE annotation's member and the returned VALUE's key are named -// differently ON PURPOSE. They are separate rules with opposite expectations — -// an inline return TYPE must mint nothing (RV-4), while a returned literal's -// keys are a function's return shape and must mint (R3-4) — and sharing a name -// left the RV-4 assertion unable to tell which rule produced the node. -export function buildInline(): { inlineReturnTypeOnlyKey: number } { - return { inlineReturnValueOnlyKey: 1 } as never; +export function buildInline(): { inlineReturnOnlyKey: number } { + return { inlineReturnOnlyKey: 1 }; } diff --git a/gitnexus/test/fixtures/pipeline-golden/mini-repo/expected-graph.json b/gitnexus/test/fixtures/pipeline-golden/mini-repo/expected-graph.json index 14fa41596..4a5d5b408 100644 --- a/gitnexus/test/fixtures/pipeline-golden/mini-repo/expected-graph.json +++ b/gitnexus/test/fixtures/pipeline-golden/mini-repo/expected-graph.json @@ -2,8 +2,8 @@ "capture": "initial capture (U8, post-U1–U7)", "fixture": "mini-repo", "totalFileCount": 7, - "symbols": 51, - "relationships": 95, + "symbols": 41, + "relationships": 85, "processes": 4, "byType": { "Class": 1, @@ -14,13 +14,13 @@ "Interface": 3, "Method": 1, "Process": 4, - "Property": 18 + "Property": 8 }, "byRelType": { "ACCESSES": 3, "CALLS": 9, "CONTAINS": 7, - "DEFINES": 26, + "DEFINES": 16, "HAS_METHOD": 1, "HAS_PROPERTY": 8, "IMPORTS": 12, @@ -28,5 +28,5 @@ "STEP_IN_PROCESS": 12, "USES": 5 }, - "edgeDigest": "5dddd1f466deeda1197eb61b480a4f3a5da67dfc0d00ace22377158366e87d00" + "edgeDigest": "c55bd8307a5fccbfd23d5aa93f241e26ef0f45c1a9e57691f2bf4f6602e8e4ab" } diff --git a/gitnexus/test/integration/resolvers/javascript-object-properties.test.ts b/gitnexus/test/integration/resolvers/javascript-object-properties.test.ts index 080212e93..43670f0ed 100644 --- a/gitnexus/test/integration/resolvers/javascript-object-properties.test.ts +++ b/gitnexus/test/integration/resolvers/javascript-object-properties.test.ts @@ -145,21 +145,11 @@ describe('JavaScript plain-object property access (A1/A5)', () => { expect(writersOf('destructuredOnlyField')).toContain('buildFlat'); }); - // This asserted `toHaveLength(1)` — no new definition — until R3-4 began - // anchoring returned literals, which mints exactly one here (`buildFlat`'s - // return shape). The assertion was the right instinct expressed as the - // wrong invariant: what R2-1b actually protects is that adding definitions - // must not move an answer that already resolved, and node count was a proxy - // for that. The property itself is now asserted directly, and it holds - // because narrowing ranks declared anchors above return shapes. - it('keeps the DECLARED definition winning despite a return-shape sibling', () => { - const nodes = propertyNames().filter((n) => n === 'destructuredOnlyField'); - expect(nodes.length).toBeGreaterThan(1); - // Every reader still resolves, and to the declared home — a read that had - // dropped to ambiguous would show up as a missing edge here. - for (const reader of ['appliesDestructured', 'appliesShorthand', 'appliesRenamed']) { - expect(readersOf('destructuredOnlyField')).toContain(reader); - } + // The point of modelling these as writes rather than definitions: more + // definitions would add same-named competitors to the narrowing that makes + // these fields resolvable in the first place. + it('mints NO new definition for a constructed record', () => { + expect(propertyNames().filter((n) => n === 'destructuredOnlyField')).toHaveLength(1); }); it('leaves an inline call-argument prop bag alone', () => { @@ -168,118 +158,6 @@ describe('JavaScript plain-object property access (A1/A5)', () => { }); }); - // R3-4. The dominant shape in idiomatic JS and the one with no anchor at all: - // 437 `return {` sites in a single backend directory of the reporting repo, - // including the ~25-field payload of its whole signal pipeline. The literal - // binds to nothing, so its keys could not even be named. - describe('anonymous returned object literals (R3-4)', () => { - it('indexes keys of a literal returned from a named function', () => { - expect(propertyNames()).toContain('returnShapeOnlyField'); - }); - - it('resolves a read of a return-shape key', () => { - expect(readersOf('returnShapeOnlyField')).toContain('readsReturnShape'); - }); - - // `{ symbol, interval, score }` is the commonest spelling of all and - // `(pair)` does not match it — tree-sitter models it as - // `shorthand_property_identifier`, where the key IS the value. Caught by - // dumping the golden fixture and seeing that a literal returning - // `{ level, message, timestamp: Date.now() }` had indexed only `timestamp`. - it('indexes SHORTHAND keys, not just explicit pairs', () => { - expect(propertyNames()).toContain('shorthandOnlyField'); - }); - - // Qualified by the owning function, so two functions returning the same key - // are two shapes rather than one merged symbol — the same collision - // `ownerName` prevents for variable-bound literals. - it('qualifies by the owning function', () => { - const ids = Array.from( - (result as unknown as { graph: { iterNodes(): Iterable } }).graph.iterNodes(), - ) - .filter((n) => n.label === 'Property') - .map((n) => String(n.id)); - expect(ids.some((id) => id.includes('formatAlert.returnShapeOnlyField'))).toBe(true); - expect(ids.some((id) => id.includes('formatSummary.summaryOnlyField'))).toBe(true); - }); - - // Production code outranks test fixtures. Measured on the reporting repo: - // four of the seven JavaScript anchors for one field were in `tests/`, - // competing with the three real ones and making every production read - // ambiguous. A test builds throwaway shapes with production field names; a - // read in shipped code cannot mean one. - it('does not let a test fixture compete with the production anchor', () => { - expect(readersOf('productionAndTestField')).toContain('readsProductionShape'); - const ids = Array.from( - (result as unknown as { graph: { iterNodes(): Iterable } }).graph.iterNodes(), - ) - .filter((n) => n.label === 'Property') - .map((n) => String(n.id)); - // Both anchors exist — it is the RANKING that differs, not the indexing. - expect(ids.some((id) => id.includes('buildProductionShape.productionAndTestField'))).toBe( - true, - ); - expect(ids.some((id) => id.includes('buildTestFixture.productionAndTestField'))).toBe(true); - }); - - // THE GUARANTEE that reconciles this with R2-1b. `sharedWithDeclared` is - // both a named-object key and a return-shape key; a read must still resolve - // to the DECLARED one, or indexing return shapes would silently move - // answers that already worked. - it('never outranks a declared anchor', () => { - expect(readersOf('sharedWithDeclared')).toContain('readsShared'); - const declaredWins = getRelationships(result, 'ACCESSES').filter( - (e) => e.target === 'sharedWithDeclared' && e.source === 'readsShared', - ); - expect(declaredWins.length).toBeGreaterThan(0); - }); - }); - - // R3-5. The question three rounds of reports could not answer: a field - // produced by SEVERAL functions. Name inference must refuse it — the name - // alone cannot say which shape is meant — so this replaces inference with - // evidence, joining the call-result type binding (which already existed) to - // the return-shape owner (which R3-4 created). - describe('return-shape members via the call result (R3-5)', () => { - const targetOf = (source: string): string[] => - getRelationships(result, 'ACCESSES') - .filter((e) => e.target === 'ambiguousProducedField' && e.source === source) - .map((e) => String((e.rel as { targetId?: string }).targetId ?? '')); - - it('resolves to the producer the receiver actually holds', () => { - expect(targetOf('readsAlpha').some((id) => id.includes('producerAlpha.'))).toBe(true); - expect(targetOf('readsBeta').some((id) => id.includes('producerBeta.'))).toBe(true); - }); - - // The discriminator. Both producers own a field of this name, so a name - // match cannot tell them apart — getting the RIGHT one is only possible - // because the receiver's binding names the producer. - it('does not cross the two producers', () => { - expect(targetOf('readsAlpha').some((id) => id.includes('producerBeta.'))).toBe(false); - expect(targetOf('readsBeta').some((id) => id.includes('producerAlpha.'))).toBe(false); - }); - - it('marks the edge as precise, not as name inference', () => { - const reasons = getRelationships(result, 'ACCESSES') - .filter((e) => e.target === 'ambiguousProducedField') - .map((e) => String(e.rel.reason ?? '')); - expect(reasons.every((r) => r.includes('return-shape member'))).toBe(true); - for (const e of getRelationships(result, 'ACCESSES').filter( - (x) => x.target === 'ambiguousProducedField', - )) { - expect(e.rel.confidence).toBeGreaterThan(0.85); - } - }); - - // THE BOUND, asserted so the mechanism is not mistaken for something it is - // not. A bare parameter has no binding here — typing it needs the CALLER's - // type to flow in, which is inter-procedural — so it falls through to name - // inference, which refuses because two producers share the name. - it('leaves an unbound receiver to name inference, which refuses it', () => { - expect(targetOf('readsUnbound')).toEqual([]); - }); - }); - // R2. Strict workspace uniqueness was measurably too blunt: in the reporting // repo `exitMinAtrMult` had 26 definitions, 16 of them in one-off scripts the // backend has no relationship with, so every backend read was refused because diff --git a/gitnexus/test/integration/resolvers/typescript-alias-fields.test.ts b/gitnexus/test/integration/resolvers/typescript-alias-fields.test.ts index 1cab69f7d..52bcbc9df 100644 --- a/gitnexus/test/integration/resolvers/typescript-alias-fields.test.ts +++ b/gitnexus/test/integration/resolvers/typescript-alias-fields.test.ts @@ -117,10 +117,7 @@ describe('TypeScript type-alias and interface members (A4)', () => { }); it('does not mint a member for an inline RETURN type', () => { - // The TYPE's member, not the returned value's key — those are different - // rules with opposite expectations, and the fixture names them apart so - // this assertion cannot be satisfied by the wrong one. - expect(propertyIds().some((id) => id.includes('inlineReturnTypeOnlyKey'))).toBe(false); + expect(propertyIds().some((id) => id.includes('inlineReturnOnlyKey'))).toBe(false); }); // The other half: anchoring must not cost real members. diff --git a/gitnexus/test/unit/incremental-parse-cache.test.ts b/gitnexus/test/unit/incremental-parse-cache.test.ts index 192360e9d..c8c22b5e4 100644 --- a/gitnexus/test/unit/incremental-parse-cache.test.ts +++ b/gitnexus/test/unit/incremental-parse-cache.test.ts @@ -141,8 +141,8 @@ describe('PARSE_CACHE_VERSION', () => { // a row. Same lesson as the note above — the pin cannot detect the tie, since // both sides asserted `toBe(45)` and that passes while main is already 45. // Only the merge-time diff against origin/main surfaces it. - it('pins SCHEMA_BUMP to 49 so concurrent bumps cannot silently collide (#2766)', () => { - expect(Number(PARSE_CACHE_VERSION.split('+', 1)[0])).toBe(49); + it('pins SCHEMA_BUMP to 48 so concurrent bumps cannot silently collide (#2766)', () => { + expect(Number(PARSE_CACHE_VERSION.split('+', 1)[0])).toBe(48); }); it('embeds the gitnexus package version (so upgrades invalidate the cache)', () => {