fix(lowering): resolve array ctor bounds through the index (#1897) - #1898
Open
ghaith wants to merge 3 commits into
Open
fix(lowering): resolve array ctor bounds through the index (#1897)#1898ghaith wants to merge 3 commits into
ghaith wants to merge 3 commits into
Conversation
Problem: An array whose bound names a constant of the declaring POU
(`ARRAY[1..five]`) failed to compile when its element type needs construction,
a sized string or a struct for example:
error: cannot generate call statement for ReferenceExpr {
kind: Member(Identifier { name: "DINT_GREATER" }), base: None }
error occurred while generating initialization code for type '__mainProg_arr'
The generated element construction loop copied the bound expression out of the
declaration. That constructor is a POU of its own, so a name that belongs to the
declaring POU does not resolve inside it, the operand was left without a type,
and the comparison was then annotated as a call to a `DINT_GREATER` function
that does not exist. The same bound worked as a global constant, because a global
is in scope everywhere.
Solution: Take the bounds from the index, which holds them already
const-evaluated, and emit them as literals. An undetermined bound is rejected
explicitly rather than through `as_int_value`, which reports it as the pointer
size and would otherwise produce a loop over the wrong range.
Regression from #1831, which introduced the element construction loop.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A constant declared in a POU sizes a string exactly like the literal it stands for, on its own (`STRING[five]`, `WSTRING[five]`), inside an expression (`STRING[five * 2 + 1]`), and as both the bound and the element length of an array (`ARRAY[1..count] OF STRING[len]`). The tests compare the generated field layout against the same declaration written with literals, rather than snapshotting IR, so they assert the sizes themselves. Only the global form was covered before, by `variable_length_strings_using_constants_can_be_created`. These pass without the array constructor fix as well: string sizing was never affected by it. They are here because the local and the global form turned out to be handled differently in a neighbouring position, and this pins the one that was already correct. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Build Artifacts🪟 Windows
From workflow run 🐧 Linux
From workflow run |
`AstStatement` was only needed for the `RangeStatement` destructuring of the declaration bounds, which the previous commit replaced with the const-evaluated bounds from the index. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
1 participant
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.
Backport of #1897 to
release/1.0.x. Fixes PRG-4730 on the v1 line.Why it is wanted on v1
The bug is present on this branch. An array whose bound names a constant of the declaring POU
fails to compile when the element type needs construction:
Reproduced on
release/1.0.xbefore porting. Note that #1831, which introduced the loop onmaster, is not on this branch; an equivalent element construction loop is, so the branch is
affected independently rather than through that commit.
See #1897 for the full cause and reasoning. In short: the loop bounds were copied out of the
declaration into the generated constructor, which is a POU of its own where a member of the
declaring POU does not resolve. The bounds now come from the index, already const-evaluated,
and are emitted as literals.
Port risk
Low. Both commits cherry-picked with no conflicts.
Verified on this branch on Windows: the repro compiles, and 2582 + 91 + 50 lib, 170 lowering,
352 correctness tests pass, with
fmtclean. The lit tests were not run locally (litneedsPython and that suite runs on Linux in CI).
What reviewers should weigh
was already a literal produces the same loop as before.
🤖 Generated with Claude Code