Page MenuHomePhabricator

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

Authored by arphaman on Oct 20 2016, 4:51 AM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

arphaman updated this revision to Diff 75285.Oct 20 2016, 4:51 AM
arphaman retitled this revision from to [Sema][Objective-C] Formatting warnings should see through Objective-C message sends.
arphaman updated this object.
arphaman added a reviewer: manmanren.
arphaman set the repository for this revision to rL LLVM.
arphaman added a subscriber: cfe-commits.
aaron.ballman added inline comments.
test/SemaObjC/format-strings-objc.m
299

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

arphaman updated this revision to Diff 75408.Oct 21 2016, 3:44 AM
arphaman marked an inline comment as done.

The updated patch includes positive test cases.

aaron.ballman accepted this revision.Oct 21 2016, 8:26 AM
aaron.ballman added a reviewer: aaron.ballman.

LGTM, thank you!

This revision is now accepted and ready to land.Oct 21 2016, 8:26 AM
This revision was automatically updated to reflect the committed changes.

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.

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)).

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.

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.

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

  1. 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.

@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 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.