Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from
Jun 25, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
97c9429
chore: replace cached `in_contract` with `in_contract()` method
TomAFrench Jun 25, 2024
d276fbd
chore: avoid baking in impl special-case to `in_contract`
TomAFrench Jun 25, 2024
b7511db
Merge branch 'master' into tf/remove_in_contract
TomAFrench Jun 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 27 additions & 33 deletions compiler/noirc_frontend/src/elaborator/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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)));

Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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));
Expand All @@ -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| {
Expand All @@ -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);
TomAFrench marked this conversation as resolved.
Show resolved Hide resolved
self.add_generics(&func.def.generics);

let mut trait_constraints = self.resolve_trait_constraints(&func.def.where_clause);
Expand Down Expand Up @@ -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;
Expand All @@ -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
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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;
}
}

Expand Down
3 changes: 1 addition & 2 deletions compiler/noirc_frontend/src/elaborator/types.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
Loading