Allow commented blocks in locals, fix formatting

Serial-ATA · Serial-ATA · commit a76bb5905735 · 2021-10-04T19:26:10.000-04:00
diff --git a/clippy_lints/src/undocumented_unsafe_blocks.rs b/clippy_lints/src/undocumented_unsafe_blocks.rs
@@ -1,4 +1,4 @@
-use clippy_utils::diagnostics::{span_lint_and_help, span_lint_and_sugg};
+use clippy_utils::diagnostics::span_lint_and_sugg;
 use clippy_utils::source::{snippet, indent_of, reindent_multiline};
 use rustc_errors::Applicability;
 use rustc_hir::{Block, BlockCheckMode, Expr, ExprKind, HirId, Local, MatchSource, UnsafeSource, YieldSource};
@@ -47,23 +47,37 @@ impl_lint_pass!(UndocumentedUnsafeBlocks => [UNDOCUMENTED_UNSAFE_BLOCKS]);
 #[derive(Default)]
 pub struct UndocumentedUnsafeBlocks {
     pub local_level: u32,
-}
-
-enum ErrorKind {
-    NoComment,
-    Placeholder
+    pub local_span: Option<Span>,
+    // The local was already checked for an overall safety comment
+    // There is no need to continue checking the blocks in the local
+    pub local_checked: bool,
 }
 
 impl LateLintPass<'_> for UndocumentedUnsafeBlocks {
     fn check_block(&mut self, cx: &LateContext<'_>, block: &'_ Block<'_>) {
         if_chain! {
-            if self.local_level == 0;
+            if !self.local_checked;
             if !is_lint_allowed(cx, UNDOCUMENTED_UNSAFE_BLOCKS, block.hir_id);
             if !in_external_macro(cx.tcx.sess, block.span);
             if let BlockCheckMode::UnsafeBlock(UnsafeSource::UserProvided) = block.rules;
             if let Some(enclosing_scope_hir_id) = cx.tcx.hir().get_enclosing_scope(block.hir_id);
             then {
-                find_candidate(cx, block.span, enclosing_scope_hir_id)
+                if block_has_safety_comment(cx.tcx, enclosing_scope_hir_id, block.span) == Some(false) {
+                    let mut span = block.span;
+
+                    if let Some(local_span) = self.local_span {
+                        span = local_span;
+
+                        let result = block_has_safety_comment(cx.tcx, enclosing_scope_hir_id, span);
+
+                        if result == Some(true) || result.is_none() {
+                            self.local_checked = true;
+                            return;
+                        }
+                    }
+
+                    err(cx, span);
+                }
             }
         }
     }
@@ -75,103 +89,131 @@ impl LateLintPass<'_> for UndocumentedUnsafeBlocks {
             if let Some(init) = local.init;
             if let Some((block, element_count)) = find_block_in_expr(&init.kind);
             if let BlockCheckMode::UnsafeBlock(UnsafeSource::UserProvided) = block.rules;
-            if let Some(enclosing_scope_hir_id) = cx.tcx.hir().get_enclosing_scope(block.hir_id);
+            if cx.tcx.hir().get_enclosing_scope(block.hir_id).is_some();
             then {
                 self.local_level = self.local_level.saturating_add(element_count);
-
-                find_candidate(cx, local.span, enclosing_scope_hir_id)
+                self.local_span = Some(local.span);
             }
         }
     }
 
     fn check_block_post(&mut self, _: &LateContext<'_>, _: &'_ Block<'_>) {
         self.local_level = self.local_level.saturating_sub(1);
+
+        if self.local_level == 0 {
+            self.local_checked = false;
+            self.local_span = None;
+        }
     }
 }
 
 fn find_block_in_expr(expr_kind: &ExprKind<'hir>) -> Option<(&'tcx Block<'hir>, u32)> {
     match expr_kind {
-        ExprKind::Array(elements) | ExprKind::Tup(elements) =>  {
-            let unsafe_blocks = elements.iter().filter_map(|e|
-                match e {
-                    Expr{
-                        kind: ExprKind::Block(block @ Block {
-                                                  rules: BlockCheckMode::UnsafeBlock(UnsafeSource::UserProvided),
-                                                  ..
-                                              }, _),
+        ExprKind::Array(elements) | ExprKind::Tup(elements) => {
+            let unsafe_blocks = elements
+                .iter()
+                .filter_map(|e| match e {
+                    Expr {
+                        kind:
+                        ExprKind::Block(
+                            block
+                            @
+                            Block {
+                                rules: BlockCheckMode::UnsafeBlock(UnsafeSource::UserProvided),
+                                ..
+                            },
+                            _,
+                        ),
                         ..
                     } => Some(block),
-                    _ => None
-                }).collect::<Vec<_>>();
+                    _ => None,
+                })
+                .collect::<Vec<_>>();
 
             if let Some(block) = unsafe_blocks.get(0) {
-                return Some((block, unsafe_blocks.len().try_into().unwrap()))
+                return Some((block, unsafe_blocks.len().try_into().unwrap()));
             }
 
             None
-            },
+        }
         ExprKind::Box(Expr {
                           kind: ExprKind::Block(block, _),
                           ..
-                      }) |
-        ExprKind::Unary(_, Expr {
-            kind: ExprKind::Block(block, _),
-            ..
-        }) |  ExprKind::Match(Expr {
-                                                            kind: ExprKind::Block(block, _),
-                                                            ..
-                                                        }, _, MatchSource::Normal) | ExprKind::Loop(block, _, _, _) | ExprKind::Block(block, _) | ExprKind::AddrOf(_, _, Expr {
-            kind: ExprKind::Block(block, _),
-            ..
-        }) | ExprKind::Break(_, Some(Expr {
-            kind: ExprKind::Block(block, _),
-            ..
-                                })) | ExprKind::Ret(Some(Expr {
-                               kind: ExprKind::Block(block, _),
-                               ..
-                           })) | ExprKind::Repeat(Expr {
-                             kind: ExprKind::Block(block, _),
-                             ..
-                         }, _) | ExprKind::Yield(Expr{
-                            kind: ExprKind::Block(block, _),
-                            ..
-                        }, YieldSource::Yield) => Some((block, 1)),
-        _ => None
+                      })
+        | ExprKind::Unary(
+            _,
+            Expr {
+                kind: ExprKind::Block(block, _),
+                ..
+            },
+        )
+        | ExprKind::Match(
+            Expr {
+                kind: ExprKind::Block(block, _),
+                ..
+            },
+            _,
+            MatchSource::Normal,
+        )
+        | ExprKind::Loop(block, _, _, _)
+        | ExprKind::Block(block, _)
+        | ExprKind::AddrOf(
+            _,
+            _,
+            Expr {
+                kind: ExprKind::Block(block, _),
+                ..
+            },
+        )
+        | ExprKind::Break(
+            _,
+            Some(Expr {
+                     kind: ExprKind::Block(block, _),
+                     ..
+                 }),
+        )
+        | ExprKind::Ret(Some(Expr {
+                                 kind: ExprKind::Block(block, _),
+                                 ..
+                             }))
+        | ExprKind::Repeat(
+            Expr {
+                kind: ExprKind::Block(block, _),
+                ..
+            },
+            _,
+        )
+        | ExprKind::Yield(
+            Expr {
+                kind: ExprKind::Block(block, _),
+                ..
+            },
+            YieldSource::Yield,
+        ) => Some((block, 1)),
+        _ => None,
     }
 }
 
-fn find_candidate(cx: &LateContext<'_>, span: Span, enclosing_hir_id: HirId) {
-    if let Some(error_kind) = block_has_safety_comment(cx.tcx, enclosing_hir_id, span) {
-        match error_kind {
-            ErrorKind::NoComment => {
-                let block_indent = indent_of(cx, span);
-                let suggestion = format!("// Safety: ...\n{}", snippet(cx, span, ".."));
-
-                span_lint_and_sugg(
-                    cx,
-                    UNDOCUMENTED_UNSAFE_BLOCKS,
-                    span,
-                    "unsafe block missing a safety comment",
-                    "consider adding a safety comment",
-                    reindent_multiline(Cow::Borrowed(&suggestion), true, block_indent).to_string(),
-                    Applicability::HasPlaceholders,
-                );
-            }
-            ErrorKind::Placeholder => {
-                span_lint_and_help(
-                    cx,
-                    UNDOCUMENTED_UNSAFE_BLOCKS,
-                    span,
-                    "unsafe block has safety comment placeholder",
-                    None,
-                    "consider replacing the placeholder with a safety comment"
-                );
-            }
-        }
-    }
+fn err(cx: &LateContext<'_>, span: Span) {
+    let block_indent = indent_of(cx, span);
+    let suggestion = format!("// Safety: ...\n{}", snippet(cx, span, ".."));
+
+    span_lint_and_sugg(
+        cx,
+        UNDOCUMENTED_UNSAFE_BLOCKS,
+        span,
+        "unsafe block missing a safety comment",
+        "consider adding a safety comment",
+        reindent_multiline(Cow::Borrowed(&suggestion), true, block_indent).to_string(),
+        Applicability::HasPlaceholders,
+    );
 }
 
-fn block_has_safety_comment(tcx: TyCtxt<'_>, enclosing_hir_id: HirId, block_span: Span) -> Option<ErrorKind> {
+fn block_has_safety_comment(
+    tcx: TyCtxt<'_>,
+    enclosing_hir_id: HirId,
+    block_span: Span,
+) -> Option<bool> {
     let map = tcx.hir();
     let source_map = tcx.sess.source_map();
 
@@ -190,32 +232,43 @@ fn block_has_safety_comment(tcx: TyCtxt<'_>, enclosing_hir_id: HirId, block_span
     for token in rustc_lexer::tokenize(&src_str) {
         match token.kind {
             TokenKind::LineComment { doc_style: None }
-            | TokenKind::BlockComment { doc_style: None, terminated: true }  => {
+            | TokenKind::BlockComment {
+                doc_style: None,
+                terminated: true,
+            } => {
                 let comment_str = src_str[pos + 2..pos + token.len].to_ascii_uppercase();
 
-                if comment_str.contains("SAFETY: ...") {
-                    return Some(ErrorKind::Placeholder);
-                } else if comment_str.contains("SAFETY:") {
+                if comment_str.contains("SAFETY:") {
                     comment = true;
                 }
-            },
-            TokenKind::Whitespace => {},
+            }
+            // We need to add all whitespace to `pos` before checking the comment's line number
+            TokenKind::Whitespace => {}
             _ => {
                 if comment {
-                    let comment_line_num = source_file.lookup_file_pos_with_col_display(BytePos((lex_start + pos).try_into().unwrap())).0;
-                    let block_line_num = tcx.sess.source_map().lookup_char_pos(block_span.lo()).line;
+                    // Get the line number of the "comment" (really wherever the trailing whitespace ended)
+                    let comment_line_num = source_file
+                        .lookup_file_pos_with_col_display(BytePos(
+                            (lex_start + pos).try_into().unwrap(),
+                        ))
+                        .0;
+                    // Find the block/local's line number
+                    let block_line_num =
+                        tcx.sess.source_map().lookup_char_pos(block_span.lo()).line;
 
-                    if block_line_num == comment_line_num + 1 || block_line_num == comment_line_num {
-                        return None;
+                    // Check the comment is immediately followed by the block/local
+                    if block_line_num == comment_line_num + 1 || block_line_num == comment_line_num
+                    {
+                        return Some(true);
                     }
 
                     comment = false;
                 }
-            },
+            }
         }
 
         pos += token.len;
     }
 
-    Some(ErrorKind::NoComment)
+    Some(false)
 }
diff --git a/tests/ui/undocumented_unsafe_blocks.rs b/tests/ui/undocumented_unsafe_blocks.rs
@@ -136,17 +136,56 @@ fn local_block_comment() {
     let _ = unsafe {};
 }
 
-// Invalid comments
+fn comment_array() {
+    // Safety:
+    let _ = [unsafe { 14 }, unsafe { 15 }, 42, unsafe { 16 }];
+}
 
-fn no_comment() {
-    unsafe {}
+fn comment_tuple() {
+    // Safety:
+    let _ = (42, unsafe {}, "test", unsafe {});
+}
+
+fn comment_unary() {
+    // Safety:
+    let _ = *unsafe { &42 };
+}
+
+#[allow(clippy::match_single_binding)]
+fn comment_match() {
+    // Safety:
+    let _ = match unsafe {} {
+        _ => {},
+    };
+}
+
+fn comment_addr_of() {
+    // Safety:
+    let _ = &unsafe {};
 }
 
-fn contains_placeholder() {
-    // Safety: ...
+fn comment_repeat() {
+    // Safety:
+    let _ = [unsafe {}; 5];
+}
+
+fn non_ascii_comment() {
+    // ॐ᧻໒ Safety: ௵∰
     unsafe {};
 }
 
+fn local_commented_block() {
+    let _ =
+        // Safety:
+        unsafe {};
+}
+
+// Invalid comments
+
+fn no_comment() {
+    unsafe {}
+}
+
 fn no_comment_array() {
     let _ = [unsafe { 14 }, unsafe { 15 }, 42, unsafe { 16 }];
 }
@@ -178,12 +217,6 @@ fn local_no_comment() {
     let _ = unsafe {};
 }
 
-fn local_commented_block() {
-    let _ =
-        // Safety:
-        unsafe {};
-}
-
 fn trailing_comment() {
     unsafe {} // Safety:
 }
diff --git a/tests/ui/undocumented_unsafe_blocks.stderr b/tests/ui/undocumented_unsafe_blocks.stderr
