[objc_direct] also go through implementations when looking for clashes

[MadCoder]

Summary

Some methods are sometimes declared in the @implementation blocks which can cause undiagnosed clashes.

Just write a checkObjCDirectMethodClashes() for this purpose.

Also make sure that "unavailable" selectors do not inherit objc_direct_members.

Signed-off-by: Pierre Habouzit <[e-mail]>
Radar-ID: rdar://problem/59332804, rdar://problem/59782963

[erik.pilkington]

LGTM

clang/lib/Sema/SemaDeclObjC.cpp

4621

"in the"

clang/lib/Sema/SemaDeclObjC.cpp

if (SemaRef.getASTContext().getTargetInfo().getPlatformMinVersion() >=
       AcceptedInVersion)
     return;
   SemaRef.Diag(Loc, diag::err_objc_method_unsupported_param_ret_type)
       << T << (Method->getReturnType()->isVectorType() ? /*return value*/ 1
                                                        : /*parameter*/ 0)
       << (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,
     ArrayRef<SourceLocation> SelectorLocs, Selector Sel,
     // optional arguments. The number of types/arguments is obtained
     // from the Sel.getNumArgs().
     ObjCArgInfo *ArgInfo, DeclaratorChunk::ParamInfo *CParamInfo,
     unsigned CNumArgs, // c-style args
if (ObjCInterfaceDecl *IDecl = ImpDecl->getClassInterface()) {
         }
 
         // Warn about defining -dealloc in a category.
         if (isa<ObjCCategoryImplDecl>(ImpDecl) && IMD->isOverriding() &&
             ObjCMethod->getSelector().getMethodFamily() == OMF_dealloc) {
           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
       // extension conforms is non-escaping and the implementation's method is
       // escaping.
       for (auto *C : IDecl->visible_categories())
         for (auto &P : C->protocols())
           if (auto *IMD = P->lookupMethod(ObjCMethod->getSelector(),
                                           ObjCMethod->isInstanceMethod())) {
             assert(ObjCMethod->parameters().size() ==
                        IMD->parameters().size() &&
                    "Methods have different number of parameters");
             auto OI = IMD->param_begin(), OE = IMD->param_end();
             auto NI = ObjCMethod->param_begin();
             for (; OI != OE; ++OI, ++NI)
               diagnoseNoescape(*NI, *OI, C, P, *this);
           }
     }
   } 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);
   }
 
   if (PrevMethod) {
     // You can never have two method definitions with the same name.
     Diag(ObjCMethod->getLocation(), diag::err_duplicate_method_decl)

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

 @interface B (Cat)
 - (void)B_Cat __attribute__((objc_direct));
 @end
 
 @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}}
 }
 - (void)B_extension { // expected-error {{direct method was declared in an extension but is implemented in a different category}}
 }
 - (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))
 @interface C
 - (void)C1 __attribute__((objc_direct)); // expected-note {{previous declaration is here}}
 - (void)C2;                              // expected-note {{previous declaration is here}}
 @end
 
 @interface C (Cat)
 - (void)C1;                              // expected-error {{method declaration conflicts with previous direct declaration of method 'C1'}}
 - (void)C2 __attribute__((objc_direct)); // expected-error {{direct method declaration conflicts with previous declaration of method 'C2'}}
 @end

clang/test/SemaObjC/method-direct.m

 // RUN: %clang_cc1 -fsyntax-only -verify -Wselector-type-mismatch %s
 
 @protocol Proto
 - (void)protoMethod;      // expected-note {{previous declaration is here}}
 + (void)classProtoMethod; // expected-note {{previous declaration is here}}
 @end
 
 @protocol ProtoDirectFail
 - (void)protoMethod __attribute__((objc_direct));      // expected-error {{'objc_direct' attribute cannot be applied to methods declared in an Objective-C protocol}}
 + (void)classProtoMethod __attribute__((objc_direct)); // expected-error {{'objc_direct' attribute cannot be applied to methods declared in an Objective-C protocol}}
 @end
 
 __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}};
 + (void)classRootDirect __attribute__((objc_direct)); // expected-note {{previous declaration is here}};
 - (void)otherRootDirect __attribute__((objc_direct)); // expected-note {{direct method 'otherRootDirect' declared here}}
 + (void)otherClassRootDirect __attribute__((objc_direct)); // expected-note {{direct method 'otherClassRootDirect' declared here}}
 + (void)otherOtherClassRootDirect __attribute__((objc_direct)); // expected-note {{direct method 'otherOtherClassRootDirect' declared here}}
 - (void)notDirectInIface;                             // expected-note {{previous declaration is here}}

 + (void)classRootCategoryRegular;                              // expected-note {{previous declaration is here}}
 - (void)rootCategoryDirect2 __attribute__((objc_direct));      // expected-note {{previous declaration is here}}
 + (void)classRootCategoryDirect2 __attribute__((objc_direct)); // expected-note {{previous declaration is here}}
 @end
 
 __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
 
 @interface Sub : Root <Proto>
 /* invalid overrides with directs */
 - (void)rootRegular __attribute__((objc_direct));               // expected-error {{methods that override superclass methods cannot be direct}}
 + (void)classRootRegular __attribute__((objc_direct));          // expected-error {{methods that override superclass methods cannot be direct}}
 - (void)protoMethod __attribute__((objc_direct));               // expected-error {{methods that implement protocol requirements cannot be direct}}

 - (void)rootCategoryDirect;        // expected-error {{cannot override a method that is declared direct by a superclass}}
 + (void)classRootCategoryDirect;   // expected-error {{cannot override a method that is declared direct by a superclass}}
 - (void)rootCategoryDirect2;       // expected-error {{cannot override a method that is declared direct by a superclass}}
 + (void)classRootCategoryDirect2;  // expected-error {{cannot override a method that is declared direct by a superclass}}
 @end
 
 __attribute__((objc_direct_members))
 @implementation Root
+- (void)unavailableInChild {
+}
 - (void)rootRegular {
 }
 + (void)classRootRegular {
 }
 - (void)rootDirect {
 }
 + (void)classRootDirect {
 }