Skip to content

Commit

Permalink
chore: remove knowledge of aztec function types from compiler (#4975)
Browse files Browse the repository at this point in the history
This PR removes the concept of a function being internal or "open" from
the Noir compiler as these are now concepts which are aztec specific. We
instead attach all of a contract functions custom attributes to it in
the build artifact so aztec can parse these to determine whether a
function is open or secret, etc.

@spalladino This follows on from #4967 but I've made this as a separate
PR into yours as I don't want to hijack it.
  • Loading branch information
TomAFrench authored Mar 6, 2024
1 parent 944f4ec commit cff565e
Show file tree
Hide file tree
Showing 13 changed files with 34 additions and 143 deletions.
9 changes: 4 additions & 5 deletions noir/noir-repo/aztec_macros/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -663,11 +663,10 @@ fn transform_function(
let inputs_name = format!("{}ContextInputs", ty);
let return_type_name = format!("{}CircuitPublicInputs", ty);

// Add check that msg sender equals this address and flag function as internal
// Add check that msg sender equals this address
if is_internal {
let is_internal_check = create_internal_check(func.name());
func.def.body.0.insert(0, is_internal_check);
func.def.is_internal = true;
}

// Add initialization check
Expand Down Expand Up @@ -714,10 +713,10 @@ fn transform_function(
func.def.return_visibility = Visibility::Public;

// Distinct return types are only required for private functions
// Public functions should have open auto-inferred
// Public functions should have unconstrained auto-inferred
match ty {
"Private" => func.def.return_distinctness = Distinctness::Distinct,
"Public" => func.def.is_open = true,
"Public" => func.def.is_unconstrained = true,
_ => (),
}

Expand All @@ -734,7 +733,7 @@ fn transform_vm_function(
func.def.body.0.insert(0, create_context);

// We want the function to be seen as a public function
func.def.is_open = true;
func.def.is_unconstrained = true;

// NOTE: the line below is a temporary hack to trigger external transpilation tools
// It will be removed once the transpiler is integrated into the Noir compiler
Expand Down
31 changes: 1 addition & 30 deletions noir/noir-repo/compiler/noirc_driver/src/contract.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,23 +9,6 @@ use noirc_evaluator::errors::SsaReport;

use super::debug::DebugFile;

/// Describes the types of smart contract functions that are allowed.
/// Unlike the similar enum in noirc_frontend, 'open' and 'unconstrained'
/// are mutually exclusive here. In the case a function is both, 'unconstrained'
/// takes precedence.
#[derive(Serialize, Deserialize, Debug, Copy, Clone, PartialEq, Eq)]
pub enum ContractFunctionType {
/// This function will be executed in a private
/// context.
Secret,
/// This function will be executed in a public
/// context.
Open,
/// This function cannot constrain any values and can use nondeterministic features
/// like arrays of a dynamic size.
Unconstrained,
}

#[derive(Clone, Debug, Serialize, Deserialize)]
pub struct CompiledContract {
pub noir_version: String,
Expand Down Expand Up @@ -55,9 +38,7 @@ pub struct CompiledContract {
pub struct ContractFunction {
pub name: String,

pub function_type: ContractFunctionType,

pub is_internal: bool,
pub custom_attributes: Vec<String>,

pub abi: Abi,

Expand All @@ -69,13 +50,3 @@ pub struct ContractFunction {

pub debug: DebugInfo,
}

impl ContractFunctionType {
pub(super) fn new(kind: noirc_frontend::ContractFunctionType, is_unconstrained: bool) -> Self {
match (kind, is_unconstrained) {
(_, true) => Self::Unconstrained,
(noirc_frontend::ContractFunctionType::Secret, false) => Self::Secret,
(noirc_frontend::ContractFunctionType::Open, false) => Self::Open,
}
}
}
19 changes: 12 additions & 7 deletions noir/noir-repo/compiler/noirc_driver/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ use noirc_frontend::hir::Context;
use noirc_frontend::macros_api::MacroProcessor;
use noirc_frontend::monomorphization::{monomorphize, monomorphize_debug, MonomorphizationError};
use noirc_frontend::node_interner::FuncId;
use noirc_frontend::token::SecondaryAttribute;
use std::path::Path;
use thiserror::Error;
use tracing::info;
Expand All @@ -30,7 +31,7 @@ mod stdlib;

use debug::filter_relevant_files;

pub use contract::{CompiledContract, ContractFunction, ContractFunctionType};
pub use contract::{CompiledContract, ContractFunction};
pub use debug::DebugFile;
pub use program::CompiledProgram;

Expand Down Expand Up @@ -404,16 +405,20 @@ fn compile_contract_inner(
};
warnings.extend(function.warnings);
let modifiers = context.def_interner.function_modifiers(&function_id);
let func_type = modifiers
.contract_function_type
.expect("Expected contract function to have a contract visibility");

let function_type = ContractFunctionType::new(func_type, modifiers.is_unconstrained);
let custom_attributes = modifiers
.attributes
.secondary
.iter()
.filter_map(
|attr| if let SecondaryAttribute::Custom(tag) = attr { Some(tag) } else { None },
)
.cloned()
.collect();

functions.push(ContractFunction {
name,
function_type,
is_internal: modifiers.is_internal.unwrap_or(false),
custom_attributes,
abi: function.abi,
bytecode: function.circuit,
debug: function.debug,
Expand Down
19 changes: 0 additions & 19 deletions noir/noir-repo/compiler/noirc_frontend/src/ast/expression.rs
Original file line number Diff line number Diff line change
Expand Up @@ -369,11 +369,6 @@ pub struct FunctionDefinition {
// and `secondary` attributes (ones that do not change the function kind)
pub attributes: Attributes,

/// 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,

Expand Down Expand Up @@ -406,18 +401,6 @@ pub enum FunctionReturnType {
Ty(UnresolvedType),
}

/// Describes the types of smart contract functions that are allowed.
/// - All Noir programs in the non-contract context can be seen as `Secret`.
#[derive(Debug, Copy, Clone, PartialEq, Eq)]
pub enum ContractFunctionType {
/// This function will be executed in a private
/// context.
Secret,
/// This function will be executed in a public
/// context.
Open,
}

#[derive(Debug, PartialEq, Eq, Clone)]
pub enum ArrayLiteral {
Standard(Vec<Expression>),
Expand Down Expand Up @@ -674,8 +657,6 @@ impl FunctionDefinition {
FunctionDefinition {
name: name.clone(),
attributes: Attributes::empty(),
is_open: false,
is_internal: false,
is_unconstrained: false,
visibility: FunctionVisibility::Private,
generics: generics.clone(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -398,8 +398,6 @@ impl<'a> ModCollector<'a> {
// TODO(Maddiaa): Investigate trait implementations with attributes see: https://github.com/noir-lang/noir/issues/2629
attributes: crate::token::Attributes::empty(),
is_unconstrained: false,
contract_function_type: None,
is_internal: None,
};

let location = Location::new(name.span(), self.file_id);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,8 +64,6 @@ pub enum ResolverError {
IncorrectGenericCount { span: Span, item_name: String, actual: usize, expected: usize },
#[error("{0}")]
ParserError(Box<ParserError>),
#[error("Function is not defined in a contract yet sets its contract visibility")]
ContractFunctionTypeInNormalFunction { span: Span },
#[error("Cannot create a mutable reference to {variable}, it was declared to be immutable")]
MutableReferenceToImmutableVariable { variable: String, span: Span },
#[error("Mutable references to array indices are unsupported")]
Expand Down Expand Up @@ -278,11 +276,6 @@ impl From<ResolverError> for Diagnostic {
)
}
ResolverError::ParserError(error) => (*error).into(),
ResolverError::ContractFunctionTypeInNormalFunction { span } => Diagnostic::simple_error(
"Only functions defined within contracts can set their contract function type".into(),
"Non-contract functions cannot be 'open'".into(),
span,
),
ResolverError::MutableReferenceToImmutableVariable { variable, span } => {
Diagnostic::simple_error(format!("Cannot mutably reference the immutable variable {variable}"), format!("{variable} is immutable"), span)
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,10 +37,10 @@ use crate::{
StatementKind,
};
use crate::{
ArrayLiteral, ContractFunctionType, Distinctness, ForRange, FunctionDefinition,
FunctionReturnType, FunctionVisibility, Generics, LValue, NoirStruct, NoirTypeAlias, Param,
Path, PathKind, Pattern, Shared, StructType, Type, TypeAlias, TypeVariable, TypeVariableKind,
UnaryOp, UnresolvedGenerics, UnresolvedTraitConstraint, UnresolvedType, UnresolvedTypeData,
ArrayLiteral, Distinctness, ForRange, FunctionDefinition, FunctionReturnType,
FunctionVisibility, Generics, LValue, NoirStruct, NoirTypeAlias, Param, Path, PathKind,
Pattern, Shared, StructType, Type, TypeAlias, TypeVariable, TypeVariableKind, UnaryOp,
UnresolvedGenerics, UnresolvedTraitConstraint, UnresolvedType, UnresolvedTypeData,
UnresolvedTypeExpression, Visibility, ERROR_IDENT,
};
use fm::FileId;
Expand Down Expand Up @@ -233,8 +233,6 @@ impl<'a> Resolver<'a> {
let def = FunctionDefinition {
name: name.clone(),
attributes: Attributes::empty(),
is_open: false,
is_internal: false,
is_unconstrained: false,
visibility: FunctionVisibility::Public, // Trait functions are always public
generics: Vec::new(), // self.generics should already be set
Expand Down Expand Up @@ -976,9 +974,6 @@ impl<'a> Resolver<'a> {

self.interner.push_definition_type(name_ident.id, typ.clone());

self.handle_function_type(&func_id);
self.handle_is_function_internal(&func_id);

FuncMeta {
name: name_ident,
kind: func.kind,
Expand Down Expand Up @@ -1018,34 +1013,14 @@ impl<'a> Resolver<'a> {
/// True if the `distinct` keyword is allowed on a function's return type
fn distinct_allowed(&self, func: &NoirFunction) -> bool {
if self.in_contract {
// "open" and "unconstrained" functions are compiled to brillig and thus duplication of
// "unconstrained" functions are compiled to brillig and thus duplication of
// witness indices in their abis is not a concern.
!func.def.is_unconstrained && !func.def.is_open
!func.def.is_unconstrained
} else {
func.name() == MAIN_FUNCTION
}
}

fn handle_function_type(&mut self, function: &FuncId) {
let function_type = self.interner.function_modifiers(function).contract_function_type;

if !self.in_contract && function_type == Some(ContractFunctionType::Open) {
let span = self.interner.function_ident(function).span();
self.errors.push(ResolverError::ContractFunctionTypeInNormalFunction { span });
self.interner.function_modifiers_mut(function).contract_function_type = None;
}
}

fn handle_is_function_internal(&mut self, function: &FuncId) {
if !self.in_contract {
if self.interner.function_modifiers(function).is_internal == Some(true) {
let span = self.interner.function_ident(function).span();
self.push_err(ResolverError::ContractFunctionInternalInNormalFunction { span });
}
self.interner.function_modifiers_mut(function).is_internal = None;
}
}

fn declare_numeric_generics(&mut self, params: &[Type], return_type: &Type) {
if self.generics.is_empty() {
return;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,8 @@ use crate::{
},
node_interner::{self, DefinitionKind, NodeInterner, StmtId, TraitImplKind, TraitMethodId},
token::FunctionAttribute,
ContractFunctionType, FunctionKind, IntegerBitSize, Signedness, Type, TypeBinding,
TypeBindings, TypeVariable, TypeVariableKind, UnaryOp, Visibility,
FunctionKind, IntegerBitSize, Signedness, Type, TypeBinding, TypeBindings, TypeVariable,
TypeVariableKind, UnaryOp, Visibility,
};

use self::ast::{Definition, FuncId, Function, LocalId, Program};
Expand Down Expand Up @@ -310,8 +310,7 @@ impl<'interner> Monomorphizer<'interner> {
Type::TraitAsType(..) => &body_return_type,
_ => meta.return_type(),
});
let unconstrained = modifiers.is_unconstrained
|| matches!(modifiers.contract_function_type, Some(ContractFunctionType::Open));
let unconstrained = modifiers.is_unconstrained;

let parameters = self.parameters(&meta.parameters);
let body = self.expr(body_expr_id)?;
Expand Down
19 changes: 2 additions & 17 deletions noir/noir-repo/compiler/noirc_frontend/src/node_interner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,8 @@ use crate::hir_def::{
};
use crate::token::{Attributes, SecondaryAttribute};
use crate::{
BinaryOpKind, ContractFunctionType, FunctionDefinition, FunctionVisibility, Generics, Shared,
TypeAlias, TypeBindings, TypeVariable, TypeVariableId, TypeVariableKind,
BinaryOpKind, FunctionDefinition, FunctionVisibility, Generics, Shared, TypeAlias,
TypeBindings, TypeVariable, TypeVariableId, TypeVariableKind,
};

/// An arbitrary number to limit the recursion depth when searching for trait impls.
Expand Down Expand Up @@ -241,15 +241,6 @@ pub struct FunctionModifiers {
pub attributes: Attributes,

pub is_unconstrained: bool,

/// This function's type in its contract.
/// If this function is not in a contract, this is always 'Secret'.
pub contract_function_type: Option<ContractFunctionType>,

/// This function's contract visibility.
/// If this function is internal can only be called by itself.
/// Will be None if not in contract.
pub is_internal: Option<bool>,
}

impl FunctionModifiers {
Expand All @@ -262,8 +253,6 @@ impl FunctionModifiers {
visibility: FunctionVisibility::Public,
attributes: Attributes::empty(),
is_unconstrained: false,
is_internal: None,
contract_function_type: None,
}
}
}
Expand Down Expand Up @@ -759,17 +748,13 @@ impl NodeInterner {
module: ModuleId,
location: Location,
) -> DefinitionId {
use ContractFunctionType::*;

// We're filling in contract_function_type and is_internal now, but these will be verified
// later during name resolution.
let modifiers = FunctionModifiers {
name: function.name.0.contents.clone(),
visibility: function.visibility,
attributes: function.attributes.clone(),
is_unconstrained: function.is_unconstrained,
contract_function_type: Some(if function.is_open { Open } else { Secret }),
is_internal: Some(function.is_internal),
};
self.push_function_definition(id, modifiers, module, location)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,6 @@ pub(super) fn function_definition(allow_self: bool) -> impl NoirParser<NoirFunct
name,
attributes,
is_unconstrained: modifiers.0,
// Whether a function is internal or is open is now set through `aztec_macros`
is_open: false,
is_internal: false,
visibility: modifiers.1,
generics,
parameters,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -120,11 +120,7 @@ pub(super) fn trait_implementation() -> impl NoirParser<TopLevelStatement> {

fn trait_implementation_body() -> impl NoirParser<Vec<TraitImplItem>> {
let function = function::function_definition(true).validate(|mut f, span, emit| {
if f.def().is_internal
|| f.def().is_unconstrained
|| f.def().is_open
|| f.def().visibility != FunctionVisibility::Private
{
if f.def().is_unconstrained || f.def().visibility != FunctionVisibility::Private {
emit(ParserError::with_reason(ParserErrorReason::TraitImplFunctionModifiers, span));
}
// Trait impl functions are always public
Expand Down
9 changes: 2 additions & 7 deletions noir/noir-repo/compiler/wasm/src/types/noir_artifact.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,19 +32,14 @@ export interface EventAbi {
fields: ABIVariable[];
}

/** The Noir function types. */
export type NoirFunctionType = 'Open' | 'Secret' | 'Unconstrained';

/**
* The compilation result of an Noir function.
*/
export interface NoirFunctionEntry {
/** The name of the function. */
name: string;
/** The type of the function. */
function_type: NoirFunctionType;
/** Whether the function is internal. */
is_internal: boolean;
/** The custom attributes applied to the function. */
custom_attributes: string[];
/** The ABI of the function. */
abi: Abi;
/** The bytecode of the function in base64. */
Expand Down
Loading

0 comments on commit cff565e

Please sign in to comment.