swift-package-migrate: Add more clarity to manifest update error message

(cherry picked from commit e1deb23)

Author: AnthonyLatsis

Fixtures/SwiftMigrate/UpdateManifest/Package.swift

@@ -0,0 +1,24 @@
+// swift-tools-version:5.8
+
+import PackageDescription
+
+var swiftSettings: [SwiftSetting] = []
+
+let package = Package(
+    name: "WithErrors",
+    targets: [
+        .target(
+            name: "CannotFindSettings",
+            swiftSettings: swiftSettings
+        ),
+        .target(name: "A"),
+        .target(name: "B"),
+    ]
+)
+
+package.targets.append(
+    .target(
+        name: "CannotFindTarget",
+        swiftSettings: swiftSettings
+    ),
+)

Fixtures/SwiftMigrate/UpdateManifest/Package.updated.targets-A-B.swift

@@ -0,0 +1,28 @@
+// swift-tools-version:5.8
+
+import PackageDescription
+
+var swiftSettings: [SwiftSetting] = []
+
+let package = Package(
+    name: "WithErrors",
+    targets: [
+        .target(
+            name: "CannotFindSettings",
+            swiftSettings: swiftSettings
+        ),
+        .target(name: "A",swiftSettings: [
+    .enableUpcomingFeature("ExistentialAny"),
+    .enableUpcomingFeature("InferIsolatedConformances"),]),
+        .target(name: "B",swiftSettings: [
+    .enableUpcomingFeature("ExistentialAny"),
+    .enableUpcomingFeature("InferIsolatedConformances"),]),
+    ]
+)
+
+package.targets.append(
+    .target(
+        name: "CannotFindTarget",
+        swiftSettings: swiftSettings
+    ),
+)

Fixtures/SwiftMigrate/UpdateManifest/Package.updated.targets-A.swift

@@ -0,0 +1,26 @@
+// swift-tools-version:5.8
+
+import PackageDescription
+
+var swiftSettings: [SwiftSetting] = []
+
+let package = Package(
+    name: "WithErrors",
+    targets: [
+        .target(
+            name: "CannotFindSettings",
+            swiftSettings: swiftSettings
+        ),
+        .target(name: "A",swiftSettings: [
+    .enableUpcomingFeature("ExistentialAny"),
+    .enableUpcomingFeature("InferIsolatedConformances"),]),
+        .target(name: "B"),
+    ]
+)
+
+package.targets.append(
+    .target(
+        name: "CannotFindTarget",
+        swiftSettings: swiftSettings
+    ),
+)

Fixtures/SwiftMigrate/UpdateManifest/Package.updated.targets-all.swift

@@ -0,0 +1,28 @@
+// swift-tools-version:5.8
+
+import PackageDescription
+
+var swiftSettings: [SwiftSetting] = []
+
+let package = Package(
+    name: "WithErrors",
+    targets: [
+        .target(
+            name: "CannotFindSettings",
+            swiftSettings: swiftSettings
+        ),
+        .target(name: "A",swiftSettings: [
+    .enableUpcomingFeature("ExistentialAny"),
+    .enableUpcomingFeature("InferIsolatedConformances"),]),
+        .target(name: "B",swiftSettings: [
+    .enableUpcomingFeature("ExistentialAny"),
+    .enableUpcomingFeature("InferIsolatedConformances"),]),
+    ]
+)
+
+package.targets.append(
+    .target(
+        name: "CannotFindTarget",
+        swiftSettings: swiftSettings
+    ),
+)

Fixtures/SwiftMigrate/UpdateManifest/Sources/A/File.swift

@@ -0,0 +1 @@
+public func foo() {}

Fixtures/SwiftMigrate/UpdateManifest/Sources/B/File.swift

@@ -0,0 +1 @@
+public func foo() {}

Fixtures/SwiftMigrate/UpdateManifest/Sources/CannotFindSettings/File.swift

@@ -0,0 +1 @@
+public func foo() {}

Fixtures/SwiftMigrate/UpdateManifest/Sources/CannotFindTarget/File.swift

@@ -0,0 +1 @@
+public func foo() {}

Sources/Commands/PackageCommands/Migrate.swift

@@ -23,6 +23,7 @@ import OrderedCollections
 
 import PackageGraph
 import PackageModel
+import enum PackageModelSyntax.ManifestEditError
 
 import SPMBuildCore
 import SwiftFixIt
@@ -153,9 +154,12 @@ extension SwiftPackageCommand {
 
             // Once the fix-its were applied, it's time to update the
             // manifest with newly adopted feature settings.
+            //
+            // Loop over a sorted array to produce deterministic results and
+            // order of diagnostics.
 
             print("> Updating manifest")
-            for (name, _) in modules {
+            for name in modules.keys.sorted() {
                 swiftCommandState.observabilityScope.emit(debug: "Adding feature(s) to '\(name)'")
                 try self.updateManifest(
                     for: name,
@@ -166,6 +170,8 @@ extension SwiftPackageCommand {
         }
 
         /// Resolves the requested feature names.
+        ///
+        /// - Returns: An array of resolved features, sorted by name.
         private func resolveRequestedFeatures(
             _ swiftCommandState: SwiftCommandState
         ) throws -> [SwiftCompilerFeature] {
@@ -200,7 +206,9 @@ extension SwiftPackageCommand {
                 resolvedFeatures.append(feature)
             }
 
-            return resolvedFeatures
+            return resolvedFeatures.sorted { lhs, rhs in
+                lhs.name < rhs.name
+            }
         }
 
         private func createBuildSystem(
@@ -266,15 +274,37 @@ extension SwiftPackageCommand {
                     verbose: !self.globalOptions.logging.quiet
                 )
             } catch {
-                try swiftCommandState.observabilityScope.emit(
-                    error: """
-                    Could not update manifest for '\(target)' (\(error)). \
-                    Please enable '\(
-                        features.map { try $0.swiftSettingDescription }
-                            .joined(separator: ", ")
-                    )' features manually
-                    """
-                )
+                var message =
+                    "Could not update manifest to enable requested features for target '\(target)' (\(error))"
+
+                // Do not suggest manual addition if something else is wrong or
+                // if the error implies that it cannot be done.
+                if let error = error as? ManifestEditError {
+                    switch error {
+                    case .cannotFindPackage,
+                         .cannotAddSettingsToPluginTarget,
+                         .existingDependency:
+                        break
+                    case .cannotFindArrayLiteralArgument,
+                         // This means the target could not be found
+                         // syntactically, not that it does not exist.
+                         .cannotFindTargets,
+                         .cannotFindTarget,
+                         // This means the swift-tools-version is lower than
+                         // the version where one of the setting was introduced.
+                         .oldManifest:
+                        let settings = try features.map {
+                            try $0.swiftSettingDescription
+                        }.joined(separator: ", ")
+
+                        message += """
+                        . Please enable them manually by adding the following Swift settings to the target: \
+                        '\(settings)'
+                        """
+                    }
+                }
+
+                swiftCommandState.observabilityScope.emit(error: message)
             }
         }
 

Tests/CommandsTests/PackageCommandTests.swift

@@ -2248,6 +2248,100 @@ class PackageCommandTestCase: CommandsBuildProviderTestCase {
         }
     }
 
+    func testMigrateCommandUpdateManifestSingleTarget() async throws {
+        try XCTSkipIf(
+            !UserToolchain.default.supportesSupportedFeatures,
+            "skipping because test environment compiler doesn't support `-print-supported-features`"
+        )
+
+        try await fixture(name: "SwiftMigrate/UpdateManifest") { fixturePath in
+            _ = try await self.execute(
+                [
+                    "migrate",
+                    "--to-feature",
+                    "ExistentialAny,InferIsolatedConformances",
+                    "--target",
+                    "A",
+                ],
+                packagePath: fixturePath
+            )
+
+            let updatedManifest = try localFileSystem.readFileContents(
+                fixturePath.appending(components: "Package.swift")
+            )
+            let expectedManifest = try localFileSystem.readFileContents(
+                fixturePath.appending(components: "Package.updated.targets-A.swift")
+            )
+            XCTAssertEqual(updatedManifest, expectedManifest)
+        }
+
+    }
+
+    func testMigrateCommandUpdateManifest2Targets() async throws {
+        try XCTSkipIf(
+            !UserToolchain.default.supportesSupportedFeatures,
+            "skipping because test environment compiler doesn't support `-print-supported-features`"
+        )
+
+        try await fixture(name: "SwiftMigrate/UpdateManifest") { fixturePath in
+            _ = try await self.execute(
+                [
+                    "migrate",
+                    "--to-feature",
+                    "ExistentialAny,InferIsolatedConformances",
+                    "--target",
+                    "A,B",
+                ],
+                packagePath: fixturePath
+            )
+
+            let updatedManifest = try localFileSystem.readFileContents(
+                fixturePath.appending(components: "Package.swift")
+            )
+            let expectedManifest = try localFileSystem.readFileContents(
+                fixturePath.appending(components: "Package.updated.targets-A-B.swift")
+            )
+            XCTAssertEqual(updatedManifest, expectedManifest)
+        }
+    }
+
+    func testMigrateCommandUpdateManifestWithErrors() async throws {
+        try XCTSkipIf(
+            !UserToolchain.default.supportesSupportedFeatures,
+            "skipping because test environment compiler doesn't support `-print-supported-features`"
+        )
+
+        try await fixture(name: "SwiftMigrate/UpdateManifest") { fixturePath in
+            try await XCTAssertThrowsCommandExecutionError(
+                await self.execute(
+                    ["migrate", "--to-feature", "ExistentialAny,InferIsolatedConformances,StrictMemorySafety"],
+                    packagePath: fixturePath
+                )
+            ) { error in
+                // 'SwiftMemorySafety.strictMemorySafety' was introduced in 6.2.
+                XCTAssertMatch(
+                    error.stderr,
+                    .contains(
+                        """
+                        error: Could not update manifest to enable requested features for target 'A' (package manifest version 5.8.0 is too old: please update to manifest version 6.2.0 or newer). Please enable them manually by adding the following Swift settings to the target: '.enableUpcomingFeature("ExistentialAny"), .enableUpcomingFeature("InferIsolatedConformances"), .strictMemorySafety()'
+                        error: Could not update manifest to enable requested features for target 'B' (package manifest version 5.8.0 is too old: please update to manifest version 6.2.0 or newer). Please enable them manually by adding the following Swift settings to the target: '.enableUpcomingFeature("ExistentialAny"), .enableUpcomingFeature("InferIsolatedConformances"), .strictMemorySafety()'
+                        error: Could not update manifest to enable requested features for target 'CannotFindSettings' (unable to find array literal for 'swiftSettings' argument). Please enable them manually by adding the following Swift settings to the target: '.enableUpcomingFeature("ExistentialAny"), .enableUpcomingFeature("InferIsolatedConformances"), .strictMemorySafety()'
+                        error: Could not update manifest to enable requested features for target 'CannotFindTarget' (unable to find target named 'CannotFindTarget' in package). Please enable them manually by adding the following Swift settings to the target: '.enableUpcomingFeature("ExistentialAny"), .enableUpcomingFeature("InferIsolatedConformances"), .strictMemorySafety()'
+                        """
+                    )
+                )
+            }
+
+            let updatedManifest = try localFileSystem.readFileContents(
+                fixturePath.appending(components: "Package.swift")
+            )
+            let expectedManifest = try localFileSystem.readFileContents(
+                fixturePath.appending(components: "Package.updated.targets-all.swift")
+            )
+            XCTAssertEqual(updatedManifest, expectedManifest)
+        }
+    }
+
     func testBuildToolPlugin() async throws {
         try await testBuildToolPlugin(staticStdlib: false)
     }
@@ -4206,6 +4300,18 @@ class PackageCommandSwiftBuildTests: PackageCommandTestCase {
         throw XCTSkip("SWBINTTODO: Build plan is not currently supported")
     }
 
+    override func testMigrateCommandUpdateManifestSingleTarget() async throws {
+        throw XCTSkip("SWBINTTODO: Build plan is not currently supported")
+    }
+
+    override func testMigrateCommandUpdateManifest2Targets() async throws {
+        throw XCTSkip("SWBINTTODO: Build plan is not currently supported")
+    }
+
+    override func testMigrateCommandUpdateManifestWithErrors() 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")