[SemaObjC] Add a warning for declarations of BOOL:1 when BOOL is a signed char

[erik.pilkington]

Summary

Instead, recommend using a real boolean type. Its possible to get away with BOOL:1, if you only read from it in a context where its contextually converted to bool, but its still broken if you, for instance, store it into a BOOL variable or try to compare it with another BOOL. Given that, I don't think there is any good reason to use it when there are better alternatives available.

rdar://29707989

[rjmccall]

This feels like it's going to bite a lot of people, but maybe it's the best option.

clang/include/clang/Basic/DiagnosticGroups.td

   ]>;
 
 def ObjCStringComparison : DiagGroup<"objc-string-compare">;
 def ObjCStringConcatenation : DiagGroup<"objc-string-concatenation">;
 def ObjCLiteralComparison : DiagGroup<"objc-literal-compare", [
     ObjCStringComparison
   ]>;
 
+def ObjCSignedCharBoolBitfield : DiagGroup<"objc-signed-char-bool-bitfield">;
 def ObjCSignedCharBool : DiagGroup<"objc-signed-char-bool",
   [ObjCSignedCharBoolImplicitIntConversion,
    ObjCSignedCharBoolImplicitFloatConversion,
    ObjCBoolConstantConversion,
-   TautologicalObjCBoolCompare]>;
+   TautologicalObjCBoolCompare,
+   ObjCSignedCharBoolBitfield]>;
 
 // Inline ASM warnings.
 def ASMOperandWidths : DiagGroup<"asm-operand-widths">;
 def ASM : DiagGroup<"asm", [
     ASMOperandWidths
   ]>;
 
 // OpenMP warnings.

clang/include/clang/Basic/DiagnosticSemaKinds.td

   "negative enumerators of enum %1 will be converted to positive values">,
   InGroup<BitFieldEnumConversion>, DefaultIgnore;
 def warn_signed_bitfield_enum_conversion : Warning<
   "signed bit-field %0 needs an extra bit to represent the largest positive "
   "enumerators of %1">,
   InGroup<BitFieldEnumConversion>, DefaultIgnore;
 def note_change_bitfield_sign : Note<
   "consider making the bitfield type %select{unsigned|signed}0">;
+def warn_objc_signed_char_bool_bitfield : Warning<
+  "BOOL bit-field of width 1 cannot represent YES when BOOL "
+  "is a signed type">,
+  InGroup<ObjCSignedCharBoolBitfield>;
 
 def warn_missing_braces : Warning<
   "suggest braces around initialization of subobject">,
   InGroup<MissingBraces>, DefaultIgnore;
 
 def err_redefinition_of_label : Error<"redefinition of label %0">;
 def err_undeclared_label_use : Error<"use of undeclared label %0">;
 def err_goto_ms_asm_label : Error<

clang/include/clang/Sema/Sema.h

   ExprResult VerifyIntegerConstantExpression(Expr *E, llvm::APSInt *Result,
                                              unsigned DiagID,
                                              bool AllowFold = true);
   ExprResult VerifyIntegerConstantExpression(Expr *E,
                                              llvm::APSInt *Result = nullptr);
 
   /// VerifyBitField - verifies that a bit field expression is an ICE and has
   /// the correct width, and that the field type is valid.
-  /// Returns false on success.
+  /// Returns BitWidth on success.
   /// Can optionally return whether the bit-field is of width 0
-  ExprResult VerifyBitField(SourceLocation FieldLoc, IdentifierInfo *FieldName,
+  ExprResult VerifyBitField(SourceLocation FieldLoc, SourceRange TypeRange,
+                            IdentifierInfo *FieldName,
                             QualType FieldTy, bool IsMsStruct,
                             Expr *BitWidth, bool *ZeroWidth = nullptr);
 
 private:
   unsigned ForceCUDAHostDeviceDepth = 0;
 
 public:
   /// Increments our count of the number of times we've seen a pragma forcing

clang/lib/Sema/SemaDecl.cpp

   // ActOnStartCXXMemberDeclarations, so we don't have to mess with
   // the FieldCollector.
 
   PopDeclContext();
 }
 
 // Note that FieldName may be null for anonymous bitfields.
 ExprResult Sema::VerifyBitField(SourceLocation FieldLoc,
+                                SourceRange TypeRange,
                                 IdentifierInfo *FieldName,
                                 QualType FieldTy, bool IsMsStruct,
                                 Expr *BitWidth, bool *ZeroWidth) {
   assert(BitWidth);
   if (BitWidth->containsErrors())
     return ExprError();
 
   // Default to true; that shouldn't confuse checks for emptiness

       if (FieldName)
         Diag(FieldLoc, diag::warn_bitfield_width_exceeds_type_width)
             << FieldName << (unsigned)Value.getZExtValue()
             << (unsigned)TypeWidth;
       else
         Diag(FieldLoc, diag::warn_anon_bitfield_width_exceeds_type_width)
             << (unsigned)Value.getZExtValue() << (unsigned)TypeWidth;
     }
+
+    if (getLangOpts().ObjC && NSAPIObj->isObjCBOOLType(FieldTy) &&
+        FieldTy->isSpecificBuiltinType(BuiltinType::SChar) &&
+        Value == 1) {
+      auto Builder = Diag(FieldLoc, diag::warn_objc_signed_char_bool_bitfield);
+      // The Foundation headers that define BOOL include <stdbool.h>, so its
+      // reasonable to assume that 'bool' is available.
+      if (TypeRange.isValid())
+        Builder << FixItHint::CreateReplacement(TypeRange, "bool");
+    }
   }
 
   return BitWidth;
 }
 
 /// ActOnField - Each field of a C struct/union is passed into this in order
 /// to create a FieldDecl object for it.
 Decl *Sema::ActOnField(Scope *S, Decl *TagD, SourceLocation DeclStart,

                                              AbstractFieldType))
     InvalidDecl = true;
 
   bool ZeroWidth = false;
   if (InvalidDecl)
     BitWidth = nullptr;
   // If this is declared as a bit-field, check the bit-field.
   if (BitWidth) {
-    BitWidth = VerifyBitField(Loc, II, T, Record->isMsStruct(Context), BitWidth,
+    BitWidth = VerifyBitField(Loc, TInfo->getTypeLoc().getSourceRange(),
+                              II, T, Record->isMsStruct(Context), BitWidth,
                               &ZeroWidth).get();
     if (!BitWidth) {
       InvalidDecl = true;
       BitWidth = nullptr;
       ZeroWidth = false;
     }
   }
 

   // FIXME: Unnamed fields can be handled in various different ways, for
   // example, unnamed unions inject all members into the struct namespace!
 
   TypeSourceInfo *TInfo = GetTypeForDeclarator(D, S);
   QualType T = TInfo->getType();
 
   if (BitWidth) {
     // 6.7.2.1p3, 6.7.2.1p4
-    BitWidth = VerifyBitField(Loc, II, T, /*IsMsStruct*/false, BitWidth).get();
+    BitWidth = VerifyBitField(Loc, TInfo->getTypeLoc().getSourceRange(),
+                              II, T, /*IsMsStruct*/false, BitWidth).get();
     if (!BitWidth)
       D.setInvalidType();
   } else {
     // Not a bitfield.
 
     // validate II.
 
   }

clang/test/SemaObjC/signed-char-bool-bitfield-fixit.m

+// RUN: %clang_cc1 -Wobjc-signed-char-bool %s -fdiagnostics-parseable-fixits 2>&1 | FileCheck %s
+
+typedef signed char BOOL;
+
+struct S {
+  BOOL bf : 1;
+  // CHECK: fix-it:"{{.*}}.m":{6:3-6:7}:"bool"
+};
+
+@interface X {
+  BOOL bf : 1;
+  // CHECK: fix-it:"{{.*}}.m":{11:3-11:7}:"bool"
+}
+@end

clang/test/SemaObjC/signed-char-bool-bitfield.m

+// RUN: %clang_cc1 -verify %s -Wobjc-signed-char-bool
+// RUN: %clang_cc1 -xobjective-c++ -verify %s -Wobjc-signed-char-bool
+
+typedef signed char BOOL;
+
+struct S {
+  BOOL b : 1; // expected-warning {{BOOL bit-field of width 1 cannot represent YES when BOOL is a signed type}}
+  BOOL b2 : 2;
+};
+
+@interface X {
+  BOOL b : 1; // expected-warning {{BOOL bit-field of width 1 cannot represent YES when BOOL is a signed type}}
+  BOOL b2 : 2;
+}
+@end