[clang-tidy] Fix bugprone-assert-side-effect to actually give warnings

[carlosgalvezp]

Summary

Some time ago a patch was merged to disable all clang-tidy warnings
from system macros. This led to bugprone-assert-side-effect
silently no longer working, since the warnings came from a system
macro. The problem was not detected because the fake assert functions
were implemented in the same file as the test, instead of being a
system include like it's done in the real world.

Move the assert to a proper system header, and fix the code to
warn at the correct location.

This patch is breakdown from https://reviews.llvm.org/D147081
by PiotrZSL.

Fixes https://github.com/llvm/llvm-project/issues/62314

[gribozavr2]

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

2

There should be no need to define the function (the real header wouldn't either).

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

1–2

Perhaps a more reliable (or a "belt and suspenders") way would be to add the following pragma to the header:

#pragma clang system_header

This way I believe it would not matter whether the header is found through -isystem or not.

[carlosgalvezp]

Thanks for the review! Fixed your comments before landing.

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

 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/test/clang-tidy/checkers/bugprone/Inputs/assert-side-effect/assert.h

+#pragma clang system_header
+
+int abort();
+
+#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.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
-
-//===--- 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)                                                       \
-        )
-
-
-//===----------------------------------------------------------------------===//
+// 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 -I %S/Inputs/assert-side-effect
+#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; }