Add argument-comment Dylint runner (#14651)

This commit is contained in:
Michael Bolin 2026-03-14 08:18:04 -07:00 committed by GitHub
parent 70eddad6b0
commit 4b31848f5b
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
20 changed files with 2348 additions and 0 deletions

View file

@ -11,6 +11,11 @@ In the codex-rs folder where the rust code lives:
- Always collapse if statements per https://rust-lang.github.io/rust-clippy/master/index.html#collapsible_if
- Always inline format! args when possible per https://rust-lang.github.io/rust-clippy/master/index.html#uninlined_format_args
- Use method references over closures when possible per https://rust-lang.github.io/rust-clippy/master/index.html#redundant_closure_for_method_calls
- 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.
- Do not add these comments for string or char literals unless the comment adds real clarity; those literals are intentionally exempt from the lint.
- If you add one of these comments, the parameter name must exactly match the callee signature.
- When possible, make `match` statements exhaustive and avoid wildcard arms.
- When writing tests, prefer comparing the equality of entire objects over fields one by one.
- When making a change that adds or changes an API, ensure that the documentation in the `docs/` folder is up to date if applicable.

View file

@ -86,6 +86,11 @@ write-app-server-schema *args:
write-hooks-schema:
cargo run --manifest-path ./codex-rs/Cargo.toml -p codex-hooks --bin write_hooks_schema_fixtures
# Run the argument-comment Dylint checks across codex-rs.
[no-cd]
argument-comment-lint *args:
./tools/argument-comment-lint/run.sh "$@"
# Tail logs from the state SQLite database
log *args:
if [ "${1:-}" = "--" ]; then shift; fi; cargo run -p codex-state --bin logs_client -- "$@"

View file

@ -0,0 +1,6 @@
[target.'cfg(all())']
rustflags = ["-C", "linker=dylint-link"]
# For Rust versions 1.74.0 and onward, the following alternative can be used
# (see https://github.com/rust-lang/cargo/pull/12535):
# linker = "dylint-link"

View file

@ -0,0 +1 @@
/target

1653
tools/argument-comment-lint/Cargo.lock generated Normal file

File diff suppressed because it is too large Load diff

View file

@ -0,0 +1,21 @@
[package]
name = "argument_comment_lint"
version = "0.1.0"
description = "Dylint lints for Rust /*param*/ argument comments"
edition = "2024"
publish = false
[lib]
crate-type = ["cdylib"]
[dependencies]
clippy_utils = { git = "https://github.com/rust-lang/rust-clippy", rev = "20ce69b9a63bcd2756cd906fe0964d1e901e042a" }
dylint_linting = "5.0.0"
[dev-dependencies]
dylint_testing = "5.0.0"
[workspace]
[package.metadata.rust-analyzer]
rustc_private = true

View file

@ -0,0 +1,104 @@
# argument-comment-lint
Isolated [Dylint](https://github.com/trailofbits/dylint) library for enforcing
Rust argument comments in the exact `/*param*/` shape.
Prefer self-documenting APIs over comment-heavy call sites when possible. If a
call site would otherwise read like `foo(false)` or `bar(None)`, consider an
enum, named helper, newtype, or another idiomatic Rust API shape first, and
use an argument comment only when a smaller compatibility-preserving change is
more appropriate.
It provides two lints:
- `argument_comment_mismatch` (`warn` by default): validates that a present
`/*param*/` comment matches the resolved callee parameter name.
- `uncommented_anonymous_literal_argument` (`allow` by default): flags
anonymous literal-like arguments such as `None`, `true`, `false`, and numeric
literals when they do not have a preceding `/*param*/` comment.
String and char literals are exempt because they are often already
self-descriptive at the callsite.
## Behavior
Given:
```rust
fn create_openai_url(base_url: Option<String>, retry_count: usize) -> String {
let _ = (base_url, retry_count);
String::new()
}
```
This is accepted:
```rust
create_openai_url(/*base_url*/ None, /*retry_count*/ 3);
```
This is warned on by `argument_comment_mismatch`:
```rust
create_openai_url(/*api_base*/ None, 3);
```
This is only warned on when `uncommented_anonymous_literal_argument` is enabled:
```rust
create_openai_url(None, 3);
```
## Development
Install the required tooling once:
```bash
cargo install cargo-dylint dylint-link
rustup toolchain install nightly-2025-09-18 \
--component llvm-tools-preview \
--component rustc-dev \
--component rust-src
```
Run the lint crate tests:
```bash
cd tools/argument-comment-lint
cargo test
```
Run the lint against `codex-rs` from the repo root:
```bash
./tools/argument-comment-lint/run.sh -p codex-core
just argument-comment-lint -p codex-core
```
If no package selection is provided, `run.sh` defaults to checking the
`codex-rs` workspace with `--workspace --no-deps`.
Repo runs also promote `uncommented_anonymous_literal_argument` to an error by
default:
```bash
./tools/argument-comment-lint/run.sh -p codex-core
```
The wrapper does that by setting `DYLINT_RUSTFLAGS`, and it leaves an explicit
existing setting alone. It also defaults `CARGO_INCREMENTAL=0` unless you have
already set it, because the current nightly Dylint flow can otherwise hit a
rustc incremental compilation ICE locally. To override that behavior for an ad
hoc run:
```bash
DYLINT_RUSTFLAGS="-A uncommented_anonymous_literal_argument" \
CARGO_INCREMENTAL=1 \
./tools/argument-comment-lint/run.sh -p codex-core
```
To expand target coverage for an ad hoc run:
```bash
./tools/argument-comment-lint/run.sh -p codex-core -- --all-targets
```

View file

@ -0,0 +1,91 @@
#!/usr/bin/env bash
set -euo pipefail
repo_root="$(cd "$(dirname "${BASH_SOURCE[0]}")/../.." && pwd)"
lint_path="$repo_root/tools/argument-comment-lint"
manifest_path="$repo_root/codex-rs/Cargo.toml"
strict_lint="uncommented_anonymous_literal_argument"
noise_lint="unknown_lints"
has_manifest_path=false
has_package_selection=false
has_no_deps=false
has_library_selection=false
expect_value=""
for arg in "$@"; do
if [[ -n "$expect_value" ]]; then
case "$expect_value" in
manifest_path)
has_manifest_path=true
;;
package_selection)
has_package_selection=true
;;
library_selection)
has_library_selection=true
;;
esac
expect_value=""
continue
fi
case "$arg" in
--)
break
;;
--manifest-path)
expect_value="manifest_path"
;;
--manifest-path=*)
has_manifest_path=true
;;
-p|--package)
expect_value="package_selection"
;;
--package=*)
has_package_selection=true
;;
--workspace)
has_package_selection=true
;;
--no-deps)
has_no_deps=true
;;
--lib|--lib-path)
expect_value="library_selection"
;;
--lib=*|--lib-path=*)
has_library_selection=true
;;
esac
done
cmd=(cargo dylint --path "$lint_path")
if [[ "$has_library_selection" == false ]]; then
cmd+=(--all)
fi
if [[ "$has_manifest_path" == false ]]; then
cmd+=(--manifest-path "$manifest_path")
fi
if [[ "$has_package_selection" == false ]]; then
cmd+=(--workspace)
fi
if [[ "$has_no_deps" == false ]]; then
cmd+=(--no-deps)
fi
cmd+=("$@")
if [[ "${DYLINT_RUSTFLAGS:-}" != *"$strict_lint"* ]]; then
export DYLINT_RUSTFLAGS="${DYLINT_RUSTFLAGS:+${DYLINT_RUSTFLAGS} }-D $strict_lint"
fi
if [[ "${DYLINT_RUSTFLAGS:-}" != *"$noise_lint"* ]]; then
export DYLINT_RUSTFLAGS="${DYLINT_RUSTFLAGS:+${DYLINT_RUSTFLAGS} }-A $noise_lint"
fi
if [[ -z "${CARGO_INCREMENTAL:-}" ]]; then
export CARGO_INCREMENTAL=0
fi
exec "${cmd[@]}"

View file

@ -0,0 +1,3 @@
[toolchain]
channel = "nightly-2025-09-18"
components = ["llvm-tools-preview", "rustc-dev", "rust-src"]

View file

@ -0,0 +1,63 @@
pub fn parse_argument_comment(text: &str) -> Option<&str> {
let trimmed = text.trim_end();
let comment_start = trimmed.rfind("/*")?;
let comment = &trimmed[comment_start..];
let name = comment.strip_prefix("/*")?.strip_suffix("*/")?;
is_identifier(name).then_some(name)
}
pub fn parse_argument_comment_prefix(text: &str) -> Option<&str> {
let trimmed = text.trim_start();
let comment = trimmed.strip_prefix("/*")?;
let (name, _) = comment.split_once("*/")?;
is_identifier(name).then_some(name)
}
fn is_identifier(text: &str) -> bool {
let mut chars = text.chars();
let Some(first) = chars.next() else {
return false;
};
if !(first == '_' || first.is_ascii_alphabetic()) {
return false;
}
chars.all(|ch| ch == '_' || ch.is_ascii_alphanumeric())
}
#[cfg(test)]
mod tests {
use super::parse_argument_comment;
use super::parse_argument_comment_prefix;
#[test]
fn parses_trailing_comment() {
assert_eq!(parse_argument_comment("(/*base_url*/ "), Some("base_url"));
assert_eq!(
parse_argument_comment(", /*timeout_ms*/ "),
Some("timeout_ms")
);
assert_eq!(
parse_argument_comment(".method::<u8>(/*base_url*/ "),
Some("base_url")
);
}
#[test]
fn rejects_non_matching_shapes() {
assert_eq!(parse_argument_comment("(\n"), None);
assert_eq!(parse_argument_comment("(/* base_url*/ "), None);
assert_eq!(parse_argument_comment("(/*base_url */ "), None);
assert_eq!(parse_argument_comment("(/*base_url=*/ "), None);
assert_eq!(parse_argument_comment("(/*1base_url*/ "), None);
assert_eq!(parse_argument_comment_prefix("/*env=*/ None"), None);
}
#[test]
fn parses_prefix_comment() {
assert_eq!(parse_argument_comment_prefix("/*env*/ None"), Some("env"));
assert_eq!(
parse_argument_comment_prefix("\n /*retry_count*/ 3"),
Some("retry_count")
);
}
}

View file

@ -0,0 +1,273 @@
#![feature(rustc_private)]
mod comment_parser;
extern crate rustc_ast;
extern crate rustc_errors;
extern crate rustc_hir;
extern crate rustc_lint;
extern crate rustc_middle;
extern crate rustc_session;
extern crate rustc_span;
use clippy_utils::diagnostics::span_lint_and_help;
use clippy_utils::diagnostics::span_lint_and_sugg;
use clippy_utils::fn_def_id;
use clippy_utils::is_res_lang_ctor;
use clippy_utils::peel_blocks;
use clippy_utils::source::snippet;
use rustc_ast::LitKind;
use rustc_errors::Applicability;
use rustc_hir::Expr;
use rustc_hir::ExprKind;
use rustc_hir::LangItem;
use rustc_hir::UnOp;
use rustc_hir::def::DefKind;
use rustc_lint::LateContext;
use rustc_lint::LateLintPass;
use rustc_span::BytePos;
use rustc_span::Span;
use crate::comment_parser::parse_argument_comment;
use crate::comment_parser::parse_argument_comment_prefix;
dylint_linting::dylint_library!();
#[unsafe(no_mangle)]
pub fn register_lints(_sess: &rustc_session::Session, lint_store: &mut rustc_lint::LintStore) {
lint_store.register_lints(&[
ARGUMENT_COMMENT_MISMATCH,
UNCOMMENTED_ANONYMOUS_LITERAL_ARGUMENT,
]);
lint_store.register_late_pass(|_| Box::new(ArgumentCommentLint));
}
rustc_session::declare_lint! {
/// ### What it does
///
/// Checks `/*param*/` argument comments and verifies that the comment
/// matches the resolved callee parameter name.
///
/// ### Why is this bad?
///
/// A mismatched comment is worse than no comment because it actively
/// misleads the reader.
///
/// ### Known problems
///
/// This lint only runs when the callee resolves to a concrete function or
/// method with available parameter names.
///
/// ### Example
///
/// ```rust
/// fn create_openai_url(base_url: Option<String>) -> String {
/// String::new()
/// }
///
/// create_openai_url(/*api_base*/ None);
/// ```
///
/// Use instead:
///
/// ```rust
/// fn create_openai_url(base_url: Option<String>) -> String {
/// String::new()
/// }
///
/// create_openai_url(/*base_url*/ None);
/// ```
pub ARGUMENT_COMMENT_MISMATCH,
Warn,
"argument comment does not match the resolved parameter name"
}
rustc_session::declare_lint! {
/// ### What it does
///
/// Requires a `/*param*/` comment before anonymous literal-like
/// arguments such as `None`, booleans, and numeric literals.
///
/// ### Why is this bad?
///
/// Bare literal-like arguments make call sites harder to read because the
/// meaning of the value is hidden in the callee signature.
///
/// ### Known problems
///
/// This lint is opinionated, so it is `allow` by default.
///
/// ### Example
///
/// ```rust
/// fn create_openai_url(base_url: Option<String>) -> String {
/// String::new()
/// }
///
/// create_openai_url(None);
/// ```
///
/// Use instead:
///
/// ```rust
/// fn create_openai_url(base_url: Option<String>) -> String {
/// String::new()
/// }
///
/// create_openai_url(/*base_url*/ None);
/// ```
pub UNCOMMENTED_ANONYMOUS_LITERAL_ARGUMENT,
Allow,
"anonymous literal-like argument is missing a `/*param*/` comment"
}
#[derive(Default)]
pub struct ArgumentCommentLint;
rustc_session::impl_lint_pass!(
ArgumentCommentLint => [ARGUMENT_COMMENT_MISMATCH, UNCOMMENTED_ANONYMOUS_LITERAL_ARGUMENT]
);
impl<'tcx> LateLintPass<'tcx> for ArgumentCommentLint {
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) {
if expr.span.from_expansion() {
return;
}
match expr.kind {
ExprKind::Call(callee, args) => {
self.check_call(cx, expr, callee.span, args, 0);
}
ExprKind::MethodCall(_, receiver, args, _) => {
self.check_call(cx, expr, receiver.span, args, 1);
}
_ => {}
}
}
}
impl ArgumentCommentLint {
fn check_call<'tcx>(
&self,
cx: &LateContext<'tcx>,
call: &'tcx Expr<'tcx>,
first_gap_anchor: Span,
args: &'tcx [Expr<'tcx>],
parameter_offset: usize,
) {
let Some(def_id) = fn_def_id(cx, call) else {
return;
};
if !def_id.is_local() && !is_workspace_crate_name(cx.tcx.crate_name(def_id.krate).as_str())
{
return;
}
if !matches!(cx.tcx.def_kind(def_id), DefKind::Fn | DefKind::AssocFn) {
return;
}
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() {
continue;
}
let Some(expected_name) = parameter_names.get(index + parameter_offset) else {
continue;
};
let Some(expected_name) = expected_name else {
continue;
};
let expected_name = expected_name.name.to_string();
if !is_meaningful_parameter_name(&expected_name) {
continue;
}
let boundary_span = if index == 0 {
first_gap_anchor
} else {
args[index - 1].span
};
let gap_span = boundary_span.between(arg.span);
let gap_text = snippet(cx, gap_span, "");
let arg_text = snippet(cx, arg.span, "..");
let lookbehind_start = BytePos(arg.span.lo().0.saturating_sub(64));
let lookbehind_text =
snippet(cx, arg.span.shrink_to_lo().with_lo(lookbehind_start), "");
let argument_comment = parse_argument_comment(gap_text.as_ref())
.or_else(|| parse_argument_comment(lookbehind_text.as_ref()))
.or_else(|| parse_argument_comment_prefix(arg_text.as_ref()));
if let Some(actual_name) = argument_comment {
if actual_name != expected_name {
span_lint_and_help(
cx,
ARGUMENT_COMMENT_MISMATCH,
arg.span,
format!(
"argument comment `/*{actual_name}*/` does not match parameter `{expected_name}`"
),
None,
format!("use `/*{expected_name}*/`"),
);
}
continue;
}
if !is_anonymous_literal_like(cx, arg) {
continue;
}
span_lint_and_sugg(
cx,
UNCOMMENTED_ANONYMOUS_LITERAL_ARGUMENT,
arg.span,
format!("anonymous literal-like argument for parameter `{expected_name}`"),
"prepend the parameter name comment",
format!("/*{expected_name}*/ {arg_text}"),
Applicability::MachineApplicable,
);
}
}
}
fn is_anonymous_literal_like(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
let expr = peel_blocks(expr);
match expr.kind {
ExprKind::Lit(lit) => !matches!(
lit.node,
LitKind::Str(..) | LitKind::ByteStr(..) | LitKind::CStr(..) | LitKind::Char(..)
),
ExprKind::Unary(UnOp::Neg, inner) => matches!(peel_blocks(inner).kind, ExprKind::Lit(_)),
ExprKind::Path(qpath) => {
is_res_lang_ctor(cx, cx.qpath_res(&qpath, expr.hir_id), LangItem::OptionNone)
}
_ => false,
}
}
fn is_meaningful_parameter_name(name: &str) -> bool {
!name.is_empty() && !name.starts_with('_')
}
fn is_workspace_crate_name(name: &str) -> bool {
name.starts_with("codex_")
|| matches!(
name,
"app_test_support" | "core_test_support" | "mcp_test_support"
)
}
#[test]
fn ui() {
dylint_testing::ui_test(env!("CARGO_PKG_NAME"), "ui");
}
#[test]
fn workspace_crate_filter_accepts_first_party_names_only() {
assert!(is_workspace_crate_name("codex_core"));
assert!(is_workspace_crate_name("codex_tui"));
assert!(is_workspace_crate_name("core_test_support"));
assert!(!is_workspace_crate_name("std"));
assert!(!is_workspace_crate_name("tokio"));
}

View file

@ -0,0 +1,9 @@
#![warn(uncommented_anonymous_literal_argument)]
fn split_top_level(body: &str, delimiter: char) {
let _ = (body, delimiter);
}
fn main() {
split_top_level("a|b|c", '|');
}

View file

@ -0,0 +1,9 @@
#![warn(uncommented_anonymous_literal_argument)]
fn describe(prefix: &str, suffix: &str) {
let _ = (prefix, suffix);
}
fn main() {
describe("openai", r"https://api.openai.com/v1");
}

View file

@ -0,0 +1,12 @@
#![warn(argument_comment_mismatch)]
fn create_openai_url(base_url: Option<String>, retry_count: usize) -> String {
let _ = (base_url, retry_count);
String::new()
}
fn main() {
let base_url = Some(String::from("https://api.openai.com"));
create_openai_url(base_url, 3);
create_openai_url(/*base_url*/ None, 3);
}

View file

@ -0,0 +1,15 @@
#![warn(argument_comment_mismatch)]
#![warn(uncommented_anonymous_literal_argument)]
fn run_git_for_stdout(repo_root: &str, args: Vec<&str>, env: Option<&str>) -> String {
let _ = (repo_root, args, env);
String::new()
}
fn main() {
let _ = run_git_for_stdout(
"/tmp/repo",
vec!["rev-parse", "HEAD"],
/*env*/ None,
);
}

View file

@ -0,0 +1,10 @@
#![warn(argument_comment_mismatch)]
fn create_openai_url(base_url: Option<String>) -> String {
let _ = base_url;
String::new()
}
fn main() {
let _ = create_openai_url(/*api_base*/ None);
}

View file

@ -0,0 +1,15 @@
warning: argument comment `/*api_base*/` does not match parameter `base_url`
--> $DIR/comment_mismatch.rs:9:44
|
LL | let _ = create_openai_url(/*api_base*/ None);
| ^^^^
|
= help: use `/*base_url*/`
note: the lint level is defined here
--> $DIR/comment_mismatch.rs:1:9
|
LL | #![warn(argument_comment_mismatch)]
| ^^^^^^^^^^^^^^^^^^^^^^^^^
warning: 1 warning emitted

View file

@ -0,0 +1,9 @@
#![warn(uncommented_anonymous_literal_argument)]
fn main() {
let line = "{\"type\":\"response_item\"}";
let _ = line.starts_with('{');
let _ = line.find("type");
let parts = ["type", "response_item"];
let _ = parts.join("\n");
}

View file

@ -0,0 +1,18 @@
#![warn(uncommented_anonymous_literal_argument)]
struct Client;
impl Client {
fn set_flag(&self, enabled: bool) {}
}
fn create_openai_url(base_url: Option<String>, retry_count: usize) -> String {
let _ = (base_url, retry_count);
String::new()
}
fn main() {
let client = Client;
let _ = create_openai_url(None, 3);
client.set_flag(true);
}

View file

@ -0,0 +1,26 @@
warning: anonymous literal-like argument for parameter `base_url`
--> $DIR/uncommented_literal.rs:16:31
|
LL | let _ = create_openai_url(None, 3);
| ^^^^ help: prepend the parameter name comment: `/*base_url*/ None`
|
note: the lint level is defined here
--> $DIR/uncommented_literal.rs:1:9
|
LL | #![warn(uncommented_anonymous_literal_argument)]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
warning: anonymous literal-like argument for parameter `retry_count`
--> $DIR/uncommented_literal.rs:16:37
|
LL | let _ = create_openai_url(None, 3);
| ^ help: prepend the parameter name comment: `/*retry_count*/ 3`
warning: anonymous literal-like argument for parameter `enabled`
--> $DIR/uncommented_literal.rs:17:21
|
LL | client.set_flag(true);
| ^^^^ help: prepend the parameter name comment: `/*enabled*/ true`
warning: 3 warnings emitted