Diagnose modules with circular dependencies

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 @@ class ModuleDecl : public DeclContext, public TypeDecl {
     unsigned TestingEnabled : 1;
     unsigned FailedToLoad : 1;
     unsigned ResilienceStrategy : 1;
+    unsigned HasResolvedImports : 1;
   } Flags;
 
   ModuleDecl(Identifier name, ASTContext &ctx);
@@ -257,6 +258,13 @@ class ModuleDecl : public DeclContext, public TypeDecl {
     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

@@ -1083,6 +1083,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;
 
@@ -1588,6 +1589,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);
@@ -1732,6 +1734,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

@@ -1437,6 +1437,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/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.