From 9d27f91236b6c502ebb20cffda153819231bb27e Mon Sep 17 00:00:00 2001 From: guipublic Date: Tue, 24 Oct 2023 12:46:20 +0000 Subject: [PATCH 1/4] add crate for pub modifier --- compiler/noirc_frontend/src/ast/expression.rs | 8 +-- compiler/noirc_frontend/src/ast/mod.rs | 8 +++ .../src/hir/resolution/errors.rs | 5 ++ .../src/hir/resolution/resolver.rs | 53 ++++++++++++++----- compiler/noirc_frontend/src/node_interner.rs | 12 ++--- compiler/noirc_frontend/src/parser/parser.rs | 43 +++++++++++---- 6 files changed, 94 insertions(+), 35 deletions(-) diff --git a/compiler/noirc_frontend/src/ast/expression.rs b/compiler/noirc_frontend/src/ast/expression.rs index 4c8e98a0d4d..66b94797822 100644 --- a/compiler/noirc_frontend/src/ast/expression.rs +++ b/compiler/noirc_frontend/src/ast/expression.rs @@ -3,7 +3,7 @@ use std::fmt::Display; use crate::token::{Attributes, Token}; use crate::{ - Distinctness, Ident, Path, Pattern, Recoverable, Statement, StatementKind, + Distinctness, FunctionVisibility, Ident, Path, Pattern, Recoverable, Statement, StatementKind, UnresolvedTraitConstraint, UnresolvedType, UnresolvedTypeData, Visibility, }; use acvm::FieldElement; @@ -366,8 +366,8 @@ pub struct FunctionDefinition { /// True if this function was defined with the 'unconstrained' keyword pub is_unconstrained: bool, - /// True if this function was defined with the 'pub' keyword - pub is_public: bool, + /// Indicate if this function was defined with the 'pub' keyword + pub visibility: FunctionVisibility, pub generics: UnresolvedGenerics, pub parameters: Vec<(Pattern, UnresolvedType, Visibility)>, @@ -644,7 +644,7 @@ impl FunctionDefinition { is_open: false, is_internal: false, is_unconstrained: false, - is_public: false, + visibility: FunctionVisibility::Private, generics: generics.clone(), parameters: p, body: body.clone(), diff --git a/compiler/noirc_frontend/src/ast/mod.rs b/compiler/noirc_frontend/src/ast/mod.rs index 03106e521c0..7ce01f461ed 100644 --- a/compiler/noirc_frontend/src/ast/mod.rs +++ b/compiler/noirc_frontend/src/ast/mod.rs @@ -270,6 +270,14 @@ impl UnresolvedTypeExpression { } } +#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)] +/// Represents whether the function can be called outside its module/crate +pub enum FunctionVisibility { + Public, + Private, + PublicCrate, +} + #[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)] /// Represents whether the parameter is public or known only to the prover. pub enum Visibility { diff --git a/compiler/noirc_frontend/src/hir/resolution/errors.rs b/compiler/noirc_frontend/src/hir/resolution/errors.rs index afdf65e5c5c..c2f787313c6 100644 --- a/compiler/noirc_frontend/src/hir/resolution/errors.rs +++ b/compiler/noirc_frontend/src/hir/resolution/errors.rs @@ -78,6 +78,8 @@ pub enum ResolverError { InvalidClosureEnvironment { typ: Type, span: Span }, #[error("{name} is private and not visible from the current module")] PrivateFunctionCalled { name: String, span: Span }, + #[error("{name} is not visible from the current crate")] + NonCrateFunctionCalled { name: String, span: Span }, #[error("Only sized types may be used in the entry point to a program")] InvalidTypeForEntryPoint { span: Span }, } @@ -296,6 +298,9 @@ impl From for Diagnostic { ResolverError::PrivateFunctionCalled { span, name } => Diagnostic::simple_warning( format!("{name} is private and not visible from the current module"), format!("{name} is private"), span), + ResolverError::NonCrateFunctionCalled { span, name } => Diagnostic::simple_warning( + format!("{name} is not visible from the current crate"), + format!("{name} is only visible within its crate"), span), ResolverError::InvalidTypeForEntryPoint { span } => Diagnostic::simple_error( "Only sized types may be used in the entry point to a program".to_string(), "Slices, references, or any type containing them may not be used in main or a contract function".to_string(), span), diff --git a/compiler/noirc_frontend/src/hir/resolution/resolver.rs b/compiler/noirc_frontend/src/hir/resolution/resolver.rs index a4a85d85929..e31bb77973a 100644 --- a/compiler/noirc_frontend/src/hir/resolution/resolver.rs +++ b/compiler/noirc_frontend/src/hir/resolution/resolver.rs @@ -36,10 +36,10 @@ use crate::{ StatementKind, }; use crate::{ - ArrayLiteral, ContractFunctionType, Distinctness, Generics, LValue, NoirStruct, NoirTypeAlias, - Path, PathKind, Pattern, Shared, StructType, Type, TypeAliasType, TypeBinding, TypeVariable, - UnaryOp, UnresolvedGenerics, UnresolvedTraitConstraint, UnresolvedType, UnresolvedTypeData, - UnresolvedTypeExpression, Visibility, ERROR_IDENT, + ArrayLiteral, ContractFunctionType, Distinctness, FunctionVisibility, Generics, LValue, + NoirStruct, NoirTypeAlias, Path, PathKind, Pattern, Shared, StructType, Type, TypeAliasType, + TypeBinding, TypeVariable, UnaryOp, UnresolvedGenerics, UnresolvedTraitConstraint, + UnresolvedType, UnresolvedTypeData, UnresolvedTypeExpression, Visibility, ERROR_IDENT, }; use fm::FileId; use iter_extended::vecmap; @@ -1039,20 +1039,39 @@ impl<'a> Resolver<'a> { } } - // Issue an error if the given private function is being called from a non-child module - fn check_can_reference_private_function(&mut self, func: FuncId, span: Span) { + // Issue an error if the given private function is being called from a non-child module, or + // if the given pub(crate) function is being called from another crate + fn check_can_reference_function( + &mut self, + func: FuncId, + span: Span, + visibility: FunctionVisibility, + ) { let function_module = self.interner.function_module(func); let current_module = self.path_resolver.module_id(); let same_crate = function_module.krate == current_module.krate; let krate = function_module.krate; let current_module = current_module.local_id; - - if !same_crate - || !self.module_descendent_of_target(krate, function_module.local_id, current_module) - { - let name = self.interner.function_name(&func).to_string(); - self.errors.push(ResolverError::PrivateFunctionCalled { span, name }); + let name = self.interner.function_name(&func).to_string(); + match visibility { + FunctionVisibility::Public => (), + FunctionVisibility::Private => { + if !same_crate + || !self.module_descendent_of_target( + krate, + function_module.local_id, + current_module, + ) + { + self.errors.push(ResolverError::PrivateFunctionCalled { span, name }); + } + } + FunctionVisibility::PublicCrate => { + if !same_crate { + self.errors.push(ResolverError::NonCrateFunctionCalled { span, name }); + } + } } } @@ -1146,9 +1165,15 @@ impl<'a> Resolver<'a> { if hir_ident.id != DefinitionId::dummy_id() { match self.interner.definition(hir_ident.id).kind { DefinitionKind::Function(id) => { - if self.interner.function_visibility(id) == Visibility::Private { + if self.interner.function_visibility(id) + != FunctionVisibility::Public + { let span = hir_ident.location.span; - self.check_can_reference_private_function(id, span); + self.check_can_reference_function( + id, + span, + self.interner.function_visibility(id), + ); } } DefinitionKind::Global(_) => {} diff --git a/compiler/noirc_frontend/src/node_interner.rs b/compiler/noirc_frontend/src/node_interner.rs index 8512e38fec0..514a65d37a2 100644 --- a/compiler/noirc_frontend/src/node_interner.rs +++ b/compiler/noirc_frontend/src/node_interner.rs @@ -21,8 +21,8 @@ use crate::hir_def::{ }; use crate::token::{Attributes, SecondaryAttribute}; use crate::{ - ContractFunctionType, FunctionDefinition, Generics, Shared, TypeAliasType, TypeBinding, - TypeBindings, TypeVariable, TypeVariableId, TypeVariableKind, Visibility, + ContractFunctionType, FunctionDefinition, FunctionVisibility, Generics, Shared, TypeAliasType, + TypeBinding, TypeBindings, TypeVariable, TypeVariableId, TypeVariableKind, }; #[derive(Eq, PartialEq, Hash, Clone)] @@ -132,7 +132,7 @@ pub struct FunctionModifiers { pub name: String, /// Whether the function is `pub` or not. - pub visibility: Visibility, + pub visibility: FunctionVisibility, pub attributes: Attributes, @@ -155,7 +155,7 @@ impl FunctionModifiers { pub fn new() -> Self { Self { name: String::new(), - visibility: Visibility::Public, + visibility: FunctionVisibility::Public, attributes: Attributes::empty(), is_unconstrained: false, is_internal: None, @@ -637,7 +637,7 @@ impl NodeInterner { // later during name resolution. let modifiers = FunctionModifiers { name: function.name.0.contents.clone(), - visibility: if function.is_public { Visibility::Public } else { Visibility::Private }, + visibility: function.visibility, attributes: function.attributes.clone(), is_unconstrained: function.is_unconstrained, contract_function_type: Some(if function.is_open { Open } else { Secret }), @@ -670,7 +670,7 @@ impl NodeInterner { /// /// The underlying function_visibilities map is populated during def collection, /// so this function can be called anytime afterward. - pub fn function_visibility(&self, func: FuncId) -> Visibility { + pub fn function_visibility(&self, func: FuncId) -> FunctionVisibility { self.function_modifiers[&func].visibility } diff --git a/compiler/noirc_frontend/src/parser/parser.rs b/compiler/noirc_frontend/src/parser/parser.rs index c9765ca1946..f35b7419d01 100644 --- a/compiler/noirc_frontend/src/parser/parser.rs +++ b/compiler/noirc_frontend/src/parser/parser.rs @@ -38,9 +38,9 @@ use crate::parser::{force, ignore_then_commit, statement_recovery}; use crate::token::{Attribute, Attributes, Keyword, SecondaryAttribute, Token, TokenKind}; use crate::{ BinaryOp, BinaryOpKind, BlockExpression, ConstrainStatement, Distinctness, FunctionDefinition, - FunctionReturnType, Ident, IfExpression, InfixExpression, LValue, Lambda, Literal, - NoirFunction, NoirStruct, NoirTrait, NoirTraitImpl, NoirTypeAlias, Path, PathKind, Pattern, - Recoverable, Statement, TraitBound, TraitImplItem, TraitItem, TypeImpl, UnaryOp, + FunctionReturnType, FunctionVisibility, Ident, IfExpression, InfixExpression, LValue, Lambda, + Literal, NoirFunction, NoirStruct, NoirTrait, NoirTraitImpl, NoirTypeAlias, Path, PathKind, + Pattern, Recoverable, Statement, TraitBound, TraitImplItem, TraitItem, TypeImpl, UnaryOp, UnresolvedTraitConstraint, UnresolvedTypeExpression, UseTree, UseTreeKind, Visibility, }; @@ -183,9 +183,15 @@ fn function_definition(allow_self: bool) -> impl NoirParser { name, attributes: attrs, is_unconstrained: modifiers.0, - is_open: modifiers.1, - is_internal: modifiers.2, - is_public: modifiers.3, + is_open: modifiers.2, + is_internal: modifiers.3, + visibility: if modifiers.1 { + FunctionVisibility::PublicCrate + } else if modifiers.4 { + FunctionVisibility::Public + } else { + FunctionVisibility::Private + }, generics, parameters, body, @@ -198,19 +204,34 @@ fn function_definition(allow_self: bool) -> impl NoirParser { }) } -/// function_modifiers: 'unconstrained'? 'open'? 'internal'? +/// function_modifiers: 'unconstrained'? 'pub(crate)'? 'pub'? 'open'? 'internal'? /// -/// returns (is_unconstrained, is_open, is_internal) for whether each keyword was present -fn function_modifiers() -> impl NoirParser<(bool, bool, bool, bool)> { +/// returns (is_unconstrained, is_pub_crate, is_open, is_internal, is_pub) for whether each keyword was present +fn function_modifiers() -> impl NoirParser<(bool, bool, bool, bool, bool)> { keyword(Keyword::Unconstrained) .or_not() + .then(is_pub_crate()) .then(keyword(Keyword::Pub).or_not()) .then(keyword(Keyword::Open).or_not()) .then(keyword(Keyword::Internal).or_not()) - .map(|(((unconstrained, public), open), internal)| { - (unconstrained.is_some(), open.is_some(), internal.is_some(), public.is_some()) + .map(|((((unconstrained, pub_crate), public), open), internal)| { + ( + unconstrained.is_some(), + pub_crate, + open.is_some(), + internal.is_some(), + public.is_some(), + ) }) } +fn is_pub_crate() -> impl NoirParser { + (keyword(Keyword::Pub) + .then_ignore(just(Token::LeftParen)) + .then(keyword(Keyword::Crate)) + .then_ignore(just(Token::RightParen))) + .or_not() + .map(|a| a.is_some()) +} /// non_empty_ident_list: ident ',' non_empty_ident_list /// | ident From 0e70476a3f411858b7e3b0671237cdafbf69c622 Mon Sep 17 00:00:00 2001 From: guipublic Date: Tue, 24 Oct 2023 12:50:23 +0000 Subject: [PATCH 2/4] add doc --- docs/docs/language_concepts/01_functions.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/docs/docs/language_concepts/01_functions.md b/docs/docs/language_concepts/01_functions.md index 2168c9203b6..48e3511797d 100644 --- a/docs/docs/language_concepts/01_functions.md +++ b/docs/docs/language_concepts/01_functions.md @@ -20,6 +20,12 @@ By default, functions are visible only within the package they are defined. To m pub fn foo() {} ``` +You can however restrict the visibility of the function to its own crate, by specifying `pub(crate)`: + +```rust +pub(crate) fn foo() {} //foo can only be called within its crate +``` + All parameters in a function must have a type and all types are known at compile time. The parameter is pre-pended with a colon and the parameter type. Multiple parameters are separated using a comma. From 46f6db65b0cff3accbe8ff446116ba41e1a799f3 Mon Sep 17 00:00:00 2001 From: jfecher Date: Tue, 24 Oct 2023 10:34:39 -0500 Subject: [PATCH 3/4] Update docs/docs/language_concepts/01_functions.md --- docs/docs/language_concepts/01_functions.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/docs/language_concepts/01_functions.md b/docs/docs/language_concepts/01_functions.md index 88813b0613a..2d7cc95a36d 100644 --- a/docs/docs/language_concepts/01_functions.md +++ b/docs/docs/language_concepts/01_functions.md @@ -20,7 +20,7 @@ By default, functions are visible only within the package they are defined. To m pub fn foo() {} ``` -You can however restrict the visibility of the function to its own crate, by specifying `pub(crate)`: +You can also restrict the visibility of the function to only the crate it was defined in, by specifying `pub(crate)`: ```rust pub(crate) fn foo() {} //foo can only be called within its crate From 2ede526552a9bb30b1a7a5142813b179457a03d3 Mon Sep 17 00:00:00 2001 From: jfecher Date: Tue, 24 Oct 2023 10:34:45 -0500 Subject: [PATCH 4/4] Update compiler/noirc_frontend/src/parser/parser.rs --- compiler/noirc_frontend/src/parser/parser.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/noirc_frontend/src/parser/parser.rs b/compiler/noirc_frontend/src/parser/parser.rs index f35b7419d01..f58f7315e8c 100644 --- a/compiler/noirc_frontend/src/parser/parser.rs +++ b/compiler/noirc_frontend/src/parser/parser.rs @@ -227,7 +227,7 @@ fn function_modifiers() -> impl NoirParser<(bool, bool, bool, bool, bool)> { fn is_pub_crate() -> impl NoirParser { (keyword(Keyword::Pub) .then_ignore(just(Token::LeftParen)) - .then(keyword(Keyword::Crate)) + .then_ignore(keyword(Keyword::Crate)) .then_ignore(just(Token::RightParen))) .or_not() .map(|a| a.is_some())