[clang-tidy] Fix issues in bugprone-assert-side-effect

[PiotrZSL]

Summary

• Fixed problem when after changes in macros handling in Clang-tidy 14 cased issues from from this check be hidden when assert is defined in system headers. Now raising diagnostic for a place from where macro is called.
• Added support for const, pure, constexpr functions/methods. Now they considered safe.
• Split configuration options CheckFunctionCalls, IgnoredFunctions, to support enabling only checking of methods, or only of functions.

Fixes: #47605, #31825, #31821

[Eugene.Zelenko]

clang-tools-extra/clang-tidy/bugprone/AssertSideEffectCheck.cpp

66

Please do not use auto if type is not spelled explicitly or iterator.

[PiotrZSL]

clang-tools-extra/clang-tidy/bugprone/AssertSideEffectCheck.cpp

66

That auto already was there, I just moved it... :)

[Eugene.Zelenko]

clang-tools-extra/clang-tidy/bugprone/AssertSideEffectCheck.cpp

66

So it's great reason to finally fix it :-)

[PiotrZSL]

Fix auto

[carlosgalvezp]

You mention in the commit message that it fixes #25569, but that seems unrelated (and is already closed)?

Personally I find it hard to review 2 github issue fixes + 1 NFC (moving the assert code to assert.h) on the same patch, would it be possible to split to have a more focused review? I believe that would improve both speed and quality of the review.

[PiotrZSL]

You right, I copied wrong issue id, should be #31825.
I do not plan to split this review. It's simply too big effort to do that, and then maintain
multiple reviews that have dependencies on each other, when they all are related to same check.

clang-tools-extra/clang-tidy/bugprone/AssertSideEffectCheck.h

 public:
   AssertSideEffectCheck(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;
 
 private:
   const bool CheckFunctionCalls;
+  const bool CheckMethodCalls;
   const StringRef RawAssertList;
   SmallVector<StringRef, 5> AssertMacros;
   const std::vector<StringRef> IgnoredFunctions;
+  const std::vector<StringRef> IgnoredMethods;
 };
 
 } // namespace clang::tidy::bugprone
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_ASSERTSIDEEFFECTCHECK_H

clang-tools-extra/clang-tidy/bugprone/AssertSideEffectCheck.cpp

 #include <string>
 
 using namespace clang::ast_matchers;
 
 namespace clang::tidy::bugprone {
 
 namespace {
 
-AST_MATCHER_P2(Expr, hasSideEffect, bool, CheckFunctionCalls,
+AST_MATCHER_P2(Expr, hasSideEffect, bool, CheckCalls,
                clang::ast_matchers::internal::Matcher<NamedDecl>,
-               IgnoredFunctionsMatcher) {
+               IgnoredDeclMatcher) {
   const Expr *E = &Node;
 
   if (const auto *Op = dyn_cast<UnaryOperator>(E)) {
     UnaryOperator::Opcode OC = Op->getOpcode();
     return OC == UO_PostInc || OC == UO_PostDec || OC == UO_PreInc ||
            OC == UO_PreDec;
   }
 
return OpKind == OO_Equal || OpKind == OO_PlusEqual ||
            OpKind == OO_PipeEqual || OpKind == OO_CaretEqual ||
            OpKind == OO_LessLessEqual || OpKind == OO_GreaterGreaterEqual ||
            OpKind == OO_PlusPlus || OpKind == OO_MinusMinus ||
            OpKind == OO_PercentEqual || OpKind == OO_New ||
            OpKind == OO_Delete || OpKind == OO_Array_New ||
            OpKind == OO_Array_Delete;
   }
 
-  if (const auto *CExpr = dyn_cast<CallExpr>(E)) {
-    bool Result = CheckFunctionCalls;
-    if (const auto *FuncDecl = CExpr->getDirectCallee()) {
-      if (FuncDecl->getDeclName().isIdentifier() &&
-          IgnoredFunctionsMatcher.matches(*FuncDecl, Finder,
-                                          Builder)) // exceptions come here
-        Result = false;
-      else if (const auto *MethodDecl = dyn_cast<CXXMethodDecl>(FuncDecl))
-        Result &= !MethodDecl->isConst();
-    }
-    return Result;
-  }
+  if (isa<CXXNewExpr>(E) || isa<CXXDeleteExpr>(E) || isa<CXXThrowExpr>(E))
+    return true;
+
+  if (!CheckCalls)
+    return false;
+
+  const auto *CExpr = dyn_cast<CallExpr>(E);
+  if (!CExpr)
+    return false;
+
+  const FunctionDecl *FuncDecl = CExpr->getDirectCallee();
+  if (!FuncDecl || !FuncDecl->getDeclName().isIdentifier())
+    return false;
 
-  return isa<CXXNewExpr>(E) || isa<CXXDeleteExpr>(E) || isa<CXXThrowExpr>(E);
+  return !IgnoredDeclMatcher.matches(*FuncDecl, Finder, Builder);
 }
 
+AST_MATCHER_P(Decl, isEnabled, bool, Value) { return Value; }
+
 } // namespace
 
 AssertSideEffectCheck::AssertSideEffectCheck(StringRef Name,
                                              ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context),
       CheckFunctionCalls(Options.get("CheckFunctionCalls", false)),
+      CheckMethodCalls(Options.get("CheckMethodCalls", false)),
       RawAssertList(Options.get("AssertMacros", "assert,NSAssert,NSCAssert")),
       IgnoredFunctions(utils::options::parseListPair(
-          "__builtin_expect;", Options.get("IgnoredFunctions", ""))) {
+          "__builtin_expect;", Options.get("IgnoredFunctions", ""))),
+      IgnoredMethods(
+          utils::options::parseStringList(Options.get("IgnoredMethods", ""))) {
   StringRef(RawAssertList).split(AssertMacros, ",", -1, false);
 }
 
 // The options are explained in AssertSideEffectCheck.h.
 void AssertSideEffectCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "CheckFunctionCalls", CheckFunctionCalls);
+  Options.store(Opts, "CheckMethodCalls", CheckMethodCalls);
   Options.store(Opts, "AssertMacros", RawAssertList);
   Options.store(Opts, "IgnoredFunctions",
                 utils::options::serializeStringList(IgnoredFunctions));
+  Options.store(Opts, "IgnoredMethods",
+                utils::options::serializeStringList(IgnoredMethods));
 }
 
 void AssertSideEffectCheck::registerMatchers(MatchFinder *Finder) {
-  auto IgnoredFunctionsMatcher =
-      matchers::matchesAnyListedName(IgnoredFunctions);
-
-  auto DescendantWithSideEffect =
-      traverse(TK_AsIs, hasDescendant(expr(hasSideEffect(
-                            CheckFunctionCalls, IgnoredFunctionsMatcher))));
+  auto IgnoredMatcher = functionDecl(
+      anyOf(
+          functionDecl(isEnabled(CheckFunctionCalls), unless(cxxMethodDecl())),
+          cxxMethodDecl(isEnabled(CheckMethodCalls))),
+      anyOf(hasAttr(clang::attr::Const), hasAttr(clang::attr::Pure),
+            isConstexpr(),
+            namedDecl(unless(cxxMethodDecl()),
+                      matchers::matchesAnyListedName(IgnoredFunctions)),
+            cxxMethodDecl(anyOf(
+                isConst(), matchers::matchesAnyListedName(IgnoredMethods)))));
+  auto DescendantWithSideEffect = traverse(
+      TK_AsIs, hasDescendant(expr(hasSideEffect(
+                   CheckFunctionCalls || CheckMethodCalls, IgnoredMatcher))));
   auto ConditionWithSideEffect = hasCondition(DescendantWithSideEffect);
+
   Finder->addMatcher(
       stmt(
           anyOf(conditionalOperator(ConditionWithSideEffect),
                 ifStmt(ConditionWithSideEffect),
                 unaryOperator(hasOperatorName("!"),
                               hasUnaryOperand(unaryOperator(
                                   hasOperatorName("!"),
                                   hasUnaryOperand(DescendantWithSideEffect))))))
           .bind("condStmt"),
       this);
 }
 
 void AssertSideEffectCheck::check(const MatchFinder::MatchResult &Result) {
   const SourceManager &SM = *Result.SourceManager;
   const LangOptions LangOpts = getLangOpts();
   SourceLocation Loc = Result.Nodes.getNodeAs<Stmt>("condStmt")->getBeginLoc();
 
   StringRef AssertMacroName;
   while (Loc.isValid() && Loc.isMacroID()) {
     StringRef MacroName = Lexer::getImmediateMacroName(Loc, SM, LangOpts);
+    Loc = SM.getImmediateMacroCallerLoc(Loc);
 
     // Check if this macro is an assert.
     if (llvm::is_contained(AssertMacros, MacroName)) {
       AssertMacroName = MacroName;
       break;
     }
-    Loc = SM.getImmediateMacroCallerLoc(Loc);
   }
   if (AssertMacroName.empty())
     return;
 
   diag(Loc, "side effect in %0() condition discarded in release builds")
       << AssertMacroName;
 }
 
 } // namespace clang::tidy::bugprone

clang-tools-extra/docs/ReleaseNotes.rst

- New alias :doc:`cert-msc24-c
   <clang-tidy/checks/bugprone/unsafe-functions>` was added.
 
 - New alias :doc:`cert-msc33-c
   <clang-tidy/checks/cert/msc33-c>` to :doc:`bugprone-unsafe-functions
   <clang-tidy/checks/bugprone/unsafe-functions>` was added.
 
 Changes in existing checks
 ^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+- The :doc:`bugprone-assert-side-effect
+  <clang-tidy/checks/bugprone/assert-side-effect>` check now supports
+  function attributes ``pure/const``, and ``constexpr`` functions. Configuration
+  options `CheckFunctionCalls` and `IgnoredFunctions` have been split into
+  separate options for functions and methods. Additionally, a bug was fixed
+  that previously required using the `--system-headers` switch to see issues
+  related to the usage of macros defined in system headers.
+
 - Improved :doc:`readability-redundant-string-cstr
   <clang-tidy/checks/readability/redundant-string-cstr>` check to recognise
   unnecessary ``std::string::c_str()`` and ``std::string::data()`` calls in
   arguments to ``std::print``, ``std::format`` or other functions listed in
   the ``StringParameterFunction`` check option.
 
 - Deprecated check-local options `HeaderFileExtensions`
   in :doc:`bugprone-dynamic-static-initializers

clang-tools-extra/docs/clang-tidy/checks/bugprone/assert-side-effect.rst

 .. title:: clang-tidy - bugprone-assert-side-effect
 
 bugprone-assert-side-effect
 ===========================
 
 Finds ``assert()`` with side effect.
 
 The condition of ``assert()`` is evaluated only in debug builds so a
 condition with side effect can cause different behavior in debug / release
 builds.
 
+Functions and methods marked with the attributes ``pure``, ``const``, or
+``constexpr`` (C++11) are not treated as potentially producing side-effects.
+
 Options
 -------
 
 .. option:: AssertMacros
 
    A comma-separated list of the names of assert macros to be checked.
 
 .. option:: CheckFunctionCalls
 
+   Whether to treat non-member functions as they produce side effects.
+   Disabled by default because it can increase the number of false
+   positive warnings.
+
+.. option:: CheckiMethodCalls
+
    Whether to treat non-const member and non-member functions as they produce
    side effects. Disabled by default because it can increase the number of false
    positive warnings.
 
 .. option:: IgnoredFunctions
 
-   A semicolon-separated list of the names of functions or methods to be
-   considered as not having side-effects. Regular expressions are accepted,
+   A semicolon-separated list of the names of functions to be considered as
+   not having side-effects. Regular expressions are accepted,
+   e.g. `[Rr]ef(erence)?$` matches every type with suffix `Ref`, `ref`,
+   `Reference` and `reference`. The default is empty. If a name in the list
+   contains the sequence `::` it is matched against the qualified typename
+   (i.e. `namespace::Type`, otherwise it is matched against only
+   the type name (i.e. `Type`).
+
+.. option:: IgnoredMethods
+
+   A semicolon-separated list of the names of methods to be considered as
+   not having side-effects. Regular expressions are accepted,
    e.g. `[Rr]ef(erence)?$` matches every type with suffix `Ref`, `ref`,
    `Reference` and `reference`. The default is empty. If a name in the list
    contains the sequence `::` it is matched against the qualified typename
    (i.e. `namespace::Type`, otherwise it is matched against only
    the type name (i.e. `Type`).

clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/assert-side-effect/assert.h

+#pragma once
+
+int abort() { return 0; }
+
+#ifdef NDEBUG
+#define assert(x) 1
+#else
+#define assert(x)                                                              \
+  if (!(x))                                                                    \
+  (void)abort()
+#endif
+
+void print(...);
+#define assert2(e) (__builtin_expect(!(e), 0) ?                                \
+                       print (#e, __FILE__, __LINE__) : (void)0)
+
+#ifdef NDEBUG
+#define my_assert(x) 1
+#else
+#define my_assert(x)                                                           \
+  ((void)((x) ? 1 : abort()))
+#endif
+
+#ifdef NDEBUG
+#define not_my_assert(x) 1
+#else
+#define not_my_assert(x)                                                       \
+  if (!(x))                                                                    \
+  (void)abort()
+#endif
+
+#define real_assert(x) ((void)((x) ? 1 : abort()))
+#define wrap1(x) real_assert(x)
+#define wrap2(x) wrap1(x)
+#define convoluted_assert(x) wrap2(x)
+
+#define msvc_assert(expression) (void)(                                        \
+            (!!(expression)) ||                                                \
+            (abort(), 0)                                                       \
+        )

clang-tools-extra/test/clang-tidy/checkers/bugprone/assert-side-effect-constexpr.cpp

+// RUN: %check_clang_tidy -std=c++11-or-later %s bugprone-assert-side-effect %t -- \
+// RUN: -config="{CheckOptions: [{key: bugprone-assert-side-effect.CheckFunctionCalls, value: true}, \
+// RUN:                          {key: bugprone-assert-side-effect.CheckMethodCalls, value: true}, \
+// RUN:                          {key: bugprone-assert-side-effect.AssertMacros, value: 'assert'}]}" \
+// RUN: -- -fexceptions -isystem %S/Inputs/assert-side-effect
+
+#include <assert.h>
+
+class MyClass {
+public:
+  bool badFunc(int a, int b) { return a * b > 0; }
+  constexpr bool goodFunc(int a, int b) { return a * b > 0; }
+};
+
+bool badFunction() {
+  return false;
+}
+
+constexpr bool goodFunction() {
+  return true;
+}
+
+int main() {
+
+  MyClass mc;
+  assert(mc.badFunc(0, 1));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: side effect in assert() condition discarded in release builds
+  assert(badFunction());
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: side effect in assert() condition discarded in release builds
+
+  assert(mc.goodFunc(0, 1));
+  assert(goodFunction);
+  return 0;
+}

clang-tools-extra/test/clang-tidy/checkers/bugprone/assert-side-effect.cpp

-// RUN: %check_clang_tidy %s bugprone-assert-side-effect %t -- -config="{CheckOptions: [{key: bugprone-assert-side-effect.CheckFunctionCalls, value: true}, {key: bugprone-assert-side-effect.AssertMacros, value: 'assert,assert2,my_assert,convoluted_assert,msvc_assert'}, {key: bugprone-assert-side-effect.IgnoredFunctions, value: 'MyClass::badButIgnoredFunc'}]}" -- -fexceptions
+// RUN: %check_clang_tidy %s bugprone-assert-side-effect %t -- \
+// RUN: -config="{CheckOptions: [{key: bugprone-assert-side-effect.CheckFunctionCalls, value: true}, \
+// RUN:                          {key: bugprone-assert-side-effect.CheckMethodCalls, value: true}, \
+// RUN:                          {key: bugprone-assert-side-effect.AssertMacros, value: 'assert,assert2,my_assert,convoluted_assert,msvc_assert'}, \
+// RUN:                          {key: bugprone-assert-side-effect.IgnoredFunctions, value: 'functionToIgnore'}, \
+// RUN:                          {key: bugprone-assert-side-effect.IgnoredMethods, value: 'MyClass::badButIgnoredFunc'}]}" \
+// RUN: -- -fexceptions -isystem %S/Inputs/assert-side-effect
 
-//===--- assert definition block ------------------------------------------===//
-int abort() { return 0; }
-
-#ifdef NDEBUG
-#define assert(x) 1
-#else
-#define assert(x)                                                              \
-  if (!(x))                                                                    \
-  (void)abort()
-#endif
-
-void print(...);
-#define assert2(e) (__builtin_expect(!(e), 0) ?                                \
-                       print (#e, __FILE__, __LINE__) : (void)0)
-
-#ifdef NDEBUG
-#define my_assert(x) 1
-#else
-#define my_assert(x)                                                           \
-  ((void)((x) ? 1 : abort()))
-#endif
-
-#ifdef NDEBUG
-#define not_my_assert(x) 1
-#else
-#define not_my_assert(x)                                                       \
-  if (!(x))                                                                    \
-  (void)abort()
-#endif
-
-#define real_assert(x) ((void)((x) ? 1 : abort()))
-#define wrap1(x) real_assert(x)
-#define wrap2(x) wrap1(x)
-#define convoluted_assert(x) wrap2(x)
-
-#define msvc_assert(expression) (void)(                                        \
-            (!!(expression)) ||                                                \
-            (abort(), 0)                                                       \
-        )
-
-
-//===----------------------------------------------------------------------===//
+#include <assert.h>
 
 bool badButIgnoredFunc(int a, int b) { return a * b > 0; }
 
 class MyClass {
 public:
   bool badFunc(int a, int b) { return a * b > 0; }
   bool badButIgnoredFunc(int a, int b) { return a * b > 0; }
   bool goodFunc(int a, int b) const { return a * b > 0; }

 public:
   bool badButIgnoredFunc(int a, int b) { return a * b > 0; }
 };
 
 bool freeFunction() {
   return true;
 }
 
+bool functionToIgnore() {
+  return true;
+}
+
+bool constFunction(bool input) __attribute__ ((const)) {
+  return !input;
+}
+
+bool pureFunction(bool input) __attribute__ ((pure)) {
+  return !input;
+}
+
+constexpr bool constexprFunction(bool input) {
+  return !input;
+}
+
 int main() {
 
   int X = 0;
   bool B = false;
   assert(X == 1);
 
   assert(X = 1);
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: side effect in assert() condition discarded in release builds [bugprone-assert-side-effect]
int main() {
   assert((throw 1, false));
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: side effect in assert() condition discarded in release builds
 
   assert2(1 == 2 - 1);
 
   msvc_assert(mc2 = mc);
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: side effect in msvc_assert() condition discarded in release builds
 
+  assert(functionToIgnore());
+  assert(constFunction(false));
+  assert(pureFunction(false));
+  assert(constexprFunction(false));
+
   return 0;
 }