Add the undocumented_unsafe_blocks lint

CHANGELOG.md

@@ -2963,6 +2963,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_blocks`]: https://rust-lang.github.io/rust-clippy/master/index.html#undocumented_unsafe_blocks
 [`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.rs

@@ -351,6 +351,7 @@ mod transmute;
 mod transmuting_null;
 mod try_err;
 mod types;
+mod undocumented_unsafe_blocks;
 mod undropped_manually_drops;
 mod unicode;
 mod unit_return_expecting_ord;
@@ -951,6 +952,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         types::REDUNDANT_ALLOCATION,
         types::TYPE_COMPLEXITY,
         types::VEC_BOX,
+        undocumented_unsafe_blocks::UNDOCUMENTED_UNSAFE_BLOCKS,
         undropped_manually_drops::UNDROPPED_MANUALLY_DROPS,
         unicode::INVISIBLE_CHARACTERS,
         unicode::NON_ASCII_LITERAL,
@@ -1047,6 +1049,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(strings::STR_TO_STRING),
         LintId::of(types::RC_BUFFER),
         LintId::of(types::RC_MUTEX),
+        LintId::of(undocumented_unsafe_blocks::UNDOCUMENTED_UNSAFE_BLOCKS),
         LintId::of(unnecessary_self_imports::UNNECESSARY_SELF_IMPORTS),
         LintId::of(unwrap_in_result::UNWRAP_IN_RESULT),
         LintId::of(verbose_file_reads::VERBOSE_FILE_READS),
@@ -2105,6 +2108,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     store.register_early_pass(move || box disallowed_script_idents::DisallowedScriptIdents::new(&scripts));
     store.register_late_pass(|| box strlen_on_c_strings::StrlenOnCStrings);
     store.register_late_pass(move || box self_named_constructors::SelfNamedConstructors);
+    store.register_early_pass(|| box undocumented_unsafe_blocks::UndocumentedUnsafeBlocks::default());
 }
 
 #[rustfmt::skip]

clippy_lints/src/undocumented_unsafe_blocks.rs

@@ -0,0 +1,128 @@
+use clippy_utils::diagnostics::span_lint;
+use if_chain::if_chain;
+use rustc_ast::ast::{BlockCheckMode, ExprKind, UnsafeSource};
+use rustc_data_structures::fx::FxHashMap;
+use rustc_lint::{EarlyContext, EarlyLintPass};
+use rustc_session::{declare_tool_lint, impl_lint_pass};
+use rustc_span::{source_map::SourceMap, BytePos, FileName, Span};
+use std::convert::TryInto;
+
+declare_clippy_lint! {
+    /// ### What it does
+    /// Checks for `unsafe` blocks without a `// SAFETY` comment
+    /// explaining why the unsafe operations performed inside
+    /// the block are safe.
+    ///
+    /// ### Why is this bad?
+    /// Undocumented unsafe blocks are hard to check and maintain.
+    /// On the other hand, writing safety comments helps uncovering
+    /// unsoundness holes and bugs.
+    ///
+    /// ### Example
+    /// ```rust
+    /// # use std::ptr::NonNull;
+    /// # let a = &mut 42;
+    ///
+    /// let ptr = unsafe { NonNull::new_unchecked(a) };
+    /// ```
+    /// You should explain why calling `NonNull::new_unchecked` is safe:
+    /// ```rust
+    /// # use std::ptr::NonNull;
+    /// # let a = &mut 42;
+    ///
+    /// // SAFETY: references are guaranteed to be non-null.
+    /// let ptr = unsafe { NonNull::new_unchecked(a) };
+    /// ```
+    pub UNDOCUMENTED_UNSAFE_BLOCKS,
+    restriction,
+    "unsafe blocks without safety comments"
+}
+
+struct SafetyComment {
+    hi: BytePos,
+    hi_line: usize,
+}
+
+#[derive(Default)]
+pub struct UndocumentedUnsafeBlocks {
+    safety_comments: FxHashMap<FileName, Option<Vec<SafetyComment>>>,
+}
+
+impl UndocumentedUnsafeBlocks {
+    fn safety_comments(&mut self, sm: &SourceMap, span: Span) -> Option<&mut Vec<SafetyComment>> {
+        let file = sm.span_to_filename(span);
+        self.safety_comments
+            .entry(file.clone())
+            .or_insert_with(|| Self::gather_safety_comments(sm, &file))
+            .as_mut()
+    }
+
+    // Inspired by `rustc_ast::utils::comments::gather_comments`.
+    fn gather_safety_comments(sm: &SourceMap, file: &FileName) -> Option<Vec<SafetyComment>> {
+        let source_file = sm.get_source_file(file)?;
+        let src = source_file.src.as_deref()?;
+        let start_bpos = source_file.start_pos;
+        let mut comments = Vec::new();
+
+        let mut inside_comment = false;
+        let mut pos = rustc_lexer::strip_shebang(src).unwrap_or(0);
+        for token in rustc_lexer::tokenize(&src[pos..]) {
+            match token.kind {
+                rustc_lexer::TokenKind::LineComment { doc_style: None }
+                | rustc_lexer::TokenKind::BlockComment {
+                    doc_style: None,
+                    terminated: true,
+                } if src[pos + 2..pos + token.len].trim_start().starts_with("SAFETY") => {
+                    inside_comment = true;
+                },
+                rustc_lexer::TokenKind::LineComment { doc_style: None }
+                | rustc_lexer::TokenKind::BlockComment { doc_style: None, .. }
+                | rustc_lexer::TokenKind::Whitespace => {},
+                _ => {
+                    if inside_comment {
+                        let hi = start_bpos + BytePos(pos.try_into().unwrap());
+                        comments.push(SafetyComment {
+                            hi,
+                            hi_line: source_file.lookup_file_pos_with_col_display(hi).0,
+                        });
+                        inside_comment = false;
+                    }
+                },
+            }
+            pos += token.len;
+        }
+        Some(comments)
+    }
+}
+
+impl_lint_pass!(UndocumentedUnsafeBlocks => [UNDOCUMENTED_UNSAFE_BLOCKS]);
+
+impl EarlyLintPass for UndocumentedUnsafeBlocks {
+    fn check_expr(&mut self, cx: &EarlyContext<'_>, expr: &rustc_ast::Expr) {
+        let (block, comments) = if_chain! {
+            if let ExprKind::Block(ref block, _) = expr.kind;
+            if let BlockCheckMode::Unsafe(UnsafeSource::UserProvided) = block.rules;
+            if let Some(comments) = self.safety_comments(cx.sess.source_map(), block.span);
+            then {
+                (block, comments)
+            }
+            else {
+                return;
+            }
+        };
+        if_chain! {
+            // Since we're consuming comments as we visit the AST, the comment
+            // we're searching for is likely to be at the beginning of the vector,
+            // so this is better than a binary search.
+            if let Some((i, comment)) = comments.iter().enumerate().take_while(|(_, c)| c.hi <= block.span.lo()).last();
+            let block_line = cx.sess.source_map().lookup_char_pos(block.span.lo()).line;
+            if block_line == comment.hi_line + 1 || block_line == comment.hi_line;
+            then {
+                comments.remove(i);
+            }
+            else {
+                span_lint(cx, UNDOCUMENTED_UNSAFE_BLOCKS, expr.span, "this `unsafe` block is missing a SAFETY comment");
+            }
+        }
+    }
+}

tests/ui/functions.rs

@@ -1,6 +1,6 @@
 #![warn(clippy::all)]
 #![allow(dead_code)]
-#![allow(unused_unsafe, clippy::missing_safety_doc)]
+#![allow(unused_unsafe, clippy::missing_safety_doc)] 
 
 // TOO_MANY_ARGUMENTS
 fn good(_one: u32, _two: u32, _three: &str, _four: bool, _five: f32, _six: f32, _seven: bool) {}

tests/ui/undocumented_unsafe_blocks.rs

@@ -0,0 +1,74 @@
+#![warn(clippy::undocumented_unsafe_blocks)]
+#![allow(unused_unsafe)]
+
+fn good_basic() {
+    // SAFETY
+    unsafe {}
+}
+
+fn good_newlines() {
+    // SAFETY
+    //
+    //
+    //
+    unsafe {}
+}
+
+#[rustfmt::skip]
+fn good_whitespaces() {
+    // SAFETY
+
+
+
+    unsafe {}
+}
+
+fn good_block() {
+    /* SAFETY */
+    unsafe {}
+}
+
+#[rustfmt::skip]
+fn good_inline_block() {
+    /* SAFETY */ unsafe {}
+}
+
+fn good_statement() {
+    let _ =
+        // SAFETY
+        unsafe {};
+}
+
+fn bad_no_comment() {
+    unsafe {}
+}
+
+fn bad_comment_after_block() {
+    unsafe {} // SAFETY
+}
+
+fn bad_comment_after_block2() {
+    unsafe {
+        //
+    } // SAFETY
+    // SAFETY
+}
+
+fn bad_comment_inside_block() {
+    unsafe {
+        // SAFETY
+    }
+}
+
+#[rustfmt::skip]
+fn bad_two_blocks_one_comment() {
+    // SAFETY
+    unsafe {}; unsafe {}
+}
+
+#[rustfmt::skip]
+fn bad_block_comment_inside_block() {
+    unsafe { /* SAFETY */ }
+}
+
+fn main() {}

tests/ui/undocumented_unsafe_blocks.stderr

@@ -0,0 +1,44 @@
+error: this `unsafe` block is missing a SAFETY comment
+  --> $DIR/undocumented_unsafe_blocks.rs:43:5
+   |
+LL |     unsafe {}
+   |     ^^^^^^^^^
+   |
+   = note: `-D clippy::undocumented-unsafe-blocks` implied by `-D warnings`
+
+error: this `unsafe` block is missing a SAFETY comment
+  --> $DIR/undocumented_unsafe_blocks.rs:47:5
+   |
+LL |     unsafe {} // SAFETY
+   |     ^^^^^^^^^
+
+error: this `unsafe` block is missing a SAFETY comment
+  --> $DIR/undocumented_unsafe_blocks.rs:51:5
+   |
+LL | /     unsafe {
+LL | |         //
+LL | |     } // SAFETY
+   | |_____^
+
+error: this `unsafe` block is missing a SAFETY comment
+  --> $DIR/undocumented_unsafe_blocks.rs:58:5
+   |
+LL | /     unsafe {
+LL | |         // SAFETY
+LL | |     }
+   | |_____^
+
+error: this `unsafe` block is missing a SAFETY comment
+  --> $DIR/undocumented_unsafe_blocks.rs:66:16
+   |
+LL |     unsafe {}; unsafe {}
+   |                ^^^^^^^^^
+
+error: this `unsafe` block is missing a SAFETY comment
+  --> $DIR/undocumented_unsafe_blocks.rs:71:5
+   |
+LL |     unsafe { /* SAFETY */ }
+   |     ^^^^^^^^^^^^^^^^^^^^^^^
+
+error: aborting due to 6 previous errors
+