feat: require safety comments instead of safety doc comments (#7295)

Co-authored-by: Tom French <tom@tomfren.ch>
Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com>

compiler/noirc_frontend/src/debug/mod.rs

@@ -523,7 +523,7 @@ pub fn build_debug_crate_file() -> String {
                 __debug_var_assign_oracle(var_id, value);
             }
             pub fn __debug_var_assign<T>(var_id: u32, value: T) {
-                /// Safety: debug context
+                // Safety: debug context
                 unsafe {
                 {
                     __debug_var_assign_inner(var_id, value);
@@ -536,7 +536,7 @@ pub fn build_debug_crate_file() -> String {
                 __debug_var_drop_oracle(var_id);
             }
             pub fn __debug_var_drop(var_id: u32) {
-                /// Safety: debug context
+                // Safety: debug context
                 unsafe {
                 {
                     __debug_var_drop_inner(var_id);
@@ -549,7 +549,7 @@ pub fn build_debug_crate_file() -> String {
                 __debug_fn_enter_oracle(fn_id);
             }
             pub fn __debug_fn_enter(fn_id: u32) {
-                /// Safety: debug context
+                // Safety: debug context
                 unsafe {
                 {
                     __debug_fn_enter_inner(fn_id);
@@ -562,7 +562,7 @@ pub fn build_debug_crate_file() -> String {
                 __debug_fn_exit_oracle(fn_id);
             }
             pub fn __debug_fn_exit(fn_id: u32) {
-                /// Safety: debug context
+                // Safety: debug context
                 unsafe {
                 {
                     __debug_fn_exit_inner(fn_id);
@@ -575,7 +575,7 @@ pub fn build_debug_crate_file() -> String {
                 __debug_dereference_assign_oracle(var_id, value);
             }
             pub fn __debug_dereference_assign<T>(var_id: u32, value: T) {
-                /// Safety: debug context
+                // Safety: debug context
                 unsafe {
                 {
                     __debug_dereference_assign_inner(var_id, value);

compiler/noirc_frontend/src/parser/errors.rs

@@ -110,10 +110,8 @@ 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 have a safety doc comment above it")]
+    #[error("Unsafe block must have a safety comment above it")]
     MissingSafetyComment,
-    #[error("Unsafe block must start with a safety comment")]
-    UnsafeDocCommentDoesNotStartWithSafety,
     #[error("Missing parameters for function definition")]
     MissingParametersForFunctionDefinition,
 }
@@ -285,25 +283,22 @@ impl<'a> From<&'a ParserError> for Diagnostic {
                     error.span,
                 ),
                 ParserErrorReason::MissingSafetyComment => Diagnostic::simple_warning(
-                    "Unsafe block must have a safety doc comment above it".into(),
-                    "The doc comment must start with the \"Safety: \" word".into(),
+                    "Unsafe block must have a safety comment above it".into(),
+                    "The 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(),
                         "Add a parameter list: `()`".into(),
                         error.span,
                     )
                 }
+                ParserErrorReason::DocCommentDoesNotDocumentAnything => {
+                    let primary = "This doc comment doesn't document anything".to_string();
+                    let secondary = "Consider changing it to a regular `//` comment".to_string();
+                    Diagnostic::simple_warning(primary, secondary, error.span)
+                }
                 other => Diagnostic::simple_error(format!("{other}"), String::new(), error.span),
             },
             None => {
@@ -313,7 +308,7 @@ impl<'a> From<&'a ParserError> for Diagnostic {
                 ) {
                     let primary = "This doc comment doesn't document anything".to_string();
                     let secondary = "Consider changing it to a regular `//` comment".to_string();
-                    Diagnostic::simple_error(primary, secondary, error.span)
+                    Diagnostic::simple_warning(primary, secondary, error.span)
                 } else {
                     let primary = error.to_string();
                     Diagnostic::simple_error(primary, String::new(), error.span)

compiler/noirc_frontend/src/parser/parser.rs

@@ -85,31 +85,25 @@ pub struct Parser<'a> {
     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
+    // We also keep track of comments that appear right before a token,
+    // because `unsafe { }` requires one before it.
+    current_token_comments: String,
+    next_token_comments: String,
+
+    /// The current statement's comments.
+    /// This is used to eventually know if an `unsafe { ... }` expression is commented
     /// in its containing statement. For example:
     ///
     /// ```noir
-    /// /// Safety: test
+    /// // 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,
+    statement_comments: Option<String>,
 }
 
 impl<'a> Parser<'a> {
     pub fn for_lexer(lexer: Lexer<'a>) -> Self {
-        Self::new(TokenStream::Lexer(lexer))
+        Self::new(TokenStream::Lexer(lexer.skip_comments(false)))
     }
 
     pub fn for_tokens(mut tokens: Tokens) -> Self {
@@ -129,7 +123,9 @@ impl<'a> Parser<'a> {
             next_token: eof_spanned_token(),
             current_token_span: Default::default(),
             previous_token_span: Default::default(),
-            statement_doc_comments: None,
+            current_token_comments: String::new(),
+            next_token_comments: String::new(),
+            statement_comments: None,
         };
         parser.read_two_first_tokens();
         parser
@@ -177,25 +173,45 @@ impl<'a> Parser<'a> {
     /// Bumps this parser by one token. Returns the token that was previously the "current" token.
     fn bump(&mut self) -> SpannedToken {
         self.previous_token_span = self.current_token_span;
-        let next_next_token = self.read_token_internal();
+        let (next_next_token, next_next_token_comments) = self.read_token_internal();
         let next_token = std::mem::replace(&mut self.next_token, next_next_token);
         let token = std::mem::replace(&mut self.token, next_token);
+
+        let next_comments =
+            std::mem::replace(&mut self.next_token_comments, next_next_token_comments);
+        let _ = std::mem::replace(&mut self.current_token_comments, next_comments);
+
         self.current_token_span = self.token.to_span();
         token
     }
 
     fn read_two_first_tokens(&mut self) {
-        self.token = self.read_token_internal();
+        let (token, comments) = self.read_token_internal();
+        self.token = token;
+        self.current_token_comments = comments;
         self.current_token_span = self.token.to_span();
-        self.next_token = self.read_token_internal();
+
+        let (token, comments) = self.read_token_internal();
+        self.next_token = token;
+        self.next_token_comments = comments;
     }
 
-    fn read_token_internal(&mut self) -> SpannedToken {
+    fn read_token_internal(&mut self) -> (SpannedToken, String) {
+        let mut last_comments = String::new();
+
         loop {
             match self.tokens.next() {
-                Some(Ok(token)) => return token,
+                Some(Ok(token)) => match token.token() {
+                    Token::LineComment(comment, None) | Token::BlockComment(comment, None) => {
+                        last_comments.push_str(comment);
+                        continue;
+                    }
+                    _ => {
+                        return (token, last_comments);
+                    }
+                },
                 Some(Err(lexer_error)) => self.errors.push(lexer_error.into()),
-                None => return eof_spanned_token(),
+                None => return (eof_spanned_token(), last_comments),
             }
         }
     }

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

@@ -269,19 +269,17 @@ 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 {
+        if !doc_comments.is_empty() {
             self.push_error(
                 ParserErrorReason::DocCommentDoesNotDocumentAnything,
-                self.span_since(span_before_doc_comments),
+                span_before_doc_comments,
             );
         }
 
+        if let Some(kind) = self.parse_unsafe_expr() {
+            return Some(kind);
+        }
+
         if let Some(literal) = self.parse_literal() {
             return Some(literal);
         }
@@ -387,41 +385,23 @@ impl<'a> Parser<'a> {
     }
 
     /// UnsafeExpression = 'unsafe' Block
-    fn parse_unsafe_expr(
-        &mut self,
-        doc_comments: &[String],
-        span_before_doc_comments: Span,
-    ) -> Option<ExpressionKind> {
+    fn parse_unsafe_expr(&mut self) -> Option<ExpressionKind> {
         let start_span = self.current_token_span;
 
         if !self.eat_keyword(Keyword::Unsafe) {
             return None;
         }
 
-        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(),
-                        ),
-                    );
+        if self.current_token_comments.is_empty() {
+            if let Some(statement_comments) = &mut self.statement_comments {
+                if !statement_comments.trim().to_lowercase().starts_with("safety:") {
+                    self.push_error(ParserErrorReason::MissingSafetyComment, start_span);
                 }
             } 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),
-            );
+        } else if !self.current_token_comments.trim().to_lowercase().starts_with("safety:") {
+            self.push_error(ParserErrorReason::MissingSafetyComment, start_span);
         }
 
         if let Some(block) = self.parse_block() {
@@ -1097,7 +1077,7 @@ mod tests {
     #[test]
     fn parses_unsafe_expression() {
         let src = "
-        /// Safety: test
+        // Safety: test
         unsafe { 1 }";
         let expr = parse_expression_no_errors(src);
         let ExpressionKind::Unsafe(block, _) = expr.kind else {
@@ -1111,7 +1091,9 @@ mod tests {
         let src = "
         /// Safety: test
         unsafe { 1 }";
-        let expr = parse_expression_no_errors(src);
+
+        let mut parser = Parser::for_str(src);
+        let expr = parser.parse_expression().unwrap();
         let ExpressionKind::Unsafe(block, _) = expr.kind else {
             panic!("Expected unsafe expression");
         };

compiler/noirc_frontend/src/parser/parser/item.rs

@@ -276,6 +276,6 @@ mod tests {
         let (module, errors) = parse_program(&src);
         assert_eq!(module.items.len(), 1);
         let error = get_single_error(&errors, span);
-        assert!(error.to_string().contains("Documentation comment does not document anything"));
+        assert!(error.to_string().contains("This doc comment doesn't document anything"));
     }
 }