Add format_dynamic_key_arg attribute to improve "-Wformat" warnings for functions that load the formatting string dynamically based on a key value

[erik.pilkington]

Summary

This patch adds a new attribute called format_dynamic_key_arg. It's intended to be used instead of format_arg for methods/functions that load the formatting string from some external source based on the given key value. The attribute tells Clang that it has to avoid -Wformat warnings when key strings have no formatting specifiers. Otherwise it tells Clang that it can assume that the key strings use the same formatting layout as the external formatting string values (i.e. Clang can perform the normal -Wformat related checks).

This attribute is useful for certain Objective-C methods like NSBundle.localizedStringForKey that load the values dynamically based on a specified key value.

[arphaman]

Ping

[aaron.ballman]

include/clang/Basic/Attr.td

862 ↗

(On Diff #79404)

Does GCC support this attribute as well? If not, this should use the GNU spelling instead of the GCC one. Also, should this have a C++11 spelling in the clang namespace?

The name doesn't really convey much about what the attribute is doing, mostly because it doesn't seem obvious what "key" means.

It seems that the crux of what this attribute says is that the attributed function accepts a string argument of a format specifier and returns the same format specifier. Perhaps returns_format_specifier is a better name?

lib/Sema/SemaDeclAttr.cpp

5632 ↗

(On Diff #79404)

We ask that all of these handle*Attr() functions have the same signature, so adding extra parameters isn't something we do. At some point, we want to generate more boilerplate code from the attribute definition files, and so having different parameter lists makes that much harder to accomplish. A better approach would be to make a function called handleFormatDynamicKeyArg() that calls a helper function with the extra argument, and modify handleFormatArgAttr() to call that helper function as well.

[arphaman]

The updated patch renames the attribute to loads_format_specifier_value_using_key and addresses Aaron's comments

include/clang/Basic/Attr.td

862 ↗

(On Diff #79404)

Does GCC support this attribute as well? If not, this should use the GNU spelling instead of the GCC one.

No, it doesn't. Thanks for pointing that out, I fixed it.

Also, should this have a C++11 spelling in the clang namespace?

I don't know, is that required for new attributes? I don't need the C++11 spelling personally, and I don't know if there is anyone else who's interested in this attribute.

The name doesn't really convey much about what the attribute is doing, mostly because it doesn't seem obvious what "key" means.

It seems that the crux of what this attribute says is that the attributed function accepts a string argument of a format specifier and returns the same format specifier. Perhaps returns_format_specifier is a better name?

You're right, the name should convey the intended meaning better. I switched to loads_format_specifier_value_using_key, how does that sound?
I think it sounds better than just returns_format_specifier because I would like to see this attribute used only for a particular kind of function. This kind of function should load the value at runtime using the key from a platform-specific file/database that might also be accessible at compile-time. It shouldn't be used for functions that might just modify the given key, which, IMO, returns_format_specifier can imply.

[arphaman]

Update to fix a test failure.

[aaron.ballman]

include/clang/Basic/Attr.td

862 ↗

(On Diff #79404)

Also, should this have a C++11 spelling in the clang namespace?

I don't know, is that required for new attributes? I don't need the C++11 spelling personally, and I don't know if there is anyone else who's interested in this attribute.

It's not required, but not everyone likes GNU-style attributes, so having something a bit less awful is a kindness.

The name doesn't really convey much about what the attribute is doing, mostly because it doesn't seem obvious what "key" means.
It seems that the crux of what this attribute says is that the attributed function accepts a string argument of a format specifier and returns the same format specifier. Perhaps returns_format_specifier is a better name?

You're right, the name should convey the intended meaning better. I switched to loads_format_specifier_value_using_key, how does that sound?
I think it sounds better than just returns_format_specifier because I would like to see this attribute used only for a particular kind of function. This kind of function should load the value at runtime using the key from a platform-specific file/database that might also be accessible at compile-time. It shouldn't be used for functions that might just modify the given key, which, IMO, returns_format_specifier can imply.

It's a bit wordy, but I think it's better than the original name.

include/clang/Basic/AttrDocs.td

1759 ↗

(On Diff #81952)

value -> specifier (same below).

1766 ↗

(On Diff #81952)

Given that this is uses novel terminology like "key string", I think an example would be useful to add.

The docs should also mention that this attribute accepts an argument, and that the argument is 1-based instead of 0-based (I can't count how many times that's tripped me up).

lib/Sema/SemaChecking.cpp

4408 ↗

(On Diff #81952)

I think the original formatting was easier to read.

4418 ↗

(On Diff #81952)

Same here.

4593 ↗

(On Diff #81952)

Can use const auto * here.

6303 ↗

(On Diff #81952)

The original formatting was better.

test/Sema/attr-format_arg.c

29 ↗

(On Diff #81952)

You should also add tests for:

it diagnoses if the attribute is added to something other than a function/method.
it diagnoses if the attribute has no arguments/arguments of the wrong type.
it works properly on a class member function in C++.

[erik.pilkington]

Stealing this (with Alex's permission). I think it makes more sense to add a special case for the method -[NSBundle localizedStringForKey:value:table:] rather then add a new general purpose attribute that'll only be used in one place.

[erik.pilkington]

Special case localizedStringForKey instead of using an attribute.

[rjmccall]

I agree with just special-casing this for now.

clang/lib/Sema/SemaChecking.cpp

6800 ↗

(On Diff #214925)

It's more efficient to do these checks with isStr instead of looking up an IdentifierInfo. You can add a Selector::isKeywordSelector(ArrayRef<StringRef>) method to make that equally convenient for selectors.

[erik.pilkington]

Address review comments.

[rjmccall]

One minor request (which will be dead code in your patch), but otherwise LGTM.

clang/include/clang/Basic/IdentifierTable.h

754 ↗

(On Diff #214977)

Thanks. Could you go ahead and add isUnarySelector(StringRef) for consistency?

[aaron.ballman]

I like this approach much better! LGTM, aside from a minor nit.

clang/include/clang/Basic/IdentifierTable.h

754 ↗

(On Diff #214977)

Also, the function should be marked const, I would imagine.

cfe/trunk/include/clang/AST/FormatString.h

 
 bool ParseFormatStringHasSArg(const char *beg, const char *end,
                               const LangOptions &LO, const TargetInfo &Target);
 
 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

cfe/trunk/include/clang/Basic/IdentifierTable.h

public:
   bool isKeywordSelector() const {
     return getIdentifierInfoFlag() != ZeroArg;
   }
 
   bool isUnarySelector() const {
     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.
   ///
   /// Note that the identifier pointer returned may be NULL. Clients that only
   /// care about the text of the identifier string, and not the specific,
   /// uniqued identifier pointer, should use \c getNameForSlot(), which returns
   /// an empty string when the identifier pointer would be NULL.

cfe/trunk/lib/AST/PrintfFormatString.cpp

while (I != E) {
     const analyze_printf::PrintfSpecifier &FS = FSR.getValue();
     // Return true if this a %s format specifier.
     if (FS.getConversionSpecifier().getKind() == ConversionSpecifier::Kind::sArg)
       return true;
   }
   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.
 //===----------------------------------------------------------------------===//
 
 ArgType PrintfSpecifier::getScalarArgType(ASTContext &Ctx,
                                           bool IsObjCLiteral) const {
   if (CS.getKind() == ConversionSpecifier::cArg)
     switch (LM.getKind()) {

cfe/trunk/lib/Basic/IdentifierTable.cpp

public:
 
   void Profile(llvm::FoldingSetNodeID &ID) {
     Profile(ID, keyword_begin(), getNumArgs());
   }
 };
 
 } // 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)
     return 0;
   if (IIF == OneArg)
     return 1;
   // We point to a MultiKeywordSelector.
   MultiKeywordSelector *SI = getMultiKeywordSelector();

cfe/trunk/lib/Sema/SemaChecking.cpp

static void CheckFormatString(Sema &S, const FormatStringLiteral *FExpr,
                               const Expr *OrigFormatExpr,
                               ArrayRef<const Expr *> Args,
                               bool HasVAListArg, unsigned format_idx,
                               unsigned firstDataArg,
                               Sema::FormatStringType Type,
                               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
 // format string, we will usually need to emit a warning.
 // True string literals are then checked by CheckFormatString.
 static StringLiteralCheckType
 checkFormatStringExpr(Sema &S, const Expr *E, ArrayRef<const Expr *> Args,
                       bool HasVAListArg, unsigned format_idx,
                       unsigned firstDataArg, Sema::FormatStringType Type,
                       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:
   assert(Offset.isSigned() && "invalid offset");
 
   if (E->isTypeDependent() || E->isValueDependent())
     return SLCT_NotALiteral;
 
case Stmt::ConditionalOperatorClass: {
     // literals in the end.
     StringLiteralCheckType Left;
     if (!CheckLeft)
       Left = SLCT_UncheckedLiteral;
     else {
       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;
   }
 
   case Stmt::ImplicitCastExprClass:
     E = cast<ImplicitCastExpr>(E)->getSubExpr();
     goto tryAgain;
 
case Stmt::CXXMemberCallExprClass: {
     const CallExpr *CE = cast<CallExpr>(E);
     if (const NamedDecl *ND = dyn_cast_or_null<NamedDecl>(CE->getCalleeDecl())) {
       bool IsFirst = true;
       StringLiteralCheckType CommonResult;
       for (const auto *FA : ND->specific_attrs<FormatArgAttr>()) {
         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;
         }
       }
       if (!IsFirst)
         return CommonResult;
 
       if (const auto *FD = dyn_cast<FunctionDecl>(ND)) {
         unsigned BuiltinID = FD->getBuiltinID();
         if (BuiltinID == Builtin::BI__builtin___CFStringMakeConstantString ||
             BuiltinID == Builtin::BI__builtin___NSStringMakeConstantString) {
           const Expr *Arg = CE->getArg(0);
           return checkFormatStringExpr(S, Arg, Args,
                                        HasVAListArg, format_idx,
                                        firstDataArg, Type, CallType,
                                        InFunctionCall, CheckedVarArgs,
-                                       UncoveredArg, Offset);
+                                       UncoveredArg, Offset,
+                                       IgnoreStringsWithoutSpecifiers);
         }
       }
     }
 
     return SLCT_NotALiteral;
   }
   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);
       }
     }
 
     return SLCT_NotALiteral;
   }
   case Stmt::ObjCStringLiteralClass:
   case Stmt::StringLiteralClass: {
     const StringLiteral *StrE = nullptr;
 
     if (const ObjCStringLiteral *ObjCFExpr = dyn_cast<ObjCStringLiteral>(E))
       StrE = ObjCFExpr->getString();
     else
       StrE = cast<StringLiteral>(E);
 
     if (StrE) {
       if (Offset.isNegative() || Offset > StrE->getLength()) {
         // TODO: It would be better to have an explicit warning for out of
         // bounds literals.
         return SLCT_NotALiteral;
       }
       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;
     }
 
     return SLCT_NotALiteral;
   }
   case Stmt::BinaryOperatorClass: {
     const BinaryOperator *BinOp = cast<BinaryOperator>(E);
 
static void CheckFormatString(Sema &S, const FormatStringLiteral *FExpr,
                               const Expr *OrigFormatExpr,
                               ArrayRef<const Expr *> Args,
                               bool HasVAListArg, unsigned format_idx,
                               unsigned firstDataArg,
                               Sema::FormatStringType Type,
                               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(
         S, inFunctionCall, Args[format_idx],
         S.PDiag(diag::warn_format_string_is_wide_literal), FExpr->getBeginLoc(),
         /*IsStringLocation*/ true, OrigFormatExpr->getSourceRange());
     return;
   }
 
   // Str - The format string.  NOTE: this is NOT null-terminated!
   StringRef StrRef = FExpr->getString();
   const char *Str = StrRef.data();
   // Account for cases where the string literal is truncated in a declaration.
   const ConstantArrayType *T =
     S.Context.getAsConstantArrayType(FExpr->getType());
   assert(T && "String literal not of constant array type!");
   size_t TypeSize = T->getSize().getZExtValue();
   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() &&
       StrRef.substr(0, TypeSize).find('\0') == StringRef::npos) {
     CheckFormatHandler::EmitFormatDiagnostic(
         S, inFunctionCall, Args[format_idx],
         S.PDiag(diag::warn_printf_format_string_not_null_terminated),
         FExpr->getBeginLoc(),

cfe/trunk/test/SemaObjC/format-strings-objc.m

 typedef struct _NSZone NSZone;
 @class NSInvocation, NSMethodSignature, NSCoder, NSString, NSEnumerator;
 @protocol NSObject  - (BOOL)isEqual:(id)object; @end
 @protocol NSCopying  - (id)copyWithZone:(NSZone *)zone; @end
 @protocol NSMutableCopying  - (id)mutableCopyWithZone:(NSZone *)zone; @end
 @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;
 
 typedef const struct __CFString * CFStringRef;
 extern void CFStringCreateWithFormat(CFStringRef format, ...) __attribute__((format(CFString, 1, 2)));
 #define CFSTR(cStr)  ((CFStringRef) __builtin___CFStringMakeConstantString ("" cStr ""))
 
- (void)foo {
   [RD23622446_Tester stringWithFormat:[self test: "%s %s"], "name"]; // expected-warning {{more '%' conversions than data arguments}}
   NSLog([self str: @"%@ %@"], @"name"); // expected-warning {{more '%' conversions than data arguments}}
   [RD23622446_Tester stringWithFormat:rd23622446("%d"), 1]; // ok
   [RD23622446_Tester stringWithFormat:[self test: "%d %d"], 1, 2]; // ok
   NSLog([self str: @"%@"], @"string"); // ok
 }
 
 @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}}
+}