Merge pull request #69257 from beccadax/convenience-implementation

Improve support for Swift-only initializers in @_objcImplementation checker

Author: swift-ci

include/swift/AST/DiagnosticsSema.def

@@ -1841,6 +1841,25 @@ NOTE(objc_implementation_requirement_here,none,
      "%kind0 declared in header here",
      (ValueDecl *))
 
+ERROR(objc_implementation_init_must_be_convenience, none,
+      "%kind0 is not valid in an '@_objcImplementation' extension because "
+      "Objective-C subclasses must be able to override "
+      "%select{designated|required}1 initializers",
+      (const ValueDecl *, /*isRequired=*/bool))
+NOTE(objc_implementation_init_turn_designated_to_convenience, none,
+     "add 'convenience' keyword to make this a convenience initializer",
+     ())
+NOTE(objc_implementation_init_turn_required_to_convenience, none,
+     "replace 'required' keyword with 'convenience' to make this a convenience "
+     "initializer",
+     ())
+
+// Fallback diagnostic; super-general by nature.
+ERROR(objc_implementation_member_requires_vtable, none,
+      "%kind0 is not valid in an '@_objcImplementation' extension because it "
+      "is an overridable Swift-only %kindonly0",
+      (const ValueDecl *))
+
 ERROR(objc_implementation_multiple_matching_requirements,none,
       "%kind0 could match several different members declared in the header",
       (ValueDecl *))
@@ -6100,7 +6119,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)|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 @_objcImplementation extension of a class (without final or @nonobjc)|marked @objc by an access note}"
 
 WARNING(attribute_meaningless_when_nonobjc,none,
         "'@%0' attribute is meaningless on a property that cannot be "

lib/AST/Decl.cpp

@@ -1818,7 +1818,7 @@ Type ExtensionDecl::getExtendedType() const {
 }
 
 bool ExtensionDecl::isObjCImplementation() const {
-  return getAttrs().hasAttribute<ObjCImplementationAttr>();
+  return getAttrs().hasAttribute<ObjCImplementationAttr>(/*AllowInvalid=*/true);
 }
 
 llvm::Optional<Identifier>
@@ -4300,6 +4300,7 @@ isObjCMemberImplementation(const ValueDecl *VD,
                     ? cast<AccessorDecl>(VD)->getStorage()
                     : VD;
       return !attrDecl->isFinal()
+                  && !attrDecl->getAttrs().hasAttribute<NonObjCAttr>()
                   && !attrDecl->getAttrs().hasAttribute<OverrideAttr>()
                   && getAccessLevel() >= AccessLevel::Internal;
     }

lib/Sema/TypeCheckDecl.cpp

@@ -1173,7 +1173,7 @@ NeedsNewVTableEntryRequest::evaluate(Evaluator &evaluator,
                                      AbstractFunctionDecl *decl) const {
   auto *dc = decl->getDeclContext();
 
-  if (!isa<ClassDecl>(dc))
+  if (!isa<ClassDecl>(dc->getImplementedObjCContext()))
     return false;
 
   // Destructors always use a fixed vtable entry.

lib/Sema/TypeCheckDeclObjC.cpp

@@ -51,6 +51,7 @@ swift::behaviorLimitForObjCReason(ObjCReason reason, ASTContext &ctx) {
   case ObjCReason::WitnessToObjC:
   case ObjCReason::ImplicitlyObjC:
   case ObjCReason::MemberOfObjCExtension:
+  case ObjCReason::MemberOfObjCImplementationExtension:
     return DiagnosticBehavior::Unspecified;
 
   case ObjCReason::ExplicitlyIBInspectable:
@@ -88,6 +89,7 @@ unsigned swift::getObjCDiagnosticAttrKind(ObjCReason reason) {
   case ObjCReason::ExplicitlyIBInspectable:
   case ObjCReason::ExplicitlyGKInspectable:
   case ObjCReason::MemberOfObjCExtension:
+  case ObjCReason::MemberOfObjCImplementationExtension:
   case ObjCReason::ExplicitlyObjCByAccessNote:
     return static_cast<unsigned>(reason);
 
@@ -137,6 +139,7 @@ void ObjCReason::describe(const Decl *D) const {
   case ObjCReason::MemberOfObjCExtension:
   case ObjCReason::MemberOfObjCMembersClass:
   case ObjCReason::MemberOfObjCSubclass:
+  case ObjCReason::MemberOfObjCImplementationExtension:
   case ObjCReason::ElementOfObjCEnum:
   case ObjCReason::Accessor:
     // No additional note required.
@@ -1463,9 +1466,12 @@ static llvm::Optional<ObjCReason> shouldMarkAsObjC(const ValueDecl *VD,
   }
 
   // A member implementation of an @objcImplementation extension is @objc.
-  if (VD->isObjCMemberImplementation())
-    // FIXME: New ObjCReason::Kind?
-    return ObjCReason(ObjCReason::ImplicitlyObjC);
+  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.
@@ -2952,6 +2958,58 @@ class ObjCImplementationChecker {
     return nullptr;
   }
 
+  void diagnoseVTableUse(ValueDecl *member) {
+    if (auto storage = dyn_cast<AbstractStorageDecl>(member)) {
+      for (auto accessor : storage->getAllAccessors()) {
+        diagnoseVTableUse(accessor);
+      }
+      return;
+    }
+
+    auto afd = dyn_cast_or_null<AbstractFunctionDecl>(member);
+    if (!afd || !afd->needsNewVTableEntry())
+      return;
+
+    // Don't diagnose if we already diagnosed an unrelated ObjC interop issue,
+    // like an un-representable type. If there's an `@objc` attribute on the
+    // member, this will be indicated by its `isInvalid()` bit; otherwise we'll
+    // use the enclosing extension's `@_objcImplementation` attribute.
+    DeclAttribute *attr = afd->getAttrs()
+                              .getAttribute<ObjCAttr>(/*AllowInvalid=*/true);
+    if (!attr)
+      attr = member->getDeclContext()->getAsDecl()->getAttrs()
+                 .getAttribute<ObjCImplementationAttr>(/*AllowInvalid=*/true);
+    assert(attr && "expected @_objcImplementation on context of member checked "
+                   "by ObjCImplementationChecker");
+    if (attr->isInvalid())
+      return;
+
+    if (auto init = dyn_cast<ConstructorDecl>(afd)) {
+      if (!init->isObjC() && (init->isRequired() ||
+                              !init->isConvenienceInit())) {
+        // Swift-only initializers have to be convenience.
+        diagnose(afd, diag::objc_implementation_init_must_be_convenience,
+                 afd, init->isRequired());
+
+        // Add appropriate fix-it to 'convenience'.
+        if (auto requiredMod = init->getAttrs().getAttribute<RequiredAttr>())
+          diagnose(afd,
+                   diag::objc_implementation_init_turn_required_to_convenience)
+            .fixItReplace(requiredMod->getRange(), "convenience");
+        else
+          diagnose(afd,
+                   diag::objc_implementation_init_turn_designated_to_convenience)
+            .fixItInsert(afd->getAttributeInsertionLoc(/*forModifier=*/true),
+                         "convenience ");
+
+        return;
+      }
+    }
+
+    // Emit a vague fallback diagnostic.
+    diagnose(afd, diag::objc_implementation_member_requires_vtable, afd);
+  }
+
   void addRequirements(IterableDeclContext *idc) {
     assert(idc->getDecl()->hasClangNode());
     for (Decl *_member : idc->getMembers()) {
@@ -2992,8 +3050,12 @@ class ObjCImplementationChecker {
 
       // Skip non-member implementations.
       // FIXME: Should we consider them if they were only rejected for privacy?
-      if (!member->isObjCMemberImplementation())
+      if (!member->isObjCMemberImplementation()) {
+        // No member of an `@_objcImplementation` extension should need a vtable
+        // entry.
+        diagnoseVTableUse(member);
         continue;
+      }
 
       // `getExplicitObjCName()` is O(N) and would otherwise be used repeatedly
       // in `matchRequirementsAtThreshold()`, so just precompute it.
@@ -3377,6 +3439,8 @@ class ObjCImplementationChecker {
     case MatchOutcome::Match:
     case MatchOutcome::MatchWithExplicitObjCName:
       // Successful outcomes!
+      // If this member will require a vtable entry, diagnose that now.
+      diagnoseVTableUse(cand);
       return;
 
     case MatchOutcome::WrongImplicitObjCName:
@@ -3542,14 +3606,26 @@ class ObjCImplementationChecker {
       diagnose(cand, diag::member_of_objc_implementation_not_objc_or_final,
                cand, cand->getDeclContext()->getSelfClassDecl());
 
-      if (canBeRepresentedInObjC(cand))
-        diagnose(cand, diag::fixit_add_private_for_objc_implementation,
-                 cand->getDescriptiveKind())
-            .fixItInsert(cand->getAttributeInsertionLoc(true), "private ");
+      if (canBeRepresentedInObjC(cand)) {
+        auto diagnostic =
+            diagnose(cand, diag::fixit_add_private_for_objc_implementation,
+                     cand->getDescriptiveKind());
+        if (auto modifier = cand->getAttrs().getAttribute<AccessControlAttr>())
+          diagnostic.fixItReplace(modifier->getRange(), "private");
+        else
+          diagnostic.fixItInsert(cand->getAttributeInsertionLoc(true),
+                                 "private ");
+      }
 
-      diagnose(cand, diag::fixit_add_final_for_objc_implementation,
-               cand->getDescriptiveKind())
-          .fixItInsert(cand->getAttributeInsertionLoc(true), "final ");
+      // Initializers can't be 'final', but they can be '@nonobjc'
+      if (isa<ConstructorDecl>(cand))
+        diagnose(cand, diag::fixit_add_nonobjc_for_objc_implementation,
+                 cand->getDescriptiveKind())
+            .fixItInsert(cand->getAttributeInsertionLoc(false), "@nonobjc ");
+      else
+        diagnose(cand, diag::fixit_add_final_for_objc_implementation,
+                 cand->getDescriptiveKind())
+            .fixItInsert(cand->getAttributeInsertionLoc(true), "final ");
     }
   }
 };

lib/Sema/TypeCheckObjC.h

@@ -71,6 +71,8 @@ class ObjCReason {
     ExplicitlyGKInspectable,
     /// Is it a member of an @objc extension of a class.
     MemberOfObjCExtension,
+    /// Is it a member of an \@\_objcImplementation extension.
+    MemberOfObjCImplementationExtension,
     /// Has an explicit '@objc' attribute added by an access note, rather than
     /// written in source code.
     ExplicitlyObjCByAccessNote,
@@ -108,6 +110,7 @@ class ObjCReason {
     case ExplicitlyNSManaged:
     case ExplicitlyIBInspectable:
     case ExplicitlyGKInspectable:
+    case MemberOfObjCImplementationExtension:
     case ExplicitlyObjCByAccessNote:
       return true;
 

test/decl/ext/objc_implementation.swift

@@ -114,7 +114,7 @@ protocol EmptySwiftProto {}
 
   internal var propertyNotFromHeader1: CInt
   // expected-warning@-1 {{property 'propertyNotFromHeader1' does not match any property declared in the headers for 'ObjCClass'; did you use the property's Swift name?; this will become an error before '@_objcImplementation' is stabilized}}
-  // expected-note@-2 {{add 'private' or 'fileprivate' to define an Objective-C-compatible property not declared in the header}} {{3-3=private }}
+  // expected-note@-2 {{add 'private' or 'fileprivate' to define an Objective-C-compatible property not declared in the header}} {{3-11=private}}
   // expected-note@-3 {{add 'final' to define a Swift property that cannot be overridden}} {{3-3=final }}
 
   @objc private var propertyNotFromHeader2: CInt
@@ -183,6 +183,38 @@ protocol EmptySwiftProto {}
   class func instanceMethod2(_: CInt) {
     // expected-warning@-1 {{class method 'instanceMethod2' does not match instance method declared in header; this will become an error before '@_objcImplementation' is stabilized}} {{3-9=}}
   }
+
+  public init(notFromHeader1: CInt) {
+    // expected-warning@-1 {{initializer 'init(notFromHeader1:)' does not match any initializer declared in the headers for 'ObjCClass'; did you use the initializer's Swift name?}}
+    // expected-note@-2 {{add 'private' or 'fileprivate' to define an Objective-C-compatible initializer not declared in the header}} {{3-9=private}}
+    // expected-note@-3 {{add '@nonobjc' to define a Swift-only initializer}} {{3-3=@nonobjc }}
+  }
+
+  public required init(notFromHeader2: CInt) {
+    // expected-warning@-1 {{initializer 'init(notFromHeader2:)' does not match any initializer declared in the headers for 'ObjCClass'; did you use the initializer's Swift name?}}
+    // expected-note@-2 {{add 'private' or 'fileprivate' to define an Objective-C-compatible initializer not declared in the header}} {{3-9=private}}
+    // expected-note@-3 {{add '@nonobjc' to define a Swift-only initializer}} {{3-3=@nonobjc }}
+  }
+
+  public convenience init(notFromHeader3: CInt) {
+    // expected-warning@-1 {{initializer 'init(notFromHeader3:)' does not match any initializer declared in the headers for 'ObjCClass'; did you use the initializer's Swift name?}}
+    // expected-note@-2 {{add 'private' or 'fileprivate' to define an Objective-C-compatible initializer not declared in the header}} {{3-9=private}}
+    // expected-note@-3 {{add '@nonobjc' to define a Swift-only initializer}} {{3-3=@nonobjc }}
+  }
+
+  @nonobjc public init(notFromHeader4: CInt) {
+    // expected-warning@-1 {{initializer 'init(notFromHeader4:)' is not valid in an '@_objcImplementation' extension because Objective-C subclasses must be able to override designated initializers}}
+    // expected-note@-2 {{add 'convenience' keyword to make this a convenience initializer}} {{12-12=convenience }}
+  }
+
+  @nonobjc public required init(notFromHeader5: CInt) {
+    // expected-warning@-1 {{initializer 'init(notFromHeader5:)' is not valid in an '@_objcImplementation' extension because Objective-C subclasses must be able to override required initializers}}
+    // expected-note@-2 {{replace 'required' keyword with 'convenience' to make this a convenience initializer}} {{19-27=convenience}}
+  }
+
+  @nonobjc public convenience init(notFromHeader6: CInt) {
+    // OK
+  }
 }
 
 @_objcImplementation(PresentAdditions) extension ObjCClass {
@@ -382,8 +414,8 @@ protocol EmptySwiftProto {}
   func nullableResult() -> Any { fatalError() } // expected-warning {{instance method 'nullableResult()' of type '() -> Any' does not match type '() -> Any?' declared by the header}}
   func nullableArgument(_: Any) {} // expected-warning {{instance method 'nullableArgument' of type '(Any) -> ()' does not match type '(Any?) -> Void' declared by the header}}
 
-  func nonPointerResult() -> CInt! { fatalError() } // expected-error{{method cannot be implicitly @objc because its result type cannot be represented in Objective-C}}
-  func nonPointerArgument(_: CInt!) {} // expected-error {{method cannot be implicitly @objc because the type of the parameter cannot be represented in Objective-C}}
+  func nonPointerResult() -> CInt! { fatalError() } // expected-error{{method cannot be in an @_objcImplementation extension of a class (without final or @nonobjc) because its result type cannot be represented in Objective-C}}
+  func nonPointerArgument(_: CInt!) {} // expected-error {{method cannot be in an @_objcImplementation extension of a class (without final or @nonobjc) because the type of the parameter cannot be represented in Objective-C}}
 }
 
 @_objcImplementation extension ObjCImplSubclass {