Correct and improve objcImpl member matching

• `@objc` is now inferred on non-`final` members of @objcImplementation extensions
• Diagnostics now suggest adding `private` to ObjC helper members, not `@objc`, in line with currently proposed behavior
• Better diagnostic for members implemented in the wrong extension

Part of rdar://103150189.

Author: beccadax

include/swift/AST/DiagnosticsSema.def

@@ -1587,14 +1587,20 @@ ERROR(member_of_objc_implementation_not_objc_or_final,none,
       "%0 %1 does not match any %0 declared in the headers for %2; did you use "
       "the %0's Swift name?",
       (DescriptiveDeclKind, ValueDecl *, ValueDecl *))
-NOTE(fixit_add_objc_for_objc_implementation,none,
-     "add '@objc' to define an Objective-C-compatible %0 not declared in "
-     "the header",
+NOTE(fixit_add_private_for_objc_implementation,none,
+     "add 'private' or 'fileprivate' to define an Objective-C-compatible %0 "
+     "not declared in the header",
      (DescriptiveDeclKind))
 NOTE(fixit_add_final_for_objc_implementation,none,
      "add 'final' to define a Swift %0 that cannot be overridden",
      (DescriptiveDeclKind))
 
+ERROR(objc_implementation_wrong_category,none,
+      "%0 %1 should be implemented in extension for "
+      "%select{main class interface|category %2}2, not "
+      "%select{main class interface|category %3}3",
+      (DescriptiveDeclKind, ValueDecl *, Identifier, Identifier))
+
 ERROR(cdecl_not_at_top_level,none,
       "@_cdecl can only be applied to global functions", ())
 ERROR(cdecl_empty_name,none,

lib/Sema/TypeCheckDeclObjC.cpp

@@ -1451,6 +1451,11 @@ Optional<ObjCReason> shouldMarkAsObjC(const ValueDecl *VD, bool allowImplicit) {
     return ObjCReason(ObjCReason::MemberOfObjCProtocol);
   }
 
+  // A member implementation of an @objcImplementation extension is @objc.
+  if (VD->isObjCMemberImplementation())
+    // FIXME: New ObjCReason::Kind?
+    return ObjCReason(ObjCReason::ImplicitlyObjC);
+
   // A @nonobjc is not @objc, even if it is an override of an @objc, so check
   // for @nonobjc first.
   if (VD->getAttrs().hasAttribute<NonObjCAttr>() ||

lib/Sema/TypeCheckDeclPrimary.cpp

@@ -1243,24 +1243,96 @@ static void checkObjCImplementationMemberAvoidsVTable(ValueDecl *VD) {
   if (!VD->isObjCMemberImplementation())
     return;
 
-  if (VD->isObjC()) {
-    assert(isa<DestructorDecl>(VD) || VD->isDynamic() &&
-           "@objc decls in @_objcImplementations should be dynamic!");
-    return;
-  }
+  auto isNotRelevantInterfaceMember = [&](ValueDecl *member) -> bool {
+    return !member->hasClangNode()
+             || VD->isInstanceMember() != member->isInstanceMember();
+  };
+  assert(!VD->hasClangNode() &&
+            "we assumed VD could not be in sameNameMembers!");
 
   auto &diags = VD->getASTContext().Diags;
-  diags.diagnose(VD, diag::member_of_objc_implementation_not_objc_or_final,
-                 VD->getDescriptiveKind(), VD, ED->getExtendedNominal());
+  auto extendedType = ED->getSelfClassDecl();
+  auto objcAttr = VD->getSemanticAttrs().getAttribute<ObjCAttr>();
+  TinyPtrVector<ValueDecl *> sameNameMembers;
+
+  // Look up by @objc(<selector>), if present
+  if (objcAttr && objcAttr->getName()) {
+    auto selectorLookup = extendedType->lookupDirect(*objcAttr->getName(),
+                                                     VD->isInstanceMember());
+    // Loop upcasts from AbstractFunctionDecl to ValueDecl
+    for (ValueDecl *member : selectorLookup) {
+      if (auto accessor = dyn_cast<AccessorDecl>(member))
+        member = accessor->getStorage();
+
+      if (!isNotRelevantInterfaceMember(member)
+            && !llvm::is_contained(sameNameMembers, member))
+        sameNameMembers.push_back(member);
+    }
+  }
+
+  // Fallback: Look up by Swift name
+  // If we tried the selector lookup but it failed, this will find near-matches
+  // we can suggest.
+  if (sameNameMembers.empty()) {
+    sameNameMembers = extendedType->lookupDirect(VD->getName());
+  }
+
+  // Filter matches down to only imported members of the same instance-ness.
+  if (!sameNameMembers.empty()) {
+    sameNameMembers.erase(llvm::remove_if(sameNameMembers,
+                                          isNotRelevantInterfaceMember),
+                          sameNameMembers.end());
+  }
+
+  // If we didn't find any methods, diagnose.
+  if (sameNameMembers.empty()) {
+    diags.diagnose(VD, diag::member_of_objc_implementation_not_objc_or_final,
+                   VD->getDescriptiveKind(), VD, ED->getExtendedNominal());
+
+    if (canBeRepresentedInObjC(VD))
+      diags.diagnose(VD, diag::fixit_add_private_for_objc_implementation,
+                     VD->getDescriptiveKind())
+          .fixItInsert(VD->getAttributeInsertionLoc(true), "private ");
 
-  if (canBeRepresentedInObjC(VD))
-    diags.diagnose(VD, diag::fixit_add_objc_for_objc_implementation,
+    diags.diagnose(VD, diag::fixit_add_final_for_objc_implementation,
                    VD->getDescriptiveKind())
-        .fixItInsert(VD->getAttributeInsertionLoc(false), "@objc ");
+        .fixItInsert(VD->getAttributeInsertionLoc(true), "final ");
+    return;
+  }
+
+  // FIXME: Diagnose multiple matches?
+
+  // If we only found methods in the wrong extensions, diagnose.
+  auto expectedInterfaceDC = ED->getImplementedObjCContext();
+  auto selectedMethodIter = llvm::find_if(sameNameMembers,
+                                          [&](ValueDecl *member) {
+    return expectedInterfaceDC == member->getDeclContext();
+  });
+  if (selectedMethodIter == sameNameMembers.end()) {
+    Identifier expectedInterfaceName;
+    if (auto ED = dyn_cast<ExtensionDecl>(expectedInterfaceDC)) {
+      expectedInterfaceName = ED->getObjCCategoryName();
+    }
+
+    // Arbitarily assume sameNameMethods.front() is the correct declaration.
+    auto actualInterfaceDC = sameNameMembers.front()->getDeclContext();
+    Identifier actualInterfaceName;
+    if (auto ED = dyn_cast<ExtensionDecl>(actualInterfaceDC)) {
+      actualInterfaceName = ED->getObjCCategoryName();
+    }
+
+    diags.diagnose(VD, diag::objc_implementation_wrong_category,
+                   VD->getDescriptiveKind(), VD,
+                   actualInterfaceName, expectedInterfaceName);
+
+    // FIXME: Add fix-it to move implementation
+
+    return;
+  }
 
-  diags.diagnose(VD, diag::fixit_add_final_for_objc_implementation,
-                 VD->getDescriptiveKind())
-      .fixItInsert(VD->getAttributeInsertionLoc(true), "final ");
+  assert(VD->isObjC());
+  assert(isa<DestructorDecl>(VD) || VD->isDynamic() &&
+         "@objc decls in @_objcImplementations should be dynamic!");
 }
 
 /// Build a default initializer string for the given pattern.

test/decl/ext/objc_implementation.swift

@@ -6,21 +6,15 @@
   // expected-note@-1 {{previously implemented by extension here}}
 
   func method(fromHeader1: CInt) {
-    // FIXME: OK, provides an implementation for the header's method.
-    // expected-error@-2 {{instance method 'method(fromHeader1:)' does not match any instance method declared in the headers for 'ObjCClass'; did you use the instance method's Swift name?}}
-    // expected-note@-3 {{add '@objc' to define an Objective-C-compatible instance method not declared in the header}} {{3-3=@objc }}
-    // expected-note@-4 {{add 'final' to define a Swift instance method that cannot be overridden}} {{3-3=final }}
+    // OK, provides an implementation for the header's method.
   }
 
   @objc func method(fromHeader2: CInt) {
     // OK, provides an implementation for the header's method.
   }
 
   func categoryMethod(fromHeader3: CInt) {
-    // FIXME: Should complain about the wrong category
-    // expected-error@-2 {{instance method 'categoryMethod(fromHeader3:)' does not match any instance method declared in the headers for 'ObjCClass'; did you use the instance method's Swift name?}}
-    // expected-note@-3 {{add '@objc' to define an Objective-C-compatible instance method not declared in the header}} {{3-3=@objc }}
-    // expected-note@-4 {{add 'final' to define a Swift instance method that cannot be overridden}} {{3-3=final }}
+    // expected-error@-1 {{instance method 'categoryMethod(fromHeader3:)' should be implemented in extension for category 'PresentAdditions', not main class interface}}
   }
 
   @objc fileprivate func methodNot(fromHeader1: CInt) {
@@ -33,24 +27,18 @@
 
   func methodNot(fromHeader3: CInt) {
     // expected-error@-1 {{instance method 'methodNot(fromHeader3:)' does not match any instance method declared in the headers for 'ObjCClass'; did you use the instance method's Swift name?}}
-    // expected-note@-2 {{add '@objc' to define an Objective-C-compatible instance method not declared in the header}} {{3-3=@objc }}
+    // expected-note@-2 {{add 'private' or 'fileprivate' to define an Objective-C-compatible instance method not declared in the header}} {{3-3=private }}
     // expected-note@-3 {{add 'final' to define a Swift instance method that cannot be overridden}} {{3-3=final }}
   }
 
   var propertyFromHeader1: CInt
-  // FIXME: OK, provides an implementation with a stored property
-  // expected-error@-2 {{property 'propertyFromHeader1' does not match any property declared in the headers for 'ObjCClass'; did you use the property's Swift name?}}
-  // expected-note@-3 {{add '@objc' to define an Objective-C-compatible property not declared in the header}} {{3-3=@objc }}
-  // expected-note@-4 {{add 'final' to define a Swift property that cannot be overridden}} {{3-3=final }}
+  // OK, provides an implementation with a stored property
 
   @objc var propertyFromHeader2: CInt
   // OK, provides an implementation with a stored property
 
   var propertyFromHeader3: CInt {
-    // FIXME: OK, provides an implementation with a computed property
-    // expected-error@-2 {{property 'propertyFromHeader3' does not match any property declared in the headers for 'ObjCClass'; did you use the property's Swift name?}}
-    // expected-note@-3 {{add '@objc' to define an Objective-C-compatible property not declared in the header}} {{3-3=@objc }}
-    // expected-note@-4 {{add 'final' to define a Swift property that cannot be overridden}} {{3-3=final }}
+    // OK, provides an implementation with a computed property
     get { return 1 }
     set {}
   }
@@ -73,19 +61,13 @@
   // FIXME: Should complain about final not fulfilling the @objc requirement
 
   var readonlyPropertyFromHeader1: CInt
-  // FIXME: OK, provides an implementation with a stored property that's nonpublicly settable
-  // expected-error@-2 {{property 'readonlyPropertyFromHeader1' does not match any property declared in the headers for 'ObjCClass'; did you use the property's Swift name?}}
-  // expected-note@-3 {{add '@objc' to define an Objective-C-compatible property not declared in the header}} {{3-3=@objc }}
-  // expected-note@-4 {{add 'final' to define a Swift property that cannot be overridden}} {{3-3=final }}
+  // OK, provides an implementation with a stored property that's nonpublicly settable
 
   @objc var readonlyPropertyFromHeader2: CInt
   // OK, provides an implementation with a stored property that's nonpublicly settable
 
   var readonlyPropertyFromHeader3: CInt {
-    // FIXME: OK, provides an implementation with a computed property
-    // expected-error@-2 {{property 'readonlyPropertyFromHeader3' does not match any property declared in the headers for 'ObjCClass'; did you use the property's Swift name?}}
-    // expected-note@-3 {{add '@objc' to define an Objective-C-compatible property not declared in the header}} {{3-3=@objc }}
-    // expected-note@-4 {{add 'final' to define a Swift property that cannot be overridden}} {{3-3=final }}
+    // OK, provides an implementation with a computed property
     get { return 1 }
     set {}
   }
@@ -104,15 +86,15 @@
     get { return 1 }
   }
 
-  var propertyNotFromHeader1: CInt
+  internal var propertyNotFromHeader1: CInt
   // expected-error@-1 {{property 'propertyNotFromHeader1' does not match any property declared in the headers for 'ObjCClass'; did you use the property's Swift name?}}
-  // expected-note@-2 {{add '@objc' to define an Objective-C-compatible property not declared in the header}} {{3-3=@objc }}
+  // expected-note@-2 {{add 'private' or 'fileprivate' to define an Objective-C-compatible property not declared in the header}} {{3-3=private }}
   // expected-note@-3 {{add 'final' to define a Swift property that cannot be overridden}} {{3-3=final }}
 
-  @objc var propertyNotFromHeader2: CInt
+  @objc private var propertyNotFromHeader2: CInt
   // OK, provides a nonpublic but ObjC-compatible stored property
 
-  @objc var propertyNotFromHeader3: CInt {
+  @objc fileprivate var propertyNotFromHeader3: CInt {
     // OK, provides a nonpublic but ObjC-compatible computed property
     get { return 1 }
     set {}
@@ -147,25 +129,16 @@
   // expected-note@-1 {{previously implemented by extension here}}
 
   func method(fromHeader3: CInt) {
-    // FIXME: Should complain about wrong category
-    // expected-error@-2 {{instance method 'method(fromHeader3:)' does not match any instance method declared in the headers for 'ObjCClass'; did you use the instance method's Swift name?}}
-    // expected-note@-3 {{add '@objc' to define an Objective-C-compatible instance method not declared in the header}} {{3-3=@objc }}
-    // expected-note@-4 {{add 'final' to define a Swift instance method that cannot be overridden}} {{3-3=final }}
+    // expected-error@-1 {{instance method 'method(fromHeader3:)' should be implemented in extension for main class interface, not category 'PresentAdditions'}}
   }
 
   var propertyFromHeader7: CInt {
-    // FIXME: Should complain about wrong category
-    // expected-error@-2 {{property 'propertyFromHeader7' does not match any property declared in the headers for 'ObjCClass'; did you use the property's Swift name?}}
-    // expected-note@-3 {{add '@objc' to define an Objective-C-compatible property not declared in the header}} {{3-3=@objc }}
-    // expected-note@-4 {{add 'final' to define a Swift property that cannot be overridden}} {{3-3=final }}
+    // expected-error@-1 {{property 'propertyFromHeader7' should be implemented in extension for main class interface, not category 'PresentAdditions'}}
     get { return 1 }
   }
 
   func categoryMethod(fromHeader1: CInt) {
-    // FIXME: OK, provides an implementation for the header's method.
-    // expected-error@-2 {{instance method 'categoryMethod(fromHeader1:)' does not match any instance method declared in the headers for 'ObjCClass'; did you use the instance method's Swift name?}}
-    // expected-note@-3 {{add '@objc' to define an Objective-C-compatible instance method not declared in the header}} {{3-3=@objc }}
-    // expected-note@-4 {{add 'final' to define a Swift instance method that cannot be overridden}} {{3-3=final }}
+    // OK, provides an implementation for the header's method.
   }
 
   @objc func categoryMethod(fromHeader2: CInt) {
@@ -182,24 +155,18 @@
 
   func categoryMethodNot(fromHeader3: CInt) {
     // expected-error@-1 {{instance method 'categoryMethodNot(fromHeader3:)' does not match any instance method declared in the headers for 'ObjCClass'; did you use the instance method's Swift name?}}
-    // expected-note@-2 {{add '@objc' to define an Objective-C-compatible instance method not declared in the header}} {{3-3=@objc }}
+    // expected-note@-2 {{add 'private' or 'fileprivate' to define an Objective-C-compatible instance method not declared in the header}} {{3-3=private }}
     // expected-note@-3 {{add 'final' to define a Swift instance method that cannot be overridden}} {{3-3=final }}
   }
 
   var categoryPropertyFromHeader1: CInt
   // expected-error@-1 {{extensions must not contain stored properties}}
-  // FIXME: expected-error@-2 {{property 'categoryPropertyFromHeader1' does not match any property declared in the headers for 'ObjCClass'; did you use the property's Swift name?}}
-  // FIXME: expected-note@-3 {{add '@objc' to define an Objective-C-compatible property not declared in the header}} {{3-3=@objc }}
-  // FIXME: expected-note@-4 {{add 'final' to define a Swift property that cannot be overridden}} {{3-3=final }}
 
   @objc var categoryPropertyFromHeader2: CInt
   // expected-error@-1 {{extensions must not contain stored properties}}
 
   var categoryPropertyFromHeader3: CInt {
-    // FIXME: OK, provides an implementation with a computed property
-    // expected-error@-2 {{property 'categoryPropertyFromHeader3' does not match any property declared in the headers for 'ObjCClass'; did you use the property's Swift name?}}
-    // expected-note@-3 {{add '@objc' to define an Objective-C-compatible property not declared in the header}} {{3-3=@objc }}
-    // expected-note@-4 {{add 'final' to define a Swift property that cannot be overridden}} {{3-3=final }}
+    // OK, provides an implementation with a computed property
     get { return 1 }
     set {}
   }