From b94c70588eeb31bc08051d6498f50347b07fdf03 Mon Sep 17 00:00:00 2001 From: collin Date: Thu, 3 Dec 2020 13:24:23 -0500 Subject: [PATCH 01/10] refactor execution of branch indicators and function returns --- compiler/src/console/assert.rs | 18 +-- compiler/src/console/console.rs | 23 +--- compiler/src/errors/statement.rs | 16 +++ compiler/src/function/function.rs | 30 +---- compiler/src/function/result/result.rs | 115 ++++++++++++------ compiler/src/statement/assign/array.rs | 8 +- compiler/src/statement/assign/assign.rs | 5 +- .../src/statement/assign/circuit_variable.rs | 6 +- compiler/src/statement/assign/tuple.rs | 7 +- compiler/src/statement/branch/branch.rs | 2 +- .../src/statement/conditional/conditional.rs | 16 +-- compiler/src/statement/iteration/iteration.rs | 2 +- compiler/src/statement/statement.rs | 15 ++- 13 files changed, 145 insertions(+), 118 deletions(-) diff --git a/compiler/src/console/assert.rs b/compiler/src/console/assert.rs index 66465bd11f..e3df68ba9d 100644 --- a/compiler/src/console/assert.rs +++ b/compiler/src/console/assert.rs @@ -16,7 +16,13 @@ //! Enforces an assert equals statement in a compiled Leo program. -use crate::{errors::ConsoleError, program::ConstrainedProgram, value::ConstrainedValue, GroupType}; +use crate::{ + errors::ConsoleError, + get_indicator_value, + program::ConstrainedProgram, + value::ConstrainedValue, + GroupType, +}; use leo_ast::{Expression, Span, Type}; use snarkos_models::{ @@ -30,7 +36,7 @@ impl> ConstrainedProgram { cs: &mut CS, file_scope: &str, function_scope: &str, - indicator: Option, + indicator: &Boolean, expression: Expression, span: &Span, ) -> Result<(), ConsoleError> { @@ -42,12 +48,8 @@ impl> ConstrainedProgram { // If the indicator bit is false, do not evaluate the assertion // This is okay since we are not enforcing any constraints - let false_boolean = Boolean::Constant(false); - - if let Some(indicator_bool) = indicator { - if indicator_bool.eq(&false_boolean) { - return Ok(()); // continue execution - } + if !get_indicator_value(indicator) { + return Ok(()); // Continue execution. } // Unwrap assertion value and handle errors diff --git a/compiler/src/console/console.rs b/compiler/src/console/console.rs index 1be1d801cb..23a7276cfa 100644 --- a/compiler/src/console/console.rs +++ b/compiler/src/console/console.rs @@ -16,7 +16,7 @@ //! Evaluates a macro in a compiled Leo program. -use crate::{errors::ConsoleError, program::ConstrainedProgram, GroupType}; +use crate::{errors::ConsoleError, program::ConstrainedProgram, statement::get_indicator_value, GroupType}; use leo_ast::{ConsoleFunction, ConsoleFunctionCall}; use snarkos_models::{ @@ -30,7 +30,7 @@ impl> ConstrainedProgram { cs: &mut CS, file_scope: &str, function_scope: &str, - indicator: Option, + indicator: &Boolean, console: ConsoleFunctionCall, ) -> Result<(), ConsoleError> { match console.function { @@ -40,21 +40,21 @@ impl> ConstrainedProgram { ConsoleFunction::Debug(string) => { let string = self.format(cs, file_scope, function_scope, string)?; - if unwrap_indicator_value(indicator) { + if get_indicator_value(indicator) { tracing::debug!("{}", string); } } ConsoleFunction::Error(string) => { let string = self.format(cs, file_scope, function_scope, string)?; - if unwrap_indicator_value(indicator) { + if get_indicator_value(indicator) { tracing::error!("{}", string); } } ConsoleFunction::Log(string) => { let string = self.format(cs, file_scope, function_scope, string)?; - if unwrap_indicator_value(indicator) { + if get_indicator_value(indicator) { tracing::info!("{}", string); } } @@ -63,16 +63,3 @@ impl> ConstrainedProgram { Ok(()) } } - -// Return the indicator boolean gadget value or true if it is None -// This is okay since we are not enforcing any constraints -fn unwrap_indicator_value(indicator: Option) -> bool { - let false_boolean = Boolean::constant(false); - - if let Some(indicator_bool) = indicator { - if indicator_bool.eq(&false_boolean) { - return false; - } - } - true -} diff --git a/compiler/src/errors/statement.rs b/compiler/src/errors/statement.rs index a70770ca53..4fd0dc7d5e 100644 --- a/compiler/src/errors/statement.rs +++ b/compiler/src/errors/statement.rs @@ -135,6 +135,22 @@ impl StatementError { Self::new_from_span(message, span) } + pub fn multiple_returns(span: Span) -> Self { + let message = + format!("This function returns multiple times and produces unreachable circuits with undefined behavior."); + + Self::new_from_span(message, span) + } + + pub fn no_returns(expected: Type, span: Span) -> Self { + let message = format!( + "function expected `{}` return type but no valid branches returned a result", + expected + ); + + Self::new_from_span(message, span) + } + pub fn select_fail(first: String, second: String, span: Span) -> Self { let message = format!( "Conditional select gadget failed to select between `{}` or `{}`", diff --git a/compiler/src/function/function.rs b/compiler/src/function/function.rs index ab944103f0..ea79ddeb46 100644 --- a/compiler/src/function/function.rs +++ b/compiler/src/function/function.rs @@ -23,11 +23,11 @@ use crate::{ GroupType, }; -use leo_ast::{Expression, Function, FunctionInput, Span, Type}; +use leo_ast::{Expression, Function, FunctionInput, Span}; use snarkos_models::{ curves::{Field, PrimeField}, - gadgets::r1cs::ConstraintSystem, + gadgets::{r1cs::ConstraintSystem, utilities::boolean::Boolean}, }; pub fn check_arguments_length(expected: usize, actual: usize, span: &Span) -> Result<(), FunctionError> { @@ -89,13 +89,14 @@ impl> ConstrainedProgram { // Evaluate every statement in the function and save all potential results let mut results = vec![]; + let indicator = Boolean::constant(true); for statement in function.statements.iter() { let mut result = self.enforce_statement( cs, scope, &function_name, - None, + &indicator, statement.clone(), function.output.clone(), declared_circuit_reference, @@ -105,26 +106,7 @@ impl> ConstrainedProgram { } // Conditionally select a result based on returned indicators - let mut return_values = ConstrainedValue::Tuple(vec![]); - - Self::conditionally_select_result(cs, &mut return_values, results, &function.span)?; - - if let ConstrainedValue::Tuple(ref returns) = return_values { - let return_types = match function.output { - Some(Type::Tuple(types)) => types.len(), - Some(_) => 1usize, - None => 0usize, - }; - - if return_types != returns.len() { - return Err(FunctionError::return_arguments_length( - return_types, - returns.len(), - function.span.clone(), - )); - } - } - - Ok(return_values) + Self::conditionally_select_result(cs, function.output, results, &function.span) + .map_err(|err| FunctionError::StatementError(err)) } } diff --git a/compiler/src/function/result/result.rs b/compiler/src/function/result/result.rs index cec2a05810..4bfa4a8165 100644 --- a/compiler/src/function/result/result.rs +++ b/compiler/src/function/result/result.rs @@ -16,9 +16,15 @@ //! Enforces that one return value is produced in a compiled Leo program. -use crate::{errors::StatementError, program::ConstrainedProgram, value::ConstrainedValue, GroupType}; +use crate::{ + errors::StatementError, + get_indicator_value, + program::ConstrainedProgram, + value::ConstrainedValue, + GroupType, +}; -use leo_ast::Span; +use leo_ast::{Span, Type}; use snarkos_models::{ curves::{Field, PrimeField}, @@ -29,49 +35,84 @@ use snarkos_models::{ }; impl> ConstrainedProgram { - /// iterates through a vector of results and selects one based off of indicators + /// + /// Returns a conditionally selected result from the given possible function returns and + /// given function return type. + /// pub fn conditionally_select_result>( cs: &mut CS, - return_value: &mut ConstrainedValue, - results: Vec<(Option, ConstrainedValue)>, + expected_return: Option, + results: Vec<(Boolean, ConstrainedValue)>, span: &Span, - ) -> Result<(), StatementError> { - // if there are no results, continue - if results.is_empty() { - return Ok(()); + ) -> Result, StatementError> { + // Initialize empty return value. + let mut return_value = ConstrainedValue::Tuple(vec![]); + + // If the function does not expect a return type, then make sure there are no returned results. + let return_type = match expected_return { + Some(return_type) => return_type, + None => { + if results.is_empty() { + // If the function has no returns, then return an empty tuple. + return Ok(return_value); + } else { + return Err(StatementError::invalid_number_of_returns( + 0, + results.len(), + span.to_owned(), + )); + } + } + }; + + // Error if the function or one of its branches does not return. + if let None = results.iter().find(|(indicator, _res)| get_indicator_value(indicator)) { + return Err(StatementError::no_returns(return_type, span.to_owned())); } - // If all indicators are none, then there are no branch conditions in the function. - // We simply return the last result. - - if results.iter().all(|(indicator, _res)| indicator.is_none()) { - let result = &results[results.len() - 1].1; - - *return_value = result.clone(); - - return Ok(()); - } - - // If there are branches in the function we need to use the `ConditionalSelectGadget` to parse through and select the correct one. - // This can be thought of as de-multiplexing all previous wires that may have returned results into one. - for (i, (indicator, result)) in results.into_iter().enumerate() { - // Set the first value as the starting point - if i == 0 { - *return_value = result.clone(); + // Find the return value + let mut ignored = vec![]; + let mut found_return = false; + for (indicator, result) in results.into_iter() { + // Error if a statement returned a result with an incorrect type + let result_type = result.to_type(span)?; + if return_type != result_type { + return Err(StatementError::arguments_type( + &return_type, + &result_type, + span.to_owned(), + )); } - let condition = indicator.unwrap_or(Boolean::Constant(true)); - let selected_value = ConstrainedValue::conditionally_select( - cs.ns(|| format!("select {} {}:{}", result, span.line, span.start)), - &condition, - &result, - return_value, - ) - .map_err(|_| StatementError::select_fail(result.to_string(), return_value.to_string(), span.to_owned()))?; - - *return_value = selected_value; + if get_indicator_value(&indicator) { + // Error if we already have a return value. + if found_return { + return Err(StatementError::multiple_returns(span.to_owned())); + } else { + // Set the function return value. + return_value = result; + found_return = true; + } + } else { + // Ignore a possible function return value. + ignored.push((indicator, result)) + } } - Ok(()) + // Conditionally select out the ignored results in the circuit. + // + // If there are branches in the function we need to use the `ConditionalSelectGadget` to parse through and select the correct one. + // This can be thought of as de-multiplexing all previous wires that may have returned results into one. + for (i, (indicator, result)) in ignored.into_iter().enumerate() { + return_value = ConstrainedValue::conditionally_select( + cs.ns(|| format!("select result {} {}:{}", i, span.line, span.start)), + &indicator, + &result, + &return_value, + ) + .map_err(|_| StatementError::select_fail(result.to_string(), return_value.to_string(), span.to_owned()))?; + } + + Ok(return_value) } } diff --git a/compiler/src/statement/assign/array.rs b/compiler/src/statement/assign/array.rs index 842cdefbfe..0f56d9461b 100644 --- a/compiler/src/statement/assign/array.rs +++ b/compiler/src/statement/assign/array.rs @@ -34,14 +34,12 @@ impl> ConstrainedProgram { cs: &mut CS, file_scope: &str, function_scope: &str, - indicator: Option, + indicator: &Boolean, name: &str, range_or_expression: RangeOrExpression, mut new_value: ConstrainedValue, span: &Span, ) -> Result<(), StatementError> { - let condition = indicator.unwrap_or(Boolean::Constant(true)); - // Resolve index so we know if we are assigning to a single value or a range of values match range_or_expression { RangeOrExpression::Expression(index) => { @@ -54,7 +52,7 @@ impl> ConstrainedProgram { let selected_value = ConstrainedValue::conditionally_select( cs.ns(|| format!("select {} {}:{}", new_value, span.line, span.start)), - &condition, + indicator, &new_value, &old[index], ) @@ -90,7 +88,7 @@ impl> ConstrainedProgram { }; let selected_array = ConstrainedValue::conditionally_select( cs.ns(|| format!("select {} {}:{}", new_array, span.line, span.start)), - &condition, + indicator, &new_array, old_array, ) diff --git a/compiler/src/statement/assign/assign.rs b/compiler/src/statement/assign/assign.rs index 8899c9b286..25006a05e6 100644 --- a/compiler/src/statement/assign/assign.rs +++ b/compiler/src/statement/assign/assign.rs @@ -42,7 +42,7 @@ impl> ConstrainedProgram { file_scope: &str, function_scope: &str, declared_circuit_reference: &str, - indicator: Option, + indicator: &Boolean, assignee: Assignee, expression: Expression, span: &Span, @@ -55,14 +55,13 @@ impl> ConstrainedProgram { // Mutate the old value into the new value if assignee.accesses.is_empty() { - let condition = indicator.unwrap_or(Boolean::Constant(true)); let old_value = self.get_mutable_assignee(&variable_name, span)?; new_value.resolve_type(Some(old_value.to_type(&span)?), span)?; let selected_value = ConstrainedValue::conditionally_select( cs.ns(|| format!("select {} {}:{}", new_value, span.line, span.start)), - &condition, + indicator, &new_value, old_value, ) diff --git a/compiler/src/statement/assign/circuit_variable.rs b/compiler/src/statement/assign/circuit_variable.rs index 146c95f983..60d0ebb7f6 100644 --- a/compiler/src/statement/assign/circuit_variable.rs +++ b/compiler/src/statement/assign/circuit_variable.rs @@ -31,14 +31,12 @@ impl> ConstrainedProgram { pub fn mutate_circuit_variable>( &mut self, cs: &mut CS, - indicator: Option, + indicator: &Boolean, circuit_name: &str, variable_name: Identifier, mut new_value: ConstrainedValue, span: &Span, ) -> Result, StatementError> { - let condition = indicator.unwrap_or(Boolean::Constant(true)); - // Get the mutable circuit by name match self.get_mutable_assignee(circuit_name, span)? { ConstrainedValue::CircuitExpression(_variable, members) => { @@ -70,7 +68,7 @@ impl> ConstrainedProgram { // Conditionally select the value if this branch is executed. let mut selected_value = ConstrainedValue::conditionally_select( cs.ns(|| format!("select {} {}:{}", new_value, span.line, span.start)), - &condition, + indicator, &new_value, &member.1, ) diff --git a/compiler/src/statement/assign/tuple.rs b/compiler/src/statement/assign/tuple.rs index 5ecb0dd767..4209a1d570 100644 --- a/compiler/src/statement/assign/tuple.rs +++ b/compiler/src/statement/assign/tuple.rs @@ -31,15 +31,12 @@ impl> ConstrainedProgram { pub fn assign_tuple>( &mut self, cs: &mut CS, - indicator: Option, + indicator: &Boolean, name: &str, index: PositiveNumber, mut new_value: ConstrainedValue, span: &Span, ) -> Result<(), StatementError> { - // Get the indicator value. - let condition = indicator.unwrap_or(Boolean::Constant(true)); - // Parse the index. let index_usize = parse_index(&index, &span)?; @@ -50,7 +47,7 @@ impl> ConstrainedProgram { let selected_value = ConstrainedValue::conditionally_select( cs.ns(|| format!("select {} {}:{}", new_value, span.line, span.start)), - &condition, + indicator, &new_value, &old[index_usize], ) diff --git a/compiler/src/statement/branch/branch.rs b/compiler/src/statement/branch/branch.rs index 9d0d208036..f948c85a73 100644 --- a/compiler/src/statement/branch/branch.rs +++ b/compiler/src/statement/branch/branch.rs @@ -30,7 +30,7 @@ impl> ConstrainedProgram { cs: &mut CS, file_scope: &str, function_scope: &str, - indicator: Option, + indicator: &Boolean, statements: Vec, return_type: Option, ) -> StatementResult>> { diff --git a/compiler/src/statement/conditional/conditional.rs b/compiler/src/statement/conditional/conditional.rs index 48bad283b8..94e3a8726e 100644 --- a/compiler/src/statement/conditional/conditional.rs +++ b/compiler/src/statement/conditional/conditional.rs @@ -49,15 +49,15 @@ impl> ConstrainedProgram { cs: &mut CS, file_scope: &str, function_scope: &str, - indicator: Option, + indicator: &Boolean, statement: ConditionalStatement, return_type: Option, span: &Span, ) -> StatementResult>> { let statement_string = statement.to_string(); - // Inherit the indicator from a previous conditional statement or assume that we are the outer parent - let outer_indicator = indicator.unwrap_or(Boolean::Constant(true)); + // Inherit an indicator from a previous statement. + let outer_indicator = indicator; // Evaluate the conditional boolean as the inner indicator let inner_indicator = match self.enforce_expression( @@ -72,7 +72,7 @@ impl> ConstrainedProgram { }; // If outer_indicator && inner_indicator, then select branch 1 - let outer_indicator_string = indicator_to_string(&outer_indicator); + let outer_indicator_string = indicator_to_string(outer_indicator); let inner_indicator_string = indicator_to_string(&inner_indicator); let branch_1_name = format!( "branch indicator 1 {} && {}", @@ -80,7 +80,7 @@ impl> ConstrainedProgram { ); let branch_1_indicator = Boolean::and( &mut cs.ns(|| format!("branch 1 {} {}:{}", statement_string, span.line, span.start)), - &outer_indicator, + outer_indicator, &inner_indicator, ) .map_err(|_| StatementError::indicator_calculation(branch_1_name, span.to_owned()))?; @@ -92,7 +92,7 @@ impl> ConstrainedProgram { cs, file_scope, function_scope, - Some(branch_1_indicator), + &branch_1_indicator, statement.statements, return_type.clone(), )?; @@ -120,7 +120,7 @@ impl> ConstrainedProgram { cs, file_scope, function_scope, - Some(branch_2_indicator), + &branch_2_indicator, *nested, return_type, span, @@ -129,7 +129,7 @@ impl> ConstrainedProgram { cs, file_scope, function_scope, - Some(branch_2_indicator), + &branch_2_indicator, statements, return_type, )?, diff --git a/compiler/src/statement/iteration/iteration.rs b/compiler/src/statement/iteration/iteration.rs index 64104cbb45..509d4c22bd 100644 --- a/compiler/src/statement/iteration/iteration.rs +++ b/compiler/src/statement/iteration/iteration.rs @@ -42,7 +42,7 @@ impl> ConstrainedProgram { cs: &mut CS, file_scope: &str, function_scope: &str, - indicator: Option, + indicator: &Boolean, index: Identifier, start: Expression, stop: Expression, diff --git a/compiler/src/statement/statement.rs b/compiler/src/statement/statement.rs index 763ea328a7..16d9577e29 100644 --- a/compiler/src/statement/statement.rs +++ b/compiler/src/statement/statement.rs @@ -25,7 +25,7 @@ use snarkos_models::{ }; pub type StatementResult = Result; -pub type IndicatorAndConstrainedValue = (Option, ConstrainedValue); +pub type IndicatorAndConstrainedValue = (Boolean, ConstrainedValue); impl> ConstrainedProgram { /// @@ -41,7 +41,7 @@ impl> ConstrainedProgram { cs: &mut CS, file_scope: &str, function_scope: &str, - indicator: Option, + indicator: &Boolean, statement: Statement, return_type: Option, declared_circuit_reference: &str, @@ -51,7 +51,7 @@ impl> ConstrainedProgram { match statement { Statement::Return(expression, span) => { let return_value = ( - indicator, + indicator.to_owned(), self.enforce_return_statement(cs, file_scope, function_scope, expression, return_type, &span)?, ); @@ -126,7 +126,7 @@ impl> ConstrainedProgram { _ => return Err(StatementError::unassigned(expression_string, span)), } - let result = (indicator, value); + let result = (indicator.to_owned(), value); results.push(result); } @@ -135,3 +135,10 @@ impl> ConstrainedProgram { Ok(results) } } + +/// Returns the indicator boolean gadget value. +/// We can directly compare a boolean constant to the indicator since we are not enforcing any +/// constraints +pub fn get_indicator_value(indicator: &Boolean) -> bool { + indicator.eq(&Boolean::constant(true)) +} From dfcc77a860d4c8dd11280621962bd14769b0de3f Mon Sep 17 00:00:00 2001 From: collin Date: Thu, 3 Dec 2020 13:38:05 -0500 Subject: [PATCH 02/10] add test for bug #430 early returns --- compiler/src/statement/statement.rs | 8 ++------ compiler/tests/function/mod.rs | 12 ++++++++++-- .../function/{multiple.leo => multiple_returns.leo} | 0 compiler/tests/function/multiple_returns_fail.leo | 7 +++++++ .../{multiple_main.leo => multiple_returns_main.leo} | 0 5 files changed, 19 insertions(+), 8 deletions(-) rename compiler/tests/function/{multiple.leo => multiple_returns.leo} (100%) create mode 100644 compiler/tests/function/multiple_returns_fail.leo rename compiler/tests/function/{multiple_main.leo => multiple_returns_main.leo} (100%) diff --git a/compiler/src/statement/statement.rs b/compiler/src/statement/statement.rs index 16d9577e29..854d46c7cc 100644 --- a/compiler/src/statement/statement.rs +++ b/compiler/src/statement/statement.rs @@ -116,19 +116,15 @@ impl> ConstrainedProgram { let expression_string = expression.to_string(); let value = self.enforce_expression(cs, file_scope, function_scope, None, expression)?; - // handle empty return value cases + // Handle empty return value cases. match &value { ConstrainedValue::Tuple(values) => { if !values.is_empty() { - return Err(StatementError::unassigned(expression_string, span)); + results.push((indicator.clone(), value)); } } _ => return Err(StatementError::unassigned(expression_string, span)), } - - let result = (indicator.to_owned(), value); - - results.push(result); } }; diff --git a/compiler/tests/function/mod.rs b/compiler/tests/function/mod.rs index dc31094f76..9a2306b17e 100644 --- a/compiler/tests/function/mod.rs +++ b/compiler/tests/function/mod.rs @@ -64,15 +64,23 @@ fn test_newlines() { #[test] fn test_multiple_returns() { - let bytes = include_bytes!("multiple.leo"); + let bytes = include_bytes!("multiple_returns.leo"); let program = parse_program(bytes).unwrap(); assert_satisfied(program); } +#[test] +fn test_multiple_returns_fail() { + let bytes = include_bytes!("multiple_returns_fail.leo"); + let program = parse_program(bytes).unwrap(); + + expect_compiler_error(program); +} + #[test] fn test_multiple_returns_main() { - let program_bytes = include_bytes!("multiple_main.leo"); + let program_bytes = include_bytes!("multiple_returns_main.leo"); let input_bytes = include_bytes!("input/registers.in"); let program = parse_program_with_input(program_bytes, input_bytes).unwrap(); diff --git a/compiler/tests/function/multiple.leo b/compiler/tests/function/multiple_returns.leo similarity index 100% rename from compiler/tests/function/multiple.leo rename to compiler/tests/function/multiple_returns.leo diff --git a/compiler/tests/function/multiple_returns_fail.leo b/compiler/tests/function/multiple_returns_fail.leo new file mode 100644 index 0000000000..d4a8b36eac --- /dev/null +++ b/compiler/tests/function/multiple_returns_fail.leo @@ -0,0 +1,7 @@ +function main () -> i8 { + if true { + return 1i8 //ignored + } + return 2i8 //ignored + return 3i8 //returns +} \ No newline at end of file diff --git a/compiler/tests/function/multiple_main.leo b/compiler/tests/function/multiple_returns_main.leo similarity index 100% rename from compiler/tests/function/multiple_main.leo rename to compiler/tests/function/multiple_returns_main.leo From 87c0dd738a8331e1b588f2a8c197b0a67c84e7c1 Mon Sep 17 00:00:00 2001 From: collin Date: Thu, 3 Dec 2020 13:44:47 -0500 Subject: [PATCH 03/10] add test for bug #427 incomplete conditional --- compiler/tests/function/mod.rs | 8 ++++++++ .../tests/function/multiple_returns_fail_conditional.leo | 9 +++++++++ 2 files changed, 17 insertions(+) create mode 100644 compiler/tests/function/multiple_returns_fail_conditional.leo diff --git a/compiler/tests/function/mod.rs b/compiler/tests/function/mod.rs index 9a2306b17e..90346098e9 100644 --- a/compiler/tests/function/mod.rs +++ b/compiler/tests/function/mod.rs @@ -78,6 +78,14 @@ fn test_multiple_returns_fail() { expect_compiler_error(program); } +#[test] +fn test_multiple_returns_fail_conditional() { + let bytes = include_bytes!("multiple_returns_fail_conditional.leo"); + let program = parse_program(bytes).unwrap(); + + expect_compiler_error(program); +} + #[test] fn test_multiple_returns_main() { let program_bytes = include_bytes!("multiple_returns_main.leo"); diff --git a/compiler/tests/function/multiple_returns_fail_conditional.leo b/compiler/tests/function/multiple_returns_fail_conditional.leo new file mode 100644 index 0000000000..04ebb9e306 --- /dev/null +++ b/compiler/tests/function/multiple_returns_fail_conditional.leo @@ -0,0 +1,9 @@ +function main () -> u16 { + if false { + let a = 1u16; + let b = a + 1u16; + return b + } else if false { + return 0u16 + } +} \ No newline at end of file From cd53cba77a5b06bc6fa5827053193387a1280392 Mon Sep 17 00:00:00 2001 From: collin Date: Fri, 4 Dec 2020 17:56:59 -0500 Subject: [PATCH 04/10] fix circuit selftype error bug --- compiler/src/function/result/result.rs | 9 ++------ compiler/src/statement/return_/return_.rs | 25 +++++++++++------------ 2 files changed, 14 insertions(+), 20 deletions(-) diff --git a/compiler/src/function/result/result.rs b/compiler/src/function/result/result.rs index 4bfa4a8165..f8fae3bb17 100644 --- a/compiler/src/function/result/result.rs +++ b/compiler/src/function/result/result.rs @@ -17,6 +17,7 @@ //! Enforces that one return value is produced in a compiled Leo program. use crate::{ + check_return_type, errors::StatementError, get_indicator_value, program::ConstrainedProgram, @@ -76,13 +77,7 @@ impl> ConstrainedProgram { for (indicator, result) in results.into_iter() { // Error if a statement returned a result with an incorrect type let result_type = result.to_type(span)?; - if return_type != result_type { - return Err(StatementError::arguments_type( - &return_type, - &result_type, - span.to_owned(), - )); - } + check_return_type(&return_type, &result_type, span)?; if get_indicator_value(&indicator) { // Error if we already have a return value. diff --git a/compiler/src/statement/return_/return_.rs b/compiler/src/statement/return_/return_.rs index f70b69ca79..55d548dbcf 100644 --- a/compiler/src/statement/return_/return_.rs +++ b/compiler/src/statement/return_/return_.rs @@ -24,20 +24,17 @@ use snarkos_models::{ gadgets::r1cs::ConstraintSystem, }; -fn check_return_type(expected: Option, actual: Type, span: &Span) -> Result<(), StatementError> { - match expected { - Some(expected) => { - if expected.ne(&actual) { - if (expected.is_self() && actual.is_circuit()) || expected.eq_flat(&actual) { - return Ok(()); - } else { - return Err(StatementError::arguments_type(&expected, &actual, span.to_owned())); - } - } +/// Returns `Ok` if the expected type == actual type, returns `Err` otherwise. +pub fn check_return_type(expected: &Type, actual: &Type, span: &Span) -> Result<(), StatementError> { + if expected.ne(&actual) { + // If the return type is `SelfType` returning the circuit type is okay. + return if (expected.is_self() && actual.is_circuit()) || expected.eq_flat(&actual) { Ok(()) - } - None => Ok(()), + } else { + Err(StatementError::arguments_type(&expected, &actual, span.to_owned())) + }; } + Ok(()) } impl> ConstrainedProgram { @@ -53,7 +50,9 @@ impl> ConstrainedProgram { let result = self.enforce_operand(cs, file_scope, function_scope, return_type.clone(), expression, span)?; // Make sure we return the correct type. - check_return_type(return_type, result.to_type(&span)?, span)?; + if let Some(expected) = return_type { + check_return_type(&expected, &result.to_type(span)?, span)?; + } Ok(result) } From 7745710dc40b55c516b14027938e427b3c76aa5d Mon Sep 17 00:00:00 2001 From: collin Date: Fri, 4 Dec 2020 19:06:29 -0500 Subject: [PATCH 05/10] cargo +nightly clippy --- compiler/src/errors/statement.rs | 4 ++-- compiler/src/function/function.rs | 2 +- compiler/src/function/result/result.rs | 6 +++++- compiler/src/statement/statement.rs | 2 +- 4 files changed, 9 insertions(+), 5 deletions(-) diff --git a/compiler/src/errors/statement.rs b/compiler/src/errors/statement.rs index 4fd0dc7d5e..b1e7cf908d 100644 --- a/compiler/src/errors/statement.rs +++ b/compiler/src/errors/statement.rs @@ -136,8 +136,8 @@ impl StatementError { } pub fn multiple_returns(span: Span) -> Self { - let message = - format!("This function returns multiple times and produces unreachable circuits with undefined behavior."); + let message = "This function returns multiple times and produces unreachable circuits with undefined behavior." + .to_string(); Self::new_from_span(message, span) } diff --git a/compiler/src/function/function.rs b/compiler/src/function/function.rs index 53ac0a82ba..d1f1d96f1d 100644 --- a/compiler/src/function/function.rs +++ b/compiler/src/function/function.rs @@ -111,6 +111,6 @@ impl> ConstrainedProgram { // Conditionally select a result based on returned indicators Self::conditionally_select_result(cs, function.output, results, &function.span) - .map_err(|err| FunctionError::StatementError(err)) + .map_err(FunctionError::StatementError) } } diff --git a/compiler/src/function/result/result.rs b/compiler/src/function/result/result.rs index f8fae3bb17..1971dd7c4e 100644 --- a/compiler/src/function/result/result.rs +++ b/compiler/src/function/result/result.rs @@ -67,7 +67,11 @@ impl> ConstrainedProgram { }; // Error if the function or one of its branches does not return. - if let None = results.iter().find(|(indicator, _res)| get_indicator_value(indicator)) { + if results + .iter() + .find(|(indicator, _res)| get_indicator_value(indicator)) + .is_none() + { return Err(StatementError::no_returns(return_type, span.to_owned())); } diff --git a/compiler/src/statement/statement.rs b/compiler/src/statement/statement.rs index 61ad80c87a..6e96c88c9a 100644 --- a/compiler/src/statement/statement.rs +++ b/compiler/src/statement/statement.rs @@ -124,7 +124,7 @@ impl> ConstrainedProgram { match &value { ConstrainedValue::Tuple(values) => { if !values.is_empty() { - results.push((indicator.clone(), value)); + results.push((*indicator, value)); } } _ => return Err(StatementError::unassigned(expression_string, span)), From 3b23eb595a1b4720f43575af5b37cfe0344651e8 Mon Sep 17 00:00:00 2001 From: collin Date: Mon, 7 Dec 2020 11:22:45 -0500 Subject: [PATCH 06/10] return iterator instead of vector --- ast/src/functions/function.rs | 9 +++------ compiler/src/function/function.rs | 2 +- 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/ast/src/functions/function.rs b/ast/src/functions/function.rs index c55daf03d3..efc3a6405e 100644 --- a/ast/src/functions/function.rs +++ b/ast/src/functions/function.rs @@ -77,13 +77,10 @@ impl Function { } /// - /// Returns a vector of [&FunctionInput] removing `self` and `mut self` inputs. + /// Returns an iterator of [&FunctionInput] removing `self` and `mut self` inputs. /// - pub fn filter_self_inputs(&self) -> Vec<&FunctionInput> { - self.input - .iter() - .filter(|input| !input.is_self()) - .collect::>() + pub fn filter_self_inputs(&self) -> impl Iterator { + self.input.iter().filter(|input| !input.is_self()) } fn format(&self, f: &mut fmt::Formatter) -> fmt::Result { diff --git a/compiler/src/function/function.rs b/compiler/src/function/function.rs index d1f1d96f1d..4d58fc4a05 100644 --- a/compiler/src/function/function.rs +++ b/compiler/src/function/function.rs @@ -46,7 +46,7 @@ impl> ConstrainedProgram { let mut_self = function.contains_mut_self(); // Store input values as new variables in resolved program - for (input_model, input_expression) in function.filter_self_inputs().iter().zip(input.into_iter()) { + for (input_model, input_expression) in function.filter_self_inputs().zip(input.into_iter()) { let (name, value) = match input_model { FunctionInput::InputKeyword(keyword) => { let value = From ed4e0943770bc6e27b73e45c379775d3b77c5d34 Mon Sep 17 00:00:00 2001 From: collin Date: Mon, 7 Dec 2020 11:33:45 -0500 Subject: [PATCH 07/10] symbol table optimizations --- symbol-table/src/types/functions/function.rs | 13 ++++++------- symbol-table/src/types/functions/function_input.rs | 9 --------- type-inference/src/objects/frame.rs | 4 +--- 3 files changed, 7 insertions(+), 19 deletions(-) diff --git a/symbol-table/src/types/functions/function.rs b/symbol-table/src/types/functions/function.rs index ffc35fde98..3c84176fdc 100644 --- a/symbol-table/src/types/functions/function.rs +++ b/symbol-table/src/types/functions/function.rs @@ -127,7 +127,9 @@ impl FunctionType { pub fn num_inputs(&self) -> usize { self.inputs .iter() - .fold(0, |acc, function_input| acc + function_input.count()) + .filter(|function_input| !function_input.is_self()) + .count() + // .fold(0, |acc, function_input| acc + function_input.arguments()) } /// @@ -139,13 +141,10 @@ impl FunctionType { } /// - /// Returns a vector of [&FunctionInputType] removing `self` and `mut self` inputs. + /// Returns an iterator of [&FunctionInputType] removing `self` and `mut self` inputs. /// - pub fn filter_self_inputs(&self) -> Vec<&FunctionInputType> { - self.inputs - .iter() - .filter(|input| !input.is_self()) - .collect::>() + pub fn filter_self_inputs(&self) -> impl Iterator { + self.inputs.iter().filter(|input| !input.is_self()) } } diff --git a/symbol-table/src/types/functions/function_input.rs b/symbol-table/src/types/functions/function_input.rs index 66a8180f98..cee5ed1df5 100644 --- a/symbol-table/src/types/functions/function_input.rs +++ b/symbol-table/src/types/functions/function_input.rs @@ -77,15 +77,6 @@ impl FunctionInputType { } } - /// - /// Returns `0` if the function input is a `self` or `mut self` keyword which does not have to - /// provided in a call to the function. - /// Returns `1` if a variable must be provided in a call to the function. - /// - pub fn count(&self) -> usize { - if self.is_self() { 0 } else { 1 } - } - /// /// Return a new `FunctionInputType` from a given `FunctionInput`. /// diff --git a/type-inference/src/objects/frame.rs b/type-inference/src/objects/frame.rs index e725bff39b..20b1c44612 100644 --- a/type-inference/src/objects/frame.rs +++ b/type-inference/src/objects/frame.rs @@ -1120,10 +1120,8 @@ impl Frame { } // Filter out `self` and `mut self` keywords. - let expected_inputs = function_type.filter_self_inputs(); - // Assert function inputs are correct types. - for (expected_input, actual_input) in expected_inputs.iter().zip(inputs) { + for (expected_input, actual_input) in function_type.filter_self_inputs().zip(inputs) { // Parse expected input type. let expected_type = expected_input.type_(); From 51049857434e52d2d52f0bec246d465607389fd7 Mon Sep 17 00:00:00 2001 From: collin Date: Mon, 7 Dec 2020 11:34:45 -0500 Subject: [PATCH 08/10] remove unused check --- type-inference/src/objects/frame.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/type-inference/src/objects/frame.rs b/type-inference/src/objects/frame.rs index 20b1c44612..52e97b803c 100644 --- a/type-inference/src/objects/frame.rs +++ b/type-inference/src/objects/frame.rs @@ -1092,8 +1092,6 @@ impl Frame { return Err(FrameError::static_call_invalid(&identifier)); } - if is_static && function_type.contains_self() {} - // Return the function type. Ok(function_type.to_owned()) } From 64f2a6586ee8364853e1f1ac23fc7b140035d152 Mon Sep 17 00:00:00 2001 From: collin Date: Mon, 7 Dec 2020 15:49:06 -0500 Subject: [PATCH 09/10] use filter_self_inputs in num_inputs method --- symbol-table/src/types/functions/function.rs | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/symbol-table/src/types/functions/function.rs b/symbol-table/src/types/functions/function.rs index 3c84176fdc..05fc6c43fb 100644 --- a/symbol-table/src/types/functions/function.rs +++ b/symbol-table/src/types/functions/function.rs @@ -120,18 +120,6 @@ impl FunctionType { Ok(()) } - /// - /// Returns the number of input variables to the function. - /// The `self` and `mut self` keywords are not counted as input variables. - /// - pub fn num_inputs(&self) -> usize { - self.inputs - .iter() - .filter(|function_input| !function_input.is_self()) - .count() - // .fold(0, |acc, function_input| acc + function_input.arguments()) - } - /// /// Returns `true` if the input `self` or `mut self` is present. /// Returns `false` otherwise. @@ -146,6 +134,14 @@ impl FunctionType { pub fn filter_self_inputs(&self) -> impl Iterator { self.inputs.iter().filter(|input| !input.is_self()) } + + /// + /// Returns the number of input variables to the function. + /// The `self` and `mut self` keywords are not counted as input variables. + /// + pub fn num_inputs(&self) -> usize { + self.filter_self_inputs().count() + } } impl PartialEq for FunctionType { From 2ef3a820a7aa3fd7fd121a2aa39ee81b0550e4cd Mon Sep 17 00:00:00 2001 From: collin Date: Mon, 7 Dec 2020 15:55:46 -0500 Subject: [PATCH 10/10] cargo +nightly clippy --- state/src/utilities/input_value.rs | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/state/src/utilities/input_value.rs b/state/src/utilities/input_value.rs index 63da26329d..70dff7d22f 100644 --- a/state/src/utilities/input_value.rs +++ b/state/src/utilities/input_value.rs @@ -30,11 +30,8 @@ pub fn find_input( .find(|(parameter, _value)| parameter.variable.name == name); match matched_parameter { - Some((_parameter, value_option)) => match value_option { - Some(value) => Ok(value.clone()), - None => Err(InputValueError::MissingParameter(name)), - }, - None => Err(InputValueError::MissingParameter(name)), + Some((_parameter, Some(value))) => Ok(value.clone()), + _ => Err(InputValueError::MissingParameter(name)), } }