Repository navigation
release: promote development to main for v0.6.7 - #236
Merged
Merged
Conversation
`HR_myData AT %MW60 : ARRAY [0..66] OF WORD` was rejected. The reason
given upstream was that MatIEC could not express a located non-elementary
type (openplc-editor#565) -- but that constraint left with MatIEC, and
nothing in this compiler stands in its way: the descriptor table is flat,
one {area, size, index, pointer} row per slot, so an array is N rows
rather than a new mechanism.
Analyzer: validate the ELEMENT type against the address size class, and
detect duplicates by slot-range overlap instead of address equality -- an
array occupies slotCount consecutive slots, so `x AT %MW60 : ARRAY[0..66]
OF WORD` collides with a plain `y AT %MW61 : WORD` even though the two
addresses differ. Reject the array shapes with no linear layout
(multi-dimensional, variable-length) with their own sentence, so none of
them falls through to the type-compatibility error and reports the
internal `__INLINE_ARRAY_<T>` spelling at the user.
Codegen: expand a located array into one descriptor per element, walking
the address forward by one slot each time (bit addresses continue across
the byte boundary).
Also fix the pointer initialiser, which located each descriptor by
variable NAME. With N descriptors sharing one name that resolved every
element to slot 0: element 0 bound N times, elements 1..N-1 left null.
Walk the array by position instead -- the index is right there.
Verified end to end: the issue's own declaration compiles to 67
descriptors over %MW60..%MW126, all 67 bound to distinct storage, and a
write through one descriptor lands in that element alone.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Regression found in review. The AST builder writes bounds onto the
TypeReference only for an INLINE `ARRAY [a..b] OF T`; a named type carries
none. `collectLocatedVar` read `decl.type.arrayDimensions` directly, while the
analyzer's `resolveLocatedShape` resolves named types through
`resolveArrayShapeByName` -- so the two passes disagreed:
TYPE Buf : ARRAY [0..9] OF WORD; END_TYPE
VAR_GLOBAL HR AT %MW60 : Buf; END_VAR
Semantics accepted it and correctly reserved ten slots (a second variable at
%MW65 was properly rejected), then codegen emitted ONE descriptor and called
`.raw_ptr()` on the array itself:
error: no member named 'raw_ptr' in 'IEC_ARRAY_1D<IECVar<unsigned short>, ArrayBounds<0, 9>>'
Worse than the state before this branch, where the analyzer refused the
declaration cleanly -- it failed in the C++ backend leaking internal type
names, which is the failure mode this branch set out to remove.
Codegen now resolves the shape by name when the declaration carries no bounds,
mirroring what the analyzer already does.
The existing test missed this because it asserted only `compile(source).success`
with no CONFIGURATION, so codegen's located path never ran. Replaced with one
that emits the path and checks what comes out: ten descriptors over
%MW60-%MW69, ten distinct pointers, and no `.raw_ptr()` on the aggregate.
Verified the generated C++ now passes `g++ -fsyntax-only`.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…613)
A block mixing a global and a local on its inputs failed to compile:
error: no matching function for call to 'ADD(int, strucpp::IEC_INT&)'
note: deduced conflicting types for parameter 'T'
('int' and 'strucpp::IECVar<int>')
read() unwrapped two layers -- GlobalVar -> IECVar -> raw scalar -- while its
own doc comment promised "the real IEC value type". The real IEC value type is
IECVar<T>; T is the underlying storage type.
Scalar globals were the only operand kind that did this. Locals emit as
IECVar<T>, literals as static_cast<IEC_INT>(...), and composite-global fields
come back from with_lock() as the field's own wrapper. So a scalar global was
the odd one out, and the std-lib templates that share one parameter across both
operands -- template<typename T> T ADD(T, T) -- could not deduce. operator T()
cannot rescue this: implicit conversions are not considered during template
argument deduction.
Returning V restores the invariant the rest of the library already assumes, so
no std-lib signature changes are needed and every affected function is fixed at
once: ADD SUB MUL DIV MOD, AND OR XOR, ATAN2. Located globals were affected too,
which made "read an input word, add a local offset" fail.
The alternative -- heterogeneous (A a, B b) signatures across the arithmetic and
bit families -- was rejected. Beyond being ~10 functions x 2 arities, it changes
the return type: T ADD(T,T) currently returns T(iec_unwrap(a) + iec_unwrap(b)),
where the explicit T(...) truncates back to the IEC width. An auto return would
let SINT + SINT promote to int and stop wrapping at 8 bits, silently changing
overflow behaviour of code that works today.
The return type is spelled V rather than auto so this cannot drift again.
Behaviour is unchanged elsewhere:
- Forcing: IECVar's copy constructor is deliberately forcing-collapsing
(value_{other.get()}, forced_{false}), so the snapshot carries the effective
forced value with the flag cleared -- what a detached read should return.
- Threading: still one lock acquisition; the copy is constructed while the
lock_guard is in scope, so the snapshot stays atomic.
- Size: identical object size on ATmega2560 at -Os, the tightest target we
ship. The wrapper copy is fully elided.
Tests are end-to-end through g++ because the defect lived entirely in C++
overload resolution -- ST -> C++ translation succeeded throughout, so a
codegen-string assertion would have stayed green. 16 of the 34 new tests fail
without this change; the 18 that pass regardless are the matched-operand
controls, kept so a future regression is attributed to the global path rather
than to the function. Three of them run real threads under STRUCPP_THREADED and
assert zero torn reads on a 64-bit global while writers hammer it.
Verified on hardware: uploaded to an AutomationDirect P1AM-100 and read back
over the Modbus RTU debugger -- ADD/SUB/MUL/AND over a global and a local give
10/4/21/16#000F, and forcing the global to 100 moves them to 103/97/300.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WHm2wRkKn33qqsN8QcUWSa
…-613)
Review follow-up. Returning `V` from `GlobalVar::read()` made a STRING global
hand back `IECStringVar<254>`, and that wrapper's special members were
`= default` — memberwise, so they copied `forced_` and `forced_value_`.
`IECVar` deliberately does not do that: a fresh copy starts unforced and
assignment routes through `set()`, precisely so generated code assigning every
scan cannot destroy a force the debugger is holding. `iec_wstring.hpp` had the
identical shape.
So `O = GS->read()` broke forcing two ways, measured on the same generated C++
for the body `o := gs;`:
- local `o` forced to 'FF', global unforced:
before o='FF' forced=1 (the force holds)
broken o='AB' forced=0 (cleared every scan)
- global forced to 'ZZ', then a write to the local:
before o='ZZ' forced=0, set('QQ') -> 'QQ'
broken o='ZZ' forced=1, set('QQ') -> 'ZZ' (write silently dropped)
STRING forcing is a first-class debug operation (force_string / unforce_string),
so this was reachable from the product.
Giving the two wrappers IECVar's semantics restores both rows exactly. The
copies reference the effective member directly rather than going through
`get()`, which returns `value_type` by value: the naive form cost an extra
258-byte temporary and pushed `run()`'s frame on ATmega2560 from 520 to 777
bytes. Referencing the member keeps it at 520, so this fix is free.
Also closes the coverage gaps that let this through 2356 green tests, and
corrects two claims:
- MUX WAS affected. It was listed as unaffected in the PR body and in
DOPE-613; `MUX(g, l, g, l)` gives one deduction error on development and
none here.
- The extensible arity was affected too: `ADD(g, l, l)`, same result.
- A located global (`AT %IW0`) is now in the matrix; it broke identically,
and is the commonest real shape — read an input word, add a local offset.
- STRING and WSTRING forcing are now pinned by the two tests above.
- SINT infix on globals is pinned. A scalar global now presents as
`IECVar<T>`, so infix binds the IECVar operators, which truncate to the
IEC width per step: `(ga + gb) / 2` for SINT 100s was 100 and is now -28.
That is the correct answer — two SINT locals already gave -28 and
IEC 61131-3 says an operation on SINT yields SINT — but it was an
unpinned behaviour change, so the test asserts globals and locals agree.
Discrimination checked independently: reverting only the string-wrapper change
fails exactly the 2 string tests; reverting only `iec_global.hpp` fails 20 of
the 40. Full suite 2362 passed / 94 files.
Re-verified on hardware with a CLI rebuilt from this branch: the P1AM-100 still
reads 10 / 4 / 21 / 16#000F / %IW0+3, and the STRING assignment runs on device.
String forcing itself could not be exercised there — the P1AM-100 debugger
returns ERROR_OUT_OF_MEMORY for a string force slot, on a path this change does
not touch — so that behaviour is covered by the unit tests.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WHm2wRkKn33qqsN8QcUWSa
…ompiles MUX is the claim that was wrong -- documented as unaffected, actually broken by the deduction failure. The shape matrix only checked that it compiles, which is weak for a selector-driven function: a wrong selector or a wrongly-bound operand still compiles. Asserts the selected value in all three mixed arrangements -- global selector with local data, local selector with global data, and both mixed -- which gives 20 / 70 / 20. On development the same program does not compile at all. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WHm2wRkKn33qqsN8QcUWSa
…cal-operand-mix fix(runtime): return the IEC value type from GlobalVar::read() (DOPE-613)
Forcing a STRING was refused on every target, reported as
`ERROR_OUT_OF_MEMORY`:
$ openplc-cli debug force Config0:gStr ZZ
error [target_error]: ERROR_OUT_OF_MEMORY
A string is length-prefixed on the wire -- `bytes[0]` is the character
(STRING) or code-unit (WSTRING) count, and force_string / write_string read
exactly that many. But the gate required `len >= type_ops[tag].size`, and for
the string tags that size is the PADDED field width the READ path emits:
DEBUG_STRING_WIDTH 127, DEBUG_WSTRING_WIDTH 253. Every caller sends a compact
payload -- the editor encodes `1 + text.length`, so "ZZ" is 3 bytes -- and
3 < 127 was refused regardless of value or target. Scalars agree on both
sides, which is why only strings were affected and this went unnoticed.
Validate the prefix instead. Too short is still refused, so a prefix that
overruns its own payload cannot make the read walk off the end, and `len`
stays a lower bound, so a caller that pads to the full width keeps working.
handle_write had the identical bug, so soft writes to a string -- the OPC-UA
write path and the retain restore walk -- were refused too. The rule now lives
in one shared validate_payload() used by both, because it was duplicated
inline and wrong in both copies.
Scope: this is the set path only. The read path stays fixed-width, so no
client change is needed and nothing has to be released in lockstep -- an
existing client works against a new device immediately. Variable-length READS
would additionally unbreak STRING debug on the 128-byte-frame AVRs
(ATmega328P/168/32U4/16U4) and WSTRING on baremetal, but that changes the wire
format for three independent readers -- this dispatch, the baremetal Modbus
slave in openplc-editor, and debug_handler.c in openplc-runtime -- plus the
client stride. Deliberately left out; tracked on the ticket.
Also not fixed here: status 0x82 is triple-booked -- STATUS_DATA_TOO_LARGE
here, ERROR_OUT_OF_MEMORY in the client, and a full write journal in
openplc-runtime. That mislabel is why this looked like a device memory limit
for so long. Tracked separately.
Tests drive the dispatch entry points directly, since the gate is the whole
defect and both transports hand it the same bytes. 5 of the 7 fail on
development; the 2 that pass regardless are the guards that the check still
refuses a lying prefix and still accepts a padded payload. Covered: compact
STRING and WSTRING (code units, not bytes, so 1 + 2n), no stale tail when a
short force follows a long one, a prefix that overruns its payload, a count
past the cap, a null payload, scalars unchanged, and force/unforce round trip
including that a write while forced is now accepted-then-dropped rather than
refused outright -- which is what made one assertion pass for the wrong reason
before handle_write was fixed too.
Verified on hardware: P1AM-100, CLI rebuilt from this branch with the client
encoder untouched. Forcing gStr to ZZ, then HELLOWORLD1, then back to AB all
take, with no stale characters; a forced STRING local holds FF across a 20 ms
scan that assigns over it every cycle; numerics unchanged at 10/4/21/16#000F.
Full suite: 2370 passed / 95 files.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WHm2wRkKn33qqsN8QcUWSa
Review feedback. The comment carried the ticket key and the history of what broke, which the commit message and PR body already hold. What a reader needs at that spot is the rule: scalars are fixed-width, strings are length-prefixed so the requirement is 1 + count (1 + 2 * count for WSTRING) rather than the padded read width, an over-cap count is refused rather than truncated, and `len` is a lower bound throughout. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WHm2wRkKn33qqsN8QcUWSa
…length-string-force fix(debug): validate a string payload by its length prefix (DOPE-614)
…E-618) Forum report: HOUR_OF_DT / MINUTE_OF_DT / SECOND_OF_DT returned 11 / 23 / 44 for DT#2026-09-02-15:36:55.360 instead of 15 / 36 / 55. `wrapTemporalArgForNumericConversion` scaled every temporal source to milliseconds and left DATE as a raw day count. That is right for TIME and TOD and wrong for DT and DATE. CODESYS -- the platform the IEC libraries we ship are written against -- holds DATE, DT and TOD in a 32-bit DWORD with a 1970-01-01 epoch, DATE and DT at SECONDS resolution and TOD at MILLISECONDS, with TIME in milliseconds and the 64-bit L variants in nanoseconds. Its own examples pin it: DT_TO_DINT(DT#2019-9-1-12:0:0.0) = 1567339200, DATE_TO_DINT(D#1970-1-2) = 86400, TOD_TO_DINT(TOD#12:0:0) = 43200000. So the library was correct and we were not. OSCAT divides DT by 86400 and DATE by 86400 expecting seconds, which is why 20 of its functions were wrong: the DT family aliased (ms since epoch overflows a DWORD every ~49.7 days, so the answer was not even a clean factor of 1000 out), and the DATE family returned constants -- DAY_OF_WEEK gave 4 for every date and YEAR_OF_DATE gave 1970 for every date. Both directions move together. OSCAT converts out and back inside a single expression -- DCF77 computes DWORD_TO_DT(DT_TO_DWORD(mez) - 7200) -- so fixing only the forward path would have left those callers worse off. The reverse scaling used to live in the runtime `TO_TIME(integer)` template, which cannot tell a millisecond count from a TIME value because every temporal type is the same C++ type there; it now happens in codegen, where the IEC type is still known, and the runtime TO_* are pure casts. The units live in one table, TEMPORAL_CONVERSION_UNITS, rather than a chain of conditionals, because the two directions have to agree on every row. Also corrects two tests that asserted the old contract rather than a bug: `DT_TO_LINT returns milliseconds since epoch` and `DATE_TO_UINT returns days since 1970-01-01 (identity, no scaling)`. The DATE one had been declared UINT, so it was quietly asserting a 16-bit truncation; it is now UDINT. The LTIME / LTOD / LDT / LDATE rows are correct per CODESYS but unreachable: the frontend does not accept those type names -- `VAR v : LDT;` is "Undefined type 'LDT'" -- and they appear nowhere outside codegen. The codegen branches for them were already dead before this change. A test pins their absence and is written to fail when the types are added, since the rows become reachable then and need coverage that cannot be written today. Verified: 5 of the 7 new tests fail on development; the 2 that pass regardless are the TOD and TIME controls, which were already correct. The DATE test uses five dates including a leap day and a year boundary, so the constant-output bug cannot pass it. Full suite 2379 passed / 96 files. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WHm2wRkKn33qqsN8QcUWSa
Five review findings, all in the change from e059c5b. Comment ownership in codegen.ts. The new table's doc block was inserted between NUMERIC_OR_BIT_CONVERSION_TARGETS and its own JSDoc, so the Set read as undocumented and the table appeared to be documented by a paragraph about the Set. The orphaned text was stale too -- it described the Set as gating "the temporal→ms scaling" for one function, when it now gates both directions and the units are no longer only milliseconds. Moved back and rewritten, including the fact that the Set and the table are disjoint, which is what makes the two directions mutually exclusive. The call-site comment still told the reader the argument is wrapped with `TIME_TO_MS` / `TOD_TO_MS` / `DT_TO_MS`. DT_TO_MS is no longer emitted here -- using it here WAS the bug -- but it still exists in the runtime, so a reader grepping for it would find it alive and conclude codegen still emits it. The comment also described only the forward direction while the call handles both. scaleConversionArg now branches on which side is temporal instead of comparing the returned string against its input. A row whose scaling is a legitimate no-op returns its argument untouched, so a value comparison cannot tell "this direction did not apply" from "it applied and had nothing to do". On that last point the review predicted a wrong number once the L types become declarable. That does not hold: NUMERIC_OR_BIT_CONVERSION_TARGETS and TEMPORAL_CONVERSION_UNITS are disjoint -- verified by extracting both sets -- so the reverse guard always rejects a forward conversion, and every no-op row is temporal-only. The fall-through was correct, not lucky. The refactor stands on readability: it asks the question directly rather than relying on two separate tables continuing to agree. DATE_FROM_SECONDS and DATE_FROM_NS truncated toward zero, so a pre-epoch input rounded the wrong way: DATE_FROM_SECONDS(-1) gave day 0 (1970-01-01) where the floor is day -1 (1969-12-31). DATE_OF_DT already spills that borrow explicitly, so the runtime answered the same question two ways depending on which helper you reached. Both now floor through a shared iec_floor_div. Pre-1970 DATE is outside CODESYS's domain, but these helpers take arbitrary user expressions via DINT_TO_DATE(x) rather than only literals, so nothing bounded the input. The day-in-nanoseconds constant had three private copies. One definition now lives in iec_types.hpp, which all three headers already include, and the existing names alias it so no call site changed. Raised as optional by the reviewer; taken because this change is the first to convert BETWEEN those types, which is when a divergence would produce wrong values rather than duplicated text. Tests: two for the pre-epoch floor -- one asserting the value, one asserting that DINT_TO_DATE and DATE_OF_DT agree with each other rather than with a hard-coded number, so they cannot drift apart again. Both fail with truncation restored ('0 -86400' vs '-86400 -86400', and DISAGREE vs agree). static_asserts pin the three day constants to the shared one. Full suite 2381 passed / 96 files. Re-verified on the runtime container after the refactor: same thirteen values, unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WHm2wRkKn33qqsN8QcUWSa
…emporal-units fix(codegen): use CODESYS units for temporal/integer conversions (DOPE-618)
…rays feat(located): support ARRAY at a physical address
…k them (RTOP-286) `analyze()` gates both type checking and semantic validation on an empty error list, and `Undefined type 'X'` is raised by the second of those. Since the language server merges every open document into one AST, a single type error in any document suppressed undefined-type reporting for the whole project. The OpenPLC editor synthesizes all data types into one document, so deleting a type that a POU still used published an empty diagnostics array and wiped the markers off every data type, including unrelated errors that were being reported a moment earlier. `validateTypeReferences` only reads the symbol tables built by the first pass; it does not depend on the type checker having run. Hoisting it ahead of the gate reports the undefined type itself rather than the cascade it causes, which is also the more useful diagnostic: a reference to a type that does not exist is the root cause, and `Condition must be a boolean or bit type, got FOO` is a symptom of it. The symbol table pass records its errors and keeps registering declarations, so running the check earlier cannot invent an undefined type when that pass itself failed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…-286) Hoisting `validateTypeReferences` ahead of the gates fixed the reported silence but created a wider one: its findings land in the same error list the two gates test, so a single undefined type skipped the type checker and every validator in `validateSemantics`. `x : Foo` alongside an unrelated `ok := neverDeclared` reported the undefined type and lost the undeclared variable, which `development` reports. The gates now ignore the errors the hoisted pass contributed, so every later pass runs exactly as it did before while an undefined type is always reported. The cascading type error that a missing type provokes comes back with them; suppressing it is not separable from suppressing everything else those passes find. The two tests added with the hoist only asserted that some undefined-type error was present, so neither caught this. The new case pins it directly. Found by review on #235. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…-type-before-typecheck fix(semantic): report undefined types before the type checker can mask them (RTOP-286)
Thiago-Pio-Autonomy
approved these changes
Sep 16, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Promotes
developmenttomainfor the v0.6.7 release.The merge is clean — I test-merged locally first.
mainis 12 commits "ahead" only in merge topology: every one is a previousdevelopment→mainpromotion merge, andmaincarries no content thatdevelopmentlacks.What this ships (11 commits since
v0.6.6)Located arrays —
feat(located): support ARRAY at a physical addressplusfix(located): resolve named ARRAY types in codegen, not just inline ones(#229). This is the half-shipped feature: the editors already acceptARRAY … AT %IW100, and the pinnedv0.6.6compiler rejects it. Releasing this is what unblocks the 4.2.12 editor release.RTOP-286 — undefined types are reported before the type checker can mask them, and the hoisted errors are kept out of the pass gates so a missing type no longer silences the type checker and all thirteen semantic validators project-wide. This is the upstream half of DOPE-553: with
v0.6.6, deleting a referenced data type publishes an empty diagnostics array for the whole synthesized datatypes document, wiping every marker.DOPE-613 —
GlobalVar::read()returns the IEC value type, and forcing semantics are kept onSTRING/WSTRINGwrappers.DOPE-614 — a string debug payload is validated by its length prefix.
DOPE-618 — CODESYS units for temporal and integer conversions.
After this merges
The release workflow triggers on a
v*tag, not on a push tomain, so merging this does not publish anything by itself. Cuttingv0.6.7onmainafterwards runsbuild-npm(npm version+npm pack, withprepackbuilding the browser-server into the tarball) andcreate-release, which publishes thestrucpp-0.6.7.tgzasset that the editors download.The
binary-versions.jsonbump in openplc-web and openplc-editor follows the tag — those PRs cannot go green before the release asset exists, sincepostinstallfetches the tarball by pin.Note
package.jsonstill reads0.6.4; that is expected, since the release workflow injects the version from the tag.🤖 Generated with Claude Code