[objc_direct] Tigthen checks for direct methods
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>
Differential Revision: https://reviews.llvm.org/D71694
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 315e836..b714cb8 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -995,6 +995,12 @@
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<
diff --git a/clang/lib/AST/DeclObjC.cpp b/clang/lib/AST/DeclObjC.cpp
index ca70afd..9a84e3c 100644
--- a/clang/lib/AST/DeclObjC.cpp
+++ b/clang/lib/AST/DeclObjC.cpp
@@ -956,32 +956,32 @@
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 ObjCImplementationDecl (the primary
+ // @implementation), then the canonical Decl is either in
+ // the class Interface, or in any of its extension.
+ //
+ // So when we don't find it in the ObjCInterfaceDecl,
+ // 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;
+ for (auto *Ext : IFD->known_extensions())
+ if (ObjCMethodDecl *MD = Ext->getMethod(Sel, isInstanceMethod()))
+ return MD;
}
} 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;
}
diff --git a/clang/lib/CodeGen/CGObjCMac.cpp b/clang/lib/CodeGen/CGObjCMac.cpp
index a38e5a4..e18105f 100644
--- a/clang/lib/CodeGen/CGObjCMac.cpp
+++ b/clang/lib/CodeGen/CGObjCMac.cpp
@@ -928,7 +928,8 @@
/// \param[out] NameOut - The return value.
void GetNameForMethod(const ObjCMethodDecl *OMD,
const ObjCContainerDecl *CD,
- SmallVectorImpl<char> &NameOut);
+ SmallVectorImpl<char> &NameOut,
+ bool ignoreCategoryNamespace = false);
/// GetMethodVarName - Return a unique constant for the given
/// selector's name. The return value has type char *.
@@ -4033,7 +4034,7 @@
return I->second;
SmallString<256> Name;
- GetNameForMethod(OMD, CD, Name);
+ GetNameForMethod(OMD, CD, Name, /*ignoreCategoryNamespace*/true);
CodeGenTypes &Types = CGM.getTypes();
llvm::FunctionType *MethodTy =
@@ -4088,9 +4089,9 @@
nullptr, true);
Builder.CreateStore(result.getScalarVal(), selfAddr);
- // Nullable `Class` expressions cannot be messaged with a direct method
- // so the only reason why the receive can be null would be because
- // of weak linking.
+ // Nullable `Class` expressions cannot be messaged with a direct method
+ // so the only reason why the receive can be null would be because
+ // of weak linking.
ReceiverCanBeNull = isWeakLinkedClass(OID);
}
@@ -5687,14 +5688,16 @@
void CGObjCCommonMac::GetNameForMethod(const ObjCMethodDecl *D,
const ObjCContainerDecl *CD,
- SmallVectorImpl<char> &Name) {
+ SmallVectorImpl<char> &Name,
+ bool ignoreCategoryNamespace) {
llvm::raw_svector_ostream OS(Name);
assert (CD && "Missing container decl in GetNameForMethod");
OS << '\01' << (D->isInstanceMethod() ? '-' : '+')
<< '[' << CD->getName();
- if (const ObjCCategoryImplDecl *CID =
- dyn_cast<ObjCCategoryImplDecl>(D->getDeclContext()))
- OS << '(' << *CID << ')';
+ if (!ignoreCategoryNamespace)
+ if (const ObjCCategoryImplDecl *CID =
+ dyn_cast<ObjCCategoryImplDecl>(D->getDeclContext()))
+ OS << '(' << *CID << ')';
OS << ' ' << D->getSelector().getAsString() << ']';
}
diff --git a/clang/lib/Sema/SemaDeclObjC.cpp b/clang/lib/Sema/SemaDeclObjC.cpp
index 70c0457..4fdddfb 100644
--- a/clang/lib/Sema/SemaDeclObjC.cpp
+++ b/clang/lib/Sema/SemaDeclObjC.cpp
@@ -4508,12 +4508,6 @@
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(
@@ -4738,16 +4732,73 @@
}
}
+ // A method is either tagged direct explicitly, or inherits it from its
+ // canonical declaration.
+ //
+ // We have to do the merge upfront and not in mergeInterfaceMethodToImpl()
+ // because IDecl->lookupMethod() returns more possible matches than just
+ // the canonical declaration.
+ 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);
- if (const auto *attr = ObjCMethod->getAttr<ObjCDirectAttr>()) {
- if (!IMD->isDirectMethod()) {
- Diag(attr->getLocation(), diag::err_objc_direct_missing_on_decl);
+
+ // The Idecl->lookupMethod() above will find declarations for ObjCMethod
+ // in one of these places:
+ //
+ // (1) the canonical declaration in an @interface container paired
+ // with the ImplDecl,
+ // (2) non canonical declarations in @interface not paired with the
+ // ImplDecl for the same Class,
+ // (3) any superclass container.
+ //
+ // Direct methods only allow for canonical declarations in the matching
+ // container (case 1).
+ //
+ // Direct methods overriding a superclass declaration (case 3) is
+ // handled during overrides checks in CheckObjCMethodOverrides().
+ //
+ // We deal with same-class container mismatches (Case 2) here.
+ if (IDecl == IMD->getClassInterface()) {
+ auto diagContainerMismatch = [&] {
+ 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);
+ };
+
+ if (const auto *attr = ObjCMethod->getAttr<ObjCDirectAttr>()) {
+ if (ObjCMethod->getCanonicalDecl() != IMD) {
+ diagContainerMismatch();
+ } 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) {
+ diagContainerMismatch();
+ } else {
+ ObjCMethod->addAttr(
+ ObjCDirectAttr::CreateImplicit(Context, attr->getLocation()));
+ }
}
}
@@ -4779,11 +4830,42 @@
}
}
} else {
- if (!ObjCMethod->isDirectMethod() &&
- ClassDecl->hasAttr<ObjCDirectMembersAttr>()) {
- ObjCMethod->addAttr(
- ObjCDirectAttr::CreateImplicit(Context, ObjCMethod->getLocation()));
+ 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(),
+ /*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() << IMD->isDirectMethod()
+ << ObjCMethod->getDeclName();
+ Diag(IMD->getLocation(), diag::note_previous_declaration);
+ }
+ }
}
+
cast<DeclContext>(ClassDecl)->addDecl(ObjCMethod);
}
diff --git a/clang/test/CodeGenObjC/direct-method.m b/clang/test/CodeGenObjC/direct-method.m
index c42910c..e53c99b 100644
--- a/clang/test/CodeGenObjC/direct-method.m
+++ b/clang/test/CodeGenObjC/direct-method.m
@@ -172,10 +172,19 @@
@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]"(
@@ -183,6 +192,17 @@
// CHECK-LABEL: define internal void @"\01-[Foo .cxx_destruct]"(
@end
+@implementation Foo (Cat)
+// CHECK-LABEL: define hidden i32 @"\01-[Foo directMethodInCategory]"(
+- (int)directMethodInCategory {
+ return 42;
+}
+// CHECK-LABEL: define hidden i32 @"\01-[Foo 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]"
@@ -195,8 +215,14 @@
// 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 directMethodInCategory]"
+ // CHECK: %{{[^ ]*}} = call i32 bitcast {{.*}} @"\01-[Foo directMethodInCategoryNoDecl]"
[f setGetDynamic_setDirect:1];
- return [f getDirect_setDynamic];
+ return [f getDirect_setDynamic] +
+ [f directMethodInExtension] +
+ [f directMethodInCategory] +
+ [f directMethodInCategoryNoDecl];
}
__attribute__((objc_root_class))
diff --git a/clang/test/SemaObjC/method-direct-one-definition.m b/clang/test/SemaObjC/method-direct-one-definition.m
new file mode 100644
index 0000000..e6355d2
--- /dev/null
+++ b/clang/test/SemaObjC/method-direct-one-definition.m
@@ -0,0 +1,53 @@
+// 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