fix: Impl methods are no longer placed in contracts (#3255)

compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs

@@ -1173,6 +1173,11 @@ fn resolve_function_set(
         resolver.set_self_type(self_type.clone());
         resolver.set_trait_id(unresolved_functions.trait_id);
 
+        // Without this, impl methods can accidentally be placed in contracts. See #3254
+        if self_type.is_some() {
+            resolver.set_in_contract(false);
+        }
+
         let (hir_func, func_meta, errs) = resolver.resolve_function(func, func_id);
         interner.push_fn_meta(func_meta, func_id);
         interner.update_fn(func_id, hir_func);

compiler/noirc_frontend/src/hir/resolution/resolver.rs

@@ -87,6 +87,14 @@ pub struct Resolver<'a> {
     /// Set to the current type if we're resolving an impl
     self_type: Option<Type>,
 
+    /// True if the current module is a contract.
+    /// This is usually determined by self.path_resolver.module_id(), but it can
+    /// be overriden for impls. Impls are an odd case since the methods within resolve
+    /// as if they're in the parent module, but should be placed in a child module.
+    /// Since they should be within a child module, in_contract is manually set to false
+    /// for these so we can still resolve them in the parent module without them being in a contract.
+    in_contract: bool,
+
     /// Contains a mapping of the current struct or functions's generics to
     /// unique type variables if we're resolving a struct. Empty otherwise.
     /// This is a Vec rather than a map to preserve the order a functions generics
@@ -119,6 +127,9 @@ impl<'a> Resolver<'a> {
         def_maps: &'a BTreeMap<CrateId, CrateDefMap>,
         file: FileId,
     ) -> Resolver<'a> {
+        let module_id = path_resolver.module_id();
+        let in_contract = module_id.module(def_maps).is_contract;
+
         Self {
             path_resolver,
             def_maps,
@@ -131,6 +142,7 @@ impl<'a> Resolver<'a> {
             errors: Vec::new(),
             lambda_stack: Vec::new(),
             file,
+            in_contract,
         }
     }
 
@@ -805,17 +817,24 @@ impl<'a> Resolver<'a> {
         }
     }
 
+    /// Override whether this name resolver is within a contract or not.
+    /// This will affect which types are allowed as parameters to methods as well
+    /// as which modifiers are allowed on a function.
+    pub(crate) fn set_in_contract(&mut self, in_contract: bool) {
+        self.in_contract = in_contract;
+    }
+
     /// True if the 'pub' keyword is allowed on parameters in this function
     fn pub_allowed(&self, func: &NoirFunction) -> bool {
-        if self.in_contract() {
+        if self.in_contract {
             !func.def.is_unconstrained
         } else {
             func.name() == MAIN_FUNCTION
         }
     }
 
     fn is_entry_point_function(&self, func: &NoirFunction) -> bool {
-        if self.in_contract() {
+        if self.in_contract {
             func.attributes().is_contract_entry_point()
         } else {
             func.name() == MAIN_FUNCTION
@@ -824,7 +843,7 @@ impl<'a> Resolver<'a> {
 
     /// True if the `distinct` keyword is allowed on a function's return type
     fn distinct_allowed(&self, func: &NoirFunction) -> bool {
-        if self.in_contract() {
+        if self.in_contract {
             // "open" and "unconstrained" functions are compiled to brillig and thus duplication of
             // witness indices in their abis is not a concern.
             !func.def.is_unconstrained && !func.def.is_open
@@ -836,15 +855,15 @@ impl<'a> Resolver<'a> {
     fn handle_function_type(&mut self, function: &FuncId) {
         let function_type = self.interner.function_modifiers(function).contract_function_type;
 
-        if !self.in_contract() && function_type == Some(ContractFunctionType::Open) {
+        if !self.in_contract && function_type == Some(ContractFunctionType::Open) {
             let span = self.interner.function_ident(function).span();
             self.errors.push(ResolverError::ContractFunctionTypeInNormalFunction { span });
             self.interner.function_modifiers_mut(function).contract_function_type = None;
         }
     }
 
     fn handle_is_function_internal(&mut self, function: &FuncId) {
-        if !self.in_contract() {
+        if !self.in_contract {
             if self.interner.function_modifiers(function).is_internal == Some(true) {
                 let span = self.interner.function_ident(function).span();
                 self.push_err(ResolverError::ContractFunctionInternalInNormalFunction { span });
@@ -1605,11 +1624,6 @@ impl<'a> Resolver<'a> {
         }
     }
 
-    fn in_contract(&self) -> bool {
-        let module_id = self.path_resolver.module_id();
-        module_id.module(self.def_maps).is_contract
-    }
-
     fn resolve_fmt_str_literal(&mut self, str: String, call_expr_span: Span) -> HirLiteral {
         let re = Regex::new(r"\{([a-zA-Z0-9_]+)\}")
             .expect("ICE: an invalid regex pattern was used for checking format strings");

tooling/nargo_cli/tests/compile_success_contract/contract_with_impl/Nargo.toml

@@ -0,0 +1,8 @@
+[package]
+name = "simple_contract"
+type = "contract"
+authors = [""]
+compiler_version = "0.1"
+
+[dependencies]
+

tooling/nargo_cli/tests/compile_success_contract/contract_with_impl/src/main.nr

@@ -0,0 +1,8 @@
+
+contract Foo {
+    struct T { x: [Field] }
+
+    impl T {
+        fn t(self){}
+    }
+}