Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ In the codex-rs folder where the rust code lives:
- Avoid bool or ambiguous `Option` parameters that force callers to write hard-to-read code such as `foo(false)` or `bar(None)`. Prefer enums, named methods, newtypes, or other idiomatic Rust API shapes when they keep the callsite self-documenting.
- When you cannot make that API change and still need a small positional-literal callsite in Rust, follow the `argument_comment_lint` convention:
- Use an exact `/*param_name*/` comment before opaque literal arguments such as `None`, booleans, and numeric literals when passing them by position.
- A method's sole non-self argument is exempt when the method and parameter names match, such as `.enabled(false)` for `fn enabled(&self, enabled: bool)`.
- Do not add these comments for string or char literals unless the comment adds real clarity; those literals are intentionally exempt from the lint.
- The parameter name in the comment must exactly match the callee signature.
- You can run `just argument-comment-lint` to run the lint check locally. This is powered by Bazel, so running it the first time can be slow if Bazel is not warmed up, though incremental invocations should take <15s. Most of the time, it is best to update the PR and let CI take responsibility for checking this (or run it asynchronously in the background after submitting the PR). Note CI checks all three platforms, which the local run does not.
Expand Down
5 changes: 5 additions & 0 deletions tools/argument-comment-lint/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,11 @@ It provides two lints:
String and char literals are exempt because they are often already
self-descriptive at the callsite.

The sole non-self method argument is also exempt when the method name matches
the resolved parameter name. For example, `.enabled(false)` is already
self-descriptive when it resolves to `fn enabled(self, enabled: bool)`. An
explicit argument comment is still checked for a mismatch.

## Behavior

Given:
Expand Down
34 changes: 30 additions & 4 deletions tools/argument-comment-lint/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,8 @@ rustc_session::declare_lint! {
///
/// Requires a `/*param*/` comment before anonymous literal-like
/// arguments such as `None`, booleans, and numeric literals.
/// A method's sole non-self argument is exempt when its name matches the
/// method name.
///
/// ### Why is this bad?
///
Expand Down Expand Up @@ -125,6 +127,11 @@ rustc_session::declare_lint! {
#[derive(Default)]
pub struct ArgumentCommentLint;

enum CallKind {
Function,
Method { name: String },
}

rustc_session::impl_lint_pass!(
ArgumentCommentLint => [ARGUMENT_COMMENT_MISMATCH, UNCOMMENTED_ANONYMOUS_LITERAL_ARGUMENT]
);
Expand All @@ -137,10 +144,18 @@ impl<'tcx> LateLintPass<'tcx> for ArgumentCommentLint {

match expr.kind {
ExprKind::Call(callee, args) => {
self.check_call(cx, expr, callee.span, args, 0);
self.check_call(cx, expr, callee.span, args, CallKind::Function);
}
ExprKind::MethodCall(_, receiver, args, _) => {
self.check_call(cx, expr, receiver.span, args, 1);
ExprKind::MethodCall(method, receiver, args, _) => {
self.check_call(
cx,
expr,
receiver.span,
args,
CallKind::Method {
name: method.ident.name.to_string(),
},
);
}
_ => {}
}
Expand All @@ -154,7 +169,7 @@ impl ArgumentCommentLint {
call: &'tcx Expr<'tcx>,
first_gap_anchor: Span,
args: &'tcx [Expr<'tcx>],
parameter_offset: usize,
call_kind: CallKind,
) {
let Some(def_id) = fn_def_id(cx, call) else {
return;
Expand All @@ -167,6 +182,11 @@ impl ArgumentCommentLint {
return;
}

// Method parameter lists include `self`, which is not present in `args`.
let (parameter_offset, method_name) = match &call_kind {
CallKind::Function => (0, None),
CallKind::Method { name } => (1, Some(name.as_str())),
};
let parameter_names: Vec<_> = cx.tcx.fn_arg_idents(def_id).iter().copied().collect();
for (index, arg) in args.iter().enumerate() {
if arg.span.from_expansion() {
Expand Down Expand Up @@ -215,6 +235,12 @@ impl ArgumentCommentLint {
continue;
}

// Don't require a clarifying comment for self-documenting arguments whose names
// match the method.
if args.len() == 1 && method_name == Some(expected_name.as_str()) {
continue;
}

if !is_anonymous_literal_like(cx, arg) {
continue;
}
Expand Down
25 changes: 25 additions & 0 deletions tools/argument-comment-lint/ui/allow_self_documenting_methods.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
#![warn(argument_comment_mismatch)]
#![warn(uncommented_anonymous_literal_argument)]

struct Builder;

impl Builder {
fn enabled(self, enabled: bool) -> Self {
let _ = enabled;
self
}

fn retry_count(self, retry_count: usize) -> Self {
let _ = retry_count;
self
}

fn base_url(self, base_url: Option<String>) -> Self {
let _ = base_url;
self
}
}

fn main() {
let _ = Builder.enabled(false).retry_count(3).base_url(None);
}
10 changes: 10 additions & 0 deletions tools/argument-comment-lint/ui/comment_mismatch.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,16 @@ fn create_openai_url(base_url: Option<String>) -> String {
String::new()
}

struct Options;

impl Options {
fn enabled(self, enabled: bool) -> Self {
let _ = enabled;
self
}
}

fn main() {
let _ = create_openai_url(/*api_base*/ None);
let _ = Options.enabled(/*value*/ false);
}
12 changes: 10 additions & 2 deletions tools/argument-comment-lint/ui/comment_mismatch.stderr
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
warning: argument comment `/*api_base*/` does not match parameter `base_url`
--> $DIR/comment_mismatch.rs:9:44
--> $DIR/comment_mismatch.rs:18:44
|
LL | let _ = create_openai_url(/*api_base*/ None);
| ^^^^
Expand All @@ -11,5 +11,13 @@ note: the lint level is defined here
LL | #![warn(argument_comment_mismatch)]
| ^^^^^^^^^^^^^^^^^^^^^^^^^

warning: 1 warning emitted
warning: argument comment `/*value*/` does not match parameter `enabled`
--> $DIR/comment_mismatch.rs:19:39
|
LL | let _ = Options.enabled(/*value*/ false);
| ^^^^^
|
= help: use `/*enabled*/`

warning: 2 warnings emitted

14 changes: 14 additions & 0 deletions tools/argument-comment-lint/ui/multiple_method_arguments.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
#![warn(uncommented_anonymous_literal_argument)]

struct Options;

impl Options {
fn enabled(self, enabled: bool, retry_count: usize) -> Self {
let _ = (enabled, retry_count);
self
}
}

fn main() {
let _ = Options.enabled(false, /*retry_count*/ 3);
}
14 changes: 14 additions & 0 deletions tools/argument-comment-lint/ui/multiple_method_arguments.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
warning: anonymous literal-like argument for parameter `enabled`
--> $DIR/multiple_method_arguments.rs:13:29
|
LL | let _ = Options.enabled(false, /*retry_count*/ 3);
| ^^^^^ help: prepend the parameter name comment: `/*enabled*/ false`
|
note: the lint level is defined here
--> $DIR/multiple_method_arguments.rs:1:9
|
LL | #![warn(uncommented_anonymous_literal_argument)]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

warning: 1 warning emitted

Loading