Sonar-dotnet: Cognitive Complexity score should not penalize with nesting for C# local functions

[rjgotten]

Description

Currently the rule S3776 - Cognitive Complexity of methods should not be too high intentionally penalizes C# local functions with a +1 for nesting.

Repro steps

Create any method of suitable complexity that uses a local function definition.

Expected behavior

Local functions should not be penalized with nesting.
Or atleast the first level of local function should not be penalized.

Actual behavior

Local functions are penalized with nesting.

Known workarounds

Discard cognitive complexity warnings and/or disable the rule altogether.

As far as I'm concerned, not using local functions is not an option and neither is promoting them to private methods, as the entire intent of local functions is to provide a way to break down logic into parts while maintaining a "private even for other private members" scope.

Some of its strongest use cases are in the implementation of inner yield or async behavior and due to this nesting penalty those methods now get an artificially greatly inflated complexity score, to the point of nonsense.

Related information

SonarC# Version : 7.10

[Evangelink]

Hi @rjgotten,

We had a lot of internal conversations about this but we ended up with this choice because when local functions reference some parameters or variables from the outer scope you actually have to keep more things in your mind to understand these local functions.

If you often end up with methods with higher score maybe you should update the threshold to a higher value so that it fits with your architecture.

[rjgotten]

@Evangelink
If you often end up with methods with higher score maybe you should update the threshold to a higher value so that it fits with your architecture.

That's not acceptable for us or probably anyone that intends to use local functions for methods that implement an inner yield or async construct.

You're basically telling me: "Oh, just double the allowed code complexity; it'll be fine."

No.
Doing so will mess with allowed code complexity for methods which do not use local functions and involves a lot of per-project futzing about to find a setting 'which works' for each individual project.
What you're suggesting is simply _completely unmaintainable_.

If that is the final answer, then I'm going to start an internal discussion with our development teams to disable the code complexity rule altogether, or apply a way to dismiss it selectively.

If only to get rid of all the noise it will generate when using local functions as the intended and _proper_ Microsoft-recommended way to implement yield with early exception throwing.

_{(You _did_ know Microsoft actually recommends using them for inner yield straight in the language reference, right? This is not an uncommon occurence.)}

---

Rather than spending a lot of time on internal discussion - or external for that matter... - and conflicting opinions, you _could_ just introduce a switch/setting that allows for all behaviors, You could even make it configurable for users to pick at what nesting level they want local functions to actually start counting towards complexity; 1 level deep, 2 levels deep; etc.

This isn't terribly complicated. You can have your cake and eat it, here.

[Evangelink]

Hi @rjgotten,

I do understand your point and as I said, this went through some internal discussion raising pros and cons.

Regarding the idea of a switch, we avoid adding extra parameters to the rules as we have noticed that most users don't use them. In the end, this creates un-needed maintenance and source of errors.

If you really think this is a MUST HAVE then I encourage you to either create your own rule with the values you want OR to create some post under the suggest new feature page of our community forum => https://community.sonarsource.com/c/suggestions/features

Thank you for your feedback!

[andrei-epure-sonarsource]

Some docs to help create custom rules:

• Importing Issues from Third-Party Roslyn Analyzers
• SDK for SonarQube Roslyn Analyzer Plugins
• our implementation of the CognitiveComplexity rule

[rjgotten]

Afaict the code that actually requires editing is here:

https://github.com/SonarSource/sonar-dotnet/blob/c6abd066b90df6d5b9f2fe484fb21d61ea404a30/sonaranalyzer-dotnet/src/SonarAnalyzer.CSharp/Metrics/CSharpCognitiveComplexityMetric.cs#L47-L57

It's called from aforementioned CognitiveComplexity class's Initialize method, where a few different actions are being hooked in. Changing the logic means calling to a different implementation of CSharpCognitiveComplexityMetric for a method declaration, which from the looks of it means writing a completely new walker, as the current one is a private inner class.

Even if I _could_ build all that (and I probably could) - that means any custom rule here would have to opt in to maintaining parity with the official walker, which is a maintainance burden I'm really not looking forward to.

And honestly: it's a fairly ridiculous proposition that you would opt to not add a switch to the built-in rule here for fear of maintainance problems on your end. If you implement a basic boolean toggle to switch nesting on or off for local functions, then it's literally _just_ one additional check of that toggle property added onto the if on line 49; nothing more.

It's your prerogative to do it, or not do it. Just know that the particular argument you're making there ... kind of rings hollow.

---

That said...
Guess I'll just go and create yet _another_ throwaway account then, so I can ask for this as a suggested new feature. Because god forbid you can make this kind of thing happen without having to sign up for a separate service _somewhere_. 😞