From 0b168c693a7c9e1307b143937d6537b36b929303 Mon Sep 17 00:00:00 2001 From: daxpedda Date: Sun, 16 Feb 2020 22:49:47 +0100 Subject: [PATCH 1/5] Add `Future` detection for `missing_errors_doc`. --- clippy_lints/src/doc.rs | 35 ++++++++++++++++++++++++++++------- 1 file changed, 28 insertions(+), 7 deletions(-) diff --git a/clippy_lints/src/doc.rs b/clippy_lints/src/doc.rs index 5db1886bb52d..4b1f2ec68cb1 100644 --- a/clippy_lints/src/doc.rs +++ b/clippy_lints/src/doc.rs @@ -1,6 +1,7 @@ use crate::utils::{is_entrypoint_fn, match_type, paths, return_ty, span_lint}; use itertools::Itertools; use rustc::lint::in_external_macro; +use rustc::ty::TyKind; use rustc_data_structures::fx::FxHashSet; use rustc_hir as hir; use rustc_lint::{LateContext, LateLintPass}; @@ -213,13 +214,33 @@ fn lint_for_missing_headers<'a, 'tcx>( "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", - ); + if !headers.errors { + if 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", + ); + } else { + use TyKind::*; + let def_id = cx.tcx.hir().local_def_id(hir_id); + let mir = cx.tcx.optimized_mir(def_id); + if let Opaque(_, subs) = mir.return_ty().kind { + if let Some(ty) = subs.types().next() { + if let Generator(_, subs, _) = ty.kind { + if match_type(cx, subs.as_generator().return_ty(def_id, cx.tcx), &paths::RESULT) { + span_lint( + cx, + MISSING_ERRORS_DOC, + span, + "docs for function returning `Result` missing `# Errors` section", + ); + } + } + } + } + } } } From 0ee393cf01b59d55a54e9e4299797bd0f7339d58 Mon Sep 17 00:00:00 2001 From: daxpedda Date: Mon, 17 Feb 2020 01:17:14 +0100 Subject: [PATCH 2/5] Add tests and improve checks. --- clippy_lints/src/doc.rs | 29 +++++++++++++++----------- clippy_lints/src/utils/paths.rs | 1 + tests/ui/doc_errors.rs | 37 +++++++++++++++++++++++++++++++++ tests/ui/doc_errors.stderr | 32 ++++++++++++++++++++++++---- 4 files changed, 83 insertions(+), 16 deletions(-) diff --git a/clippy_lints/src/doc.rs b/clippy_lints/src/doc.rs index 4b1f2ec68cb1..133a20b669a1 100644 --- a/clippy_lints/src/doc.rs +++ b/clippy_lints/src/doc.rs @@ -1,4 +1,4 @@ -use crate::utils::{is_entrypoint_fn, match_type, paths, return_ty, span_lint}; +use crate::utils::{get_trait_def_id, implements_trait, is_entrypoint_fn, match_type, paths, return_ty, span_lint}; use itertools::Itertools; use rustc::lint::in_external_macro; use rustc::ty::TyKind; @@ -223,19 +223,24 @@ fn lint_for_missing_headers<'a, 'tcx>( "docs for function returning `Result` missing `# Errors` section", ); } else { - use TyKind::*; let def_id = cx.tcx.hir().local_def_id(hir_id); let mir = cx.tcx.optimized_mir(def_id); - if let Opaque(_, subs) = mir.return_ty().kind { - if let Some(ty) = subs.types().next() { - if let Generator(_, subs, _) = ty.kind { - if match_type(cx, subs.as_generator().return_ty(def_id, cx.tcx), &paths::RESULT) { - span_lint( - cx, - MISSING_ERRORS_DOC, - span, - "docs for function returning `Result` missing `# Errors` section", - ); + if let Some(future) = get_trait_def_id(cx, &paths::FUTURE) { + if implements_trait(cx, mir.return_ty(), future, &[]) { + use TyKind::*; + + if let Opaque(_, subs) = mir.return_ty().kind { + if let Some(ty) = subs.types().next() { + if let Generator(_, subs, _) = ty.kind { + if match_type(cx, subs.as_generator().return_ty(def_id, cx.tcx), &paths::RESULT) { + span_lint( + cx, + MISSING_ERRORS_DOC, + span, + "docs for function returning `Result` missing `# Errors` section", + ); + } + } } } } diff --git a/clippy_lints/src/utils/paths.rs b/clippy_lints/src/utils/paths.rs index 0af7f946fa9d..96337e42b544 100644 --- a/clippy_lints/src/utils/paths.rs +++ b/clippy_lints/src/utils/paths.rs @@ -36,6 +36,7 @@ pub const FMT_ARGUMENTS_NEW_V1_FORMATTED: [&str; 4] = ["core", "fmt", "Arguments pub const FMT_ARGUMENTV1_NEW: [&str; 4] = ["core", "fmt", "ArgumentV1", "new"]; pub const FROM_FROM: [&str; 4] = ["core", "convert", "From", "from"]; pub const FROM_TRAIT: [&str; 3] = ["core", "convert", "From"]; +pub const FUTURE: [&str; 3] = ["std", "future", "Future"]; pub const HASH: [&str; 2] = ["hash", "Hash"]; pub const HASHMAP: [&str; 5] = ["std", "collections", "hash", "map", "HashMap"]; pub const HASHMAP_ENTRY: [&str; 5] = ["std", "collections", "hash", "map", "Entry"]; diff --git a/tests/ui/doc_errors.rs b/tests/ui/doc_errors.rs index 776a65275e9b..1401a658e037 100644 --- a/tests/ui/doc_errors.rs +++ b/tests/ui/doc_errors.rs @@ -1,3 +1,4 @@ +// compile-flags: --edition 2018 #![warn(clippy::missing_errors_doc)] use std::io; @@ -6,22 +7,42 @@ pub fn pub_fn_missing_errors_header() -> Result<(), ()> { unimplemented!(); } +pub async fn async_pub_fn_missing_errors_header() -> Result<(), ()> { + unimplemented!(); +} + /// This is not sufficiently documented. pub fn pub_fn_returning_io_result() -> io::Result<()> { unimplemented!(); } +/// This is not sufficiently documented. +pub async fn async_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!(); } +/// # Errors +/// A description of the errors goes here. +pub async fn async_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!(); } +/// This function doesn't require the documentation because it is private +async fn async_priv_fn_missing_errors_header() -> Result<(), ()> { + unimplemented!(); +} + pub struct Struct1; impl Struct1 { @@ -30,16 +51,32 @@ impl Struct1 { unimplemented!(); } + /// This is not sufficiently documented. + pub async fn async_pub_method_missing_errors_header() -> Result<(), ()> { + unimplemented!(); + } + /// # Errors /// A description of the errors goes here. pub fn pub_method_with_errors_header() -> Result<(), ()> { unimplemented!(); } + /// # Errors + /// A description of the errors goes here. + pub async fn async_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!(); } + + /// This function doesn't require the documentation because it is private. + async fn async_priv_method_missing_errors_header() -> Result<(), ()> { + unimplemented!(); + } } pub trait Trait1 { diff --git a/tests/ui/doc_errors.stderr b/tests/ui/doc_errors.stderr index f1d321cf909c..f44d6693d303 100644 --- a/tests/ui/doc_errors.stderr +++ b/tests/ui/doc_errors.stderr @@ -1,5 +1,5 @@ error: docs for function returning `Result` missing `# Errors` section - --> $DIR/doc_errors.rs:5:1 + --> $DIR/doc_errors.rs:6:1 | LL | / pub fn pub_fn_missing_errors_header() -> Result<(), ()> { LL | | unimplemented!(); @@ -11,13 +11,29 @@ LL | | } error: docs for function returning `Result` missing `# Errors` section --> $DIR/doc_errors.rs:10:1 | +LL | / pub async fn async_pub_fn_missing_errors_header() -> Result<(), ()> { +LL | | unimplemented!(); +LL | | } + | |_^ + +error: docs for function returning `Result` missing `# Errors` section + --> $DIR/doc_errors.rs:15: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 + --> $DIR/doc_errors.rs:20:1 + | +LL | / pub async fn async_pub_fn_returning_io_result() -> io::Result<()> { +LL | | unimplemented!(); +LL | | } + | |_^ + +error: docs for function returning `Result` missing `# Errors` section + --> $DIR/doc_errors.rs:50:5 | LL | / pub fn pub_method_missing_errors_header() -> Result<(), ()> { LL | | unimplemented!(); @@ -25,10 +41,18 @@ LL | | } | |_____^ error: docs for function returning `Result` missing `# Errors` section - --> $DIR/doc_errors.rs:47:5 + --> $DIR/doc_errors.rs:55:5 + | +LL | / pub async fn async_pub_method_missing_errors_header() -> Result<(), ()> { +LL | | unimplemented!(); +LL | | } + | |_____^ + +error: docs for function returning `Result` missing `# Errors` section + --> $DIR/doc_errors.rs:84:5 | LL | fn trait_method_missing_errors_header() -> Result<(), ()>; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -error: aborting due to 4 previous errors +error: aborting due to 7 previous errors From d8716f5a3fb32dfaa77e298129dae193b0b8c650 Mon Sep 17 00:00:00 2001 From: daxpedda Date: Mon, 17 Feb 2020 02:38:48 +0100 Subject: [PATCH 3/5] Fix ICE. --- clippy_lints/src/doc.rs | 45 +++++++++++++++++++++-------------------- 1 file changed, 23 insertions(+), 22 deletions(-) diff --git a/clippy_lints/src/doc.rs b/clippy_lints/src/doc.rs index 133a20b669a1..c1285ad36467 100644 --- a/clippy_lints/src/doc.rs +++ b/clippy_lints/src/doc.rs @@ -153,11 +153,11 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for DocMarkdown { fn check_item(&mut self, cx: &LateContext<'a, 'tcx>, item: &'tcx hir::Item<'_>) { let headers = check_attrs(cx, &self.valid_idents, &item.attrs); match item.kind { - hir::ItemKind::Fn(ref sig, ..) => { + hir::ItemKind::Fn(ref sig, _, body_id) => { if !(is_entrypoint_fn(cx, cx.tcx.hir().local_def_id(item.hir_id)) || in_external_macro(cx.tcx.sess, item.span)) { - lint_for_missing_headers(cx, item.hir_id, item.span, sig, headers); + lint_for_missing_headers(cx, item.hir_id, item.span, sig, headers, Some(body_id)); } }, hir::ItemKind::Impl { @@ -180,7 +180,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for DocMarkdown { let headers = check_attrs(cx, &self.valid_idents, &item.attrs); if let hir::TraitItemKind::Method(ref sig, ..) = item.kind { if !in_external_macro(cx.tcx.sess, item.span) { - lint_for_missing_headers(cx, item.hir_id, item.span, sig, headers); + lint_for_missing_headers(cx, item.hir_id, item.span, sig, headers, None); } } } @@ -190,8 +190,8 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for DocMarkdown { if self.in_trait_impl || in_external_macro(cx.tcx.sess, item.span) { return; } - if let hir::ImplItemKind::Method(ref sig, ..) = item.kind { - lint_for_missing_headers(cx, item.hir_id, item.span, sig, headers); + if let hir::ImplItemKind::Method(ref sig, body_id) = item.kind { + lint_for_missing_headers(cx, item.hir_id, item.span, sig, headers, Some(body_id)); } } } @@ -202,6 +202,7 @@ fn lint_for_missing_headers<'a, 'tcx>( span: impl Into + Copy, sig: &hir::FnSig<'_>, headers: DocHeaders, + body_id: Option, ) { if !cx.access_levels.is_exported(hir_id) { return; // Private functions do not require doc comments @@ -222,24 +223,24 @@ fn lint_for_missing_headers<'a, 'tcx>( span, "docs for function returning `Result` missing `# Errors` section", ); - } else { - let def_id = cx.tcx.hir().local_def_id(hir_id); + } else if let (Some(body_id), Some(future)) = (body_id, get_trait_def_id(cx, &paths::FUTURE)) { + let def_id = cx.tcx.hir().body_owner_def_id(body_id); let mir = cx.tcx.optimized_mir(def_id); - if let Some(future) = get_trait_def_id(cx, &paths::FUTURE) { - if implements_trait(cx, mir.return_ty(), future, &[]) { - use TyKind::*; - - if let Opaque(_, subs) = mir.return_ty().kind { - if let Some(ty) = subs.types().next() { - if let Generator(_, subs, _) = ty.kind { - if match_type(cx, subs.as_generator().return_ty(def_id, cx.tcx), &paths::RESULT) { - span_lint( - cx, - MISSING_ERRORS_DOC, - span, - "docs for function returning `Result` missing `# Errors` section", - ); - } + let ret_ty = mir.return_ty(); + + if implements_trait(cx, ret_ty, future, &[]) { + use TyKind::*; + + if let Opaque(_, subs) = ret_ty.kind { + if let Some(ty) = subs.types().next() { + if let Generator(_, subs, _) = ty.kind { + if match_type(cx, subs.as_generator().return_ty(def_id, cx.tcx), &paths::RESULT) { + span_lint( + cx, + MISSING_ERRORS_DOC, + span, + "docs for function returning `Result` missing `# Errors` section", + ); } } } From 8e2dab3b3c37b50d4148a72e09e5713412570cf8 Mon Sep 17 00:00:00 2001 From: daxpedda Date: Mon, 17 Feb 2020 12:18:00 +0100 Subject: [PATCH 4/5] Use `if_chain`. --- clippy_lints/src/doc.rs | 42 ++++++++++++++++++++--------------------- 1 file changed, 21 insertions(+), 21 deletions(-) diff --git a/clippy_lints/src/doc.rs b/clippy_lints/src/doc.rs index c1285ad36467..1bbb66189662 100644 --- a/clippy_lints/src/doc.rs +++ b/clippy_lints/src/doc.rs @@ -1,4 +1,5 @@ use crate::utils::{get_trait_def_id, implements_trait, is_entrypoint_fn, match_type, paths, return_ty, span_lint}; +use if_chain::if_chain; use itertools::Itertools; use rustc::lint::in_external_macro; use rustc::ty::TyKind; @@ -223,27 +224,26 @@ fn lint_for_missing_headers<'a, 'tcx>( span, "docs for function returning `Result` missing `# Errors` section", ); - } else if let (Some(body_id), Some(future)) = (body_id, get_trait_def_id(cx, &paths::FUTURE)) { - let def_id = cx.tcx.hir().body_owner_def_id(body_id); - let mir = cx.tcx.optimized_mir(def_id); - let ret_ty = mir.return_ty(); - - if implements_trait(cx, ret_ty, future, &[]) { - use TyKind::*; - - if let Opaque(_, subs) = ret_ty.kind { - if let Some(ty) = subs.types().next() { - if let Generator(_, subs, _) = ty.kind { - if match_type(cx, subs.as_generator().return_ty(def_id, cx.tcx), &paths::RESULT) { - span_lint( - cx, - MISSING_ERRORS_DOC, - span, - "docs for function returning `Result` missing `# Errors` section", - ); - } - } - } + } else { + use TyKind::*; + if_chain! { + if let Some(body_id) = body_id; + if let Some(future) = get_trait_def_id(cx, &paths::FUTURE); + let def_id = cx.tcx.hir().body_owner_def_id(body_id); + let mir = cx.tcx.optimized_mir(def_id); + let ret_ty = mir.return_ty(); + if implements_trait(cx, ret_ty, future, &[]); + if let Opaque(_, subs) = ret_ty.kind; + if let Some(ty) = subs.types().next(); + if let Generator(_, subs, _) = ty.kind; + if match_type(cx, subs.as_generator().return_ty(def_id, cx.tcx), &paths::RESULT); + then { + span_lint( + cx, + MISSING_ERRORS_DOC, + span, + "docs for function returning `Result` missing `# Errors` section", + ); } } } From ea5ac40a24dfac8e2b9834234d014872104e8444 Mon Sep 17 00:00:00 2001 From: daxpedda Date: Mon, 17 Feb 2020 14:19:09 +0100 Subject: [PATCH 5/5] Remove use of `TyKind`. --- clippy_lints/src/doc.rs | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/clippy_lints/src/doc.rs b/clippy_lints/src/doc.rs index 1bbb66189662..4a7d2a8b640e 100644 --- a/clippy_lints/src/doc.rs +++ b/clippy_lints/src/doc.rs @@ -2,7 +2,7 @@ use crate::utils::{get_trait_def_id, implements_trait, is_entrypoint_fn, match_t use if_chain::if_chain; use itertools::Itertools; use rustc::lint::in_external_macro; -use rustc::ty::TyKind; +use rustc::ty; use rustc_data_structures::fx::FxHashSet; use rustc_hir as hir; use rustc_lint::{LateContext, LateLintPass}; @@ -225,7 +225,6 @@ fn lint_for_missing_headers<'a, 'tcx>( "docs for function returning `Result` missing `# Errors` section", ); } else { - use TyKind::*; if_chain! { if let Some(body_id) = body_id; if let Some(future) = get_trait_def_id(cx, &paths::FUTURE); @@ -233,9 +232,9 @@ fn lint_for_missing_headers<'a, 'tcx>( let mir = cx.tcx.optimized_mir(def_id); let ret_ty = mir.return_ty(); if implements_trait(cx, ret_ty, future, &[]); - if let Opaque(_, subs) = ret_ty.kind; - if let Some(ty) = subs.types().next(); - if let Generator(_, subs, _) = ty.kind; + if let ty::Opaque(_, subs) = ret_ty.kind; + if let Some(gen) = subs.types().next(); + if let ty::Generator(_, subs, _) = gen.kind; if match_type(cx, subs.as_generator().return_ty(def_id, cx.tcx), &paths::RESULT); then { span_lint(