diff --git a/compiler/noirc_frontend/src/elaborator/expressions.rs b/compiler/noirc_frontend/src/elaborator/expressions.rs index 1799e3257ad..adac10f548e 100644 --- a/compiler/noirc_frontend/src/elaborator/expressions.rs +++ b/compiler/noirc_frontend/src/elaborator/expressions.rs @@ -276,32 +276,35 @@ impl Elaborator<'_> { unsafe_expression: UnsafeExpression, target_type: Option<&Type>, ) -> (HirExpression, Type) { - // Before entering the block we cache the old value of `in_unsafe_block` so it can be restored. + use UnsafeBlockStatus::*; + // Before entering the block we cache the old value of the unsafe block status, so it can be restored. let old_in_unsafe_block = self.unsafe_block_status; - let is_nested_unsafe_block = - !matches!(old_in_unsafe_block, UnsafeBlockStatus::NotInUnsafeBlock); + let is_nested_unsafe_block = !matches!(old_in_unsafe_block, NotInUnsafeBlock); + if is_nested_unsafe_block { self.push_err(TypeCheckError::NestedUnsafeBlock { location: unsafe_expression.unsafe_keyword_location, }); } - self.unsafe_block_status = UnsafeBlockStatus::InUnsafeBlockWithoutUnconstrainedCalls; + self.unsafe_block_status = InUnsafeBlockWithoutUnconstrainedCalls; let (hir_block_expression, typ) = self.elaborate_block_expression(unsafe_expression.block, target_type); - if let UnsafeBlockStatus::InUnsafeBlockWithoutUnconstrainedCalls = self.unsafe_block_status - { + let has_unconstrained_call = + matches!(self.unsafe_block_status, InUnsafeBlockWithUnconstrainedCalls); + + if !has_unconstrained_call { self.push_err(TypeCheckError::UnnecessaryUnsafeBlock { location: unsafe_expression.unsafe_keyword_location, }); } - // Finally, we restore the original value of `self.in_unsafe_block`, - // but only if this isn't a nested unsafe block (that way if we found an unconstrained call - // for this unsafe block we'll also consider the outer one as finding one, and we don't double error) - if !is_nested_unsafe_block { + // Finally, we restore the original value of the unsafe block status, + // unless we are in a nested block and we have found an unconstrained call, + // in which case we should consider the outer block as having that call as well. + if !is_nested_unsafe_block || !has_unconstrained_call { self.unsafe_block_status = old_in_unsafe_block; } @@ -1309,8 +1312,8 @@ impl Elaborator<'_> { let mut element_ids = Vec::with_capacity(tuple.len()); let mut element_types = Vec::with_capacity(tuple.len()); + let target_type = target_type.map(|typ| typ.follow_bindings()); for (index, element) in tuple.into_iter().enumerate() { - let target_type = target_type.map(|typ| typ.follow_bindings()); let expr_target_type = if let Some(Type::Tuple(types)) = &target_type { types.get(index) } else { None }; let (id, typ) = self.elaborate_expression_with_target_type(element, expr_target_type); @@ -1329,10 +1332,10 @@ impl Elaborator<'_> { let target_type = target_type.map(|typ| typ.follow_bindings()); if let Some(Type::Function(args, _, _, _)) = target_type { - return self.elaborate_lambda_with_parameter_type_hints(lambda, Some(&args)); + self.elaborate_lambda_with_parameter_type_hints(lambda, Some(&args)) + } else { + self.elaborate_lambda_with_parameter_type_hints(lambda, None) } - - self.elaborate_lambda_with_parameter_type_hints(lambda, None) } /// For elaborating a lambda we might get `parameters_type_hints`. These come from a potential diff --git a/compiler/noirc_frontend/src/elaborator/mod.rs b/compiler/noirc_frontend/src/elaborator/mod.rs index 680c22300e5..c55eb3cb80f 100644 --- a/compiler/noirc_frontend/src/elaborator/mod.rs +++ b/compiler/noirc_frontend/src/elaborator/mod.rs @@ -147,7 +147,7 @@ pub struct LambdaContext { enum UnsafeBlockStatus { NotInUnsafeBlock, InUnsafeBlockWithoutUnconstrainedCalls, - InUnsafeBlockWithConstrainedCalls, + InUnsafeBlockWithUnconstrainedCalls, } pub struct Loop { diff --git a/compiler/noirc_frontend/src/elaborator/types.rs b/compiler/noirc_frontend/src/elaborator/types.rs index ab7c9b9d7b3..04359f2b076 100644 --- a/compiler/noirc_frontend/src/elaborator/types.rs +++ b/compiler/noirc_frontend/src/elaborator/types.rs @@ -1528,10 +1528,10 @@ impl Elaborator<'_> { } } - // Given a unary operator and a type, this method will produce the output type - // and a boolean indicating whether to use the trait impl corresponding to the operator - // or not. A value of false indicates the caller to use a primitive operation for this - // operator, while a true value indicates a user-provided trait impl is required. + /// Given a unary operator and a type, this method will produce the output type + /// and a boolean indicating whether to use the trait impl corresponding to the operator + /// or not. A value of false indicates to the caller to use a primitive operation for this + /// operator, while a true value indicates a user-provided trait impl is required. pub(super) fn prefix_operand_type_rules( &mut self, op: &UnaryOp, @@ -2078,9 +2078,10 @@ impl Elaborator<'_> { self.push_err(TypeCheckError::Unsafe { location }); } UnsafeBlockStatus::InUnsafeBlockWithoutUnconstrainedCalls => { - self.unsafe_block_status = UnsafeBlockStatus::InUnsafeBlockWithConstrainedCalls; + self.unsafe_block_status = + UnsafeBlockStatus::InUnsafeBlockWithUnconstrainedCalls; } - UnsafeBlockStatus::InUnsafeBlockWithConstrainedCalls => (), + UnsafeBlockStatus::InUnsafeBlockWithUnconstrainedCalls => (), } if let Some(called_func_id) = self.interner.lookup_function_from_expr(&call.func) { diff --git a/compiler/noirc_frontend/src/hir/type_check/errors.rs b/compiler/noirc_frontend/src/hir/type_check/errors.rs index 9098ed5b4ab..df636bee8a2 100644 --- a/compiler/noirc_frontend/src/hir/type_check/errors.rs +++ b/compiler/noirc_frontend/src/hir/type_check/errors.rs @@ -707,35 +707,35 @@ impl<'a> From<&'a TypeCheckError> for Diagnostic { } TypeCheckError::CannotInvokeStructFieldFunctionType { method_name, object_type, location } => { Diagnostic::simple_error( - format!("Cannot invoke function field '{method_name}' on type '{object_type}' as a method"), + format!("Cannot invoke function field '{method_name}' on type '{object_type}' as a method"), format!("to call the function stored in '{method_name}', surround the field access with parentheses: '(', ')'"), *location, ) }, TypeCheckError::TypeAnnotationsNeededForIndex { location } => { Diagnostic::simple_error( - "Type annotations required before indexing this array or slice".into(), + "Type annotations required before indexing this array or slice".into(), "Type annotations needed before this point, can't decide if this is an array or slice".into(), *location, ) }, TypeCheckError::UnnecessaryUnsafeBlock { location } => { Diagnostic::simple_warning( - "Unnecessary `unsafe` block".into(), + "Unnecessary `unsafe` block".into(), "".into(), *location, ) }, TypeCheckError::NestedUnsafeBlock { location } => { Diagnostic::simple_warning( - "Unnecessary `unsafe` block".into(), + "Unnecessary `unsafe` block".into(), "Because it's nested inside another `unsafe` block".into(), *location, ) }, TypeCheckError::UnreachableCase { location } => { Diagnostic::simple_warning( - "Unreachable match case".into(), + "Unreachable match case".into(), "This pattern is redundant with one or more prior patterns".into(), *location, ) diff --git a/compiler/noirc_frontend/src/tests.rs b/compiler/noirc_frontend/src/tests.rs index ed7aa89bbad..18d2f49a81f 100644 --- a/compiler/noirc_frontend/src/tests.rs +++ b/compiler/noirc_frontend/src/tests.rs @@ -28,7 +28,7 @@ mod visibility; // XXX: These tests repeat a lot of code // what we should do is have test cases which are passed to a test harness // A test harness will allow for more expressive and readable tests -use std::collections::HashMap; +use std::collections::{HashMap, HashSet}; use crate::elaborator::{FrontendOptions, UnstableFeature}; use crate::hir::printer::display_crate; @@ -80,7 +80,7 @@ fn assert_no_errors_and_to_string(src: &str) -> String { /// /// fn main() -> pub i32 { /// ^^^ expected i32 because of return type -/// true +/// true /// ~~~~ bool returned here /// } /// @@ -167,11 +167,24 @@ fn check_errors_with_options( byte += line.len() + 1; // For '\n' last_line_length = line.len(); } - let mut primary_spans_with_errors: HashMap = - primary_spans_with_errors.into_iter().collect(); - let mut secondary_spans_with_errors: HashMap = - secondary_spans_with_errors.into_iter().collect(); + // We might see the same primary message multiple time with different secondary. + fn to_message_map(messages: Vec<(Span, String)>) -> HashMap> { + let mut map = HashMap::<_, Vec>::new(); + for (span, msg) in messages { + map.entry(span).or_default().push(msg); + } + map + } + + // By the end we want to have seen all errors. + let mut all_primaries: HashSet<(Span, String)> = + HashSet::from_iter(primary_spans_with_errors.clone()); + let mut all_secondaries: HashSet<(Span, String)> = + HashSet::from_iter(secondary_spans_with_errors.clone()); + + let primary_spans_with_errors = to_message_map(primary_spans_with_errors); + let secondary_spans_with_errors = to_message_map(secondary_spans_with_errors); let src = code_lines.join("\n"); let (_, mut context, errors) = get_program_with_options(&src, allow_parser_errors, options); @@ -210,68 +223,72 @@ fn check_errors_with_options( .secondaries .first() .unwrap_or_else(|| panic!("Expected {error:?} to have a secondary label")); + + let primary_message = &error.message; let span = secondary.location.span; - let message = &error.message; - let Some(expected_message) = primary_spans_with_errors.remove(&span) else { - if let Some(message) = secondary_spans_with_errors.get(&span) { + + let Some(expected_primaries) = primary_spans_with_errors.get(&span) else { + if let Some(secondaries) = secondary_spans_with_errors.get(&span) { report_all(context.file_manager.as_file_map(), &errors, false, false); panic!( - "Error at {span:?} with message {message:?} is annotated as secondary but should be primary" + "Error at {span:?} with message(s) {secondaries:?} is annotated as secondary but should be primary: {primary_message:?}" ); } else { report_all(context.file_manager.as_file_map(), &errors, false, false); panic!( - "Couldn't find primary error at {span:?} with message {message:?}.\nAll errors: {errors:?}" + "Couldn't find primary error at {span:?} with message {primary_message:?}.\nAll errors: {errors:?}" ); } }; - if message != &expected_message { + if !expected_primaries.contains(primary_message) { report_all(context.file_manager.as_file_map(), &errors, false, false); - assert_eq!( - message, &expected_message, - "Primary error at {span:?} has unexpected message" + panic!( + "Primary error at {span:?} has unexpected message: {primary_message:?}; should be one of {expected_primaries:?}" ); + } else { + all_primaries.remove(&(span, primary_message.clone())); } for secondary in &error.secondaries { - let message = &secondary.message; - if message.is_empty() { + let secondary_message = &secondary.message; + if secondary_message.is_empty() { continue; } let span = secondary.location.span; - let Some(expected_message) = secondary_spans_with_errors.remove(&span) else { + let Some(expected_secondaries) = secondary_spans_with_errors.get(&span) else { report_all(context.file_manager.as_file_map(), &errors, false, false); - if let Some(message) = primary_spans_with_errors.get(&span) { + if let Some(primaries) = primary_spans_with_errors.get(&span) { panic!( - "Error at {span:?} with message {message:?} is annotated as primary but should be secondary" + "Error at {span:?} with message(s) {primaries:?} is annotated as primary but should be secondary: {secondary_message:?}" ); } else { panic!( - "Couldn't find secondary error at {span:?} with message {message:?}.\nAll errors: {errors:?}" + "Couldn't find secondary error at {span:?} with message {secondary_message:?}.\nAll errors: {errors:?}" ); }; }; - if message != &expected_message { + if !expected_secondaries.contains(secondary_message) { report_all(context.file_manager.as_file_map(), &errors, false, false); - assert_eq!( - message, &expected_message, - "Secondary error at {span:?} has unexpected message" + panic!( + "Secondary error at {span:?} has unexpected message: {secondary_message:?}; should be one of {expected_secondaries:?}" ); + } else { + all_secondaries.remove(&(span, secondary_message.clone())); } } } - if !primary_spans_with_errors.is_empty() { + if !all_primaries.is_empty() { report_all(context.file_manager.as_file_map(), &errors, false, false); - panic!("These primary errors didn't happen: {primary_spans_with_errors:?}"); + panic!("These primary errors didn't happen: {all_primaries:?}"); } - if !secondary_spans_with_errors.is_empty() { + if !all_secondaries.is_empty() { report_all(context.file_manager.as_file_map(), &errors, false, false); - panic!("These secondary errors didn't happen: {secondary_spans_with_errors:?}"); + panic!("These secondary errors didn't happen: {all_secondaries:?}"); } } diff --git a/compiler/noirc_frontend/src/tests/runtime.rs b/compiler/noirc_frontend/src/tests/runtime.rs index f402b164111..39d26b2c207 100644 --- a/compiler/noirc_frontend/src/tests/runtime.rs +++ b/compiler/noirc_frontend/src/tests/runtime.rs @@ -226,6 +226,29 @@ fn warns_on_nested_unsafe() { check_errors(src); } +#[test] +fn no_warns_on_needed_unsafe_with_unneeded_nested() { + let src = r#" + fn main() { + // Safety: test + unsafe { + foo(); + // Safety: test + unsafe { + ^^^^^^ Unnecessary `unsafe` block + ~~~~~~ Because it's nested inside another `unsafe` block + bar(); + } + } + } + + unconstrained fn foo() {} + + fn bar() {} + "#; + check_errors(src); +} + #[test] fn deny_inline_attribute_on_unconstrained() { let src = r#" @@ -292,7 +315,7 @@ fn disallows_test_attribute_on_impl_method() { pub struct Foo { } impl Foo { - + #[test] fn foo() { } ^^^ The `#[test]` attribute is disallowed on `impl` methods