fix(variables): allow a comma in a declaration's type so inline multi-dimensional arrays parse - #1003
fix(variables): allow a comma in a declaration's type so inline multi-dimensional arrays parse#1003thiagoralves wants to merge 1 commit into
Conversation
…-dimensional arrays parse `m : ARRAY[0..1, 0..2] OF INT;` written in a POU's variables text failed the whole POU: the type capture group excluded the comma, so no line matched and `guessErrorReason` — whose allowed-character set also omitted the comma — blamed "invalid or unsupported characters". The only way to declare a 2D/3D array was to name an ARRAY data type first and reference that. Nothing downstream needed changing. `parseArrayType` has always split comma-separated bounds into multiple `dimensions`, and the reverse serializer emits `type.value`, which holds the full type string — so the round trip (text → variables → text) is unaffected. The data-type text parser (`PLC/data-type-text-parser.ts`) already allowed the comma; this brings the variable parser in line with it. Scope of the change is narrow by construction: the type group stays lazy and is still bounded by the following `AT` / `:=` / `;`, and a comma is never valid between a declaration's name and its type. In particular this does NOT start accepting multi-name declarations (`a, b : INT;`) — the name group is a single identifier that must be followed by the colon — and that stays covered by a test. Tests: inline 2D and 3D, with and without a space after the comma, of a base and of a user-defined type, with an initial value, in the alternate located (`name AT loc : type`) format, plus the two negative cases — multi-name still rejected, and an unrelated error no longer misreported as a bad character. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Companion PR with the identical fix in openplc-web: Autonomy-Logic/openplc-web#662 |
WalkthroughThe IEC variable parser now accepts inline multidimensional ChangesIEC array parsing
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/frontend/utils/generate-iec-string-to-variables.ts`:
- 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.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a7e47349-b219-4956-b119-2aee0b7ef270
📒 Files selected for processing (3)
src/frontend/utils/__tests__/generate-iec-string-to-variables.test.tssrc/frontend/utils/__tests__/generate-iec-variables-to-string.test.tssrc/frontend/utils/generate-iec-string-to-variables.ts
| 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*\*\))?$/ |
There was a problem hiding this comment.
🎯 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.
| 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*\*\))?$/ |
There was a problem hiding this comment.
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.
| // 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 |
There was a problem hiding this comment.
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.
2 participants
m : ARRAY[0..1, 0..2] OF INT;written in a POU's variables text failed the whole POU, not just that line:The type capture group in
generate-iec-string-to-variables.tsexcluded the comma, so no line matched — andguessErrorReason, whose allowed-character set also omitted the comma, then blamed "invalid or unsupported characters". The only way to declare a 2D/3D array was to define a named ARRAY data type first and reference that.Why the change is this small
Nothing downstream needed touching:
parseArrayTypehas always split comma-separated bounds into multipledimensionstype.value, which holds the full type string, sotext → variables → textis byte-stablePLC/data-type-text-parser.ts) already allowed the comma — this brings the variable parser in line with a sibling that got it rightScope is bounded by construction
The type group stays lazy and is still bounded by the following
AT/:=/;, and a comma is never valid between a declaration's name and its type. In particular this does not start accepting multi-name declarations (a, b : INT;) — the name group is a single identifier that must be followed by the colon — and a test pins that.Tests
13 new. Inline 2D and 3D; with and without a space after the comma; of a base type and of a user-defined type; with an initial value; in the alternate located (
name AT loc : type) format. Plus the negative cases: multi-name still rejected, and an unrelated syntax error no longer misreported as a bad character. And a round-trip test in the serializer's own file, since the variables table and the code view are two views of the same data.src/frontend: 4052 passing (179 files).Notes for the reviewer
[[1,2,3],[4,5,6]], 3D arrays) are a separate strucpp change: feat: IEC 61131-3 structure and array initialization (forum report), plus six initialization fixes STruCpp#205. This PR is what lets those types be declared inline; the two are independent but complementary.Validated end-to-end on a real project through parse → transpile → strucpp → g++ → run, including inline 2D/3D arrays of base types, of a STRUCT, and of a function block.
🤖 Generated with Claude Code
Summary by CodeRabbit
ARRAYdeclarations, including spaced and unspaced dimensions.