fix: eliminate super-dispatch self-loops and align CHA confidence#1514
fix: eliminate super-dispatch self-loops and align CHA confidence#1514carlos-alm wants to merge 4 commits into
Conversation
…ing, prototype params to JS extractor Closes #1471 Four extraction gaps in the WASM JS extractor caused 25 node diffs against the native engine in the jelly-micro parity fixture: 1. Computed property method names (e.g. `['property7']`): the query patterns in both parser.ts and wasm-worker-entry.ts only matched `property_identifier` and `private_property_identifier` as the `name` field of `method_definition`; add a third pattern for `computed_property_name`. 2. Class expressions inside functions (`return class PostMixin …`): wasm-worker-entry.ts JS_CLASS_PATTERN was a single string matching only `class_declaration`; rename to JS_CLASS_PATTERNS array and add the `(class name: …)` pattern for anonymous class-expression nodes. Also add the matching `(class name: (type_identifier) …)` pattern to TS_EXTRA_PATTERNS so TS class expressions are covered too. 3. Array destructuring constants (`const [x, y] = …`): both `extractDestructuredBindingsWalk` (query path) and `handleVariableDecl` (walk path) only handled `object_pattern`; add an `array_pattern` branch that emits a single `constant` node whose name is the full array pattern text — matching native behaviour. 4. Parameters of prototype arrow/function methods (`Arit.prototype.sum = (x, y) => …`): `emitPrototypeMethod` emitted the definition node without calling `extractParameters`, so children were always absent; add the call and wire the result through. After this fix `parity-compare.mjs --langs jelly-micro` reports 0 node diffs (was 25). 22 pre-existing edge diffs remain (tracked in #1506) — those are out of scope.
runChaPostPass (helpers.ts, WASM path) was emitting self-loop edges like B.m → B.m when expanding class A's implementors: it found B.m via findMethodStmt but had no guard against the case where methodNode.id equals the source_id (the calling method itself). Add the missing guard. runPostNativeThisDispatch (native-orchestrator.ts) similarly lacked a self-loop guard in its targets loop — add it to keep the native path consistent. Also aligns runPostNativeCha to use file-pair-aware confidence (computeConfidence - CHA_DISPATCH_PENALTY) instead of a hardcoded 0.8, matching the WASM path. Gate B now checks the same kind set as Gate A for future-proofing. Remaining jelly-micro divergences filed as separate issues: - #1510: receiver-callee-mixup attribution (findCaller tie-break for same-line defs) - #1511: dynamic pts cross-file edges (fun.js, classes2.js apply/call) - #1512: native missing f.h→f.g this-dispatch for func-prop methods - #1513: prototypes.js receiver edge non-determinism docs check acknowledged: no language, feature, or architecture changes. Closes #1472
|
Claude finished @carlos-alm's task —— View job PR Review — fix: eliminate super-dispatch self-loops and align CHA confidence
Root Cause AnalysisProblem Being Solved: The CHA (Class Hierarchy Analysis) post-pass emitted Why This Approach: The fix is a one-line guard inserted at the correct place in both CHA post-pass implementations ( Risk Assessment: Low. The self-loop guard is a pure correctness improvement. The confidence alignment uses the same function already used on the WASM path. Gate B kind-set expansion is forward-compatible. The test additions cover the new extraction behaviors from #1471. Phase 2: Configuration Change DetectionNo linting rules relaxed, no test timeouts increased, no build config modified, no TypeScript strict mode flags disabled. ✓ Phase 3: Test IntegrityThe test file adds 5 new
All assertions are additive — they verify new extraction behaviors introduced by #1471, not weakened. No existing assertions were removed or relaxed. ✓ One potential gap: there are no tests for the self-loop guard fix itself. A test like: it('does not emit B.m → B.m self-loop from super.m() CHA expansion', () => { ... })would be a valuable regression guard at the unit level. The parity-compare script catches it at integration level, but that script isn't in the automated test suite. Phase 4: Code Quality
if (t.id === callerRow.id) continue; // skip self-loopsThis is correct but note a subtle asymmetry: in
const conf =
computeConfidence(caller_file ?? '', methodNode.method_file ?? '', null) -
CHA_DISPATCH_PENALTY;
if (conf <= 0) continue;The Gate B kind-set expansion ( AND tgt.kind IN ('class', 'interface', 'trait', 'struct', 'record')Previously Gate B only checked
Both correctly mirror native engine behavior per the PR description. The array pattern branch is correctly guarded with Minor concern: Backlog Compliance
Critical ConcernsNone blocking. One minor observation: The Final Recommendation
|
Greptile SummaryThis PR fixes three CHA post-pass bugs that caused jelly-micro parity divergences after the node-extraction work in #1471. The primary fix eliminates
Confidence Score: 5/5Safe to merge — all three CHA expansion sites now guard against self-loops, and the Gate B alignment is a strictly conservative widening. The changes are narrow, targeted, and well-tested. Each fix is a one-line guard (or a constant-set alignment) with clear explanations, matching logic already present in sibling functions. The previously flagged missing self-loop guard in runPostNativeCha is fully addressed, and the confidence-asymmetry gap is explicitly tracked with a TODO. No new unsafe paths are introduced. No files require special attention. The acknowledged confidence asymmetry between runChaPostPass (hardcoded 0.8) and runPostNativeCha (dynamic computeConfidence) is tracked with a TODO in helpers.ts. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["B.m calls super.m()"] --> B["Edge: B.m → A.m inserted"]
B --> C["runChaPostPass / runPostNativeCha\nBFS over A's implementors"]
C --> D{"methodNode.id\n=== source_id?"}
D -->|"YES (self-loop) NEW GUARD"| E["continue — skip B.m → B.m"]
D -->|"NO"| F{"conf <= 0?"}
F -->|"YES"| G["continue — skip low-confidence edge"]
F -->|"NO"| H["Insert CHA dispatch edge"]
subgraph "runPostNativeCha Gate Logic"
I["changedFiles provided?"] --> J{"Gate A: any class/interface/trait/struct/record node in changed files?"}
J -->|"YES"| K["Full scan (scopeToChangedFiles=false)"]
J -->|"NO"| L{"Gate B aligned: calls to class/interface/trait/struct/record targets in changed files?"}
L -->|"YES"| K
L -->|"NO"| M["Scoped scan (scopeToChangedFiles=true)"]
end
subgraph "runPostNativeThisDispatch"
N["resolve this/super dispatch"] --> O{"t.id === callerRow.id? NEW GUARD"}
O -->|"YES"| P["continue — skip self-loop"]
O -->|"NO"| Q["Insert dispatch edge"]
end
Reviews (5): Last reviewed commit: "fix(comments): correct inaccurate WASM-p..." | Re-trigger Greptile |
Codegraph Impact Analysis3 functions changed → 6 callers affected across 5 files
|
…ence alignment (#1514) runPostNativeCha's BFS inner loop was missing the same self-loop guard already added to runChaPostPass and runPostNativeThisDispatch, allowing B.m→B.m edges to persist on the native CHA path when super-dispatch triggers CHA expansion. Also adds a TODO comment on the hardcoded 0.8 confidence in runChaPostPass noting it needs alignment with the formula used in runPostNativeCha (requires extending findMethodStmt to fetch file).
|
Fixed both issues raised in the Greptile review: P1 — Missing self-loop guard in P2 — Asymmetric confidence in Commit: fc6bc92 |
…Cha (#1514) The comments said the confidence formula "mirrors the WASM path" but the WASM path (runChaPostPass in helpers.ts) still uses hardcoded 0.8, not computeConfidence - CHA_DISPATCH_PENALTY. Correct both comments to accurately describe the native formula and cross-reference the TODO in helpers.ts for the pending alignment.
|
Addressed the comment accuracy issue flagged by Greptile: the two comments in |
1 participant
Summary
After the node extraction gaps fixed in #1471, addresses the root CHA post-pass bugs causing jelly-micro parity divergences:
runChaPostPassinhelpers.tswas expanding class A's implementors without guarding againstmethodNode.id === source_id. WhenB.mcalledsuper.m()producing aB.m → A.medge, the CHA pass walked A's children (including B) and emittedB.m → B.mself-loops. One-line guard added.runPostNativeThisDispatchinnative-orchestrator.tsalso lacked this self-loop guard. Added for consistency.runPostNativeChaconfidence aligned: UsescomputeConfidence(callerFile, targetFile) - CHA_DISPATCH_PENALTYinstead of hardcoded0.8, matching the WASM path. Gate B now checks the same kind set as Gate A.parity-compare.mjs --langs jelly-micro --hybrid: 22 wasm/native diffs → 18 (4 self-loops eliminated), 9 wasm/hybrid diffs unchanged.Remaining 4 divergences deferred to separate issues: