refactor: Removing backing property field (#1418)

Previously the property implementation would have a backing field with each property declaration in a POU.
For example two properties `foo` and `bar` would insert two variables inside the POU. This would as a side
effect affect the ffi of POUs using properties. Another consideration was duplication of used memory if the backing field ends up not being used in the getter/setter.

Additionally, using different properties to represent the same state/value in different units was not achievable without major drawbacks (i.e. having a temperature field in the POU with 3 separate getters for Celsius, Fahrenheit and Kelvin).

To mitigate that side-effect the backing field has been removed. As a result the resolver has been updated
to identify property references. The already existing lowering logic will then make use of these annotations
and simply lower GET and SET property annotations into concrete method calls.

This implementation also significantly simplifies the implementation of further OOP features regarding properties, e.g. inheriting/extending properties, which would previously result in duplicate/unused backing fields in derived POUs.

---------

Co-authored-by: Volkan <volkanxvs@gmail.com>
Co-authored-by: Volkan Sagcan <volkan.sagcan@bachmann.info>

Author: mhasel

compiler/plc_ast/src/ast.rs

@@ -42,9 +42,13 @@ pub struct Pou {
     pub generics: Vec<GenericBinding>,
     pub linkage: LinkageType,
     pub super_class: Option<Identifier>,
+    pub is_const: bool,
+
     /// A list of interfaces this POU implements
     pub interfaces: Vec<Identifier>,
-    pub is_const: bool,
+
+    /// A list of properties this POU contains
+    pub properties: Vec<PropertyBlock>,
 }
 
 #[derive(Debug, PartialEq)]
@@ -62,17 +66,15 @@ pub struct Identifier {
     pub location: SourceLocation,
 }
 
+/// The property container as a whole, which contains [`PropertyImplementation`]s
 #[derive(Debug, PartialEq, Clone)]
-pub struct Property {
-    pub name: String,
-    pub name_location: SourceLocation,
-    pub parent_kind: PouType,
-    pub parent_name: String,
-    pub parent_name_location: SourceLocation,
-    pub datatype: DataTypeDeclaration,
+pub struct PropertyBlock {
+    pub name: Identifier,
+    pub return_type: DataTypeDeclaration,
     pub implementations: Vec<PropertyImplementation>,
 }
 
+/// The declaration and implementation of a properties accessor (GET or SET)
 #[derive(Debug, PartialEq, Clone)]
 pub struct PropertyImplementation {
     pub kind: PropertyKind,
@@ -88,6 +90,18 @@ pub enum PropertyKind {
     Set,
 }
 
+impl From<&PropertyBlock> for Variable {
+    fn from(value: &PropertyBlock) -> Self {
+        Variable {
+            name: value.name.name.clone(),
+            data_type_declaration: value.return_type.clone(),
+            initializer: None,
+            address: None,
+            location: value.name.location.clone(),
+        }
+    }
+}
+
 impl std::fmt::Display for PropertyKind {
     fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
         match self {
@@ -339,9 +353,6 @@ pub enum PouType {
         /// The parent of this method, i.e. a function block, class or an interface
         parent: String,
 
-        /// The fully qualified name of the property this GET or SET method represents
-        property: Option<String>,
-
         declaration_kind: DeclarationKind,
     },
     Init,
@@ -412,7 +423,6 @@ pub struct CompilationUnit {
     pub interfaces: Vec<Interface>,
     pub user_types: Vec<UserTypeDeclaration>,
     pub file: FileMarker,
-    pub properties: Vec<Property>,
 }
 
 impl CompilationUnit {
@@ -425,7 +435,6 @@ impl CompilationUnit {
             interfaces: Vec::new(),
             user_types: Vec::new(),
             file: FileMarker::File(file_name),
-            properties: Vec::new(),
         }
     }
 
@@ -457,9 +466,6 @@ pub enum VariableBlockType {
     Global,
     InOut,
     External,
-
-    /// A compiler internal variable block representing all properties defined within a stateful POU
-    Property,
 }
 
 impl Display for VariableBlockType {
@@ -472,7 +478,6 @@ impl Display for VariableBlockType {
             VariableBlockType::Global => write!(f, "Global"),
             VariableBlockType::InOut => write!(f, "InOut"),
             VariableBlockType::External => write!(f, "External"),
-            VariableBlockType::Property => write!(f, "Property"),
         }
     }
 }
@@ -504,19 +509,6 @@ impl VariableBlock {
         self.variables = variables;
         self
     }
-
-    /// Creates a new (internal) variable block with a block type of [`Property`]
-    pub fn property(variables: Vec<Variable>) -> VariableBlock {
-        VariableBlock {
-            access: AccessModifier::Internal,
-            constant: false,
-            retain: false,
-            variables,
-            variable_block_type: VariableBlockType::Property,
-            linkage: LinkageType::Internal,
-            location: SourceLocation::internal(),
-        }
-    }
 }
 
 impl Default for VariableBlock {
@@ -1147,6 +1139,13 @@ impl AstNode {
         matches!(self.stmt, AstStatement::ReferenceExpr(..))
     }
 
+    pub fn is_member_access(&self) -> bool {
+        matches!(
+            self.stmt,
+            AstStatement::ReferenceExpr(ReferenceExpr { access: ReferenceAccess::Member(..), .. }, ..)
+        )
+    }
+
     pub fn is_call(&self) -> bool {
         matches!(self.stmt, AstStatement::CallStatement(..))
     }
@@ -1380,12 +1379,8 @@ mod tests {
         assert_eq!(PouType::Action.to_string(), "Action");
         assert_eq!(PouType::Class.to_string(), "Class");
         assert_eq!(
-            PouType::Method {
-                parent: String::new(),
-                property: None,
-                declaration_kind: DeclarationKind::Concrete
-            }
-            .to_string(),
+            PouType::Method { parent: String::new(), declaration_kind: DeclarationKind::Concrete }
+                .to_string(),
             "Method"
         );
     }

compiler/plc_ast/src/mut_visitor.rs

@@ -6,8 +6,8 @@ use std::borrow::BorrowMut;
 use crate::ast::{
     flatten_expression_list, Assignment, AstNode, AstStatement, BinaryExpression, CallStatement,
     CompilationUnit, DataType, DataTypeDeclaration, DirectAccess, HardwareAccess, Implementation, Interface,
-    JumpStatement, MultipliedStatement, Pou, RangeStatement, ReferenceAccess, ReferenceExpr, UnaryExpression,
-    UserTypeDeclaration, Variable, VariableBlock,
+    JumpStatement, MultipliedStatement, Pou, PropertyBlock, RangeStatement, ReferenceAccess, ReferenceExpr,
+    UnaryExpression, UserTypeDeclaration, Variable, VariableBlock,
 };
 use crate::control_statements::{AstControlStatement, ConditionalBlock, ReturnStatement};
 use crate::literals::AstLiteral;
@@ -208,6 +208,8 @@ pub trait AstVisitorMut: Sized {
     fn visit_allocation(&mut self, _node: &mut AstNode) {}
 
     fn visit_interface(&mut self, _interface: &mut Interface) {}
+
+    fn visit_property(&mut self, _property: &mut PropertyBlock) {}
 }
 
 impl WalkerMut for AstLiteral {
@@ -537,6 +539,10 @@ impl WalkerMut for Pou {
             visitor.visit_variable_block(block);
         }
 
+        for property in &mut self.properties {
+            visitor.visit_property(property);
+        }
+
         if let Some(rt) = self.return_type.as_mut() {
             visitor.visit_data_type_declaration(rt)
         }

compiler/plc_ast/src/visitor.rs

@@ -5,7 +5,7 @@ use crate::ast::{
     flatten_expression_list, Allocation, Assignment, AstNode, AstStatement, BinaryExpression, CallStatement,
     CompilationUnit, ConfigVariable, DataType, DataTypeDeclaration, DefaultValue, DirectAccess,
     EmptyStatement, HardwareAccess, Implementation, Interface, JumpStatement, LabelStatement,
-    MultipliedStatement, Pou, RangeStatement, ReferenceAccess, ReferenceExpr, UnaryExpression,
+    MultipliedStatement, Pou, PropertyBlock, RangeStatement, ReferenceAccess, ReferenceExpr, UnaryExpression,
     UserTypeDeclaration, Variable, VariableBlock,
 };
 use crate::control_statements::{AstControlStatement, ConditionalBlock, ReturnStatement};
@@ -166,6 +166,9 @@ pub trait AstVisitor: Sized {
         interface.walk(self);
     }
 
+    /// Visits a `Property`.
+    fn visit_property(&mut self, _property: &PropertyBlock) {}
+
     /// Visits an enum element `AstNode` node.
     /// Make sure to call `walk` on the `AstNode` node to visit its children.
     /// # Arguments
@@ -767,6 +770,10 @@ impl Walker for Pou {
             visitor.visit_variable_block(block);
         }
 
+        for property in &self.properties {
+            visitor.visit_property(property);
+        }
+
         self.return_type.as_ref().inspect(|rt| visitor.visit_data_type_declaration(rt));
     }
 }

compiler/plc_driver/src/pipelines/validator.rs

@@ -5,7 +5,9 @@ use super::{participant::PipelineParticipant, ParsedProject};
 impl PipelineParticipant for ParticipantValidator {
     fn pre_index(&mut self, project: &ParsedProject) {
         for unit in &project.units {
-            self.validate_properties(&unit.properties);
+            for pou in &unit.units {
+                self.validate_properties(pou);
+            }
         }
 
         self.report_diagnostics();