diff --git a/Cargo.lock b/Cargo.lock index 4f9f964181c..a00e70baf34 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2628,6 +2628,7 @@ dependencies = [ "clap", "fm", "fxhash", + "iter-extended", "noirc_abi", "noirc_errors", "noirc_evaluator", diff --git a/compiler/noirc_driver/Cargo.toml b/compiler/noirc_driver/Cargo.toml index f1c120e1687..31c0f695d0f 100644 --- a/compiler/noirc_driver/Cargo.toml +++ b/compiler/noirc_driver/Cargo.toml @@ -17,6 +17,7 @@ noirc_frontend.workspace = true noirc_evaluator.workspace = true noirc_abi.workspace = true acvm.workspace = true +iter-extended.workspace = true fm.workspace = true serde.workspace = true base64.workspace = true diff --git a/compiler/noirc_driver/src/lib.rs b/compiler/noirc_driver/src/lib.rs index f8908efc596..2afc3216f70 100644 --- a/compiler/noirc_driver/src/lib.rs +++ b/compiler/noirc_driver/src/lib.rs @@ -6,9 +6,11 @@ use clap::Args; use debug::filter_relevant_files; use fm::FileId; +use iter_extended::vecmap; use noirc_abi::{AbiParameter, AbiType, ContractEvent}; use noirc_errors::{CustomDiagnostic, FileDiagnostic}; use noirc_evaluator::errors::RuntimeError; +use noirc_evaluator::errors::SsaReport; use noirc_evaluator::{create_circuit, into_abi_params}; use noirc_frontend::graph::{CrateId, CrateName}; use noirc_frontend::hir::def_map::{Contract, CrateDefMap}; @@ -164,7 +166,7 @@ pub fn compile_main( cached_program: Option, force_compile: bool, ) -> CompilationResult { - let (_, warnings) = check_crate(context, crate_id, options.deny_warnings)?; + let (_, mut warnings) = check_crate(context, crate_id, options.deny_warnings)?; let main = match context.get_main_function(&crate_id) { Some(m) => m, @@ -178,8 +180,14 @@ pub fn compile_main( } }; - let compiled_program = compile_no_check(context, options, main, cached_program, force_compile) - .map_err(FileDiagnostic::from)?; + let (compiled_program, compilation_warnings) = + compile_no_check(context, options, main, cached_program, force_compile) + .map_err(FileDiagnostic::from)?; + let compilation_warnings = vecmap(compilation_warnings, FileDiagnostic::from); + if options.deny_warnings && !compilation_warnings.is_empty() { + return Err(compilation_warnings); + } + warnings.extend(compilation_warnings); if options.print_acir { println!("Compiled ACIR for main (unoptimized):"); @@ -274,7 +282,7 @@ fn compile_contract_inner( } let function = match compile_no_check(context, options, function_id, None, true) { - Ok(function) => function, + Ok((function, _warnings)) => function, Err(new_error) => { errors.push(FileDiagnostic::from(new_error)); continue; @@ -332,7 +340,7 @@ pub fn compile_no_check( main_function: FuncId, cached_program: Option, force_compile: bool, -) -> Result { +) -> Result<(CompiledProgram, Vec), RuntimeError> { let program = monomorphize(main_function, &context.def_interner); let hash = fxhash::hash64(&program); @@ -342,22 +350,25 @@ pub fn compile_no_check( if !(force_compile || options.print_acir || options.show_brillig || options.show_ssa) { if let Some(cached_program) = cached_program { if hash == cached_program.hash { - return Ok(cached_program); + return Ok((cached_program, Vec::new())); } } } - let (circuit, debug, abi) = + let (circuit, debug, abi, warnings) = create_circuit(context, program, options.show_ssa, options.show_brillig)?; let file_map = filter_relevant_files(&[debug.clone()], &context.file_manager); - Ok(CompiledProgram { - hash, - circuit, - debug, - abi, - file_map, - noir_version: NOIR_ARTIFACT_VERSION_STRING.to_string(), - }) + Ok(( + CompiledProgram { + hash, + circuit, + debug, + abi, + file_map, + noir_version: NOIR_ARTIFACT_VERSION_STRING.to_string(), + }, + warnings, + )) } diff --git a/compiler/noirc_evaluator/src/errors.rs b/compiler/noirc_evaluator/src/errors.rs index 58d13c0affd..42381d6441e 100644 --- a/compiler/noirc_evaluator/src/errors.rs +++ b/compiler/noirc_evaluator/src/errors.rs @@ -53,6 +53,37 @@ fn format_failed_constraint(message: &Option) -> String { } } +#[derive(Debug)] +pub enum SsaReport { + Warning(InternalWarning), +} + +impl From for FileDiagnostic { + fn from(error: SsaReport) -> FileDiagnostic { + match error { + SsaReport::Warning(warning) => { + let InternalWarning::ReturnConstant { ref call_stack } = warning; + let call_stack = vecmap(call_stack, |location| *location); + let file_id = call_stack.last().map(|location| location.file).unwrap_or_default(); + let message = warning.to_string(); + let location = call_stack.last().expect("Expected RuntimeError to have a location"); + let diagnostic = Diagnostic::simple_warning( + message, + "constant value".to_string(), + location.span, + ); + diagnostic.in_file(file_id).with_call_stack(call_stack) + } + } + } +} + +#[derive(Debug, PartialEq, Eq, Clone, Error)] +pub enum InternalWarning { + #[error("Returning a constant value is not allowed")] + ReturnConstant { call_stack: CallStack }, +} + #[derive(Debug, PartialEq, Eq, Clone, Error)] pub enum InternalError { #[error("ICE: Both expressions should have degree<=1")] @@ -69,8 +100,6 @@ pub enum InternalError { UndeclaredAcirVar { call_stack: CallStack }, #[error("ICE: Expected {expected:?}, found {found:?}")] UnExpected { expected: String, found: String, call_stack: CallStack }, - #[error("Returning a constant value is not allowed")] - ReturnConstant { call_stack: CallStack }, } impl RuntimeError { @@ -83,8 +112,7 @@ impl RuntimeError { | InternalError::MissingArg { call_stack, .. } | InternalError::NotAConstant { call_stack, .. } | InternalError::UndeclaredAcirVar { call_stack } - | InternalError::UnExpected { call_stack, .. } - | InternalError::ReturnConstant { call_stack, .. }, + | InternalError::UnExpected { call_stack, .. }, ) | RuntimeError::FailedConstraint { call_stack, .. } | RuntimeError::IndexOutOfBounds { call_stack, .. } @@ -111,12 +139,6 @@ impl From for FileDiagnostic { impl RuntimeError { fn into_diagnostic(self) -> Diagnostic { match self { - RuntimeError::InternalError(InternalError::ReturnConstant { ref call_stack }) => { - let message = self.to_string(); - let location = - call_stack.back().expect("Expected RuntimeError to have a location"); - Diagnostic::simple_error(message, "constant value".to_string(), location.span) - } RuntimeError::InternalError(cause) => { Diagnostic::simple_error( "Internal Consistency Evaluators Errors: \n diff --git a/compiler/noirc_evaluator/src/ssa.rs b/compiler/noirc_evaluator/src/ssa.rs index 7cf51a4f0b3..ff13878e129 100644 --- a/compiler/noirc_evaluator/src/ssa.rs +++ b/compiler/noirc_evaluator/src/ssa.rs @@ -9,7 +9,7 @@ use std::collections::BTreeSet; -use crate::errors::RuntimeError; +use crate::errors::{RuntimeError, SsaReport}; use acvm::acir::{ circuit::{Circuit, PublicInputs}, native_types::Witness, @@ -72,7 +72,7 @@ pub fn create_circuit( program: Program, enable_ssa_logging: bool, enable_brillig_logging: bool, -) -> Result<(Circuit, DebugInfo, Abi), RuntimeError> { +) -> Result<(Circuit, DebugInfo, Abi, Vec), RuntimeError> { let func_sig = program.main_function_signature.clone(); let mut generated_acir = optimize_into_acir(program, enable_ssa_logging, enable_brillig_logging)?; @@ -83,6 +83,7 @@ pub fn create_circuit( locations, input_witnesses, assert_messages, + warnings, .. } = generated_acir; @@ -119,7 +120,7 @@ pub fn create_circuit( let (optimized_circuit, transformation_map) = acvm::compiler::optimize(circuit); debug_info.update_acir(transformation_map); - Ok((optimized_circuit, debug_info, abi)) + Ok((optimized_circuit, debug_info, abi, warnings)) } // This is just a convenience object to bundle the ssa with `print_ssa_passes` for debug printing. diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs index f54ddd9f4d4..8d0111fc8e3 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs @@ -1,7 +1,7 @@ use super::generated_acir::GeneratedAcir; use crate::brillig::brillig_gen::brillig_directive; use crate::brillig::brillig_ir::artifact::GeneratedBrillig; -use crate::errors::{InternalError, RuntimeError}; +use crate::errors::{InternalError, RuntimeError, SsaReport}; use crate::ssa::acir_gen::{AcirDynamicArray, AcirValue}; use crate::ssa::ir::dfg::CallStack; use crate::ssa::ir::types::Type as SsaType; @@ -1162,8 +1162,9 @@ impl AcirContext { } /// Terminates the context and takes the resulting `GeneratedAcir` - pub(crate) fn finish(mut self, inputs: Vec) -> GeneratedAcir { + pub(crate) fn finish(mut self, inputs: Vec, warnings: Vec) -> GeneratedAcir { self.acir_ir.input_witnesses = vecmap(inputs, Witness); + self.acir_ir.warnings = warnings; self.acir_ir } diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs index 221137b9dab..f29d3c9ec05 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs @@ -4,7 +4,7 @@ use std::collections::BTreeMap; use crate::{ brillig::{brillig_gen::brillig_directive, brillig_ir::artifact::GeneratedBrillig}, - errors::{InternalError, RuntimeError}, + errors::{InternalError, RuntimeError, SsaReport}, ssa::ir::dfg::CallStack, }; @@ -53,6 +53,8 @@ pub(crate) struct GeneratedAcir { /// Correspondence between an opcode index and the error message associated with it. pub(crate) assert_messages: BTreeMap, + + pub(crate) warnings: Vec, } impl GeneratedAcir { diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs index bbd53a5a204..0c0ec7e88d2 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -23,7 +23,7 @@ use super::{ use crate::brillig::brillig_ir::artifact::GeneratedBrillig; use crate::brillig::brillig_ir::BrilligContext; use crate::brillig::{brillig_gen::brillig_fn::FunctionContext as BrilligFunctionContext, Brillig}; -use crate::errors::{InternalError, RuntimeError}; +use crate::errors::{InternalError, InternalWarning, RuntimeError, SsaReport}; pub(crate) use acir_ir::generated_acir::GeneratedAcir; use acvm::{ acir::{circuit::opcodes::BlockId, native_types::Expression}, @@ -201,9 +201,9 @@ impl Context { self.convert_ssa_instruction(*instruction_id, dfg, ssa, &brillig, last_array_uses)?; } - self.convert_ssa_return(entry_block.unwrap_terminator(), dfg)?; + let warnings = self.convert_ssa_return(entry_block.unwrap_terminator(), dfg)?; - Ok(self.acir_context.finish(input_witness.collect())) + Ok(self.acir_context.finish(input_witness.collect(), warnings)) } fn convert_brillig_main( @@ -240,7 +240,7 @@ impl Context { self.acir_context.return_var(acir_var)?; } - Ok(self.acir_context.finish(witness_inputs)) + Ok(self.acir_context.finish(witness_inputs, Vec::new())) } /// Adds and binds `AcirVar`s for each numeric block parameter or block parameter array element. @@ -1247,8 +1247,8 @@ impl Context { &mut self, terminator: &TerminatorInstruction, dfg: &DataFlowGraph, - ) -> Result<(), InternalError> { - let (return_values, _call_stack) = match terminator { + ) -> Result, InternalError> { + let (return_values, call_stack) = match terminator { TerminatorInstruction::Return { return_values, call_stack } => { (return_values, call_stack) } @@ -1258,16 +1258,16 @@ impl Context { // The return value may or may not be an array reference. Calling `flatten_value_list` // will expand the array if there is one. let return_acir_vars = self.flatten_value_list(return_values, dfg); + let mut warnings = Vec::new(); for acir_var in return_acir_vars { - // TODO(Guillaume) -- disabled as it has shown to break - // TODO with important programs. We will add it back once - // TODO we change it to a warning. - // if self.acir_context.is_constant(&acir_var) { - // return Err(InternalError::ReturnConstant { call_stack: call_stack.clone() }); - // } + if self.acir_context.is_constant(&acir_var) { + warnings.push(SsaReport::Warning(InternalWarning::ReturnConstant { + call_stack: call_stack.clone(), + })); + } self.acir_context.return_var(acir_var)?; } - Ok(()) + Ok(warnings) } /// Gets the cached `AcirVar` that was converted from the corresponding `ValueId`. If it does diff --git a/tooling/nargo/src/ops/test.rs b/tooling/nargo/src/ops/test.rs index d2ef2659e4d..6e0e553abf3 100644 --- a/tooling/nargo/src/ops/test.rs +++ b/tooling/nargo/src/ops/test.rs @@ -23,7 +23,7 @@ pub fn run_test( ) -> TestStatus { let program = compile_no_check(context, config, test_function.get_id(), None, false); match program { - Ok(program) => { + Ok((program, _)) => { // Run the backend to ensure the PWG evaluates functions like std::hash::pedersen, // otherwise constraints involving these expressions will not error. let circuit_execution =