[clang-tidy] Add options to describe individual core increments to readability-function-cognitive-complexity check.

[massberg]

Summary

Often you are only interested in the overall cognitive complexity of a
function and not every individual increment. Thus the flag
'DescribeBasicIncrements' is added. If it is set to 'true', each increment
is flagged. Otherwise, only the complexity of function with complexity
of at least the threshold are flagged.

By default 'DescribeBasisIncrements' is set to 'true', which is the original behavior of the check.

Added a new test for different flag combinations.

(The option to ignore macros which was original part of this patch will be added in another path)

[alexfh]

clang-tools-extra/clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp

227

Is a default base class constructor entry necessary in the class initializer list?

clang-tools-extra/docs/clang-tidy/checks/readability-function-cognitive-complexity.rst

22

What does "flag every basic increment" mean? Can you explain in more detail and maybe add an example?

[massberg]

Clarified option FlagBasicIncrements and removed base default constructor from class initializer list of FunctionASTVisitor.

[massberg]

Thanks for the comments!

clang-tools-extra/clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp

227

You are right, that wasn't needed.

clang-tools-extra/docs/clang-tidy/checks/readability-function-cognitive-complexity.rst

22

I hope the new description explains now the flag sufficiently.

[njames93]

How does this handle a macro where the argument has complex code.

MACRO(if (...) {});

In this case the macro code is definitely increasing the complexity.

[massberg]

Add test cases showing that when IgnoreMacros is set to 'true', also macro arguments are ignored.

[massberg]

In D96281#2556586, @njames93 wrote:

How does this handle a macro where the argument has complex code.

MACRO(if (...) {});

I agree that macro arguments should account to the complexity.

However, with this change macro arguments are ignored.
Unfortunately, I do not see an easy way how to ignore the macro code, but consider the arguments during analysis.

Optimally, macro calls should be considered like function calls in code. Consider the following example:

#define noop

#define SomeMacro(x)  \
  if (1) {            \
    x                 \
  }

void f() {
  SomeMacro(if (1) { noop; })
}

With IgnoreMacros='false' the check gives the function f() a complexity of 3, while with IgnoreMacro='true' the check gives it at complexity of 0.
IMO the complexity should be 1 (and maybe the macro definition itself should be flagged to have a complexity of 1).

So there is still room for improvements. However, from the code that I have seen it is more likely that there is complexity in the macros itself and not in their arguments when being used.
So I would tend to use the option IgnoreMacros='true'. However, as this isn't perfect and others might not agree with it I decided to add the option and also default it to
the old implementation.

I have added test cases that show this problem of ignoring macro arguments and added a comment to the documentation of the option.

[alexfh]

clang-tools-extra/clang-tidy/readability/FunctionCognitiveComplexityCheck.h

24–26

Thanks! Now the description contains enough information, but it was hard for me to understand it. I'd suggest to reword this a bit, for example: "if set to true, then for each function exceeding the complexity threshold the check will issue additional diagnostics on every piece of code (loop, if statement, etc.) which contributes to that complexity."

As mentioned earlier, an example may help understand the purpose of this option even better.

27

Trailing period is missing.

[lebedev.ri]

It would be better to split this into two parts.

clang-tools-extra/clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp

542

Drop {}

clang-tools-extra/docs/clang-tidy/checks/readability-function-cognitive-complexity.rst

20

FlagBasicIncrements is misleading, this should be DescribeBasicIncrements.

[massberg]

Change option name, improve description and minor fixes.

• Change name FlagBasicIncrements to DescribeBasicIncrements.
• Improve description of this option and added an example.
• Minor code fixes.

[massberg]

Thanks for the comments!

clang-tools-extra/clang-tidy/readability/FunctionCognitiveComplexityCheck.h

24–26

Thanks for the suggested rewording!
I took you suggestion and added a comment to the existing examples below which hopefully explains the option now.

clang-tools-extra/docs/clang-tidy/checks/readability-function-cognitive-complexity.rst

20

Thanks, I renamed it as proposed.

[lebedev.ri]

Once again, can we please split this into two patches?
I'm quite happy with the DescribeBasicIncrements stuff, that can be merged right now.

But i'm not quite sold on the macro stuff. We don't have that for readability-function-size,
nor does the CC spec say anything about macros, nor do other implementations allow to discount them.
To take it to extreme, one potentially could wrap the entire function into a macro and instantiate it once,
and end up with a zero complexity, even though it's obviously not so..

[• vgeof]

I just wanted to upvote the Macro parameter addition !
In my team, we use macros for logging purpose. We don't have access to their implementations, and they all have (sadly) a high complexity score.
So we write a lot of functions like this one, which in my opinion *should* have a low complexity:

bool doStuff(myClass* objectPtr){
    if(objectPtr == nullptr){
        LOG_WARNING("empty object");
        return false;
    }
    if(objectPtr->getAttribute() == nullptr){
        LOG_WARNING("empty object");
        return false;
    }
    use(objectPtr->getAttribute());
}

and clang-tidy report all of our functions because LOG_WARNING has a complexity of 12 in itself, so I had to disable the whole checker.
If I could disable macros accounting in the complexity score, I could enable the checker again(which I find awesome by the way) !

[massberg]

Remove IgnoreMacros option (to be added in a follow up patch) and sync to head.

[massberg]

In D96281#2596657, @lebedev.ri wrote:

Once again, can we please split this into two patches?
I'm quite happy with the DescribeBasicIncrements stuff, that can be merged right now.

I have removed the IgnoreMacros and will add it in an independent patch.

[lebedev.ri]

Does the test start to fail if you undo the .cpp change?
You may want to add some // CHECK-NOTES-NOT: warning

[lebedev.ri]

In D96281#2599812, @lebedev.ri wrote:

Does the test start to fail if you undo the .cpp change?
You may want to add some // CHECK-NOTES-NOT: warning

Actually, i guess it should fail then, lgtm.
If you need help committing this, please specify name <[e-mail]> to be used for git commit author field.

[massberg]

Add checks to ensure that notes aren't added for specific options.

[lebedev.ri]

clang-tools-extra/test/clang-tidy/checkers/readability-function-cognitive-complexity-flags.cpp

16

Note that i'm not sure if we need negative checklines, but if we do, only checking for warning:
(without @LINE stuff) should be best.

[massberg]

Remove unnecessary checks.

After testing locally it turned out that we do not need these checks.
If there is an unexpected note, the test fails.

[massberg]

clang-tools-extra/test/clang-tidy/checkers/readability-function-cognitive-complexity-flags.cpp

16

We do not need them, so I have removed them.
If there are unexpected notes, the test fails.

[lebedev.ri]

In D96281#2599951, @lebedev.ri wrote:

If you need help committing this, please specify name <[e-mail]> to be used for git commit author field.

clang-tools-extra/test/clang-tidy/checkers/readability-function-cognitive-complexity-flags.cpp

16

Great, thanks for checking!

clang-tools-extra/clang-tidy/readability/FunctionCognitiveComplexityCheck.h

 #include "../ClangTidyCheck.h"
 
 namespace clang {
 namespace tidy {
 namespace readability {
 
 /// Checks function Cognitive Complexity metric.
 ///
-/// There is only one configuration option:
+/// There are the following configuration option:
 ///
 ///   * `Threshold` - flag functions with Cognitive Complexity exceeding
 ///     this number. The default is `25`.
+///   * `DescribeBasicIncrements`- if set to `true`, then for each function
+///     exceeding the complexity threshold the check will issue additional
+///     diagnostics on every piece of code (loop, `if` statement, etc.) which
+///     contributes to that complexity.
+//      Default is `true`
 ///
 /// For the user-facing documentation see:
 /// http://clang.llvm.org/extra/clang-tidy/checks/readability-function-cognitive-complexity.html
 class FunctionCognitiveComplexityCheck : public ClangTidyCheck {
 public:
   FunctionCognitiveComplexityCheck(StringRef Name, ClangTidyContext *Context);
 
   void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
   llvm::Optional<TraversalKind> getCheckTraversalKind() const override {
     return TK_IgnoreUnlessSpelledInSource;
   }
 
 private:
   const unsigned Threshold;
+  const bool DescribeBasicIncrements;
 };
 
 } // namespace readability
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_FUNCTIONCOGNITIVECOMPLEXITYCHECK_H

clang-tools-extra/clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp

class FunctionASTVisitor final
   // Used to efficiently know the last type of the binary sequence operator
   // that was encountered. It would make sense for the function call to start
   // the new sequence, thus it is a stack.
   using OBO = Optional<BinaryOperator::Opcode>;
   std::stack<OBO, SmallVector<OBO, 4>> BinaryOperatorsStack;
 
 public:
   bool traverseStmtWithIncreasedNestingLevel(Stmt *Node) {
     ++CurrentNestingLevel;
     bool ShouldContinue = Base::TraverseStmt(Node);
     --CurrentNestingLevel;
     return ShouldContinue;
   }
 
   bool traverseDeclWithIncreasedNestingLevel(Decl *Node) {
     ++CurrentNestingLevel;
     bool ShouldContinue = Base::TraverseDecl(Node);
#undef CurrentBinaryOperator
   CognitiveComplexity CC;
 };
 
 } // namespace
 
 FunctionCognitiveComplexityCheck::FunctionCognitiveComplexityCheck(
     StringRef Name, ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context),
-      Threshold(Options.get("Threshold", CognitiveComplexity::DefaultLimit)) {}
+      Threshold(Options.get("Threshold", CognitiveComplexity::DefaultLimit)),
+      DescribeBasicIncrements(Options.get("DescribeBasicIncrements", true)) {}
 
 void FunctionCognitiveComplexityCheck::storeOptions(
     ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "Threshold", Threshold);
+  Options.store(Opts, "DescribeBasicIncrements", DescribeBasicIncrements);
 }
 
 void FunctionCognitiveComplexityCheck::registerMatchers(MatchFinder *Finder) {
   Finder->addMatcher(
       functionDecl(isDefinition(),
                    unless(anyOf(isDefaulted(), isDeleted(), isWeak())))
           .bind("func"),
       this);
void FunctionCognitiveComplexityCheck::check(
 
   if (TheDecl)
     diag(Loc, "function %0 has cognitive complexity of %1 (threshold %2)")
         << TheDecl << Visitor.CC.Total << Threshold;
   else
     diag(Loc, "lambda has cognitive complexity of %0 (threshold %1)")
         << Visitor.CC.Total << Threshold;
 
+  if (!DescribeBasicIncrements)
+    return;
+
   // Output all the basic increments of complexity.
   for (const auto &Detail : Visitor.CC.Details) {
     unsigned MsgId;          // The id of the message to output.
     unsigned short Increase; // How much of an increment?
     std::tie(MsgId, Increase) = Detail.process();
     assert(MsgId < Msgs.size() && "MsgId should always be valid");
     // Increase, on the other hand, can be 0.
 
     diag(Detail.Loc, Msgs[MsgId], DiagnosticIDs::Note)
         << (unsigned)Increase << (unsigned)Detail.Nesting << 1 + Detail.Nesting;
   }
 }
 
 } // namespace readability
 } // namespace tidy
 } // namespace clang

clang-tools-extra/docs/clang-tidy/checks/readability-function-cognitive-complexity.rst

 Options
 -------
 
 .. option:: Threshold
 
    Flag functions with Cognitive Complexity exceeding this number.
    The default is `25`.
 
+.. option:: DescribeBasicIncrements
+
+   If set to `true`, then for each function exceeding the complexity threshold
+   the check will issue additional diagnostics on every piece of code (loop,
+   `if` statement, etc.) which contributes to that complexity. See also the
+   examples below. Default is `true`.
+
 Building blocks
 ---------------
 
 There are three basic building blocks of a Cognitive Complexity metric:
 
 Increment
 ^^^^^^^^^
 
int function3(bool var1, bool var2) {
     if(var1) { // +1, nesting level +1
       if(var2)  // +2 (1 + current nesting level of 1), nesting level +1
         return 42;
     }
 
     return 0;
   }
 
+In the last example, the check will flag `function3` if the option Threshold is
+set to `2` or smaller. If the option DescribeBasicIncrements is set to `true`,
+it will additionally flag the two `if` statements with the amounts by which they
+increase to the complexity of the function and the current nesting level.
+
 Limitations
 -----------
 
 The metric is implemented with two notable exceptions:
    * `preprocessor conditionals` (``#ifdef``, ``#if``, ``#elif``, ``#else``,
      ``#endif``) are not accounted for.
    * `each method in a recursion cycle` is not accounted for. It can't be fully
      implemented, because cross-translational-unit analysis would be needed,
      which is currently not possible in clang-tidy.

clang-tools-extra/test/clang-tidy/checkers/readability-function-cognitive-complexity-flags.cpp

+// RUN: %check_clang_tidy %s readability-function-cognitive-complexity %t -- \
+// RUN:   -config='{CheckOptions: \
+// RUN:             [{key: readability-function-cognitive-complexity.Threshold, \
+// RUN:               value: 0}, \
+// RUN:              {key: readability-function-cognitive-complexity.DescribeBasicIncrements, \
+// RUN:               value: "false"} ]}'
+// RUN: %check_clang_tidy -check-suffix=THRESHOLD5 %s readability-function-cognitive-complexity %t -- \
+// RUN:   -config='{CheckOptions: \
+// RUN:             [{key: readability-function-cognitive-complexity.Threshold, \
+// RUN:               value: 5}, \
+// RUN:              {key: readability-function-cognitive-complexity.DescribeBasicIncrements, \
+// RUN:               value: "false"} ]}'
+
+void func_of_complexity_4() {
+  // CHECK-NOTES: :[[@LINE-1]]:6: warning: function 'func_of_complexity_4' has cognitive complexity of 4 (threshold 0) [readability-function-cognitive-complexity]
+  // CHECK-NOTES-THRESHOLD5-NOT: :[[@LINE-2]]:6: warning:
+  if (1) {
+    // CHECK-NOTES-NOT: :[[@LINE-1]]:3: note:
+    if (1) {
+      // CHECK-NOTES-NOT: :[[@LINE-1]]:5: note:
+    }
+  }
+  if (1) {
+    // CHECK-NOTES-NOT: :[[@LINE-1]]:3: note:
+  }
+}
+
+#define MacroOfComplexity10 \
+  if (1) {                  \
+    if (1) {                \
+      if (1) {              \
+        if (1) {            \
+        }                   \
+      }                     \
+    }                       \
+  }
+
+void function_with_macro() {
+  // CHECK-NOTES: :[[@LINE-1]]:6: warning: function 'function_with_macro' has cognitive complexity of 11 (threshold 0) [readability-function-cognitive-complexity]
+  // CHECK-NOTES-THRESHOLD5: :[[@LINE-2]]:6: warning: function 'function_with_macro' has cognitive complexity of 11 (threshold 5) [readability-function-cognitive-complexity]
+
+  MacroOfComplexity10;
+  // CHECK-NOTES-NOT: :[[@LINE-1]]:3: note:
+  // CHECK-NOTES-NOT: :[[@LINE-15]]:3: note: expanded from macro 'MacroOfComplexity10'
+
+  if (1) {
+    // CHECK-NOTES-NOT: :[[@LINE-1]]:3: note:
+  }
+}