Skip to content

fix(variables): allow a comma in a declaration's type so inline multi-dimensional arrays parse #1003

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
thiagoralves wants to merge 1 commit into
base: development
Choose a base branch
from
+117 −3
Open

fix(variables): allow a comma in a declaration's type so inline multi-dimensional arrays parse #1003

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 76 additions & 0 deletions src/frontend/utils/__tests__/generate-iec-string-to-variables.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,82 @@ describe('parseIecStringToVariables', () => {
expect(result[0].documentation).toBe('buffer')
})

// ---- multi-dimensional arrays declared inline ----
//
// The type group used to exclude the comma, so a 2D/3D array could only be
// declared by naming an ARRAY data type first; written inline it failed the
// whole POU with "invalid or unsupported characters".

it('parses a 2D ARRAY declared inline', () => {
const input = 'VAR\n m : ARRAY[0..1, 0..2] OF INT;\nEND_VAR'
const result = parseIecStringToVariables(input)

expect(result).toHaveLength(1)
expect(result[0].type.definition).toBe('array')
expect(result[0].type.value).toBe('ARRAY[0..1, 0..2] OF INT')
expect(result[0].type.data).toEqual({
baseType: { definition: 'base-type', value: 'INT' },
dimensions: [{ dimension: '0..1' }, { dimension: '0..2' }],
})
})

it('parses a 3D ARRAY declared inline', () => {
const input = 'VAR\n c : ARRAY[0..1, 0..1, 0..1] OF INT;\nEND_VAR'
const result = parseIecStringToVariables(input)

expect(result[0].type.data).toEqual({
baseType: { definition: 'base-type', value: 'INT' },
dimensions: [{ dimension: '0..1' }, { dimension: '0..1' }, { dimension: '0..1' }],
})
})

it('parses a multi-dimensional ARRAY with no space after the comma', () => {
const input = 'VAR\n m : ARRAY[0..1,0..1] OF INT;\nEND_VAR'
const result = parseIecStringToVariables(input)

expect(result[0].type.data?.dimensions).toEqual([{ dimension: '0..1' }, { dimension: '0..1' }])
})

it('parses a multi-dimensional ARRAY of a user-defined type', () => {
const input = 'VAR\n grid : ARRAY[0..1, 0..1] OF Point;\nEND_VAR'
const result = parseIecStringToVariables(input)

expect(result[0].type.data).toEqual({
baseType: { definition: 'user-data-type', value: 'Point' },
dimensions: [{ dimension: '0..1' }, { dimension: '0..1' }],
})
})

it('keeps the initial value when a multi-dimensional ARRAY has one', () => {
const input = 'VAR\n m : ARRAY[0..1, 0..2] OF INT := [[1,2,3],[4,5,6]];\nEND_VAR'
const result = parseIecStringToVariables(input)

expect(result[0].type.value).toBe('ARRAY[0..1, 0..2] OF INT')
expect(result[0].initialValue).toBe('[[1,2,3],[4,5,6]]')
})

it('parses a multi-dimensional ARRAY in the alternate located format', () => {
const input = 'VAR_GLOBAL\n m AT %MW0 : ARRAY[0..1, 0..1] OF INT;\nEND_VAR'
const result = parseIecStringToVariables(input)

expect(result[0].location).toBe('%MW0')
expect(result[0].type.data?.dimensions).toHaveLength(2)
})

it('still rejects a multi-name declaration', () => {
// Allowing the comma in the type must not make `a, b : INT;` parse — the
// name group is a single identifier followed by the colon.
const input = 'VAR\n a, b : INT;\nEND_VAR'
expect(() => parseIecStringToVariables(input)).toThrow(/Syntax error on line 2/)
})

it('no longer blames a comma for an unrelated syntax error', () => {
// A comma is legal now, so the guessed reason must fall through instead of
// reporting "invalid or unsupported characters".
const input = 'VAR\n m ARRAY[0..1, 0..1] OF INT;\nEND_VAR'
expect(() => parseIecStringToVariables(input)).toThrow(/missing colon/)
})

// ---- alternate format (line 115) ----

it('parses the alternate format: name AT location : type', () => {
Expand Down
23 changes: 23 additions & 0 deletions src/frontend/utils/__tests__/generate-iec-variables-to-string.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import type { PLCVariable } from '../../../middleware/shared/ports/types'
import { parseIecStringToVariables } from '../generate-iec-string-to-variables'
import { generateIecVariablesToString, getIecVariableLineMap } from '../generate-iec-variables-to-string'

const makeVariable = (overrides: Partial<PLCVariable> & Pick<PLCVariable, 'name'>): PLCVariable => ({
Expand Down Expand Up @@ -212,4 +213,26 @@ describe('getIecVariableLineMap', () => {
const map = getIecVariableLineMap([makeVariable({ name: 'OnlyOne', class: 'input' })])
expect(map.get('OnlyOne')?.column).toBe(5)
})

// The variables table and the code view are two views of the same data, so
// text -> variables -> text has to be byte-stable. Multi-dimensional array
// types are the interesting case: the parser splits the bounds into
// `dimensions` but keeps the full type string in `type.value`, which is what
// this serializer emits.
it('round-trips inline multi-dimensional array declarations unchanged', () => {
const input = [
' VAR',
' m : ARRAY[0..1, 0..2] OF INT := [[1,2,3],[4,5,6]];',
' c : ARRAY[0..1, 0..1, 0..1] OF INT := [1,2,3,4,5,6,7,8];',
' g : ARRAY[0..1, 0..1] OF Point;',
' END_VAR',
].join('\n')

const vars = parseIecStringToVariables(input)
expect(vars).toHaveLength(3)

const out = generateIecVariablesToString(vars)
expect(out).toBe(input)
expect(parseIecStringToVariables(out)).toEqual(vars)
})
})
21 changes: 18 additions & 3 deletions src/frontend/utils/generate-iec-string-to-variables.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
import type { LibraryState } from '../../middleware/shared/ports/library-types'
import { baseTypeSchema } from '../../middleware/shared/ports/plc-schemas'
import type { PLCDataType, PLCPou, PLCVariable } from '../../middleware/shared/ports/types'
Expand Down Expand Up @@ -27,22 +27,37 @@
'temp',
]

// The type group accepts a comma so a multi-dimensional array can be declared
// inline: `m : ARRAY[0..1, 0..2] OF INT;`. `parseArrayType` below has always
// split multi-dimensional bounds, and the data-type text parser
// (`PLC/data-type-text-parser.ts`) already allows the comma — without it here,
// the only way to declare a 2D/3D array was to name an ARRAY data type first,
// and writing it inline failed the whole POU with "invalid or unsupported
// characters".
//
// The group stays lazy and is bounded by the following `AT` / `:=` / `;`, and a
// comma is never valid between a declaration's name and its type, so this can't

@marconetsf marconetsf Aug 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment's conclusion — "a comma is never valid between a declaration's name and its type, so this can't swallow anything it didn't before" — is only about the name side; the comma is now also accepted inside a NON-array type, where nothing validates the result.

Verified with both regexes: x : INT, DINT; previously threw and now matches with type = 'INT, DINT', and since baseTypeSchema fails and _dataTypes is unused, it becomes {definition:'user-data-type', value:'INT, DINT'} — persisted, shown in the type cell as a nonexistent type, and emitted verbatim by getTypeAsText as x : INT, DINT; into the generated ST (invalid ST → strucpp failure). Same for x : INT,; → value:'INT,'.

The precedent cited in this very comment guards against exactly this: data-type-text-parser.ts also allows the comma, but buildFieldType rejects a non-array/non-base type that fails identifierRegex. Mirror that guard here rather than accepting any comma-bearing string as a user data type.

// swallow anything it didn't before. Note this does NOT enable multi-name
// declarations (`a, b : INT;`) — `name` is a single `\w+` followed by `:`.

// Primary format: name : type AT location := initialValue ; (* documentation *)
const lineRegex =
// eslint-disable-next-line no-useless-escape
/^\s*(?<name>\w+)\s*:\s*(?<type>[\w\s\[\]\.]+?)(?:\s+AT\s+(?<location>[\w\d\._%]+))?\s*(?::=\s*(?<initialValue>[^;]+?))?\s*;\s*(?:\(\*\s*(?<documentation>.*?)\s*\*\))?$/
/^\s*(?<name>\w+)\s*:\s*(?<type>[\w\s\[\],\.]+?)(?:\s+AT\s+(?<location>[\w\d\._%]+))?\s*(?::=\s*(?<initialValue>[^;]+?))?\s*;\s*(?:\(\*\s*(?<documentation>.*?)\s*\*\))?$/

@coderabbitai coderabbitai Bot Aug 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Reject commas outside inline ARRAY bounds.

Line 46 accepts a comma in every type expression. value : INT, BOOL; now matches, fails parseArrayType, and becomes the user-defined type INT, BOOL. The serializer then emits invalid IEC text.

Restrict comma support to valid ARRAY[...] OF ... expressions, or reject a comma when parseArrayType(parsedType) returns null. Add a regression test for value : INT, BOOL;.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/frontend/utils/generate-iec-string-to-variables.ts` at line 46, Update
the type parsing in the regex used by generateIecStringToVariables so commas are
accepted only within valid inline ARRAY bounds; otherwise reject declarations
such as value : INT, BOOL; instead of treating them as user-defined types.
Ensure parseArrayType returning null for a comma-containing type causes the
declaration to be rejected, and add a regression test covering this input.

@marconetsf marconetsf Aug 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Widening (?<type>[\w\s\[\],\.]+?) also admits malformed bound lists, and parseArrayType's dimensionsStr.split(',') turns them into empty dimensions instead of an error. Verified by running the old and new regexes side by side:

input before after
m : ARRAY[0..1,] OF INT; throws "invalid or unsupported characters" dimensions: [{dimension:'0..1'},{dimension:''}]
m : ARRAY[,] OF INT; throws [{dimension:''},{dimension:''}]
m : ARRAY[0..1,,0..2] OF INT; throws [{dimension:'0..1'},{dimension:''},{dimension:'0..2'}]

Nothing rejects it — reconcileVariablesText writes parsed straight into pou.interface.variables, and plc-schemas.ts has no dimension schema — so it is persisted. Downstream: getTypeAsText emits ARRAY [0..1,] OF INT into the generated ST (a compile error in generated code, with no editor diagnostic), getArrayTotalElements returns 0 (the Python-extension struct becomes uint8_t m[0]; and the copy loops become no-ops), and opening/saving the array modal silently drops the empty entry, converting the 2D array to 1D.

Reject empty/blank bounds — e.g. have parseArrayType return null when any dimensionParts entry is empty (the GUI already refuses this via arrayValidation) — and add negative tests for the trailing and doubled comma.


// Alternate format: name AT location : type := initialValue ; (* documentation *)
// This format is used by some IEC 61131-3 tools and older versions of OpenPLC Editor
const alternateLineRegex =
// eslint-disable-next-line no-useless-escape
/^\s*(?<name>\w+)\s+AT\s+(?<location>[\w\d\._%]+)\s*:\s*(?<type>[\w\s\[\]\.]+?)\s*(?::=\s*(?<initialValue>[^;]+?))?\s*;\s*(?:\(\*\s*(?<documentation>.*?)\s*\*\))?$/
/^\s*(?<name>\w+)\s+AT\s+(?<location>[\w\d\._%]+)\s*:\s*(?<type>[\w\s\[\],\.]+?)\s*(?::=\s*(?<initialValue>[^;]+?))?\s*;\s*(?:\(\*\s*(?<documentation>.*?)\s*\*\))?$/

const guessErrorReason = (line: string): string => {
if (!line.includes(';')) return 'missing semicolon (;) at the end of the declaration'
if (!line.includes(':')) return 'missing colon (:) between name and type'
// Comma is legal — multi-dimensional array bounds and comma-separated initial
// values both use it — so it must not be reported as an unsupported character.
// eslint-disable-next-line no-useless-escape
if (/[^A-Za-z0-9_\s:;=%()/*\-.\[\]]/.test(line)) return 'invalid or unsupported characters'
if (/[^A-Za-z0-9_\s:;=%()/*\-.,\[\]]/.test(line)) return 'invalid or unsupported characters'
return 'unrecognized declaration format'
}

Expand Down
Loading