Review comments

The builtinin index is now created first in the lib, and not always by
visiting an ast.
Also fixed the index import to use not skip enum and hardware constants.
As a side effect of the index being imported, some index numbers changed
in some tests

Author: ghaith

src/builtins.rs

@@ -92,19 +92,7 @@ pub fn parse_built_ins(id_provider: IdProvider) -> (CompilationUnit, Vec<Diagnos
     parser::parse(lexer::lex_with_ids(&src, id_provider), LinkageType::BuiltIn)
 }
 
-pub fn generate<'ink, 'b>(
-    builtin: &str,
-    generator: &'b ExpressionCodeGenerator<'ink, 'b>,
-    params: Vec<&AstStatement>,
-    source_location: SourceRange,
-) -> Result<BasicValueEnum<'ink>, Diagnostic> {
-    BUILTIN
-        .get(builtin.to_uppercase().as_str())
-        .ok_or_else(|| {
-            Diagnostic::codegen_error(
-                &format!("Cannot find builtin function {}", builtin),
-                source_location.clone(),
-            )
-        })
-        .and_then(|it| it.codegen(generator, params.as_slice(), source_location))
+/// Returns the requested functio from the builtin index or None
+pub fn get_builtin(name: &str) -> Option<&'static BuiltIn> {
+    BUILTIN.get(name.to_uppercase().as_str())
 }

src/codegen/generators/expression_generator.rs

@@ -1,7 +1,6 @@
 // Copyright (c) 2020 Ghaith Hachem and Mathias Rieder
 use crate::{
     ast::{self, DirectAccessType, SourceRange},
-    builtins,
     codegen::llvm_typesystem,
     diagnostics::{Diagnostic, INTERNAL_LLVM_ERROR},
     index::{ImplementationIndexEntry, ImplementationType, Index, VariableIndexEntry},
@@ -451,14 +450,17 @@ impl<'a, 'b> ExpressionCodeGenerator<'a, 'b> {
             })?;
 
         //If the function is builtin, generate a basic value enum for it
-        if self.index.is_builtin(implementation.get_call_name()) {
-            return builtins::generate(
-                implementation.get_call_name(),
+        if let Some(builtin) = self
+            .index
+            .get_builtin_function(implementation.get_call_name())
+        {
+            return builtin.codegen(
                 self,
                 parameters
                     .as_ref()
                     .map(|it| ast::flatten_expression_list(it))
-                    .unwrap_or_default(),
+                    .unwrap_or_default()
+                    .as_slice(),
                 operator.get_location(),
             );
         }

src/index.rs

@@ -6,9 +6,8 @@ use crate::{
         AstStatement, DirectAccessType, HardwareAccessType, Implementation, LinkageType, PouType,
         SourceRange, TypeNature,
     },
-    builtins,
+    builtins::{self, BuiltIn},
     diagnostics::Diagnostic,
-    lexer::IdProvider,
     typesystem::{self, *},
 };
 
@@ -428,11 +427,6 @@ pub struct Index {
 }
 
 impl Index {
-    pub fn create_with_builtins(id_provider: IdProvider) -> Index {
-        let (unit, _) = builtins::parse_built_ins(id_provider.clone());
-        visitor::visit_index(Index::default(), &unit, id_provider)
-    }
-
     /// imports all entries from the given index into the current index
     ///
     /// imports all global_variables, member_variables, types and implementations
@@ -441,43 +435,41 @@ impl Index {
     /// into the current one
     pub fn import(&mut self, mut other: Index) {
         //global variables
-        for (name, mut e) in other.global_variables.drain(..) {
-            e.initial_value =
-                self.maybe_import_const_expr(&mut other.constant_expressions, &e.initial_value);
+        for (name, e) in other.global_variables.drain(..) {
+            let e = self.transfer_constants(e, &mut other.constant_expressions);
             self.global_variables.insert(name, e);
         }
 
-        //enum_global_variables
-        for (name, mut e) in other.enum_global_variables.drain(..) {
-            e.initial_value =
-                self.maybe_import_const_expr(&mut other.constant_expressions, &e.initial_value);
-            self.enum_global_variables.insert(name, e.clone());
-            self.enum_qualified_variables
-                .insert(e.qualified_name.to_lowercase(), e);
+        //enmu_variables use the qualified variables since name conflicts will be overriden in the enum_global
+        for (qualified_name, e) in other.enum_qualified_variables.drain(..) {
+            let e = self.transfer_constants(e, &mut other.constant_expressions);
+            dbg!(&e);
+            self.enum_global_variables
+                .insert(e.get_name().to_lowercase(), e.clone());
+            self.enum_qualified_variables.insert(qualified_name, e);
         }
 
         //initializers
-        for (name, mut e) in other.global_initializers.drain(..) {
-            e.initial_value =
-                self.maybe_import_const_expr(&mut other.constant_expressions, &e.initial_value);
+        for (name, e) in other.global_initializers.drain(..) {
+            let e = self.transfer_constants(e, &mut other.constant_expressions);
             self.global_initializers.insert(name, e);
         }
 
         //member variables
         for (name, mut members) in other.member_variables.drain(..) {
             //enum qualified variables
-            for (_, mut e) in members.iter_mut() {
-                e.initial_value =
-                    self.maybe_import_const_expr(&mut other.constant_expressions, &e.initial_value);
+            let mut new_members = IndexMap::default();
+            for (name, e) in members.drain(..) {
+                let e = self.transfer_constants(e, &mut other.constant_expressions);
+                new_members.insert(name, e);
             }
-            self.member_variables.insert(name, members);
+            self.member_variables.insert(name, new_members);
         }
 
         //types
         for (name, mut e) in other.type_index.types.drain(..) {
             e.initial_value =
                 self.maybe_import_const_expr(&mut other.constant_expressions, &e.initial_value);
-
             match &mut e.information {
                 //import constant expressions in array-type-definitions
                 DataTypeInformation::Array { dimensions, .. } => {
@@ -511,6 +503,38 @@ impl Index {
         // self.constant_expressions.import(other.constant_expressions)
     }
 
+    fn transfer_constants(
+        &mut self,
+        mut variable: VariableIndexEntry,
+        import_from: &mut ConstExpressions,
+    ) -> VariableIndexEntry {
+        variable.initial_value = self.maybe_import_const_expr(import_from, &variable.initial_value);
+
+        let binding = if let Some(HardwareBinding {
+            direction,
+            access,
+            entries,
+            location,
+        }) = variable.get_hardware_binding()
+        {
+            let mut new_entries = vec![];
+            for entry in entries {
+                if let Some(e) = self.maybe_import_const_expr(import_from, &Some(*entry)) {
+                    new_entries.push(e);
+                }
+            }
+            Some(HardwareBinding {
+                direction: *direction,
+                access: *access,
+                entries: new_entries,
+                location: location.clone(),
+            })
+        } else {
+            None
+        };
+        variable.set_hardware_binding(binding)
+    }
+
     /// imports the corresponding const-expression (according to the given initializer-id) from the given ConstExpressions
     /// into self's const-expressions and returns the new Id
     fn maybe_import_const_expr(
@@ -520,7 +544,7 @@ impl Index {
     ) -> Option<ConstId> {
         initializer_id
             .as_ref()
-            .and_then(|it| import_from.remove(it))
+            .and_then(|it| import_from.clone(it))
             .map(|(init, target_type, scope)| {
                 self.get_mut_const_expressions()
                     .add_constant_expression(init, target_type, scope)
@@ -540,7 +564,7 @@ impl Index {
         let ts = match type_size {
             TypeSize::LiteralInteger(_) => Some(*type_size),
             TypeSize::ConstExpression(id) => import_from
-                .remove(id)
+                .clone(id)
                 .map(|(expr, target_type, scope)| {
                     self.get_mut_const_expressions().add_constant_expression(
                         expr,
@@ -970,11 +994,17 @@ impl Index {
         InstanceIterator::with_filter(self, inner_filter)
     }
 
-    pub fn is_builtin(&self, function: &str) -> bool {
+    /// If the provided name is a builtin function, returns it from the builtin index
+    pub fn get_builtin_function(&self, name: &str) -> Option<&'_ BuiltIn> {
         //Find a type for that function, see if that type is builtin
-        self.find_effective_type_info(function)
+        if let Some(true) = self
+            .find_effective_type_info(name)
             .map(DataTypeInformation::is_builtin)
-            .unwrap_or_default()
+        {
+            builtins::get_builtin(name)
+        } else {
+            None
+        }
     }
 }
 

src/index/const_expressions.rs

@@ -115,14 +115,29 @@ impl ConstExpressions {
         self.expressions.get(*id).map(|it| &it.expr)
     }
 
-    /// removes the expression from the ConstExpressions and returns all of its elements
-    pub fn remove(&mut self, id: &ConstId) -> Option<(AstStatement, String, Option<String>)> {
-        self.expressions.remove(*id).map(|it| match it.expr {
-            ConstExpression::Unresolved { statement, scope } => {
-                (statement, it.target_type_name, scope)
+    // /// removes the expression from the ConstExpressions and returns all of its elements
+    // pub fn remove(&mut self, id: &ConstId) -> Option<(AstStatement, String, Option<String>)> {
+    //     self.expressions.remove(*id).map(|it| match it.expr {
+    //         ConstExpression::Unresolved { statement, scope } => {
+    //             (statement, it.target_type_name, scope)
+    //         }
+    //         ConstExpression::Resolved(s) => (s, it.target_type_name, None),
+    //         ConstExpression::Unresolvable { statement: s, .. } => (s, it.target_type_name, None),
+    //     })
+    // }
+
+    /// clones the expression in the ConstExpressions and returns all of its elements
+    pub fn clone(&self, id: &ConstId) -> Option<(AstStatement, String, Option<String>)> {
+        self.expressions.get(*id).map(|it| match &it.expr {
+            ConstExpression::Unresolved { statement, scope } => (
+                statement.clone(),
+                it.target_type_name.clone(),
+                scope.clone(),
+            ),
+            ConstExpression::Resolved(s) => (s.clone(), it.target_type_name.clone(), None),
+            ConstExpression::Unresolvable { statement: s, .. } => {
+                (s.clone(), it.target_type_name.clone(), None)
             }
-            ConstExpression::Resolved(s) => (s, it.target_type_name, None),
-            ConstExpression::Unresolvable { statement: s, .. } => (s, it.target_type_name, None),
         })
     }
 

src/index/tests/builtin_tests.rs

@@ -1,16 +1,19 @@
-use crate::{index::Index, lexer::IdProvider, test_utils::tests::index};
+use crate::{builtins, lexer::IdProvider, test_utils::tests::index};
 
 #[test]
 fn builtin_functions_added_to_index() {
-    let index = Index::create_with_builtins(IdProvider::default());
+    let provider = IdProvider::default();
+    let (builtins, _) = builtins::parse_built_ins(provider.clone());
+    let index = crate::index::visitor::visit(&builtins, provider);
+
     assert!(index.find_member("ADR", "in").is_some());
     assert!(index.find_member("REF", "in").is_some());
     assert!(index.find_implementation("ADR").is_some());
     assert!(index.find_implementation("REF").is_some());
 }
 
 #[test]
-fn default_visitor_creates_builtins() {
+fn test_indexer_has_builtins() {
     let (_, index) = index("");
     assert!(index.find_member("ADR", "in").is_some());
     assert!(index.find_member("REF", "in").is_some());

src/index/tests/snapshots/rusty__index__tests__instance_resolver_tests__array_with_const_instances_are_repeated.snap

@@ -1,6 +1,6 @@
 ---
 source: src/index/tests/instance_resolver_tests.rs
-assertion_line: 169
+assertion_line: 170
 expression: "index.find_instances().collect::<Vec<Instance<'_>>>()"
 
 ---
@@ -44,7 +44,7 @@ expression: "index.find_instances().collect::<Vec<Instance<'_>>>()"
             qualified_name: "MainProg.size",
             initial_value: Some(
                 Index {
-                    index: 2,
+                    index: 0,
                     generation: 0,
                 },
             ),
@@ -99,13 +99,13 @@ expression: "index.find_instances().collect::<Vec<Instance<'_>>>()"
                         Dimension {
                             start_offset: ConstExpression(
                                 Index {
-                                    index: 0,
+                                    index: 1,
                                     generation: 0,
                                 },
                             ),
                             end_offset: ConstExpression(
                                 Index {
-                                    index: 1,
+                                    index: 2,
                                     generation: 0,
                                 },
                             ),
@@ -146,13 +146,13 @@ expression: "index.find_instances().collect::<Vec<Instance<'_>>>()"
                         Dimension {
                             start_offset: ConstExpression(
                                 Index {
-                                    index: 0,
+                                    index: 1,
                                     generation: 0,
                                 },
                             ),
                             end_offset: ConstExpression(
                                 Index {
-                                    index: 1,
+                                    index: 2,
                                     generation: 0,
                                 },
                             ),

src/index/visitor.rs

@@ -9,7 +9,8 @@ use crate::index::{Index, MemberInfo};
 use crate::lexer::IdProvider;
 use crate::typesystem::{self, *};
 
-pub fn visit_index(mut index: Index, unit: &CompilationUnit, mut id_provider: IdProvider) -> Index {
+pub fn visit(unit: &CompilationUnit, mut id_provider: IdProvider) -> Index {
+    let mut index = Index::default();
     //Create the typesystem
     let builtins = get_builtin_types();
     for data_type in builtins {
@@ -37,12 +38,6 @@ pub fn visit_index(mut index: Index, unit: &CompilationUnit, mut id_provider: Id
     index
 }
 
-/// Visits the ast, creating an index of the content. Appends builtin functions to that index
-pub fn visit(unit: &CompilationUnit, id_provider: IdProvider) -> Index {
-    let index = Index::create_with_builtins(id_provider.clone());
-    visit_index(index, unit, id_provider)
-}
-
 pub fn visit_pou(index: &mut Index, pou: &Pou) {
     let interface_name = format!("{}_interface", &pou.name);
 

src/lib.rs

@@ -440,8 +440,13 @@ fn parse_and_index<T: SourceContainer>(
     linkage: LinkageType,
 ) -> Result<(Index, Units), Diagnostic> {
     let mut index = Index::default();
+
     let mut units = Vec::new();
 
+    //parse the builtins into the index
+    let (builtins, _) = builtins::parse_built_ins(id_provider.clone());
+    index.import(index::visitor::visit(&builtins, id_provider.clone()));
+
     for container in source {
         let location: String = container.get_location().into();
         let e = container