feat: validate if template instance are configured (#1320)

mhasel · web-flow · commit 223a9e00584f · 2024-10-08T08:01:01.000+02:00
* feat: Validate unconfigured template variables

Adds a validation to find unconfigured template variables, for example
the following code would return an error because `main.fb2.foo` is not
configured.
diff --git a/compiler/plc_ast/src/ast.rs b/compiler/plc_ast/src/ast.rs
@@ -292,7 +292,7 @@ impl PouType {
     }
 }
 
-#[derive(Debug, PartialEq)]
+#[derive(Debug, PartialEq, Clone)]
 pub struct ConfigVariable {
     pub reference: AstNode,
     pub data_type: DataTypeDeclaration,
diff --git a/compiler/plc_diagnostics/src/diagnostics/diagnostics_registry.rs b/compiler/plc_diagnostics/src/diagnostics/diagnostics_registry.rs
@@ -207,6 +207,8 @@ lazy_static! {
         E103,   Error,      include_str!("./error_codes/E103.md"),  // Immutable Hardware Binding
         E104,   Error,      include_str!("./error_codes/E104.md"),  // Config Variable With Incomplete Address
         E105,   Error,      include_str!("./error_codes/E105.md"),  // CONSTANT keyword in POU
+        E107,   Error,      include_str!("./error_codes/E107.md"),  // Missing configuration for template variable
+        E108,   Error,      include_str!("./error_codes/E108.md"),  // Template variable is configured multiple times
     );
 }
 
diff --git a/compiler/plc_diagnostics/src/diagnostics/error_codes/E107.md b/compiler/plc_diagnostics/src/diagnostics/error_codes/E107.md
@@ -0,0 +1,26 @@
+# Missing configuration for template variable
+
+A template variable was left unconfigured.
+
+Erroneous code example:
+```
+VAR_CONFIG
+    main.foo.bar AT %IX1.0 : BOOL;
+END_VAR
+
+PROGRAM main
+    VAR
+        foo : foo_fb;
+    END_VAR
+END_PROGRAM
+
+FUNCTION_BLOCK foo_fb
+    VAR
+        bar AT %I* : BOOL;
+        qux AT %I* : BOOL;
+    END_VAR
+END_FUNCTION_BLOCK
+```
+
+In this example a variable named `main.foo.qux` is declared as a template, however the `VAR_CONFIG`-block does not contain
+an address-configuration for it. Each template variable needs to be configured, otherwise it could lead to segmentation faults at runtime.
diff --git a/compiler/plc_diagnostics/src/diagnostics/error_codes/E108.md b/compiler/plc_diagnostics/src/diagnostics/error_codes/E108.md
@@ -0,0 +1,25 @@
+# Template variable is configured multiple times
+
+A template variable is configured more than once, leading to ambiguity.
+
+Erroneous code example:
+```
+VAR_CONFIG
+    main.foo.bar AT %IX1.0 : BOOL;
+    main.foo.bar AT %IX1.1 : BOOL;
+END_VAR
+
+PROGRAM main
+    VAR
+        foo : foo_fb;
+    END_VAR
+END_PROGRAM
+
+FUNCTION_BLOCK foo_fb
+    VAR
+        bar AT %I* : BOOL;
+    END_VAR
+END_FUNCTION_BLOCK
+```
+
+In this example a variable named `main.foo.bar` has multiple configurations in the `VAR_CONFIG`-block. It is not clear which address this variable should map to - only a single configuration entry per instance-variable is allowed.
diff --git a/src/expression_path.rs b/src/expression_path.rs
@@ -1,17 +1,22 @@
-use std::fmt::Display;
+use std::{fmt::Display, vec};
+
+use plc_ast::ast::{AstNode, AstStatement, ConfigVariable, ReferenceAccess, ReferenceExpr};
+use plc_diagnostics::diagnostics::Diagnostic;
 
 use crate::{index::Index, typesystem::Dimension};
 
-#[derive(Clone, Copy, Debug, PartialEq, Eq)]
+#[derive(Clone, Debug, PartialEq, Eq)]
 pub enum ExpressionPathElement<'idx> {
     Name(&'idx str),
-    ArrayAccess(&'idx [Dimension]),
+    ArrayDimensions(&'idx [Dimension]),
+    ArrayAccess(Vec<i128>),
 }
 
 impl Display for ExpressionPathElement<'_> {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         match self {
             ExpressionPathElement::Name(name) => write!(f, "{name}"),
+            ExpressionPathElement::ArrayDimensions(_) => unimplemented!(),
             ExpressionPathElement::ArrayAccess(_) => unimplemented!(),
         }
     }
@@ -25,7 +30,7 @@ impl<'idx> From<&'idx str> for ExpressionPathElement<'idx> {
 
 impl<'idx> From<&'idx [Dimension]> for ExpressionPathElement<'idx> {
     fn from(dims: &'idx [Dimension]) -> Self {
-        ExpressionPathElement::ArrayAccess(dims)
+        ExpressionPathElement::ArrayDimensions(dims)
     }
 }
 
@@ -51,8 +56,8 @@ impl<'idx> ExpressionPath<'idx> {
         let mut levels: Vec<Vec<String>> = vec![];
         for seg in self.names.iter() {
             let level = match seg {
-                crate::expression_path::ExpressionPathElement::Name(s) => vec![s.to_string()],
-                crate::expression_path::ExpressionPathElement::ArrayAccess(dimensions) => {
+                ExpressionPathElement::Name(s) => vec![s.to_string()],
+                ExpressionPathElement::ArrayDimensions(dimensions) => {
                     let mut array = dimensions.iter().map(|it| it.get_range_inclusive(index).unwrap()).fold(
                         vec![],
                         |curr, it| {
@@ -74,6 +79,14 @@ impl<'idx> ExpressionPath<'idx> {
                     array.iter_mut().for_each(|s| *s = format!("[{s}]"));
                     array
                 }
+                ExpressionPathElement::ArrayAccess(idx) => {
+                    let Some(first) = idx.first() else { unreachable!("Caught at the parsing stage") };
+
+                    let access =
+                        idx.iter().skip(1).fold(format!("{first}"), |acc, idx| format!("{acc},{idx}"));
+
+                    vec![format!("[{access}]")]
+                }
             };
             levels.push(level);
         }
@@ -94,6 +107,76 @@ impl<'idx> ExpressionPath<'idx> {
     }
 }
 
+impl<'a> TryFrom<&'a ConfigVariable> for ExpressionPath<'a> {
+    type Error = Vec<Diagnostic>;
+
+    fn try_from(value: &'a ConfigVariable) -> Result<Self, Self::Error> {
+        let mut names = get_expression_path_segments(&value.reference)?;
+        names.reverse();
+        Ok(Self { names })
+    }
+}
+
+// Transforms a `ConfigVariable`'s 'AstNode' into a collection of corresponding `ExpressionPathElement`s.
+// This function will traverse the AST top-to-bottom, collecting segments along the way, which means the order of the collection
+// needs to be reversed by the caller to match the written expression.
+fn get_expression_path_segments(node: &AstNode) -> Result<Vec<ExpressionPathElement>, Vec<Diagnostic>> {
+    let mut paths = vec![];
+    let mut diagnostics = vec![];
+    let mut add_diagnostic = |location| {
+        diagnostics.push(
+            Diagnostic::new("VAR_CONFIG array access must be a literal integer").with_location(location),
+        );
+    };
+    match &node.stmt {
+        AstStatement::ReferenceExpr(ReferenceExpr { access: ReferenceAccess::Member(reference), base }) => {
+            paths.push(ExpressionPathElement::Name(reference.get_flat_reference_name().unwrap_or_default()));
+            if let Some(base) = base {
+                match get_expression_path_segments(base) {
+                    Ok(v) => paths.extend(v),
+                    Err(e) => diagnostics.extend(e),
+                };
+            }
+        }
+        AstStatement::ReferenceExpr(ReferenceExpr { access: ReferenceAccess::Index(idx), base }) => {
+            match &idx.as_ref().stmt {
+                AstStatement::Literal(_) => {
+                    if let Some(v) = idx.get_literal_integer_value().map(|it| vec![it]) {
+                        paths.push(ExpressionPathElement::ArrayAccess(v))
+                    } else {
+                        add_diagnostic(&idx.location);
+                    }
+                }
+                AstStatement::ExpressionList(vec) => {
+                    let mut res = vec![];
+                    vec.iter().for_each(|idx: &AstNode| {
+                        if let Some(v) = idx.get_literal_integer_value() {
+                            res.push(v);
+                        } else {
+                            add_diagnostic(&idx.location);
+                        }
+                    });
+                    paths.push(ExpressionPathElement::ArrayAccess(res));
+                }
+                _ => add_diagnostic(&idx.location),
+            }
+            if let Some(base) = base {
+                match get_expression_path_segments(base) {
+                    Ok(v) => paths.extend(v),
+                    Err(e) => diagnostics.extend(e),
+                };
+            }
+        }
+        _ => add_diagnostic(&node.location),
+    };
+
+    if !diagnostics.is_empty() {
+        return Err(diagnostics);
+    }
+
+    Ok(paths)
+}
+
 impl<'a> From<Vec<ExpressionPathElement<'a>>> for ExpressionPath<'a> {
     fn from(v: Vec<ExpressionPathElement<'a>>) -> Self {
         ExpressionPath { names: v }
@@ -132,7 +215,7 @@ mod tests {
         }];
 
         let name = ExpressionPath {
-            names: vec![ExpressionPathElement::Name("a"), ExpressionPathElement::ArrayAccess(&dims)],
+            names: vec![ExpressionPathElement::Name("a"), ExpressionPathElement::ArrayDimensions(&dims)],
         };
         let index = Index::default();
         assert_eq!(name.expand(&index), vec!["a[-1]".to_string(), "a[0]".to_string(), "a[1]".to_string(),])
@@ -146,7 +229,7 @@ mod tests {
         }];
 
         let name = ExpressionPath {
-            names: vec![ExpressionPathElement::Name("a"), ExpressionPathElement::ArrayAccess(&dims)],
+            names: vec![ExpressionPathElement::Name("a"), ExpressionPathElement::ArrayDimensions(&dims)],
         };
         let index = Index::default();
         assert_eq!(name.expand(&index), vec!["a[1]".to_string(),])
@@ -161,7 +244,7 @@ mod tests {
         ];
 
         let name = ExpressionPath {
-            names: vec![ExpressionPathElement::Name("a"), ExpressionPathElement::ArrayAccess(&dims)],
+            names: vec![ExpressionPathElement::Name("a"), ExpressionPathElement::ArrayDimensions(&dims)],
         };
         let index = Index::default();
         let mut res = name.expand(&index);
@@ -197,9 +280,9 @@ mod tests {
         let name = ExpressionPath {
             names: vec![
                 ExpressionPathElement::Name("a"),
-                ExpressionPathElement::ArrayAccess(&dims1),
-                ExpressionPathElement::ArrayAccess(&dims2),
-                ExpressionPathElement::ArrayAccess(&dims3),
+                ExpressionPathElement::ArrayDimensions(&dims1),
+                ExpressionPathElement::ArrayDimensions(&dims2),
+                ExpressionPathElement::ArrayDimensions(&dims3),
             ],
         };
         let index = Index::default();
diff --git a/src/index.rs b/src/index.rs
@@ -6,8 +6,8 @@ use itertools::Itertools;
 use rustc_hash::{FxHashSet, FxHasher};
 
 use plc_ast::ast::{
-    AstId, AstNode, AstStatement, DirectAccessType, GenericBinding, HardwareAccessType, LinkageType, PouType,
-    TypeNature,
+    AstId, AstNode, AstStatement, ConfigVariable, DirectAccessType, GenericBinding, HardwareAccessType,
+    LinkageType, PouType, TypeNature,
 };
 use plc_diagnostics::diagnostics::Diagnostic;
 use plc_source::source_location::SourceLocation;
@@ -891,6 +891,8 @@ pub struct Index {
 
     /// The labels contained in each pou
     labels: FxIndexMap<String, SymbolMap<String, Label>>,
+
+    config_variables: Vec<ConfigVariable>,
 }
 
 impl Index {
@@ -1002,6 +1004,8 @@ impl Index {
         //labels
         self.labels.extend(other.labels);
 
+        self.config_variables.extend(other.config_variables);
+
         //Constant expressions are intentionally not imported
         // self.constant_expressions.import(other.constant_expressions)
     }
@@ -1678,6 +1682,10 @@ impl Index {
     pub fn get_labels(&self, pou_name: &str) -> Option<&SymbolMap<String, Label>> {
         self.labels.get(pou_name)
     }
+
+    pub fn get_config_variables(&self) -> &Vec<ConfigVariable> {
+        &self.config_variables
+    }
 }
 
 /// Returns a default initialization name for a variable or type
diff --git a/src/index/instance_iterator.rs b/src/index/instance_iterator.rs
@@ -91,7 +91,7 @@ impl<'idx> InstanceIterator<'idx> {
                 self.inner = InstanceIterator::inner(
                     self.index,
                     variable.get_type_name(),
-                    &vec![entry].into(),
+                    &vec![entry.clone()].into(),
                     self.filter,
                 )
                 .map(Box::new);
diff --git a/src/index/tests/snapshots/rusty__index__tests__instance_resolver_tests__array_instances_are_repeated.snap b/src/index/tests/snapshots/rusty__index__tests__instance_resolver_tests__array_instances_are_repeated.snap
@@ -87,7 +87,7 @@ expression: "index.find_instances().collect::<Vec<Instance<'_>>>()"
                 Name(
                     "aFb",
                 ),
-                ArrayAccess(
+                ArrayDimensions(
                     [
                         Dimension {
                             start_offset: ConstExpression(
@@ -147,7 +147,7 @@ expression: "index.find_instances().collect::<Vec<Instance<'_>>>()"
                 Name(
                     "aFb",
                 ),
-                ArrayAccess(
+                ArrayDimensions(
                     [
                         Dimension {
                             start_offset: ConstExpression(
@@ -246,7 +246,7 @@ expression: "index.find_instances().collect::<Vec<Instance<'_>>>()"
                 Name(
                     "aFb1",
                 ),
-                ArrayAccess(
+                ArrayDimensions(
                     [
                         Dimension {
                             start_offset: ConstExpression(
@@ -320,7 +320,7 @@ expression: "index.find_instances().collect::<Vec<Instance<'_>>>()"
                 Name(
                     "aFb1",
                 ),
-                ArrayAccess(
+                ArrayDimensions(
                     [
                         Dimension {
                             start_offset: ConstExpression(
diff --git a/src/index/tests/snapshots/rusty__index__tests__instance_resolver_tests__array_with_const_instances_are_repeated.snap b/src/index/tests/snapshots/rusty__index__tests__instance_resolver_tests__array_with_const_instances_are_repeated.snap
@@ -131,7 +131,7 @@ expression: "index.find_instances().collect::<Vec<Instance<'_>>>()"
                 Name(
                     "aFb",
                 ),
-                ArrayAccess(
+                ArrayDimensions(
                     [
                         Dimension {
                             start_offset: ConstExpression(
@@ -191,7 +191,7 @@ expression: "index.find_instances().collect::<Vec<Instance<'_>>>()"
                 Name(
                     "aFb",
                 ),
-                ArrayAccess(
+                ArrayDimensions(
                     [
                         Dimension {
                             start_offset: ConstExpression(
diff --git a/src/index/visitor.rs b/src/index/visitor.rs
@@ -32,6 +32,11 @@ pub fn visit(unit: &CompilationUnit) -> Index {
     for implementation in &unit.implementations {
         visit_implementation(&mut index, implementation);
     }
+
+    for config_variable in &unit.var_config {
+        index.config_variables.push(config_variable.clone());
+    }
+
     index
 }
 
diff --git a/src/snapshots/rusty__hardware_binding__tests__hardware_collected_fb.snap b/src/snapshots/rusty__hardware_binding__tests__hardware_collected_fb.snap
diff --git a/src/validation.rs b/src/validation.rs
diff --git a/src/validation/tests/variable_validation_tests.rs b/src/validation/tests/variable_validation_tests.rs

Original file line number	Diff line number	Diff line change
`@@ -292,7 +292,7 @@ impl PouType {`
`292`	`292`	`}`
`293`	`293`	`}`
`294`	`294`
`295`		`-#[derive(Debug, PartialEq)]`
	`295`	`+#[derive(Debug, PartialEq, Clone)]`
`296`	`296`	`pub struct ConfigVariable {`
`297`	`297`	`pub reference: AstNode,`
`298`	`298`	`pub data_type: DataTypeDeclaration,`
Original file line number	Diff line number	Diff line change
`@@ -207,6 +207,8 @@ lazy_static! {`
`207`	`207`	`E103, Error, include_str!("./error_codes/E103.md"), // Immutable Hardware Binding`
`208`	`208`	`E104, Error, include_str!("./error_codes/E104.md"), // Config Variable With Incomplete Address`
`209`	`209`	`E105, Error, include_str!("./error_codes/E105.md"), // CONSTANT keyword in POU`
	`210`	`+ E107, Error, include_str!("./error_codes/E107.md"), // Missing configuration for template variable`
	`211`	`+ E108, Error, include_str!("./error_codes/E108.md"), // Template variable is configured multiple times`
`210`	`212`	`);`
`211`	`213`	`}`
`212`	`214`
Original file line number	Diff line number	Diff line change
`@@ -91,7 +91,7 @@ impl<'idx> InstanceIterator<'idx> {`
`91`	`91`	`self.inner = InstanceIterator::inner(`
`92`	`92`	`self.index,`
`93`	`93`	`variable.get_type_name(),`
`94`		`- &vec![entry].into(),`
	`94`	`+ &vec![entry.clone()].into(),`
`95`	`95`	`self.filter,`
`96`	`96`	`)`
`97`	`97`	`.map(Box::new);`
Original file line number	Diff line number	Diff line change
`@@ -32,6 +32,11 @@ pub fn visit(unit: &CompilationUnit) -> Index {`
`32`	`32`	`for implementation in &unit.implementations {`
`33`	`33`	`visit_implementation(&mut index, implementation);`
`34`	`34`	`}`
	`35`	`+`
	`36`	`+ for config_variable in &unit.var_config {`
	`37`	`+ index.config_variables.push(config_variable.clone());`
	`38`	`+ }`
	`39`	`+`
`35`	`40`	`index`
`36`	`41`	`}`
`37`	`42`