[ObjC] Add a -Wtautological-compare warning for 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.

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

[rjmccall]

This only applies to relational operators, right? I'm a little uncomfortable with calling this "tautological" since it's not like it's *undefined behavior* to have (BOOL) 2, it's just *unwise*. But as long as we aren't warning about reasonable idioms that are intended to handle unfortunate situations — like other code that might have left a non-{0,1} value in the BOOL — I think this is fine.

[erik.pilkington]

In D63856#1560180, @rjmccall wrote:

This only applies to relational operators, right? I'm a little uncomfortable with calling this "tautological" since it's not like it's *undefined behavior* to have (BOOL) 2, it's just *unwise*. But as long as we aren't warning about reasonable idioms that are intended to handle unfortunate situations — like other code that might have left a non-{0,1} value in the BOOL — I think this is fine.

I think the party line is that it is undefined behaviour (in some sense), since UBSan will happily crash if you try to load a non-boolean value from a BOOL. It is a bit unfortunate that "defensive" code will start warning here though :/. Maybe we can try to detect and permit something like B < NO || B > YES, or emit a note with some canonical way of checking for non-boolean BOOLs. Even if we end up having to disable it default, I think its still a good diagnostic to have. A warning on stores to BOOL would probably be a lot higher value, though.

[rjmccall]

In D63856#1560213, @erik.pilkington wrote:

In D63856#1560180, @rjmccall wrote:

This only applies to relational operators, right? I'm a little uncomfortable with calling this "tautological" since it's not like it's *undefined behavior* to have (BOOL) 2, it's just *unwise*. But as long as we aren't warning about reasonable idioms that are intended to handle unfortunate situations — like other code that might have left a non-{0,1} value in the BOOL — I think this is fine.

I think the party line is that it is undefined behaviour (in some sense), since UBSan will happily crash if you try to load a non-boolean value from a BOOL.

What? Since when?

It is a bit unfortunate that "defensive" code will start warning here though :/. Maybe we can try to detect and permit something like B < NO || B > YES, or emit a note with some canonical way of checking for non-boolean BOOLs. Even if we end up having to disable it default, I think its still a good diagnostic to have. A warning on stores to BOOL would probably be a lot higher value, though.

I'm not sure this is a problem because I'm not sure there's any reason to write defensive code besides B != NO or B == NO. It's potentially problematic if someone writes B == YES, though.

[erik.pilkington]

In D63856#1561112, @rjmccall wrote:

In D63856#1560213, @erik.pilkington wrote:

In D63856#1560180, @rjmccall wrote:

This only applies to relational operators, right? I'm a little uncomfortable with calling this "tautological" since it's not like it's *undefined behavior* to have (BOOL) 2, it's just *unwise*. But as long as we aren't warning about reasonable idioms that are intended to handle unfortunate situations — like other code that might have left a non-{0,1} value in the BOOL — I think this is fine.

I think the party line is that it is undefined behaviour (in some sense), since UBSan will happily crash if you try to load a non-boolean value from a BOOL.

What? Since when?

https://reviews.llvm.org/D27607

It is a bit unfortunate that "defensive" code will start warning here though :/. Maybe we can try to detect and permit something like B < NO || B > YES, or emit a note with some canonical way of checking for non-boolean BOOLs. Even if we end up having to disable it default, I think its still a good diagnostic to have. A warning on stores to BOOL would probably be a lot higher value, though.

I'm not sure this is a problem because I'm not sure there's any reason to write defensive code besides B != NO or B == NO. It's potentially problematic if someone writes B == YES, though.

I was thinking about something like the following, which probably isn't worth warning on. Another way of handling it would be only emitting the diagnostic if !InRange. Not exactly sure how common that pattern actually is though.

void myAPI(BOOL DoAThing) {
  if (DoAThing > YES || DoAThing < NO) DoAThing = YES;
  // ...
}

[rjmccall]

In D63856#1561132, @erik.pilkington wrote:

In D63856#1561112, @rjmccall wrote:

In D63856#1560213, @erik.pilkington wrote:

In D63856#1560180, @rjmccall wrote:

This only applies to relational operators, right? I'm a little uncomfortable with calling this "tautological" since it's not like it's *undefined behavior* to have (BOOL) 2, it's just *unwise*. But as long as we aren't warning about reasonable idioms that are intended to handle unfortunate situations — like other code that might have left a non-{0,1} value in the BOOL — I think this is fine.

I think the party line is that it is undefined behaviour (in some sense), since UBSan will happily crash if you try to load a non-boolean value from a BOOL.

What? Since when?

https://reviews.llvm.org/D27607

Interesting; I'm not sure I find that convincing. Probably the least convincing part is where it links to its own behavioral documentation as justification for doing what it does. But okay, I guess we do this.

It is a bit unfortunate that "defensive" code will start warning here though :/. Maybe we can try to detect and permit something like B < NO || B > YES, or emit a note with some canonical way of checking for non-boolean BOOLs. Even if we end up having to disable it default, I think its still a good diagnostic to have. A warning on stores to BOOL would probably be a lot higher value, though.

I'm not sure this is a problem because I'm not sure there's any reason to write defensive code besides B != NO or B == NO. It's potentially problematic if someone writes B == YES, though.

I was thinking about something like the following, which probably isn't worth warning on. Another way of handling it would be only emitting the diagnostic if !InRange. Not exactly sure how common that pattern actually is though.

void myAPI(BOOL DoAThing) {
  if (DoAThing > YES || DoAThing < NO) DoAThing = YES;
  // ...
}

I don't think it's a problem to warn about this; this is just a silly way of writing if (DoAThing != NO) { DoAThing = YES; }.

[erik.pilkington]

Ping!

[rjmccall]

Okay.

[erik.pilkington]

In D63856#1561160, @rjmccall wrote:

In D63856#1561132, @erik.pilkington wrote:

In D63856#1561112, @rjmccall wrote:

In D63856#1560213, @erik.pilkington wrote:

In D63856#1560180, @rjmccall wrote:

This only applies to relational operators, right? I'm a little uncomfortable with calling this "tautological" since it's not like it's *undefined behavior* to have (BOOL) 2, it's just *unwise*. But as long as we aren't warning about reasonable idioms that are intended to handle unfortunate situations — like other code that might have left a non-{0,1} value in the BOOL — I think this is fine.

I think the party line is that it is undefined behaviour (in some sense), since UBSan will happily crash if you try to load a non-boolean value from a BOOL.

What? Since when?

https://reviews.llvm.org/D27607

Interesting; I'm not sure I find that convincing. Probably the least convincing part is where it links to its own behavioral documentation as justification for doing what it does. But okay, I guess we do this.

Hmm, I guess you could say that they were being a little... tautological.

cfe/trunk/include/clang/Basic/DiagnosticGroups.td

 // For compatibility with GCC; -Wtype-limits = -Wtautological-constant-in-range-compare
 def TypeLimits : DiagGroup<"type-limits", [TautologicalInRangeCompare]>;
 def TautologicalOutOfRangeCompare : DiagGroup<"tautological-constant-out-of-range-compare">;
 def TautologicalConstantCompare : DiagGroup<"tautological-constant-compare",
                                             [TautologicalOutOfRangeCompare]>;
 def TautologicalPointerCompare : DiagGroup<"tautological-pointer-compare">;
 def TautologicalOverlapCompare : DiagGroup<"tautological-overlap-compare">;
 def TautologicalUndefinedCompare : DiagGroup<"tautological-undefined-compare">;
+def TautologicalObjCBoolCompare : DiagGroup<"tautological-objc-bool-compare">;
 def TautologicalCompare : DiagGroup<"tautological-compare",
                                     [TautologicalConstantCompare,
                                      TautologicalPointerCompare,
                                      TautologicalOverlapCompare,
-                                     TautologicalUndefinedCompare]>;
+                                     TautologicalUndefinedCompare,
+                                     TautologicalObjCBoolCompare]>;
 def HeaderHygiene : DiagGroup<"header-hygiene">;
 def DuplicateDeclSpecifier : DiagGroup<"duplicate-decl-specifier">;
 def CompareDistinctPointerType : DiagGroup<"compare-distinct-pointer-types">;
 def GNUUnionCast : DiagGroup<"gnu-union-cast">;
 def GNUVariableSizedTypeNotAtEnd : DiagGroup<"gnu-variable-sized-type-not-at-end">;
 def Varargs : DiagGroup<"varargs">;
 
 def Unsequenced : DiagGroup<"unsequenced">;

cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td

 def warn_unsigned_enum_always_true_comparison : Warning<
   "result of comparison of %select{%3|unsigned enum expression}0 %2 "
   "%select{unsigned enum expression|%3}0 is always %4">,
   InGroup<TautologicalUnsignedEnumZeroCompare>, DefaultIgnore;
 def warn_tautological_constant_compare : Warning<
   "result of comparison %select{%3|%1}0 %2 "
   "%select{%1|%3}0 is always %4">,
   InGroup<TautologicalTypeLimitCompare>, DefaultIgnore;
+def warn_tautological_compare_objc_bool : Warning<
+  "result of comparison of constant %0 with expression of type BOOL"
+  " is always %1, as the only well defined values for BOOL are YES and NO">,
+  InGroup<TautologicalObjCBoolCompare>;
 
 def warn_mixed_sign_comparison : Warning<
   "comparison of integers of different signs: %0 and %1">,
   InGroup<SignCompare>, DefaultIgnore;
 def warn_out_of_range_compare : Warning<
   "result of comparison of %select{constant %0|true|false}1 with "
   "%select{expression of type %2|boolean expression}3 is always %4">,
   InGroup<TautologicalOutOfRangeCompare>;

cfe/trunk/lib/Sema/SemaChecking.cpp

 
 static bool IsEnumConstOrFromMacro(Sema &S, Expr *E) {
   // Suppress cases where we are comparing against an enum constant.
   if (const DeclRefExpr *DR =
       dyn_cast<DeclRefExpr>(E->IgnoreParenImpCasts()))
     if (isa<EnumConstantDecl>(DR->getDecl()))
       return true;
 
-  // Suppress cases where the '0' value is expanded from a macro.
-  if (E->getBeginLoc().isMacroID())
-    return true;
+  // Suppress cases where the value is expanded from a macro, unless that macro
+  // is how a language represents a boolean literal. This is the case in both C
+  // and Objective-C.
+  SourceLocation BeginLoc = E->getBeginLoc();
+  if (BeginLoc.isMacroID()) {
+    StringRef MacroName = Lexer::getImmediateMacroName(
+        BeginLoc, S.getSourceManager(), S.getLangOpts());
+    return MacroName != "YES" && MacroName != "NO" &&
+           MacroName != "true" && MacroName != "false";
+  }
 
   return false;
 }
 
 static bool isKnownToHaveUnsignedValue(Expr *E) {
   return E->getType()->isIntegerType() &&
          (!E->getType()->isSignedIntegerType() ||
           !E->IgnoreParenImpCasts()->getType()->isSignedIntegerType());
static bool CheckTautologicalComparison(Sema &S, BinaryOperator *E,
 
   // TODO: Investigate using GetExprRange() to get tighter bounds
   // on the bit ranges.
   QualType OtherT = Other->getType();
   if (const auto *AT = OtherT->getAs<AtomicType>())
     OtherT = AT->getValueType();
   IntRange OtherRange = IntRange::forValueOfType(S.Context, OtherT);
 
+  // Special case for ObjC BOOL on targets where its a typedef for a signed char
+  // (Namely, macOS).
+  bool IsObjCSignedCharBool = S.getLangOpts().ObjC &&
+                              S.NSAPIObj->isObjCBOOLType(OtherT) &&
+                              OtherT->isSpecificBuiltinType(BuiltinType::SChar);
+
   // Whether we're treating Other as being a bool because of the form of
   // expression despite it having another type (typically 'int' in C).
   bool OtherIsBooleanDespiteType =
       !OtherT->isBooleanType() && Other->isKnownToHaveBooleanValue();
-  if (OtherIsBooleanDespiteType)
+  if (OtherIsBooleanDespiteType || IsObjCSignedCharBool)
     OtherRange = IntRange::forBoolType();
 
   // Determine the promoted range of the other type and see if a comparison of
   // the constant against that range is tautological.
   PromotedRange OtherPromotedRange(OtherRange, Value.getBitWidth(),
                                    Value.isUnsigned());
   auto Cmp = OtherPromotedRange.compare(Value);
   auto Result = PromotedRange::constantValue(E->getOpcode(), Cmp, RhsConstant);
static bool CheckTautologicalComparison(Sema &S, BinaryOperator *E,
   // constant in the diagnostic.
   const EnumConstantDecl *ED = nullptr;
   if (const DeclRefExpr *DR = dyn_cast<DeclRefExpr>(Constant))
     ED = dyn_cast<EnumConstantDecl>(DR->getDecl());
 
   // Should be enough for uint128 (39 decimal digits)
   SmallString<64> PrettySourceValue;
   llvm::raw_svector_ostream OS(PrettySourceValue);
-  if (ED)
+  if (ED) {
     OS << '\'' << *ED << "' (" << Value << ")";
-  else
+  } else if (auto *BL = dyn_cast<ObjCBoolLiteralExpr>(
+               Constant->IgnoreParenImpCasts())) {
+    OS << (BL->getValue() ? "YES" : "NO");
+  } else {
     OS << Value;
+  }
+
+  if (IsObjCSignedCharBool) {
+    S.DiagRuntimeBehavior(E->getOperatorLoc(), E,
+                          S.PDiag(diag::warn_tautological_compare_objc_bool)
+                              << OS.str() << *Result);
+    return true;
+  }
 
   // FIXME: We use a somewhat different formatting for the in-range cases and
   // cases involving boolean values for historical reasons. We should pick a
   // consistent way of presenting these diagnostics.
   if (!InRange || Other->isKnownToHaveBooleanValue()) {
 
     S.DiagRuntimeBehavior(
         E->getOperatorLoc(), E,

cfe/trunk/test/Sema/tautological-objc-bool-compare.m

+// RUN: %clang_cc1 %s -verify
+
+typedef signed char BOOL;
+#define YES __objc_yes
+#define NO __objc_no
+
+BOOL B;
+
+void test() {
+  int r;
+  r = B > 0;
+  r = B > 1; // expected-warning {{result of comparison of constant 1 with expression of type BOOL is always false, as the only well defined values for BOOL are YES and NO}}
+  r = B < 1;
+  r = B < 0; // expected-warning {{result of comparison of constant 0 with expression of type BOOL is always false, as the only well defined values for BOOL are YES and NO}}
+  r = B >= 0; // expected-warning {{result of comparison of constant 0 with expression of type BOOL is always true, as the only well defined values for BOOL are YES and NO}}
+  r = B <= 0;
+
+  r = B > YES; // expected-warning {{result of comparison of constant YES with expression of type BOOL is always false, as the only well defined values for BOOL are YES and NO}}
+  r = B > NO;
+  r = B < NO; // expected-warning {{result of comparison of constant NO with expression of type BOOL is always false, as the only well defined values for BOOL are YES and NO}}
+  r = B < YES;
+  r = B >= NO; // expected-warning {{result of comparison of constant NO with expression of type BOOL is always true, as the only well defined values for BOOL are YES and NO}}
+  r = B <= NO;
+}