[clang-tidy] Add option to ignore macros in readability-function-cognitive-complexity check.

[massberg]

Summary

(this was originally part of https://reviews.llvm.org/D96281 and has been split off into its own patch)

If a macro is used within a function, the code inside the macro
doesn't make the code less readable. Instead, for a reader a macro is
more like a function that is called. Thus the code inside a macro
shouldn't increase the complexity of the function in which it is called.
Thus the flag 'IgnoreMacros' is added. If set to 'true' code inside
macros isn't considered during analysis.

This isn't perfect, as now the code of a macro isn't considered at all,
even if it has a high cognitive complexity itself. It might be better if
a macro is considered in the analysis like a function and gets its own
cognitive complexity. Implementing such an analysis seems to be very
complex (if possible at all with the given AST), so we give the user the
option to either ignore macros completely or to let the expanded code
count to the calling function's complexity.

See the code example from vgeof (originally added as note in https://reviews.llvm.org/D96281)

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());
  }

The LOG_WARNING macro itself might have a high complexity, but it do not make the
the function more complex to understand like e.g. a 'printf'.

By default 'IgnoreMacros' is set to 'false', which is the original behavior of the check.

[massberg]

Sync to head.

[lebedev.ri]

Please also add a test with global IgnoreMacros=1 and readability-function-cognitive-complexity.IgnoreMacros unset.
(The code is correct as-is, global IgnoreMacros should not affect the check here.)

I'm also somewhat worried about forward compatibility.
If in future a mode will be implemented that allows to count the cost of macro arguments,
how'd we expose it in check's params, without breaking the existing IgnoreMacros=1 config support?

[lebedev.ri]

In D98070#2616042, @lebedev.ri wrote:

Please also add a test with global IgnoreMacros=1 and readability-function-cognitive-complexity.IgnoreMacros unset.
(The code is correct as-is, global IgnoreMacros should not affect the check here.)

I'm also somewhat worried about forward compatibility.
If in future a mode will be implemented that allows to count the cost of macro arguments,
how'd we expose it in check's params, without breaking the existing IgnoreMacros=1 config support?

[massberg]

Add test with global IgnoreMacros=true and readability-function-cognitive-complexity.IgnoreMacros unset.

[lebedev.ri]

K, thx.

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

31

I'd add something about this option being not guaranteed to forward-compatible.

[massberg]

Add note that the new IgnoreMarcos options isn't guaranteed to be forward compatible.

[massberg]

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

31

I have added a note, thanks!

[alexfh]

LG with a couple of nits.

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

230

nit: Please remove the semicolon and clang-format.

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

31

typo: "guarantueed" -> "guaranteed".

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

 ///
 ///   * `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`
+///   * `IgnoreMacros` - if set to `true`, the check will ignore code inside
+///     macros. Default is `false`.
 ///
 /// 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;
+  const bool IgnoreMacros;
 };
 
 } // namespace readability
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_FUNCTIONCOGNITIVECOMPLEXITYCHECK_H

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

void CognitiveComplexity::account(SourceLocation Loc, unsigned short Nesting,
 
   Total += Increase;
 }
 
 class FunctionASTVisitor final
     : public RecursiveASTVisitor<FunctionASTVisitor> {
   using Base = RecursiveASTVisitor<FunctionASTVisitor>;
 
+  // If set to true, macros are ignored during analysis.
+  const bool IgnoreMacros;
+
   // The current nesting level (increased by Criteria::IncrementNesting).
   unsigned short CurrentNestingLevel = 0;
 
   // 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:
+  explicit FunctionASTVisitor(const bool IgnoreMacros)
+      : IgnoreMacros(IgnoreMacros){};
+
   bool traverseStmtWithIncreasedNestingLevel(Stmt *Node) {
     ++CurrentNestingLevel;
     bool ShouldContinue = Base::TraverseStmt(Node);
     --CurrentNestingLevel;
     return ShouldContinue;
   }
 
   bool traverseDeclWithIncreasedNestingLevel(Decl *Node) {
#define CurrentBinaryOperator BinaryOperatorsStack.top()
   }
 
 #undef CurrentBinaryOperator
 
   bool TraverseStmt(Stmt *Node) {
     if (!Node)
       return Base::TraverseStmt(Node);
 
+    if (IgnoreMacros && Node->getBeginLoc().isMacroID())
+      return true;
+
     // Three following switch()'es have huge duplication, but it is better to
     // keep them separate, to simplify comparing them with the Specification.
 
     CognitiveComplexity::Criteria Reasons = CognitiveComplexity::Criteria::None;
     SourceLocation Location = Node->getBeginLoc();
 
     // B1. Increments
     // There is an increment for each of the following:

 };
 
 } // namespace
 
 FunctionCognitiveComplexityCheck::FunctionCognitiveComplexityCheck(
     StringRef Name, ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context),
       Threshold(Options.get("Threshold", CognitiveComplexity::DefaultLimit)),
-      DescribeBasicIncrements(Options.get("DescribeBasicIncrements", true)) {}
+      DescribeBasicIncrements(Options.get("DescribeBasicIncrements", true)),
+      IgnoreMacros(Options.get("IgnoreMacros", false)) {}
 
 void FunctionCognitiveComplexityCheck::storeOptions(
     ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "Threshold", Threshold);
   Options.store(Opts, "DescribeBasicIncrements", DescribeBasicIncrements);
+  Options.store(Opts, "IgnoreMacros", IgnoreMacros);
 }
 
 void FunctionCognitiveComplexityCheck::registerMatchers(MatchFinder *Finder) {
   Finder->addMatcher(
       functionDecl(isDefinition(),
                    unless(anyOf(isDefaulted(), isDeleted(), isWeak())))
           .bind("func"),
       this);
   Finder->addMatcher(lambdaExpr().bind("lambda"), this);
 }
 
 void FunctionCognitiveComplexityCheck::check(
     const MatchFinder::MatchResult &Result) {
 
-  FunctionASTVisitor Visitor;
+  FunctionASTVisitor Visitor(IgnoreMacros);
   SourceLocation Loc;
 
   const auto *TheDecl = Result.Nodes.getNodeAs<FunctionDecl>("func");
   const auto *TheLambdaExpr = Result.Nodes.getNodeAs<LambdaExpr>("lambda");
   if (TheDecl) {
     assert(TheDecl->hasBody() &&
            "The matchers should only match the functions that "
            "have user-provided body.");

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

 
 .. 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`.
 
+.. option:: IgnoreMacros
+
+   If set to `true`, the check will ignore code inside macros. Note, that also
+   any macro arguments are ignored, even if they should count to the complexity.
+   Default is `false`.
+
 Building blocks
 ---------------
 
 There are three basic building blocks of a Cognitive Complexity metric:
 
 Increment
 ^^^^^^^^^
 

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"} ]}'
+// RUN: %check_clang_tidy -check-suffix=IGNORE-MACROS %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.IgnoreMacros, \
+// RUN:               value: "true"}, \
+// 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-IGNORE-MACROS: :[[@LINE-2]]:6: warning: function 'func_of_complexity_4' has cognitive complexity of 4 (threshold 0) [readability-function-cognitive-complexity]
   if (1) {
     if (1) {
     }
   }
   if (1) {
   }
 }
 
 #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]
+  // CHECK-NOTES-IGNORE-MACROS: :[[@LINE-3]]:6: warning: function 'function_with_macro' has cognitive complexity of 1 (threshold 0) [readability-function-cognitive-complexity]
 
   MacroOfComplexity10;
 
   if (1) {
   }
 }
+
+#define noop \
+  {}
+
+#define SomeMacro(x) \
+  if (1) {           \
+    x;               \
+  }
+
+void func_macro_1() {
+  // CHECK-NOTES: :[[@LINE-1]]:6: warning: function 'func_macro_1' has cognitive complexity of 2 (threshold 0) [readability-function-cognitive-complexity]
+  // CHECK-NOTES-IGNORE-MACROS: :[[@LINE-2]]:6: warning: function 'func_macro_1' has cognitive complexity of 1 (threshold 0) [readability-function-cognitive-complexity]
+
+  if (1) {
+  }
+  SomeMacro(noop);
+}
+
+void func_macro_2() {
+  // CHECK-NOTES: :[[@LINE-1]]:6: warning: function 'func_macro_2' has cognitive complexity of 4 (threshold 0) [readability-function-cognitive-complexity]
+  // CHECK-NOTES-IGNORE-MACROS: :[[@LINE-2]]:6: warning: function 'func_macro_2' has cognitive complexity of 1 (threshold 0) [readability-function-cognitive-complexity]
+
+  if (1) {
+  }
+  // Note that if the IgnoreMacro option is set to 'true', currently also macro
+  // arguments are ignored. Optimally, macros should be treated like function
+  // calls, i.e. the arguments account to the complexity so that the overall
+  // complexity of this function is 2 (1 for the if statement above + 1 for
+  // the if statement in the argument).
+  SomeMacro(if (1) { noop; });
+}