[clang-tidy] bugprone-assert-side-effect: Warn on NSAssert by default.

[NoQ]

Summary

This is basically standard Objective-C assert. While we could always pass it as an option, i believe it makes perfect sense to warn on it by default.

The same applies to NSCAssert which drops additional support for displaying Objective-C method information so it acts exactly like C assert.

I tested it on live code with actual Foundation headers and it seems to work correctly out of the box.

I'm also interested in making an even more agressive step in this direction and adding an option to accept any macro that ends with "...assert" suffix, case-insensitively. This would probably go under an off-by-default flag. Would this be acceptable?

[vsavchenko]

LGTM, but it would be great if code owners take a look as well.

[aaron.ballman]

LGTM!

I'm also interested in making an even more agressive step in this direction and adding an option to accept any macro that ends with "...assert" suffix, case-insensitively. This would probably go under an off-by-default flag. Would this be acceptable?

I think that makes sense to me. We should probably look at popular OS SDKs and CRTs to see what creative ways they spell assertion macros to make sure those are covered as well, like https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/assert-asserte-assert-expr-macros?view=msvc-160 and https://docs.microsoft.com/en-us/windows-hardware/drivers/ddi/wdm/nf-wdm-assertmsg

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

 }
 
 } // namespace
 
 AssertSideEffectCheck::AssertSideEffectCheck(StringRef Name,
                                              ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context),
       CheckFunctionCalls(Options.get("CheckFunctionCalls", false)),
-      RawAssertList(Options.get("AssertMacros", "assert")) {
+      RawAssertList(Options.get("AssertMacros",
+                                "assert,NSAssert,NSCAssert")) {
   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, "AssertMacros", RawAssertList);
 }

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

+// RUN: %check_clang_tidy %s bugprone-assert-side-effect %t
+
+int abort();
+
+@interface NSObject
+@end
+
+@interface NSString
+@end
+
+@interface NSAssertionHandler
++ (NSAssertionHandler *)currentHandler;
+- handleFailureInMethod:(SEL)cmd object:(NSObject *)obj desc:(NSString *)desc;
+- handleFailureInFunction:(NSString *)desc;
+@end
+
+#ifndef NDEBUG
+#define NSAssert(condition, description, ...)                                    \
+  do {                                                                           \
+    if (__builtin_expect(!(condition), 0)) {                                     \
+      [[NSAssertionHandler currentHandler] handleFailureInMethod:_cmd            \
+                                                          object:self            \
+                                                            desc:(description)]; \
+    }                                                                            \
+  } while (0);
+#define NSCAssert(condition, description, ...)                                     \
+  do {                                                                             \
+    if (__builtin_expect(!(condition), 0)) {                                       \
+      [[NSAssertionHandler currentHandler] handleFailureInFunction:(description)]; \
+    }                                                                              \
+  } while (0);
+#else
+#define NSAssert(condition, description, ...) do {} while (0)
+#define NSCAssert(condition, description, ...) do {} while (0)
+#endif
+
+@interface I : NSObject
+- (void)foo;
+@end
+
+@implementation I
+- (void)foo {
+  int x = 0;
+  NSAssert((++x) == 1, @"Ugh.");
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: side effect in NSAssert() condition discarded in release builds [bugprone-assert-side-effect]
+}
+@end
+
+void foo() {
+  int x = 0;
+  NSCAssert((++x) == 1, @"Ugh.");
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: side effect in NSCAssert() condition discarded in release builds [bugprone-assert-side-effect]
+}