[Sema][ObjC] Fix a -Wformat false positive with localizedStringForKey

Only honour format_arg attributes on -[NSBundle localizedStringForKey] when its
argument has a format specifier in it, otherwise its likely to just be a key to
fetch localized strings.

Fixes rdar://23622446

Differential revision: https://reviews.llvm.org/D27165

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

include/clang/AST/FormatString.h

@@ -748,6 +748,12 @@ bool ParseScanfString(FormatStringHandler &H,
                       const char *beg, const char *end, const LangOptions &LO,
                       const TargetInfo &Target);
 
+/// Return true if the given string has at least one formatting specifier.
+bool parseFormatStringHasFormattingSpecifiers(const char *Begin,
+                                              const char *End,
+                                              const LangOptions &LO,
+                                              const TargetInfo &Target);
+
 } // end analyze_format_string namespace
 } // end clang namespace
 #endif

include/clang/Basic/IdentifierTable.h

@@ -750,6 +750,12 @@ public:
     return getIdentifierInfoFlag() == ZeroArg;
   }
 
+  /// If this selector is the specific keyword selector described by Names.
+  bool isKeywordSelector(ArrayRef<StringRef> Names) const;
+
+  /// If this selector is the specific unary selector described by Name.
+  bool isUnarySelector(StringRef Name) const;
+
   unsigned getNumArgs() const;
 
   /// Retrieve the identifier at a given position in the selector.

lib/AST/PrintfFormatString.cpp

@@ -463,6 +463,23 @@ bool clang::analyze_format_string::ParseFormatStringHasSArg(const char *I,
   return false;
 }
 
+bool clang::analyze_format_string::parseFormatStringHasFormattingSpecifiers(
+    const char *Begin, const char *End, const LangOptions &LO,
+    const TargetInfo &Target) {
+  unsigned ArgIndex = 0;
+  // Keep looking for a formatting specifier until we have exhausted the string.
+  FormatStringHandler H;
+  while (Begin != End) {
+    const PrintfSpecifierResult &FSR =
+        ParsePrintfSpecifier(H, Begin, End, ArgIndex, LO, Target, false, false);
+    if (FSR.shouldStop())
+      break;
+    if (FSR.hasValue())
+      return true;
+  }
+  return false;
+}
+
 //===----------------------------------------------------------------------===//
 // Methods on PrintfSpecifier.
 //===----------------------------------------------------------------------===//

lib/Basic/IdentifierTable.cpp

@@ -412,6 +412,21 @@ public:
 
 } // namespace clang.
 
+bool Selector::isKeywordSelector(ArrayRef<StringRef> Names) const {
+  assert(!Names.empty() && "must have >= 1 selector slots");
+  if (getNumArgs() != Names.size())
+    return false;
+  for (unsigned I = 0, E = Names.size(); I != E; ++I) {
+    if (getNameForSlot(I) != Names[I])
+      return false;
+  }
+  return true;
+}
+
+bool Selector::isUnarySelector(StringRef Name) const {
+  return isUnarySelector() && getNameForSlot(0) == Name;
+}
+
 unsigned Selector::getNumArgs() const {
   unsigned IIF = getIdentifierInfoFlag();
   if (IIF <= ZeroArg)

lib/Sema/SemaChecking.cpp

@@ -6575,7 +6575,8 @@ static void CheckFormatString(Sema &S, const FormatStringLiteral *FExpr,
                               bool inFunctionCall,
                               Sema::VariadicCallType CallType,
                               llvm::SmallBitVector &CheckedVarArgs,
-                              UncoveredArgHandler &UncoveredArg);
+                              UncoveredArgHandler &UncoveredArg,
+                              bool IgnoreStringsWithoutSpecifiers);
 
 // Determine if an expression is a string literal or constant string.
 // If this function returns false on the arguments to a function expecting a
@@ -6588,7 +6589,8 @@ checkFormatStringExpr(Sema &S, const Expr *E, ArrayRef<const Expr *> Args,
                       Sema::VariadicCallType CallType, bool InFunctionCall,
                       llvm::SmallBitVector &CheckedVarArgs,
                       UncoveredArgHandler &UncoveredArg,
-                      llvm::APSInt Offset) {
+                      llvm::APSInt Offset,
+                      bool IgnoreStringsWithoutSpecifiers = false) {
   if (S.isConstantEvaluated())
     return SLCT_NotALiteral;
  tryAgain:
@@ -6639,17 +6641,17 @@ checkFormatStringExpr(Sema &S, const Expr *E, ArrayRef<const Expr *> Args,
       Left = checkFormatStringExpr(S, C->getTrueExpr(), Args,
                                    HasVAListArg, format_idx, firstDataArg,
                                    Type, CallType, InFunctionCall,
-                                   CheckedVarArgs, UncoveredArg, Offset);
+                                   CheckedVarArgs, UncoveredArg, Offset,
+                                   IgnoreStringsWithoutSpecifiers);
       if (Left == SLCT_NotALiteral || !CheckRight) {
         return Left;
       }
     }
 
-    StringLiteralCheckType Right =
-        checkFormatStringExpr(S, C->getFalseExpr(), Args,
-                              HasVAListArg, format_idx, firstDataArg,
-                              Type, CallType, InFunctionCall, CheckedVarArgs,
-                              UncoveredArg, Offset);
+    StringLiteralCheckType Right = checkFormatStringExpr(
+        S, C->getFalseExpr(), Args, HasVAListArg, format_idx, firstDataArg,
+        Type, CallType, InFunctionCall, CheckedVarArgs, UncoveredArg, Offset,
+        IgnoreStringsWithoutSpecifiers);
 
     return (CheckLeft && Left < Right) ? Left : Right;
   }
@@ -6753,7 +6755,8 @@ checkFormatStringExpr(Sema &S, const Expr *E, ArrayRef<const Expr *> Args,
         const Expr *Arg = CE->getArg(FA->getFormatIdx().getASTIndex());
         StringLiteralCheckType Result = checkFormatStringExpr(
             S, Arg, Args, HasVAListArg, format_idx, firstDataArg, Type,
-            CallType, InFunctionCall, CheckedVarArgs, UncoveredArg, Offset);
+            CallType, InFunctionCall, CheckedVarArgs, UncoveredArg, Offset,
+            IgnoreStringsWithoutSpecifiers);
         if (IsFirst) {
           CommonResult = Result;
           IsFirst = false;
@@ -6771,7 +6774,8 @@ checkFormatStringExpr(Sema &S, const Expr *E, ArrayRef<const Expr *> Args,
                                        HasVAListArg, format_idx,
                                        firstDataArg, Type, CallType,
                                        InFunctionCall, CheckedVarArgs,
-                                       UncoveredArg, Offset);
+                                       UncoveredArg, Offset,
+                                       IgnoreStringsWithoutSpecifiers);
         }
       }
     }
@@ -6780,12 +6784,28 @@ checkFormatStringExpr(Sema &S, const Expr *E, ArrayRef<const Expr *> Args,
   }
   case Stmt::ObjCMessageExprClass: {
     const auto *ME = cast<ObjCMessageExpr>(E);
-    if (const auto *ND = ME->getMethodDecl()) {
-      if (const auto *FA = ND->getAttr<FormatArgAttr>()) {
+    if (const auto *MD = ME->getMethodDecl()) {
+      if (const auto *FA = MD->getAttr<FormatArgAttr>()) {
+        // As a special case heuristic, if we're using the method -[NSBundle
+        // localizedStringForKey:value:table:], ignore any key strings that lack
+        // format specifiers. The idea is that if the key doesn't have any
+        // format specifiers then its probably just a key to map to the
+        // localized strings. If it does have format specifiers though, then its
+        // likely that the text of the key is the format string in the
+        // programmer's language, and should be checked.
+        const ObjCInterfaceDecl *IFace;
+        if (MD->isInstanceMethod() && (IFace = MD->getClassInterface()) &&
+            IFace->getIdentifier()->isStr("NSBundle") &&
+            MD->getSelector().isKeywordSelector(
+                {"localizedStringForKey", "value", "table"})) {
+          IgnoreStringsWithoutSpecifiers = true;
+        }
+
         const Expr *Arg = ME->getArg(FA->getFormatIdx().getASTIndex());
         return checkFormatStringExpr(
             S, Arg, Args, HasVAListArg, format_idx, firstDataArg, Type,
-            CallType, InFunctionCall, CheckedVarArgs, UncoveredArg, Offset);
+            CallType, InFunctionCall, CheckedVarArgs, UncoveredArg, Offset,
+            IgnoreStringsWithoutSpecifiers);
       }
     }
 
@@ -6809,7 +6829,8 @@ checkFormatStringExpr(Sema &S, const Expr *E, ArrayRef<const Expr *> Args,
       FormatStringLiteral FStr(StrE, Offset.sextOrTrunc(64).getSExtValue());
       CheckFormatString(S, &FStr, E, Args, HasVAListArg, format_idx,
                         firstDataArg, Type, InFunctionCall, CallType,
-                        CheckedVarArgs, UncoveredArg);
+                        CheckedVarArgs, UncoveredArg,
+                        IgnoreStringsWithoutSpecifiers);
       return SLCT_CheckedLiteral;
     }
 
@@ -8500,7 +8521,8 @@ static void CheckFormatString(Sema &S, const FormatStringLiteral *FExpr,
                               bool inFunctionCall,
                               Sema::VariadicCallType CallType,
                               llvm::SmallBitVector &CheckedVarArgs,
-                              UncoveredArgHandler &UncoveredArg) {
+                              UncoveredArgHandler &UncoveredArg,
+                              bool IgnoreStringsWithoutSpecifiers) {
   // CHECK: is the format string a wide literal?
   if (!FExpr->isAscii() && !FExpr->isUTF8()) {
     CheckFormatHandler::EmitFormatDiagnostic(
@@ -8521,6 +8543,11 @@ static void CheckFormatString(Sema &S, const FormatStringLiteral *FExpr,
   size_t StrLen = std::min(std::max(TypeSize, size_t(1)) - 1, StrRef.size());
   const unsigned numDataArgs = Args.size() - firstDataArg;
 
+  if (IgnoreStringsWithoutSpecifiers &&
+      !analyze_format_string::parseFormatStringHasFormattingSpecifiers(
+          Str, Str + StrLen, S.getLangOpts(), S.Context.getTargetInfo()))
+    return;
+
   // Emit a warning if the string literal is truncated and does not contain an
   // embedded null character.
   if (TypeSize <= StrRef.size() &&

test/SemaObjC/format-strings-objc.m

@@ -25,7 +25,11 @@ typedef struct _NSZone NSZone;
 @protocol NSCoding  - (void)encodeWithCoder:(NSCoder *)aCoder; @end
 @interface NSObject <NSObject> {} @end
 typedef float CGFloat;
-@interface NSString : NSObject <NSCopying, NSMutableCopying, NSCoding>    - (NSUInteger)length; @end
+@interface NSString : NSObject <NSCopying, NSMutableCopying, NSCoding>
+- (NSUInteger)length;
++(instancetype)stringWithFormat:(NSString *)fmt, ...
+    __attribute__((format(__NSString__, 1, 2)));
+@end
 @interface NSSimpleCString : NSString {} @end
 @interface NSConstantString : NSSimpleCString @end
 extern void *_NSConstantStringClassReference;
@@ -302,3 +306,39 @@ const char *rd23622446(const char *format) {
 }
 
 @end
+
+@interface NSBundle : NSObject
+- (NSString *)localizedStringForKey:(NSString *)key
+                              value:(nullable NSString *)value
+                              table:(nullable NSString *)tableName
+     __attribute__((format_arg(1)));
+
+- (NSString *)someRandomMethod:(NSString *)key
+                         value:(nullable NSString *)value
+                         table:(nullable NSString *)tableName
+    __attribute__((format_arg(1)));
+@end
+
+void useLocalizedStringForKey(NSBundle *bndl) {
+  [NSString stringWithFormat:
+              [bndl localizedStringForKey:@"%d" // expected-warning{{more '%' conversions than data arguments}}
+                                      value:0
+                                      table:0]];
+  // No warning, @"flerp" doesn't have a format specifier.
+  [NSString stringWithFormat: [bndl localizedStringForKey:@"flerp" value:0 table:0], 43, @"flarp"];
+
+  [NSString stringWithFormat:
+              [bndl localizedStringForKey:@"%f"
+                                    value:0
+                                    table:0], 42]; // expected-warning{{format specifies type 'double' but the argument has type 'int'}}
+
+  [NSString stringWithFormat:
+              [bndl someRandomMethod:@"%f"
+                               value:0
+                               table:0], 42]; // expected-warning{{format specifies type 'double' but the argument has type 'int'}}
+
+  [NSString stringWithFormat:
+              [bndl someRandomMethod:@"flerp"
+                               value:0
+                               table:0], 42]; // expected-warning{{data argument not used by format string}}
+}