Refactor diagnostic generation

neonichu · neonichu · commit 49075cb16346 · 2025-08-01T11:07:14.000-07:00
Instead of emitting diagnostics separately per language, this emits them together by first computing missing dependencies for each language and then merging and 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.
diff --git a/Sources/SWBCore/Dependencies.swift b/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) {
@@ -85,7 +97,7 @@ public struct ModuleDependenciesContext: Sendable, SerializableCodable {
     /// 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 computeMissingDependencies(files: [Path]?) -> [ModuleDependency]? {
+    public func computeMissingDependencies(files: [Path]?) -> [(ModuleDependency, importLocations: [Diagnostic.Location])]? {
         guard validate != .no else { return [] }
         guard let files else {
             return nil
@@ -108,67 +120,45 @@ public struct ModuleDependenciesContext: Sendable, SerializableCodable {
             ModuleDependency(name: $0, accessLevel: .Private)
         })
 
-        return Array(missingDeps)
+        return missingDeps.map { ($0, []) }
     }
 
-    /// Make diagnostics for missing module dependencies from Clang imports.
-    public func makeDiagnostics(missingDependencies: [ModuleDependency]?) -> [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 !missingDependencies.isEmpty else { return [] }
-
-        let behavior: Diagnostic.Behavior = validate == .yesError ? .error : .warning
-
-        let fixIt = fixItContext?.makeFixIt(newModules: Array(missingDependencies))
-        let fixIts = fixIt.map { [$0] } ?? []
-
-        let message = "Missing entries in \(BuiltinMacros.MODULE_DEPENDENCIES.name): \(missingDependencies.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)]
-    }
-
-    /// 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(
@@ -179,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)
diff --git a/Sources/SWBTaskExecution/TaskActions/ValidateDependenciesTaskAction.swift b/Sources/SWBTaskExecution/TaskActions/ValidateDependenciesTaskAction.swift
@@ -63,14 +63,30 @@ public final class ValidateDependenciesTaskAction: TaskAction {
             if unsupported {
                 diagnostics.append(contentsOf: context.makeDiagnostics(missingDependencies: nil))
             } else {
-                // Filter missing C dependencies by known Swift dependencies to avoid duplicate diagnostics between the two.
-                let swiftImports = allImports.map { $0.dependency.name }
-                let missingDependencies = context.computeMissingDependencies(files: allFiles.map { Path($0) })?.filter {
-                    !swiftImports.contains($0.name)
+                let clangMissingDeps = context.computeMissingDependencies(files: allFiles.map { Path($0) })
+                let swiftMissingDeps = context.computeMissingDependencies(imports: allImports.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 = allImports.map { $0.dependency.name }
+                let uniqueClangMissingDeps = clangMissingDeps?.filter { !swiftImports.contains($0.0.name) } ?? []
 
-                diagnostics.append(contentsOf: context.makeDiagnostics(missingDependencies: missingDependencies))
-                diagnostics.append(contentsOf: context.makeDiagnostics(imports: allImports.map { ($0.dependency, $0.importLocations) }))
+                diagnostics.append(contentsOf: context.makeDiagnostics(missingDependencies: uniqueClangMissingDeps + updatedSwiftMissingDeps))
             }
 
             for diagnostic in diagnostics {
diff --git a/Tests/SWBBuildSystemTests/DependencyValidationTests.swift b/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"))
