From 078d5df691d4ea48e83c9530cd40b64917eba0a7 Mon Sep 17 00:00:00 2001 From: jfecher Date: Fri, 22 Sep 2023 18:30:39 -0500 Subject: [PATCH] fix: Fix panic in some cases when calling a private function (#2799) --- .../src/hir/def_collector/dc_mod.rs | 1 + .../src/monomorphization/mod.rs | 125 +----------------- compiler/noirc_frontend/src/node_interner.rs | 10 +- 3 files changed, 11 insertions(+), 125 deletions(-) diff --git a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs index 98dfa74a655..53c8947d046 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs @@ -252,6 +252,7 @@ impl<'a> ModCollector<'a> { let method_name = name.0.contents.clone(); let func_id = context.def_interner.push_empty_fn(); let modifiers = FunctionModifiers { + name: name.0.contents.clone(), // trait functions are always public visibility: crate::Visibility::Public, attributes: Attributes::empty(), diff --git a/compiler/noirc_frontend/src/monomorphization/mod.rs b/compiler/noirc_frontend/src/monomorphization/mod.rs index 21e9fa5cf28..e519469ab2c 100644 --- a/compiler/noirc_frontend/src/monomorphization/mod.rs +++ b/compiler/noirc_frontend/src/monomorphization/mod.rs @@ -1344,88 +1344,14 @@ fn undo_instantiation_bindings(bindings: TypeBindings) { mod tests { use std::collections::{BTreeMap, HashMap}; - use fm::FileId; - use iter_extended::vecmap; - use noirc_errors::Location; - use crate::{ graph::CrateId, hir::{ - def_map::{CrateDefMap, LocalModuleId, ModuleData, ModuleDefId, ModuleId}, - resolution::{ - import::PathResolutionError, path_resolver::PathResolver, resolver::Resolver, - }, + def_map::{CrateDefMap, LocalModuleId, ModuleDefId, ModuleId}, + resolution::{import::PathResolutionError, path_resolver::PathResolver}, }, - node_interner::{FuncId, NodeInterner}, - parse_program, }; - use super::monomorphize; - - // TODO: refactor into a more general test utility? - // mostly copied from hir / type_check / mod.rs and adapted a bit - fn type_check_src_code(src: &str, func_namespace: Vec) -> (FuncId, NodeInterner) { - let (program, errors) = parse_program(src); - let mut interner = NodeInterner::default(); - - // Using assert_eq here instead of assert(errors.is_empty()) displays - // the whole vec if the assert fails rather than just two booleans - assert_eq!(errors, vec![]); - - let main_id = interner.push_test_function_definition("main".into()); - - let func_ids = - vecmap(&func_namespace, |name| interner.push_test_function_definition(name.into())); - - let mut path_resolver = TestPathResolver(HashMap::new()); - for (name, id) in func_namespace.into_iter().zip(func_ids.clone()) { - path_resolver.insert_func(name.to_owned(), id); - } - - let mut def_maps = BTreeMap::new(); - let file = FileId::default(); - - let mut modules = arena::Arena::new(); - let location = Location::new(Default::default(), file); - modules.insert(ModuleData::new(None, location, false)); - - def_maps.insert( - CrateId::dummy_id(), - CrateDefMap { - root: path_resolver.local_module_id(), - modules, - krate: CrateId::dummy_id(), - extern_prelude: BTreeMap::new(), - }, - ); - - let func_meta = vecmap(program.functions, |nf| { - let resolver = Resolver::new(&mut interner, &path_resolver, &def_maps, file); - let (hir_func, func_meta, _resolver_errors) = resolver.resolve_function(nf, main_id); - // TODO: not sure why, we do get an error here, - // but otherwise seem to get an ok monomorphization result - // assert_eq!(resolver_errors, vec![]); - (hir_func, func_meta) - }); - - println!("Before update_fn"); - - for ((hir_func, meta), func_id) in func_meta.into_iter().zip(func_ids.clone()) { - interner.update_fn(func_id, hir_func); - interner.push_fn_meta(meta, func_id); - } - - println!("Before type_check_func"); - - // Type check section - let errors = crate::hir::type_check::type_check_func( - &mut interner, - func_ids.first().cloned().unwrap(), - ); - assert_eq!(errors, vec![]); - (func_ids.first().cloned().unwrap(), interner) - } - // TODO: refactor into a more general test utility? // TestPathResolver struct and impls copied from hir / type_check / mod.rs struct TestPathResolver(HashMap); @@ -1452,51 +1378,4 @@ mod tests { ModuleId { krate: CrateId::dummy_id(), local_id: self.local_module_id() } } } - - impl TestPathResolver { - fn insert_func(&mut self, name: String, func_id: FuncId) { - self.0.insert(name, func_id.into()); - } - } - - // a helper test method - // TODO: maybe just compare trimmed src/expected - // for easier formatting? - fn check_rewrite(src: &str, expected: &str) { - let (func, interner) = type_check_src_code(src, vec!["main".to_string()]); - let program = monomorphize(func, &interner); - // println!("[{}]", program); - assert!(format!("{}", program) == expected); - } - - #[test] - fn simple_closure_with_no_captured_variables() { - let src = r#" - fn main() -> pub Field { - let x = 1; - let closure = || x; - closure() - } - "#; - - let expected_rewrite = r#"fn main$f0() -> Field { - let x$0 = 1; - let closure$3 = { - let closure_variable$2 = { - let env$1 = (x$l0); - (env$l1, lambda$f1) - }; - closure_variable$l2 - }; - { - let tmp$4 = closure$l3; - tmp$l4.1(tmp$l4.0) - } -} -fn lambda$f1(mut env$l1: (Field)) -> Field { - env$l1.0 -} -"#; - check_rewrite(src, expected_rewrite); - } } diff --git a/compiler/noirc_frontend/src/node_interner.rs b/compiler/noirc_frontend/src/node_interner.rs index 72d71425b51..01633c29178 100644 --- a/compiler/noirc_frontend/src/node_interner.rs +++ b/compiler/noirc_frontend/src/node_interner.rs @@ -114,7 +114,12 @@ pub struct NodeInterner { primitive_methods: HashMap<(TypeMethodKey, String), FuncId>, } +/// All the information from a function that is filled out during definition collection rather than +/// name resolution. Resultingly, if information about a function is needed during name resolution, +/// this is the only place where it is safe to retrieve it (where all fields are guaranteed to be initialized). pub struct FunctionModifiers { + pub name: String, + /// Whether the function is `pub` or not. pub visibility: Visibility, @@ -138,6 +143,7 @@ impl FunctionModifiers { #[allow(clippy::new_without_default)] pub fn new() -> Self { Self { + name: String::new(), visibility: Visibility::Public, attributes: Attributes::empty(), is_unconstrained: false, @@ -594,6 +600,7 @@ impl NodeInterner { // We're filling in contract_function_type and is_internal now, but these will be verified // later during name resolution. let modifiers = FunctionModifiers { + name: function.name.0.contents.clone(), visibility: if function.is_public { Visibility::Public } else { Visibility::Private }, attributes: function.attributes.clone(), is_unconstrained: function.is_unconstrained, @@ -656,8 +663,7 @@ impl NodeInterner { } pub fn function_name(&self, func_id: &FuncId) -> &str { - let name_id = self.function_meta(func_id).name.id; - self.definition_name(name_id) + &self.function_modifiers[func_id].name } pub fn function_modifiers(&self, func_id: &FuncId) -> &FunctionModifiers {