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.

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

feat: Allow methods defined in a contract to be non-entry points #2687

Merged
merged 6 commits into from
Sep 14, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
20 changes: 16 additions & 4 deletions compiler/noirc_driver/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -232,16 +232,28 @@ fn compile_contract_inner(
) -> Result<CompiledContract, ErrorsAndWarnings> {
let mut functions = Vec::new();
let mut errors = Vec::new();
for function_id in &contract.functions {
let name = context.function_name(function_id).to_owned();
let function = match compile_no_check(context, options, *function_id) {
for contract_function in &contract.functions {
let function_id = contract_function.function_id;
let is_entry_point = contract_function.is_entry_point;

let name = context.function_name(&function_id).to_owned();

// We assume that functions have already been type checked.
// This is the exact same assumption that compile_no_check makes.
// If it is not an entry-point point, we can then just skip the
// compilation step. It will also not be added to the ABI.
if !is_entry_point {
continue;
}

let function = match compile_no_check(context, options, function_id) {
Ok(function) => function,
Err(new_error) => {
errors.push(new_error);
continue;
}
};
let func_meta = context.def_interner.function_meta(function_id);
let func_meta = context.def_interner.function_meta(&function_id);
let func_type = func_meta
.contract_function_type
.expect("Expected contract function to have a contract visibility");
Expand Down
28 changes: 25 additions & 3 deletions compiler/noirc_frontend/src/hir/def_map/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -155,13 +155,23 @@ impl CrateDefMap {

/// Go through all modules in this crate, find all `contract ... { ... }` declarations,
/// and collect them all into a Vec.
pub fn get_all_contracts(&self) -> Vec<Contract> {
pub fn get_all_contracts(&self, interner: &NodeInterner) -> Vec<Contract> {
self.modules
.iter()
.filter_map(|(id, module)| {
if module.is_contract {
let functions =
let function_ids: Vec<FuncId> =
module.value_definitions().filter_map(|id| id.as_function()).collect();

let functions = function_ids
.into_iter()
.map(|id| {
Comment on lines +166 to +168
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't collect into function_ids then call into_iter immediately afterward, the intermediate Vec is not needed.

let is_entry_point =
!interner.function_attributes(&id).has_contract_library_method();
ContractFunctionMeta { function_id: id, is_entry_point }
})
.collect();

let name = self.get_module_path(id, module.parent);
Some(Contract { name, location: module.location, functions })
} else {
Expand Down Expand Up @@ -204,13 +214,25 @@ impl CrateDefMap {
}
}

/// Specifies a contract function and extra metadata that
/// one can use when processing a contract function.
///
/// One of these is whether the contract function is an entry point.
/// The caller should only type-check these functions and not attempt
/// to create a circuit for them.
pub struct ContractFunctionMeta {
pub function_id: FuncId,
/// Indicates whether the function is an entry point
pub is_entry_point: bool,
}

/// A 'contract' in Noir source code with the given name and functions.
/// This is not an AST node, it is just a convenient form to return for CrateDefMap::get_all_contracts.
pub struct Contract {
/// To keep `name` semi-unique, it is prefixed with the names of parent modules via CrateDefMap::get_module_path
pub name: String,
pub location: Location,
pub functions: Vec<FuncId>,
pub functions: Vec<ContractFunctionMeta>,
}

/// Given a FileId, fetch the File, from the FileManager and parse it's content
Expand Down
2 changes: 1 addition & 1 deletion compiler/noirc_frontend/src/hir/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ impl Context {
pub fn get_all_contracts(&self, crate_id: &CrateId) -> Vec<Contract> {
self.def_map(crate_id)
.expect("The local crate should be analyzed already")
.get_all_contracts()
.get_all_contracts(&self.def_interner)
}

fn module(&self, module_id: def_map::ModuleId) -> &def_map::ModuleData {
Expand Down
13 changes: 13 additions & 0 deletions compiler/noirc_frontend/src/lexer/lexer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -536,6 +536,19 @@ mod tests {
&Token::Attribute(Attribute::Primary(PrimaryAttribute::Test(TestScope::None)))
);
}

#[test]
fn contract_library_method_attribute() {
let input = r#"#[contract_library_method]"#;
let mut lexer = Lexer::new(input);

let token = lexer.next().unwrap().unwrap();
assert_eq!(
token.token(),
&Token::Attribute(Attribute::Secondary(SecondaryAttribute::ContractLibraryMethod))
);
}

#[test]
fn test_attribute_with_valid_scope() {
let input = r#"#[test(should_fail)]"#;
Expand Down
19 changes: 19 additions & 0 deletions compiler/noirc_frontend/src/lexer/token.rs
Original file line number Diff line number Diff line change
Expand Up @@ -374,6 +374,16 @@ impl Attributes {
Self { primary: None, secondary: Vec::new() }
}

/// Returns true if one of the secondary attributes is `contract_library_method`
///
/// This is useful for finding out if we should compile a contract method
/// as an entry point or not.
pub fn has_contract_library_method(&self) -> bool {
self.secondary
.iter()
.any(|attribute| attribute == &SecondaryAttribute::ContractLibraryMethod)
}

/// Returns note if a deprecated secondary attribute is found
pub fn get_deprecated_note(&self) -> Option<Option<String>> {
self.secondary.iter().find_map(|attr| match attr {
Expand Down Expand Up @@ -454,6 +464,9 @@ impl Attribute {
}
// Secondary attributes
["deprecated"] => Attribute::Secondary(SecondaryAttribute::Deprecated(None)),
["contract_library_method"] => {
Attribute::Secondary(SecondaryAttribute::ContractLibraryMethod)
}
["deprecated", name] => {
if !name.starts_with('"') && !name.ends_with('"') {
return Err(LexerErrorKind::MalformedFuncAttribute {
Expand Down Expand Up @@ -527,6 +540,10 @@ impl fmt::Display for PrimaryAttribute {
#[derive(PartialEq, Eq, Hash, Debug, Clone, PartialOrd, Ord)]
pub enum SecondaryAttribute {
Deprecated(Option<String>),
// This is an attribute to specify that a function
// is a helper method for a contract and should not be seen as
// the entry point.
ContractLibraryMethod,
Custom(String),
}

Expand All @@ -538,6 +555,7 @@ impl fmt::Display for SecondaryAttribute {
write!(f, r#"#[deprecated("{note}")]"#)
}
SecondaryAttribute::Custom(ref k) => write!(f, "#[{k}]"),
SecondaryAttribute::ContractLibraryMethod => write!(f, "#[contract_library_method]"),
}
}
}
Expand All @@ -559,6 +577,7 @@ impl AsRef<str> for SecondaryAttribute {
SecondaryAttribute::Deprecated(Some(string)) => string,
SecondaryAttribute::Deprecated(None) => "",
SecondaryAttribute::Custom(string) => string,
SecondaryAttribute::ContractLibraryMethod => "",
}
}
}
Expand Down
5 changes: 5 additions & 0 deletions compiler/noirc_frontend/src/node_interner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ use crate::hir_def::{
function::{FuncMeta, HirFunction},
stmt::HirStatement,
};
use crate::token::Attributes;
use crate::{
Generics, Shared, TypeAliasType, TypeBinding, TypeBindings, TypeVariable, TypeVariableId,
TypeVariableKind,
Expand Down Expand Up @@ -547,6 +548,10 @@ impl NodeInterner {
self.definition_name(name_id)
}

pub fn function_attributes(&self, func_id: &FuncId) -> Attributes {
self.function_meta(func_id).attributes.clone()
}

/// Returns the interned statement corresponding to `stmt_id`
pub fn statement(&self, stmt_id: &StmtId) -> HirStatement {
let def =
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
[package]
name = "no_entry_points"
type = "contract"
authors = [""]
compiler_version = "0.1"

[dependencies]
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
contract Foo {
struct PlaceholderStruct{x : u32 }

#[contract_library_method]
fn has_mut(_context : &mut PlaceholderStruct) {}
}