[Sema] Delay checking whether objc_designated_initializer is being applied to an init method

[erik.pilkington]

Summary

We ran into some source breaking changes to do with the attribute reordering changes that recently landed. The problem is that the order that objc_designated_initializer and objc_method_family are processed matters, because objc_method_family can make a method an init method, which is a prerequisite for objc_designated_initializer. When the attribute order flipped, the arrangement of these attributes that lead to an error and the arrangement that worked fine also flipped. This patch fixes this by delaying the check until both attributes have been processed.

rdar://47829358

Thanks!
Erik

[aaron.ballman]

clang/lib/Sema/SemaDeclAttr.cpp

5252 ↗

(On Diff #186551)

Please don't use auto here.

7258–7267 ↗

(On Diff #186551)

Do you also have to handle redeclaration merging, or is that not a thing for ObjC method declarations?

I'm wondering if this is better handled with the mergeFooAttr() pattern from Sema::mergeDeclAttribute()?

[erik.pilkington]

Remove an auto.

clang/lib/Sema/SemaDeclAttr.cpp

7258–7267 ↗

(On Diff #186551)

You mean delaying the check until we see the method implementation? It seems like that would only enable the following:

@interface X
-(instancetype)meth __attribute__((objc_designated_initializer));
@end
@implementation X
-(instancetype)meth __attribute__((objc_method_family(init))) {}
@end

We never supported this, so there isn't any regression here. I don't even think that this makes sense, the objc_method_family attribute should really go in the @interface. It also means that we'd only diagnose this in the TU that contains the @implementation.

[aaron.ballman]

LGTM with a testing request.

clang/test/SemaObjC/attr-designated-init.m

433–438 ↗

(On Diff #186712)

For coverage purposes, can you add these tests as well?

- (instancetype)baz
    __attribute__((objc_designated_initializer, objc_method_family(init)))
- (instancetype)quux
    __attribute__((objc_method_family(init), objc_designated_initializer))

include/clang/Basic/Attr.td

def NonStaticNonConstCXXMethod
     : SubsetSubject<CXXMethod,
                     [{!S->isStatic() && !S->isConst()}],
                     "non-static non-const member functions">;
 
 def ObjCInstanceMethod : SubsetSubject<ObjCMethod,
                                        [{S->isInstanceMethod()}],
                                        "Objective-C instance methods">;
 
-def ObjCInterfaceDeclInitMethod : SubsetSubject<ObjCMethod,
-                               [{S->getMethodFamily() == OMF_init &&
-                                 (isa<ObjCInterfaceDecl>(S->getDeclContext()) ||
-                                  (isa<ObjCCategoryDecl>(S->getDeclContext()) &&
-            cast<ObjCCategoryDecl>(S->getDeclContext())->IsClassExtension()))}],
-            "init methods of interface or class extension declarations">;
-
 def Struct : SubsetSubject<Record,
                            [{!S->isUnion()}], "structs">;
 
 def TLSVar : SubsetSubject<Var,
                            [{S->getTLSKind() != 0}], "thread-local variables">;
 
 def SharedVar : SubsetSubject<Var,
                               [{S->hasGlobalStorage() && !S->getTLSKind()}],

 def ObjCExplicitProtocolImpl : InheritableAttr {
   let Spellings = [Clang<"objc_protocol_requires_explicit_implementation">];
   let Subjects = SubjectList<[ObjCProtocol], ErrorDiag>;
   let Documentation = [Undocumented];
 }
 
 def ObjCDesignatedInitializer : Attr {
   let Spellings = [Clang<"objc_designated_initializer">];
-  let Subjects = SubjectList<[ObjCInterfaceDeclInitMethod], ErrorDiag>;
+  let Subjects = SubjectList<[ObjCMethod], ErrorDiag>;
   let Documentation = [Undocumented];
 }
 
 def ObjCRuntimeName : Attr {
   let Spellings = [Clang<"objc_runtime_name">];
   let Subjects = SubjectList<[ObjCInterface, ObjCProtocol], ErrorDiag>;
   let Args = [StringArgument<"MetadataName">];
   let Documentation = [ObjCRuntimeNameDocs];

include/clang/Basic/DiagnosticSemaKinds.td

def warn_objc_secondary_init_super_init_call : Warning<
   "convenience initializer should not invoke an initializer on 'super'">,
   InGroup<ObjCDesignatedInit>;
 def warn_objc_secondary_init_missing_init_call : Warning<
   "convenience initializer missing a 'self' call to another initializer">,
   InGroup<ObjCDesignatedInit>;
 def warn_objc_implementation_missing_designated_init_override : Warning<
   "method override for the designated initializer of the superclass %objcinstance0 not found">,
   InGroup<ObjCDesignatedInit>;
+def err_designated_init_attr_non_init : Error<
+  "'objc_designated_initializer' attribute only applies to init methods "
+  "of interface or class extension declarations">;
 
 // objc_bridge attribute diagnostics.
 def err_objc_attr_not_id : Error<
   "parameter of %0 attribute must be a single name of an Objective-C %select{class|protocol}1">;
 def err_objc_attr_typedef_not_id : Error<
   "parameter of %0 attribute must be 'id' when used on a typedef">;
 def err_objc_attr_typedef_not_void_pointer : Error<
   "'objc_bridge(id)' is only allowed on structs and typedefs of void pointers">;

lib/Sema/SemaDeclAttr.cpp

static void handleObjCBridgeRelatedAttr(Sema &S, Decl *D,
   D->addAttr(::new (S.Context)
              ObjCBridgeRelatedAttr(AL.getRange(), S.Context, RelatedClass,
                                    ClassMethod, InstanceMethod,
                                    AL.getAttributeSpellingListIndex()));
 }
 
 static void handleObjCDesignatedInitializer(Sema &S, Decl *D,
                                             const ParsedAttr &AL) {
+  DeclContext *Ctx = D->getDeclContext();
+
+  // This attribute can only be applied to methods in interfaces or class
+  // extensions.
+  if (!isa<ObjCInterfaceDecl>(Ctx) &&
+      !(isa<ObjCCategoryDecl>(Ctx) &&
+        cast<ObjCCategoryDecl>(Ctx)->IsClassExtension())) {
+    S.Diag(D->getLocation(), diag::err_designated_init_attr_non_init);
+    return;
+  }
+
   ObjCInterfaceDecl *IFace;
-  if (auto *CatDecl = dyn_cast<ObjCCategoryDecl>(D->getDeclContext()))
+  if (auto *CatDecl = dyn_cast<ObjCCategoryDecl>(Ctx))
     IFace = CatDecl->getClassInterface();
   else
-    IFace = cast<ObjCInterfaceDecl>(D->getDeclContext());
+    IFace = cast<ObjCInterfaceDecl>(Ctx);
 
   if (!IFace)
     return;
 
   IFace->setHasDesignatedInitializers();
   D->addAttr(::new (S.Context)
                   ObjCDesignatedInitializerAttr(AL.getRange(), S.Context,
                                          AL.getAttributeSpellingListIndex()));
if (const auto *A = D->getAttr<ReqdWorkGroupSizeAttr>()) {
         D->setInvalidDecl();
       } else if (const auto *A = D->getAttr<AMDGPUNumVGPRAttr>()) {
         Diag(D->getLocation(), diag::err_attribute_wrong_decl_type)
             << A << ExpectedKernelFunction;
         D->setInvalidDecl();
       }
     }
   }
+
+  // Do this check after processing D's attributes because the attribute
+  // objc_method_family can change whether the given method is in the init
+  // family, and it can be applied after objc_designated_initializer. This is a
+  // bit of a hack, but we need it to be compatible with versions of clang that
+  // processed the attribute list in the wrong order.
+  if (D->hasAttr<ObjCDesignatedInitializerAttr>() &&
+      cast<ObjCMethodDecl>(D)->getMethodFamily() != OMF_init) {
+    Diag(D->getLocation(), diag::err_designated_init_attr_non_init);
+    D->dropAttr<ObjCDesignatedInitializerAttr>();
+  }
 }
 
 // Helper for delayed processing TransparentUnion attribute.
 void Sema::ProcessDeclAttributeDelayed(Decl *D,
                                        const ParsedAttributesView &AttrList) {
   for (const ParsedAttr &AL : AttrList)
     if (AL.getKind() == ParsedAttr::AT_TransparentUnion) {
       handleTransparentUnionAttr(*this, D, AL);

test/Misc/pragma-attribute-supported-attributes-list.test

 // CHECK-NEXT: OSReturnsNotRetained (SubjectMatchRule_function, SubjectMatchRule_objc_method, SubjectMatchRule_objc_property, SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: OSReturnsRetained (SubjectMatchRule_function, SubjectMatchRule_objc_method, SubjectMatchRule_objc_property, SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: OSReturnsRetainedOnNonZero (SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: OSReturnsRetainedOnZero (SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: ObjCBoxable (SubjectMatchRule_record)
 // CHECK-NEXT: ObjCBridge (SubjectMatchRule_record, SubjectMatchRule_type_alias)
 // CHECK-NEXT: ObjCBridgeMutable (SubjectMatchRule_record)
 // CHECK-NEXT: ObjCBridgeRelated (SubjectMatchRule_record)
+// CHECK-NEXT: ObjCDesignatedInitializer (SubjectMatchRule_objc_method)
 // CHECK-NEXT: ObjCException (SubjectMatchRule_objc_interface)
 // CHECK-NEXT: ObjCExplicitProtocolImpl (SubjectMatchRule_objc_protocol)
 // CHECK-NEXT: ObjCExternallyRetained (SubjectMatchRule_variable_not_is_parameter, SubjectMatchRule_function, SubjectMatchRule_block, SubjectMatchRule_objc_method)
 // CHECK-NEXT: ObjCMethodFamily (SubjectMatchRule_objc_method)
 // CHECK-NEXT: ObjCNonLazyClass (SubjectMatchRule_objc_interface)
 // CHECK-NEXT: ObjCPreciseLifetime (SubjectMatchRule_variable)
 // CHECK-NEXT: ObjCRequiresPropertyDefs (SubjectMatchRule_objc_interface)
 // CHECK-NEXT: ObjCRequiresSuper (SubjectMatchRule_objc_method)

test/SemaObjC/attr-designated-init.m

 // RUN: %clang_cc1 -fsyntax-only -Wno-incomplete-implementation -verify -fblocks %s
 
 #define NS_DESIGNATED_INITIALIZER __attribute__((objc_designated_initializer))
 #define NS_UNAVAILABLE __attribute__((unavailable))
 
-void fnfoo(void) NS_DESIGNATED_INITIALIZER; // expected-error {{only applies to init methods of interface or class extension declarations}}
+void fnfoo(void) NS_DESIGNATED_INITIALIZER; // expected-error {{'objc_designated_initializer' attribute only applies to Objective-C methods}}
 
 @protocol P1
 -(id)init NS_DESIGNATED_INITIALIZER; // expected-error {{only applies to init methods of interface or class extension declarations}}
 @end
 
 __attribute__((objc_root_class))
 @interface I1
 -(void)meth NS_DESIGNATED_INITIALIZER; // expected-error {{only applies to init methods of interface or class extension declarations}}

 
 @interface ExtensionForMissingInterface() // expected-error{{cannot find interface declaration}}
 - (instancetype)init NS_DESIGNATED_INITIALIZER;
 @end
 
 @interface CategoryForMissingInterface(Cat) // expected-error{{cannot find interface declaration}}
 - (instancetype)init NS_DESIGNATED_INITIALIZER; // expected-error{{only applies to init methods of interface or class extension declarations}}
 @end
+
+@interface TwoAttrs
+-(instancetype)foo
+    __attribute__((objc_designated_initializer))
+    __attribute__((objc_method_family(init)));
+-(instancetype)bar
+    __attribute__((objc_method_family(init)))
+    __attribute__((objc_designated_initializer));
+-(instancetype)baz
+  __attribute__((objc_designated_initializer, objc_method_family(init)));
+-(instancetype)quux
+  __attribute__((objc_method_family(init), objc_designated_initializer));
+@end