Make behavior change detection generic

Automatically detect when shouldMarkAsObjC() will behave differently based on the objcImpl early adopter flag and diagnose it. This works in either direction, although there isn’t anything yet that will emit diag:: objc_implementation_will_become_nonobjc.

NFC except for some minor changes to the wording of notes.

Author: beccadax

include/swift/AST/DiagnosticsSema.def

@@ -1814,11 +1814,16 @@ ERROR(attr_implementation_category_goes_on_objc_attr,none,
       "'@implementation'",
       ())
 
-WARNING(objc_implementation_lock_down_non_member_impl_behavior,none,
+WARNING(objc_implementation_will_become_objc,none,
         "%kind0 will become implicitly '@objc' after adopting '@objc "
         "@implementation'; add '%select{@nonobjc|final}1' to keep the current "
         "behavior",
-        (const ValueDecl *, bool))
+        (const ValueDecl *, /*suggestFinal=*/bool))
+WARNING(objc_implementation_will_become_nonobjc,none,
+        "%kind0 will no longer be implicitly '@objc' after adopting '@objc "
+        "@implementation'; add '@objc' to keep the current "
+        "behavior",
+        (const ValueDecl *))
 
 ERROR(member_of_objc_implementation_not_objc_or_final,none,
       "%kind0 does not match any %kindonly0 declared in the headers for %1; "
@@ -1828,12 +1833,10 @@ 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))
-NOTE(fixit_add_nonobjc_for_objc_implementation,none,
-     "add '@nonobjc' to define a Swift-only %0",
-     (DescriptiveDeclKind))
+NOTE(fixit_add_nonobjc_or_final_for_objc_implementation,none,
+     "add '%select{@nonobjc|final}1' to define a Swift-only %0"
+     "%select{| that cannot be overridden}1",
+     (DescriptiveDeclKind, /*suggestFinal=*/bool))
 
 ERROR(objc_implementation_wrong_category,none,
       "%kind0 should be implemented in extension for "

lib/Sema/TypeCheckDeclObjC.cpp

@@ -1390,8 +1390,9 @@ static std::optional<ObjCReason> shouldMarkClassAsObjC(const ClassDecl *CD) {
 }
 
 /// Figure out if a declaration should be exported to Objective-C.
-static std::optional<ObjCReason> shouldMarkAsObjC(const ValueDecl *VD,
-                                                  bool allowImplicit) {
+static std::optional<ObjCReason>
+shouldMarkAsObjC(const ValueDecl *VD, bool allowImplicit,
+                 ObjCImplementationAttr *implAttr, bool isImplEarlyAdopter) {
   // If Objective-C interoperability is disabled, nothing gets marked as @objc.
   if (!VD->getASTContext().LangOpts.EnableObjCInterop)
     return std::nullopt;
@@ -1495,16 +1496,10 @@ static std::optional<ObjCReason> shouldMarkAsObjC(const ValueDecl *VD,
 
   // 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);
-  }
+  // for early adopters only. (There's new logic for non-early adopters below.)
+  if (isImplEarlyAdopter && VD->isObjCMemberImplementation())
+    return ObjCReason(ObjCReason::MemberOfObjCImplementationExtension,
+                      implAttr);
 
   // A @nonobjc is not @objc, even if it is an override of an @objc, so check
   // for @nonobjc first.
@@ -1514,25 +1509,13 @@ 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 non-`final` member of an @objc @implementation extension is @objc
     // with a special reason.
     if (!VD->isFinal() && canInferImplicitObjC(/*allowAnyAccess*/true)) {
-      auto ext = VD->getDeclContext()->getAsDecl();
-      auto attr = ext->getAttrs()
-                   .getAttribute<ObjCImplementationAttr>(/*AllowInvalid=*/true);
-
-      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;
+      if (!isImplEarlyAdopter)
+        return ObjCReason(ObjCReason::MemberOfObjCImplementationExtension,
+                          implAttr);
     }
   }
   else if (isMemberOfObjCClassExtension(VD) &&
@@ -1554,20 +1537,64 @@ 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)
+  return std::nullopt;
+}
+
+static std::optional<ObjCReason> shouldMarkAsObjC(const ValueDecl *VD,
+                                                  bool allowImplicit) {
+  // The real logic is in the four-argument version above. This wrapper just
+  // detects future `@objc @implementation` behavior changes and warns about
+  // them.
+
+  ObjCImplementationAttr *implAttr = nullptr;
+  if (auto ctxDecl = VD->getDeclContext()->getAsDecl())
+    implAttr = ctxDecl->getAttrs().getAttribute<ObjCImplementationAttr>(
+                                                 /*AllowInvalid=*/true);
+
+  bool isEarlyAdopter = implAttr ? implAttr->isEarlyAdopter() : false;
+
+  auto currentlyObjC = shouldMarkAsObjC(VD, allowImplicit,
+                                        implAttr, isEarlyAdopter);
+
+  // If we weren't an early adopter, there's no differences to detect.
+  if (!isEarlyAdopter)
+    return currentlyObjC;
+
+  // Re-run the logic, but with the early adopter flag forced to `false`.
+  auto eventuallyObjC = shouldMarkAsObjC(VD, allowImplicit,
+                                         implAttr, /*isEarlyAdopter=*/false);
+
+  // Would that have behaved differently? If so, warn about it so the developer
+  // knows they should update their code.
+  if (currentlyObjC.has_value() == eventuallyObjC.has_value())
+    return currentlyObjC;
+
+  bool suggestFinal = !isa<ConstructorDecl>(VD);
+
+  if (eventuallyObjC.has_value()) {
+    ASSERT(!currentlyObjC.has_value());
+
+    VD->diagnose(diag::objc_implementation_will_become_objc, VD, suggestFinal)
       .fixItInsert(VD->getAttributeInsertionLoc(suggestFinal),
                    suggestFinal ? "final " : "@nonobjc ");
 
     VD->diagnose(diag::make_decl_objc, VD->getDescriptiveKind())
-      .fixItInsert(VD->getAttributeInsertionLoc(/*modifier=*/false), "@objc ");
+      .fixItInsert(VD->getAttributeInsertionLoc(false), "@objc ");
+  } else {
+    ASSERT(currentlyObjC.has_value());
+
+    VD->diagnose(diag::objc_implementation_will_become_nonobjc, VD)
+      .fixItInsert(VD->getAttributeInsertionLoc(false), "@objc ");
 
-    attrToWarnOfObjCImplBehaviorChange->setHasInvalidImplicitLangAttrs();
+    VD->diagnose(diag::fixit_add_nonobjc_or_final_for_objc_implementation,
+                 VD->getDescriptiveKind(), suggestFinal)
+      .fixItInsert(VD->getAttributeInsertionLoc(suggestFinal),
+                   suggestFinal ? "final " : "@nonobjc ");
   }
 
-  return std::nullopt;
+  implAttr->setHasInvalidImplicitLangAttrs();
+
+  return currentlyObjC;
 }
 
 /// Determine whether the given type is a C integer type.
@@ -3900,14 +3927,11 @@ class ObjCImplementationChecker {
       }
 
       // 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 ");
+      bool suggestFinal = !isa<ConstructorDecl>(cand);
+      diagnose(cand, diag::fixit_add_nonobjc_or_final_for_objc_implementation,
+               cand->getDescriptiveKind(), suggestFinal)
+          .fixItInsert(cand->getAttributeInsertionLoc(suggestFinal),
+                       suggestFinal ? "final " : "@nonobjc ");
     }
   }
 

test/decl/ext/Inputs/objc_implementation.h

@@ -13,6 +13,8 @@
 - (void)superclassMethod:(int)param;
 @property (assign) int superclassProperty;
 
++ (void)superclassClassMethod;
+
 @end
 
 @protocol ObjCProto
@@ -214,6 +216,10 @@
 @protocol EmptyObjCProto
 @end
 
+@interface ObjCClassWithWeirdSwiftAttributeCombo : ObjCBaseClass
+
+@end
+
 #endif
 
 void CImplFunc1(int param);

test/decl/ext/objc_implementation.swift

@@ -26,7 +26,7 @@ protocol EmptySwiftProto {}
     // FIXME: should emit expected-DISABLED-error@-1 {{instance method 'categoryMethod(fromHeader3:)' should be implemented in extension for category 'PresentAdditions', not main class interface}}
     // FIXME: 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?}}
     // FIXME: expected-note@-3 {{add 'private' or 'fileprivate' to define an Objective-C-compatible instance method not declared in the header}} {{3-3=private }}
-    // FIXME: expected-note@-4 {{add 'final' to define a Swift instance method that cannot be overridden}} {{3-3=final }}
+    // FIXME: expected-note@-4 {{add 'final' to define a Swift-only instance method that cannot be overridden}} {{3-3=final }}
   }
 
   @objc fileprivate func methodNot(fromHeader1: CInt) {
@@ -40,7 +40,7 @@ protocol EmptySwiftProto {}
   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 '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 }}
+    // expected-note@-3 {{add 'final' to define a Swift-only instance method that cannot be overridden}} {{3-3=final }}
   }
 
   var methodFromHeader5: CInt
@@ -116,7 +116,7 @@ protocol EmptySwiftProto {}
   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 '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 }}
+  // expected-note@-3 {{add 'final' to define a Swift-only property that cannot be overridden}} {{3-3=final }}
 
   @objc private var propertyNotFromHeader2: CInt
   // OK, provides a nonpublic but ObjC-compatible stored property
@@ -240,14 +240,14 @@ protocol EmptySwiftProto {}
     // FIXME: should emit expected-DISABLED-error@-1 {{instance method 'method(fromHeader3:)' should be implemented in extension for main class interface, not category 'PresentAdditions'}}
     // FIXME: 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?}}
     // FIXME: expected-note@-3 {{add 'private' or 'fileprivate' to define an Objective-C-compatible instance method not declared in the header}} {{3-3=private }}
-    // FIXME: expected-note@-4 {{add 'final' to define a Swift instance method that cannot be overridden}} {{3-3=final }}
+    // FIXME: expected-note@-4 {{add 'final' to define a Swift-only instance method that cannot be overridden}} {{3-3=final }}
   }
 
   var propertyFromHeader7: CInt {
     // FIXME: should emit expected-DISABLED-error@-1 {{property 'propertyFromHeader7' should be implemented in extension for main class interface, not category 'PresentAdditions'}}
     // FIXME: expected-error@-2 {{property 'propertyFromHeader7' does not match any property declared in the headers for 'ObjCClass'; did you use the property's Swift name?}}
     // FIXME: expected-note@-3 {{add 'private' or 'fileprivate' to define an Objective-C-compatible property not declared in the header}} {{3-3=private }}
-    // FIXME: expected-note@-4 {{add 'final' to define a Swift property that cannot be overridden}} {{3-3=final }}
+    // FIXME: expected-note@-4 {{add 'final' to define a Swift-only property that cannot be overridden}} {{3-3=final }}
     get { return 1 }
   }
 
@@ -270,7 +270,7 @@ protocol EmptySwiftProto {}
   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 '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 }}
+    // expected-note@-3 {{add 'final' to define a Swift-only instance method that cannot be overridden}} {{3-3=final }}
   }
 
   var categoryPropertyFromHeader1: CInt

test/decl/ext/objc_implementation_early_adopter.swift

@@ -26,7 +26,7 @@ protocol EmptySwiftProto {}
     // FIXME: should emit expected-DISABLED-error@-1 {{instance method 'categoryMethod(fromHeader3:)' should be implemented in extension for category 'PresentAdditions', not main class interface}}
     // FIXME: expected-warning@-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?; this will become an error after adopting '@implementation'}}
     // FIXME: expected-note@-3 {{add 'private' or 'fileprivate' to define an Objective-C-compatible instance method not declared in the header}} {{3-3=private }}
-    // FIXME: expected-note@-4 {{add 'final' to define a Swift instance method that cannot be overridden}} {{3-3=final }}
+    // FIXME: expected-note@-4 {{add 'final' to define a Swift-only instance method that cannot be overridden}} {{3-3=final }}
   }
 
   @objc fileprivate func methodNot(fromHeader1: CInt) {
@@ -40,7 +40,7 @@ protocol EmptySwiftProto {}
   func methodNot(fromHeader3: CInt) {
     // expected-warning@-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?; this will become an error after adopting '@implementation'}}
     // 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 }}
+    // expected-note@-3 {{add 'final' to define a Swift-only instance method that cannot be overridden}} {{3-3=final }}
   }
 
   var methodFromHeader5: CInt
@@ -116,7 +116,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 after adopting '@implementation'}}
   // 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 }}
+  // expected-note@-3 {{add 'final' to define a Swift-only property that cannot be overridden}} {{3-3=final }}
 
   @objc private var propertyNotFromHeader2: CInt
   // OK, provides a nonpublic but ObjC-compatible stored property
@@ -240,14 +240,14 @@ protocol EmptySwiftProto {}
     // FIXME: should emit expected-DISABLED-error@-1 {{instance method 'method(fromHeader3:)' should be implemented in extension for main class interface, not category 'PresentAdditions'}}
     // FIXME: expected-warning@-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?; this will become an error after adopting '@implementation'}}
     // FIXME: expected-note@-3 {{add 'private' or 'fileprivate' to define an Objective-C-compatible instance method not declared in the header}} {{3-3=private }}
-    // FIXME: expected-note@-4 {{add 'final' to define a Swift instance method that cannot be overridden}} {{3-3=final }}
+    // FIXME: expected-note@-4 {{add 'final' to define a Swift-only instance method that cannot be overridden}} {{3-3=final }}
   }
 
   var propertyFromHeader7: CInt {
     // FIXME: should emit expected-DISABLED-error@-1 {{property 'propertyFromHeader7' should be implemented in extension for main class interface, not category 'PresentAdditions'}}
     // FIXME: expected-warning@-2 {{property 'propertyFromHeader7' does not match any property declared in the headers for 'ObjCClass'; did you use the property's Swift name?; this will become an error after adopting '@implementation'}}
     // FIXME: expected-note@-3 {{add 'private' or 'fileprivate' to define an Objective-C-compatible property not declared in the header}} {{3-3=private }}
-    // FIXME: expected-note@-4 {{add 'final' to define a Swift property that cannot be overridden}} {{3-3=final }}
+    // FIXME: expected-note@-4 {{add 'final' to define a Swift-only property that cannot be overridden}} {{3-3=final }}
     get { return 1 }
   }
 
@@ -270,7 +270,7 @@ protocol EmptySwiftProto {}
   func categoryMethodNot(fromHeader3: CInt) {
     // expected-warning@-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?; this will become an error after adopting '@implementation'}}
     // 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 }}
+    // expected-note@-3 {{add 'final' to define a Swift-only instance method that cannot be overridden}} {{3-3=final }}
   }
 
   var categoryPropertyFromHeader1: CInt