access to VAR/VAR_TEMP from outside the declaring pou is illegal (#513)

an illegal access diagnostic is added when accessing a private field
from outside the declaring pou. private fields are VAR & VAR_TEMP
declarations.

the compiler will successfully resolve the reference (and following
ones) to avoid follow-up errors.

fixes #55

src/diagnostics.rs

@@ -62,6 +62,7 @@ pub enum ErrNo {
 
     //reference related
     reference__unresolved,
+    reference__illegal_access,
 
     //type related
     type__cast_error,
@@ -193,6 +194,14 @@ impl Diagnostic {
         }
     }
 
+    pub fn illegal_access(reference: &str, location: SourceRange) -> Diagnostic {
+        Diagnostic::SyntaxError {
+            message: format!("Illegal access to private member {:}", reference),
+            range: location,
+            err_no: ErrNo::reference__illegal_access,
+        }
+    }
+
     pub fn unresolved_generic_type(
         symbol: &str,
         nature: &str,

src/index.rs

@@ -1352,3 +1352,8 @@ impl Index {
 pub fn get_initializer_name(name: &str) -> String {
     format!("{}__init", name)
 }
+impl VariableType {
+    pub(crate) fn is_private(&self) -> bool {
+        return matches!(self, VariableType::Temp | VariableType::Local);
+    }
+}

src/index/tests/snapshots/rusty__index__tests__instance_resolver_tests__global_struct_variables_are_retrieved.snap

@@ -44,7 +44,7 @@ expression: "index.find_instances().collect::<Vec<Instance<'_>>>()"
             qualified_name: "str.a",
             initial_value: None,
             variable_type: ByVal(
-                Local,
+                Input,
             ),
             is_constant: false,
             data_type_name: "DINT",
@@ -72,7 +72,7 @@ expression: "index.find_instances().collect::<Vec<Instance<'_>>>()"
             qualified_name: "str.b",
             initial_value: None,
             variable_type: ByVal(
-                Local,
+                Input,
             ),
             is_constant: false,
             data_type_name: "DINT",
@@ -156,7 +156,7 @@ expression: "index.find_instances().collect::<Vec<Instance<'_>>>()"
             qualified_name: "str.a",
             initial_value: None,
             variable_type: ByVal(
-                Local,
+                Input,
             ),
             is_constant: false,
             data_type_name: "DINT",
@@ -187,7 +187,7 @@ expression: "index.find_instances().collect::<Vec<Instance<'_>>>()"
             qualified_name: "str.b",
             initial_value: None,
             variable_type: ByVal(
-                Local,
+                Input,
             ),
             is_constant: false,
             data_type_name: "DINT",

src/index/tests/snapshots/rusty__index__tests__instance_resolver_tests__nested_global_struct_variables_are_retrieved.snap

@@ -44,7 +44,7 @@ expression: "index.find_instances().collect::<Vec<Instance<'_>>>()"
             qualified_name: "str.a",
             initial_value: None,
             variable_type: ByVal(
-                Local,
+                Input,
             ),
             is_constant: false,
             data_type_name: "str2",
@@ -75,7 +75,7 @@ expression: "index.find_instances().collect::<Vec<Instance<'_>>>()"
             qualified_name: "str2.c",
             initial_value: None,
             variable_type: ByVal(
-                Local,
+                Input,
             ),
             is_constant: false,
             data_type_name: "DINT",
@@ -106,7 +106,7 @@ expression: "index.find_instances().collect::<Vec<Instance<'_>>>()"
             qualified_name: "str2.d",
             initial_value: None,
             variable_type: ByVal(
-                Local,
+                Input,
             ),
             is_constant: false,
             data_type_name: "DINT",
@@ -134,7 +134,7 @@ expression: "index.find_instances().collect::<Vec<Instance<'_>>>()"
             qualified_name: "str.b",
             initial_value: None,
             variable_type: ByVal(
-                Local,
+                Input,
             ),
             is_constant: false,
             data_type_name: "str2",
@@ -165,7 +165,7 @@ expression: "index.find_instances().collect::<Vec<Instance<'_>>>()"
             qualified_name: "str2.c",
             initial_value: None,
             variable_type: ByVal(
-                Local,
+                Input,
             ),
             is_constant: false,
             data_type_name: "DINT",
@@ -196,7 +196,7 @@ expression: "index.find_instances().collect::<Vec<Instance<'_>>>()"
             qualified_name: "str2.d",
             initial_value: None,
             variable_type: ByVal(
-                Local,
+                Input,
             ),
             is_constant: false,
             data_type_name: "DINT",
@@ -280,7 +280,7 @@ expression: "index.find_instances().collect::<Vec<Instance<'_>>>()"
             qualified_name: "str.a",
             initial_value: None,
             variable_type: ByVal(
-                Local,
+                Input,
             ),
             is_constant: false,
             data_type_name: "str2",
@@ -314,7 +314,7 @@ expression: "index.find_instances().collect::<Vec<Instance<'_>>>()"
             qualified_name: "str2.c",
             initial_value: None,
             variable_type: ByVal(
-                Local,
+                Input,
             ),
             is_constant: false,
             data_type_name: "DINT",
@@ -348,7 +348,7 @@ expression: "index.find_instances().collect::<Vec<Instance<'_>>>()"
             qualified_name: "str2.d",
             initial_value: None,
             variable_type: ByVal(
-                Local,
+                Input,
             ),
             is_constant: false,
             data_type_name: "DINT",
@@ -379,7 +379,7 @@ expression: "index.find_instances().collect::<Vec<Instance<'_>>>()"
             qualified_name: "str.b",
             initial_value: None,
             variable_type: ByVal(
-                Local,
+                Input,
             ),
             is_constant: false,
             data_type_name: "str2",
@@ -413,7 +413,7 @@ expression: "index.find_instances().collect::<Vec<Instance<'_>>>()"
             qualified_name: "str2.c",
             initial_value: None,
             variable_type: ByVal(
-                Local,
+                Input,
             ),
             is_constant: false,
             data_type_name: "DINT",
@@ -447,7 +447,7 @@ expression: "index.find_instances().collect::<Vec<Instance<'_>>>()"
             qualified_name: "str2.d",
             initial_value: None,
             variable_type: ByVal(
-                Local,
+                Input,
             ),
             is_constant: false,
             data_type_name: "DINT",

src/index/visitor.rs

@@ -381,7 +381,7 @@ fn visit_data_type(
                     MemberInfo {
                         container_name: struct_name,
                         variable_name: &var.name,
-                        variable_linkage: ArgumentType::ByVal(VariableType::Local),
+                        variable_linkage: ArgumentType::ByVal(VariableType::Input), // struct members act like VAR_INPUT in terms of visibility
                         variable_type_name: member_type,
                         is_constant: false, //struct members are not constants //TODO thats probably not true (you can define a struct in an CONST-block?!)
                         binding,

src/resolver.rs

@@ -16,7 +16,7 @@ use crate::{
         self, AstId, AstStatement, CompilationUnit, DataType, DataTypeDeclaration, GenericBinding,
         LinkageType, Operator, Pou, TypeNature, UserTypeDeclaration, Variable,
     },
-    index::{Index, PouIndexEntry, VariableIndexEntry},
+    index::{Index, PouIndexEntry, VariableIndexEntry, VariableType},
     typesystem::{
         self, get_bigger_type, DataTypeInformation, StringEncoding, BOOL_TYPE, BYTE_TYPE,
         DATE_AND_TIME_TYPE, DATE_TYPE, DINT_TYPE, DWORD_TYPE, LINT_TYPE, REAL_TYPE,
@@ -127,9 +127,15 @@ pub enum StatementAnnotation {
     Value { resulting_type: String },
     /// a reference that resolves to a declared variable (e.g. `a` --> `PLC_PROGRAM.a`)
     Variable {
+        /// the name of the variable's type (e.g. `"INT"`)
         resulting_type: String,
+        /// the fully qualified name of this variable (e.g. `"MyFB.a"`)
         qualified_name: String,
+        /// denotes wheter this variable is declared as a constant
         constant: bool,
+        /// denotes the varialbe type of this varialbe, hence whether it is an input, output, etc.
+        variable_type: VariableType,
+        /// denotes whether this variable-reference should be automatically dereferenced when accessed
         is_auto_deref: bool,
     },
     /// a reference to a function
@@ -348,8 +354,8 @@ impl AnnotationMapImpl {
         self.generic_nature_map.insert(s.get_id(), nature);
     }
 
-    pub fn has_type_annotation(&self, id: &usize) -> bool {
-        self.type_map.contains_key(id)
+    pub fn has_type_annotation(&self, s: &AstStatement) -> bool {
+        self.type_map.contains_key(&s.get_id())
     }
 
     pub fn get_generic_nature(&self, s: &AstStatement) -> Option<&TypeNature> {
@@ -1727,6 +1733,7 @@ fn to_variable_annotation(
         qualified_name: v.get_qualified_name().into(),
         resulting_type: effective_type_name,
         constant: v.is_constant() || constant_override,
+        variable_type: v.variable_type.get_variable_type(),
         is_auto_deref,
     }
 }

src/resolver/tests/resolve_expressions_tests.rs

@@ -2,7 +2,7 @@ use core::panic;
 
 use crate::{
     ast::{self, AstStatement, DataType, Pou, UserTypeDeclaration},
-    index::Index,
+    index::{Index, VariableType},
     resolver::{AnnotationMap, AnnotationMapImpl, StatementAnnotation},
     test_utils::tests::annotate,
     typesystem::{
@@ -1018,7 +1018,8 @@ fn function_expression_resolves_to_the_function_itself_not_its_return_type() {
             qualified_name: "foo.foo".into(),
             resulting_type: "INT".into(),
             constant: false,
-            is_auto_deref: false
+            is_auto_deref: false,
+            variable_type: VariableType::Return
         }),
         foo_annotation
     );
@@ -1204,7 +1205,8 @@ fn qualified_expressions_dont_fallback_to_globals() {
             qualified_name: "MyStruct.y".into(),
             resulting_type: "INT".into(),
             constant: false,
-            is_auto_deref: false
+            is_auto_deref: false,
+            variable_type: VariableType::Input
         }),
         annotations.get(&statements[1])
     );
@@ -1456,7 +1458,8 @@ fn method_references_are_resolved() {
             qualified_name: "cls.foo.foo".into(),
             resulting_type: "INT".into(),
             constant: false,
-            is_auto_deref: false
+            is_auto_deref: false,
+            variable_type: VariableType::Return
         }),
         annotation
     );
@@ -2478,7 +2481,8 @@ fn action_body_gets_resolved() {
                 qualified_name: "prg.x".to_string(),
                 resulting_type: "DINT".to_string(),
                 constant: false,
-                is_auto_deref: false
+                is_auto_deref: false,
+                variable_type: VariableType::Local
             }),
             a
         );

src/validation/stmt_validator.rs

@@ -35,10 +35,8 @@ impl StatementValidator {
 
     pub fn validate_statement(&mut self, statement: &AstStatement, context: &ValidationContext) {
         match statement {
-            AstStatement::Reference {
-                name, location, id, ..
-            } => {
-                self.validate_reference(id, name, location, context);
+            AstStatement::Reference { name, location, .. } => {
+                self.validate_reference(statement, name, location, context);
             }
             AstStatement::CastStatement {
                 location,
@@ -337,14 +335,33 @@ impl StatementValidator {
 
     fn validate_reference(
         &mut self,
-        id: &usize,
+        statement: &AstStatement,
         ref_name: &str,
         location: &SourceRange,
         context: &ValidationContext,
     ) {
-        if !context.ast_annotation.has_type_annotation(id) {
+        // unresolved reference
+        if !context.ast_annotation.has_type_annotation(statement) {
             self.diagnostics
                 .push(Diagnostic::unresolved_reference(ref_name, location.clone()));
+        } else if let Some(StatementAnnotation::Variable {
+            qualified_name,
+            variable_type,
+            ..
+        }) = context.ast_annotation.get(statement)
+        {
+            //check if we're accessing a private variable AND the variable's qualifier is not the
+            //POU we're accessing it from
+            if variable_type.is_private()
+                && context
+                    .qualifier
+                    .map_or(false, |q| !qualified_name.starts_with(q))
+            {
+                self.diagnostics.push(Diagnostic::illegal_access(
+                    qualified_name.as_str(),
+                    location.clone(),
+                ));
+            }
         }
     }
 

src/validation/tests/reference_resolve_tests.rs

@@ -236,3 +236,65 @@ fn resolve_function_members_via_qualifier() {
         ]
     );
 }
+
+/// tests whether references to privater variables do resolve, but end up in an validation problem
+#[test]
+fn reference_to_private_variable_is_illegal() {
+    let diagnostics = parse_and_validate(
+        "
+            PROGRAM prg
+                VAR 
+                    s : INT;
+                END_VAR
+            END_PROGRAM
+
+            FUNCTION foo : INT
+                prg.s := 7;
+            END_FUNCTION
+       ",
+    );
+
+    assert_eq!(
+        diagnostics,
+        vec![Diagnostic::illegal_access("prg.s", (175..176).into()),]
+    );
+}
+
+/// tests whether an intermediate access like: `a.priv.b` (where priv is a var_local)
+/// produces the correct error message without follow-up errors (b)
+#[test]
+fn reference_to_private_variable_in_intermediate_fb() {
+    // GIVEN a qualified reference prg.a.f.x where f is a
+    // private member of a functionblock (VAR)
+    // WHEN this is validated
+    let diagnostics = parse_and_validate(
+        "
+            FUNCTION_BLOCK fb1
+                VAR
+                    f: fb2;
+                END_VAR
+            END_FUNCTION_BLOCK
+
+            FUNCTION_BLOCK fb2
+                VAR_INPUT
+                    x : INT;                    
+                END_VAR
+            END_FUNCTION_BLOCK
+
+
+            PROGRAM prg
+                VAR
+                    a: fb1;
+                END_VAR
+                a.f.x := 7;
+            END_PROGRAM
+       ",
+    );
+
+    // THEN we get a validtion-error for accessing fb1.f, but no follow-up errors for
+    // the access of fb2 which is legit
+    assert_eq!(
+        diagnostics,
+        vec![Diagnostic::illegal_access("fb1.f", (413..414).into()),]
+    );
+}