Massive amounts of clippy warnings

[brxken128]

After running:

git clone https://github.com/Argyle-Software/kyber kyber-clippy-test && cd kyber-clippy-test && cargo clippy --features=90s-fixslice -- -W clippy::pedantic -W clippy::correctness -W clippy::perf -W clippy::style -W clippy::suspicious -W clippy::complexity -A clippy::missing_errors_doc -W clippy::nursery -A clippy::module-name-repetitions -D warnings

I get a total of 547 errors, many of which could be pretty bad (I use even harsher lints in my lib.rs, there's a lot of warnings). Even with just cargo clippy --features=90s-fixslice I get 45 warnings.

I do also understand that these are some pretty harsh lints, and the lints I personally use in my work are even harsher, but I can't begin to imagine the amount of subtle bugs/performance issues they've solved - and my code is a lot more idiomatic.

The majority of the issues stem from needless borrowing or redundant slicing, but as_conversions are probably something to keep an eye on, as well as possible truncations.

I've forked the repo and started cleaning it up and making things more safe, but sadly I removed a lot of the functionality present here as it's not requried for our needs, otherwise I would've contributed my changes. I spent a few hours and got down to 140~ errors, most of which are now just as_conversions, which I'll still be fixing and making safe as I've had an issue with an as x conversion before that took far too long to debug, so now I use From/Into or their fallible counterparts.

This evidently isn't a major issue, so feel free to close it, I just wanted to bring attention to it as clippy is great at catching subtle mistakes. It could be good to add into CI too.

[mberry]

It's a fair issue and happy for discussion on it, but do understand this was written as a direct translation of the C reference code for inspection and correctness. The issue for more idiomatic rust is #68, and even that is still missing improvements like AsRef<u8> function arguments.

It's certainly personal preference but lints like these:

r[idx + 0] = (t[0] >> 0) as u8;
                  ^^^^^^^^^^^ help: consider reducing it to: `t[0]`

For me makes it clearer about the repeated operations in code and simpler to grok for anyone looking at the code. It's also how it's written by the Kyber team.

Yes you can remove the bitshift and addition to appease clippy at the tradeoff of readability in a sea of complexity.

I spent a few hours and got down to 140~ errors, most of which are now just as_conversions, which I'll still be fixing and making safe as I've had an issue with an as x conversion before that took far too long to debug, so now I use From/Into or their fallible counterparts.

They are lints, not errors. Using as X is explicit about variable casting while .into() is usually opaque. It will probably be an uphill battle changing that view without explicit typing for every cast.

You could submit a PR on individual lint basis and we'll go from there? Unnecessary borrowing is certainly fine, other things like conversion semantics will be different and requires more attention. I won't get your hopes up here, it likely won't get down to zero but we can reduce it.

[mberry]

So I'd really like to clean up what we can and get a clippy.toml in for CI, that said, going through what seemed like the good first lints to tackle I'm a bit surprised at how buggy clippy actually is.

Example:

warning: the loop variable `i` is used to index `a`
   --> src/reference/indcpa.rs:140:14
    |
140 |     for i in 0..KYBER_K {
    |              ^^^^^^^^^^
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_range_loop

Cool, except i is also used as a function argument 3 lines after that?

if transposed {
    xof_absorb(&mut state, seed, i as u8, j as u8);

I guess they want for (polyvec, i) in a.iter().enumerate(){ instead but that defeats the point because i is still there in the end and we have yet another variable created?

Still keen to get this worked out, might need a few exemptions in the file while these sorts of bugs/perhaps intentional style things exist.

Related:

• needless_range_loop intrusiveness rust-lang/rust-clippy#6075
• False positive on loop variable being used to index a slice rust-lang/rust-clippy#5625

[tarcieri]

FWIW we often disable the needless_range_loop lint in @RustCrypto:

#![allow(clippy::needless_range_loop)]

Indeed it can be unhelpful at times where its suggestions don't improve code clarity.

[mberry]

There's a few allows in modules already that should probably be removed, a repo-wide clippy.toml file seems like the best way forward.

[tarcieri]

If you place the above attribute in lib.rs, it will apply to the whole crate.

I wasn't aware clippy.toml could be used for this purpose. If so that's a semi-recent addition (last I checked, they explicitly decided against that?)

[mberry]

they explicitly decided against that

Perhaps, yet as far as I see it using a config file is still how they present it in the docs?

https://github.com/rust-lang/rust-clippy#configure-the-behavior-of-some-lints

Clippy tends to attract the natural urge of bikeshedding which is fine but in the end code quality is what matters most, this always takes precedence.

Personally prefer 2 space indentation but gave into doing to things the cargo fmt way so now we have CI checks enforcing the dreaded 4 spaces :)

[brxken128]

AFAIK, only some lints can be configured/set this way. I do still believe you need to declare the vast majority of them in your root lib.rs file, but a clippy.toml file can be used for things such as disallowing methods (either in favour of another similar method, or outright banning them).

[mberry]

Haven't actually checked but this is due to many lints needing nightly to run I think?

For instance if you want 2 space indentation then you need to run cargo fmt +nightly instead of cargo fmt

Edit: sorry I'm conflating fmt with clippy

[brxken128]

Haven't actually checked but this is due to many lints needing nightly to run I think?

For instance if you want 2 space indentation then you need to run cargo fmt +nightly instead of cargo fmt

Quite a lot of lints are experimental/nightly only/unstable.

I'd use the dedicated rustfmt.toml file for formatting, though.

[faern]

I guess they want for (polyvec, i) in a.iter().enumerate(){ instead but that defeats the point because i is still there in the end and we have yet another variable created?

I'd argue .iter().enumerate() indeed is more idiomatic and nice. It does create an extra variable, but is that bad? It does remove a bounds check and a point of potential panic (slice indexing)