Skip to content

Add argument-comment Dylint runner#14651

Merged
bolinfest merged 1 commit intomainfrom
pr14651
Mar 14, 2026
Merged

Add argument-comment Dylint runner#14651
bolinfest merged 1 commit intomainfrom
pr14651

Conversation

@bolinfest
Copy link
Collaborator

@bolinfest bolinfest commented Mar 14, 2026

Why

Call sites like foo(false) and bar(None) are often a sign that the API is not carrying enough meaning on its own. Contributors should prefer self-documenting designs such as enums, named methods, and newtypes when that is practical.

This PR adds a repo-local lint as a fallback for the places where a larger API refactor would be too heavy. The team also preferred a lighter comment shape than clang-tidy's bugprone-argument-comment, so the convention here is exact /*param*/ comments rather than /*param=*/.

What changed

  • add an isolated Dylint library under tools/argument-comment-lint
  • add argument_comment_mismatch, which validates that an existing /*param*/ comment matches the resolved parameter name
  • add uncommented_anonymous_literal_argument, which targets opaque positional literals such as None, booleans, and numeric literals
  • keep string and char literals exempt so obviously self-descriptive arguments do not need extra comment noise
  • update the parser, diagnostics, examples, and UI coverage to use the exact /*param*/ form
  • add tools/argument-comment-lint/run.sh, a just argument-comment-lint entrypoint, and default CARGO_INCREMENTAL=0 in the wrapper to avoid the current nightly Dylint incremental-compilation ICE in local runs
  • add AGENTS.md guidance that pushes contributors toward better API design first and treats argument comments as the fallback when a smaller compatibility-preserving change is more appropriate

Examples

Preferred when a positional literal still needs help at the callsite:

fn create_openai_url(base_url: Option<String>, retry_count: usize) -> String {
    String::new()
}

create_openai_url(/*base_url*/ None, /*retry_count*/ 3);

Rejected when the comment name is wrong:

fn create_openai_url(base_url: Option<String>) -> String {
    String::new()
}

create_openai_url(/*api_base*/ None);

Still intentionally allowed without comments for self-descriptive literals:

fn split_top_level(body: &str, delimiter: char) -> Vec<&str> {
    Vec::new()
}

fn describe(prefix: &str, suffix: &str) {}

split_top_level("a|b|c", '|');
describe("openai", "https://api.openai.com/v1");

Verification

  • cargo test in tools/argument-comment-lint
  • bash -n tools/argument-comment-lint/run.sh

Stack created with Sapling. Best reviewed with ReviewStack.

Copy link
Contributor

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f9165c43e8

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +161 to +163
if !def_id.is_local() {
return;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Lint cross-crate calls instead of skipping non-local defs

Returning early on !def_id.is_local() means the lint never inspects calls to functions/methods defined in other crates, so when just argument-comment-lint runs per package it will miss argument-comment violations at crate boundaries (for example, a codex-cli call into codex-core). That significantly weakens enforcement of the new convention across the workspace and can let mismatched or missing /*param=*/ comments slip through in normal usage.

Useful? React with 👍 / 👎.

Copy link
Collaborator

@etraut-openai etraut-openai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding this!

@bolinfest bolinfest merged commit 4b31848 into main Mar 14, 2026
60 of 64 checks passed
@bolinfest bolinfest deleted the pr14651 branch March 14, 2026 15:18
@github-actions github-actions bot locked and limited conversation to collaborators Mar 14, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants