Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clippy cognitive complexity false positive #553

Closed
tcharding opened this issue Jan 29, 2020 · 5 comments
Closed

Clippy cognitive complexity false positive #553

tcharding opened this issue Jan 29, 2020 · 5 comments
Labels
closed/invalid This doesn't seem right

Comments

@tcharding
Copy link

Bug Report

Version

tracing = "0.1"
cargo 1.40.0 (bc8e4c8be 2019-11-22)
clippy 0.0.212 (c8e3cfb 2019-10-28)

Platform

Linux ares 5.4.14-050414-generic #202001230832 SMP Thu Jan 23 08:34:56 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux

Description

When using tracing library to create an event in the match arms of an enum with six or more variants the Clippy 'cognitive complexity' warning is triggered.

Here is a minimal reproducer

fn main() {
    println!("Clippy cognitive complexity repro");

    fun(Run::There);
}

fn fun(r: Run) {
    match r {
        Run::Here => tracing::error!("error"),
        Run::There => tracing::error!("error"),
        Run::Everywhere => tracing::error!("error"),
        Run::Makes => tracing::error!("error"),
        Run::Clippy => tracing::error!("error"),
        Run::Choke => tracing::error!("error"),
    }
}

#[allow(dead_code)]
enum Run {
    Here,
    There,
    Everywhere,
    Makes,
    Clippy,
    Choke,
}

And Clippy gives the following warning:

cargo clippy
warning: the function has a cognitive complexity of (26/25)
  --> src/main.rs:7:1
   |
7  | / fn fun(r: Run) {
8  | |     match r {
9  | |         Run::Here => tracing::error!("error"),
10 | |         Run::There => tracing::error!("error"),
...  |
15 | |     }
16 | | }
   | |_^
   |
   = note: `#[warn(clippy::cognitive_complexity)]` on by default
   = help: you could split it up into multiple smaller functions
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#cognitive_complexity

Removing the final variant (Choke) removes the warning.

tcharding pushed a commit to comit-network/comit-rs that referenced this issue Jan 29, 2020
Tracing library at some times causes Clippy to emit a cognitive complexity
warning.  Issue raised on the tokio-rs/tracing repo

	tokio-rs/tracing#553

Add Clippy attributes to allow cognitive complexity where needed.
@hawkw
Copy link
Member

hawkw commented Jan 31, 2020

Yeah, this has been brought up in the past. Apparently this lint is run after macro expansion, rather than before, so the code generated by tracing's macros is counted against the cognitive complexity limit. FWIW, this will also trigger if you use a lot of log macros, but it takes fewer tracing macros than log macros, as we generate more nested ifs (due to some of our internal performance optimizations).

Unfortunately, my understanding is that there isn't really anything we can do to fix this in tracing — I don't believe generating an allow attribute inside the tracing macro expansion would help, since I think it would need to be applied to the function containing the tracing macros. There's an upstream issue against clippy for making this lint treat macro invocations like function calls, so they're not counted against cognitive complexity: rust-lang/rust-clippy#3900

At one point, I thought the clippy maintainers turned this lint off by default due to the number of false positives it generates, but I think a recent clippy release may have turned it back on again?

@hawkw
Copy link
Member

hawkw commented Jan 31, 2020

cc @yaahc

@yaahc
Copy link
Collaborator

yaahc commented Jan 31, 2020

It looks like they were about to but then the issue got sidelined half a year ago and never got pushed across the finish line. I'll ping them and see where its at and if they're planning on working on it soon or if it would be possible for me to take over once I've finished with the cargo clippy --fix stuff.

@hawkw
Copy link
Member

hawkw commented Jan 31, 2020

Thanks for taking a look! It would be great if this could be fixed upstream.

@hawkw hawkw added the closed/invalid This doesn't seem right label Feb 5, 2020
@hawkw
Copy link
Member

hawkw commented Feb 5, 2020

Closing this, as it's an upstream problem we can't really solve.

@hawkw hawkw closed this as completed Feb 5, 2020
bors bot added a commit to comit-network/comit-rs that referenced this issue Apr 24, 2020
2519: Fix comment typo - no trippers around here please r=mergify[bot] a=tcharding

Fix amusing comment typo
```
-// tracing trippers clippy warning, issue reported: tokio-rs/tracing#553
+// tracing triggers clippy warning, issue reported: tokio-rs/tracing#553
```

Co-authored-by: Tobin C. Harding <tobin.harding@coblox.tech>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed/invalid This doesn't seem right
Projects
None yet
Development

No branches or pull requests

3 participants