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

xymus · web-flow · commit fd59933ff5a6 · 2024-04-23T09:22:12.000-07:00
Sema: Warn that resilient uses of `@_implementationOnly` are deprecated
diff --git a/include/swift/AST/DiagnosticsSema.def b/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",
diff --git a/include/swift/AST/Import.h b/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 {
diff --git a/lib/Sema/ImportResolution.cpp b/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;
diff --git a/test/Concurrency/preconcurrency_implementationonly_override.swift b/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
diff --git a/test/ModuleInterface/module_shadowing.swift b/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 {
diff --git a/test/SPI/implementation_only_spi_import_exposability.swift b/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}}
diff --git a/test/SPI/report-ioi-in-spi.swift b/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()
diff --git a/test/SPI/spi-only-import-conflicting.swift b/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}}
diff --git a/test/Sema/implementation-only-deprecated.swift b/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) {}
diff --git a/test/Sema/implementation-only-import-from-non-resilient.swift b/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
diff --git a/test/Sema/implementation-only-import-in-decls.swift b/test/Sema/implementation-only-import-in-decls.swift
@@ -9,6 +9,7 @@
 
 import NormalLibrary
 @_implementationOnly import BADLibrary
+// expected-warning @-1 {{'@_implementationOnly' is deprecated, use 'internal import' instead}}
 
 public struct TestConformance: BadProto {} // expected-error {{cannot use protocol 'BadProto' here; 'BADLibrary' has been imported as implementation-only}}
 
diff --git a/test/Sema/implementation-only-import-inlinable-conformances.swift b/test/Sema/implementation-only-import-inlinable-conformances.swift
@@ -7,6 +7,7 @@
 // RUN: %target-typecheck-verify-swift -I %t -enable-library-evolution -swift-version 5
 
 @_implementationOnly import BADLibrary
+// expected-warning @-1 {{'@_implementationOnly' is deprecated, use 'internal import' instead}}
 import NormalLibrary
 
 @available(*, unavailable)
diff --git a/test/Sema/implementation-only-import-inlinable-indirect.swift b/test/Sema/implementation-only-import-inlinable-indirect.swift
@@ -8,6 +8,7 @@
 // RUN:   -enable-library-evolution -swift-version 5
 
 @_implementationOnly import directs
+// expected-warning @-1 {{'@_implementationOnly' is deprecated, use 'internal import' instead}}
 
 // Types
 
diff --git a/test/Sema/implementation-only-import-inlinable-multifile.swift b/test/Sema/implementation-only-import-inlinable-multifile.swift
@@ -8,6 +8,7 @@
 // RUN:   -enable-library-evolution -swift-version 5
 
 @_implementationOnly import directs
+// expected-warning @-1 {{'@_implementationOnly' is deprecated, use 'internal import' instead}}
 // 'indirects' is imported for re-export in a secondary file
 
 // Types
diff --git a/test/Sema/implementation-only-import-inlinable.swift b/test/Sema/implementation-only-import-inlinable.swift
@@ -8,6 +8,7 @@
 // RUN:   -enable-library-evolution -swift-version 5
 
 @_implementationOnly import directs
+// expected-warning @-1 {{'@_implementationOnly' is deprecated, use 'internal import' instead}}
 import indirects
 
 // Types
diff --git a/test/Sema/implementation-only-import-library-evolution.swift b/test/Sema/implementation-only-import-library-evolution.swift
@@ -5,6 +5,7 @@
 // RUN: %target-typecheck-verify-swift -I %t -enable-library-evolution
 
 @_implementationOnly import BADLibrary
+// expected-warning @-1 {{'@_implementationOnly' is deprecated, use 'internal import' instead}}
 
 public struct PublicStructStoredProperties {
   public var publiclyBad: BadStruct? // expected-error {{cannot use struct 'BadStruct' here; 'BADLibrary' has been imported as implementation-only}}
diff --git a/test/Sema/missing-import-inlinable-code.swift b/test/Sema/missing-import-inlinable-code.swift
@@ -75,6 +75,7 @@ public import libA
 // BEGIN clientFileA-OldCheck.swift
 public import libA
 @_implementationOnly import empty
+// expected-warning @-1 {{'@_implementationOnly' is deprecated, use 'internal import' instead}}
 
 @inlinable public func bar() {
   let a = ImportedType()
@@ -102,6 +103,7 @@ public import libA
 
 // BEGIN clientFileB.swift
 @_implementationOnly import libB
+// expected-warning @-1 {{'@_implementationOnly' is deprecated, use 'internal import' instead}}
 public import libA
 extension ImportedType {
     public func localModuleMethod() {}
diff --git a/test/Sema/missing-import-typealias.swift b/test/Sema/missing-import-typealias.swift
@@ -97,6 +97,7 @@ extension StructAlias {
 
 import Aliases
 @_implementationOnly import Original
+// expected-warning @-1 {{'@_implementationOnly' is deprecated, use 'internal import' instead}}
 
 @inlinable public func inlinableFunc() {
   // expected-warning@+1 {{'StructAlias' aliases 'Original.Struct' and cannot be used in an '@inlinable' function because 'Original' has been imported as implementation-only; this is an error in the Swift 6 language mode}}
diff --git a/test/Sema/restricted-imports-priorities.swift b/test/Sema/restricted-imports-priorities.swift
@@ -45,18 +45,22 @@ public struct LibAStruct {}
 
 //--- TwoIOI.swift
 @_implementationOnly import LibB
+// expected-warning @-1 {{'@_implementationOnly' is deprecated, use 'internal import' instead}}
 @_implementationOnly import LibC
+// expected-warning @-1 {{'@_implementationOnly' is deprecated, use 'internal import' instead}}
 
 public func foo(a: LibAStruct) {} // expected-error {{cannot use struct 'LibAStruct' here; 'LibA' has been imported as implementation-only}}
 
 //--- SPIOnlyAndIOI1.swift
 @_spiOnly import LibB
 @_implementationOnly import LibC
+// expected-warning @-1 {{'@_implementationOnly' is deprecated, use 'internal import' instead}}
 
 public func foo(a: LibAStruct) {} // expected-error {{cannot use struct 'LibAStruct' here; 'LibA' was imported for SPI only}}
 
 //--- SPIOnlyAndIOI2.swift
 @_implementationOnly import LibB
+// expected-warning @-1 {{'@_implementationOnly' is deprecated, use 'internal import' instead}}
 @_spiOnly import LibC
 
 public func foo(a: LibAStruct) {} // expected-error {{cannot use struct 'LibAStruct' here; 'LibA' was imported for SPI only}}
