Activate more lints #3394

theodorebje · 2025-07-02T14:12:41Z

Motivation

Draft PR.
Earlier discussion: #3393

The purpose of this PR is to activate and fix more lints for the codebase. This will improve codebase readability and maintainability. The ultimate goal is to have as many lints from the pedantic and nursery lint groups as possible. For now, only a subset of lints are enabled.

Discussions welcome.

Solution

New lints:

I will update this list once I activate more lints.

Sub-PRs

Add `#[must_use]` attribute to functions where the attribute could be added. This fixes the `must_use_candidate` clippy lint.

In `impl`s, `Self` can be used instead of the struct/enum being implemented, to reduce unnecessary repetition. This commit uses `Self` as often as possible. This fixes the `use_self` clippy lint.

The `request_parts` and `request` modules were marked as `pub(crate)`, which is misleading since the public parts are re-exported by `lib.rs` anyway. Attempting to mark them as `pub`, like suggested by the `redundant_pub_crate` clippy lint causes another Rust-native lint to appear: `unreachable-pub`. To avoid both of these lints, it's best to simply mark the whole module as private, and re-export the public traits separately. Since these public traits are re-exported by `lib.rs` anyway, this does not affect the public API.

The `else` block adds unnecessary indentation and verbosity. To avoid this, we can simply move `return None` statement outside the `if` statement. This fixes the `redundant_else` clippy lint.

Semicolons at the end of if statements evaluating to `()` are unnecessary and may be removed to avoid confusion and visual clutter. This fixes the `unnecessary_semicolon` clippy lint.

`let...else` provides a standard construct for this same pattern that people can easily recognize (and is IMO much less confusing). It's also more compact. This fixes the `manual_let_else` clippy lint.

`if let Some(v) = ... { y } else { x }` and `match .. { Some(v) => y, None/_ => x }` are more idiomatically done with the `Option::map_or*` methods. Using the dedicated methods results in both clearer and more concise code than using an `if let` expression. This fixes the `option_if_let_else` clippy lint. NOTE: Clippy's suggested method for `try_downcast` couldn't compile properly, so I rewrote the `if let` statement slightly using `Option::take`.

Some functions and methods aren't marked as `const`, even though they could be. Not marking them as `const` prevents callers of the function/method from being `const` as well. This fixes the `missing_const_for_fn` clippy lint. I also marked some methods and functions as `const` that clippy didn't find. NOTE: Removing the `const` marker again from public functions/methods in the future will break API compatibility. NOTE-2: I marked some of the `status` methods as const. I don't know how axum's internals work very well, so it's possible that I may have broken something, but all tests are passing sooo...

Seems to not have been found by clippy on my machine. This fixes the `manual_let_else` clippy lint. See-also: ddf5830

Forgot to run `cargo-fmt` during a pervious commit. See-also: d8b6ef2

`if !..else` is a confusing syntax, as it does the inverse of what the rest of the lines says. By reordering the `if` statement we can make the `if` statement more clear. This fixes the `if_not_else` clippy lint.

The `.map(_).unwrap_or*(_)` methods are inconcise and hard-to-read. This commit replaces them with the dedicated `map_or*` methods instead. This fixes the `map_unwrap_or` clippy lint. NOTE: This makes it harder to search through the entire codebase for `unwrap`s.

Although these semicolons are optional, they aren't optional when extending the block with new code. To avoid having to change the previous last lines when adding more code to these functions, we should add the optional semicolon. This fixes the `semicolon_if_nothing_returned` clippy lint. NOTE: I `#[allow]`ed the `forever` function. Adding a semicolon at the end of `forever` causes a type inference error (E0282).

`assert!` is much more concise and readable than manually triggering a panic via an `if` statement. Once I did this, clippy also found a couple of conditions that could be simplified. I applied those simplifications. This fixes the `manual_assert` clippy lint.

Using `()` makes it clearer to both the reader and the compiler that the pattern contains no data. It will also trigger a compilation error if the type changes, which makes the pattern-matching more robust, safe, and less prone to uncaught logic errors. This fixes the `ignored_unit_patterns` clippy lint.

Wildcard imports can pollute the namespace, and can lead to confusing error messages or even unexpected behaviour. This commit uses explicit imports instead. Although slightly more verbose, it can save a lot of debugging efforts in the future. Exceptions to this rule are prelude imports, imports of `pub use` `re-exports`, and `super::*` imports in tests. This fixes the `wildcard_imports` clippy lint.

mladedav · 2025-07-02T16:02:13Z

Only None. Backticks are for code. The lint description also mentions that.

Methods returning `Self` often create new instances of a type without causing any side-effects (such as interacting with other types). Unlike for ede4d85, it is almost guaranteed that the user intended to use the newly created value, and simply just forgot. This fixes the `return_self_not_must_use` clippy lint. See-also: ede4d85

theodorebje · 2025-07-02T16:16:42Z

Updated list of non-activated lints:

43 redundant_pub_crate
28 needless_pass_by_value
24 missing_errors_doc
16 similar_names
12 missing_panics_doc
09 redundant_closure_for_method_calls
08 default_trait_access
07 doc_markdown
07 doc_link_with_quotes
06 trivially_copy_pass_by_ref
05 too_many_lines
05 ref_option
04 used_underscore_binding
04 match_same_arms
03 no_effect_underscore_binding
03 missing_fields_in_debug
03 items_after_statements
03 cast_possible_truncation
02 single_match_else
02 redundant_clone
02 mismatching_type_param_order
02 implicit_clone
02 format_push_string
02 explicit_iter_loop
02 equatable_if_let
01 unused_peekable
01 unused_async
01 needless_pass_by_ref_mut
01 future_not_send
01 derive_partial_eq_without_eq
01 debug_assert_with_mut_call

- Add backticks to the documentation for `Utf8Bytes` - Wrap the example of a `range` in a code block - [Exclude `WebSocket`](tokio-rs#3394 (comment)) from being seen as an identifier by clippy. This fixes the `doc_markdown` clippy lint.

f978792 caused a compilation warning to appear when compiling without the `http2` feature flag. This commit fixes that by only importing `InvalidProtocolPseudoheader` when the `http2` feature is enabled.

axum-core/src/body.rs

axum-extra/src/extract/cookie/mod.rs

axum-extra/src/response/multiple.rs

axum/src/test_helpers/test_client.rs

Cargo.toml

jplatte · 2025-07-02T18:01:19Z

axum-macros/src/debug_handler.rs

            });
-        } else {
-            return None;
        }
+        return None;


Could also change the whole if-else to something like

return (types_that_consume_the_request[0].0 != number_of_inputs - 1).then(|| { // construct the compile_error });

Wouldn't that cause the if statement at the bottom of the function to become dead code? Should I refactor how the construction of the compile_error is done?

if types_that_consume_the_request.len() == 2 { /* compile error */ } else { /* compile error */ }

Taken from here.

jplatte · 2025-07-02T18:04:35Z

This PR is pretty large. Can you maybe split it up? Doesn't have to be one PR for every lint, but ones that lead to lots of changes like the must_use one seem worth splitting.

mladedav

The stylistic opinions I wrote are just mine and other people may view things differently, but I kind of don't agree with some of the lints. That said if other people prefer e.g. Self over typenames in all permissible contexts, I can live with that (as long as clippy reminds me to use that).

In any case, thank you for trying to improve the codebase.

mladedav · 2025-07-02T16:28:12Z

axum-core/src/error.rs

+    #[must_use]
    pub fn into_inner(self) -> BoxError {


For errors (and possibly other cases such as builders) we might want to add #[must_use] to the type rather than methods.

I'm not sure how to avoid #[must_use] for the methods here, since BoxError is a type alias and not a "real" type. Adding #[must_use] to the Error struct doesn't do anything, either. Any ideas?

axum-core/src/extract/option.rs

mladedav · 2025-07-02T18:09:39Z

axum-macros/src/debug_handler.rs

+                || {
+                    let check_inputs_impls_from_request =
+                        check_inputs_impls_from_request(&item_fn, state_ty, kind);
+
+                    quote! {
+                        #check_inputs_impls_from_request
+                        #check_future_send
+                    }
+                },
+                |check_input_order| {
+                    quote! {
+                        #check_input_order
+                        #check_future_send
+                    }
+                },
+            )


I'm honestly not a fan of this either. This is nice when it's just one-liners (or at least the default branch is a one-liner), but these bigger blocks are just more indented versions of the original, effectively with else branch first and after that the if branch and with worse readability as to where does it end and that it is an if...else (instead of keyword just a block ends and then another closure is provided).

mladedav · 2025-07-02T19:10:31Z

axum-extra/src/routing/mod.rs

+        !path.is_empty(),
+        "Paths must start with a `/`. Use \"/\" for root routes"
+    );
+    assert!(path.as_bytes()[0] == b'/', "Paths must start with /");


You can use assert_eq for this. There is another case like this in this file.

I think there might also be a lint for this.

I don't think we should, in this case. We don't really want to be printing the first byte of the path in case the assertion fails, we just want the message.

mladedav · 2025-07-02T19:37:05Z

axum-extra/src/response/file_stream.rs

        self.content_size = Some(len);
        self
    }

    /// Return a range response.
    ///
+    /// ```rust


I think you need to add no_run here so that it's not considered as a doctest.

But regardless, it's not valid rust either, so I'd probably mark it as such (maybe text instead of rust? Not sure).

What the heck even is this line?

mladedav · 2025-07-02T19:40:14Z

axum-core/src/macros.rs

@@ -54,7 +54,7 @@ macro_rules! __define_rejection {
            }

            /// Get the status code used for this rejection.
-            pub fn status(&self) -> http::StatusCode {
+            pub const fn status(&self) -> http::StatusCode {


I don't think most of these really make sense. I don't think anyone would build a rejection in a const context to then call this.

I did not go through all of this commit though, there might be some useful ones here, but this lint might also be one to split out of this PR.

mladedav · 2025-07-02T19:54:01Z

From the list, one thing I think we really want is the future_not_send lint. It's just horrible to suddenly find out I can't use something because I'm in a future to be tokio::spawned so I'd rather prevent that.

theodorebje · 2025-07-02T19:57:44Z

Thanks for the constructive feedback, everyone! I'll look through it all tomorrow.

theodorebje · 2025-07-03T10:43:24Z

From the list, one thing I think we really want is the future_not_send lint.
@mladedav The problematic line for that one seems to be:

pub async fn from_path(path: impl AsRef<Path>) -> io::Result<FileStream<ReaderStream<File>>> { … }

(here). AsRef doesn't implement Send.

I don't have that deep knowledge about the codebase yet, so before I edit that line to fix the lint warning, what do you think would be the best type to take?

mladedav · 2025-07-03T13:29:43Z

You can just change it to path: impl AsRef<Path> + Sync.

I wouldn't expect anyone to use anything that isn't Sync there so I wouldn't expect to break someone with this, but it is a breaking change regardless. The good news is that since people wouldn't use !Sync types as parameters here, it's not really a problem the method can theoretically be !Send.

So if you have more breaking changes in a separate PR, you can put that there.

theodorebje · 2025-07-03T13:43:58Z

Thanks! Since this PR already includes breaking-ish changes, and I’m trying to group the (big) commits by function, I think I'll include this one-line fix in the main draft PR instead, if that's OK for you?

mladedav · 2025-07-03T13:52:24Z

Sure, feel free to put it here.

Now that I look at it closer, I think it should also include + Send but clippy doesn't seem to mind when that's missing.

theodorebje · 2025-07-03T17:53:04Z

Regarding redundant_pub_crate, it seems to be a bunch of false positives by Clippy: rust-lang/rust-clippy#5369.

jplatte · 2025-07-04T21:47:11Z

option_if_let_else, map_unwrap_or: these rewrite code to use map_or / map_or_else. Unlike David, I don't just dislike these methods when one or both of the arguments is a big closure. I dislike these methods in general. I'd much rather go the opposite direction and disallow both 😄

missing_const_for_fn: let's add const where people have an actual use case for it, not in every place possible up-front

ignored_unit_patterns: just looks weird and I don't think there's a big advantage

theodorebje · 2025-07-05T18:32:34Z

@jplatte Do you also dislike manual_let_else (ddf5830) or only the ones you mentioned above?

jplatte · 2025-07-05T19:22:11Z

Only the ones mentioned.

Theodore Bjernhed added 8 commits July 2, 2025 14:16

fix: add #[must_use] to relevant functions

ede4d85

Add `#[must_use]` attribute to functions where the attribute could be added. This fixes the `must_use_candidate` clippy lint.

style: use Self to avoid unnecessary repetition

8a561f3

In `impl`s, `Self` can be used instead of the struct/enum being implemented, to reduce unnecessary repetition. This commit uses `Self` as often as possible. This fixes the `use_self` clippy lint.

style: remove redundant else block

3b62dfd

The `else` block adds unnecessary indentation and verbosity. To avoid this, we can simply move `return None` statement outside the `if` statement. This fixes the `redundant_else` clippy lint.

style: remove unnecessary semicolon

a0f8a48

Semicolons at the end of if statements evaluating to `()` are unnecessary and may be removed to avoid confusion and visual clutter. This fixes the `unnecessary_semicolon` clippy lint.

style: use standard let..else construct

ddf5830

`let...else` provides a standard construct for this same pattern that people can easily recognize (and is IMO much less confusing). It's also more compact. This fixes the `manual_let_else` clippy lint.

theodorebje changed the title ~~Clippy pedantic~~ Activate more lints Jul 2, 2025

Theodore Bjernhed added 2 commits July 2, 2025 16:15

style: use let..else for router downcast

869e219

Seems to not have been found by clippy on my machine. This fixes the `manual_let_else` clippy lint. See-also: ddf5830

style(fmt): run cargo fmt

433b705

Forgot to run `cargo-fmt` during a pervious commit. See-also: d8b6ef2

This comment was marked as outdated.

Sign in to view

Theodore Bjernhed added 5 commits July 2, 2025 16:46

style: remove unnecessary if negation

d8389ac

`if !..else` is a confusing syntax, as it does the inverse of what the rest of the lines says. By reordering the `if` statement we can make the `if` statement more clear. This fixes the `if_not_else` clippy lint.

This comment was marked as resolved.

Sign in to view

Theodore Bjernhed added 2 commits July 2, 2025 18:27

docs: use markdown for code and types

0365e0b

- Add backticks to the documentation for `Utf8Bytes` - Wrap the example of a `range` in a code block - [Exclude `WebSocket`](tokio-rs#3394 (comment)) from being seen as an identifier by clippy. This fixes the `doc_markdown` clippy lint.

fix: respect feature flags in the imports

5990804

f978792 caused a compilation warning to appear when compiling without the `http2` feature flag. This commit fixes that by only importing `InvalidProtocolPseudoheader` when the `http2` feature is enabled.

jplatte reviewed Jul 2, 2025

View reviewed changes

mladedav reviewed Jul 2, 2025

View reviewed changes

This was referenced Jul 3, 2025

Add #[must_use] to types and methods #3395

Merged

style: use Self to avoid unnecessary repetition #3396

Merged

jplatte mentioned this pull request Jul 4, 2025

Enable more clippy lints ruma/ruma#2143

Merged

This was referenced Jul 7, 2025

Simplify if statements #3399

Merged

Improve value and reference passing efficiency #3406

Merged

Declutter statements #3408

Open

Activate more lints #3394

Are you sure you want to change the base?

Activate more lints #3394

Uh oh!

Conversation

theodorebje commented Jul 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Solution

Sub-PRs

Uh oh!

This comment was marked as outdated.

This comment was marked as resolved.

mladedav commented Jul 2, 2025

Uh oh!

theodorebje commented Jul 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jplatte commented Jul 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mladedav left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mladedav Jul 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mladedav Jul 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mladedav commented Jul 2, 2025

Uh oh!

theodorebje commented Jul 2, 2025

Uh oh!

theodorebje commented Jul 3, 2025

Uh oh!

mladedav commented Jul 3, 2025

Uh oh!

theodorebje commented Jul 3, 2025

Uh oh!

mladedav commented Jul 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theodorebje commented Jul 3, 2025

Uh oh!

jplatte commented Jul 4, 2025

Uh oh!

theodorebje commented Jul 5, 2025

Uh oh!

jplatte commented Jul 5, 2025

Uh oh!

Uh oh!

theodorebje commented Jul 2, 2025 •

edited

Loading

theodorebje commented Jul 2, 2025 •

edited

Loading

jplatte commented Jul 2, 2025 •

edited

Loading

mladedav left a comment •

edited

Loading

mladedav Jul 2, 2025 •

edited

Loading

mladedav Jul 2, 2025 •

edited

Loading

mladedav commented Jul 3, 2025 •

edited

Loading