Add clippy and rustfmt to CI

[notlesh]

What does it do?

This PR attempts to include both clippy and rustfmt into our CI workflow.

What important points reviewers should know?

Please examine the changes closely to ensure they are sane. It would be better to adjust any formatting/linting configuration before merging this.

This PR should leave the code functionally equivalent.

Is there something left for follow-up PRs?

No (not yet)

What alternative implementations were considered?

If we don't like either rustfmt or clippy, we could adopt only one of them.

Are there relevant PRs or issues in other repositories (Substrate, Polkadot, Frontier, Cumulus)?

No

What value does it bring to the blockchain users?

This should improve code quality which should lead to improved developer productivity.

Checklist

• Does it require a purge of the network?
• You bumped the runtime version if there are breaking changes in the runtime ?
• Does it require changes in documentation/tutorials ?

Details

This PR introduces clippy and rustfmt into CI.

To use these locally, run cargo clippy or cargo fmt respectively. Note that clippy will only produce errors which must be fixed; rustfmt will actually rewrite your code (it is "destructive"). cargo fmt -- --check will prevent this.

See 6f4e984 for the changeset introduced by rustfmt.

Remaining TODO:

• Clippy and CI do odd things (clippy runs off into .cargo/) when run via CI
• Clippy lint warnings need to be handled case-by-case
• Team review of configuration (note that .rustfmt.toml previously existed)
• Reintroduce license file checking (this produces errors on files with other licenses, e.g. "copyright Parity")

clippy vs. rustfmt

Find clippy here and rustfmt here.

rustfmt is strictly concerned with stylistic code formatting; this includes such things as when to introduce new lines, where spacing should and should not be, how many parameters can exist on one line, etc. It generally should not produce discernible changes in compiled code.

rustfmt's config is controlled by our .rustfmt.toml file. This file existed previously to this PR and appears to have been written to agree with .editorconfig.

clippy is concerned with more logical errors. An example of the type of complaint clippy might produce is comparing 0 > someUnsignedInt, which could never evaluate to true. See a list of clippy's linters here. The defaults for clippy seem reasonable, but I will have a better feel for this after addressing some of its complaints.

[JoshOrndorff]

The code changes look great to me. I'll be happy to have these tools keeping our code clean.

The .rustfmt.toml file also looks good. It is consistent with our .editorconfig as well as Parity's styleguide (as the comment says).

[4meta5]

I'm in favor of adding both, but definitely at least the .rustfmt.toml.

In the past, I've disabled clippy lints with annotations at the top of the root file i.e. #![allow(clippy::type_complexity)].

[crystalin]

I'm surprised that clippy is needed when the rust compiler is so strict. Catching things like impossible conditions should be done by the compiler.
Anyway, I'll spend some time testing it.
Also we probably need to provide easy instruction to include it in our team IDEs

[notlesh]

I'm surprised that clippy is needed when the rust compiler is so strict. Catching things like impossible conditions should be done by the compiler.

That's actually a really good point. The 0 > uint is something that I've seen other compilers complain about.

I can document using these in both vim and vs-code. What other IDEs do we want to support?

[crystalin]

vim and vscode are probably the only one we need right now

[crystalin]

I gave it a quick try and didn't have much luck with vscode neither for rustfmt nor clippy. I let you look deeper Stephen and let me know

[crystalin]

Also, to fix your CI, you need to push your Cargo.lock after building it (parachain and also standalone)

[notlesh]

These changes don't seem relevant to this PR -- is this what you expected, @crystalin ?

[notlesh]

I gave it a quick try and didn't have much luck with vscode neither for rustfmt nor clippy. I let you look deeper Stephen and let me know

This might be suitable for vscode:

https://users.rust-lang.org/t/how-to-use-clippy-in-vs-code-with-rust-analyzer/41881

This looks promising for Vim (although I find that syntastic gets in my way too often to be useful):

https://vimawesome.com/plugin/vim-clippy

Another option would be to add a git-commit hook. This looks promising for managing git hooks:

https://crates.io/crates/rusty-hook

[JoshOrndorff]

This looks good to me!

I agree with Alan that we want to document how to use this with some common IDEs so devs can be maximally productive with it.

[notlesh]

This last commit (c9042b3) adds a git pre-commit hook using rusty-hook (https://crates.io/crates/rusty-hook). To use this, you need to run cargo test one time; this will cause rusty-hook to be downloaded and it will initialize git-hooks for you based on .rusty-hook.toml.

Once this is initialized, a git pre-commit hook will invoke cargo fmt -- --check && cargo clippy every time you attempt a git commit and will fail if either produce a complaint. Any complaints will be clearly visible on the command line.

I would appreciate any feedback on this. This invocation of cargo, in my limited testing, tends to have work to do, which can be quite an annoyance when you were expecting to commit something.

Also note that removing these hooks is a bit of a pain (if you had no previous hooks set up, you'll want to rm all hooks under .git/hooks/).