swift-package-migrate: Call SwiftFixIt once for all diagnostic files

Sources/Commands/PackageCommands/Migrate.swift

@@ -90,44 +90,66 @@ extension SwiftPackageCommand {
                 features.append(feature)
             }
 
-            var targets = self.options.targets
-
             let buildSystem = try await createBuildSystem(
                 swiftCommandState,
-                targets: targets,
+                targets: self.options.targets,
                 features: features
             )
 
-            // Next, let's build all of the individual targets or the
-            // whole project to get diagnostic files.
-            print("> Starting the build")
+            // Compute the set of targets to migrate.
+            let targetsToMigrate: Set<String>
+            if self.options.targets.isEmpty {
+                let graph = try await buildSystem.getPackageGraph()
+                targetsToMigrate = Set(
+                    graph.rootPackages.lazy.map { package in
+                        package.manifest.targets.lazy.filter { target in
+                            // FIXME: Plugin target init does not have Swift settings.
+                            // Exclude them from migration.
+                            target.type != .plugin
+                        }.map(\.name)
+                    }.joined()
+                )
+            } else {
+                targetsToMigrate = Set(self.options.targets.elements)
+            }
 
-            var diagnosticsPaths: [String: [AbsolutePath]] = [:]
-            if !targets.isEmpty {
-                for target in targets {
-                    let buildResult = try await buildSystem.build(subset: .target(target), buildOutputs: [])
-                    diagnosticsPaths.merge(try buildResult.serializedDiagnosticPathsByTargetName.get(), uniquingKeysWith: { $0 + $1 })
+            // Next, let's build the requested targets or, if none were given,
+            // the whole project to get diagnostic files.
+            print("> Starting the build")
+            var diagnosticFiles: [[AbsolutePath]] = []
+            if self.options.targets.isEmpty {
+                // No targets were requested. Build everything.
+                let buildResult = try await buildSystem.build(subset: .allIncludingTests, buildOutputs: [])
+                for (target, files) in try buildResult.serializedDiagnosticPathsByTargetName.get() {
+                    if targetsToMigrate.contains(target) {
+                        diagnosticFiles.append(files)
+                    }
                 }
             } else {
-                diagnosticsPaths = try await buildSystem.build(subset: .allIncludingTests, buildOutputs: []).serializedDiagnosticPathsByTargetName.get()
+                // Build only requested targets.
+                for target in self.options.targets.elements {
+                    // TODO: It would be nice if BuildSubset had a case for an
+                    // array of targets so that we can move the build out of
+                    // this enclosing if/else and avoid repetition.
+                    let buildResult = try await buildSystem.build(subset: .target(target), buildOutputs: [])
+                    for (target, files) in try buildResult.serializedDiagnosticPathsByTargetName.get() {
+                        if targetsToMigrate.contains(target) {
+                            diagnosticFiles.append(files)
+                        }
+                    }
+                }
             }
 
-            var summary = SwiftFixIt.Summary(numberOfFixItsApplied: 0, numberOfFilesChanged: 0)
-            let graph = try await buildSystem.getPackageGraph()
-            if targets.isEmpty {
-                targets = OrderedSet(graph.rootPackages.flatMap { $0.manifest.targets.filter { $0.type != .plugin }.map(\.name) })
-            }
             print("> Applying fix-its")
+            var summary = SwiftFixIt.Summary(numberOfFixItsApplied: 0, numberOfFilesChanged: 0)
             let fixItDuration = try ContinuousClock().measure {
-                for target in targets {
-                    let fixit = try SwiftFixIt(
-                        diagnosticFiles: Array(diagnosticsPaths[target] ?? []),
-                        categories: Set(features.flatMap(\.categories)),
-                        excludedSourceDirectories: [swiftCommandState.scratchDirectory],
-                        fileSystem: swiftCommandState.fileSystem
-                    )
-                    summary += try fixit.applyFixIts()
-                }
+                let applier = try SwiftFixIt(
+                    diagnosticFiles: diagnosticFiles.joined(),
+                    categories: Set(features.flatMap(\.categories)),
+                    excludedSourceDirectories: [swiftCommandState.scratchDirectory],
+                    fileSystem: swiftCommandState.fileSystem
+                )
+                summary = try applier.applyFixIts()
             }
 
             // Report the changes.
@@ -155,9 +177,8 @@ extension SwiftPackageCommand {
 
             // Once the fix-its were applied, it's time to update the
             // manifest with newly adopted feature settings.
-
             print("> Updating manifest")
-            for target in targets {
+            for target in targetsToMigrate {
                 swiftCommandState.observabilityScope.emit(debug: "Adding feature(s) to '\(target)'")
                 try self.updateManifest(
                     for: target,

Sources/SwiftFixIt/SwiftFixIt.swift

@@ -143,7 +143,7 @@ private struct PrimaryDiagnosticFilter<Diagnostic: AnyDiagnostic>: ~Copyable {
         }
 
         // Skip if no location.
-        if diagnostic.hasNoLocation {
+        guard let location = diagnostic.location else {
             return true
         }
 
@@ -155,9 +155,12 @@ private struct PrimaryDiagnosticFilter<Diagnostic: AnyDiagnostic>: ~Copyable {
             }
         }
 
-        // Skip if the source file the diagnostic appears in is in an excluded directory.
-        if let sourceFilePath = try? diagnostic.location.map({ try AbsolutePath(validating: $0.filename) }) {
-            guard !self.excludedSourceDirectories.contains(where: { $0.isAncestor(of: sourceFilePath) }) else {
+        // Skip if excluded directories were given and the source file the
+        // diagnostic appears in is in any of them.
+        if !self.excludedSourceDirectories.isEmpty {
+            if let sourceFilePath = try? AbsolutePath(validating: location.filename),
+               self.excludedSourceDirectories.contains(where: sourceFilePath.isDescendant(of:))
+            {
                 return true
             }
         }
@@ -208,7 +211,7 @@ package struct SwiftFixIt /*: ~Copyable */ { // TODO: Crashes with ~Copyable
     private let diagnosticsPerFile: DiagnosticsPerFile
 
     package init(
-        diagnosticFiles: [AbsolutePath],
+        diagnosticFiles: some Collection<AbsolutePath>,
         categories: Set<String> = [],
         excludedSourceDirectories: Set<AbsolutePath> = [],
         fileSystem: any FileSystem

Tests/SwiftFixItTests/FilteringTests.swift

@@ -412,6 +412,7 @@ struct FilteringTests {
         }
     }
 
+    @Test
     func testDuplicateInsertionFixIts() throws {
         try testAPI1File { path in
             .init(
@@ -526,4 +527,91 @@ struct FilteringTests {
             )
         }
     }
+
+    @Test
+    func testExcludedSourceDirectories() throws {
+        // Each generated file has a distinct parent directory.
+        try testAPI2Files { path1, path2 in
+            .init(
+                edits: (
+                    .init(input: "var x = 1", result: "var x = 1"),
+                    .init(input: "var x = 1", result: "var y = 2")
+                ),
+                summary: .init(
+                    // 6 because skipped by SwiftIDEUtils.FixItApplier, not SwiftFixIt.
+                    numberOfFixItsApplied: 2,
+                    numberOfFilesChanged: 1
+                ),
+                excludedSourceDirectories: [path1.parentDirectory],
+                diagnostics: [
+                    // path1.
+                    PrimaryDiagnostic(
+                        level: .error,
+                        text: "error1_fixit1",
+                        location: .init(path: path1, line: 1, column: 5),
+                        fixIts: [
+                            // Skipped, excluded directory.
+                            .init(
+                                start: .init(path: path1, line: 1, column: 5),
+                                end: .init(path: path1, line: 1, column: 6),
+                                text: "y"
+                            ),
+                        ]
+                    ),
+                    PrimaryDiagnostic(
+                        level: .error,
+                        text: "error2_fixit2",
+                        location: .init(path: path1, line: 1, column: 9),
+                        notes: [
+                            Note(
+                                text: "error2_note1",
+                                location: .init(path: path1, line: 1, column: 9),
+                                fixIts: [
+                                    // Skipped, excluded directory.
+                                    .init(
+                                        start: .init(path: path1, line: 1, column: 9),
+                                        end: .init(path: path1, line: 1, column: 10),
+                                        text: "2"
+                                    ),
+                                ]
+                            ),
+                        ]
+                    ),
+                    // path2.
+                    PrimaryDiagnostic(
+                        level: .error,
+                        text: "error1_fixit1",
+                        location: .init(path: path2, line: 1, column: 5),
+                        fixIts: [
+                            // Applied.
+                            .init(
+                                start: .init(path: path2, line: 1, column: 5),
+                                end: .init(path: path2, line: 1, column: 6),
+                                text: "y"
+                            ),
+                        ]
+                    ),
+                    PrimaryDiagnostic(
+                        level: .error,
+                        text: "error2_fixit2",
+                        location: .init(path: path2, line: 1, column: 9),
+                        notes: [
+                            Note(
+                                text: "error2_note1",
+                                location: .init(path: path2, line: 1, column: 9),
+                                fixIts: [
+                                    // Applied.
+                                    .init(
+                                        start: .init(path: path2, line: 1, column: 9),
+                                        end: .init(path: path2, line: 1, column: 10),
+                                        text: "2"
+                                    ),
+                                ]
+                            ),
+                        ]
+                    ),
+                ]
+            )
+        }
+    }
 }

Tests/SwiftFixItTests/Utilities.swift

@@ -178,9 +178,10 @@ struct Summary {
 }
 
 struct TestCase<T> {
-    let edits: T
-    let summary: Summary
-    let diagnostics: [PrimaryDiagnostic]
+    var edits: T
+    var summary: Summary
+    var excludedSourceDirectories: Set<AbsolutePath> = []
+    var diagnostics: [PrimaryDiagnostic]
 }
 
 extension Testing.Issue {
@@ -211,6 +212,7 @@ private func _testAPI(
     _ expectedSummary: Summary,
     _ diagnostics: [PrimaryDiagnostic],
     _ categories: Set<String>,
+    _ excludedSourceDirectories: Set<AbsolutePath>,
 ) throws {
     for (path, edit) in sourceFilePathsAndEdits {
         try localFileSystem.writeFileContents(path, string: edit.input)
@@ -233,7 +235,7 @@ private func _testAPI(
     let swiftFixIt = try SwiftFixIt(
         diagnostics: flatDiagnostics,
         categories: categories,
-        excludedSourceDirectories: [],
+        excludedSourceDirectories: excludedSourceDirectories,
         fileSystem: localFileSystem
     )
     let actualSummary = try swiftFixIt.applyFixIts()
@@ -261,44 +263,46 @@ private func _testAPI(
     }
 }
 
-private func uniqueSwiftFileName() -> String {
-    "\(UUID().uuidString).swift"
-}
-
 // Cannot use variadic generics: crashes.
 func testAPI1File(
+    function: StaticString = #function,
     categories: Set<String> = [],
     _ getTestCase: (AbsolutePath) -> TestCase<SourceFileEdit>
 ) throws {
-    try testWithTemporaryDirectory { fixturePath in
-        let sourceFilePath = fixturePath.appending(uniqueSwiftFileName())
+    try testWithTemporaryDirectory(function: function) { fixturePath in
+        let sourceFilePath = fixturePath.appending("file.swift")
 
         let testCase = getTestCase(sourceFilePath)
 
         try _testAPI(
             [(sourceFilePath, testCase.edits)],
             testCase.summary,
             testCase.diagnostics,
-            categories
+            categories,
+            testCase.excludedSourceDirectories,
         )
     }
 }
 
 func testAPI2Files(
+    function: StaticString = #function,
     categories: Set<String> = [],
     _ getTestCase: (AbsolutePath, AbsolutePath) -> TestCase<(SourceFileEdit, SourceFileEdit)>,
 ) throws {
-    try testWithTemporaryDirectory { fixturePath in
-        let sourceFilePath1 = fixturePath.appending(uniqueSwiftFileName())
-        let sourceFilePath2 = fixturePath.appending(uniqueSwiftFileName())
+    try testWithTemporaryDirectory(function: function) { fixturePath in
+        // Create each file in a separate subdirectory so that we can test
+        // directory exclusion.
+        let sourceFilePath1 = fixturePath.appending(components: [UUID().uuidString, "file.swift"])
+        let sourceFilePath2 = fixturePath.appending(components: [UUID().uuidString, "file.swift"])
 
         let testCase = getTestCase(sourceFilePath1, sourceFilePath2)
 
         try _testAPI(
             [(sourceFilePath1, testCase.edits.0), (sourceFilePath2, testCase.edits.1)],
             testCase.summary,
             testCase.diagnostics,
-            categories
+            categories,
+            testCase.excludedSourceDirectories,
         )
     }
 }