From 58e92d19a610f92bc436325c25c31092ddf7cd55 Mon Sep 17 00:00:00 2001 From: TomAFrench Date: Thu, 6 Jun 2024 11:35:27 +0000 Subject: [PATCH 1/4] chore: add more lints related to oracle calls --- .../noirc_evaluator/src/ssa/acir_gen/mod.rs | 1 + .../noirc_frontend/src/elaborator/lints.rs | 31 ++++++++++++++++- compiler/noirc_frontend/src/elaborator/mod.rs | 1 + .../noirc_frontend/src/elaborator/types.rs | 33 ++++++++++++++++--- .../src/hir/resolution/errors.rs | 16 +++++++++ 5 files changed, 76 insertions(+), 6 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs index 6d7c5e570c1..853a8e78bf3 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -839,6 +839,7 @@ impl<'a> Context<'a> { self.handle_ssa_call_outputs(result_ids, outputs, dfg)?; } Value::ForeignFunction(_) => { + // TODO: Remove this once elaborator is default frontend. This is now caught by a lint inside the frontend. return Err(RuntimeError::UnconstrainedOracleReturnToConstrained { call_stack: self.acir_context.get_call_stack(), }) diff --git a/compiler/noirc_frontend/src/elaborator/lints.rs b/compiler/noirc_frontend/src/elaborator/lints.rs index d228588d367..7b50591f2ee 100644 --- a/compiler/noirc_frontend/src/elaborator/lints.rs +++ b/compiler/noirc_frontend/src/elaborator/lints.rs @@ -10,7 +10,7 @@ use crate::{ HirExpression, HirLiteral, NodeInterner, NoirFunction, UnaryOp, UnresolvedTypeData, Visibility, }, - node_interner::{DefinitionKind, ExprId}, + node_interner::{DefinitionKind, ExprId, FuncId}, Type, }; use acvm::AcirField; @@ -72,6 +72,35 @@ pub(super) fn low_level_function_outside_stdlib( } } +/// Oracle definitions (functions with the `#[oracle]` attribute) must be marked as unconstrained. +pub(super) fn oracle_not_marked_unconstrained(func: &NoirFunction) -> Option { + let is_oracle_function = + func.attributes().function.as_ref().map_or(false, |func| func.is_oracle()); + if is_oracle_function && !func.def.is_unconstrained { + Some(ResolverError::OracleMarkedAsConstrained { ident: func.name_ident().clone() }) + } else { + None + } +} + +/// Oracle functions may not be called by constrained functions directly. +/// +/// In order for a constrained function to call an oracle it must first call through an unconstrained function. +pub(super) fn oracle_called_from_constrained_function( + interner: &NodeInterner, + called_func: &FuncId, + span: Span, +) -> Option { + let function_attributes = interner.function_attributes(called_func); + let is_oracle_call = + function_attributes.function.as_ref().map_or(false, |func| func.is_oracle()); + if is_oracle_call { + Some(ResolverError::UnconstrainedOracleReturnToConstrained { span }) + } else { + None + } +} + /// `pub` is required on return types for entry point functions pub(super) fn missing_pub(func: &NoirFunction, is_entry_point: bool) -> Option { if is_entry_point diff --git a/compiler/noirc_frontend/src/elaborator/mod.rs b/compiler/noirc_frontend/src/elaborator/mod.rs index 9403e12496f..ad7dba3400e 100644 --- a/compiler/noirc_frontend/src/elaborator/mod.rs +++ b/compiler/noirc_frontend/src/elaborator/mod.rs @@ -556,6 +556,7 @@ impl<'context> Elaborator<'context> { self.run_lint(|elaborator| { lints::unnecessary_pub_return(func, elaborator.pub_allowed(func)).map(Into::into) }); + self.run_lint(|_| lints::oracle_not_marked_unconstrained(func).map(Into::into)); self.run_lint(|elaborator| { lints::low_level_function_outside_stdlib(func, elaborator.crate_id).map(Into::into) }); diff --git a/compiler/noirc_frontend/src/elaborator/types.rs b/compiler/noirc_frontend/src/elaborator/types.rs index 159ad1c7493..a38472826b7 100644 --- a/compiler/noirc_frontend/src/elaborator/types.rs +++ b/compiler/noirc_frontend/src/elaborator/types.rs @@ -27,7 +27,9 @@ use crate::{ HirExpression, HirLiteral, HirStatement, Path, PathKind, SecondaryAttribute, Signedness, UnaryOp, UnresolvedType, UnresolvedTypeData, }, - node_interner::{DefinitionKind, ExprId, GlobalId, TraitId, TraitImplKind, TraitMethodId}, + node_interner::{ + DefinitionKind, ExprId, FuncId, GlobalId, TraitId, TraitImplKind, TraitMethodId, + }, Generics, Type, TypeBinding, TypeVariable, TypeVariableKind, }; @@ -1150,6 +1152,16 @@ impl<'context> Elaborator<'context> { let is_unconstrained_call = self.is_unconstrained_call(call.func); let crossing_runtime_boundary = is_current_func_constrained && is_unconstrained_call; if crossing_runtime_boundary { + let called_func_id = + self.func_id_from_expr_id(call.func).expect("Called function should exist"); + self.run_lint(|elaborator| { + lints::oracle_called_from_constrained_function( + elaborator.interner, + &called_func_id, + span, + ) + .map(Into::into) + }); let errors = lints::unconstrained_function_args(&args); for error in errors { self.push_err(error); @@ -1167,16 +1179,27 @@ impl<'context> Elaborator<'context> { return_type } - fn is_unconstrained_call(&self, expr: ExprId) -> bool { + fn func_id_from_expr_id(&self, expr: ExprId) -> Option { if let HirExpression::Ident(HirIdent { id, .. }, _) = self.interner.expression(&expr) { if let Some(DefinitionKind::Function(func_id)) = self.interner.try_definition(id).map(|def| &def.kind) { - let modifiers = self.interner.function_modifiers(func_id); - return modifiers.is_unconstrained; + Some(*func_id) + } else { + None } + } else { + None + } + } + + fn is_unconstrained_call(&self, expr: ExprId) -> bool { + if let Some(func_id) = self.func_id_from_expr_id(expr) { + let modifiers = self.interner.function_modifiers(&func_id); + modifiers.is_unconstrained + } else { + false } - false } /// Check if the given method type requires a mutable reference to the object type, and check diff --git a/compiler/noirc_frontend/src/hir/resolution/errors.rs b/compiler/noirc_frontend/src/hir/resolution/errors.rs index e8985deda11..d6dd1c2cd54 100644 --- a/compiler/noirc_frontend/src/hir/resolution/errors.rs +++ b/compiler/noirc_frontend/src/hir/resolution/errors.rs @@ -80,6 +80,12 @@ pub enum ResolverError { AbiAttributeOutsideContract { span: Span }, #[error("Usage of the `#[foreign]` or `#[builtin]` function attributes are not allowed outside of the Noir standard library")] LowLevelFunctionOutsideOfStdlib { ident: Ident }, + #[error( + "Usage of the `#[oracle]` function attribute is only valid on unconstrained functions" + )] + OracleMarkedAsConstrained { ident: Ident }, + #[error("Oracle functions cannot be called directly from constrained functions")] + UnconstrainedOracleReturnToConstrained { span: Span }, #[error("Dependency cycle found, '{item}' recursively depends on itself: {cycle} ")] DependencyCycle { span: Span, item: String, cycle: String }, #[error("break/continue are only allowed in unconstrained functions")] @@ -327,6 +333,16 @@ impl<'a> From<&'a ResolverError> for Diagnostic { "Usage of the `#[foreign]` or `#[builtin]` function attributes are not allowed outside of the Noir standard library".into(), ident.span(), ), + ResolverError::OracleMarkedAsConstrained { ident } => Diagnostic::simple_error( + error.to_string(), + "Oracle functions must have the `unconstrained` keyword applied".into(), + ident.span(), + ), + ResolverError::UnconstrainedOracleReturnToConstrained { span } => Diagnostic::simple_error( + error.to_string(), + "This oracle call must be wrapped in a call to another unconstrained function before being returned to a constrained runtime".into(), + *span, + ), ResolverError::DependencyCycle { span, item, cycle } => { Diagnostic::simple_error( "Dependency cycle found".into(), From 1ea017626e6583b67b41b9790843d455f2c8833b Mon Sep 17 00:00:00 2001 From: TomAFrench Date: Thu, 6 Jun 2024 11:52:35 +0000 Subject: [PATCH 2/4] chore: move method from elaborator to node interner --- .../noirc_evaluator/src/ssa/acir_gen/mod.rs | 2 +- .../noirc_frontend/src/elaborator/types.rs | 28 +++++-------------- compiler/noirc_frontend/src/node_interner.rs | 16 +++++++++++ 3 files changed, 24 insertions(+), 22 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs index 853a8e78bf3..2aac083d727 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -842,7 +842,7 @@ impl<'a> Context<'a> { // TODO: Remove this once elaborator is default frontend. This is now caught by a lint inside the frontend. return Err(RuntimeError::UnconstrainedOracleReturnToConstrained { call_stack: self.acir_context.get_call_stack(), - }) + }); } _ => unreachable!("expected calling a function but got {function_value:?}"), } diff --git a/compiler/noirc_frontend/src/elaborator/types.rs b/compiler/noirc_frontend/src/elaborator/types.rs index a38472826b7..712ae964195 100644 --- a/compiler/noirc_frontend/src/elaborator/types.rs +++ b/compiler/noirc_frontend/src/elaborator/types.rs @@ -17,7 +17,7 @@ use crate::{ }, hir_def::{ expr::{ - HirBinaryOp, HirCallExpression, HirIdent, HirMemberAccess, HirMethodReference, + HirBinaryOp, HirCallExpression, HirMemberAccess, HirMethodReference, HirPrefixExpression, }, function::{FuncMeta, Parameters}, @@ -27,9 +27,7 @@ use crate::{ HirExpression, HirLiteral, HirStatement, Path, PathKind, SecondaryAttribute, Signedness, UnaryOp, UnresolvedType, UnresolvedTypeData, }, - node_interner::{ - DefinitionKind, ExprId, FuncId, GlobalId, TraitId, TraitImplKind, TraitMethodId, - }, + node_interner::{DefinitionKind, ExprId, GlobalId, TraitId, TraitImplKind, TraitMethodId}, Generics, Type, TypeBinding, TypeVariable, TypeVariableKind, }; @@ -1152,8 +1150,10 @@ impl<'context> Elaborator<'context> { let is_unconstrained_call = self.is_unconstrained_call(call.func); let crossing_runtime_boundary = is_current_func_constrained && is_unconstrained_call; if crossing_runtime_boundary { - let called_func_id = - self.func_id_from_expr_id(call.func).expect("Called function should exist"); + let called_func_id = self + .interner + .lookup_function_from_expr(&call.func) + .expect("Called function should exist"); self.run_lint(|elaborator| { lints::oracle_called_from_constrained_function( elaborator.interner, @@ -1179,22 +1179,8 @@ impl<'context> Elaborator<'context> { return_type } - fn func_id_from_expr_id(&self, expr: ExprId) -> Option { - if let HirExpression::Ident(HirIdent { id, .. }, _) = self.interner.expression(&expr) { - if let Some(DefinitionKind::Function(func_id)) = - self.interner.try_definition(id).map(|def| &def.kind) - { - Some(*func_id) - } else { - None - } - } else { - None - } - } - fn is_unconstrained_call(&self, expr: ExprId) -> bool { - if let Some(func_id) = self.func_id_from_expr_id(expr) { + if let Some(func_id) = self.interner.lookup_function_from_expr(&expr) { let modifiers = self.interner.function_modifiers(&func_id); modifiers.is_unconstrained } else { diff --git a/compiler/noirc_frontend/src/node_interner.rs b/compiler/noirc_frontend/src/node_interner.rs index 40ca9ce04e0..7e4085f94a4 100644 --- a/compiler/noirc_frontend/src/node_interner.rs +++ b/compiler/noirc_frontend/src/node_interner.rs @@ -20,6 +20,7 @@ use crate::hir::def_map::{LocalModuleId, ModuleId}; use crate::ast::{BinaryOpKind, FunctionDefinition, ItemVisibility}; use crate::hir::resolution::errors::ResolverError; +use crate::hir_def::expr::HirIdent; use crate::hir_def::stmt::HirLetStatement; use crate::hir_def::traits::TraitImpl; use crate::hir_def::traits::{Trait, TraitConstraint}; @@ -824,6 +825,21 @@ impl NodeInterner { self.function_modules[&func] } + /// Returns the [`FuncId`] corresponding to the function referred to by `expr_id` + pub fn lookup_function_from_expr(&self, expr: &ExprId) -> Option { + if let HirExpression::Ident(HirIdent { id, .. }, _) = self.expression(expr) { + if let Some(DefinitionKind::Function(func_id)) = + self.try_definition(id).map(|def| &def.kind) + { + Some(*func_id) + } else { + None + } + } else { + None + } + } + /// Returns the interned HIR function corresponding to `func_id` // // Cloning HIR structures is cheap, so we return owned structures From 67c5dd0ce375ba2322cfaec0c7b5a215b2f6b8ab Mon Sep 17 00:00:00 2001 From: TomAFrench Date: Thu, 6 Jun 2024 13:36:29 +0000 Subject: [PATCH 3/4] chore: explicitly tell `oracle_called_from_constrained_function` whether call originates from constrained runtime --- compiler/noirc_frontend/src/elaborator/lints.rs | 5 +++++ compiler/noirc_frontend/src/elaborator/types.rs | 1 + 2 files changed, 6 insertions(+) diff --git a/compiler/noirc_frontend/src/elaborator/lints.rs b/compiler/noirc_frontend/src/elaborator/lints.rs index 7b50591f2ee..4859ac5f97c 100644 --- a/compiler/noirc_frontend/src/elaborator/lints.rs +++ b/compiler/noirc_frontend/src/elaborator/lints.rs @@ -89,8 +89,13 @@ pub(super) fn oracle_not_marked_unconstrained(func: &NoirFunction) -> Option Option { + if !calling_from_constrained_runtime { + return None; + } + let function_attributes = interner.function_attributes(called_func); let is_oracle_call = function_attributes.function.as_ref().map_or(false, |func| func.is_oracle()); diff --git a/compiler/noirc_frontend/src/elaborator/types.rs b/compiler/noirc_frontend/src/elaborator/types.rs index 712ae964195..9c1737a95b0 100644 --- a/compiler/noirc_frontend/src/elaborator/types.rs +++ b/compiler/noirc_frontend/src/elaborator/types.rs @@ -1158,6 +1158,7 @@ impl<'context> Elaborator<'context> { lints::oracle_called_from_constrained_function( elaborator.interner, &called_func_id, + is_current_func_constrained, span, ) .map(Into::into) From ad80e0ac54aff26f3b26cd8fd7152092f412cf7d Mon Sep 17 00:00:00 2001 From: TomAFrench Date: Thu, 6 Jun 2024 13:45:02 +0000 Subject: [PATCH 4/4] chore: format --- test_programs/execution_success/schnorr/src/main.nr | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test_programs/execution_success/schnorr/src/main.nr b/test_programs/execution_success/schnorr/src/main.nr index 8ff868bc61b..1067d9707b2 100644 --- a/test_programs/execution_success/schnorr/src/main.nr +++ b/test_programs/execution_success/schnorr/src/main.nr @@ -30,7 +30,7 @@ fn main( let pub_key = embedded_curve_ops::EmbeddedCurvePoint { x: pub_key_x, y: pub_key_y, is_infinite: false }; let valid_signature = verify_signature_noir(pub_key, signature, message2); assert(valid_signature); - assert_valid_signature(pub_key,signature,message2); + assert_valid_signature(pub_key, signature, message2); } // TODO: to put in the stdlib once we have numeric generics