From a116b9bdba6c3c00f4544a937af690bd462ca428 Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Mon, 14 Nov 2022 13:42:47 +0100 Subject: [PATCH 1/5] Lint unnecessary safety comments on items --- CHANGELOG.md | 1 + clippy_lints/src/declared_lints.rs | 1 + .../src/undocumented_unsafe_blocks.rs | 240 +++++++++++++----- tests/ui/undocumented_unsafe_blocks.rs | 2 +- tests/ui/undocumented_unsafe_blocks.stderr | 15 +- 5 files changed, 200 insertions(+), 59 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5b6b12c623af..23912bb3ed6b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4451,6 +4451,7 @@ Released 2018-09-13 [`unnecessary_mut_passed`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_mut_passed [`unnecessary_operation`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_operation [`unnecessary_owned_empty_strings`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_owned_empty_strings +[`unnecessary_safety_comment`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_safety_comment [`unnecessary_safety_doc`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_safety_doc [`unnecessary_self_imports`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_self_imports [`unnecessary_sort_by`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_sort_by diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index eb3210946f11..e4d76f07d6b4 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -584,6 +584,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[ crate::types::TYPE_COMPLEXITY_INFO, crate::types::VEC_BOX_INFO, crate::undocumented_unsafe_blocks::UNDOCUMENTED_UNSAFE_BLOCKS_INFO, + crate::undocumented_unsafe_blocks::UNNECESSARY_SAFETY_COMMENT_INFO, crate::unicode::INVISIBLE_CHARACTERS_INFO, crate::unicode::NON_ASCII_LITERAL_INFO, crate::unicode::UNICODE_NOT_NFC_INFO, diff --git a/clippy_lints/src/undocumented_unsafe_blocks.rs b/clippy_lints/src/undocumented_unsafe_blocks.rs index e8f15a444735..57ee2735aa03 100644 --- a/clippy_lints/src/undocumented_unsafe_blocks.rs +++ b/clippy_lints/src/undocumented_unsafe_blocks.rs @@ -59,8 +59,36 @@ declare_clippy_lint! { restriction, "creating an unsafe block without explaining why it is safe" } +declare_clippy_lint! { + /// ### What it does + /// Checks for `// SAFETY: ` comments on safe code. + /// + /// ### Why is this bad? + /// Safe code has no safety requirements, so there is no need to + /// describe safety invariants. + /// + /// ### Example + /// ```rust + /// use std::ptr::NonNull; + /// let a = &mut 42; + /// + /// // SAFETY: references are guaranteed to be non-null. + /// let ptr = NonNull::new(a).unwrap(); + /// ``` + /// Use instead: + /// ```rust + /// use std::ptr::NonNull; + /// let a = &mut 42; + /// + /// let ptr = NonNull::new(a).unwrap(); + /// ``` + #[clippy::version = "1.66.0"] + pub UNNECESSARY_SAFETY_COMMENT, + restriction, + "creating an unsafe block without explaining why it is safe" +} -declare_lint_pass!(UndocumentedUnsafeBlocks => [UNDOCUMENTED_UNSAFE_BLOCKS]); +declare_lint_pass!(UndocumentedUnsafeBlocks => [UNDOCUMENTED_UNSAFE_BLOCKS, UNNECESSARY_SAFETY_COMMENT]); impl LateLintPass<'_> for UndocumentedUnsafeBlocks { fn check_block(&mut self, cx: &LateContext<'_>, block: &'_ Block<'_>) { @@ -90,28 +118,95 @@ impl LateLintPass<'_> for UndocumentedUnsafeBlocks { } fn check_item(&mut self, cx: &LateContext<'_>, item: &hir::Item<'_>) { - if let hir::ItemKind::Impl(imple) = item.kind - && imple.unsafety == hir::Unsafety::Unsafe - && !in_external_macro(cx.tcx.sess, item.span) - && !is_lint_allowed(cx, UNDOCUMENTED_UNSAFE_BLOCKS, item.hir_id()) - && !is_unsafe_from_proc_macro(cx, item.span) - && !item_has_safety_comment(cx, item) - { + if in_external_macro(cx.tcx.sess, item.span) { + return; + } + + let mk_spans = |pos: BytePos| { let source_map = cx.tcx.sess.source_map(); + let span = Span::new(pos, pos, SyntaxContext::root(), None); + let help_span = source_map.span_extend_to_next_char(span, '\n', true); let span = if source_map.is_multiline(item.span) { source_map.span_until_char(item.span, '\n') } else { item.span }; + (span, help_span) + }; - span_lint_and_help( - cx, - UNDOCUMENTED_UNSAFE_BLOCKS, - span, - "unsafe impl missing a safety comment", - None, - "consider adding a safety comment on the preceding line", - ); + let item_has_safety_comment = item_has_safety_comment(cx, item); + match (&item.kind, item_has_safety_comment) { + (hir::ItemKind::Impl(impl_), HasSafetyComment::No) if impl_.unsafety == hir::Unsafety::Unsafe => { + if !is_lint_allowed(cx, UNDOCUMENTED_UNSAFE_BLOCKS, item.hir_id()) + && !is_unsafe_from_proc_macro(cx, item.span) + { + let source_map = cx.tcx.sess.source_map(); + let span = if source_map.is_multiline(item.span) { + source_map.span_until_char(item.span, '\n') + } else { + item.span + }; + + span_lint_and_help( + cx, + UNDOCUMENTED_UNSAFE_BLOCKS, + span, + "unsafe impl missing a safety comment", + None, + "consider adding a safety comment on the preceding line", + ); + } + }, + (hir::ItemKind::Impl(impl_), HasSafetyComment::Yes(pos)) if impl_.unsafety == hir::Unsafety::Normal => { + if !is_lint_allowed(cx, UNNECESSARY_SAFETY_COMMENT, item.hir_id()) { + let (span, help_span) = mk_spans(pos); + + span_lint_and_help( + cx, + UNNECESSARY_SAFETY_COMMENT, + span, + "impl has unnecessary safety comment", + Some(help_span), + "consider removing the safety comment", + ); + } + }, + (hir::ItemKind::Impl(_), _) => {}, + (&hir::ItemKind::Const(.., body) | &hir::ItemKind::Static(.., body), HasSafetyComment::Yes(pos)) => { + if !is_lint_allowed(cx, UNNECESSARY_SAFETY_COMMENT, body.hir_id) { + let body = cx.tcx.hir().body(body); + if !matches!( + body.value.kind, hir::ExprKind::Block(block, _) + if block.rules == BlockCheckMode::UnsafeBlock(UnsafeSource::UserProvided) + ) { + let (span, help_span) = mk_spans(pos); + + span_lint_and_help( + cx, + UNNECESSARY_SAFETY_COMMENT, + span, + &format!("{} has unnecessary safety comment", item.kind.descr()), + Some(help_span), + "consider removing the safety comment", + ); + } + } + }, + (_, HasSafetyComment::Yes(pos)) => { + if !is_lint_allowed(cx, UNNECESSARY_SAFETY_COMMENT, item.hir_id()) { + let (span, help_span) = mk_spans(pos); + + span_lint_and_help( + cx, + UNNECESSARY_SAFETY_COMMENT, + span, + &format!("{} has unnecessary safety comment", item.kind.descr()), + Some(help_span), + "consider removing the safety comment", + ); + } + }, + _ => (), } } } @@ -170,28 +265,38 @@ fn block_has_safety_comment(cx: &LateContext<'_>, span: Span) -> bool { // won't work. This is to avoid dealing with where such a comment should be place relative to // attributes and doc comments. - span_from_macro_expansion_has_safety_comment(cx, span) || span_in_body_has_safety_comment(cx, span) + matches!( + span_from_macro_expansion_has_safety_comment(cx, span), + HasSafetyComment::Yes(_) + ) || span_in_body_has_safety_comment(cx, span) +} + +enum HasSafetyComment { + Yes(BytePos), + No, + Maybe, } /// Checks if the lines immediately preceding the item contain a safety comment. #[allow(clippy::collapsible_match)] -fn item_has_safety_comment(cx: &LateContext<'_>, item: &hir::Item<'_>) -> bool { - if span_from_macro_expansion_has_safety_comment(cx, item.span) { - return true; +fn item_has_safety_comment(cx: &LateContext<'_>, item: &hir::Item<'_>) -> HasSafetyComment { + match span_from_macro_expansion_has_safety_comment(cx, item.span) { + HasSafetyComment::Maybe => (), + has_safety_comment => return has_safety_comment, } if item.span.ctxt() == SyntaxContext::root() { if let Some(parent_node) = get_parent_node(cx.tcx, item.hir_id()) { let comment_start = match parent_node { Node::Crate(parent_mod) => { - comment_start_before_impl_in_mod(cx, parent_mod, parent_mod.spans.inner_span, item) + comment_start_before_item_in_mod(cx, parent_mod, parent_mod.spans.inner_span, item) }, Node::Item(parent_item) => { if let ItemKind::Mod(parent_mod) = &parent_item.kind { - comment_start_before_impl_in_mod(cx, parent_mod, parent_item.span, item) + comment_start_before_item_in_mod(cx, parent_mod, parent_item.span, item) } else { // Doesn't support impls in this position. Pretend a comment was found. - return true; + return HasSafetyComment::Maybe; } }, Node::Stmt(stmt) => { @@ -200,17 +305,17 @@ fn item_has_safety_comment(cx: &LateContext<'_>, item: &hir::Item<'_>) -> bool { Node::Block(block) => walk_span_to_context(block.span, SyntaxContext::root()).map(Span::lo), _ => { // Doesn't support impls in this position. Pretend a comment was found. - return true; + return HasSafetyComment::Maybe; }, } } else { // Problem getting the parent node. Pretend a comment was found. - return true; + return HasSafetyComment::Maybe; } }, _ => { // Doesn't support impls in this position. Pretend a comment was found. - return true; + return HasSafetyComment::Maybe; }, }; @@ -222,33 +327,40 @@ fn item_has_safety_comment(cx: &LateContext<'_>, item: &hir::Item<'_>) -> bool { && let Some(src) = unsafe_line.sf.src.as_deref() { unsafe_line.sf.lines(|lines| { - comment_start_line.line < unsafe_line.line && text_has_safety_comment( - src, - &lines[comment_start_line.line + 1..=unsafe_line.line], - unsafe_line.sf.start_pos.to_usize(), - ) + if comment_start_line.line >= unsafe_line.line { + HasSafetyComment::No + } else { + match text_has_safety_comment( + src, + &lines[comment_start_line.line + 1..=unsafe_line.line], + unsafe_line.sf.start_pos.to_usize(), + ) { + Some(b) => HasSafetyComment::Yes(b), + None => HasSafetyComment::No, + } + } }) } else { // Problem getting source text. Pretend a comment was found. - true + HasSafetyComment::Maybe } } else { // No parent node. Pretend a comment was found. - true + HasSafetyComment::Maybe } } else { - false + HasSafetyComment::No } } -fn comment_start_before_impl_in_mod( +fn comment_start_before_item_in_mod( cx: &LateContext<'_>, parent_mod: &hir::Mod<'_>, parent_mod_span: Span, - imple: &hir::Item<'_>, + item: &hir::Item<'_>, ) -> Option { parent_mod.item_ids.iter().enumerate().find_map(|(idx, item_id)| { - if *item_id == imple.item_id() { + if *item_id == item.item_id() { if idx == 0 { // mod A { /* comment */ unsafe impl T {} ... } // ^------------------------------------------^ returns the start of this span @@ -270,11 +382,11 @@ fn comment_start_before_impl_in_mod( }) } -fn span_from_macro_expansion_has_safety_comment(cx: &LateContext<'_>, span: Span) -> bool { +fn span_from_macro_expansion_has_safety_comment(cx: &LateContext<'_>, span: Span) -> HasSafetyComment { let source_map = cx.sess().source_map(); let ctxt = span.ctxt(); if ctxt == SyntaxContext::root() { - false + HasSafetyComment::Maybe } else { // From a macro expansion. Get the text from the start of the macro declaration to start of the // unsafe block. @@ -286,15 +398,22 @@ fn span_from_macro_expansion_has_safety_comment(cx: &LateContext<'_>, span: Span && let Some(src) = unsafe_line.sf.src.as_deref() { unsafe_line.sf.lines(|lines| { - macro_line.line < unsafe_line.line && text_has_safety_comment( - src, - &lines[macro_line.line + 1..=unsafe_line.line], - unsafe_line.sf.start_pos.to_usize(), - ) + if macro_line.line < unsafe_line.line { + match text_has_safety_comment( + src, + &lines[macro_line.line + 1..=unsafe_line.line], + unsafe_line.sf.start_pos.to_usize(), + ) { + Some(b) => HasSafetyComment::Yes(b), + None => HasSafetyComment::No, + } + } else { + HasSafetyComment::No + } }) } else { // Problem getting source text. Pretend a comment was found. - true + HasSafetyComment::Maybe } } } @@ -333,7 +452,7 @@ fn span_in_body_has_safety_comment(cx: &LateContext<'_>, span: Span) -> bool { src, &lines[body_line.line + 1..=unsafe_line.line], unsafe_line.sf.start_pos.to_usize(), - ) + ).is_some() }) } else { // Problem getting source text. Pretend a comment was found. @@ -345,30 +464,34 @@ fn span_in_body_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: &[BytePos], offset: usize) -> bool { +fn text_has_safety_comment(src: &str, line_starts: &[BytePos], offset: usize) -> Option { let mut lines = line_starts .array_windows::<2>() .rev() .map_while(|[start, end]| { let start = start.to_usize() - offset; let end = end.to_usize() - offset; - src.get(start..end).map(|text| (start, text.trim_start())) + let text = src.get(start..end)?; + let trimmed = text.trim_start(); + Some((start + (text.len() - trimmed.len()), trimmed)) }) .filter(|(_, text)| !text.is_empty()); let Some((line_start, line)) = lines.next() else { - return false; + return None; }; // Check for a sequence of line comments. if line.starts_with("//") { - let mut line = line; + let (mut line, mut line_start) = (line, line_start); loop { if line.to_ascii_uppercase().contains("SAFETY:") { - return true; + return Some(BytePos( + u32::try_from(line_start).unwrap() + u32::try_from(offset).unwrap(), + )); } match lines.next() { - Some((_, x)) if x.starts_with("//") => line = x, - _ => return false, + Some((s, x)) if x.starts_with("//") => (line, line_start) = (x, s), + _ => return None, } } } @@ -377,16 +500,19 @@ fn text_has_safety_comment(src: &str, line_starts: &[BytePos], offset: usize) -> let (mut line_start, mut line) = (line_start, line); loop { if line.starts_with("/*") { - let src = src[line_start..line_starts.last().unwrap().to_usize() - offset].trim_start(); + let src = &src[line_start..line_starts.last().unwrap().to_usize() - offset]; let mut tokens = tokenize(src); - return src[..tokens.next().unwrap().len as usize] + return (src[..tokens.next().unwrap().len as usize] .to_ascii_uppercase() .contains("SAFETY:") - && tokens.all(|t| t.kind == TokenKind::Whitespace); + && tokens.all(|t| t.kind == TokenKind::Whitespace)) + .then_some(BytePos( + u32::try_from(line_start).unwrap() + u32::try_from(offset).unwrap(), + )); } match lines.next() { Some(x) => (line_start, line) = x, - None => return false, + None => return None, } } } diff --git a/tests/ui/undocumented_unsafe_blocks.rs b/tests/ui/undocumented_unsafe_blocks.rs index cbc6768033ec..c05eb447b2eb 100644 --- a/tests/ui/undocumented_unsafe_blocks.rs +++ b/tests/ui/undocumented_unsafe_blocks.rs @@ -1,6 +1,6 @@ // aux-build:proc_macro_unsafe.rs -#![warn(clippy::undocumented_unsafe_blocks)] +#![warn(clippy::undocumented_unsafe_blocks, clippy::unnecessary_safety_comment)] #![allow(clippy::let_unit_value, clippy::missing_safety_doc)] extern crate proc_macro_unsafe; diff --git a/tests/ui/undocumented_unsafe_blocks.stderr b/tests/ui/undocumented_unsafe_blocks.stderr index ba4de9806d17..4c6f6cd18c51 100644 --- a/tests/ui/undocumented_unsafe_blocks.stderr +++ b/tests/ui/undocumented_unsafe_blocks.stderr @@ -239,6 +239,19 @@ LL | unsafe impl TrailingComment for () {} // SAFETY: | = help: consider adding a safety comment on the preceding line +error: constant item has unnecessary safety comment + --> $DIR/undocumented_unsafe_blocks.rs:471:5 + | +LL | const BIG_NUMBER: i32 = 1000000; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: consider removing the safety comment + --> $DIR/undocumented_unsafe_blocks.rs:470:5 + | +LL | // SAFETY: + | ^^^^^^^^^^ + = note: `-D clippy::unnecessary-safety-comment` implied by `-D warnings` + error: unsafe impl missing a safety comment --> $DIR/undocumented_unsafe_blocks.rs:472:5 | @@ -287,5 +300,5 @@ LL | let bar = unsafe {}; | = help: consider adding a safety comment on the preceding line -error: aborting due to 34 previous errors +error: aborting due to 35 previous errors From b8c3f64cee8b07470572f274712dc9ab2634221b Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Thu, 17 Nov 2022 11:00:51 +0100 Subject: [PATCH 2/5] Add some more test cases for undocumented_unsafe_blocks --- .../src/undocumented_unsafe_blocks.rs | 7 +- tests/ui/undocumented_unsafe_blocks.rs | 13 ++++ tests/ui/undocumented_unsafe_blocks.stderr | 72 +++++++++++++++++-- 3 files changed, 85 insertions(+), 7 deletions(-) diff --git a/clippy_lints/src/undocumented_unsafe_blocks.rs b/clippy_lints/src/undocumented_unsafe_blocks.rs index 57ee2735aa03..080a481e87f5 100644 --- a/clippy_lints/src/undocumented_unsafe_blocks.rs +++ b/clippy_lints/src/undocumented_unsafe_blocks.rs @@ -82,7 +82,7 @@ declare_clippy_lint! { /// /// let ptr = NonNull::new(a).unwrap(); /// ``` - #[clippy::version = "1.66.0"] + #[clippy::version = "1.67.0"] pub UNNECESSARY_SAFETY_COMMENT, restriction, "creating an unsafe block without explaining why it is safe" @@ -136,6 +136,7 @@ impl LateLintPass<'_> for UndocumentedUnsafeBlocks { let item_has_safety_comment = item_has_safety_comment(cx, item); match (&item.kind, item_has_safety_comment) { + // lint unsafe impl without safety comment (hir::ItemKind::Impl(impl_), HasSafetyComment::No) if impl_.unsafety == hir::Unsafety::Unsafe => { if !is_lint_allowed(cx, UNDOCUMENTED_UNSAFE_BLOCKS, item.hir_id()) && !is_unsafe_from_proc_macro(cx, item.span) @@ -157,6 +158,7 @@ impl LateLintPass<'_> for UndocumentedUnsafeBlocks { ); } }, + // lint safe impl with unnecessary safety comment (hir::ItemKind::Impl(impl_), HasSafetyComment::Yes(pos)) if impl_.unsafety == hir::Unsafety::Normal => { if !is_lint_allowed(cx, UNNECESSARY_SAFETY_COMMENT, item.hir_id()) { let (span, help_span) = mk_spans(pos); @@ -172,6 +174,7 @@ impl LateLintPass<'_> for UndocumentedUnsafeBlocks { } }, (hir::ItemKind::Impl(_), _) => {}, + // const and static items only need a safety comment if their body is an unsafe block, lint otherwise (&hir::ItemKind::Const(.., body) | &hir::ItemKind::Static(.., body), HasSafetyComment::Yes(pos)) => { if !is_lint_allowed(cx, UNNECESSARY_SAFETY_COMMENT, body.hir_id) { let body = cx.tcx.hir().body(body); @@ -192,6 +195,8 @@ impl LateLintPass<'_> for UndocumentedUnsafeBlocks { } } }, + // Aside from unsafe impls and consts/statics with an unsafe block, items in general + // do not have safety invariants that need to be documented, so lint those. (_, HasSafetyComment::Yes(pos)) => { if !is_lint_allowed(cx, UNNECESSARY_SAFETY_COMMENT, item.hir_id()) { let (span, help_span) = mk_spans(pos); diff --git a/tests/ui/undocumented_unsafe_blocks.rs b/tests/ui/undocumented_unsafe_blocks.rs index c05eb447b2eb..f68e0b4915ea 100644 --- a/tests/ui/undocumented_unsafe_blocks.rs +++ b/tests/ui/undocumented_unsafe_blocks.rs @@ -472,6 +472,19 @@ mod unsafe_impl_invalid_comment { unsafe impl Interference for () {} } +mod unsafe_items_invalid_comment { + // SAFETY: + const CONST: u32 = 0; + // SAFETY: + static STATIC: u32 = 0; + // SAFETY: + struct Struct; + // SAFETY: + enum Enum {} + // SAFETY: + mod module {} +} + unsafe trait ImplInFn {} fn impl_in_fn() { diff --git a/tests/ui/undocumented_unsafe_blocks.stderr b/tests/ui/undocumented_unsafe_blocks.stderr index 4c6f6cd18c51..becad4f61a92 100644 --- a/tests/ui/undocumented_unsafe_blocks.stderr +++ b/tests/ui/undocumented_unsafe_blocks.stderr @@ -260,16 +260,76 @@ LL | unsafe impl Interference for () {} | = help: consider adding a safety comment on the preceding line -error: unsafe impl missing a safety comment +error: constant item has unnecessary safety comment + --> $DIR/undocumented_unsafe_blocks.rs:477:5 + | +LL | const CONST: u32 = 0; + | ^^^^^^^^^^^^^^^^^^^^^ + | +help: consider removing the safety comment + --> $DIR/undocumented_unsafe_blocks.rs:476:5 + | +LL | // SAFETY: + | ^^^^^^^^^^ + +error: static item has unnecessary safety comment --> $DIR/undocumented_unsafe_blocks.rs:479:5 | +LL | static STATIC: u32 = 0; + | ^^^^^^^^^^^^^^^^^^^^^^^ + | +help: consider removing the safety comment + --> $DIR/undocumented_unsafe_blocks.rs:478:5 + | +LL | // SAFETY: + | ^^^^^^^^^^ + +error: struct has unnecessary safety comment + --> $DIR/undocumented_unsafe_blocks.rs:481:5 + | +LL | struct Struct; + | ^^^^^^^^^^^^^^ + | +help: consider removing the safety comment + --> $DIR/undocumented_unsafe_blocks.rs:480:5 + | +LL | // SAFETY: + | ^^^^^^^^^^ + +error: enum has unnecessary safety comment + --> $DIR/undocumented_unsafe_blocks.rs:483:5 + | +LL | enum Enum {} + | ^^^^^^^^^^^^ + | +help: consider removing the safety comment + --> $DIR/undocumented_unsafe_blocks.rs:482:5 + | +LL | // SAFETY: + | ^^^^^^^^^^ + +error: module has unnecessary safety comment + --> $DIR/undocumented_unsafe_blocks.rs:485:5 + | +LL | mod module {} + | ^^^^^^^^^^^^^ + | +help: consider removing the safety comment + --> $DIR/undocumented_unsafe_blocks.rs:484:5 + | +LL | // SAFETY: + | ^^^^^^^^^^ + +error: unsafe impl missing a safety comment + --> $DIR/undocumented_unsafe_blocks.rs:492:5 + | LL | unsafe impl ImplInFn for () {} | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = help: consider adding a safety comment on the preceding line error: unsafe impl missing a safety comment - --> $DIR/undocumented_unsafe_blocks.rs:488:1 + --> $DIR/undocumented_unsafe_blocks.rs:501:1 | LL | unsafe impl CrateRoot for () {} | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -277,7 +337,7 @@ LL | unsafe impl CrateRoot for () {} = help: consider adding a safety comment on the preceding line error: unsafe block missing a safety comment - --> $DIR/undocumented_unsafe_blocks.rs:498:9 + --> $DIR/undocumented_unsafe_blocks.rs:511:9 | LL | unsafe {}; | ^^^^^^^^^ @@ -285,7 +345,7 @@ LL | unsafe {}; = help: consider adding a safety comment on the preceding line error: unsafe block missing a safety comment - --> $DIR/undocumented_unsafe_blocks.rs:502:12 + --> $DIR/undocumented_unsafe_blocks.rs:515:12 | LL | if unsafe { true } { | ^^^^^^^^^^^^^^^ @@ -293,12 +353,12 @@ LL | if unsafe { true } { = help: consider adding a safety comment on the preceding line error: unsafe block missing a safety comment - --> $DIR/undocumented_unsafe_blocks.rs:505:23 + --> $DIR/undocumented_unsafe_blocks.rs:518:23 | LL | let bar = unsafe {}; | ^^^^^^^^^ | = help: consider adding a safety comment on the preceding line -error: aborting due to 35 previous errors +error: aborting due to 40 previous errors From 4fa57575302a5931517590daf8df2ef8a93f7a7b Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Thu, 17 Nov 2022 15:14:03 +0100 Subject: [PATCH 3/5] Lint unnecessary safety comments on statements and block tail expressions --- .../src/undocumented_unsafe_blocks.rs | 126 +++++++++++++++++- clippy_utils/src/visitors.rs | 12 +- tests/ui/undocumented_unsafe_blocks.rs | 31 +++++ tests/ui/undocumented_unsafe_blocks.stderr | 60 ++++++++- 4 files changed, 220 insertions(+), 9 deletions(-) diff --git a/clippy_lints/src/undocumented_unsafe_blocks.rs b/clippy_lints/src/undocumented_unsafe_blocks.rs index 080a481e87f5..c60144df757c 100644 --- a/clippy_lints/src/undocumented_unsafe_blocks.rs +++ b/clippy_lints/src/undocumented_unsafe_blocks.rs @@ -1,6 +1,10 @@ +use std::ops::ControlFlow; + use clippy_utils::diagnostics::span_lint_and_help; use clippy_utils::source::walk_span_to_context; +use clippy_utils::visitors::{for_each_expr_with_closures, Descend}; use clippy_utils::{get_parent_node, is_lint_allowed}; +use hir::HirId; use rustc_data_structures::sync::Lrc; use rustc_hir as hir; use rustc_hir::{Block, BlockCheckMode, ItemKind, Node, UnsafeSource}; @@ -90,8 +94,8 @@ declare_clippy_lint! { declare_lint_pass!(UndocumentedUnsafeBlocks => [UNDOCUMENTED_UNSAFE_BLOCKS, UNNECESSARY_SAFETY_COMMENT]); -impl LateLintPass<'_> for UndocumentedUnsafeBlocks { - fn check_block(&mut self, cx: &LateContext<'_>, block: &'_ Block<'_>) { +impl<'tcx> LateLintPass<'tcx> for UndocumentedUnsafeBlocks { + fn check_block(&mut self, cx: &LateContext<'tcx>, block: &'tcx Block<'tcx>) { if block.rules == BlockCheckMode::UnsafeBlock(UnsafeSource::UserProvided) && !in_external_macro(cx.tcx.sess, block.span) && !is_lint_allowed(cx, UNDOCUMENTED_UNSAFE_BLOCKS, block.hir_id) @@ -115,6 +119,45 @@ impl LateLintPass<'_> for UndocumentedUnsafeBlocks { "consider adding a safety comment on the preceding line", ); } + + if let Some(tail) = block.expr + && !is_lint_allowed(cx, UNNECESSARY_SAFETY_COMMENT, tail.hir_id) + && !in_external_macro(cx.tcx.sess, tail.span) + && let HasSafetyComment::Yes(pos) = stmt_has_safety_comment(cx, tail.span, tail.hir_id) + && let Some(help_span) = expr_has_unnecessary_safety_comment(cx, tail, pos) + { + span_lint_and_help( + cx, + UNNECESSARY_SAFETY_COMMENT, + tail.span, + "expression has unnecessary safety comment", + Some(help_span), + "consider removing the safety comment", + ); + } + } + + fn check_stmt(&mut self, cx: &LateContext<'tcx>, stmt: &hir::Stmt<'tcx>) { + let expr = match stmt.kind { + hir::StmtKind::Local(&hir::Local { init: Some(expr), .. }) + | hir::StmtKind::Expr(expr) + | hir::StmtKind::Semi(expr) => expr, + _ => return, + }; + if !is_lint_allowed(cx, UNNECESSARY_SAFETY_COMMENT, stmt.hir_id) + && !in_external_macro(cx.tcx.sess, stmt.span) + && let HasSafetyComment::Yes(pos) = stmt_has_safety_comment(cx, stmt.span, stmt.hir_id) + && let Some(help_span) = expr_has_unnecessary_safety_comment(cx, expr, pos) + { + span_lint_and_help( + cx, + UNNECESSARY_SAFETY_COMMENT, + stmt.span, + "statement has unnecessary safety comment", + Some(help_span), + "consider removing the safety comment", + ); + } } fn check_item(&mut self, cx: &LateContext<'_>, item: &hir::Item<'_>) { @@ -216,6 +259,36 @@ impl LateLintPass<'_> for UndocumentedUnsafeBlocks { } } +fn expr_has_unnecessary_safety_comment<'tcx>( + cx: &LateContext<'tcx>, + expr: &'tcx hir::Expr<'tcx>, + comment_pos: BytePos, +) -> Option { + // this should roughly be the reverse of `block_parents_have_safety_comment` + if for_each_expr_with_closures(cx, expr, |expr| match expr.kind { + hir::ExprKind::Block( + Block { + rules: BlockCheckMode::UnsafeBlock(UnsafeSource::UserProvided), + .. + }, + _, + ) => ControlFlow::Break(()), + // statements will be handled by check_stmt itself again + hir::ExprKind::Block(..) => ControlFlow::Continue(Descend::No), + _ => ControlFlow::Continue(Descend::Yes), + }) + .is_some() + { + return None; + } + + let source_map = cx.tcx.sess.source_map(); + let span = Span::new(comment_pos, comment_pos, SyntaxContext::root(), None); + let help_span = source_map.span_extend_to_next_char(span, '\n', true); + + Some(help_span) +} + fn is_unsafe_from_proc_macro(cx: &LateContext<'_>, span: Span) -> bool { let source_map = cx.sess().source_map(); let file_pos = source_map.lookup_byte_offset(span.lo()); @@ -358,6 +431,55 @@ fn item_has_safety_comment(cx: &LateContext<'_>, item: &hir::Item<'_>) -> HasSaf } } +/// Checks if the lines immediately preceding the item contain a safety comment. +#[allow(clippy::collapsible_match)] +fn stmt_has_safety_comment(cx: &LateContext<'_>, span: Span, hir_id: HirId) -> HasSafetyComment { + match span_from_macro_expansion_has_safety_comment(cx, span) { + HasSafetyComment::Maybe => (), + has_safety_comment => return has_safety_comment, + } + + if span.ctxt() == SyntaxContext::root() { + if let Some(parent_node) = get_parent_node(cx.tcx, hir_id) { + let comment_start = match parent_node { + Node::Block(block) => walk_span_to_context(block.span, SyntaxContext::root()).map(Span::lo), + _ => return HasSafetyComment::Maybe, + }; + + 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) + && Lrc::ptr_eq(&unsafe_line.sf, &comment_start_line.sf) + && let Some(src) = unsafe_line.sf.src.as_deref() + { + unsafe_line.sf.lines(|lines| { + if comment_start_line.line >= unsafe_line.line { + HasSafetyComment::No + } else { + match text_has_safety_comment( + src, + &lines[comment_start_line.line + 1..=unsafe_line.line], + unsafe_line.sf.start_pos.to_usize(), + ) { + Some(b) => HasSafetyComment::Yes(b), + None => HasSafetyComment::No, + } + } + }) + } else { + // Problem getting source text. Pretend a comment was found. + HasSafetyComment::Maybe + } + } else { + // No parent node. Pretend a comment was found. + HasSafetyComment::Maybe + } + } else { + HasSafetyComment::No + } +} + fn comment_start_before_item_in_mod( cx: &LateContext<'_>, parent_mod: &hir::Mod<'_>, diff --git a/clippy_utils/src/visitors.rs b/clippy_utils/src/visitors.rs index d4294f18fd50..863fb60fcfca 100644 --- a/clippy_utils/src/visitors.rs +++ b/clippy_utils/src/visitors.rs @@ -170,22 +170,22 @@ where cb: F, } - struct WithStmtGuarg<'a, F> { + struct WithStmtGuard<'a, F> { val: &'a mut RetFinder, prev_in_stmt: bool, } impl RetFinder { - fn inside_stmt(&mut self, in_stmt: bool) -> WithStmtGuarg<'_, F> { + fn inside_stmt(&mut self, in_stmt: bool) -> WithStmtGuard<'_, F> { let prev_in_stmt = std::mem::replace(&mut self.in_stmt, in_stmt); - WithStmtGuarg { + WithStmtGuard { val: self, prev_in_stmt, } } } - impl std::ops::Deref for WithStmtGuarg<'_, F> { + impl std::ops::Deref for WithStmtGuard<'_, F> { type Target = RetFinder; fn deref(&self) -> &Self::Target { @@ -193,13 +193,13 @@ where } } - impl std::ops::DerefMut for WithStmtGuarg<'_, F> { + impl std::ops::DerefMut for WithStmtGuard<'_, F> { fn deref_mut(&mut self) -> &mut Self::Target { self.val } } - impl Drop for WithStmtGuarg<'_, F> { + impl Drop for WithStmtGuard<'_, F> { fn drop(&mut self) { self.val.in_stmt = self.prev_in_stmt; } diff --git a/tests/ui/undocumented_unsafe_blocks.rs b/tests/ui/undocumented_unsafe_blocks.rs index f68e0b4915ea..cb99ce0d4214 100644 --- a/tests/ui/undocumented_unsafe_blocks.rs +++ b/tests/ui/undocumented_unsafe_blocks.rs @@ -522,4 +522,35 @@ fn issue_9142() { }; } +mod unnecessary_from_macro { + trait T {} + + macro_rules! no_safety_comment { + ($t:ty) => { + impl T for $t {} + }; + } + + // FIXME: This is not caught + // Safety: unnecessary + no_safety_comment!(()); + + macro_rules! with_safety_comment { + ($t:ty) => { + // Safety: unnecessary + impl T for $t {} + }; + } + + with_safety_comment!(i32); +} + +fn unnecessary_on_stmt_and_expr() -> u32 { + // SAFETY: unnecessary + let num = 42; + + // SAFETY: unnecessary + 24 +} + fn main() {} diff --git a/tests/ui/undocumented_unsafe_blocks.stderr b/tests/ui/undocumented_unsafe_blocks.stderr index becad4f61a92..919fd51351cb 100644 --- a/tests/ui/undocumented_unsafe_blocks.stderr +++ b/tests/ui/undocumented_unsafe_blocks.stderr @@ -344,6 +344,24 @@ LL | unsafe {}; | = help: consider adding a safety comment on the preceding line +error: statement has unnecessary safety comment + --> $DIR/undocumented_unsafe_blocks.rs:514:5 + | +LL | / let _ = { +LL | | if unsafe { true } { +LL | | todo!(); +LL | | } else { +... | +LL | | } +LL | | }; + | |______^ + | +help: consider removing the safety comment + --> $DIR/undocumented_unsafe_blocks.rs:513:5 + | +LL | // SAFETY: this is more than one level away, so it should warn + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + error: unsafe block missing a safety comment --> $DIR/undocumented_unsafe_blocks.rs:515:12 | @@ -360,5 +378,45 @@ LL | let bar = unsafe {}; | = help: consider adding a safety comment on the preceding line -error: aborting due to 40 previous errors +error: impl has unnecessary safety comment + --> $DIR/undocumented_unsafe_blocks.rs:541:13 + | +LL | impl T for $t {} + | ^^^^^^^^^^^^^^^^ +... +LL | with_safety_comment!(i32); + | ------------------------- in this macro invocation + | +help: consider removing the safety comment + --> $DIR/undocumented_unsafe_blocks.rs:540:13 + | +LL | // Safety: unnecessary + | ^^^^^^^^^^^^^^^^^^^^^^ + = note: this error originates in the macro `with_safety_comment` (in Nightly builds, run with -Z macro-backtrace for more info) + +error: expression has unnecessary safety comment + --> $DIR/undocumented_unsafe_blocks.rs:553:5 + | +LL | 24 + | ^^ + | +help: consider removing the safety comment + --> $DIR/undocumented_unsafe_blocks.rs:552:5 + | +LL | // SAFETY: unnecessary + | ^^^^^^^^^^^^^^^^^^^^^^ + +error: statement has unnecessary safety comment + --> $DIR/undocumented_unsafe_blocks.rs:550:5 + | +LL | let num = 42; + | ^^^^^^^^^^^^^ + | +help: consider removing the safety comment + --> $DIR/undocumented_unsafe_blocks.rs:549:5 + | +LL | // SAFETY: unnecessary + | ^^^^^^^^^^^^^^^^^^^^^^ + +error: aborting due to 44 previous errors From 9c69e1cc893878987991d77b8fb54d1f6de29733 Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Thu, 17 Nov 2022 15:17:28 +0100 Subject: [PATCH 4/5] Simplify --- .../src/undocumented_unsafe_blocks.rs | 173 ++++++++---------- 1 file changed, 78 insertions(+), 95 deletions(-) diff --git a/clippy_lints/src/undocumented_unsafe_blocks.rs b/clippy_lints/src/undocumented_unsafe_blocks.rs index c60144df757c..d7e483423064 100644 --- a/clippy_lints/src/undocumented_unsafe_blocks.rs +++ b/clippy_lints/src/undocumented_unsafe_blocks.rs @@ -363,72 +363,60 @@ fn item_has_safety_comment(cx: &LateContext<'_>, item: &hir::Item<'_>) -> HasSaf has_safety_comment => return has_safety_comment, } - if item.span.ctxt() == SyntaxContext::root() { - if let Some(parent_node) = get_parent_node(cx.tcx, item.hir_id()) { - let comment_start = match parent_node { - Node::Crate(parent_mod) => { - comment_start_before_item_in_mod(cx, parent_mod, parent_mod.spans.inner_span, item) - }, - Node::Item(parent_item) => { - if let ItemKind::Mod(parent_mod) = &parent_item.kind { - comment_start_before_item_in_mod(cx, parent_mod, parent_item.span, item) - } else { - // Doesn't support impls in this position. Pretend a comment was found. - return HasSafetyComment::Maybe; - } - }, - Node::Stmt(stmt) => { - if let Some(stmt_parent) = get_parent_node(cx.tcx, stmt.hir_id) { - match stmt_parent { - Node::Block(block) => walk_span_to_context(block.span, SyntaxContext::root()).map(Span::lo), - _ => { - // Doesn't support impls in this position. Pretend a comment was found. - return HasSafetyComment::Maybe; - }, - } - } else { - // Problem getting the parent node. Pretend a comment was found. - return HasSafetyComment::Maybe; - } - }, - _ => { + if item.span.ctxt() != SyntaxContext::root() { + return HasSafetyComment::No; + } + if let Some(parent_node) = get_parent_node(cx.tcx, item.hir_id()) { + let comment_start = match parent_node { + Node::Crate(parent_mod) => { + comment_start_before_item_in_mod(cx, parent_mod, parent_mod.spans.inner_span, item) + }, + Node::Item(parent_item) => { + if let ItemKind::Mod(parent_mod) = &parent_item.kind { + comment_start_before_item_in_mod(cx, parent_mod, parent_item.span, item) + } else { // Doesn't support impls in this position. Pretend a comment was found. return HasSafetyComment::Maybe; - }, - }; + } + }, + Node::Stmt(stmt) => { + if let Some(Node::Block(block)) = get_parent_node(cx.tcx, stmt.hir_id) { + walk_span_to_context(block.span, SyntaxContext::root()).map(Span::lo) + } else { + // Problem getting the parent node. Pretend a comment was found. + return HasSafetyComment::Maybe; + } + }, + _ => { + // Doesn't support impls in this position. Pretend a comment was found. + return HasSafetyComment::Maybe; + }, + }; - let source_map = cx.sess().source_map(); - if let Some(comment_start) = comment_start - && let Ok(unsafe_line) = source_map.lookup_line(item.span.lo()) - && let Ok(comment_start_line) = source_map.lookup_line(comment_start) - && Lrc::ptr_eq(&unsafe_line.sf, &comment_start_line.sf) - && let Some(src) = unsafe_line.sf.src.as_deref() - { - unsafe_line.sf.lines(|lines| { - if comment_start_line.line >= unsafe_line.line { - HasSafetyComment::No - } else { - match text_has_safety_comment( - src, - &lines[comment_start_line.line + 1..=unsafe_line.line], - unsafe_line.sf.start_pos.to_usize(), - ) { - Some(b) => HasSafetyComment::Yes(b), - None => HasSafetyComment::No, - } + let source_map = cx.sess().source_map(); + if let Some(comment_start) = comment_start + && let Ok(unsafe_line) = source_map.lookup_line(item.span.lo()) + && let Ok(comment_start_line) = source_map.lookup_line(comment_start) + && Lrc::ptr_eq(&unsafe_line.sf, &comment_start_line.sf) + && let Some(src) = unsafe_line.sf.src.as_deref() + { + return unsafe_line.sf.lines(|lines| { + if comment_start_line.line >= unsafe_line.line { + HasSafetyComment::No + } else { + match text_has_safety_comment( + src, + &lines[comment_start_line.line + 1..=unsafe_line.line], + unsafe_line.sf.start_pos.to_usize(), + ) { + Some(b) => HasSafetyComment::Yes(b), + None => HasSafetyComment::No, } - }) - } else { - // Problem getting source text. Pretend a comment was found. - HasSafetyComment::Maybe - } - } else { - // No parent node. Pretend a comment was found. - HasSafetyComment::Maybe + } + }); } - } else { - HasSafetyComment::No } + HasSafetyComment::Maybe } /// Checks if the lines immediately preceding the item contain a safety comment. @@ -439,45 +427,40 @@ fn stmt_has_safety_comment(cx: &LateContext<'_>, span: Span, hir_id: HirId) -> H has_safety_comment => return has_safety_comment, } - if span.ctxt() == SyntaxContext::root() { - if let Some(parent_node) = get_parent_node(cx.tcx, hir_id) { - let comment_start = match parent_node { - Node::Block(block) => walk_span_to_context(block.span, SyntaxContext::root()).map(Span::lo), - _ => return HasSafetyComment::Maybe, - }; + if span.ctxt() != SyntaxContext::root() { + return HasSafetyComment::No; + } - 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) - && Lrc::ptr_eq(&unsafe_line.sf, &comment_start_line.sf) - && let Some(src) = unsafe_line.sf.src.as_deref() - { - unsafe_line.sf.lines(|lines| { - if comment_start_line.line >= unsafe_line.line { - HasSafetyComment::No - } else { - match text_has_safety_comment( - src, - &lines[comment_start_line.line + 1..=unsafe_line.line], - unsafe_line.sf.start_pos.to_usize(), - ) { - Some(b) => HasSafetyComment::Yes(b), - None => HasSafetyComment::No, - } + if let Some(parent_node) = get_parent_node(cx.tcx, hir_id) { + let comment_start = match parent_node { + Node::Block(block) => walk_span_to_context(block.span, SyntaxContext::root()).map(Span::lo), + _ => return HasSafetyComment::Maybe, + }; + + 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) + && Lrc::ptr_eq(&unsafe_line.sf, &comment_start_line.sf) + && let Some(src) = unsafe_line.sf.src.as_deref() + { + return unsafe_line.sf.lines(|lines| { + if comment_start_line.line >= unsafe_line.line { + HasSafetyComment::No + } else { + match text_has_safety_comment( + src, + &lines[comment_start_line.line + 1..=unsafe_line.line], + unsafe_line.sf.start_pos.to_usize(), + ) { + Some(b) => HasSafetyComment::Yes(b), + None => HasSafetyComment::No, } - }) - } else { - // Problem getting source text. Pretend a comment was found. - HasSafetyComment::Maybe - } - } else { - // No parent node. Pretend a comment was found. - HasSafetyComment::Maybe + } + }); } - } else { - HasSafetyComment::No } + HasSafetyComment::Maybe } fn comment_start_before_item_in_mod( From f96dd383188e9f24e5b401e664cca97b8bd73825 Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Thu, 24 Nov 2022 09:47:50 +0100 Subject: [PATCH 5/5] Address reviews --- .../src/undocumented_unsafe_blocks.rs | 9 +- tests/ui/undocumented_unsafe_blocks.rs | 44 ------- tests/ui/undocumented_unsafe_blocks.stderr | 116 ++---------------- tests/ui/unnecessary_safety_comment.rs | 51 ++++++++ tests/ui/unnecessary_safety_comment.stderr | 115 +++++++++++++++++ 5 files changed, 178 insertions(+), 157 deletions(-) create mode 100644 tests/ui/unnecessary_safety_comment.rs create mode 100644 tests/ui/unnecessary_safety_comment.stderr diff --git a/clippy_lints/src/undocumented_unsafe_blocks.rs b/clippy_lints/src/undocumented_unsafe_blocks.rs index d7e483423064..2e1b6d8d4ea7 100644 --- a/clippy_lints/src/undocumented_unsafe_blocks.rs +++ b/clippy_lints/src/undocumented_unsafe_blocks.rs @@ -89,7 +89,7 @@ declare_clippy_lint! { #[clippy::version = "1.67.0"] pub UNNECESSARY_SAFETY_COMMENT, restriction, - "creating an unsafe block without explaining why it is safe" + "annotating safe code with a safety comment" } declare_lint_pass!(UndocumentedUnsafeBlocks => [UNDOCUMENTED_UNSAFE_BLOCKS, UNNECESSARY_SAFETY_COMMENT]); @@ -138,12 +138,11 @@ impl<'tcx> LateLintPass<'tcx> for UndocumentedUnsafeBlocks { } fn check_stmt(&mut self, cx: &LateContext<'tcx>, stmt: &hir::Stmt<'tcx>) { - let expr = match stmt.kind { + let ( hir::StmtKind::Local(&hir::Local { init: Some(expr), .. }) | hir::StmtKind::Expr(expr) - | hir::StmtKind::Semi(expr) => expr, - _ => return, - }; + | hir::StmtKind::Semi(expr) + ) = stmt.kind else { return }; if !is_lint_allowed(cx, UNNECESSARY_SAFETY_COMMENT, stmt.hir_id) && !in_external_macro(cx.tcx.sess, stmt.span) && let HasSafetyComment::Yes(pos) = stmt_has_safety_comment(cx, stmt.span, stmt.hir_id) diff --git a/tests/ui/undocumented_unsafe_blocks.rs b/tests/ui/undocumented_unsafe_blocks.rs index cb99ce0d4214..c05eb447b2eb 100644 --- a/tests/ui/undocumented_unsafe_blocks.rs +++ b/tests/ui/undocumented_unsafe_blocks.rs @@ -472,19 +472,6 @@ mod unsafe_impl_invalid_comment { unsafe impl Interference for () {} } -mod unsafe_items_invalid_comment { - // SAFETY: - const CONST: u32 = 0; - // SAFETY: - static STATIC: u32 = 0; - // SAFETY: - struct Struct; - // SAFETY: - enum Enum {} - // SAFETY: - mod module {} -} - unsafe trait ImplInFn {} fn impl_in_fn() { @@ -522,35 +509,4 @@ fn issue_9142() { }; } -mod unnecessary_from_macro { - trait T {} - - macro_rules! no_safety_comment { - ($t:ty) => { - impl T for $t {} - }; - } - - // FIXME: This is not caught - // Safety: unnecessary - no_safety_comment!(()); - - macro_rules! with_safety_comment { - ($t:ty) => { - // Safety: unnecessary - impl T for $t {} - }; - } - - with_safety_comment!(i32); -} - -fn unnecessary_on_stmt_and_expr() -> u32 { - // SAFETY: unnecessary - let num = 42; - - // SAFETY: unnecessary - 24 -} - fn main() {} diff --git a/tests/ui/undocumented_unsafe_blocks.stderr b/tests/ui/undocumented_unsafe_blocks.stderr index 919fd51351cb..d1c1bb5ffeac 100644 --- a/tests/ui/undocumented_unsafe_blocks.stderr +++ b/tests/ui/undocumented_unsafe_blocks.stderr @@ -260,68 +260,8 @@ LL | unsafe impl Interference for () {} | = help: consider adding a safety comment on the preceding line -error: constant item has unnecessary safety comment - --> $DIR/undocumented_unsafe_blocks.rs:477:5 - | -LL | const CONST: u32 = 0; - | ^^^^^^^^^^^^^^^^^^^^^ - | -help: consider removing the safety comment - --> $DIR/undocumented_unsafe_blocks.rs:476:5 - | -LL | // SAFETY: - | ^^^^^^^^^^ - -error: static item has unnecessary safety comment - --> $DIR/undocumented_unsafe_blocks.rs:479:5 - | -LL | static STATIC: u32 = 0; - | ^^^^^^^^^^^^^^^^^^^^^^^ - | -help: consider removing the safety comment - --> $DIR/undocumented_unsafe_blocks.rs:478:5 - | -LL | // SAFETY: - | ^^^^^^^^^^ - -error: struct has unnecessary safety comment - --> $DIR/undocumented_unsafe_blocks.rs:481:5 - | -LL | struct Struct; - | ^^^^^^^^^^^^^^ - | -help: consider removing the safety comment - --> $DIR/undocumented_unsafe_blocks.rs:480:5 - | -LL | // SAFETY: - | ^^^^^^^^^^ - -error: enum has unnecessary safety comment - --> $DIR/undocumented_unsafe_blocks.rs:483:5 - | -LL | enum Enum {} - | ^^^^^^^^^^^^ - | -help: consider removing the safety comment - --> $DIR/undocumented_unsafe_blocks.rs:482:5 - | -LL | // SAFETY: - | ^^^^^^^^^^ - -error: module has unnecessary safety comment - --> $DIR/undocumented_unsafe_blocks.rs:485:5 - | -LL | mod module {} - | ^^^^^^^^^^^^^ - | -help: consider removing the safety comment - --> $DIR/undocumented_unsafe_blocks.rs:484:5 - | -LL | // SAFETY: - | ^^^^^^^^^^ - error: unsafe impl missing a safety comment - --> $DIR/undocumented_unsafe_blocks.rs:492:5 + --> $DIR/undocumented_unsafe_blocks.rs:479:5 | LL | unsafe impl ImplInFn for () {} | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -329,7 +269,7 @@ LL | unsafe impl ImplInFn for () {} = help: consider adding a safety comment on the preceding line error: unsafe impl missing a safety comment - --> $DIR/undocumented_unsafe_blocks.rs:501:1 + --> $DIR/undocumented_unsafe_blocks.rs:488:1 | LL | unsafe impl CrateRoot for () {} | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -337,7 +277,7 @@ LL | unsafe impl CrateRoot for () {} = help: consider adding a safety comment on the preceding line error: unsafe block missing a safety comment - --> $DIR/undocumented_unsafe_blocks.rs:511:9 + --> $DIR/undocumented_unsafe_blocks.rs:498:9 | LL | unsafe {}; | ^^^^^^^^^ @@ -345,7 +285,7 @@ LL | unsafe {}; = help: consider adding a safety comment on the preceding line error: statement has unnecessary safety comment - --> $DIR/undocumented_unsafe_blocks.rs:514:5 + --> $DIR/undocumented_unsafe_blocks.rs:501:5 | LL | / let _ = { LL | | if unsafe { true } { @@ -357,13 +297,13 @@ LL | | }; | |______^ | help: consider removing the safety comment - --> $DIR/undocumented_unsafe_blocks.rs:513:5 + --> $DIR/undocumented_unsafe_blocks.rs:500:5 | LL | // SAFETY: this is more than one level away, so it should warn | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: unsafe block missing a safety comment - --> $DIR/undocumented_unsafe_blocks.rs:515:12 + --> $DIR/undocumented_unsafe_blocks.rs:502:12 | LL | if unsafe { true } { | ^^^^^^^^^^^^^^^ @@ -371,52 +311,12 @@ LL | if unsafe { true } { = help: consider adding a safety comment on the preceding line error: unsafe block missing a safety comment - --> $DIR/undocumented_unsafe_blocks.rs:518:23 + --> $DIR/undocumented_unsafe_blocks.rs:505:23 | LL | let bar = unsafe {}; | ^^^^^^^^^ | = help: consider adding a safety comment on the preceding line -error: impl has unnecessary safety comment - --> $DIR/undocumented_unsafe_blocks.rs:541:13 - | -LL | impl T for $t {} - | ^^^^^^^^^^^^^^^^ -... -LL | with_safety_comment!(i32); - | ------------------------- in this macro invocation - | -help: consider removing the safety comment - --> $DIR/undocumented_unsafe_blocks.rs:540:13 - | -LL | // Safety: unnecessary - | ^^^^^^^^^^^^^^^^^^^^^^ - = note: this error originates in the macro `with_safety_comment` (in Nightly builds, run with -Z macro-backtrace for more info) - -error: expression has unnecessary safety comment - --> $DIR/undocumented_unsafe_blocks.rs:553:5 - | -LL | 24 - | ^^ - | -help: consider removing the safety comment - --> $DIR/undocumented_unsafe_blocks.rs:552:5 - | -LL | // SAFETY: unnecessary - | ^^^^^^^^^^^^^^^^^^^^^^ - -error: statement has unnecessary safety comment - --> $DIR/undocumented_unsafe_blocks.rs:550:5 - | -LL | let num = 42; - | ^^^^^^^^^^^^^ - | -help: consider removing the safety comment - --> $DIR/undocumented_unsafe_blocks.rs:549:5 - | -LL | // SAFETY: unnecessary - | ^^^^^^^^^^^^^^^^^^^^^^ - -error: aborting due to 44 previous errors +error: aborting due to 36 previous errors diff --git a/tests/ui/unnecessary_safety_comment.rs b/tests/ui/unnecessary_safety_comment.rs new file mode 100644 index 000000000000..7fefea7051d6 --- /dev/null +++ b/tests/ui/unnecessary_safety_comment.rs @@ -0,0 +1,51 @@ +#![warn(clippy::undocumented_unsafe_blocks, clippy::unnecessary_safety_comment)] +#![allow(clippy::let_unit_value, clippy::missing_safety_doc)] + +mod unsafe_items_invalid_comment { + // SAFETY: + const CONST: u32 = 0; + // SAFETY: + static STATIC: u32 = 0; + // SAFETY: + struct Struct; + // SAFETY: + enum Enum {} + // SAFETY: + mod module {} +} + +mod unnecessary_from_macro { + trait T {} + + macro_rules! no_safety_comment { + ($t:ty) => { + impl T for $t {} + }; + } + + // FIXME: This is not caught + // Safety: unnecessary + no_safety_comment!(()); + + macro_rules! with_safety_comment { + ($t:ty) => { + // Safety: unnecessary + impl T for $t {} + }; + } + + with_safety_comment!(i32); +} + +fn unnecessary_on_stmt_and_expr() -> u32 { + // SAFETY: unnecessary + let num = 42; + + // SAFETY: unnecessary + if num > 24 {} + + // SAFETY: unnecessary + 24 +} + +fn main() {} diff --git a/tests/ui/unnecessary_safety_comment.stderr b/tests/ui/unnecessary_safety_comment.stderr new file mode 100644 index 000000000000..7b2af67d64c7 --- /dev/null +++ b/tests/ui/unnecessary_safety_comment.stderr @@ -0,0 +1,115 @@ +error: constant item has unnecessary safety comment + --> $DIR/unnecessary_safety_comment.rs:6:5 + | +LL | const CONST: u32 = 0; + | ^^^^^^^^^^^^^^^^^^^^^ + | +help: consider removing the safety comment + --> $DIR/unnecessary_safety_comment.rs:5:5 + | +LL | // SAFETY: + | ^^^^^^^^^^ + = note: `-D clippy::unnecessary-safety-comment` implied by `-D warnings` + +error: static item has unnecessary safety comment + --> $DIR/unnecessary_safety_comment.rs:8:5 + | +LL | static STATIC: u32 = 0; + | ^^^^^^^^^^^^^^^^^^^^^^^ + | +help: consider removing the safety comment + --> $DIR/unnecessary_safety_comment.rs:7:5 + | +LL | // SAFETY: + | ^^^^^^^^^^ + +error: struct has unnecessary safety comment + --> $DIR/unnecessary_safety_comment.rs:10:5 + | +LL | struct Struct; + | ^^^^^^^^^^^^^^ + | +help: consider removing the safety comment + --> $DIR/unnecessary_safety_comment.rs:9:5 + | +LL | // SAFETY: + | ^^^^^^^^^^ + +error: enum has unnecessary safety comment + --> $DIR/unnecessary_safety_comment.rs:12:5 + | +LL | enum Enum {} + | ^^^^^^^^^^^^ + | +help: consider removing the safety comment + --> $DIR/unnecessary_safety_comment.rs:11:5 + | +LL | // SAFETY: + | ^^^^^^^^^^ + +error: module has unnecessary safety comment + --> $DIR/unnecessary_safety_comment.rs:14:5 + | +LL | mod module {} + | ^^^^^^^^^^^^^ + | +help: consider removing the safety comment + --> $DIR/unnecessary_safety_comment.rs:13:5 + | +LL | // SAFETY: + | ^^^^^^^^^^ + +error: impl has unnecessary safety comment + --> $DIR/unnecessary_safety_comment.rs:33:13 + | +LL | impl T for $t {} + | ^^^^^^^^^^^^^^^^ +... +LL | with_safety_comment!(i32); + | ------------------------- in this macro invocation + | +help: consider removing the safety comment + --> $DIR/unnecessary_safety_comment.rs:32:13 + | +LL | // Safety: unnecessary + | ^^^^^^^^^^^^^^^^^^^^^^ + = note: this error originates in the macro `with_safety_comment` (in Nightly builds, run with -Z macro-backtrace for more info) + +error: expression has unnecessary safety comment + --> $DIR/unnecessary_safety_comment.rs:48:5 + | +LL | 24 + | ^^ + | +help: consider removing the safety comment + --> $DIR/unnecessary_safety_comment.rs:47:5 + | +LL | // SAFETY: unnecessary + | ^^^^^^^^^^^^^^^^^^^^^^ + +error: statement has unnecessary safety comment + --> $DIR/unnecessary_safety_comment.rs:42:5 + | +LL | let num = 42; + | ^^^^^^^^^^^^^ + | +help: consider removing the safety comment + --> $DIR/unnecessary_safety_comment.rs:41:5 + | +LL | // SAFETY: unnecessary + | ^^^^^^^^^^^^^^^^^^^^^^ + +error: statement has unnecessary safety comment + --> $DIR/unnecessary_safety_comment.rs:45:5 + | +LL | if num > 24 {} + | ^^^^^^^^^^^^^^ + | +help: consider removing the safety comment + --> $DIR/unnecessary_safety_comment.rs:44:5 + | +LL | // SAFETY: unnecessary + | ^^^^^^^^^^^^^^^^^^^^^^ + +error: aborting due to 9 previous errors +