Skip to content

Commit c9719f9

Browse files
committed
chore(lib): start a strict clippy config
This starts a change to embrace a strict allowlist with Clippy. ## Why? Clippy can detect a lot of mistakes. The defaults are very good. But it has the power to detect many more _probable_ mistakes, and enforce coding patterns beyond formatting. With the increase in LLM generated code, a stricter Clippy can protect us from the LLM generating poorer code. The pedantic and restriction groups are not enabled by default. There is even a warning to not enable the restriction group blindly. But, even they have good lints. The usual recommendation is to just turn on the lints you care about. Instead, this embraces restricting everything by default, and keep an explicit allowlist. The benefits for this are that we explicitly consider every possible "bad code" lint. We decide if it's something to ignore. And we also don't accidentally not notice a new lint. Every time Clippy upgrades, we may see some new lints that could improve our code. That is excellent! When that happens, we can decide whether to adjust the code, or allow the lint. [More reading](https://billylevin.dev/posts/clippy-config/) ## How? Restricting all these lints in one go would be a large amount of changes. Some of them can be done automatically (`cargo clippy --fix`), some of them an LLM can very easily do, and some require manual inspection in each place. This starts by enabling all the groups, listing out every lint that was triggered, and then allows them explicitly for now. I've split that list into two separate smaller lists (described here in reverse order): - Lints that are expliticly allowed. - Lints that should be decided on, either by fixing the code, or removing the TODO and putting them in the explicitly allowed list (ideally explaining why). Follow up commits can address those lints, and when doing so, update the list. This will prevent rot from occurring by keeping the PR open for a long time, or conflicts.
1 parent 5dbcae7 commit c9719f9

2 files changed

Lines changed: 136 additions & 0 deletions

File tree

.github/workflows/CI.yml

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ jobs:
1717
runs-on: ubuntu-latest
1818
needs:
1919
- style
20+
- lint
2021
- test
2122
- msrv
2223
- miri
@@ -51,6 +52,23 @@ jobs:
5152
exit 1
5253
fi
5354
55+
lint:
56+
name: Linter
57+
#needs: [style]
58+
runs-on: ubuntu-latest
59+
steps:
60+
- name: Checkout
61+
uses: actions/checkout@v6
62+
63+
- name: Install Rust
64+
uses: dtolnay/rust-toolchain@stable
65+
66+
- uses: Swatinem/rust-cache@v2
67+
68+
# not --all-targets, lints can't seem to tell integrations tests are tests
69+
- name: Clippy
70+
run: cargo clippy --features full -- -D warnings
71+
5472
test:
5573
name: Test ${{ matrix.rust }} on ${{ matrix.os }}
5674
needs: [style]

Cargo.toml

Lines changed: 118 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -105,6 +105,124 @@ check-cfg = [
105105
'cfg(hyper_unstable_ffi)'
106106
]
107107

108+
[lints.clippy]
109+
pedantic = { level = "warn", priority = -1 }
110+
restriction = { level = "warn", priority = -2 }
111+
112+
# keep an allow list of lints
113+
114+
# lints to decide on
115+
116+
arithmetic_side_effects = "allow" # TODO: consider
117+
as_conversions = "allow" # TODO: tricky
118+
borrow_as_ptr = "allow"
119+
cast_lossless = "allow" # TODO: easy fix
120+
cast_possible_truncation = "allow" # TODO: consider
121+
cast_precision_loss = "allow" # TODO: consider
122+
checked_conversions = "allow"
123+
collapsible_match = "allow"
124+
decimal_literal_representation = "allow" # TODO: consider
125+
default_trait_access = "allow"
126+
else_if_without_else = "allow"
127+
empty_structs_with_brackets = "allow" # TODO: easy fix
128+
enum_glob_use = "allow"
129+
explicit_iter_loop = "allow" # TODO: easy fix
130+
float_arithmetic = "allow"
131+
ignored_unit_patterns = "allow"
132+
indexing_slicing = "allow"
133+
integer_division = "allow"
134+
integer_division_remainder_used = "allow"
135+
large_enum_variant = "allow"
136+
let_unit_value = "allow"
137+
manual_assert = "allow" # TODO: easy fix
138+
manual_assert_eq = "allow" # TODO: easy fix
139+
map_err_ignore = "allow"
140+
map_unwrap_or = "allow"
141+
match_wild_err_arm = "allow"
142+
missing_fields_in_debug = "allow" # TODO: use finish_non_exhaustive
143+
missing_errors_doc = "allow" # TODO: good to fix
144+
missing_panics_doc = "allow" # TODO: might be false
145+
multiple_inherent_impl = "allow"
146+
multiple_unsafe_ops_per_block = "allow"
147+
needless_continue = "allow"
148+
needless_pass_by_value = "allow"
149+
panic = "allow"
150+
pattern_type_mismatch = "allow"
151+
ptr_as_ptr = "allow"
152+
question_mark = "allow" # TODO: probably easy fix
153+
redundant_closure_for_method_calls = "allow"
154+
redundant_else = "allow"
155+
ref_option = "allow"
156+
ref_patterns = "allow" # TODO: perhaps deny?
157+
semicolon_if_nothing_returned = "allow" # TODO: easy fix
158+
single_char_lifetime_names = "allow"
159+
single_match_else = "allow" # TODO: easy fix
160+
struct_excessive_bools = "allow" # TODO: bogus lint?
161+
trivially_copy_pass_by_ref = "allow"
162+
undocumented_unsafe_blocks = "allow" # TODO: fix me
163+
uninlined_format_args = "allow" # TODO: easy fix
164+
unnecessary_semicolon = "allow" # TODO: easy fix
165+
unnecessary_trailing_comma = "allow"
166+
unnested_or_patterns = "allow"
167+
unused_async = "allow" # TODO: is it for API?
168+
unused_trait_names = "allow" # TODO: kinda annoying, but might be good to deny
169+
unwrap_in_result = "allow"
170+
useless_borrows_in_formatting = "allow"
171+
wildcard_enum_match_arm = "allow"
172+
wildcard_imports = "allow" # TODO: never, except for tests
173+
174+
# explicitly allowed
175+
absolute_paths = "allow" # sometimes its cleaner
176+
arbitrary_source_item_ordering = "allow" # order: std, deps, crate
177+
blanket_clippy_restriction_lints = "allow" # allowlist is better
178+
clone_on_ref_ptr = "allow" # Arc::clone(blah) is needlessly noisy
179+
cognitive_complexity = "allow" # is this ever useful?
180+
default_numeric_fallback = "allow" # too many false positives
181+
expect_used = "allow" # expect is self-documenting
182+
error_impl_error = "allow" # mod::Error is a fine name
183+
field_scoped_visibility_modifiers = "allow" # possibly good idea, noisy for now
184+
if_not_else = "allow" # order depends on which is more common, not truthiness
185+
if_then_some_else_none = "allow" # control flow as if better than closures
186+
items_after_statements = "allow" # these can be useful
187+
implicit_return = "allow" # standard rust
188+
impl_trait_in_params = "allow" # turbofish ignored on purpose
189+
inline_modules = "allow" # common for sealed types
190+
inline_trait_bounds = "allow" # more concise if shorter bounds
191+
let_underscore_must_use = "allow" # the entire point was to ignore must_use
192+
let_underscore_untyped = "allow"
193+
mod_module_files = "allow" # easier to find than self-named modules
194+
module_inception = "allow" # sometimes that happens
195+
min_ident_chars = "allow" # not to be abused, nor forced
196+
missing_assert_message = "allow" # not much value
197+
missing_docs_in_private_items = "allow" # these docs aren't rendered
198+
missing_inline_in_public_items = "allow" # bad lint
199+
missing_trait_methods = "allow"
200+
must_use_candidate = "allow" # bad lint
201+
module_name_repetitions = "allow" # sometimes it happens, not bad at all
202+
new_without_default = "allow" # not everything needs a Default impl
203+
panic_in_result_fn = "allow" # panics are for invariants
204+
pub_use = "allow"
205+
pub_with_shorthand = "allow"
206+
question_mark_used = "allow" # question mark is excellent
207+
unreachable = "allow" # self-documenting invariants
208+
renamed_function_params = "allow" # we can pick clearer names
209+
same_name_method = "allow"
210+
shadow_reuse = "allow" # shadowing is useful
211+
shadow_same = "allow" # shadowing is useful
212+
shadow_unrelated = "allow" # shadowing is useful
213+
single_call_fn = "allow" # abstracting to a function is the point
214+
self_named_module_files = "deny" # already denied but, rly, dont do it
215+
semicolon_inside_block ="allow" # depends on context
216+
semicolon_outside_block ="allow" # depends on context
217+
std_instead_of_alloc = "allow" # std is more idiomatic
218+
std_instead_of_core = "allow" # std is more idiomatic
219+
struct_field_names = "allow" # not really helpful
220+
too_many_lines = "allow" # reconsider someday?
221+
type_complexity = "allow" # not helpful
222+
used_underscore_items = "allow"
223+
unnecessary_safety_comment = "allow" # more safety comments are a good thing
224+
unseparated_literal_suffix = "allow" # i don't like 0_u8 (do i?)
225+
108226
[package.metadata.docs.rs]
109227
features = ["ffi", "full", "tracing"]
110228
rustdoc-args = ["--cfg", "hyper_unstable_ffi", "--cfg", "hyper_unstable_tracing"]

0 commit comments

Comments
 (0)