Re-allow @objc final

Treating this as forbidden was incorrect; an `@objc final` method is simply one that isn’t a member implementation but does have an ObjC entry point.

Formalize and centralize the definition of what a member implementation is, tweak it so that it’s basically “non-final and internal or greater”, and permit `@objc final`. Also remove the inference of `final` on `let`s in @_objcImpl extensions.

Author: beccadax

include/swift/AST/Decl.h

@@ -2379,6 +2379,10 @@ class ValueDecl : public Decl {
   /// Asserts if this is not a member of a protocol.
   bool isProtocolRequirement() const;
 
+  /// Return true if this is a member implementation for an \c @_objcImplementation
+  /// extension.
+  bool isObjCMemberImplementation() const;
+
   void setUserAccessible(bool Accessible) {
     Bits.ValueDecl.IsUserAccessible = Accessible;
   }

include/swift/AST/DiagnosticsSema.def

@@ -1511,10 +1511,6 @@ ERROR(attr_objc_implementation_category_not_found,none,
 NOTE(attr_objc_implementation_fixit_remove_category_name,none,
      "remove arguments to implement the main '@interface' for this class",
      ())
-ERROR(attr_objc_implementation_no_objc_final,none,
-      "%0 %1 cannot be 'final' because Objective-C subclasses of %2 can "
-      "override it",
-      (DescriptiveDeclKind, ValueDecl *, ValueDecl *))
 
 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 "

lib/AST/Decl.cpp

@@ -3739,18 +3739,23 @@ static bool checkAccessUsingAccessScopes(const DeclContext *useDC,
 /// Checks if \p VD is an ObjC member implementation:
 ///
 /// \li It's in an \c \@_objcImplementation extension
-/// \li It's \c \@objc
+/// \li It's not explicitly \c final
 /// \li Its access level is not \c private or \c fileprivate
 static bool
 isObjCMemberImplementation(const ValueDecl *VD,
                            llvm::function_ref<AccessLevel()> getAccessLevel) {
   if (auto ED = dyn_cast<ExtensionDecl>(VD->getDeclContext()))
-    if (ED->isObjCImplementation())
-      return VD->isObjC() && getAccessLevel() >= AccessLevel::Internal;
+    if (ED->isObjCImplementation() && !isa<TypeDecl>(VD))
+      return !VD->isFinal() && getAccessLevel() >= AccessLevel::Internal;
 
   return false;
 }
 
+bool ValueDecl::isObjCMemberImplementation() const {
+  return ::isObjCMemberImplementation(
+              this, [&]() { return this->getFormalAccess(); });
+}
+
 /// Checks if \p VD may be used from \p useDC, taking \@testable and \@_spi
 /// imports into account.
 ///

lib/Sema/TypeCheckDecl.cpp

@@ -811,22 +811,6 @@ IsFinalRequest::evaluate(Evaluator &evaluator, ValueDecl *decl) const {
   if (!cls)
     return false;
 
-  // Objective-C doesn't have a way to prevent overriding, so if this member
-  // is accessible from ObjC and it's possible to subclass its parent from ObjC,
-  // `final` doesn't make sense even when inferred.
-  //
-  // FIXME: This is technically true iff `!cls->hasKnownSwiftImplementation()`,
-  //        but modeling that would be source-breaking, so instead we only
-  //        enforce it in `@_objcImplementation extension`s.
-  if (auto ext = dyn_cast<ExtensionDecl>(decl->getDeclContext()))
-    if (ext->isObjCImplementation() && decl->isObjC()) {
-      if (explicitFinalAttr)
-        diagnoseAndRemoveAttr(decl, explicitFinalAttr,
-                              diag::attr_objc_implementation_no_objc_final,
-                              decl->getDescriptiveKind(), decl, cls);
-      return false;
-    }
-
   switch (decl->getKind()) {
     case DeclKind::Var: {
       // Properties are final if they are declared 'static' or a 'let'
@@ -854,6 +838,12 @@ IsFinalRequest::evaluate(Evaluator &evaluator, ValueDecl *decl) const {
           return true;
 
         if (VD->isLet()) {
+          // If this `let` is in an `@_objcImplementation extension`, don't
+          // infer `final` unless it is written explicitly.
+          auto ed = dyn_cast<ExtensionDecl>(VD->getDeclContext());
+          if (!explicitFinalAttr && ed && ed->isObjCImplementation())
+            return false;
+
           if (VD->getFormalAccess() == AccessLevel::Open) {
             auto &context = decl->getASTContext();
             auto diagID = diag::implicitly_final_cannot_be_open;
@@ -974,6 +964,11 @@ 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
@@ -1005,12 +1000,6 @@ IsDynamicRequest::evaluate(Evaluator &evaluator, ValueDecl *decl) const {
   if (isa<ExtensionDecl>(dc) && dc->getSelfClassDecl())
     return true;
 
-  // @objc declarations in @_objcImplementation extensions are implicitly
-  // dynamic.
-  if (auto ED = dyn_cast_or_null<ExtensionDecl>(dc->getAsDecl()))
-    if (ED->isObjCImplementation())
-      return true;
-
   // If any of the declarations overridden by this declaration are dynamic
   // or were imported from Objective-C, this declaration is dynamic.
   // Don't do this if the declaration is not exposed to Objective-C; that's

lib/Sema/TypeCheckDeclObjC.cpp

@@ -1881,26 +1881,6 @@ static ObjCSelector inferObjCName(ValueDecl *decl) {
   return *decl->getObjCRuntimeName(true);
 }
 
-static bool isObjCImplementation(AbstractFunctionDecl *method,
-                                 ObjCSelector methodName) {
-  auto ext = dyn_cast<ExtensionDecl>(method->getDeclContext());
-  if (!ext || !ext->isObjCImplementation())
-    return false;
-
-  auto contextInterface =
-      dyn_cast<IterableDeclContext>(ext->getImplementedObjCDecl());
-
-  for (auto otherMember : contextInterface->getMembers()) {
-    auto otherMethod = dyn_cast<AbstractFunctionDecl>(otherMember);
-    if (!otherMethod) continue;
-
-    if (otherMethod->getObjCSelector() == methodName)
-      return true;
-  }
-
-  return false;
-}
-
 /// Mark the given declaration as being Objective-C compatible (or
 /// not) as appropriate.
 ///
@@ -2062,7 +2042,7 @@ void markAsObjC(ValueDecl *D, ObjCReason reason,
 
     // Record the method in the type, if it's a member of one.
     if (auto tyDecl = D->getDeclContext()->getSelfNominalTypeDecl()) {
-      if (!isObjCImplementation(method, selector))
+      if (!method->isObjCMemberImplementation())
         tyDecl->recordObjCMethod(method, selector);
     }
 

lib/Sema/TypeCheckDeclPrimary.cpp

@@ -1230,8 +1230,11 @@ static void checkObjCImplementationMemberAvoidsVTable(ValueDecl *VD) {
          !ED->getSelfClassDecl()->hasKnownSwiftImplementation() &&
          "@_objcImplementation on non-class or Swift class?");
 
-  if (VD->isSemanticallyFinal() || VD->isObjC()) {
-    assert(isa<DestructorDecl>(VD) || !VD->isObjC() || VD->isDynamic() &&
+  if (!VD->isObjCMemberImplementation())
+    return;
+
+  if (VD->isObjC()) {
+    assert(isa<DestructorDecl>(VD) || VD->isDynamic() &&
            "@objc decls in @_objcImplementations should be dynamic!");
     return;
   }

test/decl/ext/objc_implementation.swift

@@ -122,7 +122,7 @@
   // OK, provides a Swift-only stored property
 
   @objc final var propertyNotFromHeader5: CInt
-  // expected-error@-1 {{property 'propertyNotFromHeader5' cannot be 'final' because Objective-C subclasses of 'ObjCClass' can override it}} {{9-15=}}
+  // OK, @objc final is weird but supported, not a member impl
 }
 
 // FIXME: Should complain about categoryMethodFromHeader4:
@@ -227,7 +227,6 @@ func usesAreNotAmbiguous(obj: ObjCClass) {
 
   obj.methodNot(fromHeader1: 1)
   obj.methodNot(fromHeader2: 2)
-  obj.methodNot(fromHeader3: 3)
 
   obj.categoryMethod(fromHeader1: 1)
   obj.categoryMethod(fromHeader2: 2)
@@ -236,5 +235,4 @@ func usesAreNotAmbiguous(obj: ObjCClass) {
 
   obj.categoryMethodNot(fromHeader1: 1)
   obj.categoryMethodNot(fromHeader2: 2)
-  obj.categoryMethodNot(fromHeader3: 3)
 }