Fix Clippy warnings and errors

[casey]

Running cargo clippy produces many (> 550) warnings and errors. These should be fixed, or, on a case-by-case basis, lints can be disabled. Additionally, clippy should probably run as part of CI, failing builds if new warnings or errors occur. Some of the warnings are style issues, but some indicate bugs. For example:

error: non-binding let on a synchronization lock
    --> lightning/src/ln/channelmanager.rs:3662:3
     |
3662 |         let _ = self.total_consistency_lock.write().unwrap();
     |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
     |

Unlike other bindings, let _ = ... immediately drops the right-hand side of the binding, which is almost certainly not what is intended.

[TheBlueMatt]

Unlike other bindings, let _ = ... immediately drops the right-hand side of the binding

Huh, that is...an incredibly surprising language "feature".

In general I haven't spent any time looking at clippy because when I've looked in the past its all been lints telling you to upgrade to the latest features of rustc, when we have an MSRV that isn't "current nightly", but it sounds like something we should definitely spend some time looking into.

[casey]

Huh, that is...an incredibly surprising language "feature".

It is an undesirable consequence of consistency between pattern matching in this and other contexts, where not holding on to values is desirable.

Also, it is preferable to avoid having types like RwLock<()>. Ideally, locks should wrap the data that they are meant to protect, so that this bug is impossible. This may be difficult, but is worth considering.

Of the 550 warnings, the majority do not require a particular version of rustc to fix, and the rest can be disabled on a case-by-case basis.

Lints can be disabled using #![allow(clippy::LINT)] in the crate root, a module, or function body.

[TheBlueMatt]

It is an undesirable consequence of consistency between pattern matching in this and other contexts, where not holding on to values is desirable.

Ah, that explains it...definitely not clear from just scanning the code, of course, though. Also awkward that _X is different from _.

Ideally, locks should wrap the data that they are meant to protect, so that this bug is impossible. This may be difficult, but is worth considering.

Right, we use that pattern where we can, though in this case we have a redundant lock to let us take a "big global write lock" which covers several other things which have their own locks. We can maybe move them inside, though, which would fix it, but we're due to redo all our channel locks anyway.

[douglaz]

@TheBlueMatt would you accept PRs focused on reducing clippy warnings? (either by changing the code to comply or by setting clippy to allow/ignore that code if the lint doesn't make much sense)

[TheBlueMatt]

It depends a bit on the warning - if the warning is telling us to change to match some style guide that isn't really addressing readability in any way but is just some astract value judgement on code style, probably not. If its actually a useful cleanup, yes, absolutely. I'm not a huge fan of a big pile of #[clippy::ignore_useless_warning]s cluttering the code everywhere, but if its a very small set I'd be okay with it.

[cyphersnake]

On 1.70 rust-toolchain - 1789 warnings (1253 duplicates)
Currently using toolchain 1.47 in CI.

Has there been any change of opinion on reducing the number of warnings? Would a pull request with reduced warnings and increased toolchain version at CI be relevant?

[TheBlueMatt]

AFAIK the crates here build without any (addressable) rustc warnings, and we try to maintain that as much as possible. As indicated above, if some other tooling suggests changing stuff around then that will be evaluated as any other PR - if its an improvement in readability or code cleanliness or performance or correctness it should definitely get merged, if its just moving things around to make some tooling happy, I'm not sure why we'd bother. Sadly clippy isn't standard tooling in that it can only be run with rustup and so isn't always available for developers.