swiftlang · ahoppen · Jun 7, 2024 · Jun 7, 2024 · Jun 7, 2024 · Jun 7, 2024
@@ -0,0 +1,67 @@
+# Logging
+
+The following is a guide of how to write log messages and which level they should be logged at. There may be reasons to deviate from these guides but they should be a good starting point.
+
+## Log Levels
+
+The following log levels should be used by log messages in default. The [Explore logging in Swift](https://developer.apple.com/wwdc20/10168?time=604) session from WWDC20 has some explanation of how these levels are persisted by OS log and we follow those guidelines.
+
+### Fault
+
+> From Explore Logging in Swift: _Bug in program_
+
+A bug in SourceKit-LSP, sourcekitd or any other project included in the Swift toolchain that should never happen and is not due to malformed user input. The fix for faults should always be in a project controlled by the Swift toolchain (most likely in SourceKit-LSP itself) and we should never close them as a third party to resolve. Think of these as light assertions that don’t crash sourcekit-lsp because it is able to recover in some way.
+
+Examples:
+- Some global state invariant is broken like a file to `startProgress` not being followed by `endProgress`
+- sourcekitd crashes
+- Two targets in SwiftPM have the same name
+
+### Error
+
+> From Explore Logging in Swift: _Error seen during execution_
+
+An error that is due to user input or eg. stale state of files on disk. It indicates that something is going wrong which might explain unexpected behavior. Errors could be due to malformed user input such as invalid requests from the editor or due to stale state that will eventually converge again.
+
+Examples:
+- The client sends an invalid request
+- Preparation of a file failed due to a syntax error in the user’s code
+- The index contains a reference to a source file but the source fail has been modified since the index was last updated and is thus no longer valid
+
+## Log/Notice/Default
+
+`logger.default` logs at the `default` aka `notice` level.
+
+> From Explore Logging in Swift: _Essential for troubleshooting_
+
+Essential state transitions during SourceKit-LSP’s execution that allow use to determine what interactions the user performed. These logs will most likely be included in diagnose bundles from `sourcekit-lsp` diagnose and should help us solve most problems.
+
+Examples:
+- The user sends an LSP request
+- Indexing of a file starts or finishes
+- New build settings for a file have been computed
+- Responses from sourcekitd
+
+## Info
+
+> From Explore Logging in Swift: _Helpful but not essential for troubleshooting_ (not persisted, logged to memory)
+
+Internal state transitions that are helpful. If eg. a request fails and it’s not immediately obvious at which it failed, these should help us narrow down the code that it failed in. These messages might be missing from the logs generated by `sourcekit-lsp diagnose` and should not generally be needed to fix issues
+
+Examples:
+- Requests sent to `sourcekitd` or `clangd`
+- Logging the main file for a header file
+
+## Debug
+
+> From Explore Logging in Swift: _Useful only during debugging_ (only logged during debugging)
+
+Log messages that are useful eg. when debugging a test failure but that is not needed for diagnosing most real-world issues, like detailed information about when a function starts executing to diagnose race conditions.
+
+Examples:
+- Tasks start and finish executing in `TaskScheduler`
+
+## Log messages
+
+Log messages should resemble English sentences and start with an uppercase letter. If the log is a single sentence it should not have a period at its end.
+
@@ -19,8 +19,7 @@ import SourceKitLSP
 public final class InProcessSourceKitLSPClient: Sendable {
   private let server: SourceKitLSPServer
 
-  /// `nonisolated(unsafe)` if fine because `nextRequestID` is atomic.
-  private nonisolated(unsafe) var nextRequestID = AtomicUInt32(initialValue: 0)
+  private let nextRequestID = AtomicUInt32(initialValue: 0)
 
   /// Create a new `SourceKitLSPServer`. An `InitializeRequest` is automatically sent to the server.
   ///

@@ -10,15 +10,6 @@
 //
 //===----------------------------------------------------------------------===//
 
-/// Which log level to use (from https://developer.apple.com/wwdc20/10168?time=604)
-///  - Debug: Useful only during debugging (only logged during debugging)
-///  - Info: Helpful but not essential for troubleshooting (not persisted, logged to memory)
-///  - Notice/log/default: Essential for troubleshooting
-///  - Error: Error seen during execution
-///    - Used eg. if the user sends an erroneous request or if a request fails
-///  - Fault: Bug in program
-///    - Used eg. if invariants inside sourcekit-lsp are broken and the error is not due to user-provided input
-
 import Foundation
 
 #if canImport(os) && !SOURCEKITLSP_FORCE_NON_DARWIN_LOGGER

@@ -58,7 +58,7 @@ public let builtinRequests: [_RequestType.Type] = [
   InlineValueRequest.self,
   LinkedEditingRangeRequest.self,
   MonikersRequest.self,
-  OpenInterfaceRequest.self,
+  OpenGeneratedInterfaceRequest.self,
   PollIndexRequest.self,
   PrepareRenameRequest.self,
   ReferencesRequest.self,

@@ -10,11 +10,11 @@
 //
 //===----------------------------------------------------------------------===//
 
-/// Request a textual interface of a module to display in the IDE.
+/// Request a generated interface of a module to display in the IDE.
 /// **(LSP Extension)**
-public struct OpenInterfaceRequest: TextDocumentRequest, Hashable {
+public struct OpenGeneratedInterfaceRequest: TextDocumentRequest, Hashable {
   public static let method: String = "textDocument/openInterface"
-  public typealias Response = InterfaceDetails?
+  public typealias Response = GeneratedInterfaceDetails?
 
   /// The document whose compiler arguments should be used to generate the interface.
   public var textDocument: TextDocumentIdentifier
@@ -45,7 +45,7 @@ public struct OpenInterfaceRequest: TextDocumentRequest, Hashable {
 }
 
 /// The textual output of a module interface.
-public struct InterfaceDetails: ResponseType, Hashable {
+public struct GeneratedInterfaceDetails: ResponseType, Hashable {
 
   public var uri: DocumentURI
   public var position: Position?

@@ -134,7 +134,7 @@ public final class JSONRPCConnection: Connection {
       queue: queue,
       cleanupHandler: { (error: Int32) in
         if error != 0 {
-          logger.error("IO error \(error)")
+          logger.fault("IO error \(error)")
         }
         ioGroup.leave()
       }
@@ -153,7 +153,7 @@ public final class JSONRPCConnection: Connection {
       queue: sendQueue,
       cleanupHandler: { (error: Int32) in
         if error != 0 {
-          logger.error("IO error \(error)")
+          logger.fault("IO error \(error)")
         }
         ioGroup.leave()
       }
@@ -195,7 +195,7 @@ public final class JSONRPCConnection: Connection {
         guard errorCode == 0 else {
           #if !os(Windows)
           if errorCode != POSIXError.ECANCELED.rawValue {
-            logger.error("IO error reading \(errorCode)")
+            logger.fault("IO error reading \(errorCode)")
           }
           #endif
           if done { self.closeAssumingOnQueue() }
@@ -365,7 +365,7 @@ public final class JSONRPCConnection: Connection {
     precondition(state != .created, "tried to send message before calling start(messageHandler:)")
     let ready = state == .running
     if shouldLog && !ready {
-      logger.error("ignoring message; state = \(String(reflecting: self.state), privacy: .public)")
+      logger.error("Ignoring message; state = \(String(reflecting: self.state), privacy: .public)")
     }
     return ready
   }
@@ -444,7 +444,7 @@ public final class JSONRPCConnection: Connection {
 
     sendIO.write(offset: 0, data: dispatchData, queue: sendQueue) { [weak self] done, _, errorCode in
       if errorCode != 0 {
-        logger.error("IO error sending message \(errorCode)")
+        logger.fault("IO error sending message \(errorCode)")
         if done, let self {
           // An unrecoverable error occurs on the channel’s file descriptor.
           // Close the connection.
@@ -546,7 +546,7 @@ public final class JSONRPCConnection: Connection {
       guard state == .running else { return }
       state = .closed
 
-      logger.log("closing JSONRPCConnection...")
+      logger.log("Closing JSONRPCConnection...")
       // Attempt to close the reader immediately; we do not need to accept remaining inputs.
       receiveIO.close(flags: .stop)
       // Close the writer after it finishes outstanding work.

@@ -12,8 +12,7 @@
 
 import LanguageServerProtocol
 
-/// *Public For Testing*. A single JSONRPC message suitable for encoding/decoding.
-public enum JSONRPCMessage {
+@_spi(Testing) public enum JSONRPCMessage {
   case notification(NotificationType)
   case request(_RequestType, id: RequestID)
   case response(ResponseType, id: RequestID)

@@ -118,7 +118,7 @@ public actor BuildServerBuildSystem: MessageHandler {
       // config file was missing, no build server for this workspace
       return nil
     } catch {
-      logger.fault("failed to start build server: \(error.forLogging)")
+      logger.fault("Failed to start build server: \(error.forLogging)")
       return nil
     }
   }
@@ -127,7 +127,7 @@ public actor BuildServerBuildSystem: MessageHandler {
     if let buildServer = self.buildServer {
       _ = buildServer.send(ShutdownBuild()) { result in
         if let error = result.failure {
-          logger.fault("error shutting down build server: \(error.forLogging)")
+          logger.fault("Error shutting down build server: \(error.forLogging)")
         }
         buildServer.send(ExitBuildNotification())
         buildServer.close()
@@ -175,7 +175,7 @@ public actor BuildServerBuildSystem: MessageHandler {
     let buildServer = try makeJSONRPCBuildServer(client: self, serverPath: serverPath, serverFlags: flags)
     let response = try await buildServer.send(initializeRequest)
     buildServer.send(InitializedBuildNotification())
-    logger.log("initialized build server \(response.displayName)")
+    logger.log("Initialized build server \(response.displayName)")
 
     // see if index store was set as part of the server metadata
     if let indexDbPath = readReponseDataKey(data: response.data, key: "indexDatabasePath") {
@@ -307,7 +307,7 @@ extension BuildServerBuildSystem: BuildSystem {
     let request = RegisterForChanges(uri: uri, action: .register)
     _ = self.buildServer?.send(request) { result in
       if let error = result.failure {
-        logger.error("error registering \(uri): \(error.forLogging)")
+        logger.error("Error registering \(uri): \(error.forLogging)")
 
         Task {
           // BuildServer registration failed, so tell our delegate that no build
@@ -324,7 +324,7 @@ extension BuildServerBuildSystem: BuildSystem {
     let request = RegisterForChanges(uri: uri, action: .unregister)
     _ = self.buildServer?.send(request) { result in
       if let error = result.failure {
-        logger.error("error unregistering \(uri.forLogging): \(error.forLogging)")
+        logger.error("Error unregistering \(uri.forLogging): \(error.forLogging)")
       }
     }
   }
@@ -416,7 +416,7 @@ private func makeJSONRPCBuildServer(
   process.terminationHandler = { process in
     logger.log(
       level: process.terminationReason == .exit ? .default : .error,
-      "build server exited: \(String(reflecting: process.terminationReason)) \(process.terminationStatus)"
+      "Build server exited: \(String(reflecting: process.terminationReason)) \(process.terminationStatus)"
     )
     connection.close()
   }

@@ -265,7 +265,7 @@ extension BuildSystemManager {
 
   public func unregisterForChangeNotifications(for uri: DocumentURI) async {
     guard let mainFile = self.watchedFiles[uri]?.mainFile else {
-      logger.error("Unbalanced calls for registerForChangeNotifications and unregisterForChangeNotifications")
+      logger.fault("Unbalanced calls for registerForChangeNotifications and unregisterForChangeNotifications")
       return
     }
     self.watchedFiles[uri] = nil
@@ -417,8 +417,10 @@ extension BuildSystemManager {
 
 extension BuildSystemManager {
 
-  /// *For Testing* Returns the main file used for `uri`, if this is a registered file.
-  public func _cachedMainFile(for uri: DocumentURI) -> DocumentURI? {
+  /// Returns the main file used for `uri`, if this is a registered file.
+  ///
+  /// For testing purposes only.
+  @_spi(Testing) public func cachedMainFile(for uri: DocumentURI) -> DocumentURI? {
     return self.watchedFiles[uri]?.mainFile
   }
 }

@@ -173,7 +173,7 @@ extension CompilationDatabaseBuildSystem: BuildSystem {
     }
 
     if compdb == nil {
-      logger.error("could not open compilation database for \(path)")
+      logger.error("Could not open compilation database for \(path)")
     }
 
     return compdb

@@ -126,7 +126,7 @@ public actor QueuedTask<TaskDescription: TaskDescriptionProtocol> {
   /// Every time `execute` gets called, a new task is placed in this continuation. See comment on `executionTask`.
   private let executionTaskCreatedContinuation: AsyncStream<Task<ExecutionTaskFinishStatus, Never>>.Continuation
 
-  nonisolated(unsafe) private var _priority: AtomicUInt8
+  private let _priority: AtomicUInt8
 
   /// The latest known priority of the task.
   ///
@@ -147,9 +147,9 @@ public actor QueuedTask<TaskDescription: TaskDescriptionProtocol> {
   private var cancelledToBeRescheduled: Bool = false
 
   /// Whether `resultTask` has been cancelled.
-  private nonisolated(unsafe) var resultTaskCancelled: AtomicBool = .init(initialValue: false)
+  private let resultTaskCancelled: AtomicBool = .init(initialValue: false)
 
-  private nonisolated(unsafe) var _isExecuting: AtomicBool = .init(initialValue: false)
+  private let _isExecuting: AtomicBool = .init(initialValue: false)
 
   /// Whether the task is currently executing or still queued to be executed later.
   public nonisolated var isExecuting: Bool {

@@ -71,7 +71,7 @@ public final actor ToolchainRegistry {
 
   /// Create a toolchain registry with a pre-defined list of toolchains.
   ///
-  /// For testing purposes.
+  /// For testing purposes only.
   @_spi(Testing)
   public init(toolchains: [Toolchain]) {
     self.init(

@@ -105,7 +105,9 @@ extension XCToolchainPlist: Codable {
     self.displayName = try container.decodeIfPresent(String.self, forKey: .DisplayName)
   }
 
-  /// Encode the info plist. **For testing**.
+  /// Encode the info plist.
+  ///
+  /// For testing purposes only.
   public func encode(to encoder: Encoder) throws {
     var container = encoder.container(keyedBy: CodingKeys.self)
     if identifier.starts(with: "com.apple") {

@@ -16,8 +16,8 @@ import CAtomics
 #warning("We should be able to use atomics in the stdlib when we raise the deployment target to require Swift 6")
 #endif
 
-public class AtomicBool {
-  private let atomic: UnsafeMutablePointer<CAtomicUInt32>
+public final class AtomicBool: Sendable {
+  private nonisolated(unsafe) let atomic: UnsafeMutablePointer<CAtomicUInt32>
 
   public init(initialValue: Bool) {
     self.atomic = atomic_uint32_create(initialValue ? 1 : 0)
@@ -37,8 +37,8 @@ public class AtomicBool {
   }
 }
 
-public class AtomicUInt8 {
-  private let atomic: UnsafeMutablePointer<CAtomicUInt32>
+public final class AtomicUInt8: Sendable {
+  private nonisolated(unsafe) let atomic: UnsafeMutablePointer<CAtomicUInt32>
 
   public init(initialValue: UInt8) {
     self.atomic = atomic_uint32_create(UInt32(initialValue))
@@ -58,8 +58,8 @@ public class AtomicUInt8 {
   }
 }
 
-public class AtomicUInt32 {
-  private let atomic: UnsafeMutablePointer<CAtomicUInt32>
+public final class AtomicUInt32: Sendable {
+  private nonisolated(unsafe) let atomic: UnsafeMutablePointer<CAtomicUInt32>
 
   public init(initialValue: UInt32) {
     self.atomic = atomic_uint32_create(initialValue)

@@ -118,16 +118,16 @@ extension Process {
 private func setProcessPriority(pid: Process.ProcessID, newPriority: TaskPriority) {
   #if os(Windows)
   guard let handle = OpenProcess(UInt32(PROCESS_SET_INFORMATION), /*bInheritHandle*/ false, UInt32(pid)) else {
-    logger.error("Failed to get process handle for \(pid) to change its priority: \(GetLastError())")
+    logger.fault("Failed to get process handle for \(pid) to change its priority: \(GetLastError())")
     return
   }
   defer {
     CloseHandle(handle)
   }
   if !SetPriorityClass(handle, UInt32(newPriority.windowsProcessPriority)) {
-    logger.error("Failed to set process priority of \(pid) to \(newPriority.rawValue): \(GetLastError())")
+    logger.fault("Failed to set process priority of \(pid) to \(newPriority.rawValue): \(GetLastError())")
   }
-  #elseif canImport(Darwin)
+  #elseif canImport(Darwin) || canImport(Android)
   // `setpriority` is only able to decrease a process's priority and cannot elevate it. Since Swift task’s priorities
   // can only be elevated, this means that we can effectively only change a process's priority once, when it is created.
   // All subsequent calls to `setpriority` will fail. Because of this, don't log an error.

@@ -87,8 +87,7 @@ fileprivate extension ConfiguredTarget {
   static let forPackageManifest = ConfiguredTarget(targetID: "", runDestinationID: "")
 }
 
-/// `nonisolated(unsafe)` is fine because `preparationTaskID` is atomic.
-fileprivate nonisolated(unsafe) var preparationTaskID: AtomicUInt32 = AtomicUInt32(initialValue: 0)
+fileprivate let preparationTaskID: AtomicUInt32 = AtomicUInt32(initialValue: 0)
 
 /// Swift Package Manager build system and workspace support.
 ///
@@ -305,7 +304,7 @@ public actor SwiftPMBuildSystem {
     } catch Error.noManifest {
       return nil
     } catch {
-      logger.error("failed to create SwiftPMWorkspace at \(uri.forLogging): \(error.forLogging)")
+      logger.error("Failed to create SwiftPMWorkspace at \(uri.forLogging): \(error.forLogging)")
       return nil
     }
   }
@@ -451,7 +450,7 @@ extension SwiftPMBuildSystem: SKCore.BuildSystem {
     }
 
     guard let buildTarget = self.targets[configuredTarget]?.buildTarget else {
-      logger.error("Did not find target with name \(configuredTarget.targetID)")
+      logger.fault("Did not find target with name \(configuredTarget.targetID)")
       return nil
     }