Format strings: suggest casts for NS(U)Integer and [SU]Int32 on Darwin.

These types are defined differently on 32-bit and 64-bit platforms, and
trying to offer a fixit for one platform would only mess up the format
string for the other. The Apple-recommended solution is to cast to a type
that is known to be large enough and always use that to print the value.

This should only have an impact on compile time if the format string is
incorrect; in cases where the format string matches the definition on the
current platform, no warning will be emitted.

<rdar://problem/9135072&12164284>

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@163266 91177308-0d34-0410-b5e6-96231b3b80d8

include/clang/Basic/DiagnosticSemaKinds.td

@@ -5410,6 +5410,10 @@ def warn_scanf_nonzero_width : Warning<
 def warn_printf_conversion_argument_type_mismatch : Warning<
   "format specifies type %0 but the argument has type %1">,
   InGroup<Format>;
+def warn_format_argument_needs_cast : Warning<
+  "values of type '%0' should not be used as format arguments; add an explicit "
+  "cast to %1 instead">,
+  InGroup<Format>;
 def warn_printf_positional_arg_exceeds_data_args : Warning <
   "data argument position '%0' exceeds the number of data arguments (%1)">,
   InGroup<Format>;

lib/Sema/SemaChecking.cpp

@@ -1967,7 +1967,7 @@ public:
                                    PartialDiagnostic PDiag,
                                    SourceLocation StringLoc,
                                    bool IsStringLocation, Range StringRange,
-                                   FixItHint Fixit = FixItHint());
+                            ArrayRef<FixItHint> Fixit = ArrayRef<FixItHint>());
 
 protected:
   bool HandleInvalidConversionSpecifier(unsigned argIndex, SourceLocation Loc,
@@ -1994,7 +1994,7 @@ protected:
   template <typename Range>
   void EmitFormatDiagnostic(PartialDiagnostic PDiag, SourceLocation StringLoc,
                             bool IsStringLocation, Range StringRange,
-                            FixItHint Fixit = FixItHint());
+                            ArrayRef<FixItHint> Fixit = ArrayRef<FixItHint>());
 
   void CheckPositionalAndNonpositionalArgs(
       const analyze_format_string::FormatSpecifier *FS);
@@ -2185,7 +2185,7 @@ void CheckFormatHandler::EmitFormatDiagnostic(PartialDiagnostic PDiag,
                                               SourceLocation Loc,
                                               bool IsStringLocation,
                                               Range StringRange,
-                                              FixItHint FixIt) {
+                                              ArrayRef<FixItHint> FixIt) {
   EmitFormatDiagnostic(S, inFunctionCall, Args[FormatIdx], PDiag,
                        Loc, IsStringLocation, StringRange, FixIt);
 }
@@ -2224,15 +2224,27 @@ void CheckFormatHandler::EmitFormatDiagnostic(Sema &S, bool InFunctionCall,
                                               SourceLocation Loc,
                                               bool IsStringLocation,
                                               Range StringRange,
-                                              FixItHint FixIt) {
-  if (InFunctionCall)
-    S.Diag(Loc, PDiag) << StringRange << FixIt;
-  else {
+                                              ArrayRef<FixItHint> FixIt) {
+  if (InFunctionCall) {
+    const Sema::SemaDiagnosticBuilder &D = S.Diag(Loc, PDiag);
+    D << StringRange;
+    for (ArrayRef<FixItHint>::iterator I = FixIt.begin(), E = FixIt.end();
+         I != E; ++I) {
+      D << *I;
+    }
+  } else {
     S.Diag(IsStringLocation ? ArgumentExpr->getExprLoc() : Loc, PDiag)
       << ArgumentExpr->getSourceRange();
-    S.Diag(IsStringLocation ? Loc : StringRange.getBegin(),
-           diag::note_format_string_defined)
-      << StringRange << FixIt;
+
+    const Sema::SemaDiagnosticBuilder &Note =
+      S.Diag(IsStringLocation ? Loc : StringRange.getBegin(),
+             diag::note_format_string_defined);
+
+    Note << StringRange;
+    for (ArrayRef<FixItHint>::iterator I = FixIt.begin(), E = FixIt.end();
+         I != E; ++I) {
+      Note << *I;
+    }
   }
 }
 
@@ -2585,6 +2597,30 @@ CheckPrintfHandler::HandlePrintfSpecifier(const analyze_printf::PrintfSpecifier
   return checkFormatExpr(FS, startSpecifier, specifierLen, Arg);
 }
 
+static bool requiresParensToAddCast(const Expr *E) {
+  // FIXME: We should have a general way to reason about operator
+  // precedence and whether parens are actually needed here.
+  // Take care of a few common cases where they aren't.
+  const Expr *Inside = E->IgnoreImpCasts();
+  if (const PseudoObjectExpr *POE = dyn_cast<PseudoObjectExpr>(Inside))
+    Inside = POE->getSyntacticForm()->IgnoreImpCasts();
+
+  switch (Inside->getStmtClass()) {
+  case Stmt::ArraySubscriptExprClass:
+  case Stmt::CallExprClass:
+  case Stmt::DeclRefExprClass:
+  case Stmt::MemberExprClass:
+  case Stmt::ObjCIvarRefExprClass:
+  case Stmt::ObjCMessageExprClass:
+  case Stmt::ObjCPropertyRefExprClass:
+  case Stmt::ParenExprClass:
+  case Stmt::UnaryOperatorClass:
+    return false;
+  default:
+    return true;
+  }
+}
+
 bool
 CheckPrintfHandler::checkFormatExpr(const analyze_printf::PrintfSpecifier &FS,
                                     const char *StartSpecifier,
@@ -2598,7 +2634,9 @@ CheckPrintfHandler::checkFormatExpr(const analyze_printf::PrintfSpecifier &FS,
                                                     ObjCContext);
   if (!AT.isValid())
     return true;
-  if (AT.matchesType(S.Context, E->getType()))
+
+  QualType IntendedTy = E->getType();
+  if (AT.matchesType(S.Context, IntendedTy))
     return true;
 
   // Look through argument promotions for our error message's reported type.
@@ -2609,6 +2647,7 @@ CheckPrintfHandler::checkFormatExpr(const analyze_printf::PrintfSpecifier &FS,
     if (ICE->getCastKind() == CK_IntegralCast ||
         ICE->getCastKind() == CK_FloatingCast) {
       E = ICE->getSubExpr();
+      IntendedTy = E->getType();
 
       // Check if we didn't match because of an implicit cast from a 'char'
       // or 'short' to an 'int'.  This is done because printf is a varargs
@@ -2616,15 +2655,28 @@ CheckPrintfHandler::checkFormatExpr(const analyze_printf::PrintfSpecifier &FS,
       if (ICE->getType() == S.Context.IntTy ||
           ICE->getType() == S.Context.UnsignedIntTy) {
         // All further checking is done on the subexpression.
-        if (AT.matchesType(S.Context, E->getType()))
+        if (AT.matchesType(S.Context, IntendedTy))
           return true;
       }
     }
   }
 
+  if (S.Context.getTargetInfo().getTriple().isOSDarwin()) {
+    // Special-case some of Darwin's platform-independence types.
+    if (const TypedefType *UserTy = IntendedTy->getAs<TypedefType>()) {
+      StringRef Name = UserTy->getDecl()->getName();
+      IntendedTy = llvm::StringSwitch<QualType>(Name)
+        .Case("NSInteger", S.Context.LongTy)
+        .Case("NSUInteger", S.Context.UnsignedLongTy)
+        .Case("SInt32", S.Context.IntTy)
+        .Case("UInt32", S.Context.UnsignedIntTy)
+        .Default(IntendedTy);
+    }
+  }
+
   // We may be able to offer a FixItHint if it is a supported type.
   PrintfSpecifier fixedFS = FS;
-  bool success = fixedFS.fixType(E->getType(), S.getLangOpts(),
+  bool success = fixedFS.fixType(IntendedTy, S.getLangOpts(),
                                  S.Context, ObjCContext);
 
   if (success) {
@@ -2633,16 +2685,67 @@ CheckPrintfHandler::checkFormatExpr(const analyze_printf::PrintfSpecifier &FS,
     llvm::raw_svector_ostream os(buf);
     fixedFS.toString(os);
 
-    EmitFormatDiagnostic(
-      S.PDiag(diag::warn_printf_conversion_argument_type_mismatch)
-        << AT.getRepresentativeTypeName(S.Context) << E->getType()
-        << E->getSourceRange(),
-      E->getLocStart(),
-      /*IsStringLocation*/false,
-      getSpecifierRange(StartSpecifier, SpecifierLen),
-      FixItHint::CreateReplacement(
-        getSpecifierRange(StartSpecifier, SpecifierLen),
-        os.str()));
+    CharSourceRange SpecRange = getSpecifierRange(StartSpecifier, SpecifierLen);
+
+    if (IntendedTy != E->getType()) {
+      // The canonical type for formatting this value is different from the
+      // actual type of the expression. (This occurs, for example, with Darwin's
+      // NSInteger on 32-bit platforms, where it is typedef'd as 'int', but
+      // should be printed as 'long' for 64-bit compatibility.)
+      // Rather than emitting a normal format/argument mismatch, we want to
+      // add a cast to the recommended type (and correct the format string
+      // if necessary).
+      SmallString<16> CastBuf;
+      llvm::raw_svector_ostream CastFix(CastBuf);
+      CastFix << "(";
+      IntendedTy.print(CastFix, S.Context.getPrintingPolicy());
+      CastFix << ")";
+
+      SmallVector<FixItHint,4> Hints;
+      if (!AT.matchesType(S.Context, IntendedTy))
+        Hints.push_back(FixItHint::CreateReplacement(SpecRange, os.str()));
+
+      if (const CStyleCastExpr *CCast = dyn_cast<CStyleCastExpr>(E)) {
+        // If there's already a cast present, just replace it.
+        SourceRange CastRange(CCast->getLParenLoc(), CCast->getRParenLoc());
+        Hints.push_back(FixItHint::CreateReplacement(CastRange, CastFix.str()));
+
+      } else if (!requiresParensToAddCast(E)) {
+        // If the expression has high enough precedence,
+        // just write the C-style cast.
+        Hints.push_back(FixItHint::CreateInsertion(E->getLocStart(),
+                                                   CastFix.str()));
+      } else {
+        // Otherwise, add parens around the expression as well as the cast.
+        CastFix << "(";
+        Hints.push_back(FixItHint::CreateInsertion(E->getLocStart(),
+                                                   CastFix.str()));
+
+        SourceLocation After = S.PP.getLocForEndOfToken(E->getLocEnd());
+        Hints.push_back(FixItHint::CreateInsertion(After, ")"));
+      }
+
+      // We extract the name from the typedef because we don't want to show
+      // the underlying type in the diagnostic.
+      const TypedefType *UserTy = cast<TypedefType>(E->getType());
+      StringRef Name = UserTy->getDecl()->getName();
+
+      // Finally, emit the diagnostic.
+      EmitFormatDiagnostic(S.PDiag(diag::warn_format_argument_needs_cast)
+                             << Name << IntendedTy
+                             << E->getSourceRange(),
+                           E->getLocStart(), /*IsStringLocation=*/false,
+                           SpecRange, Hints);
+    } else {
+      EmitFormatDiagnostic(
+        S.PDiag(diag::warn_printf_conversion_argument_type_mismatch)
+          << AT.getRepresentativeTypeName(S.Context) << IntendedTy
+          << E->getSourceRange(),
+        E->getLocStart(),
+        /*IsStringLocation*/false,
+        SpecRange,
+        FixItHint::CreateReplacement(SpecRange, os.str()));
+    }
   } else {
     const CharSourceRange &CSR = getSpecifierRange(StartSpecifier,
                                                    SpecifierLen);

test/FixIt/format-darwin.m

@@ -0,0 +1,181 @@
+// RUN: %clang_cc1 -triple i386-apple-darwin9 -fsyntax-only -fblocks -verify %s
+// RUN: %clang_cc1 -triple x86_64-apple-darwin9 -fsyntax-only -fblocks -verify %s
+
+// RUN: %clang_cc1 -triple i386-apple-darwin9 -fdiagnostics-parseable-fixits -fblocks %s 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-apple-darwin9 -fdiagnostics-parseable-fixits -fblocks %s 2>&1 | FileCheck %s
+
+// RUN: %clang_cc1 -triple i386-apple-darwin9 -fdiagnostics-parseable-fixits -fblocks %s 2>&1 | FileCheck -check-prefix=CHECK-32 %s
+// RUN: %clang_cc1 -triple x86_64-apple-darwin9 -fdiagnostics-parseable-fixits -fblocks %s 2>&1 | FileCheck -check-prefix=CHECK-64 %s
+
+int printf(const char * restrict, ...);
+
+#if __LP64__
+typedef long NSInteger;
+typedef unsigned long NSUInteger;
+typedef int SInt32;
+typedef unsigned int UInt32;
+
+#else
+
+typedef int NSInteger;
+typedef unsigned int NSUInteger;
+typedef long SInt32;
+typedef unsigned long UInt32;
+#endif
+
+NSInteger getNSInteger();
+NSUInteger getNSUInteger();
+SInt32 getSInt32();
+UInt32 getUInt32();
+
+void testCorrectionInAllCases() {
+  printf("%s", getNSInteger()); // expected-warning{{values of type 'NSInteger' should not be used as format arguments; add an explicit cast to 'long' instead}}
+  printf("%s", getNSUInteger()); // expected-warning{{values of type 'NSUInteger' should not be used as format arguments; add an explicit cast to 'unsigned long' instead}}
+  printf("%s", getSInt32()); // expected-warning{{values of type 'SInt32' should not be used as format arguments; add an explicit cast to 'int' instead}}
+  printf("%s", getUInt32()); // expected-warning{{values of type 'UInt32' should not be used as format arguments; add an explicit cast to 'unsigned int' instead}}
+
+  // CHECK: fix-it:"{{.*}}":{32:11-32:13}:"%ld"
+  // CHECK: fix-it:"{{.*}}":{32:16-32:16}:"(long)"
+
+  // CHECK: fix-it:"{{.*}}":{33:11-33:13}:"%lu"
+  // CHECK: fix-it:"{{.*}}":{33:16-33:16}:"(unsigned long)"
+
+  // CHECK: fix-it:"{{.*}}":{34:11-34:13}:"%d"
+  // CHECK: fix-it:"{{.*}}":{34:16-34:16}:"(int)"
+
+  // CHECK: fix-it:"{{.*}}":{35:11-35:13}:"%u"
+  // CHECK: fix-it:"{{.*}}":{35:16-35:16}:"(unsigned int)"
+}
+
+@interface Foo {
+@public
+  NSInteger _value;
+}
+- (NSInteger)getInteger;
+
+@property NSInteger value;
+@end
+
+struct Bar {
+  NSInteger value;
+};
+
+
+void testParens(Foo *obj, struct Bar *record) {
+  NSInteger arr[4] = {0};
+  NSInteger i = 0;
+
+  // These cases match the cases in CheckPrintfHandler::checkFormatExpr.
+  printf("%s", arr[0]);  // expected-warning{{values of type 'NSInteger' should not be used as format arguments; add an explicit cast to 'long' instead}}
+  printf("%s", getNSInteger());  // expected-warning{{values of type 'NSInteger' should not be used as format arguments; add an explicit cast to 'long' instead}}
+  printf("%s", i);  // expected-warning{{values of type 'NSInteger' should not be used as format arguments; add an explicit cast to 'long' instead}}
+  printf("%s", obj->_value);  // expected-warning{{values of type 'NSInteger' should not be used as format arguments; add an explicit cast to 'long' instead}}
+  printf("%s", [obj getInteger]);  // expected-warning{{values of type 'NSInteger' should not be used as format arguments; add an explicit cast to 'long' instead}}
+  printf("%s", obj.value);  // expected-warning{{values of type 'NSInteger' should not be used as format arguments; add an explicit cast to 'long' instead}}
+  printf("%s", record->value);  // expected-warning{{values of type 'NSInteger' should not be used as format arguments; add an explicit cast to 'long' instead}}
+  printf("%s", (i ? i : i));  // expected-warning{{values of type 'NSInteger' should not be used as format arguments; add an explicit cast to 'long' instead}}
+  printf("%s", *arr);  // expected-warning{{values of type 'NSInteger' should not be used as format arguments; add an explicit cast to 'long' instead}}
+
+  // CHECK-NOT: fix-it:{{.*}}:")"
+
+  printf("%s", i ? i : i); // expected-warning{{values of type 'NSInteger' should not be used as format arguments; add an explicit cast to 'long' instead}}
+
+  // CHECK: fix-it:"{{.*}}":{81:11-81:13}:"%ld"
+  // CHECK: fix-it:"{{.*}}":{81:16-81:16}:"(long)("
+  // CHECK: fix-it:"{{.*}}":{81:25-81:25}:")"
+}
+
+
+#if __LP64__
+
+void testWarn() {
+  printf("%d", getNSInteger()); // expected-warning{{values of type 'NSInteger' should not be used as format arguments; add an explicit cast to 'long' instead}}
+  printf("%u", getNSUInteger()); // expected-warning{{values of type 'NSUInteger' should not be used as format arguments; add an explicit cast to 'unsigned long' instead}}
+  printf("%ld", getSInt32()); // expected-warning{{values of type 'SInt32' should not be used as format arguments; add an explicit cast to 'int' instead}}
+  printf("%lu", getUInt32()); // expected-warning{{values of type 'UInt32' should not be used as format arguments; add an explicit cast to 'unsigned int' instead}}
+
+  // CHECK-64: fix-it:"{{.*}}":{92:11-92:13}:"%ld"
+  // CHECK-64: fix-it:"{{.*}}":{92:16-92:16}:"(long)"
+
+  // CHECK-64: fix-it:"{{.*}}":{93:11-93:13}:"%lu"
+  // CHECK-64: fix-it:"{{.*}}":{93:16-93:16}:"(unsigned long)"
+
+  // CHECK-64: fix-it:"{{.*}}":{94:11-94:14}:"%d"
+  // CHECK-64: fix-it:"{{.*}}":{94:17-94:17}:"(int)"
+
+  // CHECK-64: fix-it:"{{.*}}":{95:11-95:14}:"%u"
+  // CHECK-64: fix-it:"{{.*}}":{95:17-95:17}:"(unsigned int)"
+}
+
+void testPreserveHex() {
+  printf("%x", getNSInteger()); // expected-warning{{values of type 'NSInteger' should not be used as format arguments; add an explicit cast to 'long' instead}}
+  printf("%x", getNSUInteger()); // expected-warning{{values of type 'NSUInteger' should not be used as format arguments; add an explicit cast to 'unsigned long' instead}}
+
+  // CHECK-64: fix-it:"{{.*}}":{111:11-111:13}:"%lx"
+  // CHECK-64: fix-it:"{{.*}}":{111:16-111:16}:"(long)"
+
+  // CHECK-64: fix-it:"{{.*}}":{112:11-112:13}:"%lx"
+  // CHECK-64: fix-it:"{{.*}}":{112:16-112:16}:"(unsigned long)"
+}
+
+void testNoWarn() {
+  printf("%ld", getNSInteger()); // no-warning
+  printf("%lu", getNSUInteger()); // no-warning
+  printf("%d", getSInt32()); // no-warning
+  printf("%u", getUInt32()); // no-warning
+}
+
+#else
+
+void testWarn() {
+  printf("%ld", getNSInteger()); // expected-warning{{values of type 'NSInteger' should not be used as format arguments; add an explicit cast to 'long' instead}}
+  printf("%lu", getNSUInteger()); // expected-warning{{values of type 'NSUInteger' should not be used as format arguments; add an explicit cast to 'unsigned long' instead}}
+  printf("%d", getSInt32()); // expected-warning{{values of type 'SInt32' should not be used as format arguments; add an explicit cast to 'int' instead}}
+  printf("%u", getUInt32()); // expected-warning{{values of type 'UInt32' should not be used as format arguments; add an explicit cast to 'unsigned int' instead}}
+
+  // CHECK-32: fix-it:"{{.*}}":{131:17-131:17}:"(long)"
+
+  // CHECK-32: fix-it:"{{.*}}":{132:17-132:17}:"(unsigned long)"
+
+  // CHECK-32: fix-it:"{{.*}}":{133:16-133:16}:"(int)"
+
+  // CHECK-32: fix-it:"{{.*}}":{134:16-134:16}:"(unsigned int)"
+}
+
+void testPreserveHex() {
+  printf("%lx", getNSInteger()); // expected-warning{{values of type 'NSInteger' should not be used as format arguments; add an explicit cast to 'long' instead}}
+  printf("%lx", getNSUInteger()); // expected-warning{{values of type 'NSUInteger' should not be used as format arguments; add an explicit cast to 'unsigned long' instead}}
+
+  // CHECK-32: fix-it:"{{.*}}":{146:17-146:17}:"(long)"
+
+  // CHECK-32: fix-it:"{{.*}}":{147:17-147:17}:"(unsigned long)"
+}
+
+void testNoWarn() {
+  printf("%d", getNSInteger()); // no-warning
+  printf("%u", getNSUInteger()); // no-warning
+  printf("%ld", getSInt32()); // no-warning
+  printf("%lu", getUInt32()); // no-warning
+}
+
+#endif
+
+
+void testCasts() {
+  printf("%s", (NSInteger)0); // expected-warning{{values of type 'NSInteger' should not be used as format arguments; add an explicit cast to 'long' instead}}
+  printf("%s", (NSUInteger)0); // expected-warning{{values of type 'NSUInteger' should not be used as format arguments; add an explicit cast to 'unsigned long' instead}}
+  printf("%s", (SInt32)0); // expected-warning{{values of type 'SInt32' should not be used as format arguments; add an explicit cast to 'int' instead}}
+  printf("%s", (UInt32)0); // expected-warning{{values of type 'UInt32' should not be used as format arguments; add an explicit cast to 'unsigned int' instead}}
+
+  // CHECK: fix-it:"{{.*}}":{165:11-165:13}:"%ld"
+  // CHECK: fix-it:"{{.*}}":{165:16-165:27}:"(long)"
+
+  // CHECK: fix-it:"{{.*}}":{166:11-166:13}:"%lu"
+  // CHECK: fix-it:"{{.*}}":{166:16-166:28}:"(unsigned long)"
+
+  // CHECK: fix-it:"{{.*}}":{167:11-167:13}:"%d"
+  // CHECK: fix-it:"{{.*}}":{167:16-167:24}:"(int)"
+
+  // CHECK: fix-it:"{{.*}}":{168:11-168:13}:"%u"
+  // CHECK: fix-it:"{{.*}}":{168:16-168:24}:"(unsigned int)"
+}