Lints Survey

[7596ff]

Lints Survey

This post is intended to collect information on which lints we globally deny or allow, and reasons for those, in order to arrive at a standardized, consistent, and fair list of denied and allowed lints for every crate.

Crates considered:

• cache-inmemory
• gateway
• gateway-queue
• http
• http-ratelimiting
• lavalink
• mention
• model
• standby
• util
• validate

This is the summation of all lints we deny and allow at the crate-level:

#![deny(
    clippy::all,
    clippy::missing_const_for_fn,
    clippy::missing_docs_in_private_items,
    clippy::pedantic,
    future_incompatible,
    missing_docs,
    nonstandard_style,
    rust_2018_idioms,
    rustdoc::broken_intra_doc_links,
    unsafe_code,
    unused
)]
#![allow(
    clippy::let_unit_value,
    clippy::missing_errors_doc,
    clippy::module_name_repetitions,
    clippy::must_use_candidate,
    clippy::semicolon_if_nothing_returned,
    clippy::unnecessary_wraps,
    clippy::used_underscore_binding
)]

rustc lints

future_incompatible, nonstandard_style, rust_2018_idioms, unused

Denied:

• gateway
• http
• http-ratelimiting
• lavalink
• mention
• model
• standby
• util
• validate

Exceptions:

• cache_inmemory::UpdateCache::update: unused_variables (unused)
• gateway::shard::TlsError: dead_code, unused (unused)
• model::util::image_hash: dead_code, unused_mut (unused)

These are lint groups. They are a useful base level for writing idiomatic and future-proof code.

missing_docs

Denied:

• cache-inmemory
• gateway
• http-ratelimiting
• lavalink
• mention
• standby
• util
• validate

This is a lint we strive to include on every crate, but there is a quite a bit of technical debt in
undocumented items. Even if we do not include it in a new standard list of lints, we should in the
future.

unsafe_code

Denied:

• cache-inmemory
• gateway
• gateway-queue
• http
• http-ratelimiting
• lavalink
• mention
• model
• standby
• util
• valdiate

Exceptions:

• model::id::Id::new_unchecked

We should always deny this lint, unless we are re-implementing a std API.

Denied Clippy Lints

clippy::all

Denied:

• gateway
• http
• http-ratelimiting
• lavalink
• mention
• model
• standby
• util
• validate

clippy::pedantic

Denied:

• gateway
• http
• http-ratelimiting
• mention
• model
• util
• validate

Clippy helps ensure higher quality code than just rustc lints can. These two lint groups should be denied on all crates.

clippy::missing_const_for_fn

Denied:

• cache-inmemory
• gateway
• http
• http-ratelimiting
• lavalink
• mention
• model
• standby
• util
• validate

We would like to make as many functions const as possible, so we deny this lint, but there are
many occurrences of allows due to false positives. I don't know quite how to proceed with this,
suggestions would be appreciated.

clippy::missing_docs_in_private_items

Denied:

• http-ratelimiting
• standby
• validate

As with missing_docs, we should strive for this.

Allowed Clippy Lints

clippy::let_unit_value

Allowed:

• gateway

This lint was added due to a false positive quite a while ago, and never removed. It can now be removed.

clippy::missing_errors_doc

Allowed:

• http

We should remove this lint from the allowed list, because it does ensure good practices.

clippy::module_name_repetitions

Allowed:

• gateway
• http
• http-ratelimiting
• mention
• model
• validate

We disagree with this lint from a design standpoint.

clippy::must_use_candidate

Allowed:

• gateway
• http
• model

I don't know the best way to proceed with this lint. I would like to hear what people have to say. It should be considered alongside clippy::return_self_not_must_use.

clippy::semicolon_if_nothing_returned

Allowed:

• gateway
• http
• http-ratelimiting
• model
• util

I believe we should stop allowing this lint, because it encourages better style.

clippy::unnecessary_wraps

Allowed:

• http
• http-ratelimiting

In cases where this is ignored, it seems to be necessary from a design standpoint, but that can be considered.

clippy::used_underscore_binding

Allowed:

• gateway
• model

Allowing this lint is useful for when tracing is enabled.

Conclusion

I believe that a decent list of lints to deny and allow to start with is as follows:

#![deny(
    clippy::all,
    clippy::missing_const_for_fn,
    clippy::pedantic,
    future_incompatible,
    missing_docs,
    nonstandard_style,
    rust_2018_idioms,
    rustdoc::broken_intra_doc_links,
    unsafe_code,
    unused
)]
#![allow(
    clippy::module_name_repetitions,
    clippy::must_use_candidate,
    clippy::unnecessary_wraps,
    clippy::used_underscore_binding
)]

[Erk-]

Some prior art at EmbarkStudios/rust-ecosystem#59 and EmbarkStudios/rust-ecosystem#75

[PyroTechniac]

What about considering making certain allow-by-default lints, such as missing_copy_implementation warn/error?

[7596ff]

Cases for denying lints can be made in this thread, I suppose. At the moment the purpose of this is to collect information about what we currently have and use that to pare down/make consistent what we have.

[vilgotf]

Late contender. I propose for #![deny(unreachable_pub)] to be added to clear up when pub marked items are part of the public API. Currently pub marked items may be private with their parents being private (or pub(crate), etc…) or public with their parents being public or re-exporting private modules' public items.

[Erk-]

I think it may be a good idea to move some of our lints from deny to warn and then run CI with -Dwarnings lints such as unused variables can be annoying to run into

[vilgotf]

I was thinking just this a couple of days ago. We already run CI with -D warnings so it's a trivial fix. Plus we can then remove all our warn by default overrides, such as rust_2018_idioms

[7596ff]

This seems like a good idea