Skip to content

Conversation

volsa
Copy link
Member

@volsa volsa commented Jul 9, 2025

No description provided.

@volsa volsa force-pushed the vosa/fnptr2 branch 3 times, most recently from 8d24e01 to 81c0500 Compare July 23, 2025 15:57
volsa and others added 4 commits August 3, 2025 13:57
Previously when dealing with function pointer calls such as `fnPtr^()`
the resolver would only annotate the operator and any arguments. Some
codegen parts require an annotation on the whole call statement however
(which makes sense), as such this commit fixes the described issue.
Visuallized, assuming `fnPtr` points to a function returning a DINT:
```
fnPtr^();
^^^^^^^^ -> StatementAnnotation::Value { resulting_type: "DINT" }
```
Function pointers can now point to the body of a function block, allowing
for code execution such as
```
FUNCTION_BLOCK A
    VAR
        localState: DINT := 5;
    END_VAR

    METHOD foo
        // ...
    END_METHOD

    printf('localState = %d$N', localState);
END_FUNCTION_BLOCK

FUNCTION main
    VAR
        instanceA: A;
        fnBodyPtr: FNPTR A := ADR(A);
    END_VAR

    fnBodyPtr^(instanceA); // prints "localState = 5"
END_FUNCTION
```
@volsa volsa force-pushed the vosa/fnptr2 branch 2 times, most recently from d4157a3 to cd6dcc0 Compare August 26, 2025 07:21
@@ -298,6 +298,9 @@ pub enum Token {
#[token("POINTER", ignore(case))]
KeywordPointer,

#[token("FNPTR", ignore(case))]
Copy link
Member Author

@volsa volsa Aug 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy to change this if there are "better" suggestions. Also due to integration tests the token now leaks to the end-user although function pointers are an internal-only construct as of now. No idea what we want to do here tbh. Leave as is or return a validation error but disable it in tests (if possible)? In any case this becomes a reserved keyword.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could always just not parse it and have it as an ast only construct?
Also just a nit, but FPOINTER? FUNCT_REF? FUNCT_PTR?

Copy link
Member Author

@volsa volsa Aug 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would you test if code execution is correct then? Specifically, we need to parse them if you take a look at the integration / lit tests, no?

Edit: FPOINTER sounds better than FNPTR, will change 👍

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since not intended for the user, we test through polymorphism.. but I don't mind them here, just be careful about reserving new keywords: I would go with __FPOINTER otherwise any variable called FPOINTER or FNPTR in the old version would no longer work

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I would like to explicitly test them since that should (in theory) make debugging bugs related to function pointers easier rather than opaquely couple them to polymorphism. Changed FNPTR to __FPOINTER

Comment on lines +2091 to +2093
// TODO(vosa): THIS^(), needs to be handled in the polymorphism PR but is
// ignored here (!stmt.is_this_deref())
if *is_function
&& (pou.is_method() | pou.is_function_block() && !stmt.is_this_deref())
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ignore this for now, I can't remember why the !is_this_deref() check was neccessary (tests seem to work without it). I'll have to re-check in the polymorphism branch.

@volsa volsa requested review from ghaith, riederm and mhasel August 28, 2025 11:22
@volsa volsa marked this pull request as ready for review August 28, 2025 11:22
Copy link
Collaborator

@ghaith ghaith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good, i just added some small comments

ExpressionValue::RValue(_) => unreachable!(),
};

// Generate the argument list; our assumption is function pointers are only supported for methods
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this still true? fbs are not function pointers?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see them as methods, that being said I updated the code documentation to make it clearer

@@ -298,6 +298,9 @@ pub enum Token {
#[token("POINTER", ignore(case))]
KeywordPointer,

#[token("FNPTR", ignore(case))]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since not intended for the user, we test through polymorphism.. but I don't mind them here, just be careful about reserving new keywords: I would go with __FPOINTER otherwise any variable called FPOINTER or FNPTR in the old version would no longer work

src/resolver.rs Outdated
@@ -307,7 +307,20 @@ impl TypeAnnotator<'_> {

let mut generics_candidates: FxHashMap<String, Vec<String>> = FxHashMap::default();
let mut params = vec![];
let mut parameters = parameters.into_iter();
let mut parameters = if self.annotation_map.get(operator).is_some_and(|opt| opt.is_fnptr()) {
// When dealing with a function pointer (which are only supported in the context of methods), the
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this still correct? function blocks can be function pointers

@volsa volsa requested a review from ghaith August 28, 2025 14:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants