From b4cf0183344c19b553d54a8b891927372da41dfc Mon Sep 17 00:00:00 2001 From: Thiago Alves Date: Mon, 27 Jul 2026 16:13:22 -0400 Subject: [PATCH 1/2] fix(lsp): resolve alias-bound locations before serializing ST for strucpp MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- .../st-lsp/__tests__/project-sync.test.ts | 138 ++++++++++++++++++ src/frontend/services/st-lsp/index.ts | 14 +- src/frontend/services/st-lsp/project-sync.ts | 33 ++++- .../store/__tests__/project-slice.test.ts | 32 ++++ src/frontend/store/slices/project/slice.ts | 52 ++++++- src/frontend/store/slices/project/types.ts | 12 ++ .../pou-signature-serializer.test.ts | 124 ++++++++++++++++ .../resource-globals-serializer.test.ts | 24 +++ .../utils/PLC/pou-signature-serializer.ts | 60 +++++++- .../utils/PLC/resource-globals-serializer.ts | 20 ++- 10 files changed, 494 insertions(+), 15 deletions(-) diff --git a/src/frontend/services/st-lsp/__tests__/project-sync.test.ts b/src/frontend/services/st-lsp/__tests__/project-sync.test.ts index 0e9574164..23c490300 100644 --- a/src/frontend/services/st-lsp/__tests__/project-sync.test.ts +++ b/src/frontend/services/st-lsp/__tests__/project-sync.test.ts @@ -42,6 +42,93 @@ function makeStubService() { } } +/** A POU whose only variable is bound to a producer alias by NAME — the + * single-field location model the LSP projection has to resolve. */ +function makeAliasPou(name: string): PLCPou { + return { + name, + pouType: 'program', + interface: { + variables: [ + { + id: '1', + name: 'label2', + class: 'local', + type: { definition: 'base-type', value: 'INT' }, + documentation: '', + debug: false, + location: 'label2', + }, + ], + }, + body: { language: 'st', value: 'x := 1;' }, + documentation: '', + } as PLCPou +} + +/** Seed a Runtime v4 target plus one Modbus holding-register point carrying + * `alias`, so the store's alias index resolves it to `%IW0`. */ +function seedAliasProducer(alias: string) { + const { deviceActions, projectActions } = openPLCStoreBase.getState() + deviceActions.setAvailableOptions({ + availableBoards: new Map([ + [ + 'OpenPLC Runtime v4', + { + compiler: 'openplc-compiler', + core: 'rt-v4', + preview: '', + specs: {}, + capabilities: { + pinMapping: false, + vppIo: false, + modbusTcpRemote: true, + ethercat: true, + modbusTcpServer: true, + opcuaServer: true, + s7Server: true, + debuggerTransports: ['websocket'], + pythonFunctionBlocks: true, + arduinoApiCompletions: false, + hasRuntimeStats: true, + isInProcessSimulator: false, + directUsbUpload: false, + }, + }, + ], + ]) as never, + }) + deviceActions.setDeviceBoard('OpenPLC Runtime v4') + + const current = openPLCStoreBase.getState().project + projectActions.setProject({ + ...current, + data: { + ...current.data, + remoteDevices: [ + { + name: 'Dev1', + protocol: 'modbus-tcp', + modbusTcpConfig: { host: '127.0.0.1', port: 502, slaveId: 1, timeout: 1000, ioGroups: [] }, + }, + ], + }, + }) + projectActions.addIOGroup('Dev1', { + id: 'g1', + name: 'group-g1', + functionCode: '3', + cycleTime: 100, + offset: '0', + length: 1, + errorHandling: 'keep-last-value', + ioPoints: [], + }) + const pointId = + openPLCStoreBase.getState().project.data.remoteDevices![0].modbusTcpConfig!.ioGroups[0].ioPoints![0].id + projectActions.updateIOPointAlias('Dev1', 'g1', pointId, alias) +} + function setProjectPous(pous: PLCPou[]) { openPLCStoreBase.setState((s) => ({ ...s, @@ -218,6 +305,57 @@ describe('attachProjectSync', () => { expect(new Set(versions).size).toBe(versions.length) handle.dispose() }) + + // `AT ` is not valid IEC ST — strucpp abandons the whole VAR block on + // it, so every symbol after the first alias-bound variable falls out of the + // POU's scope (no autocomplete, red boxes in the LD/FBD editors). + it('never publishes a bare alias name as a location', () => { + setProjectPous([makeAliasPou('Main')]) + const service = makeStubService() + const handle = attachProjectSync(service) + + const text = service.openDocument.mock.calls.find((c) => c[0] === 'inmemory://pou/Main.st')?.[1] as string + expect(text).not.toContain('AT label2') + expect(text).toContain('label2 : INT;') + // The store still holds the alias-name form — only the LSP projection is + // resolved. + expect(openPLCStoreBase.getState().project.data.pous[0].interface?.variables?.[0].location).toBe('label2') + handle.dispose() + }) + + it('publishes the address an alias currently resolves to', () => { + seedAliasProducer('label2') + setProjectPous([makeAliasPou('Main')]) + const service = makeStubService() + const handle = attachProjectSync(service) + + const text = service.openDocument.mock.calls.find((c) => c[0] === 'inmemory://pou/Main.st')?.[1] as string + expect(text).toContain('label2 : INT AT %IW0;') + handle.dispose() + }) + + it('re-publishes when producer state changes without a POU edit', () => { + seedAliasProducer('label2') + setProjectPous([makeAliasPou('Main')]) + const service = makeStubService() + const handle = attachProjectSync(service) + service.changeDocument.mockClear() + + // Dropping the producer touches only `remoteDevices` — the POU array is + // untouched, so only the alias-index subscription can catch it. Without + // it the stub would keep advertising the now-dead `%IW0`. + openPLCStoreBase.setState((s) => ({ + ...s, + project: { ...s.project, data: { ...s.project.data, remoteDevices: [] } }, + })) + + expect(service.changeDocument).toHaveBeenCalledTimes(1) + const [uri, text] = service.changeDocument.mock.calls[0] as [string, string] + expect(uri).toBe('inmemory://pou/Main.st') + expect(text).toContain('label2 : INT;') + expect(text).not.toContain('%IW0') + handle.dispose() + }) }) function makeSystemLibrary(name: string, version: string = '1.0.0'): SystemLibrary { diff --git a/src/frontend/services/st-lsp/index.ts b/src/frontend/services/st-lsp/index.ts index 8c7c3fde1..dd74eb89f 100644 --- a/src/frontend/services/st-lsp/index.ts +++ b/src/frontend/services/st-lsp/index.ts @@ -297,15 +297,20 @@ export function startStLsp(opts: StLspStartOptions): StLspService { await scopeWarmReady const connection = serviceConnection if (!connection) return [] - const pou = openPLCStoreBase.getState().project.data.pous.find((p) => p.name === pouName) + const { project, projectActions } = openPLCStoreBase.getState() + const pou = project.data.pous.find((p) => p.name === pouName) if (!pou) return [] + // Alias-bound locations must be resolved to literal `%…` addresses or the + // query doc's VAR block fails to parse and strucpp returns no candidates + // for the whole POU. See `serializePouScopeForQuery`. + const aliasIndex = projectActions.getAliasIndex() // Once warm, fresh per-query docs resolve instantly; a single short retry // covers a rare transient miss. Each attempt uses a unique URI + unique // synthetic POU name so docs never collide. for (let attempt = 0; attempt < SCOPE_QUERY_MAX_ATTEMPTS; attempt += 1) { const id = (scopeQuerySeq += 1) - const { text, position } = serializePouScopeForQuery(pou, prefix, id) + const { text, position } = serializePouScopeForQuery(pou, prefix, id, aliasIndex) const uri = `inmemory://scopequery/${id}.st` const items = await requestOnce(connection, uri, text, position) if (items.length > 0) return items @@ -345,10 +350,11 @@ export function startStLsp(opts: StLspStartOptions): StLspService { } await new Promise((r) => setTimeout(r, SCOPE_WARMUP_INITIAL_DELAY_MS)) for (let poll = 0; poll < SCOPE_WARMUP_MAX_POLLS; poll += 1) { - const pou = openPLCStoreBase.getState().project.data.pous[0] + const { project, projectActions } = openPLCStoreBase.getState() + const pou = project.data.pous[0] if (pou) { const id = (scopeQuerySeq += 1) - const { text, position } = serializePouScopeForQuery(pou, '', id) + const { text, position } = serializePouScopeForQuery(pou, '', id, projectActions.getAliasIndex()) const uri = `inmemory://scopequery/warmup-${id}.st` const items = await requestOnce(connection, uri, text, position) if (items.length > 0) { diff --git a/src/frontend/services/st-lsp/project-sync.ts b/src/frontend/services/st-lsp/project-sync.ts index 51ee261e7..29cf9b65b 100644 --- a/src/frontend/services/st-lsp/project-sync.ts +++ b/src/frontend/services/st-lsp/project-sync.ts @@ -87,6 +87,15 @@ export function attachProjectSync(service: StLspService): ProjectSyncHandle { const snapshot = emptySnapshot() let disposed = false + // A variable's `location` holds EITHER a producer alias name OR a literal + // `%addr`. `AT ` is not valid IEC ST — strucpp abandons the whole VAR + // block on it, so every symbol after the first alias-bound variable vanishes + // from the POU's scope. The LSP therefore sees the same resolved addresses + // the compiler does (`getCompileReadyProjectData`); only the projection is + // resolved, the store keeps the alias names for display. The store memoizes + // this on producer-state identity, so calling it per reconcile is cheap. + const aliasIndex = (): ReadonlyMap => openPLCStoreBase.getState().projectActions.getAliasIndex() + // Reconcile a single fixed-URI synthesized document (data types, resource // globals, softmotion axes …) against the worker: open on first non-empty // text, didChange on a text change, didClose when it becomes empty. Centralised @@ -119,7 +128,7 @@ export function attachProjectSync(service: StLspService): ProjectSyncHandle { // The project's configuration-level `VAR_GLOBAL`s wrapped in a CONFIGURATION, // so a POU's `VAR_EXTERNAL` resolves against a matching global. function reconcileResourceGlobals(globals: PLCVariable[]): void { - reconcileSyntheticDoc(RESOURCE_GLOBALS_URI, serializeResourceGlobalsToST(globals)) + reconcileSyntheticDoc(RESOURCE_GLOBALS_URI, serializeResourceGlobalsToST(globals, aliasIndex())) } // A `VAR_GLOBAL : AXIS_REF_SM3` per recognized CiA 402 drive, so editor @@ -132,6 +141,8 @@ export function attachProjectSync(service: StLspService): ProjectSyncHandle { function reconcile(pous: PLCPou[]): void { if (disposed) return + // Read once per reconcile — every POU stub resolves against the same index. + const resolvedAliases = aliasIndex() const seenNames = new Set() const seenUris = new Set() // The synthesized documents (data types, resource globals, SoftMotion axes) @@ -149,7 +160,7 @@ export function attachProjectSync(service: StLspService): ProjectSyncHandle { // Axes are surfaced as ambient globals (SOFTMOTION_GLOBALS_URI), so the // editor documents keep the exact line layout the user wrote and // go-to-definition line mapping stays correct. - const { text: nextText, bodyLineOffset } = serializePouSignatureToSTWithBodyOffset(pou) + const { text: nextText, bodyLineOffset } = serializePouSignatureToSTWithBodyOffset(pou, resolvedAliases) // POU name unchanged but URI switched (body language change). // Send didClose for the previous URI before didOpen on the new. @@ -222,6 +233,23 @@ export function attachProjectSync(service: StLspService): ProjectSyncHandle { (state) => state.project.data.remoteDevices, (remoteDevices) => reconcileSoftMotionGlobals(remoteDevices), ) + // Every document that declares variables is serialized against the alias → + // address index, so a producer-only change must re-emit them. Renaming an + // alias already cascades into `project.data.pous` (via `renameAlias`) and + // reconciles through the subscription above, but a pure *re-address* + // (`recalculateIecAddresses` compacting after an IO point is removed) + // touches only the producers — the stubs would otherwise keep the old + // `%addr`. Selecting the index itself is the exact trigger: the store + // memoizes it on producer-state identity, so this selector is a handful of + // `===` checks and the listener fires only when the index really moved. + const unsubscribeAliasIndex = openPLCStoreBase.subscribe( + (state) => state.projectActions.getAliasIndex(), + () => { + const live = openPLCStoreBase.getState() + reconcileResourceGlobals(live.project.data.configurations.resource.globalVariables) + reconcile(live.project.data.pous) + }, + ) // Initial reconcile against whatever is already in the store. The // synthesized globals/types load first so any POU that references @@ -257,6 +285,7 @@ export function attachProjectSync(service: StLspService): ProjectSyncHandle { unsubscribeDataTypes() unsubscribeResourceGlobals() unsubscribeRemoteDevices() + unsubscribeAliasIndex() // Close every doc we'd opened so the worker stays consistent // if the service is restarted in the same session. for (const uri of snapshot.contentByUri.keys()) { diff --git a/src/frontend/store/__tests__/project-slice.test.ts b/src/frontend/store/__tests__/project-slice.test.ts index be3d99f72..c965e69bf 100644 --- a/src/frontend/store/__tests__/project-slice.test.ts +++ b/src/frontend/store/__tests__/project-slice.test.ts @@ -2900,6 +2900,38 @@ describe('createProjectSlice', () => { }) }) + describe('getAliasIndex', () => { + beforeEach(() => { + seedRuntimeV4Board(store) + }) + + it('exposes the live alias → address map', () => { + seedRemoteDevice(store, makeRemoteDevice('Dev1')) + store.getState().projectActions.addIOGroup('Dev1', makeIOGroup('g1', '3', 2)) // %IW0, %IW1 + const pointId = store.getState().project.data.remoteDevices![0].modbusTcpConfig!.ioGroups[0].ioPoints![0].id + store.getState().projectActions.updateIOPointAlias('Dev1', 'g1', pointId, 'flow') + + expect(store.getState().projectActions.getAliasIndex().get('flow')).toBe('%IW0') + }) + + it('returns the same reference until producer state changes', () => { + seedRemoteDevice(store, makeRemoteDevice('Dev1')) + store.getState().projectActions.addIOGroup('Dev1', makeIOGroup('g1', '3', 2)) + const first = store.getState().projectActions.getAliasIndex() + // A POU edit must NOT invalidate the memo — the LSP calls this on every + // reconcile, i.e. on every keystroke in an ST editor. + seedPou(store, makePou('Prog', 'program', [locVar('x', '')])) + expect(store.getState().projectActions.getAliasIndex()).toBe(first) + + // A producer change must. + const pointId = store.getState().project.data.remoteDevices![0].modbusTcpConfig!.ioGroups[0].ioPoints![0].id + store.getState().projectActions.updateIOPointAlias('Dev1', 'g1', pointId, 'flow') + const second = store.getState().projectActions.getAliasIndex() + expect(second).not.toBe(first) + expect(second.get('flow')).toBe('%IW0') + }) + }) + // ------------------------------------------------------------------------- // Defensive guard coverage — operations on servers missing expected configs // ------------------------------------------------------------------------- diff --git a/src/frontend/store/slices/project/slice.ts b/src/frontend/store/slices/project/slice.ts index 87c226667..d81e93b31 100644 --- a/src/frontend/store/slices/project/slice.ts +++ b/src/frontend/store/slices/project/slice.ts @@ -295,6 +295,55 @@ function buildIecRegistry(live: ProjectSliceRoot): IecAddressRegistry { return recalculateRegistry(unpinAllocatableChannels(restored, ALLOCATED_KINDS), { activeKinds }).registry } +/** + * 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 +} +let aliasIndexCache: AliasIndexCache | null = null + +function getMemoizedAliasIndex(live: ProjectSliceRoot): ReadonlyMap { + 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 +} + /** Flatten the registry into a `channelKey -> { address, alias }` index for * writing results back onto each producer. */ function indexRegistry(registry: IecAddressRegistry): Map { @@ -1616,7 +1665,7 @@ const createProjectSlice: StateCreator = // The store keeps the alias-name form for display; only this snapshot is // resolved. const live = getState() - const aliasIndex = buildAliasIndex(buildIecRegistry(live)) + const aliasIndex = getMemoizedAliasIndex(live) const data = structuredClone(live.project.data) const resolveAll = (variables: PLCVariable[] | undefined): void => { if (!variables) return @@ -1626,6 +1675,7 @@ const createProjectSlice: StateCreator = resolveAll(data.configurations?.resource?.globalVariables) return data }, + getAliasIndex: () => getMemoizedAliasIndex(getState()), addIOGroup: (deviceName, group) => { // Read producer state from the live store before entering produce // so the pool reflects every active source (pin-mapping, VPP, diff --git a/src/frontend/store/slices/project/types.ts b/src/frontend/store/slices/project/types.ts index e9518b207..6c652d245 100644 --- a/src/frontend/store/slices/project/types.ts +++ b/src/frontend/store/slices/project/types.ts @@ -182,6 +182,18 @@ export type ProjectActions = { */ getCompileReadyProjectData: () => ProjectState['data'] + /** + * The live `alias → IEC address` index derived from every active producer + * (pin mapping, VPP module slots, Modbus TCP remote IO, EtherCAT channels). + * + * Exposed for consumers that must project variables into IEC text the way + * the compiler sees them — currently the ST language server, whose stub and + * scope-query documents would otherwise emit `AT ` and fail to parse. + * Memoized on producer-state identity, so the LSP can call it on every + * project reconcile without rebuilding the address registry. + */ + getAliasIndex: () => ReadonlyMap + /** * Cascade-rename bound variables' `location` from `oldAlias` to `newAlias` * across all POU-local and global variables. In the single-field model a diff --git a/src/frontend/utils/PLC/__tests__/pou-signature-serializer.test.ts b/src/frontend/utils/PLC/__tests__/pou-signature-serializer.test.ts index da19b1745..0e5787451 100644 --- a/src/frontend/utils/PLC/__tests__/pou-signature-serializer.test.ts +++ b/src/frontend/utils/PLC/__tests__/pou-signature-serializer.test.ts @@ -4,6 +4,7 @@ import { SCOPE_QUERY_POU_NAME, serializePouScopeForQuery, serializePouSignatureToST, + serializePouSignatureToSTWithBodyOffset, } from '../pou-signature-serializer' function makePou(overrides: Partial = {}): PLCPou { @@ -258,4 +259,127 @@ describe('serializePouSignatureToST', () => { expect(endIdx).toBeGreaterThan(bodyIdx) }) }) + + // A variable's `location` holds EITHER a producer alias name OR a literal + // `%addr`. `AT ` is not valid IEC ST — strucpp abandons the whole VAR + // block on it, taking every later symbol in the POU out of scope (the bug: + // no autocomplete + red boxes in the LD/FBD editors). The LSP projection + // must therefore resolve aliases exactly like the compiler does. + describe('alias-bound locations', () => { + const ALIAS_INDEX: ReadonlyMap = new Map([ + ['label2', '%IW0'], + ['label3', '%IW1'], + ]) + + const aliasPou = (): PLCPou => + makePou({ + name: 'main', + pouType: 'program', + body: { language: 'ld', value: {} as never }, + interface: { + variables: [ + { + id: '1', + name: 'label2', + class: 'local', + type: { definition: 'base-type', value: 'INT' }, + documentation: '', + debug: false, + location: 'label2', + }, + { + id: '2', + name: 'orphan', + class: 'local', + type: { definition: 'base-type', value: 'INT' }, + documentation: '', + debug: false, + location: 'gone_alias', + }, + { + id: '3', + name: 'manual', + class: 'local', + type: { definition: 'base-type', value: 'INT' }, + documentation: '', + debug: false, + location: '%QW7', + }, + { + id: '4', + name: 'ligado', + class: 'local', + type: { definition: 'base-type', value: 'BOOL' }, + documentation: '', + debug: false, + location: '', + }, + ], + }, + }) + + it('resolves an alias to its address, keeps a literal verbatim, drops an orphan', () => { + const text = serializePouSignatureToST(aliasPou(), ALIAS_INDEX) + expect(text).toContain('label2 : INT AT %IW0;') + expect(text).toContain('manual : INT AT %QW7;') + expect(text).toContain('orphan : INT;') + expect(text).toContain('ligado : BOOL;') + expect(text).not.toContain('AT label2') + expect(text).not.toContain('AT gone_alias') + }) + + it('emits no alias identifier when no index is supplied', () => { + // The empty-index default must still produce parseable ST rather than + // leaking `AT ` — this is the safety net for boot / test callers. + const text = serializePouSignatureToST(aliasPou()) + expect(text).toContain('label2 : INT;') + expect(text).toContain('manual : INT AT %QW7;') + expect(text).not.toContain('AT label2') + }) + + it('keeps the body line offset stable — resolution never adds or drops lines', () => { + const withIndex = serializePouSignatureToSTWithBodyOffset(aliasPou(), ALIAS_INDEX) + const withoutIndex = serializePouSignatureToSTWithBodyOffset(aliasPou()) + expect(withIndex.bodyLineOffset).toBe(withoutIndex.bodyLineOffset) + expect(withIndex.text.split('\n')).toHaveLength(withoutIndex.text.split('\n').length) + }) + + it('does not mutate the source variables', () => { + const pou = aliasPou() + serializePouSignatureToST(pou, ALIAS_INDEX) + serializePouScopeForQuery(pou, 'lab', 1, ALIAS_INDEX) + expect(pou.interface?.variables.map((v) => v.location)).toEqual(['label2', 'gone_alias', '%QW7', '']) + }) + + it('resolves aliases in the scope-query document too', () => { + const { text } = serializePouScopeForQuery(aliasPou(), 'lab', 7, ALIAS_INDEX) + expect(text).toContain('label2 : INT AT %IW0;') + expect(text).not.toContain('AT label2') + expect(text).not.toContain('AT gone_alias') + }) + + it('resolves an alias on an external variable while still rewriting it to a plain VAR', () => { + const pou = makePou({ + name: 'main', + pouType: 'program', + body: { language: 'ld', value: {} as never }, + interface: { + variables: [ + { + id: 'g1', + name: 'shared', + class: 'external', + type: { definition: 'base-type', value: 'INT' }, + documentation: '', + debug: false, + location: 'label3', + }, + ], + }, + }) + const { text } = serializePouScopeForQuery(pou, 'sh', 2, ALIAS_INDEX) + expect(text).toContain('shared : INT AT %IW1;') + expect(text).not.toContain('VAR_EXTERNAL') + }) + }) }) diff --git a/src/frontend/utils/PLC/__tests__/resource-globals-serializer.test.ts b/src/frontend/utils/PLC/__tests__/resource-globals-serializer.test.ts index d7bc53a2c..3b600f4d4 100644 --- a/src/frontend/utils/PLC/__tests__/resource-globals-serializer.test.ts +++ b/src/frontend/utils/PLC/__tests__/resource-globals-serializer.test.ts @@ -35,4 +35,28 @@ describe('serializeResourceGlobalsToST', () => { const st = serializeResourceGlobalsToST([global('counter', 'INT', { location: '%MW0', initialValue: '5' })]) expect(st).toContain('counter : INT AT %MW0 := 5;') }) + + // A global bound to a producer alias stores the alias NAME in `location`. + // `AT ` is not valid IEC ST and would take the whole VAR_GLOBAL block + // — and every VAR_EXTERNAL that resolves against it — down with it. + it('resolves an alias-bound location to its IEC address', () => { + const st = serializeResourceGlobalsToST( + [global('pressure', 'INT', { location: 'tank_alias' })], + new Map([['tank_alias', '%IW4']]), + ) + expect(st).toContain('pressure : INT AT %IW4;') + expect(st).not.toContain('AT tank_alias') + }) + + it('drops an alias that no longer resolves instead of emitting invalid ST', () => { + const st = serializeResourceGlobalsToST([global('pressure', 'INT', { location: 'gone' })], new Map()) + expect(st).toContain('pressure : INT;') + expect(st).not.toContain('AT gone') + }) + + it('never emits a bare alias identifier when no index is supplied', () => { + const st = serializeResourceGlobalsToST([global('pressure', 'INT', { location: 'tank_alias' })]) + expect(st).toContain('pressure : INT;') + expect(st).not.toContain('AT tank_alias') + }) }) diff --git a/src/frontend/utils/PLC/pou-signature-serializer.ts b/src/frontend/utils/PLC/pou-signature-serializer.ts index f361ae99b..0661b6ba4 100644 --- a/src/frontend/utils/PLC/pou-signature-serializer.ts +++ b/src/frontend/utils/PLC/pou-signature-serializer.ts @@ -34,12 +34,44 @@ * the user can't act on. */ -import type { PLCPou } from '../../../middleware/shared/ports/types' +import type { PLCPou, PLCVariable } from '../../../middleware/shared/ports/types' +import { resolveLocation } from '../../../middleware/shared/utils/iec-address/registry' import { generateIecVariablesToString } from '../generate-iec-variables-to-string' import { getEndKeyword, getStartKeyword } from './pou-file-extensions' const OPAQUE_BODY_PLACEHOLDER = '; (* graphical body — opaque to LSP *)' +/** Default for callers that have no alias index (tests, boot before the + * registry exists). `resolveLocation` maps every non-literal location to + * '' against it, so the emitted ST is always parseable. */ +const EMPTY_ALIAS_INDEX: ReadonlyMap = new Map() + +/** + * Resolve every variable's `location` from the stored single-field form + * (alias name OR literal `%addr`) to the literal address strucpp can parse. + * + * The LSP is a consumer that must never see aliases, exactly like the + * compiler — `AT label2` is not valid IEC ST, and strucpp abandons the whole + * VAR block on it, so every symbol after the first alias-bound variable + * disappears from the POU's scope (no autocomplete, red graphical boxes). + * The mapping mirrors `getCompileReadyProjectData()`: a literal passes + * through verbatim, a live alias becomes its address, an orphaned alias + * becomes '' (the `AT` clause is dropped). + * + * Only the LSP projection is resolved. The store keeps the alias-name form + * so the variables table / text view and the saved file all still show + * `label2`, not `%IW0`. + * + * Line-count invariant: this only ever rewrites text *within* a declaration + * line, so `bodyLineOffset` and the `pouvars://` diagnostics mirror stay + * correct. + */ +function withResolvedLocations(variables: PLCVariable[], aliasIndex: ReadonlyMap): PLCVariable[] { + return variables.map((variable) => + variable.location ? { ...variable, location: resolveLocation(variable.location, aliasIndex) } : variable, + ) +} + function buildDeclarationLine(pou: PLCPou): string { const startKeyword = getStartKeyword(pou.pouType) if (pou.pouType === 'function' && pou.interface?.returnType) { @@ -62,8 +94,11 @@ function buildDeclarationLine(pou: PLCPou): string { * no textual ST representation at all. Each of those POU * editors keeps its own native autocomplete / tooling. */ -export function serializePouSignatureToST(pou: PLCPou): string { - return serializePouSignatureToSTWithBodyOffset(pou).text +export function serializePouSignatureToST( + pou: PLCPou, + aliasIndex: ReadonlyMap = EMPTY_ALIAS_INDEX, +): string { + return serializePouSignatureToSTWithBodyOffset(pou, aliasIndex).text } /** @@ -80,13 +115,20 @@ export function serializePouSignatureToST(pou: PLCPou): string { * * Computed as the line count of `${declaration}\n${variables}\n` — * the literal prefix the template prepends before `${body}`. + * + * `aliasIndex` maps a producer alias to its current IEC address; every + * variable's `location` is resolved through it so the stub carries literal + * `%…` addresses only. See {@link withResolvedLocations}. */ -export function serializePouSignatureToSTWithBodyOffset(pou: PLCPou): { +export function serializePouSignatureToSTWithBodyOffset( + pou: PLCPou, + aliasIndex: ReadonlyMap = EMPTY_ALIAS_INDEX, +): { text: string bodyLineOffset: number } { const declaration = buildDeclarationLine(pou) - const variables = generateIecVariablesToString(pou.interface?.variables ?? []) + const variables = generateIecVariablesToString(withResolvedLocations(pou.interface?.variables ?? [], aliasIndex)) const body = pou.body.language === 'st' ? (pou.body.value as string) : OPAQUE_BODY_PLACEHOLDER const endKeyword = getEndKeyword(pou.pouType) const prefix = `${declaration}\n${variables}\n` @@ -117,6 +159,11 @@ export function serializePouSignatureToSTWithBodyOffset(pou: PLCPou): { * * `bodyExpr` MUST be a single line (no newlines) — the position math * assumes the expression occupies one body line. + * + * `aliasIndex` resolves alias-bound locations to literal `%…` addresses, as + * in {@link serializePouSignatureToSTWithBodyOffset} — without it a single + * alias-bound variable breaks the VAR block and the query returns no + * candidates at all. */ const SCOPE_QUERY_POU_NAME = '__openplc_scope_query__' @@ -124,6 +171,7 @@ export function serializePouScopeForQuery( pou: PLCPou, bodyExpr: string, uniqueId?: number | string, + aliasIndex: ReadonlyMap = EMPTY_ALIAS_INDEX, ): { text: string; position: { line: number; character: number } } { // Emit with the POU's REAL kind + return type so the VAR sections stay // legal (e.g. VAR_IN_OUT is only valid in FUNCTION_BLOCK/FUNCTION — a @@ -146,7 +194,7 @@ export function serializePouScopeForQuery( // yellow, no autocomplete). Re-emit externals as plain `VAR`: they carry their // real type inline, so the symbol and its members resolve self-containedly. // Scope-query-only — the POU's real stub keeps `VAR_EXTERNAL`. - const scopeVariables = (pou.interface?.variables ?? []).map((variable) => + const scopeVariables = withResolvedLocations(pou.interface?.variables ?? [], aliasIndex).map((variable) => variable.class === 'external' ? { ...variable, class: 'local' as const } : variable, ) const variables = generateIecVariablesToString(scopeVariables) diff --git a/src/frontend/utils/PLC/resource-globals-serializer.ts b/src/frontend/utils/PLC/resource-globals-serializer.ts index f6b6c39eb..51a202238 100644 --- a/src/frontend/utils/PLC/resource-globals-serializer.ts +++ b/src/frontend/utils/PLC/resource-globals-serializer.ts @@ -1,6 +1,7 @@ // SPDX-License-Identifier: GPL-3.0-or-later // Copyright (C) 2026 Autonomy / OpenPLC Project import type { PLCVariable } from '../../../middleware/shared/ports/types' +import { resolveLocation } from '../../../middleware/shared/utils/iec-address/registry' import { generateIecVariablesToString } from '../generate-iec-variables-to-string' const GLOBALS_CONFIG_NAME = '__globals_cfg__' @@ -22,12 +23,27 @@ const GLOBALS_RESOURCE_NAME = '__globals_res__' * The `VAR_GLOBAL` block is produced by `generateIecVariablesToString` — the * same formatter the variables editor uses — so declarations stay in lockstep * with how variables render everywhere else. Returns '' when there are none. + * + * `aliasIndex` resolves alias-bound locations to literal `%…` addresses. A + * global bound to an IO alias would otherwise serialize as `AT my_alias`, + * which strucpp rejects — taking the whole VAR_GLOBAL block (and thus every + * `VAR_EXTERNAL` resolution) down with it. Defaults to an empty index, which + * drops unresolvable locations rather than emitting invalid ST. */ -export function serializeResourceGlobalsToST(globals: PLCVariable[]): string { +export function serializeResourceGlobalsToST( + globals: PLCVariable[], + aliasIndex: ReadonlyMap = new Map(), +): string { if (!globals || globals.length === 0) return '' // Force the global class so the shared formatter always emits a single // VAR_GLOBAL block, regardless of any stray class on the stored variable. - const varBlock = generateIecVariablesToString(globals.map((g) => ({ ...g, class: 'global' }))) + const varBlock = generateIecVariablesToString( + globals.map((g) => ({ + ...g, + class: 'global', + location: g.location ? resolveLocation(g.location, aliasIndex) : '', + })), + ) return [ `CONFIGURATION ${GLOBALS_CONFIG_NAME}`, varBlock, From cfa647958e1ab4c8256998224bfa26f583ae38c7 Mon Sep 17 00:00:00 2001 From: Thiago Alves Date: Mon, 27 Jul 2026 23:26:09 -0400 Subject: [PATCH 2/2] fix(graphical): let the LSP resolve array elements MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit strucpp 0.6.1 stops publishing the internal `__INLINE_ARRAY_` 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 ` 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) --- binary-versions.json | 2 +- src/frontend/services/graphical-scope.ts | 30 ++---- .../__tests__/array-variable-utils.test.ts | 92 ------------------- .../utils/PLC/array-variable-utils.ts | 58 ------------ 4 files changed, 10 insertions(+), 172 deletions(-) diff --git a/binary-versions.json b/binary-versions.json index 5539337ec..53d60cdb5 100644 --- a/binary-versions.json +++ b/binary-versions.json @@ -1,6 +1,6 @@ { "strucpp": { - "version": "v0.6.0", + "version": "v0.6.1", "repository": "Autonomy-Logic/STruCpp" } } diff --git a/src/frontend/services/graphical-scope.ts b/src/frontend/services/graphical-scope.ts index da7c79f68..1c972717e 100644 --- a/src/frontend/services/graphical-scope.ts +++ b/src/frontend/services/graphical-scope.ts @@ -71,19 +71,6 @@ function splitExpression(value: string): { anchor: string; segment: string } { return { anchor: value.slice(0, lastDot + 1), segment: value.slice(lastDot + 1) } } -/** Strip a trailing array subscript (`foo[3]` → `foo`). Returns the name and whether a subscript was present. */ -function stripSubscript(segment: string): { name: string; indexed: boolean } { - const bracket = segment.indexOf('[') - if (bracket < 0) return { name: segment, indexed: false } - return { name: segment.slice(0, bracket), indexed: true } -} - -/** Pull the element type out of an `ARRAY [..] OF ` detail string. */ -function arrayElementType(type: string): string | undefined { - const match = type.match(/\bOF\s+([A-Za-z_][A-Za-z0-9_]*)/i) - return match ? match[1] : undefined -} - /** * Autocomplete candidates for `value` typed into a box in `pouName`'s * scope. `value` is the full current box text (e.g. `TON0.Q`, `mo`). @@ -138,8 +125,14 @@ export async function getScopeCompletions( /** * Resolve the IEC type of `expression` in `pouName`'s scope. Handles bare - * identifiers, member chains (`TON0.Q`, `s.a.b`) and 1-D array element - * access (`arr[3]`). See {@link ScopeTypeResult} for the tri-state result. + * identifiers, member chains (`TON0.Q`, `s.a.b`) and array element access + * (`arr[3]`, `grid[1,2]`). See {@link ScopeTypeResult} for the tri-state result. + * + * Array elements need no special casing: strucpp lists each in-bounds element + * as its own symbol typed as the element type, so `arr[3]` matches by label + * like any other. That also makes the bounds authoritative — `arr[99]` simply + * isn't a symbol, so it resolves `unknown` and the box is flagged, which a + * client-side subscript-stripping heuristic could never detect. */ export async function resolveScopeExpressionType(pouName: string, expression: string): Promise { const api = getScopedQueryApi() @@ -154,16 +147,11 @@ export async function resolveScopeExpressionType(pouName: string, expression: st // context yet — treat as unavailable rather than flag a false invalid. if (items.length === 0) return { status: 'unavailable' } - const { name, indexed } = stripSubscript(segment) const match = items.find( - (item) => isValueCompletionKind(item.kind) && item.label.toLowerCase() === name.toLowerCase(), + (item) => isValueCompletionKind(item.kind) && item.label.toLowerCase() === segment.toLowerCase(), ) if (!match || !match.type) return { status: 'unknown' } - if (indexed) { - const element = arrayElementType(match.type) - return element ? { status: 'resolved', type: element } : { status: 'unknown' } - } return { status: 'resolved', type: match.type } } diff --git a/src/frontend/utils/PLC/__tests__/array-variable-utils.test.ts b/src/frontend/utils/PLC/__tests__/array-variable-utils.test.ts index b400c67b5..e8d7b8dca 100644 --- a/src/frontend/utils/PLC/__tests__/array-variable-utils.test.ts +++ b/src/frontend/utils/PLC/__tests__/array-variable-utils.test.ts @@ -1,7 +1,5 @@ import type { PLCVariable } from '../../../../middleware/shared/ports/types' import { - expandArrayVariable, - expandArrayVariables, parseArrayAccess, parseDimensionRange, resolveArrayElement, @@ -203,96 +201,6 @@ describe('resolveArrayElement', () => { }) }) -// --------------------------------------------------------------------------- -// expandArrayVariable -// --------------------------------------------------------------------------- -describe('expandArrayVariable', () => { - it('expands a 1D array into indexed elements', () => { - const v = makeArrayVar('arr', 'base-type', 'INT', ['0..2']) - const result = expandArrayVariable(v) - expect(result.length).toBe(3) - expect(result.map((r) => r.name)).toEqual(['arr[0]', 'arr[1]', 'arr[2]']) - expect(result[0].type.definition).toBe('base-type') - expect(result[0].type.value).toBe('INT') - }) - - it('expands a 2D array with comma notation', () => { - const v = makeArrayVar('matrix', 'base-type', 'REAL', ['0..1', '0..1']) - const result = expandArrayVariable(v) - expect(result.length).toBe(4) - expect(result.map((r) => r.name)).toEqual(['matrix[0,0]', 'matrix[0,1]', 'matrix[1,0]', 'matrix[1,1]']) - }) - - it('returns the original variable for non-array types', () => { - const v = makeScalarVar('x') - const result = expandArrayVariable(v) - expect(result).toEqual([v]) - }) - - it('returns the original variable for array without data', () => { - const v: PLCVariable = { - name: 'arr', - class: 'local', - type: { definition: 'array', value: 'ARRAY[0..5] OF INT' }, - location: '', - documentation: '', - } - const result = expandArrayVariable(v) - expect(result).toEqual([v]) - }) - - it('returns the original variable when total elements exceed MAX_EXPANSION (100)', () => { - // 0..100 = 101 elements > 100 - const v = makeArrayVar('big', 'base-type', 'INT', ['0..100']) - const result = expandArrayVariable(v) - expect(result).toEqual([v]) - }) - - it('returns the original variable when a dimension range is invalid', () => { - const v: PLCVariable = { - name: 'arr', - class: 'local', - type: { - definition: 'array', - value: 'ARRAY[bad] OF INT', - data: { - baseType: { definition: 'base-type', value: 'INT' }, - dimensions: [{ dimension: 'bad' }], - }, - }, - location: '', - documentation: '', - } - const result = expandArrayVariable(v) - expect(result).toEqual([v]) - }) - - it('expands exactly 100 elements (at the limit)', () => { - // 0..99 = 100 elements - const v = makeArrayVar('big', 'base-type', 'INT', ['0..99']) - const result = expandArrayVariable(v) - expect(result.length).toBe(100) - }) -}) - -// --------------------------------------------------------------------------- -// expandArrayVariables -// --------------------------------------------------------------------------- -describe('expandArrayVariables', () => { - it('expands all arrays and passes scalars through', () => { - const vars = [makeScalarVar('x'), makeArrayVar('arr', 'base-type', 'INT', ['0..1'])] - const result = expandArrayVariables(vars) - expect(result.length).toBe(3) // 1 scalar + 2 array elements - expect(result[0].name).toBe('x') - expect(result[1].name).toBe('arr[0]') - expect(result[2].name).toBe('arr[1]') - }) - - it('handles empty list', () => { - expect(expandArrayVariables([])).toEqual([]) - }) -}) - // --------------------------------------------------------------------------- // resolveArrayVariableByName // --------------------------------------------------------------------------- diff --git a/src/frontend/utils/PLC/array-variable-utils.ts b/src/frontend/utils/PLC/array-variable-utils.ts index e97de4664..2672a0ea6 100644 --- a/src/frontend/utils/PLC/array-variable-utils.ts +++ b/src/frontend/utils/PLC/array-variable-utils.ts @@ -1,9 +1,6 @@ import type { PLCVariable } from '../../../middleware/shared/ports/types' -import type { DimensionRange } from './dimension-range' import { parseDimensionRange } from './dimension-range' -const MAX_EXPANSION = 100 - interface ParsedArrayAccess { baseName: string indices: number[] @@ -73,61 +70,6 @@ export const resolveArrayElement = (baseVariable: PLCVariable, access: ParsedArr } as PLCVariable } -/** - * Expand an array variable into all its indexed elements for autocomplete. - * Multidimensional arrays use comma notation: Matrix[0,0], Matrix[0,1], ... - * Capped at MAX_EXPANSION (100) elements to avoid flooding the UI. - */ -export const expandArrayVariable = (variable: PLCVariable): PLCVariable[] => { - if (variable.type.definition !== 'array') return [variable] - - const { data } = variable.type - if (!data) return [variable] - const ranges = data.dimensions.map((d) => parseDimensionRange(d.dimension)).filter((r): r is DimensionRange => !!r) - - if (ranges.length !== data.dimensions.length) return [variable] - - // Calculate total element count - const totalElements = ranges.reduce((acc, range) => acc * (range.upper - range.lower + 1), 1) - if (totalElements > MAX_EXPANSION) return [variable] - - // Generate all index combinations - const combinations: number[][] = [] - const generateCombinations = (dimIndex: number, current: number[]) => { - if (dimIndex === ranges.length) { - combinations.push([...current]) - return - } - const range = ranges[dimIndex] - for (let i = range.lower; i <= range.upper; i++) { - current.push(i) - generateCombinations(dimIndex + 1, current) - current.pop() - } - } - generateCombinations(0, []) - - return combinations.map((indices) => { - const indexStr = indices.join(',') - return { - ...variable, - name: `${variable.name}[${indexStr}]`, - type: { - definition: data.baseType.definition as PLCVariable['type']['definition'], - value: data.baseType.value, - }, - } as PLCVariable - }) -} - -/** - * Expand all array variables in a list, replacing each array with its elements. - * Non-array variables pass through unchanged. - */ -export const expandArrayVariables = (variables: PLCVariable[]): PLCVariable[] => { - return variables.flatMap(expandArrayVariable) -} - /** * Try to resolve a name as an array access against a variable list. * Returns the resolved synthetic PLCVariable, or undefined if not resolvable.