Fixed the ICE and safety comments between attributes

- No longer using attr.span()
- ignoring attributes manually

Author: teofr

clippy_lints/src/undocumented_unsafe_blocks.rs

@@ -113,7 +113,7 @@ impl<'tcx> LateLintPass<'tcx> for UndocumentedUnsafeBlocks {
             && !block.span.in_external_macro(cx.tcx.sess.source_map())
             && !is_lint_allowed(cx, UNDOCUMENTED_UNSAFE_BLOCKS, block.hir_id)
             && !is_unsafe_from_proc_macro(cx, block.span)
-            && !block_has_safety_comment(cx, block.span)
+            && !block_has_safety_comment(cx, block.span, self.accept_comment_above_attributes)
             && !block_parents_have_safety_comment(
                 self.accept_comment_above_statement,
                 self.accept_comment_above_attributes,
@@ -408,46 +408,29 @@ fn block_parents_have_safety_comment(
     cx: &LateContext<'_>,
     id: HirId,
 ) -> bool {
-    let (span, hir_id) = match cx.tcx.parent_hir_node(id) {
-        Node::Expr(expr) if let Some(inner) = find_unsafe_block_parent_in_expr(cx, expr) => inner,
+    let span = match cx.tcx.parent_hir_node(id) {
+        Node::Expr(expr) if let Some((span, _)) = find_unsafe_block_parent_in_expr(cx, expr) => span,
         Node::Stmt(hir::Stmt {
             kind:
-                hir::StmtKind::Let(hir::LetStmt { span, hir_id, .. })
-                | hir::StmtKind::Expr(hir::Expr { span, hir_id, .. })
-                | hir::StmtKind::Semi(hir::Expr { span, hir_id, .. }),
+                hir::StmtKind::Let(hir::LetStmt { span, .. })
+                | hir::StmtKind::Expr(hir::Expr { span, .. })
+                | hir::StmtKind::Semi(hir::Expr { span, .. }),
             ..
         })
-        | Node::LetStmt(hir::LetStmt { span, hir_id, .. }) => (*span, *hir_id),
+        | Node::LetStmt(hir::LetStmt { span, .. }) => *span,
 
-        node if let Some((span, hir_id)) = span_and_hid_of_item_alike_node(&node)
+        node if let Some((span, _)) = span_and_hid_of_item_alike_node(&node)
             && is_const_or_static(&node) =>
         {
-            (span, hir_id)
+            span
         },
 
         _ => return false,
     };
     // if unsafe block is part of a let/const/static statement,
     // and accept_comment_above_statement is set to true
     // we accept the safety comment in the line the precedes this statement.
-    accept_comment_above_statement
-        && span_with_attrs_has_safety_comment(cx, span, hir_id, accept_comment_above_attributes)
-}
-
-/// Extends `span` to also include its attributes, then checks if that span has a safety comment.
-fn span_with_attrs_has_safety_comment(
-    cx: &LateContext<'_>,
-    span: Span,
-    hir_id: HirId,
-    accept_comment_above_attributes: bool,
-) -> bool {
-    let span = if accept_comment_above_attributes {
-        include_attrs_in_span(cx, hir_id, span)
-    } else {
-        span
-    };
-
-    span_has_safety_comment(cx, span)
+    accept_comment_above_statement && span_has_safety_comment(cx, span, accept_comment_above_attributes)
 }
 
 /// Checks if an expression is "branchy", e.g. loop, match/if/etc.
@@ -459,7 +442,7 @@ fn is_branchy(expr: &hir::Expr<'_>) -> bool {
 }
 
 /// Checks if the lines immediately preceding the block contain a safety comment.
-fn block_has_safety_comment(cx: &LateContext<'_>, span: Span) -> bool {
+fn block_has_safety_comment(cx: &LateContext<'_>, span: Span, accept_comment_above_attributes: bool) -> bool {
     // This intentionally ignores text before the start of a function so something like:
     // ```
     //     // SAFETY: reason
@@ -469,18 +452,9 @@ fn block_has_safety_comment(cx: &LateContext<'_>, span: Span) -> bool {
     // attributes and doc comments.
 
     matches!(
-        span_from_macro_expansion_has_safety_comment(cx, span),
+        span_from_macro_expansion_has_safety_comment(cx, span, accept_comment_above_attributes),
         HasSafetyComment::Yes(_)
-    ) || span_has_safety_comment(cx, span)
-}
-
-fn include_attrs_in_span(cx: &LateContext<'_>, hir_id: HirId, span: Span) -> Span {
-    span.to(cx.tcx.hir_attrs(hir_id).iter().fold(span, |acc, attr| {
-        if attr.is_doc_comment() {
-            return acc;
-        }
-        acc.to(attr.span())
-    }))
+    ) || span_has_safety_comment(cx, span, accept_comment_above_attributes)
 }
 
 #[derive(Debug)]
@@ -497,7 +471,7 @@ fn item_has_safety_comment(
     item: &hir::Item<'_>,
     accept_comment_above_attributes: bool,
 ) -> HasSafetyComment {
-    match span_from_macro_expansion_has_safety_comment(cx, item.span) {
+    match span_from_macro_expansion_has_safety_comment(cx, item.span, accept_comment_above_attributes) {
         HasSafetyComment::Maybe => (),
         has_safety_comment => return has_safety_comment,
     }
@@ -530,15 +504,10 @@ fn item_has_safety_comment(
         },
     };
 
-    let mut span = item.span;
-    if accept_comment_above_attributes {
-        span = include_attrs_in_span(cx, item.hir_id(), span);
-    }
-
     let source_map = cx.sess().source_map();
     // If the comment is in the first line of the file, there is no preceding line
     if let Some(comment_start) = comment_start
-        && let Ok(unsafe_line) = source_map.lookup_line(span.lo())
+        && let Ok(unsafe_line) = source_map.lookup_line(item.span.lo())
         && let Ok(comment_start_line) = source_map.lookup_line(comment_start.into())
         && let include_first_line_of_file = matches!(comment_start, CommentStartBeforeItem::Start)
         && (include_first_line_of_file || Arc::ptr_eq(&unsafe_line.sf, &comment_start_line.sf))
@@ -552,6 +521,7 @@ fn item_has_safety_comment(
                 &unsafe_line.sf.lines()
                     [(comment_start_line.line + usize::from(!include_first_line_of_file))..=unsafe_line.line],
                 unsafe_line.sf.start_pos,
+                accept_comment_above_attributes,
             ) {
                 Some(b) => HasSafetyComment::Yes(b),
                 None => HasSafetyComment::No,
@@ -569,7 +539,7 @@ fn stmt_has_safety_comment(
     hir_id: HirId,
     accept_comment_above_attributes: bool,
 ) -> HasSafetyComment {
-    match span_from_macro_expansion_has_safety_comment(cx, span) {
+    match span_from_macro_expansion_has_safety_comment(cx, span, accept_comment_above_attributes) {
         HasSafetyComment::Maybe => (),
         has_safety_comment => return has_safety_comment,
     }
@@ -583,26 +553,21 @@ fn stmt_has_safety_comment(
         _ => return HasSafetyComment::Maybe,
     };
 
-    let mut span = span;
-    if accept_comment_above_attributes {
-        span = include_attrs_in_span(cx, hir_id, span);
-    }
-
     let source_map = cx.sess().source_map();
     if let Some(comment_start) = comment_start
         && let Ok(unsafe_line) = source_map.lookup_line(span.lo())
         && let Ok(comment_start_line) = source_map.lookup_line(comment_start)
         && Arc::ptr_eq(&unsafe_line.sf, &comment_start_line.sf)
         && let Some(src) = unsafe_line.sf.src.as_deref()
     {
-        // dbg!(src);
         return if comment_start_line.line >= unsafe_line.line {
             HasSafetyComment::No
         } else {
             match text_has_safety_comment(
                 src,
                 &unsafe_line.sf.lines()[comment_start_line.line + 1..=unsafe_line.line],
                 unsafe_line.sf.start_pos,
+                accept_comment_above_attributes,
             ) {
                 Some(b) => HasSafetyComment::Yes(b),
                 None => HasSafetyComment::No,
@@ -659,7 +624,11 @@ fn comment_start_before_item_in_mod(
     })
 }
 
-fn span_from_macro_expansion_has_safety_comment(cx: &LateContext<'_>, span: Span) -> HasSafetyComment {
+fn span_from_macro_expansion_has_safety_comment(
+    cx: &LateContext<'_>,
+    span: Span,
+    accept_comment_above_attributes: bool,
+) -> HasSafetyComment {
     let source_map = cx.sess().source_map();
     let ctxt = span.ctxt();
     if ctxt == SyntaxContext::root() {
@@ -679,6 +648,7 @@ fn span_from_macro_expansion_has_safety_comment(cx: &LateContext<'_>, span: Span
                 src,
                 &unsafe_line.sf.lines()[macro_line.line + 1..=unsafe_line.line],
                 unsafe_line.sf.start_pos,
+                accept_comment_above_attributes,
             ) {
                 Some(b) => HasSafetyComment::Yes(b),
                 None => HasSafetyComment::No,
@@ -725,7 +695,7 @@ fn get_body_search_span(cx: &LateContext<'_>) -> Option<Span> {
     None
 }
 
-fn span_has_safety_comment(cx: &LateContext<'_>, span: Span) -> bool {
+fn span_has_safety_comment(cx: &LateContext<'_>, span: Span, accept_comment_above_attributes: bool) -> bool {
     let source_map = cx.sess().source_map();
     let ctxt = span.ctxt();
     if ctxt.is_root()
@@ -745,6 +715,7 @@ fn span_has_safety_comment(cx: &LateContext<'_>, span: Span) -> bool {
                     src,
                     &unsafe_line.sf.lines()[body_line.line + 1..=unsafe_line.line],
                     unsafe_line.sf.start_pos,
+                    accept_comment_above_attributes,
                 )
                 .is_some()
         } else {
@@ -757,7 +728,14 @@ fn span_has_safety_comment(cx: &LateContext<'_>, span: Span) -> bool {
 }
 
 /// Checks if the given text has a safety comment for the immediately proceeding line.
-fn text_has_safety_comment(src: &str, line_starts: &[RelativeBytePos], start_pos: BytePos) -> Option<BytePos> {
+/// 
+/// If `accept_comment_above_attributes` is true, it will ignore attributes inbetween blocks of comments
+fn text_has_safety_comment(
+    src: &str,
+    line_starts: &[RelativeBytePos],
+    start_pos: BytePos,
+    accept_comment_above_attributes: bool,
+) -> Option<BytePos> {
     let mut lines = line_starts
         .array_windows::<2>()
         .rev()
@@ -768,7 +746,7 @@ fn text_has_safety_comment(src: &str, line_starts: &[RelativeBytePos], start_pos
             let trimmed = text.trim_start();
             Some((start + (text.len() - trimmed.len()), trimmed))
         })
-        .filter(|(_, text)| !text.is_empty());
+        .filter(|(_, text)| !(text.is_empty() || (accept_comment_above_attributes && is_attribute(text))));
 
     let (line_start, line) = lines.next()?;
     let mut in_codeblock = false;
@@ -812,6 +790,10 @@ fn text_has_safety_comment(src: &str, line_starts: &[RelativeBytePos], start_pos
     }
 }
 
+fn is_attribute(text: &str) -> bool {
+    (text.starts_with("#[") || text.starts_with("#![")) && text.trim_end().ends_with(']')
+}
+
 fn span_and_hid_of_item_alike_node(node: &Node<'_>) -> Option<(Span, HirId)> {
     match node {
         Node::Item(item) => Some((item.span, item.owner_id.into())),

tests/ui-toml/undocumented_unsafe_blocks/undocumented_unsafe_blocks.default.stderr

@@ -390,5 +390,29 @@ help: consider removing the safety comment
 LL |     // SAFETY: ...
    |     ^^^^^^^^^^^^^^
 
-error: aborting due to 43 previous errors
+error: module has unnecessary safety comment
+  --> tests/ui-toml/undocumented_unsafe_blocks/undocumented_unsafe_blocks.rs:750:5
+   |
+LL |     mod z {}
+   |     ^^^^^^^^
+   |
+help: consider removing the safety comment
+  --> tests/ui-toml/undocumented_unsafe_blocks/undocumented_unsafe_blocks.rs:749:5
+   |
+LL |     // SAFETY: ...
+   |     ^^^^^^^^^^^^^^
+
+error: module has unnecessary safety comment
+  --> tests/ui-toml/undocumented_unsafe_blocks/undocumented_unsafe_blocks.rs:758:5
+   |
+LL |     mod y {}
+   |     ^^^^^^^^
+   |
+help: consider removing the safety comment
+  --> tests/ui-toml/undocumented_unsafe_blocks/undocumented_unsafe_blocks.rs:756:5
+   |
+LL |     // SAFETY: ...
+   |     ^^^^^^^^^^^^^^
+
+error: aborting due to 45 previous errors
 

tests/ui-toml/undocumented_unsafe_blocks/undocumented_unsafe_blocks.rs

@@ -748,7 +748,15 @@ mod issue_14555 {
     #[doc(hidden)]
     // SAFETY: ...
     mod z {}
-    //~[disabled]^ unnecessary_safety_comment
+    //~^ unnecessary_safety_comment
+}
+
+mod issue_todo {
+    #[must_use]
+    // SAFETY: ...
+    #[doc(hidden)]
+    mod y {}
+    //~[default]^ unnecessary_safety_comment
 }
 
 fn main() {}

tests/ui/crashes/ice-15684.rs

@@ -0,0 +1,10 @@
+#![warn(clippy::unnecessary_safety_comment)]
+
+fn foo() -> i32 {
+    // SAFETY: fail ONLY if `accept-comment-above-attribute = false`
+    #[must_use]
+    return 33;
+    //~^ unnecessary_safety_comment
+}
+
+fn main() {}

tests/ui/crashes/ice-15684.stderr

@@ -0,0 +1,16 @@
+error: statement has unnecessary safety comment
+  --> tests/ui/crashes/ice-15684.rs:6:5
+   |
+LL |     return 33;
+   |     ^^^^^^^^^^
+   |
+help: consider removing the safety comment
+  --> tests/ui/crashes/ice-15684.rs:4:5
+   |
+LL |     // SAFETY: fail ONLY if `accept-comment-above-attribute = false`
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   = note: `-D clippy::unnecessary-safety-comment` implied by `-D warnings`
+   = help: to override `-D warnings` add `#[allow(clippy::unnecessary_safety_comment)]`
+
+error: aborting due to 1 previous error
+