Add undocumented_unsafe_block_safety lint

Author: Serial-ATA

CHANGELOG.md

@@ -3032,6 +3032,7 @@ Released 2018-09-13
 [`try_err`]: https://rust-lang.github.io/rust-clippy/master/index.html#try_err
 [`type_complexity`]: https://rust-lang.github.io/rust-clippy/master/index.html#type_complexity
 [`type_repetition_in_bounds`]: https://rust-lang.github.io/rust-clippy/master/index.html#type_repetition_in_bounds
+[`undocumented_unsafe_block_safety`]: https://rust-lang.github.io/rust-clippy/master/index.html#undocumented_unsafe_block_safety
 [`undropped_manually_drops`]: https://rust-lang.github.io/rust-clippy/master/index.html#undropped_manually_drops
 [`unicode_not_nfc`]: https://rust-lang.github.io/rust-clippy/master/index.html#unicode_not_nfc
 [`unimplemented`]: https://rust-lang.github.io/rust-clippy/master/index.html#unimplemented

clippy_lints/src/lib.mods.rs

@@ -204,6 +204,7 @@ mod transmute;
 mod transmuting_null;
 mod try_err;
 mod types;
+mod undocumented_unsafe_block_safety;
 mod undropped_manually_drops;
 mod unicode;
 mod unit_return_expecting_ord;

clippy_lints/src/lib.register_lints.rs

@@ -464,6 +464,7 @@ store.register_lints(&[
     types::REDUNDANT_ALLOCATION,
     types::TYPE_COMPLEXITY,
     types::VEC_BOX,
+    undocumented_unsafe_block_safety::UNDOCUMENTED_UNSAFE_BLOCK_SAFETY,
     undropped_manually_drops::UNDROPPED_MANUALLY_DROPS,
     unicode::INVISIBLE_CHARACTERS,
     unicode::NON_ASCII_LITERAL,

clippy_lints/src/lib.register_restriction.rs

@@ -56,6 +56,7 @@ store.register_group(true, "clippy::restriction", Some("clippy_restriction"), ve
     LintId::of(strings::STR_TO_STRING),
     LintId::of(types::RC_BUFFER),
     LintId::of(types::RC_MUTEX),
+    LintId::of(undocumented_unsafe_block_safety::UNDOCUMENTED_UNSAFE_BLOCK_SAFETY),
     LintId::of(unnecessary_self_imports::UNNECESSARY_SELF_IMPORTS),
     LintId::of(unwrap_in_result::UNWRAP_IN_RESULT),
     LintId::of(verbose_file_reads::VERBOSE_FILE_READS),

clippy_lints/src/undocumented_unsafe_block_safety.rs

@@ -0,0 +1,198 @@
+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, ExprKind, HirId, Local, UnsafeSource};
+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 core::str::Lines;
+use core::iter::Rev;
+use std::borrow::Cow;
+use clippy_utils::is_lint_allowed;
+
+declare_clippy_lint! {
+    /// ### What it does
+    /// Checks for `unsafe` blocks without a `// Safety: ` comment
+    /// explaining why the unsafe operations performed inside
+    /// the block are (or are not) safe.
+    ///
+    /// ### Why is this bad?
+    /// Unsafe blocks without a safety comment can be difficult to
+    /// understand for those that are unfamiliar with the block's code
+    /// and/or unsafe in general.
+    ///
+    /// ### Example
+    /// ```rust
+    ///   unsafe {};
+    ///
+    ///   let _ = unsafe {};
+    /// ```
+    /// Use instead:
+    /// ```rust
+    /// // Safety: ...
+    /// unsafe {}
+    ///
+    /// // Safety: ...
+    /// let _ = unsafe {};
+    /// ```
+    pub UNDOCUMENTED_UNSAFE_BLOCK_SAFETY,
+    restriction,
+    "creating an unsafe block without explaining why it is or is not safe"
+}
+
+impl_lint_pass!(UndocumentedUnsafeBlockSafety => [UNDOCUMENTED_UNSAFE_BLOCK_SAFETY]);
+
+#[derive(Default)]
+pub struct UndocumentedUnsafeBlockSafety {
+    pub local: bool
+}
+
+#[derive(PartialEq)]
+enum CommentType {
+    Safety,
+    Unrelated,
+    None,
+}
+
+impl LateLintPass<'_> for UndocumentedUnsafeBlockSafety {
+    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_BLOCK_SAFETY, 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)
+            }
+        }
+    }
+
+    fn check_local(&mut self, cx: &LateContext<'_>, local: &'_ Local<'_>) {
+        if_chain! {
+            if !is_lint_allowed(cx, UNDOCUMENTED_UNSAFE_BLOCK_SAFETY, 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 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;
+                find_candidate(cx, local.span, enclosing_scope_hir_id)
+            }
+        }
+    }
+}
+
+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;
+    }
+
+    let block_indent = indent_of(cx, span);
+    let suggestion = format!("// Safety: ...\n{}", snippet(cx, span, ".."));
+
+    span_lint_and_sugg(
+        cx,
+        UNDOCUMENTED_UNSAFE_BLOCK_SAFETY,
+        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<bool> {
+    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 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();
+
+    // Remove all whitespace, but retain newlines to verify the block immediately follows the comment
+    src_str.retain(|c| c == '\n' || !c.is_whitespace());
+
+    let mut src_str_split = src_str.lines().rev();
+
+    let mut found_safety_comment = false;
+
+    while let Some(line) =  src_str_split.next() {
+        match line {
+            "*/" => return Some(check_multiline_block(&mut src_str_split, None)),
+            line if (line.starts_with('*') && line.ends_with("*/")) => return Some(check_multiline_block(&mut src_str_split, Some(&line[1..]))),
+            // Covers both line comments and single line block comments
+            line => match contains_safety_comment(line, false) {
+                CommentType::Safety => { found_safety_comment = true; break },
+                CommentType::Unrelated => continue,
+                CommentType::None => break,
+            }
+        }
+    }
+
+    Some(found_safety_comment)
+}
+
+fn check_multiline_block(lines: &mut Rev<Lines<'_>>, same_line_terminator: Option<&str>) -> bool {
+    let mut found_safety = false;
+
+    if let Some(line) = same_line_terminator {
+        found_safety = contains_safety_comment(line, true) == CommentType::Safety;
+    }
+
+    for next_line in lines {
+        if found_safety {
+            break
+        }
+
+        let text_start = if next_line.starts_with('*') {
+            1
+        } else if next_line.starts_with("/*") {
+            2
+        } else {
+            return false
+        };
+
+        found_safety = contains_safety_comment(&next_line[text_start..], true) == CommentType::Safety;
+    }
+
+    found_safety
+}
+
+fn contains_safety_comment(comment: &str, block: bool) -> CommentType {
+    if block || is_comment(comment) {
+        let comment_upper = comment.to_uppercase();
+        if comment_upper.trim_start().starts_with("SAFETY:") || comment_upper.contains("SAFETY:") {
+            return CommentType::Safety
+        }
+
+        return CommentType::Unrelated
+    }
+
+    CommentType::None
+}
+
+fn is_comment(comment: &str) -> bool {
+    if let Some(token) = rustc_lexer::tokenize(comment).next() {
+        match token.kind {
+            TokenKind::LineComment { doc_style: None }
+            | TokenKind::BlockComment { doc_style: None, terminated: true } => return true,
+            _ => {},
+        }
+    }
+
+    false
+}

tests/ui/undocumented_unsafe_block_safety.rs

@@ -0,0 +1,106 @@
+#![warn(clippy::undocumented_unsafe_block_safety)]
+
+// Valid comments
+
+fn line_comment() {
+    // Safety:
+    unsafe {}
+}
+
+fn line_comment_with_extras() {
+    // This is a description
+    // Safety:
+    unsafe {}
+}
+
+fn let_statement_line_comment() {
+    // Safety:
+    let _ = unsafe {};
+}
+
+fn block_comment() {
+    /* Safety: */
+    unsafe {}
+}
+
+#[rustfmt::skip]
+fn inline_block_comment() {
+    /* Safety: */unsafe {}
+}
+
+fn block_comment_with_extras() {
+    /* This is a description
+     * Safety:
+     */
+    unsafe {}
+}
+
+fn block_comment_terminator_same_line() {
+    /* This is a description
+     * Safety: */
+    unsafe {}
+}
+
+fn let_statement_block_comment() {
+    /* Safety: */
+    let _ = unsafe {};
+}
+
+fn buried_safety() {
+    // Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor
+    // incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation
+    // ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in
+    // reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint
+    // occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est
+    // laborum. Safety:
+    // Tellus elementum sagittis vitae et leo duis ut diam quam. Sit amet nulla facilisi
+    // morbi tempus iaculis urna. Amet luctus venenatis lectus magna. At quis risus sed vulputate odio
+    // ut. Luctus venenatis lectus magna fringilla urna. Tortor id aliquet lectus proin nibh nisl
+    // condimentum id venenatis. Vulputate dignissim suspendisse in est ante in nibh mauris cursus.
+    unsafe {}
+}
+
+fn safety_with_prepended_text() {
+    // This is a test. Safety:
+    unsafe {}
+}
+
+// Invalid comments
+
+fn no_comment() {
+    unsafe {}
+}
+
+fn let_statement_no_comment() {
+    let _ = unsafe {};
+}
+
+fn let_statement_commented_block() {
+    let _ =
+        // Safety:
+        unsafe {};
+}
+
+fn trailing_comment() {
+    unsafe {} // Safety:
+}
+
+fn internal_comment() {
+    unsafe {
+        // Safety:
+    }
+}
+
+fn line_comment_newlines() {
+    // Safety:
+
+    unsafe {}
+}
+
+fn block_comment_newlines() {
+    /* Safety: */
+
+    unsafe {}
+}
+
+fn main() {}