chore: require safety doc comment for unsafe instead of //@safety

compiler/noirc_frontend/src/debug/mod.rs

@@ -308,7 +308,7 @@
             }
             ast::LValue::Dereference(_lv, span) => {
                 // TODO: this is a dummy statement for now, but we should
                 // somehow track the derefence and update the pointed to
                 // variable
                 ast::Statement {
                     kind: ast::StatementKind::Expression(uint_expr(0, *span)),
@@ -523,8 +523,8 @@
                 __debug_var_assign_oracle(var_id, value);
             }
             pub fn __debug_var_assign<T>(var_id: u32, value: T) {
+                /// Safety: debug context
                 unsafe {
-                //@safety: debug context
                 {
                     __debug_var_assign_inner(var_id, value);
                 }}
@@ -536,8 +536,8 @@
                 __debug_var_drop_oracle(var_id);
             }
             pub fn __debug_var_drop(var_id: u32) {
+                /// Safety: debug context
                 unsafe {
-                //@safety: debug context
                 {
                     __debug_var_drop_inner(var_id);
                 }}
@@ -549,8 +549,8 @@
                 __debug_fn_enter_oracle(fn_id);
             }
             pub fn __debug_fn_enter(fn_id: u32) {
+                /// Safety: debug context
                 unsafe {
-                //@safety: debug context
                 {
                     __debug_fn_enter_inner(fn_id);
                 }}
@@ -562,8 +562,8 @@
                 __debug_fn_exit_oracle(fn_id);
             }
             pub fn __debug_fn_exit(fn_id: u32) {
+                /// Safety: debug context
                 unsafe {
-                //@safety: debug context
                 {
                     __debug_fn_exit_inner(fn_id);
                 }}
@@ -575,8 +575,8 @@
                 __debug_dereference_assign_oracle(var_id, value);
             }
             pub fn __debug_dereference_assign<T>(var_id: u32, value: T) {
+                /// Safety: debug context
                 unsafe {
-                //@safety: debug context
                 {
                     __debug_dereference_assign_inner(var_id, value);
                 }}
@@ -603,8 +603,8 @@
                     __debug_oracle_member_assign_{n}(var_id, value, {vars});
                 }}
                 pub fn __debug_member_assign_{n}<T, Index>(var_id: u32, value: T, {var_sig}) {{
+                    /// Safety: debug context
                     unsafe {{
-                        //@safety: debug context
                         __debug_inner_member_assign_{n}(var_id, value, {vars});
                     }}
                 }}
@@ -715,9 +715,9 @@
             ast::Pattern::Tuple(patterns, _) => {
                 stack.extend(patterns.iter().map(|pattern| (pattern, false)));
             }
             ast::Pattern::Struct(_, pids, _) => {
                 stack.extend(pids.iter().map(|(_, pattern)| (pattern, is_mut)));
                 vars.extend(pids.iter().map(|(id, _)| (id.clone(), false)));
             }
             ast::Pattern::Interned(_, _) => (),
         }
@@ -728,7 +728,7 @@
 fn pattern_to_string(pattern: &ast::Pattern) -> String {
     match pattern {
         ast::Pattern::Identifier(id) => id.0.contents.clone(),
         ast::Pattern::Mutable(mpat, _, _) => format!("mut {}", pattern_to_string(mpat.as_ref())),
         ast::Pattern::Tuple(elements, _) => format!(
             "({})",
             elements.iter().map(pattern_to_string).collect::<Vec<String>>().join(", ")

compiler/noirc_frontend/src/elaborator/expressions.rs

@@ -58,8 +58,8 @@ impl<'context> Elaborator<'context> {
             ExpressionKind::Comptime(comptime, _) => {
                 return self.elaborate_comptime_block(comptime, expr.span)
             }
-            ExpressionKind::Unsafe(block_expression, _) => {
-                self.elaborate_unsafe_block(block_expression, expr.span)
+            ExpressionKind::Unsafe(block_expression, span) => {
+                self.elaborate_unsafe_block(block_expression, span)
             }
             ExpressionKind::Resolved(id) => return (id, self.interner.id_type(id)),
             ExpressionKind::Interned(id) => {

compiler/noirc_frontend/src/lexer/lexer.rs

@@ -47,7 +47,7 @@
             done: false,
             skip_comments: true,
             skip_whitespaces: true,
             max_integer: BigInt::from_biguint(num_bigint::Sign::Plus, FieldElement::modulus())
                 - BigInt::one(),
         }
     }
@@ -726,7 +726,7 @@
     }
 
     fn parse_comment(&mut self, start: u32) -> SpannedTokenResult {
-        let mut doc_style = match self.peek_char() {
+        let doc_style = match self.peek_char() {
             Some('!') => {
                 self.next_char();
                 Some(DocStyle::Inner)
@@ -735,17 +735,9 @@
                 self.next_char();
                 Some(DocStyle::Outer)
             }
-            Some('@') => Some(DocStyle::Safety),
             _ => None,
         };
-        let mut comment = self.eat_while(None, |ch| ch != '\n');
-        if doc_style == Some(DocStyle::Safety) {
-            if comment.starts_with("@safety") {
-                comment = comment["@safety".len()..].to_string();
-            } else {
-                doc_style = None;
-            }
-        }
+        let comment = self.eat_while(None, |ch| ch != '\n');
 
         if !comment.is_ascii() {
             let span = Span::from(start..self.position);
@@ -760,7 +752,7 @@
     }
 
     fn parse_block_comment(&mut self, start: u32) -> SpannedTokenResult {
-        let mut doc_style = match self.peek_char() {
+        let doc_style = match self.peek_char() {
             Some('!') => {
                 self.next_char();
                 Some(DocStyle::Inner)
@@ -769,7 +761,6 @@
                 self.next_char();
                 Some(DocStyle::Outer)
             }
-            Some('@') => Some(DocStyle::Safety),
             _ => None,
         };
 
@@ -796,13 +787,6 @@
                 ch => content.push(ch),
             }
         }
-        if doc_style == Some(DocStyle::Safety) {
-            if content.starts_with("@safety") {
-                content = content["@safety".len()..].to_string();
-            } else {
-                doc_style = None;
-            }
-        }
         if depth == 0 {
             if !content.is_ascii() {
                 let span = Span::from(start..self.position);
@@ -1410,7 +1394,7 @@
     //   (expected_token_discriminator, strings_to_lex)
     // expected_token_discriminator matches a given token when
     // std::mem::discriminant returns the same discriminant for both.
     fn blns_base64_to_statements(base64_str: String) -> Vec<(Option<Token>, Vec<String>)> {
         use base64::engine::general_purpose;
         use std::borrow::Cow;
         use std::io::Cursor;
@@ -1468,7 +1452,7 @@
     fn test_big_list_of_naughty_strings() {
         use std::mem::discriminant;
 
         let blns_contents = include_str!("./blns/blns.base64.json");
         let blns_base64: Vec<String> =
             serde_json::from_str(blns_contents).expect("BLNS json invalid");
         for blns_base64_str in blns_base64 {

compiler/noirc_frontend/src/lexer/token.rs

@@ -345,7 +345,6 @@ impl Display for FmtStrFragment {
 pub enum DocStyle {
     Outer,
     Inner,
-    Safety,
 }
 
 #[derive(Debug, Clone, PartialEq, Eq, Hash, PartialOrd, Ord)]
@@ -425,13 +424,11 @@ impl fmt::Display for Token {
             Token::LineComment(ref s, style) => match style {
                 Some(DocStyle::Inner) => write!(f, "//!{s}"),
                 Some(DocStyle::Outer) => write!(f, "///{s}"),
-                Some(DocStyle::Safety) => write!(f, "//@safety{s}"),
                 None => write!(f, "//{s}"),
             },
             Token::BlockComment(ref s, style) => match style {
                 Some(DocStyle::Inner) => write!(f, "/*!{s}*/"),
                 Some(DocStyle::Outer) => write!(f, "/**{s}*/"),
-                Some(DocStyle::Safety) => write!(f, "/*@safety{s}*/"),
                 None => write!(f, "/*{s}*/"),
             },
             Token::Quote(ref stream) => {

compiler/noirc_frontend/src/parser/errors.rs

@@ -110,8 +110,10 @@ pub enum ParserErrorReason {
     WrongNumberOfAttributeArguments { name: String, min: usize, max: usize, found: usize },
     #[error("The `deprecated` attribute expects a string argument")]
     DeprecatedAttributeExpectsAStringArgument,
-    #[error("Unsafe block must start with a safety comment")]
+    #[error("Unsafe block must have a safety doc comment above it")]
     MissingSafetyComment,
+    #[error("Unsafe block must start with a safety comment")]
+    UnsafeDocCommentDoesNotStartWithSafety,
     #[error("Missing parameters for function definition")]
     MissingParametersForFunctionDefinition,
 }
@@ -283,10 +285,18 @@ impl<'a> From<&'a ParserError> for Diagnostic {
                     error.span,
                 ),
                 ParserErrorReason::MissingSafetyComment => Diagnostic::simple_warning(
-                    "Missing Safety Comment".into(),
-                    "Unsafe block must start with a safety comment: //@safety".into(),
+                    "Unsafe block must have a safety doc comment above it".into(),
+                    "The doc comment must start with the \"Safety: \" word".into(),
                     error.span,
                 ),
+                ParserErrorReason::UnsafeDocCommentDoesNotStartWithSafety => {
+                    Diagnostic::simple_warning(
+                        "Unsafe block must start with a safety comment".into(),
+                        "The doc comment above this unsafe block must start with the \"Safety: \" word"
+                            .into(),
+                        error.span,
+                    )
+                }
                 ParserErrorReason::MissingParametersForFunctionDefinition => {
                     Diagnostic::simple_error(
                         "Missing parameters for function definition".into(),

compiler/noirc_frontend/src/parser/parser.rs

@@ -83,6 +83,27 @@ pub struct Parser<'a> {
     next_token: SpannedToken,
     current_token_span: Span,
     previous_token_span: Span,
+
+    /// The current statement's doc comments.
+    /// This is used to eventually know if an `unsafe { ... }` expression is documented
+    /// in its containing statement. For example:
+    ///
+    /// ```noir
+    /// /// Safety: test
+    /// let x = unsafe { call() };
+    /// ```
+    statement_doc_comments: Option<StatementDocComments>,
+}
+
+#[derive(Debug)]
+pub(crate) struct StatementDocComments {
+    pub(crate) doc_comments: Vec<String>,
+    pub(crate) start_span: Span,
+    pub(crate) end_span: Span,
+
+    /// Were these doc comments "read" by an unsafe statement?
+    /// If not, these doc comments aren't documenting anything and they produce an error.
+    pub(crate) read: bool,
 }
 
 impl<'a> Parser<'a> {
@@ -107,6 +128,7 @@ impl<'a> Parser<'a> {
             next_token: eof_spanned_token(),
             current_token_span: Default::default(),
             previous_token_span: Default::default(),
+            statement_doc_comments: None,
         };
         parser.read_two_first_tokens();
         parser

compiler/noirc_frontend/src/parser/parser/expression.rs

@@ -8,7 +8,7 @@ use crate::{
         MemberAccessExpression, MethodCallExpression, Statement, TypePath, UnaryOp, UnresolvedType,
     },
     parser::{labels::ParsingRuleLabel, parser::parse_many::separated_by_comma, ParserErrorReason},
-    token::{DocStyle, Keyword, SpannedToken, Token, TokenKind},
+    token::{Keyword, Token, TokenKind},
 };
 
 use super::{
@@ -267,6 +267,21 @@ impl<'a> Parser<'a> {
     }
 
     fn parse_atom_kind(&mut self, allow_constructors: bool) -> Option<ExpressionKind> {
+        let span_before_doc_comments = self.current_token_span;
+        let doc_comments = self.parse_outer_doc_comments();
+        let has_doc_comments = !doc_comments.is_empty();
+
+        if let Some(kind) = self.parse_unsafe_expr(&doc_comments, span_before_doc_comments) {
+            return Some(kind);
+        }
+
+        if has_doc_comments {
+            self.push_error(
+                ParserErrorReason::DocCommentDoesNotDocumentAnything,
+                self.span_since(span_before_doc_comments),
+            );
+        }
+
         if let Some(literal) = self.parse_literal() {
             return Some(literal);
         }
@@ -275,10 +290,6 @@ impl<'a> Parser<'a> {
             return Some(kind);
         }
 
-        if let Some(kind) = self.parse_unsafe_expr() {
-            return Some(kind);
-        }
-
         if let Some(kind) = self.parse_path_expr(allow_constructors) {
             return Some(kind);
         }
@@ -370,26 +381,43 @@ impl<'a> Parser<'a> {
     }
 
     /// UnsafeExpression = 'unsafe' Block
-    fn parse_unsafe_expr(&mut self) -> Option<ExpressionKind> {
+    fn parse_unsafe_expr(
+        &mut self,
+        doc_comments: &[String],
+        span_before_doc_comments: Span,
+    ) -> Option<ExpressionKind> {
+        let start_span = self.current_token_span;
+
         if !self.eat_keyword(Keyword::Unsafe) {
             return None;
         }
 
-        let next_token = self.next_token.token();
-        if matches!(
-            next_token,
-            Token::LineComment(_, Some(DocStyle::Safety))
-                | Token::BlockComment(_, Some(DocStyle::Safety))
-        ) {
-            //Checks the safety comment is there, and skip it
-            let span = self.current_token_span;
-            self.eat_left_brace();
-            self.token = SpannedToken::new(Token::LeftBrace, span);
-        } else {
-            self.push_error(ParserErrorReason::MissingSafetyComment, self.current_token_span);
+        if doc_comments.is_empty() {
+            if let Some(statement_doc_comments) = &mut self.statement_doc_comments {
+                statement_doc_comments.read = true;
+
+                let doc_comments = &statement_doc_comments.doc_comments;
+                let span_before_doc_comments = statement_doc_comments.start_span;
+                let span_after_doc_comments = statement_doc_comments.end_span;
+
+                if !doc_comments[0].trim().to_lowercase().starts_with("safety:") {
+                    self.push_error(
+                        ParserErrorReason::UnsafeDocCommentDoesNotStartWithSafety,
+                        Span::from(
+                            span_before_doc_comments.start()..span_after_doc_comments.start(),
+                        ),
+                    );
+                }
+            } else {
+                self.push_error(ParserErrorReason::MissingSafetyComment, start_span);
+            }
+        } else if !doc_comments[0].trim().to_lowercase().starts_with("safety:") {
+            self.push_error(
+                ParserErrorReason::UnsafeDocCommentDoesNotStartWithSafety,
+                self.span_since(span_before_doc_comments),
+            );
         }
 
-        let start_span = self.current_token_span;
         if let Some(block) = self.parse_block() {
             Some(ExpressionKind::Unsafe(block, self.span_since(start_span)))
         } else {
@@ -971,8 +999,21 @@ mod tests {
 
     #[test]
     fn parses_unsafe_expression() {
-        let src = "unsafe { //@safety: test
-        1 }";
+        let src = "
+        /// Safety: test
+        unsafe { 1 }";
+        let expr = parse_expression_no_errors(src);
+        let ExpressionKind::Unsafe(block, _) = expr.kind else {
+            panic!("Expected unsafe expression");
+        };
+        assert_eq!(block.statements.len(), 1);
+    }
+
+    #[test]
+    fn parses_unsafe_expression_with_doc_comment() {
+        let src = "
+        /// Safety: test
+        unsafe { 1 }";
         let expr = parse_expression_no_errors(src);
         let ExpressionKind::Unsafe(block, _) = expr.kind else {
             panic!("Expected unsafe expression");