mirror of
https://github.com/abhigyanpatwari/GitNexus.git
synced 2026-08-28 05:25:25 +00:00
fix(scope-resolution): require the return-shape producer to RESOLVE, not merely to name-match
Review finding 2, reached independently by three Claude lanes and two Codex
legs, and reproduced here. `emitReturnShapeMemberAccesses` took the receiver's
type binding, then filtered a WHOLE-GRAPH property index with `idNamesMember` —
a textual match on the node id. Any node whose id happened to read
`<producer>.<member>` qualified, in any file and any language, and it emitted at
the 0.9 PRECISE tier where a `minConfidence` floor cannot filter it out. The
sibling unique-name pass was given a per-language restriction for exactly this
hazard; this pass consumed the same shared index with none.
Three guards, catching different shapes:
- the producer must RESOLVE to a definition (`findCallableBindingInScope` — a
CALLABLE lookup: the producer is the function whose return shape owns the
member, and it resolves through finalized import bindings so a producer in
another file still yields its own file);
- the member must live in that definition's file;
- that file must belong to the language being resolved.
The third is not redundant with the second, which is the part worth recording.
A receiver typed by CONSTRUCTION (`const bound = new Loyalty()`) resolves through
the shared class registry, which is polyglot — so the producer resolves into
`Loyalty.java`, its members legitimately live in that same file, and file
equality waves the cross-language edge straight through.
Also fixes the sibling P2: a site where the receiver IS typed to a producer that
owns no such member now claims the site. That branch is the strongest negative
evidence the pipeline can produce, and letting it fall through meant the 0.5 name
fallback answered a question the precise pass had just DISPROVED — measured,
linking a read to an unrelated same-named key in another file.
`polyglot-property-isolation` gains the bound-receiver arm the review asked for,
and it is the right arm: the pre-existing case has an untyped receiver and so
only ever exercised the unique-name pass, while one extra token routes an
identical read through this one. Mutation-verified — restoring the pre-fix
matching makes exactly the new leak assertion fail. The first version of that arm
was silently vacuous (it introduced a JS key of the same name, which destroyed
the fixture's Java-only premise), which is why it now asserts on the TARGET FILE
rather than on the absence of a name.
KNOWN LIMIT, stated rather than papered over: a member-call producer
(`const r = svc.make()`) binds `svc.make`, which resolves to no callable, so this
pass now declines it. Codex B3 raised that converse case and it is real. Fixing
it means typing `svc` and then finding `make` on that type — a larger piece of
work, queued for the follow-up PR. Declining is the correct interim behaviour:
the alternative is matching `make.<member>` by name across the graph, which is
the fabrication this commit removes.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
0aa65d6c25
commit
690470865e
3 changed files with 131 additions and 5 deletions
|
|
@ -41,7 +41,7 @@ import type { KnowledgeGraph } from '../../../graph/types.js';
|
||||||
import type { ScopeResolutionIndexes } from '../../model/scope-resolution-indexes.js';
|
import type { ScopeResolutionIndexes } from '../../model/scope-resolution-indexes.js';
|
||||||
import type { GraphNodeLookup } from '../graph-bridge/node-lookup.js';
|
import type { GraphNodeLookup } from '../graph-bridge/node-lookup.js';
|
||||||
import { resolveCallerGraphId } from '../graph-bridge/ids.js';
|
import { resolveCallerGraphId } from '../graph-bridge/ids.js';
|
||||||
import { findReceiverTypeBinding } from '../scope/walkers.js';
|
import { findCallableBindingInScope, findReceiverTypeBinding } from '../scope/walkers.js';
|
||||||
import { callableFlowSiteKey } from './callable-value-flow.js';
|
import { callableFlowSiteKey } from './callable-value-flow.js';
|
||||||
import type { PropertyNameIndex } from './unique-name-properties.js';
|
import type { PropertyNameIndex } from './unique-name-properties.js';
|
||||||
|
|
||||||
|
|
@ -97,6 +97,19 @@ export function emitReturnShapeMemberAccesses(
|
||||||
let memberNotOnShape = 0;
|
let memberNotOnShape = 0;
|
||||||
const seen = new Set<string>();
|
const seen = new Set<string>();
|
||||||
|
|
||||||
|
// The files of the language being resolved. `parsedFiles` is already scoped to
|
||||||
|
// it, so this needs no new plumbing — it is the same restriction the sibling
|
||||||
|
// unique-name pass gets from `candidatesForLanguage`.
|
||||||
|
//
|
||||||
|
// The file guard below is not sufficient on its own, and the reason is worth
|
||||||
|
// keeping: a receiver typed by CONSTRUCTION (`const cfg = new Loyalty()`)
|
||||||
|
// resolves `Loyalty` through the shared class registry, which is polyglot. The
|
||||||
|
// producer then legitimately resolves to `Loyalty.java`, its members
|
||||||
|
// legitimately live in that same file, and a file-equality check waves the
|
||||||
|
// cross-language edge straight through. Restricting to the current language's
|
||||||
|
// own files is what actually closes it.
|
||||||
|
const ownFilePaths = new Set(parsedFiles.map((p) => p.filePath));
|
||||||
|
|
||||||
for (const parsed of parsedFiles) {
|
for (const parsed of parsedFiles) {
|
||||||
for (const site of parsed.referenceSites) {
|
for (const site of parsed.referenceSites) {
|
||||||
if (site.kind !== 'read' && site.kind !== 'write') continue;
|
if (site.kind !== 'read' && site.kind !== 'write') continue;
|
||||||
|
|
@ -109,17 +122,86 @@ export function emitReturnShapeMemberAccesses(
|
||||||
// whole point: `formatSpikeAlert` is a function, and before R3-4 there
|
// whole point: `formatSpikeAlert` is a function, and before R3-4 there
|
||||||
// was nothing named after it to look a member up on.
|
// was nothing named after it to look a member up on.
|
||||||
const typeRef = findReceiverTypeBinding(site.inScope, receiver, indexes);
|
const typeRef = findReceiverTypeBinding(site.inScope, receiver, indexes);
|
||||||
const producer = typeRef?.rawName;
|
const producerRef = typeRef?.rawName;
|
||||||
if (producer === undefined || producer.length === 0) continue;
|
if (producerRef === undefined || producerRef.length === 0) continue;
|
||||||
|
|
||||||
|
// R3-4 qualifies a returned key by the producing function's own name, so
|
||||||
|
// the owner segment to match is the LAST one. For a plain producer this is
|
||||||
|
// a no-op.
|
||||||
|
//
|
||||||
|
// A MEMBER-CALL producer (`const r = svc.make()`) binds `svc.make`, and
|
||||||
|
// that spelling resolves to no value binding below, so this pass DECLINES
|
||||||
|
// rather than resolving it. That is a known coverage limit, not a fix:
|
||||||
|
// answering it means typing `svc` first and then finding `make` on that
|
||||||
|
// type, which is a different (and larger) piece of work. Declining is the
|
||||||
|
// correct behaviour in the meantime — the alternative, matching
|
||||||
|
// `make.<member>` by name across the graph, is precisely the fabrication
|
||||||
|
// the file guard below exists to stop.
|
||||||
|
const producer = producerRef.slice(producerRef.lastIndexOf('.') + 1);
|
||||||
|
if (producer.length === 0) continue;
|
||||||
|
|
||||||
|
// Resolve the producer to a real definition and keep only members that
|
||||||
|
// live in ITS file.
|
||||||
|
//
|
||||||
|
// Without this the join is textual over a whole-graph index: any node
|
||||||
|
// whose id happens to read `<producer>.<member>` matches, in any file and
|
||||||
|
// any LANGUAGE. Measured, that fabricated a 0.9-confidence edge from a JS
|
||||||
|
// component to a Java field — and 0.9 is the precise tier, so a
|
||||||
|
// `minConfidence` floor cannot filter it out. The sibling unique-name pass
|
||||||
|
// was given a per-language restriction for exactly this; this pass
|
||||||
|
// consumes the same shared index and had none.
|
||||||
|
//
|
||||||
|
// The file identity is the evidence, not a heuristic: R3-4 anchors a
|
||||||
|
// returned literal's keys to the function that returns them, so the
|
||||||
|
// member's node necessarily sits in the same file as that function. A
|
||||||
|
// candidate elsewhere is a different symbol wearing the same name.
|
||||||
|
// A CALLABLE lookup, not a value one: the producer is the function whose
|
||||||
|
// return shape owns the member. It also resolves through finalized import
|
||||||
|
// bindings, so a producer imported from another file still yields its own
|
||||||
|
// file — the guard restricts to the RIGHT file, it does not force same-file.
|
||||||
|
// Three guards, and they catch different shapes — none is redundant:
|
||||||
|
//
|
||||||
|
// producerDef — the producer must RESOLVE. This is the one that stops
|
||||||
|
// the measured cross-language leak: `new Loyalty()` in JS
|
||||||
|
// yields `producerRef = 'Loyalty'`, and a Java class does
|
||||||
|
// not resolve as a callable from a JS scope, so the pass
|
||||||
|
// declines instead of name-matching into `Loyalty.java`.
|
||||||
|
// Mutation-verified by `polyglot-property-isolation`.
|
||||||
|
// filePath — among same-named producers, keep the members of the one
|
||||||
|
// actually resolved. Defence in depth for the case where
|
||||||
|
// the producer DOES resolve and a same-named function
|
||||||
|
// exists in another file.
|
||||||
|
// ownFilePaths — a receiver typed by construction resolves through the
|
||||||
|
// shared, POLYGLOT class registry, so a producer can
|
||||||
|
// resolve into another language with its members
|
||||||
|
// legitimately in that same file. File equality passes
|
||||||
|
// there; only the language restriction closes it.
|
||||||
|
const producerDef = findCallableBindingInScope(site.inScope, producerRef, indexes);
|
||||||
|
const producerFile = producerDef?.filePath;
|
||||||
|
if (producerFile === undefined) continue;
|
||||||
|
if (!ownFilePaths.has(producerFile)) continue;
|
||||||
|
|
||||||
const candidates = propertyNameIndex.get(site.name);
|
const candidates = propertyNameIndex.get(site.name);
|
||||||
if (candidates === undefined) continue;
|
if (candidates === undefined) continue;
|
||||||
const owned = candidates.filter((c) => idNamesMember(c.id, producer, site.name));
|
const owned = candidates.filter(
|
||||||
|
(c) => c.filePath === producerFile && idNamesMember(c.id, producer, site.name),
|
||||||
|
);
|
||||||
// Exactly one, or nothing. Two nodes claiming `<producer>.<member>` would
|
// Exactly one, or nothing. Two nodes claiming `<producer>.<member>` would
|
||||||
// mean the id qualifier failed to separate them, and picking between them
|
// mean the id qualifier failed to separate them, and picking between them
|
||||||
// would be the guess this pass exists to avoid.
|
// would be the guess this pass exists to avoid.
|
||||||
if (owned.length !== 1) {
|
if (owned.length !== 1) {
|
||||||
if (owned.length === 0) memberNotOnShape++;
|
if (owned.length === 0) {
|
||||||
|
memberNotOnShape++;
|
||||||
|
// CLAIM THE SITE ANYWAY. This branch is the strongest NEGATIVE
|
||||||
|
// evidence the pipeline can produce: the receiver is typed to a
|
||||||
|
// producer, that producer's shape is known, and it owns no member of
|
||||||
|
// this name. Falling through let the 0.5 name fallback answer a
|
||||||
|
// question the precise pass had just DISPROVED — measured, it linked
|
||||||
|
// a read to an unrelated same-named key in another file. Disproving a
|
||||||
|
// member and then inventing it one pass later is worse than either
|
||||||
|
// answer alone.
|
||||||
|
handledSink.add(siteKey);
|
||||||
|
}
|
||||||
continue;
|
continue;
|
||||||
}
|
}
|
||||||
const target = owned[0]!;
|
const target = owned[0]!;
|
||||||
|
|
|
||||||
|
|
@ -14,3 +14,20 @@ export const jsConfig = {
|
||||||
export function readsJsOnly(bag) {
|
export function readsJsOnly(bag) {
|
||||||
return bag.jsOnlyThreshold;
|
return bag.jsOnlyThreshold;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// BOUND-RECEIVER ARM (review finding 2). The reads above have UNTYPED receivers,
|
||||||
|
// so they route through unique-name inference — the pass this fixture was
|
||||||
|
// written to police. One extra token gives the receiver a type and routes an
|
||||||
|
// identical read through `return-shape-members.ts` instead: a sibling pass that
|
||||||
|
// consumed the same whole-graph index with no language restriction, and emitted
|
||||||
|
// at the 0.9 PRECISE tier where a `minConfidence` floor cannot filter it out.
|
||||||
|
//
|
||||||
|
// `Loyalty` is declared ONLY in Java. Construction types the receiver through
|
||||||
|
// the shared (polyglot) class registry, so the producer resolves into
|
||||||
|
// `Loyalty.java` and its member genuinely lives in that same file — which is
|
||||||
|
// why a same-FILE check alone waves this through and only a same-LANGUAGE check
|
||||||
|
// stops it. Nothing here may resolve.
|
||||||
|
export function readsBoundLoyalty() {
|
||||||
|
const bound = new Loyalty();
|
||||||
|
return bound.loyaltyPointsBalance;
|
||||||
|
}
|
||||||
|
|
|
||||||
|
|
@ -36,6 +36,33 @@ describe('cross-language property inference (RV-5)', () => {
|
||||||
expect(readersOf('loyaltyPointsBalance')).not.toContain('renderLoyalty');
|
expect(readersOf('loyaltyPointsBalance')).not.toContain('renderLoyalty');
|
||||||
});
|
});
|
||||||
|
|
||||||
|
// The SAME boundary, reached through the other pass. `renderLoyalty` above has
|
||||||
|
// an untyped receiver, so it routes through unique-name inference — the pass
|
||||||
|
// this fixture was written to police. Typing the receiver by construction
|
||||||
|
// routes an identical read through `return-shape-members.ts` instead, which
|
||||||
|
// consumed the same whole-graph index with no language restriction and emitted
|
||||||
|
// at 0.9 rather than 0.5 — the PRECISE tier, where a `minConfidence` floor
|
||||||
|
// cannot filter the result out.
|
||||||
|
//
|
||||||
|
// Note WHY a same-file check was not enough here. `new Loyalty()` types the
|
||||||
|
// receiver through the shared class registry, which is polyglot, so the
|
||||||
|
// producer resolves into `Loyalty.java` and its member genuinely lives in that
|
||||||
|
// same file. File equality is satisfied; only the language restriction stops
|
||||||
|
// the edge.
|
||||||
|
describe('the bound-receiver path (review finding 2)', () => {
|
||||||
|
const targetsOf = (source: string): string[] =>
|
||||||
|
getRelationships(result, 'ACCESSES')
|
||||||
|
.filter((e) => e.source === source)
|
||||||
|
.map((e) => e.targetFilePath ?? '');
|
||||||
|
|
||||||
|
it('never reaches the Java field through the bound path', () => {
|
||||||
|
// Asserted on TARGET FILE, not on absence of the name: the leak this
|
||||||
|
// catches is an edge that exists and points into another language, which
|
||||||
|
// a name-only assertion would not distinguish from a correct local edge.
|
||||||
|
expect(targetsOf('readsBoundLoyalty').some((f) => f.includes('Loyalty.java'))).toBe(false);
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
// The other half: restricting by language must not disable the pass.
|
// The other half: restricting by language must not disable the pass.
|
||||||
it('still resolves a same-language unique name', () => {
|
it('still resolves a same-language unique name', () => {
|
||||||
expect(readersOf('jsOnlyThreshold')).toContain('readsJsOnly');
|
expect(readersOf('jsOnlyThreshold')).toContain('readsJsOnly');
|
||||||
|
|
|
||||||
Loading…
Add table
Reference in a new issue