Improve the cognitive_complexity lint to show what lines are causing the complexity

[iddm]

We could improve the cognitive_complexity lint further by showing the ^ sign under tokens which cause the increment of cognitive complexity. Doing so would let users know what actually was causing the complexity and so they would spend their time more effectively fixing the issue.

fn f(a: u8) {
    if a == 0 {
    } else if a == 3 {
    } else {
    }
}

warning: the function has a cognitive complexity of (3/2)
  --> src/main.rs:12:1
   |
12 | / fn f(a: u8) {
13 | |     if a == 0 {
14 | |     } else if a == 3 {
15 | |     } else {
16 | |     }
17 | | }
   | |_^
   |
   = 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

note: these places cause additional cognitive complexity
   |
1 | /     fn f(a: u8) {
2 | |         if a == 0 {
  | |_________^ increments to 1

   |
1 | /     fn f(a: u8) {
2 | |         if a == 0 {
3 | |         } else if a == 3 {
  | |___________^ increments to 2

   |
1 | /     fn f(a: u8) {
2 | |         if a == 0 {
... |
4 | |         } else {
  | |___________^ increments to 3

Right now, for people unfamiliar with the cognitive complexity, it is impossible to know for sure what is causing problems in their code, they have to study the paper. Also, even people who had it known before the problem appeared, could simply have forgotten it. This explicit show will not only help them understand what causes a problem here but also will teach them.

[flip1995]

cc @felix91gr

That's a good idea IMO! But we have to be careful to not spam the output, so that the actual message can't be found anymore. If we implement this feature, I'd suggest to hide it behind a configuration / attribute, e.g. #[clippy::cognitive_complexity_verbosity = "5"], which then would indicate every code block, that increments the CoC by at least 5. We then could add a note to the CoC lint that tells the user, that this attribute exists. Something like

note: you can set `#[clippy::cognitive_compexity_verbosity = "x"]` to show code blocks with a complexity of `x`

[iddm]

@flip1995 Good point!

I think it is also a good idea to follow the behaviour of RUST_BACKTRACE=full, so that we could not only set the verbosity level to 5 as in your example, but if a user wants, we could show him the full description (for example, the user already knows about the problem and wants to know it in details, so he might have redirected his standard error output to a file for studying it later):

#[clippy::cognitive_complexity_verbosity = "full"]

[iddm]

I have almost finished working on this. So please don't start working on this one.

which then would indicate every code block, that increments the CoC by at least 5

@flip1995 According to the code in the cognitive_complexity.rs file, I don't know how to implement it. I am storing spans now instead of just u64 variables for returns and other parameters. I can only show then first x spans causing complexity up to the specified number. In order to show the code block above, in the context, I must know it, but Span objects do not store any references to the outer context it seems. Do you think it is okay if I'll just show first x spans?

[iddm]

How do I know if some Span is unreachable? For example:

if true && false && true && false && true && false

Every span of && with operands after the first true are unreachable and they're not counted in the current implementation. But I don't see how it is detected in the code.

Now I have this printed instead, what is incorrect:

note: Increases cognitive complexity to 1
  --> $DIR/cognitive_complexity.rs:138:5
   |
LL |     true && false && true && false && true && false && true
   |     ^^^^^^^^^^^^^
note: Increases cognitive complexity to 2
  --> $DIR/cognitive_complexity.rs:138:5
   |
LL |     true && false && true && false && true && false && true
   |     ^^^^^^^^^^^^^^^^^^^^^
note: Increases cognitive complexity to 3
  --> $DIR/cognitive_complexity.rs:138:5
   |
LL |     true && false && true && false && true && false && true
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: Increases cognitive complexity to 4
  --> $DIR/cognitive_complexity.rs:138:5
   |
LL |     true && false && true && false && true && false && true
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: Increases cognitive complexity to 5
  --> $DIR/cognitive_complexity.rs:138:5
   |
LL |     true && false && true && false && true && false && true
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: Increases cognitive complexity to 6
  --> $DIR/cognitive_complexity.rs:138:5
   |
LL |     true && false && true && false && true && false && true
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

What is also questionable is that if we are using some constants there, then the user still has the cognitive complexity of 6, because he doesn't know the values of constants:

IS_X && IS_Y && IS_Z && IS_T && IS_V && IS_W && IS_H

This ^ still needs to have cognitive complexity of 6. However, the code may be unreachable in this case as well.

[flip1995]

Is your implementation based on the implementation on the Clippy master branch or the PR #3963*? Either way I don't really know how the CoC lint works / is implemented.

In #3963, the CoC lint is a visitor implementation, that increases the CoC by visiting expressions (IIUC). We could then add code like this:

cb = coc_before();
walk_expr(expr);
ca = coc_after();
some_structure.store((ca - cb, expr.span));

When the CoC calculation is finished (and the lint is triggered) we can produce notes:

span_lint_and_then(.., |db| {
    for (coc_inc, expr) in some_structure {
        if coc_inc > threshold {
            db.span_note(.., "this code increases the CoC by {}");
        }
    } 
});

Since functions with high CoC, are most of the time large, building some_structure can require a lot of memory and should only be done if a verbosity level is set.

---

As I stated above, I don't really know, if what I suggested is possible or not. @felix91gr can help you here more (in case he got some time on his hands).

* IMO you should implement this based on #3963 and create a PR on the branch of the PR, instead of the master branch. Otherwise, either your work has to be redone completely, once #3963 gets merged or will be discarded in favor of the CoC MVP.

[iddm]

@flip1995

Thank you very much for the information! I'll rebase my PR on top of #3963 one.
Also, in my code (WIP!) I store a tuple of Span and ExprKind in a vector. So you think it requires a lot of memory as well? It was just easy to do :) We could try storing such date only when the verbosity is set, but we don't know in advance, whether the value we are counting exceeds the limit set by the user or not, right?

[flip1995]

We definitely know, if the verbosity is set or not, so we can store every expression or none at all, pretty easily. If this could be implemented, like I suggested above, you can check after visiting an expression if the complexity of the expression exceeds the threshold, by just checking if ca - cb > threshold. If it cannot be implemented like that it would still be awesome to filter them out in another way. Otherwise every expression would get stored (multiple times -> expr in an expr).

[iddm]

We definitely know, if the verbosity is set or not, so we can store every expression or none at all, pretty easily. If this could be implemented, like I suggested above, you can check after visiting an expression if the complexity of the expression exceeds the threshold, by just checking if ca - cb > threshold. If it cannot be implemented like that it would still be awesome to filter them out in another way. Otherwise every expression would get stored (multiple times -> expr in an expr).

Oh, excuse me, was reading between the lines. Yes, sure, we can do this.