improve string interpolation of errors (#6482)

motivation: better error messages

changes:
* audit usage of "\(error)" and replace them with "\(error.interpolationDescription)"
* add ADPI to pass an optional underlying error to observability's debug, info and warn calls such that it automatically interpolates the error correctly
* update call sites

rdar://108418554

Author: tomerd

Sources/Basics/AuthorizationProvider.swift

@@ -24,7 +24,13 @@ public protocol AuthorizationProvider {
 }
 
 public protocol AuthorizationWriter {
-    func addOrUpdate(for url: URL, user: String, password: String, persist: Bool, callback: @escaping (Result<Void, Error>) -> Void)
+    func addOrUpdate(
+        for url: URL,
+        user: String,
+        password: String,
+        persist: Bool,
+        callback: @escaping (Result<Void, Error>) -> Void
+    )
 
     func remove(for url: URL, callback: @escaping (Result<Void, Error>) -> Void)
 }
@@ -36,9 +42,9 @@ public enum AuthorizationProviderError: Error {
     case other(String)
 }
 
-public extension AuthorizationProvider {
+extension AuthorizationProvider {
     @Sendable
-    func httpAuthorizationHeader(for url: URL) -> String? {
+    public func httpAuthorizationHeader(for url: URL) -> String? {
         guard let (user, password) = self.authentication(for: url) else {
             return nil
         }
@@ -50,8 +56,8 @@ public extension AuthorizationProvider {
     }
 }
 
-private extension URL {
-    var authenticationID: String? {
+extension URL {
+    fileprivate var authenticationID: String? {
         guard let host = host?.lowercased() else {
             return nil
         }
@@ -75,7 +81,13 @@ public class NetrcAuthorizationProvider: AuthorizationProvider, AuthorizationWri
         _ = try Self.load(fileSystem: fileSystem, path: path)
     }
 
-    public func addOrUpdate(for url: URL, user: String, password: String, persist: Bool = true, callback: @escaping (Result<Void, Error>) -> Void) {
+    public func addOrUpdate(
+        for url: URL,
+        user: String,
+        password: String,
+        persist: Bool = true,
+        callback: @escaping (Result<Void, Error>) -> Void
+    ) {
         guard let machine = url.authenticationID else {
             return callback(.failure(AuthorizationProviderError.invalidURLHost))
         }
@@ -87,7 +99,9 @@ public class NetrcAuthorizationProvider: AuthorizationProvider, AuthorizationWri
 
         // Same entry already exists, no need to add or update
         let netrc = try? Self.load(fileSystem: self.fileSystem, path: self.path)
-        guard netrc?.machines.first(where: { $0.name.lowercased() == machine && $0.login == user && $0.password == password }) == nil else {
+        guard netrc?.machines
+            .first(where: { $0.name.lowercased() == machine && $0.login == user && $0.password == password }) == nil
+        else {
             return callback(.success(()))
         }
 
@@ -108,12 +122,18 @@ public class NetrcAuthorizationProvider: AuthorizationProvider, AuthorizationWri
 
             callback(.success(()))
         } catch {
-            callback(.failure(AuthorizationProviderError.other("Failed to update netrc file at \(self.path): \(error)")))
+            callback(.failure(
+                AuthorizationProviderError
+                    .other("Failed to update netrc file at \(self.path): \(error.interpolationDescription)")
+            ))
         }
     }
 
     public func remove(for url: URL, callback: @escaping (Result<Void, Error>) -> Void) {
-        callback(.failure(AuthorizationProviderError.other("User must edit netrc file at \(self.path) manually to remove entries")))
+        callback(.failure(
+            AuthorizationProviderError
+                .other("User must edit netrc file at \(self.path) manually to remove entries")
+        ))
     }
 
     public func authentication(for url: URL) -> (user: String, password: String)? {
@@ -124,7 +144,9 @@ public class NetrcAuthorizationProvider: AuthorizationProvider, AuthorizationWri
     }
 
     private func machine(for url: URL) -> Basics.Netrc.Machine? {
-        if let machine = url.authenticationID, let existing = self.machines.first(where: { $0.name.lowercased() == machine }) {
+        if let machine = url.authenticationID,
+           let existing = self.machines.first(where: { $0.name.lowercased() == machine })
+        {
             return existing
         }
         if let existing = self.machines.first(where: { $0.isDefault }) {
@@ -164,7 +186,13 @@ public class KeychainAuthorizationProvider: AuthorizationProvider, Authorization
         self.observabilityScope = observabilityScope
     }
 
-    public func addOrUpdate(for url: URL, user: String, password: String, persist: Bool = true, callback: @escaping (Result<Void, Error>) -> Void) {
+    public func addOrUpdate(
+        for url: URL,
+        user: String,
+        password: String,
+        persist: Bool = true,
+        callback: @escaping (Result<Void, Error>) -> Void
+    ) {
         guard let server = url.authenticationID else {
             return callback(.failure(AuthorizationProviderError.invalidURLHost))
         }
@@ -214,12 +242,14 @@ public class KeychainAuthorizationProvider: AuthorizationProvider, Authorization
         }
 
         do {
-            guard let existingItem = try self.search(server: server, protocol: self.protocol(for: url)) as? [String: Any],
-                  let passwordData = existingItem[kSecValueData as String] as? Data,
-                  let password = String(data: passwordData, encoding: String.Encoding.utf8),
-                  let account = existingItem[kSecAttrAccount as String] as? String
+            guard let existingItem = try self
+                .search(server: server, protocol: self.protocol(for: url)) as? [String: Any],
+                let passwordData = existingItem[kSecValueData as String] as? Data,
+                let password = String(data: passwordData, encoding: String.Encoding.utf8),
+                let account = existingItem[kSecAttrAccount as String] as? String
             else {
-                throw AuthorizationProviderError.other("Failed to extract credentials for server \(server) from keychain")
+                throw AuthorizationProviderError
+                    .other("Failed to extract credentials for server \(server) from keychain")
             }
             return (user: account, password: password)
         } catch {
@@ -229,7 +259,10 @@ public class KeychainAuthorizationProvider: AuthorizationProvider, Authorization
             case AuthorizationProviderError.other(let detail):
                 self.observabilityScope.emit(error: detail)
             default:
-                self.observabilityScope.emit(error: "Failed to find credentials for server \(server) in keychain: \(error)")
+                self.observabilityScope.emit(
+                    error: "Failed to find credentials for server \(server) in keychain",
+                    underlyingError: error
+                )
             }
             return nil
         }
@@ -244,7 +277,8 @@ public class KeychainAuthorizationProvider: AuthorizationProvider, Authorization
 
         let status = SecItemAdd(query as CFDictionary, nil)
         guard status == errSecSuccess else {
-            throw AuthorizationProviderError.other("Failed to save credentials for server \(server) to keychain: status \(status)")
+            throw AuthorizationProviderError
+                .other("Failed to save credentials for server \(server) to keychain: status \(status)")
         }
     }
 
@@ -260,7 +294,8 @@ public class KeychainAuthorizationProvider: AuthorizationProvider, Authorization
             return false
         }
         guard status == errSecSuccess else {
-            throw AuthorizationProviderError.other("Failed to update credentials for server \(server) in keychain: status \(status)")
+            throw AuthorizationProviderError
+                .other("Failed to update credentials for server \(server) in keychain: status \(status)")
         }
         return true
     }
@@ -271,7 +306,8 @@ public class KeychainAuthorizationProvider: AuthorizationProvider, Authorization
                                     kSecAttrProtocol as String: `protocol`]
         let status = SecItemDelete(query as CFDictionary)
         guard status == errSecSuccess else {
-            throw AuthorizationProviderError.other("Failed to delete credentials for server \(server) from keychain: status \(status)")
+            throw AuthorizationProviderError
+                .other("Failed to delete credentials for server \(server) from keychain: status \(status)")
         }
     }
 
@@ -290,7 +326,8 @@ public class KeychainAuthorizationProvider: AuthorizationProvider, Authorization
             throw AuthorizationProviderError.notFound
         }
         guard status == errSecSuccess else {
-            throw AuthorizationProviderError.other("Failed to find credentials for server \(server) in keychain: status \(status)")
+            throw AuthorizationProviderError
+                .other("Failed to find credentials for server \(server) in keychain: status \(status)")
         }
 
         return item

Sources/Basics/Cancellator.swift

@@ -25,7 +25,11 @@ public final class Cancellator: Cancellable {
 
     private let observabilityScope: ObservabilityScope?
     private let registry = ThreadSafeKeyValueStore<String, (name: String, handler: CancellationHandler)>()
-    private let cancelationQueue = DispatchQueue(label: "org.swift.swiftpm.cancellator", qos: .userInteractive, attributes: .concurrent)
+    private let cancelationQueue = DispatchQueue(
+        label: "org.swift.swiftpm.cancellator",
+        qos: .userInteractive,
+        attributes: .concurrent
+    )
     private let cancelling = ThreadSafeBox<Bool>(false)
 
     private static let signalHandlerLock = NSLock()
@@ -44,60 +48,60 @@ public final class Cancellator: Cancellable {
     public func installSignalHandlers() {
         Self.signalHandlerLock.withLock {
             precondition(!Self.isSignalHandlerInstalled)
-            
-#if os(Windows)
+
+            #if os(Windows)
             // Closures passed to `SetConsoleCtrlHandler` can't capture context, working around that with a global.
             Self.shared = self
-            
+
             // set shutdown handler to terminate sub-processes, etc
             _ = SetConsoleCtrlHandler({ _ in
                 // Terminate all processes on receiving an interrupt signal.
                 try? Cancellator.shared?.cancel(deadline: .now() + .seconds(30))
-                
+
                 // Reset the handler.
                 _ = SetConsoleCtrlHandler(nil, false)
-                
+
                 // Exit as if by signal()
                 TerminateProcess(GetCurrentProcess(), 3)
-                
+
                 return true
             }, true)
-#else
+            #else
             // trap SIGINT to terminate sub-processes, etc
             signal(SIGINT, SIG_IGN)
             let interruptSignalSource = DispatchSource.makeSignalSource(signal: SIGINT)
             interruptSignalSource.setEventHandler { [weak self] in
                 // cancel the trap?
                 interruptSignalSource.cancel()
-                
+
                 // Terminate all processes on receiving an interrupt signal.
                 try? self?.cancel(deadline: .now() + .seconds(30))
-                
-#if canImport(Darwin) || os(OpenBSD)
+
+                #if canImport(Darwin) || os(OpenBSD)
                 // Install the default signal handler.
                 var action = sigaction()
                 action.__sigaction_u.__sa_handler = SIG_DFL
                 sigaction(SIGINT, &action, nil)
                 kill(getpid(), SIGINT)
-#elseif os(Android)
+                #elseif os(Android)
                 // Install the default signal handler.
                 var action = sigaction()
                 action.sa_handler = SIG_DFL
                 sigaction(SIGINT, &action, nil)
                 kill(getpid(), SIGINT)
-#else
+                #else
                 var action = sigaction()
                 action.__sigaction_handler = unsafeBitCast(
                     SIG_DFL,
                     to: sigaction.__Unnamed_union___sigaction_handler.self
                 )
                 sigaction(SIGINT, &action, nil)
                 kill(getpid(), SIGINT)
-#endif
+                #endif
             }
             interruptSignalSource.resume()
-#endif
-            
+            #endif
+
             Self.isSignalHandlerInstalled = true
         }
     }
@@ -138,20 +142,21 @@ public final class Cancellator: Cancellable {
         self.registry[key] = nil
     }
 
-    public func cancel(deadline: DispatchTime) throws -> Void {
+    public func cancel(deadline: DispatchTime) throws {
         self._cancel(deadline: deadline)
     }
 
     // marked internal for testing
     @discardableResult
-    internal func _cancel(deadline: DispatchTime? = .none)-> Int {
+    internal func _cancel(deadline: DispatchTime? = .none) -> Int {
         self.cancelling.put(true)
 
-        self.observabilityScope?.emit(info: "starting cancellation cycle with \(self.registry.count) cancellation handlers registered")
+        self.observabilityScope?
+            .emit(info: "starting cancellation cycle with \(self.registry.count) cancellation handlers registered")
 
         let deadline = deadline ?? .now() + .seconds(30)
         // deadline for individual handlers set slightly before overall deadline
-        let delta: DispatchTimeInterval = .nanoseconds(abs(deadline.distance(to: .now()).nanoseconds() ?? 0)  / 5)
+        let delta: DispatchTimeInterval = .nanoseconds(abs(deadline.distance(to: .now()).nanoseconds() ?? 0) / 5)
         let handlersDeadline = deadline - delta
 
         let cancellationHandlers = self.registry.get()
@@ -164,13 +169,19 @@ public final class Cancellator: Cancellable {
                     try handler(handlersDeadline)
                     cancelled.append(name)
                 } catch {
-                    self.observabilityScope?.emit(warning: "failed cancelling '\(name)': \(error)")
+                    self.observabilityScope?.emit(
+                        warning: "failed cancelling '\(name)'",
+                        underlyingError: error
+                    )
                 }
             }
         }
 
         if case .timedOut = group.wait(timeout: deadline) {
-            self.observabilityScope?.emit(warning: "timeout waiting for cancellation with \(cancellationHandlers.count - cancelled.count) cancellation handlers remaining")
+            self.observabilityScope?
+                .emit(
+                    warning: "timeout waiting for cancellation with \(cancellationHandlers.count - cancelled.count) cancellation handlers remaining"
+                )
         } else {
             self.observabilityScope?.emit(info: "cancellation cycle completed successfully")
         }
@@ -197,7 +208,8 @@ public struct CancellationError: Error, CustomStringConvertible {
     }
 
     static func failedToRegisterProcess(_ process: TSCBasic.Process) -> Self {
-        Self(description: """
+        Self(
+            description: """
             failed to register a cancellation handler for this process invocation `\(
                 process.arguments.joined(separator: " ")
             )`

Sources/Basics/Errors.swift

@@ -30,8 +30,11 @@ public struct InternalError: Error {
 extension Error {
     public var interpolationDescription: String {
         switch self {
+        // special case because `LocalizedError` conversion will hide the underlying error
+        case let _error as DecodingError:
+            return "\(_error)"
         case let _error as LocalizedError:
-            var description = _error.localizedDescription
+            var description = _error.errorDescription ?? _error.localizedDescription
             if let recoverySuggestion = _error.recoverySuggestion {
                 description += ". \(recoverySuggestion)"
             }
@@ -45,7 +48,6 @@ extension Error {
                 description += ". \(localizedRecoverySuggestion)"
             }
             return description
-
         default:
             return "\(self)"
         }