From b93bec7e36df085223c071114e3a3b51f115fd3b Mon Sep 17 00:00:00 2001 From: Pavel Yaskevich Date: Fri, 11 Jul 2025 12:24:12 -0700 Subject: [PATCH] [Commands] Migrate: Avoid duplicate fix-its and manifest updates when building target for both host and destination This is a limited cherry-pick of https://github.com/swiftlang/swift-package-manager/pull/8888 For example if a target is a dependency of a plugin we build it for host and destination and produce the same fix-it twice and attempt to update manifest twice because uniquing is done on the ModuleBuildDescriptions. The changes do the following: - Coalesce all the diagnostic files for the same target regardless of host or destination build to make it possible for `SwiftFixIt` to filter duplicate fix-its; - Unique based on module identity to prevent duplicate setting insertion. Resolves: rdar://155569285 --- .../Package.swift | 13 ++++++ .../Plugins/Plugin/Plugin.swift | 17 +++++++ .../Sources/CommonLibrary/Common.swift | 6 +++ .../Sources/Library/Test.swift | 6 +++ .../Sources/Tool/tool.swift | 13 ++++++ .../Package.swift | 12 +++++ .../Plugins/Plugin/Plugin.swift | 17 +++++++ .../Sources/Library/Test.swift | 20 ++++++++ .../Sources/Tool/tool.swift | 12 +++++ .../Commands/PackageCommands/Migrate.swift | 17 +++---- Sources/SwiftFixIt/SwiftFixIt.swift | 19 +++++++- Tests/CommandsTests/PackageCommandTests.swift | 46 +++++++++++++++++++ Tests/SwiftFixItTests/Utilities.swift | 1 + 13 files changed, 189 insertions(+), 10 deletions(-) create mode 100644 Fixtures/SwiftMigrate/ExistentialAnyWithCommonPluginDependencyMigration/Package.swift create mode 100644 Fixtures/SwiftMigrate/ExistentialAnyWithCommonPluginDependencyMigration/Plugins/Plugin/Plugin.swift create mode 100644 Fixtures/SwiftMigrate/ExistentialAnyWithCommonPluginDependencyMigration/Sources/CommonLibrary/Common.swift create mode 100644 Fixtures/SwiftMigrate/ExistentialAnyWithCommonPluginDependencyMigration/Sources/Library/Test.swift create mode 100644 Fixtures/SwiftMigrate/ExistentialAnyWithCommonPluginDependencyMigration/Sources/Tool/tool.swift create mode 100644 Fixtures/SwiftMigrate/ExistentialAnyWithPluginMigration/Package.swift create mode 100644 Fixtures/SwiftMigrate/ExistentialAnyWithPluginMigration/Plugins/Plugin/Plugin.swift create mode 100644 Fixtures/SwiftMigrate/ExistentialAnyWithPluginMigration/Sources/Library/Test.swift create mode 100644 Fixtures/SwiftMigrate/ExistentialAnyWithPluginMigration/Sources/Tool/tool.swift diff --git a/Fixtures/SwiftMigrate/ExistentialAnyWithCommonPluginDependencyMigration/Package.swift b/Fixtures/SwiftMigrate/ExistentialAnyWithCommonPluginDependencyMigration/Package.swift new file mode 100644 index 00000000000..638cb789758 --- /dev/null +++ b/Fixtures/SwiftMigrate/ExistentialAnyWithCommonPluginDependencyMigration/Package.swift @@ -0,0 +1,13 @@ +// swift-tools-version:5.8 + +import PackageDescription + +let package = Package( + name: "ExistentialAnyMigration", + targets: [ + .target(name: "Library", dependencies: ["CommonLibrary"], plugins: [.plugin(name: "Plugin")]), + .plugin(name: "Plugin", capability: .buildTool, dependencies: ["Tool"]), + .executableTarget(name: "Tool", dependencies: ["CommonLibrary"]), + .target(name: "CommonLibrary"), + ] +) diff --git a/Fixtures/SwiftMigrate/ExistentialAnyWithCommonPluginDependencyMigration/Plugins/Plugin/Plugin.swift b/Fixtures/SwiftMigrate/ExistentialAnyWithCommonPluginDependencyMigration/Plugins/Plugin/Plugin.swift new file mode 100644 index 00000000000..2410af9dbef --- /dev/null +++ b/Fixtures/SwiftMigrate/ExistentialAnyWithCommonPluginDependencyMigration/Plugins/Plugin/Plugin.swift @@ -0,0 +1,17 @@ +import PackagePlugin +import Foundation + +@main struct Plugin: BuildToolPlugin { + func createBuildCommands(context: PluginContext, target: Target) throws -> [Command] { + let tool = try context.tool(named: "Tool") + let output = context.pluginWorkDirectory.appending(["generated.swift"]) + return [ + .buildCommand( + displayName: "Plugin", + executable: tool.path, + arguments: [output], + inputFiles: [], + outputFiles: [output]) + ] + } +} diff --git a/Fixtures/SwiftMigrate/ExistentialAnyWithCommonPluginDependencyMigration/Sources/CommonLibrary/Common.swift b/Fixtures/SwiftMigrate/ExistentialAnyWithCommonPluginDependencyMigration/Sources/CommonLibrary/Common.swift new file mode 100644 index 00000000000..4f6ab41fc9b --- /dev/null +++ b/Fixtures/SwiftMigrate/ExistentialAnyWithCommonPluginDependencyMigration/Sources/CommonLibrary/Common.swift @@ -0,0 +1,6 @@ +public func common() {} + + +protocol P {} + +func needsMigration(_ p: P) {} diff --git a/Fixtures/SwiftMigrate/ExistentialAnyWithCommonPluginDependencyMigration/Sources/Library/Test.swift b/Fixtures/SwiftMigrate/ExistentialAnyWithCommonPluginDependencyMigration/Sources/Library/Test.swift new file mode 100644 index 00000000000..bc8a65d8348 --- /dev/null +++ b/Fixtures/SwiftMigrate/ExistentialAnyWithCommonPluginDependencyMigration/Sources/Library/Test.swift @@ -0,0 +1,6 @@ +import CommonLibrary + +func bar() { + generatedFunction() + common() +} diff --git a/Fixtures/SwiftMigrate/ExistentialAnyWithCommonPluginDependencyMigration/Sources/Tool/tool.swift b/Fixtures/SwiftMigrate/ExistentialAnyWithCommonPluginDependencyMigration/Sources/Tool/tool.swift new file mode 100644 index 00000000000..e69e49865a4 --- /dev/null +++ b/Fixtures/SwiftMigrate/ExistentialAnyWithCommonPluginDependencyMigration/Sources/Tool/tool.swift @@ -0,0 +1,13 @@ +import Foundation +import CommonLibrary + +@main struct Entry { + public static func main() async throws { + common() + let outputPath = CommandLine.arguments[1] + let contents = """ + func generatedFunction() {} + """ + FileManager.default.createFile(atPath: outputPath, contents: contents.data(using: .utf8)) + } +} diff --git a/Fixtures/SwiftMigrate/ExistentialAnyWithPluginMigration/Package.swift b/Fixtures/SwiftMigrate/ExistentialAnyWithPluginMigration/Package.swift new file mode 100644 index 00000000000..5a689efe60d --- /dev/null +++ b/Fixtures/SwiftMigrate/ExistentialAnyWithPluginMigration/Package.swift @@ -0,0 +1,12 @@ +// swift-tools-version:5.8 + +import PackageDescription + +let package = Package( + name: "ExistentialAnyMigration", + targets: [ + .target(name: "Library", plugins: [.plugin(name: "Plugin")]), + .plugin(name: "Plugin", capability: .buildTool, dependencies: ["Tool"]), + .executableTarget(name: "Tool"), + ] +) diff --git a/Fixtures/SwiftMigrate/ExistentialAnyWithPluginMigration/Plugins/Plugin/Plugin.swift b/Fixtures/SwiftMigrate/ExistentialAnyWithPluginMigration/Plugins/Plugin/Plugin.swift new file mode 100644 index 00000000000..2410af9dbef --- /dev/null +++ b/Fixtures/SwiftMigrate/ExistentialAnyWithPluginMigration/Plugins/Plugin/Plugin.swift @@ -0,0 +1,17 @@ +import PackagePlugin +import Foundation + +@main struct Plugin: BuildToolPlugin { + func createBuildCommands(context: PluginContext, target: Target) throws -> [Command] { + let tool = try context.tool(named: "Tool") + let output = context.pluginWorkDirectory.appending(["generated.swift"]) + return [ + .buildCommand( + displayName: "Plugin", + executable: tool.path, + arguments: [output], + inputFiles: [], + outputFiles: [output]) + ] + } +} diff --git a/Fixtures/SwiftMigrate/ExistentialAnyWithPluginMigration/Sources/Library/Test.swift b/Fixtures/SwiftMigrate/ExistentialAnyWithPluginMigration/Sources/Library/Test.swift new file mode 100644 index 00000000000..dfae68a022d --- /dev/null +++ b/Fixtures/SwiftMigrate/ExistentialAnyWithPluginMigration/Sources/Library/Test.swift @@ -0,0 +1,20 @@ +protocol P { +} + +func test1(_: P) { +} + +func test2(_: P.Protocol) { +} + +func test3() { + let _: [P?] = [] +} + +func test4() { + var x = 42 +} + +func bar() { + generatedFunction() +} diff --git a/Fixtures/SwiftMigrate/ExistentialAnyWithPluginMigration/Sources/Tool/tool.swift b/Fixtures/SwiftMigrate/ExistentialAnyWithPluginMigration/Sources/Tool/tool.swift new file mode 100644 index 00000000000..18c38c1df83 --- /dev/null +++ b/Fixtures/SwiftMigrate/ExistentialAnyWithPluginMigration/Sources/Tool/tool.swift @@ -0,0 +1,12 @@ +import Foundation + +@main struct Entry { + public static func main() async throws { + let outputPath = CommandLine.arguments[1] + let contents = """ + func generatedFunction() {} + func dontmodifyme(_: P) {} + """ + FileManager.default.createFile(atPath: outputPath, contents: contents.data(using: .utf8)) + } +} diff --git a/Sources/Commands/PackageCommands/Migrate.swift b/Sources/Commands/PackageCommands/Migrate.swift index 95098638934..3e5234a4094 100644 --- a/Sources/Commands/PackageCommands/Migrate.swift +++ b/Sources/Commands/PackageCommands/Migrate.swift @@ -113,11 +113,11 @@ extension SwiftPackageCommand { // Determine all of the targets we need up update. let buildPlan = try buildSystem.buildPlan - var modules: [any ModuleBuildDescription] = [] + var modules = [String: [AbsolutePath]]() if !targets.isEmpty { for buildDescription in buildPlan.buildModules where targets.contains(buildDescription.module.name) { - modules.append(buildDescription) + modules[buildDescription.module.name, default: []].append(contentsOf: buildDescription.diagnosticFiles) } } else { let graph = try await buildSystem.getPackageGraph() @@ -128,7 +128,7 @@ extension SwiftPackageCommand { guard module.type != .plugin, !module.implicit else { continue } - modules.append(buildDescription) + modules[buildDescription.module.name, default: []].append(contentsOf: buildDescription.diagnosticFiles) } } @@ -139,10 +139,11 @@ extension SwiftPackageCommand { var summary = SwiftFixIt.Summary(numberOfFixItsApplied: 0, numberOfFilesChanged: 0) let fixItDuration = try ContinuousClock().measure { - for module in modules { + for (_, diagnosticFiles) in modules { let fixit = try SwiftFixIt( - diagnosticFiles: module.diagnosticFiles, + diagnosticFiles: diagnosticFiles, categories: Set(features.flatMap(\.categories)), + excludedSourceDirectories: [swiftCommandState.scratchDirectory], fileSystem: swiftCommandState.fileSystem ) summary += try fixit.applyFixIts() @@ -176,10 +177,10 @@ extension SwiftPackageCommand { // manifest with newly adopted feature settings. print("> Updating manifest") - for module in modules.map(\.module) { - swiftCommandState.observabilityScope.emit(debug: "Adding feature(s) to '\(module.name)'") + for (name, _) in modules { + swiftCommandState.observabilityScope.emit(debug: "Adding feature(s) to '\(name)'") try self.updateManifest( - for: module.name, + for: name, add: features, using: swiftCommandState ) diff --git a/Sources/SwiftFixIt/SwiftFixIt.swift b/Sources/SwiftFixIt/SwiftFixIt.swift index da0babf5456..f182f3960ae 100644 --- a/Sources/SwiftFixIt/SwiftFixIt.swift +++ b/Sources/SwiftFixIt/SwiftFixIt.swift @@ -124,9 +124,11 @@ private struct PrimaryDiagnosticFilter: ~Copyable { private var uniquePrimaryDiagnostics: Set = [] let categories: Set + let excludedSourceDirectories: Set - init(categories: Set) { + init(categories: Set, excludedSourceDirectories: Set) { self.categories = categories + self.excludedSourceDirectories = excludedSourceDirectories } /// Returns a Boolean value indicating whether to skip the given primary @@ -153,6 +155,13 @@ 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 { + return true + } + } + let notes = primaryDiagnosticWithNotes.dropFirst() precondition(notes.allSatisfy(\.isNote)) @@ -201,6 +210,7 @@ package struct SwiftFixIt /*: ~Copyable */ { // TODO: Crashes with ~Copyable package init( diagnosticFiles: [AbsolutePath], categories: Set = [], + excludedSourceDirectories: Set = [], fileSystem: any FileSystem ) throws { // Deserialize the diagnostics. @@ -212,6 +222,7 @@ package struct SwiftFixIt /*: ~Copyable */ { // TODO: Crashes with ~Copyable self = try SwiftFixIt( diagnostics: diagnostics, categories: categories, + excludedSourceDirectories: excludedSourceDirectories, fileSystem: fileSystem ) } @@ -219,11 +230,15 @@ package struct SwiftFixIt /*: ~Copyable */ { // TODO: Crashes with ~Copyable init( diagnostics: some Collection, categories: Set, + excludedSourceDirectories: Set, fileSystem: any FileSystem ) throws { self.fileSystem = fileSystem - var filter = PrimaryDiagnosticFilter(categories: categories) + var filter = PrimaryDiagnosticFilter( + categories: categories, + excludedSourceDirectories: excludedSourceDirectories + ) _ = consume categories // Build a map from source files to `SwiftDiagnostics` diagnostics. diff --git a/Tests/CommandsTests/PackageCommandTests.swift b/Tests/CommandsTests/PackageCommandTests.swift index 26b028fd56d..ebe33039584 100644 --- a/Tests/CommandsTests/PackageCommandTests.swift +++ b/Tests/CommandsTests/PackageCommandTests.swift @@ -2158,6 +2158,44 @@ class PackageCommandTestCase: CommandsBuildProviderTestCase { try await doMigration(featureName: "InferIsolatedConformances", expectedSummary: "Applied 3 fix-its in 2 files") } + func testMigrateCommandWithBuildToolPlugins() async throws { + try XCTSkipIf( + !UserToolchain.default.supportesSupportedFeatures, + "skipping because test environment compiler doesn't support `-print-supported-features`" + ) + + try await fixture(name: "SwiftMigrate/ExistentialAnyWithPluginMigration") { fixturePath in + let (stdout, _) = try await self.execute( + ["migrate", "--to-feature", "ExistentialAny"], + packagePath: fixturePath + ) + + // Check the plugin target in the manifest wasn't updated + let manifestContent = try localFileSystem.readFileContents(fixturePath.appending(component: "Package.swift")).description + XCTAssertTrue(manifestContent.contains(".plugin(name: \"Plugin\", capability: .buildTool, dependencies: [\"Tool\"]),")) + + // Building the package produces migration fix-its in both an authored and generated source file. Check we only applied fix-its to the hand-authored one. + XCTAssertMatch(stdout, .regex("> \("Applied 3 fix-its in 1 file")" + #" \([0-9]\.[0-9]{1,3}s\)"#)) + } + } + + func testMigrateCommandWhenDependencyBuildsForHostAndTarget() async throws { + try XCTSkipIf( + !UserToolchain.default.supportesSupportedFeatures, + "skipping because test environment compiler doesn't support `-print-supported-features`" + ) + + try await fixture(name: "SwiftMigrate/ExistentialAnyWithCommonPluginDependencyMigration") { fixturePath in + let (stdout, _) = try await self.execute( + ["migrate", "--to-feature", "ExistentialAny"], + packagePath: fixturePath + ) + + // Even though the CommonLibrary dependency built for both the host and destination, we should only apply a single fix-it once to its sources. + XCTAssertMatch(stdout, .regex("> \("Applied 1 fix-it in 1 file")" + #" \([0-9]\.[0-9]{1,3}s\)"#)) + } + } + func testBuildToolPlugin() async throws { try await testBuildToolPlugin(staticStdlib: false) } @@ -4108,6 +4146,14 @@ class PackageCommandSwiftBuildTests: PackageCommandTestCase { throw XCTSkip("SWBINTTODO: Build plan is not currently supported") } + override func testMigrateCommandWithBuildToolPlugins() async throws { + throw XCTSkip("SWBINTTODO: Build plan is not currently supported") + } + + override func testMigrateCommandWhenDependencyBuildsForHostAndTarget() async throws { + throw XCTSkip("SWBINTTODO: Build plan is not currently supported") + } + override func testCommandPluginTestingCallbacks() async throws { throw XCTSkip("SWBINTTODO: Requires PIF generation to adopt new test runner product type") try XCTSkipOnWindows(because: "TSCBasic/Path.swift:969: Assertion failed, https://github.com/swiftlang/swift-package-manager/issues/8602") diff --git a/Tests/SwiftFixItTests/Utilities.swift b/Tests/SwiftFixItTests/Utilities.swift index a1643bf485d..60b0f39ffeb 100644 --- a/Tests/SwiftFixItTests/Utilities.swift +++ b/Tests/SwiftFixItTests/Utilities.swift @@ -233,6 +233,7 @@ private func _testAPI( let swiftFixIt = try SwiftFixIt( diagnostics: flatDiagnostics, categories: categories, + excludedSourceDirectories: [], fileSystem: localFileSystem ) let actualSummary = try swiftFixIt.applyFixIts()