Rename lint, use rust lexer, allow whitespace

Author: Serial-ATA

clippy_lints/src/lib.mods.rs

@@ -202,7 +202,7 @@ mod transmute;
 mod transmuting_null;
 mod try_err;
 mod types;
-mod undocumented_unsafe_block_safety;
+mod undocumented_unsafe_blocks;
 mod undropped_manually_drops;
 mod unicode;
 mod unit_return_expecting_ord;

clippy_lints/src/lib.register_lints.rs

@@ -462,7 +462,7 @@ store.register_lints(&[
     types::REDUNDANT_ALLOCATION,
     types::TYPE_COMPLEXITY,
     types::VEC_BOX,
-    undocumented_unsafe_block_safety::UNDOCUMENTED_UNSAFE_BLOCK_SAFETY,
+    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

@@ -56,7 +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(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

@@ -535,7 +535,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     store.register_late_pass(move || Box::new(feature_name::FeatureName));
     store.register_late_pass(move || Box::new(iter_not_returning_iterator::IterNotReturningIterator));
     store.register_late_pass(move || Box::new(if_then_panic::IfThenPanic));
-    store.register_late_pass(move || Box::new(undocumented_unsafe_block_safety::UndocumentedUnsafeBlockSafety::default()));
+    store.register_late_pass(move || Box::new(undocumented_unsafe_blocks::UndocumentedUnsafeBlocks::default()));
 }
 
 #[rustfmt::skip]

clippy_lints/src/undocumented_unsafe_block_safety.rs renamed to clippy_lints/src/undocumented_unsafe_blocks.rs

@@ -8,64 +8,56 @@ 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.
+    /// the block are 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.
+    /// Undocumented unsafe blocks can make it difficult to
+    /// read and maintain code, as well as uncover unsoundness
+    /// and bugs.
     ///
     /// ### Example
     /// ```rust
-    ///   unsafe {};
+    /// use std::ptr::NonNull;
+    /// let a = &mut 42;
     ///
-    ///   let _ = unsafe {};
+    /// let ptr = unsafe { NonNull::new_unchecked(a) };
     /// ```
     /// Use instead:
     /// ```rust
-    /// // Safety: ...
-    /// unsafe {}
+    /// use std::ptr::NonNull;
+    /// let a = &mut 42;
     ///
-    /// // Safety: ...
-    /// let _ = unsafe {};
+    /// // Safety: references are guaranteed to be non-null.
+    /// let ptr = unsafe { NonNull::new_unchecked(a) };
     /// ```
-    pub UNDOCUMENTED_UNSAFE_BLOCK_SAFETY,
+    pub UNDOCUMENTED_UNSAFE_BLOCKS,
     restriction,
-    "creating an unsafe block without explaining why it is or is not safe"
+    "creating an unsafe block without explaining why it is safe"
 }
 
-impl_lint_pass!(UndocumentedUnsafeBlockSafety => [UNDOCUMENTED_UNSAFE_BLOCK_SAFETY]);
+impl_lint_pass!(UndocumentedUnsafeBlocks => [UNDOCUMENTED_UNSAFE_BLOCKS]);
 
 #[derive(Default)]
-pub struct UndocumentedUnsafeBlockSafety {
+pub struct UndocumentedUnsafeBlocks {
     pub local: bool
 }
 
-#[derive(PartialEq)]
-enum CommentType {
-    Safety,
-    Unrelated,
-    None,
-}
-
-impl LateLintPass<'_> for UndocumentedUnsafeBlockSafety {
+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_BLOCK_SAFETY, block.hir_id);
+            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);
@@ -77,7 +69,7 @@ impl LateLintPass<'_> for UndocumentedUnsafeBlockSafety {
 
     fn check_local(&mut self, cx: &LateContext<'_>, local: &'_ Local<'_>) {
         if_chain! {
-            if !is_lint_allowed(cx, UNDOCUMENTED_UNSAFE_BLOCK_SAFETY, local.hir_id);
+            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;
@@ -101,7 +93,7 @@ fn find_candidate(cx: &LateContext<'_>, span: Span, enclosing_hir_id: HirId) {
 
     span_lint_and_sugg(
         cx,
-        UNDOCUMENTED_UNSAFE_BLOCK_SAFETY,
+        UNDOCUMENTED_UNSAFE_BLOCKS,
         span,
         "unsafe block missing a safety comment",
         "consider adding a safety comment",
@@ -123,76 +115,28 @@ fn block_has_safety_comment(tcx: TyCtxt<'_>, enclosing_hir_id: HirId, block_span
     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());
+    src_str.retain(|c| !c.is_whitespace());
 
-    let mut src_str_split = src_str.lines().rev();
+    let src_str_split = src_str.lines().rev().collect::<Vec<_>>();
+    let src_str = src_str_split.join("");
 
+    let mut pos = 0;
     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() {
+    for token in rustc_lexer::tokenize(&src_str) {
         match token.kind {
             TokenKind::LineComment { doc_style: None }
-            | TokenKind::BlockComment { doc_style: None, terminated: true } => return true,
-            _ => {},
+            | 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
+                }
+            },
+            _ => break
         }
+
+        pos += token.len;
     }
 
-    false
+    Some(found_safety_comment)
 }

tests/ui/undocumented_unsafe_block_safety.rs renamed to tests/ui/undocumented_unsafe_blocks.rs

@@ -1,4 +1,4 @@
-#![warn(clippy::undocumented_unsafe_block_safety)]
+#![warn(clippy::undocumented_unsafe_blocks)]
 
 // Valid comments
 
@@ -7,22 +7,37 @@ fn line_comment() {
     unsafe {}
 }
 
-fn line_comment_with_extras() {
-    // This is a description
+fn line_comment_newlines() {
     // Safety:
+
     unsafe {}
 }
 
-fn let_statement_line_comment() {
+fn line_comment_empty() {
     // Safety:
-    let _ = unsafe {};
+    //
+    //
+    //
+    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 {}
@@ -41,11 +56,6 @@ fn block_comment_terminator_same_line() {
     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
@@ -65,17 +75,36 @@ fn safety_with_prepended_text() {
     unsafe {}
 }
 
+fn nested_local() {
+    let _ = {
+        let _ = {
+            // Safety:
+            let _ = unsafe {};
+        };
+    };
+}
+
+fn local_line_comment() {
+    // Safety:
+    let _ = unsafe {};
+}
+
+fn local_block_comment() {
+    /* Safety: */
+    let _ = unsafe {};
+}
+
 // Invalid comments
 
 fn no_comment() {
     unsafe {}
 }
 
-fn let_statement_no_comment() {
+fn local_no_comment() {
     let _ = unsafe {};
 }
 
-fn let_statement_commented_block() {
+fn local_commented_block() {
     let _ =
         // Safety:
         unsafe {};
@@ -91,16 +120,12 @@ fn internal_comment() {
     }
 }
 
-fn line_comment_newlines() {
-    // Safety:
-
-    unsafe {}
-}
+fn interference() {
+    // Safety
 
-fn block_comment_newlines() {
-    /* Safety: */
+    let _ = 42;
 
-    unsafe {}
+    unsafe {};
 }
 
 fn main() {}