[SemaObjC] Forbid storing an unboxed integer literal in an NSNumber

[erik.pilkington]

Summary

This fixes a common mistake (the 3 should be @3): NSNumber *n = 3. This extends an existing check for NSString. Also, this only errs if the initializer isn't a null pointer constant, so NSNumber *n = 0; continues to work.

Fixes rdar://47029572

[bendjones]

Seems reasonable to me. Thanks @erik.pilkington!

clang/include/clang/Basic/DiagnosticSemaKinds.td

 def err_box_literal_collection : Error<
   "%select{string|character|boolean|numeric}0 literal must be prefixed by '@' "
   "in a collection">;
 def warn_objc_literal_comparison : Warning<
   "direct comparison of %select{an array literal|a dictionary literal|"
   "a numeric literal|a boxed expression|}0 has undefined behavior">,
   InGroup<ObjCLiteralComparison>;
 def err_missing_atsign_prefix : Error<
-  "string literal must be prefixed by '@' ">;
+  "%select{string|numeric}0 literal must be prefixed by '@'">;
 def warn_objc_string_literal_comparison : Warning<
   "direct comparison of a string literal has undefined behavior">,
   InGroup<ObjCStringComparison>;
 def warn_concatenated_nsarray_literal : Warning<
   "concatenated NSString literal for an NSArray expression - "
   "possibly missing a comma">,
   InGroup<ObjCStringConcatenation>;
 def note_objc_literal_comparison_isequal : Note<

clang/include/clang/Sema/Sema.h

bool checkObjCBridgeRelatedComponents(SourceLocation Loc,
                                         ObjCMethodDecl *&InstanceMethod,
                                         TypedefNameDecl *&TDNDecl,
                                         bool CfToNs, bool Diagnose = true);
 
   bool CheckObjCBridgeRelatedConversions(SourceLocation Loc,
                                          QualType DestType, QualType SrcType,
                                          Expr *&SrcExpr, bool Diagnose = true);
 
-  bool ConversionToObjCStringLiteralCheck(QualType DstType, Expr *&SrcExpr,
-                                          bool Diagnose = true);
+  bool CheckConversionToObjCLiteral(QualType DstType, Expr *&SrcExpr,
 
   bool checkInitMethod(ObjCMethodDecl *method, QualType receiverTypeIfCall);
 
   /// Check whether the given new method is a valid override of the
   /// given overridden method, and set any properties that should be inherited.
   void CheckObjCMethodOverride(ObjCMethodDecl *NewMethod,
                                const ObjCMethodDecl *Overridden);
 

clang/lib/Sema/SemaExpr.cpp

if (getLangOpts().allowsNonTrivialObjCLifetimeQualifiers() &&
         CheckObjCConversion(SourceRange(), Ty, E, CCK_ImplicitConversion,
                             Diagnose, DiagnoseCFAudited) != ACR_okay) {
       if (!Diagnose)
         return Incompatible;
     }
     if (getLangOpts().ObjC &&
         (CheckObjCBridgeRelatedConversions(E->getBeginLoc(), LHSType,
                                            E->getType(), E, Diagnose) ||
-         ConversionToObjCStringLiteralCheck(LHSType, E, Diagnose))) {
+         CheckConversionToObjCLiteral(LHSType, E, Diagnose))) {
       if (!Diagnose)
         return Incompatible;
       // Replace the expression with a corrected version and continue so we
       // can find further errors.
       RHS = E;
       return Compatible;
     }
 

 ExprResult Sema::BuildSourceLocExpr(SourceLocExpr::IdentKind Kind,
                                     SourceLocation BuiltinLoc,
                                     SourceLocation RPLoc,
                                     DeclContext *ParentContext) {
   return new (Context)
       SourceLocExpr(Context, Kind, BuiltinLoc, RPLoc, ParentContext);
 }
 
-bool Sema::ConversionToObjCStringLiteralCheck(QualType DstType, Expr *&Exp,
-                                              bool Diagnose) {
+bool Sema::CheckConversionToObjCLiteral(QualType DstType, Expr *&Exp,
   if (!getLangOpts().ObjC)
     return false;
 
   const ObjCObjectPointerType *PT = DstType->getAs<ObjCObjectPointerType>();
   if (!PT)
     return false;
-
-  if (!PT->isObjCIdType()) {
-    // Check if the destination is the 'NSString' interface.
-    const ObjCInterfaceDecl *ID = PT->getInterfaceDecl();
-    if (!ID || !ID->getIdentifier()->isStr("NSString"))
-      return false;
-  }
 
   // Ignore any parens, implicit casts (should only be
   // array-to-pointer decays), and not-so-opaque values.  The last is
   // important for making this trigger for property assignments.
   Expr *SrcExpr = Exp->IgnoreParenImpCasts();
   if (OpaqueValueExpr *OV = dyn_cast<OpaqueValueExpr>(SrcExpr))
     if (OV->getSourceExpr())
       SrcExpr = OV->getSourceExpr()->IgnoreParenImpCasts();
 
-  StringLiteral *SL = dyn_cast<StringLiteral>(SrcExpr);
-  if (!SL || !SL->isAscii())
+  if (auto *SL = dyn_cast<StringLiteral>(SrcExpr)) {
+    if (!PT->isObjCIdType() &&
+        !(ID && ID->getIdentifier()->isStr("NSString")))
+      return false;
+    if (!SL->isAscii())
       return false;
+
     if (Diagnose) {
       Diag(SL->getBeginLoc(), diag::err_missing_atsign_prefix)
-        << FixItHint::CreateInsertion(SL->getBeginLoc(), "@");
+          << /*string*/0 << FixItHint::CreateInsertion(SL->getBeginLoc(), "@");
       Exp = BuildObjCStringLiteral(SL->getBeginLoc(), SL).get();
     }
     return true;
   }
 
+  if ((isa<IntegerLiteral>(SrcExpr) || isa<CharacterLiteral>(SrcExpr) ||
+      isa<FloatingLiteral>(SrcExpr) || isa<ObjCBoolLiteralExpr>(SrcExpr) ||
+      isa<CXXBoolLiteralExpr>(SrcExpr)) &&
+      !SrcExpr->isNullPointerConstant(
+          getASTContext(), Expr::NPC_NeverValueDependent)) {
+    if (!ID || !ID->getIdentifier()->isStr("NSNumber"))
+      return false;
+    if (Diagnose) {
+      Diag(SrcExpr->getBeginLoc(), diag::err_missing_atsign_prefix)
+          << /*number*/1
+          << FixItHint::CreateInsertion(SrcExpr->getBeginLoc(), "@");
+      Expr *NumLit =
+          BuildObjCNumericLiteral(SrcExpr->getBeginLoc(), SrcExpr).get();
+      if (NumLit)
+        Exp = NumLit;
+    }
+    return true;
+  }
+
+  return false;
+}
+
 static bool maybeDiagnoseAssignmentToFunction(Sema &S, QualType DstType,
                                               const Expr *SrcExpr) {
   if (!DstType->isFunctionPointerType() ||
       !SrcExpr->getType()->isFunctionType())
     return false;
 
   auto *DRE = dyn_cast<DeclRefExpr>(SrcExpr->IgnoreParenImpCasts());
   if (!DRE)

clang/lib/Sema/SemaExprObjC.cpp

Sema::CheckObjCConversion(SourceRange castRange, QualType castType,
   // in an acceptable context.
   if (exprACTC == ACTC_retainable && isAnyRetainable(castACTC) && isCast(CCK))
     return ACR_unbridged;
 
   // Issue a diagnostic about a missing @-sign when implicit casting a cstring
   // to 'NSString *', instead of falling through to report a "bridge cast"
   // diagnostic.
   if (castACTC == ACTC_retainable && exprACTC == ACTC_none &&
-      ConversionToObjCStringLiteralCheck(castType, castExpr, Diagnose))
+      CheckConversionToObjCLiteral(castType, castExpr, Diagnose))
     return ACR_error;
 
   // Do not issue "bridge cast" diagnostic when implicit casting
   // a retainable object to a CF type parameter belonging to an audited
   // CF API function. Let caller issue a normal type mismatched diagnostic
   // instead.
   if ((!DiagnoseCFAudited || exprACTC != ACTC_retainable ||
        castACTC != ACTC_coreFoundation) &&

clang/lib/Sema/SemaInit.cpp

void InitializationSequence::InitializeFrom(Sema &S,
   QualType SourceType;
   Expr *Initializer = nullptr;
   if (Args.size() == 1) {
     Initializer = Args[0];
     if (S.getLangOpts().ObjC) {
       if (S.CheckObjCBridgeRelatedConversions(Initializer->getBeginLoc(),
                                               DestType, Initializer->getType(),
                                               Initializer) ||
-          S.ConversionToObjCStringLiteralCheck(DestType, Initializer))
+          S.CheckConversionToObjCLiteral(DestType, Initializer))
         Args[0] = Initializer;
     }
     if (!isa<InitListExpr>(Initializer))
       SourceType = Initializer->getType();
   }
 
   //     - If the initializer is a (non-parenthesized) braced-init-list, the
   //       object is list-initialized (8.5.4).

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

+// RUN: %clang_cc1 -triple x86_64-apple-macos10.10 %s                 -verify=c,expected
+// RUN: %clang_cc1 -triple x86_64-apple-macos10.10 %s -xobjective-c++ -verify=cxx,expected
+// RUN: %clang_cc1 -triple x86_64-apple-macos10.10 %s -fobjc-arc      -verify=c,arc,expected
+
+typedef signed char BOOL;
+#define YES __objc_yes
+#define NO __objc_no
+
+@interface NSNumber
++(instancetype)numberWithChar:(char)value;
++(instancetype)numberWithInt:(int)value;
++(instancetype)numberWithDouble:(double)value;
++(instancetype)numberWithBool:(BOOL)value;
+@end
+
+void test() {
+  NSNumber *n = YES; // expected-error{{numeric literal must be prefixed by '@'}}
+  NSNumber *n1 = 1; // expected-error{{numeric literal must be prefixed by '@'}}
+
+  NSNumber *n2 = NO; // c-warning{{expression which evaluates to zero treated as a null pointer constant}}
+                     // cxx-error@-1{{numeric literal must be prefixed by '@'}}
+  NSNumber *n3 = 0;
+  NSNumber *n4 = 0.0; // expected-error{{numeric literal must be prefixed by '@'}}
+
+  NSNumber *n5 = '\0'; // c-warning{{expression which evaluates to zero treated as a null pointer constant}}
+                       // cxx-error@-1{{numeric literal must be prefixed by '@'}}
+
+
+  NSNumber *n6 = '1'; // expected-error{{numeric literal must be prefixed by '@'}}
+
+  int i;
+  NSNumber *n7 = i; // c-warning{{incompatible integer to pointer conversion}}
+                    // arc-error@-1{{implicit conversion of 'int' to 'NSNumber *' is disallowed with ARC}}
+                    // cxx-error@-2{{cannot initialize a variable of type 'NSNumber *' with an lvalue of type 'int'}}
+
+  id n8 = 1; // c-warning{{incompatible integer to pointer conversion}}
+             // arc-error@-1{{implicit conversion of 'int' to 'id' is disallowed with ARC}}
+             // cxx-error@-2{{cannot initialize a variable of type 'id' with an rvalue of type 'int'}}
+}