When swift-format crashes before we send all data to its stdin, we get a SIGPIPE when trying to write more data to its studio on Linux. This, in turn, takes SourceKit-LSP down.
Do the same trick that we do when launching `JSONRPCConnection` of disabling SIGPIPE globally if we can’t disable it for individual pipes.
rdar://147665695
swift-foundation recently landed a change (in
swiftlang/swift-foundation#764) which requires `any Sendable` values in
`JSONEncoder.userInfo`. This causes a build failure:
```
JSONRPCConnection.swift:370:50: error: type '(RequestID) -> Optional<any ResponseType.Type>' does not conform to the 'Sendable' protocol
368 |
369 | // Setup callback for response type.
370 | decoder.userInfo[.responseTypeCallbackKey] = { (id: RequestID) -> ResponseType.Type? in
| |- error: type '(RequestID) -> Optional<any ResponseType.Type>' does not conform to the 'Sendable' protocol
| `- note: a function type must be marked '@Sendable' to conform to 'Sendable'
371 | guard let outstanding = self.outstandingRequests[id] else {
372 | logger.error("Unknown request for \(id, privacy: .public)")
```
Make the closure sendable, which is safe as we're only reading from
`outstandingRequests` (where all our writes are guarded by the same
queue that the decoding is on).
This allows us to record the communication between SourceKit-LSP and the editor on a very low level to inspect any transfer issues. It also allows us to record an entire SourceKit-LSP session and replay it using
```
cat /path/to/mirror.log - | path/to/sourcekit-lsp
```
`URL.path` returns forward slashes in the path on Windows (https://github.com/swiftlang/swift-foundation/issues/973) where we expect backslashes. Work around that by defining our own `filePath` property that is backed by `withUnsafeFileSystemRepresentation`, which produces backslashes.
rdar://137963660
Otherwise, we can have a retain cycle where the `OutstandingRequest` keeps the `JSONRPCConnection` alive.
Also move the code that handled `outstandingRequests` out of the `Task` because `outstandingRequests` should only be accessed from `queue`.
The interaction to an out-of-process BSP server still went through the `BuildServerBuildSystem`, which doesn’t forward all messages to the build system and uses the old push-based model for build settings.
If we discover that the BSP server supports the new pull-based build settings model, we now forward all methods to it directly, without going through `BuiltInBuildSystemAdapter`, which has been renamed to `LegacyBuildServerBuildSystem`.
rdar://136106323
rdar://127606323
rdar://126493405
Fixes#1226Fixes#1173
The initialize request and response are often critical to debug issues with editor setups. Log them completely over multiple log messages, instead of truncating them in the normal logging path.
Even if we don’t want to log any sensitive information, we can still log the keys of JSON objects and insensitive values such as integers and booleans.
Reverts swiftlang/sourcekit-lsp#1688
This seems to be breaking toolchain builds:
```
JSONRPCConnection.swift:168:35: error: reference to property 'outstandingRequests' in closure
requires explicit use of 'self' to make capture semantics explicit
for outstandingRequest in outstandingRequests.values {
```
https://ci.swift.org/job/swift-PR-toolchain-macos/1519/console
Do one of the following for every `FIXME` or `TODO` comment
- Add an issue that tracks the task
- Remove the comment if we are not planning to address it
Change a l public declarations to the `package` access level, accept for:
- The `LanguageServerProtocol` module
- The `BuildServerProtocol` module
- `InProcessClient.InProcessSourceKitLSPClient`
- `LanguageServerProtocolJSONRPC` (I would like to create a more ergonomic API for this like `InProcessSourceKitLSPClient` in the future, but for now, we’ll leave it public)
Unfortunately, our pattern of marking functions as `@_spi(Testing) public` no longer works with the `package` access level because declarations at the `package` access level cannot be marked as SPI. I have decided to just mark these functions as `package`. Alternatives would be:
- Add an underscore to these functions, like we did for functions exposed for testing before the introduction of `SPI`
- Use `@testable` import in the test targets and mark the methods as `internal`
Resolves#1315
rdar://128295618
The purpose of the different modules wasn’t clearly defined, which lead to inconsistent responsibilities between the different modules. Define each module’s purpose and move a few files between modules to satisfy these definitions.
There are a few more larger changes that will need to be made for a fully consistent module structure. These are FIXMEs in the new Modules.md document and I’ll address them in follow-up PRs.
Previously, we `fatalError`ing when `JSONDecoder` failed to decode a message from the client. Instead of crashing, try recovering from such invalid messages as best as possible. If we know that the state might have gotten out of sync with the client, show a notification message to the user, asking them to file an issue.
rdar://112991102
With the `test-sourcekit-lsp.py` integration test rewritten to not use `--sync`, there are no more users of this option. Remove it since we don’t really test it anyway.
This started off by guarding `state` against concurrent access and then escalated slightly.
The main changes are:
- `state` must only be accessed on `queue` now, preventing race conditions of message handling while the connection is being closed.
- A few functions were renamed, merged, and documented to simplify the code
- A few dead members were removed
The client ID was needed when a `MessageHandler` could handle messages from multiple connections. We don’t support this anymore (because it wasn’t needed) and so the client ID doesn’t need to get passed through as well.
OSLog is the suggesting logging solution on Apple platforms and we should be using it there, taking advantage of the different log levels and privacy masking.
Switch sourcekit-lsp to use OSLog on Apple platforms and implement a logger that is API-compatible with OSLog for all uses in sourcekit-lsp and which can be used on non-Darwin platforms.
The goal of this commit is to introduce the new logging API. There are still improvements about what we log and we can display more privacy-insensitive information after masking. Those changes will be in follow-up commits.
Add `.swift-format` to the repo and format the repo with `swift-format`.
This commit does not add any automation to enforce formatting of sourcekit-lsp in CI. The goal of this commit is to get the majority of source changes out of the way so that the diff of actually enforcing formatting will have fewer changes or conflicts.
This generally seems like the cleaner design because `SourceKitServer` is actually able to semantically inspect the message and decide whether it can be handled concurrently with other requests.
Unfortuantely, we have a few potential out-of-order exeuction possibilities while we migrate everything else to also be asyncronous. But those should be low-probability issues that we can fix in follow-up commits, so I think it’s fine for now. All of these places are marked with `FIXME: (async)`
This is the prerequisite for making `SourceKitServer` an actor, which will mean that the `handle` methods will be `async`.
The current paradigm of returning from `handle` once we can guarantee that there’s no out-of-order execution and then returning the actual result via the callback that’s attached to `Request` is a little weird still. I am hoping to change this paradigm to return the actual result and have a callback function that `handle` can call to indicate that it’s ready to accept another message while guaranteeing in-order execution, essentially flipping the role of the return value and the closure callback. But that’s something to be done after the entire stack has been asyncificied.
When we switch `SourceKitServer`, `SwiftLanguageServer` etc. to be actors, we can’t rely on them to provide ordering guarantees anymore because Swift concurrency doesn’t provide any ordering guarantees.
What we should thus do, is to handle all messages on a serial queue on the `Connection` level. This queue will be blocked from handling any new messages until a message has been sufficiently handled to avoid out-of-order handling of messages. For sourcekitd, this means that
a request has been sent to sourcekitd and for clangd, this means that we have forwarded the request to clangd.
Note that this serial queue is not the main thread, so we will continue accepting data over stdin, just the handling of those messages is blocked.
The asyncification changes caused some non-deterministic test failures. I believe that some of these are due to race conditions that are the result of the partial transition to actors.
Instead of merging the asyncification piece by piece, I will collect the changes asyncification changes in a branch and then qualify that branch througougly (running CI multiple times) before merging it into `main`.
