# Batch 2 — Analysis Layer — Findings

**Date:** 2026-03-26
**Files reviewed:** `analysis/executionContext.js`, `analysis/functionAnalysis.js`, `analysis/functionResolution.js`, `analysis/handleBindingAnalysis.js`, `analysis/macroAnalysis.js`, `analysis/documentFacts.js`
**Status:** Complete

---

## Summary

The analysis layer is well-structured overall — good separation of concerns, clear JSDoc,
consistent patterns for scanning documents. The dominant finding is a **duplicate and diverging
`parseFunctionDeclaration`** between `functionAnalysis.js` and `parser.js`, which is a latent
source of parser inconsistency. `executionContext.js` has a misleading while-loop condition in
continuation-line joining that works by accident. `handleBindingAnalysis.js` has a fragile use
of a stateful module-level regex. No critical issues; no architecture violations.

---

## Findings

### `executionContext.js`

#### [MINOR] `getJoinedContinuationLines` — while condition tests the wrong variable — line 288

**Issue:** The while loop reads:
```javascript
while (currentIndex + 1 < document.lineCount && isLineWithContinuation(rawLine))
```
`rawLine` is the *initial* line (declared at line 272), not the line at `currentIndex`. Since
the function is only entered when the initial line has `...`, `isLineWithContinuation(rawLine)`
is always `true` for the entire loop. The loop relies entirely on the `break` statement (line
302) and the EOF boundary check for termination.

The code produces correct output, but the condition is misleading. A future maintainer expecting
`isLineWithContinuation` to guard against runaway joins will be confused, and any refactor that
changes where `rawLine` is assigned would silently break the join logic.

**Recommendation:** Replace `isLineWithContinuation(rawLine)` in the while condition with
`true`, and add a comment: `// loop exits via break when non-continuation line found`. Or
check the current index line: `isLineWithContinuation(document.lineAt(currentIndex).text)`.

---

#### [MINOR] `isLineWithContinuation` — backslash escape detection may not apply to Architect — line 245

**Issue:** Line 245 tests `lineText[i - 1] !== "\\"` to detect escaped quotes, treating `\'`
and `\"` as non-toggling. If Architect strings do not use backslash as an escape character
(which is typical for non-C-family scripting languages), this check is wrong — a literal `\`
before a quote would incorrectly suppress the quote-state toggle.

**Recommendation:** Verify against the language specification whether `\"` and `\'` are valid
escape sequences inside Architect strings. If not, remove the backslash condition.

---

#### [OBSERVATION] `buildOutputLineMap` and `buildExecutionClassificationMap` share logic but are separate

**Issue:** Both functions implement hash-mode tracking, output-line detection, and comment-line
filtering. They are maintained independently. A change to one (e.g. a new output-line prefix)
must be applied to both. Already `buildOutputLineMap` uses `rawTrimmed` for the `??` check
while `buildExecutionClassificationMap` uses `trimmed` — a subtle drift.

**Recommendation:** Accept for now. If the two functions drift further, consider extracting
shared line-classification logic into a single pass that both consume.

---

### `functionAnalysis.js`

#### [MAJOR] Duplicate `parseFunctionDeclaration` — diverges from `parser.js` version

**Issue:** `functionAnalysis.js` defines its own `parseFunctionDeclaration` (line 15). It also
exists in `parser.js` (line 260, exported). The two regexes differ:

- `parser.js`: `FUNCTION\s+...\s*\((.*?)\)\s*:` — matches up to the `:`, no return-type capture
- `functionAnalysis.js`: `FUNCTION\s+...\s*\((.*?)\)\s*:\s*([A-Za-z0-9_]+)?` — captures return type

`functionAnalysis.js` does not import the version from `parser.js`. This means there are two
independent parsers for the same FUNCTION declaration syntax. If the grammar ever changes (e.g.
multiword return types, optional colon), only one version would be updated.

Additionally `parser.js` exports `parseFunctionDeclaration` which is less capable than the one
in `functionAnalysis.js` — it lacks the return-type capture that callers in `functionAnalysis`
depend on.

**Recommendation:** Make `functionAnalysis.js`'s `parseFunctionDeclaration` the canonical
implementation. Replace the one in `parser.js` with a re-export or remove it from `parser.js`'s
exports if no external caller needs it. Consolidate to one parser.

---

#### [MINOR] `params` Map uses case-sensitive keys — line 101

**Issue:** `new Map(funcDecl.params.map((p) => [p.name, p.type]))` stores parameter names
exactly as declared. If the Architect language treats variable names case-insensitively,
a lookup for `MyParam` versus `MYPARAM` would fail. This would cause false "undeclared variable"
diagnostics for parameters referenced with different casing.

**Recommendation:** Verify case-sensitivity rules for variable/parameter names from the
language spec. If case-insensitive, store keys as `.toUpperCase()` and normalise at lookup.

---

#### [OBSERVATION] `extractUserDefinedFunctions` is a thin wrapper that discards metadata

**Issue:** `extractUserDefinedFunctions` calls `buildUserDefinedFunctionCatalog` and returns
only `{ upperName: lineIndex }`, discarding params and returnType. Any caller that needs the
full metadata must call `buildUserDefinedFunctionCatalog` directly (double parse if not cached).
In practice the cache in `documentFacts.js` prevents the double-parse at runtime.

**Recommendation:** Accept as-is given the cache. Note for future: if the cache is ever
removed, this wrapper causes an unnecessary second pass.

---

### `functionResolution.js`

#### [OBSERVATION] `getFunctionSignature` called with original case — line 18 (flag for Batch 3)

**Issue:** `resolveKnownFunction` calls `getFunctionSignature(functionName)` with the name as
written at the call site (not normalised to uppercase). Whether this resolves correctly depends
on `getFunctionSignature`'s internal case handling. Verify in Batch 3 when `builtins/functions.js`
is reviewed.

---

### `handleBindingAnalysis.js`

#### [MINOR] Module-level stateful regex — line 16

**Issue:** `FUNC_CALL_RE` is a `/g` regex defined as a module-level constant. Regex with the
`g` flag carry `lastIndex` state between uses. The code resets `lastIndex = 0` at line 35
before each call, which is correct for the happy path. However, if an exception is thrown
inside the while loop, `lastIndex` would be left in a mid-string position, causing the next
call to `buildHandleBindingsAtLine` to start matching from the wrong offset in the first line.

**Recommendation:** Move the regex definition inside `buildHandleBindingsAtLine` (or use
`matchAll` with a fresh regex per call). The performance cost is negligible.

---

#### [MINOR] `INDEX_CLOSE` handle lookup is case-sensitive — line 61

**Issue:** `bindings.delete(handleArg.trim())` looks up the handle variable name exactly as
written. If the open call stored `sHandle` but the close call uses `SHANDLE` (different case),
the delete fails and the handle remains in the binding map as still-open. Whether this is a
real risk depends on whether Architect variable references are case-sensitive.

**Recommendation:** Normalise handle names (e.g. `.toLowerCase()`) at both insertion and
deletion. Matches the defensive approach used elsewhere (uppercase catalogs, etc.).

---

### `macroAnalysis.js`

#### [MINOR] `extractMacroInvocations` — argument count incorrect for nested calls — line 105

**Issue:** The args capture regex `\([^)]*\)` and the comma-split count (line 125) both fail
for nested function calls in macro arguments. `MY_MACRO(FOO(1,2), BAR)` would report 3 args
(splitting `FOO(1,2` and `BAR` after the regex stops at the first `)`), rather than 2. This
is documented in the code comment ("simplified — doesn't handle nested calls").

**Recommendation:** Accept the limitation for now. If macro argument count validation is
ever surfaced to users as a diagnostic, replace with the `extractArgAt` logic from `parser.js`
which correctly handles nesting.

---

#### [MINOR] `buildMacroCatalog` and `findDuplicateMacroDeclarations` each scan the document independently

**Issue:** Both functions call `parseMacroDeclaration` on every line of the document. In
`collectDocumentFacts`, both are called consecutively, scanning the same document twice. A
combined pass could produce both results in one scan.

**Recommendation:** Low priority given document sizes. If profiling shows `collectDocumentFacts`
is a bottleneck on large documents, merge the two scans.

---

### `documentFacts.js`

#### [MINOR] Cache hit/miss and eviction logic has no unit test coverage

**Issue:** `collectDocumentFacts` has memoization (cache hit returns early) and an eviction
strategy (`evictIfNeeded`). Neither is exercised by the current test suite (no
`documentFacts.test.js` exists). A regression in the cache keying (e.g. from the `uri.toString()`
path/URI mismatch noted in Batch 1) would not be caught by tests.

**Recommendation:** Add a unit test covering: (1) cache hit returns same reference, (2)
version change triggers recompute, (3) eviction removes the oldest entry when over capacity.

---

#### [OBSERVATION] `document.uri?.fsPath || document.fileName` — defensive but never needed — line 93

**Issue:** The optional chain and fallback guard against `uri` being undefined, but both
`vscode.TextDocument` and `PlainTextDocument` always have `uri`. The fallback `document.fileName`
is correct but dead code.

**Recommendation:** Accept as-is — defensive code here has zero runtime cost and protects
against unexpected document shapes from future callers.

---

## Positive observations

- `functionResolution.js` is exemplary: clean interface, single responsibility, well-documented
  return type in JSDoc.
- `buildFunctionScopes` correctly handles unclosed functions (extends scope to EOF).
- `extractOutputDelimiters` correctly short-circuits on the first `OPTION` match.
- `escapeRegexCharacter` in `executionContext.js` correctly escapes user-supplied delimiter
  characters before embedding in a regex — no ReDoS risk.
- The `seen` Set in `getInterpolationTargetsForLine` prevents duplicate targets cleanly.
- `documentFacts.js` caches by `(uri, version)` — correct invalidation strategy.

---

## Actions for backlog

| Severity | File | Issue | Recommended action |
|---|---|---|---|
| Major | `core/src/analysis/functionAnalysis.js:15` | Duplicate `parseFunctionDeclaration` diverges from `parser.js` | Consolidate to one canonical implementation in `functionAnalysis.js` |
| Minor | `core/src/analysis/executionContext.js:288` | `while` condition tests `rawLine` instead of current line | Replace with `true` + clarifying comment, or fix variable |
| Minor | `core/src/analysis/executionContext.js:245` | Backslash escape detection may not apply to Architect | Verify against language spec; remove if unneeded |
| Minor | `core/src/analysis/handleBindingAnalysis.js:16` | Module-level stateful `/g` regex | Move regex definition inside function |
| Minor | `core/src/analysis/handleBindingAnalysis.js:61` | `INDEX_CLOSE` case-sensitive handle lookup | Normalise to lowercase at insert and delete |
| Minor | `core/src/analysis/functionAnalysis.js:101` | `params` Map uses case-sensitive keys | Verify language spec; normalise to uppercase if case-insensitive |
| Minor | `core/src/analysis/macroAnalysis.js:105` | Arg count wrong for nested macro args | Accept; replace with `extractArgAt` if rule is ever user-facing |
| Minor | `core/src/analysis/macroAnalysis.js` | Two separate document scans for macro catalog + duplicates | Merge into single pass if profiling warrants |
| Minor | `core/src/analysis/documentFacts.js` | No unit tests for cache hit/miss/eviction | Add `documentFacts.test.js` covering cache behaviour |
