Support more expressions in Locals, check for an unaltered placeholder

Serial-ATA · Serial-ATA · commit e498f854b326 · 2021-10-04T19:06:56.000-04:00
diff --git a/clippy_lints/src/undocumented_unsafe_blocks.rs b/clippy_lints/src/undocumented_unsafe_blocks.rs
@@ -1,13 +1,13 @@
-use clippy_utils::diagnostics::span_lint_and_sugg;
+use clippy_utils::diagnostics::{span_lint_and_help, span_lint_and_sugg};
 use clippy_utils::source::{snippet, indent_of, reindent_multiline};
 use rustc_errors::Applicability;
-use rustc_hir::{Block, BlockCheckMode, ExprKind, HirId, Local, UnsafeSource};
+use rustc_hir::{Block, BlockCheckMode, Expr, ExprKind, HirId, Local, MatchSource, UnsafeSource, YieldSource};
 use rustc_lexer::TokenKind;
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_middle::lint::in_external_macro;
 use rustc_middle::ty::TyCtxt;
 use rustc_session::{impl_lint_pass, declare_tool_lint};
-use rustc_span::Span;
+use rustc_span::{BytePos, Span};
 use std::borrow::Cow;
 use clippy_utils::is_lint_allowed;
 
@@ -46,23 +46,25 @@ impl_lint_pass!(UndocumentedUnsafeBlocks => [UNDOCUMENTED_UNSAFE_BLOCKS]);
 
 #[derive(Default)]
 pub struct UndocumentedUnsafeBlocks {
-    pub local: bool
+    pub local_level: u32,
+}
+
+enum ErrorKind {
+    NoComment,
+    Placeholder
 }
 
 impl LateLintPass<'_> for UndocumentedUnsafeBlocks {
     fn check_block(&mut self, cx: &LateContext<'_>, block: &'_ Block<'_>) {
-        if self.local {
-            self.local = false;
-            return
-        }
-
-        if_chain! {
-            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 self.local_level == 0 {
+            if_chain! {
+                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)
+                }
             }
         }
     }
@@ -72,71 +74,149 @@ impl LateLintPass<'_> for UndocumentedUnsafeBlocks {
             if !is_lint_allowed(cx, UNDOCUMENTED_UNSAFE_BLOCKS, local.hir_id);
             if !in_external_macro(cx.tcx.sess, local.span);
             if let Some(init) = local.init;
-            if let ExprKind::Block(block, _) = init.kind;
+            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);
             then {
-                self.local = true;
+                self.local_level = self.local_level.saturating_add(element_count);
+
                 find_candidate(cx, local.span, enclosing_scope_hir_id)
             }
         }
     }
+
+    fn check_block_post(&mut self, _: &LateContext<'_>, _: &'_ Block<'_>) {
+        self.local_level = self.local_level.saturating_sub(1);
+    }
 }
 
-fn find_candidate(cx: &LateContext<'_>, span: Span, enclosing_hir_id: HirId) {
-    if let Some(true) = block_has_safety_comment(cx.tcx, enclosing_hir_id, span) {
-        return;
+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),
+                                                  ..
+                                              }, _),
+                        ..
+                    } => Some(block),
+                    _ => None
+                }).collect::<Vec<_>>();
+
+            if let Some(block) = unsafe_blocks.get(0) {
+                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
     }
+}
 
-    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 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 block_has_safety_comment(tcx: TyCtxt<'_>, enclosing_hir_id: HirId, block_span: Span) -> Option<bool> {
+fn block_has_safety_comment(tcx: TyCtxt<'_>, enclosing_hir_id: HirId, block_span: Span) -> Option<ErrorKind> {
     let map = tcx.hir();
     let source_map = tcx.sess.source_map();
 
     let enclosing_scope_span = map.opt_span(enclosing_hir_id)?;
-    let between_span = enclosing_scope_span.until(block_span);
+    let between_span = enclosing_scope_span.to(block_span);
 
     let file_name = source_map.span_to_filename(between_span);
     let source_file = source_map.get_source_file(&file_name)?;
 
     let lex_start = (between_span.lo().0 + 1) as usize;
-    let mut src_str = source_file.src.as_ref()?[lex_start..between_span.hi().0 as usize].to_string();
-
-    src_str.retain(|c| !c.is_whitespace());
-
-    let src_str_split = src_str.lines().rev().collect::<Vec<_>>();
-    let src_str = src_str_split.join("");
+    let src_str = source_file.src.as_ref()?[lex_start..between_span.hi().0 as usize].to_string();
 
     let mut pos = 0;
-    let mut found_safety_comment = false;
+    let mut comment = false;
 
     for token in rustc_lexer::tokenize(&src_str) {
         match token.kind {
             TokenKind::LineComment { doc_style: None }
-            | TokenKind::BlockComment { doc_style: None, terminated: true } => {
-                if src_str[pos + 2.. pos + token.len].to_ascii_uppercase().contains("SAFETY:") {
-                    found_safety_comment = true;
-                    break
+            | 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:") {
+                    comment = true;
+                }
+            },
+            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;
+
+                    if block_line_num == comment_line_num + 1 || block_line_num == comment_line_num {
+                        return None;
+                    }
+
+                    comment = false;
                 }
             },
-            _ => break
         }
 
         pos += token.len;
     }
 
-    Some(found_safety_comment)
+    Some(ErrorKind::NoComment)
 }
diff --git a/tests/ui/undocumented_unsafe_blocks.rs b/tests/ui/undocumented_unsafe_blocks.rs
@@ -2,6 +2,57 @@
 
 // Valid comments
 
+fn nested_local() {
+    let _ = {
+        let _ = {
+            // Safety:
+            let _ = unsafe {};
+        };
+    };
+}
+
+fn deep_nest() {
+    let _ = {
+        let _ = {
+            // Safety:
+            let _ = unsafe {};
+
+            // Safety:
+            unsafe {};
+
+            let _ = {
+                let _ = {
+                    let _ = {
+                        let _ = {
+                            let _ = {
+                                // Safety:
+                                let _ = unsafe {};
+
+                                // Safety:
+                                unsafe {};
+                            };
+                        };
+                    };
+
+                    // Safety:
+                    unsafe {};
+                };
+            };
+        };
+
+        // Safety:
+        unsafe {};
+    };
+
+    // Safety:
+    unsafe {};
+}
+
+fn local_tuple_expression() {
+    // Safety:
+    let _ = (42, unsafe {});
+}
+
 fn line_comment() {
     // Safety:
     unsafe {}
@@ -75,15 +126,6 @@ fn safety_with_prepended_text() {
     unsafe {}
 }
 
-fn nested_local() {
-    let _ = {
-        let _ = {
-            // Safety:
-            let _ = unsafe {};
-        };
-    };
-}
-
 fn local_line_comment() {
     // Safety:
     let _ = unsafe {};
@@ -100,6 +142,38 @@ fn no_comment() {
     unsafe {}
 }
 
+fn contains_placeholder() {
+    // Safety: ...
+    unsafe {};
+}
+
+fn no_comment_array() {
+    let _ = [unsafe { 14 }, unsafe { 15 }, 42, unsafe { 16 }];
+}
+
+fn no_comment_tuple() {
+    let _ = (42, unsafe {}, "test", unsafe {});
+}
+
+fn no_comment_unary() {
+    let _ = *unsafe { &42 };
+}
+
+#[allow(clippy::match_single_binding)]
+fn no_comment_match() {
+    let _ = match unsafe {} {
+        _ => {},
+    };
+}
+
+fn no_comment_addr_of() {
+    let _ = &unsafe {};
+}
+
+fn no_comment_repeat() {
+    let _ = [unsafe {}; 5];
+}
+
 fn local_no_comment() {
     let _ = unsafe {};
 }
diff --git a/tests/ui/undocumented_unsafe_blocks.stderr b/tests/ui/undocumented_unsafe_blocks.stderr
