[objc_direct] Tigthen checks for direct methods

[MadCoder]

Summary

Because the name of a direct method must be agreed upon by the caller and the implementation, certain bad practices that one can get away with when using dynamism are fatal with direct methods.

To avoid really weird and unscruttable linker error, tighten the front-end error reporting.

Rule 1:

Direct methods can only have at most one declaration in an @interface container. Any redeclaration is strictly forbidden.
Today some amount of redeclaration is tolerated between the main interface and categories for dynamic methods, but we can't have that.

Rule 2:

Direct method implementations can only be declared in a matching @interface container: when implemented in the primary @implementation then the declaration must be in the primary @interface or an extension, and when implemented in a category, the declaration must be in the @interface for the same category.

Also fix another issue with ObjCMethod::getCanonicalDecl(): when an implementation lives in the primary @interface, then its canonical declaration can be in any extension, even when it's not an accessor.

Add Sema tests to cover the new errors, and CG tests to beef up testing around function names for categories and extensions.

Radar-Id: rdar://problem/58054563

[MadCoder]

Small update because I uploaded before the full completion of the test-suite and somehow it's sentient and punished me... (some diagnostics were too verbose and stuttering things with Sema::CheckObjCMethodOverrides).

[MadCoder]

clang/test/CodeGenObjC/direct-method.m

196

this may be questionable as a chosen mangling. it's exactly what dynamic dispatch does, but because a direct method cannot have clashes on the same class, this extra namespacing is not useful and is likely making the life of debuggers harder for expression evaluation.

@aprantl should I generate @"\01-[Foo directMethodInCategory]" instead? if yes I'd rather do it in this review

[aprantl]

Added superficial LLVM coding style comments.

clang/lib/Sema/SemaDeclObjC.cpp

4735

missing . at end of sentence.

"Upfront" as opposed to? It would be helpful to add a ", because ..." here

4760

// If

Is there a better way to express Class (not super)?

4783

how about factoring this into a diagContainerMismatch lambda?

4819

.

4835

full sentence please

[aprantl]

clang/lib/AST/DeclObjC.cpp

963–968

What does "principal" mean here?

966

Does principal mean the ObjCInterfaceDecl of ImplD? Should we say that instead?

[MadCoder]

clang/lib/AST/DeclObjC.cpp

963–968

primary sorry, will fix

[MadCoder]

Addressed @aprantl review feedback.

[MadCoder]

I had forgotten to clang format and left some tabs here... some day I'll fix my vim config

[aprantl]

clang/test/CodeGenObjC/direct-method.m

196

@teemperor implemented the LLDB support. I would assume that changing it to Foo without the category will be necessary for LLDB to find it. @teemperor Can you add an LLDB test for this scenario?

[MadCoder]

clang/test/CodeGenObjC/direct-method.m

196

that was my expectation as well, let me update the code to do so then :)

[MadCoder]

Removed the category from the direct method name mangling as discussed with @aprantl

[teemperor]

clang/test/CodeGenObjC/direct-method.m

196

Added a test for that in 29bd2194979c50097edb39a4beb714bff8c153a1

clang/include/clang/Basic/DiagnosticSemaKinds.td

def warn_undef_method_impl : Warning<"method definition for %0 not found">,
   InGroup<DiagGroup<"incomplete-implementation">>;
 def warn_objc_boxing_invalid_utf8_string : Warning<
   "string is ill-formed as UTF-8 and will become a null %0 when boxed">,
   InGroup<ObjCBoxing>;
 
 def err_objc_direct_on_protocol : Error<
   "'objc_direct' attribute cannot be applied to %select{methods|properties}0 "
   "declared in an Objective-C protocol">;
+def err_objc_direct_duplicate_decl : Error<
+  "%select{|direct }0method declaration conflicts "
+  "with previous %select{|direct }1declaration of method %2">;
+def err_objc_direct_impl_decl_mismatch : Error<
+  "direct method was declared in %select{the primary interface|an extension|a category}0 "
+  "but is implemented in %select{the primary interface|a category|a different category}1">;
 def err_objc_direct_missing_on_decl : Error<
   "direct method implementation was previously declared not direct">;
 def err_objc_direct_on_override : Error<
   "methods that %select{override superclass methods|implement protocol requirements}0 cannot be direct">;
 def err_objc_override_direct_method : Error<
   "cannot override a method that is declared direct by a superclass">;
 def warn_objc_direct_ignored : Warning<
   "%0 attribute isn't implemented by this Objective-C runtime">,

clang/lib/AST/DeclObjC.cpp

return cast<ObjCContainerDecl>(CtxD)->getMethod(getSelector(),
                                                     isInstanceMethod());
   }
 
   return Redecl ? Redecl : this;
 }
 
 ObjCMethodDecl *ObjCMethodDecl::getCanonicalDecl() {
   auto *CtxD = cast<Decl>(getDeclContext());
+  const auto &Sel = getSelector();
 
   if (auto *ImplD = dyn_cast<ObjCImplementationDecl>(CtxD)) {
     if (ObjCInterfaceDecl *IFD = ImplD->getClassInterface()) {
-      if (ObjCMethodDecl *MD = IFD->getMethod(getSelector(),
-                                              isInstanceMethod()))
+      // When the container is the principal @implementation,
+      // the canonical Decl is either in an @interface, or in an extension.
+      //
+      // So when we don't find it in the principal @interface,
+      // sift through extensions too.
+      if (ObjCMethodDecl *MD = IFD->getMethod(Sel, isInstanceMethod()))
         return MD;
-      // readwrite properties may have been re-declared in an extension.
-      // look harder.
-      if (isPropertyAccessor())
-        for (auto *Ext : IFD->known_extensions())
-          if (ObjCMethodDecl *MD =
-                  Ext->getMethod(getSelector(), isInstanceMethod()))
-            return MD;
+        if (ObjCMethodDecl *MD = Ext->getMethod(Sel, isInstanceMethod()))
     }
   } else if (auto *CImplD = dyn_cast<ObjCCategoryImplDecl>(CtxD)) {
     if (ObjCCategoryDecl *CatD = CImplD->getCategoryDecl())
-      if (ObjCMethodDecl *MD = CatD->getMethod(getSelector(),
-                                               isInstanceMethod()))
+      if (ObjCMethodDecl *MD = CatD->getMethod(Sel, isInstanceMethod()))
         return MD;
   }
 
   if (isRedeclaration()) {
     // It is possible that we have not done deserializing the ObjCMethod yet.
     ObjCMethodDecl *MD =
-        cast<ObjCContainerDecl>(CtxD)->getMethod(getSelector(),
-                                                 isInstanceMethod());
+        cast<ObjCContainerDecl>(CtxD)->getMethod(Sel, isInstanceMethod());
     return MD ? MD : this;
   }
 
   return this;
 }
 
 SourceLocation ObjCMethodDecl::getEndLoc() const {
   if (Stmt *Body = getBody())

clang/lib/Sema/SemaDeclObjC.cpp

static void mergeInterfaceMethodToImpl(Sema &S,
   if (prevMethod->hasAttr<ObjCRequiresSuperAttr>() &&
       !method->hasAttr<ObjCRequiresSuperAttr>()) {
     // merge the attribute into implementation.
     method->addAttr(
       ObjCRequiresSuperAttr::CreateImplicit(S.Context,
                                             method->getLocation()));
   }
 
-  if (!method->isDirectMethod())
-    if (const auto *attr = prevMethod->getAttr<ObjCDirectAttr>()) {
-      method->addAttr(
-          ObjCDirectAttr::CreateImplicit(S.Context, attr->getLocation()));
-    }
-
   // Merge nullability of the result type.
   QualType newReturnType
     = mergeTypeNullabilityForRedecl(
         S, method->getReturnTypeSourceRange().getBegin(),
         method->getReturnType(),
         method->getObjCDeclQualifier() & Decl::OBJC_TQ_CSNullability,
         prevMethod->getReturnTypeSourceRange().getBegin(),
         prevMethod->getReturnType(),
for (ObjCPropertyImplDecl *PropertyImpl : ImpDecl->property_impls()) {
         if (Getter->getSelector() == Sel &&
             Getter->isInstanceMethod() == ObjCMethod->isInstanceMethod()) {
           assert(Getter->isSynthesizedAccessorStub() && "autosynth stub expected");
           PropertyImpl->setGetterMethodDecl(ObjCMethod);
           break;
         }
     }
 
+    // Merge directness from the canonical declaration upfront
+    if (!ObjCMethod->isDirectMethod()) {
+      const ObjCMethodDecl *CanonicalMD = ObjCMethod->getCanonicalDecl();
+      if (const auto *attr = CanonicalMD->getAttr<ObjCDirectAttr>()) {
+        ObjCMethod->addAttr(
+            ObjCDirectAttr::CreateImplicit(Context, attr->getLocation()));
+      }
+    }
+
     // Merge information from the @interface declaration into the
     // @implementation.
     if (ObjCInterfaceDecl *IDecl = ImpDecl->getClassInterface()) {
       if (auto *IMD = IDecl->lookupMethod(ObjCMethod->getSelector(),
                                           ObjCMethod->isInstanceMethod())) {
         mergeInterfaceMethodToImpl(*this, ObjCMethod, IMD);
+
+        bool directContainerMismatch = false;
         if (const auto *attr = ObjCMethod->getAttr<ObjCDirectAttr>()) {
-          if (!IMD->isDirectMethod()) {
+          if (ObjCMethod->getCanonicalDecl() != IMD) {
+            directContainerMismatch = true;
+          } else if (!IMD->isDirectMethod()) {
             Diag(attr->getLocation(), diag::err_objc_direct_missing_on_decl);
             Diag(IMD->getLocation(), diag::note_previous_declaration);
           }
+        } else if (const auto *attr = IMD->getAttr<ObjCDirectAttr>()) {
+          if (ObjCMethod->getCanonicalDecl() != IMD) {
+            directContainerMismatch = true;
+          } else {
+            ObjCMethod->addAttr(
+                ObjCDirectAttr::CreateImplicit(Context, attr->getLocation()));
+          }
+        }
+
+        if (directContainerMismatch) {
+          int decl = 0, impl = 0;
+
+          if (auto *Cat = dyn_cast<ObjCCategoryDecl>(IMD->getDeclContext()))
+            decl = Cat->IsClassExtension() ? 1 : 2;
+
+          if (auto *Cat = dyn_cast<ObjCCategoryImplDecl>(ImpDecl))
+            impl = 1 + (decl != 0);
+
+          Diag(ObjCMethod->getLocation(),
+               diag::err_objc_direct_impl_decl_mismatch)
+              << decl << impl;
+          Diag(IMD->getLocation(), diag::note_previous_declaration);
         }
 
         // 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()));
if (ObjCInterfaceDecl *IDecl = ImpDecl->getClassInterface()) {
                    "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()));
       }
+
+      // 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();
+
+      if (IDecl)
+        if (auto *IMD = IDecl->lookupMethod(ObjCMethod->getSelector(),
+                                            ObjCMethod->isInstanceMethod())) {
+          if (ObjCMethod->isDirectMethod() || IMD->isDirectMethod()) {
+            Diag(ObjCMethod->getLocation(), diag::err_objc_direct_duplicate_decl)
+                << ObjCMethod->isDirectMethod() << IMD->isDirectMethod()
+                << ObjCMethod->getDeclName();
+            Diag(IMD->getLocation(), diag::note_previous_declaration);
+          }
+        }
+    }
+
     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)
       << ObjCMethod->getDeclName();
     Diag(PrevMethod->getLocation(), diag::note_previous_declaration);

clang/test/CodeGenObjC/direct-method.m

 }
 @property(nonatomic, readonly, direct) int getDirect_setDynamic;
 @property(nonatomic, readonly) int getDynamic_setDirect;
 @end
 
 @interface Foo ()
 @property(nonatomic, readwrite) int getDirect_setDynamic;
 @property(nonatomic, readwrite, direct) int getDynamic_setDirect;
+- (int)directMethodInExtension __attribute__((objc_direct));
+@end
+
+@interface Foo (Cat)
+- (int)directMethodInCategory __attribute__((objc_direct));
 @end
 
 __attribute__((objc_direct_members))
 @implementation Foo
+// CHECK-LABEL: define hidden i32 @"\01-[Foo directMethodInExtension]"(
+- (int)directMethodInExtension {
+  return 42;
+}
 // CHECK-LABEL: define hidden i32 @"\01-[Foo getDirect_setDynamic]"(
 // CHECK-LABEL: define internal void @"\01-[Foo setGetDirect_setDynamic:]"(
 // CHECK-LABEL: define internal i32 @"\01-[Foo getDynamic_setDirect]"(
 // CHECK-LABEL: define hidden void @"\01-[Foo setGetDynamic_setDirect:]"(
 // CHECK-LABEL: define internal void @"\01-[Foo .cxx_destruct]"(
 @end
 
+@implementation Foo (Cat)
+// CHECK-LABEL: define hidden i32 @"\01-[Foo(Cat) directMethodInCategory]"(
+- (int)directMethodInCategory {
+  return 42;
+}
+// CHECK-LABEL: define hidden i32 @"\01-[Foo(Cat) directMethodInCategoryNoDecl]"(
+- (int)directMethodInCategoryNoDecl __attribute__((objc_direct)) {
+  return 42;
+}
+@end
+
 int useRoot(Root *r) {
   // CHECK-LABEL: define i32 @useRoot
   // CHECK: %{{[^ ]*}} = call i32 bitcast {{.*}} @"\01-[Root getInt]"
   // CHECK: %{{[^ ]*}} = call i32 bitcast {{.*}} @"\01-[Root intProperty]"
   // CHECK: %{{[^ ]*}} = call i32 bitcast {{.*}} @"\01-[Root intProperty2]"
   return [r getInt] + [r intProperty] + [r intProperty2];
 }
 
 int useFoo(Foo *f) {
   // CHECK-LABEL: define i32 @useFoo
   // CHECK: call void bitcast {{.*}} @"\01-[Foo setGetDynamic_setDirect:]"
   // CHECK: %{{[^ ]*}} = call i32 bitcast {{.*}} @"\01-[Foo getDirect_setDynamic]"
+  // CHECK: %{{[^ ]*}} = call i32 bitcast {{.*}} @"\01-[Foo directMethodInExtension]"
+  // CHECK: %{{[^ ]*}} = call i32 bitcast {{.*}} @"\01-[Foo(Cat) directMethodInCategory]"
+  // CHECK: %{{[^ ]*}} = call i32 bitcast {{.*}} @"\01-[Foo(Cat) directMethodInCategoryNoDecl]"
   [f setGetDynamic_setDirect:1];
-  return [f getDirect_setDynamic];
+  return [f getDirect_setDynamic] +
+         [f directMethodInExtension] +
+         [f directMethodInCategory] +
+         [f directMethodInCategoryNoDecl];
 }
 
 __attribute__((objc_root_class))
 @interface RootDeclOnly
 @property(direct, readonly) int intProperty;
 @end
 
 int useRootDeclOnly(RootDeclOnly *r) {
   // CHECK-LABEL: define i32 @useRootDeclOnly
   // CHECK: %{{[^ ]*}} = call i32 bitcast {{.*}} @"\01-[RootDeclOnly intProperty]"
   return [r intProperty];
 }

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

+// RUN: %clang_cc1 -fsyntax-only -verify -Wno-objc-protocol-method-implementation %s
+
+__attribute__((objc_root_class))
+@interface A
+@end
+
+@interface A (Cat)
+- (void)A_Cat __attribute__((objc_direct)); // expected-note {{previous declaration is here}}
+@end
+
+@implementation A
+- (void)A_Cat {
+} // expected-error {{direct method was declared in a category but is implemented in the primary interface}}
+@end
+
+__attribute__((objc_root_class))
+@interface B
+- (void)B_primary __attribute__((objc_direct)); // expected-note {{previous declaration is here}}
+@end
+
+@interface B ()
+- (void)B_extension __attribute__((objc_direct)); // expected-note {{previous declaration is here}}
+@end
+
+@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 (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}}
+}
+@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