improve "exhausted attempts" diagnostics (#6956)

tomerd · web-flow · commit b699e0747f3f · 2023-09-29T14:49:40.000-07:00
motivation: better diagnostics when depdendcy graph fails to resolve. the "exhausted attempts" error message should normally not be encountered, other than in edge cases where the graph is not resolvable. nevertheless, it should be readable by users

changes:
* conslidate the formatting of the "exhausted attempts" error message
* format the "exhausted attempts" more clearly so it is more readable
* update test

rdar://112134269
diff --git a/Sources/Workspace/Diagnostics.swift b/Sources/Workspace/Diagnostics.swift
@@ -28,12 +28,11 @@ public struct ManifestParseDiagnostic: CustomStringConvertible {
     }
 
     public var description: String {
-        "manifest parse error(s):\n" + errors.joined(separator: "\n")
+        "manifest parse error(s):\n" + self.errors.joined(separator: "\n")
     }
 }
 
 public enum WorkspaceDiagnostics {
-
     // MARK: - Errors
 
     /// The diagnostic triggered when an operation fails because its completion
@@ -43,7 +42,7 @@ public enum WorkspaceDiagnostics {
         public let repositoryPath: AbsolutePath
 
         public var description: String {
-            return "repository '\(repositoryPath)' has uncommited changes"
+            "repository '\(self.repositoryPath)' has uncommited changes"
         }
     }
 
@@ -54,7 +53,7 @@ public enum WorkspaceDiagnostics {
         public let repositoryPath: AbsolutePath
 
         public var description: String {
-            return "repository '\(repositoryPath)' has unpushed changes"
+            "repository '\(self.repositoryPath)' has unpushed changes"
         }
     }
 
@@ -65,7 +64,7 @@ public enum WorkspaceDiagnostics {
         public let dependencyName: String
 
         public var description: String {
-            return "dependency '\(dependencyName)' not in edit mode"
+            "dependency '\(self.dependencyName)' not in edit mode"
         }
     }
 
@@ -76,7 +75,7 @@ public enum WorkspaceDiagnostics {
         public let branch: String
 
         public var description: String {
-            return "branch '\(branch)' already exists"
+            "branch '\(self.branch)' already exists"
         }
     }
 
@@ -87,7 +86,7 @@ public enum WorkspaceDiagnostics {
         public let revision: String
 
         public var description: String {
-            return "revision '\(revision)' does not exist"
+            "revision '\(self.revision)' does not exist"
         }
     }
 }
@@ -98,11 +97,15 @@ extension Basics.Diagnostic {
     }
 
     static func editBranchNotCheckedOut(packageName: String, branchName: String) -> Self {
-        .warning("dependency '\(packageName)' already exists at the edit destination; not checking-out branch '\(branchName)'")
+        .warning(
+            "dependency '\(packageName)' already exists at the edit destination; not checking-out branch '\(branchName)'"
+        )
     }
 
     static func editRevisionNotUsed(packageName: String, revisionIdentifier: String) -> Self {
-        .warning("dependency '\(packageName)' already exists at the edit destination; not using revision '\(revisionIdentifier)'")
+        .warning(
+            "dependency '\(packageName)' already exists at the edit destination; not using revision '\(revisionIdentifier)'"
+        )
     }
 
     static func editedDependencyMissing(packageName: String) -> Self {
@@ -122,35 +125,49 @@ extension Basics.Diagnostic {
     }
 
     static func artifactInvalidArchive(artifactURL: URL, targetName: String) -> Self {
-        .error("invalid archive returned from '\(artifactURL.absoluteString)' which is required by binary target '\(targetName)'")
+        .error(
+            "invalid archive returned from '\(artifactURL.absoluteString)' which is required by binary target '\(targetName)'"
+        )
     }
 
     static func artifactChecksumChanged(targetName: String) -> Self {
-        .error("artifact of binary target '\(targetName)' has changed checksum; this is a potential security risk so the new artifact won't be downloaded")
+        .error(
+            "artifact of binary target '\(targetName)' has changed checksum; this is a potential security risk so the new artifact won't be downloaded"
+        )
     }
 
     static func artifactInvalidChecksum(targetName: String, expectedChecksum: String, actualChecksum: String?) -> Self {
-        .error("checksum of downloaded artifact of binary target '\(targetName)' (\(actualChecksum ?? "none")) does not match checksum specified by the manifest (\(expectedChecksum))")
+        .error(
+            "checksum of downloaded artifact of binary target '\(targetName)' (\(actualChecksum ?? "none")) does not match checksum specified by the manifest (\(expectedChecksum))"
+        )
     }
 
     static func artifactFailedDownload(artifactURL: URL, targetName: String, reason: String) -> Self {
-        .error("failed downloading '\(artifactURL.absoluteString)' which is required by binary target '\(targetName)': \(reason)")
+        .error(
+            "failed downloading '\(artifactURL.absoluteString)' which is required by binary target '\(targetName)': \(reason)"
+        )
     }
 
     static func artifactFailedValidation(artifactURL: URL, targetName: String, reason: String) -> Self {
-        .error("failed validating archive from '\(artifactURL.absoluteString)' which is required by binary target '\(targetName)': \(reason)")
+        .error(
+            "failed validating archive from '\(artifactURL.absoluteString)' which is required by binary target '\(targetName)': \(reason)"
+        )
     }
 
     static func remoteArtifactFailedExtraction(artifactURL: URL, targetName: String, reason: String) -> Self {
-        .error("failed extracting '\(artifactURL.absoluteString)' which is required by binary target '\(targetName)': \(reason)")
+        .error(
+            "failed extracting '\(artifactURL.absoluteString)' which is required by binary target '\(targetName)': \(reason)"
+        )
     }
 
     static func localArtifactFailedExtraction(artifactPath: AbsolutePath, targetName: String, reason: String) -> Self {
         .error("failed extracting '\(artifactPath)' which is required by binary target '\(targetName)': \(reason)")
     }
 
     static func remoteArtifactNotFound(artifactURL: URL, targetName: String) -> Self {
-        .error("downloaded archive of binary target '\(targetName)' from '\(artifactURL.absoluteString)' does not contain a binary artifact.")
+        .error(
+            "downloaded archive of binary target '\(targetName)' from '\(artifactURL.absoluteString)' does not contain a binary artifact."
+        )
     }
 
     static func localArchivedArtifactNotFound(archivePath: AbsolutePath, targetName: String) -> Self {
@@ -160,11 +177,25 @@ extension Basics.Diagnostic {
     static func localArtifactNotFound(artifactPath: AbsolutePath, targetName: String) -> Self {
         .error("local binary target '\(targetName)' at '\(artifactPath)' does not contain a binary artifact.")
     }
-}
 
+    static func exhaustedAttempts(missing: [PackageReference]) -> Self {
+        let missing = missing.sorted(by: { $0.identity < $1.identity }).map {
+            switch $0.kind {
+            case .registry(let identity):
+                return "'\(identity.description)'"
+            case .remoteSourceControl(let url):
+                return "'\($0.identity)' from \(url)"
+            case .localSourceControl(let path), .fileSystem(let path), .root(let path):
+                return "'\($0.identity)' at \(path)"
+            }
+        }
+        return .error(
+            "exhausted attempts to resolve the dependencies graph, with the following dependencies unresolved:\n* \(missing.joined(separator: "\n* "))"
+        )
+    }
+}
 
 extension FileSystemError: CustomStringConvertible {
-
     public var description: String {
         guard let path else {
             switch self.kind {
diff --git a/Sources/Workspace/Workspace.swift b/Sources/Workspace/Workspace.swift
@@ -1180,8 +1180,7 @@ extension Workspace {
         // If we have missing packages, something is fundamentally wrong with the resolution of the graph
         let stillMissingPackages = try updatedDependencyManifests.missingPackages
         guard stillMissingPackages.isEmpty else {
-            let missing = stillMissingPackages.map{ $0.description }
-            observabilityScope.emit(error: "exhausted attempts to resolve the dependencies graph, with '\(missing.sorted().joined(separator: "', '"))' unresolved.")
+            observabilityScope.emit(.exhaustedAttempts(missing: stillMissingPackages))
             return nil
         }
 
@@ -2970,8 +2969,7 @@ extension Workspace {
         // If we still have missing packages, something is fundamentally wrong with the resolution of the graph
         let stillMissingPackages = try updatedDependencyManifests.missingPackages
         guard stillMissingPackages.isEmpty else {
-            let missing = stillMissingPackages.map{ $0.description }
-            observabilityScope.emit(error: "exhausted attempts to resolve the dependencies graph, with '\(missing.sorted().joined(separator: "', '"))' unresolved.")
+            observabilityScope.emit(.exhaustedAttempts(missing: stillMissingPackages))
             return updatedDependencyManifests
         }
 
diff --git a/Tests/WorkspaceTests/WorkspaceTests.swift b/Tests/WorkspaceTests/WorkspaceTests.swift
@@ -11269,7 +11269,11 @@ final class WorkspaceTests: XCTestCase {
                     severity: .error
                 )
                 result.check(
-                    diagnostic: "exhausted attempts to resolve the dependencies graph, with 'bar remoteSourceControl http://scm.com/org/bar', 'foo remoteSourceControl http://scm.com/org/foo' unresolved.",
+                    diagnostic: """
+                    exhausted attempts to resolve the dependencies graph, with the following dependencies unresolved:
+                    * 'bar' from http://scm.com/org/bar
+                    * 'foo' from http://scm.com/org/foo
+                    """,
                     severity: .error
                 )
             }

Original file line number	Diff line number	Diff line change
`@@ -28,12 +28,11 @@ public struct ManifestParseDiagnostic: CustomStringConvertible {`
`28`	`28`	`}`
`29`	`29`
`30`	`30`	`public var description: String {`
`31`		`- "manifest parse error(s):\n" + errors.joined(separator: "\n")`
	`31`	`+ "manifest parse error(s):\n" + self.errors.joined(separator: "\n")`
`32`	`32`	`}`
`33`	`33`	`}`
`34`	`34`
`35`	`35`	`public enum WorkspaceDiagnostics {`
`36`		`-`
`37`	`36`	`// MARK: - Errors`
`38`	`37`
`39`	`38`	`/// The diagnostic triggered when an operation fails because its completion`
`@@ -43,7 +42,7 @@ public enum WorkspaceDiagnostics {`
`43`	`42`	`public let repositoryPath: AbsolutePath`
`44`	`43`
`45`	`44`	`public var description: String {`
`46`		`- return "repository '\(repositoryPath)' has uncommited changes"`
	`45`	`+ "repository '\(self.repositoryPath)' has uncommited changes"`
`47`	`46`	`}`
`48`	`47`	`}`
`49`	`48`
`@@ -54,7 +53,7 @@ public enum WorkspaceDiagnostics {`
`54`	`53`	`public let repositoryPath: AbsolutePath`
`55`	`54`
`56`	`55`	`public var description: String {`
`57`		`- return "repository '\(repositoryPath)' has unpushed changes"`
	`56`	`+ "repository '\(self.repositoryPath)' has unpushed changes"`
`58`	`57`	`}`
`59`	`58`	`}`
`60`	`59`
`@@ -65,7 +64,7 @@ public enum WorkspaceDiagnostics {`
`65`	`64`	`public let dependencyName: String`
`66`	`65`
`67`	`66`	`public var description: String {`
`68`		`- return "dependency '\(dependencyName)' not in edit mode"`
	`67`	`+ "dependency '\(self.dependencyName)' not in edit mode"`
`69`	`68`	`}`
`70`	`69`	`}`
`71`	`70`
`@@ -76,7 +75,7 @@ public enum WorkspaceDiagnostics {`
`76`	`75`	`public let branch: String`
`77`	`76`
`78`	`77`	`public var description: String {`
`79`		`- return "branch '\(branch)' already exists"`
	`78`	`+ "branch '\(self.branch)' already exists"`
`80`	`79`	`}`
`81`	`80`	`}`
`82`	`81`
`@@ -87,7 +86,7 @@ public enum WorkspaceDiagnostics {`
`87`	`86`	`public let revision: String`
`88`	`87`
`89`	`88`	`public var description: String {`
`90`		`- return "revision '\(revision)' does not exist"`
	`89`	`+ "revision '\(self.revision)' does not exist"`
`91`	`90`	`}`
`92`	`91`	`}`
`93`	`92`	`}`
`@@ -98,11 +97,15 @@ extension Basics.Diagnostic {`
`98`	`97`	`}`
`99`	`98`
`100`	`99`	`static func editBranchNotCheckedOut(packageName: String, branchName: String) -> Self {`
`101`		`- .warning("dependency '\(packageName)' already exists at the edit destination; not checking-out branch '\(branchName)'")`
	`100`	`+ .warning(`
	`101`	`+ "dependency '\(packageName)' already exists at the edit destination; not checking-out branch '\(branchName)'"`
	`102`	`+ )`
`102`	`103`	`}`
`103`	`104`
`104`	`105`	`static func editRevisionNotUsed(packageName: String, revisionIdentifier: String) -> Self {`
`105`		`- .warning("dependency '\(packageName)' already exists at the edit destination; not using revision '\(revisionIdentifier)'")`
	`106`	`+ .warning(`
	`107`	`+ "dependency '\(packageName)' already exists at the edit destination; not using revision '\(revisionIdentifier)'"`
	`108`	`+ )`
`106`	`109`	`}`
`107`	`110`
`108`	`111`	`static func editedDependencyMissing(packageName: String) -> Self {`
`@@ -122,35 +125,49 @@ extension Basics.Diagnostic {`
`122`	`125`	`}`
`123`	`126`
`124`	`127`	`static func artifactInvalidArchive(artifactURL: URL, targetName: String) -> Self {`
`125`		`- .error("invalid archive returned from '\(artifactURL.absoluteString)' which is required by binary target '\(targetName)'")`
	`128`	`+ .error(`
	`129`	`+ "invalid archive returned from '\(artifactURL.absoluteString)' which is required by binary target '\(targetName)'"`
	`130`	`+ )`
`126`	`131`	`}`
`127`	`132`
`128`	`133`	`static func artifactChecksumChanged(targetName: String) -> Self {`
`129`		`- .error("artifact of binary target '\(targetName)' has changed checksum; this is a potential security risk so the new artifact won't be downloaded")`
	`134`	`+ .error(`
	`135`	`+ "artifact of binary target '\(targetName)' has changed checksum; this is a potential security risk so the new artifact won't be downloaded"`
	`136`	`+ )`
`130`	`137`	`}`
`131`	`138`
`132`	`139`	`static func artifactInvalidChecksum(targetName: String, expectedChecksum: String, actualChecksum: String?) -> Self {`
`133`		`- .error("checksum of downloaded artifact of binary target '\(targetName)' (\(actualChecksum ?? "none")) does not match checksum specified by the manifest (\(expectedChecksum))")`
	`140`	`+ .error(`
	`141`	`+ "checksum of downloaded artifact of binary target '\(targetName)' (\(actualChecksum ?? "none")) does not match checksum specified by the manifest (\(expectedChecksum))"`
	`142`	`+ )`
`134`	`143`	`}`
`135`	`144`
`136`	`145`	`static func artifactFailedDownload(artifactURL: URL, targetName: String, reason: String) -> Self {`
`137`		`- .error("failed downloading '\(artifactURL.absoluteString)' which is required by binary target '\(targetName)': \(reason)")`
	`146`	`+ .error(`
	`147`	`+ "failed downloading '\(artifactURL.absoluteString)' which is required by binary target '\(targetName)': \(reason)"`
	`148`	`+ )`
`138`	`149`	`}`
`139`	`150`
`140`	`151`	`static func artifactFailedValidation(artifactURL: URL, targetName: String, reason: String) -> Self {`
`141`		`- .error("failed validating archive from '\(artifactURL.absoluteString)' which is required by binary target '\(targetName)': \(reason)")`
	`152`	`+ .error(`
	`153`	`+ "failed validating archive from '\(artifactURL.absoluteString)' which is required by binary target '\(targetName)': \(reason)"`
	`154`	`+ )`
`142`	`155`	`}`
`143`	`156`
`144`	`157`	`static func remoteArtifactFailedExtraction(artifactURL: URL, targetName: String, reason: String) -> Self {`
`145`		`- .error("failed extracting '\(artifactURL.absoluteString)' which is required by binary target '\(targetName)': \(reason)")`
	`158`	`+ .error(`
	`159`	`+ "failed extracting '\(artifactURL.absoluteString)' which is required by binary target '\(targetName)': \(reason)"`
	`160`	`+ )`
`146`	`161`	`}`
`147`	`162`
`148`	`163`	`static func localArtifactFailedExtraction(artifactPath: AbsolutePath, targetName: String, reason: String) -> Self {`
`149`	`164`	`.error("failed extracting '\(artifactPath)' which is required by binary target '\(targetName)': \(reason)")`
`150`	`165`	`}`
`151`	`166`
`152`	`167`	`static func remoteArtifactNotFound(artifactURL: URL, targetName: String) -> Self {`
`153`		`- .error("downloaded archive of binary target '\(targetName)' from '\(artifactURL.absoluteString)' does not contain a binary artifact.")`
	`168`	`+ .error(`
	`169`	`+ "downloaded archive of binary target '\(targetName)' from '\(artifactURL.absoluteString)' does not contain a binary artifact."`
	`170`	`+ )`
`154`	`171`	`}`
`155`	`172`
`156`	`173`	`static func localArchivedArtifactNotFound(archivePath: AbsolutePath, targetName: String) -> Self {`
`@@ -160,11 +177,25 @@ extension Basics.Diagnostic {`
`160`	`177`	`static func localArtifactNotFound(artifactPath: AbsolutePath, targetName: String) -> Self {`
`161`	`178`	`.error("local binary target '\(targetName)' at '\(artifactPath)' does not contain a binary artifact.")`
`162`	`179`	`}`
`163`		`-}`
`164`	`180`
	`181`	`+ static func exhaustedAttempts(missing: [PackageReference]) -> Self {`
	`182`	`+ let missing = missing.sorted(by: { $0.identity < $1.identity }).map {`
	`183`	`+ switch $0.kind {`
	`184`	`+ case .registry(let identity):`
	`185`	`+ return "'\(identity.description)'"`
	`186`	`+ case .remoteSourceControl(let url):`
	`187`	`+ return "'\($0.identity)' from \(url)"`
	`188`	`+ case .localSourceControl(let path), .fileSystem(let path), .root(let path):`
	`189`	`+ return "'\($0.identity)' at \(path)"`
	`190`	`+ }`
	`191`	`+ }`
	`192`	`+ return .error(`
	`193`	`+ "exhausted attempts to resolve the dependencies graph, with the following dependencies unresolved:\n* \(missing.joined(separator: "\n* "))"`
	`194`	`+ )`
	`195`	`+ }`
	`196`	`+}`
`165`	`197`
`166`	`198`	`extension FileSystemError: CustomStringConvertible {`
`167`		`-`
`168`	`199`	`public var description: String {`
`169`	`200`	`guard let path else {`
`170`	`201`	`switch self.kind {`
Original file line number	Diff line number	Diff line change
`@@ -11269,7 +11269,11 @@ final class WorkspaceTests: XCTestCase {`
`11269`	`11269`	`severity: .error`
`11270`	`11270`	`)`
`11271`	`11271`	`result.check(`
`11272`		`- diagnostic: "exhausted attempts to resolve the dependencies graph, with 'bar remoteSourceControl http://scm.com/org/bar', 'foo remoteSourceControl http://scm.com/org/foo' unresolved.",`
	`11272`	`+ diagnostic: """`
	`11273`	`+ exhausted attempts to resolve the dependencies graph, with the following dependencies unresolved:`
	`11274`	`+ * 'bar' from http://scm.com/org/bar`
	`11275`	`+ * 'foo' from http://scm.com/org/foo`
	`11276`	`+ """,`
`11273`	`11277`	`severity: .error`
`11274`	`11278`	`)`
`11275`	`11279`	`}`