Add new too_long_first_doc_paragraph first paragraph lint

CHANGELOG.md

@@ -5913,6 +5913,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

@@ -148,6 +148,7 @@ pub(crate) 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: &'static 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,94 @@
+use rustc_ast::ast::Attribute;
+use rustc_errors::Applicability;
+use rustc_hir::{Item, ItemKind};
+use rustc_lint::LateContext;
+
+use clippy_utils::diagnostics::{span_lint, span_lint_and_then};
+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 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)
+ {
+ 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| {
+ diag.span_suggestion(
+ new_span,
+ "add an empty line",
+ format!("{snippet}///\n"),
+ Applicability::MachineApplicable,
+ );
+ },
+ );
+ return;
+ }
+ span_lint(
+ cx,
+ TOO_LONG_FIRST_DOC_PARAGRAPH,
+ first_span.with_hi(last_span.lo()),
+ "first doc comment paragraph is too long",
+ );
+}

clippy_utils/src/lib.rs

@@ -150,6 +150,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.
@@ -180,6 +181,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.
 ///
@@ -428,12 +430,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`.
 ///
@@ -482,12 +484,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`.
 ///
@@ -903,6 +905,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 {
@@ -1059,6 +1062,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).
@@ -2361,8 +2365,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

@@ -527,9 +527,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
@@ -572,9 +573,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
 ///