[Sema] Split of versions of -Wimplicit-{float,int}-conversion for Objective-C BOOL

[erik.pilkington]

Summary

Also, add a diagnostic group, -Wobjc-signed-char-bool, to control all these related diagnostics.

See also: D63912, D63856

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

[steven_wu]

The diagnostics message looks good to me.

[aaron.ballman]

Mostly LGTM but a few nits.

clang/include/clang/Basic/DiagnosticGroups.td

65

This makes me wish that all shells would support autocomplete of program options. :-D

clang/include/clang/Basic/DiagnosticSemaKinds.td

3312

Can you put single quotes around BOOL since it's a type name? Same below.

clang/lib/AST/Expr.cpp

191

const auto * ?

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

4

Can you add a test verifying the fix-it behaviors (with and without parens)?

[erik.pilkington]

Address review comments.

clang/include/clang/Basic/DiagnosticSemaKinds.td

3312

Sure, this also needs to be fixed for warn_impcast_constant_value_to_objc_bool and warn_tautological_compare_objc_bool, I'll do that separately.

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

4

Sure, while testing this I noticed that we don't handle parens with OpaqueValueExprs properly, so I added a case.

[aaron.ballman]

LGTM.

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

37

What about:

b = 1 ? (2 ? 3 : 4) : 5;

Will it YES/NO all the way down?

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

48

'BOOL', 'YES', and 'NO' at some point (doesn't have to be in this patch).

[erik.pilkington]

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

37

Yeah, we produce the following for this:

b = 1 ? (2 ? 3 ? YES : NO : 4 ? YES : NO) : 5 ? YES : NO;

Not pretty, but not incorrect.

clang/include/clang/Basic/DiagnosticGroups.td

 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">;
 def EnumConversion : DiagGroup<"enum-conversion">;
-def ImplicitIntConversion : DiagGroup<"implicit-int-conversion">;
+def ObjCSignedCharBoolImplicitIntConversion :
+  DiagGroup<"objc-signed-char-bool-implicit-int-conversion">;
+def ImplicitIntConversion : DiagGroup<"implicit-int-conversion",
+                                     [ObjCSignedCharBoolImplicitIntConversion]>;
 def ImplicitIntFloatConversion : DiagGroup<"implicit-int-float-conversion">;
-def ImplicitFloatConversion : DiagGroup<"implicit-float-conversion", [ImplicitIntFloatConversion]>;
+def ObjCSignedCharBoolImplicitFloatConversion :
+  DiagGroup<"objc-signed-char-bool-implicit-float-conversion">;
+def ImplicitFloatConversion : DiagGroup<"implicit-float-conversion",
+  [ImplicitIntFloatConversion,
+   ObjCSignedCharBoolImplicitFloatConversion]>;
 def ImplicitFixedPointConversion : DiagGroup<"implicit-fixed-point-conversion">;
 
 def FloatOverflowConversion : DiagGroup<"float-overflow-conversion">;
 def FloatZeroConversion : DiagGroup<"float-zero-conversion">;
 def FloatConversion :
   DiagGroup<"float-conversion", [FloatOverflowConversion,
                                  FloatZeroConversion]>;
 
def ObjCCocoaAPI : DiagGroup<"objc-cocoa-api", [
   ]>;
 
 def ObjCStringComparison : DiagGroup<"objc-string-compare">;
 def ObjCStringConcatenation : DiagGroup<"objc-string-concatenation">;
 def ObjCLiteralComparison : DiagGroup<"objc-literal-compare", [
     ObjCStringComparison
   ]>;
 
+def ObjCSignedCharBool : DiagGroup<"objc-signed-char-bool",
+  [ObjCSignedCharBoolImplicitIntConversion,
+   ObjCSignedCharBoolImplicitFloatConversion,
+   ObjCBoolConstantConversion,
+   TautologicalObjCBoolCompare]>;
+
 // Inline ASM warnings.
 def ASMOperandWidths : DiagGroup<"asm-operand-widths">;
 def ASMIgnoredQualifier : DiagGroup<"asm-ignored-qualifier">;
 def ASM : DiagGroup<"asm", [
     ASMOperandWidths, ASMIgnoredQualifier
   ]>;
 
 // OpenMP warnings.

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<
+def warn_impcast_constant_value_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>;
 def warn_impcast_literal_float_to_integer_out_of_range : Warning<
   "implicit conversion of out of range value from %0 to %1 is undefined">,
   InGroup<LiteralConversion>;
 def warn_impcast_float_integer : Warning<
   "implicit conversion turns floating-point number into integer: %0 to %1">,
   InGroup<FloatConversion>, DefaultIgnore;
+def warn_impcast_float_to_objc_signed_char_bool : Warning<
+  "implicit conversion from floating-point type %0 to 'BOOL'">,
+  InGroup<ObjCSignedCharBoolImplicitFloatConversion>;
+def warn_impcast_int_to_objc_signed_char_bool : Warning<
+  "implicit conversion from integral type %0 to 'BOOL'">,
+  InGroup<ObjCSignedCharBoolImplicitIntConversion>, DefaultIgnore;
 
 // Implicit int -> float conversion precision loss warnings.
 def warn_impcast_integer_float_precision : Warning<
   "implicit conversion from %0 to %1 may lose precision">,
   InGroup<ImplicitIntFloatConversion>, DefaultIgnore;
 def warn_impcast_integer_float_precision_constant : Warning<
   "implicit conversion from %2 to %3 changes value from %0 to %1">,
   InGroup<ImplicitIntFloatConversion>;

clang/lib/AST/Expr.cpp

case BO_Assign:
       return BO->getRHS()->isKnownToHaveBooleanValue();
     }
   }
 
   if (const ConditionalOperator *CO = dyn_cast<ConditionalOperator>(E))
     return CO->getTrueExpr()->isKnownToHaveBooleanValue() &&
            CO->getFalseExpr()->isKnownToHaveBooleanValue();
 
+  if (isa<ObjCBoolLiteralExpr>(E))
+    return true;
+
+  if (const auto *OVE = dyn_cast<OpaqueValueExpr>(E))
+    return OVE->getSourceExpr()->isKnownToHaveBooleanValue();
+
   return false;
 }
 
 // Amusing macro metaprogramming hack: check whether a class provides
 // a more specific implementation of getExprLoc().
 //
 // See also Stmt.cpp:{getBeginLoc(),getEndLoc()}.
 namespace {

clang/lib/Sema/SemaChecking.cpp

 
 /// Diagnose an implicit cast;  purely a helper for CheckImplicitConversion.
 static void DiagnoseImpCast(Sema &S, Expr *E, QualType T,
                             SourceLocation CContext,
                             unsigned diag, bool pruneControlFlow = false) {
   DiagnoseImpCast(S, E, E->getType(), T, CContext, diag, pruneControlFlow);
 }
 
+static bool isObjCSignedCharBool(Sema &S, QualType Ty) {
+  return Ty->isSpecificBuiltinType(BuiltinType::SChar) &&
+      S.getLangOpts().ObjC && S.NSAPIObj->isObjCBOOLType(Ty);
+}
+
+static void adornObjCBoolConversionDiagWithTernaryFixit(
+    Sema &S, Expr *SourceExpr, const Sema::SemaDiagnosticBuilder &Builder) {
+  Expr *Ignored = SourceExpr->IgnoreImplicit();
+  if (const auto *OVE = dyn_cast<OpaqueValueExpr>(Ignored))
+    Ignored = OVE->getSourceExpr();
+  bool NeedsParens = isa<AbstractConditionalOperator>(Ignored) ||
+                     isa<BinaryOperator>(Ignored) ||
+                     isa<CXXOperatorCallExpr>(Ignored);
+  SourceLocation EndLoc = S.getLocForEndOfToken(SourceExpr->getEndLoc());
+  if (NeedsParens)
+    Builder << FixItHint::CreateInsertion(SourceExpr->getBeginLoc(), "(")
+            << FixItHint::CreateInsertion(EndLoc, ")");
+  Builder << FixItHint::CreateInsertion(EndLoc, " ? YES : NO");
+}
+
 /// Diagnose an implicit cast from a floating point value to an integer value.
 static void DiagnoseFloatingImpCast(Sema &S, Expr *E, QualType T,
                                     SourceLocation CContext) {
   const bool IsBool = T->isSpecificBuiltinType(BuiltinType::Bool);
   const bool PruneWarnings = S.inTemplateInstantiation();
 
   Expr *InnerE = E->IgnoreParenImpCasts();
   // We also want to warn on, e.g., "int i = -1.234"
   if (UnaryOperator *UOp = dyn_cast<UnaryOperator>(InnerE))
     if (UOp->getOpcode() == UO_Minus || UOp->getOpcode() == UO_Plus)
       InnerE = UOp->getSubExpr()->IgnoreParenImpCasts();
 
   const bool IsLiteral =
       isa<FloatingLiteral>(E) || isa<FloatingLiteral>(InnerE);
 
   llvm::APFloat Value(0.0);
   bool IsConstant =
     E->EvaluateAsFloat(Value, S.Context, Expr::SE_AllowSideEffects);
   if (!IsConstant) {
+    if (isObjCSignedCharBool(S, T)) {
+      return adornObjCBoolConversionDiagWithTernaryFixit(
+          S, E,
+          S.Diag(CContext, diag::warn_impcast_float_to_objc_signed_char_bool)
+              << E->getType());
+    }
+
     return DiagnoseImpCast(S, E, T, CContext,
                            diag::warn_impcast_float_integer, PruneWarnings);
   }
 
   bool isExact = false;
 
   llvm::APSInt IntegerValue(S.Context.getIntWidth(T),
                             T->hasUnsignedIntegerRepresentation());
   llvm::APFloat::opStatus Result = Value.convertToInteger(
       IntegerValue, llvm::APFloat::rmTowardZero, &isExact);
 
+  // FIXME: Force the precision of the source value down so we don't print
+  // digits which are usually useless (we don't really care here if we
+  // truncate a digit by accident in edge cases).  Ideally, APFloat::toString
+  // would automatically print the shortest representation, but it's a bit
+  // tricky to implement.
+  SmallString<16> PrettySourceValue;
+  unsigned precision = llvm::APFloat::semanticsPrecision(Value.getSemantics());
+  precision = (precision * 59 + 195) / 196;
+  Value.toString(PrettySourceValue, precision);
+
+  if (isObjCSignedCharBool(S, T) && IntegerValue != 0 && IntegerValue != 1) {
+    return adornObjCBoolConversionDiagWithTernaryFixit(
+        S, E,
+        S.Diag(CContext, diag::warn_impcast_constant_value_to_objc_bool)
+            << PrettySourceValue);
+  }
+
   if (Result == llvm::APFloat::opOK && isExact) {
     if (IsLiteral) return;
     return DiagnoseImpCast(S, E, T, CContext, diag::warn_impcast_float_integer,
                            PruneWarnings);
   }
 
   // Conversion of a floating-point value to a non-bool integer where the
   // integral part cannot be represented by the integer type is undefined.

         return DiagnoseImpCast(S, E, T, CContext,
                                diag::warn_impcast_float_integer, PruneWarnings);
       }
     }
     // Warn on evaluatable floating point expression to integer conversion.
     DiagID = diag::warn_impcast_float_to_integer;
   }
 
-  // FIXME: Force the precision of the source value down so we don't print
-  // digits which are usually useless (we don't really care here if we
-  // truncate a digit by accident in edge cases).  Ideally, APFloat::toString
-  // would automatically print the shortest representation, but it's a bit
-  // tricky to implement.
-  SmallString<16> PrettySourceValue;
-  unsigned precision = llvm::APFloat::semanticsPrecision(Value.getSemantics());
-  precision = (precision * 59 + 195) / 196;
-  Value.toString(PrettySourceValue, precision);
-
   SmallString<16> PrettyTargetValue;
   if (IsBool)
     PrettyTargetValue = Value.isZero() ? "false" : "true";
   else
     IntegerValue.toString(PrettyTargetValue);
 
   if (PruneWarnings) {
     S.DiagRuntimeBehavior(E->getExprLoc(), E,

         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,
                                     bool IsListInit = false) {
   if (E->isTypeDependent() || E->isValueDependent()) return;
 
   const Type *Source = S.Context.getCanonicalType(E->getType()).getTypePtr();
   const Type *Target = S.Context.getCanonicalType(T).getTypePtr();

   }
 
   // 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");
+                         Expr::SE_AllowSideEffects)) {
+      if (Result.Val.getInt() != 1 && Result.Val.getInt() != 0) {
+        adornObjCBoolConversionDiagWithTernaryFixit(
+            S, E,
+            S.Diag(CC, diag::warn_impcast_constant_value_to_objc_bool)
+                << Result.Val.getInt().toString(10));
+      }
       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);

   if (!Source->isIntegerType() || !Target->isIntegerType())
     return;
 
   // TODO: remove this early return once the false positives for constant->bool
   // in templates, macros, etc, are reduced or removed.
   if (Target->isSpecificBuiltinType(BuiltinType::Bool))
     return;
 
+  if (isObjCSignedCharBool(S, T) && !Source->isCharType() &&
+      !E->isKnownToHaveBooleanValue()) {
+    return adornObjCBoolConversionDiagWithTernaryFixit(
+        S, E,
+        S.Diag(CC, diag::warn_impcast_int_to_objc_signed_char_bool)
+            << E->getType());
+  }
+
   IntRange SourceRange = GetExprRange(S.Context, E, S.isConstantEvaluated());
   IntRange TargetRange = IntRange::forTargetOfCanonicalType(S.Context, Target);
 
   if (SourceRange.Width > TargetRange.Width) {
     // If the source is a constant, use a default-on diagnostic.
     // TODO: this should happen for bitfield stores, too.
     Expr::EvalResult Result;
     if (E->EvaluateAsInt(Result, S.Context, Expr::SE_AllowSideEffects,

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
+// RUN: %clang_cc1 -Werror=objc-signed-char-bool %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;
int main() {
   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;
+
+  int i;
+
+  b = i;
+  // CHECK: b = i ? YES : NO;
+
+  b = i * 2;
+  // CHECK b = (i * 2) ? YES : NO;
+
+  b = 1 ? 2 : 3;
+  // CHECK: b = 1 ? 2 ? YES : NO : 3 ? 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];
+
+  int i;
+
+  bp.b = i;
+  // CHECK: bp.b = i ? YES : NO;
+
+  bp.b = i + 1;
+  // CHECK: bp.b = (i + 1) ? YES : NO;
 }

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

+// RUN: %clang_cc1 %s -verify -Wobjc-signed-char-bool
+// RUN: %clang_cc1 -xobjective-c++ %s -verify -Wobjc-signed-char-bool
+
+typedef signed char BOOL;
+#define YES __objc_yes
+#define NO __objc_no
+
+typedef unsigned char Boolean;
+
+BOOL b;
+Boolean boolean;
+float fl;
+int i;
+int *ptr;
+
+void t1() {
+  b = boolean;
+  b = fl; // expected-warning {{implicit conversion from floating-point type 'float' to 'BOOL'}}
+  b = i; // expected-warning {{implicit conversion from integral type 'int' to 'BOOL'}}
+
+  b = 1.0;
+  b = 0.0;
+  b = 1.1; // expected-warning {{implicit conversion from 'double' to 'BOOL' (aka 'signed char') changes value from 1.1 to 1}}
+  b = 2.1; // expected-warning {{implicit conversion from constant value 2.1 to BOOL; the only well defined values for BOOL are YES and NO}}
+
+  b = YES;
+#ifndef __cplusplus
+  b = ptr; // expected-warning {{incompatible pointer to integer conversion assigning to 'BOOL' (aka 'signed char') from 'int *'}}
+#endif
+}
+
+@interface BoolProp
+@property BOOL p;
+@end
+
+void t2(BoolProp *bp) {
+  bp.p = YES;
+  bp.p = NO;
+  bp.p = boolean;
+  bp.p = fl; // expected-warning {{implicit conversion from floating-point type 'float' to 'BOOL'}}
+  bp.p = i; // expected-warning {{implicit conversion from integral type 'int' to 'BOOL'}}
+  bp.p = b;
+  bp.p = bp.p;
+#ifndef __cplusplus
+  bp.p = ptr; // expected-warning {{incompatible pointer to integer conversion assigning to 'BOOL' (aka 'signed char') from 'int *'}}
+#endif
+  bp.p = 1;
+  bp.p = 2; // expected-warning {{implicit conversion from constant value 2 to BOOL; the only well defined values for BOOL are YES and NO}}
+}