Skip to content

Run Clippy on CI #2201

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

Run Clippy on CI #2201

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f8ab2f4
Run Clippy on CI
ada4a Oct 15, 2025
a92cf22
Handle current warnings
ada4a Oct 15, 2025
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
5 changes: 5 additions & 0 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,11 @@ jobs:
run: cargo test --workspace --all-targets
- name: Check formatting
run: cargo fmt --all --check
- name: Lint
run: |
# Install Clippy (not included in the `minimal` profile)
rustup component add clippy
cargo clippy --all --all-targets
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would add -- -Dwarnings here -- let me know if that's desired

Copy link
Member

Choose a reason for hiding this comment

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

Unclear if we want to block triagebot CI on clippy. Any opinions @rust-lang/triagebot?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I would not like to enforce Clippy in the CI. I gave my option already in #1950, unsure why this work about Clippy linting moved forward.

Copy link
Member

Choose a reason for hiding this comment

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

I personally use cargo clippy -- -Dwarnings on many Rust projects, but no strong opinion on having it here. If you two are opposed, I'm fine with not enforcing it.

Copy link
Member

Choose a reason for hiding this comment

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

I would not like to enforce Clippy in the CI

If I may ask, what are you concerns with enforcing clippy lints?

unsure why this work about Clippy linting moved forward.

Because clippy lints are generally useful, and I did exactly what you suggested in the issue1: I carefully reviewed the changes, asked for them to split up, vetoed some lints, all of that to improve the quality of the codebase.

Footnotes

  1. https://github.com/rust-lang/triagebot/issues/1950#issuecomment-2820951235 ↩

Copy link
Contributor

Choose a reason for hiding this comment

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

what are you concerns with enforcing clippy lints?

clippy is a bit of a moving target so linting this codebase moves the requirements for contributors as well. My understanding is that clippy is meant more as a "personal choice" for the developer. We prevent contributors to choose their own style of linting. If there are disagreements on a lint we then would need to add allow/deny directives which (imo) is noise in the code.

For all these reasons I think I don't see many Rust projects enforcing linting besides rustfmt but I'd like to hear if there are counterarguments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

clippy is a bit of a moving target

Not having -- -Dwarnings helps with that a bit, as on a new Clippy version, the CI will in the worst case get a couple more warnings, instead of breaking completely.

On the other hand, my personal argument for -- -Dwarnings is that it much more strongly stops new PRs from introducing additional warnings, requiring every PR to "clean-up" after itself.

We prevent contributors to choose their own style of linting

At least the warn-by-default lints are meant to be fairly uncontroversial -- if there's disagreement about enabling any of those, that could be considered a reason to ask for the lint to be downgraded to allow-by-default.

Copy link
Contributor

Choose a reason for hiding this comment

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

Personally I'm not a fan of the default clippy set of warnings. The correctness and complexity groups are the only ones that seem relatively safe to enable. perf, style, and suspicious tend to have too many false positives or noise for my taste (or I just disagree with them). There are individual lints that are useful, but the list is so huge that figuring that out is too much of an annoyance for me.

However, I'm not terribly active here, so I don't have a strong say.


deploy:
name: Deploy
Expand Down
4 changes: 0 additions & 4 deletions src/handlers.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,10 +69,6 @@ pub struct Context {
pub gha_logs: Arc<tokio::sync::RwLock<GitHubActionLogsCache>>,
}

#[expect(
clippy::collapsible_if,
reason = "we check the preconditions in the outer if, and handle errors inside"
)]
pub async fn handle(ctx: &Context, host: &str, event: &Event) -> Vec<HandlerError> {
let config = config::get(&ctx.github, event.repo()).await;
if let Err(e) = &config {
Expand Down
2 changes: 0 additions & 2 deletions src/rfcbot.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@ pub struct Concern {
pub reviewer: Reviewer,
}
#[derive(Serialize, Deserialize, Debug, Clone)]
#[expect(clippy::upper_case_acronyms, reason = "see reason on `FCP`")]
pub struct FCPIssue {
pub id: u32,
pub number: u32,
Expand Down Expand Up @@ -66,7 +65,6 @@ pub struct StatusComment {
}

#[derive(Serialize, Deserialize, Debug, Clone)]
#[expect(clippy::upper_case_acronyms, reason = "see reason on `FCP`")]
pub struct FullFCP {
pub fcp: FCP,
pub reviews: Vec<Review>,
Expand Down
Loading