diff --git a/Sources/Commands/PackageCommands/Migrate.swift b/Sources/Commands/PackageCommands/Migrate.swift index 094c0afb998..09ae95752c7 100644 --- a/Sources/Commands/PackageCommands/Migrate.swift +++ b/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 + 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, diff --git a/Sources/SwiftFixIt/SwiftFixIt.swift b/Sources/SwiftFixIt/SwiftFixIt.swift index 78cd4428c0c..3001affe36e 100644 --- a/Sources/SwiftFixIt/SwiftFixIt.swift +++ b/Sources/SwiftFixIt/SwiftFixIt.swift @@ -143,7 +143,7 @@ private struct PrimaryDiagnosticFilter: ~Copyable { } // Skip if no location. - if diagnostic.hasNoLocation { + guard let location = diagnostic.location else { return true } @@ -155,9 +155,12 @@ private struct PrimaryDiagnosticFilter: ~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, categories: Set = [], excludedSourceDirectories: Set = [], fileSystem: any FileSystem diff --git a/Tests/SwiftFixItTests/FilteringTests.swift b/Tests/SwiftFixItTests/FilteringTests.swift index dad7176c94f..653bb7ecae9 100644 --- a/Tests/SwiftFixItTests/FilteringTests.swift +++ b/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" + ), + ] + ), + ] + ), + ] + ) + } + } } diff --git a/Tests/SwiftFixItTests/Utilities.swift b/Tests/SwiftFixItTests/Utilities.swift index 60b0f39ffeb..ee3959b8d1d 100644 --- a/Tests/SwiftFixItTests/Utilities.swift +++ b/Tests/SwiftFixItTests/Utilities.swift @@ -178,9 +178,10 @@ struct Summary { } struct TestCase { - let edits: T - let summary: Summary - let diagnostics: [PrimaryDiagnostic] + var edits: T + var summary: Summary + var excludedSourceDirectories: Set = [] + var diagnostics: [PrimaryDiagnostic] } extension Testing.Issue { @@ -211,6 +212,7 @@ private func _testAPI( _ expectedSummary: Summary, _ diagnostics: [PrimaryDiagnostic], _ categories: Set, + _ excludedSourceDirectories: Set, ) 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,17 +263,14 @@ private func _testAPI( } } -private func uniqueSwiftFileName() -> String { - "\(UUID().uuidString).swift" -} - // Cannot use variadic generics: crashes. func testAPI1File( + function: StaticString = #function, categories: Set = [], _ getTestCase: (AbsolutePath) -> TestCase ) 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) @@ -279,18 +278,22 @@ func testAPI1File( [(sourceFilePath, testCase.edits)], testCase.summary, testCase.diagnostics, - categories + categories, + testCase.excludedSourceDirectories, ) } } func testAPI2Files( + function: StaticString = #function, categories: Set = [], _ 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) @@ -298,7 +301,8 @@ func testAPI2Files( [(sourceFilePath1, testCase.edits.0), (sourceFilePath2, testCase.edits.1)], testCase.summary, testCase.diagnostics, - categories + categories, + testCase.excludedSourceDirectories, ) } }