[Dependency Scanning] Improve the cycle-detection diagnostic for Swift overlay dependencies

Add tracing of the underlying Clang module for an overlay dependency if said overlay dependency forms a cycle

include/swift/AST/DiagnosticsCommon.def

@@ -199,7 +199,10 @@ WARNING(cross_imported_by_both_modules, none,
 //------------------------------------------------------------------------------
 
 ERROR(scanner_find_cycle, none,
-      "dependency scanner detected dependency cycle: '%0'", (StringRef))
+      "module dependency cycle: '%0'", (StringRef))
+
+NOTE(scanner_find_cycle_swift_overlay_path, none,
+     "Swift Overlay dependency of '%0' on '%1' via Clang module dependency: '%2'", (StringRef, StringRef, StringRef))
 
 ERROR(scanner_arguments_invalid, none,
       "dependencies scanner cannot be configured with arguments: '%0'", (StringRef))

include/swift/AST/ModuleDependencies.h

@@ -1036,6 +1036,14 @@ public:
   std::vector<ModuleDependencyID>
   getAllDependencies(const ModuleDependencyID &moduleID) const;
 
+  /// Query only direct import dependencies
+  llvm::ArrayRef<ModuleDependencyID>
+  getOnlyDirectDependencies(const ModuleDependencyID &moduleID) const;
+
+  /// Query only Swift overlay dependencies
+  llvm::ArrayRef<ModuleDependencyID>
+  getOnlyOverlayDependencies(const ModuleDependencyID &moduleID) const;
+
   /// Look for module dependencies for a module with the given ID
   ///
   /// \returns the cached result, or \c None if there is no cached entry.

lib/AST/ModuleDependencies.cpp

@@ -795,12 +795,12 @@ void ModuleDependenciesCache::setSwiftOverlayDependencies(ModuleDependencyID mod
 
 std::vector<ModuleDependencyID>
 ModuleDependenciesCache::getAllDependencies(const ModuleDependencyID &moduleID) const {
-  const auto &optionalModuleInfo = findDependency(moduleID);
-  assert(optionalModuleInfo.has_value());
+  const auto &moduleInfo = findDependency(moduleID);
+  assert(moduleInfo.has_value());
   auto directDependenciesRef =
-      optionalModuleInfo.value()->getDirectModuleDependencies();
+      moduleInfo.value()->getDirectModuleDependencies();
   auto overlayDependenciesRef =
-      optionalModuleInfo.value()->getSwiftOverlayDependencies();
+      moduleInfo.value()->getSwiftOverlayDependencies();
   std::vector<ModuleDependencyID> result;
   result.insert(std::end(result), directDependenciesRef.begin(),
                 directDependenciesRef.end());
@@ -808,3 +808,17 @@ ModuleDependenciesCache::getAllDependencies(const ModuleDependencyID &moduleID)
                 overlayDependenciesRef.end());
   return result;
 }
+
+ArrayRef<ModuleDependencyID>
+ModuleDependenciesCache::getOnlyOverlayDependencies(const ModuleDependencyID &moduleID) const {
+  const auto &moduleInfo = findDependency(moduleID);
+  assert(moduleInfo.has_value());
+  return moduleInfo.value()->getSwiftOverlayDependencies();
+}
+
+ArrayRef<ModuleDependencyID>
+ModuleDependenciesCache::getOnlyDirectDependencies(const ModuleDependencyID &moduleID) const {
+  const auto &moduleInfo = findDependency(moduleID);
+  assert(moduleInfo.has_value());
+  return moduleInfo.value()->getDirectModuleDependencies();
+}

lib/DependencyScan/ScanDependencies.cpp

@@ -63,6 +63,7 @@
 #include <set>
 #include <sstream>
 #include <string>
+#include <algorithm>
 
 using namespace swift;
 using namespace swift::dependencies;
@@ -1244,47 +1245,129 @@ computeTransitiveClosureOfExplicitDependencies(
   return result;
 }
 
+static std::vector<ModuleDependencyID>
+findClangDepPath(const ModuleDependencyID &from, const ModuleDependencyID &to,
+                 ModuleDependenciesCache &cache) {
+  std::unordered_set<ModuleDependencyID> visited;
+  std::vector<ModuleDependencyID> result;
+  std::stack<ModuleDependencyID, std::vector<ModuleDependencyID>> stack;
+
+  // Must be explicitly-typed to allow recursion
+  std::function<void(const ModuleDependencyID &)> visit;
+
+  visit = [&visit, &cache, &visited, &result, &stack,
+           to](const ModuleDependencyID &moduleID) {
+    if (!visited.insert(moduleID).second)
+      return;
+
+    if (moduleID == to) {
+      // Copy stack contents to the result
+      auto end = &stack.top() + 1;
+      auto begin = end - stack.size();
+      result.assign(begin, end);
+      return;
+    }
+
+    // Otherwise, visit each child node.
+    for (const auto &succID : cache.getAllDependencies(moduleID)) {
+      stack.push(succID);
+      visit(succID);
+      stack.pop();
+    }
+  };
+
+  stack.push(from);
+  visit(from);
+  return result;
+}
+
 static bool diagnoseCycle(CompilerInstance &instance,
                           ModuleDependenciesCache &cache,
                           ModuleDependencyID mainId) {
   ModuleDependencyIDSetVector openSet;
   ModuleDependencyIDSetVector closeSet;
-  // Start from the main module.
+
+  auto kindIsSwiftDependency = [&](const ModuleDependencyID &ID) {
+    return ID.Kind == swift::ModuleDependencyKind::SwiftInterface ||
+           ID.Kind == swift::ModuleDependencyKind::SwiftBinary;
+  };
+
+  auto emitModulePath = [&](const std::vector<ModuleDependencyID> path,
+                            llvm::SmallString<64> &buffer) {
+    llvm::interleave(
+        path,
+        [&buffer](const ModuleDependencyID &id) {
+          buffer.append(id.ModuleName);
+          switch (id.Kind) {
+          case swift::ModuleDependencyKind::SwiftInterface:
+            buffer.append(".swiftinterface");
+            break;
+          case swift::ModuleDependencyKind::SwiftBinary:
+            buffer.append(".swiftmodule");
+            break;
+          case swift::ModuleDependencyKind::Clang:
+            buffer.append(".pcm");
+            break;
+          default:
+            llvm::report_fatal_error(
+                Twine("Invalid Module Dependency Kind in cycle: ") +
+                id.ModuleName);
+            break;
+          }
+        },
+        [&buffer] { buffer.append(" -> "); });
+  };
+
+  auto emitCycleDiagnostic = [&](const ModuleDependencyID &dep) {
+    auto startIt = std::find(openSet.begin(), openSet.end(), dep);
+    assert(startIt != openSet.end());
+    std::vector cycleNodes(startIt, openSet.end());
+    cycleNodes.push_back(*startIt);
+    llvm::SmallString<64> errorBuffer;
+    emitModulePath(cycleNodes, errorBuffer);
+    instance.getASTContext().Diags.diagnose(
+        SourceLoc(), diag::scanner_find_cycle, errorBuffer.str());
+
+    // TODO: for (std::tuple<const ModuleDependencyID&, const
+    // ModuleDependencyID&> v : cycleNodes | std::views::adjacent<2>)
+    for (auto it = cycleNodes.begin(), end = cycleNodes.end(); it != end;
+         it++) {
+      if (it + 1 == cycleNodes.end())
+        continue;
+
+      const auto &thisID = *it;
+      const auto &nextID = *(it + 1);
+      if (kindIsSwiftDependency(thisID) && kindIsSwiftDependency(nextID) &&
+          llvm::any_of(
+              cache.getOnlyOverlayDependencies(thisID),
+              [&](const ModuleDependencyID id) { return id == nextID; })) {
+        llvm::SmallString<64> noteBuffer;
+        auto clangDepPath = findClangDepPath(
+            thisID,
+            ModuleDependencyID{nextID.ModuleName, ModuleDependencyKind::Clang},
+            cache);
+        emitModulePath(clangDepPath, noteBuffer);
+        instance.getASTContext().Diags.diagnose(
+            SourceLoc(), diag::scanner_find_cycle_swift_overlay_path,
+            thisID.ModuleName, nextID.ModuleName, noteBuffer.str());
+      }
+    }
+  };
+
+  // Start from the main module and check direct and overlay dependencies
   openSet.insert(mainId);
   while (!openSet.empty()) {
-    auto &lastOpen = openSet.back();
+    auto lastOpen = openSet.back();
     auto beforeSize = openSet.size();
     assert(cache.findDependency(lastOpen).has_value() &&
            "Missing dependency info during cycle diagnosis.");
-
     for (const auto &dep : cache.getAllDependencies(lastOpen)) {
       if (closeSet.count(dep))
         continue;
       if (openSet.insert(dep)) {
         break;
       } else {
-        // Find a cycle, diagnose.
-        auto startIt = std::find(openSet.begin(), openSet.end(), dep);
-        assert(startIt != openSet.end());
-        llvm::SmallString<64> buffer;
-        for (auto it = startIt; it != openSet.end(); ++it) {
-          buffer.append(it->ModuleName);
-          buffer.append((it->Kind == ModuleDependencyKind::SwiftInterface ||
-                         it->Kind == ModuleDependencyKind::SwiftSource ||
-                         it->Kind == ModuleDependencyKind::SwiftBinary)
-                            ? ".swiftmodule"
-                            : ".pcm");
-          buffer.append(" -> ");
-        }
-        buffer.append(startIt->ModuleName);
-        buffer.append(
-            (startIt->Kind == ModuleDependencyKind::SwiftInterface ||
-             startIt->Kind == ModuleDependencyKind::SwiftSource ||
-             startIt->Kind == ModuleDependencyKind::SwiftBinary)
-                ? ".swiftmodule"
-                : ".pcm");
-        instance.getASTContext().Diags.diagnose(
-            SourceLoc(), diag::scanner_find_cycle, buffer.str());
+        emitCycleDiagnostic(dep);
         return true;
       }
     }
@@ -1297,6 +1380,7 @@ static bool diagnoseCycle(CompilerInstance &instance,
     }
   }
   assert(openSet.empty());
+  closeSet.clear();
   return false;
 }
 

test/ScanDependencies/Inputs/CHeaders/CycleClangMiddle.h

@@ -0,0 +1 @@
+#include "CycleOverlay.h"

test/ScanDependencies/Inputs/CHeaders/CycleOverlay.h

@@ -0,0 +1 @@
+void funcOverlay(void);

test/ScanDependencies/Inputs/CHeaders/module.modulemap

@@ -56,4 +56,12 @@ module Y {
 module ClangModuleWithOverlayedDep {
   header "ClangModuleWithOverlayedDep.h"
   export *
-}
+}
+module CycleClangMiddle {
+  header "CycleClangMiddle.h"
+  export *
+}
+module CycleOverlay {
+  header "CycleOverlay.h"
+  export *
+}

test/ScanDependencies/Inputs/Swift/CycleOverlay.swiftinterface

@@ -0,0 +1,4 @@
+// swift-interface-format-version: 1.0
+// swift-module-flags: -module-name CycleOverlay
+import Swift
+import CycleSwiftMiddle

test/ScanDependencies/Inputs/Swift/CycleSwiftMiddle.swiftinterface

@@ -0,0 +1,4 @@
+// swift-interface-format-version: 1.0
+// swift-module-flags: -module-name CycleSwiftMiddle
+import Swift
+import CycleClangMiddle

test/ScanDependencies/diagnose_dependency_cycle.swift

@@ -5,6 +5,6 @@
 
 // RUN: %FileCheck %s < %t/out.txt
 
-// CHECK: dependency scanner detected dependency cycle: 'CycleOne.swiftmodule -> CycleTwo.swiftmodule -> CycleThree.swiftmodule -> CycleOne.swiftmodule'
+// CHECK: module dependency cycle: 'CycleOne.swiftinterface -> CycleTwo.swiftinterface -> CycleThree.swiftinterface -> CycleOne.swiftinterface'
 
 import CycleOne

test/ScanDependencies/diagnose_dependency_cycle_overlay.swift

@@ -0,0 +1,10 @@
+// RUN: %empty-directory(%t)
+// RUN: mkdir -p %t/clang-module-cache
+
+// RUN: not %target-swift-frontend -scan-dependencies -module-cache-path %t/clang-module-cache %s -o %t/deps.json -I %S/Inputs/CHeaders -I %S/Inputs/Swift &> %t/out.txt
+// RUN: %FileCheck %s < %t/out.txt
+
+// CHECK: error: module dependency cycle: 'CycleOverlay.swiftinterface -> CycleSwiftMiddle.swiftinterface -> CycleOverlay.swiftinterface'
+// CHECK: note: Swift Overlay dependency of 'CycleSwiftMiddle' on 'CycleOverlay' via Clang module dependency: 'CycleSwiftMiddle.swiftinterface -> CycleClangMiddle.pcm -> CycleOverlay.pcm'
+
+import CycleOverlay