[clang-tidy] Fix for Bug 23161

[eszasip]

Summary

The misc-static-assert check will not warn on the followings:

• assert("Some message" == NULL);
• assert(NULL == "Some message");

Added corresponding test cases.

Bug 23161

[alexfh]

clang-tidy/misc/StaticAssertCheck.cpp

31

Maybe wrap anyOf in an expr() and use .bind() on it?

ignoringParenImpCasts(
  expr(anyOf(...)).bind("isAlwaysFalse"));

test/clang-tidy/misc-static-assert.cpp

18

Will it work with a more common definition of NULL?

#define NULL ((void*)0)

[eszasip]

The new fix handles the case when NULL is defined as ((T)0), where T is an arbitrary type.
Please tell me, if you think that T should be restricted to void*.

[alexfh]

clang-tidy/misc/StaticAssertCheck.cpp

34

I'd say that the cast should be restricted to T*. Also, hasDescendant may be to loose. Isn't has (or has(ignoringParenImpCasts(...)) if needed) enough here?

[eszasip]

The result type of the cast (in the definition of NULL) is restricted to T*.
Also replaced hasDescendant() with has().

[alexfh]

Looks good! Thanks for the fix!

Should I submit it for you?

[alexfh]

clang-tidy/misc/StaticAssertCheck.cpp

84

Wait, doesn't CastExpr->getType().isPointerType() work?

[eszasip]

Using built-in method to decide whether the result type is a pointer type.

[eszasip]

clang-tidy/misc/StaticAssertCheck.cpp

84

I also searched for a method like this but QualType didn't have such and I haven't looked at Type.

[alexfh]

Now looks good. Should I commit the patch for you?

[eszasip]

Committed in r234596

In D8920#154632, @alexfh wrote:

Now looks good. Should I commit the patch for you?

Thanks.
I've just got commit access so I can commit my patches from now on.

[alexfh]

Awesome! And thanks again for fixing the issue!

clang-tidy/misc/StaticAssertCheck.cpp

     : ClangTidyCheck(Name, Context) {}
 
 void StaticAssertCheck::registerMatchers(MatchFinder *Finder) {
-  auto IsAlwaysFalse = ignoringParenImpCasts(
-      anyOf(boolLiteral(equals(false)).bind("isAlwaysFalse"),
-            integerLiteral(equals(0)).bind("isAlwaysFalse")));
+  auto IsAlwaysFalse = expr(ignoringParenImpCasts(
+      expr(anyOf(boolLiteral(equals(false)), integerLiteral(equals(0)),
+          nullPtrLiteralExpr())).bind("isAlwaysFalse")));
+  auto IsAlwaysFalseWithCast = ignoringParenImpCasts(anyOf(IsAlwaysFalse,
+      cStyleCastExpr(has(IsAlwaysFalse)).bind("castExpr")));
   auto AssertExprRoot = anyOf(
       binaryOperator(
-          hasOperatorName("&&"),
+          anyOf(hasOperatorName("&&"), hasOperatorName("==")),
           hasEitherOperand(ignoringImpCasts(stringLiteral().bind("assertMSG"))),
-          anyOf(binaryOperator(hasEitherOperand(IsAlwaysFalse)), anything()))
-          .bind("assertExprRoot"),
+          anyOf(binaryOperator(hasEitherOperand(IsAlwaysFalseWithCast)),
+          anything())).bind("assertExprRoot"),
       IsAlwaysFalse);
   auto Condition = expr(anyOf(
       expr(ignoringParenCasts(anyOf(

   const auto *AssertMSG = Result.Nodes.getNodeAs<StringLiteral>("assertMSG");
   const auto *AssertExprRoot =
       Result.Nodes.getNodeAs<BinaryOperator>("assertExprRoot");
+  const auto *CastExpr = Result.Nodes.getNodeAs<CStyleCastExpr>("castExpr");
   SourceLocation AssertExpansionLoc = CondStmt->getLocStart();
 
   if (!Opts.CPlusPlus11 || !AssertExpansionLoc.isValid() ||

     return;
 
   // False literal is not the result of macro expansion.
-  if (IsAlwaysFalse) {
+  if (IsAlwaysFalse && (!CastExpr || CastExpr->getType()->isPointerType())) {
     SourceLocation FalseLiteralLoc =
         SM.getImmediateSpellingLoc(IsAlwaysFalse->getExprLoc());
     if (!FalseLiteralLoc.isMacroID())

 
     StringRef FalseMacroName =
         Lexer::getImmediateMacroName(FalseLiteralLoc, SM, Opts);
-    if (FalseMacroName.compare_lower("false") == 0)
+    if (FalseMacroName.compare_lower("false") == 0 ||
+        FalseMacroName.compare_lower("null") == 0)
       return;
   }
 

test/clang-tidy/misc-static-assert.cpp

   assert(false && "Don't report me!");
   // CHECK-FIXES: {{^  }}assert(false && "Don't report me!");
 
+#define NULL ((void*)0)
+  assert(NULL && "Don't report me!");
+  // CHECK-FIXES: {{^  }}assert(NULL && "Don't report me!");
+
+  assert(NULL == "Don't report me!");
+  // CHECK-FIXES: {{^  }}assert(NULL == "Don't report me!");
+
+  assert("Don't report me!" == NULL);
+  // CHECK-FIXES: {{^  }}assert("Don't report me!" == NULL);
+
+  assert(0 == "Don't report me!");
+  // CHECK-FIXES: {{^  }}assert(0 == "Don't report me!");
+
+#define NULL ((unsigned int)0)
+  assert(NULL && "Report me!");
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: found assert() that could be
+  // CHECK-FIXES: {{^  }}static_assert(NULL , "Report me!");
+#undef NULL
+
   assert(ZERO_MACRO && "Report me!");
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: found assert() that could be
   // CHECK-FIXES: {{^  }}static_assert(ZERO_MACRO , "Report me!");