linux-kernel - Re: [PATCH 0/2] checkpatch.pl: add warning for // comments on private Rust items

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <D9BBEQHO1XMG.2C5Q7FF02BDLJ@proton.me>
Date: Sun, 20 Apr 2025 08:15:24 +0000
From: Benno Lossin <benno.lossin@...ton.me>
To: Arnaud Lecomte <contact@...aud-lcm.com>, Andy Whitcroft <apw@...onical.com>, Joe Perches <joe@...ches.com>, Dwaipayan Ray <dwaipayanray1@...il.com>, Lukas Bulwahn <lukas.bulwahn@...il.com>, Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>, Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>, Björn Roy Baron <bjorn3_gh@...tonmail.com>, Andreas Hindborg <a.hindborg@...nel.org>, Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>, Danilo Krummrich <dakr@...nel.org>, Nathan Chancellor <nathan@...nel.org>, Nick Desaulniers <nick.desaulniers+lkml@...il.com>, Bill Wendling <morbo@...gle.com>, Justin Stitt <justinstitt@...gle.com>
Cc: linux-kernel@...r.kernel.org, rust-for-linux@...r.kernel.org, llvm@...ts.linux.dev, skhan@...uxfoundation.org
Subject: Re: [PATCH 0/2] checkpatch.pl: add warning for // comments on private Rust items

On Sat Apr 19, 2025 at 10:24 PM CEST, Arnaud Lecomte wrote:
> Hi,
>
> Background
> ----------
>
> The Rust-for-Linux project currently lacks enforcement of documentation for private Rust items,
> as highlighted in https://github.com/Rust-for-Linux/linux/issues/1157.
> While rustc already lints missing documentation for public items, private items remain unchecked.
> This patch series aims to close that gap by ensuring proper documentation practices
> for private Rust items in the kernel.
> As underlined in this issue:
> https://github.com/Rust-for-Linux/linux/issues/1157, the purposes of
> this patch serie is to ensure the proper documentation of private rust
> items. Public items missing documentation are already linted by rustc.
>
> The actual solution comes in several parts
> ------------------------------------------
>
>  1) Patch 1 : Implements detection logic to emit warnings for improperly
>  documented private Rust items (e.g., // comments instead of ///).
>  2) Patch 2 : Adds an auto-fix mechanism via the --fix option to help
>  developers correct documentation issues.
>
> Results
> --------------------

Thanks for this helpful example, I'd recommend you to run your modified
version on real patches from the list and/or on already existing commits
in the kernel.

>
> The following implementation has been tested against this input file:
> // SPDX-License-Identifier: GPL-2.0

[...]

> pub struct Point2D {
>   pub x: f32,
>   pub y: f32
> }
>
> mod test_module {
>     // Module inner comment - should not trigger
> }
>
> // Comment before macro - should not trigger
> macro_rules! my_macro {

I think we should also trigger this for macros. All macros are private
by default and only made public with the `#[macro_export]` annotation.

>     // Comment inside macro - should not trigger
>     () => {};
> }
>
> // Comment before unsafe block - should not trigger
> unsafe {
>     // Comment inside unsafe block - should not trigger
>     let x = 5;
> }
>
> // Comment with unsafe word - should trigger
> fn with_unsafe_keyword() {
>     println!("test");
> }
>
> // Comment with code example: - should trigger
> // let x = 5; - should trigger

Code examples are usually wrapped in '```', so they are another
indicator that it probably should be a doc-comment.

> fn with_code_example() {
>     println!("test");
> }
>
> // NOTE: important consideration - should not trigger
> fn note_marker() -> bool {
>     true
> }
>
> // Comment with code example: - should trigger

I'm not 100% convinced that this should trigger. We have several cases
of the following (in some cases the function is public, I don't
remember if we have a private case):

    /// Normal function docs...
    // We probably should refactor XYZ.
    fn foo() {}

There it should not trigger.

---
Cheers,
Benno

> /// let x = 5; - should not trigger
> fn with_mixed_comments() {
>     println!("test");
> }