[Sema][Objective-C] Formatting warnings should see through Objective-C message sends

[arphaman]

Summary

This patch improves the '-Wformat' warnings by ensuring that the formatting checker can see through Objective-C message sends when we are calling an Objective-C method with an appropriate format_arg attribute.

[aaron.ballman]

test/SemaObjC/format-strings-objc.m

299 ↗

(On Diff #75285)

Can you add an example showing a positive use case as well?

[arphaman]

The updated patch includes positive test cases.

[aaron.ballman]

LGTM, thank you!

[benhamilton]

This caused a regression where __attribute__((format_arg(X)) no longer works with ObjC message sends.

I filed https://bugs.llvm.org/show_bug.cgi?id=36599 and will write a fix.

[benhamilton]

Actually, looking more deeply, it's possible this is a bug in the Apple SDK:

@interface NSBundle
- (NSString *)localizedStringForKey:(NSString *)key value:(nullable NSString *)value table:(nullable NSString *)tableName __attribute__((format_arg(1)));
@end

Since format_arg(X) is 1-based, and value is the format param here, I wonder if this shouldn't be __attribute__((format_arg(2)).

[aaron.ballman]

In D25820#1027216, @benhamilton wrote:

Actually, looking more deeply, it's possible this is a bug in the Apple SDK:

@interface NSBundle
- (NSString *)localizedStringForKey:(NSString *)key value:(nullable NSString *)value table:(nullable NSString *)tableName __attribute__((format_arg(1)));
@end

Since format_arg(X) is 1-based, and value is the format param here, I wonder if this shouldn't be __attribute__((format_arg(2)).

That does look like a bug in the SDK, to me.

[benhamilton]

That does look like a bug in the SDK, to me.

OK. I'll file a Radar bug upstream; in the meantime, this means since this diff landed in 2016, clang has been raising -Wformat-extra-args when compiling any code which passes the result of -[NSBundle localizedStringForKey:value:table:] to +[NSString stringWithFormat:] and friends (which is a fairly common operation—any localized string with an argument will do this).

I assume Apple will fix this in the SDK the next time they integrate their fork of clang from master.

[benhamilton]

I filed rdar://38143508 to track this.

The fix won't be easy; I think __attribute__((format_arg(X))) isn't enough to capture how -[NSBundle localizedStringForKey:value:table:] works:

1. __attribute__((format_arg(X))) doesn't support multiple format arguments, and it appears to ignore everything but the last such declaration:

https://github.com/llvm-mirror/clang/blob/master/lib/Sema/SemaChecking.cpp#L5030

2. NSLocalizedString() and friends pass @"" as the value argument, which raises -Wformat-zero-length if we tag it as a format argument:

test.m:21:12: error: format string is empty [-Werror,-Wformat-zero-length]
          @"",
          ~^~
test.m:14:46: note: expanded from macro 'NSLocalizedStringWithDefaultValue'
  [bundle localizedStringForKey:(key) value:(val) table:(tbl)]
                                             ^~~
1 error generated.

This means we'll either need a new attribute which allows a zero-length format string, or a way to change format_arg to understand this "argument with default value" semantic.

[arphaman]

@benhamilton We're planning to use the new attribute in https://reviews.llvm.org/D27165 to address this issue. I'll get back to that patch in the near future.

[benhamilton]

>@benhamilton We're planning to use the new attribute in https://reviews.llvm.org/D27165 to address this issue. I'll get back to that patch in the near future.

Great, thanks for the update.

cfe/trunk/lib/Sema/SemaChecking.cpp

if (const NamedDecl *ND = dyn_cast_or_null<NamedDecl>(CE->getCalleeDecl())) {
                                        InFunctionCall, CheckedVarArgs,
                                        UncoveredArg, Offset);
         }
       }
     }
 
     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>()) {
+        unsigned ArgIndex = FA->getFormatIdx();
+        const Expr *Arg = ME->getArg(ArgIndex - 1);
+        return checkFormatStringExpr(
+            S, Arg, Args, HasVAListArg, format_idx, firstDataArg, Type,
+            CallType, InFunctionCall, CheckedVarArgs, UncoveredArg, Offset);
+      }
+    }
+
+    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);

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

void testObjCModifierFlags() {
   NSLog(@"%[tt", @"Foo");  // expected-warning {{incomplete format specifier}}
   NSLog(@"%[tt]@", @"Foo"); // no-warning
   NSLog(@"%[tt]@ %s", @"Foo", "hello"); // no-warning
   NSLog(@"%s %[tt]@", "hello", @"Foo"); // no-warning
   NSLog(@"%[blark]@", @"Foo"); // expected-warning {{'blark' is not a valid object format flag}}
   NSLog(@"%2$[tt]@ %1$[tt]@", @"Foo", @"Bar"); // no-warning
   NSLog(@"%2$[tt]@ %1$[tt]s", @"Foo", @"Bar"); // expected-warning {{object format flags cannot be used with 's' conversion specifier}}
 }
+
+// rdar://23622446
+@interface RD23622446_Tester: NSObject
+
++ (void)stringWithFormat:(const char *)format, ... __attribute__((format(__printf__, 1, 2)));
+
+@end
+
+@implementation RD23622446_Tester
+
+__attribute__ ((format_arg(1)))
+const char *rd23622446(const char *format) {
+  return format;
+}
+
++ (void)stringWithFormat:(const char *)format, ... {
+  return;
+}
+
+- (const char *)test:(const char *)format __attribute__ ((format_arg(1))) {
+  return format;
+}
+
+- (NSString *)str:(NSString *)format __attribute__ ((format_arg(1))) {
+  return format;
+}
+
+- (void)foo {
+  [RD23622446_Tester stringWithFormat:rd23622446("%u"), 1, 2]; // expected-warning {{data argument not used by format string}}
+  [RD23622446_Tester stringWithFormat:[self test: "%u"], 1, 2]; // expected-warning {{data argument not used by format string}}
+  [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