refactor: check for additional lints #200
Conversation
| "unsafe_op_in_unsafe_fn", | ||
| "invalid_reference_casting", | ||
| "pointer_structural_match", | ||
| "clippy::undocumented_unsafe_blocks", | ||
| "clippy::multiple_unsafe_ops_per_block", | ||
| "clippy::transmute_ptr_to_ptr", | ||
| "clippy::as_ptr_cast_mut", | ||
| "clippy::cast_ptr_alignment", | ||
| "clippy::fn_to_numeric_cast_any", | ||
| "clippy::ptr_cast_constness", |
There was a problem hiding this comment.
We probably always want these enabled when working with unsafe code (crates such as ironrdp-cliprdr-native should enable them with crate-level annotations in order to get direct feedback in editor)
| "unused_tuple_struct_fields", | ||
| "clippy::arithmetic_side_effects", | ||
| "clippy::cast_lossless", | ||
| "clippy::cast_possible_truncation", | ||
| "clippy::cast_possible_wrap", | ||
| "clippy::cast_sign_loss", | ||
| "clippy::float_cmp", | ||
| "clippy::as_underscore", | ||
| // TODO: "clippy::unwrap_used", // let’s either handle `None`, `Err` or use `expect` to give a reason | ||
| "clippy::large_stack_frames", |
There was a problem hiding this comment.
Check for additional potential correctness issues. Legacy code is full of such code so I disabled the check in some crates for now. Ideally, new code should not ignore these warnings.
clippy::arithmetic_side_effects is sometimes a bit too pedantic and we may relax it at some places. Let’s see how it turns out.
| "absolute_paths_not_starting_with_crate", | ||
| "single_use_lifetimes", | ||
| "unreachable_pub", | ||
| "unused_lifetimes", | ||
| "unused_qualifications", | ||
| "keyword_idents", | ||
| "noop_method_call", | ||
| "clippy::semicolon_outside_block", // with semicolon-outside-block-ignore-multiline = true | ||
| "clippy::clone_on_ref_ptr", | ||
| "clippy::cloned_instead_of_copied", | ||
| "clippy::trait_duplication_in_bounds", | ||
| "clippy::type_repetition_in_bounds", | ||
| "clippy::checked_conversions", | ||
| "clippy::get_unwrap", | ||
| // TODO: "clippy::similar_names", // reduce risk of confusing similar names together, and protects against typos when variable shadowing was intended | ||
| "clippy::str_to_string", | ||
| "clippy::string_to_string", | ||
| // TODO: "clippy::std_instead_of_alloc", | ||
| // TODO: "clippy::std_instead_of_core", | ||
| "clippy::unreadable_literal", | ||
| "clippy::separated_literal_suffix", | ||
| "clippy::unused_self", | ||
| // TODO: "clippy::use_self", // NOTE(@CBenoit): not sure about that one | ||
| "clippy::useless_let_if_seq", | ||
| // TODO: "clippy::partial_pub_fields", | ||
| "clippy::string_add", | ||
| "clippy::range_plus_one", | ||
| // TODO: "missing_docs" // NOTE(@CBenoit): we probably want to ensure this in core tier crates only |
There was a problem hiding this comment.
Helps ensuring consistent, straightforward and more readable code style across the codebase
| "unused_crate_dependencies", | ||
| "unused_macro_rules", | ||
| "clippy::inline_always", | ||
| "clippy::or_fun_call", | ||
| "clippy::unnecessary_box_returns", |
There was a problem hiding this comment.
Code which may hurt compile-time or runtime performance and generally easy to fix
| "clippy::print_stderr", | ||
| "clippy::print_stdout", | ||
| "clippy::dbg_macro", |
There was a problem hiding this comment.
Protects us from merging unintended temporary print!/eprint!/dbg! statements that are often artifacts of ad-hoc print statement debugging
| "clippy::str_to_string", | ||
| "clippy::string_to_string", |
There was a problem hiding this comment.
Taking something that is a string and converting it to a string using to_string() kind of misses the point of why we are doing the conversion in the first place, and also fails to document this to the reader. to_owned() fully captures the reason that a conversion is required at a particular spot: borrowed (&str) to owned (String).
| // == Style, readability == // | ||
| "absolute_paths_not_starting_with_crate", | ||
| "single_use_lifetimes", | ||
| "unreachable_pub", |
There was a problem hiding this comment.
Protects us from leaving unreachable pub items. In most cases, it’s either a mistake or an implementation detail which is not intended to be exposed. We should use pub(crate) or no pub at all for implementation details. In some cases it’s actually desirable to make a type pub without it being reachable (effectively preventing the user from naming this type), but it’s rare enough that we can just explicitly add an #[allow] annotation when necessary.
Coverage Report 🤖 ⚙️Past: New: Diff: -0.02% [this comment will be updated automatically] |
CBenoit
force-pushed
the
hardened-lints
branch
2 times, most recently
from
bac1b43 to
21329b6
Compare
CBenoit
force-pushed
the
hardened-lints
branch
from
21329b6 to
0e1b285
Compare
CBenoit
force-pushed
the
hardened-lints
branch
from
0fd14e5 to
27ec5ad
Compare
cc @pacmancoder @ibeckermayer
I realized it was possible to enforce a few guidelines by enabling additional, a bit more pedantic lints (that may be relaxed at some places).
I did not enforce with
#![deny(…)]everywhere so we don’t need to be bothered until final PR clean up: these are checked only when runningcargo xtask check lintsor by the CI.