[ObjC] Add a warning for implicit conversions of a constant non-boolean value to BOOL

[erik.pilkington]

Summary

On macOS, BOOL is a typedef for signed char, but it should never hold a value that isn't 1 or 0. Any code that expects a different value in their BOOL should be fixed. See also D63856.

rdar://problem/51954400 ObjC: Add diagnostics to catch incorrect usage of BOOL

[erik.pilkington]

Ping!

[rjmccall]

clang/include/clang/Basic/DiagnosticSemaKinds.td

3251

The from seems awkward here; %0 is the number, not the type. (I agree that the type isn't important for the diagnostic.)

Thoughts about this being in a sub-group? I'm not sure either way.

[erik.pilkington]

Address review comments.

clang/include/clang/Basic/DiagnosticSemaKinds.td

3251

How about: "implicit conversion from constant value 42 to BOOL; ..."? I guess this is different enough from the other -Wconstant-conversion diagnostics to get a subgroup, so I added one in the new patch.

[rjmccall]

clang/include/clang/Basic/DiagnosticSemaKinds.td

3251

SGTM

clang/include/clang/Basic/DiagnosticGroups.td

def BinaryLiteral : DiagGroup<"binary-literal", [CXX14BinaryLiteral,
                                                  GNUBinaryLiteral]>;
 def GNUCompoundLiteralInitializer : DiagGroup<"gnu-compound-literal-initializer">;
 def BitFieldConstantConversion : DiagGroup<"bitfield-constant-conversion">;
 def BitFieldEnumConversion : DiagGroup<"bitfield-enum-conversion">;
 def BitFieldWidth : DiagGroup<"bitfield-width">;
 def CoroutineMissingUnhandledException :
   DiagGroup<"coroutine-missing-unhandled-exception">;
 def Coroutine : DiagGroup<"coroutine", [CoroutineMissingUnhandledException]>;
-def ConstantConversion :
-  DiagGroup<"constant-conversion", [ BitFieldConstantConversion ] >;
+def ObjCBoolConstantConversion : DiagGroup<"objc-bool-constant-conversion">;
+def ConstantConversion : DiagGroup<"constant-conversion",
+                                   [BitFieldConstantConversion,
+                                    ObjCBoolConstantConversion]>;
 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">;
 def BoolConversion : DiagGroup<"bool-conversion", [PointerBoolConversion,
                                                    UndefinedBoolConversion]>;
 def IntConversion : DiagGroup<"int-conversion">;

clang/include/clang/Basic/DiagnosticSemaKinds.td

   "implicit conversion loses integer precision: %0 to %1">,
   InGroup<Shorten64To32>, DefaultIgnore;
 def warn_impcast_integer_precision_constant : Warning<
   "implicit conversion from %2 to %3 changes value from %0 to %1">,
   InGroup<ConstantConversion>;
 def warn_impcast_bitfield_precision_constant : Warning<
   "implicit truncation from %2 to bit-field changes value from %0 to %1">,
   InGroup<BitFieldConstantConversion>;
+def warn_impcast_constant_int_to_objc_bool : Warning<
+  "implicit conversion from constant value %0 to BOOL; "
+  "the only well defined values for BOOL are YES and NO">,
+  InGroup<ObjCBoolConstantConversion>;
 
 def warn_impcast_fixed_point_range : Warning<
   "implicit conversion from %0 cannot fit within the range of values for %1">,
   InGroup<ImplicitFixedPointConversion>;
 
 def warn_impcast_literal_float_to_integer : Warning<
   "implicit conversion from %0 to %1 changes value from %2 to %3">,
   InGroup<LiteralConversion>;

clang/lib/Sema/SemaChecking.cpp

         S.getSourceManager().getCharacterData(CC)[0];
     if (FirstContextCharacter == '{')
       return false;
   }
 
   return true;
 }
 
+static bool isObjCSignedCharBool(Sema &S, QualType Ty) {
+  return Ty->isSpecificBuiltinType(BuiltinType::SChar) &&
+         S.getLangOpts().ObjC && S.NSAPIObj->isObjCBOOLType(Ty);
+}
+
 static void
 CheckImplicitConversion(Sema &S, Expr *E, QualType T, SourceLocation CC,
                         bool *ICContext = nullptr) {
   if (E->isTypeDependent() || E->isValueDependent()) return;
 
   const Type *Source = S.Context.getCanonicalType(E->getType()).getTypePtr();
   const Type *Target = S.Context.getCanonicalType(T).getTypePtr();
   if (Source == Target) return;

     }
     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 the we're converting a constant to an ObjC BOOL on a platform where BOOL
+  // is a typedef for signed char (macOS), then that constant value has to be 1
+  // or 0.
+  if (isObjCSignedCharBool(S, T) && Source->isIntegralType(S.Context)) {
+    Expr::EvalResult Result;
+    if (E->EvaluateAsInt(Result, S.getASTContext(),
+                         Expr::SE_AllowSideEffects) &&
+        Result.Val.getInt() != 1 && Result.Val.getInt() != 0) {
+      auto Builder = S.Diag(CC, diag::warn_impcast_constant_int_to_objc_bool)
+                     << Result.Val.getInt().toString(10);
+      Expr *Ignored = E->IgnoreImplicit();
+      bool NeedsParens = isa<AbstractConditionalOperator>(Ignored) ||
+                         isa<BinaryOperator>(Ignored) ||
+                         isa<CXXOperatorCallExpr>(Ignored);
+      SourceLocation EndLoc = S.getLocForEndOfToken(E->getEndLoc());
+      if (NeedsParens)
+        Builder << FixItHint::CreateInsertion(E->getBeginLoc(), "(")
+                << FixItHint::CreateInsertion(EndLoc, ")");
+      Builder << FixItHint::CreateInsertion(EndLoc, " ? YES : NO");
+      return;
+    }
+  }
+
   // 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);
 
   // Strip vector types.

clang/test/Sema/objc-bool-constant-conversion-fixit.m

+// RUN: %clang_cc1 -Werror=constant-conversion %s -fixit-recompile -fixit-to-temporary -E -o - | FileCheck %s
+
+typedef signed char BOOL;
+
+BOOL b;
+
+int main() {
+  BOOL b = 2;
+  // CHECK: BOOL b = 2 ? YES : NO;
+
+  b = b ? 2 : 1;
+  // CHECK: b = b ? 2 ? YES : NO : 1;
+
+  b = b ? 1 : 2;
+  // CHECK: b = b ? 1 : 2 ? YES : NO;
+
+  b = b ? 2 : 2;
+  // CHECK: b = b ? 2 ? YES : NO : 2 ? YES : NO;
+
+  b = 1 + 1;
+  // CHECK: b = (1 + 1) ? YES : NO;
+
+  b = 1 | 2;
+  // CHECK: b = (1 | 2) ? YES : NO;
+
+  b = 1 << 1;
+  // CHECK: b = (1 << 1) ? YES : NO;
+}
+
+@interface BoolProp
+@property BOOL b;
+@end
+
+void f(BoolProp *bp) {
+  bp.b = 43;
+  // CHECK: bp.b = 43 ? YES : NO;
+
+  [bp setB:43];
+  // CHECK: [bp setB:43 ? YES : NO];
+}

clang/test/Sema/objc-bool-constant-conversion.m

+// RUN: %clang_cc1 %s -verify -fsyntax-only
+
+typedef signed char BOOL;
+#define YES __objc_yes
+#define NO __objc_no
+
+BOOL B;
+
+int main() {
+  B = 0;
+  B = 1;
+  B = YES;
+  B = NO;
+
+  B = -1; // expected-warning{{implicit conversion from constant value -1 to BOOL; the only well defined values for BOOL are YES and NO}}
+  B = 0 - 1; // expected-warning{{implicit conversion from constant value -1 to BOOL; the only well defined values for BOOL are YES and NO}}
+  B = YES + YES; // expected-warning {{implicit conversion from constant value 2 to BOOL; the only well defined values for BOOL are YES and NO}}
+  B = YES | YES;
+
+  B = B ? 2 : 2; // expected-warning 2 {{implicit conversion from constant value 2 to BOOL; the only well defined values for BOOL are YES and NO}}
+
+  BOOL Init = -1; // expected-warning{{implicit conversion from constant value -1 to BOOL; the only well defined values for BOOL are YES and NO}}
+  BOOL Init2 = B ? 2 : 2; // expected-warning 2 {{implicit conversion from constant value 2 to BOOL; the only well defined values for BOOL are YES and NO}}
+
+  void takesbool(BOOL);
+  takesbool(43); // expected-warning {{implicit conversion from constant value 43 to BOOL; the only well defined values for BOOL are YES and NO}}
+
+  BOOL OutOfRange = 400; // expected-warning{{implicit conversion from constant value 400 to BOOL; the only well defined values for BOOL are YES and NO}}
+}
+
+@interface BoolProp
+@property BOOL b;
+@end
+
+void f(BoolProp *bp) {
+  bp.b = 43; // expected-warning {{implicit conversion from constant value 43 to BOOL; the only well defined values for BOOL are YES and NO}}
+  [bp setB:43]; // expected-warning {{implicit conversion from constant value 43 to BOOL; the only well defined values for BOOL are YES and NO}}
+}