Auto merge of #12993 - GuillaumeGomez:too_long_first_doc_paragraph, r…

…=Centri3

Add new `too_long_first_doc_paragraph` first paragraph lint

Fixes #12989.

changelog: Add new `too_long_first_doc_paragraph` first paragraph lint

CHANGELOG.md

@@ -5914,6 +5914,7 @@ Released 2018-09-13
 [`to_string_in_format_args`]: https://rust-lang.github.io/rust-clippy/master/index.html#to_string_in_format_args
 [`to_string_trait_impl`]: https://rust-lang.github.io/rust-clippy/master/index.html#to_string_trait_impl
 [`todo`]: https://rust-lang.github.io/rust-clippy/master/index.html#todo
+[`too_long_first_doc_paragraph`]: https://rust-lang.github.io/rust-clippy/master/index.html#too_long_first_doc_paragraph
 [`too_many_arguments`]: https://rust-lang.github.io/rust-clippy/master/index.html#too_many_arguments
 [`too_many_lines`]: https://rust-lang.github.io/rust-clippy/master/index.html#too_many_lines
 [`toplevel_ref_arg`]: https://rust-lang.github.io/rust-clippy/master/index.html#toplevel_ref_arg

clippy_lints/src/declared_lints.rs

@@ -144,6 +144,7 @@ pub static LINTS: &[&crate::LintInfo] = &[
     crate::doc::NEEDLESS_DOCTEST_MAIN_INFO,
     crate::doc::SUSPICIOUS_DOC_COMMENTS_INFO,
     crate::doc::TEST_ATTR_IN_DOCTEST_INFO,
+    crate::doc::TOO_LONG_FIRST_DOC_PARAGRAPH_INFO,
     crate::doc::UNNECESSARY_SAFETY_DOC_INFO,
     crate::double_parens::DOUBLE_PARENS_INFO,
     crate::drop_forget_ref::DROP_NON_DROP_INFO,

clippy_lints/src/doc/mod.rs

@@ -1,4 +1,6 @@
 mod lazy_continuation;
+mod too_long_first_doc_paragraph;
+
 use clippy_config::Conf;
 use clippy_utils::attrs::is_doc_hidden;
 use clippy_utils::diagnostics::{span_lint, span_lint_and_help};
@@ -422,6 +424,38 @@ declare_clippy_lint! {
     "require every line of a paragraph to be indented and marked"
 }
 
+declare_clippy_lint! {
+    /// ### What it does
+    /// Checks if the first line in the documentation of items listed in module page is too long.
+    ///
+    /// ### Why is this bad?
+    /// Documentation will show the first paragraph of the doscstring in the summary page of a
+    /// module, so having a nice, short summary in the first paragraph is part of writing good docs.
+    ///
+    /// ### Example
+    /// ```no_run
+    /// /// A very short summary.
+    /// /// A much longer explanation that goes into a lot more detail about
+    /// /// how the thing works, possibly with doclinks and so one,
+    /// /// and probably spanning a many rows.
+    /// struct Foo {}
+    /// ```
+    /// Use instead:
+    /// ```no_run
+    /// /// A very short summary.
+    /// ///
+    /// /// A much longer explanation that goes into a lot more detail about
+    /// /// how the thing works, possibly with doclinks and so one,
+    /// /// and probably spanning a many rows.
+    /// struct Foo {}
+    /// ```
+    #[clippy::version = "1.81.0"]
+    pub TOO_LONG_FIRST_DOC_PARAGRAPH,
+    style,
+    "ensure that the first line of a documentation paragraph isn't too long"
+}
+
+#[derive(Clone)]
 pub struct Documentation {
     valid_idents: FxHashSet<String>,
     check_private_items: bool,
@@ -448,6 +482,7 @@ impl_lint_pass!(Documentation => [
     SUSPICIOUS_DOC_COMMENTS,
     EMPTY_DOCS,
     DOC_LAZY_CONTINUATION,
+    TOO_LONG_FIRST_DOC_PARAGRAPH,
 ]);
 
 impl<'tcx> LateLintPass<'tcx> for Documentation {
@@ -457,39 +492,50 @@ impl<'tcx> LateLintPass<'tcx> for Documentation {
         };
 
         match cx.tcx.hir_node(cx.last_node_with_lint_attrs) {
-            Node::Item(item) => match item.kind {
-                ItemKind::Fn(sig, _, body_id) => {
-                    if !(is_entrypoint_fn(cx, item.owner_id.to_def_id()) || in_external_macro(cx.tcx.sess, item.span)) {
-                        let body = cx.tcx.hir().body(body_id);
-
-                        let panic_info = FindPanicUnwrap::find_span(cx, cx.tcx.typeck(item.owner_id), body.value);
-                        missing_headers::check(
+            Node::Item(item) => {
+                too_long_first_doc_paragraph::check(
+                    cx,
+                    item,
+                    attrs,
+                    headers.first_paragraph_len,
+                    self.check_private_items,
+                );
+                match item.kind {
+                    ItemKind::Fn(sig, _, body_id) => {
+                        if !(is_entrypoint_fn(cx, item.owner_id.to_def_id())
+                            || in_external_macro(cx.tcx.sess, item.span))
+                        {
+                            let body = cx.tcx.hir().body(body_id);
+
+                            let panic_info = FindPanicUnwrap::find_span(cx, cx.tcx.typeck(item.owner_id), body.value);
+                            missing_headers::check(
+                                cx,
+                                item.owner_id,
+                                sig,
+                                headers,
+                                Some(body_id),
+                                panic_info,
+                                self.check_private_items,
+                            );
+                        }
+                    },
+                    ItemKind::Trait(_, unsafety, ..) => match (headers.safety, unsafety) {
+                        (false, Safety::Unsafe) => span_lint(
                             cx,
-                            item.owner_id,
-                            sig,
-                            headers,
-                            Some(body_id),
-                            panic_info,
-                            self.check_private_items,
-                        );
-                    }
-                },
-                ItemKind::Trait(_, unsafety, ..) => match (headers.safety, unsafety) {
-                    (false, Safety::Unsafe) => span_lint(
-                        cx,
-                        MISSING_SAFETY_DOC,
-                        cx.tcx.def_span(item.owner_id),
-                        "docs for unsafe trait missing `# Safety` section",
-                    ),
-                    (true, Safety::Safe) => span_lint(
-                        cx,
-                        UNNECESSARY_SAFETY_DOC,
-                        cx.tcx.def_span(item.owner_id),
-                        "docs for safe trait have unnecessary `# Safety` section",
-                    ),
+                            MISSING_SAFETY_DOC,
+                            cx.tcx.def_span(item.owner_id),
+                            "docs for unsafe trait missing `# Safety` section",
+                        ),
+                        (true, Safety::Safe) => span_lint(
+                            cx,
+                            UNNECESSARY_SAFETY_DOC,
+                            cx.tcx.def_span(item.owner_id),
+                            "docs for safe trait have unnecessary `# Safety` section",
+                        ),
+                        _ => (),
+                    },
                     _ => (),
-                },
-                _ => (),
+                }
             },
             Node::TraitItem(trait_item) => {
                 if let TraitItemKind::Fn(sig, ..) = trait_item.kind
@@ -547,6 +593,7 @@ struct DocHeaders {
     safety: bool,
     errors: bool,
     panics: bool,
+    first_paragraph_len: usize,
 }
 
 /// Does some pre-processing on raw, desugared `#[doc]` attributes such as parsing them and
@@ -653,6 +700,7 @@ fn check_doc<'a, Events: Iterator<Item = (pulldown_cmark::Event<'a>, Range<usize
     let mut paragraph_range = 0..0;
     let mut code_level = 0;
     let mut blockquote_level = 0;
+    let mut is_first_paragraph = true;
 
     let mut containers = Vec::new();
 
@@ -720,6 +768,10 @@ fn check_doc<'a, Events: Iterator<Item = (pulldown_cmark::Event<'a>, Range<usize
                 }
                 ticks_unbalanced = false;
                 paragraph_range = range;
+                if is_first_paragraph {
+                    headers.first_paragraph_len = doc[paragraph_range.clone()].chars().count();
+                    is_first_paragraph = false;
+                }
             },
             End(TagEnd::Heading(_) | TagEnd::Paragraph | TagEnd::Item) => {
                 if let End(TagEnd::Heading(_)) = event {

clippy_lints/src/doc/too_long_first_doc_paragraph.rs

@@ -0,0 +1,91 @@
+use rustc_ast::ast::Attribute;
+use rustc_errors::Applicability;
+use rustc_hir::{Item, ItemKind};
+use rustc_lint::LateContext;
+
+use clippy_utils::diagnostics::span_lint_and_then;
+use clippy_utils::is_from_proc_macro;
+use clippy_utils::source::snippet_opt;
+
+use super::TOO_LONG_FIRST_DOC_PARAGRAPH;
+
+pub(super) fn check(
+    cx: &LateContext<'_>,
+    item: &Item<'_>,
+    attrs: &[Attribute],
+    mut first_paragraph_len: usize,
+    check_private_items: bool,
+) {
+    if !check_private_items && !cx.effective_visibilities.is_exported(item.owner_id.def_id) {
+        return;
+    }
+    if first_paragraph_len <= 200
+        || !matches!(
+            item.kind,
+            // This is the list of items which can be documented AND are displayed on the module
+            // page. So associated items or impl blocks are not part of this list.
+            ItemKind::Static(..)
+                | ItemKind::Const(..)
+                | ItemKind::Fn(..)
+                | ItemKind::Macro(..)
+                | ItemKind::Mod(..)
+                | ItemKind::TyAlias(..)
+                | ItemKind::Enum(..)
+                | ItemKind::Struct(..)
+                | ItemKind::Union(..)
+                | ItemKind::Trait(..)
+                | ItemKind::TraitAlias(..)
+        )
+    {
+        return;
+    }
+
+    let mut spans = Vec::new();
+    let mut should_suggest_empty_doc = false;
+
+    for attr in attrs {
+        if let Some(doc) = attr.doc_str() {
+            spans.push(attr.span);
+            let doc = doc.as_str();
+            let doc = doc.trim();
+            if spans.len() == 1 {
+                // We make this suggestion only if the first doc line ends with a punctuation
+                // because it might just need to add an empty line with `///`.
+                should_suggest_empty_doc = doc.ends_with('.') || doc.ends_with('!') || doc.ends_with('?');
+            }
+            let len = doc.chars().count();
+            if len >= first_paragraph_len {
+                break;
+            }
+            first_paragraph_len -= len;
+        }
+    }
+
+    let &[first_span, .., last_span] = spans.as_slice() else {
+        return;
+    };
+    if is_from_proc_macro(cx, item) {
+        return;
+    }
+
+    span_lint_and_then(
+        cx,
+        TOO_LONG_FIRST_DOC_PARAGRAPH,
+        first_span.with_hi(last_span.lo()),
+        "first doc comment paragraph is too long",
+        |diag| {
+            if should_suggest_empty_doc
+                && let Some(second_span) = spans.get(1)
+                && let new_span = first_span.with_hi(second_span.lo()).with_lo(first_span.hi())
+                && let Some(snippet) = snippet_opt(cx, new_span)
+            {
+                diag.span_suggestion(
+                    new_span,
+                    "add an empty line",
+                    format!("{snippet}///\n"),
+                    Applicability::MachineApplicable,
+                );
+            }
+        },
+    );
+}

clippy_utils/src/lib.rs

@@ -149,6 +149,7 @@ macro_rules! extract_msrv_attr {
 
 /// If the given expression is a local binding, find the initializer expression.
 /// If that initializer expression is another local binding, find its initializer again.
+///
 /// This process repeats as long as possible (but usually no more than once). Initializer
 /// expressions with adjustments are ignored. If this is not desired, use [`find_binding_init`]
 /// instead.
@@ -179,6 +180,7 @@ pub fn expr_or_init<'a, 'b, 'tcx: 'b>(cx: &LateContext<'tcx>, mut expr: &'a Expr
 }
 
 /// Finds the initializer expression for a local binding. Returns `None` if the binding is mutable.
+///
 /// By only considering immutable bindings, we guarantee that the returned expression represents the
 /// value of the binding wherever it is referenced.
 ///
@@ -431,12 +433,12 @@ pub fn qpath_generic_tys<'tcx>(qpath: &QPath<'tcx>) -> impl Iterator<Item = &'tc
         })
 }
 
-/// THIS METHOD IS DEPRECATED and will eventually be removed since it does not match against the
+/// THIS METHOD IS DEPRECATED. Matches a `QPath` against a slice of segment string literals.
+///
+/// This method is deprecated and will eventually be removed since it does not match against the
 /// entire path or resolved `DefId`. Prefer using `match_def_path`. Consider getting a `DefId` from
 /// `QPath::Resolved.1.res.opt_def_id()`.
 ///
-/// Matches a `QPath` against a slice of segment string literals.
-///
 /// There is also `match_path` if you are dealing with a `rustc_hir::Path` instead of a
 /// `rustc_hir::QPath`.
 ///
@@ -485,12 +487,12 @@ pub fn is_path_diagnostic_item<'tcx>(
     path_def_id(cx, maybe_path).map_or(false, |id| cx.tcx.is_diagnostic_item(diag_item, id))
 }
 
-/// THIS METHOD IS DEPRECATED and will eventually be removed since it does not match against the
+/// THIS METHOD IS DEPRECATED. Matches a `Path` against a slice of segment string literals.
+///
+/// This method is deprecated and will eventually be removed since it does not match against the
 /// entire path or resolved `DefId`. Prefer using `match_def_path`. Consider getting a `DefId` from
 /// `QPath::Resolved.1.res.opt_def_id()`.
 ///
-/// Matches a `Path` against a slice of segment string literals.
-///
 /// There is also `match_qpath` if you are dealing with a `rustc_hir::QPath` instead of a
 /// `rustc_hir::Path`.
 ///
@@ -905,6 +907,7 @@ pub fn is_default_equivalent_call(cx: &LateContext<'_>, repl_func: &Expr<'_>) ->
 }
 
 /// Returns true if the expr is equal to `Default::default()` of it's type when evaluated.
+///
 /// It doesn't cover all cases, for example indirect function calls (some of std
 /// functions are supported) but it is the best we have.
 pub fn is_default_equivalent(cx: &LateContext<'_>, e: &Expr<'_>) -> bool {
@@ -1061,6 +1064,7 @@ impl std::ops::BitOrAssign for CaptureKind {
 }
 
 /// Given an expression referencing a local, determines how it would be captured in a closure.
+///
 /// Note as this will walk up to parent expressions until the capture can be determined it should
 /// only be used while making a closure somewhere a value is consumed. e.g. a block, match arm, or
 /// function argument (other than a receiver).
@@ -2365,8 +2369,9 @@ pub fn fn_def_id_with_node_args<'tcx>(
 }
 
 /// Returns `Option<String>` where String is a textual representation of the type encapsulated in
-/// the slice iff the given expression is a slice of primitives (as defined in the
-/// `is_recursively_primitive_type` function) and `None` otherwise.
+/// the slice iff the given expression is a slice of primitives.
+///
+/// (As defined in the `is_recursively_primitive_type` function.) Returns `None` otherwise.
 pub fn is_slice_of_primitives(cx: &LateContext<'_>, expr: &Expr<'_>) -> Option<String> {
     let expr_type = cx.typeck_results().expr_ty_adjusted(expr);
     let expr_kind = expr_type.kind();

clippy_utils/src/macros.rs

@@ -150,10 +150,11 @@ pub fn first_node_macro_backtrace(cx: &LateContext<'_>, node: &impl HirNode) ->
 }
 
 /// If `node` is the "first node" in a macro expansion, returns `Some` with the `ExpnId` of the
-/// macro call site (i.e. the parent of the macro expansion). This generally means that `node`
-/// is the outermost node of an entire macro expansion, but there are some caveats noted below.
-/// This is useful for finding macro calls while visiting the HIR without processing the macro call
-/// at every node within its expansion.
+/// macro call site (i.e. the parent of the macro expansion).
+///
+/// This generally means that `node` is the outermost node of an entire macro expansion, but there
+/// are some caveats noted below. This is useful for finding macro calls while visiting the HIR
+/// without processing the macro call at every node within its expansion.
 ///
 /// If you already have immediate access to the parent node, it is simpler to
 /// just check the context of that span directly (e.g. `parent.span.from_expansion()`).

clippy_utils/src/source.rs

@@ -589,9 +589,10 @@ pub fn snippet_block_with_context<'a>(
     (reindent_multiline(snip, true, indent), from_macro)
 }
 
-/// Same as `snippet_with_applicability`, but first walks the span up to the given context. This
-/// will result in the macro call, rather than the expansion, if the span is from a child context.
-/// If the span is not from a child context, it will be used directly instead.
+/// Same as `snippet_with_applicability`, but first walks the span up to the given context.
+///
+/// This will result in the macro call, rather than the expansion, if the span is from a child
+/// context. If the span is not from a child context, it will be used directly instead.
 ///
 /// e.g. Given the expression `&vec![]`, getting a snippet from the span for `vec![]` as a HIR node
 /// would result in `box []`. If given the context of the address of expression, this function will
@@ -634,9 +635,10 @@ fn snippet_with_context_sess<'a>(
 }
 
 /// Walks the span up to the target context, thereby returning the macro call site if the span is
-/// inside a macro expansion, or the original span if it is not. Note this will return `None` in the
-/// case of the span being in a macro expansion, but the target context is from expanding a macro
-/// argument.
+/// inside a macro expansion, or the original span if it is not.
+///
+/// Note this will return `None` in the case of the span being in a macro expansion, but the target
+/// context is from expanding a macro argument.
 ///
 /// Given the following
 ///