mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
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`.
This commit is contained in:
committed by
GitHub
Unverified
parent
9d88e299a7
commit
52db447c77
@@ -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.
|
||||
|
||||
@@ -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:
|
||||
|
||||
@@ -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;
|
||||
}
|
||||
|
||||
@@ -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);
|
||||
}
|
||||
@@ -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);
|
||||
}
|
||||
|
||||
@@ -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
|
||||
|
||||
|
||||
@@ -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);
|
||||
}
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user