This should be a last stop-gap measure in case sourcekitd or clangd get stuck, don’t respond to any requests anymore and don’t honor cancellation either. In that case we can restore SourceKit-LSP behavior by killing them and using the crash recovery logic to restore functionality.
rdar://149492159
Just something I noticed while looking through code. I am not aware of any issues caused by this.
Also relax the `precondition` in `DLHandle.deinit` to a fault. If we do not close a `DLHandle`, that’s a bug but it’s not so bad that we should crash sourcekit-lsp for it.
We don’t have any guarantees which thread these blocks will be called on by sourcekitd, so we shouldn’t make any assumptions about it in Swift. We should thus mark them as Sendable.
There is only one real class that implements the `SourceKitD` protocol, so there really isn’t any need for the protocol + class split at all. Unify them to make code simpler to reason about.
Otherwise, the cancellation could get handled before the request actually gets sent to sourcekitd, which is not what we want to test here. There appears to be a second underlying issue that causes unexpected results when we cancel the fast completion request after sending the slow completion request. I’ll look at that in a follow-up PR.
Otherwise, we would log sending the sourcekitd request even if the task was already cancelled and we thus took an early exit in `withCancellableCheckedThrowingContinuation`.
We need to jump through a few extra hoops when the SourceKit plugin is located in SourceKit-LSP’s build folder and we are using `sourcekitd_plugin_initialize` instead of `sourcekitd_plugin_initialize_2`.
This adds a sourcekitd plugin that drives the code completion requests. It also includes a `CompletionScoring` module that’s used to rank code completion results based on their contextual match, allowing us to show more relevant code completion results at the top.
We made quite a few fixes recently to make sure that path handling works correctly using `URL` on Windows. Use `URL` in most places to have a single type that represents file paths instead of sometimes using `AbsolutePath`.
While doing so, also remove usages of `TSCBasic.FileSystem` an `InMemoryFileSystem`. The pattern of using `InMemoryFileSystem` for tests was never consistently used and it was a little confusing that some types took a `FileSystem` parameter while other always assumed to work on the local file system.
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
Fixes all build warnings in SourceKit-LSP so that it builds without any issues using recent Swift development snapshots (`swift-DEVELOPMENT-SNAPSHOT-2024-07-22-a` and `swift-6.0-DEVELOPMENT-SNAPSHOT-2024-07-24-a`)
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
VS Code does not cancel semantic tokens requests. If a source file gets into a state where an AST build takes very long, this can cause us to wait for the semantic tokens from sourcekitd for a few minutes, effectively blocking all other semantic functionality in that file.
To circumvent this problem (or any other problem where an editor might not be cancelling requests they are no longer interested in) add a maximum request duration for SourceKitD requests, defaulting to 2 minutes.
rdar://130948453
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.
Turns out that sourcekitd test hooks were a bad idea because of the following comment that I wrote:
```
`testHooks` are only considered when an instance is being created. If a sourcekitd instance at the given path already exists, its test hooks will be used.
```
During test execution in Xcode, we generate a bunch of `SourceKitServer` instances in the same process that all call `DynamicallyLoadedSourceKitD.getOrCreate`. Now, if `testDontReturnEmptyDiagnosticsIfDiagnosticRequestIsCancelled` is not the first test being executed in the process (which usually it is not), the test hooks in it won’t get used.
Switch back to using the preparation hooks, essentially reverting https://github.com/apple/sourcekit-lsp/pull/1412 and keeping the following snippet to fix the underlying issue
```swift
// Poll until the `CancelRequestNotification` has been propagated to the request handling.
for _ in 0..<Int(defaultTimeout * 100) {
if Task.isCancelled {
break
}
usleep(10_000)
}
```
I saw a few non-deterministic test failures. I think the issue was that handling of the `CancelRequestNotification` is done asynchronously, which left a short window in which the sourcekitd diagnostics request could run and return results instead of being cancelled. Wait for the diagnostic request to actually be cancelled before running it for real.
While doing this, also introduce proper sourcekitd test hooks instead of relying on the preparation test hooks, which just got run as a side effect.
Otherwise, I think `ThreadSafeBox` might still have data races. This also requires us to make `TestSourceKitLSPClient.RequestHandler` sendable.
rdar://128572489
I saw a failure in CI where a sourcekitd diagnostic request got cancelled without an LSP cancellation request. I am suspecting that there is some implicit cancellation going on in sourcekitd despite `key.cancel_builds: 0`, which doesn’t make sense to me. Adding some explicit logging to sourcekit-lsp to check if we cancel the request for some reason.
This would have caught a race condition in background indexing that was caused by accessing `CheckedIndex` from multiple threads despite it not being thread-safe.
