diff --git a/src/frontend/store/__tests__/project-validation-variables.test.ts b/src/frontend/store/__tests__/project-validation-variables.test.ts index 9fbb4ffb8..e7f675e54 100644 --- a/src/frontend/store/__tests__/project-validation-variables.test.ts +++ b/src/frontend/store/__tests__/project-validation-variables.test.ts @@ -29,6 +29,53 @@ 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, + 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: '', + } +} + // =========================================================================== // extractNumberAtEnd // =========================================================================== @@ -337,12 +384,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 @@ -767,3 +837,167 @@ 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('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. + // 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')] + 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('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')) + 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..9f65277fd 100644 --- a/src/frontend/store/slices/project/validation/variables.ts +++ b/src/frontend/store/slices/project/validation/variables.ts @@ -1,13 +1,19 @@ 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' 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' import type { ProjectResponse } from '../types' /** @@ -35,16 +41,69 @@ 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. + * + * 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 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: * - * `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. + * 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 checkIfLocationExists = (variables: PLCVariable[], location: string, exclude?: PLCVariable) => { - return variables.some((variable) => variable !== exclude && variable.location === location) +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? + * + * 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 +176,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 +257,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.` } } @@ -228,82 +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, 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 - } -} /** Safety bound on the auto-increment loop in `createVariableValidation`. * Picked well above any realistic project size (8 bits × N bytes = @@ -331,25 +338,57 @@ 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)) - let candidate = variableLocation - let iterations = 0 - while (inUse.has(candidate) && iterations < MAX_AUTO_INCREMENT_ITERATIONS) { - const next = incrementLocationByOne(candidate, variable.type.value) - if (!next || next === candidate) break // unknown type / no progress — bail - candidate = next - iterations += 1 + // 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). + // + // 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) }) + } + // 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 } @@ -407,6 +446,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 @@ -424,10 +471,19 @@ 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. - if (checkIfLocationExists(variables, location, variableToUpdate)) { + if (checkIfLocationExists(variables, location, effectiveSlots, variableToUpdate)) { response = { ok: false, title: 'Location already exists', @@ -436,19 +492,40 @@ const updateVariableValidation = ( return response } - if (!variableLocationValidation(location, variableToUpdate.type.value)) { + if (!variableLocationValidation(location, effectiveAddressClass)) { 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(effectiveAddressClass)}`, } return response } } if (dataToBeUpdated.type) { - if (!variableLocationValidation(variableToUpdate.location, dataToBeUpdated.type.value)) { + 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 ( + // 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' } 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,