From f5d0a452ba0b50b796cbbcc49cc6e25f2b29a256 Mon Sep 17 00:00:00 2001 From: RobbieClarken Date: Fri, 6 Dec 2019 10:30:23 +1030 Subject: [PATCH] Add lint for pub fns returning a `Result` without documenting errors The Rust Book recommends that functions that return a `Result` type have a doc comment with an `# Errors` section describing the kind of errors that can be returned (https://doc.rust-lang.org/book/ch14-02-publishing-to-crates-io.html#commonly-used-sections). This change adds a lint to enforce this. The lint is allow by default; it can be enabled with `#![warn(clippy::missing_errors_doc)]`. Closes #4854. --- CHANGELOG.md | 1 + README.md | 2 +- clippy_lints/src/doc.rs | 137 +++++++++++++++++++++++++------------ clippy_lints/src/lib.rs | 2 + src/lintlist/mod.rs | 9 ++- tests/ui/doc_errors.rs | 64 +++++++++++++++++ tests/ui/doc_errors.stderr | 34 +++++++++ 7 files changed, 202 insertions(+), 47 deletions(-) create mode 100644 tests/ui/doc_errors.rs create mode 100644 tests/ui/doc_errors.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index 4e747524e9e..a9448a57f7f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1092,6 +1092,7 @@ Released 2018-09-13 [`misrefactored_assign_op`]: https://rust-lang.github.io/rust-clippy/master/index.html#misrefactored_assign_op [`missing_const_for_fn`]: https://rust-lang.github.io/rust-clippy/master/index.html#missing_const_for_fn [`missing_docs_in_private_items`]: https://rust-lang.github.io/rust-clippy/master/index.html#missing_docs_in_private_items +[`missing_errors_doc`]: https://rust-lang.github.io/rust-clippy/master/index.html#missing_errors_doc [`missing_inline_in_public_items`]: https://rust-lang.github.io/rust-clippy/master/index.html#missing_inline_in_public_items [`missing_safety_doc`]: https://rust-lang.github.io/rust-clippy/master/index.html#missing_safety_doc [`mistyped_literal_suffixes`]: https://rust-lang.github.io/rust-clippy/master/index.html#mistyped_literal_suffixes diff --git a/README.md b/README.md index e8818412e90..97a7c97b49a 100644 --- a/README.md +++ b/README.md @@ -6,7 +6,7 @@ A collection of lints to catch common mistakes and improve your [Rust](https://github.com/rust-lang/rust) code. -[There are 338 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html) +[There are 339 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html) We have a bunch of lint categories to allow you to choose how much Clippy is supposed to ~~annoy~~ help you: diff --git a/clippy_lints/src/doc.rs b/clippy_lints/src/doc.rs index 0fcc9763d37..8f8d8ad96bb 100644 --- a/clippy_lints/src/doc.rs +++ b/clippy_lints/src/doc.rs @@ -1,4 +1,4 @@ -use crate::utils::span_lint; +use crate::utils::{match_type, paths, return_ty, span_lint}; use itertools::Itertools; use pulldown_cmark; use rustc::hir; @@ -8,7 +8,7 @@ use rustc_data_structures::fx::FxHashSet; use rustc_session::declare_tool_lint; use std::ops::Range; use syntax::ast::{AttrKind, Attribute}; -use syntax::source_map::{BytePos, Span}; +use syntax::source_map::{BytePos, MultiSpan, Span}; use syntax_pos::Pos; use url::Url; @@ -45,7 +45,7 @@ declare_clippy_lint! { /// /// **Known problems:** None. /// - /// **Examples**: + /// **Examples:** /// ```rust ///# type Universe = (); /// /// This function should really be documented @@ -70,6 +70,35 @@ declare_clippy_lint! { "`pub unsafe fn` without `# Safety` docs" } +declare_clippy_lint! { + /// **What it does:** Checks the doc comments of publicly visible functions that + /// return a `Result` type and warns if there is no `# Errors` section. + /// + /// **Why is this bad?** Documenting the type of errors that can be returned from a + /// function can help callers write code to handle the errors appropriately. + /// + /// **Known problems:** None. + /// + /// **Examples:** + /// + /// Since the following function returns a `Result` it has an `# Errors` section in + /// its doc comment: + /// + /// ```rust + ///# use std::io; + /// /// # Errors + /// /// + /// /// Will return `Err` if `filename` does not exist or the user does not have + /// /// permission to read it. + /// pub fn read(filename: String) -> io::Result { + /// unimplemented!(); + /// } + /// ``` + pub MISSING_ERRORS_DOC, + pedantic, + "`pub fn` returns `Result` without `# Errors` in doc comment" +} + declare_clippy_lint! { /// **What it does:** Checks for `fn main() { .. }` in doctests /// @@ -114,7 +143,7 @@ impl DocMarkdown { } } -impl_lint_pass!(DocMarkdown => [DOC_MARKDOWN, MISSING_SAFETY_DOC, NEEDLESS_DOCTEST_MAIN]); +impl_lint_pass!(DocMarkdown => [DOC_MARKDOWN, MISSING_SAFETY_DOC, MISSING_ERRORS_DOC, NEEDLESS_DOCTEST_MAIN]); impl<'a, 'tcx> LateLintPass<'a, 'tcx> for DocMarkdown { fn check_crate(&mut self, cx: &LateContext<'a, 'tcx>, krate: &'tcx hir::Crate) { @@ -122,20 +151,10 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for DocMarkdown { } fn check_item(&mut self, cx: &LateContext<'a, 'tcx>, item: &'tcx hir::Item) { - if check_attrs(cx, &self.valid_idents, &item.attrs) { - return; - } - // no safety header + let headers = check_attrs(cx, &self.valid_idents, &item.attrs); match item.kind { hir::ItemKind::Fn(ref sig, ..) => { - if cx.access_levels.is_exported(item.hir_id) && sig.header.unsafety == hir::Unsafety::Unsafe { - span_lint( - cx, - MISSING_SAFETY_DOC, - item.span, - "unsafe function's docs miss `# Safety` section", - ); - } + lint_for_missing_headers(cx, item.hir_id, item.span, sig, headers); }, hir::ItemKind::Impl(_, _, _, _, ref trait_ref, ..) => { self.in_trait_impl = trait_ref.is_some(); @@ -151,40 +170,51 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for DocMarkdown { } fn check_trait_item(&mut self, cx: &LateContext<'a, 'tcx>, item: &'tcx hir::TraitItem) { - if check_attrs(cx, &self.valid_idents, &item.attrs) { - return; - } - // no safety header + let headers = check_attrs(cx, &self.valid_idents, &item.attrs); if let hir::TraitItemKind::Method(ref sig, ..) = item.kind { - if cx.access_levels.is_exported(item.hir_id) && sig.header.unsafety == hir::Unsafety::Unsafe { - span_lint( - cx, - MISSING_SAFETY_DOC, - item.span, - "unsafe function's docs miss `# Safety` section", - ); - } + lint_for_missing_headers(cx, item.hir_id, item.span, sig, headers); } } fn check_impl_item(&mut self, cx: &LateContext<'a, 'tcx>, item: &'tcx hir::ImplItem) { - if check_attrs(cx, &self.valid_idents, &item.attrs) || self.in_trait_impl { + let headers = check_attrs(cx, &self.valid_idents, &item.attrs); + if self.in_trait_impl { return; } - // no safety header if let hir::ImplItemKind::Method(ref sig, ..) = item.kind { - if cx.access_levels.is_exported(item.hir_id) && sig.header.unsafety == hir::Unsafety::Unsafe { - span_lint( - cx, - MISSING_SAFETY_DOC, - item.span, - "unsafe function's docs miss `# Safety` section", - ); - } + lint_for_missing_headers(cx, item.hir_id, item.span, sig, headers); } } } +fn lint_for_missing_headers<'a, 'tcx>( + cx: &LateContext<'a, 'tcx>, + hir_id: hir::HirId, + span: impl Into + Copy, + sig: &hir::FnSig, + headers: DocHeaders, +) { + if !cx.access_levels.is_exported(hir_id) { + return; // Private functions do not require doc comments + } + if !headers.safety && sig.header.unsafety == hir::Unsafety::Unsafe { + span_lint( + cx, + MISSING_SAFETY_DOC, + span, + "unsafe function's docs miss `# Safety` section", + ); + } + if !headers.errors && match_type(cx, return_ty(cx, hir_id), &paths::RESULT) { + span_lint( + cx, + MISSING_ERRORS_DOC, + span, + "docs for function returning `Result` missing `# Errors` section", + ); + } +} + /// Cleanup documentation decoration (`///` and such). /// /// We can't use `syntax::attr::AttributeMethods::with_desugared_doc` or @@ -243,7 +273,13 @@ pub fn strip_doc_comment_decoration(comment: &str, span: Span) -> (String, Vec<( panic!("not a doc-comment: {}", comment); } -pub fn check_attrs<'a>(cx: &LateContext<'_, '_>, valid_idents: &FxHashSet, attrs: &'a [Attribute]) -> bool { +#[derive(Copy, Clone)] +struct DocHeaders { + safety: bool, + errors: bool, +} + +fn check_attrs<'a>(cx: &LateContext<'_, '_>, valid_idents: &FxHashSet, attrs: &'a [Attribute]) -> DocHeaders { let mut doc = String::new(); let mut spans = vec![]; @@ -255,7 +291,11 @@ pub fn check_attrs<'a>(cx: &LateContext<'_, '_>, valid_idents: &FxHashSet(cx: &LateContext<'_, '_>, valid_idents: &FxHashSet, Range, events: Events, spans: &[(usize, Span)], -) -> bool { +) -> DocHeaders { // true if a safety header was found use pulldown_cmark::Event::*; use pulldown_cmark::Tag::*; - let mut safety_header = false; + let mut headers = DocHeaders { + safety: false, + errors: false, + }; let mut in_code = false; let mut in_link = None; let mut in_heading = false; @@ -323,7 +369,8 @@ fn check_doc<'a, Events: Iterator, Range o, Err(e) => e - 1, @@ -340,7 +387,7 @@ fn check_doc<'a, Events: Iterator, Range, text: &str, span: Span) { diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 432a38a11a5..d14f946c8eb 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -488,6 +488,7 @@ pub fn register_plugins(store: &mut lint::LintStore, sess: &Session, conf: &Conf &derive::DERIVE_HASH_XOR_EQ, &derive::EXPL_IMPL_CLONE_ON_COPY, &doc::DOC_MARKDOWN, + &doc::MISSING_ERRORS_DOC, &doc::MISSING_SAFETY_DOC, &doc::NEEDLESS_DOCTEST_MAIN, &double_comparison::DOUBLE_COMPARISONS, @@ -1013,6 +1014,7 @@ pub fn register_plugins(store: &mut lint::LintStore, sess: &Session, conf: &Conf LintId::of(&default_trait_access::DEFAULT_TRAIT_ACCESS), LintId::of(&derive::EXPL_IMPL_CLONE_ON_COPY), LintId::of(&doc::DOC_MARKDOWN), + LintId::of(&doc::MISSING_ERRORS_DOC), LintId::of(&empty_enum::EMPTY_ENUM), LintId::of(&enum_glob_use::ENUM_GLOB_USE), LintId::of(&enum_variants::MODULE_NAME_REPETITIONS), diff --git a/src/lintlist/mod.rs b/src/lintlist/mod.rs index 5b08571c258..1f1c24b2c30 100644 --- a/src/lintlist/mod.rs +++ b/src/lintlist/mod.rs @@ -6,7 +6,7 @@ pub use lint::Lint; pub use lint::LINT_LEVELS; // begin lint list, do not remove this comment, it’s used in `update_lints` -pub const ALL_LINTS: [Lint; 338] = [ +pub const ALL_LINTS: [Lint; 339] = [ Lint { name: "absurd_extreme_comparisons", group: "correctness", @@ -1127,6 +1127,13 @@ pub const ALL_LINTS: [Lint; 338] = [ deprecation: None, module: "missing_doc", }, + Lint { + name: "missing_errors_doc", + group: "pedantic", + desc: "`pub fn` returns `Result` without `# Errors` in doc comment", + deprecation: None, + module: "doc", + }, Lint { name: "missing_inline_in_public_items", group: "restriction", diff --git a/tests/ui/doc_errors.rs b/tests/ui/doc_errors.rs new file mode 100644 index 00000000000..408cf573896 --- /dev/null +++ b/tests/ui/doc_errors.rs @@ -0,0 +1,64 @@ +#![warn(clippy::missing_errors_doc)] + +use std::io; + +pub fn pub_fn_missing_errors_header() -> Result<(), ()> { + unimplemented!(); +} + +/// This is not sufficiently documented. +pub fn pub_fn_returning_io_result() -> io::Result<()> { + unimplemented!(); +} + +/// # Errors +/// A description of the errors goes here. +pub fn pub_fn_with_errors_header() -> Result<(), ()> { + unimplemented!(); +} + +/// This function doesn't require the documentation because it is private +fn priv_fn_missing_errors_header() -> Result<(), ()> { + unimplemented!(); +} + +pub struct Struct1; + +impl Struct1 { + /// This is not sufficiently documented. + pub fn pub_method_missing_errors_header() -> Result<(), ()> { + unimplemented!(); + } + + /// # Errors + /// A description of the errors goes here. + pub fn pub_method_with_errors_header() -> Result<(), ()> { + unimplemented!(); + } + + /// This function doesn't require the documentation because it is private. + fn priv_method_missing_errors_header() -> Result<(), ()> { + unimplemented!(); + } +} + +pub trait Trait1 { + /// This is not sufficiently documented. + fn trait_method_missing_errors_header() -> Result<(), ()>; + + /// # Errors + /// A description of the errors goes here. + fn trait_method_with_errors_header() -> Result<(), ()>; +} + +impl Trait1 for Struct1 { + fn trait_method_missing_errors_header() -> Result<(), ()> { + unimplemented!(); + } + + fn trait_method_with_errors_header() -> Result<(), ()> { + unimplemented!(); + } +} + +fn main() {} diff --git a/tests/ui/doc_errors.stderr b/tests/ui/doc_errors.stderr new file mode 100644 index 00000000000..f1d321cf909 --- /dev/null +++ b/tests/ui/doc_errors.stderr @@ -0,0 +1,34 @@ +error: docs for function returning `Result` missing `# Errors` section + --> $DIR/doc_errors.rs:5:1 + | +LL | / pub fn pub_fn_missing_errors_header() -> Result<(), ()> { +LL | | unimplemented!(); +LL | | } + | |_^ + | + = note: `-D clippy::missing-errors-doc` implied by `-D warnings` + +error: docs for function returning `Result` missing `# Errors` section + --> $DIR/doc_errors.rs:10:1 + | +LL | / pub fn pub_fn_returning_io_result() -> io::Result<()> { +LL | | unimplemented!(); +LL | | } + | |_^ + +error: docs for function returning `Result` missing `# Errors` section + --> $DIR/doc_errors.rs:29:5 + | +LL | / pub fn pub_method_missing_errors_header() -> Result<(), ()> { +LL | | unimplemented!(); +LL | | } + | |_____^ + +error: docs for function returning `Result` missing `# Errors` section + --> $DIR/doc_errors.rs:47:5 + | +LL | fn trait_method_missing_errors_header() -> Result<(), ()>; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: aborting due to 4 previous errors +