add clippy to CI #9694

gilescope · 2021-09-04T17:16:16Z

This PR enforces the majority of the lints from clippy's correctness category.

Future PRs can gradually turn additional lints on where we all agree there's value in the lint.

CI doesn't know about it yet.

TriplEight

cc rust-lang/rust-clippy#7353

.gitlab-ci.yml

lints.toml

.gitlab-ci.yml

gilescope · 2021-09-19T12:06:11Z

CI now should have clippy thanks to cumulus: paritytech/scripts#342

bkchr · 2021-09-19T12:23:07Z

I'm personally not convinced about clippy and its value.

People can use clippy locally as part of their IDE and fix what it complains about in their code, but in overall I don't see any need to force everyone to use this.

gilescope · 2021-09-19T13:32:54Z

It's clear that we adhere to subsets of clippy's rules. For example if we run ``` cargo clippy -- -A clippy::all -D clippy::correctness ``` on substrate then there's only one violation from that set of rules in the codebase - `unsafe fn memory_as_slice_mut(&self) -> &mut[u8]`. That's a pretty good sign that our values and clippys values align for certain categories of lints. I'm not suggesting we turn clippy up to 11, but I do think there's value turning it up from 0 currently to 3 or 4. At some point it's going to catch a prod issue before it escapes into the wild.

…

On Sun, 19 Sept 2021 at 13:23, Bastian Köcher ***@***.***> wrote: I'm personally not convinced about clippy and its value. People can use clippy locally as part of their IDE and fix what it complains about in their code, but in overall I don't see any need to force everyone to use this. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#9694 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAGEJCCA7IRJPVPZDHCO6B3UCXI3LANCNFSM5DNPFPOQ> .

kianenigma · 2021-09-19T14:01:38Z

I'm personally not a fan of Clippy, nor have I ever found a good spot for it in my workflow locally. But I am open to allowing some lints that don't have a huge initial impact and gradually find a balance.

dvdplm · 2021-09-20T07:31:16Z

I like clippy and its community; more than once I've learned about some new Rust feature through its warnings. If we do this sensibly (read: avoiding pedantry) and gradually I have no objections.

TriplEight

Btw why not a toml config?

.maintain/clippy.sh

gilescope · 2021-09-20T10:56:02Z

Just focusing on the correctness lints. There's a couple of places where we impl Hash explicitly but we rely on the derived PartialEq. That makes for very hard to find bugs in my experience. ( https://rust-lang.github.io/rust-clippy/master/index.html#derive_hash_xor_eq ). That's the kind of lint I'm happy to have turned on.

client/network/src/state_request_handler.rs:81:1
client/network/src/block_request_handler.rs:88:1

Hmm, I think this way round is not so much of a problem - if we were just deriving Hash we could introduce a bug here, but these definitions look ok to me.

TriplEight · 2021-09-20T14:41:44Z

To be merged after #9637

gilescope · 2021-09-25T11:11:51Z

Someone somewhere asked about how rust analyser would stay in sync with the clippy lints defined in the .sh file.
After a bit of digging through the history ( rust-lang/cargo#5034 ) it looks like we can now define this in rustflags in .cargo/config.toml which should mean that every tool has the same view.

This relies on a compiler patch that landed 8th July 2021. If people are using an earlier version of the compiler everything will still work unless they try and run clippy.

client/network/src/block_request_handler.rs

client/executor/wasmtime/src/instance_wrapper.rs

client/executor/src/wasm_runtime.rs

Co-authored-by: Bastian Köcher <[email protected]>

athei

People can use clippy locally as part of their IDE and fix what it complains about in their code, but in overall I don't see any need to force everyone to use this.

I am in favor on enforcing clippy globally because I never found a way to do exactly this. rust-analyzer annoys me with warnings from the whole workspace no matter in which sub directory I open my IDE in.

.gitlab-ci.yml

.cargo/config.toml

gilescope · 2021-10-01T09:27:00Z

Even clippy critics seem to care about the issues that clippy has raised here, so clippy as currently configured seems to be focusing on the right things and is providing us with additional assurances (while not being too noisy).

I'm going to merge this as there's sufficient thumbs-up thought I do note that people are in two minds about this. Let's try it for a few weeks and see if it causes any disruption. We can rollback easily or mute certain lints if they're not paying their way.

TriplEight

Aight to merge after these nits. Those who will have to rebase on this should not face much trouble.

.gitlab-ci.yml

athei · 2021-10-01T10:21:38Z

It appears that there is support for a clipply.toml: https://github.com/rust-lang/rust-clippy#configuration

Why aren't we using this?

gilescope · 2021-10-01T10:30:30Z

Clippy.toml only allows you to specify how much cyclometric complexity you're willing to tolerate etc. It does not allow you to specify whether to allow or deny any of the lints. There's a very long thread on this: TL/DR looks like there's a PR to allow common metadata in workspaces; after that they might make it specifiable in Cargo.toml. The reason for Clippy.toml not being in the running is that these clippy allow / deny should be specified in the same way as tuning on/off rustc lints are. (They have been debating this point for 5 years now, meanwhile as of July this year, the .cargo/config.toml workaround is now viable)

athei · 2021-10-01T11:34:59Z

I see thanks.

Co-authored-by: Denis Pisarev <[email protected]>

* master: (125 commits) Update multiple dependencies (#9936) Speed up timestamp generation when logging (#9933) First word should be Substrate not Polkadot (#9935) Improved file not found error message (#9931) don't read events in elections anymore. (#9898) Remove incorrect sanity check (#9924) Require crypto scheme for `insert-key` (#9909) chore: refresh of the substrate_builder image (#9808) Introduce block authorship soft deadline (#9663) Rework Transaction Priority calculation (#9834) Do not propagate host RUSTFLAGS when checking for WASM toolchain (#9926) Small quoting comment fix (#9927) add clippy to CI (#9694) Ensure BeforeBestBlockBy voting rule accounts for base (#9920) rm `.maintain` lock (#9919) Downstream `node-template` pull (#9915) Implement core::fmt::Debug for BoundedVec (#9914) Quickly skip invalid transactions during block authorship. (#9789) Add SS58 prefix for Automata (#9805) Clean up sc-peerset (#9806) ...

no more clippy slippage

7c4b1fd

gilescope requested a review from a team as a code owner September 4, 2021 17:16

gilescope added 2 commits September 4, 2021 18:19

For testing include clippy explicitly

ea7eb38

Commenting out new lint

e3ad291

CI doesn't know about it yet.

gilescope changed the title ~~WIP: add clippy to CI~~ add clippy to CI Sep 4, 2021

gilescope added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Sep 4, 2021

Add instructions

9289226

TriplEight suggested changes Sep 8, 2021

View reviewed changes

.gitlab-ci.yml Outdated Show resolved Hide resolved

lints.toml Outdated Show resolved Hide resolved

.gitlab-ci.yml Outdated Show resolved Hide resolved

gilescope added 3 commits September 19, 2021 12:46

Remove need for cargo-lints

3de2469

Clippy component already installed

a34944b

nightly clippy

3f6ce99

TriplEight suggested changes Sep 20, 2021

View reviewed changes

.maintain/clippy.sh Outdated Show resolved Hide resolved

Focus on correctness lints

d22159e

These seem well defined.

2a42bf6

TriplEight approved these changes Sep 20, 2021

View reviewed changes

gilescope added 2 commits September 25, 2021 13:01

use 'standard' way to state which lints we opt into.

ce2090f

This relies on a compiler patch that landed 8th July 2021. If people are using an earlier version of the compiler everything will still work unless they try and run clippy.

Avoid the override

421546b

gilescope added 4 commits September 25, 2021 14:12

grr gitlab

e820930

almost

ac0daf9

take 2

720d02b

File was in the .gitignore list

6372041

bkchr reviewed Sep 25, 2021

View reviewed changes

client/network/src/block_request_handler.rs Show resolved Hide resolved

client/executor/wasmtime/src/instance_wrapper.rs Outdated Show resolved Hide resolved

client/executor/src/wasm_runtime.rs Outdated Show resolved Hide resolved

gilescope and others added 3 commits September 26, 2021 06:43

Update client/executor/src/wasm_runtime.rs

49f1e8f

Co-authored-by: Bastian Köcher <[email protected]>

unset

ac1bbcd

Merge branch 'master' into giles-enum-size

0a38ebd

athei reviewed Sep 27, 2021

View reviewed changes

.gitlab-ci.yml Outdated Show resolved Hide resolved

.cargo/config.toml Show resolved Hide resolved

gilescope added 2 commits September 27, 2021 12:06

Include bool in hash

de16dcb

Correctness is mandatory.

8cff766

athei approved these changes Sep 27, 2021

View reviewed changes

TriplEight and others added 3 commits September 28, 2021 13:06

Merge branch 'master' into giles-enum-size

4e1f626

Explicit Ord impl

ea64032

Merge branch 'master' into giles-enum-size

9f44097

TriplEight suggested changes Oct 1, 2021

View reviewed changes

.gitlab-ci.yml Outdated Show resolved Hide resolved

.gitlab-ci.yml Show resolved Hide resolved

Update .gitlab-ci.yml

633f914

Co-authored-by: Denis Pisarev <[email protected]>

gilescope requested a review from TriplEight October 1, 2021 14:47

TriplEight approved these changes Oct 1, 2021

View reviewed changes

gilescope merged commit 5413a1f into paritytech:master Oct 3, 2021

gilescope mentioned this pull request Sep 29, 2022

Member Request polkadot-fellows/seeding#26

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add clippy to CI #9694

add clippy to CI #9694

gilescope commented Sep 4, 2021 •

edited

Loading

TriplEight left a comment

gilescope commented Sep 19, 2021

bkchr commented Sep 19, 2021

gilescope commented Sep 19, 2021 via email •

edited

Loading

kianenigma commented Sep 19, 2021

dvdplm commented Sep 20, 2021

TriplEight left a comment

gilescope commented Sep 20, 2021 •

edited

Loading

TriplEight commented Sep 20, 2021

gilescope commented Sep 25, 2021

athei left a comment

gilescope commented Oct 1, 2021 •

edited

Loading

TriplEight left a comment

athei commented Oct 1, 2021

gilescope commented Oct 1, 2021 •

edited

Loading

athei commented Oct 1, 2021

add clippy to CI #9694

add clippy to CI #9694

Conversation

gilescope commented Sep 4, 2021 • edited Loading

TriplEight left a comment

Choose a reason for hiding this comment

gilescope commented Sep 19, 2021

bkchr commented Sep 19, 2021

gilescope commented Sep 19, 2021 via email • edited Loading

kianenigma commented Sep 19, 2021

dvdplm commented Sep 20, 2021

TriplEight left a comment

Choose a reason for hiding this comment

gilescope commented Sep 20, 2021 • edited Loading

TriplEight commented Sep 20, 2021

gilescope commented Sep 25, 2021

athei left a comment

Choose a reason for hiding this comment

gilescope commented Oct 1, 2021 • edited Loading

TriplEight left a comment

Choose a reason for hiding this comment

athei commented Oct 1, 2021

gilescope commented Oct 1, 2021 • edited Loading

athei commented Oct 1, 2021

gilescope commented Sep 4, 2021 •

edited

Loading

gilescope commented Sep 19, 2021 via email •

edited

Loading

gilescope commented Sep 20, 2021 •

edited

Loading

gilescope commented Oct 1, 2021 •

edited

Loading

gilescope commented Oct 1, 2021 •

edited

Loading