Enable clippy::pedantic

[martinthomson]

There are a bunch of problems that might be revealed by clippy::pedantic. While false positives are a risk, we can suppress individual warnings as they are analyzed.

[o0Ignition0o]

Ok so the PR is going to be huge, maybe it makes more sense if I make several smaller PRs while keeping the linter in clippy::all mode, and we can add the clippy::pedantic once I've resolved all the lints ?

[agrover]

especially since this is likely to cause merge conflicts in other in-flight PRs, smaller chunks might be good.

[o0Ignition0o]

Ok I'll remove the pedantic flag and submit it as is for now.

[agrover]

how's this going?

[o0Ignition0o]

Not too well actually, The Merge Requests become huge, so I've started working on a tool that would allow us to enable pedantic only on our diffs, so we can progressively increase the quality of the codebase, but I haven't gotten too far yet.

OTOH this action might prove useful : https://github.com/actions-rs/clippy-check

It's up to you: Do we want to enable pedantic asap, and handle the conflicts / huge PRs, or would we rather use such a github action for example and improve the quality progresively ?

[agrover]

I guess the second option sounds better to me...

[o0Ignition0o]

I've given it an other look, and it seems the clippy action does not focus on the code that has been changed, so I'll try to work on cargo-scout a bit more, as soon as I get some spare time :)

[o0Ignition0o]

So the cargo-scout executable is almost ready (https://github.com/o0Ignition0o/cargo-scout), but as explained in rust unofficial anti patterns #[deny(warnings)] prevents cargo clippy to complete and list all the lints, and it fails early.

Clippy went around that with a feature gate which allows the command runner (in our case the CI) to pass the -D warning flag.

I'm going to release a 0.2 for cargo-scout, and I would love to create a pull request to neqo which would feature gate #[deny(warnings)] and have the CI fail if a warning is present anyway.

Do you think feature gate-ing #[deny(warnings)] would make sense in neqo ?

Again thanks for your time and patience.

[martinthomson]

As long as the default feature set included #[deny(warnings)], I'd be very happy with that.

[o0Ignition0o]

Ok so I think I just opened pandora's box, given the state of packages / default features in workspaces seems to be pretty complicated right now (see rust-lang/cargo#5364 and rust-lang/cargo#7507 for instance)

A workaround given we want #deny(warnings) to remain enabled by default would be to iterate over each crate included in the workspace, and run clippy on explicitly enabled features only.

I'm going to give it a try this weekend

[o0Ignition0o]

Small heads up:

• Once this pull request has landed, cargo-scout will be able to iterate over all workspace members.
• As mentionned here using a feature that is not quite stable yet cargo +nightly clippy-preview -Z unstable-options --no-default-features -- -W clippy::pedantic worked on neqo and --no-default-features worked out as expected in workspace members.

I'll probably add a custom flag to cargo-scout to enable clippy-preview, which would allow us to use it on neqo, and finally display only relevant lints for a given pull request 🎉

[o0Ignition0o]

As explained in this comment cargo-scout 0.3.0 is out and has all the required features to run on neqo.

cargo-scout -p --no-default-features works well everywhere (-p makes it use clippy-preview with unstable features), except when there's a build-rs file (which is the case in neqo-crypto).

I need to figure out why that is, and find a workaround.
Once it is done we might be able to use it to progressively finish the clippy::pedantic lints !