Skip to content

Consider checkpatch.pl rule to emit a warning on // on top of private items #1157

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
ojeda opened this issue Apr 16, 2025 · 1 comment
Open

Consider checkpatch.pl rule to emit a warning on // on top of private items #1157

ojeda opened this issue Apr 16, 2025 · 1 comment
Labels
• docs Related to `Documentation/rust/`, `samples/`, generated docs, doctests, typos... good first issue Good for newcomers medium Expected to be an issue of medium difficulty to resolve.

Comments

@ojeda
Copy link
Member

ojeda commented Apr 16, 2025 •
edited
Loading

Consider adding a scripts/checkpatch.pl rule for Rust code that emits a warning (not an error) on comments being on top of a private item rather than documentation, e.g. from https://lore.kernel.org/rust-for-linux/20250416095943.f3jxy55bamekscst@vireshk-i7/:

       // Returns a reference to the underlying [`cpufreq::Table`].
       #[inline]
       fn table(&self) -> &cpufreq::Table {
           // SAFETY: The `ptr` is guaranteed by the C code to be valid.
           unsafe { cpufreq::Table::from_raw(self.ptr) }
       }

Please note this should be considered first, i.e. if there is no reasonable way to create the lint in a way that does not have a lot of false positives, then we should not implement it.

For instance, we could use heuristics to limit when we lint, e.g. when we notice things patterns like [`...`] in the text (like in the example above). Similarly, we could think about avoiding to lint when there is already documentation (///) on top of it (that can still be a mistake, but the chances are lower that someone forgot a single /), or when we see TODO: or similar tags.

Public items missing documentation should be already linted by rustc's missing_doc, but perhaps could be covered too.

Please note that the patch would need to go through the checkpatch.pl maintainers, so they will need to agree to the patch.

This requires submitting a proper patch to the LKML and the Rust for Linux mailing list. Please recall to test your changes (including generating the documentation if changed, running the Rust doctests if changed, etc.), to use a proper title for the commit, to sign your commit under the Developer's Certificate of Origin and to add a Suggested-by: tag, and a Link: tag to this issue. Please see https://docs.kernel.org/process/submitting-patches.html and https://rust-for-linux.com/contributing for details.
@ojeda ojeda added good first issue Good for newcomers medium Expected to be an issue of medium difficulty to resolve. • docs Related to `Documentation/rust/`, `samples/`, generated docs, doctests, typos... labels Apr 16, 2025
intel-lab-lkp pushed a commit to intel-lab-lkp/linux that referenced this issue Apr 16, 2025
@ojeda @intel-lab-lkp
docs: rust: explain that /// vs. // applies to private items too
a7d2a2e 
Sometimes kernel developers use `//` for documenting private items,
since those do not get rendered at the moment.

That is reasonable, but the intention behind `///` (and `//!`) vs. `//`
is to convey the distinction between documentation and other kinds of
comments, such as implementation details or TODOs.

It also increases consistency with the public items and thus e.g. allows
to change visibility of an item with less changed involved.

It is not just useful for human readers, but also tooling. For instance,
we may want to eventually generate documentation for private items
(perhaps as a toggle in the HTML UI). On top of that, `rustdoc` lints
as usual for those, too, so we may want to do it even if we do not use
the result.

Thus document this explicitly.

Cc: Viresh Kumar <[email protected]>
Link: https://lore.kernel.org/rust-for-linux/CANiq72n_C7exSOMe5yf-7jKKnhSCv+a9QcD=OE2B_Q2UFBL3Xg@mail.gmail.com/
Link: Rust-for-Linux#1157
Signed-off-by: Miguel Ojeda <[email protected]>
@ArnaudLcm
Copy link

For the reference: https://lore.kernel.org/all/20250419-checkpatch-rust-private-item-comment-v1-0-0f8bc109bd5a@arnaud-lcm.com/

ojeda added a commit to ojeda/linux that referenced this issue May 8, 2025
@ojeda
docs: rust: explain that /// vs. // applies to private items too
89fc274 
Sometimes kernel developers use `//` for documenting private items,
since those do not get rendered at the moment.

That is reasonable, but the intention behind `///` (and `//!`) vs. `//`
is to convey the distinction between documentation and other kinds of
comments, such as implementation details or TODOs.

It also increases consistency with the public items and thus e.g. allows
to change visibility of an item with less changes involved.

It is not just useful for human readers, but also tooling. For instance,
we may want to eventually generate documentation for private items
(perhaps as a toggle in the HTML UI). On top of that, `rustdoc` lints
as usual for those, too, so we may want to do it even if we do not use
the result.

Thus document this explicitly.

Link: https://lore.kernel.org/rust-for-linux/CANiq72n_C7exSOMe5yf-7jKKnhSCv+a9QcD=OE2B_Q2UFBL3Xg@mail.gmail.com/
Link: Rust-for-Linux#1157
Reviewed-by: Alice Ryhl <[email protected]>
Reviewed-by: Christian Schrefl <[email protected]>
Reviewed-by: Viresh Kumar <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
[ Fixed typo. - Miguel ]
Signed-off-by: Miguel Ojeda <[email protected]>
ojeda added a commit to ojeda/linux that referenced this issue May 11, 2025
@ojeda
docs: rust: explain that /// vs. // applies to private items too
744ec6f 
Sometimes kernel developers use `//` for documenting private items,
since those do not get rendered at the moment.

That is reasonable, but the intention behind `///` (and `//!`) vs. `//`
is to convey the distinction between documentation and other kinds of
comments, such as implementation details or TODOs.

It also increases consistency with the public items and thus e.g. allows
to change visibility of an item with less changes involved.

It is not just useful for human readers, but also tooling. For instance,
we may want to eventually generate documentation for private items
(perhaps as a toggle in the HTML UI). On top of that, `rustdoc` lints
as usual for those, too, so we may want to do it even if we do not use
the result.

Thus document this explicitly.

Link: https://lore.kernel.org/rust-for-linux/CANiq72n_C7exSOMe5yf-7jKKnhSCv+a9QcD=OE2B_Q2UFBL3Xg@mail.gmail.com/
Link: Rust-for-Linux#1157
Reviewed-by: Alice Ryhl <[email protected]>
Reviewed-by: Christian Schrefl <[email protected]>
Reviewed-by: Viresh Kumar <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
[ Fixed typo. - Miguel ]
Signed-off-by: Miguel Ojeda <[email protected]>
ojeda added a commit to ojeda/linux that referenced this issue May 11, 2025
@ojeda
docs: rust: explain that /// vs. // applies to private items too
79d04e7 
Sometimes kernel developers use `//` for documenting private items,
since those do not get rendered at the moment.

That is reasonable, but the intention behind `///` (and `//!`) vs. `//`
is to convey the distinction between documentation and other kinds of
comments, such as implementation details or TODOs.

It also increases consistency with the public items and thus e.g. allows
to change visibility of an item with less changes involved.

It is not just useful for human readers, but also tooling. For instance,
we may want to eventually generate documentation for private items
(perhaps as a toggle in the HTML UI). On top of that, `rustdoc` lints
as usual for those, too, so we may want to do it even if we do not use
the result.

Thus document this explicitly.

Link: https://lore.kernel.org/rust-for-linux/CANiq72n_C7exSOMe5yf-7jKKnhSCv+a9QcD=OE2B_Q2UFBL3Xg@mail.gmail.com/
Link: Rust-for-Linux#1157
Reviewed-by: Alice Ryhl <[email protected]>
Reviewed-by: Christian Schrefl <[email protected]>
Reviewed-by: Viresh Kumar <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
[ Fixed typo. - Miguel ]
Signed-off-by: Miguel Ojeda <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
• docs Related to `Documentation/rust/`, `samples/`, generated docs, doctests, typos... good first issue Good for newcomers medium Expected to be an issue of medium difficulty to resolve.
Milestone
No milestone
Development

No branches or pull requests
2 participants
@ojeda @ArnaudLcm