morluto/rea

Fork 0

mirror of https://github.com/morluto/rea.git synced 2026-11-05 06:45:22 +00:00

[PR #4349] [CLOSED] fix(javascript): close full-read alias cycles at the binding in call resolution #6193

New issue

Closed

opened 2026-10-31 01:12:34 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-31 01:12:34 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/4349
Author: @kaoru0822-kitauji
Created: 10/16/2026
Status: ❌ Closed

Base: main ← Head: fix/call-resolution-full-read-cycles

📝 Commits (3)

91a863c fix(javascript): close full-read alias cycles at the binding in call resolution
840d707 docs(javascript): state the quadratic bound for full-read alias meshes
a262e1c test(javascript): pin mesh call targets and cycle dependence

📊 Changes

2 files changed (+48 additions, -1 deletions)

View changed files

📝 src/domain/javascript/javascriptSemanticCallResolution.test.ts (+23 -0)
📝 src/domain/javascript/javascriptSemanticCallResolution.ts (+25 -1)

📄 Description

Summary

#4243 made resolveLocalCallables detect cycles per initializer instead of per binding. Results inside a cycle are cycleDependent and never cached, so mutual reassignments whose reads select every write now enumerate every initializer order. This PR closes those full-read cycles at the binding again. Position-narrowed reads keep #4243's per-initializer cycles, and outputs are unchanged.

Refs #4348. This fixes the call-resolution part (1) of that issue. Part (2), returned-reference expansion on the playwright-core React bundles, is still open and needs a design decision there.

Problem and expected behavior

let a = f, b = g; function run() { a = b; b = a; /* … n writes … */ a().x = 1; }

Inside run, every read of a and b has unresolved ordering, so semanticBindingAtRead returns the whole binding. resolveBindingCallables then cuts an initializer only when that exact initializer is already in seenInitializers, which walks every simple path through the initializers. With 12 writes this takes 1.5 s, about 35× more per added pair; before #4243 it took 1 ms. A CPU profile attributes 90% of the time to resolveIdentifier → resolveBindingCallables.

Expected: polynomial work in the number of writes, with the same callableIds, complete and cycleDependent results.

Change and scope

resolveIdentifier checks whether the read selected every initializer (read.binding === binding). If so and the binding is already on the active chain (new internal seenBindings), it returns the existing cycle-cut result. Otherwise it records the binding before resolving the initializers.
Why the result is unchanged: such a re-entry resolves exactly the candidates of the outer entry, under a superset of seen keys. Resolution is monotone in that set (more seen keys means more cuts, so fewer callables and never more complete), so the inner union is contained in the outer one. The outer complete is already false at a cycle, and both forms mark cycleDependent.
Position-narrowed reads keep per-initializer cycles, which #4243 introduced for them.
ResolveLocalCallablesInput is unchanged; seenBindings is internal.
Not included: createSemanticReturnedReferences (#4348 part 2).

Contract and boundary impact

Semantic owner and earliest changed stage: javascriptSemanticCallResolution.ts (domain call resolution).
Contribution scope fit and linked maintainer decision: not applicable (bug fix inside the existing analysis boundary).
CLI and MCP/tool-catalog contract: none.
Provider, bridge, target-format, or platform compatibility: none.
Evidence, artifact, provenance, or reconstruction contract: unchanged; outputs are byte-identical (below).
Target selection, authority, upstream behavior, and cleanup details: not applicable.
Generated metadata, package, or installation impact: none.
Read existing files or captured data.

Evidence and regression coverage

Executed on macOS 26 arm64, Node 26.5.0, main at 42fc6c53a.

Workload	main	This PR
Mesh above, semantic phase, n = 10 / 12	44 ms / 1,526 ms	2 ms / 5 ms
`resolveLocalCallables` binding-cache lookups for `a()`, n = 6 / 10 / 20 / 40 / 80	exponential (111,462 at n = 10 in the test)	17 / 37 / 122 / 442 / 1,682 (≈ (n/2)², 1.2 ms at n = 80)
New test's binding-cache lookups, 6 → 10 writes	278 → 111,462	17 → 37
Semantic IR over 151 real files (VS Code app bundle, `node_modules`, Obsidian sources), one child per file, no other load	45.6 s and 45.8 s (two runs)	45.7 s; 0 of 151 digests differ

Tests added: javascriptSemanticCallResolution.test.ts adds closes reassignment meshes at bindings whose reads select every write. It resolves a() after 6 and 10 alternating writes, expects exactly f and g with complete: false and cycleDependent: true (main returns the same), and requires the cache lookups to grow less than 3× (the fix is quadratic: each write to a still expands every write to b once before the cycle closes, because cycle-dependent results are not cached). On main the test fails with expected 111462 to be less than 834; with the fix, 11/11 pass. #4243's acceptance test tests/acceptance/applications/javascriptCallbackTargets.test.ts passes (2/2).
User-visible CLI/MCP output: unchanged.
Remaining proof gaps: the two playwright-core bundles in #4348 still exhaust the heap; their cost is in returned-reference expansion. No Linux or Windows measurements.
Observed, derived, and inferred claims remain distinguishable.
Artifact identity, source provenance, and failed attempts remain preserved.
Unsupported, incomplete, unavailable, or uncertain outcomes remain visible.

Validation performed

npm run test:local -- <94 JavaScript domain/application/worker/reference-source test files>: 94 files, 2253 tests passed.
npm run test:local -- src/domain/javascript/javascriptSemanticCallResolution.test.ts on main's implementation: 1 failed (expected 111462 to be less than 834); with the fix: 11 passed.
npm run test:focused -- tests/acceptance/applications/javascriptCallbackTargets.test.ts: 2 passed.
npm run check:fast: typecheck and lint passed.
Per-file semantic comparison over 151 real files, main vs fix: 0 differing digests; timing above.

Compatibility, safety, and release

Breaking changes or migration steps: none.
Real Hopper/Ghidra, browser, or OS coverage: macOS 26 arm64 only.
Package or release metadata impact: none.
Security, privacy, process, or containment review: removes an exponential on inert parsing; no new process, network, or file behavior.

Author readiness

Human owner:

I reviewed the final diff and relevant existing code, understand the change and its edge cases, and take responsibility for the complete PR, including AI-assisted work.
I checked the evidence above, confirmed it supports the current change, and recorded meaningful validation gaps.
I declare this PR ready for initial maintainer review.

Before merge

This PR addresses a concrete problem or an agreed enhancement.
Material scope and authority questions are resolved, and any required maintainer scope decision covers the final diff.
The PR has one focused outcome and the title follows type(scope): outcome.
Related issue is linked, or the reason for not linking one is stated above.
Tests cover changed observable behavior and meaningful failure paths.
Owning docs, contracts, and generated metadata are updated where needed.
User-visible CLI/MCP changes include representative output.
I checked the final diff for secrets, unrelated cleanup, and unsupported claims.

🤖 Generated with Claude Code

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/morluto/rea/pull/4349 **Author:** [@kaoru0822-kitauji](https://github.com/kaoru0822-kitauji) **Created:** 10/16/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `fix/call-resolution-full-read-cycles` --- ### 📝 Commits (3) - [`91a863c`](https://github.com/morluto/rea/commit/91a863c73a79a9a6973313d6d4430240383e4a43) fix(javascript): close full-read alias cycles at the binding in call resolution - [`840d707`](https://github.com/morluto/rea/commit/840d707257ae234acccdba36e442c934fb816cc3) docs(javascript): state the quadratic bound for full-read alias meshes - [`a262e1c`](https://github.com/morluto/rea/commit/a262e1c2208bab0f3a892f60759ce6e8b6e3b1c9) test(javascript): pin mesh call targets and cycle dependence ### 📊 Changes **2 files changed** (+48 additions, -1 deletions) <details> <summary>View changed files</summary> 📝 `src/domain/javascript/javascriptSemanticCallResolution.test.ts` (+23 -0) 📝 `src/domain/javascript/javascriptSemanticCallResolution.ts` (+25 -1) </details> ### 📄 Description ## Summary #4243 made `resolveLocalCallables` detect cycles per initializer instead of per binding. Results inside a cycle are `cycleDependent` and never cached, so mutual reassignments whose reads select every write now enumerate every initializer order. This PR closes those full-read cycles at the binding again. Position-narrowed reads keep #4243's per-initializer cycles, and outputs are unchanged. Refs #4348. This fixes the call-resolution part (1) of that issue. Part (2), returned-reference expansion on the `playwright-core` React bundles, is still open and needs a design decision there. ## Problem and expected behavior ```js let a = f, b = g; function run() { a = b; b = a; /* … n writes … */ a().x = 1; } ``` Inside `run`, every read of `a` and `b` has unresolved ordering, so `semanticBindingAtRead` returns the whole binding. `resolveBindingCallables` then cuts an initializer only when that exact initializer is already in `seenInitializers`, which walks every simple path through the initializers. With 12 writes this takes 1.5 s, about 35× more per added pair; before #4243 it took 1 ms. A CPU profile attributes 90% of the time to `resolveIdentifier` → `resolveBindingCallables`. Expected: polynomial work in the number of writes, with the same `callableIds`, `complete` and `cycleDependent` results. ## Change and scope - `resolveIdentifier` checks whether the read selected every initializer (`read.binding === binding`). If so and the binding is already on the active chain (new internal `seenBindings`), it returns the existing cycle-cut result. Otherwise it records the binding before resolving the initializers. - Why the result is unchanged: such a re-entry resolves exactly the candidates of the outer entry, under a superset of seen keys. Resolution is monotone in that set (more seen keys means more cuts, so fewer callables and never more complete), so the inner union is contained in the outer one. The outer `complete` is already `false` at a cycle, and both forms mark `cycleDependent`. - Position-narrowed reads keep per-initializer cycles, which #4243 introduced for them. - `ResolveLocalCallablesInput` is unchanged; `seenBindings` is internal. - Not included: `createSemanticReturnedReferences` (#4348 part 2). ## Contract and boundary impact - Semantic owner and earliest changed stage: `javascriptSemanticCallResolution.ts` (domain call resolution). - Contribution scope fit and linked maintainer decision: not applicable (bug fix inside the existing analysis boundary). - CLI and MCP/tool-catalog contract: none. - Provider, bridge, target-format, or platform compatibility: none. - Evidence, artifact, provenance, or reconstruction contract: unchanged; outputs are byte-identical (below). - Target selection, authority, upstream behavior, and cleanup details: not applicable. - Generated metadata, package, or installation impact: none. - [x] Read existing files or captured data. ## Evidence and regression coverage Executed on macOS 26 arm64, Node 26.5.0, main at `42fc6c53a`. | Workload | main | This PR | | --- | ---: | ---: | | Mesh above, semantic phase, n = 10 / 12 | 44 ms / 1,526 ms | 2 ms / 5 ms | | `resolveLocalCallables` binding-cache lookups for `a()`, n = 6 / 10 / 20 / 40 / 80 | exponential (111,462 at n = 10 in the test) | 17 / 37 / 122 / 442 / 1,682 (≈ (n/2)², 1.2 ms at n = 80) | | New test's binding-cache lookups, 6 → 10 writes | 278 → **111,462** | 17 → 37 | | Semantic IR over 151 real files (VS Code app bundle, `node_modules`, Obsidian sources), one child per file, no other load | 45.6 s and 45.8 s (two runs) | 45.7 s; **0 of 151 digests differ** | - Tests added: `javascriptSemanticCallResolution.test.ts` adds `closes reassignment meshes at bindings whose reads select every write`. It resolves `a()` after 6 and 10 alternating writes, expects exactly `f` and `g` with `complete: false` and `cycleDependent: true` (main returns the same), and requires the cache lookups to grow less than 3× (the fix is quadratic: each write to `a` still expands every write to `b` once before the cycle closes, because cycle-dependent results are not cached). On main the test fails with `expected 111462 to be less than 834`; with the fix, 11/11 pass. #4243's acceptance test `tests/acceptance/applications/javascriptCallbackTargets.test.ts` passes (2/2). - User-visible CLI/MCP output: unchanged. - Remaining proof gaps: the two `playwright-core` bundles in #4348 still exhaust the heap; their cost is in returned-reference expansion. No Linux or Windows measurements. - [x] Observed, derived, and inferred claims remain distinguishable. - [x] Artifact identity, source provenance, and failed attempts remain preserved. - [x] Unsupported, incomplete, unavailable, or uncertain outcomes remain visible. ## Validation performed - `npm run test:local -- <94 JavaScript domain/application/worker/reference-source test files>`: 94 files, 2253 tests passed. - `npm run test:local -- src/domain/javascript/javascriptSemanticCallResolution.test.ts` on main's implementation: 1 failed (`expected 111462 to be less than 834`); with the fix: 11 passed. - `npm run test:focused -- tests/acceptance/applications/javascriptCallbackTargets.test.ts`: 2 passed. - `npm run check:fast`: typecheck and lint passed. - Per-file semantic comparison over 151 real files, main vs fix: 0 differing digests; timing above. ## Compatibility, safety, and release - Breaking changes or migration steps: none. - Real Hopper/Ghidra, browser, or OS coverage: macOS 26 arm64 only. - Package or release metadata impact: none. - Security, privacy, process, or containment review: removes an exponential on inert parsing; no new process, network, or file behavior. ## Author readiness Human owner:  - [ ] I reviewed the final diff and relevant existing code, understand the change and its edge cases, and take responsibility for the complete PR, including AI-assisted work. - [ ] I checked the evidence above, confirmed it supports the current change, and recorded meaningful validation gaps. - [ ] I declare this PR ready for initial maintainer review. ## Before merge - [ ] This PR addresses a concrete problem or an agreed enhancement. - [ ] Material scope and authority questions are resolved, and any required maintainer scope decision covers the final diff. - [ ] The PR has one focused outcome and the title follows `type(scope): outcome`. - [ ] Related issue is linked, or the reason for not linking one is stated above. - [ ] Tests cover changed observable behavior and meaningful failure paths. - [ ] Owning docs, contracts, and generated metadata are updated where needed. - [ ] User-visible CLI/MCP changes include representative output. - [ ] I checked the final diff for secrets, unrelated cleanup, and unsupported claims. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-31 01:12:34 +00:00

closed this issue
added the
pull-request
label

bsakaguchi referenced this issue

2026-11-01 21:46:06 +00:00

[PR #6206] [MERGED] feat(javascript): link assignment right-hand sides to their writes #7203

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

morluto/rea#6193

No description provided.

Rows
Columns