Disallow using ObjC literals in direct comparisons (== and friends).

Objective-C literals conceptually always create new objects, but may be
optimized by the compiler or runtime (constant folding, singletons, etc).
Comparing addresses of these objects is relying on this optimization
behavior, which is really an implementation detail.

In the case of == and !=, offer a fixit to a call to -isEqual:, if the
method is available. This fixit is directly on the error so that it is
automatically applied.

Most of the time, this is really a newbie mistake, hence the fixit.

llvm-svn: 158230

clang/include/clang/AST/NSAPI.h

@@ -132,6 +132,11 @@ public:
     return getOrInitSelector(Ids, setObjectAtIndexedSubscriptSel);
   }
 
+  /// \brief Returns selector for "isEqual:".
+  Selector getIsEqualSelector() const {
+    return getOrInitSelector(StringRef("isEqual"), isEqualSel);
+  }
+
   /// \brief Enumerates the NSNumber methods used to generate literals.
   enum NSNumberLiteralMethodKind {
     NSNumberWithChar,
@@ -203,7 +208,8 @@ private:
   mutable Selector NSNumberInstanceSelectors[NumNSNumberLiteralMethods];
 
   mutable Selector objectForKeyedSubscriptSel, objectAtIndexedSubscriptSel,
-                   setObjectForKeyedSubscriptSel,setObjectAtIndexedSubscriptSel;
+                   setObjectForKeyedSubscriptSel,setObjectAtIndexedSubscriptSel,
+                   isEqualSel;
 
   mutable IdentifierInfo *BOOLId, *NSIntegerId, *NSUIntegerId;
   mutable IdentifierInfo *NSASCIIStringEncodingId, *NSUTF8StringEncodingId;

clang/include/clang/Basic/DiagnosticSemaKinds.td

@@ -1593,6 +1593,10 @@ def err_invalid_collection_element : Error<
 def err_box_literal_collection : Error<
   "%select{string|character|boolean|numeric}0 literal must be prefixed by '@' "
   "in a collection">;
+def err_objc_literal_comparison : Error<
+  "direct comparison of %select{a string literal|an array literal|"
+  "a dictionary literal|a numeric literal|a boxed expression|}0 is not "
+  "allowed%select{|; use -isEqual: instead}1">;
 
 let CategoryName = "Cocoa API Issue" in {
 def warn_objc_redundant_literal_use : Warning<

clang/lib/Sema/SemaExpr.cpp

@@ -6589,6 +6589,153 @@ static void diagnoseFunctionPointerToVoidComparison(Sema &S, SourceLocation Loc,
     << LHS.get()->getSourceRange() << RHS.get()->getSourceRange();
 }
 
+static bool isObjCObjectLiteral(ExprResult &E) {
+  switch (E.get()->getStmtClass()) {
+  case Stmt::ObjCArrayLiteralClass:
+  case Stmt::ObjCDictionaryLiteralClass:
+  case Stmt::ObjCStringLiteralClass:
+  case Stmt::ObjCBoxedExprClass:
+    return true;
+  default:
+    // Note that ObjCBoolLiteral is NOT an object literal!
+    return false;
+  }
+}
+
+static DiagnosticBuilder diagnoseObjCLiteralComparison(Sema &S,
+                                                       SourceLocation Loc,
+                                                       ExprResult &LHS,
+                                                       ExprResult &RHS,
+                                                       bool CanFix = false) {
+  Expr *Literal = (isObjCObjectLiteral(LHS) ? LHS : RHS).get();
+
+  unsigned LiteralKind;
+  switch (Literal->getStmtClass()) {
+  case Stmt::ObjCStringLiteralClass:
+    // "string literal"
+    LiteralKind = 0;
+    break;
+  case Stmt::ObjCArrayLiteralClass:
+    // "array literal"
+    LiteralKind = 1;
+    break;
+  case Stmt::ObjCDictionaryLiteralClass:
+    // "dictionary literal"
+    LiteralKind = 2;
+    break;
+  case Stmt::ObjCBoxedExprClass: {
+    Expr *Inner = cast<ObjCBoxedExpr>(Literal)->getSubExpr();
+    switch (Inner->getStmtClass()) {
+    case Stmt::IntegerLiteralClass:
+    case Stmt::FloatingLiteralClass:
+    case Stmt::CharacterLiteralClass:
+    case Stmt::ObjCBoolLiteralExprClass:
+    case Stmt::CXXBoolLiteralExprClass:
+      // "numeric literal"
+      LiteralKind = 3;
+      break;
+    case Stmt::ImplicitCastExprClass: {
+      CastKind CK = cast<CastExpr>(Inner)->getCastKind();
+      // Boolean literals can be represented by implicit casts.
+      if (CK == CK_IntegralToBoolean || CK == CK_IntegralCast) {
+        LiteralKind = 3;
+        break;
+      }
+      // FALLTHROUGH
+    }
+    default:
+      // "boxed expression"
+      LiteralKind = 4;
+      break;
+    }
+    break;
+  }
+  default:
+    llvm_unreachable("Unknown Objective-C object literal kind");
+  }
+
+  return S.Diag(Loc, diag::err_objc_literal_comparison)
+           << LiteralKind << CanFix << Literal->getSourceRange();
+}
+
+static ExprResult fixObjCLiteralComparison(Sema &S, SourceLocation OpLoc,
+                                           ExprResult &LHS,
+                                           ExprResult &RHS,
+                                           BinaryOperatorKind Op) {
+  assert((Op == BO_EQ || Op == BO_NE) && "Cannot fix other operations.");
+
+  // Get the LHS object's interface type.
+  QualType Type = LHS.get()->getType();
+  QualType InterfaceType;
+  if (const ObjCObjectPointerType *PTy = Type->getAs<ObjCObjectPointerType>()) {
+    InterfaceType = PTy->getPointeeType();
+    if (const ObjCObjectType *iQFaceTy =
+        InterfaceType->getAsObjCQualifiedInterfaceType())
+      InterfaceType = iQFaceTy->getBaseType();
+  } else {
+    // If this is not actually an Objective-C object, bail out.
+    return ExprEmpty();
+  }
+
+  // If the RHS isn't an Objective-C object, bail out.
+  if (!RHS.get()->getType()->isObjCObjectPointerType())
+    return ExprEmpty();
+
+  // Try to find the -isEqual: method.
+  Selector IsEqualSel = S.NSAPIObj->getIsEqualSelector();
+  ObjCMethodDecl *Method = S.LookupMethodInObjectType(IsEqualSel,
+                                                      InterfaceType,
+                                                      /*instance=*/true);
+  bool ReceiverIsId = (Type->isObjCIdType() || Type->isObjCQualifiedIdType());
+
+  if (!Method && ReceiverIsId) {
+    Method = S.LookupInstanceMethodInGlobalPool(IsEqualSel, SourceRange(),
+                                                /*receiverId=*/true,
+                                                /*warn=*/false);
+  }
+
+  if (!Method)
+    return ExprEmpty();
+
+  QualType T = Method->param_begin()[0]->getType();
+  if (!T->isObjCObjectPointerType())
+    return ExprEmpty();
+
+  QualType R = Method->getResultType();
+  if (!R->isScalarType())
+    return ExprEmpty();
+
+  // At this point we know we have a good -isEqual: method.
+  // Emit the diagnostic and fixit.
+  DiagnosticBuilder Diag = diagnoseObjCLiteralComparison(S, OpLoc,
+                                                         LHS, RHS, true);
+
+  Expr *LHSExpr = LHS.take();
+  Expr *RHSExpr = RHS.take();
+
+  SourceLocation Start = LHSExpr->getLocStart();
+  SourceLocation End = S.PP.getLocForEndOfToken(RHSExpr->getLocEnd());
+  SourceRange OpRange(OpLoc, S.PP.getLocForEndOfToken(OpLoc));
+
+  Diag << FixItHint::CreateInsertion(Start, Op == BO_EQ ? "[" : "![")
+       << FixItHint::CreateReplacement(OpRange, "isEqual:")
+       << FixItHint::CreateInsertion(End, "]");
+
+  // Finally, build the call to -isEqual: (and possible logical not).
+  ExprResult Call = S.BuildInstanceMessage(LHSExpr, LHSExpr->getType(),
+                                           /*SuperLoc=*/SourceLocation(),
+                                           IsEqualSel, Method,
+                                           OpLoc, OpLoc, OpLoc,
+                                           MultiExprArg(S, &RHSExpr, 1),
+                                           /*isImplicit=*/false);
+
+  ExprResult CallCond = S.CheckBooleanCondition(Call.get(), OpLoc);
+
+  if (Op == BO_NE)
+    return S.CreateBuiltinUnaryOp(OpLoc, UO_LNot, CallCond.get());
+  return CallCond;
+}
+
 // C99 6.5.8, C++ [expr.rel]
 QualType Sema::CheckCompareOperands(ExprResult &LHS, ExprResult &RHS,
                                     SourceLocation Loc, unsigned OpaqueOpc,
@@ -6913,6 +7060,9 @@ QualType Sema::CheckCompareOperands(ExprResult &LHS, ExprResult &RHS,
       if (!Context.areComparableObjCPointerTypes(LHSType, RHSType))
         diagnoseDistinctPointerComparison(*this, Loc, LHS, RHS,
                                           /*isError*/false);
+      if (isObjCObjectLiteral(LHS) || isObjCObjectLiteral(RHS))
+        diagnoseObjCLiteralComparison(*this, Loc, LHS, RHS);
+
       if (LHSIsNull && !RHSIsNull)
         LHS = ImpCastExprToType(LHS.take(), RHSType, CK_BitCast);
       else
@@ -7971,6 +8121,13 @@ ExprResult Sema::CreateBuiltinBinOp(SourceLocation OpLoc,
     break;
   case BO_EQ:
   case BO_NE:
+    if (isObjCObjectLiteral(LHS) || isObjCObjectLiteral(RHS)) {
+      ExprResult IsEqualCall = fixObjCLiteralComparison(*this, OpLoc,
+                                                        LHS, RHS, Opc);
+      if (IsEqualCall.isUsable())
+        return IsEqualCall;
+      // Otherwise, fall back to the normal diagnostic in CheckCompareOperands.
+    }
     ResultTy = CheckCompareOperands(LHS, RHS, OpLoc, Opc, false);
     break;
   case BO_And:

clang/test/FixIt/objc-literals.m

@@ -2,13 +2,15 @@
 // RUN: cp %s %t
 // RUN: not %clang_cc1 -fsyntax-only -fixit -x objective-c %t
 // RUN: %clang_cc1 -fsyntax-only -pedantic -Werror -x objective-c %t
-// RUN: grep arrayWithObjects %t
+// RUN: FileCheck -input-file=%t %s
 
 typedef unsigned char BOOL;
 
-@interface NSNumber @end
+@interface NSObject
+- (BOOL)isEqual:(id)other;
+@end
 
-@interface NSNumber (NSNumberCreation)
+@interface NSNumber : NSObject
 + (NSNumber *)numberWithChar:(char)value;
 + (NSNumber *)numberWithUnsignedChar:(unsigned char)value;
 + (NSNumber *)numberWithShort:(short)value;
@@ -24,17 +26,17 @@ typedef unsigned char BOOL;
 + (NSNumber *)numberWithBool:(BOOL)value;
 @end
 
-@interface NSArray
-@end
-
-@interface NSArray (NSArrayCreation)
+@interface NSArray : NSObject
 + (id)arrayWithObjects:(const id [])objects count:(unsigned long)cnt;
 @end
 
-@interface NSDictionary
+@interface NSDictionary : NSObject
 + (id)dictionaryWithObjects:(const id [])objects forKeys:(const id [])keys count:(unsigned long)cnt;
 @end
 
+@interface NSString : NSObject
+@end
+
 void fixes() {
   id arr = @[
     17, // expected-error{{numeric literal must be prefixed by '@' in a collection}}
@@ -42,3 +44,23 @@ void fixes() {
     "blah" // expected-error{{string literal must be prefixed by '@'}}
   ];
 }
+
+void testComparisons(id obj) {
+  if (obj == @"abc") return; // expected-error{{direct comparison of a string literal is not allowed; use -isEqual: instead}}
+  if (obj != @"def") return; // expected-error{{direct comparison of a string literal is not allowed; use -isEqual: instead}}
+  if (@"ghi" == obj) return; // expected-error{{direct comparison of a string literal is not allowed; use -isEqual: instead}}
+
+  // CHECK: void testComparisons(id obj) {
+  // Make sure these three substitutions aren't matching the CHECK lines.
+  // CHECK-NEXT: if ([obj isEqual: @"abc"]) return;
+  // CHECK-NEXT: if (![obj isEqual: @"def"]) return;
+  // CHECK-NEXT: if ([@"ghi" isEqual: obj]) return;
+
+  if (@[] == obj) return; // expected-error{{direct comparison of an array literal is not allowed; use -isEqual: instead}}
+  if (@{} == obj) return; // expected-error{{direct comparison of a dictionary literal is not allowed; use -isEqual: instead}}
+  if (@12 == obj) return; // expected-error{{direct comparison of a numeric literal is not allowed; use -isEqual: instead}}
+  if (@1.0 == obj) return; // expected-error{{direct comparison of a numeric literal is not allowed; use -isEqual: instead}}
+  if (@__objc_yes == obj) return; // expected-error{{direct comparison of a numeric literal is not allowed; use -isEqual: instead}}
+  if (@(1+1) == obj) return; // expected-error{{direct comparison of a boxed expression is not allowed; use -isEqual: instead}}
+}
+

clang/test/SemaObjC/objc-literal-comparison.m

@@ -0,0 +1,68 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+typedef unsigned char BOOL;
+
+@interface BaseObject
++ (instancetype)new;
+@end
+
+@interface NSObject : BaseObject
+- (BOOL)isEqual:(id)other;
+@end
+
+@interface NSNumber : NSObject
++ (NSNumber *)numberWithInt:(int)value;
++ (NSNumber *)numberWithDouble:(double)value;
++ (NSNumber *)numberWithBool:(BOOL)value;
+@end
+
+@interface NSArray : NSObject
++ (id)arrayWithObjects:(const id [])objects count:(unsigned long)cnt;
+@end
+
+@interface NSDictionary : NSObject
++ (id)dictionaryWithObjects:(const id [])objects forKeys:(const id [])keys count:(unsigned long)cnt;
+@end
+
+@interface NSString : NSObject
+@end
+
+void testComparisonsWithFixits(id obj) {
+  if (obj == @"") return; // expected-error{{direct comparison of a string literal is not allowed; use -isEqual: instead}}
+  if (obj != @"") return; // expected-error{{direct comparison of a string literal is not allowed; use -isEqual: instead}}
+  if (@"" == obj) return; // expected-error{{direct comparison of a string literal is not allowed; use -isEqual: instead}}
+  if (@"" == @"") return; // expected-error{{direct comparison of a string literal is not allowed; use -isEqual: instead}}
+
+  if (@[] == obj) return; // expected-error{{direct comparison of an array literal is not allowed; use -isEqual: instead}}
+  if (@{} == obj) return; // expected-error{{direct comparison of a dictionary literal is not allowed; use -isEqual: instead}}
+  if (@12 == obj) return; // expected-error{{direct comparison of a numeric literal is not allowed; use -isEqual: instead}}
+  if (@1.0 == obj) return; // expected-error{{direct comparison of a numeric literal is not allowed; use -isEqual: instead}}
+  if (@__objc_yes == obj) return; // expected-error{{direct comparison of a numeric literal is not allowed; use -isEqual: instead}}
+  if (@(1+1) == obj) return; // expected-error{{direct comparison of a boxed expression is not allowed; use -isEqual: instead}}
+}
+
+
+@interface BadEqualReturnString : NSString
+- (void)isEqual:(id)other;
+@end
+
+@interface BadEqualArgString : NSString
+- (BOOL)isEqual:(int)other;
+@end
+
+
+void testComparisonsWithoutFixits() {
+  // All of these verifications use regex form to ensure we /don't/ append
+  // "use -isEqual: instead" to any of these.
+
+  if ([BaseObject new] == @"") return; // expected-error-re{{direct comparison of a string literal is not allowed$}}
+
+  if ([BadEqualReturnString new] == @"") return; // expected-error-re{{direct comparison of a string literal is not allowed$}}
+  if ([BadEqualArgString new] == @"") return; // expected-error-re{{direct comparison of a string literal is not allowed$}}
+
+  if (@"" < @"") return; // expected-error-re{{direct comparison of a string literal is not allowed$}}
+  if (@"" > @"") return; // expected-error-re{{direct comparison of a string literal is not allowed$}}
+  if (@"" <= @"") return; // expected-error-re{{direct comparison of a string literal is not allowed$}}
+  if (@"" >= @"") return; // expected-error-re{{direct comparison of a string literal is not allowed$}}
+}
+