From 52db447c77d19606ee30e3d20cd2591f32f9e4f2 Mon Sep 17 00:00:00 2001 From: "Adam Perry @ OpenAI" Date: Thu, 11 Jun 2026 10:24:42 -0700 Subject: [PATCH] lint: allow self-documenting builder arguments (#27507) Builder-style setters often repeat the setting name in both the method and its sole argument. Calls such as `.enabled(false)` are already self-documenting, so requiring `/*enabled*/` adds noise without clarifying the call. ## What changed - Exempt a method's sole non-self argument when its resolved parameter name matches the method name. - Continue validating any explicit argument comment against the resolved parameter name. - Continue requiring comments when method and parameter names differ or when a method has multiple non-self arguments. - Document the exception in `AGENTS.md` and the lint's own behavior documentation. ## Examples Before this change we'd need redundant comments like this: ```rust builder.enabled(/*false*/ false); builder.retry_count(/*retry_count*/ 3); builder.base_url(/*base_url*/ None); ``` Now can be written like this: ```rust builder.enabled(false); builder.retry_count(3); builder.base_url(None); ``` Still disallowed: ```rust client.set_flag(true); // Method name does not match parameter `enabled`. options.enabled(false, /*retry_count*/ 3); // More than one non-self argument. options.enabled(/*value*/ false); // Explicit comment does not match `enabled`. ``` ## Validation Added UI coverage for boolean, numeric, and `None` builder arguments, multi-argument methods, and explicit comment mismatches. Ran `rustup run nightly-2025-09-18 cargo test` in `tools/argument-comment-lint`. --- AGENTS.md | 1 + tools/argument-comment-lint/README.md | 5 +++ tools/argument-comment-lint/src/lib.rs | 34 ++++++++++++++++--- .../ui/allow_self_documenting_methods.rs | 25 ++++++++++++++ .../ui/comment_mismatch.rs | 10 ++++++ .../ui/comment_mismatch.stderr | 12 +++++-- .../ui/multiple_method_arguments.rs | 14 ++++++++ .../ui/multiple_method_arguments.stderr | 14 ++++++++ 8 files changed, 109 insertions(+), 6 deletions(-) create mode 100644 tools/argument-comment-lint/ui/allow_self_documenting_methods.rs create mode 100644 tools/argument-comment-lint/ui/multiple_method_arguments.rs create mode 100644 tools/argument-comment-lint/ui/multiple_method_arguments.stderr diff --git a/AGENTS.md b/AGENTS.md index 46d2626e3..0118ef77b 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -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. diff --git a/tools/argument-comment-lint/README.md b/tools/argument-comment-lint/README.md index 7270c4886..889bae1df 100644 --- a/tools/argument-comment-lint/README.md +++ b/tools/argument-comment-lint/README.md @@ -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: diff --git a/tools/argument-comment-lint/src/lib.rs b/tools/argument-comment-lint/src/lib.rs index 5586509f2..76b9e1773 100644 --- a/tools/argument-comment-lint/src/lib.rs +++ b/tools/argument-comment-lint/src/lib.rs @@ -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? /// @@ -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] ); @@ -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(), + }, + ); } _ => {} } @@ -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; @@ -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() { @@ -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; } diff --git a/tools/argument-comment-lint/ui/allow_self_documenting_methods.rs b/tools/argument-comment-lint/ui/allow_self_documenting_methods.rs new file mode 100644 index 000000000..d57f7c521 --- /dev/null +++ b/tools/argument-comment-lint/ui/allow_self_documenting_methods.rs @@ -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) -> Self { + let _ = base_url; + self + } +} + +fn main() { + let _ = Builder.enabled(false).retry_count(3).base_url(None); +} diff --git a/tools/argument-comment-lint/ui/comment_mismatch.rs b/tools/argument-comment-lint/ui/comment_mismatch.rs index 3eebdd9dc..f3ad4db75 100644 --- a/tools/argument-comment-lint/ui/comment_mismatch.rs +++ b/tools/argument-comment-lint/ui/comment_mismatch.rs @@ -5,6 +5,16 @@ fn create_openai_url(base_url: Option) -> 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); } diff --git a/tools/argument-comment-lint/ui/comment_mismatch.stderr b/tools/argument-comment-lint/ui/comment_mismatch.stderr index 6ede65602..058871395 100644 --- a/tools/argument-comment-lint/ui/comment_mismatch.stderr +++ b/tools/argument-comment-lint/ui/comment_mismatch.stderr @@ -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); | ^^^^ @@ -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 diff --git a/tools/argument-comment-lint/ui/multiple_method_arguments.rs b/tools/argument-comment-lint/ui/multiple_method_arguments.rs new file mode 100644 index 000000000..e15f15624 --- /dev/null +++ b/tools/argument-comment-lint/ui/multiple_method_arguments.rs @@ -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); +} diff --git a/tools/argument-comment-lint/ui/multiple_method_arguments.stderr b/tools/argument-comment-lint/ui/multiple_method_arguments.stderr new file mode 100644 index 000000000..f9b452e4f --- /dev/null +++ b/tools/argument-comment-lint/ui/multiple_method_arguments.stderr @@ -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 +