fix(elaborator): Keep the status of the outer unsafe block if the inner is unnecessary

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

compiler/noirc_frontend/src/elaborator/mod.rs

@@ -147,7 +147,7 @@ pub struct LambdaContext {
 enum UnsafeBlockStatus {
     NotInUnsafeBlock,
     InUnsafeBlockWithoutUnconstrainedCalls,
-    InUnsafeBlockWithConstrainedCalls,
+    InUnsafeBlockWithUnconstrainedCalls,
 }
 
 pub struct Loop {

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) {

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

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<Span, String> =
-        primary_spans_with_errors.into_iter().collect();
 
-    let mut secondary_spans_with_errors: HashMap<Span, String> =
-        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<Span, Vec<String>> {
+        let mut map = HashMap::<_, Vec<String>>::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:?}");
     }
 }
 

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