dev: run cargo clippy --all -- -W clippy::all -W clippy::pedantic -W clippy::restriction -W clippy::nursery -D warnings and fix the repo, then add this command in the CI

[Eikix]

Describe the enhancement request

Describe Preferred Solution

Tasks

Preview Give feedback

1. Run cargo clippy --all -- -W clippy::all -W clippy::pedantic -W clippy::restriction -W clippy::nursery -D warnings in the repo
2. Fix the problem. If there are too many, split the clippy run into each category: pedantic, restriction, nursery, etc. and make new issues and repeat the process
3. Then, add a CI check in the cargo clippy action to incorporate all these

[bhavyagosai]

hey can I take this up?

[Eikix]

Sure!
If the result of this command is huge (as in lots of comments by clippy), be sure to discuss in this issue's discussion which fixes to implement first. Don't be scared to split the fixes into multiple PRs.
Finally, if you can find a way for everyone to have a strict clippy, that'd be great, for instance the strictest clippy.toml file.

[bhavyagosai]

yes there are alot of lints there, i am starting with clippy::nursery. also some of the lints have suggestions which are breaking changes so for now i am just fixing things that are non-breaking changes.

[bhavyagosai]

added a PR for clippy::nursery lints, it still contains some lints but they might require breaking changes so will do them in the end

[bhavyagosai]

in clippy::pedantic there are many lints which are related to doc comments. For example, missing # Panic, # Returns etc sections. What should be done regarding them?

[Eikix]

in clippy::pedantic there are many lints which are related to doc comments. For example, missing # Panic, # Returns etc sections. What should be done regarding them?

I think if a test util can panic (meaning it's under a cfg test), it's not worth adding the # panic rustdoc,
If not, I think it's worth it

[Eikix]

added a PR for clippy::nursery lints, it still contains some lints but they might require breaking changes so will do them in the end

What do you mean breaking chage?

[bhavyagosai]

added a PR for clippy::nursery lints, it still contains some lints but they might require breaking changes so will do them in the end

What do you mean breaking chage?

haven't look too deep but i was seeing lints saying Future needs to be Send, some functions that where async need not to be async, a From was suggested to be converted into TryFrom, etc...

so for now i am doing ones that are straight forward and i know for sure that doesn't affect public API

[bhavyagosai]

also according to docs we shouldn't enable this lint groups completely since they can contain many false positives and we would need to use alot of #[allow] everywhere. we should only whitelist lints from those group where they are necessary.

• https://doc.rust-lang.org/clippy/lints.html#pedantic
• https://doc.rust-lang.org/clippy/lints.html#restriction

for example see this lint, it even says in the comment doing this is not idiomatic rust code. I checked other projects and none seem to be using them. For CI i think we can follow what starkware-libs/cairo does https://github.com/starkware-libs/cairo/blob/main/scripts/clippy.sh

what do you think @Eikix?

[Eikix]

also according to docs we shouldn't enable this lint groups completely since they can contain many false positives and we would need to use alot of #[allow] everywhere. we should only whitelist lints from those group where they are necessary.

• https://doc.rust-lang.org/clippy/lints.html#pedantic

• https://doc.rust-lang.org/clippy/lints.html#restriction

for example see this lint, it even says in the comment doing this is not idiomatic rust code. I checked other projects and none seem to be using them. For CI i think we can follow what starkware-libs/cairo does https://github.com/starkware-libs/cairo/blob/main/scripts/clippy.sh

what do you think @Eikix?

Great idea! Is idiomatic rust 2018 the latest version?
But regardless it's a great idea,
Why integrate it in a .sh file though? Can't we use clippy.toml to dictate the behaviour?

[bhavyagosai]

Is idiomatic rust 2018 the latest version?

I think latest is 2021

Why integrate it in a .sh file though? Can't we use clippy.toml to dictate the behaviour?

afaik just so new contributors can run it easily and don't need to remove it, or even add that to git hooks.

[Eikix]

Is idiomatic rust 2018 the latest version?

I think latest is 2021

We can do 2021 if it makes sense for us

Why integrate it in a .sh file though? Can't we use clippy.toml to dictate the behaviour?

afaik just so new contributors can run it easily and don't need to remove it, or even add that to git hooks.

Mmh I'd think clippy.toml file feels sounder (+ CI checks we already have afaik)

[bhavyagosai]

We can do 2021 if it makes sense for us

just checked there is no rust-2021-idioms

Mmh I'd think clippy.toml file feels sounder

i don't think clippy.toml file can be used to enable or disable lints, clippy docs also suggests using flags.

There is rust-lang/cargo#12115 that got recently merged, but using it would require everyone to have latest rust version.

(+ CI checks we already have afaik)

currently CI uses:

cargo clippy --workspace --all-features --all-targets -- -D warnings

but we should also add -D future-incompatible -D nonstandard-style -D rust-2018-idioms -D rust-2021-compatibility -D unused flags as well in CI. and maybe clippy::nursery once Future related llnts are resolved.