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

[fcloutier]

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

[fcloutier]

Forgot to run clang-format.

[ahatanak]

clang/lib/Sema/SemaDeclAttr.cpp

3403

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]

clang/lib/Sema/SemaDeclAttr.cpp

3403

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.

[fcloutier]

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]

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

[ahatanak]

LGTM

clang/lib/Sema/SemaDeclAttr.cpp

3392

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

3403

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

clang/lib/Sema/SemaDeclAttr.cpp

 }
 
 static bool isInstanceMethod(const Decl *D) {
   if (const auto *MethodDecl = dyn_cast<CXXMethodDecl>(D))
     return MethodDecl->isInstance();
   return false;
 }
 
+static inline bool isNSStringInterface(ASTContext &Ctx, ObjCInterfaceDecl *Cls, bool AllowNSAttributedString = false) {
+  if (!Cls)
+    return false;
+
+  IdentifierInfo *ClsName = Cls->getIdentifier();
+
+  if (ClsName == &Ctx.Idents.get("NSAttributedString"))
+    return AllowNSAttributedString;
+  // FIXME: Should we walk the chain of classes?
+  return ClsName == &Ctx.Idents.get("NSString") ||
+         ClsName == &Ctx.Idents.get("NSMutableString");
+}
+
 static inline bool isNSStringType(QualType T, ASTContext &Ctx,
                                   bool AllowNSAttributedString = false) {
   const auto *PT = T->getAs<ObjCObjectPointerType>();
   if (!PT)
     return false;
   
-  ObjCInterfaceDecl *Cls = PT->getObjectType()->getInterface();
-  if (!Cls)
+  return isNSStringInterface(Ctx, PT->getObjectType()->getInterface(), AllowNSAttributedString);
+}
+
+static inline bool isInstancetypeNSStringType(ASTContext &Ctx, QualType T, Decl *D, bool AllowNSAttributedString = false) {
+  if (T.getTypePtr() != Ctx.getObjCInstanceTypeDecl()->getTypeForDecl())
     return false;
   
-  IdentifierInfo* ClsName = Cls->getIdentifier();
+  auto *OMD = dyn_cast<ObjCMethodDecl>(D);
+  if (!OMD)
+    return false;
   
-  if (AllowNSAttributedString &&
-      ClsName == &Ctx.Idents.get("NSAttributedString"))
-    return true;
-  // FIXME: Should we walk the chain of classes?
-  return ClsName == &Ctx.Idents.get("NSString") ||
-         ClsName == &Ctx.Idents.get("NSMutableString");
+  return isNSStringInterface(Ctx, OMD->getClassInterface(), AllowNSAttributedString);
 }
 
 static inline bool isCFStringType(QualType T, ASTContext &Ctx) {
   const auto *PT = T->getAs<PointerType>();
   if (!PT)
     return false;
 
   const auto *RT = PT->getPointeeType()->getAs<RecordType>();

 static void handleFormatArgAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
   Expr *IdxExpr = AL.getArgAsExpr(0);
   ParamIdx Idx;
   if (!checkFunctionOrMethodParameterIndex(S, D, AL, 1, IdxExpr, Idx))
     return;
 
   // Make sure the format string is really a string.
   QualType Ty = getFunctionOrMethodParamType(D, Idx.getASTIndex());
 
   bool NotNSStringTy = !isNSStringType(Ty, S.Context);
   if (NotNSStringTy &&
       !isCFStringType(Ty, S.Context) &&
       (!Ty->isPointerType() ||
        !Ty->castAs<PointerType>()->getPointeeType()->isCharType())) {
     S.Diag(AL.getLoc(), diag::err_format_attribute_not)
         << "a string type" << IdxExpr->getSourceRange()
         << getFunctionOrMethodParamRange(D, 0);
     return;
   }
   Ty = getFunctionOrMethodResultType(D);
   if (!isNSStringType(Ty, S.Context, /*AllowNSAttributedString=*/true) &&
       !isCFStringType(Ty, S.Context) &&
+      !isInstancetypeNSStringType(S.Context, Ty, D) &&
       (!Ty->isPointerType() ||
        !Ty->castAs<PointerType>()->getPointeeType()->isCharType())) {
     S.Diag(AL.getLoc(), diag::err_format_attribute_result_not)
         << (NotNSStringTy ? "string type" : "NSString")
         << IdxExpr->getSourceRange() << getFunctionOrMethodParamRange(D, 0);
     return;
   }
 

clang/test/SemaObjC/format-arg-attribute.m

 // RUN: %clang_cc1 -verify -fsyntax-only %s
 
-@class NSString;
+@interface NSString
++(instancetype)stringWithCString:(const char *)cstr __attribute__((format_arg(1)));
+@end
+
 @class NSAttributedString;
 
 extern NSString *fa2 (const NSString *) __attribute__((format_arg(1)));
 extern NSString *fa3 (NSString *) __attribute__((format_arg(1)));
 
 extern void fc1 (const NSString *) __attribute__((format_arg));  // expected-error {{'format_arg' attribute takes one argument}}
 extern void fc2 (const NSString *) __attribute__((format_arg())); // expected-error {{'format_arg' attribute takes one argument}}
 extern void fc3 (const NSString *) __attribute__((format_arg(1, 2))); // expected-error {{'format_arg' attribute takes one argument}}