Objective-C literals to bool conversion warning

[rtrieu]

Summary

Warn when a Objective-C literal is converted to a boolean. This warns on NSNumber*, NSString*, NSArray* and NSDictionary* literals. Excludes objc_yes and objc_no.

I don't normally work with Objective-C so let me know if I am missing any test cases.

[jordan_rose]

Huh, I thought we already had a check for this. This seems fine, but we might as well test every Objective-C literal type. None of them can evaluate to nil...and then I'd rather group the @"" conversion with those, rather than -Wstring-conversion.

[rtrieu]

Is there a list of Objective C literal types that this new warning should cover?

[jordan_rose]

Besides strings, anything on this page: http://clang.llvm.org/docs/ObjectiveCLiterals.html

That corresponds to ObjCStringLiteral, ObjCBoxedExpr, ObjCArrayLiteral, and ObjCDictionaryLiteral...but not ObjCBoolLiteral (which is just __objc_yes and __objc_no). -Wobjc-literal-conversion, perhaps?

(I suppose you could throw ObjCSelectorExpr in there as well, though that one probably deserves custom text; the others are all "Objective-C object literals". ObjCEncodeExpr just expands to a C string, so that could go under -Wstring-conversion if you cared.)

[jordan_rose]

Remember, "Objective-C" is always hyphenated. :-)

include/clang/Basic/DiagnosticSemaKinds.td

2299–2301 ↗

(On Diff #6701)

Ideally I'd like something more like "implicit boolean conversion of %select{array literal|dictionary literal|numeric literal|boxed expression}0 always evaluates to true". Failing that, let's go with "Objective-C object literal".

lib/Sema/SemaChecking.cpp

5338–5339

There are a lot of "Objective-C types". How about "This covers the literal expressions that evaluate to Objective-C objects"?

[rtrieu]

Hyphenate Objective-C, fix warning text, fix comment text.

[jordan_rose]

Justification for dropping the "boolean" in "implicit boolean conversion"? I'm worried that a non-compiler-writer looking at the code won't see where the conversion is. (I didn't phrase it as "conversion...to bool" because Objective-C programmers don't usually think about "bool", even though they're probably compiling in gnu99 mode.)

[rtrieu]

Justification for dropping the "boolean" in "implicit boolean conversion"? I'm worried that a non-compiler-writer looking at the code won't see where the conversion is. (I didn't phrase it as "conversion...to bool" because Objective-C programmers don't usually think about "bool", even though they're probably compiling in gnu99 mode.)

Oops, dropped it while copying the text. I'll go fix the text.

[rtrieu]

"implicit conversion" -> "implicit boolean conversion"

[jordan_rose]

Looks good to me. I think we can even turn it on by default. Thanks, Richard!

[rtrieu]

Committed at r200356. Switched to on by default instead of DefaultIgnore. One analysis test required disabling this warning.

lib/Sema/SemaChecking.cpp

void CheckImplicitConversion(Sema &S, Expr *E, QualType T,
   // a system macro. The problem is that 'getSpellingLoc()' is slow, so we
   // delay this check as long as possible. Once we detect we are in that
   // scenario, we just return.
   if (CC.isInvalid())
     return;
 
   // Diagnose implicit casts to bool.
   if (Target->isSpecificBuiltinType(BuiltinType::Bool)) {
-    if (isa<StringLiteral>(E))
+    if (isa<StringLiteral>(E) || isa<ObjCStringLiteral>(E))
       // Warn on string literal to bool.  Checks for string literals in logical
       // expressions, for instances, 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 (Source->isFunctionType()) {
       // Warn on function to bool. Checks free functions and static member
       // functions. Weakly imported functions are excluded from the check,
       // since it's common to test their value to check whether the linker
       // found a definition for them.
       ValueDecl *D = 0;
       if (DeclRefExpr* R = dyn_cast<DeclRefExpr>(E)) {
         D = R->getDecl();
       } else if (MemberExpr *M = dyn_cast<MemberExpr>(E)) {
         D = M->getMemberDecl();
       }
 
void AnalyzeImplicitConversions(Sema &S, Expr *OrigE, SourceLocation CC) {
   BinaryOperator *BO = dyn_cast<BinaryOperator>(E);
   bool IsLogicalOperator = BO && BO->isLogicalOp();
   for (Stmt::child_range I = E->children(); I; ++I) {
     Expr *ChildExpr = dyn_cast_or_null<Expr>(*I);
     if (!ChildExpr)
       continue;
 
     if (IsLogicalOperator &&
-        isa<StringLiteral>(ChildExpr->IgnoreParenImpCasts()))
+        (isa<StringLiteral>(ChildExpr->IgnoreParenImpCasts()) ||
+         isa<ObjCStringLiteral>(ChildExpr->IgnoreParenImpCasts())))
       // Ignore checking string literals that are in logical operators.
       continue;
     AnalyzeImplicitConversions(S, ChildExpr, CC);
   }
 }
 
 } // end anonymous namespace
 

test/SemaObjCXX/warn-string-conversion.mm

+// RUN: %clang_cc1 -fsyntax-only -Wstring-conversion -verify %s
+
+// Warn on cases where a string literal is converted into a bool.
+// An exception is made for this in logical 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 && "";
+  assert("error"); // expected-warning{{implicit conversion turns string literal into bool: 'const char [6]' to 'bool'}}
+  assert(0 && "error");
+
+  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'}}
+}
+
+void test1() {
+  bool b0 = @"hi"; // expected-warning{{implicit conversion turns string literal into bool: 'NSString *' to 'bool'}}
+  b0 = @""; // expected-warning{{implicit conversion turns string literal into bool: 'NSString *' to 'bool'}}
+  b0 = 0 && @"";
+  assert(@"error"); // expected-warning{{implicit conversion turns string literal into bool: 'NSString *' to 'bool'}}
+  assert(0 && @"error");
+
+  while(@"hi") {} // expected-warning{{implicit conversion turns string literal into bool: 'NSString *' to 'bool'}}
+  do {} while(@"hi"); // expected-warning{{implicit conversion turns string literal into bool: 'NSString *' to 'bool'}}
+  for (;@"hi";); // expected-warning{{implicit conversion turns string literal into bool: 'NSString *' to 'bool'}}
+  if(@"hi") {} // expected-warning{{implicit conversion turns string literal into bool: 'NSString *' to 'bool'}}
+}
+