Mimic old objcImpl behavior for early adopters

Before the update to support the new syntax, the decl checker treated private and fileprivate members of objcImpl extensions as non-@objc by default. But SE-0436 specified that private and fileprivate members should be implicitly @objc unless opted out, so when support for the final syntax was added, this behavior was changed retroactively.

Unfortunately, we’ve found that some early adopters depended on the old behavior. Restore some logic deleted by swiftlang#73309 for early adopter syntax *only*, with a warning telling the developer to put `final` or `@nonobjc` on the declaration if they want to preserve the behavior after updating it.

Also tweaks the ObjCImplementationChecker to ensure this logic will actually be run in time for it to suppress the “upgrade to @objc @implementation” warning, and corrects a couple of regressions arising from that change.

Fixes rdar://135747897.

Author: beccadax

include/swift/AST/DiagnosticsSema.def

@@ -1814,6 +1814,12 @@ ERROR(attr_implementation_category_goes_on_objc_attr,none,
       "'@implementation'",
       ())
 
+WARNING(objc_implementation_lock_down_non_member_impl_behavior,none,
+        "%kind0 will become implicitly '@objc' after adopting '@objc "
+        "@implementation'; add '%select{@nonobjc|final}1' to keep the current "
+        "behavior",
+        (const ValueDecl *, bool))
+
 ERROR(member_of_objc_implementation_not_objc_or_final,none,
       "%kind0 does not match any %kindonly0 declared in the headers for %1; "
       "did you use the %kindonly0's Swift name?",

lib/Sema/TypeCheckDeclObjC.cpp

@@ -1493,6 +1493,19 @@ static std::optional<ObjCReason> shouldMarkAsObjC(const ValueDecl *VD,
     return ObjCReason(ObjCReason::MemberOfObjCProtocol);
   }
 
+  // Emulating old logic:
+  // 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);
+
+    // ...for early adopters only. (There's new logic for non-early adopters
+    // below.)
+    if (attr->isEarlyAdopter())
+      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>() ||
@@ -1501,6 +1514,10 @@ static std::optional<ObjCReason> shouldMarkAsObjC(const ValueDecl *VD,
         .hasAttribute<NonObjCAttr>()))
     return std::nullopt;
 
+  // Set to non-null to indicate a difference between early-adopter and final
+  // objcImpl behavior.
+  ObjCImplementationAttr *attrToWarnOfObjCImplBehaviorChange = nullptr;
+
   if (isMemberOfObjCImplementationExtension(VD)) {
     // A `final` member of an @objc @implementation extension is not @objc.
     if (VD->isFinal())
@@ -1510,7 +1527,14 @@ static std::optional<ObjCReason> shouldMarkAsObjC(const ValueDecl *VD,
       auto ext = VD->getDeclContext()->getAsDecl();
       auto attr = ext->getAttrs()
                    .getAttribute<ObjCImplementationAttr>(/*AllowInvalid=*/true);
-      return ObjCReason(ObjCReason::MemberOfObjCImplementationExtension, attr);
+
+      if (!attr->isEarlyAdopter())
+        return ObjCReason(ObjCReason::MemberOfObjCImplementationExtension,attr);
+
+      // If we got here, that means the new syntax might behave differently from
+      // the old one, unless one of the remaining `return ObjCReason`s below
+      // would have made it isObjC() anyway.
+      attrToWarnOfObjCImplBehaviorChange = attr;
     }
   }
   else if (isMemberOfObjCClassExtension(VD) &&
@@ -1532,6 +1556,19 @@ static std::optional<ObjCReason> shouldMarkAsObjC(const ValueDecl *VD,
       return ObjCReason::witnessToObjC(requirements.front());
   }
 
+  if (attrToWarnOfObjCImplBehaviorChange) {
+    bool suggestFinal = !isa<ConstructorDecl>(VD);
+    VD->diagnose(diag::objc_implementation_lock_down_non_member_impl_behavior,
+                 VD, suggestFinal)
+      .fixItInsert(VD->getAttributeInsertionLoc(suggestFinal),
+                   suggestFinal ? "final " : "@nonobjc ");
+
+    VD->diagnose(diag::make_decl_objc, VD->getDescriptiveKind())
+      .fixItInsert(VD->getAttributeInsertionLoc(/*modifier=*/false), "@objc ");
+
+    attrToWarnOfObjCImplBehaviorChange->setHasInvalidImplicitLangAttrs();
+  }
+
   return std::nullopt;
 }
 
@@ -2412,6 +2449,11 @@ bool swift::diagnoseUnintendedObjCMethodOverrides(SourceFile &sf) {
         continue;
     }
 
+    // Skip declarations that are member implementations;
+    // ObjCImplmentationChecker will ensure they match a redeclared method.
+    if (method->isObjCMemberImplementation())
+      continue;
+
     auto classDecl = method->getDeclContext()->getSelfClassDecl();
     if (!classDecl)
       continue; // error-recovery path, only
@@ -3263,6 +3305,9 @@ class ObjCImplementationChecker {
         continue;
 
       if (auto VD = dyn_cast<ValueDecl>(member)) {
+        // Force isObjC() to make sure hasInvalidImplicitLangAttrs() is set.
+        (void)VD->isObjC();
+
         // Skip non-member implementations.
         // FIXME: Should we consider them if only rejected for access level?
         if (!VD->isObjCMemberImplementation()) {
@@ -3283,7 +3328,8 @@ class ObjCImplementationChecker {
       return ObjCSelector(VD->getASTContext(), 0, { ident });
     }
     if (auto objcAttr = VD->getAttrs().getAttribute<ObjCAttr>())
-      return objcAttr->getName().value_or(ObjCSelector());
+      if (!objcAttr->isNameImplicit())
+        return objcAttr->getName().value_or(ObjCSelector());
     return ObjCSelector();
   }
 
@@ -3870,7 +3916,8 @@ class ObjCImplementationChecker {
   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())
+    if (hasDiagnosed || !getAttr()->isEarlyAdopter()
+          || getAttr()->hasInvalidImplicitLangAttrs())
       return;
 
     // Only encourage adoption if the corresponding language feature is enabled.

test/SILOptimizer/let_properties_opts_objc.swift

@@ -18,7 +18,7 @@ public func testObjcInterface(_ x: ObjcInterface) -> Int {
 
 // Test that private @objc constants aren't optimized, but instead continue to pass through ObjC message dispatch.
 @_objcImplementation extension ObjcInterfaceConstInit {
-  private let constant: Int = 0
+  @objc private let constant: Int = 0
 
   // CHECK-LABEL: sil hidden @$sSo22ObjcInterfaceConstInitC4testE0E15PrivateConstantSiyF : $@convention(method) (@guaranteed ObjcInterfaceConstInit) -> Int {
   // CHECK: objc_method %0 : $ObjcInterfaceConstInit, #ObjcInterfaceConstInit.constant!getter.foreign

test/Serialization/objc_implementation.swift

@@ -18,7 +18,8 @@ import objc_implementation
 @_objcImplementation extension ObjCImpl {
   // CHECK-DAG: func cannotBeObjCMethod(_ value: Int?)
   private func cannotBeObjCMethod(_ value: Int?) {}
-  // expected-warning@-1 {{method cannot be in an @objc @implementation extension of a class (without final or @nonobjc) because the type of the parameter cannot be represented in Objective-C}}
+  // expected-warning@-1 {{instance method 'cannotBeObjCMethod' will become implicitly '@objc' after adopting '@objc @implementation'; add 'final' to keep the current behavior}} {{3-3=final }}
+  // expected-note@-2 {{add '@objc' to expose this instance method to Objective-C}} {{3-3=@objc }}
 
   // CHECK-DAG: @objc func goodMethod()
   @objc public func goodMethod() {}

test/decl/ext/objc_implementation_early_adopter.swift

@@ -443,8 +443,8 @@ protocol EmptySwiftProto {}
   }
 
   private func privateNonObjCMethod(_: EmptySwiftProto) {
-    // expected-warning@-1 {{method cannot be in an @objc @implementation extension of a class (without final or @nonobjc) because the type of the parameter cannot be represented in Objective-C}}
-    // expected-note@-2 {{protocol-constrained type containing protocol 'EmptySwiftProto' cannot be represented in Objective-C}}
+    // expected-warning@-1 {{instance method 'privateNonObjCMethod' will become implicitly '@objc' after adopting '@objc @implementation'; add 'final' to keep the current behavior}} {{3-3=final }}
+    // expected-note@-2 {{add '@objc' to expose this instance method to Objective-C}} {{3-3=@objc }}
   }
 }