Skip to content

Commit

Permalink
Require @objc to be used with @implementation
Browse files Browse the repository at this point in the history
…for extensions. This change also removes @implementation(CategoryName); you should attach the category name to the @objc attribute instead.
  • Loading branch information
beccadax committed Apr 27, 2024
1 parent 1013c64 commit 80b9929
Show file tree
Hide file tree
Showing 13 changed files with 210 additions and 82 deletions.
3 changes: 3 additions & 0 deletions include/swift/AST/Attr.h
Original file line number Diff line number Diff line change
Expand Up @@ -2422,6 +2422,9 @@ class DocumentationAttr: public DeclAttribute {

class ObjCImplementationAttr final : public DeclAttribute {
public:
/// Name of the category being implemented. This should only be used with
/// the early adopter \@\_objcImplementation syntax, but we support it there
/// for backwards compatibility.
Identifier CategoryName;

ObjCImplementationAttr(Identifier CategoryName, SourceLoc AtLoc,
Expand Down
17 changes: 12 additions & 5 deletions include/swift/AST/DiagnosticsSema.def
Original file line number Diff line number Diff line change
Expand Up @@ -763,6 +763,9 @@ ERROR(expr_selector_not_objc,none,
NOTE(make_decl_objc,none,
"add '@objc' to expose this %0 to Objective-C",
(DescriptiveDeclKind))
NOTE(make_decl_objc_for_implementation,none,
"add '@objc' to implement an Objective-C %0",
(DescriptiveDeclKind))

// Selectors-as-string-literals.
WARNING(selector_literal_invalid,none,
Expand Down Expand Up @@ -1760,10 +1763,6 @@ WARNING(objc_implementation_early_spelling_deprecated,none,
ERROR(attr_objc_implementation_must_be_unconditional,none,
"only unconditional extensions can implement an Objective-C '@interface'",
())
ERROR(attr_objc_implementation_must_extend_class,none,
"cannot mark extension of %kind0 with '@_objcImplementation'; it is not "
"an imported Objective-C class",
(ValueDecl *))
ERROR(attr_objc_implementation_must_be_imported,none,
"'@_objcImplementation' cannot be used to extend %kind0 because it was "
"defined by a Swift 'class' declaration, not an imported Objective-C "
Expand Down Expand Up @@ -1800,6 +1799,14 @@ ERROR(attr_objc_implementation_raise_minimum_deployment_target,none,
"'@implementation' of an Objective-C class requires a minimum deployment "
"target of at least %0 %1",
(StringRef, llvm::VersionTuple))
ERROR(attr_implementation_requires_language,none,
"'@implementation' used without specifying the language being "
"implemented",
())
ERROR(attr_implementation_category_goes_on_objc_attr,none,
"Objective-C category should be specified on '@objc', not "
"'@implementation'",
())

ERROR(member_of_objc_implementation_not_objc_or_final,none,
"%kind0 does not match any %kindonly0 declared in the headers for %1; "
Expand Down Expand Up @@ -6295,7 +6302,7 @@ ERROR(objc_extension_not_class,none,
"'@objc' can only be applied to an extension of a class", ())

// If you change this, also change enum ObjCReason
#define OBJC_ATTR_SELECT "select{marked @_cdecl|marked dynamic|marked @objc|marked @objcMembers|marked @IBOutlet|marked @IBAction|marked @IBSegueAction|marked @NSManaged|a member of an @objc protocol|implicitly @objc|an @objc override|an implementation of an @objc requirement|marked @IBInspectable|marked @GKInspectable|in an @objc extension of a class (without @nonobjc)|in an @_objcImplementation extension of a class (without final or @nonobjc)|marked @objc by an access note}"
#define OBJC_ATTR_SELECT "select{marked @_cdecl|marked dynamic|marked @objc|marked @objcMembers|marked @IBOutlet|marked @IBAction|marked @IBSegueAction|marked @NSManaged|a member of an @objc protocol|implicitly @objc|an @objc override|an implementation of an @objc requirement|marked @IBInspectable|marked @GKInspectable|in an @objc extension of a class (without @nonobjc)|in an @objc @implementation extension of a class (without final or @nonobjc)|marked @objc by an access note}"

ERROR(objc_invalid_on_var,none,
"property cannot be %" OBJC_ATTR_SELECT "0 "
Expand Down
6 changes: 0 additions & 6 deletions lib/AST/Decl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3717,12 +3717,6 @@ Identifier ExtensionDecl::getObjCCategoryName() const {
return Identifier();
}

// Fall back to @_objcImplementation attribute.
if (auto implAttr =
getAttrs().getAttribute<ObjCImplementationAttr>(/*AllowInvalid=*/true)) {
return implAttr->CategoryName;
}

// Not a category, evidently.
return Identifier();
}
Expand Down
11 changes: 10 additions & 1 deletion lib/ClangImporter/ClangImporter.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -6002,7 +6002,16 @@ constructResult(const llvm::TinyPtrVector<Decl *> &interfaces,
static bool isCategoryNameValid(ExtensionDecl *ext) {
auto attr = ext->getAttrs()
.getAttribute<ObjCImplementationAttr>(/*AllowInvalid=*/true);
return attr && !attr->isCategoryNameInvalid();

if (!attr)
return false;

// Clients using the stable syntax shouldn't have a category name on the attr.
// This is diagnosed in AttributeChecker::visitObjCImplementationAttr().
if (!attr->isEarlyAdopter() && !attr->CategoryName.empty())
return false;

return !attr->isCategoryNameInvalid();
}

static ObjCInterfaceAndImplementation
Expand Down
14 changes: 14 additions & 0 deletions lib/Parse/ParseDecl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -7124,6 +7124,20 @@ Parser::parseDeclExtension(ParseDeclOptions Flags, DeclAttributes &Attributes) {
status |= whereStatus;
}

// @implementation requires an explicit @objc attribute, but
// @_objcImplementation didn't. Insert one if necessary.
auto implAttr = Attributes.getAttribute<ObjCImplementationAttr>();
if (implAttr && implAttr->isEarlyAdopter()
&& !Attributes.hasAttribute<ObjCAttr>()) {
ObjCAttr *objcAttr;
if (implAttr->CategoryName.empty())
objcAttr = ObjCAttr::createUnnamedImplicit(Context);
else
objcAttr = ObjCAttr::createNullary(Context, implAttr->CategoryName,
/*isNameImplicit=*/false);
Attributes.add(objcAttr);
}

ExtensionDecl *ext = ExtensionDecl::create(Context, ExtensionLoc,
extendedType.getPtrOrNull(),
Context.AllocateCopy(Inherited),
Expand Down
97 changes: 78 additions & 19 deletions lib/Sema/TypeCheckAttr.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1547,23 +1547,86 @@ static bool hasObjCImplementationFeature(Decl *D, ObjCImplementationAttr *attr,
return false;
}

static SmallString<64> makeObjCAttrInsertionText(Identifier categoryName) {
SmallString<64> objcAttrText;
objcAttrText += "@objc";
if (!categoryName.empty()) {
objcAttrText += "(";
objcAttrText += categoryName.str();
objcAttrText += ")";
}
objcAttrText += " ";
return objcAttrText;
}

static SourceRange getArgListRange(ASTContext &Ctx, DeclAttribute *attr) {
// attr->getRange() covers the attr name and argument list; adjust it to
// exclude the first token.
auto newStart = Lexer::getLocForEndOfToken(Ctx.SourceMgr,
attr->getRange().Start);
if (attr->getRange().contains(newStart)) {
return SourceRange(newStart, attr->getRange().End);
}
return SourceRange();
}

void AttributeChecker::
visitObjCImplementationAttr(ObjCImplementationAttr *attr) {
DeclAttribute * langAttr =
D->getAttrs().getAttribute<ObjCAttr>(/*AllowInvalid=*/true);
if (!langAttr)
langAttr = D->getAttrs().getAttribute<CDeclAttr>(/*AllowInvalid=*/true);

if (!langAttr) {
diagnose(attr->getLocation(), diag::attr_implementation_requires_language);

// If this is an extension, suggest '@objc'.
if (auto ED = dyn_cast<ExtensionDecl>(D)) {
SourceLoc insertLoc = ED->getAttrs().getStartLoc();
if (insertLoc.isInvalid())
insertLoc = ED->getStartLoc();

diagnose(attr->getLocation(), diag::make_decl_objc_for_implementation,
D->getDescriptiveKind())
.fixItInsert(insertLoc,
makeObjCAttrInsertionText(attr->CategoryName));
}

return;
}

if (auto ED = dyn_cast<ExtensionDecl>(D)) {
if (!hasObjCImplementationFeature(D, attr, Feature::ObjCImplementation))
return;

// Only early adopters should specify the category name on the attribute;
// the stabilized syntax uses @objc(CustomName) for that.
if (!attr->isEarlyAdopter() && !attr->CategoryName.empty()) {
auto diag = diagnose(attr->getLocation(),
diag::attr_implementation_category_goes_on_objc_attr);

auto argListRange = getArgListRange(Ctx, attr);
if (argListRange.isValid()) {
diag.fixItReplace(langAttr->getRangeWithAt(),
makeObjCAttrInsertionText(attr->CategoryName));
diag.fixItRemove(argListRange);
}

cast<ObjCAttr>(langAttr)->setName(ObjCSelector(Ctx, 0,
{attr->CategoryName}),
/*implicit=*/false);

return;
}

if (ED->isConstrainedExtension())
diagnoseAndRemoveAttr(attr,
diag::attr_objc_implementation_must_be_unconditional);

auto CD = dyn_cast<ClassDecl>(ED->getExtendedNominal());
if (!CD) {
diagnoseAndRemoveAttr(attr,
diag::attr_objc_implementation_must_extend_class,
ED->getExtendedNominal());
ED->getExtendedNominal()->diagnose(diag::decl_declared_here,
ED->getExtendedNominal());
// This will be diagnosed as diag::objc_extension_not_class.
attr->setInvalid();
return;
}

Expand All @@ -1590,14 +1653,14 @@ visitObjCImplementationAttr(ObjCImplementationAttr *attr) {
if (!attr->isCategoryNameInvalid() && !ED->getImplementedObjCDecl()) {
diagnose(attr->getLocation(),
diag::attr_objc_implementation_category_not_found,
attr->CategoryName, CD);

// attr->getRange() covers the attr name and argument list; adjust it to
// exclude the first token.
auto newStart = Lexer::getLocForEndOfToken(Ctx.SourceMgr,
attr->getRange().Start);
if (attr->getRange().contains(newStart)) {
auto argListRange = SourceRange(newStart, attr->getRange().End);
ED->getObjCCategoryName(), CD);

SourceRange argListRange;
if (attr->CategoryName.empty())
argListRange = getArgListRange(Ctx, langAttr);
else
argListRange = getArgListRange(Ctx, attr);
if (argListRange.isValid()) {
diagnose(attr->getLocation(),
diag::attr_objc_implementation_fixit_remove_category_name)
.fixItRemove(argListRange);
Expand Down Expand Up @@ -1628,12 +1691,8 @@ visitObjCImplementationAttr(ObjCImplementationAttr *attr) {
diagnose(attr->getLocation(),
diag::attr_objc_implementation_no_category_for_func, AFD);

// attr->getRange() covers the attr name and argument list; adjust it to
// exclude the first token.
auto newStart = Lexer::getLocForEndOfToken(Ctx.SourceMgr,
attr->getRange().Start);
if (attr->getRange().contains(newStart)) {
auto argListRange = SourceRange(newStart, attr->getRange().End);
auto argListRange = getArgListRange(Ctx, attr);
if (argListRange.isValid()) {
diagnostic.fixItRemove(argListRange);
}

Expand Down
5 changes: 0 additions & 5 deletions lib/Sema/TypeCheckDecl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -960,11 +960,6 @@ IsDynamicRequest::evaluate(Evaluator &evaluator, ValueDecl *decl) const {
return true;
}

// @_objcImplementation extension member implementations are implicitly
// dynamic.
if (decl->isObjCMemberImplementation())
return true;

if (auto accessor = dyn_cast<AccessorDecl>(decl)) {
// Runtime-replaceable accessors are dynamic when their storage declaration
// is dynamic and they were explicitly defined or they are implicitly defined
Expand Down
47 changes: 36 additions & 11 deletions lib/Sema/TypeCheckDeclObjC.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1249,6 +1249,11 @@ static bool isMemberOfObjCClassExtension(const ValueDecl *VD) {
return ext->getSelfClassDecl() && ext->getAttrs().hasAttribute<ObjCAttr>();
}

static bool isMemberOfObjCImplementationExtension(const ValueDecl *VD) {
return isMemberOfObjCClassExtension(VD) &&
cast<ExtensionDecl>(VD->getDeclContext())->isObjCImplementation();
}

/// Whether this declaration is a member of a class with the `@objcMembers`
/// attribute.
static bool isMemberOfObjCMembersClass(const ValueDecl *VD) {
Expand Down Expand Up @@ -1471,14 +1476,6 @@ static std::optional<ObjCReason> shouldMarkAsObjC(const ValueDecl *VD,
return ObjCReason(ObjCReason::MemberOfObjCProtocol);
}

// A member implementation of an @objcImplementation extension is @objc.
if (VD->isObjCMemberImplementation()) {
auto ext = VD->getDeclContext()->getAsDecl();
auto attr = ext->getAttrs()
.getAttribute<ObjCImplementationAttr>(/*AllowInvalid=*/true);
return ObjCReason(ObjCReason::MemberOfObjCImplementationExtension, attr);
}

// A @nonobjc is not @objc, even if it is an override of an @objc, so check
// for @nonobjc first.
if (VD->getAttrs().hasAttribute<NonObjCAttr>() ||
Expand All @@ -1487,10 +1484,23 @@ static std::optional<ObjCReason> shouldMarkAsObjC(const ValueDecl *VD,
.hasAttribute<NonObjCAttr>()))
return std::nullopt;

if (isMemberOfObjCClassExtension(VD) &&
if (isMemberOfObjCImplementationExtension(VD)) {
// A `final` member of an @objc @implementation extension is not @objc.
if (VD->isFinal())
return std::nullopt;
// Other members get a special reason.
if (canInferImplicitObjC(/*allowAnyAccess*/true)) {
auto ext = VD->getDeclContext()->getAsDecl();
auto attr = ext->getAttrs()
.getAttribute<ObjCImplementationAttr>(/*AllowInvalid=*/true);
return ObjCReason(ObjCReason::MemberOfObjCImplementationExtension, attr);
}
}
else if (isMemberOfObjCClassExtension(VD) &&
canInferImplicitObjC(/*allowAnyAccess*/true))
return ObjCReason(ObjCReason::MemberOfObjCExtension);
if (isMemberOfObjCMembersClass(VD) &&

if (isMemberOfObjCMembersClass(VD) &&
canInferImplicitObjC(/*allowAnyAccess*/false))
return ObjCReason(ObjCReason::MemberOfObjCMembersClass);

Expand Down Expand Up @@ -3802,6 +3812,18 @@ class ObjCImplementationChecker {
}
}

static SmallString<64> makeObjCAttrInsertionText(Identifier categoryName) {
SmallString<64> objcAttrText;
objcAttrText += "@objc";
if (!categoryName.empty()) {
objcAttrText += "(";
objcAttrText += categoryName.str();
objcAttrText += ")";
}
objcAttrText += " ";
return objcAttrText;
}

void diagnoseEarlyAdopterDeprecation() {
// Only encourage use of @implementation for early adopters, and only when
// there are no mismatches that they might be working around with it.
Expand All @@ -3822,9 +3844,12 @@ class ObjCImplementationChecker {
if (!isa<ExtensionDecl>(decl))
return;

auto attrText = makeObjCAttrInsertionText(getAttr()->CategoryName);
attrText += "@implementation";

diagnose(getAttr()->getLocation(),
diag::objc_implementation_early_spelling_deprecated)
.fixItReplace(getAttr()->getLocation(), "implementation");
.fixItReplace(getAttr()->getRangeWithAt(), attrText);
}
};
}
Expand Down
2 changes: 1 addition & 1 deletion lib/Sema/TypeCheckObjC.h
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ class ObjCReason {
ExplicitlyGKInspectable,
/// Is it a member of an @objc extension of a class.
MemberOfObjCExtension,
/// Is it a member of an \@\_objcImplementation extension.
/// Is it a member of an \@objc \@implementation extension.
MemberOfObjCImplementationExtension,
/// Has an explicit '@objc' attribute added by an access note, rather than
/// written in source code.
Expand Down
9 changes: 9 additions & 0 deletions test/decl/ext/Inputs/objc_implementation.h
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,15 @@

@end

@interface ObjCBadClass : NSObject
@end

@interface ObjCBadClass (BadCategory1)
@end

@interface ObjCBadClass (BadCategory2)
@end

@protocol EmptyObjCProto
@end

Expand Down
Loading

0 comments on commit 80b9929

Please sign in to comment.