Rust-clippy: Ignore cognitive complexity in macro expanded code

[bodil]

Would it be possible to run the cognitive/cyclomatic complexity lints before macro expansion, or otherwise have them ignore macro output? I've got a number of macros with pretty heavy code generation, and I invariably have to turn off these lints in order to avoid triggering them even for macros that are very simple from the user's perspective.

[oli-obk]

@felix91gr I think we had discussions about this, right? Like moving the cognitive complexity lint to a pre-expansion pass so macro invocations are just as "complex" as a single function call.

[felix91gr]

@oli-obk yes we did indeed :)

@bodil, we're planning on doing a rewrite of the Cognitive Complexity lint soon. We will convert it to a pre-expansion lint, and among other things this is precisely one of the changes we have planned :)

[felix91gr]

I've added this to the list of _(Nesting-Indepentend) Increments_ constructs, right where function calls are: https://github.com/rust-lang/rust-clippy/issues/3793#issuecomment-466160733 :)

[yaahc]

Should this lint be disabled in the interim? tracing in particular is falling prey to it because their macros introduce branches and so if you do a bit of logging in your function suddenly clippy shouts at you.

Also, according to @hawkw log should also run into this issue.

Here's an egregious example of a gist that triggers cognitive complexity

https://gist.github.com/jarrodldavis/c6ba356b4243b898a658e27f9042a483

[felix91gr]

@yaahc maybe. I'm sorry for what's happening. The plan was for the changes to be finished around April or so, but life happened and I'm on a hiatus for the moment.

Since the old version (the one currently on master) is a post-macro-expansion pass, I don't think a quick fix on the old version can help here.

Maybe downgrading the existing one to nursery is an option, given that we'll do that regardless after we merge the new version. We plan on doing so for fine-tuning of the new lint's defaults, and therefore doing so right now would probably be fine, I think?

@centril @oli-obk @manishearth what do you think of that?

[felix91gr]

And fwiw, the short version of why we want to ultimately not check the post-expansion code in the lint, is because the lint checks how hard to grasp is a piece of code. Since macros do introduce abstraction, they are not "weightless". But they are cognitively (we believe) very close to reading a function call.
Therefore we score the pre-expansion code in the new version :)

[yaahc]

That's exactly what I would expect. A macro abstracts code in the exact way a function does, it's only the underlying implementation that is different.

Downgrading to nursery sounds good :+1:

[flip1995]

Therefore we score the pre-expansion code in the new version

We need to think about a case like this though :

some_macro!({
    // Some complex expression
} ) 

Because in a pre expansion pass, we can't access the macro arg expressions, since they're not parsed by that point. Maybe we could first run a pre expansion parse, but don't lint there, but calculate the CoC and mark the spans of the macros that need to be rechecked. After that, we run an early lint pass and recheck these spans. Not sure how to handle macro in macro situations though.

[felix91gr]

If I remember correctly, @flip1995, we are walking inside all expressions, including the ones inside macro invocations. So we should be scoring whatever they have inside.

What we're not scoring, however, is the post-macro-expansion code, which since it's abstracted away while a user is reading the invocation code, it's ignored in the score.

That is however, a good reminder. I will probably remember to double check this before we ship it, thanks :3

[flip1995]

I doubt that. We have a PR open with this exact same problem, where we're kind of stuck, since there is no good solution for this (yet?). See https://github.com/rust-lang/rust-clippy/pull/4186#issuecomment-500225345

So if you found a way to check them, I would be really happy about it, since this would resolve a lot of problems with linting macro code. But make sure to check this. (Maybe I get to write a test for this. Your CoC PR is already a pre-expansion pass, isn't it?)

[felix91gr]

Your CoC PR is already a pre-expansion pass, isn't it?

It is! Do take a look, and feel free to make tests for it. We definitely need more tests before calling it done :3

[Manishearth]

Mostly ambivalent about moving it to nursery.

[phansch]

The lint has been moved to the nursery a while ago. Since the macro handling case is now tracked in #3793, I'm going to go ahead and close this issue.