Improve diagnostics for literal conversion to Boolean

[aaron.ballman]

Summary

This patch improves the literal conversion diagnostics where the target type is a Boolean by handling literals more consistently, and telling the user whether the resulting value will be true or false.

This patch changes the behavior of literal conversions from floating-point literals in two ways: (1) we now always diagnose instead of silently accepting conversions where the float can be converted to an exact integer value, and (2) we drop the information about what the source value of the floating-point literal is (it's unimportant information in the context of the Boolean conversion).

This patch removes the string literal conversion diagnostic (and -Wstring-conversion) which is off by default, and rolls its functionality into -Wliteral-conversion, which is on by default. Since we already handle string literal to bool conversion through logical && to not diagnose, I do not believe this will increase the chattiness of the diagnostic. In fact, turning the diagnostic to on by default caught a few tests that were accidentally using string->bool literal conversions. This does change the wording of the previous diagnostic, but I think the new wording is an improvement.

This patch now diagnoses character literal conversions to Boolean types where they used to be silently accepted.

[aaron.ballman]

Ping

[manmanren]

Thanks for working on this. This looks good to me overall, but I am not the expert on Sema :]

Cheers,
Manman

lib/Sema/SemaChecking.cpp

6985

Why do we remove the pretty printing for boolean here? Providing context here will be nice.

[aaron.ballman]

lib/Sema/SemaChecking.cpp

6985

Because float->Boolean conversion has moved elsewhere (to line 7207 in the new file).

[aaron.ballman]

Ping

[aaron.ballman]

Ping. Rebased on ToT.

[aaron.ballman]

Post-holiday ping.

[rsmith]

include/clang/Basic/DiagnosticSemaKinds.td

3147–3149

Warning groups are cheap, and fine-grained groups give users a way to incrementally roll out our new improved warnings. Can we retain the string-conversion group and add a new warning flag for the new cases (both as subgroups of literal-conversion), rather than rolling them into a single group?

I think this diagnostic text is worse for the string literal case than the old text: the type of the string literal is not relevant to the warning, and displaying it seems confusing. For consistency, something like "implicit conversion from string literal to 'bool'; will always evaluate to 'true'" would work.

Also, please include the word "literal" in the version of this warning for floating-point and character literals. (And actually, maybe using the "implicit conversion from floating-point literal to 'bool' [...]" formulation would be clearer there too, rather than listing a largely-irrelevant type.)

include/clang/Basic/DiagnosticGroups.td

 def ConstantConversion :
   DiagGroup<"constant-conversion", [ BitFieldConstantConversion ] >;
 def LiteralConversion : DiagGroup<"literal-conversion">;
-def StringConversion : DiagGroup<"string-conversion">;
 def SignConversion : DiagGroup<"sign-conversion">;
 def PointerBoolConversion : DiagGroup<"pointer-bool-conversion">;
 def UndefinedBoolConversion : DiagGroup<"undefined-bool-conversion">;

                             NonLiteralNullConversion, // (1-1)->pointer (etc)
                             NullConversion, // NULL->non-pointer
                             ObjCLiteralConversion,
-                            SignConversion,
-                            StringConversion]>,
+                            SignConversion]>,
                  DiagCategory<"Value Conversion Issue">;
 
 def Unused : DiagGroup<"unused",

include/clang/Basic/DiagnosticSemaKinds.td

   "implicit conversion from %0 to %1 changes non-zero value from %2 to %3">,
   InGroup<FloatZeroConversion>, DefaultIgnore;
 
-def warn_impcast_string_literal_to_bool : Warning<
-  "implicit conversion turns string literal into bool: %0 to %1">,
-  InGroup<StringConversion>, DefaultIgnore;
+def warn_impcast_literal_to_bool : Warning<
+  "implicit conversion from %0 to %1; will always evaluate to "
+  "'%select{false|true}2'">, InGroup<LiteralConversion>;
 def warn_impcast_different_enum_types : Warning<
   "implicit conversion from enumeration type %0 to different enumeration type "
   "%1">, InGroup<EnumConversion>;

lib/Sema/SemaChecking.cpp

   Value.toString(PrettySourceValue, precision);
 
   SmallString<16> PrettyTargetValue;
-  if (IsBool)
-    PrettyTargetValue = Value.isZero() ? "false" : "true";
-  else
-    IntegerValue.toString(PrettyTargetValue);
+  IntegerValue.toString(PrettyTargetValue);
 
   if (PruneWarnings) {
     S.DiagRuntimeBehavior(E->getExprLoc(), E,

 
   // Diagnose implicit casts to bool.
   if (Target->isSpecificBuiltinType(BuiltinType::Bool)) {
-    if (isa<StringLiteral>(E))
-      // Warn on string literal to bool.  Checks for string literals in logical
-      // and expressions, for instance, assert(0 && "error here"), are
-      // prevented by a check in AnalyzeImplicitConversions().
-      return DiagnoseImpCast(S, E, T, CC,
-                             diag::warn_impcast_string_literal_to_bool);
+    if (isa<StringLiteral>(E)) {
+      // Warn on string literal to bool. Checks for string literals in logical
+      // AND expressions, for instance, assert(0 && "error here"), are
+      // prevented by a check in AnalyzeImplicitConversions(). All string
+      // literal implicit conversions to bool evaluate to true.
+      S.Diag(E->getLocStart(), diag::warn_impcast_literal_to_bool)
+          << E->getType() << T << true << E->getSourceRange()
+          << SourceRange(CC);
+      return;
+    }
+    if (const auto *CharLit = dyn_cast<CharacterLiteral>(E)) {
+      // Warn on char literal to bool.
+      S.Diag(CharLit->getLocation(), diag::warn_impcast_literal_to_bool)
+          << E->getType() << T << (bool)CharLit->getValue()
+          << E->getSourceRange() << SourceRange(CC);
+      return;
+    }
+    if (const auto *FL = dyn_cast<FloatingLiteral>(E)) {
+      // When converting from a floating-point literal to boolean, always
+      // diagnose, even if the literal will exactly convert to an interger.
+      S.Diag(FL->getExprLoc(), diag::warn_impcast_literal_to_bool)
+          << FL->getType() << T.getUnqualifiedType() << !FL->getValue().isZero()
+          << FL->getSourceRange() << SourceRange(CC);
+      return;
+    }
     if (isa<ObjCStringLiteral>(E) || isa<ObjCArrayLiteral>(E) ||
         isa<ObjCDictionaryLiteral>(E) || isa<ObjCBoxedExpr>(E)) {
       // This covers the literal expressions that evaluate to Objective-C

test/Analysis/casts.c

-// RUN: %clang_analyze_cc1 -triple x86_64-apple-darwin9 -analyzer-checker=core,alpha.core,debug.ExprInspection -analyzer-store=region -verify %s
-// RUN: %clang_analyze_cc1 -triple i386-apple-darwin9 -analyzer-checker=core,alpha.core,debug.ExprInspection -analyzer-store=region -verify %s
+// RUN: %clang_analyze_cc1 -triple x86_64-apple-darwin9 -Wno-literal-conversion -analyzer-checker=core,alpha.core,debug.ExprInspection -analyzer-store=region -verify %s
+// RUN: %clang_analyze_cc1 -triple i386-apple-darwin9 -Wno-literal-conversion -analyzer-checker=core,alpha.core,debug.ExprInspection -analyzer-store=region -verify %s
 
 extern void clang_analyzer_eval(_Bool);
 
 // Test if the 'storage' region gets properly initialized after it is cast to
-// 'struct sockaddr *'. 
+// 'struct sockaddr *'.
 
 typedef unsigned char __uint8_t;
 typedef unsigned int __uint32_t;

test/CXX/dcl.decl/dcl.init/dcl.init.list/p7-0x.cpp

-// RUN: %clang_cc1 -fsyntax-only -std=c++11 -triple x86_64-apple-macosx10.6.7 -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wno-literal-conversion -std=c++11 -triple x86_64-apple-macosx10.6.7 -verify %s
 
 // Verify that narrowing conversions in initializer lists cause errors in C++0x
 // mode.

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

 
 bool b = !0;
 
-bool b2 = !1.2; //expected-warning{{implicit conversion from 'double' to 'bool' changes value from 1.2 to true}}
+bool b2 = !1.2; //expected-warning{{implicit conversion from 'double' to 'bool'; will always evaluate to 'true'}}
 
 bool b3 = !4;
 

test/Sema/exprs.c

 
 // Make sure we do function/array decay.
 void test22() {
-  if ("help")
+  if ("help") // expected-warning {{implicit conversion from 'char [5]' to '_Bool'; will always evaluate to 'true'}}
     (void) 0;
 
   if (test22) // expected-warning {{address of function 'test22' will always evaluate to 'true'}} \

test/Sema/warn-string-conversion.c

-// RUN: %clang_cc1 -verify -fsyntax-only -Wstring-conversion %s
+// RUN: %clang_cc1 -verify -fsyntax-only %s
 
 void do_nothing();
 void assert_error();

 void test1() {
   assert1(0 && "foo");
   assert1("foo" && 0);
-  assert1(0 || "foo"); // expected-warning {{string literal}}
-  assert1("foo"); // expected-warning {{string literal}}
+  assert1(0 || "foo"); // expected-warning {{implicit conversion from 'char [4]' to '_Bool'; will always evaluate to 'true'}}
+  assert1("foo"); // expected-warning {{implicit conversion from 'char [4]' to '_Bool'; will always evaluate to 'true'}}
 
   assert2(0 && "foo");
   assert2("foo" && 0);
-  assert2(0 || "foo"); // expected-warning {{string literal}}
-  assert2("foo"); // expected-warning {{string literal}}
+  assert2(0 || "foo"); // expected-warning {{implicit conversion from 'char [4]' to '_Bool'; will always evaluate to 'true'}}
+  assert2("foo"); // expected-warning {{implicit conversion from 'char [4]' to '_Bool'; will always evaluate to 'true'}}
 }
 
 void test2() {
-  if ("hi") {}           // expected-warning {{string literal}}
-  while ("hello") {}     // expected-warning {{string literal}}
-  for (;"howdy";) {}     // expected-warning {{string literal}}
-  do { } while ("hey");  // expected-warning {{string literal}}
-  int x = "hey" ? 1 : 2; // expected-warning {{string literal}}
+  if ("hi") {}           // expected-warning {{implicit conversion from 'char [3]' to '_Bool'; will always evaluate to 'true'}}
+  while ("hello") {}     // expected-warning {{implicit conversion from 'char [6]' to '_Bool'; will always evaluate to 'true'}}
+  for (;"howdy";) {}     // expected-warning {{implicit conversion from 'char [6]' to '_Bool'; will always evaluate to 'true'}}
+  do { } while ("hey");  // expected-warning {{implicit conversion from 'char [4]' to '_Bool'; will always evaluate to 'true'}}
+  int x = "hey" ? 1 : 2; // expected-warning {{implicit conversion from 'char [4]' to '_Bool'; will always evaluate to 'true'}}
 }

test/SemaCXX/condition.cpp

 
 // Make sure we do function/array decay.
 void test3() {
-  if ("help")
+  if ("help") // expected-warning {{implicit conversion from 'const char [5]' to 'bool'; will always evaluate to 'true'}}
     (void) 0;
 
   if (test3) // expected-warning {{address of function 'test3' will always evaluate to 'true'}} \

test/SemaCXX/overload-call.cpp

 #else
   // expected-error@-4 {{cannot initialize a variable of type 'int *' with an rvalue of type 'double *'}}
 #endif
-  double* dp1 = k(L"foo");
+  double* dp1 = k(L"foo"); // expected-warning {{implicit conversion from 'const wchar_t [4]' to 'bool'; will always evaluate to 'true'}}
 }
 
 int* l(wchar_t*);

 #else
   // expected-error@-4 {{cannot initialize a variable of type 'int *' with an rvalue of type 'double *'}}
 #endif
-  double* dp1 = l("foo");
+  double* dp1 = l("foo"); // expected-warning {{implicit conversion from 'const char [4]' to 'bool'; will always evaluate to 'true'}}
 }
 
 int* m(const char*);

test/SemaCXX/warn-literal-conversion.cpp

-// RUN: %clang_cc1 -fsyntax-only -Wliteral-conversion -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wliteral-conversion -std=c++1z -verify %s
 
 void foo(int y);
 

 // Similarly, test floating point conversion to bool. Only float values of zero
 // are converted to false; everything else is converted to true.
 void test1() {
-  bool b1 = 0.99f; // expected-warning {{implicit conversion from 'float' to 'bool' changes value from 0.99 to true}}
-  bool b2 = 0.99; // expected-warning {{implicit conversion from 'double' to 'bool' changes value from 0.99 to true}}
-  // These do not warn because they can be directly converted to integral
-  // values.
-  bool b3 = 0.0f;
-  bool b4 = 0.0;
+  bool b1 = 0.99f; // expected-warning {{implicit conversion from 'float' to 'bool'; will always evaluate to 'true'}}
+  bool b2 = 0.99; // expected-warning {{implicit conversion from 'double' to 'bool'; will always evaluate to 'true'}}
+  bool b3 = 0.0f; // expected-warning {{implicit conversion from 'float' to 'bool'; will always evaluate to 'false'}}
+  bool b4 = 0.0; // expected-warning {{implicit conversion from 'double' to 'bool'; will always evaluate to 'false'}}
+
+  bool b5 = "false"; // expected-warning {{implicit conversion from 'const char [6]' to 'bool'; will always evaluate to 'true'}}
+  bool b6 = L"false"; // expected-warning {{implicit conversion from 'const wchar_t [6]' to 'bool'; will always evaluate to 'true'}}
+  bool b7 = u"0"; // expected-warning {{implicit conversion from 'const char16_t [2]' to 'bool'; will always evaluate to 'true'}}
+  bool b8 = U"false"; // expected-warning {{implicit conversion from 'const char32_t [6]' to 'bool'; will always evaluate to 'true'}}
+  bool b9 = u8"0"; // expected-warning {{implicit conversion from 'const char [2]' to 'bool'; will always evaluate to 'true'}}
+
+  bool b10 = '\0'; // expected-warning {{implicit conversion from 'char' to 'bool'; will always evaluate to 'false'}}
+  bool b11 = L'1'; // expected-warning {{implicit conversion from 'wchar_t' to 'bool'; will always evaluate to 'true'}}
+  bool b12 = U'\0'; // expected-warning {{implicit conversion from 'char32_t' to 'bool'; will always evaluate to 'false'}}
+  bool b13 = u'1'; // expected-warning {{implicit conversion from 'char16_t' to 'bool'; will always evaluate to 'true'}}
+  bool b14 = u8'\0'; // expected-warning {{implicit conversion from 'char' to 'bool'; will always evaluate to 'false'}}
 }
+
+#define assert(x) (void)(!!(x))
+void should_not_diagnose() {
+  assert(1 && "false");
+  assert(1 && "some random string");
+}

test/SemaCXX/warn-string-conversion.cpp

-// RUN: %clang_cc1 -fsyntax-only -Wstring-conversion -verify %s
+// RUN: %clang_cc1 -fsyntax-only -verify %s
 
 // Warn on cases where a string literal is converted into a bool.
 // An exception is made for this in logical and operators.
 void assert(bool condition);
 void test0() {
-  bool b0 = "hi"; // expected-warning{{implicit conversion turns string literal into bool: 'const char [3]' to 'bool'}}
-  b0 = ""; // expected-warning{{implicit conversion turns string literal into bool: 'const char [1]' to 'bool'}}
-  b0 = 0 || ""; // expected-warning{{implicit conversion turns string literal into bool: 'const char [1]' to 'bool'}}
-  b0 = "" || 0; // expected-warning{{implicit conversion turns string literal into bool: 'const char [1]' to 'bool'}}
+  bool b0 = "hi"; // expected-warning{{implicit conversion from 'const char [3]' to 'bool'; will always evaluate to 'true'}}
+  b0 = ""; // expected-warning{{implicit conversion from 'const char [1]' to 'bool'; will always evaluate to 'true'}}
+  b0 = 0 || ""; // expected-warning{{implicit conversion from 'const char [1]' to 'bool'; will always evaluate to 'true'}}
+  b0 = "" || 0; // expected-warning{{implicit conversion from 'const char [1]' to 'bool'; will always evaluate to 'true'}}
   b0 = 0 && "";
   b0 = "" && 0;
-  assert("error"); // expected-warning{{implicit conversion turns string literal into bool: 'const char [6]' to 'bool'}}
-  assert(0 || "error"); // expected-warning{{implicit conversion turns string literal into bool: 'const char [6]' to 'bool'}}
-  assert("error" || 0); // expected-warning{{implicit conversion turns string literal into bool: 'const char [6]' to 'bool'}}
+  assert("error"); // expected-warning{{implicit conversion from 'const char [6]' to 'bool'; will always evaluate to 'true'}}
+  assert(0 || "error"); // expected-warning{{implicit conversion from 'const char [6]' to 'bool'; will always evaluate to 'true'}}
+  assert("error" || 0); // expected-warning{{implicit conversion from 'const char [6]' to 'bool'; will always evaluate to 'true'}}
   assert(0 && "error");
   assert("error" && 0);
 
-  while("hi") {} // expected-warning{{implicit conversion turns string literal into bool: 'const char [3]' to 'bool'}}
-  do {} while("hi"); // expected-warning{{implicit conversion turns string literal into bool: 'const char [3]' to 'bool'}}
-  for (;"hi";); // expected-warning{{implicit conversion turns string literal into bool: 'const char [3]' to 'bool'}}
-  if("hi") {} // expected-warning{{implicit conversion turns string literal into bool: 'const char [3]' to 'bool'}}
+  while("hi") {} // expected-warning{{implicit conversion from 'const char [3]' to 'bool'; will always evaluate to 'true'}}
+  do {} while("hi"); // expected-warning{{implicit conversion from 'const char [3]' to 'bool'; will always evaluate to 'true'}}
+  for (;"hi";); // expected-warning{{implicit conversion from 'const char [3]' to 'bool'; will always evaluate to 'true'}}
+  if("hi") {} // expected-warning{{implicit conversion from 'const char [3]' to 'bool'; will always evaluate to 'true'}}
 }
 

test/SemaCXX/warn-thread-safety-analysis.cpp

   struct Bas { void operator& (Foo &) {} };
   void mumble()
   {
-    Bas() & Bar().func() << "" << "";
-    Bas() & Bar().func() << "";
+    Bas() & Bar().func() << true << true;
+    Bas() & Bar().func() << true;
   }
 } // end namespace thread_annot_lock_61_modified