[clang-tidy] Add a checker for implicit bool conversion of a bool*.

[bkramer]

Summary

The goal is to find code like the example below, which is likely a typo
where someone meant to write "if (*b)".
bool *b = SomeFunction();
if (b) {

// b never dereferenced

}

This checker naturally has a relatively high false positive rate so it
applies some heuristics to avoid cases where the pointer is checked for
nullptr before being written.

[djasper]

clang-tidy/misc/BoolPointerImplicitConversion.cpp

47 ↗

(On Diff #11257)

You could move this part into your matcher with something like

expr(ignoringImpCasts(declRefExpr().bind("expr")))

test/clang-tidy/misc-bool-pointer-implicit-conversion.cpp

1 ↗

(On Diff #11257)

Maybe add a test cases with a binary expression in the if condition.

15 ↗

(On Diff #11257)

Don't you need at least one more CHECK-MESSAGES-NOT: warning to verify that there aren't any further warnings?

[alexfh]

Thank you for working on this!

test/clang-tidy/misc-bool-pointer-implicit-conversion.cpp

7 ↗

(On Diff #11257)

Please add tests with macros and templates.

15 ↗

(On Diff #11257)

I'm also working on a patch for FileCheck to add an option to add implicit CHECK-NOT with certain pattern around each CHECK. It is rather trivial to do, I'm just stuck with the naming. Current version is -explicit-matches=<pattern>

[bkramer]

Addressed the first round of Daniel's comments.

[bkramer]

Add a test with macros and templates. We seem to be doing the right thing here.

[djasper]

I think there are still many corner cases that can be improved, but this is good as a first version.

test/clang-tidy/misc-bool-pointer-implicit-conversion.cpp

8 ↗

(On Diff #11263)

Can you name this something other than CHECK? I find this a bit confusing with CHECK-MESSAGES and CHECK-FIXES..

Also consider moving this down so it is close to its invocation.

[bkramer]

Don't call a macro CHECK :)

[alexfh]

clang-tidy/misc/BoolPointerImplicitConversion.cpp

51 ↗

(On Diff #11271)

As discussed off-the-list, presence of other pointer-specific operations could be used to avoid some false positives: use of "delete" and passing the value as an argument of a function taking bool*, reference to bool* or anything that bool* can be converted to, but bool can't (void*, maybe something else).

56 ↗

(On Diff #11271)

I'd check here the type of the corresponding parameter. If it is "bool", this would be a strong indicator of a true positive.

test/clang-tidy/misc-bool-pointer-implicit-conversion.cpp

32 ↗

(On Diff #11271)

How is this supposed to check the right thing?

37 ↗

(On Diff #11271)

I'd go for a "CHECK-MESSAGES-NOT: warning:", at least for consistency.

[bkramer]

Allow delete expressions in the if block. I also tried to extend the callExpr filter to allow calls with implicit conversions on the arguments. Sadly hasAnyArgument has the annoying behavior of stripping all implicit casts early so this is a FIXME for now.

[alexfh]

Looks good.

clang-tidy/misc/BoolPointerImplicitConversion.h

22 ↗

(On Diff #11276)

Maybe "// Never used in a pointer-specific way."?

[bkramer]

Closed by commit rL212797 (authored by d0k).

clang-tools-extra/trunk/clang-tidy/misc/BoolPointerImplicitConversion.h

+//===--- BoolPointerImplicitConversion.h - clang-tidy -----------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_BOOL_POINTER_IMPLICIT_CONV_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_BOOL_POINTER_IMPLICIT_CONV_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+
+/// \brief Checks for conditions based on implicit conversion from a bool
+/// pointer to bool e.g.
+/// bool *p;
+/// if (p) {
+///   // Never used in a pointer-specific way.
+/// }
+class BoolPointerImplicitConversion : public ClangTidyCheck {
+public:
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_BOOL_POINTER_IMPLICIT_CONV_H
+

clang-tools-extra/trunk/clang-tidy/misc/BoolPointerImplicitConversion.cpp

+//===--- BoolPointerImplicitConversion.cpp - clang-tidy -------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "BoolPointerImplicitConversion.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace ast_matchers {
+
+AST_MATCHER(CastExpr, isPointerToBoolean) {
+  return Node.getCastKind() == CK_PointerToBoolean;
+}
+AST_MATCHER(QualType, isBoolean) { return Node->isBooleanType(); }
+
+} // namespace ast_matchers
+
+namespace tidy {
+
+void BoolPointerImplicitConversion::registerMatchers(MatchFinder *Finder) {
+  // Look for ifs that have an implicit bool* to bool conversion in the
+  // condition. Filter negations.
+  Finder->addMatcher(
+      ifStmt(hasCondition(findAll(implicitCastExpr(
+                 allOf(unless(hasParent(unaryOperator(hasOperatorName("!")))),
+                       hasSourceExpression(expr(
+                           hasType(pointerType(pointee(isBoolean()))),
+                           ignoringParenImpCasts(declRefExpr().bind("expr")))),
+                       isPointerToBoolean()))))).bind("if"),
+      this);
+}
+
+void
+BoolPointerImplicitConversion::check(const MatchFinder::MatchResult &Result) {
+  auto *If = Result.Nodes.getStmtAs<IfStmt>("if");
+  auto *Var = Result.Nodes.getStmtAs<DeclRefExpr>("expr");
+
+  // Only allow variable accesses for now, no function calls or member exprs.
+  // Check that we don't dereference the variable anywhere within the if. This
+  // avoids false positives for checks of the pointer for nullptr before it is
+  // dereferenced. If there is a dereferencing operator on this variable don't
+  // emit a diagnostic. Also ignore array subscripts.
+  const Decl *D = Var->getDecl();
+  auto DeclRef = ignoringParenImpCasts(declRefExpr(to(equalsNode(D))));
+  if (!match(findAll(
+                 unaryOperator(hasOperatorName("*"), hasUnaryOperand(DeclRef))),
+             *If, *Result.Context).empty() ||
+      !match(findAll(arraySubscriptExpr(hasBase(DeclRef))), *If,
+             *Result.Context).empty() ||
+      // FIXME: We should still warn if the paremater is implicitly converted to
+      // bool.
+      !match(findAll(callExpr(hasAnyArgument(DeclRef))), *If, *Result.Context)
+           .empty() ||
+      !match(findAll(deleteExpr(has(expr(DeclRef)))), *If, *Result.Context)
+           .empty())
+    return;
+
+  diag(Var->getLocStart(), "dubious check of 'bool *' against 'nullptr', did "
+                           "you mean to dereference it?")
+      << FixItHint::CreateInsertion(Var->getLocStart(), "*");
+}
+
+} // namespace tidy
+} // namespace clang

clang-tools-extra/trunk/clang-tidy/misc/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyMiscModule
   ArgumentCommentCheck.cpp
+  BoolPointerImplicitConversion.cpp
   MiscTidyModule.cpp
   RedundantSmartptrGet.cpp
   UseOverride.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangTidy
   )

clang-tools-extra/trunk/clang-tidy/misc/MiscTidyModule.cpp

 //===--- MiscTidyModule.cpp - clang-tidy ----------------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "ArgumentCommentCheck.h"
+#include "BoolPointerImplicitConversion.h"
 #include "RedundantSmartptrGet.h"
 #include "UseOverride.h"
 
 namespace clang {
 namespace tidy {
 
 class MiscModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.addCheckFactory(
         "misc-argument-comment",
         new ClangTidyCheckFactory<ArgumentCommentCheck>());
     CheckFactories.addCheckFactory(
+        "misc-bool-pointer-implicit-conversion",
+        new ClangTidyCheckFactory<BoolPointerImplicitConversion>());
+    CheckFactories.addCheckFactory(
         "misc-redundant-smartptr-get",
         new ClangTidyCheckFactory<RedundantSmartptrGet>());
     CheckFactories.addCheckFactory(
         "misc-use-override",
         new ClangTidyCheckFactory<UseOverride>());
   }
 };
 

clang-tools-extra/trunk/test/clang-tidy/misc-bool-pointer-implicit-conversion.cpp

+// RUN: $(dirname %s)/check_clang_tidy_fix.sh %s misc-bool-pointer-implicit-conversion %t
+// REQUIRES: shell
+
+bool *SomeFunction();
+void SomeOtherFunction(bool*);
+bool F();
+void G(bool);
+
+
+template <typename T>
+void t(T b) {
+  if (b) {
+  }
+}
+
+void foo() {
+  bool *b = SomeFunction();
+  if (b) {
+// CHECK-MESSAGES: dubious check of 'bool *' against 'nullptr'
+// CHECK-FIXES: if (*b) {
+  }
+
+  if (F() && b) {
+// CHECK-MESSAGES: dubious check of 'bool *' against 'nullptr'
+// CHECK-FIXES: if (F() && *b) {
+  }
+
+  // TODO: warn here.
+  if (b) {
+    G(b);
+  }
+
+#define TESTMACRO if (b || F())
+
+  TESTMACRO {
+// CHECK-MESSAGES: dubious check of 'bool *' against 'nullptr'
+// Can't fix this.
+// CHECK-FIXES: #define TESTMACRO if (b || F())
+// CHECK-FIXES: TESTMACRO {
+  }
+
+// CHECK-MESSAGES-NOT: warning:
+
+  t(b);
+
+  if (!b) {
+    // no-warning
+  }
+
+  if (SomeFunction()) {
+    // no-warning
+  }
+
+  bool *c = SomeFunction();
+  if (c) {
+    (void)c;
+    (void)*c; // no-warning
+  }
+
+  if (c) {
+    *c = true; // no-warning
+  }
+
+  if (c) {
+    c[0] = false; // no-warning
+  }
+
+  if (c) {
+    SomeOtherFunction(c); // no-warning
+  }
+
+  if (c) {
+    delete[] c; // no-warning
+  }
+
+  if (c) {
+    *(c) = false; // no-warning
+  }
+
+  struct {
+    bool *b;
+  } d = { SomeFunction() };
+
+  if (d.b)
+    (void)*d.b; // no-warning
+}