Address review comments

rauhul · rauhul · commit f9bb69f6c572 · 2024-05-30T17:30:51.000-07:00
diff --git a/Sources/Build/BuildDescription/ClangTargetBuildDescription.swift b/Sources/Build/BuildDescription/ClangTargetBuildDescription.swift
@@ -207,6 +207,20 @@ public final class ClangTargetBuildDescription {
         }
     }
 
+    /// Determines the arguments needed to run `swift-symbolgraph-extract` for
+    /// this module.
+    public 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)"]
+        }
+        return args
+    }
+
     /// Builds up basic compilation arguments for a source file in this target; these arguments may be different for C++
     /// vs non-C++.
     /// NOTE: The parameter to specify whether to get C++ semantics is currently optional, but this is only for revlock
diff --git a/Sources/Build/BuildDescription/SwiftTargetBuildDescription.swift b/Sources/Build/BuildDescription/SwiftTargetBuildDescription.swift
@@ -552,26 +552,7 @@ public final class SwiftTargetBuildDescription {
             args += ["-color-diagnostics"]
         }
 
-        // If this is a generated test discovery target or a test entry point, it might import a test
-        // target that is built with C++ interop enabled. In that case, the test
-        // discovery target must enable C++ interop as well
-        switch testTargetRole {
-        case .discovery, .entryPoint:
-            for dependency in try self.target.recursiveTargetDependencies() {
-                let dependencyScope = self.defaultBuildParameters.createScope(for: dependency)
-                let dependencySwiftFlags = dependencyScope.evaluate(.OTHER_SWIFT_FLAGS)
-                if let interopModeFlag = dependencySwiftFlags.first(where: { $0.hasPrefix("-cxx-interoperability-mode=") }) {
-                    args += [interopModeFlag]
-                    if interopModeFlag != "-cxx-interoperability-mode=off" {
-                        if let cxxStandard = self.package.manifest.cxxLanguageStandard {
-                            args += ["-Xcc", "-std=\(cxxStandard)"]
-                        }
-                    }
-                    break
-                }
-            }
-        default: break
-        }
+        args += try self.cxxInteroperabilityModeArguments(allowDuplicate: false)
 
         // Add arguments from declared build settings.
         args += try self.buildSettingsFlags()
@@ -649,6 +630,67 @@ public final class SwiftTargetBuildDescription {
 
         return args
     }
+    
+    /// Determines the arguments needed to run `swift-symbolgraph-extract` for
+    /// this module.
+    public func symbolGraphExtractArguments() throws -> [String] {
+        var args = [String]()
+        args += try self.cxxInteroperabilityModeArguments(allowDuplicate: true)
+        return args
+    }
+
+    /// Determines the arguments needed for cxx interop for this module.
+    ///
+    /// If the current module or any of its linked dependencies requires cxx
+    /// interop, cxx interop will be enabled on the current module.
+    func cxxInteroperabilityModeArguments(
+        // FIXME: Remove argument
+        // This argument is added as a stop gap to support generating arguments
+        // for tools which currently dont leverage "OTHER_SWIFT_FLAGS". In the
+        // fullness of time, this function should operate on a strongly typed
+        // "interopMode" property of SwiftTargetBuildDescription. Instead of
+        // digging through "OTHER_SWIFT_FLAGS" manually.
+        allowDuplicate: Bool
+    ) throws -> [String] {
+        func _cxxInteroperabilityMode(for module: ResolvedModule) -> String? {
+            let scope = self.buildParameters.createScope(for: module)
+            let flags = scope.evaluate(.OTHER_SWIFT_FLAGS)
+            return flags.first { $0.hasPrefix("-cxx-interoperability-mode=") }
+        }
+
+        // Look for cxx interop mode in the current module, if set exit early,
+        // the flag is already present.
+        var cxxInteroperabilityMode: String?
+        if let mode = _cxxInteroperabilityMode(for: self.target) {
+            if allowDuplicate {
+                cxxInteroperabilityMode = mode
+            }
+        }
+
+        // If the current module doesn't have cxx interop mode set, search
+        // through the module's dependencies looking for the a module that
+        // enables cxx interop and copy it's flag.
+        if cxxInteroperabilityMode == nil {
+            for module in try self.target.recursiveTargetDependencies() {
+                if let mode = _cxxInteroperabilityMode(for: module) {
+                    cxxInteroperabilityMode = mode
+                    break
+                }
+            }
+        }
+
+        var args = [String]()
+        if let cxxInteroperabilityMode {
+            // FIXME: this should reference a local cxxLanguageStandard property
+            // It definitely should _never_ reach back into the manifest
+            args = [cxxInteroperabilityMode]
+            if let cxxStandard = self.package.manifest.cxxLanguageStandard {
+                args += ["-Xcc", "-std=\(cxxStandard)"]
+            }
+        }
+        return args
+    }
+
 
     /// When `scanInvocation` argument is set to `true`, omit the side-effect producing arguments
     /// such as emitting a module or supplementary outputs.
diff --git a/Sources/Build/BuildDescription/TargetBuildDescription.swift b/Sources/Build/BuildDescription/TargetBuildDescription.swift
@@ -115,4 +115,13 @@ public enum TargetBuildDescription {
             return clangTargetBuildDescription.toolsVersion
         }
     }
+
+    /// Determines the arguments needed to run `swift-symbolgraph-extract` for
+    /// this module.
+    public func symbolGraphExtractArguments() throws -> [String] {
+        switch self {
+        case .swift(let target): try target.symbolGraphExtractArguments()
+        case .clang(let target): try target.symbolGraphExtractArguments()
+        }
+    }
 }
diff --git a/Sources/Build/BuildPlan/BuildPlan.swift b/Sources/Build/BuildPlan/BuildPlan.swift
@@ -660,6 +660,13 @@ public class BuildPlan: SPMBuildCore.BuildPlan {
             try binaryTarget.parseXCFrameworks(for: triple, fileSystem: self.fileSystem)
         }
     }
+
+    public func symbolGraphExtractArguments(for module: ResolvedModule) throws -> [String] {
+        guard let description = self.targetMap[module.id] else {
+            throw InternalError("Expected description for module \(module)")
+        }
+        return try description.symbolGraphExtractArguments()
+    }
 }
 
 extension Basics.Diagnostic {
diff --git a/Sources/Commands/Utilities/SymbolGraphExtract.swift b/Sources/Commands/Utilities/SymbolGraphExtract.swift
@@ -13,7 +13,7 @@
 import ArgumentParser
 import Basics
 import PackageGraph
-@_spi(SwiftPMInternal) import PackageModel
+import PackageModel
 import SPMBuildCore
 
 #if USE_IMPL_ONLY_IMPORTS
@@ -65,6 +65,9 @@ public struct SymbolGraphExtract {
 
         // Construct arguments for extracting symbols for a single target.
         var commandLine = [self.tool.pathString]
+        commandLine += try buildPlan.symbolGraphExtractArguments(for: module)
+
+        // FIXME: everything here should be in symbolGraphExtractArguments
         commandLine += ["-module-name", module.c99name]
         commandLine += try buildParameters.tripleArgs(for: module)
         commandLine += try buildPlan.createAPIToolCommonArgs(includeLibrarySearchPaths: true)
@@ -82,22 +85,7 @@ public struct SymbolGraphExtract {
         if includeSPISymbols {
             commandLine += ["-include-spi-symbols"]
         }
-
-        // If this target or any dependencies is is built with C++ interop
-        // enabled then swift-symbolgraph-extract must also enable C++ interop.
-        var cxxInterop = module.cxxInterop()
-        if !cxxInterop {
-            for module in try module.recursiveTargetDependencies() {
-                if module.cxxInterop() {
-                    cxxInterop = true
-                    break
-                }
-            }
-        }
-        if cxxInterop {
-            commandLine += ["-cxx-interoperability-mode=default"]
-        }
-
+        
         let extensionBlockSymbolsFlag = emitExtensionBlockSymbols ? "-emit-extension-block-symbols" : "-omit-extension-block-symbols"
         if DriverSupport.checkSupportedFrontendFlags(flags: [extensionBlockSymbolsFlag.trimmingCharacters(in: ["-"])], toolchain: buildParameters.toolchain, fileSystem: fileSystem) {
             commandLine += [extensionBlockSymbolsFlag]
@@ -122,14 +110,3 @@ public struct SymbolGraphExtract {
         return try process.waitUntilExit()
     }
 }
-
-extension ResolvedModule {
-    func cxxInterop() -> Bool {
-        for setting in self.underlying.buildSettingsDescription {
-            if case .interoperabilityMode(.Cxx) = setting.kind {
-                return true
-            }
-        }
-        return false
-    }
-}
diff --git a/Sources/SPMBuildCore/BuildSystem/BuildSystem.swift b/Sources/SPMBuildCore/BuildSystem/BuildSystem.swift
@@ -90,6 +90,8 @@ public protocol BuildPlan {
 
     func createAPIToolCommonArgs(includeLibrarySearchPaths: Bool) throws -> [String]
     func createREPLArguments() throws -> [String]
+
+    func symbolGraphExtractArguments(for module: ResolvedModule) throws -> [String]
 }
 
 extension BuildPlan {
diff --git a/Tests/BuildTests/BuildPlanTests.swift b/Tests/BuildTests/BuildPlanTests.swift
@@ -1727,13 +1727,36 @@ final class BuildPlanTests: XCTestCase {
             )
         )
 
+        // Assert compile args for swift modules importing cxx modules
         let swiftInteropLib = try result.target(for: "swiftInteropLib").swiftTarget().compileArguments()
         XCTAssertMatch(
             swiftInteropLib,
             [.anySequence, "-cxx-interoperability-mode=default", "-Xcc", "-std=c++1z", .anySequence]
         )
         let swiftLib = try result.target(for: "swiftLib").swiftTarget().compileArguments()
         XCTAssertNoMatch(swiftLib, [.anySequence, "-Xcc", "-std=c++1z", .anySequence])
+
+        // Assert symbolgraph-extract args for swift modules importing cxx modules
+        do {
+            let swiftInteropLib = try result.target(for: "swiftInteropLib").swiftTarget().compileArguments()
+            XCTAssertMatch(
+                swiftInteropLib,
+                [.anySequence, "-cxx-interoperability-mode=default", "-Xcc", "-std=c++1z", .anySequence]
+            )
+            let swiftLib = try result.target(for: "swiftLib").swiftTarget().compileArguments()
+            XCTAssertNoMatch(swiftLib, [.anySequence, "-Xcc", "-std=c++1z", .anySequence])
+        }
+
+        // Assert symbolgraph-extract args for cxx modules
+        do {
+            let swiftInteropLib = try result.target(for: "swiftInteropLib").swiftTarget().compileArguments()
+            XCTAssertMatch(
+                swiftInteropLib,
+                [.anySequence, "-cxx-interoperability-mode=default", "-Xcc", "-std=c++1z", .anySequence]
+            )
+            let swiftLib = try result.target(for: "swiftLib").swiftTarget().compileArguments()
+            XCTAssertNoMatch(swiftLib, [.anySequence, "-Xcc", "-std=c++1z", .anySequence])
+        }
     }
 
     func testSwiftCMixed() throws {
@@ -1910,6 +1933,81 @@ final class BuildPlanTests: XCTestCase {
         ])
     }
 
+    func testSwiftSettings_interoperabilityMode_cxx() throws {
+        let Pkg: AbsolutePath = "/Pkg"
+
+        let fs: FileSystem = InMemoryFileSystem(
+            emptyFiles:
+            Pkg.appending(components: "Sources", "cxxLib", "lib.cpp").pathString,
+            Pkg.appending(components: "Sources", "cxxLib", "include", "lib.h").pathString,
+            Pkg.appending(components: "Sources", "swiftLib", "lib.swift").pathString,
+            Pkg.appending(components: "Sources", "swiftLib2", "lib2.swift").pathString
+        )
+
+        let observability = ObservabilitySystem.makeForTesting()
+        let graph = try loadModulesGraph(
+            fileSystem: fs,
+            manifests: [
+                Manifest.createRootManifest(
+                    displayName: "Pkg",
+                    path: .init(validating: Pkg.pathString),
+                    cxxLanguageStandard: "c++20",
+                    targets: [
+                        TargetDescription(name: "cxxLib", dependencies: []),
+                        TargetDescription(
+                            name: "swiftLib",
+                            dependencies: ["cxxLib"],
+                            settings: [.init(tool: .swift, kind: .interoperabilityMode(.Cxx))]
+                        ),
+                        TargetDescription(name: "swiftLib2", dependencies: ["swiftLib"]),
+                    ]
+                ),
+            ],
+            observabilityScope: observability.topScope
+        )
+        XCTAssertNoDiagnostics(observability.diagnostics)
+
+        let plan = try BuildPlan(
+            buildParameters: mockBuildParameters(),
+            graph: graph,
+            fileSystem: fs,
+            observabilityScope: observability.topScope
+        )
+        let result = try BuildPlanResult(plan: plan)
+
+        // Cxx module
+        do {
+            try XCTAssertMatch(
+                result.target(for: "cxxLib").clangTarget().symbolGraphExtractArguments(),
+                [.anySequence, "-cxx-interoperability-mode=default", "-Xcc", "-std=c++20", .anySequence]
+            )
+        }
+
+        // Swift module directly importing cxx module
+        do {
+            try XCTAssertMatch(
+                result.target(for: "swiftLib").swiftTarget().compileArguments(),
+                [.anySequence, "-cxx-interoperability-mode=default", "-Xcc", "-std=c++20", .anySequence]
+            )
+            try XCTAssertMatch(
+                result.target(for: "swiftLib").swiftTarget().symbolGraphExtractArguments(),
+                [.anySequence, "-cxx-interoperability-mode=default", "-Xcc", "-std=c++20", .anySequence]
+            )
+        }
+
+        // Swift module transitively importing cxx module
+        do {
+            try XCTAssertMatch(
+                result.target(for: "swiftLib2").swiftTarget().compileArguments(),
+                [.anySequence, "-cxx-interoperability-mode=default", "-Xcc", "-std=c++20", .anySequence]
+            )
+            try XCTAssertMatch(
+                result.target(for: "swiftLib2").swiftTarget().symbolGraphExtractArguments(),
+                [.anySequence, "-cxx-interoperability-mode=default", "-Xcc", "-std=c++20", .anySequence]
+            )
+        }
+    }
+
     func testREPLArguments() throws {
         let Dep = AbsolutePath("/Dep")
         let fs = InMemoryFileSystem(

Original file line number	Diff line number	Diff line change
`@@ -115,4 +115,13 @@ public enum TargetBuildDescription {`
`115`	`115`	`return clangTargetBuildDescription.toolsVersion`
`116`	`116`	`}`
`117`	`117`	`}`
	`118`	`+`
	`119`	+ /// Determines the arguments needed to run `swift-symbolgraph-extract` for
	`120`	`+ /// this module.`
	`121`	`+ public func symbolGraphExtractArguments() throws -> [String] {`
	`122`	`+ switch self {`
	`123`	`+ case .swift(let target): try target.symbolGraphExtractArguments()`
	`124`	`+ case .clang(let target): try target.symbolGraphExtractArguments()`
	`125`	`+ }`
	`126`	`+ }`
`118`	`127`	`}`
Original file line number	Diff line number	Diff line change
`@@ -660,6 +660,13 @@ public class BuildPlan: SPMBuildCore.BuildPlan {`
`660`	`660`	`try binaryTarget.parseXCFrameworks(for: triple, fileSystem: self.fileSystem)`
`661`	`661`	`}`
`662`	`662`	`}`
	`663`	`+`
	`664`	`+ public func symbolGraphExtractArguments(for module: ResolvedModule) throws -> [String] {`
	`665`	`+ guard let description = self.targetMap[module.id] else {`
	`666`	`+ throw InternalError("Expected description for module \(module)")`
	`667`	`+ }`
	`668`	`+ return try description.symbolGraphExtractArguments()`
	`669`	`+ }`
`663`	`670`	`}`
`664`	`671`
`665`	`672`	`extension Basics.Diagnostic {`
Original file line number	Diff line number	Diff line change
`@@ -90,6 +90,8 @@ public protocol BuildPlan {`
`90`	`90`
`91`	`91`	`func createAPIToolCommonArgs(includeLibrarySearchPaths: Bool) throws -> [String]`
`92`	`92`	`func createREPLArguments() throws -> [String]`
	`93`	`+`
	`94`	`+ func symbolGraphExtractArguments(for module: ResolvedModule) throws -> [String]`
`93`	`95`	`}`
`94`	`96`
`95`	`97`	`extension BuildPlan {`