From 0a5fece52d820550ef51070c55524ced37c896a2 Mon Sep 17 00:00:00 2001 From: LHerskind Date: Thu, 6 Jul 2023 10:08:01 +0000 Subject: [PATCH 1/8] feat: add internal keyword --- .../test_data_ssa_refactor/contracts/src/main.nr | 1 + crates/noirc_frontend/src/ast/expression.rs | 2 ++ .../noirc_frontend/src/hir/resolution/errors.rs | 7 +++++++ .../src/hir/resolution/resolver.rs | 16 ++++++++++++++++ crates/noirc_frontend/src/hir/type_check/mod.rs | 1 + crates/noirc_frontend/src/hir_def/function.rs | 3 +++ crates/noirc_frontend/src/lexer/token.rs | 3 +++ crates/noirc_frontend/src/parser/parser.rs | 14 +++++++++++--- 8 files changed, 44 insertions(+), 3 deletions(-) diff --git a/crates/nargo_cli/tests/test_data_ssa_refactor/contracts/src/main.nr b/crates/nargo_cli/tests/test_data_ssa_refactor/contracts/src/main.nr index 53e094eb4cc..1eacc2fd485 100644 --- a/crates/nargo_cli/tests/test_data_ssa_refactor/contracts/src/main.nr +++ b/crates/nargo_cli/tests/test_data_ssa_refactor/contracts/src/main.nr @@ -5,4 +5,5 @@ fn main(x : Field, y : pub Field) { contract Foo { fn double(x: Field) -> pub Field { x * 2 } fn triple(x: Field) -> pub Field { x * 3 } + internal fn quadruple(x: Field) -> pub Field { x * 4 } } diff --git a/crates/noirc_frontend/src/ast/expression.rs b/crates/noirc_frontend/src/ast/expression.rs index f3788919d85..88fff617284 100644 --- a/crates/noirc_frontend/src/ast/expression.rs +++ b/crates/noirc_frontend/src/ast/expression.rs @@ -328,6 +328,8 @@ pub struct FunctionDefinition { /// True if this function was defined with the 'open' keyword pub is_open: bool, + pub is_internal: bool, + /// True if this function was defined with the 'unconstrained' keyword pub is_unconstrained: bool, diff --git a/crates/noirc_frontend/src/hir/resolution/errors.rs b/crates/noirc_frontend/src/hir/resolution/errors.rs index 80638897a59..296be28b5a6 100644 --- a/crates/noirc_frontend/src/hir/resolution/errors.rs +++ b/crates/noirc_frontend/src/hir/resolution/errors.rs @@ -64,6 +64,8 @@ pub enum ResolverError { MutableReferenceToImmutableVariable { variable: String, span: Span }, #[error("Mutable references to array indices are unsupported")] MutableReferenceToArrayElement { span: Span }, + #[error("Function is not defined in a contract yet sets is_internal")] + ContractFunctionInternalInNormalFunction { span: Span }, } impl ResolverError { @@ -268,6 +270,11 @@ impl From for Diagnostic { ResolverError::MutableReferenceToArrayElement { span } => { Diagnostic::simple_error("Mutable references to array elements are currently unsupported".into(), "Try storing the element in a fresh variable first".into(), span) }, + ResolverError::ContractFunctionInternalInNormalFunction { span } => Diagnostic::simple_error( + "Only functions defined within contracts can set their functions to be internal".into(), + "Non-contract functions cannot be 'internal'".into(), + span, + ), } } } diff --git a/crates/noirc_frontend/src/hir/resolution/resolver.rs b/crates/noirc_frontend/src/hir/resolution/resolver.rs index 72b03689ec2..97a569b2b07 100644 --- a/crates/noirc_frontend/src/hir/resolution/resolver.rs +++ b/crates/noirc_frontend/src/hir/resolution/resolver.rs @@ -663,6 +663,7 @@ impl<'a> Resolver<'a> { kind: func.kind, attributes, contract_function_type: self.handle_function_type(func), + is_contract_function_internal: self.handle_is_function_internal(func), is_unconstrained: func.def.is_unconstrained, location, typ, @@ -708,6 +709,21 @@ impl<'a> Resolver<'a> { } } + fn handle_is_function_internal(&mut self, func: &NoirFunction) -> Option { + if func.def.is_internal { + if self.in_contract() { + Some(true) + } else { + self.push_err(ResolverError::ContractFunctionInternalInNormalFunction { + span: func.name_ident().span(), + }); + None + } + } else { + Some(false) + } + } + fn declare_numeric_generics(&mut self, params: &[Type], return_type: &Type) { if self.generics.is_empty() { return; diff --git a/crates/noirc_frontend/src/hir/type_check/mod.rs b/crates/noirc_frontend/src/hir/type_check/mod.rs index fa47477a94e..9b6c20284f0 100644 --- a/crates/noirc_frontend/src/hir/type_check/mod.rs +++ b/crates/noirc_frontend/src/hir/type_check/mod.rs @@ -232,6 +232,7 @@ mod test { attributes: None, location, contract_function_type: None, + is_contract_function_internal: None, is_unconstrained: false, typ: Type::Function(vec![Type::field(None), Type::field(None)], Box::new(Type::Unit)), parameters: vec![ diff --git a/crates/noirc_frontend/src/hir_def/function.rs b/crates/noirc_frontend/src/hir_def/function.rs index 60cb8ea4f85..7a3d3c60578 100644 --- a/crates/noirc_frontend/src/hir_def/function.rs +++ b/crates/noirc_frontend/src/hir_def/function.rs @@ -125,6 +125,9 @@ pub struct FuncMeta { /// If this function is not in a contract, this is always 'Secret'. pub contract_function_type: Option, + /// @lherskind This is for internal flag. + pub is_contract_function_internal: Option, + pub is_unconstrained: bool, pub parameters: Parameters, diff --git a/crates/noirc_frontend/src/lexer/token.rs b/crates/noirc_frontend/src/lexer/token.rs index 3eb5b49914f..9dbf97f2d8a 100644 --- a/crates/noirc_frontend/src/lexer/token.rs +++ b/crates/noirc_frontend/src/lexer/token.rs @@ -431,6 +431,7 @@ pub enum Keyword { Impl, If, In, + Internal, Let, Mod, Mut, @@ -466,6 +467,7 @@ impl fmt::Display for Keyword { Keyword::Impl => write!(f, "impl"), Keyword::If => write!(f, "if"), Keyword::In => write!(f, "in"), + Keyword::Internal => write!(f, "internal"), Keyword::Let => write!(f, "let"), Keyword::Mod => write!(f, "mod"), Keyword::Mut => write!(f, "mut"), @@ -504,6 +506,7 @@ impl Keyword { "impl" => Keyword::Impl, "if" => Keyword::If, "in" => Keyword::In, + "internal" => Keyword::Internal, "let" => Keyword::Let, "mod" => Keyword::Mod, "mut" => Keyword::Mut, diff --git a/crates/noirc_frontend/src/parser/parser.rs b/crates/noirc_frontend/src/parser/parser.rs index ec3357c56d4..7fa044f6e94 100644 --- a/crates/noirc_frontend/src/parser/parser.rs +++ b/crates/noirc_frontend/src/parser/parser.rs @@ -162,7 +162,10 @@ fn function_definition(allow_self: bool) -> impl NoirParser { .map( |( ( - ((((attribute, (is_unconstrained, is_open)), name), generics), parameters), + ( + (((attribute, (is_unconstrained, is_open, is_internal)), name), generics), + parameters, + ), ((return_distinctness, return_visibility), return_type), ), body, @@ -172,6 +175,7 @@ fn function_definition(allow_self: bool) -> impl NoirParser { name, attribute, // XXX: Currently we only have one attribute defined. If more attributes are needed per function, we can make this a vector and make attribute definition more expressive is_open, + is_internal, is_unconstrained, generics, parameters, @@ -188,11 +192,15 @@ fn function_definition(allow_self: bool) -> impl NoirParser { /// function_modifiers: 'unconstrained' 'open' | 'unconstrained' | 'open' | %empty /// /// returns (is_unconstrained, is_open) for whether each keyword was present -fn function_modifiers() -> impl NoirParser<(bool, bool)> { +fn function_modifiers() -> impl NoirParser<(bool, bool, bool)> { + // @todo @lherskind This is retarded, send help. Also update comments. keyword(Keyword::Unconstrained) .or_not() .then(keyword(Keyword::Open).or_not()) - .map(|(unconstrained, open)| (unconstrained.is_some(), open.is_some())) + .then(keyword(Keyword::Internal).or_not()) + .map(|((unconstrained, open), internal)| { + (unconstrained.is_some(), open.is_some(), internal.is_some()) + }) } /// non_empty_ident_list: ident ',' non_empty_ident_list From 5a9e33992f0434fa5493ac0c9417fbb9fef354a6 Mon Sep 17 00:00:00 2001 From: LHerskind Date: Thu, 6 Jul 2023 10:18:52 +0000 Subject: [PATCH 2/8] =?UTF-8?q?chore:=20remove=20=F0=9F=91=BA?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- crates/noirc_frontend/src/parser/parser.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/noirc_frontend/src/parser/parser.rs b/crates/noirc_frontend/src/parser/parser.rs index 7fa044f6e94..635decc99a2 100644 --- a/crates/noirc_frontend/src/parser/parser.rs +++ b/crates/noirc_frontend/src/parser/parser.rs @@ -189,11 +189,11 @@ fn function_definition(allow_self: bool) -> impl NoirParser { ) } -/// function_modifiers: 'unconstrained' 'open' | 'unconstrained' | 'open' | %empty +/// function_modifiers: 'internal' 'unconstrained' 'open' | 'internal' 'unconstrained' | 'internal' 'open' | +/// 'internal' | 'unconstrained' | 'open' | %empty /// -/// returns (is_unconstrained, is_open) for whether each keyword was present +/// returns (is_unconstrained, is_open, is_internal) for whether each keyword was present fn function_modifiers() -> impl NoirParser<(bool, bool, bool)> { - // @todo @lherskind This is retarded, send help. Also update comments. keyword(Keyword::Unconstrained) .or_not() .then(keyword(Keyword::Open).or_not()) From 97aaf891b16738f987dc5d23a6d681bfece83aa3 Mon Sep 17 00:00:00 2001 From: LHerskind Date: Thu, 6 Jul 2023 10:21:12 +0000 Subject: [PATCH 3/8] fix: comment cleanup --- crates/noirc_frontend/src/hir_def/function.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/crates/noirc_frontend/src/hir_def/function.rs b/crates/noirc_frontend/src/hir_def/function.rs index 7a3d3c60578..a2181d098ad 100644 --- a/crates/noirc_frontend/src/hir_def/function.rs +++ b/crates/noirc_frontend/src/hir_def/function.rs @@ -125,7 +125,8 @@ pub struct FuncMeta { /// If this function is not in a contract, this is always 'Secret'. pub contract_function_type: Option, - /// @lherskind This is for internal flag. + /// This function's ability to be called from other contracts. + /// If this function is internal can only be called by itself. pub is_contract_function_internal: Option, pub is_unconstrained: bool, From 6d081aaf11521a28ed0c495738348202fbc04b8b Mon Sep 17 00:00:00 2001 From: LHerskind Date: Thu, 6 Jul 2023 12:53:45 +0000 Subject: [PATCH 4/8] feat: add `is_internal` to abi --- crates/nargo/src/artifacts/contract.rs | 2 ++ crates/nargo/src/ops/preprocess.rs | 1 + .../tests/test_data_ssa_refactor/contracts/src/main.nr | 1 + crates/noirc_driver/src/contract.rs | 2 ++ crates/noirc_driver/src/lib.rs | 6 ++++++ crates/noirc_frontend/src/hir_def/function.rs | 4 ++-- crates/noirc_frontend/src/parser/parser.rs | 4 +--- 7 files changed, 15 insertions(+), 5 deletions(-) diff --git a/crates/nargo/src/artifacts/contract.rs b/crates/nargo/src/artifacts/contract.rs index 4174207692f..a718eac9796 100644 --- a/crates/nargo/src/artifacts/contract.rs +++ b/crates/nargo/src/artifacts/contract.rs @@ -28,6 +28,8 @@ pub struct PreprocessedContractFunction { pub function_type: ContractFunctionType, + pub is_internal: bool, + pub abi: Abi, #[serde( diff --git a/crates/nargo/src/ops/preprocess.rs b/crates/nargo/src/ops/preprocess.rs index 90364bec603..b1613ea3195 100644 --- a/crates/nargo/src/ops/preprocess.rs +++ b/crates/nargo/src/ops/preprocess.rs @@ -53,6 +53,7 @@ pub fn preprocess_contract_function( Ok(PreprocessedContractFunction { name: func.name, function_type: func.function_type, + is_internal: func.is_internal, abi: func.abi, bytecode: optimized_bytecode, diff --git a/crates/nargo_cli/tests/test_data_ssa_refactor/contracts/src/main.nr b/crates/nargo_cli/tests/test_data_ssa_refactor/contracts/src/main.nr index 1eacc2fd485..c8d70cc2d0e 100644 --- a/crates/nargo_cli/tests/test_data_ssa_refactor/contracts/src/main.nr +++ b/crates/nargo_cli/tests/test_data_ssa_refactor/contracts/src/main.nr @@ -6,4 +6,5 @@ contract Foo { fn double(x: Field) -> pub Field { x * 2 } fn triple(x: Field) -> pub Field { x * 3 } internal fn quadruple(x: Field) -> pub Field { x * 4 } + open internal fn skibbidy(x: Field) -> pub Field { x * 5 } } diff --git a/crates/noirc_driver/src/contract.rs b/crates/noirc_driver/src/contract.rs index c0a54534941..a25d258c9be 100644 --- a/crates/noirc_driver/src/contract.rs +++ b/crates/noirc_driver/src/contract.rs @@ -41,6 +41,8 @@ pub struct ContractFunction { pub function_type: ContractFunctionType, + pub is_internal: bool, + pub abi: Abi, #[serde(serialize_with = "serialize_circuit", deserialize_with = "deserialize_circuit")] diff --git a/crates/noirc_driver/src/lib.rs b/crates/noirc_driver/src/lib.rs index 0eb5f9e03b2..f077792c4a9 100644 --- a/crates/noirc_driver/src/lib.rs +++ b/crates/noirc_driver/src/lib.rs @@ -299,9 +299,15 @@ fn compile_contract( let function_type = ContractFunctionType::new(func_type, func_meta.is_unconstrained); + let is_internal = match func_meta.is_contract_function_internal.is_some() { + true => func_meta.is_contract_function_internal.unwrap(), + false => false, + }; + functions.push(ContractFunction { name, function_type, + is_internal, abi: function.abi, bytecode: function.circuit, }); diff --git a/crates/noirc_frontend/src/hir_def/function.rs b/crates/noirc_frontend/src/hir_def/function.rs index a2181d098ad..c654df2dfcd 100644 --- a/crates/noirc_frontend/src/hir_def/function.rs +++ b/crates/noirc_frontend/src/hir_def/function.rs @@ -121,11 +121,11 @@ pub struct FuncMeta { /// Attribute per function. pub attributes: Option, - /// This function's visibility in its contract. + /// This function's type in its contract. /// If this function is not in a contract, this is always 'Secret'. pub contract_function_type: Option, - /// This function's ability to be called from other contracts. + /// This function's visibility. /// If this function is internal can only be called by itself. pub is_contract_function_internal: Option, diff --git a/crates/noirc_frontend/src/parser/parser.rs b/crates/noirc_frontend/src/parser/parser.rs index 635decc99a2..b8fdb4294d9 100644 --- a/crates/noirc_frontend/src/parser/parser.rs +++ b/crates/noirc_frontend/src/parser/parser.rs @@ -189,9 +189,7 @@ fn function_definition(allow_self: bool) -> impl NoirParser { ) } -/// function_modifiers: 'internal' 'unconstrained' 'open' | 'internal' 'unconstrained' | 'internal' 'open' | -/// 'internal' | 'unconstrained' | 'open' | %empty -/// +/// function_modifiers: 'open internal' | 'internal' | 'unconstrained' | 'open' | %empty /// returns (is_unconstrained, is_open, is_internal) for whether each keyword was present fn function_modifiers() -> impl NoirParser<(bool, bool, bool)> { keyword(Keyword::Unconstrained) From 3fdc946917eaaab86c60672328b230242efa1c33 Mon Sep 17 00:00:00 2001 From: Lasse Herskind <16536249+LHerskind@users.noreply.github.com> Date: Thu, 6 Jul 2023 14:56:23 +0200 Subject: [PATCH 5/8] Update crates/noirc_frontend/src/hir/resolution/resolver.rs Co-authored-by: jfecher --- .../src/hir/resolution/resolver.rs | 21 ++++++++----------- 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/crates/noirc_frontend/src/hir/resolution/resolver.rs b/crates/noirc_frontend/src/hir/resolution/resolver.rs index 97a569b2b07..af1684e6596 100644 --- a/crates/noirc_frontend/src/hir/resolution/resolver.rs +++ b/crates/noirc_frontend/src/hir/resolution/resolver.rs @@ -710,18 +710,15 @@ impl<'a> Resolver<'a> { } fn handle_is_function_internal(&mut self, func: &NoirFunction) -> Option { - if func.def.is_internal { - if self.in_contract() { - Some(true) - } else { - self.push_err(ResolverError::ContractFunctionInternalInNormalFunction { - span: func.name_ident().span(), - }); - None - } - } else { - Some(false) - } +if self.in_contract() { + Some(func.def.is_internal) +} else { + if func.def.is_internal { + self.push_err(ResolverError::ContractFunctionInternalInNormalFunction { + span: func.name_ident().span(), + }); + None +} } fn declare_numeric_generics(&mut self, params: &[Type], return_type: &Type) { From 18838cb45765f43edd4e8b28c98623b71bfb3fe3 Mon Sep 17 00:00:00 2001 From: LHerskind Date: Thu, 6 Jul 2023 13:04:35 +0000 Subject: [PATCH 6/8] fix: cleanup --- .../src/hir/resolution/resolver.rs | 21 ++++++++++--------- .../noirc_frontend/src/hir/type_check/mod.rs | 2 +- crates/noirc_frontend/src/hir_def/function.rs | 3 ++- 3 files changed, 14 insertions(+), 12 deletions(-) diff --git a/crates/noirc_frontend/src/hir/resolution/resolver.rs b/crates/noirc_frontend/src/hir/resolution/resolver.rs index af1684e6596..23540f6029d 100644 --- a/crates/noirc_frontend/src/hir/resolution/resolver.rs +++ b/crates/noirc_frontend/src/hir/resolution/resolver.rs @@ -663,7 +663,7 @@ impl<'a> Resolver<'a> { kind: func.kind, attributes, contract_function_type: self.handle_function_type(func), - is_contract_function_internal: self.handle_is_function_internal(func), + is_internal: self.handle_is_function_internal(func), is_unconstrained: func.def.is_unconstrained, location, typ, @@ -710,15 +710,16 @@ impl<'a> Resolver<'a> { } fn handle_is_function_internal(&mut self, func: &NoirFunction) -> Option { -if self.in_contract() { - Some(func.def.is_internal) -} else { - if func.def.is_internal { - self.push_err(ResolverError::ContractFunctionInternalInNormalFunction { - span: func.name_ident().span(), - }); - None -} + if self.in_contract() { + Some(func.def.is_internal) + } else { + if func.def.is_internal { + self.push_err(ResolverError::ContractFunctionInternalInNormalFunction { + span: func.name_ident().span(), + }); + } + None + } } fn declare_numeric_generics(&mut self, params: &[Type], return_type: &Type) { diff --git a/crates/noirc_frontend/src/hir/type_check/mod.rs b/crates/noirc_frontend/src/hir/type_check/mod.rs index 9b6c20284f0..b8bb6c788e9 100644 --- a/crates/noirc_frontend/src/hir/type_check/mod.rs +++ b/crates/noirc_frontend/src/hir/type_check/mod.rs @@ -232,7 +232,7 @@ mod test { attributes: None, location, contract_function_type: None, - is_contract_function_internal: None, + is_internal: None, is_unconstrained: false, typ: Type::Function(vec![Type::field(None), Type::field(None)], Box::new(Type::Unit)), parameters: vec![ diff --git a/crates/noirc_frontend/src/hir_def/function.rs b/crates/noirc_frontend/src/hir_def/function.rs index c654df2dfcd..304772f1d0e 100644 --- a/crates/noirc_frontend/src/hir_def/function.rs +++ b/crates/noirc_frontend/src/hir_def/function.rs @@ -127,7 +127,8 @@ pub struct FuncMeta { /// This function's visibility. /// If this function is internal can only be called by itself. - pub is_contract_function_internal: Option, + /// Will be None if not in contract. + pub is_internal: Option, pub is_unconstrained: bool, From 50de98fcc720a09316af37f328f5e8d0987ed2d2 Mon Sep 17 00:00:00 2001 From: LHerskind Date: Fri, 7 Jul 2023 10:34:10 +0000 Subject: [PATCH 7/8] fix: is_internal name --- crates/noirc_driver/src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/noirc_driver/src/lib.rs b/crates/noirc_driver/src/lib.rs index f077792c4a9..f70b0e4ca66 100644 --- a/crates/noirc_driver/src/lib.rs +++ b/crates/noirc_driver/src/lib.rs @@ -299,8 +299,8 @@ fn compile_contract( let function_type = ContractFunctionType::new(func_type, func_meta.is_unconstrained); - let is_internal = match func_meta.is_contract_function_internal.is_some() { - true => func_meta.is_contract_function_internal.unwrap(), + let is_internal = match func_meta.is_internal.is_some() { + true => func_meta.is_internal.unwrap(), false => false, }; From 91992188078e5e7bf9c621a920e502759e37987d Mon Sep 17 00:00:00 2001 From: kevaundray Date: Fri, 7 Jul 2023 13:24:53 +0100 Subject: [PATCH 8/8] Update crates/noirc_driver/src/lib.rs Co-authored-by: jfecher --- crates/noirc_driver/src/lib.rs | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/crates/noirc_driver/src/lib.rs b/crates/noirc_driver/src/lib.rs index f70b0e4ca66..e717d325a95 100644 --- a/crates/noirc_driver/src/lib.rs +++ b/crates/noirc_driver/src/lib.rs @@ -299,15 +299,10 @@ fn compile_contract( let function_type = ContractFunctionType::new(func_type, func_meta.is_unconstrained); - let is_internal = match func_meta.is_internal.is_some() { - true => func_meta.is_internal.unwrap(), - false => false, - }; - functions.push(ContractFunction { name, function_type, - is_internal, + is_internal: func_meta.is_internal.unwrap_or(false), abi: function.abi, bytecode: function.circuit, });