chore: replace cached in_contract with in_contract() method (#5324)

# Description

## Problem\*

Resolves
#5298 (comment)

## Summary\*

This PR replaces the `in_contract` field with a `in_contract` function.
This does expand the `self_type.is_some()` override to a couple of
locations however which I need to check whether it's going to cause
issues.

## Additional Context



## Documentation\*

Check one:
- [x] No documentation needed.
- [ ] Documentation included in this PR.
- [ ] **[For Experimental Features]** Documentation to be submitted in a
separate PR.

# PR Checklist\*

- [x] I have tested the changes locally.
- [x] I have formatted the changes with [Prettier](https://prettier.io/)
and/or `cargo fmt` on default settings.

compiler/noirc_frontend/src/elaborator/mod.rs

@@ -93,14 +93,6 @@ pub struct Elaborator<'context> {
     in_unconstrained_fn: bool,
     nested_loops: usize,
 
-    /// True if the current module is a contract.
-    /// This is usually determined by self.path_resolver.module_id(), but it can
-    /// be overridden 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
@@ -183,7 +175,6 @@ impl<'context> Elaborator<'context> {
             file: FileId::dummy(),
             in_unconstrained_fn: false,
             nested_loops: 0,
-            in_contract: false,
             generics: Vec::new(),
             lambda_stack: Vec::new(),
             self_type: None,
@@ -319,12 +310,6 @@ impl<'context> Elaborator<'context> {
 
         let old_function = std::mem::replace(&mut self.current_function, Some(id));
 
-        // Without this, impl methods can accidentally be placed in contracts. See #3254
-        let was_in_contract = self.in_contract;
-        if self.self_type.is_some() {
-            self.in_contract = false;
-        }
-
         self.scopes.start_function();
         let old_item = std::mem::replace(&mut self.current_item, Some(DependencyId::Function(id)));
 
@@ -429,7 +414,6 @@ impl<'context> Elaborator<'context> {
         self.type_variables.clear();
         self.in_unconstrained_fn = false;
         self.interner.update_fn(id, hir_func);
-        self.in_contract = was_in_contract;
         self.current_function = old_function;
         self.current_item = old_item;
     }
@@ -610,11 +594,13 @@ impl<'context> Elaborator<'context> {
     ) {
         self.current_function = Some(func_id);
 
-        // Without this, impl methods can accidentally be placed in contracts. See #3254
-        let was_in_contract = self.in_contract;
-        if self.self_type.is_some() {
-            self.in_contract = false;
-        }
+        let in_contract = if self.self_type.is_some() {
+            // Without this, impl methods can accidentally be placed in contracts.
+            // See: https://github.com/noir-lang/noir/issues/3254
+            false
+        } else {
+            self.in_contract()
+        };
 
         self.scopes.start_function();
         self.current_item = Some(DependencyId::Function(func_id));
@@ -623,12 +609,13 @@ impl<'context> Elaborator<'context> {
         let id = self.interner.function_definition_id(func_id);
         let name_ident = HirIdent::non_trait_method(id, location);
 
-        let is_entry_point = self.is_entry_point_function(func);
+        let is_entry_point = self.is_entry_point_function(func, in_contract);
 
         self.run_lint(|_| lints::inlining_attributes(func).map(Into::into));
         self.run_lint(|_| lints::missing_pub(func, is_entry_point).map(Into::into));
         self.run_lint(|elaborator| {
-            lints::unnecessary_pub_return(func, elaborator.pub_allowed(func)).map(Into::into)
+            lints::unnecessary_pub_return(func, elaborator.pub_allowed(func, in_contract))
+                .map(Into::into)
         });
         self.run_lint(|_| lints::oracle_not_marked_unconstrained(func).map(Into::into));
         self.run_lint(|elaborator| {
@@ -644,7 +631,7 @@ impl<'context> Elaborator<'context> {
         let has_no_predicates_attribute = func.attributes().is_no_predicates();
         let should_fold = func.attributes().is_foldable();
         let has_inline_attribute = has_no_predicates_attribute || should_fold;
-        let is_pub_allowed = self.pub_allowed(func);
+        let is_pub_allowed = self.pub_allowed(func, in_contract);
         self.add_generics(&func.def.generics);
 
         let mut trait_constraints = self.resolve_trait_constraints(&func.def.where_clause);
@@ -727,7 +714,6 @@ impl<'context> Elaborator<'context> {
         };
 
         self.interner.push_fn_meta(meta, func_id);
-        self.in_contract = was_in_contract;
         self.current_function = None;
         self.scopes.end_function();
         self.current_item = None;
@@ -752,12 +738,23 @@ impl<'context> Elaborator<'context> {
 
     /// True if the `pub` keyword is allowed on parameters in this function
     /// `pub` on function parameters is only allowed for entry point functions
-    fn pub_allowed(&self, func: &NoirFunction) -> bool {
-        self.is_entry_point_function(func) || func.attributes().is_foldable()
+    fn pub_allowed(&self, func: &NoirFunction, in_contract: bool) -> bool {
+        self.is_entry_point_function(func, in_contract) || func.attributes().is_foldable()
+    }
+
+    /// Returns `true` if the current module is a contract.
+    ///
+    /// This is usually determined by `self.module_id()`, but it can
+    /// be overridden 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, they should be elaborated as if
+    /// `in_contract` is `false` so we can still resolve them in the parent module without them being in a contract.
+    fn in_contract(&self) -> bool {
+        self.module_id().module(self.def_maps).is_contract
     }
 
-    fn is_entry_point_function(&self, func: &NoirFunction) -> bool {
-        if self.in_contract {
+    fn is_entry_point_function(&self, func: &NoirFunction, in_contract: bool) -> bool {
+        if in_contract {
             func.attributes().is_contract_entry_point()
         } else {
             func.name() == MAIN_FUNCTION
@@ -1289,7 +1286,7 @@ impl<'context> Elaborator<'context> {
         self.current_item = Some(DependencyId::Global(global_id));
         let let_stmt = global.stmt_def;
 
-        if !self.module_id().module(self.def_maps).is_contract
+        if !self.in_contract()
             && let_stmt.attributes.iter().any(|attr| matches!(attr, SecondaryAttribute::Abi(_)))
         {
             let span = let_stmt.pattern.span();
@@ -1418,12 +1415,9 @@ impl<'context> Elaborator<'context> {
 
         for (local_module, id, func) in &mut function_set.functions {
             self.local_module = *local_module;
-            let was_in_contract = self.in_contract;
-            self.in_contract = self.module_id().module(self.def_maps).is_contract;
             self.recover_generics(|this| {
                 this.define_function_meta(func, *id, false);
             });
-            self.in_contract = was_in_contract;
         }
     }
 

compiler/noirc_frontend/src/elaborator/types.rs

@@ -254,8 +254,7 @@ impl<'context> Elaborator<'context> {
                 }
 
                 let expected_generic_count = struct_type.borrow().generics.len();
-
-                if !self.in_contract
+                if !self.in_contract()
                     && self
                         .interner
                         .struct_attributes(&struct_type.borrow().id)