[PR #550] [MERGED] refactor(domain): consolidate duplicated semantic member and literal helpers #2065

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: cleanup/semantic-helper-consolidation

📝 Commits (1)

33df4cb refactor(domain): consolidate duplicated semantic member and literal helpers

📊 Changes

4 files changed (+20 additions, -66 deletions)

View changed files

📝 src/domain/javascriptSemanticAsyncEffects.ts (+6 -23)
📝 src/domain/javascriptSemanticChildProcesses.ts (+8 -25)
📝 src/domain/javascriptSemanticPromises.ts (+4 -10)
📝 src/domain/javascriptSemanticResources.ts (+2 -8)

📄 Description

Covers B-4, and reports the B-3 outcome honestly: B-3 turned out to be unnecessary, and I did not do it.

B-3 investigated, then dropped — the compiler already does the job

The plan said four error-tag lists must be kept in sync by hand and should be derived from one table. I tested that rather than assuming it. Adding a single tag to ANALYSIS_ERROR_TAGS produces four compile errors:

KNOWN_ERROR_TAGS is declared satisfies Readonly<Record<AnalysisErrorTag, true>>, so omitting a new tag fails the build
STATIC_ERROR_CODES is declared satisfies Readonly<Record<Exclude<AnalysisErrorTag, SpecializedErrorTag>, ...>>, same
the indexing call sites then fail too
STATIC_ERROR_CATEGORIES is deliberately Partial, falling back to execution_failure

So the drift risk the refactor was meant to remove is already removed by the type system. Deriving the lists from a single table would have replaced compile-time enforcement with a runtime table plus a resolver — strictly worse, and more code.

The second B-3 claim was that Ghidra failures are plain Errors collapsing into a generic ProviderAdapterError and should get their own tags. Also wrong: projectSessionError already maps Ghidra's kind exhaustively onto the shared taxonomy (AnalysisCancelledError, AnalysisTimeoutError, AnalysisInputError, AnalysisCapabilityUnavailableError, ProviderAdapterError). Keeping provider-neutral errors in the shared contract is the better shape, not a gap.

No B-3 change was made. Recording it here rather than shipping a refactor that removes safety.

B-4: helpers that had drifted into four copies

memberCallee — an identical six-line function in AsyncEffects, ChildProcesses, Resources and Promises, while dataEffectMemberCallee already existed for exactly this.
literalString — the same eleven-line function duplicated in ChildProcesses and AsyncEffects, alongside dataEffectLiteralString.

All six now call the shared implementations. 66 lines removed against 20 added, no behaviour change.

This matters beyond line count: these copies are exactly the shallow-module pattern from #544. A future fix to how a member callee or literal is read would have needed to land six times, and the copies already differ slightly in accepted parameter types — which is how such copies start disagreeing.

Tests: no new tests, deliberately. These are pure relocations of already-tested helpers, so the existing 1946-test suite is the coverage; adding tests that assert a helper still returns what it returned would be the "tests that freeze arbitrary implementation" the guidelines warn against.

_{🔄 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/550 **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-helper-consolidation` --- ### 📝 Commits (1) - [`33df4cb`](https://github.com/morluto/rea/commit/33df4cbd4a9857e1cd194cda74d3044b3b37269d) refactor(domain): consolidate duplicated semantic member and literal helpers ### 📊 Changes **4 files changed** (+20 additions, -66 deletions) <details> <summary>View changed files</summary> 📝 `src/domain/javascriptSemanticAsyncEffects.ts` (+6 -23) 📝 `src/domain/javascriptSemanticChildProcesses.ts` (+8 -25) 📝 `src/domain/javascriptSemanticPromises.ts` (+4 -10) 📝 `src/domain/javascriptSemanticResources.ts` (+2 -8) </details> ### 📄 Description Covers B-4, and reports the B-3 outcome honestly: **B-3 turned out to be unnecessary, and I did not do it.** ## B-3 investigated, then dropped — the compiler already does the job The plan said four error-tag lists must be kept in sync by hand and should be derived from one table. I tested that rather than assuming it. Adding a single tag to `ANALYSIS_ERROR_TAGS` produces four compile errors: - `KNOWN_ERROR_TAGS` is declared `satisfies Readonly<Record<AnalysisErrorTag, true>>`, so omitting a new tag fails the build - `STATIC_ERROR_CODES` is declared `satisfies Readonly<Record<Exclude<AnalysisErrorTag, SpecializedErrorTag>, ...>>`, same - the indexing call sites then fail too - `STATIC_ERROR_CATEGORIES` is deliberately `Partial`, falling back to `execution_failure` So the drift risk the refactor was meant to remove is already removed by the type system. Deriving the lists from a single table would have *replaced* compile-time enforcement with a runtime table plus a resolver — strictly worse, and more code. The second B-3 claim was that Ghidra failures are plain `Error`s collapsing into a generic `ProviderAdapterError` and should get their own tags. Also wrong: `projectSessionError` already maps Ghidra's `kind` exhaustively onto the shared taxonomy (`AnalysisCancelledError`, `AnalysisTimeoutError`, `AnalysisInputError`, `AnalysisCapabilityUnavailableError`, `ProviderAdapterError`). Keeping provider-neutral errors in the shared contract is the better shape, not a gap. **No B-3 change was made.** Recording it here rather than shipping a refactor that removes safety. ## B-4: helpers that had drifted into four copies - `memberCallee` — an identical six-line function in `AsyncEffects`, `ChildProcesses`, `Resources` and `Promises`, while `dataEffectMemberCallee` already existed for exactly this. - `literalString` — the same eleven-line function duplicated in `ChildProcesses` and `AsyncEffects`, alongside `dataEffectLiteralString`. All six now call the shared implementations. 66 lines removed against 20 added, no behaviour change. This matters beyond line count: these copies are exactly the shallow-module pattern from #544. A future fix to how a member callee or literal is read would have needed to land six times, and the copies already differ slightly in accepted parameter types — which is how such copies start disagreeing. Tests: no new tests, deliberately. These are pure relocations of already-tested helpers, so the existing 1946-test suite is the coverage; adding tests that assert a helper still returns what it returned would be the "tests that freeze arbitrary implementation" the guidelines warn against. --- <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:53 +00:00