morluto/rea

Fork 0

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

[PR #2881] [CLOSED] fix(javascript): classify member assignment targets as property writes #5465

New issue

Closed

opened 2026-10-31 01:06:51 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-31 01:06:51 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/2881
Author: @0x-saurabh
Created: 10/13/2026
Status: ❌ Closed

Base: main ← Head: fix/javascript-member-write-classification

📝 Commits (4)

fb4b8a4 fix(javascript): classify member assignment targets as property writes
0439fd2 fix(javascript): retain reads for wrapped member update targets
0ec4480 refactor(javascript): share read-modify-write predicate
ef85297 fix(javascript): resolve wrapped write targets through the shared write-site owner

📊 Changes

10 files changed (+297 additions, -49 deletions)

View changed files

📝 src/application/javascript/JavaScriptSemanticGraphBuilder.test.ts (+26 -0)
📝 src/domain/javascript/javascriptAstValues.ts (+54 -0)
📝 src/domain/javascript/javascriptElectronMemberWrites.ts (+3 -10)
📝 src/domain/javascript/javascriptSemanticAnalysis.dataflow.test.ts (+89 -0)
📝 src/domain/javascript/javascriptSemanticAnalysis.ts (+9 -6)
📝 src/domain/javascript/javascriptSemanticMemberMutations.ts (+3 -6)
📝 src/domain/javascript/javascriptSemanticObjects.ts (+27 -9)
📝 src/domain/javascript/javascriptSemanticProjection.ts (+26 -8)
📝 src/domain/javascript/javascriptSymbolicBindings.test.ts (+28 -0)
📝 src/domain/javascript/javascriptSymbolicBindings.ts (+32 -10)

📄 Description

Summary

Fixes #2567.

JavaScript semantic analysis classified a member expression as a property write only when its immediate parent was an assignment or update expression. A member that is the target of a destructuring assignment ([o.count] = [2]), a for...of / for...in head (for (o.count of list)), or delete o.count therefore produced a read object operation and a false reads-property relation in the semantic graph, although none of these forms reads the property value.

Problem and expected behavior

Smallest trigger:

const source = { count: 1 };
[source.count] = [2];

ir.objectOperations reported ["read"] and the graph projected a read:/count occurrence with a reads-property relation. The violated invariant is the one the sibling member-mutation analysis already follows: these constructs assign into their target and are writes, so the object operation must be write, the occurrence write:/count, and the relation writes-property. A direct source.count = 2 must stay a write; a plain source.count access, and a member used as a destructuring default value ([v = source.count] = []), must stay reads.

Change and scope

The object collector now classifies a member from its owning write site rather than from its immediate parent. When the traversal enters a write site it records each written member leaf, keyed to that site, through the existing semanticWriteTargets walker; collectMember consumes the entry. Read-before-write is decided from the owning site through a new shared predicate, semanticWriteSiteReadsFirst, so TypeScript-wrapped targets such as source.count!++ or (source.count as number) += 1 keep their property read (previously these were read-only, since the wrapper hid the assignment parent).

The notion of "which constructs are write sites" previously lived inline in javascriptSemanticMemberMutations.ts and again in javascriptElectronMemberWrites.ts. It is now one exported helper, semanticWriteSiteTarget, in javascriptAstValues.ts next to semanticWriteTargets, and the object collector, the member-mutation analysis, the Electron provenance collector, the identifier reference collector, binding assignment, and the symbolic bindings resolver all dispatch through it. No consumer reinterprets the syntax.

Review follow-ups on the same owner:

semanticReadsBeforeWrite delegates to semanticWriteSiteTarget and semanticWriteSiteReadsFirst (maintainer commit 5fbeb4313), so identifier references and member operations share one read-before-write rule.
Wrapped identifier targets (count! += 1, (count as number)++) are classified from their write site through a new semanticWrappedWriteSite helper, so the reference collector emits the read and the write, and binding assignment sees through the wrapper so the old exact value is not retained.
The symbolic bindings resolver (javascriptSymbolicBindings.ts) dispatches through semanticWriteSiteTarget. A direct identifier or member target records its value as before; a destructuring pattern or loop head is flattened with semanticWriteTargets and each leaf conservatively invalidates its root binding, so [table[1]] = [replacement] or for (table[1] of list) no longer leaves a stale bootstrap slot. Wrappers inside a member chain ((table as never)[1]) are unwrapped during the walk, matching the object collector.

Not included: the value-projection side (writes-property relations added from the value analysis) is untouched; this change only removes the false read occurrence and adds the missing write occurrence at the object-operation boundary. No CLI, MCP, or catalog contract changes.

Follow the contribution scope.

Contract and boundary impact

Semantic owner and earliest changed stage: collectJavaScriptSemanticObjects in src/domain/javascript/javascriptSemanticObjects.ts (domain static analysis). The graph projection in JavaScriptSemanticGraphObjectProjection.ts consumes the corrected operation kind unchanged.
Contribution scope fit and linked maintainer decision: ordinary static-analysis correctness fix within existing boundaries; not applicable.
CLI and MCP/tool-catalog contract: none. Object operation kinds and relation names already exist; only their assignment to these syntax forms changes.
Provider, bridge, target-format, or platform compatibility: none.
Evidence, artifact, provenance, or reconstruction contract: semantic-graph output for affected sources gains a writes-property relation and loses a false reads-property relation for the same location. No identity or provenance fields change.
Target selection, authority, upstream behavior, and cleanup details (if applicable): not applicable; static source parsing only.
Generated metadata (docs/public/product-catalog.json), package, or installation impact: none.

Expected effects:

Read existing files or captured data.
Launch or attach to a process.
Inject instrumentation or run scripts/code.
Change process memory, registers, files, or configuration.
Contact network services.
Recover protected code or work around protections.
Documentation or usability only; no target interaction.
Not sure yet; investigation or maintainer guidance needed.

Evidence and regression coverage

Tests added or updated (listed per file; the last two bullets are the review follow-ups):
- src/domain/javascript/javascriptSemanticAnalysis.dataflow.test.ts: seven it.each cases (destructuring assignment, with default, nested, rest, for...of head, for...in head, delete) assert objectOperations is exactly ["write"]; one control asserts a member in a destructuring default value stays ["read"]. Four wrapped read-modify-write cases (source.count!++, source.count! += 1, (source.count as number) += 1, (source.count)++) assert ["read", "write"], and two wrapped write-only cases ((source.count as number) = 2, delete source.count!) assert ["write"]. The existing direct-assignment and read-modify-write cases remain and still pass.
- src/domain/javascript/javascriptSemanticAnalysis.dataflow.test.ts (wrapped binding targets): count! += 1; (count as number)++; const copy = count! yields reference roles read, write, read, write, read and the binding value becomes unknown; count! = 2 yields a single write.
- src/domain/javascript/javascriptSymbolicBindings.test.ts: a bootstrap module slot overwritten through a destructuring assignment, a for...of head, or a wrapped member chain makes the later load(1) unknown.
- src/application/javascript/JavaScriptSemanticGraphBuilder.test.ts: [source.count] = [2] projects a single write:/count occurrence with a resolved writes-property relation to the /count slot and no reads-property relation.
Base reproduction or other evidence: executed. With the source files stashed and only the tests applied, the seven new dataflow cases fail (each reports ["read"]); the direct-assignment control and the default-value control pass. The three TypeScript-wrapped read-modify-write cases fail against the first commit of this PR (write-only) and pass after the second. With the fix applied all cases pass.
User-visible CLI/MCP output (if applicable): not applicable.
Remaining proof gaps: none known for the listed forms. Member targets inside destructuring declarations (const [x] = ...) cannot contain member expressions and are unaffected.
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:focused -- src/domain/javascript/javascriptSemanticAnalysis.dataflow.test.ts src/domain/javascript/javascriptSymbolicBindings.test.ts src/domain/javascript/javascriptSemanticAnalysis.member_mutations.test.ts src/application/javascript/JavaScriptSemanticGraphBuilder.test.ts — 4 files, 336 tests passed (at 7051da087).
Same dataflow and symbolic-bindings test files with the four changed source files reverted to aa3047ad5 (tests only) — the five review follow-up cases fail (two wrapped-identifier, three symbolic-bindings); the rest pass.
Same focused run with the three source files stashed (tests only) — the seven new dataflow cases fail with ["read"]; both controls pass. Confirms the regression reproduces on the base.
npm run check:fast — typecheck and lint, 2 tasks successful (at 7051da087).
npm run check:changed — 89 test files, 2000 tests passed (at 7051da087).
git diff --check — clean.
Toolchain: Node 24.18.0 (.nvmrc), npm 11.16.0, macOS arm64. No real-provider lane applies.

Compatibility, safety, and release

Breaking changes or migration steps: none.
Real Hopper/Ghidra, browser, or OS coverage: not applicable; pure static JavaScript analysis.
Package or release metadata impact: none.
Security, privacy, process, or containment review: no new I/O, process, or network behavior.

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/2881 **Author:** [@0x-saurabh](https://github.com/0x-saurabh) **Created:** 10/13/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `fix/javascript-member-write-classification` --- ### 📝 Commits (4) - [`fb4b8a4`](https://github.com/morluto/rea/commit/fb4b8a47095747c64c2710553aa1f194bfce00f2) fix(javascript): classify member assignment targets as property writes - [`0439fd2`](https://github.com/morluto/rea/commit/0439fd2c4a6a35b1ae3f2f614a8f393ac38df358) fix(javascript): retain reads for wrapped member update targets - [`0ec4480`](https://github.com/morluto/rea/commit/0ec4480c0a787285c50ee35f8f7aea0fa5e77737) refactor(javascript): share read-modify-write predicate - [`ef85297`](https://github.com/morluto/rea/commit/ef852975999fe24d14084664b13aa56c01b30d9e) fix(javascript): resolve wrapped write targets through the shared write-site owner ### 📊 Changes **10 files changed** (+297 additions, -49 deletions) <details> <summary>View changed files</summary> 📝 `src/application/javascript/JavaScriptSemanticGraphBuilder.test.ts` (+26 -0) 📝 `src/domain/javascript/javascriptAstValues.ts` (+54 -0) 📝 `src/domain/javascript/javascriptElectronMemberWrites.ts` (+3 -10) 📝 `src/domain/javascript/javascriptSemanticAnalysis.dataflow.test.ts` (+89 -0) 📝 `src/domain/javascript/javascriptSemanticAnalysis.ts` (+9 -6) 📝 `src/domain/javascript/javascriptSemanticMemberMutations.ts` (+3 -6) 📝 `src/domain/javascript/javascriptSemanticObjects.ts` (+27 -9) 📝 `src/domain/javascript/javascriptSemanticProjection.ts` (+26 -8) 📝 `src/domain/javascript/javascriptSymbolicBindings.test.ts` (+28 -0) 📝 `src/domain/javascript/javascriptSymbolicBindings.ts` (+32 -10) </details> ### 📄 Description ## Summary Fixes #2567. JavaScript semantic analysis classified a member expression as a property write only when its immediate parent was an assignment or update expression. A member that is the target of a destructuring assignment (`[o.count] = [2]`), a `for...of` / `for...in` head (`for (o.count of list)`), or `delete o.count` therefore produced a `read` object operation and a false `reads-property` relation in the semantic graph, although none of these forms reads the property value. ## Problem and expected behavior Smallest trigger: ```js const source = { count: 1 }; [source.count] = [2]; ``` `ir.objectOperations` reported `["read"]` and the graph projected a `read:/count` occurrence with a `reads-property` relation. The violated invariant is the one the sibling member-mutation analysis already follows: these constructs assign into their target and are writes, so the object operation must be `write`, the occurrence `write:/count`, and the relation `writes-property`. A direct `source.count = 2` must stay a write; a plain `source.count` access, and a member used as a destructuring *default value* (`[v = source.count] = []`), must stay reads. ## Change and scope The object collector now classifies a member from its owning write site rather than from its immediate parent. When the traversal enters a write site it records each written member leaf, keyed to that site, through the existing `semanticWriteTargets` walker; `collectMember` consumes the entry. Read-before-write is decided from the owning site through a new shared predicate, `semanticWriteSiteReadsFirst`, so TypeScript-wrapped targets such as `source.count!++` or `(source.count as number) += 1` keep their property read (previously these were read-only, since the wrapper hid the assignment parent). The notion of "which constructs are write sites" previously lived inline in `javascriptSemanticMemberMutations.ts` and again in `javascriptElectronMemberWrites.ts`. It is now one exported helper, `semanticWriteSiteTarget`, in `javascriptAstValues.ts` next to `semanticWriteTargets`, and the object collector, the member-mutation analysis, the Electron provenance collector, the identifier reference collector, binding assignment, and the symbolic bindings resolver all dispatch through it. No consumer reinterprets the syntax. Review follow-ups on the same owner: - `semanticReadsBeforeWrite` delegates to `semanticWriteSiteTarget` and `semanticWriteSiteReadsFirst` (maintainer commit `5fbeb4313`), so identifier references and member operations share one read-before-write rule. - Wrapped identifier targets (`count! += 1`, `(count as number)++`) are classified from their write site through a new `semanticWrappedWriteSite` helper, so the reference collector emits the read and the write, and binding assignment sees through the wrapper so the old exact value is not retained. - The symbolic bindings resolver (`javascriptSymbolicBindings.ts`) dispatches through `semanticWriteSiteTarget`. A direct identifier or member target records its value as before; a destructuring pattern or loop head is flattened with `semanticWriteTargets` and each leaf conservatively invalidates its root binding, so `[table[1]] = [replacement]` or `for (table[1] of list)` no longer leaves a stale bootstrap slot. Wrappers inside a member chain (`(table as never)[1]`) are unwrapped during the walk, matching the object collector. Not included: the value-projection side (`writes-property` relations added from the value analysis) is untouched; this change only removes the false read occurrence and adds the missing write occurrence at the object-operation boundary. No CLI, MCP, or catalog contract changes. Follow the [contribution scope](https://github.com/morluto/rea/blob/main/CONTRIBUTING.md#contribution-scope). ## Contract and boundary impact - Semantic owner and earliest changed stage: `collectJavaScriptSemanticObjects` in `src/domain/javascript/javascriptSemanticObjects.ts` (domain static analysis). The graph projection in `JavaScriptSemanticGraphObjectProjection.ts` consumes the corrected operation kind unchanged. - Contribution scope fit and linked maintainer decision: ordinary static-analysis correctness fix within existing boundaries; not applicable. - CLI and MCP/tool-catalog contract: none. Object operation kinds and relation names already exist; only their assignment to these syntax forms changes. - Provider, bridge, target-format, or platform compatibility: none. - Evidence, artifact, provenance, or reconstruction contract: semantic-graph output for affected sources gains a `writes-property` relation and loses a false `reads-property` relation for the same location. No identity or provenance fields change. - Target selection, authority, upstream behavior, and cleanup details (if applicable): not applicable; static source parsing only. - Generated metadata (`docs/public/product-catalog.json`), package, or installation impact: none. Expected effects: - [x] Read existing files or captured data. - [ ] Launch or attach to a process. - [ ] Inject instrumentation or run scripts/code. - [ ] Change process memory, registers, files, or configuration. - [ ] Contact network services. - [ ] Recover protected code or work around protections. - [ ] Documentation or usability only; no target interaction. - [ ] Not sure yet; investigation or maintainer guidance needed. ## Evidence and regression coverage - Tests added or updated (listed per file; the last two bullets are the review follow-ups): - `src/domain/javascript/javascriptSemanticAnalysis.dataflow.test.ts`: seven `it.each` cases (destructuring assignment, with default, nested, rest, `for...of` head, `for...in` head, `delete`) assert `objectOperations` is exactly `["write"]`; one control asserts a member in a destructuring default value stays `["read"]`. Four wrapped read-modify-write cases (`source.count!++`, `source.count! += 1`, `(source.count as number) += 1`, `(source.count)++`) assert `["read", "write"]`, and two wrapped write-only cases (`(source.count as number) = 2`, `delete source.count!`) assert `["write"]`. The existing direct-assignment and read-modify-write cases remain and still pass. - `src/domain/javascript/javascriptSemanticAnalysis.dataflow.test.ts` (wrapped binding targets): `count! += 1; (count as number)++; const copy = count!` yields reference roles `read, write, read, write, read` and the binding value becomes unknown; `count! = 2` yields a single `write`. - `src/domain/javascript/javascriptSymbolicBindings.test.ts`: a bootstrap module slot overwritten through a destructuring assignment, a `for...of` head, or a wrapped member chain makes the later `load(1)` unknown. - `src/application/javascript/JavaScriptSemanticGraphBuilder.test.ts`: `[source.count] = [2]` projects a single `write:/count` occurrence with a resolved `writes-property` relation to the `/count` slot and no `reads-property` relation. - Base reproduction or other evidence: executed. With the source files stashed and only the tests applied, the seven new dataflow cases fail (each reports `["read"]`); the direct-assignment control and the default-value control pass. The three TypeScript-wrapped read-modify-write cases fail against the first commit of this PR (write-only) and pass after the second. With the fix applied all cases pass. - User-visible CLI/MCP output (if applicable): not applicable. - Remaining proof gaps: none known for the listed forms. Member targets inside destructuring *declarations* (`const [x] = ...`) cannot contain member expressions and are unaffected. - [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:focused -- src/domain/javascript/javascriptSemanticAnalysis.dataflow.test.ts src/domain/javascript/javascriptSymbolicBindings.test.ts src/domain/javascript/javascriptSemanticAnalysis.member_mutations.test.ts src/application/javascript/JavaScriptSemanticGraphBuilder.test.ts` — 4 files, 336 tests passed (at 7051da087). - Same dataflow and symbolic-bindings test files with the four changed source files reverted to `aa3047ad5` (tests only) — the five review follow-up cases fail (two wrapped-identifier, three symbolic-bindings); the rest pass. - Same focused run with the three source files stashed (tests only) — the seven new dataflow cases fail with `["read"]`; both controls pass. Confirms the regression reproduces on the base. - `npm run check:fast` — typecheck and lint, 2 tasks successful (at 7051da087). - `npm run check:changed` — 89 test files, 2000 tests passed (at 7051da087). - `git diff --check` — clean. - Toolchain: Node 24.18.0 (`.nvmrc`), npm 11.16.0, macOS arm64. No real-provider lane applies. ## Compatibility, safety, and release - Breaking changes or migration steps: none. - Real Hopper/Ghidra, browser, or OS coverage: not applicable; pure static JavaScript analysis. - Package or release metadata impact: none. - Security, privacy, process, or containment review: no new I/O, process, or network behavior. ## Before merge - [x] This PR addresses a concrete problem or an agreed enhancement. - [x] Material scope and authority questions are resolved, and any required maintainer scope decision covers the final diff. - [x] The PR has one focused outcome and the title follows `type(scope): outcome`. - [x] Related issue is linked, or the reason for not linking one is stated above. - [x] Tests cover changed observable behavior and meaningful failure paths. - [x] Owning docs, contracts, and generated metadata are updated where needed. - [x] User-visible CLI/MCP changes include representative output. - [x] 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:06:51 +00:00

closed this issue
added the
pull-request
label

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#5465

No description provided.

Rows
Columns