Sema: Warn on resilient uses of @_implementationOnly as deprecated

Report uses of `@_implementationOnly` in resilient modules as deprecated.
With a fixit to replace it with `internal` or delete it when imports
are internal by default.

Uses of `@_implementationOnly` in non-resilient modules is already reported
as being unsafe.

Author: xymus

include/swift/AST/DiagnosticsSema.def

@@ -1164,6 +1164,14 @@ WARNING(implementation_only_requires_library_evolution,none,
         "using '@_implementationOnly' without enabling library evolution "
         "for %0 may lead to instability during execution",
         (Identifier))
+WARNING(implementation_only_deprecated_explicit,none,
+        "'@_implementationOnly' is deprecated, use 'internal import' "
+        "and family instead",
+        ())
+WARNING(implementation_only_deprecated_implicit,none,
+        "'@_implementationOnly' is deprecated, use a bare import "
+        "as 'InternalImportsByDefault' is enabled",
+        ())
 
 ERROR(module_allowable_client_violation,none,
       "module %0 doesn't allow importation from module %1",

include/swift/AST/Import.h

@@ -592,17 +592,22 @@ struct AttributedImport {
   /// if the access level was implicit or explicit.
   SourceRange accessLevelRange;
 
+  /// Location of the `@_implementationOnly` attribute if set.
+  SourceRange implementationOnlyRange;
+
   AttributedImport(ModuleInfo module, SourceLoc importLoc = SourceLoc(),
                    ImportOptions options = ImportOptions(),
                    StringRef filename = {}, ArrayRef<Identifier> spiGroups = {},
                    SourceRange preconcurrencyRange = {},
                    std::optional<AccessLevel> docVisibility = std::nullopt,
                    AccessLevel accessLevel = AccessLevel::Public,
-                   SourceRange accessLevelRange = SourceRange())
+                   SourceRange accessLevelRange = SourceRange(),
+                   SourceRange implementationOnlyRange = SourceRange())
       : module(module), importLoc(importLoc), options(options),
         sourceFileArg(filename), spiGroups(spiGroups),
         preconcurrencyRange(preconcurrencyRange), docVisibility(docVisibility),
-        accessLevel(accessLevel), accessLevelRange(accessLevelRange) {
+        accessLevel(accessLevel), accessLevelRange(accessLevelRange),
+        implementationOnlyRange(implementationOnlyRange) {
     assert(!(options.contains(ImportFlags::Exported) &&
              options.contains(ImportFlags::ImplementationOnly)) ||
            options.contains(ImportFlags::Reserved));
@@ -613,7 +618,8 @@ struct AttributedImport {
     : AttributedImport(module, other.importLoc, other.options,
                        other.sourceFileArg, other.spiGroups,
                        other.preconcurrencyRange, other.docVisibility,
-                       other.accessLevel, other.accessLevelRange) { }
+                       other.accessLevel, other.accessLevelRange,
+                       other.implementationOnlyRange) { }
 
   friend bool operator==(const AttributedImport<ModuleInfo> &lhs,
                          const AttributedImport<ModuleInfo> &rhs) {
@@ -623,7 +629,8 @@ struct AttributedImport {
            lhs.spiGroups == rhs.spiGroups &&
            lhs.docVisibility == rhs.docVisibility &&
            lhs.accessLevel == rhs.accessLevel &&
-           lhs.accessLevelRange == rhs.accessLevelRange;
+           lhs.accessLevelRange == rhs.accessLevelRange &&
+           lhs.implementationOnlyRange == rhs.implementationOnlyRange;
   }
 
   AttributedImport<ImportedModule> getLoaded(ModuleDecl *loadedModule) const {

lib/Sema/ImportResolution.cpp

@@ -577,8 +577,10 @@ UnboundImport::UnboundImport(ImportDecl *ID)
   if (ID->getAttrs().hasAttribute<TestableAttr>())
     import.options |= ImportFlags::Testable;
 
-  if (ID->getAttrs().hasAttribute<ImplementationOnlyAttr>())
+  if (auto attr = ID->getAttrs().getAttribute<ImplementationOnlyAttr>()) {
     import.options |= ImportFlags::ImplementationOnly;
+    import.implementationOnlyRange = attr->Range;
+  }
 
   import.accessLevel = ID->getAccessLevel();
   if (auto attr = ID->getAttrs().getAttribute<AccessControlAttr>()) {
@@ -802,7 +804,21 @@ void UnboundImport::validateResilience(NullablePtr<ModuleDecl> topLevelModule,
   // We exempt some imports using @_implementationOnly in a safe way from
   // packages that cannot be resilient.
   if (import.options.contains(ImportFlags::ImplementationOnly) &&
-      !SF.getParentModule()->isResilient() && topLevelModule &&
+      import.implementationOnlyRange.isValid()) {
+    if (SF.getParentModule()->isResilient()) {
+      // Encourage replacing `@_implementationOnly` with `internal import`.
+      if (ctx.LangOpts.hasFeature(Feature::InternalImportsByDefault)) {
+        auto inFlight =
+          ctx.Diags.diagnose(import.importLoc,
+                             diag::implementation_only_deprecated_implicit);
+        inFlight.fixItRemove(import.implementationOnlyRange);
+      } else {
+        auto inFlight =
+          ctx.Diags.diagnose(import.importLoc,
+                             diag::implementation_only_deprecated_explicit);
+        inFlight.fixItReplace(import.implementationOnlyRange, "internal");
+      }
+    } else if ( // Non-resilient
       !(((targetName.str() == "CCryptoBoringSSL" ||
           targetName.str() == "CCryptoBoringSSLShims") &&
          (importerName.str() == "Crypto" ||
@@ -811,11 +827,13 @@ void UnboundImport::validateResilience(NullablePtr<ModuleDecl> topLevelModule,
         ((targetName.str() == "CNIOBoringSSL" ||
           targetName.str() == "CNIOBoringSSLShims") &&
          importerName.str() == "NIOSSL"))) {
-    ctx.Diags.diagnose(import.importLoc,
-                       diag::implementation_only_requires_library_evolution,
-                       importerName);
+      ctx.Diags.diagnose(import.importLoc,
+                         diag::implementation_only_requires_library_evolution,
+                         importerName);
+    }
   }
 
+  // Report public imports of non-resilient modules from a resilient module.
   if (import.options.contains(ImportFlags::ImplementationOnly) ||
       import.accessLevel < AccessLevel::Public)
     return;

test/Sema/implementation-only-deprecated.swift

@@ -0,0 +1,23 @@
+// RUN: %empty-directory(%t)
+// RUN: split-file %s %t --leading-lines
+
+// RUN: %target-swift-frontend -emit-module %t/Lib.swift \
+// RUN:   -enable-library-evolution -swift-version 5 \
+// RUN:   -emit-module-path %t/Lib.swiftmodule
+// RUN: %target-swift-frontend -typecheck %t/Client.swift -I %t \
+// RUN:   -enable-library-evolution -swift-version 5 \
+// RUN:   -verify -verify-additional-prefix swift-5-
+// RUN: %target-swift-frontend -typecheck %t/Client.swift -I %t \
+// RUN:   -enable-library-evolution -swift-version 5 \
+// RUN:   -enable-upcoming-feature InternalImportsByDefault \
+// RUN:   -verify -verify-additional-prefix default-to-internal-
+
+//--- Lib.swift
+public struct SomeType {}
+
+//--- Client.swift
+@_implementationOnly import Lib
+// expected-swift-5-warning @-1 {{'@_implementationOnly' is deprecated, use 'internal import' and family instead}} {{1-21=internal}}
+// expected-default-to-internal-warning @-2 {{'@_implementationOnly' is deprecated, use a bare import as 'InternalImportsByDefault' is enabled}} {{1-22=}}
+
+internal func foo(_: SomeType) {}