[PR #537] [MERGED] fix(javascript): classify assignment targets and retain compound-assignment reads #2059

New issue

Closed

opened 2026-10-16 21:13:49 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-16 21:13:49 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/537
Author: @morluto
Created: 10/5/2026
Status: ✅ Merged
Merged: 10/5/2026
Merged by: @morluto

Base: main ← Head: cleanup/semantic-reference-roles

📝 Commits (1)

6d884b0 fix(javascript): classify assignment targets and retain compound-assignment reads

📊 Changes

4 files changed (+204 additions, -27 deletions)

View changed files

📝 src/domain/javascriptSemanticAnalysis.structure.test.ts (+42 -0)
📝 src/domain/javascriptSemanticObjects.ts (+2 -3)
📝 src/domain/javascriptSemanticProjection.ts (+137 -18)
📝 src/domain/javascriptSemanticTraversal.ts (+23 -6)

📄 Description

Follow-up to #508, which fixed read-modify-write for member operations but left identifier references behind.

1. Compound assignment and update lose the read. #508 made source.count += 1 emit both a read and a write in objectOperations. The identifier path in semanticReferenceRole still collapsed total += 1 and total++ to a single write, so the two families disagreed about the same program. Extracted one predicate, semanticReadsBeforeWrite, now shared by both — the rule lives in one place instead of two copies that can drift.

2. Assignment targets were classified as reads. ({a} = o), [b] = arr, ({p:{q}} = o) and for (a of list) all reported read for the target identifiers, which inverts the meaning of a dataflow consumer.

Root cause here is structural, not a missing branch: traverseJavaScriptAst passed only the immediate parent, but a binding inside a destructuring pattern has an ObjectProperty/ArrayPattern as its parent. The parent alone cannot distinguish const {a} = o (a declaration) from ({a} = o) (an assignment) — that needs ancestry.

So rather than adding yet another parent-shape special case, the traversal now passes ancestors as well. That is the deepening: one traversal, all positional meaning available at every call site, instead of each collector re-deriving context from whatever its parent happens to be.

Two correctness details worth calling out:

Only the bound side of a pattern counts. ({a = compute()} = o) binds a but merely reads compute; bindsIdentifier checks the bound slot so a default-value expression is not misreported as a write.
semanticReferenceRole stays exported and unchanged for callers that only have a parent, so existing call sites are untouched.

Tests: destructuring/loop targets classify as writes, catch ({message}) binds as a declaration with a later read, and the compound-assignment sequence is asserted exactly. Full suite green: 1923 passed, tsc and check:fast clean.

_{🔄 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/537 **Author:** [@morluto](https://github.com/morluto) **Created:** 10/5/2026 **Status:** ✅ Merged **Merged:** 10/5/2026 **Merged by:** [@morluto](https://github.com/morluto) **Base:** `main` ← **Head:** `cleanup/semantic-reference-roles` --- ### 📝 Commits (1) - [`6d884b0`](https://github.com/morluto/rea/commit/6d884b071dcbf0d28359be03274f88c3d0f8ded3) fix(javascript): classify assignment targets and retain compound-assignment reads ### 📊 Changes **4 files changed** (+204 additions, -27 deletions) <details> <summary>View changed files</summary> 📝 `src/domain/javascriptSemanticAnalysis.structure.test.ts` (+42 -0) 📝 `src/domain/javascriptSemanticObjects.ts` (+2 -3) 📝 `src/domain/javascriptSemanticProjection.ts` (+137 -18) 📝 `src/domain/javascriptSemanticTraversal.ts` (+23 -6) </details> ### 📄 Description Follow-up to #508, which fixed read-modify-write for *member* operations but left identifier references behind. **1. Compound assignment and update lose the read.** #508 made `source.count += 1` emit both a read and a write in `objectOperations`. The identifier path in `semanticReferenceRole` still collapsed `total += 1` and `total++` to a single `write`, so the two families disagreed about the same program. Extracted one predicate, `semanticReadsBeforeWrite`, now shared by both — the rule lives in one place instead of two copies that can drift. **2. Assignment targets were classified as reads.** `({a} = o)`, `[b] = arr`, `({p:{q}} = o)` and `for (a of list)` all reported `read` for the target identifiers, which inverts the meaning of a dataflow consumer. Root cause here is structural, not a missing branch: `traverseJavaScriptAst` passed only the immediate `parent`, but a binding inside a destructuring pattern has an `ObjectProperty`/`ArrayPattern` as its parent. The parent alone *cannot* distinguish `const {a} = o` (a declaration) from `({a} = o)` (an assignment) — that needs ancestry. So rather than adding yet another parent-shape special case, the traversal now passes `ancestors` as well. That is the deepening: one traversal, all positional meaning available at every call site, instead of each collector re-deriving context from whatever its parent happens to be. Two correctness details worth calling out: - Only the *bound* side of a pattern counts. `({a = compute()} = o)` binds `a` but merely reads `compute`; `bindsIdentifier` checks the bound slot so a default-value expression is not misreported as a write. - `semanticReferenceRole` stays exported and unchanged for callers that only have a parent, so existing call sites are untouched. Tests: destructuring/loop targets classify as writes, `catch ({message})` binds as a declaration with a later read, and the compound-assignment sequence is asserted exactly. Full suite green: 1923 passed, tsc and check:fast clean. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-16 21:13:49 +00:00