Merge pull request #73179 from xymus/deprecated-impl-only

Sema: Warn that resilient uses of `@_implementationOnly` are deprecated

Author: xymus

include/swift/AST/DiagnosticsSema.def

@@ -1167,6 +1167,9 @@ 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,none,
+        "'@_implementationOnly' is deprecated, use 'internal import' instead",
+        ())
 
 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

@@ -586,8 +586,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>()) {
@@ -811,7 +813,14 @@ 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`.
+      auto inFlight =
+        ctx.Diags.diagnose(import.importLoc,
+                           diag::implementation_only_deprecated);
+      inFlight.fixItReplace(import.implementationOnlyRange, "internal");
+    } else if ( // Non-resilient
       !(((targetName.str() == "CCryptoBoringSSL" ||
           targetName.str() == "CCryptoBoringSSLShims") &&
          (importerName.str() == "Crypto" ||
@@ -820,11 +829,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/Concurrency/preconcurrency_implementationonly_override.swift

@@ -11,6 +11,7 @@
 // REQUIRES: asserts
 
 @_implementationOnly import ImplementationOnlyDefs
+// expected-warning @-1 {{'@_implementationOnly' is deprecated, use 'internal import' instead}}
 
 class D: C {
   @_implementationOnly

test/ModuleInterface/module_shadowing.swift

@@ -29,7 +29,7 @@
 import ShadowyHorror
 // OTHER-DAG: ShadowyHorror.module_shadowing:{{[0-9]+:[0-9]+}}: warning: public class 'ShadowyHorror.module_shadowing' shadows module 'module_shadowing', which may cause failures when importing 'module_shadowing' or its clients in some configurations; please rename either the class 'ShadowyHorror.module_shadowing' or the module 'module_shadowing', or see https://github.com/apple/swift/issues/56573 for workarounds{{$}}
 
-@_implementationOnly import TestModule
+internal import TestModule
 #endif
 
 public struct ShadowyHorror {

test/SPI/implementation_only_spi_import_exposability.swift

@@ -27,6 +27,7 @@ public protocol IOIProtocol {}
 #elseif CLIENT
 
 @_spi(A) @_implementationOnly import Lib
+// expected-warning @-1 {{'@_implementationOnly' is deprecated, use 'internal import' instead}}
 
 @_spi(B) public func leakSPIStruct(_ a: SPIStruct) -> SPIStruct { fatalError() } // expected-warning 2 {{cannot use struct 'SPIStruct' here; 'Lib' has been imported as implementation-only}}
 @_spi(B) public func leakIOIStruct(_ a: IOIStruct) -> IOIStruct { fatalError() } // expected-warning 2 {{cannot use struct 'IOIStruct' here; 'Lib' has been imported as implementation-only}}

test/SPI/report-ioi-in-spi.swift

@@ -26,6 +26,7 @@ public struct IOIStruct {
 //--- ClientSPIOnlyMode.swift
 
 @_implementationOnly import Lib
+// expected-warning @-1 {{'@_implementationOnly' is deprecated, use 'internal import' instead}}
 
 @_spi(X) public func spiClient(s: IOIStruct) -> IOIStruct { // expected-error 2 {{cannot use struct 'IOIStruct' here; 'Lib' has been imported as implementation-only}}
     return IOIStruct()
@@ -39,6 +40,7 @@ public struct IOIStruct {
 //--- ClientDefaultMode.swift
 
 @_implementationOnly import Lib
+// expected-warning @-1 {{'@_implementationOnly' is deprecated, use 'internal import' instead}}
 
 @_spi(X) public func spiClient(s: IOIStruct) -> IOIStruct { // expected-warning 2 {{cannot use struct 'IOIStruct' here; 'Lib' has been imported as implementation-only}}
     return IOIStruct()

test/SPI/spi-only-import-conflicting.swift

@@ -60,13 +60,15 @@ import Lib // expected-error {{'Lib' inconsistently imported for SPI only}}
 // expected-warning @-1 {{'Lib' inconsistently imported as implementation-only}}
 @_implementationOnly import Lib // expected-error {{'Lib' inconsistently imported for SPI only}}
 // expected-note @-1 {{imported as implementation-only here}}
+// expected-warning @-2 {{'@_implementationOnly' is deprecated, use 'internal import' instead}}
 
 /// Many confliciting imports lead to many diagnostics.
 //--- SPIOnly_IOI_Exported_Default.swift
 @_spiOnly import Lib // expected-note 3 {{imported for SPI only here}}
 // expected-warning @-1 {{'Lib' inconsistently imported as implementation-only}}
 @_implementationOnly import Lib // expected-error {{'Lib' inconsistently imported for SPI only}}
 // expected-note @-1 3 {{imported as implementation-only here}}
+// expected-warning @-2 {{'@_implementationOnly' is deprecated, use 'internal import' instead}}
 @_exported import Lib // expected-error {{'Lib' inconsistently imported for SPI only}}
 // expected-warning @-1 {{'Lib' inconsistently imported as implementation-only}}
 import Lib // expected-error {{'Lib' inconsistently imported for SPI only}}
@@ -82,6 +84,7 @@ import Lib
 /// Different IOI in different files of the same module are still rejected.
 //--- IOI_Default_FileA.swift
 @_implementationOnly import Lib // expected-note {{imported as implementation-only here}}
+// expected-warning @-1 {{'@_implementationOnly' is deprecated, use 'internal import' instead}}
 
 //--- IOI_Default_FileB.swift
 import Lib // expected-warning {{'Lib' inconsistently imported as implementation-only}}

test/Sema/implementation-only-deprecated.swift

@@ -0,0 +1,22 @@
+// 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
+// RUN: %target-swift-frontend -typecheck %t/Client.swift -I %t \
+// RUN:   -enable-library-evolution -swift-version 5 \
+// RUN:   -enable-upcoming-feature InternalImportsByDefault \
+// RUN:   -verify
+
+//--- Lib.swift
+public struct SomeType {}
+
+//--- Client.swift
+@_implementationOnly import Lib
+// expected-warning @-1 {{'@_implementationOnly' is deprecated, use 'internal import' instead}} {{1-21=internal}}
+
+internal func foo(_: SomeType) {}

test/Sema/implementation-only-import-from-non-resilient.swift

@@ -27,6 +27,7 @@ import B
 
 //--- client-resilient.swift
 @_implementationOnly import A
+// expected-warning @-1 {{'@_implementationOnly' is deprecated, use 'internal import' instead}}
 import B
 
 //--- Crypto.swift