Require @objc to be used with @implementation

…for extensions. This change also removes @implementation(CategoryName); you should attach the category name to the @objc attribute instead. And there are small changes to how much checking the compiler will do on an @objc @implementation after the decl checker has discovered a problem with it.

Author: beccadax

include/swift/AST/Attr.h

@@ -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,

include/swift/AST/DiagnosticsSema.def

@@ -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,
@@ -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 "
@@ -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; "
@@ -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 "

lib/AST/Decl.cpp

@@ -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();
 }

lib/ClangImporter/ClangImporter.cpp

@@ -5982,7 +5982,7 @@ constructResult(const llvm::TinyPtrVector<Decl *> &interfaces,
     auto &diags = interfaces.front()->getASTContext().Diags;
     for (auto extraImpl : llvm::ArrayRef<Decl *>(impls).drop_front()) {
       auto attr = extraImpl->getAttrs().getAttribute<ObjCImplementationAttr>();
-      attr->setCategoryNameInvalid();
+      attr->setInvalid();
 
       // @objc @implementations for categories are diagnosed as category
       // conflicts, so we're only concerned with main class bodies and
@@ -5999,10 +5999,18 @@ constructResult(const llvm::TinyPtrVector<Decl *> &interfaces,
   return ObjCInterfaceAndImplementation(interfaces, impls.front());
 }
 
-static bool isCategoryNameValid(ExtensionDecl *ext) {
-  auto attr = ext->getAttrs()
-                .getAttribute<ObjCImplementationAttr>(/*AllowInvalid=*/true);
-  return attr && !attr->isCategoryNameInvalid();
+static bool isImplValid(ExtensionDecl *ext) {
+  auto attr = ext->getAttrs().getAttribute<ObjCImplementationAttr>();
+
+  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
@@ -6020,7 +6028,7 @@ findContextInterfaceAndImplementation(DeclContext *dc) {
 
   if (auto ext = dyn_cast<ExtensionDecl>(dc)) {
     assert(ext);
-    if (!ext->hasClangNode() && !isCategoryNameValid(ext))
+    if (!ext->hasClangNode() && !isImplValid(ext))
       return {};
 
     categoryName = ext->getObjCCategoryName();
@@ -6038,7 +6046,7 @@ findContextInterfaceAndImplementation(DeclContext *dc) {
   for (ExtensionDecl *ext : classDecl->getExtensions()) {
     if (ext->isObjCImplementation()
           && ext->getObjCCategoryName() == categoryName
-          && isCategoryNameValid(ext))
+          && isImplValid(ext))
       implDecls.push_back(ext);
   }
 

lib/Parse/ParseDecl.cpp

@@ -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),

lib/Sema/TypeCheckAttr.cpp

@@ -1536,7 +1536,9 @@ void AttributeChecker::visitNonObjCAttr(NonObjCAttr *attr) {
 
 static bool hasObjCImplementationFeature(Decl *D, ObjCImplementationAttr *attr,
                                          Feature requiredFeature) {
-  if (D->getASTContext().LangOpts.hasFeature(requiredFeature))
+  auto &ctx = D->getASTContext();
+
+  if (ctx.LangOpts.hasFeature(requiredFeature))
     return true;
 
   // Allow the use of @_objcImplementation *without* Feature::ObjCImplementation
@@ -1547,29 +1549,81 @@ static bool hasObjCImplementationFeature(Decl *D, ObjCImplementationAttr *attr,
 
   // Either you're using Feature::ObjCImplementation without the early adopter
   // syntax, or you're using Feature::CImplementation. Either way, no go.
-  swift::diagnoseAndRemoveAttr(D, attr, diag::requires_experimental_feature,
-                               attr->getAttrName(), attr->isDeclModifier(),
-                               getFeatureName(requiredFeature));
+  ctx.Diags.diagnose(attr->getLocation(), diag::requires_experimental_feature,
+                     attr->getAttrName(), attr->isDeclModifier(),
+                     getFeatureName(requiredFeature));
   return false;
 }
 
+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)) {
+      auto diag = diagnose(attr->getLocation(),
+                           diag::make_decl_objc_for_implementation,
+                           D->getDescriptiveKind());
+
+      ObjCSelector correctSelector(Ctx, 0, {attr->CategoryName});
+      fixDeclarationObjCName(diag, ED, ObjCSelector(), correctSelector);
+    }
+
+    return;
+  }
+
   if (auto ED = dyn_cast<ExtensionDecl>(D)) {
     if (!hasObjCImplementationFeature(D, attr, Feature::ObjCImplementation))
       return;
 
+    auto objcLangAttr = dyn_cast<ObjCAttr>(langAttr);
+    assert(objcLangAttr && "extension with @_cdecl or another lang attr???");
+
+    // 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);
+
+      ObjCSelector correctSelector(Ctx, 0, {attr->CategoryName});
+      auto argListRange = getArgListRange(Ctx, attr);
+      if (argListRange.isValid()) {
+        diag.fixItRemove(argListRange);
+        fixDeclarationObjCName(diag, ED, objcLangAttr->getName(),
+                               correctSelector);
+      }
+      objcLangAttr->setName(correctSelector, /*implicit=*/false);
+
+      attr->setCategoryNameInvalid();
+
+      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;
     }
 
@@ -1581,35 +1635,36 @@ visitObjCImplementationAttr(ObjCImplementationAttr *attr) {
     }
 
     if (!CD->hasSuperclass()) {
-      diagnoseAndRemoveAttr(attr, diag::attr_objc_implementation_must_have_super,
-                            CD);
+      diagnoseAndRemoveAttr(attr,
+                            diag::attr_objc_implementation_must_have_super, CD);
       CD->diagnose(diag::decl_declared_here, CD);
       return;
     }
 
     if (CD->isTypeErasedGenericClass()) {
-      diagnoseAndRemoveAttr(attr, diag::objc_implementation_cannot_have_generics,
-                            CD);
+      diagnoseAndRemoveAttr(attr,
+                            diag::objc_implementation_cannot_have_generics, CD);
       CD->diagnose(diag::decl_declared_here, CD);
+      return;
     }
 
     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);
       }
 
-      attr->setCategoryNameInvalid();
+      attr->setInvalid();
 
       return;
     }
@@ -1634,12 +1689,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);
       }
 

lib/Sema/TypeCheckDecl.cpp

@@ -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

lib/Sema/TypeCheckDeclObjC.cpp

@@ -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) {
@@ -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>() ||
@@ -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);
 
@@ -3849,9 +3859,13 @@ class ObjCImplementationChecker {
     if (!isa<ExtensionDecl>(decl))
       return;
 
-    diagnose(getAttr()->getLocation(),
-             diag::objc_implementation_early_spelling_deprecated)
-        .fixItReplace(getAttr()->getLocation(), "implementation");
+    auto diag = diagnose(getAttr()->getLocation(),
+                         diag::objc_implementation_early_spelling_deprecated);
+    diag.fixItReplace(getAttr()->getRangeWithAt(), "@implementation");
+
+    ObjCSelector correctSelector(decl->getASTContext(), 0,
+                                 {getAttr()->CategoryName});
+    fixDeclarationObjCName(diag, decl, ObjCSelector(), correctSelector);
   }
 };
 }

lib/Sema/TypeCheckObjC.h

@@ -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.