Tweak @_objcImpl deprecation warning behavior

• Simpler wording
• Now emitted only if there are no other @objcImpl diagnostics (so @implementation would not emit any new errors)

Author: beccadax

include/swift/AST/DiagnosticsSema.def

@@ -1740,8 +1740,8 @@ ERROR(swift_native_objc_runtime_base_not_on_root_class,none,
       "to root classes", ())
 
 WARNING(objc_implementation_early_spelling_deprecated,none,
-        "warning-only '@_objcImplementation' spelling is deprecated; use "
-        "'@implementation' instead", ())
+        "'@_objcImplementation' is deprecated; use '@implementation' instead",
+        ())
 ERROR(attr_objc_implementation_must_be_unconditional,none,
       "only unconditional extensions can implement an Objective-C '@interface'",
       ())

lib/Sema/TypeCheckAttr.cpp

@@ -1518,18 +1518,8 @@ void AttributeChecker::visitNonObjCAttr(NonObjCAttr *attr) {
 
 static bool hasObjCImplementationFeature(Decl *D, ObjCImplementationAttr *attr,
                                          Feature requiredFeature) {
-  if (D->getASTContext().LangOpts.hasFeature(requiredFeature)) {
-    // Encourage early ObjCImplementation adopters to adopt @implementation.
-    // (We'll do this for CImplementation when we're ready to stabilize it.)
-    if (requiredFeature == Feature::ObjCImplementation
-            && attr->isEarlyAdopter())
-      D->getASTContext().Diags.diagnose(
-            attr->getLocation(),
-            diag::objc_implementation_early_spelling_deprecated)
-        .fixItReplace(attr->getLocation(), "implementation");
-
+  if (D->getASTContext().LangOpts.hasFeature(requiredFeature))
     return true;
-  }
 
   // Allow the use of @_objcImplementation *without* Feature::ObjCImplementation
   // as long as you're using the early adopter syntax. (Avoids breaking existing

lib/Sema/TypeCheckDeclObjC.cpp

@@ -2803,17 +2803,25 @@ fixDeclarationStaticSpelling(InFlightDiagnostic &diag, ValueDecl *VD,
 
 namespace {
 class ObjCImplementationChecker {
-  DiagnosticEngine &diags;
+  Decl *decl;
+
+  ObjCImplementationAttr *getAttr() const {
+    return decl->getAttrs()
+               .getAttribute<ObjCImplementationAttr>(/*AllowInvalid=*/true);
+  }
 
   template<typename Loc, typename ...ArgTypes>
   InFlightDiagnostic diagnose(Loc loc, Diag<ArgTypes...> diagID,
                         typename detail::PassArgument<ArgTypes>::type... Args) {
+    hasDiagnosed = true;
+
+    auto &diags = decl->getASTContext().Diags;
     auto diag = diags.diagnose(loc, diagID, std::forward<ArgTypes>(Args)...);
 
     // Early adopters using the '@_objcImplementation' syntax may have had the
     // ObjCImplementationChecker evolve out from under them. Soften their errors
     // to warnings so we don't break their projects.
-    if (isEarlyAdopter
+    if (getAttr()->isEarlyAdopter()
          && diags.declaredDiagnosticKindFor(diagID.ID) == DiagnosticKind::Error)
       diag.wrapIn(diag::wrap_objc_implementation_will_become_error);
 
@@ -2825,18 +2833,14 @@ class ObjCImplementationChecker {
   /// Candidates with their explicit ObjC names, if any.
   llvm::SmallDenseMap<ValueDecl *, ObjCSelector, 16> unmatchedCandidates;
 
-  bool isEarlyAdopter;
+  bool hasDiagnosed = false;
 
 public:
   ObjCImplementationChecker(Decl *D)
-      : diags(D->getASTContext().Diags)
+      : decl(D), hasDiagnosed(getAttr()->isInvalid())
   {
     assert(!D->hasClangNode() && "passed interface, not impl, to checker");
 
-    isEarlyAdopter = D->getAttrs()
-        .getAttribute<ObjCImplementationAttr>(/*AllowInvalid=*/true)
-        ->isEarlyAdopter();
-
     if (auto func = dyn_cast<AbstractFunctionDecl>(D)) {
       addCandidate(D);
       addRequirement(D->getImplementedObjCDecl());
@@ -3607,6 +3611,31 @@ class ObjCImplementationChecker {
             .fixItInsert(cand->getAttributeInsertionLoc(true), "final ");
     }
   }
+
+  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())
+      return;
+
+    // Only encourage adoption if the corresponding language feature is enabled.
+    if (isa<ExtensionDecl>(decl) &&
+        !decl->getASTContext().LangOpts.hasFeature(Feature::ObjCImplementation))
+      return;
+
+    if (isa<AbstractFunctionDecl>(decl) &&
+        !decl->getASTContext().LangOpts.hasFeature(Feature::CImplementation))
+      return;
+
+    // Only encourage @_objcImplementation *extension* adopters to adopt
+    // @implementation; @_objcImplementation @_cdecl hasn't been stabilized yet.
+    if (!isa<ExtensionDecl>(decl))
+      return;
+
+    diagnose(getAttr()->getLocation(),
+             diag::objc_implementation_early_spelling_deprecated)
+        .fixItReplace(getAttr()->getLocation(), "implementation");
+  }
 };
 }
 
@@ -3626,6 +3655,7 @@ evaluate(Evaluator &evaluator, Decl *D) const {
   checker.matchRequirements();
   checker.diagnoseUnmatchedCandidates();
   checker.diagnoseUnmatchedRequirements();
+  checker.diagnoseEarlyAdopterDeprecation();
 
   return evaluator::SideEffect();
 }

test/decl/ext/Inputs/objc_implementation.h

@@ -127,6 +127,9 @@
 
 @end
 
+@interface ObjCClass (EmptyCategory)
+@end
+
 @protocol PartiallyOptionalProtocol
 
 - (void)requiredMethod1;

test/decl/ext/cdecl_implementation_features.swift

@@ -3,7 +3,7 @@
 
 // YES-DAG: cdecl_implementation_features.swift:[[@LINE+4]]:{{[0-9]+}}: warning: global function 'CImplFunc1' of type '(Double) -> ()' does not match type '(Int32) -> Void' declared by the header; this will become an error after adopting '@implementation'
 // NO-DAG: cdecl_implementation_features.swift:[[@LINE+3]]:{{[0-9]+}}: error: '_objcImplementation' attribute is only valid when experimental feature CImplementation is enabled
-// YES-NOT: cdecl_implementation_features.swift:[[@LINE+2]]:{{[0-9]+}}: warning: warning-only '@_objcImplementation' spelling is deprecated; use '@implementation' instead
+// YES-NOT: cdecl_implementation_features.swift:[[@LINE+2]]:{{[0-9]+}}: warning: '@_objcImplementation' is deprecated; use '@implementation' instead
 // TODO: When @implementation @_cdecl stabilizes, YES-NOT on the line above will become YES-DAG
 @_objcImplementation @_cdecl("CImplFunc1") func CImplFunc1(_: Double) {}
 

test/decl/ext/objc_implementation.swift

@@ -1,4 +1,4 @@
-// RUN: %target-typecheck-verify-swift -import-objc-header %S/Inputs/objc_implementation.h -enable-experimental-feature CImplementation
+// RUN: %target-typecheck-verify-swift -import-objc-header %S/Inputs/objc_implementation.h -enable-experimental-feature ObjCImplementation -enable-experimental-feature CImplementation
 // REQUIRES: objc_interop
 
 protocol EmptySwiftProto {}
@@ -432,14 +432,21 @@ protocol EmptySwiftProto {}
   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}}
 }
 
+// Intentionally using `@_objcImplementation` for this test; do not upgrade!
+@_objcImplementation(EmptyCategory) extension ObjCClass {
+  // expected-warning@-1 {{'@_objcImplementation' is deprecated; use '@implementation' instead}} {{2-21=implementation}}
+}
+
 @_objcImplementation extension ObjCImplSubclass {
-    @objc(initFromProtocol1:)
+  // expected-warning@-1 {{'@_objcImplementation' is deprecated; use '@implementation' instead}} {{2-21=implementation}}
+  @objc(initFromProtocol1:)
     required public init?(fromProtocol1: CInt) {
       // OK
     }
 }
 
 @_objcImplementation extension ObjCBasicInitClass {
+  // expected-warning@-1 {{'@_objcImplementation' is deprecated; use '@implementation' instead}} {{2-21=implementation}}
   init() {
     // OK
   }

test/decl/ext/objc_implementation_features.swift

@@ -3,9 +3,12 @@
 // RUN: not %target-swift-frontend(mock-sdk: %clang-importer-sdk-nosource) -typecheck %s -import-objc-header %S/Inputs/objc_implementation.h 2>&1 | %FileCheck --check-prefixes NO,CHECK %s
 // RUN: not %target-swift-frontend(mock-sdk: %clang-importer-sdk-nosource) -typecheck %s -import-objc-header %S/Inputs/objc_implementation.h -enable-experimental-feature ObjCImplementation 2>&1 | %FileCheck --check-prefixes YES,CHECK %s
 
-// CHECK-DAG: objc_implementation_features.swift:[[@LINE+3]]:{{[0-9]+}}: warning: extension for main class interface should provide implementation for instance method 'subclassMethod(fromHeader1:)'; this will become an error after adopting '@implementation'
-// NO-NOT: objc_implementation_features.swift:[[@LINE+2]]:{{[0-9]+}}: warning: warning-only '@_objcImplementation' spelling is deprecated; use '@implementation' instead
-// YES-DAG: objc_implementation_features.swift:[[@LINE+1]]:{{[0-9]+}}: warning: warning-only '@_objcImplementation' spelling is deprecated; use '@implementation' instead
+// NO-NOT: objc_implementation_features.swift:[[@LINE+2]]:{{[0-9]+}}: warning: '@_objcImplementation' is deprecated; use '@implementation' instead
+// YES-DAG: objc_implementation_features.swift:[[@LINE+1]]:{{[0-9]+}}: warning: '@_objcImplementation' is deprecated; use '@implementation' instead
+@_objcImplementation(EmptyCategory) extension ObjCClass {}
+
+// CHECK-DAG: objc_implementation_features.swift:[[@LINE+2]]:{{[0-9]+}}: warning: extension for main class interface should provide implementation for instance method 'subclassMethod(fromHeader1:)'; this will become an error after adopting '@implementation'
+// CHECK-NOT: objc_implementation_features.swift:[[@LINE+1]]:{{[0-9]+}}: warning: '@_objcImplementation' is deprecated; use '@implementation' instead
 @_objcImplementation extension ObjCSubclass {}
 
 // CHECK-DAG: objc_implementation_features.swift:[[@LINE+3]]:{{[0-9]+}}: error: extension for main class interface should provide implementation for initializer 'init()'{{$}}