fix(lsp): resolve alias locations and let strucpp own array resolution - #966
Merged
Merged
Conversation
…ucpp A variable's `location` holds EITHER a producer alias name OR a literal `%addr`. The LSP documents (POU signature stubs, graphical scope-query docs, the resource-globals CONFIGURATION) were serializing that field verbatim, emitting `AT label2` — not valid IEC ST. strucpp abandons the whole VAR block on it, so every symbol declared after the first alias-bound variable falls out of the POU's scope: LD/FBD autocomplete offers at most one candidate, unrelated variables read as undefined, and their boxes paint red. The codebase already holds the invariant that the compiler never sees aliases (`getCompileReadyProjectData`). The LSP is a third consumer that must not either — it was missed. Resolve at the LSP serialization boundary only: a literal passes through verbatim, a live alias becomes its address, an orphaned alias drops the `AT` clause. Resolution only rewrites text within a declaration line, so `bodyLineOffset` and the `pouvars://` diagnostics mirror are unaffected. The store keeps the alias-name form throughout — the variables table, its text view, and the saved file all still show `label2`, never `%IW0`. `generateIecVariablesToString` is deliberately unchanged: persistence and the round-trippable text view must stay verbatim or alias bindings would be destroyed on save. `projectActions.getAliasIndex()` exposes the index, memoized on producer-state identity — the LSP reconciles on every POU mutation, and rebuilding the address registry per keystroke would be far too costly. A subscription on that index re-publishes the documents when a producer-only change (`recalculateIecAddresses`) moves an address without touching any POU. Mirrors openplc-web byte-for-byte (shared surface). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
strucpp 0.6.1 stops publishing the internal `__INLINE_ARRAY_<T>` name on its LSP surface and lists each in-bounds array element as its own symbol typed as the element type. That is everything the graphical editors needed, and it removes the reason the editor was deriving array facts itself. `resolveScopeExpressionType` no longer strips subscripts and regex-parses an element type out of the detail string: `arr[3]` now matches an LSP symbol by label like any other expression. The old `arrayElementType()` looked for an `ARRAY [..] OF <type>` rendering that strucpp never actually emitted, which is why an array-element box always painted red. Deferring to the LSP also makes array bounds authoritative — `arr[99]` isn't a symbol, so it resolves `unknown` and the box is correctly flagged. The subscript-stripping heuristic accepted any index. With completion candidates now coming from the compiler, the local index-enumeration helpers (`expandArrayVariable` / `expandArrayVariables`) are a second source of truth for something strucpp owns, and have had no caller since the graphical boxes moved to LSP-backed completion. Removed with their tests. `resolveArrayVariableByName` and friends stay for now: they serve the synchronous render path in the ladder/FBD node utilities, which would need to become async to consume LSP types. Tracked separately. Requires strucpp v0.6.1 (binary-versions.json). Mirrors openplc-web byte-for-byte (shared surface). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
WalkthroughThe PR adds memoized alias-to-address resolution across project state and ST-LSP serialization, updates graphical scope typing to trust LSP completions, removes frontend array expansion helpers, extends coverage, and updates the recorded strucpp binary version. ChangesAlias resolution pipeline
Scope and array resolution
Binary version metadata
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant projectActions
participant attachProjectSync
participant STSerializer
projectActions->>attachProjectSync: provide alias index
attachProjectSync->>STSerializer: serialize POU or resource globals
STSerializer-->>attachProjectSync: emit literal IEC addresses
projectActions->>attachProjectSync: notify alias index change
attachProjectSync->>STSerializer: reserialize affected documents
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/frontend/store/slices/project/slice.ts`:
- Around line 298-346: Move aliasIndexCache out of module scope and into the
per-store closure used by createProjectSlice, ensuring getMemoizedAliasIndex
accesses that store-local cache. Preserve the existing identity checks and
rebuild behavior, while preventing alias-index reuse between separate project
stores.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2535369c-c197-4940-8017-cdde509a79a4
📒 Files selected for processing (14)
binary-versions.jsonsrc/frontend/services/graphical-scope.tssrc/frontend/services/st-lsp/__tests__/project-sync.test.tssrc/frontend/services/st-lsp/index.tssrc/frontend/services/st-lsp/project-sync.tssrc/frontend/store/__tests__/project-slice.test.tssrc/frontend/store/slices/project/slice.tssrc/frontend/store/slices/project/types.tssrc/frontend/utils/PLC/__tests__/array-variable-utils.test.tssrc/frontend/utils/PLC/__tests__/pou-signature-serializer.test.tssrc/frontend/utils/PLC/__tests__/resource-globals-serializer.test.tssrc/frontend/utils/PLC/array-variable-utils.tssrc/frontend/utils/PLC/pou-signature-serializer.tssrc/frontend/utils/PLC/resource-globals-serializer.ts
💤 Files with no reviewable changes (2)
- src/frontend/utils/PLC/array-variable-utils.ts
- src/frontend/utils/PLC/tests/array-variable-utils.test.ts
Comment on lines
+298
to
+346
| /** | ||
| * Single-entry memo for the `alias → address` index. | ||
| * | ||
| * The LSP asks for this index on every project reconcile — which fires on | ||
| * every POU mutation, i.e. every keystroke in an ST editor — while | ||
| * `buildIecRegistry` does a full migrate + reallocate over every producer. | ||
| * The index depends only on producer state (pins, VPP entries, remote | ||
| * devices, active board, alias memory), none of which a POU edit touches, so | ||
| * an identity-keyed cache turns the hot path into a handful of `===` checks. | ||
| * | ||
| * Zustand/Immer guarantee reference stability when a slice didn't change, so | ||
| * identity comparison is sound; when any input's identity moves, the next | ||
| * call rebuilds and replaces the entry. | ||
| */ | ||
| interface AliasIndexCache { | ||
| /** Raw `pinsByBoard[board]` — NOT defaulted to `[]`, which would mint a new | ||
| * array identity on every call and defeat the cache. */ | ||
| pins: ProjectSliceRoot['deviceDefinitions']['pinMapping']['pinsByBoard'][string] | undefined | ||
| vendorScreenData: ProjectSliceRoot['deviceDefinitions']['configuration']['vendorScreenData'] | ||
| remoteDevices: ProjectSliceRoot['project']['data']['remoteDevices'] | ||
| board: ProjectSliceRoot['deviceDefinitions']['configuration']['deviceBoard'] | ||
| aliasMemory: ProjectSliceRoot['iecAliasMemory'] | ||
| index: ReadonlyMap<string, string> | ||
| } | ||
| let aliasIndexCache: AliasIndexCache | null = null | ||
|
|
||
| function getMemoizedAliasIndex(live: ProjectSliceRoot): ReadonlyMap<string, string> { | ||
| const board = live.deviceDefinitions.configuration.deviceBoard | ||
| const pins = live.deviceDefinitions.pinMapping.pinsByBoard[board] | ||
| const vendorScreenData = live.deviceDefinitions.configuration.vendorScreenData | ||
| const remoteDevices = live.project.data.remoteDevices | ||
| const aliasMemory = live.iecAliasMemory | ||
|
|
||
| if ( | ||
| aliasIndexCache && | ||
| aliasIndexCache.pins === pins && | ||
| aliasIndexCache.vendorScreenData === vendorScreenData && | ||
| aliasIndexCache.remoteDevices === remoteDevices && | ||
| aliasIndexCache.board === board && | ||
| aliasIndexCache.aliasMemory === aliasMemory | ||
| ) { | ||
| return aliasIndexCache.index | ||
| } | ||
|
|
||
| const index = buildAliasIndex(buildIecRegistry(live)) | ||
| aliasIndexCache = { pins, vendorScreenData, remoteDevices, board, aliasMemory, index } | ||
| return index | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm device-slice defaults for the alias-cache keys are always freshly
# created per slice instantiation (not shared module-level constants), and
# check whether project-slice.test.ts creates a fresh store per test.
fd -e ts -i device -p src/frontend/store/slices
rg -n "pinsByBoard|vendorScreenData" src/frontend/store/slices/device -A3 -B3
rg -n "beforeEach" src/frontend/store/__tests__/project-slice.test.ts -A3 | head -40Repository: Autonomy-Logic/openplc-editor
Length of output: 15624
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== project slice alias cache area =="
sed -n '260,360p' src/frontend/store/slices/project/slice.ts
echo
echo "== module-level createProjectSlice / getState references around getMemoizedAliasIndex =="
rg -n "function makeStore|function createProjectSlice|const createProjectSlice|export .*ProjectSlice|getState|getMemoizedAliasIndex|aliasIndexCache|buildAliasIndex|buildIecRegistry" src/frontend/store/slices/project/slice.ts -A2 -B2
echo
echo "== tests makeStore definitions/usages =="
sed -n '330,370p' src/frontend/store/__tests__/project-slice.test.ts
rg -n "function makeStore|const makeStore|makeStore\\(" src/frontend/store/__tests__/project-slice.test.ts src/frontend/store -A2 -B2 | head -120
echo
echo "== project slice exports outline =="
ast-grep outline src/frontend/store/slices/project/slice.ts --match getMemoizedAliasIndex --view expanded || trueRepository: Autonomy-Logic/openplc-editor
Length of output: 26889
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== makeStore implementations using project slice and shared data in tests =="
cat -n src/frontend/store/__tests__/project-slice.test.ts | sed -n '1,140p'
cat -n src/frontend/store/__tests__/flow-writeback.test.ts | sed -n '1,90p'
cat -n src/frontend/store/__tests__/alias-location-on-board-load.test.ts | sed -n '1,80p'
echo
echo "== project slice slice factory and getAliasIndex / addIecAliases locations =="
cat -n src/frontend/store/slices/project/slice.ts | sed -n '1650,1705p'
sed -n '195,290p' src/frontend/store/slices/project/slice.ts
sed -n '504,514p' src/frontend/store/slices/project/slice.tsRepository: Autonomy-Logic/openplc-editor
Length of output: 20609
Move the alias index cache out of module scope.
aliasIndexCache is a shared mutable slot for every createProjectSlice(...) store created in the process. Separate stores can easily share remoteDevices or other producer-state references, causing one store’s alias→address map to be reused by another when resolving compile-ready data. Keep the cache inside the per-store closure or key it through the stable getState reference.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/frontend/store/slices/project/slice.ts` around lines 298 - 346, Move
aliasIndexCache out of module scope and into the per-store closure used by
createProjectSlice, ensuring getMemoizedAliasIndex accesses that store-local
cache. Preserve the existing identity checks and rebuild behavior, while
preventing alias-index reuse between separate project stores.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Editor half of the shared-surface fix. Mirrors openplc-web#629 byte-for-byte and requires strucpp v0.6.1 (STruCpp#200).
Two LSP defects that both broke autocomplete and painted graphical boxes red.
1. Alias-bound locations broke the whole VAR block
A variable's
locationholds either a producer alias name or a literal%addr. The LSP documents (POU stubs, graphical scope-query docs, resource-globals CONFIGURATION) serialized it verbatim, emittingAT label2— not valid IEC ST. strucpp abandons the entire VAR block on it:Every symbol after the first alias-bound variable fell out of scope — autocomplete offered at most one candidate, unrelated variables read as undefined, their boxes painted red.
The codebase already holds the invariant that the compiler never sees aliases (
getCompileReadyProjectData). The LSP is a third consumer that must not either; it was missed. Now resolved at the LSP serialization boundary only: literal passes through verbatim, live alias becomes its address, orphaned alias drops theATclause.generateIecVariablesToStringis deliberately unchanged — persistence and the round-trippable variables text view must stay verbatim or alias bindings would be destroyed on save. The store keeps the alias-name form throughout: table, text view and saved file all still showlabel2, never%IW0.projectActions.getAliasIndex()exposes the index memoized on producer-state identity (the LSP reconciles on every POU mutation — rebuilding the address registry per keystroke would be far too costly). A subscription on it re-publishes whenrecalculateIecAddressesmoves an address without touching any POU.Resolution only rewrites text within a declaration line, so
bodyLineOffsetand thepouvars://diagnostics mirror are unaffected.2. Arrays were erased by strucpp's completion surface
strucpp published the internal
__INLINE_ARRAY_BOOLas a variable's type, discarding the element type and bounds. SosomeArraynever matched a BOOL box, andsomeArray[0]resolved to nothing.Fixed in the compiler (STruCpp#200). Here, the client-side workarounds go away:
resolveScopeExpressionTypeno longer strips subscripts and regex-parses an element type out of the detail string —arr[3]matches an LSP symbol by label like any other expression. The oldarrayElementType()looked for anARRAY [..] OF <type>rendering strucpp never actually emitted.Deferring to the LSP also makes bounds authoritative:
arr[99]isn't a symbol, so it resolvesunknownand the box is flagged. The subscript heuristic accepted any index.expandArrayVariable/expandArrayVariables— a second source of truth for index enumeration, with no caller since the graphical boxes moved to LSP-backed completion — removed with their tests.resolveArrayVariableByNameand friends stay for now: they serve the synchronous render path in the ladder/FBD node utilities, which would need to become async to consume LSP types. Worth a follow-up.Verification
Behaviour was verified end-to-end in openplc-web (shared UI layer and shared LSP code — see #629 for the walkthrough). Here:
tscclean apart from the 4 pre-existing@xyflowOnNodeDragerrors,validate:archpasses, and the mirrored jest suites pass (array-variable-utils31,project-slice287,pou-signature-serializer+resource-globals-serializer+project-sync51).🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Chores
strucppbinary version from v0.6.0 to v0.6.1.