Serialization: Error on xref to implementation-only dependencies

Introduce a last resort check reporting references to
implementation-only dependencies that would appear in the generated
swiftmodule. This check is applied at serialization, long after
exportability checking applied at typechecking. It should act as a back
stop to references missed by typechecking or @_implementationOnly decls
that should have been skipped.

This check is gated behind CheckImplementationOnlyStrict and should be
used with embedded only.

rdar://160697599

Author: xymus

include/swift/AST/DiagnosticsSema.def

@@ -887,6 +887,10 @@ REMARK(serialization_skipped_invalid_type_unknown_name,none,
 ERROR(serialization_failed,none,
       "serialization of module %0 failed due to the errors above",
       (const ModuleDecl *))
+ERROR(serialization_xref_to_hidden_dependency,none,
+      "invalid reference to implementation-only imported module %0"
+      "%select{| for %1}1",
+      (const ModuleDecl *, const Decl *))
 
 WARNING(can_import_invalid_swiftmodule,none,
         "canImport() evaluated to false due to invalid swiftmodule: %0", (StringRef))

include/swift/AST/Module.h

@@ -1064,11 +1064,15 @@ class ModuleDecl
   void
   getImportedModulesForLookup(SmallVectorImpl<ImportedModule> &imports) const;
 
-  /// Has \p module been imported via an '@_implementationOnly' import
-  /// instead of another kind of import?
+  /// Has \p module been imported via an '@_implementationOnly' import and
+  /// not by anything more visible?
   ///
-  /// This assumes that \p module was imported.
-  bool isImportedImplementationOnly(const ModuleDecl *module) const;
+  /// If \p assumeImported, assume that \p module was imported and avoid the
+  /// work to confirm it is imported at all. Transitive modules not reexported
+  /// are not considered imported here and may lead to false positive without
+  /// this setting.
+  bool isImportedImplementationOnly(const ModuleDecl *module,
+      bool assumeImported = true) const;
 
   /// Finds all top-level decls of this module.
   ///

lib/AST/Module.cpp

@@ -3078,7 +3078,8 @@ void ModuleDecl::setPackageName(Identifier name) {
   Package = PackageUnit::create(name, *this, getASTContext());
 }
 
-bool ModuleDecl::isImportedImplementationOnly(const ModuleDecl *module) const {
+bool ModuleDecl::isImportedImplementationOnly(const ModuleDecl *module,
+    bool assumeImported) const {
   if (module == this) return false;
 
   auto &imports = getASTContext().getImportCache();
@@ -3099,7 +3100,17 @@ bool ModuleDecl::isImportedImplementationOnly(const ModuleDecl *module) const {
       return false;
   }
 
-  return true;
+  if (assumeImported)
+    return true;
+
+  results.clear();
+  getImportedModules(results,
+      {ModuleDecl::ImportFilterKind::ImplementationOnly});
+  for (auto &desc : results)
+    if (imports.isImportedBy(module, desc.importedModule))
+      return true;
+
+  return false;
 }
 
 void SourceFile::lookupImportedSPIGroups(

lib/Serialization/Serialization.cpp

@@ -756,6 +756,16 @@ IdentifierID Serializer::addContainingModuleRef(const DeclContext *DC,
   if (M->isClangHeaderImportModule())
     return OBJC_HEADER_MODULE_ID;
 
+  // Reject references to hidden dependencies.
+  if (getASTContext().LangOpts.hasFeature(
+          Feature::CheckImplementationOnlyStrict) &&
+      !allowCompilerErrors() &&
+      this->M->isImportedImplementationOnly(M, /*assumeImported=*/false)) {
+    getASTContext().Diags.diagnose(SourceLoc(),
+        diag::serialization_xref_to_hidden_dependency,
+        M, crossReferencedDecl);
+  }
+
   auto exportedModuleName = file->getExportedModuleName();
   assert(!exportedModuleName.empty());
   auto moduleID = M->getASTContext().getIdentifier(exportedModuleName);
@@ -2452,6 +2462,8 @@ void Serializer::writeCrossReference(const Decl *D) {
 
   unsigned abbrCode;
 
+  llvm::SaveAndRestore<const Decl *> SaveDecl(crossReferencedDecl, D);
+
   if (auto op = dyn_cast<OperatorDecl>(D)) {
     writeCrossReference(op->getDeclContext(), 1);
 
@@ -5397,8 +5409,7 @@ void Serializer::writeASTBlockEntity(const Decl *D) {
 
     // Skip non-public @export(interface) functions.
     auto FD = dyn_cast<AbstractFunctionDecl>(D);
-    if (FD &&
-        FD->getAttrs().hasAttribute<NeverEmitIntoClientAttr>() &&
+    if (FD && FD->isNeverEmittedIntoClient() &&
         !FD->getFormalAccessScope(/*useDC*/nullptr,
           /*treatUsableFromInlineAsPublic*/true).isPublicOrPackage())
       return;

lib/Serialization/Serialization.h

@@ -118,6 +118,9 @@ class Serializer : public SerializerBase {
 
   bool hadImplementationOnlyImport = false;
 
+  /// Current decl being serialized.
+  const Decl* crossReferencedDecl = nullptr;
+
   /// Helper for serializing entities in the AST block object graph.
   ///
   /// Keeps track of assigning IDs to newly-seen entities, and collecting

test/Serialization/implementation-only-embedded-fallback.swift

@@ -0,0 +1,139 @@
+/// Test CheckImplementationOnlyStrict fallback errors at serialization.
+
+// RUN: %empty-directory(%t)
+// RUN: split-file --leading-lines %s %t
+
+// RUN: %target-swift-frontend -emit-module %t/HiddenLib.swift -o %t -I %t \
+// RUN:   -swift-version 5 -target arm64-apple-none-macho \
+// RUN:   -enable-experimental-feature Embedded
+
+/// Report errors on invalid references. Disable the early checks to trigger
+/// underlying ones.
+// RUN: not env SWIFT_DISABLE_IMPLICIT_CHECK_IMPLEMENTATION_ONLY=1 \
+// RUN:   %target-swift-frontend -emit-module %t/MiddleLib.swift -o %t -I %t \
+// RUN:   -D BROKEN \
+// RUN:   -swift-version 5 -target arm64-apple-none-macho \
+// RUN:   -enable-experimental-feature Embedded \
+// RUN:   -enable-experimental-feature CheckImplementationOnlyStrict 2> %t/out
+// RUN: %FileCheck --input-file %t/out %t/MiddleLib.swift
+
+/// Build a valid version of the library, skipping @_implementationOnly decls.
+/// for a client to build against.
+// RUN: %target-swift-frontend -emit-module %t/MiddleLib.swift -o %t -I %t \
+// RUN:   -swift-version 5 -target arm64-apple-none-macho \
+// RUN:   -enable-experimental-feature Embedded \
+// RUN:   -enable-experimental-feature CheckImplementationOnlyStrict
+
+/// Build an actual client.
+// RUN: %target-swift-frontend -typecheck %t/Client.swift -I %t \
+// RUN:   -swift-version 5 -target arm64-apple-none-macho \
+// RUN:   -enable-experimental-feature Embedded \
+// RUN:   -enable-experimental-feature CheckImplementationOnlyStrict
+
+// REQUIRES: swift_feature_Embedded
+// REQUIRES: swift_feature_CheckImplementationOnlyStrict
+// REQUIRES: embedded_stdlib_cross_compiling
+
+//--- HiddenLib.swift
+
+public struct A {}
+public struct B {}
+public struct C {}
+public struct D {}
+public struct E {}
+public struct F {}
+
+public struct OkA {}
+public struct OkB {}
+public struct OkC {}
+public struct OkD {}
+public struct OkE {}
+public struct OkF {}
+
+public protocol Proto {}
+public protocol OkProto {}
+
+//--- MiddleLib.swift
+
+@_implementationOnly import HiddenLib
+
+/// Referenced types
+
+#if BROKEN
+internal struct InternalStruct: Proto {
+// CHECK-DAG: error: invalid reference to implementation-only imported module 'HiddenLib' for 'Proto'
+  var a: A
+// CHECK-DAG: error: invalid reference to implementation-only imported module 'HiddenLib' for 'A'
+}
+
+internal enum InternalEnum {
+  case b(B)
+// CHECK-DAG: error: invalid reference to implementation-only imported module 'HiddenLib' for 'B'
+  case c(C)
+// CHECK-DAG: error: invalid reference to implementation-only imported module 'HiddenLib' for 'C'
+}
+
+public class PublicClass {
+  init() { fatalError() }
+  internal var internalField: D
+// CHECK-DAG: error: invalid reference to implementation-only imported module 'HiddenLib' for 'D'
+  private var privateField: E
+// CHECK-DAG: error: invalid reference to implementation-only imported module 'HiddenLib' for 'E'
+}
+
+@export(interface)
+private func PrivateFunc(h: F) {}
+// CHECK-DAG: error: invalid reference to implementation-only imported module 'HiddenLib' for 'F'
+#endif
+
+@_implementationOnly
+internal struct OkInternalStruct: OkProto {
+  var a: OkA
+}
+
+@_implementationOnly
+internal struct NesterStruct {
+  var a: OkA
+
+  @_implementationOnly
+  struct Nested {
+    var b: OkB
+  }
+}
+
+internal struct NesterStructB {
+  @_implementationOnly
+  struct Nested {
+    var b: OkB
+  }
+}
+
+@_implementationOnly
+internal enum OkInternalEnum {
+  case b(OkB)
+  case c(OkC)
+}
+
+@_implementationOnly
+internal class OkInternalClass {
+  init() { fatalError() }
+  internal var internalField: OkD
+  private var privateField: OkE
+}
+
+@export(interface)
+internal func OkPrivateFunc(h: OkF) {}
+
+public struct PublicStruct {}
+
+@export(interface)
+public func PublicFunc() -> PublicStruct {
+  let _: OkA
+  return PublicStruct()
+}
+
+//--- Client.swift
+
+import MiddleLib
+
+let _ = PublicFunc()