* [PATCH v3 0/2] checkpatch.pl: Add warning for // comments on private Rust items
@ 2025-04-22 12:56 Arnaud Lecomte
2025-04-22 12:58 ` [PATCH v3 1/2] checkpatch.pl: warn about " Arnaud Lecomte
` (3 more replies)
0 siblings, 4 replies; 11+ messages in thread
From: Arnaud Lecomte @ 2025-04-22 12:56 UTC (permalink / raw)
To: Andy Whitcroft, Joe Perches, Dwaipayan Ray, Lukas Bulwahn,
Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, Danilo Krummrich, Nathan Chancellor,
Nick Desaulniers, Bill Wendling, Justin Stitt
Cc: linux-kernel, rust-for-linux, llvm, contact, skhan
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.
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 ///)
through a set of heuristics.
2) Patch 2 : Adds an auto-fix mechanism via the --fix option to help
developers correct documentation issues.
Results
--------------------
The following implementation has been tested against real patches:
- https://lore.kernel.org/rust-for-linux/dc63bdc4bff8f084714e2c8ff30e4c0d435e85b7.camel@redhat.com/T/#t
- https://lore.kernel.org/rust-for-linux/20250418-ptr-as-ptr-v10-0-3d63d27907aa@gmail.com/T/#t
- https://lore.kernel.org/rust-for-linux/20250420-nova-frts-v1-1-ecd1cca23963@nvidia.com/T/#u
and this file:
// SPDX-License-Identifier: GPL-2.0
// Simple comment - should not trigger
// Returns a reference to the underlying [`cpufreq::Table`]. - should trigger
fn table(&self) -> &cpufreq::Table {
// SAFETY: The `ptr` is guaranteed by the C code to be valid. - should not trigger
unsafe { cpufreq::Table::from_raw(self.ptr) }
}
// Meaning of life - should not trigger
fn test() -> u32 {
42
}
/// Proper doc comment for a private function.
fn proper_doc() -> u32 {
42
}
// TODO: implement more logic - should not trigger
fn todo_marker() -> bool {
true
}
// Just a regular comment not followed by code - should not trigger
pub fn public_function() {
// Public function - should not trigger
}
// Test - should not trigger
struct Point2D {
pub x: f32,
pub y: f32
}
// Test - should not trigger
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 {
// Comment inside macro - should not trigger
() => {};
}
// Comment before public macro - should not trigger
macro_rules! my_public_macro {
}
// Comment before unsafe block - should not trigger
unsafe {
// Comment inside unsafe block - should not trigger
let x = 5;
}
// Comment with multiple
// Line
// Youhouuu
// - should trigger
fn with_unsafe_keyword() {
println!("test");
}
// NOTE: important consideration - should not trigger
fn note_marker() -> bool {
true
}
// Comment with code example: - should trigger
// ``` let x = 5; ```
fn with_mixed_comments() {
println!("test");
}
which led to this output:
WARNING: Consider using `///` if the comment was intended as documentation (line 5).
+// Returns a reference to the underlying [`cpufreq::Table`]. - should trigger
WARNING: Consider using `///` if the comment was intended as documentation (line 72).
+// Comment with multiple
WARNING: Consider using `///` if the comment was intended as documentation (line 73).
+// Line
WARNING: Consider using `///` if the comment was intended as documentation (line 74).
+// Youhouuu
WARNING: Consider using `///` if the comment was intended as documentation (line 75).
+// - should trigger
WARNING: Consider using `///` if the comment was intended as documentation (line 92).
+// Comment with code example: - should trigger
total: 0 errors, 6 warnings, 96 lines checked
NOTE: For some of the reported defects, checkpatch may be able to
mechanically convert to the typical style using --fix or --fix-inplace.
test.rs has style problems, please review.
NOTE: If any of the errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.
To: Andy Whitcroft <apw@canonical.com>
To: Joe Perches <joe@perches.com>
To: Dwaipayan Ray <dwaipayanray1@gmail.com>
To: Lukas Bulwahn <lukas.bulwahn@gmail.com>
To: Miguel Ojeda <ojeda@kernel.org>
To: Alex Gaynor <alex.gaynor@gmail.com>
To: Boqun Feng <boqun.feng@gmail.com>
To: Gary Guo <gary@garyguo.net>
To: Björn Roy Baron <bjorn3_gh@protonmail.com>
To: Benno Lossin <benno.lossin@proton.me>
To: Andreas Hindborg <a.hindborg@kernel.org>
To: Alice Ryhl <aliceryhl@google.com>
To: Trevor Gross <tmgross@umich.edu>
To: Danilo Krummrich <dakr@kernel.org>
To: Nathan Chancellor <nathan@kernel.org>
To: Nick Desaulniers <nick.desaulniers+lkml@gmail.com>
To: Bill Wendling <morbo@google.com>
To: Justin Stitt <justinstitt@google.com>
Cc: linux-kernel@vger.kernel.org
Cc: rust-for-linux@vger.kernel.org
Cc: llvm@lists.linux.dev
Cc: contact@arnaud-lcm.com
Cc: skhan@linuxfoundation.org
Link: https://lore.kernel.org/all/20250420184700.47144-1-contact@arnaud-lcm.com/
Signed-off-by: Arnaud Lecomte <contact@arnaud-lcm.com>
--
2.43.0
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v3 1/2] checkpatch.pl: warn about // comments on private Rust items
2025-04-22 12:56 [PATCH v3 0/2] checkpatch.pl: Add warning for // comments on private Rust items Arnaud Lecomte
@ 2025-04-22 12:58 ` Arnaud Lecomte
2025-04-22 13:46 ` Miguel Ojeda
2025-04-22 12:58 ` [PATCH v3 3/3] checkpatch.pl: --fix support for `//` comments rust private items Arnaud Lecomte
` (2 subsequent siblings)
3 siblings, 1 reply; 11+ messages in thread
From: Arnaud Lecomte @ 2025-04-22 12:58 UTC (permalink / raw)
To: Andy Whitcroft, Joe Perches, Dwaipayan Ray, Lukas Bulwahn,
Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, Danilo Krummrich, Nathan Chancellor,
Nick Desaulniers, Bill Wendling, Justin Stitt
Cc: linux-kernel, rust-for-linux, llvm, contact, skhan
Add a new checkpatch.pl warning to detect //-style comments above
private Rust items that were likely intended to be doc comments (///).
This helps catch documentation that won't appear in rustdoc output.
The detection uses multiple heuristics to identify likely doc comments:
- Comments containing markdown
- Comments starting with an imperative tone
- Comments with mention of params
- Comments with code block
- Comments with keywords: Returns, ...
- Block comments longer than 3 lines.
- Comments with references: @see, @link, ...
Comments are only flagged if they:
- Appear above private items
- Don't contain obvious non-doc patterns (TODO, FIXME)
- Score sufficiently on heuristics
The warning triggered then suggests to convert flagged comments to
doc comments.
Signed-off-by: Arnaud Lecomte <contact@arnaud-lcm.com>
---
Changes in v3:
- Add heuristics to decide whether we should trigger the warning on comments
- Link to v2: https://lore.kernel.org/all/20250420194806.54354-1-contact@arnaud-lcm.com/
---
scripts/checkpatch.pl | 110 ++++++++++++++++++++++++++++++++++++++++++
1 file changed, 110 insertions(+)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 3d22bf863eec..eb564a119d68 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1987,6 +1987,97 @@ sub ctx_locate_comment {
chomp($current_comment);
return($current_comment);
}
+
+sub ctx_detect_missing_doc_comment_behind_rust_private_item {
+ my ($linenr) = @_;
+ my $start_line = $linenr - 1;
+ my $idx = $start_line;
+
+ # Skip if it's a public Rust item (starts with 'pub')
+ return unless $rawlines[$start_line] !~ /^\+?\s*pub(\s|\([^)]*\)\s)/;
+
+ # Check if it's an empty line
+ return unless $rawlines[$start_line] !~ /^\s*\+?\s*$/;
+
+ # Check if it's a comment
+ return unless $rawlines[$start_line] !~ /^\+?\s*\/\/\/?\/?.*$/;
+
+ # Check if it is a private macro
+ if ($rawlines[$start_line] =~ /^\+?\s*macro_rules!\s+([a-zA-Z_][a-zA-Z0-9_]*|#?[a-zA-Z_][a-zA-Z0-9_]*)\s*(?:\{|$)/ && $idx > 0) {
+ return unless $rawlines[$idx -1] !~ /^\+?\s*#\s*\[\s*macro_export\s*\]\s*$/;
+ } else {
+ # Check if it's a Rust item
+ return unless $rawlines[$start_line] =~ /^\+?\s*\b(fn|struct|const|enum|trait|type|mod|impl|use)\b/;
+ }
+
+ # Walk backwards through non-empty lines
+ $idx--;
+ my @comment_lines;
+ while ($idx >= 0 && $rawlines[$idx] !~ /^\s*\+?\s*$/) {
+ # Stop if a doc comment is found
+ last if $rawlines[$idx] =~ m@^\+?\s*///@;
+ # Clean the line first (remove diff markers, trim spaces)
+ my $clean_line = $rawlines[$idx];
+ $clean_line =~ s/^\+?\s*//;
+ $clean_line =~ s/\s*$//;
+ if($clean_line =~ m@^\+?\s*//@) { # Only add comments
+ unshift @comment_lines, $clean_line;
+ }
+ $idx--;
+ }
+
+ # No comments to check
+ return unless @comment_lines;
+
+ # Heuristic: Check for documentation keywords
+ my $keywords = qr/Returns|Arguments|Examples?|Panics|Safety|Note|Errors|See\s+Also/i;
+ my $has_keywords = grep { /\/\/\s*$keywords/ } @comment_lines;
+
+ # Heuristic: Markdown syntax
+ my $markdown = qr{
+ `[^`]+` # Inline code
+ | \*\*[^*]+\*\* # Bold with proper boundaries
+ | \b_[^_\s][^_]*_\b # Italic with word boundaries
+ | \#[^#\s] # Headings like # Title
+ | ^\s*```[\w]*\s*$ # Code block marker line (allow optional language)
+ }x;
+
+ my $has_markdown = grep { /$markdown/ } @comment_lines;
+
+ # Heuristic: Parameter mentions
+ my $item_line = $rawlines[$start_line];
+ my @params = ($item_line =~ /(\b\w+)\s*:\s*\w+/g); # Extract param names
+ my $param_mentions = 0;
+ if (@params) {
+ $param_mentions = grep { my $line = $_; grep { $line =~ /\b$_\b/ } @params } @comment_lines;
+ }
+
+ # Heuristic: Start with imperative tones
+ my $first_line = $comment_lines[0];
+ my $imperative_tone = $first_line =~ /^\s*\/\/\s*(Returns?|Computes?|Checks?|Converts?|Creates?)\b/i;
+
+ # Heuristic: References
+ my $has_refs = grep { /\[`\w+`\]|\@see|\@link/i } @comment_lines;
+
+ # Heuristic: Code block
+ my $has_code_block = grep { /```|\buse\s+\w+::|^\s*\/\/\s*\w+\(.*\)/ } @comment_lines;
+ # Combine heuristics
+ my $score = 0;
+ $score += 1 if $has_keywords;
+ $score += 2 if $has_markdown;
+ $score += 1 if $param_mentions;
+ $score += 2 if $imperative_tone;
+ $score += 2 if $has_refs;
+ $score += 2 if scalar(@comment_lines) >= 3;
+ $score += 2 if $has_code_block;
+
+ # Trigger if score meets threshold
+ return unless $score >= 2;
+
+ return ($idx+1, $start_line);
+}
+
+
sub ctx_has_comment {
my ($first_line, $end_line) = @_;
my $cmt = ctx_locate_comment($first_line, $end_line);
@@ -2950,6 +3041,25 @@ sub process {
}
}
+# check for incorrect use of `//` instead of `\\\` for doc comments in Rust
+ if ($perl_version_ok && $realfile =~ /\.rs$/) {
+ my ($start_l, $end_l) = ctx_detect_missing_doc_comment_behind_rust_private_item($linenr);
+ if (defined($start_l) && defined($end_l)) {
+ for (my $i = $start_l; $i <= $end_l; $i++) {
+ my $comment_line = $rawlines[$i - 1];
+ next unless $comment_line =~ m@^\+//([^/].*)$@;
+ my $comment = $1;
+ # Skip obvious non-doc comments
+ next if $comment =~ m@^\s*(?:TODO|FIXME|XXX|NOTE|HACK|SAFETY):?@i;
+ next if $comment =~ m@^\s*[[:punct:]]{2,}@;
+ my $line_issue = $i - 3;
+ WARN("POSSIBLE_MISSING_RUST_DOC",
+ "Consider using `///` if the comment was intended as documentation (line $line_issue).\n" .
+ "$here\n$comment_line");
+ }
+ }
+ }
+
# Check the patch for a From:
if (decode("MIME-Header", $line) =~ /^From:\s*(.*)/) {
$author = $1;
--
2.43.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v3 3/3] checkpatch.pl: --fix support for `//` comments rust private items
2025-04-22 12:56 [PATCH v3 0/2] checkpatch.pl: Add warning for // comments on private Rust items Arnaud Lecomte
2025-04-22 12:58 ` [PATCH v3 1/2] checkpatch.pl: warn about " Arnaud Lecomte
@ 2025-04-22 12:58 ` Arnaud Lecomte
2025-04-22 13:02 ` [PATCH v3 2/2 resend] " Arnaud Lecomte
2025-04-22 14:10 ` [PATCH v3 0/2] checkpatch.pl: Add warning for // comments on private Rust items Miguel Ojeda
3 siblings, 0 replies; 11+ messages in thread
From: Arnaud Lecomte @ 2025-04-22 12:58 UTC (permalink / raw)
To: Andy Whitcroft, Joe Perches, Dwaipayan Ray, Lukas Bulwahn,
Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, Danilo Krummrich, Nathan Chancellor,
Nick Desaulniers, Bill Wendling, Justin Stitt
Cc: linux-kernel, rust-for-linux, llvm, contact, skhan
Extend the Rust private item documentation checker (added in the previous
patch) to support automatic fixes when the `--fix` option is enabled.
Link: https://lore.kernel.org/all/20250419222505.9009-1-contact@arnaud-lcm.com/
Signed-off-by: Arnaud Lecomte <contact@arnaud-lcm.com>
---
scripts/checkpatch.pl | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index eb564a119d68..ad3ae5fdd830 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3053,9 +3053,11 @@ sub process {
next if $comment =~ m@^\s*(?:TODO|FIXME|XXX|NOTE|HACK|SAFETY):?@i;
next if $comment =~ m@^\s*[[:punct:]]{2,}@;
my $line_issue = $i - 3;
- WARN("POSSIBLE_MISSING_RUST_DOC",
+ if (WARN("POSSIBLE_MISSING_RUST_DOC",
"Consider using `///` if the comment was intended as documentation (line $line_issue).\n" .
- "$here\n$comment_line");
+ "$here\n$comment_line") && $fix) {
+ $fixed[$i - 1] =~ s/^\+(\/\/)/+$1\//;
+ }
}
}
}
--
2.43.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v3 2/2 resend] checkpatch.pl: --fix support for `//` comments rust private items
2025-04-22 12:56 [PATCH v3 0/2] checkpatch.pl: Add warning for // comments on private Rust items Arnaud Lecomte
2025-04-22 12:58 ` [PATCH v3 1/2] checkpatch.pl: warn about " Arnaud Lecomte
2025-04-22 12:58 ` [PATCH v3 3/3] checkpatch.pl: --fix support for `//` comments rust private items Arnaud Lecomte
@ 2025-04-22 13:02 ` Arnaud Lecomte
2025-04-22 14:10 ` [PATCH v3 0/2] checkpatch.pl: Add warning for // comments on private Rust items Miguel Ojeda
3 siblings, 0 replies; 11+ messages in thread
From: Arnaud Lecomte @ 2025-04-22 13:02 UTC (permalink / raw)
To: Andy Whitcroft, Joe Perches, Dwaipayan Ray, Lukas Bulwahn,
Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, Danilo Krummrich, Nathan Chancellor,
Nick Desaulniers, Bill Wendling, Justin Stitt
Cc: linux-kernel, rust-for-linux, llvm, contact, skhan
Extend the Rust private item documentation checker (added in the previous
patch) to support automatic fixes when the `--fix` option is enabled.
Link: https://lore.kernel.org/all/20250419222505.9009-1-contact@arnaud-lcm.com/
Signed-off-by: Arnaud Lecomte <contact@arnaud-lcm.com>
---
scripts/checkpatch.pl | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index eb564a119d68..ad3ae5fdd830 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3053,9 +3053,11 @@ sub process {
next if $comment =~ m@^\s*(?:TODO|FIXME|XXX|NOTE|HACK|SAFETY):?@i;
next if $comment =~ m@^\s*[[:punct:]]{2,}@;
my $line_issue = $i - 3;
- WARN("POSSIBLE_MISSING_RUST_DOC",
+ if (WARN("POSSIBLE_MISSING_RUST_DOC",
"Consider using `///` if the comment was intended as documentation (line $line_issue).\n" .
- "$here\n$comment_line");
+ "$here\n$comment_line") && $fix) {
+ $fixed[$i - 1] =~ s/^\+(\/\/)/+$1\//;
+ }
}
}
}
--
2.43.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v3 1/2] checkpatch.pl: warn about // comments on private Rust items
2025-04-22 12:58 ` [PATCH v3 1/2] checkpatch.pl: warn about " Arnaud Lecomte
@ 2025-04-22 13:46 ` Miguel Ojeda
2025-04-22 14:37 ` Arnaud Lecomte
0 siblings, 1 reply; 11+ messages in thread
From: Miguel Ojeda @ 2025-04-22 13:46 UTC (permalink / raw)
To: Arnaud Lecomte
Cc: Andy Whitcroft, Joe Perches, Dwaipayan Ray, Lukas Bulwahn,
Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, Danilo Krummrich, Nathan Chancellor,
Nick Desaulniers, Bill Wendling, Justin Stitt, linux-kernel,
rust-for-linux, llvm, skhan
On Tue, Apr 22, 2025 at 2:58 PM Arnaud Lecomte <contact@arnaud-lcm.com> wrote:
>
> The detection uses multiple heuristics to identify likely doc comments:
> - Comments containing markdown
Markdown is required in both documentation and comments, so I don't
think we can use some of those, e.g. inline code spans (i.e.
backticks) are common (and actually expected) in comments. Something
like a title (i.e. `#`) or intra-doc links are uncommon, though.
> - Comments starting with an imperative tone
Some people document using the third-person, e.g. some functions say
"Returns ..." like you have below. (It is not easy to enforce
kernel-wide a single style here, thus so far we don't.)
(Looking briefly at the code) Ah, I think you are covering both, good.
> - Comments with references: @see, @link, ...
Do you mean Markdown references? Or javadoc-like ones?
(Looking again at the code...) I think you are referring to actually
strings like `@see`. Hmm... I don't think we have those -- the only
`@` I would expect in a comment are thinks like emails or the
`rustdoc` syntax to disambiguate the "kind" of item, e.g.
`type@NotThreadSafe`. I do see a `@maxlen` somewhere, but that should
have been an inline code span, and anyway it is not a `@see` or
`@link`. But I may be confused here?
> Comments are only flagged if they:
> - Appear above private items
> - Don't contain obvious non-doc patterns (TODO, FIXME)
> - Score sufficiently on heuristics
Nice work! I wasn't expecting something with actual weighted scoring,
but if the maintainers are OK with something as involved as that, then
I guess it is fine. We may need to tweak the scoring in the future,
but it may be a good experiment.
Thanks!
Cheers,
Miguel
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 0/2] checkpatch.pl: Add warning for // comments on private Rust items
2025-04-22 12:56 [PATCH v3 0/2] checkpatch.pl: Add warning for // comments on private Rust items Arnaud Lecomte
` (2 preceding siblings ...)
2025-04-22 13:02 ` [PATCH v3 2/2 resend] " Arnaud Lecomte
@ 2025-04-22 14:10 ` Miguel Ojeda
2025-04-22 14:38 ` Arnaud Lecomte
3 siblings, 1 reply; 11+ messages in thread
From: Miguel Ojeda @ 2025-04-22 14:10 UTC (permalink / raw)
To: Arnaud Lecomte
Cc: Andy Whitcroft, Joe Perches, Dwaipayan Ray, Lukas Bulwahn,
Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, Danilo Krummrich, Nathan Chancellor,
Nick Desaulniers, Bill Wendling, Justin Stitt, linux-kernel,
rust-for-linux, llvm, skhan
On Tue, Apr 22, 2025 at 2:56 PM Arnaud Lecomte <contact@arnaud-lcm.com> wrote:
>
> 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.
I think this background wasn't changed from the previous version --
you can include a changelog in the cover letter too (some do that
only, instead of per-patch), which may help to mention what changed in
the cover letter itself, if any, since this is a good and long cover
letter :)
By the way, some of this cover letter (the "Results" section in
particular with the examples) could be nice as part of the commit
message on the first commit -- that will make it forever part of the
Git repository, which can be useful to understand the commit itself
and why you designed the heuristics like you did.
Thanks!
Cheers,
Miguel
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 1/2] checkpatch.pl: warn about // comments on private Rust items
2025-04-22 13:46 ` Miguel Ojeda
@ 2025-04-22 14:37 ` Arnaud Lecomte
2025-04-22 15:32 ` Miguel Ojeda
0 siblings, 1 reply; 11+ messages in thread
From: Arnaud Lecomte @ 2025-04-22 14:37 UTC (permalink / raw)
To: Miguel Ojeda
Cc: Andy Whitcroft, Joe Perches, Dwaipayan Ray, Lukas Bulwahn,
Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, Danilo Krummrich, Nathan Chancellor,
Nick Desaulniers, Bill Wendling, Justin Stitt, linux-kernel,
rust-for-linux, llvm, skhan
On 22/04/2025 15:46, Miguel Ojeda wrote:
> On Tue, Apr 22, 2025 at 2:58 PM Arnaud Lecomte <contact@arnaud-lcm.com> wrote:
>> The detection uses multiple heuristics to identify likely doc comments:
>> - Comments containing markdown
> Markdown is required in both documentation and comments, so I don't
> think we can use some of those, e.g. inline code spans (i.e.
> backticks) are common (and actually expected) in comments. Something
> like a title (i.e. `#`) or intra-doc links are uncommon, though.
Let's then maybe reduce the score of this heuristic and remove intra-doc
links and titles detection. After reviewing, it indeed doesn't really
make sense.
>> - Comments starting with an imperative tone
> Some people document using the third-person, e.g. some functions say
> "Returns ..." like you have below. (It is not easy to enforce
> kernel-wide a single style here, thus so far we don't.)
>
> (Looking briefly at the code) Ah, I think you are covering both, good.
As mentioned earlier, we can reduce the score of any heuristic which
could lead to any important false positive.
In my opinion, as long as the heuristic is relevant, we always have the
possibility to diminish the score associated with the heuristic, hence
preventing unnecessary false positives.
>> - Comments with references: @see, @link, ...
> Do you mean Markdown references? Or javadoc-like ones?
>
> (Looking again at the code...) I think you are referring to actually
> strings like `@see`. Hmm... I don't think we have those -- the only
> `@` I would expect in a comment are thinks like emails or the
> `rustdoc` syntax to disambiguate the "kind" of item, e.g.
> `type@NotThreadSafe`. I do see a `@maxlen` somewhere, but that should
> have been an inline code span, and anyway it is not a `@see` or
> `@link`. But I may be confused here?
>
I think that you are definitely more experienced with what's done
commonly in rust code. Let's maybe change this heuristic definition with
@ related to types or some other annotation. Do you have some example I
could have a look to come in the next version with a relevant list of @
we can encounter.
>> Comments are only flagged if they:
>> - Appear above private items
>> - Don't contain obvious non-doc patterns (TODO, FIXME)
>> - Score sufficiently on heuristics
> Nice work! I wasn't expecting something with actual weighted scoring,
> but if the maintainers are OK with something as involved as that, then
> I guess it is fine. We may need to tweak the scoring in the future,
> but it may be a good experiment.
I think it is a nice approach due to the granularity it offers. This
will drastically reduce the number of false positives.
> Thanks!
>
> Cheers,
> Miguel
Thanks for your feedbacks,
Arnaud
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 0/2] checkpatch.pl: Add warning for // comments on private Rust items
2025-04-22 14:10 ` [PATCH v3 0/2] checkpatch.pl: Add warning for // comments on private Rust items Miguel Ojeda
@ 2025-04-22 14:38 ` Arnaud Lecomte
0 siblings, 0 replies; 11+ messages in thread
From: Arnaud Lecomte @ 2025-04-22 14:38 UTC (permalink / raw)
To: Miguel Ojeda
Cc: Andy Whitcroft, Joe Perches, Dwaipayan Ray, Lukas Bulwahn,
Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, Danilo Krummrich, Nathan Chancellor,
Nick Desaulniers, Bill Wendling, Justin Stitt, linux-kernel,
rust-for-linux, llvm, skhan
On 22/04/2025 16:10, Miguel Ojeda wrote:
> On Tue, Apr 22, 2025 at 2:56 PM Arnaud Lecomte <contact@arnaud-lcm.com> wrote:
>> 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.
> I think this background wasn't changed from the previous version --
> you can include a changelog in the cover letter too (some do that
> only, instead of per-patch), which may help to mention what changed in
> the cover letter itself, if any, since this is a good and long cover
> letter :)
>
> By the way, some of this cover letter (the "Results" section in
> particular with the examples) could be nice as part of the commit
> message on the first commit -- that will make it forever part of the
> Git repository, which can be useful to understand the commit itself
> and why you designed the heuristics like you did.
>
> Thanks!
>
> Cheers,
> Miguel
Definitely make sense to be part of the commit message for the first
patch, thanks !
Arnaud
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 1/2] checkpatch.pl: warn about // comments on private Rust items
2025-04-22 14:37 ` Arnaud Lecomte
@ 2025-04-22 15:32 ` Miguel Ojeda
2025-04-23 11:33 ` Arnaud Lecomte
0 siblings, 1 reply; 11+ messages in thread
From: Miguel Ojeda @ 2025-04-22 15:32 UTC (permalink / raw)
To: Arnaud Lecomte
Cc: Andy Whitcroft, Joe Perches, Dwaipayan Ray, Lukas Bulwahn,
Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, Danilo Krummrich, Nathan Chancellor,
Nick Desaulniers, Bill Wendling, Justin Stitt, linux-kernel,
rust-for-linux, llvm, skhan
On Tue, Apr 22, 2025 at 4:37 PM Arnaud Lecomte <contact@arnaud-lcm.com> wrote:
>
> As mentioned earlier, we can reduce the score of any heuristic which
> could lead to any important false positive.
> In my opinion, as long as the heuristic is relevant, we always have the
> possibility to diminish the score associated with the heuristic, hence
> preventing unnecessary false positives.
Definitely (my comment above for this one was just a note, i.e. there
may be nothing to change -- I just thought the commit message referred
to just checking "Return" and not "Returns", since it said
"imperative").
> I think that you are definitely more experienced with what's done
> commonly in rust code. Let's maybe change this heuristic definition with
> @ related to types or some other annotation. Do you have some example I
> could have a look to come in the next version with a relevant list of @
> we can encounter.
What does "references" mean in that heuristic? If you mean external
links to some URL, then we typically use Markdown for those. The
inline ones like `<https://...>`happen in both comments and docs. The
`[...]: https://...` ones are way less common in comments I think (I
can't find one).
As for `@`, if you mean the actual character, I grepped for it in Rust
files with the /.*@ regex and found just ~13 matches, and all were the
emails and disambiguators I mentioned. So I don't think we really use
it for "references" (assuming I understand what that means).
I guess you already looked in some `rust/kernel/` files -- some of
those are really the best examples of how docs and comments should
generally be written.
Thanks!
Cheers,
Miguel
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 1/2] checkpatch.pl: warn about // comments on private Rust items
2025-04-22 15:32 ` Miguel Ojeda
@ 2025-04-23 11:33 ` Arnaud Lecomte
2025-05-23 7:15 ` Arnaud Lecomte
0 siblings, 1 reply; 11+ messages in thread
From: Arnaud Lecomte @ 2025-04-23 11:33 UTC (permalink / raw)
To: Miguel Ojeda
Cc: Andy Whitcroft, Joe Perches, Dwaipayan Ray, Lukas Bulwahn,
Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, Danilo Krummrich, Nathan Chancellor,
Nick Desaulniers, Bill Wendling, Justin Stitt, linux-kernel,
rust-for-linux, llvm, skhan
On 22/04/2025 17:32, Miguel Ojeda wrote:
> On Tue, Apr 22, 2025 at 4:37 PM Arnaud Lecomte <contact@arnaud-lcm.com> wrote:
>> As mentioned earlier, we can reduce the score of any heuristic which
>> could lead to any important false positive.
>> In my opinion, as long as the heuristic is relevant, we always have the
>> possibility to diminish the score associated with the heuristic, hence
>> preventing unnecessary false positives.
> Definitely (my comment above for this one was just a note, i.e. there
> may be nothing to change -- I just thought the commit message referred
> to just checking "Return" and not "Returns", since it said
> "imperative").
>
>> I think that you are definitely more experienced with what's done
>> commonly in rust code. Let's maybe change this heuristic definition with
>> @ related to types or some other annotation. Do you have some example I
>> could have a look to come in the next version with a relevant list of @
>> we can encounter.
> What does "references" mean in that heuristic? If you mean external
> links to some URL, then we typically use Markdown for those. The
> inline ones like `<https://...>`happen in both comments and docs. The
> `[...]: https://...` ones are way less common in comments I think (I
> can't find one).
We should then remove it. I'll wait for other reviewers for their
feedback and then send a new version of the patch serie. Thanks :)
> As for `@`, if you mean the actual character, I grepped for it in Rust
> files with the /.*@ regex and found just ~13 matches, and all were the
> emails and disambiguators I mentioned. So I don't think we really use
> it for "references" (assuming I understand what that means).
>
> I guess you already looked in some `rust/kernel/` files -- some of
> those are really the best examples of how docs and comments should
> generally be written.
> Thanks!
>
> Cheers,
> Miguel
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 1/2] checkpatch.pl: warn about // comments on private Rust items
2025-04-23 11:33 ` Arnaud Lecomte
@ 2025-05-23 7:15 ` Arnaud Lecomte
0 siblings, 0 replies; 11+ messages in thread
From: Arnaud Lecomte @ 2025-05-23 7:15 UTC (permalink / raw)
To: Miguel Ojeda
Cc: Andy Whitcroft, Joe Perches, Dwaipayan Ray, Lukas Bulwahn,
Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, Danilo Krummrich, Nathan Chancellor,
Nick Desaulniers, Bill Wendling, Justin Stitt, linux-kernel,
rust-for-linux, llvm, skhan
Hi everyone !
Is there any update so far ?
Best regards,
Arnaud
On 23/04/2025 12:33, Arnaud Lecomte wrote:
> On 22/04/2025 17:32, Miguel Ojeda wrote:
>
>> On Tue, Apr 22, 2025 at 4:37 PM Arnaud Lecomte
>> <contact@arnaud-lcm.com> wrote:
>>> As mentioned earlier, we can reduce the score of any heuristic which
>>> could lead to any important false positive.
>>> In my opinion, as long as the heuristic is relevant, we always have the
>>> possibility to diminish the score associated with the heuristic, hence
>>> preventing unnecessary false positives.
>> Definitely (my comment above for this one was just a note, i.e. there
>> may be nothing to change -- I just thought the commit message referred
>> to just checking "Return" and not "Returns", since it said
>> "imperative").
>>
>>> I think that you are definitely more experienced with what's done
>>> commonly in rust code. Let's maybe change this heuristic definition
>>> with
>>> @ related to types or some other annotation. Do you have some example I
>>> could have a look to come in the next version with a relevant list of @
>>> we can encounter.
>> What does "references" mean in that heuristic? If you mean external
>> links to some URL, then we typically use Markdown for those. The
>> inline ones like `<https://...>`happen in both comments and docs. The
>> `[...]: https://...` ones are way less common in comments I think (I
>> can't find one).
> We should then remove it. I'll wait for other reviewers for their
> feedback and then send a new version of the patch serie. Thanks :)
>> As for `@`, if you mean the actual character, I grepped for it in Rust
>> files with the /.*@ regex and found just ~13 matches, and all were the
>> emails and disambiguators I mentioned. So I don't think we really use
>> it for "references" (assuming I understand what that means).
>>
>> I guess you already looked in some `rust/kernel/` files -- some of
>> those are really the best examples of how docs and comments should
>> generally be written.
>> Thanks!
>>
>> Cheers,
>> Miguel
>>
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2025-05-23 7:25 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-04-22 12:56 [PATCH v3 0/2] checkpatch.pl: Add warning for // comments on private Rust items Arnaud Lecomte
2025-04-22 12:58 ` [PATCH v3 1/2] checkpatch.pl: warn about " Arnaud Lecomte
2025-04-22 13:46 ` Miguel Ojeda
2025-04-22 14:37 ` Arnaud Lecomte
2025-04-22 15:32 ` Miguel Ojeda
2025-04-23 11:33 ` Arnaud Lecomte
2025-05-23 7:15 ` Arnaud Lecomte
2025-04-22 12:58 ` [PATCH v3 3/3] checkpatch.pl: --fix support for `//` comments rust private items Arnaud Lecomte
2025-04-22 13:02 ` [PATCH v3 2/2 resend] " Arnaud Lecomte
2025-04-22 14:10 ` [PATCH v3 0/2] checkpatch.pl: Add warning for // comments on private Rust items Miguel Ojeda
2025-04-22 14:38 ` Arnaud Lecomte
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox