This is an archive of the discontinued LLVM Phabricator instance.

__attribute__((format_arg(__NSString__, N))) does not support instancetype in NSString interface
ClosedPublic

Authored by fcloutier on Oct 27 2021, 3:32 PM.

Details

Summary

It has come to my attention that _inside the interface of NSString_, using __attribute__((format_arg(__NSString__, N))) does not work for a method that returns instancetype. This is a pretty specific problem because not a lot of people define NSString. Thankfully, the fix isn't very difficult: if checking an Objective-C method definition, and it returns instancetype, we also check if the class being defined is NSString.

rdar://84729746

Diff Detail

Event Timeline

fcloutier created this revision.Oct 27 2021, 3:32 PM
fcloutier requested review of this revision.Oct 27 2021, 3:32 PM
fcloutier updated this revision to Diff 383179.Oct 28 2021, 3:29 PM

Forgot to run clang-format.

ahatanak added inline comments.Oct 28 2021, 6:01 PM
clang/lib/Sema/SemaDeclAttr.cpp
3390–3396

Is it possible to just replace Ty with the class pointer type here if it is an instancetype instead of defining another function (isNSStringInterface) that looks at the class name and determines whether it's NSString?

fcloutier added inline comments.Oct 28 2021, 7:35 PM
clang/lib/Sema/SemaDeclAttr.cpp
3390–3396

I think that it would be awkward and I'd like to offer modest pushback. You need the enclosing Decl to make sense of instancetype because it's just a typedef to id. There are three uses of isNSStringType and only one could benefit from the Decl it because the other two don't refer to a return type. IMO, the split is the right way to go.

FWIW, it's not so much defining another function that looks at the class name so much as moving the look-at-the-class-name logic out of an existing function. I took the guts out of isNSStringType to make isNSStringInterface, but Phabricator isn't very good at showing code that moved.

Apologies, Phabricator showed the comment below line 197 in the diff, but the email showed it to be below line 3404. I can check if the return type is instancetype in handleFormatArgAttr and use that instead.

fcloutier updated this revision to Diff 383222.Oct 28 2021, 8:01 PM
fcloutier updated this revision to Diff 383227.Oct 28 2021, 8:20 PM

Add test for a protocol method with format_arg, second NSString method accepting a NSString instead of a C string

ahatanak accepted this revision.Oct 28 2021, 8:24 PM

LGTM

clang/lib/Sema/SemaDeclAttr.cpp
3390–3396

Oh I see. But I still feel this is better since isNSStringInterface would be called twice in the previous patch.

3392

You can use ASTContext::getObjCInstanceType, which returns QualType.

This revision is now accepted and ready to land.Oct 28 2021, 8:24 PM
This revision was landed with ongoing or failed builds.Oct 28 2021, 8:32 PM
This revision was automatically updated to reflect the committed changes.