fix: extended function block body calls #1545

volsa · 2025-11-10T16:31:13Z

Fixes an issue where calling the body of an extended function block would fail. Essentially the compiler today does not resolve inherited variables in a function block call statement context, which results in an error in the validation.

This commit correctly annotates them and changes the codegen logic to access inherited variables by GEPing directly, i.e. not inbound, onto the underlying struct. This is achieved by updating the StatementAnnotation:Argument variant slightly to also include metadata to find out at which depth and position a given argument is declared in. The depth is then used to GEP n times to fetch the underlying struct.

Note: This is currently only supported for named arguments. For positional arguments the logic is untouched.

A minimal reproducible example would be

FUNCTION_BLOCK foo
    VAR_INPUT
        x : DINT;
    END_VAR
END_FUNCTION_BLOCK

FUNCTION_BLOCK bar EXTENDS foo
    VAR_INPUT
        y : DINT;
    END_VAR
END_FUNCTION_BLOCK

FUNCTION main
    VAR
        instanceBar: bar;
    END_VAR

    instanceBar(x := 5, y := 10);
END_FUNCTION

which should return

error[E032]: this POU takes 1 argument but 2 arguments were supplied
   ┌─ target/demo.st:18:5
   │
18 │     instanceBar(x := 5, y := 10);
   │     ^^^^^^^^^^^ this POU takes 1 argument but 2 arguments were supplied

error[E089]: Invalid call parameters
   ┌─ target/demo.st:18:17
   │
18 │     instanceBar(x := 5, y := 10);
   │                 ^^^^^^ Invalid call parameters

error[E048]: Could not resolve reference to x
   ┌─ target/demo.st:18:17
   │
18 │     instanceBar(x := 5, y := 10);
   │                 ^ Could not resolve reference to x

codecov · 2025-11-13T14:18:04Z

Codecov Report

❌ Patch coverage is 98.55072% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 95.02%. Comparing base (ce68921) to head (70a80f3).
⚠️ Report is 36 commits behind head on master.

Files with missing lines	Patch %	Lines
src/codegen/generators/expression_generator.rs	96.07%	2 Missing ⚠️
src/builtins.rs	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1545      +/-   ##
==========================================
+ Coverage   94.03%   95.02%   +0.99%     
==========================================
  Files         174      175       +1     
  Lines       58611    56078    -2533     
==========================================
- Hits        55112    53287    -1825     
+ Misses       3499     2791     -708

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

mhasel

Mostly nits (sorry) and a couple of questions/suggestions. Overall, I like it. Nice! 🚀

mhasel · 2025-11-20T10:46:37Z

src/codegen/generators/expression_generator.rs

+                let Some(StatementAnnotation::Argument { position, depth, pou, .. }) =
+                    self.annotations.get_hint(assignment_statement)
+                else {
+                    panic!()


should we return an error here instead of an explicit panic? If we are confident this code-path can't be reached, we might wanna change this to unreachable instead

mhasel · 2025-11-20T11:26:31Z

src/codegen/generators/expression_generator.rs

+
+    /// The name of the POU where the parameter is declared
+    /// See also [`StatementAnnotation::Argument::pou`].
+    pou_parameter: &'b str,


If it weren't for the doc-comment, I'd have no idea that pou_parameter refers to the name of the POU. How do you feel about something like declaring_pou or source_pou?

mhasel · 2025-11-20T11:30:07Z

src/codegen/generators/expression_generator.rs

-        let function_name = param_context.function_name;
-        let index = param_context.index;
-        let parameter_struct = param_context.parameter_struct;
+        // let function_name = param_context.function_name;


missed a comment

mhasel · 2025-11-20T11:47:25Z

src/codegen/generators/expression_generator.rs

+                .map(|value| i32_type.const_int(value as u64, false))
+                .collect::<Vec<_>>();
+
+            self.llvm.builder.build_gep(context.parameter_struct, &gep_index, "").unwrap()


Can you tell me why we aren't using build_in_bounds_gep here? In your PR message you mention

GEPing directly, i.e. not inbound

An inbound GEP is still a direct GEP, just with additional safety guarantees and it allows LLVM to optimize more aggressively. Inheritance hierarchy access is well defined and guaranteed to be within bounds of the allocated struct, or am I missing something?
I have changed this line to use build_in_bounds_gep and the LIT tests still pass locally

mhasel · 2025-11-20T12:28:58Z

src/validation/tests/statement_validation_tests.rs

+    error[E037]: Invalid assignment: cannot assign 'SEL with wrong parameter names a := SEL(WRONG := sel, IN0 := a, IN1 := b); a := SEL(G := sel, INVALID := a,' to 'ARRAY[0..5] OF INT'
+       ┌─ <internal>:14:13
+       │
+    14 │             arr2 := MOVE(SOURCE := arr);
+       │             ^^^^^^^^^^^^^^^^^^^^^^^^^^^ Invalid assignment: cannot assign 'SEL with wrong parameter names a := SEL(WRONG := sel, IN0 := a, IN1 := b); a := SEL(G := sel, INVALID := a,' to 'ARRAY[0..5] OF INT'


Looks like the reported location is off here. Also, I'd expect to get two such errors, since we have two call statements to SEL with different invalid argument names in the test

mhasel · 2025-11-20T12:45:27Z

src/index.rs

+    // XXX: This is super specific and currently only being used in the context of argument annotations. Do we
+    // want to move this to the resolver?


Considering how many specialized public methods we have on our Index, I'd be tempted to say one more won't matter, but I think I'd prefer this to be a private method/function in the resolver.

mhasel · 2025-11-20T12:50:56Z

src/index.rs

-        self.get_pou_members(pou_name).iter().any(|member| member.is_parameter() && member.is_variadic())
+    /// Returns all declared parameters (INPUT, OUTPUT or IN_OUT) of a POU, including those defined in parent
+    /// POUs. The returned list is ordered by the inheritance chain, from base to derived.
+    pub fn get_declared_parameters(&self, pou: &str) -> Vec<&VariableIndexEntry> {


This is really nitty, but I think the doc comment and method name here are semantically inconsistent. Parameters from parent POUs are not declared but inherited. Here I'd almost like this to be two methods, one get_declared_parameters that only fetches the parameters declared by the given POU and another one (e.g. get_available_parameters) which can include the inherited params aswell.
Alternatively, if there is no need for a method that only returns the declared parameters, rename this one to get_available_parameters?

mhasel · 2025-11-20T12:56:59Z

src/resolver.rs

-        let parameters = if let Some(parameters) = parameters_stmt {
-            self.visit_statement(ctx, parameters);
-            flatten_expression_list(parameters)
+    pub fn annotate_arguments(&mut self, operator: &AstNode, arguments_node: &AstNode, ctx: &VisitorContext) {


yet another nitpick: we aren't just annotating the arguments here, but also the operator (in the tail call to update_generic_call_statement), so I once again feel like the original method name was more fitting

mhasel · 2025-11-20T13:28:41Z

src/resolver.rs

+        /// The position of the parameter within its declared POU.
        position: usize,
+
+        /// The depth between where the arguments parameter is declared versus where it is being called from.


nitty gritty: what do you think about
/// Inheritance depth from parameter declaration to calling context.?

mhasel · 2025-11-20T13:36:08Z

tests/correctness/functions.rs

                var2 : DINT;
            END_VAR
-            foo(7, 8, output1 => var1, output2 => var2);
+            foo(input1 := 7, input2 := 8, output1 => var1, output2 => var2);


I thought this change in the test looked like a masked regression for a second here, but this exact example doesn't compile on master using the regular pipeline, but apparently it works in the test. Another case of where our test results don't match reality?

fix: function block calls with inherited parameters

6cba4ce

volsa force-pushed the vosa/PRG-3463 branch 3 times, most recently from 89ff513 to e5139c0 Compare November 13, 2025 13:08

refactor: self-review

cb65351

volsa force-pushed the vosa/PRG-3463 branch from e5139c0 to cb65351 Compare November 13, 2025 14:05

Merge branch 'master' into vosa/PRG-3463

70a80f3

volsa marked this pull request as ready for review November 13, 2025 14:33

volsa requested review from abroooo, ghaith and mhasel November 13, 2025 14:33

volsa and others added 2 commits November 14, 2025 09:55

docs: add code comment explaining GEP logic

1793c9f

Merge branch 'master' into vosa/PRG-3463

67867b3

mhasel requested changes Nov 20, 2025

View reviewed changes

		// XXX: This is super specific and currently only being used in the context of argument annotations. Do we
		// want to move this to the resolver?

fix: extended function block body calls #1545

Are you sure you want to change the base?

fix: extended function block body calls #1545

Uh oh!

Conversation

volsa commented Nov 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Nov 13, 2025

Codecov Report

Uh oh!

mhasel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

volsa commented Nov 10, 2025 •

edited

Loading