feat: handle warnings in evaluator (#3205)

Co-authored-by: kevaundray <kevtheappdev@gmail.com>
Co-authored-by: github-merge-queue[bot] <github-merge-queue[bot]@users.noreply.github.com>
Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com>
Co-authored-by: kek kek kek <andriy.n@obox.systems>
Co-authored-by: Martin Verzilli <martin.verzilli@gmail.com>
Co-authored-by: Savio <72797635+Savio-Sou@users.noreply.github.com>
Co-authored-by: Alex Gherghisan <alexghr@users.noreply.github.com>
Co-authored-by: vezenovm <mvezenov@gmail.com>

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

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<CompiledProgram>,
     force_compile: bool,
 ) -> CompilationResult<CompiledProgram> {
-    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<CompiledProgram>,
     force_compile: bool,
-) -> Result<CompiledProgram, RuntimeError> {
+) -> Result<(CompiledProgram, Vec<SsaReport>), 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,
+    ))
 }

compiler/noirc_evaluator/src/errors.rs

@@ -53,6 +53,37 @@ fn format_failed_constraint(message: &Option<String>) -> String {
     }
 }
 
+#[derive(Debug)]
+pub enum SsaReport {
+    Warning(InternalWarning),
+}
+
+impl From<SsaReport> 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<RuntimeError> 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

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<SsaReport>), 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.

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<u32>) -> GeneratedAcir {
+    pub(crate) fn finish(mut self, inputs: Vec<u32>, warnings: Vec<SsaReport>) -> GeneratedAcir {
         self.acir_ir.input_witnesses = vecmap(inputs, Witness);
+        self.acir_ir.warnings = warnings;
         self.acir_ir
     }
 

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<OpcodeLocation, String>,
+
+    pub(crate) warnings: Vec<SsaReport>,
 }
 
 impl GeneratedAcir {

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<Vec<SsaReport>, 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

tooling/nargo/src/ops/test.rs

@@ -23,7 +23,7 @@ pub fn run_test<B: BlackBoxFunctionSolver>(
 ) -> 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 =