Add new `too_long_first_doc_paragraph` first paragraph lint #12993

GuillaumeGomez · 2024-06-24T16:42:50Z

changelog: Add new too_long_first_doc_paragraph first paragraph lint

rustbot · 2024-06-24T16:42:55Z

rustbot has assigned @Centri3.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

bitfield · 2024-06-24T17:38:08Z

clippy_lints/src/doc/mod.rs

@@ -420,6 +421,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 not


Suggested change

/// Checks if the first line in the documentation of items listed in module page is not

/// Checks if the first line in the documentation of items listed in module page is

Clippy lints usually say something like "Checks for [BAD PATTERN]", so in this case your lint might say that it checks whether the first line is too long [which would be bad].

Good point!

J-ZhengLi · 2024-06-25T09:53:16Z

clippy_lints/src/doc/too_long_first_doc_paragraph.rs

+    item_kind: ItemKind<'_>,
+    mut first_paragraph_len: usize,
+) {
+    if first_paragraph_len <= 80


Just being curious 🤔~ why limit it to 80? Is it some kind of standard? Should it be mentioned on the lint documentation or having a configuration otherwise?

Not sure if having a config would improve things much. This lint is mostly to prevent things like:

80 seems way too low for a default limit, if there's only one line: I don't think we should emit a warning here.
This looks fine to me, there's nothing to split up here really:

We can take a bigger limit. But in any case, the first line is supposed to be a summary, not a description. But I don't consider this a universal thing, hence why I put it as allow by default.

I think a good limit would be whenever it splits it on a 1080p/720p monitor in fullscreen mode on the browser (that way it's consistent), maybe a little less so there's wiggle room on higher zoom or an even smaller monitor/window (somehow)

hence why I put it as allow by default.

It's currently style, but IMO it should stay like that

It's worth referring back to what the rustdoc book says about this:

It is recommended that each item's documentation follows this basic structure:

[short sentence explaining what it is]

[more detailed explanation]

[at least one code example that users can copy/paste to try it]

[even more advanced explanations if necessary]

Everything before the first empty line will be reused to describe the component in searches and module overviews. For example, the function std::env::args() above will be shown on the std::env module documentation. It is good practice to keep the summary to one line: concise writing is a goal of good documentation.
—https://doc.rust-lang.org/rustdoc/how-to-write-documentation.html

The std documentation sticks to this pretty well; it's mostly one short sentence (sometimes two: "Removes and returns the last element in the map. The key of this element is the maximum key that was in the map.") On the other hand, sometimes the 'more detailed explanation' paragraph is joined on to the short intro sentence when it doesn't need to be.

So, how long is "one line"? For what it's worth, on my system doc.rust-lang.org wraps the text at 132 characters per line, so what about warning at something like 250 characters? That would in any case be a fairly long sentence.

@GuillaumeGomez, you could try running your lint over the std documentation with this setting and see how often it trips?

If your type is named SomethinVeryLongAndBlaBlaBlaBlaBlaBlaBlaBlaBlaBlaBlaBlaBlaBlaBlaBlaBlaBlaBlaBla, it will wrap the text at much less, hence why I picked 80 as a default. (It's an issue in rustdoc that needs to be fixed too.)

Big +1 for copying the explanations from the book.

@GuillaumeGomez, you could try running your lint over the std documentation with this setting and see how often it trips?

I have no idea how to run a local clippy on anything else. If you know how or the location explaining how, it'd be very useful. ;)

I have no idea how to run a local clippy on anything else. If you know how or the location explaining how, it'd be very useful. ;)

I don't know about running it on the standard library specifically, since it's special, but for normal cargo projects you can just run cargo dev lint path/to/cargo/project and it should just work.

std::env in particular has 13 out of 23 items that have >80 characters in their first paragraph. slice has 169 characters.
This rate seems fairly high for the standard library if we were to emit a warning for all of those.

I also did a lintcheck run on the top 200 popular crates and there are 2008 warnings in total of this lint.

I have no idea how to run a local clippy on anything else. If you know how or the location explaining how, it'd be very useful. ;)

I don't know about running it on the standard library specifically, since it's special, but for normal cargo projects you can just run cargo dev lint path/to/cargo/project and it should just work.

Nice, thanks!

std::env in particular has 13 out of 23 items that have >80 characters in their first paragraph. slice has 169 characters. This rate seems fairly high for the standard library if we were to emit a warning for all of those.

I also did a lintcheck run on the top 200 popular crates and there are 2008 warnings in total of this lint.

Well, unless I miss something, style is allow by default, no? If so should be fine. Would still be better to not emit this many warnings though...

clippy_lints/src/doc/mod.rs

GuillaumeGomez · 2024-06-28T16:02:08Z

I increased the limit to 100 and I also applied suggestions from @flip1995 to suggest to add an empty doc line to separate the first sentence from the rest when applicable.

GuillaumeGomez · 2024-07-01T09:26:13Z

Any idea how I could disable just this lint on clippy tests by any chance?

flip1995 · 2024-07-02T17:39:53Z

I would rather take a closer look at these and improve the lint a bit. E.g. this seems like a FP to me: https://github.com/rust-lang/rust-clippy/actions/runs/9715600206/job/26817360799?pr=12993#step:5:3958

error: first doc comment paragraph is too long
  --> tests/ui/index_refutable_slice/if_let_slice_binding.rs:168:1
   |
LL | / /// The lint will ignore bindings with sub patterns as it would be hard
LL | | /// to build correct suggestions for these instances :)
   | |_

This is still a really short comment and IMO fine to have in documentation.

GuillaumeGomez · 2024-07-04T09:28:57Z

But it's not a short description. In this case it should be:

/// The lint will ignore bindings with sub patterns.

For the second part (the "why?"), it should be part of the rest of the doc comment.

flip1995 · 2024-07-04T10:16:13Z

I disagree. If the explanation for the first statement is as short as this, I'd rather be able to read it in the docs.rs overview, than having to click on the item to try to understand why this is the case.

I think it is hard to argue that the first (long) version is bad doc writing. This is a screenshot when using half my screen. It's barely 2 lines in the summary, but I have all information I need, without having to click on the item.

In any case, I don't think this should warn on private items, that won't be shown in the documentation. Or at least, this should be configurable, with the default being not to lint on private items.

GuillaumeGomez · 2024-07-08T13:23:47Z

I can config for the length of the line (default to 120?) and if it should warn on private items (default to false?). What do you think?

flip1995 · 2024-07-08T14:53:06Z

I think that would be a good step. However, I don't know how much it helps to make the line length configurable.

GuillaumeGomez · 2024-07-08T14:58:20Z

I don't either. We either need to pick the maximum length or allow users to change it however they want. But it seems like we can't agree on a line length here already. 😆

bitfield · 2024-07-08T15:20:04Z

I think it's a wise principle for lints to err on the side of being conservative (that is, giving the user's code the benefit of the doubt). A lint that triggers too often will soon be silenced, whereas a lint that's a little too easy-going is still useful.

I looked again at a few samples from the standard library, and the longest intro paragraphs that seemed to me to keep the spirit of "one short sentence" were no longer than about 150 characters. Example:

Returns vector content as a slice of T, along with the remaining spare capacity of the vector as a slice of MaybeUninit<T>.

Clearly if that sentence were longer, it would not be short! But it also seems quite reasonable to me. Adding a 50% safety margin would give us a limit of 225.

There are lots of cases in the stdlib docs where this limit is exceeded, but I think those cases should trigger the lint! For example:

Consumes and leaks the Vec, returning a mutable reference to the contents, &'a mut [T]. Note that the type T must outlive the chosen lifetime 'a. If the type has only static references, or none at all, then this may be chosen to be 'static.

The second and third sentences here should really move to the next paragraph, so the lint would already be delivering value here. A limit of 250 characters would have missed this, so that's clearly too high. A limit of less than 200 would already be incorrectly catching lots of cases that, on inspection, are quite reasonable.

Thus, anything within the 200-250 range is defensible, but it seems hard to argue for a limit outside that.

GuillaumeGomez · 2024-07-08T16:34:18Z

Let's go for 200 then. We can always add the config later on if needed.

flip1995 · 2024-07-09T09:02:14Z

200 sounds good to me. The default comment width is 80 in rustfmt. So 200 is 2.5x of that. Which is in line with @bitfield argument to add a 50% safety margin, just from a different starting point. 2 lines of first line documentation seems fine to me, as weird as that sounds.

If you think there's value in making this configurable, go for it. It doesn't cost much.

I still haven't looked at the implementation, but if the lint produces a suggestion, I think a good idea would be to suggest a break after the first . or other punctuation found in the too-long-line.

GuillaumeGomez · 2024-07-09T14:41:28Z

Updated the limit to 200. Added configuration for private items and fixed dogfood.

flip1995 · 2024-07-10T09:55:19Z

Lintcheck diff summary looks good and those all seem to be cases where the documentation could be improved. Sometimes trivially.

I'll let @Centri3 do the code review of the lint, but the behavior now seems good to me.

bors · 2024-07-17T19:04:06Z

☔ The latest upstream changes (presumably #13088) made this pull request unmergeable. Please resolve the merge conflicts.

GuillaumeGomez · 2024-07-17T19:10:54Z

Fixed merge conflict.

Centri3

LGTM overall

clippy_lints/src/doc/too_long_first_doc_paragraph.rs

GuillaumeGomez · 2024-07-21T15:30:03Z

Updated!

…items unless config allows it

…T_DOC_PARAGRAPH` lint

GuillaumeGomez · 2024-07-26T09:34:35Z

Can't believe I forgot to run uibless... Fixed the CI.

Centri3 · 2024-08-17T23:40:23Z

clippy_lints/src/doc/too_long_first_doc_paragraph.rs

+    };
+


We should call is_from_proc_macro here, it'll always lint beyond this point

That's a bit tricky here since it needs to check all attributes. Adding it though.

I think it'd be simpler to just check if the item is from a proc macro instead. Gonna do that.

clippy_lints/src/doc/too_long_first_doc_paragraph.rs

Centri3 · 2024-08-17T23:43:49Z

clippy_lints/src/doc/too_long_first_doc_paragraph.rs

+    span_lint(
+        cx,
+        TOO_LONG_FIRST_DOC_PARAGRAPH,
+        first_span.with_hi(last_span.lo()),
+        "first doc comment paragraph is too long",
+    );


nit: This if could be moved into the span_lint_and_then, thus removing this call

…from proc-macro and simplify code to emit lint

GuillaumeGomez · 2024-08-18T18:52:38Z

Updated!

Centri3 · 2024-08-24T15:22:27Z

@bors r+

bors · 2024-08-24T15:22:29Z

📌 Commit a203342 has been approved by Centri3

It is now in the queue for this repository.

bors · 2024-08-24T15:23:37Z

⌛ Testing commit a203342 with merge 30e0b69...

bors · 2024-08-24T15:32:11Z

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: Centri3
Pushing 30e0b69 to master...

Upgrades toolchain to 08/28 Culprit upstream changes: 1. rust-lang/rust#128812 2. rust-lang/rust#128703 3. rust-lang/rust#127679 4. rust-lang/rust-clippy#12993 5. rust-lang/cargo#14370 6. rust-lang/rust#128806 Resolves #3429 By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 and MIT licenses.

Rust 1.82.0's Clippy is introducing [1][2] a new lint, `too_long_first_doc_paragraph` [3], which is intended to catch titles of code documentation items that are too long (likely because no title was provided and the item documentation starts with a paragraph). This lint does not currently trigger anywhere, but it does detect a couple cases we had in private cases if checking for private items gets enabled (which we will do in the next commit): error: first doc comment paragraph is too long --> rust/kernel/init/__internal.rs:18:1 | 18 | / /// This is the module-internal type implementing `PinInit` and `Init`. It is unsafe to create this 19 | | /// type, since the closure needs to fulfill the same safety requirement as the 20 | | /// `__pinned_init`/`__init` functions. | |_ | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#too_long_first_doc_paragraph = note: `-D clippy::too-long-first-doc-paragraph` implied by `-D warnings` = help: to override `-D warnings` add `#[allow(clippy::too_long_first_doc_paragraph)]` error: first doc comment paragraph is too long --> rust/kernel/sync/arc/std_vendor.rs:3:1 | 3 | / //! The contents of this file come from the Rust standard library, hosted in 4 | | //! the <https://github.com/rust-lang/rust> repository, licensed under 5 | | //! "Apache-2.0 OR MIT" and adapted for kernel use. For copyright details, 6 | | //! see <https://github.com/rust-lang/rust/blob/master/COPYRIGHT>. | |_ | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#too_long_first_doc_paragraph Thus clean the two instances we hit and enable the lint. In addition, since we have have a second `std_vendor.rs` file with a similar header, do the same there too (even if that one does not trigger the lint, because it is `doc(hidden)`). Link: rust-lang/rust#129531 [1] Link: rust-lang/rust-clippy#12993 [2] Link: https://rust-lang.github.io/rust-clippy/master/index.html#/too_long_first_doc_paragraph [3] Signed-off-by: Miguel Ojeda <ojeda@kernel.org>

Rust 1.82.0's Clippy is introducing [1][2] a new warn-by-default lint, `too_long_first_doc_paragraph` [3], which is intended to catch titles of code documentation items that are too long (likely because no title was provided and the item documentation starts with a paragraph). This lint does not currently trigger anywhere, but it does detect a couple cases we had in private cases if checking for private items gets enabled (which we will do in the next commit): error: first doc comment paragraph is too long --> rust/kernel/init/__internal.rs:18:1 | 18 | / /// This is the module-internal type implementing `PinInit` and `Init`. It is unsafe to create this 19 | | /// type, since the closure needs to fulfill the same safety requirement as the 20 | | /// `__pinned_init`/`__init` functions. | |_ | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#too_long_first_doc_paragraph = note: `-D clippy::too-long-first-doc-paragraph` implied by `-D warnings` = help: to override `-D warnings` add `#[allow(clippy::too_long_first_doc_paragraph)]` error: first doc comment paragraph is too long --> rust/kernel/sync/arc/std_vendor.rs:3:1 | 3 | / //! The contents of this file come from the Rust standard library, hosted in 4 | | //! the <https://github.com/rust-lang/rust> repository, licensed under 5 | | //! "Apache-2.0 OR MIT" and adapted for kernel use. For copyright details, 6 | | //! see <https://github.com/rust-lang/rust/blob/master/COPYRIGHT>. | |_ | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#too_long_first_doc_paragraph Thus clean the two instances we hit and enable the lint. In addition, since we have have a second `std_vendor.rs` file with a similar header, do the same there too (even if that one does not trigger the lint, because it is `doc(hidden)`). Link: rust-lang/rust#129531 [1] Link: rust-lang/rust-clippy#12993 [2] Link: https://rust-lang.github.io/rust-clippy/master/index.html#/too_long_first_doc_paragraph [3] Signed-off-by: Miguel Ojeda <ojeda@kernel.org>

Rust 1.82.0's Clippy is introducing [1][2] a new warn-by-default lint, `too_long_first_doc_paragraph` [3], which is intended to catch titles of code documentation items that are too long (likely because no title was provided and the item documentation starts with a paragraph). This lint does not currently trigger anywhere, but it does detect a couple cases we had in private cases if checking for private items gets enabled (which we will do in the next commit): error: first doc comment paragraph is too long --> rust/kernel/init/__internal.rs:18:1 | 18 | / /// This is the module-internal type implementing `PinInit` and `Init`. It is unsafe to create this 19 | | /// type, since the closure needs to fulfill the same safety requirement as the 20 | | /// `__pinned_init`/`__init` functions. | |_ | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#too_long_first_doc_paragraph = note: `-D clippy::too-long-first-doc-paragraph` implied by `-D warnings` = help: to override `-D warnings` add `#[allow(clippy::too_long_first_doc_paragraph)]` error: first doc comment paragraph is too long --> rust/kernel/sync/arc/std_vendor.rs:3:1 | 3 | / //! The contents of this file come from the Rust standard library, hosted in 4 | | //! the <https://github.com/rust-lang/rust> repository, licensed under 5 | | //! "Apache-2.0 OR MIT" and adapted for kernel use. For copyright details, 6 | | //! see <https://github.com/rust-lang/rust/blob/master/COPYRIGHT>. | |_ | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#too_long_first_doc_paragraph Thus clean the two instances we hit and enable the lint. In addition, since we have a second `std_vendor.rs` file with a similar header, do the same there too (even if that one does not trigger the lint, because it is `doc(hidden)`). Link: rust-lang/rust#129531 [1] Link: rust-lang/rust-clippy#12993 [2] Link: https://rust-lang.github.io/rust-clippy/master/index.html#/too_long_first_doc_paragraph [3] Signed-off-by: Miguel Ojeda <ojeda@kernel.org>

rustbot assigned Centri3 Jun 24, 2024

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jun 24, 2024

bitfield reviewed Jun 24, 2024

View reviewed changes

GuillaumeGomez force-pushed the too_long_first_doc_paragraph branch from 89c4707 to b043fe3 Compare June 24, 2024 20:29

J-ZhengLi reviewed Jun 25, 2024

View reviewed changes

flip1995 reviewed Jun 27, 2024

View reviewed changes

clippy_lints/src/doc/mod.rs Show resolved Hide resolved

GuillaumeGomez force-pushed the too_long_first_doc_paragraph branch from 89123a5 to 63df22d Compare June 28, 2024 16:01

GuillaumeGomez force-pushed the too_long_first_doc_paragraph branch 3 times, most recently from e48c00b to ed9862a Compare July 9, 2024 14:34

GuillaumeGomez force-pushed the too_long_first_doc_paragraph branch from ed9862a to dc8bdbc Compare July 17, 2024 19:10

Centri3 reviewed Jul 21, 2024

View reviewed changes

GuillaumeGomez force-pushed the too_long_first_doc_paragraph branch from dc8bdbc to 367da34 Compare July 21, 2024 15:29

GuillaumeGomez force-pushed the too_long_first_doc_paragraph branch from 367da34 to 7e2cbf0 Compare July 26, 2024 09:16

GuillaumeGomez added 4 commits July 26, 2024 11:21

Add new too_long_first_doc_paragraph first paragraph lint

855a9d1

Set the limit of characters to 200 and don't run the lint on private …

f455587

…items unless config allows it

Fix dogfood lints

4969960

Add comment explaining what the matching items are for `TOO_LONG_FIRS…

3c6e5ef

…T_DOC_PARAGRAPH` lint

GuillaumeGomez force-pushed the too_long_first_doc_paragraph branch from 7e2cbf0 to 3c6e5ef Compare July 26, 2024 09:21

Centri3 approved these changes Aug 17, 2024

View reviewed changes

Do not emit TOO_LONG_FIRST_DOC_PARAGRAPH lint if item is generated …

a203342

…from proc-macro and simplify code to emit lint

bors merged commit 30e0b69 into rust-lang:master Aug 24, 2024
11 checks passed

GuillaumeGomez deleted the too_long_first_doc_paragraph branch August 24, 2024 16:17

This was referenced Aug 24, 2024

new lint: doc_comment_double_space_linebreak #12876

Open

Rewrite empty_line_after_doc_comments and empty_line_after_outer_attr, move them from nursery to suspicious #13091

Merged

AsakuraMizu mentioned this pull request Aug 26, 2024

too_long_first_doc_paragraph incorrect suggestion for module-level doc #13309

Closed

jaisnan mentioned this pull request Aug 28, 2024

Upgrade toolchain to 08/28 model-checking/kani#3454

Merged

Urgau mentioned this pull request Aug 30, 2024

add extra linebreaks so rustdoc can identify the first sentence rust-lang/rust#129793

Merged

	/// Checks if the first line in the documentation of items listed in module page is not
	/// Checks if the first line in the documentation of items listed in module page is

Add new too_long_first_doc_paragraph first paragraph lint #12993

Add new too_long_first_doc_paragraph first paragraph lint #12993

Conversation

GuillaumeGomez commented Jun 24, 2024

rustbot commented Jun 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Centri3 Jun 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuillaumeGomez commented Jun 28, 2024

GuillaumeGomez commented Jul 1, 2024

flip1995 commented Jul 2, 2024

GuillaumeGomez commented Jul 4, 2024

flip1995 commented Jul 4, 2024

GuillaumeGomez commented Jul 8, 2024

flip1995 commented Jul 8, 2024

GuillaumeGomez commented Jul 8, 2024

bitfield commented Jul 8, 2024 • edited Loading

GuillaumeGomez commented Jul 8, 2024

flip1995 commented Jul 9, 2024

GuillaumeGomez commented Jul 9, 2024

flip1995 commented Jul 10, 2024

bors commented Jul 17, 2024

GuillaumeGomez commented Jul 17, 2024

Centri3 left a comment

Choose a reason for hiding this comment

GuillaumeGomez commented Jul 21, 2024

GuillaumeGomez commented Jul 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuillaumeGomez commented Aug 18, 2024

Centri3 commented Aug 24, 2024

bors commented Aug 24, 2024

bors commented Aug 24, 2024

bors commented Aug 24, 2024

Add new `too_long_first_doc_paragraph` first paragraph lint #12993

Add new `too_long_first_doc_paragraph` first paragraph lint #12993

Centri3 Jun 26, 2024 •

edited

Loading

bitfield commented Jul 8, 2024 •

edited

Loading