Merge pull request #72596 from beccadax/objcimpl-real-name

beccadax · web-flow · commit 576a4bac364b · 2024-03-28T23:48:24.000-07:00
Add @implementation and feature flags for objcImpl
diff --git a/docs/ReferenceGuides/UnderscoredAttributes.md b/docs/ReferenceGuides/UnderscoredAttributes.md
@@ -779,6 +779,11 @@ of a header as non-`Sendable` so that you can make spot exceptions with
 
 ## `@_objcImplementation(CategoryName)`
 
+A pre-stable form of `@implementation`. The main difference between them is that
+many things that are errors with `@implementation` are warnings with
+`@_objcImplementation`, which permitted workarounds for compiler bugs and 
+changes in compiler behavior.
+
 Declares an extension that defines an implementation for the Objective-C
 category `CategoryName` on the class in question, or for the main `@interface`
 if the argument list is omitted.
@@ -841,12 +846,6 @@ Notes:
 
 * We don't currently plan to support ObjC generics.
 
-* Eventually, we want the main `@_objcImplementation` extension to be able to
-  declare stored properties that aren't in the interface. We also want
-  `final` stored properties to be allowed to be resilent Swift types, but
-  it's not clear how to achieve that without boxing them in `__SwiftValue`
-  (which we might do as a stopgap).
-     
 * We should think about ObjC "direct" members, but that would probably
   require a way to spell this in Swift. 
 
diff --git a/include/swift/AST/ASTBridging.h b/include/swift/AST/ASTBridging.h
@@ -625,10 +625,10 @@ BridgedObjCAttr BridgedObjCAttr_createParsedSelector(
     BridgedArrayRef cNameLocs, BridgedArrayRef cNames,
     BridgedSourceLoc cRParenLoc);
 
-SWIFT_NAME("BridgedObjCImplementationAttr.createParsed(_:atLoc:range:name:)")
+SWIFT_NAME("BridgedObjCImplementationAttr.createParsed(_:atLoc:range:name:isEarlyAdopter:)")
 BridgedObjCImplementationAttr BridgedObjCImplementationAttr_createParsed(
     BridgedASTContext cContext, BridgedSourceLoc cAtLoc,
-    BridgedSourceRange cRange, BridgedIdentifier cName);
+    BridgedSourceRange cRange, BridgedIdentifier cName, bool isEarlyAdopter);
 
 SWIFT_NAME("BridgedObjCRuntimeNameAttr.createParsed(_:atLoc:range:name:)")
 BridgedObjCRuntimeNameAttr BridgedObjCRuntimeNameAttr_createParsed(
diff --git a/include/swift/AST/Attr.h b/include/swift/AST/Attr.h
@@ -184,8 +184,9 @@ class DeclAttribute : public AttributeBase {
       isUnchecked : 1
     );
 
-    SWIFT_INLINE_BITFIELD(ObjCImplementationAttr, DeclAttribute, 1,
-      isCategoryNameInvalid : 1
+    SWIFT_INLINE_BITFIELD(ObjCImplementationAttr, DeclAttribute, 2,
+      isCategoryNameInvalid : 1,
+      isEarlyAdopter : 1
     );
 
     SWIFT_INLINE_BITFIELD(NonisolatedAttr, DeclAttribute, 1,
@@ -2424,11 +2425,19 @@ class ObjCImplementationAttr final : public DeclAttribute {
   Identifier CategoryName;
 
   ObjCImplementationAttr(Identifier CategoryName, SourceLoc AtLoc,
-                         SourceRange Range, bool Implicit = false,
+                         SourceRange Range, bool isEarlyAdopter = false,
+                         bool Implicit = false,
                          bool isCategoryNameInvalid = false)
       : DeclAttribute(DeclAttrKind::ObjCImplementation, AtLoc, Range, Implicit),
         CategoryName(CategoryName) {
     Bits.ObjCImplementationAttr.isCategoryNameInvalid = isCategoryNameInvalid;
+    Bits.ObjCImplementationAttr.isEarlyAdopter = isEarlyAdopter;
+  }
+
+  /// Early adopters use the \c \@_objcImplementation spelling. For backwards
+  /// compatibility, issues with them are diagnosed as warnings, not errors.
+  bool isEarlyAdopter() const {
+    return Bits.ObjCImplementationAttr.isEarlyAdopter;
   }
 
   bool isCategoryNameInvalid() const {
diff --git a/include/swift/AST/DeclAttr.def b/include/swift/AST/DeclAttr.def
@@ -188,9 +188,10 @@ SIMPLE_DECL_ATTR(_staticInitializeObjCMetadata, StaticInitializeObjCMetadata,
 DECL_ATTR(_restatedObjCConformance, RestatedObjCConformance,
   OnProtocol | UserInaccessible | LongAttribute | RejectByParser | NotSerialized | ABIStableToAdd | ABIStableToRemove | APIStableToAdd | APIStableToRemove,
   70)
-DECL_ATTR(_objcImplementation, ObjCImplementation,
+DECL_ATTR(implementation, ObjCImplementation,
   OnExtension | OnAbstractFunction | UserInaccessible | ABIBreakingToAdd | ABIBreakingToRemove | APIBreakingToAdd | APIBreakingToRemove,
   72)
+DECL_ATTR_ALIAS(_objcImplementation, ObjCImplementation)
 DECL_ATTR(_optimize, Optimize,
   OnAbstractFunction | OnSubscript | OnVar | UserInaccessible | ABIStableToAdd | ABIStableToRemove | APIStableToAdd | APIStableToRemove,
   73)
diff --git a/include/swift/AST/DiagnosticsSema.def b/include/swift/AST/DiagnosticsSema.def
@@ -1739,6 +1739,9 @@ ERROR(swift_native_objc_runtime_base_not_on_root_class,none,
       "@_swift_native_objc_runtime_base_not_on_root_class can only be applied "
       "to root classes", ())
 
+WARNING(objc_implementation_early_spelling_deprecated,none,
+        "'@_objcImplementation' is deprecated; use '@implementation' instead",
+        ())
 ERROR(attr_objc_implementation_must_be_unconditional,none,
       "only unconditional extensions can implement an Objective-C '@interface'",
       ())
@@ -1917,8 +1920,7 @@ NOTE(objc_implementation_one_matched_requirement,none,
      (ValueDecl *, ObjCSelector, bool, StringRef))
 
 WARNING(wrap_objc_implementation_will_become_error,none,
-        "%0; this will become an error before '@_objcImplementation' is "
-        "stabilized",
+        "%0; this will become an error after adopting '@implementation'",
         (DiagnosticInfo *))
 
 ERROR(cdecl_not_at_top_level,none,
diff --git a/include/swift/Basic/Features.def b/include/swift/Basic/Features.def
@@ -367,6 +367,12 @@ EXPERIMENTAL_FEATURE(IsolatedAny2, true)
 // Enable usability improvements for global-actor-isolated types.
 EXPERIMENTAL_FEATURE(GlobalActorIsolatedTypesUsability, false)
 
+// Enable @implementation on extensions of ObjC classes.
+EXPERIMENTAL_FEATURE(ObjCImplementation, true)
+
+// Enable @implementation on @_cdecl functions.
+EXPERIMENTAL_FEATURE(CImplementation, true)
+
 #undef EXPERIMENTAL_FEATURE_EXCLUDED_FROM_MODULE_INTERFACE
 #undef EXPERIMENTAL_FEATURE
 #undef UPCOMING_FEATURE
diff --git a/lib/AST/ASTBridging.cpp b/lib/AST/ASTBridging.cpp
@@ -662,9 +662,10 @@ BridgedObjCAttr BridgedObjCAttr_createParsedSelector(
 
 BridgedObjCImplementationAttr BridgedObjCImplementationAttr_createParsed(
     BridgedASTContext cContext, BridgedSourceLoc cAtLoc,
-    BridgedSourceRange cRange, BridgedIdentifier cName) {
+    BridgedSourceRange cRange, BridgedIdentifier cName, bool isEarlyAdopter) {
   return new (cContext.unbridged()) ObjCImplementationAttr(
-      cName.unbridged(), cAtLoc.unbridged(), cRange.unbridged());
+      cName.unbridged(), cAtLoc.unbridged(), cRange.unbridged(),
+      isEarlyAdopter);
 }
 
 BridgedObjCRuntimeNameAttr BridgedObjCRuntimeNameAttr_createParsed(
diff --git a/lib/AST/ASTDumper.cpp b/lib/AST/ASTDumper.cpp
@@ -1084,6 +1084,16 @@ namespace {
 
       printFlag(D->isImplicit(), "implicit", DeclModifierColor);
       printFlag(D->isHoisted(), "hoisted", DeclModifierColor);
+
+      if (auto implAttr = D->getAttrs().getAttribute<ObjCImplementationAttr>()) {
+        StringRef label =
+            implAttr->isEarlyAdopter() ? "objc_impl" : "clang_impl";
+        if (implAttr->CategoryName.empty())
+          printFlag(label);
+        else
+          printFieldQuoted(implAttr->CategoryName.str(), label);
+      }
+
       printSourceRange(D->getSourceRange(), &D->getASTContext());
       printFlag(D->TrailingSemiLoc.isValid(), "trailing_semi",
                 DeclModifierColor);
diff --git a/lib/AST/Attr.cpp b/lib/AST/Attr.cpp
@@ -1820,7 +1820,9 @@ StringRef DeclAttribute::getAttrName() const {
   case DeclAttrKind::ObjCRuntimeName:
     return "objc";
   case DeclAttrKind::ObjCImplementation:
-    return "_objcImplementation";
+    if (cast<ObjCImplementationAttr>(this)->isEarlyAdopter())
+      return "_objcImplementation";
+    return "implementation";
   case DeclAttrKind::MainType:
     return "main";
   case DeclAttrKind::DynamicReplacement:
diff --git a/lib/AST/FeatureSet.cpp b/lib/AST/FeatureSet.cpp
@@ -687,6 +687,9 @@ static bool usesFeatureGlobalActorIsolatedTypesUsability(Decl *decl) {
   return false;
 }
 
+UNINTERESTING_FEATURE(ObjCImplementation)
+UNINTERESTING_FEATURE(CImplementation)
+
 // ----------------------------------------------------------------------------
 // MARK: - FeatureSet
 // ----------------------------------------------------------------------------
diff --git a/lib/ASTGen/Sources/ASTGen/DeclAttrs.swift b/lib/ASTGen/Sources/ASTGen/DeclAttrs.swift
@@ -732,19 +732,25 @@ extension ASTGenVisitor {
   }
 
   func generateObjCImplementationAttr(attribute node: AttributeSyntax) -> BridgedObjCImplementationAttr? {
-    let name: BridgedIdentifier? = self.generateSingleAttrOption(attribute: node) {
-      self.generateIdentifier($0)
-    }
+    let name: BridgedIdentifier? = self.generateSingleAttrOption(
+      attribute: node,
+      self.generateIdentifier,
+      valueIfOmitted: BridgedIdentifier()
+    )
     guard let name else {
-      // TODO: Diagnose.
+      // Should be diagnosed by `generateSingleAttrOption`.
       return nil
     }
 
+    let attrName = node.attributeName.as(IdentifierTypeSyntax.self)?.name.text
+    let isEarlyAdopter = attrName != "implementation"
+
     return .createParsed(
       self.ctx,
       atLoc: self.generateSourceLoc(node.atSign),
       range: self.generateSourceRange(node),
-      name: name
+      name: name,
+      isEarlyAdopter: isEarlyAdopter
     )
   }
 
@@ -1060,6 +1066,11 @@ extension ASTGenVisitor {
       return nil
     }
 
+    if case .token(let tok) = arguments {
+      // Special case: was parsed as a token, not an an argument list
+      return valueGeneratorFunction(tok)
+    }
+
     guard var arguments = arguments.as(LabeledExprListSyntax.self)?[...] else {
       // TODO: Diagnose.
       return nil
diff --git a/lib/Parse/ParseDecl.cpp b/lib/Parse/ParseDecl.cpp
@@ -3640,7 +3640,9 @@ ParserStatus Parser::parseNewDeclAttribute(DeclAttributes &Attributes,
     if (!name)
       return makeParserSuccess();
 
-    Attributes.add(new (Context) ObjCImplementationAttr(*name, AtLoc, range));
+    bool isEarlyAdopter = (AttrName != "implementation");
+    Attributes.add(new (Context) ObjCImplementationAttr(*name, AtLoc, range,
+                                                        isEarlyAdopter));
     break;
   }
   case DeclAttrKind::ObjCRuntimeName: {
diff --git a/lib/Sema/TypeCheckAttr.cpp b/lib/Sema/TypeCheckAttr.cpp
@@ -1516,9 +1516,31 @@ void AttributeChecker::visitNonObjCAttr(NonObjCAttr *attr) {
   }
 }
 
+static bool hasObjCImplementationFeature(Decl *D, ObjCImplementationAttr *attr,
+                                         Feature requiredFeature) {
+  if (D->getASTContext().LangOpts.hasFeature(requiredFeature))
+    return true;
+
+  // Allow the use of @_objcImplementation *without* Feature::ObjCImplementation
+  // as long as you're using the early adopter syntax. (Avoids breaking existing
+  // adopters.)
+  if (requiredFeature == Feature::ObjCImplementation && attr->isEarlyAdopter())
+    return true;
+
+  // 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));
+  return false;
+}
+
 void AttributeChecker::
 visitObjCImplementationAttr(ObjCImplementationAttr *attr) {
   if (auto ED = dyn_cast<ExtensionDecl>(D)) {
+    if (!hasObjCImplementationFeature(D, attr, Feature::ObjCImplementation))
+      return;
+
     if (ED->isConstrainedExtension())
       diagnoseAndRemoveAttr(attr,
                             diag::attr_objc_implementation_must_be_unconditional);
@@ -1575,6 +1597,9 @@ visitObjCImplementationAttr(ObjCImplementationAttr *attr) {
     }
   }
   else if (auto AFD = dyn_cast<AbstractFunctionDecl>(D)) {
+    if (!hasObjCImplementationFeature(D, attr, Feature::CImplementation))
+      return;
+
     if (!attr->CategoryName.empty()) {
       auto diagnostic =
           diagnose(attr->getLocation(),
diff --git a/lib/Sema/TypeCheckDeclObjC.cpp b/lib/Sema/TypeCheckDeclObjC.cpp
@@ -2803,16 +2803,26 @@ fixDeclarationStaticSpelling(InFlightDiagnostic &diag, ValueDecl *VD,
 
 namespace {
 class ObjCImplementationChecker {
-  DiagnosticEngine &diags;
+  Decl *decl;
+
+  ObjCImplementationAttr *getAttr() const {
+    return decl->getAttrs()
+               .getAttribute<ObjCImplementationAttr>(/*AllowInvalid=*/true);
+  }
 
   template<typename Loc, typename ...ArgTypes>
   InFlightDiagnostic diagnose(Loc loc, Diag<ArgTypes...> diagID,
                         typename detail::PassArgument<ArgTypes>::type... Args) {
+    hasDiagnosed = true;
+
+    auto &diags = decl->getASTContext().Diags;
     auto diag = diags.diagnose(loc, diagID, std::forward<ArgTypes>(Args)...);
 
-    // WORKAROUND (5.9): Soften newly-introduced errors to make things easier
-    // for early adopters.
-    if (diags.declaredDiagnosticKindFor(diagID.ID) == DiagnosticKind::Error)
+    // Early adopters using the '@_objcImplementation' syntax may have had the
+    // ObjCImplementationChecker evolve out from under them. Soften their errors
+    // to warnings so we don't break their projects.
+    if (getAttr()->isEarlyAdopter()
+         && diags.declaredDiagnosticKindFor(diagID.ID) == DiagnosticKind::Error)
       diag.wrapIn(diag::wrap_objc_implementation_will_become_error);
 
     return diag;
@@ -2823,9 +2833,11 @@ class ObjCImplementationChecker {
   /// Candidates with their explicit ObjC names, if any.
   llvm::SmallDenseMap<ValueDecl *, ObjCSelector, 16> unmatchedCandidates;
 
+  bool hasDiagnosed = false;
+
 public:
   ObjCImplementationChecker(Decl *D)
-      : diags(D->getASTContext().Diags)
+      : decl(D), hasDiagnosed(getAttr()->isInvalid())
   {
     assert(!D->hasClangNode() && "passed interface, not impl, to checker");
 
@@ -3599,6 +3611,31 @@ class ObjCImplementationChecker {
             .fixItInsert(cand->getAttributeInsertionLoc(true), "final ");
     }
   }
+
+  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.
+    if (hasDiagnosed || !getAttr()->isEarlyAdopter())
+      return;
+
+    // Only encourage adoption if the corresponding language feature is enabled.
+    if (isa<ExtensionDecl>(decl) &&
+        !decl->getASTContext().LangOpts.hasFeature(Feature::ObjCImplementation))
+      return;
+
+    if (isa<AbstractFunctionDecl>(decl) &&
+        !decl->getASTContext().LangOpts.hasFeature(Feature::CImplementation))
+      return;
+
+    // Only encourage @_objcImplementation *extension* adopters to adopt
+    // @implementation; @_objcImplementation @_cdecl hasn't been stabilized yet.
+    if (!isa<ExtensionDecl>(decl))
+      return;
+
+    diagnose(getAttr()->getLocation(),
+             diag::objc_implementation_early_spelling_deprecated)
+        .fixItReplace(getAttr()->getLocation(), "implementation");
+  }
 };
 }
 
@@ -3618,6 +3655,7 @@ evaluate(Evaluator &evaluator, Decl *D) const {
   checker.matchRequirements();
   checker.diagnoseUnmatchedCandidates();
   checker.diagnoseUnmatchedRequirements();
+  checker.diagnoseEarlyAdopterDeprecation();
 
   return evaluator::SideEffect();
 }
diff --git a/lib/Serialization/Deserialization.cpp b/lib/Serialization/Deserialization.cpp
@@ -6127,13 +6127,15 @@ llvm::Error DeclDeserializer::deserializeDeclCommon() {
       case decls_block::ObjCImplementation_DECL_ATTR: {
         bool isImplicit;
         bool isCategoryNameInvalid;
+        bool isEarlyAdopter;
         uint64_t categoryNameID;
         serialization::decls_block::ObjCImplementationDeclAttrLayout::
             readRecord(scratch, isImplicit, isCategoryNameInvalid,
-                       categoryNameID);
+                       isEarlyAdopter, categoryNameID);
         Identifier categoryName = MF.getIdentifier(categoryNameID);
         Attr = new (ctx) ObjCImplementationAttr(categoryName, SourceLoc(),
-                                                SourceRange(), isImplicit,
+                                                SourceRange(), isEarlyAdopter,
+                                                isImplicit,
                                                 isCategoryNameInvalid);
         break;
       }
diff --git a/lib/Serialization/ModuleFormat.h b/lib/Serialization/ModuleFormat.h
@@ -58,7 +58,7 @@ const uint16_t SWIFTMODULE_VERSION_MAJOR = 0;
 /// describe what change you made. The content of this comment isn't important;
 /// it just ensures a conflict if two people change the module format.
 /// Don't worry about adhering to the 80-column limit for this line.
-const uint16_t SWIFTMODULE_VERSION_MINOR = 868; // end_apply result
+const uint16_t SWIFTMODULE_VERSION_MINOR = 869; // @implementation
 
 /// A standard hash seed used for all string hashes in a serialized module.
 ///
@@ -2314,6 +2314,7 @@ namespace decls_block {
     ObjCImplementation_DECL_ATTR,
     BCFixed<1>,                // implicit flag
     BCFixed<1>,                // category name invalid
+    BCFixed<1>,                // is early adopter
     IdentifierIDField          // category name
   >;
 
diff --git a/lib/Serialization/Serialization.cpp b/lib/Serialization/Serialization.cpp
@@ -2947,7 +2947,7 @@ class Serializer::DeclSerializer : public DeclVisitor<DeclSerializer> {
           S.DeclTypeAbbrCodes[ObjCImplementationDeclAttrLayout::Code];
       ObjCImplementationDeclAttrLayout::emitRecord(S.Out, S.ScratchRecord,
           abbrCode, theAttr->isImplicit(), theAttr->isCategoryNameInvalid(),
-          categoryNameID);
+          theAttr->isEarlyAdopter(), categoryNameID);
       return;
     }
 
diff --git a/test/ASTGen/attrs.swift b/test/ASTGen/attrs.swift
@@ -57,3 +57,8 @@ func testMutating(value: S3) {
 class C1 {}
 @_alignment(7) // expected-error {{alignment value must be a power of two}}
 struct S4 {}
+
+@implementation extension ObjCClass1 {} // expected-error {{cannot find type 'ObjCClass1' in scope}}
+@implementation(Category) extension ObjCClass1 {} // expected-error {{cannot find type 'ObjCClass1' in scope}}
+@_objcImplementation extension ObjCClass2 {} // expected-error {{cannot find type 'ObjCClass2' in scope}}
+@_objcImplementation(Category) extension ObjCClass2 {} // expected-error {{cannot find type 'ObjCClass2' in scope}}
diff --git a/test/IRGen/objc_implementation.swift b/test/IRGen/objc_implementation.swift
diff --git a/test/Interpreter/objc_implementation.swift b/test/Interpreter/objc_implementation.swift
diff --git a/test/Interpreter/objc_implementation_swift_client.swift b/test/Interpreter/objc_implementation_swift_client.swift
diff --git a/test/PrintAsObjC/objc_implementation.swift b/test/PrintAsObjC/objc_implementation.swift
diff --git a/test/decl/ext/Inputs/objc_implementation.h b/test/decl/ext/Inputs/objc_implementation.h
diff --git a/test/decl/ext/cdecl_implementation_features.swift b/test/decl/ext/cdecl_implementation_features.swift
diff --git a/test/decl/ext/objc_implementation.swift b/test/decl/ext/objc_implementation.swift
diff --git a/test/decl/ext/objc_implementation_features.swift b/test/decl/ext/objc_implementation_features.swift

Original file line number	Diff line number	Diff line change
`@@ -687,6 +687,9 @@ static bool usesFeatureGlobalActorIsolatedTypesUsability(Decl *decl) {`
`687`	`687`	`return false;`
`688`	`688`	`}`
`689`	`689`
	`690`	`+UNINTERESTING_FEATURE(ObjCImplementation)`
	`691`	`+UNINTERESTING_FEATURE(CImplementation)`
	`692`	`+`
`690`	`693`	`// ----------------------------------------------------------------------------`
`691`	`694`	`// MARK: - FeatureSet`
`692`	`695`	`// ----------------------------------------------------------------------------`