Warn on bool* to bool conversion

[hiraditya]

Summary

We found conversion from bool* to bool to be a common source of bug, so we have implemented this warning. Sample use case:

int bar(bool b) {
  return b;
}

int baz() {
  bool *b; 
  bar(b);
  return 0;
}

Typically, there would be a function which takes a bool, which gets a pointer to boolean at the call site. The compiler currently does not warn which results
in a difficult to debug runtime failure.

[Eugene.Zelenko]

There is Clang-tidy's readability-implicit-bool-conversion check.

It may be reasonable to check pointers to integers too, since integers are often implicitly converted to bools.

Please mention new warning in Release Notes.

[• Quuxplusone]

test/CXX/expr/expr.unary/expr.unary.op/p6.cpp

18 ↗

(On Diff #142295)

This is not "comparing" anything, so the warning seems inappropriate here. Maybe "implicitly converting 'bool *' to 'bool' in operand of '!'` would be more appropriate?

Please add test cases for operator!, operator&&, operator||, and operator?.

test/Sema/static-init.c

10 ↗

(On Diff #142295)

Again, not "comparing" anything.
Incidentally, do you know why this line fails to produce an "address of 't' will always evaluate to 'true'" warning?

test/SemaCXX/warn-bool-ptr-to-bool.cpp

6

Please add a test case

template<class T>
T foo(T *ptr) {
    return ptr ? *ptr : T{};
}
bool bar(bool *ptr) { return foo(ptr); }

and make sure the warning does not trigger in this case. (It would be OK to trigger a clang-tidy check in this case, but IMHO not a -W -Wall -Wextra warning.)

[lebedev.ri]

@hiraditya I personally don't like when i'm being told so, but i'd like to see some numbers...
Please run this on some big C++ project (LLVM (but you'll have to enable this diag specifically), google chrome, ???), and analyse the results.

In D45601#1066572, @Eugene.Zelenko wrote:

There is Clang-tidy's readability-implicit-bool-conversion check.

It may be reasonable to check pointers to integers too, since integers are often implicitly converted to bools.

While warning on bool*->bool *might* be ok, also warning for all inttype*->bool will likely be *too* noisy for clang diagnostic.

[lebedev.ri]

include/clang/Basic/DiagnosticSemaKinds.td

7806

Also, this really really should be under it's own flag, which in turn may be under Extra.

[aaron.ballman]

I'd also be interested to see the number of false positives and true positives when run over some large code bases (both C and C++). For instance, I would imagine code like this to be somewhat common and very reasonable:

void some_func(int someArg, bool *someResult) {
  if (someResult)
    *someResult = false;
}

Especially in C code where references are not available. This makes me wary of making this a compiler diagnostic, but clang-tidy may still be a reasonable place for this functionality to live.

[lebedev.ri]

In D45601#1067057, @aaron.ballman wrote:

...
This makes me wary of making this a compiler diagnostic, but clang-tidy may still be a reasonable place for this functionality to live.

In D45601#1066572, @Eugene.Zelenko wrote:

There is Clang-tidy's readability-implicit-bool-conversion check.

[hiraditya]

I'll probably make this warning for function arguments only (when bool* is passed to a function accepting bool). Many conditionals use bool* -> bool conversion as pointed out by @Quuxplusone and @aaron.ballman

[sberg]

A random data point from trying this patch on the LibreOffice code base:

• a little over 100 cases that are easily identified as false positives (many of the form "if (p) *p = ...")

• two or three cases that looked suspicious on first glance but turned out to be false positives too (but where the code apparently benefits from cleaning up)

• two cases of false positives that are awkward to silence (via "!= nullptr") in a way compatible with older C++ standards, as they are of the form "if (bool * p = ...)", and can be rewritten to "if (bool * p = ...; p != nullptr)" only for recent C++

• one false positive in system-provided /usr/include/qt5/QtCore/qstring.h

• no true positives found

[gparker42]

Note that we recently relaxed a similar diagnostic for NSNumber * in the static analyzer. Such code is semantically similar to inttype *.
https://reviews.llvm.org/D44044

[thakis]

include/clang/Basic/DiagnosticSemaKinds.td

7806

The guideline is actually "most warnings should be high-value and have next to no false positives", and because of that most warnings should be default-on. We don't have a clear guideline when to put warnings under -Wall instead of making them default-on, but sometimes "does the warning need to build a CFG?" is used for that. Almost nothing should be in Extra but not in All.

[lebedev.ri]

include/clang/Basic/DiagnosticSemaKinds.td

7806

The point i was trying to make:
if the warning is under a global umbrella flag only (All, Extra),
it can't be selectively disabled. And that is not good regardless.

[hiraditya]

Warn on bool* to bool conversion during a call only.

[efriedma]

lib/Sema/SemaChecking.cpp

2623

Have you considered skipping the isBooleanType() check? Passing any pointer to a function which expects a boolean parameter seems fundamentally suspicious.

[• Quuxplusone]

lib/Sema/SemaChecking.cpp

2623

We already know that LLVM itself passes pointers to functions in many places, and they're not bugs. I mean, this isn't the first time someone's proposed that pointer->bool conversions are evil. :)

I know this just came up on a mailing list somewhere recently (rsmith was involved in the rebuttal, as was I), but all I can find on Google is:
https://groups.google.com/a/isocpp.org/d/msg/std-proposals/a3g9qXFVGCs/Wj40qj48H3wJ
Anyway, the punch line I'm thinking of is something like

FooBar *FB = ...;
LLVMAssert(FB, "expected fb to be set");

but with whatever the real names are. Found a bunch of those when I wrote the diagnostic. And they're fairly solidly in the "not bugs" category.

[efriedma]

The thing about the bool*-only version is that bool pointers are rare in C++, so I'm not sure we're gaining much. But if we can't do something more general, there's still some benefit.

I see your point about false positives for the more general version. I was sort of considering an attribute to allow implicit conversions to bool for a specific function (like an assertion), and then we could ban implicit conversions to bool for parameters to other functions. But that's probably overkill.

[• Quuxplusone]

lib/Sema/SemaChecking.cpp

2623

The thing about the bool*-only version is that bool pointers are rare in C++, so I'm not sure we're gaining much. But if we can't do something more general, there's still some benefit.
I see your point about false positives for the more general version. I was sort of considering an attribute to allow implicit conversions to bool for a specific function (like an assertion), and then we could ban implicit conversions to bool for parameters to other functions. But that's probably overkill.

Based on @sberg's feedback that this warning found no true positives, and @Eugene.Zelenko's feedback that this warning already exists in clang-tidy today, personally I favor the resolution of "leave this warning in clang-tidy and abandon the attempt to move it into clang".

[aaron.ballman]

lib/Sema/SemaChecking.cpp

2623

... personally I favor the resolution of "leave this warning in clang-tidy and abandon the attempt to move it into clang".

This is my view as well.

[hiraditya]

The bug which motivated this warning is: https://github.com/jemalloc/jemalloc/commit/4df483f0fd76a64e116b1c4f316f8b941078114d#diff-7b26b977303fe92c093a2245b0eaf255

[rjmccall]

I agree that this is one of those warnings that just can't be done in any reasonable way.

[hiraditya]

It appears this warning may not be always useful because there will be false positives.

include/clang/Basic/DiagnosticSemaKinds.td

   "format specifies type %0 but the argument has "
   "%select{type|underlying type}2 %1">,
   InGroup<FormatPedantic>;
+def warn_pointer_to_bool : Warning<
+  "passing %0 as a boolean">,
+  InGroup<PointerBoolConversion>;
 def warn_format_argument_needs_cast : Warning<
   "%select{values of type|enum values with underlying type}2 '%0' should not "
   "be used as format arguments; add an explicit cast to %1 instead">,

lib/Sema/SemaChecking.cpp

   }
 }
 
+/// Diagnose an implicit cast;  purely a helper for CheckImplicitConversion.
+void DiagnoseImpCast(Sema &S, const Expr *E, QualType SourceType, QualType T,
+                     SourceLocation CContext, unsigned diag,
+                     bool pruneControlFlow = false) {
+  if (pruneControlFlow) {
+    S.DiagRuntimeBehavior(E->getExprLoc(), E,
+                          S.PDiag(diag)
+                            << SourceType << T << E->getSourceRange()
+                            << SourceRange(CContext));
+    return;
+  }
+  S.Diag(E->getExprLoc(), diag)
+    << SourceType << T << E->getSourceRange() << SourceRange(CContext);
+}
+
+/// Diagnose an implicit cast;  purely a helper for CheckImplicitConversion.
+void DiagnoseImpCast(Sema &S, const Expr *E, QualType T, SourceLocation CContext,
+                     unsigned diag, bool pruneControlFlow = false) {
+  DiagnoseImpCast(S, E, E->getType(), T, CContext, diag, pruneControlFlow);
+}
+
 /// Handles the checks for format strings, non-POD arguments to vararg
 /// functions, NULL arguments passed to non-NULL parameters, and diagnose_if
 /// attributes.

     }
   }
 
+  if (FD) {
+    for (unsigned ArgIdx = 0; ArgIdx < Args.size(); ++ArgIdx) {
+      // Args[ArgIdx] can be null in malformed code.
+      if (const Expr *Arg = Args[ArgIdx]) {
+        if (auto C = dyn_cast<ImplicitCastExpr>(Arg)) {
+          if (C->getCastKind() == CK_PointerToBoolean) {
+            if (auto ICast = dyn_cast<ImplicitCastExpr>(C->getSubExpr())) {
+              if (ICast->getCastKind() == CK_LValueToRValue) {
+                const Expr *Pointer = ICast->getSubExpr();
+                QualType QT = Pointer->getType()->getPointeeType();
+                if (!QT.isNull() && QT->isBooleanType())
+                  // Warn on bool* to bool conversion.
+                  DiagnoseImpCast(*this, Pointer, QT, Arg->getLocStart(),
+                                  diag::warn_pointer_to_bool);
+              }
+            }
+          }
+        }
+      }
+    }
+  }
+
   if (FDecl || Proto) {
     CheckNonNullArguments(*this, FDecl, Proto, Args, Loc);
 

   AnalyzeImplicitConversions(S, E->getRHS(), E->getOperatorLoc());
 }
 
-/// Diagnose an implicit cast;  purely a helper for CheckImplicitConversion.
-void DiagnoseImpCast(Sema &S, Expr *E, QualType SourceType, QualType T, 
-                     SourceLocation CContext, unsigned diag,
-                     bool pruneControlFlow = false) {
-  if (pruneControlFlow) {
-    S.DiagRuntimeBehavior(E->getExprLoc(), E,
-                          S.PDiag(diag)
-                            << SourceType << T << E->getSourceRange()
-                            << SourceRange(CContext));
-    return;
-  }
-  S.Diag(E->getExprLoc(), diag)
-    << SourceType << T << E->getSourceRange() << SourceRange(CContext);
-}
-
-/// Diagnose an implicit cast;  purely a helper for CheckImplicitConversion.
-void DiagnoseImpCast(Sema &S, Expr *E, QualType T, SourceLocation CContext,
-                     unsigned diag, bool pruneControlFlow = false) {
-  DiagnoseImpCast(S, E, E->getType(), T, CContext, diag, pruneControlFlow);
-}
-
 
 /// Diagnose an implicit cast from a floating point value to an integer value.
 void DiagnoseFloatingImpCast(Sema &S, Expr *E, QualType T,

test/SemaCXX/warn-bool-ptr-to-bool.cpp

+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+int foo(bool *b) {
+  if (b) // no-warning
+    return 10;
+  return 0;
+}
+
+int bar(bool b) {
+  return b;
+}
+
+int baz() {
+  bool *b;
+  bar(b);
+  // expected-warning@-1 {{passing 'bool *' as a boolean}}
+  return 0;
+}
+
+template<class T>
+T foo1(T *ptr) {
+  return ptr ? *ptr : T{}; // no-warning
+}
+
+bool bar1(bool *ptr) {
+  return foo1(ptr);
+}
+