fix(transpiler): resolve polymorphic TO_<TYPE> conversions to concrete call names#945
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe ST transpiler now resolves supported ChangesPolymorphic conversion emission
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant GraphicalPou as generateGraphicalPou
participant LdWalker as emitLdBody
participant Resolver as resolveConversionFunctionName
GraphicalPou->>LdWalker: emit conversion block
LdWalker->>Resolver: resolve TO_<TYPE> from source type
Resolver-->>LdWalker: concrete conversion name or null
LdWalker-->>GraphicalPou: emit ST call
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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/backend/shared/transpilers/st-transpiler/walker/__tests__/ld.test.ts`:
- Around line 83-221: Add coverage for the uncovered branches: in
src/backend/shared/transpilers/st-transpiler/walker/__tests__/ld.test.ts#L83-L221,
add a conversion case whose sole input is wired to another block’s output to
exercise resolveConversionCallName upstream/variableName fallbacks; in
src/backend/shared/transpilers/st-transpiler/helpers/block-library.ts#L103-L125,
add a test for an unknown shorthand such as TO_FOO covering
resolveConversionFunctionName’s target-membership guard; in
src/backend/shared/transpilers/st-transpiler/emit/pou-graphical.ts#L146-L166,
add a derived/non-base-type interface or global variable case covering the false
branch in buildVariableTypeIndex.
🪄 Autofix (Beta)
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
Run ID: d68c012a-e5a0-4930-b5bc-ca45bc90ae37
📒 Files selected for processing (6)
src/backend/shared/transpilers/st-transpiler/emit/__tests__/pou-graphical.test.tssrc/backend/shared/transpilers/st-transpiler/emit/pou-graphical.tssrc/backend/shared/transpilers/st-transpiler/helpers/block-library.tssrc/backend/shared/transpilers/st-transpiler/walker/__tests__/ld.test.tssrc/backend/shared/transpilers/st-transpiler/walker/fbd.tssrc/backend/shared/transpilers/st-transpiler/walker/ld.ts
…e call names (Autonomy-Logic#944) Blocks like TO_INT/TO_UINT/TO_REAL are placed on the FBD/LD canvas with the generic IEC 61131-3 conversion shorthand as their type name, but that shorthand is not itself a valid ST function -- only the fully qualified <SRC>_TO_<DST> family exists (std_block_catalog.json never has a bare TO_INT entry). The JSON->ST walker emitted the shorthand verbatim as the call name, producing e.g. `TO_INT(O_R)` for a REAL source, which matiec/strucpp reject as an undefined function -- the exact "';' missing at the end of statement" parse error reported in Autonomy-Logic#944 for `TO_INT(O_R)`. PR Autonomy-Logic#854 already fixed the sibling defect where the synthesized output temp's *declared type* stayed ANY for these blocks, but explicitly left the call-name resolution as future work ("computeConnectionTypes port"). This closes that specific gap: emitFunctionCall (walker/ld.ts) now resolves the block's single wired input to its declared type (from a new project/POU variable-type index built in pou-graphical.ts) and substitutes the concrete <SRC>_TO_<DST> name when the catalog has a matching entry, falling back to the original shorthand otherwise so an unresolvable case still surfaces the same "undefined function" error instead of a different wrong one. Scope: this fixes the in-process JSON->ST transpiler (backend/shared/transpilers/st-transpiler/), which is currently opt-in via OPENPLC_USE_NEW_TRANSPILER and is the path PR Autonomy-Logic#854 already targeted. The default legacy pipeline (XmlGenerator -> external xml2st subprocess) has the identical defect inherited from the original Python generator (xml2st's own PLCGenerator.py also passes the block's raw type name through unresolved) and needs a separate fix -- old-editor/fbd-xml.ts has no project-wide variable-type context available at its call site today, so resolving it there needs its own plumbing change. Flagging that as follow-up scope rather than bundling a larger, untested change into this fix. Verification: - New tests reproduce the exact issue Autonomy-Logic#944 scenario (REAL O_R -> TO_INT -> INT output) at both the walker level (ld.test.ts) and the full generateGraphicalPou level (pou-graphical.test.ts); each fails against the pre-fix code (bare TO_INT(O_R)) and passes with the fix (REAL_TO_INT(O_R)) -- confirmed by running both ways. - Added coverage for: unconnected input, unknown source type, no matching catalog entry, non-conversion blocks (ADD-style), sibling conversions (TO_DINT from BOOL), multi-input blocks, and global vs. local variable shadowing. - Full suite: 243 suites / 5216 tests, 0 failures, 0 regressions. - tsc --noEmit and eslint clean on all changed files. Could not run the Electron GUI or the external matiec/strucpp compilers in this environment to compile-verify the generated ST end-to-end; verification is at the transpiler-output level (the generated ST text matches the exact fully-qualified call the catalog and STruC++'s own test suite confirm is valid, e.g. REAL_TO_INT). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
dcaaa82 to
8d2868b
Compare
…olymorphic-conversion-944 # Conflicts: # src/backend/shared/transpilers/st-transpiler/emit/pou-graphical.ts # src/backend/shared/transpilers/st-transpiler/walker/fbd.ts # src/backend/shared/transpilers/st-transpiler/walker/ld.ts
|
Merged current |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/backend/shared/transpilers/st-transpiler/helpers/block-library.ts (1)
43-75: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftSatisfy the mandatory 100% coverage gate for new shared code.
This PR adds branch-heavy code here, as well as in
pou-graphical.tsandld.ts, while the reported repository-wide Jest coverage thresholds remain unmet. Add coverage for malformed variants, global/local shadowing, conversion-pair exits, and LD fallback paths before merging.As per coding guidelines,
src/backend/shared/must maintain 100% test coverage when new code is added.Also applies to: 106-162
🤖 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/backend/shared/transpilers/st-transpiler/helpers/block-library.ts` around lines 43 - 75, Add tests covering the new shared transpiler branches, including malformed variants and variable records in blockInfosFromVariant, global/local shadowing, conversion-pair exits, and LD fallback paths in the related pou-graphical.ts and ld.ts logic. Ensure every newly added branch in src/backend/shared reaches 100% coverage while preserving existing behavior.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@src/backend/shared/transpilers/st-transpiler/helpers/block-library.ts`:
- Around line 43-75: Add tests covering the new shared transpiler branches,
including malformed variants and variable records in blockInfosFromVariant,
global/local shadowing, conversion-pair exits, and LD fallback paths in the
related pou-graphical.ts and ld.ts logic. Ensure every newly added branch in
src/backend/shared reaches 100% coverage while preserving existing behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9b5eaa64-45b0-4241-8b65-986d232fad7b
📒 Files selected for processing (6)
src/backend/shared/transpilers/st-transpiler/emit/__tests__/pou-graphical.test.tssrc/backend/shared/transpilers/st-transpiler/emit/pou-graphical.tssrc/backend/shared/transpilers/st-transpiler/helpers/__tests__/block-library.test.tssrc/backend/shared/transpilers/st-transpiler/helpers/block-library.tssrc/backend/shared/transpilers/st-transpiler/walker/__tests__/ld.test.tssrc/backend/shared/transpilers/st-transpiler/walker/ld.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/backend/shared/transpilers/st-transpiler/helpers/tests/block-library.test.ts
- src/backend/shared/transpilers/st-transpiler/emit/tests/pou-graphical.test.ts
- src/backend/shared/transpilers/st-transpiler/walker/tests/ld.test.ts
|
Added the requested edge-case coverage in 9202033: malformed block variants/variable records, legacy |
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/backend/shared/transpilers/st-transpiler/helpers/__tests__/block-library.test.ts`:
- Around line 3-47: Add tests for the uncovered valid default and
function-variant branches in blockInfosFromVariant, including variants that omit
optional fields and use the function variant type. Assert the normalized output
and rerun coverage until block-library.ts reaches 100% branch coverage.
🪄 Autofix (Beta)
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
Run ID: b7a2fa3e-68db-4781-8d74-ddb4b0ecbd04
📒 Files selected for processing (3)
src/backend/shared/transpilers/st-transpiler/emit/__tests__/pou-graphical.test.tssrc/backend/shared/transpilers/st-transpiler/helpers/__tests__/block-library.test.tssrc/backend/shared/transpilers/st-transpiler/walker/__tests__/ld.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/backend/shared/transpilers/st-transpiler/walker/tests/ld.test.ts
|
synced the branch with latest development, no conflicts. transpiler + backend tests all pass |
1 similar comment
|
synced the branch with latest development, no conflicts. transpiler + backend tests all pass |
Pull request info
References
This PR resolves #944.
Link to Jira task
N/A — this is a community GitHub issue.
Description of the changes proposed
Graphical
TO_<TYPE>blocks were emitted verbatim as Structured Text calls. For example, a REAL input wired intoTO_INTproducedTO_INT(O_R), even though the standard catalog and both supported compilers use the concreteREAL_TO_INT(O_R)form.This change centralizes the supported conversion targets and resolves a single-input conversion call to
<SOURCE>_TO_<DESTINATION>when the input is a declared base-type local or global variable and the concrete function exists in the standard block catalog. The variable-type index is threaded through both LD and FBD emission. Unknown, unconnected, multi-input, derived-type, and block-output cases retain the previous shorthand instead of guessing a source type.The regression suite covers the issue's exact REAL-to-INT case, local/global shadowing, sibling conversions, unknown
TO_FOOtargets, another block as the upstream source, derived variables, unconnected and unknown inputs, unsupported source types, non-conversion functions, and defensive multi-input handling.Verification completed on the rebased branch:
block-library.ts; test files are ignored by the repository lint configuration).🤖 Generated with Claude Code and updated after review with OpenAI Codex.
DOD checklist
Summary by CodeRabbit
TO_<TYPE>conversion blocks by emitting specific conversion calls when the wired source type can be determined.