Skip to content

Enable clippy linting in repo#1114

Merged
lionel- merged 10 commits intomainfrom
task/clippy-ci
Mar 16, 2026
Merged

Enable clippy linting in repo#1114
lionel- merged 10 commits intomainfrom
task/clippy-ci

Conversation

@lionel-
Copy link
Copy Markdown
Contributor

@lionel- lionel- commented Mar 12, 2026

Branched from #1113
Closes #1104

I'm a bit hesitant enabling it in rust-analyser because of this (from #1104 (comment)):

One UX downside to be aware of: After each edits, clippy runs longer checks on the repo, which locks other gestures requiring compilation (running tests etc).

Image

But I guess we can override in our personal configs if we want a different workflow (check with clippy before sending PR).

@lionel-
Copy link
Copy Markdown
Contributor Author

lionel- commented Mar 12, 2026

oh clippy has LSP fixes!

Screenshot 2026-03-12 at 12 33 29

@lionel- lionel- requested a review from DavisVaughan March 12, 2026 16:54
Comment on lines +16 to 17
#[cfg(target_os = "macos")]
use ark_test::SourceFile;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Uh, what?

#[cfg(target_os = "macos")]
use ark_test::SourceFile;
use url::Url;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Locally the clippy command does not pass for me

cargo clippy --all-targets --all-features --locked -- -D warnings
error: this expression always evaluates to false
  --> crates/harp/src/command.rs:32:14
   |
32 |     assert!(!COMMAND_R_NAMES.is_empty());
   |              ^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#const_is_empty
   = note: `-D clippy::const-is-empty` implied by `-D warnings`
   = help: to override `-D warnings` add `#[allow(clippy::const_is_empty)]`

error: this expression always evaluates to false
   --> crates/harp/src/command.rs:101:14
    |
101 |     assert!(!COMMAND_R_NAMES.is_empty());
    |              ^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#const_is_empty

error: could not compile `harp` (lib) due to 2 previous errors
warning: build failed, waiting for other jobs to finish...
error: could not compile `harp` (lib test) due to 2 previous errors

And indeed LSP clippy is flagging this

Image

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Are you up to date with rustup update stable?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We determined that Davis was using nightly, switching to stable fixed it.

Comment on lines +30 to +32
- uses: Swatinem/rust-cache@v2
with:
save-if: ${{ github.ref == 'refs/heads/main' }}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

IIUC this is actually useful because to run clippy you have to compile

@lionel- lionel- force-pushed the task/clippy-manual-all-targets branch from 8aa7393 to d4d8027 Compare March 16, 2026 14:16
Base automatically changed from task/clippy-manual-all-targets to main March 16, 2026 15:13
@lionel- lionel- merged commit 4057f8d into main Mar 16, 2026
13 checks passed
@lionel- lionel- deleted the task/clippy-ci branch March 16, 2026 16:28
@github-actions github-actions bot locked and limited conversation to collaborators Mar 16, 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.

Enable clippy linting in repo

2 participants