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

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

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

5

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

 def warn_format_conversion_argument_type_mismatch : Warning<
   "format specifies type %0 but the argument has "
   "%select{type|underlying type}2 %1">,
   InGroup<Format>;
 def warn_format_conversion_argument_type_mismatch_pedantic : Extension<
   "format specifies type %0 but the argument has "
   "%select{type|underlying type}2 %1">,
   InGroup<FormatPedantic>;
+def warn_pointer_to_bool : Warning<
+  "comparing %0 as a boolean">,
+  InGroup<Extra>;
 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">,
   InGroup<Format>;
 def warn_printf_positional_arg_exceeds_data_args : Warning <
   "data argument position '%0' exceeds the number of data arguments (%1)">,
   InGroup<Format>;
 def warn_format_zero_positional_specifier : Warning<

lib/Sema/SemaChecking.cpp

       return DiagnoseImpCast(S, E, T, CC,
                              diag::warn_impcast_objective_c_literal_to_bool);
     }
     if (Source->isPointerType() || Source->canDecayToPointerType()) {
       // Warn on pointer to bool conversion that is always true.
       S.DiagnoseAlwaysNonNullPointer(E, Expr::NPCK_NotNull, /*IsEqual*/ false,
                                      SourceRange(CC));
     }
+    if (Source->isPointerType() || Source->canDecayToPointerType()) {
+      QualType PointeeType = Source->getPointeeType();
+      // Warn on bool* to bool conversion.
+      if (!PointeeType.isNull() && PointeeType->isBooleanType())
+        return DiagnoseImpCast(S, E, T, CC, diag::warn_pointer_to_bool);
+    }
   }
 
   // Check implicit casts from Objective-C collection literals to specialized
   // collection types, e.g., NSArray<NSString *> *.
   if (auto *ArrayLiteral = dyn_cast<ObjCArrayLiteral>(E))
     checkObjCArrayLiteral(S, QualType(Target, 0), ArrayLiteral);
   else if (auto *DictionaryLiteral = dyn_cast<ObjCDictionaryLiteral>(E))
     checkObjCDictionaryLiteral(S, QualType(Target, 0), DictionaryLiteral);

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

 
 // -- unscoped enumeration
 enum { E, F };
 
 bool b4 = !E;
 bool b5 = !F;
 
 // --  pointer, 
 bool b6 = !&b4; // expected-warning{{address of 'b4' will always evaluate to 'true'}}
+// expected-warning@-1 {{comparing 'bool *' as a boolean}}
 void f();
 bool b61 = !&f;
 
 // -- or pointer to member type can be converted to a prvalue of type bool.
 struct S { void f() { } };
 
 bool b7 = !&S::f;
 

test/Sema/static-init.c

 // RUN: %clang_cc1 -fsyntax-only -verify -Wno-bool-conversion %s
 
 typedef __typeof((int*) 0 - (int*) 0) intptr_t;
 
 static int f = 10;
 static int b = f; // expected-error {{initializer element is not a compile-time constant}}
 
 float r  = (float) (intptr_t) &r; // expected-error {{initializer element is not a compile-time constant}}
 intptr_t s = (intptr_t) &s;
-_Bool t = &t;
+_Bool t = &t; // expected-warning {{comparing '_Bool *' as a boolean}}
 
 
 union bar {
   int i;
 };
 
 struct foo {
   short ptr;
 };
 
 union bar u[1];
 struct foo x = {(intptr_t) u}; // expected-error {{initializer element is not a compile-time constant}}
 struct foo y = {(char) u}; // expected-error {{initializer element is not a compile-time constant}}
 intptr_t z = (intptr_t) u; // no-error

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

+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+int foo(bool *b) {
+  if (b)
+    // expected-warning@-1 {{comparing 'bool *' as a boolean}}
+    return 10;
+  return 0;
+}
+
+
+int bar(bool b) {
+  return b;
+}
+
+int baz() {
+  bool *b;
+  bar(b);
+  // expected-warning@-1 {{comparing 'bool *' as a boolean}}
+  return 0;
+}
+
+
+