fix: disallow returning constant values (#2978)

Co-authored-by: kevaundray <kevtheappdev@gmail.com>
noir-lang · Oct 12, 2023 · 0b5eecf · 0b5eecf
1 parent 8270cac
commit 0b5eecf
Show file tree

Hide file tree

Showing 30 changed files with 138 additions and 60 deletions.
diff --git a/compiler/noirc_errors/src/position.rs b/compiler/noirc_errors/src/position.rs
@@ -119,4 +119,8 @@ impl Location {
     pub fn new(span: Span, file: FileId) -> Self {
         Self { span, file }
     }
+
+    pub fn dummy() -> Self {
+        Self { span: Span::single_char(0), file: FileId::dummy() }
+    }
 }
diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs
@@ -153,7 +153,7 @@ impl<'block> BrilligBlock<'block> {
                     self.create_block_label_for_current_function(*destination_block),
                 );
             }
-            TerminatorInstruction::Return { return_values } => {
+            TerminatorInstruction::Return { return_values, .. } => {
                 let return_registers: Vec<_> = return_values
                     .iter()
                     .flat_map(|value_id| {

diff --git a/compiler/noirc_evaluator/src/errors.rs b/compiler/noirc_evaluator/src/errors.rs
@@ -67,6 +67,8 @@ 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 {
@@ -79,7 +81,8 @@ impl RuntimeError {
                 | InternalError::MissingArg { call_stack, .. }
                 | InternalError::NotAConstant { call_stack, .. }
                 | InternalError::UndeclaredAcirVar { call_stack }
-                | InternalError::UnExpected { call_stack, .. },
+                | InternalError::UnExpected { call_stack, .. }
+                | InternalError::ReturnConstant { call_stack, .. },
             )
             | RuntimeError::FailedConstraint { call_stack, .. }
             | RuntimeError::IndexOutOfBounds { call_stack, .. }
@@ -96,16 +99,21 @@ impl RuntimeError {
 impl From<RuntimeError> for FileDiagnostic {
     fn from(error: RuntimeError) -> FileDiagnostic {
         let call_stack = vecmap(error.call_stack(), |location| *location);
-        let diagnostic = error.into_diagnostic();
         let file_id = call_stack.last().map(|location| location.file).unwrap_or_default();
-
+        let diagnostic = error.into_diagnostic();
         diagnostic.in_file(file_id).with_call_stack(call_stack)
     }
 }
 
 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/acir_gen/acir_ir/acir_variable.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs
@@ -249,6 +249,11 @@ impl AcirContext {
         }
     }
 
+    /// True if the given AcirVar refers to a constant value
+    pub(crate) fn is_constant(&self, var: &AcirVar) -> bool {
+        matches!(self.vars[var], AcirVarData::Const(_))
+    }
+
     /// Adds a new Variable to context whose value will
     /// be constrained to be the negation of `var`.
     ///

diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs
@@ -1248,15 +1248,20 @@ impl Context {
         terminator: &TerminatorInstruction,
         dfg: &DataFlowGraph,
     ) -> Result<(), InternalError> {
-        let return_values = match terminator {
-            TerminatorInstruction::Return { return_values } => return_values,
+        let (return_values, call_stack) = match terminator {
+            TerminatorInstruction::Return { return_values, call_stack } => {
+                (return_values, call_stack)
+            }
             _ => unreachable!("ICE: Program must have a singular return"),
         };
 
         // 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);
         for acir_var in return_acir_vars {
+            if self.acir_context.is_constant(&acir_var) {
+                return Err(InternalError::ReturnConstant { call_stack: call_stack.clone() });
+            }
             self.acir_context.return_var(acir_var)?;
         }
         Ok(())
@@ -1869,6 +1874,7 @@ mod tests {
 
     use crate::{
         brillig::Brillig,
+        errors::{InternalError, RuntimeError},
         ssa::{
             function_builder::FunctionBuilder,
             ir::{function::RuntimeType, map::Id, types::Type},
@@ -1897,12 +1903,10 @@ mod tests {
         let ssa = builder.finish();
 
         let context = Context::new();
-        let mut acir = context.convert_ssa(ssa, Brillig::default(), &HashMap::default()).unwrap();
-
-        let expected_opcodes =
-            vec![Opcode::Arithmetic(&Expression::one() - &Expression::from(Witness(1)))];
-        assert_eq!(acir.take_opcodes(), expected_opcodes);
-        assert_eq!(acir.return_witnesses, vec![Witness(1)]);
+        let acir = context
+            .convert_ssa(ssa, Brillig::default(), &HashMap::default())
+            .expect_err("Return constant value");
+        assert!(matches!(acir, RuntimeError::InternalError(InternalError::ReturnConstant { .. })));
     }
 }
 //
diff --git a/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs b/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs
@@ -316,7 +316,8 @@ impl FunctionBuilder {
 
     /// Terminate the current block with a return instruction
     pub(crate) fn terminate_with_return(&mut self, return_values: Vec<ValueId>) {
-        self.terminate_block_with(TerminatorInstruction::Return { return_values });
+        let call_stack = self.call_stack.clone();
+        self.terminate_block_with(TerminatorInstruction::Return { return_values, call_stack });
     }
 
     /// Returns a ValueId pointing to the given function or imports the function

diff --git a/compiler/noirc_evaluator/src/ssa/ir/basic_block.rs b/compiler/noirc_evaluator/src/ssa/ir/basic_block.rs
@@ -1,4 +1,5 @@
 use super::{
+    dfg::CallStack,
     instruction::{InstructionId, TerminatorInstruction},
     map::Id,
     value::ValueId,
@@ -117,7 +118,13 @@ impl BasicBlock {
     /// being kept, which is likely unwanted.
     pub(crate) fn take_terminator(&mut self) -> TerminatorInstruction {
         let terminator = self.terminator.as_mut().expect("Expected block to have a terminator");
-        std::mem::replace(terminator, TerminatorInstruction::Return { return_values: Vec::new() })
+        std::mem::replace(
+            terminator,
+            TerminatorInstruction::Return {
+                return_values: Vec::new(),
+                call_stack: CallStack::new(),
+            },
+        )
     }
 
     /// Return the jmp arguments, if any, of this block's TerminatorInstruction.

diff --git a/compiler/noirc_evaluator/src/ssa/ir/cfg.rs b/compiler/noirc_evaluator/src/ssa/ir/cfg.rs
@@ -131,7 +131,9 @@ impl ControlFlowGraph {
 
 #[cfg(test)]
 mod tests {
-    use crate::ssa::ir::{instruction::TerminatorInstruction, map::Id, types::Type};
+    use crate::ssa::ir::{
+        dfg::CallStack, instruction::TerminatorInstruction, map::Id, types::Type,
+    };
 
     use super::{super::function::Function, ControlFlowGraph};
 
@@ -140,7 +142,10 @@ mod tests {
         let func_id = Id::test_new(0);
         let mut func = Function::new("func".into(), func_id);
         let block_id = func.entry_block();
-        func.dfg[block_id].set_terminator(TerminatorInstruction::Return { return_values: vec![] });
+        func.dfg[block_id].set_terminator(TerminatorInstruction::Return {
+            return_values: vec![],
+            call_stack: CallStack::new(),
+        });
 
         ControlFlowGraph::with_function(&func);
     }
@@ -173,7 +178,10 @@ mod tests {
             then_destination: block1_id,
             else_destination: block2_id,
         });
-        func.dfg[block2_id].set_terminator(TerminatorInstruction::Return { return_values: vec![] });
+        func.dfg[block2_id].set_terminator(TerminatorInstruction::Return {
+            return_values: vec![],
+            call_stack: CallStack::new(),
+        });
 
         let mut cfg = ControlFlowGraph::with_function(&func);
 
@@ -218,8 +226,10 @@ mod tests {
         //     return ()
         // }
         let ret_block_id = func.dfg.make_block();
-        func.dfg[ret_block_id]
-            .set_terminator(TerminatorInstruction::Return { return_values: vec![] });
+        func.dfg[ret_block_id].set_terminator(TerminatorInstruction::Return {
+            return_values: vec![],
+            call_stack: CallStack::new(),
+        });
         func.dfg[block2_id].set_terminator(TerminatorInstruction::Jmp {
             destination: ret_block_id,
             arguments: vec![],

diff --git a/compiler/noirc_evaluator/src/ssa/ir/dom.rs b/compiler/noirc_evaluator/src/ssa/ir/dom.rs
@@ -250,6 +250,7 @@ mod tests {
         function_builder::FunctionBuilder,
         ir::{
             basic_block::BasicBlockId,
+            dfg::CallStack,
             dom::DominatorTree,
             function::{Function, RuntimeType},
             instruction::TerminatorInstruction,
@@ -265,7 +266,7 @@ mod tests {
         let block0_id = func.entry_block();
         func.dfg.set_block_terminator(
             block0_id,
-            TerminatorInstruction::Return { return_values: vec![] },
+            TerminatorInstruction::Return { return_values: vec![], call_stack: CallStack::new() },
         );
         let mut dom_tree = DominatorTree::with_function(&func);
         assert!(dom_tree.dominates(block0_id, block0_id));

diff --git a/compiler/noirc_evaluator/src/ssa/ir/function.rs b/compiler/noirc_evaluator/src/ssa/ir/function.rs
@@ -93,7 +93,7 @@ impl Function {
         let mut function_return_values = None;
         for block in blocks {
             let terminator = self.dfg[block].terminator();
-            if let Some(TerminatorInstruction::Return { return_values }) = terminator {
+            if let Some(TerminatorInstruction::Return { return_values, .. }) = terminator {
                 function_return_values = Some(return_values);
                 break;
             }

diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs
@@ -536,7 +536,7 @@ pub(crate) enum TerminatorInstruction {
     /// unconditionally jump to a single exit block with the return values
     /// as the block arguments. Then the exit block can terminate in a return
     /// instruction returning these values.
-    Return { return_values: Vec<ValueId> },
+    Return { return_values: Vec<ValueId>, call_stack: CallStack },
 }
 
 impl TerminatorInstruction {
@@ -557,9 +557,10 @@ impl TerminatorInstruction {
                 arguments: vecmap(arguments, |value| f(*value)),
                 call_stack: call_stack.clone(),
             },
-            Return { return_values } => {
-                Return { return_values: vecmap(return_values, |value| f(*value)) }
-            }
+            Return { return_values, call_stack } => Return {
+                return_values: vecmap(return_values, |value| f(*value)),
+                call_stack: call_stack.clone(),
+            },
         }
     }
 
@@ -575,7 +576,7 @@ impl TerminatorInstruction {
                     *argument = f(*argument);
                 }
             }
-            Return { return_values } => {
+            Return { return_values, .. } => {
                 for return_value in return_values {
                     *return_value = f(*return_value);
                 }
@@ -595,7 +596,7 @@ impl TerminatorInstruction {
                     f(*argument);
                 }
             }
-            Return { return_values } => {
+            Return { return_values, .. } => {
                 for return_value in return_values {
                     f(*return_value);
                 }

diff --git a/compiler/noirc_evaluator/src/ssa/ir/printer.rs b/compiler/noirc_evaluator/src/ssa/ir/printer.rs
@@ -112,7 +112,7 @@ pub(crate) fn display_terminator(
                 else_destination
             )
         }
-        Some(TerminatorInstruction::Return { return_values }) => {
+        Some(TerminatorInstruction::Return { return_values, .. }) => {
             writeln!(f, "    return {}", value_list(function, return_values))
         }
         None => writeln!(f, "    (no terminator instruction)"),

diff --git a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs
@@ -234,7 +234,7 @@ mod test {
         assert_eq!(block.instructions().len(), 0);
 
         match block.terminator() {
-            Some(TerminatorInstruction::Return { return_values }) => {
+            Some(TerminatorInstruction::Return { return_values, .. }) => {
                 let value = main
                     .dfg
                     .get_numeric_constant(return_values[0])
@@ -278,7 +278,7 @@ mod test {
         assert_ne!(new_add_instr_result, v1);
 
         let return_value_id = match entry_block.unwrap_terminator() {
-            TerminatorInstruction::Return { return_values } => return_values[0],
+            TerminatorInstruction::Return { return_values, .. } => return_values[0],
             _ => unreachable!(),
         };
         let return_element = match &main.dfg[return_value_id] {

diff --git a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs
@@ -325,11 +325,13 @@ impl<'f> Context<'f> {
                 let arguments = vecmap(arguments.clone(), |value| self.inserter.resolve(value));
                 self.inline_block(destination, &arguments)
             }
-            TerminatorInstruction::Return { return_values } => {
+            TerminatorInstruction::Return { return_values, call_stack } => {
+                let call_stack = call_stack.clone();
                 let return_values =
                     vecmap(return_values.clone(), |value| self.inserter.resolve(value));
+                let new_return = TerminatorInstruction::Return { return_values, call_stack };
                 let entry = self.inserter.function.entry_block();
-                let new_return = TerminatorInstruction::Return { return_values };
+
                 self.inserter.function.dfg.set_block_terminator(entry, new_return);
                 block
             }
@@ -1079,7 +1081,7 @@ mod test {
 
         let main = ssa.main();
         let ret = match main.dfg[main.entry_block()].terminator() {
-            Some(TerminatorInstruction::Return { return_values }) => return_values[0],
+            Some(TerminatorInstruction::Return { return_values, .. }) => return_values[0],
             _ => unreachable!(),
         };
 
@@ -1442,7 +1444,7 @@ mod test {
 
         // The return value should be 200, not 310
         match main.dfg[main.entry_block()].terminator() {
-            Some(TerminatorInstruction::Return { return_values }) => {
+            Some(TerminatorInstruction::Return { return_values, .. }) => {
                 match main.dfg.get_numeric_constant(return_values[0]) {
                     Some(constant) => {
                         let value = constant.to_u128();

diff --git a/compiler/noirc_evaluator/src/ssa/opt/inlining.rs b/compiler/noirc_evaluator/src/ssa/opt/inlining.rs
@@ -480,10 +480,15 @@ impl<'function> PerFunctionContext<'function> {
                 }
                 None
             }
-            TerminatorInstruction::Return { return_values } => {
+            TerminatorInstruction::Return { return_values, call_stack } => {
                 let return_values = vecmap(return_values, |value| self.translate_value(*value));
                 if self.inlining_entry {
-                    self.context.builder.terminate_with_return(return_values.clone());
+                    let mut new_call_stack = self.context.call_stack.clone();
+                    new_call_stack.append(call_stack.clone());
+                    self.context
+                        .builder
+                        .set_call_stack(new_call_stack)
+                        .terminate_with_return(return_values.clone());
                 }
                 // Note that `translate_block` would take us back to the point at which the
                 // inlining of this source block began. Since additional blocks may have been
@@ -686,7 +691,7 @@ mod test {
         let b6 = &main.dfg[b6_id];
 
         match b6.terminator() {
-            Some(TerminatorInstruction::Return { return_values }) => {
+            Some(TerminatorInstruction::Return { return_values, .. }) => {
                 assert_eq!(return_values.len(), 1);
                 let value = main
                     .dfg

diff --git a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs
@@ -385,7 +385,7 @@ impl<'f> PerFunctionContext<'f> {
                     }
                 }
             }
-            TerminatorInstruction::Return { return_values } => {
+            TerminatorInstruction::Return { return_values, .. } => {
                 // Removing all `last_stores` for each returned reference is more important here
                 // than setting them all to ReferenceValue::Unknown since no other block should
                 // have a block with a Return terminator as a predecessor anyway.
@@ -449,7 +449,7 @@ mod tests {
         assert_eq!(count_stores(block_id, &func.dfg), 0);
 
         let ret_val_id = match func.dfg[block_id].terminator().unwrap() {
-            TerminatorInstruction::Return { return_values } => return_values.first().unwrap(),
+            TerminatorInstruction::Return { return_values, .. } => return_values.first().unwrap(),
             _ => unreachable!(),
         };
         assert_eq!(func.dfg[*ret_val_id], func.dfg[two]);
@@ -485,7 +485,7 @@ mod tests {
         assert_eq!(count_stores(block_id, &func.dfg), 1);
 
         let ret_val_id = match func.dfg[block_id].terminator().unwrap() {
-            TerminatorInstruction::Return { return_values } => return_values.first().unwrap(),
+            TerminatorInstruction::Return { return_values, .. } => return_values.first().unwrap(),
             _ => unreachable!(),
         };
         assert_eq!(func.dfg[*ret_val_id], func.dfg[one]);
@@ -518,7 +518,7 @@ mod tests {
         assert_eq!(instructions.len(), 2);
 
         let ret_val_id = match func.dfg[block_id].terminator().unwrap() {
-            TerminatorInstruction::Return { return_values } => *return_values.first().unwrap(),
+            TerminatorInstruction::Return { return_values, .. } => *return_values.first().unwrap(),
             _ => unreachable!(),
         };