diff --git a/CHANGELOG.md b/CHANGELOG.md index ac745793dda5..a4ef7e7b11ae 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index f49b382c5ea3..a1c850d7efe9 100644 --- a/clippy_lints/src/lib.rs +++ b/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] diff --git a/clippy_lints/src/undocumented_unsafe_blocks.rs b/clippy_lints/src/undocumented_unsafe_blocks.rs new file mode 100644 index 000000000000..bc21409887d3 --- /dev/null +++ b/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>>, +} + +impl UndocumentedUnsafeBlocks { + fn safety_comments(&mut self, sm: &SourceMap, span: Span) -> Option<&mut Vec> { + 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> { + 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"); + } + } + } +} diff --git a/tests/ui/undocumented_unsafe_blocks.rs b/tests/ui/undocumented_unsafe_blocks.rs new file mode 100644 index 000000000000..29ad7f5f88ee --- /dev/null +++ b/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() {} diff --git a/tests/ui/undocumented_unsafe_blocks.stderr b/tests/ui/undocumented_unsafe_blocks.stderr new file mode 100644 index 000000000000..713f8a9d8304 --- /dev/null +++ b/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 +