Fix swift-symbolgraph-extract search paths (#7621)

Fixes an architecture and implementation bug where the include paths
for `swift-symbolgraph-extract` were being determined inside build plan
instead of by the target build description. This bug resulted in the
wrong set of paths being determined.

The new code moves the path determination logic into the proper target
build descriptions and releverages the already computed include paths.
The result of this is symbol graphs can be generated for Swift and C
targets properly now without duplicate module in the search paths and
with correct `-fmodule` usage.

Author: rauhul

Sources/Build/BuildDescription/ClangTargetBuildDescription.swift

@@ -209,15 +209,19 @@ public final class ClangTargetBuildDescription {
 
     /// Determines the arguments needed to run `swift-symbolgraph-extract` for
     /// this module.
-    public func symbolGraphExtractArguments() throws -> [String] {
+    package func symbolGraphExtractArguments() throws -> [String] {
         var args = [String]()
-
         if self.clangTarget.isCXX {
             args += ["-cxx-interoperability-mode=default"]
         }
         if let cxxLanguageStandard = self.clangTarget.cxxLanguageStandard {
             args += ["-Xcc", "-std=\(cxxLanguageStandard)"]
         }
+        args += ["-I", self.clangTarget.includeDir.pathString]
+        args += self.additionalFlags.asSwiftcCCompilerFlags()
+        // Unconditionally use clang modules with swift tools.
+        args += try self.clangModuleArguments().asSwiftcCCompilerFlags()
+        args += try self.currentModuleMapFileArguments().asSwiftcCCompilerFlags()
         return args
     }
 
@@ -263,7 +267,7 @@ public final class ClangTargetBuildDescription {
         // clang modules aren't fully supported in C++ mode in the current Darwin SDKs.
         let enableModules = triple.isDarwin() && !isCXX
         if enableModules {
-            args += ["-fmodules", "-fmodule-name=" + target.c99name]
+            args += try self.clangModuleArguments()
         }
 
         // Only add the build path to the framework search path if there are binary frameworks to link against.
@@ -273,9 +277,7 @@ public final class ClangTargetBuildDescription {
 
         args += ["-I", clangTarget.includeDir.pathString]
         args += additionalFlags
-        if enableModules {
-            args += try moduleCacheArgs
-        }
+
         args += buildParameters.sanitizers.compileCFlags()
 
         // Add arguments from declared build settings.
@@ -421,11 +423,22 @@ public final class ClangTargetBuildDescription {
         return compilationConditions
     }
 
-    /// Module cache arguments.
-    private var moduleCacheArgs: [String] {
-        get throws {
-            try ["-fmodules-cache-path=\(buildParameters.moduleCache.pathString)"]
+    /// Enable Clang module flags.
+    private func clangModuleArguments() throws -> [String] {
+        let cachePath = try self.buildParameters.moduleCache.pathString
+        return [
+            "-fmodules",
+            "-fmodule-name=\(self.target.c99name)",
+            "-fmodules-cache-path=\(cachePath)",
+        ]
+    }
+    
+    private func currentModuleMapFileArguments() throws -> [String] {
+        // Pass the path to the current module's module map if present.
+        if let moduleMap = self.moduleMap {
+            return ["-fmodule-map-file=\(moduleMap.pathString)"]
         }
+        return []
     }
 
     /// Generate the resource bundle accessor, if appropriate.

Sources/Build/BuildDescription/SwiftTargetBuildDescription.swift

@@ -623,10 +623,26 @@ public final class SwiftTargetBuildDescription {
 
     /// Determines the arguments needed to run `swift-symbolgraph-extract` for
     /// this module.
-    public func symbolGraphExtractArguments() throws -> [String] {
+    package func symbolGraphExtractArguments() throws -> [String] {
         var args = [String]()
         args += try self.cxxInteroperabilityModeArguments(
             propagateFromCurrentModuleOtherSwiftFlags: true)
+
+        args += self.buildParameters.toolchain.extraFlags.swiftCompilerFlags
+
+        // Include search paths determined during planning
+        args += self.additionalFlags
+        // FIXME: only pass paths to the actual dependencies of the module
+        // Include search paths for swift module dependencies.
+        args += ["-I", self.modulesPath.pathString]
+
+        // FIXME: Only include valid args
+        // This condition should instead only include args which are known to be
+        // compatible instead of filtering out specific unknown args.
+        //
+        // swift-symbolgraph-extract does not support parsing `-use-ld=lld` and
+        // will silently error failing the operation.
+        args = args.filter { !$0.starts(with: "-use-ld=") }
         return args
     }
 

Sources/Build/BuildDescription/TargetBuildDescription.swift

@@ -118,7 +118,7 @@ public enum TargetBuildDescription {
 
     /// Determines the arguments needed to run `swift-symbolgraph-extract` for
     /// this module.
-    public func symbolGraphExtractArguments() throws -> [String] {
+    package func symbolGraphExtractArguments() throws -> [String] {
         switch self {
         case .swift(let target): try target.symbolGraphExtractArguments()
         case .clang(let target): try target.symbolGraphExtractArguments()

Sources/Build/BuildPlan/BuildPlan.swift

@@ -648,6 +648,8 @@ public class BuildPlan: SPMBuildCore.BuildPlan {
         }
     }
 
+    /// Determines the arguments needed to run `swift-symbolgraph-extract` for
+    /// a particular module.
     public func symbolGraphExtractArguments(for module: ResolvedModule) throws -> [String] {
         guard let description = self.targetMap[module.id] else {
             throw InternalError("Expected description for module \(module)")

Sources/Commands/Utilities/SymbolGraphExtract.swift

@@ -70,7 +70,6 @@ public struct SymbolGraphExtract {
         // FIXME: everything here should be in symbolGraphExtractArguments
         commandLine += ["-module-name", module.c99name]
         commandLine += try buildParameters.tripleArgs(for: module)
-        commandLine += try buildPlan.createAPIToolCommonArgs(includeLibrarySearchPaths: true)
         commandLine += ["-module-cache-path", try buildParameters.moduleCache.pathString]
         if verboseOutput {
             commandLine += ["-v"]

Sources/SPMBuildCore/BuildSystem/BuildSystem.swift

@@ -91,6 +91,8 @@ public protocol BuildPlan {
     func createAPIToolCommonArgs(includeLibrarySearchPaths: Bool) throws -> [String]
     func createREPLArguments() throws -> [String]
 
+    /// Determines the arguments needed to run `swift-symbolgraph-extract` for
+    /// a particular module.
     func symbolGraphExtractArguments(for module: ResolvedModule) throws -> [String]
 }
 

Tests/BuildTests/BuildPlanTests.swift

@@ -1408,13 +1408,13 @@ final class BuildPlanTests: XCTestCase {
         args += ["-O0", "-DSWIFT_PACKAGE=1", "-DDEBUG=1"]
         args += ["-fblocks"]
         #if os(macOS) // FIXME(5473) - support modules on non-Apple platforms
-        args += ["-fmodules", "-fmodule-name=extlib"]
+        args += [
+            "-fmodules",
+            "-fmodule-name=extlib",
+            "-fmodules-cache-path=\(buildPath.appending(components: "ModuleCache"))",
+        ]
         #endif
         args += ["-I", ExtPkg.appending(components: "Sources", "extlib", "include").pathString]
-        #if os(macOS) // FIXME(5473) - support modules on non-Apple platforms
-        args += ["-fmodules-cache-path=\(buildPath.appending(components: "ModuleCache"))"]
-        #endif
-
         args += [hostTriple.isWindows() ? "-gdwarf" : "-g"]
 
         if hostTriple.isLinux() {
@@ -1437,7 +1437,11 @@ final class BuildPlanTests: XCTestCase {
         args += ["-O0", "-DSWIFT_PACKAGE=1", "-DDEBUG=1"]
         args += ["-fblocks"]
         #if os(macOS) // FIXME(5473) - support modules on non-Apple platforms
-        args += ["-fmodules", "-fmodule-name=exe"]
+        args += [
+            "-fmodules",
+            "-fmodule-name=exe",
+            "-fmodules-cache-path=\(buildPath.appending(components: "ModuleCache"))",
+        ]
         #endif
         args += [
             "-I", Pkg.appending(components: "Sources", "exe", "include").pathString,
@@ -1446,9 +1450,6 @@ final class BuildPlanTests: XCTestCase {
             "-I", ExtPkg.appending(components: "Sources", "extlib", "include").pathString,
             "-fmodule-map-file=\(buildPath.appending(components: "extlib.build", "module.modulemap"))",
         ]
-        #if os(macOS) // FIXME(5473) - support modules on non-Apple platforms
-        args += ["-fmodules-cache-path=\(buildPath.appending(components: "ModuleCache"))"]
-        #endif
         args += [hostTriple.isWindows() ? "-gdwarf" : "-g"]
 
         if hostTriple.isLinux() {
@@ -1794,12 +1795,13 @@ final class BuildPlanTests: XCTestCase {
         args += ["-O0", "-DSWIFT_PACKAGE=1", "-DDEBUG=1"]
         args += ["-fblocks"]
         #if os(macOS) // FIXME(5473) - support modules on non-Apple platforms
-        args += ["-fmodules", "-fmodule-name=lib"]
+        args += [
+            "-fmodules",
+            "-fmodule-name=lib",
+            "-fmodules-cache-path=\(buildPath.appending(components: "ModuleCache"))",
+        ]
         #endif
         args += ["-I", Pkg.appending(components: "Sources", "lib", "include").pathString]
-        #if os(macOS) // FIXME(5473) - support modules on non-Apple platforms
-        args += ["-fmodules-cache-path=\(buildPath.appending(components: "ModuleCache"))"]
-        #endif
         args += [hostTriple.isWindows() ? "-gdwarf" : "-g"]
 
         if hostTriple.isLinux() {
@@ -1997,6 +1999,115 @@ final class BuildPlanTests: XCTestCase {
         }
     }
 
+    func test_symbolGraphExtract_arguments() throws {
+        // ModuleGraph:
+        // .
+        // ├── A (Swift)
+        // │   ├── B (Swift)
+        // │   └── C (C)
+        // └── D (C)
+        //     ├── B (Swift)
+        //     └── C (C)
+
+        let Pkg: AbsolutePath = "/Pkg"
+        let fs: FileSystem = InMemoryFileSystem(
+            emptyFiles:
+            // A
+            Pkg.appending(components: "Sources", "A", "A.swift").pathString,
+            // B
+            Pkg.appending(components: "Sources", "B", "B.swift").pathString,
+            // C
+            Pkg.appending(components: "Sources", "C", "C.c").pathString,
+            Pkg.appending(components: "Sources", "C", "include", "C.h").pathString,
+            // D
+            Pkg.appending(components: "Sources", "D", "D.c").pathString,
+            Pkg.appending(components: "Sources", "D", "include", "D.h").pathString
+        )
+
+        let observability = ObservabilitySystem.makeForTesting()
+        let graph = try loadModulesGraph(
+            fileSystem: fs,
+            manifests: [
+                Manifest.createRootManifest(
+                    displayName: "Pkg",
+                    path: .init(validating: Pkg.pathString),
+                    targets: [
+                        TargetDescription(name: "A", dependencies: ["B", "C"]),
+                        TargetDescription(name: "B", dependencies: []),
+                        TargetDescription(name: "C", dependencies: []),
+                        TargetDescription(name: "D", dependencies: ["B", "C"]),
+                    ]
+                ),
+            ],
+            observabilityScope: observability.topScope
+        )
+        XCTAssertNoDiagnostics(observability.diagnostics)
+
+        let plan = try mockBuildPlan(
+            graph: graph,
+            fileSystem: fs,
+            observabilityScope: observability.topScope
+        )
+
+        let result = try BuildPlanResult(plan: plan)
+        let triple = result.plan.destinationBuildParameters.triple
+
+        func XCTAssertMatchesSubSequences(
+            _ value: [String],
+            _ patterns: [StringPattern]...,
+            file: StaticString = #file,
+            line: UInt = #line
+        ) {
+            for pattern in patterns {
+                var pattern = pattern
+                pattern.insert(.anySequence, at: 0)
+                pattern.append(.anySequence)
+                XCTAssertMatch(value, pattern, file: file, line: line)
+            }
+        }
+
+        // A
+        do {
+            try XCTAssertMatchesSubSequences(
+                result.target(for: "A").symbolGraphExtractArguments(),
+                // Swift Module dependencies
+                ["-I", "/path/to/build/\(triple)/debug/Modules"],
+                // C Module dependencies
+                ["-Xcc", "-I", "-Xcc", "/Pkg/Sources/C/include"],
+                ["-Xcc", "-fmodule-map-file=/path/to/build/\(triple)/debug/C.build/module.modulemap"]
+            )
+        }
+
+        // D
+        do {
+            try XCTAssertMatchesSubSequences(
+                result.target(for: "D").symbolGraphExtractArguments(),
+                // Self Module
+                ["-I", "/Pkg/Sources/D/include"],
+                ["-Xcc", "-fmodule-map-file=/path/to/build/\(triple)/debug/D.build/module.modulemap"],
+
+                // C Module dependencies
+                ["-Xcc", "-I", "-Xcc", "/Pkg/Sources/C/include"],
+                ["-Xcc", "-fmodule-map-file=/path/to/build/\(triple)/debug/C.build/module.modulemap"],
+
+                // General Args
+                [
+                    "-Xcc", "-fmodules",
+                    "-Xcc", "-fmodule-name=D",
+                    "-Xcc", "-fmodules-cache-path=/path/to/build/\(triple)/debug/ModuleCache",
+                ]
+            )
+
+#if os(macOS)
+            try XCTAssertMatchesSubSequences(
+                result.target(for: "D").symbolGraphExtractArguments(),
+                // Swift Module dependencies
+                ["-Xcc", "-fmodule-map-file=/path/to/build/\(triple)/debug/B.build/module.modulemap"]
+            )
+#endif
+        }
+    }
+
     func testREPLArguments() throws {
         let Dep = AbsolutePath("/Dep")
         let fs = InMemoryFileSystem(
@@ -3033,12 +3144,13 @@ final class BuildPlanTests: XCTestCase {
         expectedExeBasicArgs += ["-target", defaultTargetTriple]
         expectedExeBasicArgs += ["-O0", "-DSWIFT_PACKAGE=1", "-DDEBUG=1", "-fblocks"]
         #if os(macOS) // FIXME(5473) - support modules on non-Apple platforms
-        expectedExeBasicArgs += ["-fmodules", "-fmodule-name=exe"]
+        expectedExeBasicArgs += [
+            "-fmodules",
+            "-fmodule-name=exe",
+            "-fmodules-cache-path=\(buildPath.appending(components: "ModuleCache"))"
+        ]
         #endif
         expectedExeBasicArgs += ["-I", Pkg.appending(components: "Sources", "exe", "include").pathString]
-        #if os(macOS) // FIXME(5473) - support modules on non-Apple platforms
-        expectedExeBasicArgs += ["-fmodules-cache-path=\(buildPath.appending(components: "ModuleCache"))"]
-        #endif
 
         expectedExeBasicArgs += [triple.isWindows() ? "-gdwarf" : "-g"]