Diagnose modules with circular dependencies (#16075)

This can't arise from a clean build, but it can happen if you have
products lingering in a search path and then either rebuild one of
the modules in the cycle, or change the search paths.

The way this is implemented is for each module to track whether its
imports have all been resolved. If, when loading a module, one of its
dependencies hasn't resolved all of its imports yet, then we know
there's a cycle.

This doesn't produce the best diagnostics, but it's hard to get into
this state in the first place, so that's probably okay.

https://bugs.swift.org/browse/SR-7483

include/swift/AST/DiagnosticsSema.def

@@ -585,6 +585,9 @@ ERROR(serialization_missing_single_dependency,Fatal,
       "missing required module '%0'", (StringRef))
 ERROR(serialization_missing_dependencies,Fatal,
       "missing required modules: %0", (StringRef))
+ERROR(serialization_circular_dependency,Fatal,
+      "circular dependency between modules '%0' and %1",
+      (StringRef, Identifier))
 ERROR(serialization_missing_shadowed_module,Fatal,
       "cannot load underlying module for %0", (Identifier))
 ERROR(serialization_name_mismatch,Fatal,

include/swift/AST/Module.h

@@ -206,6 +206,7 @@ private:
     unsigned TestingEnabled : 1;
     unsigned FailedToLoad : 1;
     unsigned ResilienceStrategy : 1;
+    unsigned HasResolvedImports : 1;
   } Flags;
 
   ModuleDecl(Identifier name, ASTContext &ctx);
@@ -257,6 +258,13 @@ public:
     Flags.FailedToLoad = failed;
   }
 
+  bool hasResolvedImports() const {
+    return Flags.HasResolvedImports;
+  }
+  void setHasResolvedImports() {
+    Flags.HasResolvedImports = true;
+  }
+
   ResilienceStrategy getResilienceStrategy() const {
     return ResilienceStrategy(Flags.ResilienceStrategy);
   }

include/swift/Serialization/Validation.h

@@ -43,6 +43,10 @@ enum class Status {
   /// The module file is an overlay for a Clang module, which can't be found.
   MissingShadowedModule,
 
+  /// The module file depends on a module that is still being loaded, i.e.
+  /// there is a circular dependency.
+  CircularDependency,
+
   /// The module file depends on a bridging header that can't be loaded.
   FailedToLoadBridgingHeader,
 

lib/AST/ASTContext.cpp

@@ -428,6 +428,7 @@ ConstraintCheckerArenaRAII::~ConstraintCheckerArenaRAII() {
 static ModuleDecl *createBuiltinModule(ASTContext &ctx) {
   auto M = ModuleDecl::create(ctx.getIdentifier(BUILTIN_NAME), ctx);
   M->addFile(*new (ctx) BuiltinUnit(*M));
+  M->setHasResolvedImports();
   return M;
 }
 

lib/AST/Module.cpp

@@ -354,7 +354,7 @@ void SourceLookupCache::invalidate() {
 ModuleDecl::ModuleDecl(Identifier name, ASTContext &ctx)
   : DeclContext(DeclContextKind::Module, nullptr),
     TypeDecl(DeclKind::Module, &ctx, name, SourceLoc(), { }),
-    Flags({0, 0, 0}) {
+    Flags() {
   ctx.addDestructorCleanup(*this);
   setImplicit();
   setInterfaceType(ModuleType::get(this));

lib/ClangImporter/ClangImporter.cpp

@@ -1086,6 +1086,7 @@ ClangImporter::create(ASTContext &ctx,
   importer->Impl.ImportedHeaderUnit =
     new (ctx) ClangModuleUnit(*importedHeaderModule, importer->Impl, nullptr);
   importedHeaderModule->addFile(*importer->Impl.ImportedHeaderUnit);
+  importedHeaderModule->setHasResolvedImports();
 
   importer->Impl.IsReadingBridgingPCH = false;
 
@@ -1591,6 +1592,7 @@ ModuleDecl *ClangImporter::Implementation::finishLoadingClangModule(
     result = ModuleDecl::create(name, SwiftContext);
     // Silence error messages about testably importing a Clang module.
     result->setTestingEnabled();
+    result->setHasResolvedImports();
 
     wrapperUnit =
       new (SwiftContext) ClangModuleUnit(*result, *this, clangModule);
@@ -1735,6 +1737,7 @@ ClangModuleUnit *ClangImporter::Implementation::getWrapperForModule(
   auto wrapper = ModuleDecl::create(name, SwiftContext);
   // Silence error messages about testably importing a Clang module.
   wrapper->setTestingEnabled();
+  wrapper->setHasResolvedImports();
 
   auto file = new (SwiftContext) ClangModuleUnit(*wrapper, *this,
                                                  underlying);

lib/Frontend/Frontend.cpp

@@ -614,6 +614,14 @@ void CompilerInstance::parseAndCheckTypes(
                               TypeCheckOptions);
   }
 
+  assert(llvm::all_of(MainModule->getFiles(), [](const FileUnit *File) -> bool {
+    auto *SF = dyn_cast<SourceFile>(File);
+    if (!SF)
+      return true;
+    return SF->ASTStage >= SourceFile::NameBound;
+  }) && "some files have not yet had their imports resolved");
+  MainModule->setHasResolvedImports();
+
   const auto &options = Invocation.getFrontendOptions();
   forEachFileToTypeCheck([&](SourceFile &SF) {
     performTypeChecking(SF, PersistentState.getTopLevelContext(),

lib/Sema/SourceLoader.cpp

@@ -155,6 +155,7 @@ ModuleDecl *SourceLoader::loadModule(SourceLoc importLoc,
   else
     performTypeChecking(*importFile, persistentState.getTopLevelContext(),
                         None);
+  importMod->setHasResolvedImports();
   return importMod;
 }
 

lib/Serialization/ModuleFile.cpp

@@ -1439,6 +1439,13 @@ Status ModuleFile::associateWithFileContext(FileUnit *file,
       dependency.Import = {ctx.AllocateCopy(llvm::makeArrayRef(accessPathElem)),
                            module};
     }
+
+    if (!module->hasResolvedImports()) {
+      // Notice that we check this condition /after/ recording the module that
+      // caused the problem. Clients need to be able to track down what the
+      // cycle was.
+      return error(Status::CircularDependency);
+    }
   }
 
   if (missingDependency) {

lib/Serialization/SerializedModuleLoader.cpp

@@ -15,6 +15,7 @@
 #include "swift/Strings.h"
 #include "swift/AST/ASTContext.h"
 #include "swift/AST/DiagnosticsSema.h"
+#include "swift/Basic/Defer.h"
 #include "swift/Basic/STLExtras.h"
 #include "swift/Basic/SourceManager.h"
 #include "swift/Basic/Version.h"
@@ -316,6 +317,25 @@ FileUnit *SerializedModuleLoader::loadAST(
     break;
   }
 
+  case serialization::Status::CircularDependency: {
+    auto circularDependencyIter =
+        llvm::find_if(loadedModuleFile->getDependencies(),
+                      [](const ModuleFile::Dependency &next) {
+      return !next.Import.second->hasResolvedImports();
+    });
+    assert(circularDependencyIter != loadedModuleFile->getDependencies().end()
+           && "circular dependency reported, but no module with unresolved "
+              "imports found");
+
+    // FIXME: We should include the path of the circularity as well, but that's
+    // hard because we're discovering this /while/ resolving imports, which
+    // means the problematic modules haven't been recorded yet.
+    Ctx.Diags.diagnose(*diagLoc, diag::serialization_circular_dependency,
+                       circularDependencyIter->getPrettyPrintedPath(),
+                       M.getName());
+    break;
+  }
+
   case serialization::Status::MissingShadowedModule: {
     Ctx.Diags.diagnose(*diagLoc, diag::serialization_missing_shadowed_module,
                        M.getName());
@@ -434,6 +454,7 @@ ModuleDecl *SerializedModuleLoader::loadModule(SourceLoc importLoc,
 
   auto M = ModuleDecl::create(moduleID.first, Ctx);
   Ctx.LoadedModules[moduleID.first] = M;
+  SWIFT_DEFER { M->setHasResolvedImports(); };
 
   if (!loadAST(*M, moduleID.second, std::move(moduleInputBuffer),
                std::move(moduleDocInputBuffer), isFramework)) {

test/Serialization/circular-import.swift

@@ -0,0 +1,34 @@
+// Circularities involving the module currently being type-checked.
+
+// RUN: %empty-directory(%t)
+// RUN: %target-swift-frontend -emit-module %s -module-name A -o %t
+// RUN: %target-swift-frontend -emit-module %s -module-name B -D IMPORT_A -I %t -o %t
+// RUN: not %target-swift-frontend -typecheck %s -module-name A -D IMPORT_B -I %t 2>&1 | %FileCheck -check-prefix CHECK-ABA %s
+
+// RUN: %target-swift-frontend -emit-module %s -module-name C -D IMPORT_B -I %t -o %t
+// RUN: not %target-swift-frontend -typecheck %s -module-name A -D IMPORT_C -I %t 2>&1 | %FileCheck -check-prefix CHECK-ABCA %s
+
+// Circularities not involving the module currently being type-checked.
+// RUN: %empty-directory(%t/plain)
+// RUN: %target-swift-frontend -emit-module %s -module-name A -o %t/plain
+// RUN: %target-swift-frontend -emit-module %s -module-name B -o %t/plain
+// RUN: %empty-directory(%t/cycle)
+// RUN: %target-swift-frontend -emit-module %s -module-name A -D IMPORT_B -o %t/cycle -I %t/plain
+// RUN: %target-swift-frontend -emit-module %s -module-name B -D IMPORT_A -o %t/cycle -I %t/plain
+// RUN: not %target-swift-frontend -typecheck %s -module-name C -D IMPORT_A -I %t/cycle 2>&1 | %FileCheck -check-prefix CHECK-ABA-BUILT %s
+
+
+#if IMPORT_A
+import A
+// CHECK-ABA-BUILT: <unknown>:0: error: circular dependency between modules 'A' and 'B'
+#endif
+
+#if IMPORT_B
+import B
+// CHECK-ABA: :[[@LINE-1]]:8: error: circular dependency between modules 'A' and 'B'
+#endif
+
+#if IMPORT_C
+import C
+// CHECK-ABCA: <unknown>:0: error: circular dependency between modules 'A' and 'B'
+#endif

test/SourceKit/Indexing/Inputs/cycle-depend/A.response

@@ -1,105 +0,0 @@
-{
-  key.hash: <hash>,
-  key.dependencies: [
-    {
-      key.kind: source.lang.swift.import.module.swift,
-      key.name: "B",
-      key.filepath: B.swiftmodule,
-      key.hash: <hash>,
-      key.dependencies: [
-        {
-          key.kind: source.lang.swift.import.module.swift,
-          key.name: "A",
-          key.filepath: A.swiftmodule,
-          key.hash: <hash>,
-          key.dependencies: [
-            {
-              key.kind: source.lang.swift.import.module.swift,
-              key.name: "B",
-              key.filepath: B.swiftmodule,
-              key.hash: <hash>
-            },
-            {
-              key.kind: source.lang.swift.import.module.swift,
-              key.name: "Swift",
-              key.filepath: Swift.swiftmodule,
-              key.hash: <hash>,
-              key.is_system: 1
-            },
-            {
-              key.kind: source.lang.swift.import.module.swift,
-              key.name: "SwiftOnoneSupport",
-              key.filepath: SwiftOnoneSupport.swiftmodule,
-              key.hash: <hash>,
-              key.dependencies: [
-                {
-                  key.kind: source.lang.swift.import.module.swift,
-                  key.name: "Swift",
-                  key.filepath: Swift.swiftmodule,
-                  key.hash: <hash>,
-                  key.is_system: 1
-                }
-              ]
-            }
-          ]
-        },
-        {
-          key.kind: source.lang.swift.import.module.swift,
-          key.name: "Swift",
-          key.filepath: Swift.swiftmodule,
-          key.hash: <hash>,
-          key.is_system: 1
-        },
-        {
-          key.kind: source.lang.swift.import.module.swift,
-          key.name: "SwiftOnoneSupport",
-          key.filepath: SwiftOnoneSupport.swiftmodule,
-          key.hash: <hash>
-        }
-      ]
-    },
-    {
-      key.kind: source.lang.swift.import.module.swift,
-      key.name: "Swift",
-      key.filepath: Swift.swiftmodule,
-      key.hash: <hash>,
-      key.is_system: 1
-    },
-    {
-      key.kind: source.lang.swift.import.module.swift,
-      key.name: "SwiftOnoneSupport",
-      key.filepath: SwiftOnoneSupport.swiftmodule,
-      key.hash: <hash>
-    }
-  ],
-  key.entities: [
-    {
-      key.kind: source.lang.swift.decl.class,
-      key.name: "A",
-      key.usr: "s:1AAAC",
-      key.entities: [
-        {
-          key.kind: source.lang.swift.decl.var.instance,
-          key.name: "x",
-          key.usr: "s:1AAAC1x1BADCvp",
-          key.entities: [
-            {
-              key.kind: source.lang.swift.decl.function.accessor.getter,
-              key.usr: "s:1AAAC1x1BADCvg",
-              key.is_dynamic: 1
-            },
-            {
-              key.kind: source.lang.swift.decl.function.accessor.setter,
-              key.usr: "s:1AAAC1x1BADCvs",
-              key.is_dynamic: 1
-            }
-          ]
-        },
-        {
-          key.kind: source.lang.swift.decl.function.constructor,
-          key.usr: "s:1AAACABycfc"
-        }
-      ]
-    }
-  ]
-}

test/SourceKit/Indexing/Inputs/cycle-depend/A.swift

@@ -1,5 +0,0 @@
-import B
-
-public class A {
-  var x: B = B()
-}

test/SourceKit/Indexing/Inputs/cycle-depend/B.swift

@@ -1,6 +0,0 @@
-import A
-
-public class B {
-  var x: A?
-  public init() {}
-}

test/SourceKit/Indexing/index_module_cycle.swift

@@ -1,6 +0,0 @@
-// RUN: %empty-directory(%t)
-// RUN: %swift -emit-module -o %t %S/Inputs/cycle-depend/A.swift -I %S/Inputs/cycle-depend -enable-source-import
-// RUN: %swift -emit-module -o %t %S/Inputs/cycle-depend/B.swift -I %S/Inputs/cycle-depend -enable-source-import
-
-// RUN: %sourcekitd-test -req=index %t/A.swiftmodule -- %t/A.swiftmodule -I %t | %sed_clean > %t.response
-// RUN: diff -u %S/Inputs/cycle-depend/A.response %t.response

test/SourceKit/Indexing/index_module_missing_depend.swift

@@ -1,8 +1,12 @@
 // RUN: %empty-directory(%t)
-// RUN: %swift -emit-module -o %t %S/Inputs/cycle-depend/A.swift -I %S/Inputs/cycle-depend -enable-source-import
+// RUN: %target-swift-frontend -emit-module -o %t %S/../../Inputs/empty.swift
+// RUN: %target-swift-frontend -emit-module -o %t %s -I %t -module-name A
+// RUN: rm %t/empty.swiftmodule
 
 // RUN: not %sourcekitd-test -req=index %t/A.swiftmodule -- %t/A.swiftmodule 2>&1 | %FileCheck %s
 
+import empty
+
 // FIXME: Report the reason we couldn't load a module.
 // CHECK-DISABLED: error response (Request Failed): missing module dependency
 // CHECK: error response (Request Failed): failed to load module

tools/SourceKit/lib/SwiftLang/SwiftIndexing.cpp

@@ -184,6 +184,8 @@ static void indexModule(llvm::MemoryBuffer *Input,
       IdxConsumer.failed("failed to load module");
       return;
     }
+
+    Mod->setHasResolvedImports();
   }
 
   // Setup a typechecker for protocol conformance resolving.