Handle cross-language duplications for missing dependencies (#696)

Instead of emitting diagnostics separately per language, this emits them together by first computing missing dependencies for each language and then merging/deduplicating them before diagnostic generation.

The code here will be forward compatible when we extend the Clang implementation to respect access levels and/or provide import locations.

Author: neonichu

Sources/SWBCore/Dependencies.swift

@@ -17,7 +17,7 @@ public struct ModuleDependency: Hashable, Sendable, SerializableCodable {
     public let name: String
     public let accessLevel: AccessLevel
 
-    public enum AccessLevel: String, Hashable, Sendable, CaseIterable, Codable, Serializable {
+    public enum AccessLevel: String, Hashable, Sendable, CaseIterable, Codable, Serializable, Comparable {
         case Private = "private"
         case Package = "package"
         case Public = "public"
@@ -29,6 +29,18 @@ public struct ModuleDependency: Hashable, Sendable, SerializableCodable {
 
             self = accessLevel
         }
+
+        // This allows easy merging of different access levels to always end up with the broadest one needed for a target.
+        public static func < (lhs: Self, rhs: Self) -> Bool {
+            switch lhs {
+            case .Private:
+                return true
+            case .Public:
+                return false
+            case .Package:
+                return rhs == .Public
+            }
+        }
     }
 
     public init(name: String, accessLevel: AccessLevel) {
@@ -80,18 +92,15 @@ public struct ModuleDependenciesContext: Sendable, SerializableCodable {
         self.init(validate: validate, moduleDependencies: settings.moduleDependencies, fixItContext: fixItContext)
     }
 
-    /// Make diagnostics for missing module dependencies from Clang imports.
+    /// Compute missing module dependencies from Clang imports.
     ///
     /// The compiler tracing information does not provide the import locations or whether they are public imports
     /// (which depends on whether the import is in an installed header file).
     /// If `files` is nil, the current toolchain does support the feature to trace imports.
-    public func makeDiagnostics(files: [Path]?) -> [Diagnostic] {
+    public func computeMissingDependencies(files: [Path]?) -> [(ModuleDependency, importLocations: [Diagnostic.Location])]? {
         guard validate != .no else { return [] }
         guard let files else {
-            return [Diagnostic(
-                behavior: .warning,
-                location: .unknown,
-                data: DiagnosticData("The current toolchain does not support \(BuiltinMacros.VALIDATE_MODULE_DEPENDENCIES.name)"))]
+            return nil
         }
 
         // The following is a provisional/incomplete mechanism for resolving a module dependency from a file path.
@@ -111,55 +120,45 @@ public struct ModuleDependenciesContext: Sendable, SerializableCodable {
             ModuleDependency(name: $0, accessLevel: .Private)
         })
 
-        guard !missingDeps.isEmpty else { return [] }
-
-        let behavior: Diagnostic.Behavior = validate == .yesError ? .error : .warning
-
-        let fixIt = fixItContext?.makeFixIt(newModules: Array(missingDeps))
-        let fixIts = fixIt.map { [$0] } ?? []
-
-        let message = "Missing entries in \(BuiltinMacros.MODULE_DEPENDENCIES.name): \(missingDeps.map { $0.asBuildSettingEntryQuotedIfNeeded }.sorted().joined(separator: " "))"
-
-        let location: Diagnostic.Location = fixIt.map {
-            Diagnostic.Location.path($0.sourceRange.path, line: $0.sourceRange.endLine, column: $0.sourceRange.endColumn)
-        } ?? Diagnostic.Location.buildSetting(BuiltinMacros.MODULE_DEPENDENCIES)
-
-        return [Diagnostic(
-            behavior: behavior,
-            location: location,
-            data: DiagnosticData(message),
-            fixIts: fixIts)]
+        return missingDeps.map { ($0, []) }
     }
 
-    /// Make diagnostics for missing module dependencies from Swift imports.
+    /// Compute missing module dependencies from Swift imports.
     ///
     /// If `imports` is nil, the current toolchain does not support the features to gather imports.
-    public func makeDiagnostics(imports: [(ModuleDependency, importLocations: [Diagnostic.Location])]?) -> [Diagnostic] {
+    public func computeMissingDependencies(imports: [(ModuleDependency, importLocations: [Diagnostic.Location])]?) -> [(ModuleDependency, importLocations: [Diagnostic.Location])]? {
         guard validate != .no else { return [] }
         guard let imports else {
-            return [Diagnostic(
-                behavior: .warning,
-                location: .unknown,
-                data: DiagnosticData("The current toolchain does not support \(BuiltinMacros.VALIDATE_MODULE_DEPENDENCIES.name)"))]
+            return nil
         }
 
-        let missingDeps = imports.filter {
+        return imports.filter {
             // ignore module deps without source locations, these are inserted by swift / swift-build and we should treat them as implementation details which we can track without needing the user to declare them
             if $0.importLocations.isEmpty { return false }
 
             // TODO: if the difference is just the access modifier, we emit a new entry, but ultimately our fixit should update the existing entry or emit an error about a conflict
             if moduleDependencies.contains($0.0) { return false }
             return true
         }
+    }
+
+    /// Make diagnostics for missing module dependencies.
+    public func makeDiagnostics(missingDependencies: [(ModuleDependency, importLocations: [Diagnostic.Location])]?) -> [Diagnostic] {
+        guard let missingDependencies else {
+            return [Diagnostic(
+                behavior: .warning,
+                location: .unknown,
+                data: DiagnosticData("The current toolchain does not support \(BuiltinMacros.VALIDATE_MODULE_DEPENDENCIES.name)"))]
+        }
 
-        guard !missingDeps.isEmpty else { return [] }
+        guard !missingDependencies.isEmpty else { return [] }
 
         let behavior: Diagnostic.Behavior = validate == .yesError ? .error : .warning
 
-        let fixIt = fixItContext?.makeFixIt(newModules: missingDeps.map { $0.0 })
+        let fixIt = fixItContext?.makeFixIt(newModules: missingDependencies.map { $0.0 })
         let fixIts = fixIt.map { [$0] } ?? []
 
-        let importDiags: [Diagnostic] = missingDeps
+        let importDiags: [Diagnostic] = missingDependencies
             .flatMap { dep in
                 dep.1.map {
                     return Diagnostic(
@@ -170,7 +169,7 @@ public struct ModuleDependenciesContext: Sendable, SerializableCodable {
                 }
             }
 
-        let message = "Missing entries in \(BuiltinMacros.MODULE_DEPENDENCIES.name): \(missingDeps.map { $0.0.asBuildSettingEntryQuotedIfNeeded }.sorted().joined(separator: " "))"
+        let message = "Missing entries in \(BuiltinMacros.MODULE_DEPENDENCIES.name): \(missingDependencies.map { $0.0.asBuildSettingEntryQuotedIfNeeded }.sorted().joined(separator: " "))"
 
         let location: Diagnostic.Location = fixIt.map {
             Diagnostic.Location.path($0.sourceRange.path, line: $0.sourceRange.endLine, column: $0.sourceRange.endColumn)

Sources/SWBTaskExecution/TaskActions/ValidateDependenciesTaskAction.swift

@@ -36,8 +36,8 @@ public final class ValidateDependenciesTaskAction: TaskAction {
         }
 
         do {
-            var allFiles = Set<String>()
-            var allImports = Set<DependencyValidationInfo.Import>()
+            var allClangFiles = Set<String>()
+            var allSwiftImports = Set<DependencyValidationInfo.Import>()
             var unsupported = false
 
             for inputPath in task.inputPaths {
@@ -47,11 +47,11 @@ public final class ValidateDependenciesTaskAction: TaskAction {
                 switch info.payload {
                 case .clangDependencies(let files):
                     files.forEach {
-                        allFiles.insert($0)
+                        allClangFiles.insert($0)
                     }
                 case .swiftDependencies(let imports):
                     imports.forEach {
-                        allImports.insert($0)
+                        allSwiftImports.insert($0)
                     }
                 case .unsupported:
                     unsupported = true
@@ -61,10 +61,32 @@ public final class ValidateDependenciesTaskAction: TaskAction {
             var diagnostics: [Diagnostic] = []
 
             if unsupported {
-                diagnostics.append(contentsOf: context.makeDiagnostics(files: nil))
+                diagnostics.append(contentsOf: context.makeDiagnostics(missingDependencies: nil))
             } else {
-                diagnostics.append(contentsOf: context.makeDiagnostics(files: allFiles.map { Path($0) }))
-                diagnostics.append(contentsOf: context.makeDiagnostics(imports: allImports.map { ($0.dependency, $0.importLocations) }))
+                let clangMissingDeps = context.computeMissingDependencies(files: allClangFiles.map { Path($0) })
+                let swiftMissingDeps = context.computeMissingDependencies(imports: allSwiftImports.map { ($0.dependency, $0.importLocations) })
+
+                // Update Swift dependencies with information from Clang dependencies on the same module.
+                var clangMissingDepsByName = [String: (ModuleDependency, importLocations: [Diagnostic.Location])]()
+                clangMissingDeps?.forEach {
+                    clangMissingDepsByName[$0.0.name] = $0
+                }
+                let updatedSwiftMissingDeps: [(ModuleDependency, importLocations: [Diagnostic.Location])] = swiftMissingDeps?.map {
+                    if let clangMissingDep = clangMissingDepsByName[$0.0.name] {
+                        return (
+                            ModuleDependency(name: $0.0.name, accessLevel: max($0.0.accessLevel, clangMissingDep.0.accessLevel)),
+                            $0.importLocations + clangMissingDep.importLocations
+                        )
+                    } else {
+                        return $0
+                    }
+                } ?? []
+
+                // Filter missing C dependencies by known Swift dependencies to avoid duplicate diagnostics.
+                let swiftImports = Set(allSwiftImports.map { $0.dependency.name })
+                let uniqueClangMissingDeps = clangMissingDeps?.filter { !swiftImports.contains($0.0.name) } ?? []
+
+                diagnostics.append(contentsOf: context.makeDiagnostics(missingDependencies: uniqueClangMissingDeps + updatedSwiftMissingDeps))
             }
 
             for diagnostic in diagnostics {

Sources/SWBTestSupport/CoreBasedTests.swift

@@ -155,6 +155,14 @@ extension CoreBasedTests {
         }
     }
 
+    /// The path to clang in the default toolchain.
+    public var defaultClangPath: Path {
+        get async throws {
+            let clangInfo = try await clangInfo
+            return clangInfo.toolPath
+        }
+    }
+
     /// The path to the libClang.dylib compiler in the default toolchain.
     package var libClangPath: Path {
         get async throws {

Tests/SWBBuildSystemTests/DependencyValidationTests.swift

@@ -19,6 +19,23 @@ import SWBMacro
 
 @Suite
 fileprivate struct DependencyValidationTests: CoreBasedTests {
+    @Test
+    func moduleDependencyAccessLevelComparable() async throws {
+        #expect(ModuleDependency.AccessLevel.Private == .Private)
+        #expect(ModuleDependency.AccessLevel.Private < .Package)
+        #expect(ModuleDependency.AccessLevel.Private < .Public)
+
+        #expect(ModuleDependency.AccessLevel.Package == .Package)
+        #expect(ModuleDependency.AccessLevel.Package < .Public)
+        #expect(ModuleDependency.AccessLevel.Package > .Private)
+
+        #expect(ModuleDependency.AccessLevel.Public == .Public)
+        #expect(ModuleDependency.AccessLevel.Public > .Private)
+        #expect(ModuleDependency.AccessLevel.Public > .Package)
+
+        #expect(max(ModuleDependency.AccessLevel.Public, ModuleDependency.AccessLevel.Private) == .Public)
+    }
+
     @Test(.requireSDKs(.macOS))
     func dependencyValidation() async throws {
         try await testDependencyValidation(BuildParameters(configuration: "Debug"))
@@ -529,4 +546,78 @@ fileprivate struct DependencyValidationTests: CoreBasedTests {
         }
     }
 
+    @Test(.requireSDKs(.host), .requireClangFeatures(.printHeadersDirectPerFile), .skipHostOS(.windows, "toolchain too old"), .skipHostOS(.linux, "toolchain too old"))
+    func validateModuleDependenciesMixedSource() async throws {
+        try await withTemporaryDirectory { tmpDir async throws -> Void in
+            let testWorkspace = try await TestWorkspace(
+                "Test",
+                sourceRoot: tmpDir.join("Test"),
+                projects: [
+                    TestProject(
+                        "aProject",
+                        groupTree: TestGroup(
+                            "Sources", path: "Sources",
+                            children: [
+                                TestFile("CoreFoo.m"),
+                                TestFile("Swift.swift"),
+                            ]),
+                        buildConfigurations: [
+                            TestBuildConfiguration(
+                                "Debug",
+                                buildSettings: [
+                                    "PRODUCT_NAME": "$(TARGET_NAME)",
+                                    "CLANG_ENABLE_MODULES": "YES",
+                                    "CLANG_ENABLE_EXPLICIT_MODULES": "YES",
+                                    "SWIFT_ENABLE_EXPLICIT_MODULES": "YES",
+                                    "SWIFT_UPCOMING_FEATURE_INTERNAL_IMPORTS_BY_DEFAULT": "YES",
+                                    "SWIFT_VERSION": swiftVersion,
+                                    "GENERATE_INFOPLIST_FILE": "YES",
+                                    "VALIDATE_MODULE_DEPENDENCIES": "YES_ERROR",
+                                    "SDKROOT": "$(HOST_PLATFORM)",
+                                    "SUPPORTED_PLATFORMS": "$(HOST_PLATFORM)",
+                                    "DSTROOT": tmpDir.join("dstroot").str,
+
+                                    // Temporarily override to use the latest toolchain in CI because we depend on swift and swift-driver changes which aren't in the baseline tools yet
+                                    "TOOLCHAINS": "swift",
+                                    // We still want to use the default clang since that is used to gate the test
+                                    "CC": defaultClangPath.str,
+                                ]
+                            )
+                        ],
+                        targets: [
+                            TestStandardTarget(
+                                "CoreFoo", type: .framework,
+                                buildPhases: [
+                                    TestSourcesBuildPhase(["CoreFoo.m", "Swift.swift"]),
+                                    TestFrameworksBuildPhase()
+                                ])
+                        ])
+                ]
+            )
+
+            let tester = try await BuildOperationTester(getCore(), testWorkspace, simulated: false)
+            let SRCROOT = testWorkspace.sourceRoot.join("aProject")
+
+            try await tester.fs.writeFileContents(SRCROOT.join("Sources/Swift.swift")) { stream in
+                stream <<< """
+                    import Foundation
+                    import AppKit
+                """
+            }
+
+            try await tester.fs.writeFileContents(SRCROOT.join("Sources/CoreFoo.m")) { contents in
+                contents <<< """
+                    #include <Foundation/Foundation.h>
+                    #include <Accelerate/Accelerate.h>
+
+                    void f(void) { };
+                """
+            }
+
+            try await tester.checkBuild(parameters: BuildParameters(configuration: "Debug"), runDestination: .host, persistent: true) { results in
+                results.checkError(.contains("Missing entries in MODULE_DEPENDENCIES: Accelerate AppKit Foundation"))
+            }
+        }
+    }
+
 }