Refactor analyzer name resolution (resolve_name/NamedThing)

crates/analyzer/src/builtins.rs

@@ -1,54 +1,4 @@
-use std::str::FromStr;
-use strum::{AsRefStr, EnumString, IntoStaticStr};
-
-#[derive(Debug, PartialEq, EnumString, AsRefStr)]
-#[strum(serialize_all = "lowercase")]
-pub enum ReservedName {
-    Type,
-    Object,
-    // Module, // someday: "std"
-    Function,
-}
-
-/// Check if a name shadows a built-in type, object, or function.
-/// Ideally, some of these things will move into a .fe standard library,
-/// and we won't need some of this special name collision logic.
-pub fn reserved_name(name: &str) -> Option<ReservedName> {
-    if TypeName::from_str(name).is_ok() {
-        Some(ReservedName::Type)
-    } else if Object::from_str(name).is_ok() {
-        Some(ReservedName::Object)
-    } else if GlobalMethod::from_str(name).is_ok() {
-        Some(ReservedName::Function)
-    } else {
-        None
-    }
-}
-
-#[derive(Debug, PartialEq, EnumString)]
-#[strum(serialize_all = "lowercase")]
-pub enum TypeName {
-    // Array, // when syntax is changed
-    #[strum(serialize = "Map")]
-    Map,
-    #[strum(serialize = "String")]
-    String_,
-
-    Address,
-    Bool,
-    I8,
-    I16,
-    I32,
-    I64,
-    I128,
-    I256,
-    U8,
-    U16,
-    U32,
-    U64,
-    U128,
-    U256,
-}
+use strum::{AsRefStr, EnumIter, EnumString};
 
 #[derive(Debug, PartialEq, EnumString)]
 #[strum(serialize_all = "snake_case")]
@@ -58,20 +8,20 @@ pub enum ValueMethod {
     AbiEncode,
 }
 
-#[derive(Clone, Copy, Debug, PartialEq, Eq, EnumString, IntoStaticStr, Hash)]
+#[derive(Clone, Copy, Debug, PartialEq, Eq, EnumString, AsRefStr, Hash, EnumIter)]
 #[strum(serialize_all = "snake_case")]
 pub enum GlobalMethod {
     Keccak256,
 }
 
-#[derive(strum::ToString, Debug, PartialEq, EnumString)]
+#[derive(Debug, PartialEq, EnumString, AsRefStr)]
 #[strum(serialize_all = "snake_case")]
 pub enum ContractTypeMethod {
     Create,
     Create2,
 }
 
-#[derive(strum::ToString, Debug, PartialEq, EnumString)]
+#[derive(Copy, Clone, Debug, Eq, PartialEq, EnumString, EnumIter, AsRefStr)]
 #[strum(serialize_all = "lowercase")]
 pub enum Object {
     Block,

crates/analyzer/src/context.rs

@@ -1,7 +1,8 @@
 use crate::builtins::GlobalMethod;
 use crate::errors::{self, CannotMove, TypeError};
-use crate::namespace::items::{DiagnosticSink, EventId};
+use crate::namespace::items::{DiagnosticSink, EventId, FunctionId, Item};
 use crate::namespace::types::{FixedSize, Type};
+use crate::AnalyzerDb;
 use fe_common::diagnostics::Diagnostic;
 pub use fe_common::diagnostics::Label;
 use fe_common::Span;
@@ -26,8 +27,9 @@ impl<T> Analysis<T> {
 }
 
 pub trait AnalyzerContext {
-    fn resolve_type(&self, name: &str) -> Option<Result<Type, TypeError>>;
+    fn resolve_name(&self, name: &str) -> Option<NamedThing>;
     fn add_diagnostic(&mut self, diag: Diagnostic);
+    fn db(&self) -> &dyn AnalyzerDb;
 
     fn error(&mut self, message: &str, label_span: Span, label: &str) -> DiagnosticVoucher {
         self.register_diag(errors::error(message, label_span, label))
@@ -68,12 +70,62 @@ pub trait AnalyzerContext {
         ))
     }
 
+    fn name_conflict_error(
+        &mut self,
+        name_kind: &str, // Eg "function parameter" or "variable name"
+        name: &str,
+        original: &NamedThing,
+        original_span: Option<Span>,
+        duplicate_span: Span,
+    ) -> DiagnosticVoucher {
+        self.register_diag(errors::name_conflict_error(
+            name_kind,
+            name,
+            original,
+            original_span,
+            duplicate_span,
+        ))
+    }
+
     fn register_diag(&mut self, diag: Diagnostic) -> DiagnosticVoucher {
         self.add_diagnostic(diag);
         DiagnosticVoucher(PhantomData::default())
     }
 }
 
+#[derive(Clone, Debug, PartialEq, Eq)]
+pub enum NamedThing {
+    Item(Item),
+    Variable {
+        name: String,
+        typ: Result<FixedSize, TypeError>,
+        span: Span,
+    },
+}
+
+impl NamedThing {
+    pub fn name_span(&self, db: &dyn AnalyzerDb) -> Option<Span> {
+        match self {
+            NamedThing::Item(item) => item.name_span(db),
+            NamedThing::Variable { span, .. } => Some(*span),
+        }
+    }
+
+    pub fn is_builtin(&self) -> bool {
+        match self {
+            NamedThing::Item(item) => item.is_builtin(),
+            NamedThing::Variable { .. } => false,
+        }
+    }
+
+    pub fn item_kind_display_name(&self) -> &str {
+        match self {
+            NamedThing::Item(item) => item.item_kind_display_name(),
+            NamedThing::Variable { .. } => "variable",
+        }
+    }
+}
+
 /// This should only be created by [`AnalyzerContext`].
 #[derive(Debug, Clone, PartialEq, Eq, Hash)]
 pub struct DiagnosticVoucher(PhantomData<()>);
@@ -83,8 +135,11 @@ pub struct TempContext {
     pub diagnostics: Vec<Diagnostic>,
 }
 impl AnalyzerContext for TempContext {
-    fn resolve_type(&self, _name: &str) -> Option<Result<Type, TypeError>> {
-        panic!("TempContext can't resolve types")
+    fn db(&self) -> &dyn AnalyzerDb {
+        panic!("TempContext has no analyzer db")
+    }
+    fn resolve_name(&self, _name: &str) -> Option<NamedThing> {
+        panic!("TempContext can't resolve names")
     }
     fn add_diagnostic(&mut self, diag: Diagnostic) {
         self.diagnostics.push(diag)
@@ -189,10 +244,10 @@ impl fmt::Display for ExpressionAttributes {
 /// The type of a function call.
 #[derive(Clone, Debug, PartialEq, Eq)]
 pub enum CallType {
-    BuiltinFunction { func: GlobalMethod },
+    BuiltinFunction(GlobalMethod),
     TypeConstructor { typ: Type },
     SelfAttribute { func_name: String, self_span: Span },
-    Pure { func_name: String },
+    Pure(FunctionId),
     ValueAttribute,
     TypeAttribute { typ: Type, func_name: String },
 }

crates/analyzer/src/db.rs

@@ -1,8 +1,8 @@
 use crate::context::{Analysis, FunctionBody};
 use crate::errors::TypeError;
 use crate::namespace::items::{
-    self, ContractFieldId, ContractId, EventId, FunctionId, ModuleConstantId, ModuleId,
-    StructFieldId, StructId, TypeAliasId, TypeDefId,
+    self, ContractFieldId, ContractId, EventId, FunctionId, Item, ModuleConstantId, ModuleId,
+    StructFieldId, StructId, TypeAliasId,
 };
 use crate::namespace::types;
 use indexmap::IndexMap;
@@ -48,23 +48,16 @@ pub trait AnalyzerDb {
     fn intern_event(&self, data: Rc<items::Event>) -> EventId;
 
     // Module
-    #[salsa::invoke(queries::module::module_all_type_defs)]
-    fn module_all_type_defs(&self, module: ModuleId) -> Rc<Vec<TypeDefId>>;
-    #[salsa::invoke(queries::module::module_type_def_map)]
-    fn module_type_def_map(&self, module: ModuleId) -> Analysis<Rc<IndexMap<String, TypeDefId>>>;
-    #[salsa::invoke(queries::module::module_resolve_type)]
-    #[salsa::cycle(queries::module::module_resolve_type_cycle)]
-    fn module_resolve_type(
-        &self,
-        module: ModuleId,
-        name: String,
-    ) -> Option<Result<types::Type, TypeError>>;
+    #[salsa::invoke(queries::module::module_all_items)]
+    fn module_all_items(&self, module: ModuleId) -> Rc<Vec<Item>>;
+    #[salsa::invoke(queries::module::module_item_map)]
+    fn module_item_map(&self, module: ModuleId) -> Analysis<Rc<IndexMap<String, Item>>>;
+    #[salsa::invoke(queries::module::module_imported_item_map)]
+    fn module_imported_item_map(&self, module: ModuleId) -> Rc<IndexMap<String, Item>>;
     #[salsa::invoke(queries::module::module_contracts)]
     fn module_contracts(&self, module: ModuleId) -> Rc<Vec<ContractId>>;
     #[salsa::invoke(queries::module::module_structs)]
     fn module_structs(&self, module: ModuleId) -> Rc<Vec<StructId>>;
-    #[salsa::invoke(queries::module::module_constants)]
-    fn module_constants(&self, module: ModuleId) -> Rc<Vec<ModuleConstantId>>;
 
     // Module Constant
     #[salsa::invoke(queries::module::module_constant_type)]

crates/analyzer/src/db/queries/contracts.rs

@@ -1,8 +1,7 @@
-use crate::builtins;
-use crate::context::AnalyzerContext;
+use crate::context::{AnalyzerContext, NamedThing};
 use crate::db::{Analysis, AnalyzerDb};
 use crate::errors;
-use crate::namespace::items::{self, ContractFieldId, ContractId, EventId, FunctionId};
+use crate::namespace::items::{self, ContractFieldId, ContractId, EventId, FunctionId, Item};
 use crate::namespace::scopes::ItemScope;
 use crate::namespace::types;
 use crate::traversal::types::type_desc;
@@ -13,6 +12,7 @@ use indexmap::map::{Entry, IndexMap};
 use std::rc::Rc;
 
 pub fn contract_all_functions(db: &dyn AnalyzerDb, contract: ContractId) -> Rc<Vec<FunctionId>> {
+    let module = contract.module(db);
     let body = &contract.data(db).ast.kind.body;
     Rc::new(
         body.iter()
@@ -21,7 +21,8 @@ pub fn contract_all_functions(db: &dyn AnalyzerDb, contract: ContractId) -> Rc<V
                 ast::ContractStmt::Function(node) => {
                     Some(db.intern_function(Rc::new(items::Function {
                         ast: node.clone(),
-                        parent: contract,
+                        contract,
+                        module,
                     })))
                 }
             })
@@ -42,14 +43,25 @@ pub fn contract_function_map(
         if def_name == "__init__" {
             continue;
         }
-        if let Some(reserved) = builtins::reserved_name(def_name) {
-            scope.error(
-                &format!(
-                    "function name conflicts with built-in {}",
-                    reserved.as_ref()
-                ),
+
+        if let Some(event) = contract.event(db, def_name) {
+            scope.name_conflict_error(
+                "function",
+                def_name,
+                &NamedThing::Item(Item::Event(event)),
+                Some(event.name_span(db)),
+                def.kind.name.span,
+            );
+            continue;
+        }
+
+        if let Some(named_item) = scope.resolve_name(def_name) {
+            scope.name_conflict_error(
+                "function",
+                def_name,
+                &named_item,
+                named_item.name_span(db),
                 def.kind.name.span,
-                &format!("`{}` is a built-in {}", def_name, reserved.as_ref()),
             );
             continue;
         }

crates/analyzer/src/db/queries/functions.rs

@@ -66,7 +66,16 @@ pub fn function_signature(
                     ))),
                 });
 
-                if let Some(dup_idx) = names.get(&name.kind) {
+                if let Some(named_item) = scope.resolve_name(&name.kind) {
+                    scope.name_conflict_error(
+                        "function parameter",
+                        &name.kind,
+                        &named_item,
+                        named_item.name_span(db),
+                        name.span,
+                    );
+                    None
+                } else if let Some(dup_idx) = names.get(&name.kind) {
                     let dup_arg: &Node<ast::FunctionArg> = &def.args[*dup_idx];
                     scope.duplicate_name_error(
                         &format!("duplicate parameter names in function `{}`", def.name.kind),
@@ -156,8 +165,6 @@ pub fn function_body(db: &dyn AnalyzerDb, function: FunctionId) -> Analysis<Rc<F
 
     let mut block_scope = BlockScope::new(&scope, BlockScopeType::Function);
 
-    add_module_constants_to_scope(db, &mut block_scope);
-
     // If `traverse_statements` fails, we can be confident that a diagnostic
     // has been emitted, either while analyzing this fn body or while analyzing
     // a type or fn used in this fn body, because of the `DiagnosticVoucher`
@@ -169,32 +176,6 @@ pub fn function_body(db: &dyn AnalyzerDb, function: FunctionId) -> Analysis<Rc<F
     }
 }
 
-fn add_module_constants_to_scope(db: &dyn AnalyzerDb, scope: &mut BlockScope) {
-    for module_const in scope.root.function.module(db).all_constants(db).iter() {
-        let typ = match module_const.typ(db) {
-            Ok(typ) => typ,
-            Err(_) => {
-                // No need to report module_constant_type() already reports this
-                continue;
-            }
-        };
-
-        let const_type: FixedSize = match typ.try_into() {
-            Ok(typ) => typ,
-            Err(_) => {
-                // No need to report module_constant_type() already reports this
-                continue;
-            }
-        };
-
-        let const_name = module_const.name(db);
-        // add_var emits a msg on err; we can ignore the Result.
-        scope
-            .add_var(&const_name, const_type, module_const.span(db))
-            .ok();
-    }
-}
-
 fn all_paths_return_or_revert(block: &[Node<ast::FuncStmt>]) -> bool {
     for statement in block.iter().rev() {
         match &statement.kind {