Merge pull request #960 from apple/eng/PR-59332804-59782963

[objc_direct] also go through implementations when looking for clashes

Author: MadCoder

clang/lib/Sema/SemaDeclObjC.cpp

@@ -4636,6 +4636,62 @@ static void checkObjCMethodX86VectorTypes(Sema &SemaRef,
       << (Triple.isMacOSX() ? "macOS 10.11" : "iOS 9");
 }
 
+static void mergeObjCDirectMembers(Sema &S, Decl *CD, ObjCMethodDecl *Method) {
+  if (!Method->isDirectMethod() && !Method->hasAttr<UnavailableAttr>() &&
+      CD->hasAttr<ObjCDirectMembersAttr>()) {
+    Method->addAttr(
+        ObjCDirectAttr::CreateImplicit(S.Context, Method->getLocation()));
+  }
+}
+
+static void checkObjCDirectMethodClashes(Sema &S, ObjCInterfaceDecl *IDecl,
+                                         ObjCMethodDecl *Method,
+                                         ObjCImplDecl *ImpDecl = nullptr) {
+  auto Sel = Method->getSelector();
+  bool isInstance = Method->isInstanceMethod();
+  bool diagnosed = false;
+
+  auto diagClash = [&](const ObjCMethodDecl *IMD) {
+    if (diagnosed || IMD->isImplicit())
+      return;
+    if (Method->isDirectMethod() || IMD->isDirectMethod()) {
+      S.Diag(Method->getLocation(), diag::err_objc_direct_duplicate_decl)
+          << Method->isDirectMethod() << /* method */ 0 << IMD->isDirectMethod()
+          << Method->getDeclName();
+      S.Diag(IMD->getLocation(), diag::note_previous_declaration);
+      diagnosed = true;
+    }
+  };
+
+  // Look for any other declaration of this method anywhere we can see in this
+  // compilation unit.
+  //
+  // We do not use IDecl->lookupMethod() because we have specific needs:
+  //
+  // - we absolutely do not need to walk protocols, because
+  //   diag::err_objc_direct_on_protocol has already been emitted
+  //   during parsing if there's a conflict,
+  //
+  // - when we do not find a match in a given @interface container,
+  //   we need to attempt looking it up in the @implementation block if the
+  //   translation unit sees it to find more clashes.
+
+  if (auto *IMD = IDecl->getMethod(Sel, isInstance))
+    diagClash(IMD);
+  else if (auto *Impl = IDecl->getImplementation())
+    if (Impl != ImpDecl)
+      if (auto *IMD = IDecl->getImplementation()->getMethod(Sel, isInstance))
+        diagClash(IMD);
+
+  for (const auto *Cat : IDecl->visible_categories())
+    if (auto *IMD = Cat->getMethod(Sel, isInstance))
+      diagClash(IMD);
+    else if (auto CatImpl = Cat->getImplementation())
+      if (CatImpl != ImpDecl)
+        if (auto *IMD = Cat->getMethod(Sel, isInstance))
+          diagClash(IMD);
+}
+
 Decl *Sema::ActOnMethodDeclaration(
     Scope *S, SourceLocation MethodLoc, SourceLocation EndLoc,
     tok::TokenKind MethodType, ObjCDeclSpec &ReturnQT, ParsedType ReturnType,
@@ -4866,9 +4922,9 @@ Decl *Sema::ActOnMethodDeclaration(
           Diag(ObjCMethod->getLocation(), diag::warn_dealloc_in_category)
             << ObjCMethod->getDeclName();
         }
-      } else if (ImpDecl->hasAttr<ObjCDirectMembersAttr>()) {
-        ObjCMethod->addAttr(
-            ObjCDirectAttr::CreateImplicit(Context, ObjCMethod->getLocation()));
+      } else {
+        mergeObjCDirectMembers(*this, ClassDecl, ObjCMethod);
+        checkObjCDirectMethodClashes(*this, IDecl, ObjCMethod, ImpDecl);
       }
 
       // Warn if a method declared in a protocol to which a category or
@@ -4889,39 +4945,16 @@ Decl *Sema::ActOnMethodDeclaration(
     }
   } else {
     if (!isa<ObjCProtocolDecl>(ClassDecl)) {
-      if (!ObjCMethod->isDirectMethod() &&
-          ClassDecl->hasAttr<ObjCDirectMembersAttr>()) {
-        ObjCMethod->addAttr(
-            ObjCDirectAttr::CreateImplicit(Context, ObjCMethod->getLocation()));
-      }
+      mergeObjCDirectMembers(*this, ClassDecl, ObjCMethod);
 
-      // There can be a single declaration in any @interface container
-      // for a given direct method, look for clashes as we add them.
-      //
-      // For valid code, we should always know the primary interface
-      // declaration by now, however for invalid code we'll keep parsing
-      // but we won't find the primary interface and IDecl will be nil.
       ObjCInterfaceDecl *IDecl = dyn_cast<ObjCInterfaceDecl>(ClassDecl);
       if (!IDecl)
         IDecl = cast<ObjCCategoryDecl>(ClassDecl)->getClassInterface();
-
+      // For valid code, we should always know the primary interface
+      // declaration by now, however for invalid code we'll keep parsing
+      // but we won't find the primary interface and IDecl will be nil.
       if (IDecl)
-        if (auto *IMD = IDecl->lookupMethod(ObjCMethod->getSelector(),
-                                            ObjCMethod->isInstanceMethod(),
-                                            /*shallowCategoryLookup=*/false,
-                                            /*followSuper=*/false)) {
-          if (isa<ObjCProtocolDecl>(IMD->getDeclContext())) {
-            // Do not emit a diagnostic for the Protocol case:
-            // diag::err_objc_direct_on_protocol has already been emitted
-            // during parsing for these with a nicer diagnostic.
-          } else if (ObjCMethod->isDirectMethod() || IMD->isDirectMethod()) {
-            Diag(ObjCMethod->getLocation(),
-                 diag::err_objc_direct_duplicate_decl)
-                << ObjCMethod->isDirectMethod() << /* method */ 0
-                << IMD->isDirectMethod() << ObjCMethod->getDeclName();
-            Diag(IMD->getLocation(), diag::note_previous_declaration);
-          }
-        }
+        checkObjCDirectMethodClashes(*this, IDecl, ObjCMethod);
     }
 
     cast<DeclContext>(ClassDecl)->addDecl(ObjCMethod);

clang/test/SemaObjC/method-direct-one-definition.m

@@ -30,6 +30,15 @@ @interface B (OtherCat)
 - (void)B_OtherCat __attribute__((objc_direct)); // expected-note {{previous declaration is here}}
 @end
 
+@implementation B
+- (void)B_primary {
+}
+- (void)B_extension {
+}
+- (void)B_implOnly __attribute__((objc_direct)) { // expected-note {{previous declaration is here}}
+}
+@end
+
 @implementation B (Cat)
 - (void)B_primary { // expected-error {{direct method was declared in the primary interface but is implemented in a category}}
 }
@@ -39,6 +48,8 @@ - (void)B_Cat {
 }
 - (void)B_OtherCat { // expected-error {{direct method was declared in a category but is implemented in a different category}}
 }
+- (void)B_implOnly __attribute__((objc_direct)) { // expected-error {{direct method declaration conflicts with previous direct declaration of method 'B_implOnly'}}
+}
 @end
 
 __attribute__((objc_root_class))

clang/test/SemaObjC/method-direct.m

@@ -12,6 +12,7 @@ + (void)classProtoMethod __attribute__((objc_direct)); // expected-error {{'objc
 
 __attribute__((objc_root_class))
 @interface Root
+- (void)unavailableInChild;
 - (void)rootRegular;                                  // expected-note {{previous declaration is here}}
 + (void)classRootRegular;                             // expected-note {{previous declaration is here}}
 - (void)rootDirect __attribute__((objc_direct));      // expected-note {{previous declaration is here}};
@@ -52,6 +53,7 @@ + (void)classRootCategoryDirect2 __attribute__((objc_direct)); // expected-note
 __attribute__((objc_direct_members))
 @interface SubDirectMembers : Root
 @property int foo; // expected-note {{previous declaration is here}}
+- (void)unavailableInChild __attribute__((unavailable)); // should not warn
 - (instancetype)init;
 @end
 
@@ -81,6 +83,8 @@ + (void)classRootCategoryDirect2;  // expected-error {{cannot override a method
 
 __attribute__((objc_direct_members))
 @implementation Root
+- (void)unavailableInChild {
+}
 - (void)rootRegular {
 }
 + (void)classRootRegular {