Run clippy in the CI with warnings disallowed; address most warnings

[MarijnS95]

Depends on #1413 + #1408.

This is a draft mainly to look at how cargo clippy --fix and myself have addressed warnings for most issues. There are still a few remaining, and clippy found some odd/fishy things we should look into.

• Tested on all platforms changed
  • Android + EGL;
  • Wayland + EGL;
  • Wayland + EGL on sharing example (doesn't run at all, but has nontrivial clippy warnings);
  • Windows.
• Compilation warnings were addressed
• cargo fmt has been run on this branch
• cargo doc builds successfully
• Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users
• Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
• Created or updated an example program if it would help users understand this functionality

[MarijnS95]

Looks like nightly clippy is throwing some false-positives on objc::msg_send![]'s return-type annotation https://docs.rs/objc/latest/objc/#messaging-objects.

That raises the question: is it desired to run nightly clippy? I'm personally all in favour of having clippy in the CI (some call it low signal-to-noise ratio, I find it making the code more pleasant to read), but nightly clippy is a bit overzealous. It has a high amount of false-positives compared to the actual future-proofing one might get; better wait for the stabilized version.

So for now this PR will only run stable clippy :)

[kchibisov]

Running on nightly doesn't make much sense, except if the target requires nightly for whatever reason. Just stable is enough, I guess.

[MarijnS95]

Doing that now, and all checks succeed 🎉 - what's your thought about getting the other PRs and this one in?

[kchibisov]

Actually, could you add into lib.rs of each crate.

#![deny(clippy::all)
#![cfg_attr(feature = "cargo-clippy", deny(warnings))]

Instead of denying on CI?

[MarijnS95]

@kchibisov What would be the advantage of those annotations? I personally don't like disallowing all clippy warnings during development, it doesn't allow contributors to compile code when perhaps having one or two testing variables / functions commented out.

Secondly, what's the advantage of disallowing normal rustc warnings through a cargo-clippy feature? I don't think I've seen such a setup yet.

[kchibisov]

@kchibisov What would be the advantage of those annotations?

I mean that with those two you can compile normally, but when you run clippy everything errors out. The intention is to error out on clippy, while allowing checks/builds to throw warnings as usual.

Secondly, what's the advantage of disallowing normal rustc warnings through a cargo-clippy feature? I don't think I've seen such a setup yet.

That's what we're using in alacritty for years. Basically it means: "when running clippy, treat all rustc warnings as errors".

[MarijnS95]

I mean that with those two you can compile normally, but when you run clippy everything errors out. The intention is to error out on clippy, while allowing checks/builds to throw warnings as usual.

Right, yes. I use clippy in rust-analyzer (to see clippy lints directly in my editor, instead of only rustc warnings) and it typically stops linting on the first crate that encounters errors (deny, -Dwarnings) meaning it was not working on glutin_examples until #1408/#1413 were addressed (both are deny lints).

Strangely enough it seems to be fine now, without those commits 🤷

That's what we're using in alacritty for years. Basically it means: "when running clippy, treat all rustc warnings as errors".

Ack, understood. I have previously seen but forgot that cargo-clippy is an actual feature (not cfg) that's enabled inside crates when "compiling" code under clippy.

Do you mind if I leave deny(clippy::all) out? That group consists of warn/deny lints only, and is already unambiguously covered by #![cfg_attr(feature = "cargo-clippy", deny(warnings))]. For example:

note: `#[deny(clippy::missing_safety_doc)]` implied by `#[deny(warnings)]`

Instead of denying on CI?

Do you mind if I leave this? In the hypothetical case that a new crate is added? Otherwise this should also be added to every binary (test/example) target.

I think that's the downside of this approach: having to add it to every target and not forgetting it on new targets. IMHO it's fine for users to push and open a PR, and address remaining lints after the fact. They usually won't (be able to) test/lint against every target / feature combination anyway. If they run clippy on their own accord at all :)

[kchibisov]

Do you mind if I leave deny(clippy::all) out? That group consists of warn/deny lints only, and is already unambiguously covered by #![cfg_attr(feature = "cargo-clippy", deny(warnings))]. For example:

That's fine. You can go ahead.

[MarijnS95]

@kchibisov I think I did that in the last push, would you mind re-reviewing this now? What's your suggestion on the sharing example, the drop-changes seem trivial yet against intended usage, but I can't test it on my machine.

[kchibisov]

@MarijnS95 sharing is also failing for me. It could be failing that way for a long time on Wayland, and I won't be surprised. Not sure that we should bother with it here since it's not something new.

[MarijnS95]

@kchibisov Perfect, let's go for it then. If you can review this once more and approve it, I'll fix the conflict and re-push!

[kchibisov]

Just a small nits I've found after re-review.

[MarijnS95]

Nice catch, I hadn't spotted that one in the automatic clippy --fix diff :)