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.

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

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

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

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

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

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.

test/decl/ext/Inputs/objc_implementation.h

@@ -191,6 +191,15 @@
 
 @end
 
+@interface ObjCBadClass : NSObject
+@end
+
+@interface ObjCBadClass (BadCategory1)
+@end
+
+@interface ObjCBadClass (BadCategory2)
+@end
+
 @protocol EmptyObjCProto
 @end