refactor: self review

Author: volsa

.vscode/launch.json

@@ -4,31 +4,29 @@
     // For more information, visit: https://go.microsoft.com/fwlink/?linkid=830387
     "version": "0.2.0",
     "configurations": [
-        {
-            "type": "codelldb",
-            "request": "launch",
-            "name": "Debug codelldb standard_lib",
-            "cargo": {
-                "args": [
-                    "build",
-                    "--bin=plc",
-                    "--package=plc_driver"
-                ],
-                "filter": {
-                    "name": "plc",
-                    "kind": "bin"
-                }
-            },
-            "program": "target/debug/plc",
+    {
+        "type": "codelldb",
+        "request": "launch",
+        "name": "Debug codelldb standard_lib",
+        "cargo": {
             "args": [
-                "libs/stdlib/iec61131-st/*.st"
+                "build",
+                "--bin=plc",
+                "--package=plc_driver"
             ],
-            "cwd": "${workspaceFolder}",
-            "env": {
-                "RUST_LOG": "rusty"
-            },
-            "terminal": "integrated"
+            "filter": {
+                "name": "plc",
+                "kind": "bin"
+            }
+        },
+        "program": "target/debug/plc",
+        "args": ["libs/stdlib/iec61131-st/*.st"],
+        "cwd": "${workspaceFolder}",
+        "env": {
+            "RUST_LOG": "rusty"
         },
+        "terminal": "integrated"
+    },
         {
             "type": "codelldb",
             "request": "launch",
@@ -45,11 +43,7 @@
                 }
             },
             "program": "target/debug/plc",
-            "args": [
-                "target/demo.st",
-                "-g",
-                "-c"
-            ],
+            "args": ["target/demo.st", "-g", "-c"],
             "cwd": "${workspaceFolder}",
             "env": {
                 "RUST_LOG": "rusty"
@@ -73,8 +67,7 @@
             },
             "args": [
                 "target/demo.st",
-                "tests/lit/util/printf.pli",
-                "-j=1",
+                "tests/lit/util/printf.pli"
             ],
             "cwd": "${workspaceFolder}",
             "env": {
@@ -114,6 +107,7 @@
             "cwd": "${workspaceFolder}",
             "program": "target/demo",
             "terminal": "integrated"
+
         }
     ]
-}
+}

Makefile

@@ -286,30 +286,29 @@ impl<'ink> DataTypeGenerator<'ink, '_> {
         }
     }
 
-    // TODO(vosa): Is this neccessary? Are the function types indexed and later on used in the expression
-    // generator? If not this whole commit could be reverted, double-check before merging
-    fn create_function_type(&mut self, pou_name: &str) -> Result<FunctionType<'ink>, Diagnostic> {
+    fn create_function_type(&mut self, method_name: &str) -> Result<FunctionType<'ink>, Diagnostic> {
         let return_type = self
             .types_index
-            .find_associated_type(self.index.get_return_type_or_void(pou_name).get_name())
+            .find_associated_type(self.index.get_return_type_or_void(method_name).get_name())
             .map(|opt| opt.as_any_type_enum())
             .unwrap_or(self.llvm.context.void_type().as_any_type_enum());
 
         let mut parameter_types = Vec::new();
 
-        // Methods are defined as functions in the LLVM IR, but carry the underlying POU type as their first
-        // parameter to operate on them, hence push the POU type to the very first position.
-        match self.index.find_pou(pou_name) {
+        match self.index.find_pou(method_name) {
             Some(PouIndexEntry::Method { parent_name, .. }) => {
                 let ty = self.types_index.get_associated_type(parent_name).expect("must exist");
                 let ty_ptr = ty.ptr_type(AddressSpace::from(ADDRESS_SPACE_GENERIC)).into();
 
+                // Methods are defined as functions in the LLVM IR, but carry the underlying POU type as their
+                // first parameter to operate on them, hence push the POU type to the very first position.
                 parameter_types.push(ty_ptr);
-                for parameter in self.index.get_declared_parameters(pou_name) {
-                    // Instead of relying on the LLVM index, we create data-types directly in here because some of
-                    // them may not have been registered yet. For example, at the time of writing this comment the
-                    // `__auto_pointer_to_DINT` type was not present in the index for a VAR_IN_OUT parameter which
-                    // resulted in an error
+
+                for parameter in self.index.get_declared_parameters(method_name) {
+                    // Instead of relying on the LLVM index, we create data-types on the fly here because some
+                    // types have not yet been visited and as a result may not be in the index. For example at
+                    // the time of writing this the index was not able to find a input parameter of type
+                    // `__auto_pointer_to_DINT`, consequently panicking
                     let ty = self.create_type(
                         parameter.get_name(),
                         self.index.get_type(&parameter.data_type_name).expect("must exist"),
@@ -319,13 +318,13 @@ impl<'ink> DataTypeGenerator<'ink, '_> {
                 }
             }
 
-            // Function blocks are a bit "weird" in that they only expect an instance argument even if they
-            // define input, output and/or inout parameters. Effectively, while being methods per-se, their
-            // calling convention differs from regular methods.
             Some(PouIndexEntry::FunctionBlock { name, .. }) => {
                 let ty = self.types_index.get_associated_type(name).expect("must exist");
                 let ty_ptr = ty.ptr_type(AddressSpace::from(ADDRESS_SPACE_GENERIC)).into();
 
+                // Function blocks are a bit "weird" in that they only expect an instance argument even if
+                // they define input, output and/or inout parameters. Effectively, while being methods per-se,
+                // their calling convention differs from regular methods.
                 parameter_types.push(ty_ptr);
             }
 

src/codegen/generators/data_type_generator.rs

@@ -490,7 +490,6 @@ impl<'ink, 'b> ExpressionCodeGenerator<'ink, 'b> {
         operator: &AstNode,
         parameters: Option<&AstNode>,
     ) -> Result<ExpressionValue<'ink>, Diagnostic> {
-        // Check if we are dealing with something alike `foo^(...)`
         if self.annotations.get(operator).is_some_and(StatementAnnotation::is_fnptr) {
             return self.generate_fnptr_call(operator, parameters);
         }
@@ -595,6 +594,8 @@ impl<'ink, 'b> ExpressionCodeGenerator<'ink, 'b> {
         arguments: Option<&AstNode>,
     ) -> Result<ExpressionValue<'ink>, Diagnostic> {
         let Some(ReferenceExpr { base: Some(ref base), .. }) = operator.get_deref_expr() else {
+            // XXX: This would fail for auto-deref pointers, but given (for now) function pointers are never
+            // auto-derefed this should be fine.
             unreachable!("internal error, invalid method call")
         };
 
@@ -652,13 +653,10 @@ impl<'ink, 'b> ExpressionCodeGenerator<'ink, 'b> {
 
         let value = match call.try_as_basic_value() {
             Either::Left(value) => value,
-            Either::Right(_) => {
-                // TODO: When is this neccessary?
-                get_llvm_int_type(self.llvm.context, INT_SIZE, INT_TYPE)
-                    .ptr_type(AddressSpace::from(ADDRESS_SPACE_CONST))
-                    .const_null()
-                    .as_basic_value_enum()
-            }
+            Either::Right(_) => get_llvm_int_type(self.llvm.context, INT_SIZE, INT_TYPE)
+                .ptr_type(AddressSpace::from(ADDRESS_SPACE_CONST))
+                .const_null()
+                .as_basic_value_enum(),
         };
 
         // Output variables are assigned after the function block call, effectively gep'ing the instance

src/codegen/generators/expression_generator.rs

@@ -27,7 +27,6 @@ pub struct LlvmTypedIndex<'ink> {
 pub trait TypeHelper<'ink> {
     #[allow(clippy::wrong_self_convention)]
     fn as_basic_type(self) -> Option<BasicTypeEnum<'ink>>;
-
     fn create_ptr_type(&self, address_space: AddressSpace) -> PointerType<'ink>;
 }
 
@@ -113,7 +112,7 @@ impl<'ink> LlvmTypedIndex<'ink> {
     ) -> Result<(), Diagnostic> {
         let name = type_name.to_lowercase();
 
-        log::debug!("registered `{name}` as type `{}`", target_type.print_to_string());
+        log::trace!("registered `{name}` as type `{}`", target_type.print_to_string());
         self.type_associations.insert(name, target_type);
         Ok(())
     }
@@ -125,7 +124,7 @@ impl<'ink> LlvmTypedIndex<'ink> {
     ) -> Result<(), Diagnostic> {
         let name = type_name.to_lowercase();
 
-        log::debug!("registered `{name}` as POU type `{}`", target_type.print_to_string());
+        log::trace!("registered `{name}` as POU type `{}`", target_type.print_to_string());
         self.pou_type_associations.insert(name, target_type);
         Ok(())
     }
@@ -137,7 +136,7 @@ impl<'ink> LlvmTypedIndex<'ink> {
     ) -> Result<(), Diagnostic> {
         let name = type_name.to_lowercase();
 
-        log::debug!("registered `{name}` as initial value type `{}`", initial_value.print_to_string());
+        log::trace!("registered `{name}` as initial value type `{}`", initial_value.print_to_string());
         self.initial_value_associations.insert(name, initial_value);
         Ok(())
     }
@@ -150,7 +149,7 @@ impl<'ink> LlvmTypedIndex<'ink> {
     ) -> Result<(), Diagnostic> {
         let name = qualified_name(container_name, variable_name).to_lowercase();
 
-        log::debug!("registered `{name}` as loaded local type `{}`", target_value.print_to_string());
+        log::trace!("registered `{name}` as loaded local type `{}`", target_value.print_to_string());
         self.loaded_variable_associations.insert(name, target_value);
         Ok(())
     }
@@ -162,7 +161,7 @@ impl<'ink> LlvmTypedIndex<'ink> {
     ) -> Result<(), Diagnostic> {
         let name = variable_name.to_lowercase();
 
-        log::debug!("registered `{name}` as global variable type `{}`", global_variable.print_to_string());
+        log::trace!("registered `{name}` as global variable type `{}`", global_variable.print_to_string());
         self.global_values.insert(name.clone(), global_variable);
         self.initial_value_associations.insert(name, global_variable.as_pointer_value().into());
 
@@ -177,19 +176,19 @@ impl<'ink> LlvmTypedIndex<'ink> {
     ) -> Result<(), Diagnostic> {
         let name = callable_name.to_lowercase();
 
-        log::debug!("registered `{name}` as implementation type `{}`", function_value.print_to_string());
+        log::trace!("registered `{name}` as implementation type `{}`", function_value.print_to_string());
         self.implementations.insert(name, function_value);
 
         Ok(())
     }
 
     pub fn associate_utf08_literal(&mut self, literal: &str, literal_variable: GlobalValue<'ink>) {
-        log::debug!("registered literal {literal}");
+        log::trace!("registered literal {literal}");
         self.utf08_literals.insert(literal.to_string(), literal_variable);
     }
 
     pub fn associate_utf16_literal(&mut self, literal: &str, literal_variable: GlobalValue<'ink>) {
-        log::debug!("registered literal {literal}");
+        log::trace!("registered literal {literal}");
         self.utf16_literals.insert(literal.to_string(), literal_variable);
     }
 

src/codegen/llvm_index.rs

@@ -346,9 +346,8 @@ impl AstVisitorMut for AggregateTypeLowerer {
             let mut parameters =
                 stmt.parameters.as_mut().map(|it| steal_expression_list(it.borrow_mut())).unwrap_or_default();
 
-            // When dealing with function pointers (which for now support only methods), then the alloca
-            // variable must be placed at index 1 rather than 0 because the instance variable must always be
-            // the first argument
+            // Place the alloca aggregate variable at index 1 when dealing with function pointers because 0
+            // is reserved for the instance variable.
             if self.annotation.as_ref().unwrap().get(&stmt.operator).is_some_and(|opt| opt.is_fnptr()) {
                 parameters.insert(1, reference);
             } else {
@@ -1211,7 +1210,50 @@ mod tests {
     }
 
     #[test]
-    fn fnptr_no_argument() {
+    fn fnptr_with_arguments() {
+        let id_provider = IdProvider::default();
+        let (mut unit, index) = index_with_ids(
+            r#"
+            FUNCTION_BLOCK FbA
+                METHOD foo: STRING
+                    VAR_INPUT
+                        x: DINT;
+                        y: STRING;
+                    END_VAR
+                END_METHOD
+            END_FUNCTION_BLOCK
+
+            FUNCTION main
+                VAR
+                    instanceFbA: FbA;
+                    fooPtr: FNPTR FbA.foo;
+                    localX: DINT;
+                    localY: STRING;
+                    result: STRING;
+                END_VAR
+
+                result := fooPtr^(instanceFbA, localX, localY);
+            END_FUNCTION
+            "#,
+            id_provider.clone(),
+        );
+
+        let mut lowerer = AggregateTypeLowerer {
+            index: Some(index),
+            annotation: None,
+            id_provider: id_provider.clone(),
+            ..Default::default()
+        };
+        lowerer.visit_compilation_unit(&mut unit);
+        lowerer.index.replace(index_unit_with_id(&unit, id_provider.clone()));
+        let annotations = annotate_with_ids(&unit, lowerer.index.as_mut().unwrap(), id_provider.clone());
+        lowerer.annotation.replace(Box::new(annotations));
+        lowerer.visit_compilation_unit(&mut unit);
+        assert_debug_snapshot!(unit.implementations[2].statements[0]);
+    }
+
+    #[test]
+    fn fnptr_without_arguments() {
         let id_provider = IdProvider::default();
         let (mut unit, index) = index_with_ids(
             r#"