diff options
| -rw-r--r-- | CHANGELOG.md | 1 | ||||
| -rw-r--r-- | clippy_lints/src/declared_lints.rs | 1 | ||||
| -rw-r--r-- | clippy_lints/src/doc.rs | 94 | ||||
| -rw-r--r-- | tests/ui/auxiliary/doc_unsafe_macros.rs | 8 | ||||
| -rw-r--r-- | tests/ui/doc_unnecessary_unsafe.rs | 148 | ||||
| -rw-r--r-- | tests/ui/doc_unnecessary_unsafe.stderr | 51 |
6 files changed, 278 insertions, 25 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md index 9bd67617f5b..f1697312d5c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4449,6 +4449,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_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 [`unnecessary_to_owned`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_to_owned diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 98265410e84..0ff039a4cdf 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -127,6 +127,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[ crate::doc::MISSING_PANICS_DOC_INFO, crate::doc::MISSING_SAFETY_DOC_INFO, crate::doc::NEEDLESS_DOCTEST_MAIN_INFO, + crate::doc::UNNECESSARY_SAFETY_DOC_INFO, crate::double_parens::DOUBLE_PARENS_INFO, crate::drop_forget_ref::DROP_COPY_INFO, crate::drop_forget_ref::DROP_NON_DROP_INFO, diff --git a/clippy_lints/src/doc.rs b/clippy_lints/src/doc.rs index fbc00836b39..2ff58ade31f 100644 --- a/clippy_lints/src/doc.rs +++ b/clippy_lints/src/doc.rs @@ -221,6 +221,42 @@ declare_clippy_lint! { "possible typo for an intra-doc link" } +declare_clippy_lint! { + /// ### What it does + /// Checks for the doc comments of publicly visible + /// safe functions and traits and warns if there is a `# Safety` section. + /// + /// ### Why is this bad? + /// Safe functions and traits are safe to implement and therefore do not + /// need to describe safety preconditions that users are required to uphold. + /// + /// ### Examples + /// ```rust + ///# type Universe = (); + /// /// # Safety + /// /// + /// /// This function should not be called before the horsemen are ready. + /// pub fn start_apocalypse_but_safely(u: &mut Universe) { + /// unimplemented!(); + /// } + /// ``` + /// + /// The function is safe, so there shouldn't be any preconditions + /// that have to be explained for safety reasons. + /// + /// ```rust + ///# type Universe = (); + /// /// This function should really be documented + /// pub fn start_apocalypse(u: &mut Universe) { + /// unimplemented!(); + /// } + /// ``` + #[clippy::version = "1.66.0"] + pub UNNECESSARY_SAFETY_DOC, + style, + "`pub fn` or `pub trait` with `# Safety` docs" +} + #[expect(clippy::module_name_repetitions)] #[derive(Clone)] pub struct DocMarkdown { @@ -243,7 +279,8 @@ impl_lint_pass!(DocMarkdown => [ MISSING_SAFETY_DOC, MISSING_ERRORS_DOC, MISSING_PANICS_DOC, - NEEDLESS_DOCTEST_MAIN + NEEDLESS_DOCTEST_MAIN, + UNNECESSARY_SAFETY_DOC, ]); impl<'tcx> LateLintPass<'tcx> for DocMarkdown { @@ -254,7 +291,7 @@ impl<'tcx> LateLintPass<'tcx> for DocMarkdown { fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx hir::Item<'_>) { let attrs = cx.tcx.hir().attrs(item.hir_id()); - let headers = check_attrs(cx, &self.valid_idents, attrs); + let Some(headers) = check_attrs(cx, &self.valid_idents, attrs) else { return }; match item.kind { hir::ItemKind::Fn(ref sig, _, body_id) => { if !(is_entrypoint_fn(cx, item.def_id.to_def_id()) || in_external_macro(cx.tcx.sess, item.span)) { @@ -271,15 +308,20 @@ impl<'tcx> LateLintPass<'tcx> for DocMarkdown { hir::ItemKind::Impl(impl_) => { self.in_trait_impl = impl_.of_trait.is_some(); }, - hir::ItemKind::Trait(_, unsafety, ..) => { - if !headers.safety && unsafety == hir::Unsafety::Unsafe { - span_lint( - cx, - MISSING_SAFETY_DOC, - cx.tcx.def_span(item.def_id), - "docs for unsafe trait missing `# Safety` section", - ); - } + hir::ItemKind::Trait(_, unsafety, ..) => match (headers.safety, unsafety) { + (false, hir::Unsafety::Unsafe) => span_lint( + cx, + MISSING_SAFETY_DOC, + cx.tcx.def_span(item.def_id), + "docs for unsafe trait missing `# Safety` section", + ), + (true, hir::Unsafety::Normal) => span_lint( + cx, + UNNECESSARY_SAFETY_DOC, + cx.tcx.def_span(item.def_id), + "docs for safe trait have unnecessary `# Safety` section", + ), + _ => (), }, _ => (), } @@ -293,7 +335,7 @@ impl<'tcx> LateLintPass<'tcx> for DocMarkdown { fn check_trait_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx hir::TraitItem<'_>) { let attrs = cx.tcx.hir().attrs(item.hir_id()); - let headers = check_attrs(cx, &self.valid_idents, attrs); + let Some(headers) = check_attrs(cx, &self.valid_idents, attrs) else { return }; if let hir::TraitItemKind::Fn(ref sig, ..) = item.kind { if !in_external_macro(cx.tcx.sess, item.span) { lint_for_missing_headers(cx, item.def_id.def_id, sig, headers, None, None); @@ -303,7 +345,7 @@ impl<'tcx> LateLintPass<'tcx> for DocMarkdown { fn check_impl_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx hir::ImplItem<'_>) { let attrs = cx.tcx.hir().attrs(item.hir_id()); - let headers = check_attrs(cx, &self.valid_idents, attrs); + let Some(headers) = check_attrs(cx, &self.valid_idents, attrs) else { return }; if self.in_trait_impl || in_external_macro(cx.tcx.sess, item.span) { return; } @@ -343,14 +385,20 @@ fn lint_for_missing_headers( } let span = cx.tcx.def_span(def_id); - - if !headers.safety && sig.header.unsafety == hir::Unsafety::Unsafe { - span_lint( + match (headers.safety, sig.header.unsafety) { + (false, hir::Unsafety::Unsafe) => span_lint( cx, MISSING_SAFETY_DOC, span, "unsafe function's docs miss `# Safety` section", - ); + ), + (true, hir::Unsafety::Normal) => span_lint( + cx, + UNNECESSARY_SAFETY_DOC, + span, + "safe function's docs have unnecessary `# Safety` section", + ), + _ => (), } if !headers.panics && panic_span.is_some() { span_lint_and_note( @@ -452,7 +500,7 @@ struct DocHeaders { panics: bool, } -fn check_attrs(cx: &LateContext<'_>, valid_idents: &FxHashSet<String>, attrs: &[Attribute]) -> DocHeaders { +fn check_attrs(cx: &LateContext<'_>, valid_idents: &FxHashSet<String>, attrs: &[Attribute]) -> Option<DocHeaders> { use pulldown_cmark::{BrokenLink, CowStr, Options}; /// We don't want the parser to choke on intra doc links. Since we don't /// actually care about rendering them, just pretend that all broken links are @@ -473,11 +521,7 @@ fn check_attrs(cx: &LateContext<'_>, valid_idents: &FxHashSet<String>, attrs: &[ } else if attr.has_name(sym::doc) { // ignore mix of sugared and non-sugared doc // don't trigger the safety or errors check - return DocHeaders { - safety: true, - errors: true, - panics: true, - }; + return None; } } @@ -489,7 +533,7 @@ fn check_attrs(cx: &LateContext<'_>, valid_idents: &FxHashSet<String>, attrs: &[ } if doc.is_empty() { - return DocHeaders::default(); + return Some(DocHeaders::default()); } let mut cb = fake_broken_link_callback; @@ -512,7 +556,7 @@ fn check_attrs(cx: &LateContext<'_>, valid_idents: &FxHashSet<String>, attrs: &[ (previous, current) => Err(((previous, previous_range), (current, current_range))), } }); - check_doc(cx, valid_idents, events, &spans) + Some(check_doc(cx, valid_idents, events, &spans)) } const RUST_CODE: &[&str] = &["rust", "no_run", "should_panic", "compile_fail"]; diff --git a/tests/ui/auxiliary/doc_unsafe_macros.rs b/tests/ui/auxiliary/doc_unsafe_macros.rs index 869672d1eda..3d917e3dc75 100644 --- a/tests/ui/auxiliary/doc_unsafe_macros.rs +++ b/tests/ui/auxiliary/doc_unsafe_macros.rs @@ -6,3 +6,11 @@ macro_rules! undocd_unsafe { } }; } +#[macro_export] +macro_rules! undocd_safe { + () => { + pub fn vey_oy() { + unimplemented!(); + } + }; +} diff --git a/tests/ui/doc_unnecessary_unsafe.rs b/tests/ui/doc_unnecessary_unsafe.rs new file mode 100644 index 00000000000..d9e9363b0f4 --- /dev/null +++ b/tests/ui/doc_unnecessary_unsafe.rs @@ -0,0 +1,148 @@ +// aux-build:doc_unsafe_macros.rs + +#![allow(clippy::let_unit_value)] + +#[macro_use] +extern crate doc_unsafe_macros; + +/// This is has no safety section, and does not need one either +pub fn destroy_the_planet() { + unimplemented!(); +} + +/// This one does not need a `Safety` section +/// +/// # Safety +/// +/// This function shouldn't be called unless the horsemen are ready +pub fn apocalypse(universe: &mut ()) { + unimplemented!(); +} + +/// This is a private function, skip to match behavior with `missing_safety_doc`. +/// +/// # Safety +/// +/// Boo! +fn you_dont_see_me() { + unimplemented!(); +} + +mod private_mod { + /// This is public but unexported function, skip to match behavior with `missing_safety_doc`. + /// + /// # Safety + /// + /// Very safe! + pub fn only_crate_wide_accessible() { + unimplemented!(); + } + + /// # Safety + /// + /// Unnecessary safety! + pub fn republished() { + unimplemented!(); + } +} + +pub use private_mod::republished; + +pub trait SafeTraitSafeMethods { + fn woefully_underdocumented(self); + + /// # Safety + /// + /// Unnecessary! + fn documented(self); +} + +pub trait SafeTrait { + fn method(); +} + +/// # Safety +/// +/// Unnecessary! +pub trait DocumentedSafeTrait { + fn method2(); +} + +pub struct Struct; + +impl SafeTraitSafeMethods for Struct { + fn woefully_underdocumented(self) { + // all is well + } + + fn documented(self) { + // all is still well + } +} + +impl SafeTrait for Struct { + fn method() {} +} + +impl DocumentedSafeTrait for Struct { + fn method2() {} +} + +impl Struct { + /// # Safety + /// + /// Unnecessary! + pub fn documented() -> Self { + unimplemented!(); + } + + pub fn undocumented(&self) { + unimplemented!(); + } + + /// Private, fine again to stay consistent with `missing_safety_doc`. + /// + /// # Safety + /// + /// Unnecessary! + fn private(&self) { + unimplemented!(); + } +} + +macro_rules! very_safe { + () => { + pub fn whee() { + unimplemented!() + } + + /// # Safety + /// + /// Driving is very safe already! + pub fn drive() { + whee() + } + }; +} + +very_safe!(); + +// we don't lint code from external macros +undocd_safe!(); + +fn main() {} + +// do not lint if any parent has `#[doc(hidden)]` attribute +// see #7347 +#[doc(hidden)] +pub mod __macro { + pub struct T; + impl T { + pub unsafe fn f() {} + } +} + +/// # Implementation safety +pub trait DocumentedSafeTraitWithImplementationHeader { + fn method(); +} diff --git a/tests/ui/doc_unnecessary_unsafe.stderr b/tests/ui/doc_unnecessary_unsafe.stderr new file mode 100644 index 00000000000..83b2efbb346 --- /dev/null +++ b/tests/ui/doc_unnecessary_unsafe.stderr @@ -0,0 +1,51 @@ +error: safe function's docs have unnecessary `# Safety` section + --> $DIR/doc_unnecessary_unsafe.rs:18:1 + | +LL | pub fn apocalypse(universe: &mut ()) { + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: `-D clippy::unnecessary-safety-doc` implied by `-D warnings` + +error: safe function's docs have unnecessary `# Safety` section + --> $DIR/doc_unnecessary_unsafe.rs:44:5 + | +LL | pub fn republished() { + | ^^^^^^^^^^^^^^^^^^^^ + +error: safe function's docs have unnecessary `# Safety` section + --> $DIR/doc_unnecessary_unsafe.rs:57:5 + | +LL | fn documented(self); + | ^^^^^^^^^^^^^^^^^^^^ + +error: docs for safe trait have unnecessary `# Safety` section + --> $DIR/doc_unnecessary_unsafe.rs:67:1 + | +LL | pub trait DocumentedSafeTrait { + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: safe function's docs have unnecessary `# Safety` section + --> $DIR/doc_unnecessary_unsafe.rs:95:5 + | +LL | pub fn documented() -> Self { + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: safe function's docs have unnecessary `# Safety` section + --> $DIR/doc_unnecessary_unsafe.rs:122:9 + | +LL | pub fn drive() { + | ^^^^^^^^^^^^^^ +... +LL | very_safe!(); + | ------------ in this macro invocation + | + = note: this error originates in the macro `very_safe` (in Nightly builds, run with -Z macro-backtrace for more info) + +error: docs for safe trait have unnecessary `# Safety` section + --> $DIR/doc_unnecessary_unsafe.rs:146:1 + | +LL | pub trait DocumentedSafeTraitWithImplementationHeader { + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: aborting due to 7 previous errors + |