[Sema] Report uses of implicitly imported decls in inlinable code

xymus · xymus · commit 9127f90d8d73 · 2022-06-29T16:09:42.000-07:00
Implicitly imported decls may end up in inlinable code and break the
module API. This have been known to lead to deserialization crash and
could in theory break the generated swiftinterfaces files. Let's
explicitly check for such a case, keeping it to a warning until Swift 6
where we can make it an error.

rdar://95816286
diff --git a/include/swift/AST/DiagnosticsSema.def b/include/swift/AST/DiagnosticsSema.def
@@ -2903,14 +2903,17 @@ ERROR(decl_from_hidden_module,none,
       "in an extension with conditional conformances}2; "
       "%select{%3 has been imported as implementation-only|"
       "it is an SPI imported from %3|"
-      "it is SPI}4",
+      "it is SPI|"
+      "%3 was implicitly imported}4",
       (DescriptiveDeclKind, DeclName, unsigned, Identifier, unsigned))
 WARNING(decl_from_hidden_module_warn,none,
       "cannot use %0 %1 %select{here|as property wrapper here|"
       "as result builder here|"
       "in an extension with public or '@usableFromInline' members|"
       "in an extension with conditional conformances}2; "
-      "%select{%3 has been imported as implementation-only}4",
+      "%select{%3 has been imported as implementation-only|"
+      "<<ERROR>>|<<ERROR>>|"
+      "%3 was implicitly imported}4",
       (DescriptiveDeclKind, DeclName, unsigned, Identifier, unsigned))
 ERROR(conformance_from_implementation_only_module,none,
       "cannot use conformance of %0 to %1 %select{here|as property wrapper here|"
@@ -5781,7 +5784,15 @@ ERROR(inlinable_decl_ref_from_hidden_module,
       none, "%0 %1 cannot be used in " FRAGILE_FUNC_KIND "2 "
       "because %select{%3 was imported implementation-only|"
       "it is an SPI imported from %3|"
-      "it is SPI}4",
+      "it is SPI|"
+      "%3 was implicitly imported}4",
+      (DescriptiveDeclKind, DeclName, unsigned, Identifier, unsigned))
+
+WARNING(inlinable_decl_ref_from_hidden_module_warn,
+      none, "%0 %1 cannot be used in " FRAGILE_FUNC_KIND "2 "
+      "because %select{<<ERROR>>|<<ERROR>>|<<ERROR>>|"
+      "%3 was implicitly imported}4"
+      "; this is an error in Swift 6",
       (DescriptiveDeclKind, DeclName, unsigned, Identifier, unsigned))
 
 ERROR(availability_macro_in_inlinable, none,
diff --git a/include/swift/AST/SourceFile.h b/include/swift/AST/SourceFile.h
@@ -31,6 +31,7 @@ class PersistentParserState;
 /// \sa getRestrictedImportKind
 enum class RestrictedImportKind {
   ImplementationOnly,
+  Implicit,
   None // No restriction, i.e. the module is imported publicly.
 };
 
diff --git a/lib/AST/Module.cpp b/lib/AST/Module.cpp
@@ -2440,14 +2440,18 @@ bool SourceFile::hasTestableOrPrivateImport(
 
 RestrictedImportKind SourceFile::getRestrictedImportKind(const ModuleDecl *module) const {
   auto &imports = getASTContext().getImportCache();
+  RestrictedImportKind importKind = RestrictedImportKind::Implicit;
 
   // Look at the imports of this source file.
   for (auto &desc : *Imports) {
     // Ignore implementation-only imports.
-    if (desc.options.contains(ImportFlags::ImplementationOnly))
+    if (desc.options.contains(ImportFlags::ImplementationOnly)) {
+      if (imports.isImportedBy(module, desc.module.importedModule))
+        importKind = RestrictedImportKind::ImplementationOnly;
       continue;
+    }
 
-    // If the module is imported this way, it's not imported
+    // If the module is imported publicly, it's not imported
     // implementation-only.
     if (imports.isImportedBy(module, desc.module.importedModule))
       return RestrictedImportKind::None;
@@ -2457,7 +2461,7 @@ RestrictedImportKind SourceFile::getRestrictedImportKind(const ModuleDecl *modul
   if (imports.isImportedBy(module, getParentModule()))
     return RestrictedImportKind::None;
 
-  return RestrictedImportKind::ImplementationOnly;
+  return importKind;
 }
 
 bool ModuleDecl::isImportedImplementationOnly(const ModuleDecl *module) const {
diff --git a/lib/Sema/ResilienceDiagnostics.cpp b/lib/Sema/ResilienceDiagnostics.cpp
@@ -146,7 +146,16 @@ TypeChecker::diagnoseDeclRefExportability(SourceLoc loc,
 
     D->diagnose(diag::kind_declared_here, DescriptiveDeclKind::Type);
   } else {
-    ctx.Diags.diagnose(loc, diag::inlinable_decl_ref_from_hidden_module,
+    // Only implicitly imported decls should be reported as a warning,
+    // and only for language versions below Swift 6.
+    assert(downgradeToWarning == DowngradeToWarning::No ||
+           originKind == DisallowedOriginKind::ImplicitlyImported &&
+           "Only implicitly imported decls should be reported as a warning.");
+    auto errorOrWarning = downgradeToWarning == DowngradeToWarning::Yes?
+                              diag::inlinable_decl_ref_from_hidden_module_warn:
+                              diag::inlinable_decl_ref_from_hidden_module;
+
+    ctx.Diags.diagnose(loc, errorOrWarning,
                        D->getDescriptiveKind(), D->getName(),
                        fragileKind.getSelector(), definingModule->getName(),
                        static_cast<unsigned>(originKind));
diff --git a/lib/Sema/TypeCheckAccess.cpp b/lib/Sema/TypeCheckAccess.cpp
@@ -1511,6 +1511,15 @@ swift::getDisallowedOriginKind(const Decl *decl,
     if (where.isSPI())
       downgradeToWarning = DowngradeToWarning::Yes;
 
+    // Before Swift 6, implicit imports were not reported unless an
+    // implementation-only import was also present. Downgrade to a warning
+    // just in this case.
+    if (howImported == RestrictedImportKind::Implicit &&
+        !SF->getASTContext().isSwiftVersionAtLeast(6) &&
+        !SF->hasImplementationOnlyImports()) {
+      downgradeToWarning = DowngradeToWarning::Yes;
+    }
+
     // Even if the current module is @_implementationOnly, Swift should
     // not report an error in the cases where the decl is also exported from
     // a non @_implementationOnly module. Thus, we check to see if there is
@@ -1545,7 +1554,10 @@ swift::getDisallowedOriginKind(const Decl *decl,
         }
       }
     }
-    // Implementation-only imported, cannot be reexported.
+
+    // Restrictively imported, cannot be reexported.
+    if (howImported == RestrictedImportKind::Implicit)
+      return DisallowedOriginKind::ImplicitlyImported;
     return DisallowedOriginKind::ImplementationOnly;
   } else if ((decl->isSPI() || decl->isAvailableAsSPI()) && !where.isSPI()) {
     if (decl->isAvailableAsSPI() && !decl->isSPI()) {
diff --git a/lib/Sema/TypeCheckAccess.h b/lib/Sema/TypeCheckAccess.h
@@ -35,12 +35,14 @@ void checkAccessControl(Decl *D);
 // Problematic origin of an exported type.
 //
 // This enum must be kept in sync with
+// diag::inlinable_decl_ref_from_hidden_module,
 // diag::decl_from_hidden_module and
 // diag::conformance_from_implementation_only_module.
 enum class DisallowedOriginKind : uint8_t {
   ImplementationOnly,
   SPIImported,
   SPILocal,
+  ImplicitlyImported,
   None
 };
 
diff --git a/test/Sema/implicit-import-in-inlinable-code.swift b/test/Sema/implicit-import-in-inlinable-code.swift
@@ -0,0 +1,74 @@
+/// Report the use in API of indirectly or implicitly imported decls.
+
+// RUN: %empty-directory(%t)
+// RUN: %{python} %utils/split_file.py -o %t %s
+
+// RUN: %target-swift-frontend -emit-module %t/empty.swift -module-name empty -o %t/empty.swiftmodule
+// RUN: %target-swift-frontend -emit-module %t/libA.swift -module-name libA -o %t/libA.swiftmodule
+// RUN: %target-swift-frontend -emit-module %t/libB.swift -module-name libB -o %t/libB.swiftmodule -I %t
+
+/// In pre-Swift 6, this is a warning where there's no implementation-only import present.
+// RUN: %target-swift-frontend -emit-module %t/clientFileA-Swift5.swift %t/clientFileB.swift -module-name client -o %t/client.swiftmodule -I %t -verify
+
+/// In pre-Swift 6, this remains an error when there's an implementation-only import present.
+// RUN: %target-swift-frontend -emit-module %t/clientFileA-OldCheck.swift %t/clientFileB.swift -module-name client -o %t/client.swiftmodule -I %t -verify
+
+/// In Swift 6, it's an error.
+// RUN: %target-swift-frontend -emit-module %t/clientFileA-Swift6.swift %t/clientFileB.swift -module-name client -o %t/client.swiftmodule -I %t -verify -swift-version 6
+
+// BEGIN empty.swift
+
+// BEGIN libA.swift
+public struct ImportedType {
+    public init() {}
+}
+
+// BEGIN libB.swift
+import libA
+
+extension ImportedType {
+    public func implicitlyImportedMethod() {}
+}
+
+/// Client module
+// BEGIN clientFileA-Swift5.swift
+import libA
+
+@inlinable public func bar() {
+  let a = ImportedType()
+  a.implicitlyImportedMethod() // expected-warning {{instance method 'implicitlyImportedMethod()' cannot be used in an '@inlinable' function because 'libB' was implicitly imported; this is an error in Swift 6}}
+
+  // Expected implicit imports are still fine
+  a.localModuleMethod()
+}
+
+// BEGIN clientFileA-OldCheck.swift
+import libA
+@_implementationOnly import empty
+
+@inlinable public func bar() {
+  let a = ImportedType()
+  a.implicitlyImportedMethod() // expected-error {{instance method 'implicitlyImportedMethod()' cannot be used in an '@inlinable' function because 'libB' was implicitly imported}}
+
+  // Expected implicit imports are still fine
+  a.localModuleMethod()
+}
+
+// BEGIN clientFileA-Swift6.swift
+import libA
+
+@inlinable public func bar() {
+  let a = ImportedType()
+  a.implicitlyImportedMethod() // expected-error {{instance method 'implicitlyImportedMethod()' cannot be used in an '@inlinable' function because 'libB' was implicitly imported}}
+
+  // Expected implicit imports are still fine
+  a.localModuleMethod()
+}
+
+// BEGIN clientFileB.swift
+@_implementationOnly import libB
+import libA
+extension ImportedType {
+    public func localModuleMethod() {}
+}
+
