RustCrypto / crypto-bigint

Cryptography-oriented big integer library with constant-time, stack-allocated (no_std-friendly) implementations of modern formulas
Apache License 2.0
167 stars 45 forks source link

Enable `clippy::must_use_candidate` lint + autofix #568

Closed tarcieri closed 5 months ago

tarcieri commented 5 months ago

Many of the computations we perform have results where failure to use them likely constitutes a bug.

clippy can autodetect these cases, and clippy fix can auto-apply them.

fjarri commented 5 months ago

This seems extremely inefficient to me. So any method that doesn't take &mut self and returning something (so, like, 95% of all methods at least) must be declared as must_use? I thought that attribute is for cases where it's easy to make a mistake and forget to check the result, e.g. verify(...) -> Result<(), Error>.

fjarri commented 5 months ago

I wish there was a lint where must_use was the default (under the conditions above), and you could opt out methods from it.

tarcieri commented 5 months ago

@fjarri it's a totally optional lint we can enable. Unfortunately there is no way of changing the default.

Are you concerned about compile times or what?

Since this crate is intended for safety-oriented code, and failure to use the results of these functions pretty much necessarily implies a bug, I think it might be worth enabling.

fjarri commented 5 months ago

Not compile times, just general code noise.

tarcieri commented 5 months ago

Yeah, it is a lint from the pedantic category that carries with it some false positive potential, although looking through what it annotated here, I have a hard time finding anything where I wouldn't agree that "failure to use this return value would constitute a bug"

fjarri commented 5 months ago

Any method that returns a value should have that value used, I just don't see why clippy needs an explicit annotation for that. An annotation that applies to almost every method in the codebase is just noise.

tarcieri commented 5 months ago

The annotation isn't for clippy: it's so downstream users of crypto-bigint get a must_use warning, emitted by rustc itself, in the event they fail to use a value they computed in their downstream code.

It does unfortunately cause a lot of churn in crypto-bigint itself, but that's just how must_use and the Rust compiler work.

tarcieri commented 5 months ago

This seems controversial enough I guess I'll just close it.