Merge pull request #1162 from ahoppen/fewer-fatal-errors

Replace usages of `fatalError` that are recoverable by non-fatal error propagation constructs

Author: ahoppen

Sources/LanguageServerProtocol/Requests/IndexedRenameRequest.swift

@@ -72,7 +72,9 @@ extension IndexedRenameRequest: Codable {
     )
     self.oldName = try container.decode(String.self, forKey: IndexedRenameRequest.CodingKeys.oldName)
     self.newName = try container.decode(String.self, forKey: IndexedRenameRequest.CodingKeys.newName)
-    self.positions = try container.decode([String: [Position]].self, forKey: .positions).mapKeys(DocumentURI.init)
+    self.positions = try container.decode([String: [Position]].self, forKey: .positions).compactMapKeys {
+      try? DocumentURI(string: $0)
+    }
   }
 
   public func encode(to encoder: Encoder) throws {
@@ -81,13 +83,22 @@ extension IndexedRenameRequest: Codable {
     try container.encode(self.textDocument, forKey: IndexedRenameRequest.CodingKeys.textDocument)
     try container.encode(self.oldName, forKey: IndexedRenameRequest.CodingKeys.oldName)
     try container.encode(self.newName, forKey: IndexedRenameRequest.CodingKeys.newName)
-    try container.encode(self.positions.mapKeys(\.stringValue), forKey: IndexedRenameRequest.CodingKeys.positions)
+    try container.encode(
+      self.positions.compactMapKeys { $0.stringValue },
+      forKey: IndexedRenameRequest.CodingKeys.positions
+    )
 
   }
 }
 
 fileprivate extension Dictionary {
-  func mapKeys<NewKeyType: Hashable>(_ transform: (Key) -> NewKeyType) -> [NewKeyType: Value] {
-    return [NewKeyType: Value](uniqueKeysWithValues: self.map { (transform($0.key), $0.value) })
+  func compactMapKeys<NewKeyType: Hashable>(_ transform: (Key) -> NewKeyType?) -> [NewKeyType: Value] {
+    let newKeysAndValues = self.compactMap { (key, value) -> (NewKeyType, Value)? in
+      guard let newKey = transform(key) else {
+        return nil
+      }
+      return (newKey, value)
+    }
+    return [NewKeyType: Value](uniqueKeysWithValues: newKeysAndValues)
   }
 }

Sources/LanguageServerProtocol/SupportTypes/DocumentURI.swift

@@ -12,6 +12,14 @@
 
 import Foundation
 
+struct FailedToConstructDocumentURIFromStringError: Error, CustomStringConvertible {
+  let string: String
+
+  var description: String {
+    return "Failed to construct DocumentURI from '\(string)'"
+  }
+}
+
 public struct DocumentURI: Codable, Hashable, Sendable {
   /// The URL that store the URIs value
   private let storage: URL
@@ -55,9 +63,9 @@ public struct DocumentURI: Codable, Hashable, Sendable {
 
   /// Construct a DocumentURI from the given URI string, automatically parsing
   ///  it either as a URL or an opaque URI.
-  public init(string: String) {
+  public init(string: String) throws {
     guard let url = URL(string: string) else {
-      fatalError("Failed to construct DocumentURI from '\(string)'")
+      throw FailedToConstructDocumentURIFromStringError(string: string)
     }
     self.init(url)
   }
@@ -68,7 +76,7 @@ public struct DocumentURI: Codable, Hashable, Sendable {
   }
 
   public init(from decoder: Decoder) throws {
-    self.init(string: try decoder.singleValueContainer().decode(String.self))
+    try self.init(string: decoder.singleValueContainer().decode(String.self))
   }
 
   /// Equality check to handle escape sequences in file URLs.

Sources/LanguageServerProtocol/SupportTypes/TextDocumentIdentifier.swift

@@ -26,7 +26,10 @@ extension TextDocumentIdentifier: LSPAnyCodable {
     guard case .string(let uriString)? = dictionary[CodingKeys.uri.stringValue] else {
       return nil
     }
-    self.uri = DocumentURI(string: uriString)
+    guard let uri = try? DocumentURI(string: uriString) else {
+      return nil
+    }
+    self.uri = uri
   }
 
   public func encodeToLSPAny() -> LSPAny {

Sources/LanguageServerProtocol/SupportTypes/WorkspaceEdit.swift

@@ -49,7 +49,7 @@ extension WorkspaceEdit: Codable {
     if let changesDict = try container.decodeIfPresent([String: [TextEdit]].self, forKey: .changes) {
       var changes = [DocumentURI: [TextEdit]]()
       for change in changesDict {
-        let uri = DocumentURI(string: change.key)
+        let uri = try DocumentURI(string: change.key)
         changes[uri] = change.value
       }
       self.changes = changes
@@ -311,8 +311,10 @@ extension WorkspaceEdit: LSPAnyCodable {
     }
     var dictionary = [DocumentURI: [TextEdit]]()
     for (key, value) in lspDict {
-      let uri = DocumentURI(string: key)
-      guard let edits = [TextEdit](fromLSPArray: value) else {
+      guard
+        let uri = try? DocumentURI(string: key),
+        let edits = [TextEdit](fromLSPArray: value)
+      else {
         return nil
       }
       dictionary[uri] = edits

Sources/LanguageServerProtocolJSONRPC/JSONRPCConnection.swift

@@ -230,13 +230,13 @@ public final class JSONRPCConnection: Connection {
     }
   }
 
-  /// Send a notification to the client that informs the user about a message decoding error and tells them to file an
-  /// issue.
+  /// Send a notification to the client that informs the user about a message encoding or decoding error and asks them
+  /// to file an issue.
   ///
   /// `message` describes what has gone wrong to the user.
   ///
   /// - Important: Must be called on `queue`
-  private func sendMessageDecodingErrorNotificationToClient(message: String) {
+  private func sendMessageCodingErrorNotificationToClient(message: String) {
     dispatchPrecondition(condition: .onQueue(queue))
     let showMessage = ShowMessageNotification(
       type: .error,
@@ -302,7 +302,7 @@ public final class JSONRPCConnection: Connection {
         // That way the user at least knows that something is going wrong even if the client never gets a response
         // for the request.
         logger.fault("Ignoring request because we failed to decode the request and don't have a request ID")
-        sendMessageDecodingErrorNotificationToClient(message: "sourcekit-lsp failed to decode a request")
+        sendMessageCodingErrorNotificationToClient(message: "sourcekit-lsp failed to decode a request")
         return nil
       case .response:
         if let id = error.id {
@@ -338,19 +338,19 @@ public final class JSONRPCConnection: Connection {
         // `textDocument/didChange` will result in an out-of-sync state between the editor and sourcekit-lsp.
         // Warn the user about the error.
         logger.fault("Ignoring notification that may cause corrupted behavior")
-        sendMessageDecodingErrorNotificationToClient(message: "sourcekit-lsp failed to decode a notification")
+        sendMessageCodingErrorNotificationToClient(message: "sourcekit-lsp failed to decode a notification")
         return nil
       case .unknown:
         // We don't know what has gone wrong. This could be any level of badness. Inform the user about it.
         logger.fault("Ignoring unknown message")
-        sendMessageDecodingErrorNotificationToClient(message: "sourcekit-lsp failed to decode a message")
+        sendMessageCodingErrorNotificationToClient(message: "sourcekit-lsp failed to decode a message")
         return nil
       }
     } catch {
       // We don't know what has gone wrong. This could be any level of badness. Inform the user about it and ignore the
       // message.
       logger.fault("Ignoring unknown message")
-      sendMessageDecodingErrorNotificationToClient(message: "sourcekit-lsp failed to decode an unknown message")
+      sendMessageCodingErrorNotificationToClient(message: "sourcekit-lsp failed to decode an unknown message")
       return nil
     }
   }
@@ -391,7 +391,7 @@ public final class JSONRPCConnection: Connection {
         // We failed to parse the message header. There isn't really much we can do to recover because we lost our
         // anchor in the stream where new messages start. Crashing and letting ourselves be restarted by the client is
         // probably the best option.
-        sendMessageDecodingErrorNotificationToClient(message: "Failed to find next message in connection to editor")
+        sendMessageCodingErrorNotificationToClient(message: "Failed to find next message in connection to editor")
         fatalError("fatal error encountered while splitting JSON RPC messages \(error)")
       }
 
@@ -481,8 +481,40 @@ public final class JSONRPCConnection: Connection {
     do {
       data = try encoder.encode(message)
     } catch {
-      // FIXME: attempt recovery?
-      fatalError("unexpected error while encoding response: \(error)")
+      logger.fault("Failed to encode message: \(error.forLogging)")
+      logger.fault("Malformed message: \(String(describing: message))")
+      switch message {
+      case .notification(_):
+        // We want to send a notification to the editor but failed to encode it. Since dropping the notification might
+        // result in getting out-of-sync state-wise with the editor (eg. for work progress notifications), inform the
+        // user about it.
+        sendMessageCodingErrorNotificationToClient(
+          message: "sourcekit-lsp failed to encode a notification to the editor"
+        )
+        return
+      case .request(_, _):
+        // We want to send a notification to the editor but failed to encode it. We don't know the `reply` handle for
+        // the request at this point so we can't synthesize an errorResponse for the request. This means that the
+        // request will never receive a reply. Inform the user about it.
+        sendMessageCodingErrorNotificationToClient(
+          message: "sourcekit-lsp failed to encode a request to the editor"
+        )
+        return
+      case .response(_, _):
+        // The editor sent a request to sourcekit-lsp, which failed but we can't serialize the result back to the
+        // client. This means that the request will never receive a reply. Inform the user about it and accept that
+        // we'll never send a reply.
+        sendMessageCodingErrorNotificationToClient(
+          message: "sourcekit-lsp failed to encode a response to the editor"
+        )
+        return
+      case .errorResponse(_, _):
+        // Same as `.response`. Has an optional `id`, so can't share the case.
+        sendMessageCodingErrorNotificationToClient(
+          message: "sourcekit-lsp failed to encode an error response to the editor"
+        )
+        return
+      }
     }
 
     var dispatchData = DispatchData.empty

Sources/SourceKitLSP/DocumentManager.swift

@@ -92,6 +92,7 @@ public final class DocumentManager {
   public enum Error: Swift.Error {
     case alreadyOpen(DocumentURI)
     case missingDocument(DocumentURI)
+    case failedToConvertPosition
   }
 
   let queue: DispatchQueue = DispatchQueue(label: "document-manager-queue")
@@ -159,7 +160,10 @@ public final class DocumentManager {
 
       var sourceEdits: [SourceEdit] = []
       for edit in edits {
-        sourceEdits.append(SourceEdit(edit: edit, lineTableBeforeEdit: document.latestLineTable))
+        guard let sourceEdit = SourceEdit(edit: edit, lineTableBeforeEdit: document.latestLineTable) else {
+          throw Error.failedToConvertPosition
+        }
+        sourceEdits.append(sourceEdit)
 
         if let range = edit.range {
           document.latestLineTable.replace(
@@ -230,7 +234,11 @@ extension DocumentManager {
 }
 
 fileprivate extension SourceEdit {
-  init(edit: TextDocumentContentChangeEvent, lineTableBeforeEdit: LineTable) {
+  /// Constructs a `SourceEdit` from the given `TextDocumentContentChangeEvent`.
+  ///
+  /// Returns `nil` if the `TextDocumentContentChangeEvent` refers to line:column positions that don't exist in
+  /// `LineTable`.
+  init?(edit: TextDocumentContentChangeEvent, lineTableBeforeEdit: LineTable) {
     if let range = edit.range {
       guard
         let offset = lineTableBeforeEdit.utf8OffsetOf(
@@ -242,7 +250,7 @@ fileprivate extension SourceEdit {
           utf16Column: range.upperBound.utf16index
         )
       else {
-        fatalError("invalid edit \(range)")
+        return nil
       }
       self.init(
         range: AbsolutePosition(utf8Offset: offset)..<AbsolutePosition(utf8Offset: end),

Sources/SourceKitLSP/SourceKitLSPServer.swift

@@ -2104,14 +2104,12 @@ extension SourceKitLSPServer {
   private nonisolated func extractCallHierarchyItemData(_ rawData: LSPAny?) -> (uri: DocumentURI, usr: String)? {
     guard case let .dictionary(data) = rawData,
       case let .string(uriString) = data["uri"],
-      case let .string(usr) = data["usr"]
+      case let .string(usr) = data["usr"],
+      let uri = orLog("DocumentURI for call hierarchy item", { try DocumentURI(string: uriString) })
     else {
       return nil
     }
-    return (
-      uri: DocumentURI(string: uriString),
-      usr: usr
-    )
+    return (uri: uri, usr: usr)
   }
 
   func incomingCalls(_ req: CallHierarchyIncomingCallsRequest) async throws -> [CallHierarchyIncomingCall]? {
@@ -2289,14 +2287,12 @@ extension SourceKitLSPServer {
   private nonisolated func extractTypeHierarchyItemData(_ rawData: LSPAny?) -> (uri: DocumentURI, usr: String)? {
     guard case let .dictionary(data) = rawData,
       case let .string(uriString) = data["uri"],
-      case let .string(usr) = data["usr"]
+      case let .string(usr) = data["usr"],
+      let uri = orLog("DocumentURI for type hierarchy item", { try DocumentURI(string: uriString) })
     else {
       return nil
     }
-    return (
-      uri: DocumentURI(string: uriString),
-      usr: usr
-    )
+    return (uri: uri, usr: usr)
   }
 
   func supertypes(_ req: TypeHierarchySupertypesRequest) async throws -> [TypeHierarchyItem]? {

Sources/SourceKitLSP/Swift/Diagnostic.swift

@@ -110,7 +110,8 @@ extension CodeAction {
     case (true, false):
       return "Insert '\(newText)'"
     case (true, true):
-      preconditionFailure("FixIt makes no changes")
+      logger.fault("Both oldText and newText of FixIt are empty")
+      return "Fix"
     }
   }
 }

Sources/SourceKitLSP/Swift/SwiftLanguageService.swift

@@ -518,7 +518,12 @@ extension SwiftLanguageService {
       do {
         _ = try await self.sourcekitd.send(req, fileContents: nil)
       } catch {
-        fatalError("failed to apply edit")
+        logger.fault(
+          """
+          failed to replace \(edit.range.lowerBound.utf8Offset):\(edit.range.upperBound.utf8Offset) by \
+          '\(edit.replacement)' in sourcekitd
+          """
+        )
       }
     }
 

Sources/SourceKitLSP/Swift/SyntaxHighlightingTokenParser.swift

@@ -206,7 +206,7 @@ struct SyntaxHighlightingTokenParser {
   }
 }
 
-extension Range where Bound == Position {
+extension Range<Position> {
   /// Splits a potentially multi-line range to multiple single-line ranges.
   func splitToSingleLineRanges(in snapshot: DocumentSnapshot) -> [Self] {
     if isEmpty {
@@ -220,7 +220,8 @@ extension Range where Bound == Position {
     guard let startIndex = snapshot.index(of: lowerBound),
       let endIndex = snapshot.index(of: upperBound)
     else {
-      fatalError("Range \(self) reaches outside of the document")
+      logger.fault("Range \(self) reaches outside of the document")
+      return [self]
     }
 
     let text = snapshot.text[startIndex..<endIndex]