From 3a6cac7c06656b410fafe73713a72a9697fcb899 Mon Sep 17 00:00:00 2001 From: J-ZhengLi Date: Fri, 8 Mar 2024 09:32:38 +0800 Subject: [PATCH 1/2] add `rewrite_struct` proc-macro and test case --- tests/ui/auxiliary/proc_macro_attr.rs | 28 +++++++++++++---- tests/ui/missing_doc.rs | 12 ++++++++ tests/ui/missing_doc.stderr | 43 ++++++++++++++++++--------- 3 files changed, 64 insertions(+), 19 deletions(-) diff --git a/tests/ui/auxiliary/proc_macro_attr.rs b/tests/ui/auxiliary/proc_macro_attr.rs index 75f7a20f961..7b8f45f0b12 100644 --- a/tests/ui/auxiliary/proc_macro_attr.rs +++ b/tests/ui/auxiliary/proc_macro_attr.rs @@ -12,7 +12,7 @@ use syn::spanned::Spanned; use syn::token::Star; use syn::{ parse_macro_input, parse_quote, FnArg, ImplItem, ItemFn, ItemImpl, ItemTrait, Lifetime, Pat, PatIdent, PatType, - Signature, TraitItem, Type, + Signature, TraitItem, Type, ItemStruct, Visibility, }; #[proc_macro_attribute] @@ -101,9 +101,7 @@ pub fn fake_main(_attr: TokenStream, item: TokenStream) -> TokenStream { let mut item = parse_macro_input!(item as ItemFn); let span = item.block.brace_token.span; - if item.sig.asyncness.is_some() { - item.sig.asyncness = None; - } + item.sig.asyncness = None; let crate_name = quote! { fake_crate }; let block = item.block; @@ -128,7 +126,7 @@ pub fn fake_main(_attr: TokenStream, item: TokenStream) -> TokenStream { #[proc_macro_attribute] pub fn fake_desugar_await(_args: TokenStream, input: TokenStream) -> TokenStream { - let mut async_fn = syn::parse_macro_input!(input as syn::ItemFn); + let mut async_fn = parse_macro_input!(input as syn::ItemFn); for stmt in &mut async_fn.block.stmts { if let syn::Stmt::Expr(syn::Expr::Match(syn::ExprMatch { expr: scrutinee, .. }), _) = stmt { @@ -145,3 +143,23 @@ pub fn fake_desugar_await(_args: TokenStream, input: TokenStream) -> TokenStream quote!(#async_fn).into() } + +#[proc_macro_attribute] +pub fn rewrite_struct(_args: TokenStream, input: TokenStream) -> TokenStream { + let mut item_struct = parse_macro_input!(input as syn::ItemStruct); + // remove struct attributes including doc comments. + item_struct.attrs = vec![]; + if let Visibility::Public(token) = item_struct.vis { + // set vis to `pub(crate)` to trigger `missing_docs_in_private_items` lint. + let new_vis: Visibility = syn::parse_quote_spanned!(token.span() => pub(crate)); + item_struct.vis = new_vis; + } + if let syn::Fields::Named(fields) = &mut item_struct.fields { + for field in &mut fields.named { + // remove all attributes from fields as well. + field.attrs = vec![]; + } + } + + quote!(#item_struct).into() +} diff --git a/tests/ui/missing_doc.rs b/tests/ui/missing_doc.rs index 9bfad3b96cf..9c936d7fa23 100644 --- a/tests/ui/missing_doc.rs +++ b/tests/ui/missing_doc.rs @@ -1,5 +1,6 @@ //@needs-asm-support //@aux-build: proc_macros.rs +//@aux-build: proc_macro_attr.rs #![warn(clippy::missing_docs_in_private_items)] // When denying at the crate level, be sure to not get random warnings from the @@ -8,6 +9,8 @@ //! Some garbage docs for the crate here #![doc = "More garbage"] +#[macro_use] +extern crate proc_macro_attr; extern crate proc_macros; use proc_macros::with_span; @@ -112,3 +115,12 @@ with_span!(span pub enum FooPm3 { A, B(u32), C { field: u32 }}); with_span!(span pub fn foo_pm() {}); with_span!(span pub static FOO_PM: u32 = 0;); with_span!(span pub const FOO2_PM: u32 = 0;); + +// issue #12197 +// Undocumented field originated inside of spanned proc-macro attribute +/// Some dox for struct. +#[rewrite_struct] +pub struct Test { + /// Dox + a: u8, +} diff --git a/tests/ui/missing_doc.stderr b/tests/ui/missing_doc.stderr index 7e66e2097e9..09c296e01fb 100644 --- a/tests/ui/missing_doc.stderr +++ b/tests/ui/missing_doc.stderr @@ -1,5 +1,5 @@ error: missing documentation for a type alias - --> tests/ui/missing_doc.rs:16:1 + --> tests/ui/missing_doc.rs:19:1 | LL | type Typedef = String; | ^^^^^^^^^^^^^^^^^^^^^^ @@ -8,19 +8,19 @@ LL | type Typedef = String; = help: to override `-D warnings` add `#[allow(clippy::missing_docs_in_private_items)]` error: missing documentation for a module - --> tests/ui/missing_doc.rs:19:1 + --> tests/ui/missing_doc.rs:22:1 | LL | mod module_no_dox {} | ^^^^^^^^^^^^^^^^^^^^ error: missing documentation for a function - --> tests/ui/missing_doc.rs:25:1 + --> tests/ui/missing_doc.rs:28:1 | LL | fn foo3() {} | ^^^^^^^^^^^^ error: missing documentation for an enum - --> tests/ui/missing_doc.rs:39:1 + --> tests/ui/missing_doc.rs:42:1 | LL | / enum Baz { LL | | BazA { a: isize, b: isize }, @@ -29,43 +29,43 @@ LL | | } | |_^ error: missing documentation for a variant - --> tests/ui/missing_doc.rs:40:5 + --> tests/ui/missing_doc.rs:43:5 | LL | BazA { a: isize, b: isize }, | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: missing documentation for a struct field - --> tests/ui/missing_doc.rs:40:12 + --> tests/ui/missing_doc.rs:43:12 | LL | BazA { a: isize, b: isize }, | ^^^^^^^^ error: missing documentation for a struct field - --> tests/ui/missing_doc.rs:40:22 + --> tests/ui/missing_doc.rs:43:22 | LL | BazA { a: isize, b: isize }, | ^^^^^^^^ error: missing documentation for a variant - --> tests/ui/missing_doc.rs:41:5 + --> tests/ui/missing_doc.rs:44:5 | LL | BarB, | ^^^^ error: missing documentation for a constant - --> tests/ui/missing_doc.rs:65:1 + --> tests/ui/missing_doc.rs:68:1 | LL | const FOO: u32 = 0; | ^^^^^^^^^^^^^^^^^^^ error: missing documentation for a static - --> tests/ui/missing_doc.rs:74:1 + --> tests/ui/missing_doc.rs:77:1 | LL | static BAR: u32 = 0; | ^^^^^^^^^^^^^^^^^^^^ error: missing documentation for a module - --> tests/ui/missing_doc.rs:83:1 + --> tests/ui/missing_doc.rs:86:1 | LL | / mod internal_impl { LL | | /// dox @@ -77,16 +77,31 @@ LL | | } | |_^ error: missing documentation for a function - --> tests/ui/missing_doc.rs:88:5 + --> tests/ui/missing_doc.rs:91:5 | LL | fn undocumented3() {} | ^^^^^^^^^^^^^^^^^^^^^ error: missing documentation for a function - --> tests/ui/missing_doc.rs:94:9 + --> tests/ui/missing_doc.rs:97:9 | LL | fn also_undocumented2() {} | ^^^^^^^^^^^^^^^^^^^^^^^^^^ -error: aborting due to 13 previous errors +error: missing documentation for a struct + --> tests/ui/missing_doc.rs:123:1 + | +LL | / pub struct Test { +LL | | /// Dox +LL | | a: u8, +LL | | } + | |_^ + +error: missing documentation for a struct field + --> tests/ui/missing_doc.rs:125:5 + | +LL | a: u8, + | ^^^^^ + +error: aborting due to 15 previous errors From adc91e4913875e430d946852e28f8a18e40afdfb Mon Sep 17 00:00:00 2001 From: J-ZhengLi Date: Fri, 8 Mar 2024 16:32:47 +0800 Subject: [PATCH 2/2] support manually search for docs in case attr was removed by proc macros --- clippy_lints/src/missing_doc.rs | 64 ++++++++++++++++++++++++--- tests/ui/auxiliary/proc_macro_attr.rs | 4 +- tests/ui/missing_doc.stderr | 17 +------ 3 files changed, 62 insertions(+), 23 deletions(-) diff --git a/clippy_lints/src/missing_doc.rs b/clippy_lints/src/missing_doc.rs index bf4af7946f4..6878fb3349d 100644 --- a/clippy_lints/src/missing_doc.rs +++ b/clippy_lints/src/missing_doc.rs @@ -8,6 +8,7 @@ use clippy_utils::attrs::is_doc_hidden; use clippy_utils::diagnostics::span_lint; use clippy_utils::is_from_proc_macro; +use clippy_utils::source::snippet_opt; use rustc_ast::ast::{self, MetaItem, MetaItemKind}; use rustc_hir as hir; use rustc_hir::def_id::LocalDefId; @@ -32,6 +33,13 @@ declare_clippy_lint! { "detects missing documentation for private members" } +macro_rules! note_prev_span_then_ret { + ($prev_span:expr, $span:expr) => {{ + $prev_span = Some($span); + return; + }}; +} + pub struct MissingDoc { /// Whether to **only** check for missing documentation in items visible within the current /// crate. For example, `pub(crate)` items. @@ -39,6 +47,8 @@ pub struct MissingDoc { /// Stack of whether #[doc(hidden)] is set /// at each level which has lint attributes. doc_hidden_stack: Vec, + /// Used to keep tracking of the previous item, field or variants etc, to get the search span. + prev_span: Option, } impl Default for MissingDoc { @@ -54,6 +64,7 @@ impl MissingDoc { Self { crate_items_only, doc_hidden_stack: vec![false], + prev_span: None, } } @@ -108,7 +119,8 @@ impl MissingDoc { let has_doc = attrs .iter() - .any(|a| a.doc_str().is_some() || Self::has_include(a.meta())); + .any(|a| a.doc_str().is_some() || Self::has_include(a.meta())) + || matches!(self.search_span(sp), Some(span) if span_to_snippet_contains_docs(cx, span)); if !has_doc { span_lint( @@ -119,6 +131,32 @@ impl MissingDoc { ); } } + + /// Return a span to search for doc comments manually. + /// + /// # Example + /// ```ignore + /// fn foo() { ... } + /// ^^^^^^^^^^^^^^^^ prev_span + /// ↑ + /// | search_span | + /// ↓ + /// fn bar() { ... } + /// ^^^^^^^^^^^^^^^^ cur_span + /// ``` + fn search_span(&self, cur_span: Span) -> Option { + let prev_span = self.prev_span?; + let start_pos = if prev_span.contains(cur_span) { + // In case when the prev_span is an entire struct, or enum, + // and the current span is a field, or variant, we need to search from + // the starting pos of the previous span. + prev_span.lo() + } else { + prev_span.hi() + }; + let search_span = cur_span.with_lo(start_pos).with_hi(cur_span.lo()); + Some(search_span) + } } impl_lint_pass!(MissingDoc => [MISSING_DOCS_IN_PRIVATE_ITEMS]); @@ -138,6 +176,10 @@ impl<'tcx> LateLintPass<'tcx> for MissingDoc { self.check_missing_docs_attrs(cx, CRATE_DEF_ID, attrs, cx.tcx.def_span(CRATE_DEF_ID), "the", "crate"); } + fn check_crate_post(&mut self, _: &LateContext<'tcx>) { + self.prev_span = None; + } + fn check_item(&mut self, cx: &LateContext<'tcx>, it: &'tcx hir::Item<'_>) { match it.kind { hir::ItemKind::Fn(..) => { @@ -145,7 +187,7 @@ impl<'tcx> LateLintPass<'tcx> for MissingDoc { if it.ident.name == sym::main { let at_root = cx.tcx.local_parent(it.owner_id.def_id) == CRATE_DEF_ID; if at_root { - return; + note_prev_span_then_ret!(self.prev_span, it.span); } } }, @@ -164,7 +206,7 @@ impl<'tcx> LateLintPass<'tcx> for MissingDoc { | hir::ItemKind::ForeignMod { .. } | hir::ItemKind::GlobalAsm(..) | hir::ItemKind::Impl { .. } - | hir::ItemKind::Use(..) => return, + | hir::ItemKind::Use(..) => note_prev_span_then_ret!(self.prev_span, it.span), }; let (article, desc) = cx.tcx.article_and_description(it.owner_id.to_def_id()); @@ -173,6 +215,7 @@ impl<'tcx> LateLintPass<'tcx> for MissingDoc { if !is_from_proc_macro(cx, it) { self.check_missing_docs_attrs(cx, it.owner_id.def_id, attrs, it.span, article, desc); } + self.prev_span = Some(it.span); } fn check_trait_item(&mut self, cx: &LateContext<'tcx>, trait_item: &'tcx hir::TraitItem<'_>) { @@ -182,16 +225,17 @@ impl<'tcx> LateLintPass<'tcx> for MissingDoc { if !is_from_proc_macro(cx, trait_item) { self.check_missing_docs_attrs(cx, trait_item.owner_id.def_id, attrs, trait_item.span, article, desc); } + self.prev_span = Some(trait_item.span); } fn check_impl_item(&mut self, cx: &LateContext<'tcx>, impl_item: &'tcx hir::ImplItem<'_>) { // If the method is an impl for a trait, don't doc. if let Some(cid) = cx.tcx.associated_item(impl_item.owner_id).impl_container(cx.tcx) { if cx.tcx.impl_trait_ref(cid).is_some() { - return; + note_prev_span_then_ret!(self.prev_span, impl_item.span); } } else { - return; + note_prev_span_then_ret!(self.prev_span, impl_item.span); } let (article, desc) = cx.tcx.article_and_description(impl_item.owner_id.to_def_id()); @@ -199,6 +243,7 @@ impl<'tcx> LateLintPass<'tcx> for MissingDoc { if !is_from_proc_macro(cx, impl_item) { self.check_missing_docs_attrs(cx, impl_item.owner_id.def_id, attrs, impl_item.span, article, desc); } + self.prev_span = Some(impl_item.span); } fn check_field_def(&mut self, cx: &LateContext<'tcx>, sf: &'tcx hir::FieldDef<'_>) { @@ -208,6 +253,7 @@ impl<'tcx> LateLintPass<'tcx> for MissingDoc { self.check_missing_docs_attrs(cx, sf.def_id, attrs, sf.span, "a", "struct field"); } } + self.prev_span = Some(sf.span); } fn check_variant(&mut self, cx: &LateContext<'tcx>, v: &'tcx hir::Variant<'_>) { @@ -215,5 +261,13 @@ impl<'tcx> LateLintPass<'tcx> for MissingDoc { if !is_from_proc_macro(cx, v) { self.check_missing_docs_attrs(cx, v.def_id, attrs, v.span, "a", "variant"); } + self.prev_span = Some(v.span); } } + +fn span_to_snippet_contains_docs(cx: &LateContext<'_>, search_span: Span) -> bool { + let Some(snippet) = snippet_opt(cx, search_span) else { + return false; + }; + snippet.lines().rev().any(|line| line.trim().starts_with("///")) +} diff --git a/tests/ui/auxiliary/proc_macro_attr.rs b/tests/ui/auxiliary/proc_macro_attr.rs index 7b8f45f0b12..b550d13b0b9 100644 --- a/tests/ui/auxiliary/proc_macro_attr.rs +++ b/tests/ui/auxiliary/proc_macro_attr.rs @@ -11,8 +11,8 @@ use quote::{quote, quote_spanned}; use syn::spanned::Spanned; use syn::token::Star; use syn::{ - parse_macro_input, parse_quote, FnArg, ImplItem, ItemFn, ItemImpl, ItemTrait, Lifetime, Pat, PatIdent, PatType, - Signature, TraitItem, Type, ItemStruct, Visibility, + parse_macro_input, parse_quote, FnArg, ImplItem, ItemFn, ItemImpl, ItemStruct, ItemTrait, Lifetime, Pat, PatIdent, + PatType, Signature, TraitItem, Type, Visibility, }; #[proc_macro_attribute] diff --git a/tests/ui/missing_doc.stderr b/tests/ui/missing_doc.stderr index 09c296e01fb..ef0f96a5b71 100644 --- a/tests/ui/missing_doc.stderr +++ b/tests/ui/missing_doc.stderr @@ -88,20 +88,5 @@ error: missing documentation for a function LL | fn also_undocumented2() {} | ^^^^^^^^^^^^^^^^^^^^^^^^^^ -error: missing documentation for a struct - --> tests/ui/missing_doc.rs:123:1 - | -LL | / pub struct Test { -LL | | /// Dox -LL | | a: u8, -LL | | } - | |_^ - -error: missing documentation for a struct field - --> tests/ui/missing_doc.rs:125:5 - | -LL | a: u8, - | ^^^^^ - -error: aborting due to 15 previous errors +error: aborting due to 13 previous errors