Add undocumented_unsafe_blocks lint

CHANGELOG.md

@@ -3033,6 +3033,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.register_lints.rs

@@ -465,6 +465,7 @@ store.register_lints(&[
     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,

clippy_lints/src/lib.register_restriction.rs

@@ -57,6 +57,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_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),

clippy_lints/src/lib.rs

@@ -358,6 +358,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;
@@ -769,6 +770,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     store.register_late_pass(move || Box::new(if_then_panic::IfThenPanic));
     let enable_raw_pointer_heuristic_for_send = conf.enable_raw_pointer_heuristic_for_send;
     store.register_late_pass(move || Box::new(non_send_fields_in_send_ty::NonSendFieldInSendTy::new(enable_raw_pointer_heuristic_for_send)));
+    store.register_late_pass(move || Box::new(undocumented_unsafe_blocks::UndocumentedUnsafeBlocks::default()));
 }
 
 #[rustfmt::skip]

clippy_lints/src/undocumented_unsafe_blocks.rs

@@ -0,0 +1,225 @@
+use clippy_utils::diagnostics::{span_lint_and_help, span_lint_and_sugg};
+use clippy_utils::source::{indent_of, reindent_multiline, snippet};
+use clippy_utils::{in_macro, is_lint_allowed};
+use rustc_errors::Applicability;
+use rustc_hir::intravisit::{walk_expr, NestedVisitorMap, Visitor};
+use rustc_hir::{Block, BlockCheckMode, Expr, ExprKind, HirId, Local, UnsafeSource};
+use rustc_lexer::TokenKind;
+use rustc_lint::{LateContext, LateLintPass};
+use rustc_middle::hir::map::Map;
+use rustc_middle::lint::in_external_macro;
+use rustc_middle::ty::TyCtxt;
+use rustc_session::{declare_tool_lint, impl_lint_pass};
+use rustc_span::{BytePos, Span};
+use std::borrow::Cow;
+
+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 can make it difficult to
+    /// read and maintain code, as well as uncover unsoundness
+    /// and bugs.
+    ///
+    /// ### Example
+    /// ```rust
+    /// use std::ptr::NonNull;
+    /// let a = &mut 42;
+    ///
+    /// let ptr = unsafe { NonNull::new_unchecked(a) };
+    /// ```
+    /// Use instead:
+    /// ```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,
+    "creating an unsafe block without explaining why it is safe"
+}
+
+impl_lint_pass!(UndocumentedUnsafeBlocks => [UNDOCUMENTED_UNSAFE_BLOCKS]);
+
+#[derive(Default)]
+pub struct UndocumentedUnsafeBlocks {
+    pub local_level: u32,
+    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,
+    // Since we can only check the blocks from expanded macros
+    // We have to omit the suggestion due to the actual definition
+    // Not being available to us
+    pub macro_expansion: bool,
+}
+
+impl LateLintPass<'_> for UndocumentedUnsafeBlocks {
+    fn check_block(&mut self, cx: &LateContext<'_>, block: &'_ Block<'_>) {
+        if_chain! {
+            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);
+            if self.block_has_safety_comment(cx.tcx, enclosing_scope_hir_id, block.span) == Some(false);
+            then {
+                let mut span = block.span;
+
+                if let Some(local_span) = self.local_span {
+                    span = local_span;
+
+                    let result = self.block_has_safety_comment(cx.tcx, enclosing_scope_hir_id, span);
+
+                    if result.unwrap_or(true) {
+                        self.local_checked = true;
+                        return;
+                    }
+                }
+
+                self.lint(cx, span);
+            }
+        }
+    }
+
+    fn check_local(&mut self, cx: &LateContext<'_>, local: &'_ Local<'_>) {
+        if_chain! {
+            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;
+            then {
+                self.visit_expr(init);
+
+                if self.local_level > 0 {
+                    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;
+        }
+    }
+}
+
+impl<'hir> Visitor<'hir> for UndocumentedUnsafeBlocks {
+    type Map = Map<'hir>;
+
+    fn nested_visit_map(&mut self) -> NestedVisitorMap<Self::Map> {
+        NestedVisitorMap::None
+    }
+
+    fn visit_expr(&mut self, ex: &'v Expr<'v>) {
+        match ex.kind {
+            ExprKind::Block(_, _) => self.local_level = self.local_level.saturating_add(1),
+            _ => walk_expr(self, ex),
+        }
+    }
+}
+
+impl UndocumentedUnsafeBlocks {
+    fn block_has_safety_comment(&mut self, 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 = if in_macro(block_span) {
+            self.macro_expansion = true;
+            enclosing_scope_span.with_hi(block_span.hi())
+        } else {
+            self.macro_expansion = false;
+            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 src_str = source_file.src.as_ref()?[lex_start..between_span.hi().0 as usize].to_string();
+
+        let mut pos = 0;
+        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,
+                } => {
+                    let comment_str = src_str[pos + 2..pos + token.len].to_ascii_uppercase();
+
+                    if comment_str.contains("SAFETY:") {
+                        comment = true;
+                    }
+                },
+                // We need to add all whitespace to `pos` before checking the comment's line number
+                TokenKind::Whitespace => {},
+                _ => {
+                    if comment {
+                        // 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;
+
+                        // 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(false)
+    }
+
+    fn lint(&self, cx: &LateContext<'_>, mut span: Span) {
+        let source_map = cx.tcx.sess.source_map();
+
+        if source_map.is_multiline(span) {
+            span = source_map.span_until_char(span, '\n');
+        }
+
+        if self.macro_expansion {
+            span_lint_and_help(
+                cx,
+                UNDOCUMENTED_UNSAFE_BLOCKS,
+                span,
+                "unsafe block in macro expansion missing a safety comment",
+                None,
+                "consider adding a safety comment in the macro definition",
+            );
+        } else {
+            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,
+            );
+        }
+    }
+}

tests/ui/undocumented_unsafe_blocks.rs

@@ -0,0 +1,287 @@
+#![warn(clippy::undocumented_unsafe_blocks)]
+
+// 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 {}
+}
+
+fn line_comment_newlines() {
+    // Safety:
+
+    unsafe {}
+}
+
+fn line_comment_empty() {
+    // Safety:
+    //
+    //
+    //
+    unsafe {}
+}
+
+fn line_comment_with_extras() {
+    // This is a description
+    // Safety:
+    unsafe {}
+}
+
+fn block_comment() {
+    /* Safety: */
+    unsafe {}
+}
+
+fn block_comment_newlines() {
+    /* 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 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 {}
+}
+
+fn local_line_comment() {
+    // Safety:
+    let _ = unsafe {};
+}
+
+fn local_block_comment() {
+    /* Safety: */
+    let _ = unsafe {};
+}
+
+fn comment_array() {
+    // Safety:
+    let _ = [unsafe { 14 }, unsafe { 15 }, 42, unsafe { 16 }];
+}
+
+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 comment_repeat() {
+    // Safety:
+    let _ = [unsafe {}; 5];
+}
+
+fn comment_macro_call() {
+    macro_rules! t {
+        ($b:expr) => {
+            $b
+        };
+    }
+
+    t!(
+        // Safety:
+        unsafe {}
+    );
+}
+
+fn comment_macro_def() {
+    macro_rules! t {
+        () => {
+            // Safety:
+            unsafe {}
+        };
+    }
+
+    t!();
+}
+
+fn non_ascii_comment() {
+    // ॐ᧻໒ Safety: ௵∰
+    unsafe {};
+}
+
+fn local_commented_block() {
+    let _ =
+        // Safety:
+        unsafe {};
+}
+
+fn local_nest() {
+    // Safety:
+    let _ = [(42, unsafe {}, unsafe {}), (52, unsafe {}, unsafe {})];
+}
+
+// Invalid comments
+
+fn no_comment() {
+    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 {};
+}
+
+fn no_comment_macro_call() {
+    macro_rules! t {
+        ($b:expr) => {
+            $b
+        };
+    }
+
+    t!(unsafe {});
+}
+
+fn no_comment_macro_def() {
+    macro_rules! t {
+        () => {
+            unsafe {}
+        };
+    }
+
+    t!();
+}
+
+fn trailing_comment() {
+    unsafe {} // Safety:
+}
+
+fn internal_comment() {
+    unsafe {
+        // Safety:
+    }
+}
+
+fn interference() {
+    // Safety
+
+    let _ = 42;
+
+    unsafe {};
+}
+
+fn main() {}

tests/ui/undocumented_unsafe_blocks.stderr

@@ -0,0 +1,159 @@
+error: unsafe block missing a safety comment
+  --> $DIR/undocumented_unsafe_blocks.rs:215:5
+   |
+LL |     unsafe {}
+   |     ^^^^^^^^^
+   |
+   = note: `-D clippy::undocumented-unsafe-blocks` implied by `-D warnings`
+help: consider adding a safety comment
+   |
+LL ~     // Safety: ...
+LL +     unsafe {}
+   |
+
+error: unsafe block missing a safety comment
+  --> $DIR/undocumented_unsafe_blocks.rs:219:5
+   |
+LL |     let _ = [unsafe { 14 }, unsafe { 15 }, 42, unsafe { 16 }];
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+help: consider adding a safety comment
+   |
+LL ~     // Safety: ...
+LL +     let _ = [unsafe { 14 }, unsafe { 15 }, 42, unsafe { 16 }];
+   |
+
+error: unsafe block missing a safety comment
+  --> $DIR/undocumented_unsafe_blocks.rs:223:5
+   |
+LL |     let _ = (42, unsafe {}, "test", unsafe {});
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+help: consider adding a safety comment
+   |
+LL ~     // Safety: ...
+LL +     let _ = (42, unsafe {}, "test", unsafe {});
+   |
+
+error: unsafe block missing a safety comment
+  --> $DIR/undocumented_unsafe_blocks.rs:227:5
+   |
+LL |     let _ = *unsafe { &42 };
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+help: consider adding a safety comment
+   |
+LL ~     // Safety: ...
+LL +     let _ = *unsafe { &42 };
+   |
+
+error: unsafe block missing a safety comment
+  --> $DIR/undocumented_unsafe_blocks.rs:232:5
+   |
+LL |     let _ = match unsafe {} {
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+help: consider adding a safety comment
+   |
+LL ~     // Safety: ...
+LL +     let _ = match unsafe {} {
+   |
+
+error: unsafe block missing a safety comment
+  --> $DIR/undocumented_unsafe_blocks.rs:238:5
+   |
+LL |     let _ = &unsafe {};
+   |     ^^^^^^^^^^^^^^^^^^^
+   |
+help: consider adding a safety comment
+   |
+LL ~     // Safety: ...
+LL +     let _ = &unsafe {};
+   |
+
+error: unsafe block missing a safety comment
+  --> $DIR/undocumented_unsafe_blocks.rs:242:5
+   |
+LL |     let _ = [unsafe {}; 5];
+   |     ^^^^^^^^^^^^^^^^^^^^^^^
+   |
+help: consider adding a safety comment
+   |
+LL ~     // Safety: ...
+LL +     let _ = [unsafe {}; 5];
+   |
+
+error: unsafe block missing a safety comment
+  --> $DIR/undocumented_unsafe_blocks.rs:246:5
+   |
+LL |     let _ = unsafe {};
+   |     ^^^^^^^^^^^^^^^^^^
+   |
+help: consider adding a safety comment
+   |
+LL ~     // Safety: ...
+LL +     let _ = unsafe {};
+   |
+
+error: unsafe block missing a safety comment
+  --> $DIR/undocumented_unsafe_blocks.rs:256:8
+   |
+LL |     t!(unsafe {});
+   |        ^^^^^^^^^
+   |
+help: consider adding a safety comment
+   |
+LL ~     t!(// Safety: ...
+LL ~     unsafe {});
+   |
+
+error: unsafe block in macro expansion missing a safety comment
+  --> $DIR/undocumented_unsafe_blocks.rs:262:13
+   |
+LL |             unsafe {}
+   |             ^^^^^^^^^
+...
+LL |     t!();
+   |     ----- in this macro invocation
+   |
+   = help: consider adding a safety comment in the macro definition
+   = note: this error originates in the macro `t` (in Nightly builds, run with -Z macro-backtrace for more info)
+
+error: unsafe block missing a safety comment
+  --> $DIR/undocumented_unsafe_blocks.rs:270:5
+   |
+LL |     unsafe {} // Safety:
+   |     ^^^^^^^^^
+   |
+help: consider adding a safety comment
+   |
+LL ~     // Safety: ...
+LL ~     unsafe {} // Safety:
+   |
+
+error: unsafe block missing a safety comment
+  --> $DIR/undocumented_unsafe_blocks.rs:274:5
+   |
+LL |     unsafe {
+   |     ^^^^^^^^
+   |
+help: consider adding a safety comment
+   |
+LL ~     // Safety: ...
+LL +     unsafe {
+   |
+
+error: unsafe block missing a safety comment
+  --> $DIR/undocumented_unsafe_blocks.rs:284:5
+   |
+LL |     unsafe {};
+   |     ^^^^^^^^^
+   |
+help: consider adding a safety comment
+   |
+LL ~     // Safety: ...
+LL ~     unsafe {};
+   |
+
+error: aborting due to 13 previous errors
+