From dcbcf404bddd5f0b0c799104fb2249c2c5f60834 Mon Sep 17 00:00:00 2001 From: JulioSergioFS Date: Wed, 2 Sep 2026 20:06:30 -0300 Subject: [PATCH 1/4] feat(located): allow ARRAY at a physical address, and detect range collisions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Editor half of openplc-editor#565. The compiler half is Autonomy-Logic/STruCpp#229. **Accept the declaration.** `HR_myData AT %MW60 : ARRAY [0..66] OF WORD` was refused: location validation switched on the variable's own type, and an array fell through to the default branch. It now validates the ELEMENT type against the address class, which is what actually has to fit -- 67 consecutive WORD slots, each a WORD. The refusal came from the MatIEC era, and that constraint left with MatIEC. **Detect the collisions that come with it.** An array is a contiguous area, so `arr AT %QX0.0 : ARRAY [0..9] OF BOOL` runs through `%QX1.1` and conflicts with a plain `flag AT %QX0.6` -- two different address strings, one piece of storage. `checkIfLocationExists` compared locations for string equality, which was sufficient while every variable claimed exactly one slot and silently wrong the moment one could claim more. It now compares slot RANGES, via a new `slotRangesOverlap` beside `parseAddress`, reusing the existing `getArrayTotalElements` for the span. Two properties preserved deliberately: ranges only collide within the same class (`%MW0` and `%MD0` index different runtime arrays and still never overlap), and a location that is not a literal `%…` is an alias name, where the test stays exact equality. **Auto-increment follows.** The next-free-location scan tested set membership against exact strings, so it could stop one slot inside a neighbouring array, or place a new array on top of an existing scalar. It now advances until the candidate's whole span is clear, and steps by the element type -- an array's own `type.value` is the "ARRAY [...] OF T" text, which `incrementLocationByOne` matches against nothing and would have bailed on the first pass. Also fixes `variableLocationValidationErrorMessage` returning `''` for any type with no address class, which surfaced as a bare "Please make sure that the location is valid." with no reason attached. The bundled compiler is still pinned to strucpp v0.6.4, which rejects a located array. STruCpp#229 has to be released and `binary-versions.json` bumped before this reaches users, or the editor accepts a declaration the build then refuses. Co-Authored-By: Claude Opus 5 (1M context) --- .../project-validation-variables.test.ts | 122 ++++++++++++++++++ .../slices/project/validation/variables.ts | 111 +++++++++++++--- .../registry/__tests__/address-space.test.ts | 54 +++++++- .../iec-address/registry/address-space.ts | 24 ++++ .../utils/iec-address/registry/index.ts | 10 +- 5 files changed, 300 insertions(+), 21 deletions(-) diff --git a/src/frontend/store/__tests__/project-validation-variables.test.ts b/src/frontend/store/__tests__/project-validation-variables.test.ts index 9fbb4ffb8..cead84fcd 100644 --- a/src/frontend/store/__tests__/project-validation-variables.test.ts +++ b/src/frontend/store/__tests__/project-validation-variables.test.ts @@ -29,6 +29,30 @@ function makeVariable( } } +/** + * A located ARRAY variable. `data.dimensions` is what `getArrayTotalElements` + * reads to work out how many slots the declaration claims. + */ +function makeArrayVariable( + name: string, + baseType: string, + location: string, + dimension: string, + cls: PLCVariable['class'] = 'local', +): PLCVariable { + return { + name, + class: cls, + type: { + definition: 'array', + value: `ARRAY [${dimension}] OF ${baseType}`, + data: { baseType: { definition: 'base-type', value: baseType }, dimensions: [{ dimension }] }, + }, + location, + documentation: '', + } as PLCVariable +} + // =========================================================================== // extractNumberAtEnd // =========================================================================== @@ -767,3 +791,101 @@ describe('updateGlobalVariableValidation', () => { expect(result.ok).toBe(true) }) }) + +// =========================================================================== +// Located arrays: a contiguous area, not one address +// =========================================================================== + +describe('located arrays — collision by range', () => { + // An ARRAY at a physical address occupies one slot per element. Before + // openplc-editor#565 the editor compared locations for string equality, so + // it happily accepted two variables sharing storage and only the compiler + // caught it. + + it('accepts an array at a location whose element type fits the address', () => { + const variable = makeArrayVariable('HR_myData', 'WORD', '%MW60', '0..66', 'global') + const result = updateVariableValidation([variable], { location: '%MW60' }, variable) + expect(result.ok).toBe(true) + }) + + it('rejects a scalar that lands inside an existing array', () => { + // ARRAY [0..9] OF BOOL at %QX0.0 covers %QX0.0-%QX1.1, so %QX0.6 is inside. + const existing = [makeArrayVariable('arr', 'BOOL', '%QX0.0', '0..9')] + const flag = makeVariable('flag', 'BOOL', '', 'local') + const result = updateVariableValidation(existing, { location: '%QX0.6' }, flag) + expect(result.ok).toBe(false) + expect(result.title).toBe('Location already exists') + }) + + it('rejects an array that swallows an existing scalar', () => { + const existing = [makeVariable('flag', 'BOOL', '%QX0.6')] + const arr = makeArrayVariable('arr', 'BOOL', '', '0..9') + const result = updateVariableValidation(existing, { location: '%QX0.0' }, arr) + expect(result.ok).toBe(false) + }) + + it('accepts a scalar immediately past the end of an array', () => { + // ARRAY [0..3] OF WORD at %MW0 ends at %MW3. + const existing = [makeArrayVariable('arr', 'WORD', '%MW0', '0..3')] + const other = makeVariable('other', 'WORD', '', 'local') + expect(updateVariableValidation(existing, { location: '%MW3' }, other).ok).toBe(false) + expect(updateVariableValidation(existing, { location: '%MW4' }, other).ok).toBe(true) + }) + + it('does not collide across size classes', () => { + // %MW0 and %MD0 index different runtime arrays. + const existing = [makeArrayVariable('words', 'WORD', '%MW0', '0..7')] + const dword = makeVariable('dw', 'DINT', '', 'local') + expect(updateVariableValidation(existing, { location: '%MD0' }, dword).ok).toBe(true) + }) + + it('still catches two variables bound to the same alias', () => { + // A non-`%` location is an alias name, where the test stays exact + // equality — an alias resolves to one producer channel. + const existing = [makeVariable('a', 'BOOL', 'MotorStart')] + const b = makeVariable('b', 'BOOL', '', 'local') + expect(updateVariableValidation(existing, { location: 'MotorStart' }, b).ok).toBe(false) + expect(updateVariableValidation(existing, { location: 'MotorStop' }, b).ok).toBe(true) + }) + + it('ignores an alias-bound variable when checking a literal address', () => { + // The other side has no address to compare against — its location is an + // alias name, resolved to a real address only at compile time. + const existing = [makeVariable('aliased', 'WORD', 'TankLevel')] + const other = makeVariable('other', 'WORD', '', 'local') + expect(updateVariableValidation(existing, { location: '%MW0' }, other).ok).toBe(true) + }) + + it('widens the span when the same edit turns a scalar into an array', () => { + // The check has to use the type the variable will HAVE, not the one it had. + const existing = [makeVariable('neighbour', 'WORD', '%MW3')] + const scalar = makeVariable('grow', 'WORD', '', 'local') + const asArray = makeArrayVariable('grow', 'WORD', '', '0..3').type + expect(updateVariableValidation(existing, { location: '%MW0' }, scalar).ok).toBe(true) + expect(updateVariableValidation(existing, { location: '%MW0', type: asArray }, scalar).ok).toBe(false) + }) +}) + +describe('createVariableValidation — auto-increment past occupied ranges', () => { + it('skips the whole span of an existing array', () => { + // %MW0-%MW3 taken by the array, so the next free word is %MW4. Landing on + // %MW1 (the old exact-match behaviour would have) is the same collision. + const existing = [makeArrayVariable('arr', 'WORD', '%MW0', '0..3')] + const result = createVariableValidation(existing, makeVariable('NewVar', 'WORD', '%MW0')) + expect(result.location).toBe('%MW4') + }) + + it('moves a new array clear of an existing scalar', () => { + // The candidate has to clear every slot the ARRAY would claim: at %MW0 it + // would cover %MW0-%MW3 and swallow the scalar at %MW2. + const existing = [makeVariable('taken', 'WORD', '%MW2')] + const result = createVariableValidation(existing, makeArrayVariable('arr', 'WORD', '%MW0', '0..3')) + expect(result.location).toBe('%MW3') + }) + + it('leaves a location alone when nothing overlaps', () => { + const existing = [makeArrayVariable('arr', 'WORD', '%MW10', '0..3')] + const result = createVariableValidation(existing, makeVariable('NewVar', 'WORD', '%MW0')) + expect(result.location).toBe('%MW0') + }) +}) diff --git a/src/frontend/store/slices/project/validation/variables.ts b/src/frontend/store/slices/project/validation/variables.ts index 63186db6f..a8e88dc28 100644 --- a/src/frontend/store/slices/project/validation/variables.ts +++ b/src/frontend/store/slices/project/validation/variables.ts @@ -1,4 +1,5 @@ import type { PLCVariable } from '../../../../../middleware/shared/ports/types' +import { parseAddress, slotRangesOverlap } from '../../../../../middleware/shared/utils/iec-address/registry' import { DISALLOWED_LOCATION_CLASSES } from '../../../../utils/generate-iec-string-to-variables' import { BOOL_LOCATION_REGEX, @@ -8,6 +9,7 @@ import { PLC_ADDRESS_PREFIX, WORD_LOCATION_REGEX, } from '../../../../utils/PLC/address-constants/types' +import { getArrayTotalElements, isArrayVariable } from '../../../../utils/PLC/array-codegen-helpers' import type { ProjectResponse } from '../types' /** @@ -35,16 +37,47 @@ const checkIfGlobalVariableExists = (variables: PLCVariable[], name: string) => } /** - * This is a validation to check if the value of the location is unique. + * How many consecutive slots a variable claims from its location. * - * `exclude` lets the update path skip the variable currently being - * mutated — re-setting a variable's location to its current value - * (e.g. to re-resolve a renamed alias) must not collide with itself. - * Reference-equality is enough since `variables` is the live array - * and the caller passes the same object reference. + * A scalar claims one. An ARRAY claims one per element, laid out from the + * declared address — `AT %MW60 : ARRAY [0..66] OF WORD` runs through `%MW126` + * (openplc-editor#565). `getArrayTotalElements` already computes the product + * of the dimensions and answers `0` for a shape it cannot read, which + * `slotRangesOverlap` floors back to 1: an unreadable array must not silently + * claim nothing. */ -const checkIfLocationExists = (variables: PLCVariable[], location: string, exclude?: PLCVariable) => { - return variables.some((variable) => variable !== exclude && variable.location === location) +const slotsClaimedBy = (variable: PLCVariable): number => + isArrayVariable(variable) ? getArrayTotalElements(variable) : 1 + +/** + * Does `location` collide with a location another variable already holds? + * + * Two literal `%…` addresses collide when their SLOT RANGES overlap, not when + * the strings match. An array is a contiguous area, so `arr AT %QX0.0 : + * ARRAY [0..9] OF BOOL` covers `%QX0.0`–`%QX1.1` and conflicts with a plain + * `flag AT %QX0.6` — two different strings, one piece of storage. Comparing + * for string equality (all that was needed while every variable took one slot) + * let the editor build a project the compiler then rejected. + * + * A location that is NOT a literal address is an alias name, and there the + * test stays exact equality: an alias resolves to one producer channel, so two + * variables naming the same alias collide and two different names never do. + * + * `exclude` skips the variable being updated so re-setting its own location + * doesn't trip the check against itself. + */ +const checkIfLocationExists = (variables: PLCVariable[], location: string, slots: number, exclude?: PLCVariable) => { + const parsed = parseAddress(location) + + return variables.some((variable) => { + if (variable === exclude) return false + if (parsed === null) return variable.location === location + + const otherParsed = parseAddress(variable.location) + if (otherParsed === null) return false + + return slotRangesOverlap(parsed, slots, otherParsed, slotsClaimedBy(variable)) + }) } /** @@ -117,6 +150,26 @@ const arrayValidation = ({ value }: { value: string }) => { return { ok: true } } +/** + * The type that has to match the address class, given a variable's declared + * type. + * + * For an array this is the ELEMENT type: `ARRAY [0..66] OF WORD AT %MW60` + * occupies 67 consecutive WORD slots, so what has to fit `%MW` is WORD, not + * the array (openplc-editor#565). Locating an array used to be rejected + * outright — a limitation of the MatIEC-era toolchain that left with MatIEC. + * + * Returns the type's own name for every other definition, which lands + * `user-data-type` and `derived` on the `default` branch below, where they + * belong: a STRUCT has no single address class. + */ +const addressClassTypeOf = (variableType: PLCVariable['type']): string => + // `data` is optional on the port-side type; an array without it is a + // half-built row from the array modal, and falling back to `value` (the + // "ARRAY [...] OF T" text) lands it on the `default` branch — rejected with + // a message, which is the right answer for a type that isn't finished. + variableType.definition === 'array' && variableType.data ? variableType.data.baseType.value : variableType.value + /** * Validate a variable's `location`. Single-field model: `location` is either * an alias name, a literal IEC address, or empty. @@ -178,7 +231,11 @@ const variableLocationValidationErrorMessage = (variableType: string) => { case 'LWORD': return 'Valid locations: %QL0, %IL0, %ML0 (change the number to the desired location)' default: - return '' + // Reached by a structure or an enum — types with no single address + // class. This used to return an empty string, so the dialog showed + // "Please make sure that the location is valid." and nothing else: a + // refusal with no reason and nothing to act on. + return `A variable of type "${variableType}" cannot have a physical location: only the elementary types (BOOL, BYTE, INT, WORD, DINT, REAL, ...) and arrays of them map onto an IEC address.` } } @@ -331,20 +388,31 @@ const createVariableValidation = ( response.name = `${variableNameWithoutNumber}${number}` } - if (checkIfLocationExists(variables, variableLocation) && variableLocation !== '') { + const slots = slotsClaimedBy(variable) + + if (variableLocation !== '' && checkIfLocationExists(variables, variableLocation, slots)) { // Scan forward through the address space until we find a slot // that no other variable in this table holds. Single-increment // wasn't enough: when the user kept clicking "+" through a row // of contiguous variables, the increment would eventually land // ON another already-bound row and silently produce a duplicate- // location collision that only the compiler caught (forum - // thread, v4.2.0 follow-up). An `inUse` set keeps the inner - // check O(1) so the loop is linear in the number of variables. - const inUse = new Set(variables.map((v) => v.location)) + // thread, v4.2.0 follow-up). + // + // The test is range overlap rather than set membership because an + // ARRAY occupies a contiguous area: a candidate has to clear every + // slot the new variable would claim, and has to clear the whole + // span of any array already sitting there — landing one slot inside + // a neighbouring array is the same collision as landing on its + // first address (openplc-editor#565). let candidate = variableLocation let iterations = 0 - while (inUse.has(candidate) && iterations < MAX_AUTO_INCREMENT_ITERATIONS) { - const next = incrementLocationByOne(candidate, variable.type.value) + while (checkIfLocationExists(variables, candidate, slots) && iterations < MAX_AUTO_INCREMENT_ITERATIONS) { + // Step by the ELEMENT type: `incrementLocationByOne` switches on a base + // type name, and an array's own `type.value` is the whole + // "ARRAY [0..9] OF BOOL" text, which matches nothing and would bail the + // loop on its first pass. + const next = incrementLocationByOne(candidate, addressClassTypeOf(variable.type)) if (!next || next === candidate) break // unknown type / no progress — bail candidate = next iterations += 1 @@ -427,7 +495,12 @@ const updateVariableValidation = ( // Exclude the variable being updated so re-setting its own // location (e.g. re-picking the same address to refresh a // renamed alias) doesn't trip the uniqueness check on itself. - if (checkIfLocationExists(variables, location, variableToUpdate)) { + // + // The span comes from the type the variable will HAVE after this update: + // switching a scalar to an array in the same edit widens what it claims, + // and checking the old span would let it land on top of a neighbour. + const effectiveSlots = slotsClaimedBy({ ...variableToUpdate, ...dataToBeUpdated }) + if (checkIfLocationExists(variables, location, effectiveSlots, variableToUpdate)) { response = { ok: false, title: 'Location already exists', @@ -436,18 +509,18 @@ const updateVariableValidation = ( return response } - if (!variableLocationValidation(location, variableToUpdate.type.value)) { + if (!variableLocationValidation(location, addressClassTypeOf(variableToUpdate.type))) { response = { ok: false, title: 'Location is invalid.', - message: `Please make sure that the location is valid.\n${variableLocationValidationErrorMessage(variableToUpdate.type.value)}`, + message: `Please make sure that the location is valid.\n${variableLocationValidationErrorMessage(addressClassTypeOf(variableToUpdate.type))}`, } return response } } if (dataToBeUpdated.type) { - if (!variableLocationValidation(variableToUpdate.location, dataToBeUpdated.type.value)) { + if (!variableLocationValidation(variableToUpdate.location, addressClassTypeOf(dataToBeUpdated.type))) { response.data = { ...(response.data ? response.data : {}), location: '' } } if (dataToBeUpdated.type.definition === 'derived') { diff --git a/src/middleware/shared/utils/iec-address/registry/__tests__/address-space.test.ts b/src/middleware/shared/utils/iec-address/registry/__tests__/address-space.test.ts index 6cf479ae8..89ac6b26f 100644 --- a/src/middleware/shared/utils/iec-address/registry/__tests__/address-space.test.ts +++ b/src/middleware/shared/utils/iec-address/registry/__tests__/address-space.test.ts @@ -1,4 +1,4 @@ -import { formatAddress, isBitClass, isIecAddress, parseAddress, prefixOf } from '../address-space' +import { formatAddress, isBitClass, isIecAddress, parseAddress, prefixOf, slotRangesOverlap } from '../address-space' import type { AddressClass } from '../types' describe('address-space', () => { @@ -63,4 +63,56 @@ describe('address-space', () => { expect(isIecAddress('')).toBe(false) }) }) + + // An ARRAY at a physical address occupies one slot PER ELEMENT, laid out + // consecutively (openplc-editor#565), so "do these two collide" stops being + // a string comparison and becomes a range question. + describe('slotRangesOverlap', () => { + /** Non-null parse, so the tests read as addresses rather than as guards. */ + const at = (address: string) => { + const parsed = parseAddress(address) + if (!parsed) throw new Error(`not an address: ${address}`) + return parsed + } + + it('detects a scalar landing inside an array', () => { + // The case from the issue discussion: ARRAY [0..9] OF BOOL at %QX0.0 + // covers %QX0.0-%QX1.1, so a BOOL at %QX0.6 is inside it — and the two + // address strings are different, which is why equality missed it. + expect(slotRangesOverlap(at('%QX0.0'), 10, at('%QX0.6'), 1)).toBe(true) + expect(slotRangesOverlap(at('%QX0.6'), 1, at('%QX0.0'), 10)).toBe(true) + }) + + it('lets a scalar sit immediately past the end of an array', () => { + // %MW0 + 4 slots ends at %MW3. + expect(slotRangesOverlap(at('%MW0'), 4, at('%MW3'), 1)).toBe(true) + expect(slotRangesOverlap(at('%MW0'), 4, at('%MW4'), 1)).toBe(false) + }) + + it('detects two arrays that straddle each other', () => { + expect(slotRangesOverlap(at('%IW0'), 4, at('%IW3'), 4)).toBe(true) + expect(slotRangesOverlap(at('%IW0'), 4, at('%IW4'), 4)).toBe(false) + }) + + it('walks bit ranges across the byte boundary', () => { + // %QX0.6 + 4 slots -> %QX0.6, %QX0.7, %QX1.0, %QX1.1. + expect(slotRangesOverlap(at('%QX0.6'), 4, at('%QX1.1'), 1)).toBe(true) + expect(slotRangesOverlap(at('%QX0.6'), 4, at('%QX1.2'), 1)).toBe(false) + }) + + it('never collides across classes — each prefix is its own space', () => { + // %MW0 and %MD0 index different runtime arrays; same index, unrelated + // storage. Direction separates %IW0 from %QW0 for the same reason. + expect(slotRangesOverlap(at('%MW0'), 8, at('%MD0'), 8)).toBe(false) + expect(slotRangesOverlap(at('%IW0'), 8, at('%QW0'), 8)).toBe(false) + }) + + it('reads a slot count below 1 as 1', () => { + // getArrayTotalElements answers 0 for a shape it cannot read. Claiming + // nothing would make a malformed array collide with nobody; it must + // still hold the address it names. + expect(slotRangesOverlap(at('%MW5'), 0, at('%MW5'), 1)).toBe(true) + expect(slotRangesOverlap(at('%MW5'), 0, at('%MW6'), 1)).toBe(false) + }) + }) }) diff --git a/src/middleware/shared/utils/iec-address/registry/address-space.ts b/src/middleware/shared/utils/iec-address/registry/address-space.ts index 79c646ef7..57f6dce25 100644 --- a/src/middleware/shared/utils/iec-address/registry/address-space.ts +++ b/src/middleware/shared/utils/iec-address/registry/address-space.ts @@ -57,3 +57,27 @@ export function parseAddress(address: string): ParsedAddress | null { export function isIecAddress(value: string): boolean { return parseAddress(value) !== null } + +/** + * Do two located declarations touch the same storage? + * + * A scalar occupies one slot; an ARRAY occupies one PER ELEMENT, laid out + * consecutively from its declared address. So `%QX0.0` holding an + * `ARRAY [0..9] OF BOOL` runs through `%QX1.1`, and a plain `BOOL` at + * `%QX0.6` lands inside it — a collision the compiler rejects, even though + * the two address strings differ (openplc-editor#565). + * + * Only ranges in the SAME class collide. Each prefix is its own linear space: + * `%MW0` and `%MD0` name unrelated storage (different runtime arrays), so + * they never overlap regardless of index. + * + * `slots` below 1 is read as 1 — a declaration always occupies at least the + * address it names, and an unreadable array shape must not silently claim + * nothing. + */ +export function slotRangesOverlap(a: ParsedAddress, aSlots: number, b: ParsedAddress, bSlots: number): boolean { + if (a.cls.direction !== b.cls.direction || a.cls.size !== b.cls.size) return false + const aEnd = a.linear + Math.max(1, aSlots) - 1 + const bEnd = b.linear + Math.max(1, bSlots) - 1 + return a.linear <= bEnd && b.linear <= aEnd +} diff --git a/src/middleware/shared/utils/iec-address/registry/index.ts b/src/middleware/shared/utils/iec-address/registry/index.ts index 018640cd8..758cf2c2d 100644 --- a/src/middleware/shared/utils/iec-address/registry/index.ts +++ b/src/middleware/shared/utils/iec-address/registry/index.ts @@ -1,4 +1,12 @@ -export { formatAddress, isBitClass, isIecAddress, parseAddress, type ParsedAddress, prefixOf } from './address-space' +export { + formatAddress, + isBitClass, + isIecAddress, + parseAddress, + type ParsedAddress, + prefixOf, + slotRangesOverlap, +} from './address-space' export { allocateAddresses, channelKey } from './allocate' export { ethercatConsumerId, From 327277a587ef5a515b5437d8f5700d1e709a76be Mon Sep 17 00:00:00 2001 From: JulioSergioFS Date: Wed, 2 Sep 2026 21:56:29 -0300 Subject: [PATCH 2/4] fix(located): validate against the post-edit type, and step any address class MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the review findings on this PR. **A type-only edit could widen a located variable unchecked.** The overlap check lived inside `if (dataToBeUpdated.location)`, so changing only the TYPE of an already-located variable never re-ran it: a scalar at %MW0 turned into `ARRAY [0..3] OF WORD` silently grew over %MW1-%MW3 and whatever sat there. The check now also runs on the type branch, and says what would be covered. **A joint location+type edit validated the new location against the OLD type.** Both checks now derive one effective type (`dataToBeUpdated.type ?? variableToUpdate.type`) and one effective span, computed once. **`incrementLocationByOne` could not step half the address classes.** It switched on the variable's type and had no case for BYTE / SINT / USINT, so `%IB` / `%QB` / `%MB` returned null and the auto-increment gave up, keeping a colliding location. Worse, its BOOL case stripped only the `%QX` and `%IX` prefixes, so a memory bit `%MX0.0` fell through with its prefix intact and `parseInt` produced `%IXNaN.NaN`. Replaced with two lines over the `parseAddress` / `formatAddress` pair this branch already introduced: the address states its own size class, and every class advances identically once linearised (a bit address is `byte*8 + bit`, so `%QX0.7` steps to `%QX1.0` without spelling the carry out per class). 64 lines to 16, both holes closed. One behaviour change falls out of that, and it is an improvement: the walk used to give up on any type its switch did not list, LEAVING a known duplicate in place. It now keys off the address, so a colliding location moves on regardless of the type sitting at it. Only a non-address location (an alias name) stops the walk now, which is the honest answer — resolving an alias collision means picking a different alias, not inventing an address. The test that pinned the old no-op is updated to state this. Also drops the `as PLCVariable` assertion from the test fixture, per the repo's no-type-assertions rule. Co-Authored-By: Claude Opus 5 (1M context) --- .../project-validation-variables.test.ts | 65 ++++++++- .../slices/project/validation/variables.ts | 124 +++++++----------- 2 files changed, 106 insertions(+), 83 deletions(-) diff --git a/src/frontend/store/__tests__/project-validation-variables.test.ts b/src/frontend/store/__tests__/project-validation-variables.test.ts index cead84fcd..b7ea40cee 100644 --- a/src/frontend/store/__tests__/project-validation-variables.test.ts +++ b/src/frontend/store/__tests__/project-validation-variables.test.ts @@ -50,7 +50,7 @@ function makeArrayVariable( }, location, documentation: '', - } as PLCVariable + } } // =========================================================================== @@ -361,12 +361,35 @@ describe('createVariableValidation', () => { }) // -- Default case (unknown type) -- - it('does not change location for unknown type', () => { + it('still walks when the type has no address class, as long as the address parses', () => { + // Behaviour change: the walk used to key off the variable's TYPE and gave + // up on anything its switch did not list, leaving a known duplicate in + // place. It now keys off the ADDRESS, which already states its size class, + // so a colliding %MD0 moves on regardless of the type sitting at it. const existing = [makeVariable('Var1', 'STRING', '%MD0')] const variable = makeVariable('NewVar', 'STRING', '%MD0') - const result = createVariableValidation(existing, variable) - // default case is a no-op, location stays as found - expect(result.location).toBe('%MD0') + expect(createVariableValidation(existing, variable).location).toBe('%MD1') + }) + + it('leaves an alias-bound location alone — there is nothing to increment', () => { + // A location that is not a literal address cannot be stepped, so the walk + // bails on the first pass and the duplicate stands. That is the honest + // answer: resolving an alias collision means picking a different alias, + // not inventing an address. + const existing = [makeVariable('Var1', 'BOOL', 'MotorStart')] + const variable = makeVariable('NewVar', 'BOOL', 'MotorStart') + expect(createVariableValidation(existing, variable).location).toBe('MotorStart') + }) + + it('walks byte and memory-bit addresses, which the old switch could not', () => { + // %IB had no case at all (the walk gave up), and %MX fell through the BOOL + // case with its prefix unstripped, producing "%IXNaN.NaN". + expect( + createVariableValidation([makeVariable('a', 'BYTE', '%IB0')], makeVariable('b', 'BYTE', '%IB0')).location, + ).toBe('%IB1') + expect( + createVariableValidation([makeVariable('a', 'BOOL', '%MX0.7')], makeVariable('b', 'BOOL', '%MX0.7')).location, + ).toBe('%MX1.0') }) // -- Multi-collision walk (regression for forum bug: contiguous "+" clicks @@ -856,6 +879,38 @@ describe('located arrays — collision by range', () => { expect(updateVariableValidation(existing, { location: '%MW0' }, other).ok).toBe(true) }) + it('catches a TYPE-ONLY edit that widens an already-located variable', () => { + // No location in the patch: the variable stays at %MW0 and only its type + // changes, so it silently grows over %MW1-%MW3 and swallows the neighbour. + // The location block never runs for this edit, which is how it slipped by. + const neighbour = makeVariable('neighbour', 'WORD', '%MW2') + const grow = makeVariable('grow', 'WORD', '%MW0') + const asArray = makeArrayVariable('grow', 'WORD', '%MW0', '0..3').type + const result = updateVariableValidation([neighbour, grow], { type: asArray }, grow) + expect(result.ok).toBe(false) + expect(result.message).toContain('would now cover 4 addresses') + }) + + it('allows a type-only widening that still fits', () => { + const neighbour = makeVariable('neighbour', 'WORD', '%MW9') + const grow = makeVariable('grow', 'WORD', '%MW0') + const asArray = makeArrayVariable('grow', 'WORD', '%MW0', '0..3').type + expect(updateVariableValidation([neighbour, grow], { type: asArray }, grow).ok).toBe(true) + }) + + it('validates a joint location+type edit against the NEW type', () => { + // %MW0 is a word address. Changing the type to BOOL in the same edit makes + // it invalid, and checking against the old WORD type would have passed it. + const v = makeVariable('v', 'WORD', '%MW0') + const result = updateVariableValidation( + [v], + { location: '%MW0', type: { definition: 'base-type', value: 'BOOL' } }, + v, + ) + expect(result.ok).toBe(false) + expect(result.title).toBe('Location is invalid.') + }) + it('widens the span when the same edit turns a scalar into an array', () => { // The check has to use the type the variable will HAVE, not the one it had. const existing = [makeVariable('neighbour', 'WORD', '%MW3')] diff --git a/src/frontend/store/slices/project/validation/variables.ts b/src/frontend/store/slices/project/validation/variables.ts index a8e88dc28..8ed550b83 100644 --- a/src/frontend/store/slices/project/validation/variables.ts +++ b/src/frontend/store/slices/project/validation/variables.ts @@ -1,12 +1,15 @@ import type { PLCVariable } from '../../../../../middleware/shared/ports/types' -import { parseAddress, slotRangesOverlap } from '../../../../../middleware/shared/utils/iec-address/registry' +import { + formatAddress, + parseAddress, + slotRangesOverlap, +} from '../../../../../middleware/shared/utils/iec-address/registry' import { DISALLOWED_LOCATION_CLASSES } from '../../../../utils/generate-iec-string-to-variables' import { BOOL_LOCATION_REGEX, BYTE_LOCATION_REGEX, DWORD_LOCATION_REGEX, LWORD_LOCATION_REGEX, - PLC_ADDRESS_PREFIX, WORD_LOCATION_REGEX, } from '../../../../utils/PLC/address-constants/types' import { getArrayTotalElements, isArrayVariable } from '../../../../utils/PLC/array-codegen-helpers' @@ -297,69 +300,21 @@ const checkVariableName = (variables: PLCVariable[], variableName: string) => { * currently holds, so an unknown future IEC type can't produce an * infinite loop here. */ -const incrementLocationByOne = (location: string, typeValue: string): string | null => { - switch (typeValue.toUpperCase()) { - case 'BOOL': { - const stringWithNoPrefix = location - .replace(PLC_ADDRESS_PREFIX.BOOL_OUTPUT, '') - .replace(PLC_ADDRESS_PREFIX.BOOL_INPUT, '') - const position = parseInt(stringWithNoPrefix.split('.')[0]) - const dotPosition = parseInt(stringWithNoPrefix.split('.')[1]) - const prefix = location.startsWith(PLC_ADDRESS_PREFIX.BOOL_OUTPUT) - ? PLC_ADDRESS_PREFIX.BOOL_OUTPUT - : PLC_ADDRESS_PREFIX.BOOL_INPUT - return `${prefix}${dotPosition === 7 ? position + 1 : position}.${dotPosition === 7 ? 0 : dotPosition + 1}` - } - case 'INT': - case 'UINT': - case 'WORD': { - const stringWithNoPrefix = location - .replace(PLC_ADDRESS_PREFIX.WORD_OUTPUT, '') - .replace(PLC_ADDRESS_PREFIX.WORD_INPUT, '') - .replace(PLC_ADDRESS_PREFIX.WORD_MEMORY, '') - const position = parseInt(stringWithNoPrefix) - const prefix = location.startsWith(PLC_ADDRESS_PREFIX.WORD_OUTPUT) - ? PLC_ADDRESS_PREFIX.WORD_OUTPUT - : location.startsWith(PLC_ADDRESS_PREFIX.WORD_INPUT) - ? PLC_ADDRESS_PREFIX.WORD_INPUT - : PLC_ADDRESS_PREFIX.WORD_MEMORY - return `${prefix}${position + 1}` - } - case 'DINT': - case 'UDINT': - case 'REAL': - case 'DWORD': { - const stringWithNoPrefix = location - .replace(PLC_ADDRESS_PREFIX.DWORD_OUTPUT, '') - .replace(PLC_ADDRESS_PREFIX.DWORD_INPUT, '') - .replace(PLC_ADDRESS_PREFIX.DWORD_MEMORY, '') - const position = parseInt(stringWithNoPrefix) - const prefix = location.startsWith(PLC_ADDRESS_PREFIX.DWORD_OUTPUT) - ? PLC_ADDRESS_PREFIX.DWORD_OUTPUT - : location.startsWith(PLC_ADDRESS_PREFIX.DWORD_INPUT) - ? PLC_ADDRESS_PREFIX.DWORD_INPUT - : PLC_ADDRESS_PREFIX.DWORD_MEMORY - return `${prefix}${position + 1}` - } - case 'LINT': - case 'ULINT': - case 'LREAL': - case 'LWORD': { - const stringWithNoPrefix = location - .replace(PLC_ADDRESS_PREFIX.LWORD_OUTPUT, '') - .replace(PLC_ADDRESS_PREFIX.LWORD_INPUT, '') - .replace(PLC_ADDRESS_PREFIX.LWORD_MEMORY, '') - const position = parseInt(stringWithNoPrefix) - const prefix = location.startsWith(PLC_ADDRESS_PREFIX.LWORD_OUTPUT) - ? PLC_ADDRESS_PREFIX.LWORD_OUTPUT - : location.startsWith(PLC_ADDRESS_PREFIX.LWORD_INPUT) - ? PLC_ADDRESS_PREFIX.LWORD_INPUT - : PLC_ADDRESS_PREFIX.LWORD_MEMORY - return `${prefix}${position + 1}` - } - default: - return null - } +const incrementLocationByOne = (location: string): string | null => { + // Derived from the ADDRESS, not from the variable's type. The address + // already states its size class (`%QX` bit, `%IB` byte, `%MW` word, ...), + // and every class advances the same way once linearised — `parseAddress` + // maps a bit address to `byte*8 + bit`, so `%QX0.7` steps to `%QX1.0` + // without the carry needing to be spelled out per class. + // + // This replaced a per-type switch that had two holes: it had no case for + // BYTE / SINT / USINT (so `%IB` / `%QB` / `%MB` returned null and the + // caller's search gave up, keeping a colliding location), and its BOOL case + // stripped only the `%QX` and `%IX` prefixes, so a memory bit `%MX0.0` fell + // through with its prefix intact and `parseInt` produced `%IXNaN.NaN`. + const parsed = parseAddress(location) + if (!parsed) return null + return formatAddress(parsed.cls, parsed.linear + 1) } /** Safety bound on the auto-increment loop in `createVariableValidation`. @@ -408,11 +363,7 @@ const createVariableValidation = ( let candidate = variableLocation let iterations = 0 while (checkIfLocationExists(variables, candidate, slots) && iterations < MAX_AUTO_INCREMENT_ITERATIONS) { - // Step by the ELEMENT type: `incrementLocationByOne` switches on a base - // type name, and an array's own `type.value` is the whole - // "ARRAY [0..9] OF BOOL" text, which matches nothing and would bail the - // loop on its first pass. - const next = incrementLocationByOne(candidate, addressClassTypeOf(variable.type)) + const next = incrementLocationByOne(candidate) if (!next || next === candidate) break // unknown type / no progress — bail candidate = next iterations += 1 @@ -475,6 +426,14 @@ const updateVariableValidation = ( } } + // Both location checks below reason about the variable as it will be AFTER + // this update, not as it is now. A single edit can change the location, the + // type, or both, and validating a new location against the old type (or a + // new type against the old span) is how a joint edit slips through. + const effectiveType = dataToBeUpdated.type ?? variableToUpdate.type + const effectiveAddressClass = addressClassTypeOf(effectiveType) + const effectiveSlots = slotsClaimedBy({ ...variableToUpdate, ...dataToBeUpdated }) + if (dataToBeUpdated.location) { const { location } = dataToBeUpdated @@ -495,11 +454,6 @@ const updateVariableValidation = ( // Exclude the variable being updated so re-setting its own // location (e.g. re-picking the same address to refresh a // renamed alias) doesn't trip the uniqueness check on itself. - // - // The span comes from the type the variable will HAVE after this update: - // switching a scalar to an array in the same edit widens what it claims, - // and checking the old span would let it land on top of a neighbour. - const effectiveSlots = slotsClaimedBy({ ...variableToUpdate, ...dataToBeUpdated }) if (checkIfLocationExists(variables, location, effectiveSlots, variableToUpdate)) { response = { ok: false, @@ -509,19 +463,33 @@ const updateVariableValidation = ( return response } - if (!variableLocationValidation(location, addressClassTypeOf(variableToUpdate.type))) { + if (!variableLocationValidation(location, effectiveAddressClass)) { response = { ok: false, title: 'Location is invalid.', - message: `Please make sure that the location is valid.\n${variableLocationValidationErrorMessage(addressClassTypeOf(variableToUpdate.type))}`, + message: `Please make sure that the location is valid.\n${variableLocationValidationErrorMessage(effectiveAddressClass)}`, } return response } } if (dataToBeUpdated.type) { - if (!variableLocationValidation(variableToUpdate.location, addressClassTypeOf(dataToBeUpdated.type))) { + if (!variableLocationValidation(variableToUpdate.location, effectiveAddressClass)) { response.data = { ...(response.data ? response.data : {}), location: '' } + } else if ( + // A type-only edit can widen what an already-located variable claims: + // turning a scalar at %MW0 into an ARRAY [0..3] makes it swallow %MW1-3 + // and whatever sits there. The block above only runs when the LOCATION + // is part of the edit, so without this the widening lands unchecked. + variableToUpdate.location !== '' && + checkIfLocationExists(variables, variableToUpdate.location, effectiveSlots, variableToUpdate) + ) { + response = { + ok: false, + title: 'Location already exists', + message: `"${variableToUpdate.name}" at ${variableToUpdate.location} would now cover ${effectiveSlots} addresses, overlapping another variable. Move it, or shorten the array.`, + } + return response } if (dataToBeUpdated.type.definition === 'derived') { response.data = { ...(response.data ? response.data : {}), location: '', initialValue: '', class: 'local' } From bdab871a54dd78fe19c53d0eee3972cf6a9b02da Mon Sep 17 00:00:00 2001 From: JulioSergioFS Date: Wed, 2 Sep 2026 22:17:58 -0300 Subject: [PATCH 3/4] fix(located): refuse multi-dimensional arrays, and hoist the walk's scan MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two more review findings. **A multi-dimensional array could be located here and not there.** `getArrayTotalElements` returns the product of every dimension, so `AT %MW0 : ARRAY [0..3, 0..3] OF WORD` was accepted, reserved 16 slots and validated its WORD base type — while the compiler refuses it outright: Located variable 'MD' at %MW0 cannot be placed: a 2-dimensional array has no single linear run of addresses to occupy. That is the same accept-here/reject-there divergence this branch exists to close, so the editor now refuses it too, in both paths that can produce it: a location edit, and the type-only patch the array modal dispatches when a user adds a dimension to an already-located array. **The auto-increment walk re-scanned every variable on every step.** Replacing the hoisted `Set` with a `checkIfLocationExists` call per iteration made it O(iterations x variables x regex): the walk steps one element slot at a time, so placing an `ARRAY [0..999]` on a taken address would run ~1000 iterations over every variable, each re-parsing its address, synchronously inside the store's `produce`. The occupied spans are now parsed once before the loop and each step is a plain interval comparison. Co-Authored-By: Claude Opus 5 (1M context) --- .../project-validation-variables.test.ts | 57 ++++++++++++++++++ .../slices/project/validation/variables.ts | 59 ++++++++++++++++++- 2 files changed, 115 insertions(+), 1 deletion(-) diff --git a/src/frontend/store/__tests__/project-validation-variables.test.ts b/src/frontend/store/__tests__/project-validation-variables.test.ts index b7ea40cee..e7f675e54 100644 --- a/src/frontend/store/__tests__/project-validation-variables.test.ts +++ b/src/frontend/store/__tests__/project-validation-variables.test.ts @@ -33,6 +33,29 @@ function makeVariable( * A located ARRAY variable. `data.dimensions` is what `getArrayTotalElements` * reads to work out how many slots the declaration claims. */ +/** A located ARRAY with an arbitrary number of dimensions. */ +function makeMultiDimArrayVariable( + name: string, + baseType: string, + location: string, + dimensions: string[], +): PLCVariable { + return { + name, + class: 'local', + type: { + definition: 'array', + value: `ARRAY [${dimensions.join(', ')}] OF ${baseType}`, + data: { + baseType: { definition: 'base-type', value: baseType }, + dimensions: dimensions.map((dimension) => ({ dimension })), + }, + }, + location, + documentation: '', + } +} + function makeArrayVariable( name: string, baseType: string, @@ -879,6 +902,32 @@ describe('located arrays — collision by range', () => { expect(updateVariableValidation(existing, { location: '%MW0' }, other).ok).toBe(true) }) + it('refuses a multi-dimensional array at a location, as the compiler does', () => { + // ARRAY [0..3, 0..3] has no single run of consecutive addresses to sit on. + // getArrayTotalElements answers 16 for it, so without an explicit refusal + // the editor would place it and the build would fail later. + const md = makeMultiDimArrayVariable('md', 'WORD', '', ['0..3', '0..3']) + const result = updateVariableValidation([md], { location: '%MW0' }, md) + expect(result.ok).toBe(false) + expect(result.message).toContain('multi-dimensional array cannot have a physical location') + }) + + it('refuses a type-only edit that turns a located 1-D array into a 2-D one', () => { + // The array modal dispatches a type-only patch, so this is the path a user + // actually takes to get here. + const arr = makeArrayVariable('arr', 'WORD', '%MW0', '0..3') + const twoD = makeMultiDimArrayVariable('arr', 'WORD', '%MW0', ['0..3', '0..3']).type + const result = updateVariableValidation([arr], { type: twoD }, arr) + expect(result.ok).toBe(false) + expect(result.message).toContain('multi-dimensional array cannot have a physical location') + }) + + it('leaves an unlocated multi-dimensional array alone', () => { + // The restriction is about the location, not the shape. + const md = makeMultiDimArrayVariable('md', 'WORD', '', ['0..3', '0..3']) + expect(updateVariableValidation([md], { documentation: 'note' }, md).ok).toBe(true) + }) + it('catches a TYPE-ONLY edit that widens an already-located variable', () => { // No location in the patch: the variable stays at %MW0 and only its type // changes, so it silently grows over %MW1-%MW3 and swallows the neighbour. @@ -938,6 +987,14 @@ describe('createVariableValidation — auto-increment past occupied ranges', () expect(result.location).toBe('%MW3') }) + it('walks past a long occupied run without rescanning per step', () => { + // The spans are parsed once outside the loop; this pins the SEMANTICS of + // that change — 40 contiguous words taken, so a new one lands at %MW40. + const existing = Array.from({ length: 40 }, (_, i) => makeVariable(`v${i}`, 'WORD', `%MW${i}`)) + const result = createVariableValidation(existing, makeVariable('NewVar', 'WORD', '%MW0')) + expect(result.location).toBe('%MW40') + }) + it('leaves a location alone when nothing overlaps', () => { const existing = [makeArrayVariable('arr', 'WORD', '%MW10', '0..3')] const result = createVariableValidation(existing, makeVariable('NewVar', 'WORD', '%MW0')) diff --git a/src/frontend/store/slices/project/validation/variables.ts b/src/frontend/store/slices/project/validation/variables.ts index 8ed550b83..d67d976cd 100644 --- a/src/frontend/store/slices/project/validation/variables.ts +++ b/src/frontend/store/slices/project/validation/variables.ts @@ -2,6 +2,7 @@ import type { PLCVariable } from '../../../../../middleware/shared/ports/types' import { formatAddress, parseAddress, + type ParsedAddress, slotRangesOverlap, } from '../../../../../middleware/shared/utils/iec-address/registry' import { DISALLOWED_LOCATION_CLASSES } from '../../../../utils/generate-iec-string-to-variables' @@ -52,6 +53,28 @@ const checkIfGlobalVariableExists = (variables: PLCVariable[], name: string) => const slotsClaimedBy = (variable: PLCVariable): number => isArrayVariable(variable) ? getArrayTotalElements(variable) : 1 +/** + * A multi-dimensional array cannot be located. + * + * `AT %MW0 : ARRAY [0..3, 0..3] OF WORD` has no single linear run of addresses + * to occupy, and the compiler says exactly that: + * + * Located variable 'MD' at %MW0 cannot be placed: a 2-dimensional array has + * no single linear run of addresses to occupy. + * + * The editor has to refuse it too. `getArrayTotalElements` happily returns the + * product of every dimension (16 here), so without this the editor would place + * it, reserve 16 slots, and let the user discover the problem at build time — + * the same accept-here/reject-there divergence this whole change exists to + * close. + */ +const hasUnlocatableShape = (variableType: PLCVariable['type']): boolean => + variableType.definition === 'array' && (variableType.data?.dimensions.length ?? 0) > 1 + +/** Wording shared by both places that refuse a multi-dimensional located array. */ +const UNLOCATABLE_SHAPE_MESSAGE = + 'A multi-dimensional array cannot have a physical location: it has no single run of consecutive addresses to occupy. Use a one-dimensional array, or leave it unlocated.' + /** * Does `location` collide with a location another variable already holds? * @@ -360,9 +383,27 @@ const createVariableValidation = ( // span of any array already sitting there — landing one slot inside // a neighbouring array is the same collision as landing on its // first address (openplc-editor#565). + // + // The occupied spans are parsed ONCE, outside the loop. Calling + // `checkIfLocationExists` per iteration would re-scan every variable and + // re-run its address regex on each, and the walk steps one element slot at + // a time — placing an `ARRAY [0..999]` on a taken address would be ~1000 + // iterations x N variables x 2 regexes, synchronously inside the store's + // `produce`. Parsing up front makes each step a plain interval comparison. + const occupied: Array<{ parsed: ParsedAddress; slots: number }> = [] + for (const other of variables) { + const parsed = parseAddress(other.location) + if (parsed) occupied.push({ parsed, slots: slotsClaimedBy(other) }) + } + const collides = (address: string): boolean => { + const parsed = parseAddress(address) + if (!parsed) return false + return occupied.some((o) => slotRangesOverlap(parsed, slots, o.parsed, o.slots)) + } + let candidate = variableLocation let iterations = 0 - while (checkIfLocationExists(variables, candidate, slots) && iterations < MAX_AUTO_INCREMENT_ITERATIONS) { + while (collides(candidate) && iterations < MAX_AUTO_INCREMENT_ITERATIONS) { const next = incrementLocationByOne(candidate) if (!next || next === candidate) break // unknown type / no progress — bail candidate = next @@ -451,6 +492,15 @@ const updateVariableValidation = ( return response } + if (hasUnlocatableShape(effectiveType)) { + response = { + ok: false, + title: 'Location is not allowed.', + message: UNLOCATABLE_SHAPE_MESSAGE, + } + return response + } + // Exclude the variable being updated so re-setting its own // location (e.g. re-picking the same address to refresh a // renamed alias) doesn't trip the uniqueness check on itself. @@ -474,6 +524,13 @@ const updateVariableValidation = ( } if (dataToBeUpdated.type) { + if (variableToUpdate.location !== '' && hasUnlocatableShape(effectiveType)) { + // Reached from the array modal, which dispatches a type-only patch: the + // user turns a located 1-D array into a 2-D one and it stops having a + // linear run of addresses to sit on. + response = { ok: false, title: 'Location is not allowed.', message: UNLOCATABLE_SHAPE_MESSAGE } + return response + } if (!variableLocationValidation(variableToUpdate.location, effectiveAddressClass)) { response.data = { ...(response.data ? response.data : {}), location: '' } } else if ( From bb0ec3854d7c99e6f64198d6343fd8f8590d1131 Mon Sep 17 00:00:00 2001 From: JulioSergioFS Date: Wed, 2 Sep 2026 22:25:37 -0300 Subject: [PATCH 4/4] refactor(located): walk the linear index, dropping incrementLocationByOne Hoisting the occupied spans out of the auto-increment loop left `incrementLocationByOne` with two unreachable statements: `collides()` returns false for anything that does not parse, so the loop body only ever ran on a parseable address, and neither its `return null` nor the caller's `break` could be taken. Dead code that no test could honestly cover -- and openplc-web requires 100% statement coverage on this directory, so it would have failed there while passing here (the editor asks for 97). Walking the linear index instead removes the function altogether. Every size class advances identically once linearised, so `%QX0.7 -> %QX1.0` and `%IB0 -> %IB1` are both `+ 1` and the carry never has to be spelled out. The address is parsed once instead of being re-formatted and re-parsed per step. Behaviour is unchanged, including the alias case: a location that does not parse has nothing to step, so it is left as it stands. Co-Authored-By: Claude Opus 5 (1M context) --- .../slices/project/validation/variables.ts | 63 +++++++------------ 1 file changed, 21 insertions(+), 42 deletions(-) diff --git a/src/frontend/store/slices/project/validation/variables.ts b/src/frontend/store/slices/project/validation/variables.ts index d67d976cd..9f65277fd 100644 --- a/src/frontend/store/slices/project/validation/variables.ts +++ b/src/frontend/store/slices/project/validation/variables.ts @@ -311,34 +311,6 @@ const checkVariableName = (variables: PLCVariable[], variableName: string) => { * This is a validation to check if it is needed changing the name of a variable at creation. * If the variable exists change the variable name. **/ -/** - * Increment an IEC 61131-3 address by one slot, respecting the - * width of the variable's underlying type. For BOOL addresses - * (`%IX/%QX.`) the bit field wraps from .7 back to .0 - * with the byte index bumping by one; for word / dword / lword - * forms the numeric index after the prefix increments by one. - * - * Returns `null` when the type isn't recognised — the caller stops - * the auto-increment loop and falls back to whatever location it - * currently holds, so an unknown future IEC type can't produce an - * infinite loop here. - */ -const incrementLocationByOne = (location: string): string | null => { - // Derived from the ADDRESS, not from the variable's type. The address - // already states its size class (`%QX` bit, `%IB` byte, `%MW` word, ...), - // and every class advances the same way once linearised — `parseAddress` - // maps a bit address to `byte*8 + bit`, so `%QX0.7` steps to `%QX1.0` - // without the carry needing to be spelled out per class. - // - // This replaced a per-type switch that had two holes: it had no case for - // BYTE / SINT / USINT (so `%IB` / `%QB` / `%MB` returned null and the - // caller's search gave up, keeping a colliding location), and its BOOL case - // stripped only the `%QX` and `%IX` prefixes, so a memory bit `%MX0.0` fell - // through with its prefix intact and `parseInt` produced `%IXNaN.NaN`. - const parsed = parseAddress(location) - if (!parsed) return null - return formatAddress(parsed.cls, parsed.linear + 1) -} /** Safety bound on the auto-increment loop in `createVariableValidation`. * Picked well above any realistic project size (8 bits × N bytes = @@ -395,21 +367,28 @@ const createVariableValidation = ( const parsed = parseAddress(other.location) if (parsed) occupied.push({ parsed, slots: slotsClaimedBy(other) }) } - const collides = (address: string): boolean => { - const parsed = parseAddress(address) - if (!parsed) return false - return occupied.some((o) => slotRangesOverlap(parsed, slots, o.parsed, o.slots)) - } - - let candidate = variableLocation - let iterations = 0 - while (collides(candidate) && iterations < MAX_AUTO_INCREMENT_ITERATIONS) { - const next = incrementLocationByOne(candidate) - if (!next || next === candidate) break // unknown type / no progress — bail - candidate = next - iterations += 1 + // Walking the LINEAR index rather than re-formatting and re-parsing an + // address each step: every size class advances the same way once + // linearised, so `%QX0.7 -> %QX1.0` and `%IB0 -> %IB1` are one `+ 1` and + // the carry never has to be spelled out per class. + // + // A location that does not parse is an alias name. There is nothing to + // step, and inventing an address would be the wrong answer — resolving an + // alias collision means picking a different alias — so the location is + // left as it stands. + const start = parseAddress(variableLocation) + if (start) { + let linear = start.linear + let iterations = 0 + const collidesAt = (at: number): boolean => + occupied.some((o) => slotRangesOverlap({ cls: start.cls, linear: at }, slots, o.parsed, o.slots)) + + while (collidesAt(linear) && iterations < MAX_AUTO_INCREMENT_ITERATIONS) { + linear += 1 + iterations += 1 + } + response.location = formatAddress(start.cls, linear) } - response.location = candidate } return response }